4 Jun
2015
4 Jun
'15
10:34 a.m.
--ieNMXl1Fr3cevapt Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On 06/04, Nir Soffer wrote: >=20 >=20 > ----- Original Message ----- > > From: "Oved Ourfali" <ovedo@redhat.com> > > To: "Eyal Edri" <eedri@redhat.com> > > Cc: devel@ovirt.org, infra@ovirt.org > > Sent: Thursday, June 4, 2015 10:03:02 AM > > Subject: Re: [ovirt-devel] gerrit+ci improvement proposal > >=20 > >=20 > >=20 > > ----- Original Message ----- > > > From: "Eyal Edri" <eedri@redhat.com> > > > To: "Sandro Bonazzola" <sbonazzo@redhat.com> > > > Cc: infra@ovirt.org, devel@ovirt.org > > > Sent: Thursday, June 4, 2015 9:46:40 AM > > > Subject: Re: [ovirt-devel] gerrit+ci improvement proposal > > >=20 > > >=20 > > >=20 > > > ----- Original Message ----- > > > > From: "Sandro Bonazzola" <sbonazzo@redhat.com> > > > > To: "Eyal Edri" <eedri@redhat.com>, "Max Kovgan" <mkovgan@redhat.co= m> > > > > Cc: devel@ovirt.org, infra@ovirt.org > > > > Sent: Thursday, June 4, 2015 9:11:10 AM > > > > Subject: Re: [ovirt-devel] gerrit+ci improvement proposal > > > >=20 > > > > Il 03/06/2015 21:46, Eyal Edri ha scritto: > > > > >=20 > > > > >=20 > > > > > ----- 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 cy= cles > > > > >> and > > > > >> encourage developers to write tests. > > > > >> > > > > >> # Problem > > > > >> > > > > >> Many patches are neither ready for review nor for CI upon submis= sion, > > > > >> which > > > > >> is OK. > > > > >> But running all the jobs on those patches with limited resources > > > > >> results > > > > >> 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 m= ore resources > > > > >> for > > > > >> the > > > > >> jobs we need to run. > > > > >> We have been experimenting to make our CI stabler and quicker to > > > > >> respond > > > > >> by > > > > >> using gerrit flags. This has improved in both directions very we= ll > > > > >> internally. > > > > >> Now it seems a good time to let all the oVirt projects to use th= is. > > > > >> 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) > > > > >> should > > > > >> 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 c= an 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" f= or the > > > > >> patch > > > > >> to be reviewed, and heavily auto-tested. > > > > >> Maintainer now needs to set "Workflow+2" and wait for "Submit" b= utton > > > > >> to > > > > >> appear after CI has completed running gating tests. > > > > >> > > > > >> > > > > >> Next step will be to automate merge the change after Workflow+2 = has > > > > >> 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 ru= n from 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 furth= er: > > > > >> 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 > > > > >> enforced: > > > > >> 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 th= is > > > > >> text > > > > >> too. > > > > >> > > > > >=20 > > > > > +1 from me, releasing CI from running non critical and un-essenti= al > > > > > jobs > > > > > will not only reduce load from ci, > > > > > and shorted response time for developers, it will allow us to add= much > > > > > more > > > > > powerful tests such as functional & system > > > > > tests that actually add hosts and run VMs, improving our ability = to > > > > > find > > > > > regression much more effectively. > > > > >=20 > > > > > 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, t= he > > > > > 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 > > > > > reviewers > > > > > - > > > > > for example people can add an email rule > > > > > to alert them only when they are added to patches that have Workf= low+1, > > > > > and > > > > > not before. > > > >=20 > > > > 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? > > > i agree if everyone would use drafts load on jenkins was already much > > > lower, > > > unfortunately its not the case. > > >=20 > >=20 > > IMO we don't need the "workflow" flag. > > I'm okay with CI not running on "drafts". And yes... we do use them. > > We can try and educate people to use them more where needed. > > Drafts should be widely used in first-phase development, and less on > > bug-fixes. > >=20 > > In addition, I think the patch owners shouldn't add reviewers, unless t= hey > > need their input in the stage of the development. > > Once they want input, they should add reviewers. > >=20 > > 1. So, if the patch is draft then no CI runs on it. > > 2. Once it turns into non-draft, you can run "light-CI" on it. > > 3. Once the patch has at least one +1 from a (human) reviewer, then it = should > > run the "heavy" CI. >=20 > This is pointless, we will have to ask someone (or add another user) to += 1 the > patch to enable the CI :-) >=20 > > 4. Once the patch has +1 from heavy CI, and +2 from reviewer (maintaine= r), > > then it can be merged. > >=20 > > That's the process we have today, with slight change on when to run the= CI > > and what CI to run (no CI on drafts, light CI on non-draft, heavy CI on= +1 > > patches). >=20 > I think Oved proposal is simpler and more useful. >=20 > However, we need a way to run *any* ci jobs even on a draft. If we cannot > afford this automatically with current system, lets add a way to trigger = this > manually from gerrit. >=20 > So this becomes simply: >=20 > 1. drafts do not run the ci automatically, but the owner can run the ci m= anually There are two ways I can think of such a trigger, through flags (that are easier to manage) or comments. The flags approach is the one that we exposed and tested on a few projects. > 2. published patches run the light ci jobs automatically, owner can run h= eavy jobs manually A big issue here is that there's currently no separation between light and heavy jobs, not even a list of what should be considered as such. So for starters any job is considered to be heavy. > 3. run all ci jobs before merge, maintain can ignore ci results >=20 > This works with the system David suggested in the past, where each projec= t=20 > implement scripts for checking patches and checking patches before merge. Both work with that system that was never implemented on any major project = so far due to reluctance to add ci scripts inside the code base (where I stron= gly think they belong, as the code base evolves, the scripts must change with i= t) So this is not yet implemented everywhere. >=20 > Nir > _______________________________________________ > 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 --ieNMXl1Fr3cevapt Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJVcA2CAAoJEEBxx+HSYmnDe/UH/06AN7Idm85qhypzl3pWv7Y6 7aCaY28VSFfnLprieMGYQXLVRDpDULiKCfja3nGVSUFIZYtYn0rEH3WCLa0+tmUB aS2FAFAvbXxZNfwXcMMJULf1X8fp74FCZU49BCWpnv+SAjWXPbytE+HPrpWI8nR+ PqoHut52Ba5NSDfY/SJT2nmBZrQdbSY5cJBwz3BoovkI3gjfKES5/rd1AAlS8Wip xEUOzcGU3HDD9ucjsp/AHMSHn0tP7U6rkWQSFZPdezjCtDU9LNBIF/TC+eTNuHfo hNmH884ZdLnGEsrrIMMOWD/cJWG5bMmvLfX6VOpXIBxQoSjztOg/I/d/SjLRsnQ= =MvB8 -----END PGP SIGNATURE----- --ieNMXl1Fr3cevapt--