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

For now can we agree that we want the ci flag at least?

On 06/07, Oved Ourfali wrote: > > On Jun 7, 2015 10:00 AM, Eyal Edri <eedri(a)redhat.com&gt; wrote: >> >> >> >> ----- Original Message ----- >>> From: "Oved Ourfali" <ovedo(a)redhat.com&gt; >>> To: "Eyal Edri" <eedri(a)redhat.com&gt; >>> Cc: infra(a)ovirt.org, devel(a)ovirt.org >>> Sent: Sunday, June 7, 2015 9:55:56 AM >>> Subject: Re: [ovirt-devel] gerrit+ci improvement proposal >>> >>> >>> >>> ----- Original Message ----- >>>> From: "Eyal Edri" <eedri(a)redhat.com&gt; >>>> To: "Eli Mesika" <emesika(a)redhat.com&gt; >>>> Cc: "Oved Ourfali" <ovedo(a)redhat.com&gt;, devel(a)ovirt.org, infra(a)ovirt.org >>>> Sent: Sunday, June 7, 2015 9:52:15 AM >>>> Subject: Re: [ovirt-devel] gerrit+ci improvement proposal >>>> >>>> >>>> >>>> ----- Original Message ----- >>>>> From: "Eli Mesika" <emesika(a)redhat.com&gt; >>>>> To: "Oved Ourfali" <ovedo(a)redhat.com&gt; >>>>> Cc: "Eyal Edri" <eedri(a)redhat.com&gt;, infra(a)ovirt.org, devel(a)ovirt.org >>>>> Sent: Thursday, June 4, 2015 3:49:05 PM >>>>> Subject: Re: [ovirt-devel] gerrit+ci improvement proposal >>>>> >>>>> >>>>> >>>>> ----- Original Message ----- >>>>>> From: "Oved Ourfali" <ovedo(a)redhat.com&gt; >>>>>> To: "Eyal Edri" <eedri(a)redhat.com&gt; >>>>>> Cc: devel(a)ovirt.org, infra(a)ovirt.org >>>>>> Sent: Thursday, June 4, 2015 10:03:02 AM >>>>>> Subject: Re: [ovirt-devel] gerrit+ci improvement proposal >>>>>> >>>>>> >>>>>> >>>>>> ----- Original Message ----- >>>>>>> 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. >>>>>>> >>>>>> >>>>>> 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. >>>>>> >>>>>> In addition, I think the patch owners shouldn't add reviewers, unless >>>>>> they >>>>>> need their input in the stage of the development. >>>>>> Once they want input, they should add reviewers. >>>>>> >>>>>> 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. >>>>>> 4. Once the patch has +1 from heavy CI, and +2 from reviewer >>>>>> (maintainer), >>>>>> then it can be merged. >>>>>> >>>>>> 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). >>>>> >>>>> +1 >>>>> >>>>> This is he right approach to go (I am also using drafts and if other >>>>> don't, >>>>> we can change that....) >>>>> Also, regarding the claim that publishing a draft is a one-way process, I >>>>> don't think that this is problematic, you should publish a draft after it >>>>> is >>>>> stable and you addressed all comments and run all tests locally >>>>> >>>> >>>> this might be true, but the problem is: >>>> 1. we can't enforce people to use drafts (technically), so until they do, >>>> we'll still have a resource problem >>> >>> >>> We can educate, and I don't see an issue with that. >>> >>>> 2. until we do, even "light ci" jobs running per patch will overload the >>>> ci >>>> without need, this is why relying on another >>>> flag will help - if adding workflow is a problem, we can use the CR+1 >>>> as >>>> first attempt to improve the flow, >>>> and consider in the future to use workflow if it will be needed. (maybe >>>> we can even set it automatically somehow) >>>> >>> >>> Perhaps marking as "verified" can be this flag. >>> If the patch is verified by the author, then you run light CI on it. >>> If it was also CR+1, run the heavy CI. >> >> question is how soon does an author ticks verify on his patch? >> does he wait for code review before? for e.g. - i heard from some developers they wait >> for CI to give them +1 until they even add reviewers, so this might be the chicken and egg problem. > > It depends on the patch I guess. > Again, I think drafts are enough, and that we shouldn't add another flag here, so suggesting alternatives for that. > We can "vote" on that flag addition, and other alternatives, and see what people say. > >> >>> >>> That way you both don't need a new flag, and you don't waste resources on >>> non-manually-verified bugs. >>> >>>>>> >>>>>> >>>>>> >>>>>>>> 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 >>>>>>> >>>>>>> >>>>>>> >>>>>> _______________________________________________ >>>>>> Infra mailing list >>>>>> Infra(a)ovirt.org >>>>>> http://lists.ovirt.org/mailman/listinfo/infra >>>>>> >>>>> _______________________________________________ >>>>> Infra mailing list >>>>> Infra(a)ovirt.org >>>>> http://lists.ovirt.org/mailman/listinfo/infra >>>>> >>>>> >>>>> >>>> _______________________________________________ >>>> Infra mailing list >>>> Infra(a)ovirt.org >>>> http://lists.ovirt.org/mailman/listinfo/infra >>>> >>> _______________________________________________ >>> 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 >> >> > _______________________________________________ > 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