hi, Dear Developers! =20 We are aware of the urge and pressure to push your patches a.s.a.p before=
git review -D -r <repo> <branch> example with repo "origin" and branch "master": git review -D -r origin master =20
Howeever, with current flow of patch updates, we currently have an overlo= aded CI, which will result in a very long waiting time for jobs to start SO, in order to reduce that time, please make sure you are using DRAFTs [= 1] for early stage patches [during human reviews] =20 how to use drafts: 1) using plain git push - refer to [1] 2) using git review plugin if you've installed git review plugin: plugin can be installed as explained here: [2] =20 =20 [1] http://stackoverflow.com/questions/18106064/how-to-push-drafts-to-ger= rit [2] http://www.mediawiki.org/wiki/Gerrit/git-review#Fedora.2FCentOS =20 =20 N.B. =20 Using drafts is the only alternative for adding 2 new gerrit flags. If you have better ideas - you are still welcome to suggest. =20 Earlier today I did some mining on gerrit patches status: out of ~1400 open patches ~350 are drafts, which was incouraging. Yet out of all 66 committers for those patches only 6 are using drafts: In [25]: for c in changes: ....: if c.get('status') !=3D 'DRAFT': ....: continue ....: saints.add(c.get('owner').get('name')) ....: =20 In [26]: saints Out[26]:=20 {u'Alon Bar-Lev', u'Liron Aravot', u'Martin Mucha', u'Nir Soffer', u'Piotr Kliczewski', u'Tomer Saban'} Kudos. =20 =20 =20 Max Kovgan =20 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 =20 ----- Forwarded Message ----- From: "Oved Ourfali" <oourfali@redhat.com> To: "Eyal Edri" <eedri@redhat.com> Cc: "Oved Ourfali" <ovedo@redhat.com>, devel@ovirt.org, "infra" <infra@ov= irt.org> Sent: Sunday, June 7, 2015 10:05:23 AM Subject: Re: [ovirt-devel] gerrit+ci improvement proposal =20 =20 On Jun 7, 2015 10:00 AM, Eyal Edri <eedri@redhat.com> wrote:
----- Original Message -----=20
From: "Oved Ourfali" <ovedo@redhat.com>=20 To: "Eyal Edri" <eedri@redhat.com>=20 Cc: infra@ovirt.org, devel@ovirt.org=20 Sent: Sunday, June 7, 2015 9:55:56 AM=20 Subject: Re: [ovirt-devel] gerrit+ci improvement proposal=20 =20 =20 =20 ----- Original Message -----=20
From: "Eyal Edri" <eedri@redhat.com>=20 To: "Eli Mesika" <emesika@redhat.com>=20 Cc: "Oved Ourfali" <ovedo@redhat.com>, devel@ovirt.org, infra@ovirt=
=2Eorg=20
Sent: Sunday, June 7, 2015 9:52:15 AM=20 Subject: Re: [ovirt-devel] gerrit+ci improvement proposal=20 =20 =20 =20 ----- Original Message -----=20
From: "Eli Mesika" <emesika@redhat.com>=20 To: "Oved Ourfali" <ovedo@redhat.com>=20 Cc: "Eyal Edri" <eedri@redhat.com>, infra@ovirt.org, devel@ovirt.= org=20 Sent: Thursday, June 4, 2015 3:49:05 PM=20 Subject: Re: [ovirt-devel] gerrit+ci improvement proposal=20 =20 =20 =20 ----- Original Message -----=20
From: "Oved Ourfali" <ovedo@redhat.com>=20 To: "Eyal Edri" <eedri@redhat.com>=20 Cc: devel@ovirt.org, infra@ovirt.org=20 Sent: Thursday, June 4, 2015 10:03:02 AM=20 Subject: Re: [ovirt-devel] gerrit+ci improvement proposal=20 =20 =20 =20 ----- Original Message -----=20 > From: "Eyal Edri" <eedri@redhat.com>=20 > To: "Sandro Bonazzola" <sbonazzo@redhat.com>=20 > Cc: infra@ovirt.org, devel@ovirt.org=20 > Sent: Thursday, June 4, 2015 9:46:40 AM=20 > Subject: Re: [ovirt-devel] gerrit+ci improvement proposal=20 >=20 >=20 >=20 > ----- Original Message -----=20 > > From: "Sandro Bonazzola" <sbonazzo@redhat.com>=20 > > To: "Eyal Edri" <eedri@redhat.com>, "Max Kovgan"=20 > > <mkovgan@redhat.com>=20 > > Cc: devel@ovirt.org, infra@ovirt.org=20 > > Sent: Thursday, June 4, 2015 9:11:10 AM=20 > > Subject: Re: [ovirt-devel] gerrit+ci improvement proposal= =20 > >=20 > > Il 03/06/2015 21:46, Eyal Edri ha scritto:=20 > > >=20 > > >=20 > > > ----- Original Message -----=20 > > >> From: "Max Kovgan" <mkovgan@redhat.com>=20 > > >> To: devel@ovirt.org=20 > > >> Cc: infra@ovirt.org=20 > > >> Sent: Wednesday, June 3, 2015 8:22:54 PM=20 > > >> Subject: [ovirt-devel] gerrit+ci improvement proposal=20 > > >>=20 > > >> Hi everyone!=20 > > >> We really want to have reliable and snappy CI: to allow = short=20 > > >> cycles=20 > > >> and=20 > > >> encourage developers to write tests.=20 > > >>=20 > > >> # Problem=20 > > >>=20 > > >> Many patches are neither ready for review nor for CI upo= n=20 > > >> submission,=20 > > >> which=20 > > >> is OK.=20 > > >> But running all the jobs on those patches with limited r= esources=20 > > >> results=20 > > >> in:=20 > > >> overloaded resources, slow response time, unhappy develo=
> > >>=20 > > >> # Proposed Solution=20 > > >>=20 > > >> To run less jobs we know we don=E2=80=99t need to, thus = making more=20 > > >> resources=20 > > >> for=20 > > >> the=20 > > >> jobs we need to run.=20 > > >> We have been experimenting to make our CI stabler and qu= icker to=20 > > >> respond=20 > > >> by=20 > > >> using gerrit flags. This has improved in both directions= very=20 > > >> well=20 > > >> internally.=20 > > >> Now it seems a good time to let all the oVirt projects t= o use=20 > > >> this.=20 > > >> This solution indirectly promotes reviews and quick test= s - =E2=80=9Cto=20 > > >> fail=20 > > >> early=E2=80=9D,=20 > > >> yet full blown static code analysis and long tests to ru= n =E2=80=9Cwhen=20 > > >> ready=E2=80=9D.=20 > > >>=20 > > >> # How it works=20 > > >>=20 > > >> 2 new gerrit independent flags are added to gerrit.=20 > > >>=20 > > >> ## CI flag=20 > > >>=20 > > >> Will express patch CI status. Values:=20 > > >>=C2=A0 * +1 CI passed=20 > > >>=C2=A0 *=C2=A0 0 CI did not run yet=20 > > >>=C2=A0 * -1 CI failed=20 > > >> Permissions for setting: project maintainers (for specia= l cases)=20 > > >> should=20 > > >> be=20 > > >> able to set/override (except Jenkins).=20 > > >>=20 > > >> ## Workflow flag=20 > > >>=20 > > >> Will express patch =E2=80=9Cworkflow=E2=80=9D state. Val= ues:=20 > > >>=C2=A0 *=C2=A0 0 Work In Progress=20 > > >>=C2=A0 * +1 Ready For Review=20 > > >>=C2=A0 * +2 Ready For Merge=20 > > >> Permissions for setting: Owner can set +1, Project Maint= ainers=20 > > >> can=20 > > >> set=20 > > >> +2=20 > > >>=20 > > >> ## Review + CI Integration:=20 > > >>=20 > > >> Merging [=E2=80=9CSubmit=E2=80=9D button to appear] will= require: Review+1,=20 > > >> CI+1,=20 > > >> Workflow+2=20 > > >> Patch lifecycle now is:=20 > > >> --------------------------------------------------------= -------=20 > > >> patch state=C2=A0=C2=A0 |owner=C2=A0=C2=A0=C2=A0=C2=A0 |= reviewer |maintainer |CI tests |pass=20 > > >> --------------------------------------------------------= -------=20 > > >> added/updated |-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 |-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |-=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |quick=C2=A0=C2=A0=C2=A0 |CI+1=20 > > >> review=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |Workfl= ow+1|Review+1 |-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |hea= vy |CI+1=20 > > >> merge ready=C2=A0=C2=A0 |-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 |-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |Workflow+2= |gating=C2=A0=C2=A0 |CI+1=20 > > >> merge=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |-= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |-=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 |merge=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |merge=C2=A0=C2=A0= =C2=A0 |CI+1=20 > > >>=20 > > >> Changes from current workflow:=20 > > >> Owner only adds reviewers, now owner needs to set "Workf= low+1"=20 > > >> for=20 > > >> the=20 > > >> patch=20 > > >> to be reviewed, and heavily auto-tested.=20 > > >> Maintainer now needs to set "Workflow+2" and wait for "S= ubmit"=20 > > >> button=20 > > >> to=20 > > >> appear after CI has completed running gating tests.=20 > > >>=20 > > >>=20 > > >> Next step will be to automate merge the change after Wor= kflow+2=20 > > >> has=20 > > >> been=20 > > >> set=20 > > >> by the Maintainer and gating tests passed.=20 > > >>=20 > > >>=20 > > >> ## Why now?=20 > > >>=20 > > >> It is elimination of waste. The sooner - the better.=20 > > >> The solution has been used for a while and it works.=20 > > >> Resolving the problem without gerrit involved will lead = to=20 > > >> adding=20 > > >> unreliable=20 > > >> code into jobs, and will still be prone to problems:=20 > > >>=C2=A0=C2=A0 Just recently, 3d ago we=E2=80=99ve tried de= tecting what to run from=20 > > >>=C2=A0=C2=A0 jenkins=20 > > >>=C2=A0=C2=A0 relying only on gerrit comments so that upon= Verified+1, we=E2=80=99d=20 > > >>=C2=A0=C2=A0 run=20 > > >>=C2=A0=C2=A0 the=20 > > >>=C2=A0=C2=A0 job.=20 > > >>=C2=A0=C2=A0 We could not use =E2=80=9CReview+1=E2=80=9D,= because it makes no sense at all,=20 > > >>=C2=A0=C2=A0 so=20 > > >>=C2=A0=C2=A0 we=20 > > >>=C2=A0=C2=A0 left=20 > > >>=C2=A0=C2=A0 the job to set Verified+1.=20 > > >>=C2=A0=C2=A0 Meaning - re-trigger itself immediately more=
> > >>=C2=A0=C2=A0=20 > > >>=C2=A0=C2=A0 Jenkins and its visitors very unhappy, and w= e had to stop=20 > > >>=C2=A0=C2=A0 those=20 > > >>=C2=A0=C2=A0 jobs,=20 > > >>=C2=A0=C2=A0 clean=20 > > >>=C2=A0=C2=A0 up the queue, and spam developers.=20 > > >>=20 > > >> ## OK OK OK. Now what?=20 > > >>=20 > > >> Now we want your comments and opinions before pushing th= is=20 > > >> further:=20 > > >> Please participate in this thread, so we can start tryin= g it=20 > > >> out.=20 > > >> Ask, Suggest better ideas, all this is welcome.=20 > > >>=20 > > >>=20 > > >> Best Regards!=20 > > >>=20 > > >>=20 > > >> N.B.=20 > > >> Of course, this is not written in stone, in case we find= a=20 > > >> better=20 > > >> approach=20 > > >> on=20 > > >> solving those issues, we will change to it.=20 > > >> And we will keep improving so don't be afraid that it wi= ll be=20 > > >> enforced:=20 > > >> if=20 > > >> this does not work out we will discard it.=20 > > >>=20 > > >> P.S.=20 > > >> Kudos to dcaro, most of the work was done by him, and mo= st of=20 > > >> this=20 > > >> text=20 > > >> too.=20 > > >>=20 > > >=20 > > > +1 from me, releasing CI from running non critical and=20 > > > un-essential=20 > > > jobs=20 > > > will not only reduce load from ci,=20 > > > and shorted response time for developers, it will allow u= s to add=20 > > > much=20 > > > more=20 > > > powerful tests such as functional & system=20 > > > tests that actually add hosts and run VMs, improving our = ability=20 > > > to=20 > > > find=20 > > > regression much more effectively.=20 > > >=20 > > > Another benefit to consider is saving reviewers time. I.e= not=20 > > > only=20 > > > jenkins=20 > > > benefits from Worklow+1, but also human reviewers.=20 > > > Instead of looking at a patch that is too early to be rev= iewed,=20 > > > the=20 > > > author=20 > > > can set the Workflow+1 when the code is ready to review= =20 > > > (even if he didn't verified it yet), thus saving time to = other=20 > > > reviewers=20 > > > -=20 > > > for example people can add an email rule=20 > > > to alert them only when they are added to patches that ha= ve=20 > > > Workflow+1,=20 > > > and=20 > > > not before.=20 > >=20 > > For human reviewers I suggest to keep using drafts until th= e patch=20 > > is=20 > > finished.=20 >=20 > keep using? how many developers do you know are working with = drafts=20 > until=20 > their patch is ready?=20 > i agree if everyone would use drafts load on jenkins was alre= ady much=20 > lower,=20 > unfortunately its not the case.=20 >=20 =20 IMO we don't need the "workflow" flag.=20 I'm okay with CI not running on "drafts". And yes... we do use =
We can try and educate people to use them more where needed.=20 Drafts should be widely used in first-phase development, and le= ss on=20 bug-fixes.=20 =20 In addition, I think the patch owners shouldn't add reviewers, = unless=20 they=20 need their input in the stage of the development.=20 Once they want input, they should add reviewers.=20 =20 1. So, if the patch is draft then no CI runs on it.=20 2. Once it turns into non-draft, you can run "light-CI" on it.= =20 3. Once the patch has at least one +1 from a (human) reviewer, =
should=20 run the "heavy" CI.=20 4. Once the patch has +1 from heavy CI, and +2 from reviewer=20 (maintainer),=20 then it can be merged.=20 =20 That's the process we have today, with slight change on when to= run the=20 CI=20 and what CI to run (no CI on drafts, light CI on non-draft, hea= vy CI on=20 +1=20 patches).=20 =20 +1=20 =20 This is he right approach to go (I am also using drafts and if ot= her=20 don't,=20 we can change that....)=20 Also, regarding the claim that publishing a draft is a one-way pr= ocess, I=20 don't think that this is problematic, you should publish a draft = after it=20 is=20 stable and you addressed all comments and run all tests locally= =20 =20 =20 this might be true, but the problem is:=20 =C2=A0 1. we can't enforce people to use drafts (technically), so un= til they do,=20 =C2=A0 we'll still have a resource problem=20 =20 =20 We can educate, and I don't see an issue with that.=20 =20 =C2=A0 2. until we do, even "light ci" jobs running per patch will o= verload the=20 =C2=A0 ci=20 =C2=A0 without need, this is why relying on another=20 =C2=A0=C2=A0=C2=A0=C2=A0 flag will help - if adding workflow is a pr= oblem, we can use the CR+1=20 =C2=A0=C2=A0=C2=A0=C2=A0 as=20 =C2=A0=C2=A0=C2=A0=C2=A0 first attempt to improve the flow,=20 =C2=A0=C2=A0=C2=A0=C2=A0 and consider in the future to use workflow = if it will be needed. (maybe=20 =C2=A0=C2=A0=C2=A0=C2=A0 we can even set it automatically somehow)= =20 =20 =20 Perhaps marking as "verified" can be this flag.=20 If the patch is verified by the author, then you run light CI on it.= =20 If it was also CR+1, run the heavy CI.=20
question is how soon does an author ticks verify on his patch?=20 does he wait for code review before? for e.g. - i heard from some devel= opers they wait=20 for CI to give them +1 until they even add reviewers, so this might be =
=20 It depends on the patch I guess.=20 Again, I think drafts are enough, and that we shouldn't add another flag= here, so suggesting alternatives for that.=20 We can "vote" on that flag addition, and other alternatives, and see what=
--i7F3eY7HS/tUJxUd Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable As a first contention measure to alleviate the flood, I've changed the way findbugs is triggered to get triggered only on verified +1 (it will set code review flag on pass or failure, and will run on drafts, can't use more than= one condition for the trigger). On 06/09, Max Kovgan wrote: the code freeze. pers.=20 than 1 times.=20 them.=20 then it=20 the chicken and egg problem.=20 people say.=20
=20
=20 That way you both don't need a new flag, and you don't waste resource=
s on=20
non-manually-verified bugs.=20 =20
=20 =20 =20 > > Once it's finished and humans reviewed the logic of the pat= ch,=20 > > Workflow+1=20 > > should be triggered allowing automation to check the correc= tness of=20 > > the=20 > > patch.=20 > > IMHO there's no reason for wasting CI time on patches that = will be=20 > > correct=20 > > from an automation point of view but nacked by reviewers.= =20 > > Especially=20 > > if=20 > > the=20 > > patches are part of a big patchset.=20 > >=20 > >=20 > > >=20 > > > And one final note, for Workflow+2 -> this is a preparati= on for a=20 > > > gating=20 > > > system, like Zuul used by openstack, that in the future= =20 > > > we might use as automatic merger pending passing a verifi= cation=20 > > > step.=20 > > > this=20 > > > will prevent errors that happen sometimes=20 > > > post merge due to conflicts or other issues, and will be = another=20 > > > level=20 > > > of=20 > > > validation before final merge.=20 > > > But as max said, its all part of the plan and we'll test = it of=20 > > > course=20 > > > before implementing to see its value.=20 > > >=20 > > >=20 > > >>=20 > > >>=20 > > >>=20 > > >> Max Kovgan=20 > > >>=20 > > >> Senior Software Engineer=20 > > >> Red Hat - EMEA ENG Virtualization R&D=20 > > >> Tel.: +972 9769 2060=20 > > >> Email: mkovgan [at] redhat [dot] com=20 > > >> Web: http://www.redhat.com=20 > > >> RHT Global #: 82-72060=20 > > >>=20 > > >> _______________________________________________=20 > > >> Devel mailing list=20 > > >> Devel@ovirt.org=20 > > >> http://lists.ovirt.org/mailman/listinfo/devel=20 > > > _______________________________________________=20 > > > Devel mailing list=20 > > > Devel@ovirt.org=20 > > > http://lists.ovirt.org/mailman/listinfo/devel=20 > > >=20 > >=20 > >=20 > > --=20 > > Sandro Bonazzola=20 > > Better technology. Faster innovation. Powered by community= =20 > > collaboration.=20 > > See how it works at redhat.com=20 > > _______________________________________________=20 > > Infra mailing list=20 > > Infra@ovirt.org=20 > > http://lists.ovirt.org/mailman/listinfo/infra=20 > >=20 > >=20 > >=20 > _______________________________________________=20 > Devel mailing list=20 > Devel@ovirt.org=20 > http://lists.ovirt.org/mailman/listinfo/devel=20 >=20 >=20 >=20 _______________________________________________=20 Infra mailing list=20 Infra@ovirt.org=20 http://lists.ovirt.org/mailman/listinfo/infra=20 =20 _______________________________________________=20 Infra mailing list=20 Infra@ovirt.org=20 http://lists.ovirt.org/mailman/listinfo/infra=20 =20 =20 =20 _______________________________________________=20 Infra mailing list=20 Infra@ovirt.org=20 http://lists.ovirt.org/mailman/listinfo/infra=20 =20 _______________________________________________=20 Infra mailing list=20 Infra@ovirt.org=20 http://lists.ovirt.org/mailman/listinfo/infra=20 =20 =20 =20 _______________________________________________=20 Devel mailing list=20 Devel@ovirt.org=20 http://lists.ovirt.org/mailman/listinfo/devel=20
_______________________________________________ Devel mailing list Devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/devel _______________________________________________ Infra mailing list Infra@ovirt.org http://lists.ovirt.org/mailman/listinfo/infra
--=20 David Caro Red Hat S.L. Continuous Integration Engineer - EMEA ENG Virtualization R&D Tel.: +420 532 294 605 Email: dcaro@redhat.com Web: www.redhat.com RHT Global #: 82-62605 --i7F3eY7HS/tUJxUd Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJVdwkcAAoJEEBxx+HSYmnDfEAH/il/ed5ZiAYMp9cUKtchSizw LR0N4uhWJjZffFSIpt+YhRPTRm9/Ymbpw/2fuQTFpeY6tQ5ls43dIhZsX6lMdBPd qYZhmR8WSpWBnANUI631vhElp9jLwCuYT5G/wRPBGC4q7vxNDfhMLCSWQv3zdNY2 vGHPTjGhSe87tJmeXvFr41eksrvBIfsHCA6pmyFhMehYhn4A+ByOgr3y0U8AQraw EuXZDNM19vzA5WMerXBlh4EEmiHTX5rlyGQd5sIrgA8BQ2moUo3lowRaJX2eI4xM eYtBicXuupYHBxDwr5FY3FGqp2TW/QrijlTnaBzk1HeVZyssGgm5NobKVIF4d1o= =OTxj -----END PGP SIGNATURE----- --i7F3eY7HS/tUJxUd--