Il 11/06/2015 12:19, David Caro ha scritto: > > > For now can we agree that we want the ci flag at least?
+1 > > On 06/07, Oved Ourfali wrote: >> >> On Jun 7, 2015 10:00 AM, Eyal Edri <[email protected]> wrote: >>> >>> >>> >>> ----- Original Message ----- >>>> From: "Oved Ourfali" <[email protected]> >>>> To: "Eyal Edri" <[email protected]> >>>> Cc: [email protected], [email protected] >>>> Sent: Sunday, June 7, 2015 9:55:56 AM >>>> Subject: Re: [ovirt-devel] gerrit+ci improvement proposal >>>> >>>> >>>> >>>> ----- Original Message ----- >>>>> From: "Eyal Edri" <[email protected]> >>>>> To: "Eli Mesika" <[email protected]> >>>>> Cc: "Oved Ourfali" <[email protected]>, [email protected], [email protected] >>>>> Sent: Sunday, June 7, 2015 9:52:15 AM >>>>> Subject: Re: [ovirt-devel] gerrit+ci improvement proposal >>>>> >>>>> >>>>> >>>>> ----- Original Message ----- >>>>>> From: "Eli Mesika" <[email protected]> >>>>>> To: "Oved Ourfali" <[email protected]> >>>>>> Cc: "Eyal Edri" <[email protected]>, [email protected], [email protected] >>>>>> Sent: Thursday, June 4, 2015 3:49:05 PM >>>>>> Subject: Re: [ovirt-devel] gerrit+ci improvement proposal >>>>>> >>>>>> >>>>>> >>>>>> ----- Original Message ----- >>>>>>> From: "Oved Ourfali" <[email protected]> >>>>>>> To: "Eyal Edri" <[email protected]> >>>>>>> Cc: [email protected], [email protected] >>>>>>> Sent: Thursday, June 4, 2015 10:03:02 AM >>>>>>> Subject: Re: [ovirt-devel] gerrit+ci improvement proposal >>>>>>> >>>>>>> >>>>>>> >>>>>>> ----- Original Message ----- >>>>>>>> From: "Eyal Edri" <[email protected]> >>>>>>>> To: "Sandro Bonazzola" <[email protected]> >>>>>>>> Cc: [email protected], [email protected] >>>>>>>> Sent: Thursday, June 4, 2015 9:46:40 AM >>>>>>>> Subject: Re: [ovirt-devel] gerrit+ci improvement proposal >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> ----- Original Message ----- >>>>>>>>> From: "Sandro Bonazzola" <[email protected]> >>>>>>>>> To: "Eyal Edri" <[email protected]>, "Max Kovgan" >>>>>>>>> <[email protected]> >>>>>>>>> Cc: [email protected], [email protected] >>>>>>>>> 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" <[email protected]> >>>>>>>>>>> To: [email protected] >>>>>>>>>>> Cc: [email protected] >>>>>>>>>>> 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 >>>>>>>>>>> [email protected] >>>>>>>>>>> http://lists.ovirt.org/mailman/listinfo/devel >>>>>>>>>> _______________________________________________ >>>>>>>>>> Devel mailing list >>>>>>>>>> [email protected] >>>>>>>>>> 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 >>>>>>>>> [email protected] >>>>>>>>> http://lists.ovirt.org/mailman/listinfo/infra >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>> _______________________________________________ >>>>>>>> Devel mailing list >>>>>>>> [email protected] >>>>>>>> http://lists.ovirt.org/mailman/listinfo/devel >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> _______________________________________________ >>>>>>> Infra mailing list >>>>>>> [email protected] >>>>>>> http://lists.ovirt.org/mailman/listinfo/infra >>>>>>> >>>>>> _______________________________________________ >>>>>> Infra mailing list >>>>>> [email protected] >>>>>> http://lists.ovirt.org/mailman/listinfo/infra >>>>>> >>>>>> >>>>>> >>>>> _______________________________________________ >>>>> Infra mailing list >>>>> [email protected] >>>>> http://lists.ovirt.org/mailman/listinfo/infra >>>>> >>>> _______________________________________________ >>>> Infra mailing list >>>> [email protected] >>>> http://lists.ovirt.org/mailman/listinfo/infra >>>> >>>> >>>> >>> _______________________________________________ >>> Devel mailing list >>> [email protected] >>> http://lists.ovirt.org/mailman/listinfo/devel >>> >>> >> _______________________________________________ >> Infra mailing list >> [email protected] >> http://lists.ovirt.org/mailman/listinfo/infra > > > > _______________________________________________ > Devel mailing list > [email protected] > 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 [email protected] http://lists.ovirt.org/mailman/listinfo/infra
