[ovirt-devel] gerrit+ci improvement proposal
Sandro Bonazzola
sbonazzo at redhat.com
Thu Jun 11 13:59:30 UTC 2015
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 <eedri at redhat.com> wrote:
>>>
>>>
>>>
>>> ----- Original Message -----
>>>> From: "Oved Ourfali" <ovedo at redhat.com>
>>>> To: "Eyal Edri" <eedri at redhat.com>
>>>> Cc: infra at ovirt.org, devel at 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 at redhat.com>
>>>>> To: "Eli Mesika" <emesika at redhat.com>
>>>>> Cc: "Oved Ourfali" <ovedo at redhat.com>, devel at ovirt.org, infra at 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 at redhat.com>
>>>>>> To: "Oved Ourfali" <ovedo at redhat.com>
>>>>>> Cc: "Eyal Edri" <eedri at redhat.com>, infra at ovirt.org, devel at 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 at redhat.com>
>>>>>>> To: "Eyal Edri" <eedri at redhat.com>
>>>>>>> Cc: devel at ovirt.org, infra at 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 at redhat.com>
>>>>>>>> To: "Sandro Bonazzola" <sbonazzo at redhat.com>
>>>>>>>> Cc: infra at ovirt.org, devel at 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 at redhat.com>
>>>>>>>>> To: "Eyal Edri" <eedri at redhat.com>, "Max Kovgan"
>>>>>>>>> <mkovgan at redhat.com>
>>>>>>>>> Cc: devel at ovirt.org, infra at 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 at redhat.com>
>>>>>>>>>>> To: devel at ovirt.org
>>>>>>>>>>> Cc: infra at 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 at ovirt.org
>>>>>>>>>>> http://lists.ovirt.org/mailman/listinfo/devel
>>>>>>>>>> _______________________________________________
>>>>>>>>>> Devel mailing list
>>>>>>>>>> Devel at 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 at ovirt.org
>>>>>>>>> http://lists.ovirt.org/mailman/listinfo/infra
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> Devel mailing list
>>>>>>>> Devel at ovirt.org
>>>>>>>> http://lists.ovirt.org/mailman/listinfo/devel
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> Infra mailing list
>>>>>>> Infra at ovirt.org
>>>>>>> http://lists.ovirt.org/mailman/listinfo/infra
>>>>>>>
>>>>>> _______________________________________________
>>>>>> Infra mailing list
>>>>>> Infra at ovirt.org
>>>>>> http://lists.ovirt.org/mailman/listinfo/infra
>>>>>>
>>>>>>
>>>>>>
>>>>> _______________________________________________
>>>>> Infra mailing list
>>>>> Infra at ovirt.org
>>>>> http://lists.ovirt.org/mailman/listinfo/infra
>>>>>
>>>> _______________________________________________
>>>> Infra mailing list
>>>> Infra at ovirt.org
>>>> http://lists.ovirt.org/mailman/listinfo/infra
>>>>
>>>>
>>>>
>>> _______________________________________________
>>> Devel mailing list
>>> Devel at ovirt.org
>>> http://lists.ovirt.org/mailman/listinfo/devel
>>>
>>>
>> _______________________________________________
>> Infra mailing list
>> Infra at ovirt.org
>> http://lists.ovirt.org/mailman/listinfo/infra
>
>
>
> _______________________________________________
> Devel mailing list
> Devel at 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
More information about the Devel
mailing list