Re: [ovirt-devel] gerrit+ci improvement proposal

From: "Eyal Edri" <eedri(a)redhat.com&gt; To: "Sandro Bonazzola" <sbonazzo(a)redhat.com&gt; Cc: infra(a)ovirt.org, devel(a)ovirt.org Sent: Thursday, June 4, 2015 9:46:40 AM Subject: Re: [ovirt-devel] gerrit+ci improvement proposal ----- Original Message ----- > From: "Sandro Bonazzola" <sbonazzo(a)redhat.com&gt; > To: "Eyal Edri" <eedri(a)redhat.com&gt;, "Max Kovgan" <mkovgan(a)redhat.com&gt; > Cc: devel(a)ovirt.org, infra(a)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(a)redhat.com&gt; > >> To: devel(a)ovirt.org > >> Cc: infra(a)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 cycles and > >> encourage developers to write tests. > >> > >> # Problem > >> > >> Many patches are neither ready for review nor for CI upon submission, > >> 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’t need to, thus making more 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 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 - “to fail > >> early”, > >> yet full blown static code analysis and long tests to run “when ready”. > >> > >> # 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 “workflow” 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 [“Submit” 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 the > >> patch > >> to be reviewed, and heavily auto-tested. > >> Maintainer now needs to set "Workflow+2" and wait for "Submit" button 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’ve tried detecting what to run from jenkins > >> relying only on gerrit comments so that upon Verified+1, we’d run the > >> job. > >> We could not use “Review+1”, 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. > >> > >> 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 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 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 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. > > > > 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 reviewers > > - > > for example people can add an email rule > > to alert them only when they are added to patches that have Workflow+1, > > and > > not before. > > For human reviewers I suggest to keep using drafts until the patch is > finished. keep using? how many developers do you know are working with drafts until their patch is ready? i agree if everyone would use drafts load on jenkins was already much lower, unfortunately its not the case.

> Once it's finished and humans reviewed the logic of the patch, Workflow+1 > should be triggered allowing automation to check the correctness of the > patch. > IMHO there's no reason for wasting CI time on patches that will be correct > 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 gating > > system, like Zuul used by openstack, that in the future > > we might use as automatic merger pending passing a verification step. > > this > > will prevent errors that happen sometimes > > post merge due to conflicts or other issues, and will be another level of > > validation before final merge. > > But as max said, its all part of the plan and we'll test it of course > > 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(a)ovirt.org > >> http://lists.ovirt.org/mailman/listinfo/devel > > _______________________________________________ > > Devel mailing list > > Devel(a)ovirt.org > > http://lists.ovirt.org/mailman/listinfo/devel > > > > > -- > Sandro Bonazzola > Better technology. Faster innovation. Powered by community collaboration. > See how it works at redhat.com > _______________________________________________ > Infra mailing list > Infra(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/infra > > > _______________________________________________ Devel mailing list Devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/devel