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.

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(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/devel

From: "Eyal Edri" <eedri(a)redhat.com&gt; To: "Max Kovgan" <mkovgan(a)redhat.com&gt; Cc: infra(a)ovirt.org, devel(a)ovirt.org Sent: Wednesday, June 3, 2015 3:46:06 PM Subject: Re: [ovirt-devel] gerrit+ci improvement proposal ----- Original Message ----- > From: "Max Kovgan" <mkovgan(a)redhat.com&gt; > To: devel(a)ovirt.org > Cc: infra(a)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. 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(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/devel _______________________________________________ Infra mailing list Infra(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/infra

From: "Sandro Bonazzola" <sbonazzo(a)redhat.com&gt; To: "Eyal Edri" <eedri(a)redhat.com&gt;, "Max Kovgan" <mkovgan(a)redhat.com&gt; Cc: devel(a)ovirt.org, infra(a)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(a)redhat.com&gt; >> To: devel(a)ovirt.org >> Cc: infra(a)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.

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(a)ovirt.org >> http://lists.ovirt.org/mailman/listinfo/devel > _______________________________________________ > Devel mailing list > Devel(a)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(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/infra

From: "Sandro Bonazzola" <sbonazzo(a)redhat.com&gt; To: "Eyal Edri" <eedri(a)redhat.com&gt; Cc: infra(a)ovirt.org, devel(a)ovirt.org Sent: Thursday, June 4, 2015 9:50:56 AM Subject: Re: [ovirt-devel] gerrit+ci improvement proposal Il 04/06/2015 08:46, Eyal Edri ha scritto: > > > ----- Original Message ----- >> From: "Sandro Bonazzola" <sbonazzo(a)redhat.com&gt; >> To: "Eyal Edri" <eedri(a)redhat.com&gt;, "Max Kovgan" <mkovgan(a)redhat.com&gt; >> Cc: devel(a)ovirt.org, infra(a)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(a)redhat.com&gt; >>>> To: devel(a)ovirt.org >>>> Cc: infra(a)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? We have ~350 drafts in gerrit so someone is using them :-) https://gerrit.ovirt.org/#/q/is:draft

> i agree if everyone would use drafts load on jenkins was already much > lower, > unfortunately its not the case. > >> 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(a)ovirt.org >>>> http://lists.ovirt.org/mailman/listinfo/devel >>> _______________________________________________ >>> Devel mailing list >>> Devel(a)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(a)ovirt.org >> http://lists.ovirt.org/mailman/listinfo/infra >> >> >> -- Sandro Bonazzola Better technology. Faster innovation. Powered by community collaboration. See how it works at redhat.com _______________________________________________ Infra mailing list Infra(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/infra

From: "Eyal Edri" <eedri(a)redhat.com&gt; To: "Sandro Bonazzola" <sbonazzo(a)redhat.com&gt; Cc: infra(a)ovirt.org, devel(a)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(a)redhat.com&gt; > To: "Eyal Edri" <eedri(a)redhat.com&gt;, "Max Kovgan" <mkovgan(a)redhat.com&gt; > Cc: devel(a)ovirt.org, infra(a)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(a)redhat.com&gt; > >> To: devel(a)ovirt.org > >> Cc: infra(a)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.

> 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(a)ovirt.org > >> http://lists.ovirt.org/mailman/listinfo/devel > > _______________________________________________ > > Devel mailing list > > Devel(a)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(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/infra > > > _______________________________________________ Devel mailing list Devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/devel

=20 =20 ----- Original Message ----- > From: "Oved Ourfali" <ovedo(a)redhat.com&gt; > To: "Eyal Edri" <eedri(a)redhat.com&gt; > Cc: devel(a)ovirt.org, infra(a)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(a)redhat.com&gt; > > To: "Sandro Bonazzola" <sbonazzo(a)redhat.com&gt; > > Cc: infra(a)ovirt.org, devel(a)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(a)redhat.com&gt; > > > To: "Eyal Edri" <eedri(a)redhat.com&gt;, "Max Kovgan" <mkovgan(a)redhat.co=

> > > Cc: devel(a)ovirt.org, infra(a)ovirt.org > > > Sent: Thursday, June 4, 2015 9:11:10 AM > > > Subject: Re: [ovirt-devel] gerrit+ci improvement proposal > > >=20 > > > Il 03/06/2015 21:46, Eyal Edri ha scritto: > > > >=20 > > > >=20 > > > > ----- Original Message ----- > > > >> From: "Max Kovgan" <mkovgan(a)redhat.com&gt; > > > >> To: devel(a)ovirt.org > > > >> Cc: infra(a)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 cy=

> > > >> and > > > >> encourage developers to write tests. > > > >> > > > >> # Problem > > > >> > > > >> Many patches are neither ready for review nor for CI upon submis=

> > > >> 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=E2=80=99t need to, thus making m=

> > > >> 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 we=

> > > >> internally. > > > >> Now it seems a good time to let all the oVirt projects to use th=

> > > >> This solution indirectly promotes reviews and quick tests - =E2=

> > > >> early=E2=80=9D, > > > >> yet full blown static code analysis and long tests to run =E2=80=

> > > >> ready=E2=80=9D. > > > >> > > > >> # 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 =E2=80=9Cworkflow=E2=80=9D state. Values: > > > >> * 0 Work In Progress > > > >> * +1 Ready For Review > > > >> * +2 Ready For Merge > > > >> Permissions for setting: Owner can set +1, Project Maintainers c=

> > > >> +2 > > > >> > > > >> ## Review + CI Integration: > > > >> > > > >> Merging [=E2=80=9CSubmit=E2=80=9D button to appear] will require=

> > > >> 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" f=

> > > >> patch > > > >> to be reviewed, and heavily auto-tested. > > > >> Maintainer now needs to set "Workflow+2" and wait for "Submit" b=

> > > >> to > > > >> appear after CI has completed running gating tests. > > > >> > > > >> > > > >> Next step will be to automate merge the change after Workflow+2 =

> > > >> 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=E2=80=99ve tried detecting what to ru=

> > > >> relying only on gerrit comments so that upon Verified+1, we=E2=

> > > >> the > > > >> job. > > > >> We could not use =E2=80=9CReview+1=E2=80=9D, because it makes =

> > > >> left > > > >> the job to set Verified+1. > > > >> Meaning - re-trigger itself immediately more than 1 times. > > > >> =20 > > > >> 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 furth=

> > > >> 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 th=

> > > >> text > > > >> too. > > > >> > > > >=20 > > > > +1 from me, releasing CI from running non critical and un-essenti=

> > > > jobs > > > > will not only reduce load from ci, > > > > and shorted response time for developers, it will allow us to add=

> > > > more > > > > powerful tests such as functional & system > > > > tests that actually add hosts and run VMs, improving our ability =

> > > > 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, t=

> > > > 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 Workf=

> > > > and > > > > not before. > > >=20 > > > For human reviewers I suggest to keep using drafts until the patch =

> > > finished. > >=20 > > keep using? how many developers do you know are working with drafts u=

> > 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, and less on > bug-fixes. >=20 > In addition, I think the patch owners shouldn't add reviewers, unless t=

> 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) reviewer, then it =

> run the "heavy" CI. =20 This is pointless, we will have to ask someone (or add another user) to +=

patch to enable the CI :-) =20 > 4. Once the patch has +1 from heavy CI, and +2 from reviewer (maintaine=

> then it can be merged. >=20 > That's the process we have today, with slight change on when to run the=

> and what CI to run (no CI on drafts, light CI on non-draft, heavy CI on=

> patches). =20 I think Oved proposal is simpler and more useful. =20 However, we need a way to run *any* ci jobs even on a draft. If we cannot afford this automatically with current system, lets add a way to trigger =

manually from gerrit. =20 So this becomes simply: =20 1. drafts do not run the ci automatically, but the owner can run the ci m=

2. published patches run the light ci jobs automatically, owner can run h=

3. run all ci jobs before merge, maintain can ignore ci results =20 This works with the system David suggested in the past, where each projec=

implement scripts for checking patches and checking patches before merge.

=20 Nir _______________________________________________ Infra mailing list Infra(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/infra

-----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJVcA2CAAoJEEBxx+HSYmnDe/UH/06AN7Idm85qhypzl3pWv7Y6 7aCaY28VSFfnLprieMGYQXLVRDpDULiKCfja3nGVSUFIZYtYn0rEH3WCLa0+tmUB aS2FAFAvbXxZNfwXcMMJULf1X8fp74FCZU49BCWpnv+SAjWXPbytE+HPrpWI8nR+ PqoHut52Ba5NSDfY/SJT2nmBZrQdbSY5cJBwz3BoovkI3gjfKES5/rd1AAlS8Wip xEUOzcGU3HDD9ucjsp/AHMSHn0tP7U6rkWQSFZPdezjCtDU9LNBIF/TC+eTNuHfo hNmH884ZdLnGEsrrIMMOWD/cJWG5bMmvLfX6VOpXIBxQoSjztOg/I/d/SjLRsnQ= =MvB8 -----END PGP SIGNATURE-----

=20 =20 ----- Original Message ----- > From: "David Caro" <dcaroest(a)redhat.com&gt; > To: "Nir Soffer" <nsoffer(a)redhat.com&gt; > Cc: "Oved Ourfali" <ovedo(a)redhat.com&gt;, "Eyal Edri" <eedri(a)redhat.com&gt;, =

> Sent: Thursday, June 4, 2015 11:34:10 AM > Subject: Re: [ovirt-devel] gerrit+ci improvement proposal > On 06/04, Nir Soffer wrote: > > > > ----- Original Message ----- > > > From: "Oved Ourfali" <ovedo(a)redhat.com&gt; > > > To: "Eyal Edri" <eedri(a)redhat.com&gt; > > > Cc: devel(a)ovirt.org, infra(a)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(a)redhat.com&gt; > > > > To: "Sandro Bonazzola" <sbonazzo(a)redhat.com&gt; > > > > Cc: infra(a)ovirt.org, devel(a)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(a)redhat.com&gt; > > > > > To: "Eyal Edri" <eedri(a)redhat.com&gt;, "Max Kovgan" <mkovgan@redha=

> > > > > Cc: devel(a)ovirt.org, infra(a)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(a)redhat.com&gt; > > > > > >> To: devel(a)ovirt.org > > > > > >> Cc: infra(a)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 resou=

> > > > > >> results > > > > > >> in: > > > > > >> overloaded resources, slow response time, unhappy developers. > > > > > >> > > > > > >> # Proposed Solution > > > > > >> > > > > > >> To run less jobs we know we don=E2=80=99t need to, thus maki=

> > > > > >> resources > > > > > >> for > > > > > >> the > > > > > >> jobs we need to run. > > > > > >> We have been experimenting to make our CI stabler and quicke=

> > > > > >> respond > > > > > >> by > > > > > >> using gerrit flags. This has improved in both directions ver=

> > > > > >> internally. > > > > > >> Now it seems a good time to let all the oVirt projects to use > > > > > >> this. > > > > > >> This solution indirectly promotes reviews and quick tests - =

> > > > > >> fail > > > > > >> early=E2=80=9D, > > > > > >> yet full blown static code analysis and long tests to run =

> > > > > >> ready=E2=80=9D. > > > > > >> > > > > > >> # 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 ca=

> > > > > >> should > > > > > >> be > > > > > >> able to set/override (except Jenkins). > > > > > >> > > > > > >> ## Workflow flag > > > > > >> > > > > > >> Will express patch =E2=80=9Cworkflow=E2=80=9D state. Values: > > > > > >> * 0 Work In Progress > > > > > >> * +1 Ready For Review > > > > > >> * +2 Ready For Merge > > > > > >> Permissions for setting: Owner can set +1, Project Maintaine=

> > > > > >> set > > > > > >> +2 > > > > > >> > > > > > >> ## Review + CI Integration: > > > > > >> > > > > > >> Merging [=E2=80=9CSubmit=E2=80=9D button to appear] will req=

> > > > > >> Workflow+2 > > > > > >> Patch lifecycle now is: > > > > > >> ------------------------------------------------------------=

> > > > > >> patch state |owner |reviewer |maintainer |CI tests |pa=

> > > > > >> ------------------------------------------------------------=

> > > > > >> added/updated |- |- |- |quick |CI=

> > > > > >> review |Workflow+1|Review+1 |- |heavy |CI+1 > > > > > >> merge ready |- |- |Workflow+2 |gating |CI=

> > > > > >> merge |- |- |merge |merge |CI=

> > > > > >> > > > > > >> Changes from current workflow: > > > > > >> Owner only adds reviewers, now owner needs to set "Workflow+=

> > > > > >> the > > > > > >> patch > > > > > >> to be reviewed, and heavily auto-tested. > > > > > >> Maintainer now needs to set "Workflow+2" and wait for "Submi=

> > > > > >> button > > > > > >> to > > > > > >> appear after CI has completed running gating tests. > > > > > >> > > > > > >> > > > > > >> Next step will be to automate merge the change after Workflo=

> > > > > >> 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 a=

> > > > > >> unreliable > > > > > >> code into jobs, and will still be prone to problems: > > > > > >> Just recently, 3d ago we=E2=80=99ve tried detecting what t=

> > > > > >> jenkins > > > > > >> relying only on gerrit comments so that upon Verified+1, w=

> > > > > >> run > > > > > >> the > > > > > >> job. > > > > > >> We could not use =E2=80=9CReview+1=E2=80=9D, because it ma=

> > > > > >> so we > > > > > >> left > > > > > >> the job to set Verified+1. > > > > > >> Meaning - re-trigger itself immediately more than 1 times. > > > > > >> =20 > > > > > >> Jenkins and its visitors very unhappy, and we had to stop =

> > > > > >> 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=

> > > > > >> 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 b=

> > > > > >> 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 o=

> > > > > >> text > > > > > >> too. > > > > > >> > > > > > > > > > > > +1 from me, releasing CI from running non critical and un-ess=

> > > > > > jobs > > > > > > will not only reduce load from ci, > > > > > > and shorted response time for developers, it will allow us to=

> > > > > > much > > > > > > more > > > > > > powerful tests such as functional & system > > > > > > tests that actually add hosts and run VMs, improving our abil=

> > > > > > find > > > > > > regression much more effectively. > > > > > > > > > > > Another benefit to consider is saving reviewers time. I.e not=

> > > > > > jenkins > > > > > > benefits from Worklow+1, but also human reviewers. > > > > > > Instead of looking at a patch that is too early to be reviewe=

> > > > > > 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 pa=

> > > > > finished. > > > > > > > keep using? how many developers do you know are working with draf=

> > > > until > > > > their patch is ready? > > > > i agree if everyone would use drafts load on jenkins was already =

> > > > 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, unle=

> > > 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=

> > > should > > > run the "heavy" CI. > > > This is pointless, we will have to ask someone (or add another user) =

> > the > > patch to enable the 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=

> > > CI > > > and what CI to run (no CI on drafts, light CI on non-draft, heavy C=

> > > +1 > > > patches). > > > I think Oved proposal is simpler and more useful. > > > However, we need a way to run *any* ci jobs even on a draft. If we ca=

> > afford this automatically with current system, lets add a way to trig=

> > this > > manually from gerrit. > > > So this becomes simply: > > > 1. drafts do not run the ci automatically, but the owner can run the =

> > manually > There are two ways I can think of such a trigger, through flags (that a=

> easier to manage) or comments. The flags approach is the one that we ex=

> and tested on a few projects. > > 2. published patches run the light ci jobs automatically, owner can r=

> > heavy jobs manually > A big issue here is that there's currently no separation between light =

> heavy jobs, not even a list of what should be considered as such. So for > starters any job is considered to be heavy. > > 3. run all ci jobs before merge, maintain can ignore ci results > > > This works with the system David suggested in the past, where each pr=

> > implement scripts for checking patches and checking patches before me=

> Both work with that system that was never implemented on any major proj=

> far due to reluctance to add ci scripts inside the code base (where I > strongly > think they belong, as the code base evolves, the scripts must change wi=

> So this is not yet implemented everywhere. =20 Did you send patches for vdsm for this?

=20 In vdsm, we can use the @slowtest to mark tests that must run only before=

Currently the slow tests are always disabled in the ci, it would be nice =

them before merge anyway. =20 Another solution, add orthogonal decorator (@ci("configname")) to include=

test in some ci configuration. =20 The ci can run the tests with NOSE_CI=3Dfull or similar flag to select te=

=20

Nir

-----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJVcBX4AAoJEEBxx+HSYmnDWbMH/RmgI4PNl67xhPXFnm69lo7p qZtWzfhzcOi43HR0tePRpTZvFIl06UiuWnQOPpcfzmApfpAFAngtQcFZgdukG3pK ekSVJQgWBjw/wEszJ6qqrzQUKIXig3V9jVjYwYedoFcIKBiQCKeRTJz7mytyH3RB Qs6EAS1S54VOiLiJi0jnj9DpnBXhrIBpRFTV6QwBYesqNtPpuzp4SX4JNaWlfqKf ECKBMASrod+w8uZeEkce+qce+S/I6FsU0o0goM7JhmqhwJ+Lk8KUWm4P+yv1fibb TGyYkH48c1vieQZda1+S4ZLDjwoQrcdKKUW4bS+LAVWFHA7eXByJUQhQytj9HSU= =7E0W -----END PGP SIGNATURE-----

From: "Eli Mesika" <emesika(a)redhat.com&gt; To: "Oved Ourfali" <ovedo(a)redhat.com&gt; Cc: "Eyal Edri" <eedri(a)redhat.com&gt;, infra(a)ovirt.org, devel(a)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(a)redhat.com&gt; > To: "Eyal Edri" <eedri(a)redhat.com&gt; > Cc: devel(a)ovirt.org, infra(a)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(a)redhat.com&gt; > > To: "Sandro Bonazzola" <sbonazzo(a)redhat.com&gt; > > Cc: infra(a)ovirt.org, devel(a)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(a)redhat.com&gt; > > > To: "Eyal Edri" <eedri(a)redhat.com&gt;, "Max Kovgan" <mkovgan(a)redhat.com&gt; > > > Cc: devel(a)ovirt.org, infra(a)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(a)redhat.com&gt; > > > >> To: devel(a)ovirt.org > > > >> Cc: infra(a)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

> > > > > > 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(a)ovirt.org > > > >> http://lists.ovirt.org/mailman/listinfo/devel > > > > _______________________________________________ > > > > Devel mailing list > > > > Devel(a)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(a)ovirt.org > > > http://lists.ovirt.org/mailman/listinfo/infra > > > > > > > > > > > _______________________________________________ > > Devel mailing list > > Devel(a)ovirt.org > > http://lists.ovirt.org/mailman/listinfo/devel > > > > > > > _______________________________________________ > Infra mailing list > Infra(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/infra > _______________________________________________ Infra mailing list Infra(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/infra

From: "Oved Ourfali" <ovedo(a)redhat.com&gt; To: "Eyal Edri" <eedri(a)redhat.com&gt; Cc: infra(a)ovirt.org, devel(a)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(a)redhat.com&gt; > To: "Eli Mesika" <emesika(a)redhat.com&gt; > Cc: "Oved Ourfali" <ovedo(a)redhat.com&gt;, devel(a)ovirt.org, infra(a)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(a)redhat.com&gt; > > To: "Oved Ourfali" <ovedo(a)redhat.com&gt; > > Cc: "Eyal Edri" <eedri(a)redhat.com&gt;, infra(a)ovirt.org, devel(a)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(a)redhat.com&gt; > > > To: "Eyal Edri" <eedri(a)redhat.com&gt; > > > Cc: devel(a)ovirt.org, infra(a)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(a)redhat.com&gt; > > > > To: "Sandro Bonazzola" <sbonazzo(a)redhat.com&gt; > > > > Cc: infra(a)ovirt.org, devel(a)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(a)redhat.com&gt; > > > > > To: "Eyal Edri" <eedri(a)redhat.com&gt;, "Max Kovgan" > > > > > <mkovgan(a)redhat.com&gt; > > > > > Cc: devel(a)ovirt.org, infra(a)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(a)redhat.com&gt; > > > > > >> To: devel(a)ovirt.org > > > > > >> Cc: infra(a)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.

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(a)ovirt.org > > > > > >> http://lists.ovirt.org/mailman/listinfo/devel > > > > > > _______________________________________________ > > > > > > Devel mailing list > > > > > > Devel(a)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(a)ovirt.org > > > > > http://lists.ovirt.org/mailman/listinfo/infra > > > > > > > > > > > > > > > > > > > _______________________________________________ > > > > Devel mailing list > > > > Devel(a)ovirt.org > > > > http://lists.ovirt.org/mailman/listinfo/devel > > > > > > > > > > > > > > > _______________________________________________ > > > Infra mailing list > > > Infra(a)ovirt.org > > > http://lists.ovirt.org/mailman/listinfo/infra > > > > > _______________________________________________ > > Infra mailing list > > Infra(a)ovirt.org > > http://lists.ovirt.org/mailman/listinfo/infra > > > > > > > _______________________________________________ > Infra mailing list > Infra(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/infra > _______________________________________________ Infra mailing list Infra(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/infra

From: "Max Kovgan" <mkovgan(a)redhat.com&gt; To: devel(a)ovirt.org Cc: infra(a)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.

From: "David Caro" <dcaroest(a)redhat.com&gt; To: "Nir Soffer" <nsoffer(a)redhat.com&gt; Cc: "Max Kovgan" <mkovgan(a)redhat.com&gt;, infra(a)ovirt.org, devel(a)ovirt.org Sent: Thursday, June 4, 2015 10:35:12 AM Subject: Re: [ovirt-devel] gerrit+ci improvement proposal On 06/04, Nir Soffer wrote: > ----- Original Message ----- > > From: "Max Kovgan" <mkovgan(a)redhat.com&gt; > > To: devel(a)ovirt.org > > Cc: infra(a)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 > > -1 > > You must have CI error state. Most of the errors I have seen, the CI fail > to run, > and the failure is not related to the tested patch. You'll have to prove this, as the last time we discussed it less than 10% of the failures from the previous 2 weeks were that case.

Also that's why maintainers can set +1 here and -1 is not a blocker.

> > 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 > > -1 > > Cannot require CI+1, the CI is not reliable enough yet. > > Even if the CI will be reliable, a failed test which is not related to the > submitted > patch should not block unrelated changes. This situation makes no sense, if it's reliable theres no error due to non changed stuff (unless the issue that triggers the error is part of your patch history, because it's merged or you are based on a broken patch, in this case it should fail and block)

> > 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. > > -1 > > This means more work for developers. > > Instead, make workflow default to +1. If the want to disable the CI, she > can > set it to 0. > > Developers need more power and less process overhead. We have to make some stats, but from the small sample of patches that we checked, most than half of them were not ready to pass any ci,

it's a bit more work, if you keep using drafts, if you don't it's the same. It's not ment to be used alongside drafts but instead of it. So the only case where it increases the overhead is if you did not use drafts, in which case you should start using them, so no real downside there.

From: "Nir Soffer" <nsoffer(a)redhat.com&gt; To: "Max Kovgan" <mkovgan(a)redhat.com&gt; Cc: infra(a)ovirt.org, devel(a)ovirt.org Sent: Thursday, June 4, 2015 10:26:22 AM Subject: Re: [ovirt-devel] gerrit+ci improvement proposal ----- Original Message ----- > From: "Max Kovgan" <mkovgan(a)redhat.com&gt; > To: devel(a)ovirt.org > Cc: infra(a)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 -1 You must have CI error state. Most of the errors I have seen, the CI fail to run, and the failure is not related to the tested patch.

> 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 -1 Cannot require CI+1, the CI is not reliable enough yet. Even if the CI will be reliable, a failed test which is not related to the submitted patch should not block unrelated changes.

> 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. -1 This means more work for developers. Instead, make workflow default to +1. If the want to disable the CI, she can set it to 0.

Developers need more power and less process overhead. Nir _______________________________________________ Infra mailing list Infra(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/infra