--Pk6IbRAofICFmK5e Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On 06/11, Nir Soffer wrote:
----- Original Message -----
From: "David Caro" <dcaroest@redhat.com> To: "Oved Ourfali" <oourfali@redhat.com> Cc: "Oved Ourfali" <ovedo@redhat.com>, "Eyal Edri" <eedri@redhat.com>, = "infra" <infra@ovirt.org>, devel@ovirt.org Sent: Thursday, June 11, 2015 1:19:01 PM Subject: Re: [ovirt-devel] gerrit+ci improvement proposal =20 =20 =20 For now can we agree that we want the ci flag at least? =20 We should not vote on the "how the system will be implemented", but on "what the system should do".
I'm not asking for a vote, I'm asking for agreement on a small solution to solve an issue that is hurting us right now, that will grant us time to discuss the direction we want to take. When you are bleeding a lot the first step is stopping the bleed, then you = can reevaluate the situation and decide if you should or should not to put your hand in the blender again.
=20
=20 On 06/07, Oved Ourfali wrote:
=20 On Jun 7, 2015 10:00 AM, Eyal Edri <eedri@redhat.com> wrote:
----- Original Message -----
From: "Oved Ourfali" <ovedo@redhat.com> To: "Eyal Edri" <eedri@redhat.com> Cc: infra@ovirt.org, devel@ovirt.org Sent: Sunday, June 7, 2015 9:55:56 AM Subject: Re: [ovirt-devel] gerrit+ci improvement proposal =20 =20 =20 ----- Original Message -----
From: "Eyal Edri" <eedri@redhat.com> To: "Eli Mesika" <emesika@redhat.com> Cc: "Oved Ourfali" <ovedo@redhat.com>, devel@ovirt.org, infra@ovirt.org Sent: Sunday, June 7, 2015 9:52:15 AM Subject: Re: [ovirt-devel] gerrit+ci improvement proposal =20 =20 =20 ----- Original Message ----- > From: "Eli Mesika" <emesika@redhat.com> > To: "Oved Ourfali" <ovedo@redhat.com> > Cc: "Eyal Edri" <eedri@redhat.com>, infra@ovirt.org, > devel@ovirt.org > Sent: Thursday, June 4, 2015 3:49:05 PM > Subject: Re: [ovirt-devel] gerrit+ci improvement proposal >=20 >=20 >=20 > ----- Original Message ----- > > From: "Oved Ourfali" <ovedo@redhat.com> > > To: "Eyal Edri" <eedri@redhat.com> > > Cc: devel@ovirt.org, infra@ovirt.org > > Sent: Thursday, June 4, 2015 10:03:02 AM > > Subject: Re: [ovirt-devel] gerrit+ci improvement proposal > >=20 > >=20 > >=20 > > ----- Original Message ----- > > > From: "Eyal Edri" <eedri@redhat.com> > > > To: "Sandro Bonazzola" <sbonazzo@redhat.com> > > > Cc: infra@ovirt.org, devel@ovirt.org > > > Sent: Thursday, June 4, 2015 9:46:40 AM > > > Subject: Re: [ovirt-devel] gerrit+ci improvement proposal > > >=20 > > >=20 > > >=20 > > > ----- Original Message ----- > > > > From: "Sandro Bonazzola" <sbonazzo@redhat.com> > > > > To: "Eyal Edri" <eedri@redhat.com>, "Max Kovgan" > > > > <mkovgan@redhat.com> > > > > Cc: devel@ovirt.org, infra@ovirt.org > > > > Sent: Thursday, June 4, 2015 9:11:10 AM > > > > Subject: Re: [ovirt-devel] gerrit+ci improvement propos=
al
> > > >=20 > > > > Il 03/06/2015 21:46, Eyal Edri ha scritto: > > > > >=20 > > > > >=20 > > > > > ----- Original Message ----- > > > > >> From: "Max Kovgan" <mkovgan@redhat.com> > > > > >> To: devel@ovirt.org > > > > >> Cc: infra@ovirt.org > > > > >> Sent: Wednesday, June 3, 2015 8:22:54 PM > > > > >> Subject: [ovirt-devel] gerrit+ci improvement proposal > > > > >>=20 > > > > >> Hi everyone! > > > > >> We really want to have reliable and snappy CI: to al= low > > > > >> short > > > > >> cycles > > > > >> and > > > > >> encourage developers to write tests. > > > > >>=20 > > > > >> # Problem > > > > >>=20 > > > > >> Many patches are neither ready for review nor for CI= upon > > > > >> submission, > > > > >> which > > > > >> is OK. > > > > >> But running all the jobs on those patches with limit= ed > > > > >> resources > > > > >> results > > > > >> in: > > > > >> overloaded resources, slow response time, unhappy > > > > >> developers. > > > > >>=20 > > > > >> # Proposed Solution > > > > >>=20 > > > > >> To run less jobs we know we don=E2=80=99t need to, t= hus 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 direct= ions > > > > >> very > > > > >> well > > > > >> internally. > > > > >> Now it seems a good time to let all the oVirt projec= ts to > > > > >> use > > > > >> this. > > > > >> This solution indirectly promotes reviews and quick = tests > > > > >> - =E2=80=9Cto > > > > >> fail > > > > >> early=E2=80=9D, > > > > >> yet full blown static code analysis and long tests t= o run > > > > >> =E2=80=9Cwhen > > > > >> ready=E2=80=9D. > > > > >>=20 > > > > >> # How it works > > > > >>=20 > > > > >> 2 new gerrit independent flags are added to gerrit. > > > > >>=20 > > > > >> ## CI flag > > > > >>=20 > > > > >> Will express patch CI status. Values: > > > > >>=C2=A0 * +1 CI passed > > > > >>=C2=A0 *=C2=A0 0 CI did not run yet > > > > >>=C2=A0 * -1 CI failed > > > > >> Permissions for setting: project maintainers (for sp= ecial > > > > >> cases) > > > > >> should > > > > >> be > > > > >> able to set/override (except Jenkins). > > > > >>=20 > > > > >> ## Workflow flag > > > > >>=20 > > > > >> Will express patch =E2=80=9Cworkflow=E2=80=9D state.= Values: > > > > >>=C2=A0 *=C2=A0 0 Work In Progress > > > > >>=C2=A0 * +1 Ready For Review > > > > >>=C2=A0 * +2 Ready For Merge > > > > >> Permissions for setting: Owner can set +1, Project > > > > >> Maintainers > > > > >> can > > > > >> set > > > > >> +2 > > > > >>=20 > > > > >> ## Review + CI Integration: > > > > >>=20 > > > > >> Merging [=E2=80=9CSubmit=E2=80=9D button to appear] = will require: > > > > >> Review+1, > > > > >> CI+1, > > > > >> Workflow+2 > > > > >> Patch lifecycle now is: > > > > >> ----------------------------------------------------=
> > > > >> patch state=C2=A0=C2=A0 |owner=C2=A0=C2=A0=C2=A0=C2= =A0 |reviewer |maintainer |CI tests > > > > >> |pass > > > > >> ----------------------------------------------------=
> > > > >> 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 > > > > >> review=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |Wo= rkflow+1|Review+1 |-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 = |heavy > > > > >> |CI+1 > > > > >> 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 |Work= flow+2 |gating > > > > >> =C2=A0=C2=A0 |CI+1 > > > > >> 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 > > > > >> 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. > > > > >>=20 > > > > >>=20 > > > > >> Next step will be to automate merge the change after > > > > >> Workflow+2 > > > > >> has > > > > >> been > > > > >> set > > > > >> by the Maintainer and gating tests passed. > > > > >>=20 > > > > >>=20 > > > > >> ## Why now? > > > > >>=20 > > > > >> 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 l= ead to > > > > >> adding > > > > >> unreliable > > > > >> code into jobs, and will still be prone to problems: > > > > >>=C2=A0=C2=A0 Just recently, 3d ago we=E2=80=99ve trie= d detecting what to run > > > > >>=C2=A0=C2=A0 from > > > > >>=C2=A0=C2=A0 jenkins > > > > >>=C2=A0=C2=A0 relying only on gerrit comments so that = upon Verified+1, > > > > >>=C2=A0=C2=A0 we=E2=80=99d > > > > >>=C2=A0=C2=A0 run > > > > >>=C2=A0=C2=A0 the > > > > >>=C2=A0=C2=A0 job. > > > > >>=C2=A0=C2=A0 We could not use =E2=80=9CReview+1=E2=80= =9D, because it makes no sense > > > > >>=C2=A0=C2=A0 at all, > > > > >>=C2=A0=C2=A0 so > > > > >>=C2=A0=C2=A0 we > > > > >>=C2=A0=C2=A0 left > > > > >>=C2=A0=C2=A0 the job to set Verified+1. > > > > >>=C2=A0=C2=A0 Meaning - re-trigger itself immediately = more than 1 > > > > >>=C2=A0=C2=A0 times. > > > > >>=C2=A0=C2=A0=20 > > > > >>=C2=A0=C2=A0 Jenkins and its visitors very unhappy, a= nd we had to > > > > >>=C2=A0=C2=A0 stop > > > > >>=C2=A0=C2=A0 those > > > > >>=C2=A0=C2=A0 jobs, > > > > >>=C2=A0=C2=A0 clean > > > > >>=C2=A0=C2=A0 up the queue, and spam developers. > > > > >>=20 > > > > >> ## OK OK OK. Now what? > > > > >>=20 > > > > >> Now we want your comments and opinions before pushin= g this > > > > >> further: > > > > >> Please participate in this thread, so we can start t= rying > > > > >> it > > > > >> out. > > > > >> Ask, Suggest better ideas, all this is welcome. > > > > >>=20 > > > > >>=20 > > > > >> Best Regards! > > > > >>=20 > > > > >>=20 > > > > >> 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 i= t will > > > > >> be > > > > >> enforced: > > > > >> if > > > > >> this does not work out we will discard it. > > > > >>=20 > > > > >> P.S. > > > > >> Kudos to dcaro, most of the work was done by him, an= d most > > > > >> of > > > > >> this > > > > >> text > > > > >> too. > > > > >>=20 > > > > >=20 > > > > > +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 all= ow 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. > > > > >=20 > > > > > 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 revi= ew > > > > > (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 tha= t have > > > > > Workflow+1, > > > > > and > > > > > not before. > > > >=20 > > > > For human reviewers I suggest to keep using drafts unti= l the > > > > patch > > > > is > > > > finished. > > >=20 > > > keep using? how many developers do you know are working w= ith > > > 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. > > >=20 > >=20 > > 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, an= d less > > on > > bug-fixes. > >=20 > > In addition, I think the patch owners shouldn't add reviewe= rs, > > unless > > they > > need their input in the stage of the development. > > Once they want input, they should add reviewers. > >=20 > > 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) review= er, > > 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. > >=20 > > That's the process we have today, with slight change on whe= n to > > run the > > CI > > and what CI to run (no CI on drafts, light CI on non-draft,= heavy > > CI on > > +1 > > patches). >=20 > +1 >=20 > 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 dr= aft > after it > is > stable and you addressed all comments and run all tests local= ly >=20 =20 this might be true, but the problem is: =C2=A0 1. we can't enforce people to use drafts (technically), s= o until =C2=A0 they do, =C2=A0 we'll still have a resource problem =20 =20 We can educate, and I don't see an issue with that. =20 =C2=A0 2. until we do, even "light ci" jobs running per patch wi= ll overload =C2=A0 the =C2=A0 ci =C2=A0 without need, this is why relying on another =C2=A0=C2=A0=C2=A0=C2=A0 flag will help - if adding workflow is = a problem, we can use the =C2=A0=C2=A0=C2=A0=C2=A0 CR+1 =C2=A0=C2=A0=C2=A0=C2=A0 as =C2=A0=C2=A0=C2=A0=C2=A0 first attempt to improve the flow, =C2=A0=C2=A0=C2=A0=C2=A0 and consider in the future to use workf= low if it will be needed. =C2=A0=C2=A0=C2=A0=C2=A0 (maybe =C2=A0=C2=A0=C2=A0=C2=A0 we can even set it automatically someho= w) =20 =20 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. =20 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. =20
=20 That way you both don't need a new flag, and you don't waste reso= urces on non-manually-verified bugs. =20
> >=20 > >=20 > >=20 > > > > 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 t= hat > > > > will be > > > > correct > > > > from an automation point of view but nacked by reviewer= s. > > > > Especially > > > > if > > > > the > > > > patches are part of a big patchset. > > > >=20 > > > >=20 > > > > >=20 > > > > > And one final note, for Workflow+2 -> this is a prepa= ration > > > > > for a > > > > > gating > > > > > system, like Zuul used by openstack, that in the futu= re > > > > > 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 t= est it > > > > > of > > > > > course > > > > > before implementing to see its value. > > > > >=20 > > > > >=20 > > > > >>=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 > > > > >> _______________________________________________ > > > > >> Devel mailing list > > > > >> Devel@ovirt.org > > > > >> http://lists.ovirt.org/mailman/listinfo/devel > > > > > _______________________________________________ > > > > > Devel mailing list > > > > > Devel@ovirt.org > > > > > http://lists.ovirt.org/mailman/listinfo/devel > > > > >=20 > > > >=20 > > > >=20 > > > > -- > > > > Sandro Bonazzola > > > > Better technology. Faster innovation. Powered by commun= ity > > > > collaboration. > > > > See how it works at redhat.com > > > > _______________________________________________ > > > > Infra mailing list > > > > Infra@ovirt.org > > > > http://lists.ovirt.org/mailman/listinfo/infra > > > >=20 > > > >=20 > > > >=20 > > > _______________________________________________ > > > Devel mailing list > > > Devel@ovirt.org > > > http://lists.ovirt.org/mailman/listinfo/devel > > >=20 > > >=20 > > >=20 > > _______________________________________________ > > Infra mailing list > > Infra@ovirt.org > > http://lists.ovirt.org/mailman/listinfo/infra > >=20 > _______________________________________________ > Infra mailing list > Infra@ovirt.org > http://lists.ovirt.org/mailman/listinfo/infra >=20 >=20 >=20 _______________________________________________ Infra mailing list Infra@ovirt.org http://lists.ovirt.org/mailman/listinfo/infra =20
Infra mailing list Infra@ovirt.org http://lists.ovirt.org/mailman/listinfo/infra =20 =20 =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 =20 Red Hat S.L. Continuous Integration Engineer - EMEA ENG Virtualization R&D =20 Tel.: +420 532 294 605 Email: dcaro@redhat.com Web: www.redhat.com RHT Global #: 82-62605 =20
Infra mailing list Infra@ovirt.org http://lists.ovirt.org/mailman/listinfo/infra =20
--=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 --Pk6IbRAofICFmK5e Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJVeZ2nAAoJEEBxx+HSYmnDTXAH/ROekk+O92yyt+Uoa28Q8gjR aQNbFAGnVnxeidTOWn6q9osaa1E+3V6d4w0ZSp3YB6gR1kUJms7qTxmhPuMqwq/b pILZA6P37hMEL3fa2tQRGm06ccYiIH6ZL6GkYrG5oc5+3n6GvUyB2UqMXAaR/zqu YkxTKOcPgssKg8ncwrVnNFIP2hMT7rEGrb1CWPVsROvjvo2dMogUMj8m8XitXXPE Q0EQ7/B9BhMJZZES+6VuHBDJV2wO8G+zazj3BNYZvGjZlpojue185pX9n+4joULN dUtNw04x+RCJnb7lzYRdJtxL/SqndX4zsXwNaTKh5yzVqbV2wWa0xdU4YCGGJls= =lXz0 -----END PGP SIGNATURE----- --Pk6IbRAofICFmK5e--