=20 =20 ----- Original Message -----
From: "Sandro Bonazzola" <sbonazzo@redhat.com> To: "Eyal Edri" <eedri@redhat.com> Cc: infra@ovirt.org, devel@ovirt.org Sent: Thursday, June 4, 2015 9:50:56 AM Subject: Re: [ovirt-devel] gerrit+ci improvement proposal =20 Il 04/06/2015 08:46, Eyal Edri ha scritto:
=20 =20 ----- Original Message -----
From: "Sandro Bonazzola" <sbonazzo@redhat.com> To: "Eyal Edri" <eedri@redhat.com>, "Max Kovgan" <mkovgan@redhat.com> Cc: devel@ovirt.org, infra@ovirt.org Sent: Thursday, June 4, 2015 9:11:10 AM Subject: Re: [ovirt-devel] gerrit+ci improvement proposal
Il 03/06/2015 21:46, Eyal Edri ha scritto:
----- Original Message -----
From: "Max Kovgan" <mkovgan@redhat.com> To: devel@ovirt.org Cc: infra@ovirt.org Sent: Wednesday, June 3, 2015 8:22:54 PM Subject: [ovirt-devel] gerrit+ci improvement proposal
Hi everyone! We really want to have reliable and snappy CI: to allow short cycl=
es and
encourage developers to write tests.
# Problem
Many patches are neither ready for review nor for CI upon submissi= on, which is OK. But running all the jobs on those patches with limited resources r= esults in: overloaded resources, slow response time, unhappy developers.
# Proposed Solution
To run less jobs we know we don=E2=80=99t need to, thus making mor= e resources for the jobs we need to run. We have been experimenting to make our CI stabler and quicker to r= espond by using gerrit flags. This has improved in both directions very well internally. Now it seems a good time to let all the oVirt projects to use this. This solution indirectly promotes reviews and quick tests - =E2=80= =9Cto fail early=E2=80=9D, yet full blown static code analysis and long tests to run =E2=80= =9Cwhen ready=E2=80=9D.
# How it works
2 new gerrit independent flags are added to gerrit.
## CI flag
Will express patch CI status. Values: * +1 CI passed * 0 CI did not run yet * -1 CI failed Permissions for setting: project maintainers (for special cases) s= hould be able to set/override (except Jenkins).
## Workflow flag
Will express patch =E2=80=9Cworkflow=E2=80=9D state. Values: * 0 Work In Progress * +1 Ready For Review * +2 Ready For Merge Permissions for setting: Owner can set +1, Project Maintainers can= set +2
## Review + CI Integration:
Merging [=E2=80=9CSubmit=E2=80=9D button to appear] will require: = Review+1, CI+1, Workflow+2 Patch lifecycle now is: --------------------------------------------------------------- patch state |owner |reviewer |maintainer |CI tests |pass --------------------------------------------------------------- added/updated |- |- |- |quick |CI+1 review |Workflow+1|Review+1 |- |heavy |CI+1 merge ready |- |- |Workflow+2 |gating |CI+1 merge |- |- |merge |merge |CI+1
Changes from current workflow: Owner only adds reviewers, now owner needs to set "Workflow+1" for=
--K8nIJk4ghYZn606h Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On 06/04, Eyal Edri wrote: the
patch to be reviewed, and heavily auto-tested. Maintainer now needs to set "Workflow+2" and wait for "Submit" but= ton to appear after CI has completed running gating tests.
Next step will be to automate merge the change after Workflow+2 ha= s been set by the Maintainer and gating tests passed.
## Why now?
It is elimination of waste. The sooner - the better. The solution has been used for a while and it works. Resolving the problem without gerrit involved will lead to adding unreliable code into jobs, and will still be prone to problems: Just recently, 3d ago we=E2=80=99ve tried detecting what to run = =66rom jenkins relying only on gerrit comments so that upon Verified+1, we=E2= =80=99d run the job. We could not use =E2=80=9CReview+1=E2=80=9D, because it makes no= sense at all, so we left the job to set Verified+1. Meaning - re-trigger itself immediately more than 1 times. =20 Jenkins and its visitors very unhappy, and we had to stop those = jobs, clean up the queue, and spam developers.
## OK OK OK. Now what?
Now we want your comments and opinions before pushing this further: Please participate in this thread, so we can start trying it out. Ask, Suggest better ideas, all this is welcome.
Best Regards!
N.B. Of course, this is not written in stone, in case we find a better approach on solving those issues, we will change to it. And we will keep improving so don't be afraid that it will be enfo= rced: if this does not work out we will discard it.
P.S. Kudos to dcaro, most of the work was done by him, and most of this= text too.
+1 from me, releasing CI from running non critical and un-essential= jobs will not only reduce load from ci, and shorted response time for developers, it will allow us to add m= uch more powerful tests such as functional & system tests that actually add hosts and run VMs, improving our ability to= find regression much more effectively.
Another benefit to consider is saving reviewers time. I.e not only jenkins benefits from Worklow+1, but also human reviewers. Instead of looking at a patch that is too early to be reviewed, the author can set the Workflow+1 when the code is ready to review (even if he didn't verified it yet), thus saving time to other revi= ewers - for example people can add an email rule to alert them only when they are added to patches that have Workflo= w+1, and not before.
For human reviewers I suggest to keep using drafts until the patch is finished. =20 keep using? how many developers do you know are working with drafts u= ntil their patch is ready? =20 We have ~350 drafts in gerrit so someone is using them :-) https://gerrit.ovirt.org/#/q/is:draft =20 good to know :) so i guess even with that amount its still hard on jenkins....=20
=20
=20
i agree if everyone would use drafts load on jenkins was already much lower, unfortunately its not the case. =20
Once it's finished and humans reviewed the logic of the patch, Workf= low+1 should be triggered allowing automation to check the correctness of =
patch. IMHO there's no reason for wasting CI time on patches that will be c= orrect from an automation point of view but nacked by reviewers. Especially= if the patches are part of a big patchset.
And one final note, for Workflow+2 -> this is a preparation for a g=
ating
system, like Zuul used by openstack, that in the future we might use as automatic merger pending passing a verification ste=
The main issue with drafts is that once it's published, you can't go back. = And for most patches I've seen (followed just a dozen) there are quite more non-ready patches after publishing than in draft stage. the p.
this will prevent errors that happen sometimes post merge due to conflicts or other issues, and will be another le= vel of validation before final merge. But as max said, its all part of the plan and we'll test it of cour= se before implementing to see its value.
Max Kovgan
Senior Software Engineer Red Hat - EMEA ENG Virtualization R&D Tel.: +972 9769 2060 Email: mkovgan [at] redhat [dot] com Web: http://www.redhat.com RHT Global #: 82-72060
_______________________________________________ Devel mailing list Devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/devel
_______________________________________________ Devel mailing list Devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/devel
-- Sandro Bonazzola Better technology. Faster innovation. Powered by community collabora= tion. See how it works at redhat.com _______________________________________________ Infra mailing list Infra@ovirt.org http://lists.ovirt.org/mailman/listinfo/infra
=20 =20 -- Sandro Bonazzola Better technology. Faster innovation. Powered by community collaboratio= n. See how it works at redhat.com
Infra mailing list Infra@ovirt.org http://lists.ovirt.org/mailman/listinfo/infra =20 =20 =20
Infra mailing list Infra@ovirt.org http://lists.ovirt.org/mailman/listinfo/infra
--=20 David Caro Red Hat S.L. Continuous Integration Engineer - EMEA ENG Virtualization R&D Tel.: +420 532 294 605 Email: dcaro@redhat.com Web: www.redhat.com RHT Global #: 82-62605 --K8nIJk4ghYZn606h Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJVb/3PAAoJEEBxx+HSYmnDnxIH/jHHtyMnvplDSSWZSoNC5r8v x7l7LUzc/21Ai1q7DRW9fM1jYtZM+cJQcsrb8YJC5RF6GMV7zL/AF6yXAhbLFQUg vgRC4QgeEk7usZ9Tss70JJKgr1044SAXqxwpcTFrDkBKNmqQpk1eQ5oMHLRe0I9u MxVPlLY+w4m208BAiMRHpITs0B/339a4f4mEqxUl8MMuYRxh5qwe1PQs0mFGt4c8 de7NJUi0AluTMq4soDJUPUQDmknLabUwYL2Dg9EG+x2I66wZI9Rxi44PjUmuXmLf Ig0uHapD6z4DS6HZGAUt+RAKXW4jTgig1vElYgtMUK237FRuDbPvqOzE0fCLfHU= =uymq -----END PGP SIGNATURE----- --K8nIJk4ghYZn606h--