[ovirt-devel] gerrit+ci improvement proposal
Nir Soffer
nsoffer at redhat.com
Thu Jun 11 14:27:12 UTC 2015
----- Original Message -----
> From: "David Caro" <dcaroest at redhat.com>
> To: "Oved Ourfali" <oourfali at redhat.com>
> Cc: "Oved Ourfali" <ovedo at redhat.com>, "Eyal Edri" <eedri at redhat.com>, "infra" <infra at ovirt.org>, devel at ovirt.org
> Sent: Thursday, June 11, 2015 1:19:01 PM
> Subject: Re: [ovirt-devel] gerrit+ci improvement proposal
>
>
>
> For now can we agree that we want the ci flag at least?
We should not vote on the "how the system will be implemented", but on
"what the system should do".
>
> 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
>
> --
> David Caro
>
> Red Hat S.L.
> Continuous Integration Engineer - EMEA ENG Virtualization R&D
>
> Tel.: +420 532 294 605
> Email: dcaro at redhat.com
> Web: www.redhat.com
> RHT Global #: 82-62605
>
> _______________________________________________
> Infra mailing list
> Infra at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/infra
>
More information about the Devel
mailing list