[ovirt-devel] gerrit+ci improvement proposal
Eyal Edri
eedri at redhat.com
Sun Jun 7 06:52:15 UTC 2015
----- 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
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)
> >
> >
> >
> > > > 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
>
>
>
More information about the Devel
mailing list