Is Gerrit 'merge if necessary' strategy inherently broken?

[lucamilanesio]

Hi, all Gerrit community & developers :-)

This week there is the Gerrit Hackathon in Sweden and the Team is very productive ... possibly too productive?

Bottom line is => we managed to "break" the stable master workflow with two changes that were verified in parallel, were not "syntactically in conflict" but, yes, semantically they were incompatible.

The real-life example is:

https://gerrit-review.googlesource.com/c/gerrit/+/172990

What happened

We started with a stable master branch: all good, all working.

Dave created  Change Id4ad7bcdafa2 that removed the import of ProjectConfig *and* its use in the AccountIT class.

So far, so good => build green. Ready to be merged.

Han-Wen created Change I3a15fce564381 that leveraged the ProjectConfig class in AccountIT, adding some extra code but NOT touching the import of ProjectConfig.

All good, because Dave's change hasn't been merged yet.  => build green. Ready to be merged.

Then ... at 4 PM ... boom. Dave pushed the "Submit" button, Han-Wen pushed the "Submit" button ... and the master became *RED*.

Why did that happen?

Gerrit uses "Merge if Necessary" strategy in Gerrit, which allows changes to be merged even if they aren't rebased.

Dave and Han-Wen had no way to detect that their changes were semantically in a conflict between each other and then assumed that they were ready to go without having to sync with them.

Our Gerrit-CI validation already validates that the *result* of the merge with the target branch works. However, the validation should remember the "target SHA1" that was tested and, if that changed, the Verified flag should be removed. We don't do that, because the submit button doesn't call the CI build before submitting the change.

How to fix the problem?

The safest approach IMHO would just change the Gerrit submit type to "Always rebase". The above problem would not have happened.

What do you think?

Did you have the same problem? How did you tackle it?

Luca.

 

[Martin Fick]

On Wednesday, April 18, 2018 02:40:19 PM lucamilanesio
wrote:

> Hi, all Gerrit community & developers :-)
>
> This week there is the Gerrit Hackathon in Sweden and the
> Team is very productive ... possibly too productive?
> Bottom line is => we managed to "break" the stable master
> workflow with two changes that were verified in parallel,
> were not "syntactically in conflict" but, yes,
> semantically they were incompatible.

...

> *How to fix the problem?*

>
> The safest approach IMHO would just change the Gerrit
> submit type to "Always rebase". The above problem would
> not have happened.

Would "always rebase" have created a new patchset and
removed the verified bit, preventing submit and causing CI to
run again? I thought that it would rebase and then submit
without waiting for a new verification job?

-Martin

--
The Qualcomm Innovation Center, Inc. is a member of Code
Aurora Forum, hosted by The Linux Foundation

[Matthias Sohn]

to my experience this happens pretty rarely. I guess in order to fix that auto-merge / auto-rebase on the server

needs to reset at least the verified label and trigger another build and wait for another validation. If that's successful

I guess the change could be auto-submitted. This should fix the problem but may lead to races with other changes

in flight around the same time for the same project.

-Matthias 

[Gert van Dijk]

Using "Rebase Always" will have Gerrit rebase the change on-submit time, yet allows you to have out-of-date patch sets to be uploaded (and verified). Thus, "Rebase Always" will not solve the problem.

However, the "Fast foward only" prevents it, as the parent SHA1 must be the same as the target branch on submit time. That means a patch set out of change with the target branch even is never submittable. On busy target branches this means you will be hitting the rebase button very frequently and also triggering much more CI verification runs.

I believe this can't really be fixed, because you can't ask Gerrit+CI to verify all open changes in all combinations of each other as you don't know in what order they will be submitted. A "work around" could be an additional periodic run of all open changes in a "simulated submit" state and vote -1. Apart from that it won't cover quick submissions after each other, it is also wasting a lot of resources.

In my situation I have regarded it as a residual risk, as in practise most cases will be caught already with a Merge Conflict state.

[Luca Milanesio]

> On 18 Apr 2018, at 22:52, Martin Fick <mf...@codeaurora.org> wrote:
>
> On Wednesday, April 18, 2018 02:40:19 PM lucamilanesio
> wrote:
>> Hi, all Gerrit community & developers :-)
>>
>> This week there is the Gerrit Hackathon in Sweden and the
>> Team is very productive ... possibly too productive?
>> Bottom line is => we managed to "break" the stable master
>> workflow with two changes that were verified in parallel,
>> were not "syntactically in conflict" but, yes,
>> semantically they were incompatible.
> ...
>
>> *How to fix the problem?*
>>
>> The safest approach IMHO would just change the Gerrit
>> submit type to "Always rebase". The above problem would
>> not have happened.
>
> Would "always rebase" have created a new patchset and
> removed the verified bit, preventing submit and causing CI to
> run again?

The documentation says:
"Rebase Always
Basically, the same as Rebase If Necessary, but it creates a new patchset even if fast forward is possible"

New PS means, if the project.config is set to do so, reset the verified flag and thus re-trigger a new CI build.

> I thought that it would rebase and then submit
> without waiting for a new verification job?

It depends on how it is configured: let me do a simple test tomorrow morning on that :-)

[Luca Milanesio]

On 18 Apr 2018, at 22:57, Matthias Sohn <matthi...@gmail.com> wrote:

On Wed, Apr 18, 2018 at 11:40 PM, lucamilanesio <luca.mi...@gmail.com> wrote:

Hi, all Gerrit community & developers :-)

This week there is the Gerrit Hackathon in Sweden and the Team is very productive ... possibly too productive?

Bottom line is => we managed to "break" the stable master workflow with two changes that were verified in parallel, were not "syntactically in conflict" but, yes, semantically they were incompatible.

The real-life example is:

https://gerrit-review.googlesource.com/c/gerrit/+/172990

What happened

We started with a stable master branch: all good, all working.

Dave created  Change Id4ad7bcdafa2 that removed the import of ProjectConfig *and* its use in the AccountIT class.

So far, so good => build green. Ready to be merged.

Han-Wen created Change I3a15fce564381 that leveraged the ProjectConfig class in AccountIT, adding some extra code but NOT touching the import of ProjectConfig.

All good, because Dave's change hasn't been merged yet.  => build green. Ready to be merged.

Then ... at 4 PM ... boom. Dave pushed the "Submit" button, Han-Wen pushed the "Submit" button ... and the master became *RED*.

Why did that happen?

Gerrit uses "Merge if Necessary" strategy in Gerrit, which allows changes to be merged even if they aren't rebased.

Dave and Han-Wen had no way to detect that their changes were semantically in a conflict between each other and then assumed that they were ready to go without having to sync with them.

Our Gerrit-CI validation already validates that the *result* of the merge with the target branch works. However, the validation should remember the "target SHA1" that was tested and, if that changed, the Verified flag should be removed. We don't do that, because the submit button doesn't call the CI build before submitting the change.

How to fix the problem?

The safest approach IMHO would just change the Gerrit submit type to "Always rebase". The above problem would not have happened.

What do you think?

Did you have the same problem? How did you tackle it?

to my experience this happens pretty rarely.

True, this is the only the 2nd time that this happened on the Gerrit project.

I guess in order to fix that auto-merge / auto-rebase on the server

needs to reset at least the verified label and trigger another build and wait for another validation.

Yes, that would be the way to fix it.

The "autosubmitter plugin" actually does something similar *BUT* does not rebase. We should add  the "rebase" option so that the plugin could do the rebase and submit when the CI is successful.

That would be neat :-)

If that's successful

I guess the change could be auto-submitted. This should fix the problem but may lead to races with other changes

in flight around the same time for the same project.

Yes, but *IF* something else gets merged, just rebase again and re-validate :-)

-Matthias 

[Gert van Dijk]

On Thursday, 19 April 2018 00:17:27 UTC+2, lucamilanesio wrote:

The documentation says:
"Rebase Always
Basically, the same as Rebase If Necessary, but it creates a new patchset even if fast forward is possible"

New PS means, if the project.config is set to do so, reset the verified flag and thus re-trigger a new CI build.

No, it won't. Have you tried it or am I missing something? It will merge *that* new PS on-submit. Ie. submitting PS2 in an open change will create a PS3 in a merged-state change.

Ideally, it could wait for CI to finish with +1 to actually merge, but what would you do if another change get merged while CI is running?

[Martin Fick]

On Wednesday, April 18, 2018 03:13:09 PM Gert van Dijk
wrote:

> Using "Rebase Always" will have Gerrit rebase the change
> on-submit time, yet allows you to have out-of-date patch
> sets to be uploaded (and verified). Thus, "Rebase Always"
> will not solve the problem. However, the "Fast foward
> only" prevents it, as the parent SHA1 must be the same as
> the target branch on submit time. That means a patch set
> out of change with the target branch even is never
> submittable. On busy target branches this means you will
> be hitting the rebase button very frequently and also
> triggering much more CI verification runs.
>
> I believe this can't really be fixed, because you can't
> ask Gerrit+CI to verify all open changes in all
> combinations of each other as you don't know in what
> order they will be submitted. A "work around" could be an
> additional periodic run of all open changes in a
> "simulated submit" state and vote -1. Apart from that it
> won't cover quick submissions after each other, it is
> also wasting a lot of resources.

This is what the "batch" plugin is designed to help you do
efficiently. It allows you to merge changes server side for
serialized verification, and then "submitting" the exact sha1
you test. We use it internally for a lot of work. See the
plugin here:

https://gerrit-review.googlesource.com/admin/repos/plugins/batch


We have been working on porting the batch plugin to the
latest Gerrit master, specifically so that it will run on
google's servers. See the series here:

https://gerrit-review.googlesource.com/q/project:plugins%252Fbatch+branch:master+status:open

It is not ready yet, but we could consider adding a submit
CI that use the plugin once the plugin is running on google
infrastructure,

[Luca Milanesio]

On 18 Apr 2018, at 23:13, Gert van Dijk <gert...@gmail.com> wrote:

Using "Rebase Always" will have Gerrit rebase the change on-submit time, yet allows you to have out-of-date patch sets to be uploaded (and verified). Thus, "Rebase Always" will not solve the problem.

Yes, but in theory only if you have copyAllScoresOnTrivialRebase (defaulted to false). In practice, I believe you are right. The CI will be triggered anyway because you have a new PS pushed *BUT* the validation would be useless because it could provide a -1 when the change has been merged already :-(

@Martin: yes, you are right. However I'd like to make some more tests tomorrow ...

However, the "Fast foward only" prevents it, as the parent SHA1 must be the same as the target branch on submit time. That means a patch set out of change with the target branch even is never submittable. On busy target branches this means you will be hitting the rebase button very frequently and also triggering much more CI verification runs.

Yes, that's the price to pay for having more security on the sanity check of the master branch.

Nowadays with Google Cloud and AWS, the scalability of the build slaves isn't an issue. It is just a matter of costs associated.

If you need more security of green builds, that would cost more :-)

I believe this can't really be fixed, because you can't ask Gerrit+CI to verify all open changes in all combinations of each other as you don't know in what order they will be submitted.

No, BUT you can automate the submission, like you do with the "autosubmitter" Gerrit plugin.

I heard other people as well that disabled the "Submit button" in Gerrit and allowed only automatic submissions. (@Martin you guys do that, if I remember correctly)

A "work around" could be an additional periodic run of all open changes in a "simulated submit" state and vote -1. Apart from that it won't cover quick submissions after each other, it is also wasting a lot of resources.

Yes, that would just waste resources without a lot of benefits.

In my situation I have regarded it as a residual risk, as in practise most cases will be caught already with a Merge Conflict state.

Yes, it is, unless the conflict is semantic and not syntactic, like in our case today :-)

P.S. Happened twice in 10 years of the Gerrit project ... I may say that isn't a common scenario after all !

On Wednesday, 18 April 2018 23:40:20 UTC+2, lucamilanesio wrote:

Hi, all Gerrit community & developers :-)

This week there is the Gerrit Hackathon in Sweden and the Team is very productive ... possibly too productive?

Bottom line is => we managed to "break" the stable master workflow with two changes that were verified in parallel, were not "syntactically in conflict" but, yes, semantically they were incompatible.

The real-life example is:

https://gerrit-review.googlesource.com/c/gerrit/+/172990

What happened

We started with a stable master branch: all good, all working.

Dave created  Change Id4ad7bcdafa2 that removed the import of ProjectConfig *and* its use in the AccountIT class.

So far, so good => build green. Ready to be merged.

Han-Wen created Change I3a15fce564381 that leveraged the ProjectConfig class in AccountIT, adding some extra code but NOT touching the import of ProjectConfig.

All good, because Dave's change hasn't been merged yet.  => build green. Ready to be merged.

Then ... at 4 PM ... boom. Dave pushed the "Submit" button, Han-Wen pushed the "Submit" button ... and the master became *RED*.

Why did that happen?

Gerrit uses "Merge if Necessary" strategy in Gerrit, which allows changes to be merged even if they aren't rebased.

Dave and Han-Wen had no way to detect that their changes were semantically in a conflict between each other and then assumed that they were ready to go without having to sync with them.

Our Gerrit-CI validation already validates that the *result* of the merge with the target branch works. However, the validation should remember the "target SHA1" that was tested and, if that changed, the Verified flag should be removed. We don't do that, because the submit button doesn't call the CI build before submitting the change.

How to fix the problem?

The safest approach IMHO would just change the Gerrit submit type to "Always rebase". The above problem would not have happened.

What do you think?

Did you have the same problem? How did you tackle it?

Luca.

 

--
--
To unsubscribe, email repo-discuss...@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Gert van Dijk]

On Thursday, 19 April 2018 00:27:57 UTC+2, MartinFick wrote:

This is what the "batch" plugin is designed to help you do
efficiently.  It allows you to merge changes server side for
serialized verification, and then "submitting" the exact sha1
you test.  

Brilliant. Will definitely look at that when I find time. Thanks!

[Luca Milanesio]

That would be great !

We have been discussing in the Gerrit project about changing the way we submit changes.
Sometimes there are changes that stay "ready to be submitted" for too long ... and then when you submit them they don't work anymore :-(

The best option is definitely: automation.
If everyone says "the change is ready to go" and "merges and works fine with target" => why bothering any longer? Let's just automate its submission :-)

>
> -Martin
>
> --
> The Qualcomm Innovation Center, Inc. is a member of Code
> Aurora Forum, hosted by The Linux Foundation
>

[Martin Fick]

On Wednesday, April 18, 2018 11:33:52 PM Luca Milanesio
wrote:

> > On 18 Apr 2018, at 23:27, Martin Fick
> > <mf...@codeaurora.org> wrote:
> > On Wednesday, April 18, 2018 03:13:09 PM Gert van Dijk
> > wrote:
> >
> > This is what the "batch" plugin is designed to help you
> > do efficiently.

...

> > It is not ready yet, but we could consider adding a
> > submit CI that use the plugin once the plugin is
> > running on google infrastructure,
>
> That would be great !
>
> We have been discussing in the Gerrit project about
> changing the way we submit changes. Sometimes there are
> changes that stay "ready to be submitted" for too long
> ... and then when you submit them they don't work anymore
> :-(
>
> The best option is definitely: automation.
> If everyone says "the change is ready to go" and "merges
> and works fine with target" => why bothering any longer?
> Let's just automate its submission :-)

And if you still want a human to make the submit decision,
you can just add another label called "submit via ci" or
something like that which triggers the build and submits at
the end if it passes,

[Gert van Dijk]

On Thursday, 19 April 2018 00:31:10 UTC+2, lucamilanesio wrote:

Using "Rebase Always" will have Gerrit rebase the change on-submit time, yet allows you to have out-of-date patch sets to be uploaded (and verified). Thus, "Rebase Always" will not solve the problem.

Yes, but in theory only if you have copyAllScoresOnTrivialRebase (defaulted to false). In practice, I believe you are right. The CI will be triggered anyway because you have a new PS pushed

No, it won't fire a patchet-created event for me, but only one of '"type":"change-merged"'.

 

*BUT* the validation would be useless because it could provide a -1 when the change has been merged already :-(

And yes, even if you did get a patchset-created event as well it won't help you like you explained.

[Luca Milanesio]

On 18 Apr 2018, at 23:38, Gert van Dijk <gert...@gmail.com> wrote:

On Thursday, 19 April 2018 00:31:10 UTC+2, lucamilanesio wrote:

Using "Rebase Always" will have Gerrit rebase the change on-submit time, yet allows you to have out-of-date patch sets to be uploaded (and verified). Thus, "Rebase Always" will not solve the problem.

Yes, but in theory only if you have copyAllScoresOnTrivialRebase (defaulted to false). In practice, I believe you are right. The CI will be triggered anyway because you have a new PS pushed

No, it won't fire a patchet-created event for me, but only one of '"type":"change-merged"'.

That's a bug IMHO: if Gerrit creates a new PS and then merge it, it should generate:

- patchset-created

- change-merged

 

*BUT* the validation would be useless because it could provide a -1 when the change has been merged already :-(

And yes, even if you did get a patchset-created event as well it won't help you like you explained.

Not really, it would help to understand what's going on.

Even if the CI feedback won't block the submission, it will be recorded on the change to say "Hey mate, that change broke the build !"

It took a while for us to understand what was going on in Gerrit: I thought that our CI validation scripts were wrong .... a bit of panic :-(

[Gert van Dijk]

(On a side note in this discussion, I just wanted to point out my top annoyance when using "Rebase Always"; this unfriendly behaviour of Gerrit: https://bugs.chromium.org/p/gerrit/issues/detail?id=6878#c10. I can't believe only two people starred that issue; is Rebase Always that impopular?)

[Andrew Grimberg]

From my point of view Rebase always is a bad option. We strongly
encourage our developers to PGP sign their changes (not that they all
do) but if Gerrit goes and rebases their change that data is completely
lost since Gerrit doesn't have their private key to resign it with, nor
should it resign it!

-Andy-

> --
> --
> To unsubscribe, email repo-discuss...@googlegroups.com
> More info at http://groups.google.com/group/repo-discuss?hl=en
>
> ---
> You received this message because you are subscribed to the Google
> Groups "Repo and Gerrit Discussion" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to repo-discuss...@googlegroups.com

> <mailto:repo-discuss...@googlegroups.com>.

[Duft Markus]

Hey,

 

Experience from a large’ish project (~100 full time devs, 5M LOC, ~200 changes per day):

 

1)      Such semantic conflicts do occur. For us it was I think twice in ~6 years. Typically it is detected quite quickly (no developer workspace will compile anymore, tests will fail, etc. ;)) and fixed within minutes to maybe worst case one or two hours.

2)      On the other hand the rebase strategies have a major drawback for us. Developers tend to fetch and rebase a lot. With merge if necessary this will fast forward in case the change is already merged. With rebase strategies, the same commit will be (tried to be) applied again and developers are confronted with conflicts. I know, we could just change the way devs work, but it’s not easy J

3)      CI Builds are taking quite some time for us. With database changes, a single CI can take up to half an hour. This is a lot of time. Thinking of the ~200 changes per day on that particular project we just cannot affort spending more CI time than necessary, and will rather take the risk is hitting this issue from time to time instead.

 

Cheers,

Markus

--

--
To unsubscribe, email repo-discuss...@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.

To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss...@googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

SSI Schäfer IT Solutions GmbH | Friesachstrasse 15 | 8114 Friesach | Austria
Registered Office: Friesach | Commercial Register: 49324 K | VAT no. ATU28654300
Commercial Court: Landesgericht für Zivilrechtssachen Graz

[Luca Milanesio]

On 19 Apr 2018, at 06:48, Duft Markus <Marku...@ssi-schaefer.com> wrote:

Hey,

 

Experience from a large’ish project (~100 full time devs, 5M LOC, ~200 changes per day):

 

1)      Such semantic conflicts do occur. For us it was I think twice in ~6 years. Typically it is detected quite quickly (no developer workspace will compile anymore, tests will fail, etc. ;)) and fixed within minutes to maybe worst case one or two hours.

True, we found it quite quickly: basically everything became RED :-)

2)      On the other hand the rebase strategies have a major drawback for us. Developers tend to fetch and rebase a lot. With merge if necessary this will fast forward in case the change is already merged. With rebase strategies, the same commit will be (tried to be) applied again and developers are confronted with conflicts. I know, we could just change the way devs work, but it’s not easy J

If rebase is done automatically, then no, you don't have those drawbacks.

3)      CI Builds are taking quite some time for us. With database changes, a single CI can take up to half an hour. This is a lot of time. Thinking of the ~200 changes per day on that particular project we just cannot affort spending more CI time than necessary, and will rather take the risk is hitting this issue from time to time instead.

True, in your case the build time makes the option not worth the price.

[Åsmund Østvold]

hi,

I worked on a project that had many concurrently moving components ASIC (software module simulator, hardware simulator and actual hardware), firmware, driver, userspace library, tools, tests, and CI system. As you can guess this had a lot of dependencies but also logically belong to different repositories.  The way we solved this was:

1. All changes was first tested in parallel against current master on push
   This tests did not run full test battery because of hw resources 
2. if passing #1 trigger a non-parallel Jenkins job running the full battery of tests. At the end this job would attempt to submit the change. 
• Some repositories did require human review some did not.  
  
• This job could be triggered if the human review had not happened before the job completed.

The project was made simpler by stating we did only handle single commits. Patch series was not supported. 

We started with merging after step 1. With that we did have failures every other week and it took between 1 and 2 days to figure out what was the issue and solve it. 

With topics with multiple commits/patch series more logic would be needed because only full patch-sets/topics should be merged.

Cheers,

Åsmund

--
--
To unsubscribe, email repo-discuss+unsubscribe@googlegroups.com

More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.

To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss+unsubscribe@googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

SSI Schäfer IT Solutions GmbH | Friesachstrasse 15 | 8114 Friesach | Austria
Registered Office: Friesach | Commercial Register: 49324 K | VAT no. ATU28654300
Commercial Court: Landesgericht für Zivilrechtssachen Graz

--
--
To unsubscribe, email repo-discuss+unsubscribe@googlegroups.com

More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.

To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss+unsubscribe@googlegroups.com.

[Duft Markus]

From: Luca Milanesio [mailto:luca.mi...@gmail.com]
Sent: Thursday, April 19, 2018 8:57 AM
To: Duft Markus <Marku...@ssi-schaefer.com>
Cc: Luca Milanesio <Luca.Mi...@gmail.com>; Repo and Gerrit Discussion <repo-d...@googlegroups.com>
Subject: Re: Is Gerrit 'merge if necessary' strategy inherently broken?

 

 

On 19 Apr 2018, at 06:48, Duft Markus <Marku...@ssi-schaefer.com> wrote:

 

Hey,

 

Experience from a large’ish project (~100 full time devs, 5M LOC, ~200 changes per day):

 

1)      Such semantic conflicts do occur. For us it was I think twice in ~6 years. Typically it is detected quite quickly (no developer workspace will compile anymore, tests will fail, etc. ;)) and fixed within minutes to maybe worst case one or two hours.

 

True, we found it quite quickly: basically everything became RED :-)

2)      On the other hand the rebase strategies have a major drawback for us. Developers tend to fetch and rebase a lot. With merge if necessary this will fast forward in case the change is already merged. With rebase strategies, the same commit will be (tried to be) applied again and developers are confronted with conflicts. I know, we could just change the way devs work, but it’s not easy J

 

If rebase is done automatically, then no, you don't have those drawbacks.

 

Oh, we do! It does not matter how the rebase happens. If it is not FF, the local branch at the committers workplace will have a different commit ID, thus the rebase will try to re-apply the very same commit on top of whatever is fetched from the repository J

 

We even tried out the option for a few weeks, and we switched back exactly for this reason (mostly J).

[Åsmund Østvold]

On Thu, Apr 19, 2018 at 12:42 AM, Luca Milanesio <luca.mi...@gmail.com> wrote:

On 18 Apr 2018, at 23:38, Gert van Dijk <gert...@gmail.com> wrote:

On Thursday, 19 April 2018 00:31:10 UTC+2, lucamilanesio wrote:

Using "Rebase Always" will have Gerrit rebase the change on-submit time, yet allows you to have out-of-date patch sets to be uploaded (and verified). Thus, "Rebase Always" will not solve the problem.

Yes, but in theory only if you have copyAllScoresOnTrivialRebase (defaulted to false). In practice, I believe you are right. The CI will be triggered anyway because you have a new PS pushed

No, it won't fire a patchet-created event for me, but only one of '"type":"change-merged"'.

That's a bug IMHO: if Gerrit creates a new PS and then merge it, it should generate:

- patchset-created

- change-merged

If this is introduced our CI process would need adjustments. On patchset-created event we trigger regression checkin and if the change is already merged it will fail and spam developers with messages. On change-merged we prepare binaries for test runs outside of checkin regression. 

 

 

*BUT* the validation would be useless because it could provide a -1 when the change has been merged already :-(

And yes, even if you did get a patchset-created event as well it won't help you like you explained.

Not really, it would help to understand what's going on.

Even if the CI feedback won't block the submission, it will be recorded on the change to say "Hey mate, that change broke the build !"

It took a while for us to understand what was going on in Gerrit: I thought that our CI validation scripts were wrong .... a bit of panic :-(

--
--
To unsubscribe, email repo-discuss+unsubscribe@googlegroups.com

More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.

To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss+unsubscribe@googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--
--
To unsubscribe, email repo-discuss+unsubscribe@googlegroups.com

More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.

To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss+unsubscribe@googlegroups.com.

[Luca Milanesio]

On 19 Apr 2018, at 08:28, Åsmund Østvold <asm...@gmail.com> wrote:

On Thu, Apr 19, 2018 at 12:42 AM, Luca Milanesio <luca.mi...@gmail.com> wrote:

On 18 Apr 2018, at 23:38, Gert van Dijk <gert...@gmail.com> wrote:

On Thursday, 19 April 2018 00:31:10 UTC+2, lucamilanesio wrote:

Using "Rebase Always" will have Gerrit rebase the change on-submit time, yet allows you to have out-of-date patch sets to be uploaded (and verified). Thus, "Rebase Always" will not solve the problem.

Yes, but in theory only if you have copyAllScoresOnTrivialRebase (defaulted to false). In practice, I believe you are right. The CI will be triggered anyway because you have a new PS pushed

No, it won't fire a patchet-created event for me, but only one of '"type":"change-merged"'.

That's a bug IMHO: if Gerrit creates a new PS and then merge it, it should generate:

- patchset-created

- change-merged

If this is introduced our CI process would need adjustments. On patchset-created event we trigger regression checkin and if the change is already merged it will fail and spam developers with messages. On change-merged we prepare binaries for test runs outside of checkin regression. 

*IF* we introduce an extra patchset-created, then it should respect the label settings for copying the score over.

*IF* there is no copy max score policy, then it cannot merge automatically.

That's my idea, however, we are all here to discuss if that would be useful OR not. :-)

Luca.

[Åsmund Østvold]

On Thu, Apr 19, 2018 at 9:32 AM, Luca Milanesio <luca.mi...@gmail.com> wrote:

On 19 Apr 2018, at 08:28, Åsmund Østvold <asm...@gmail.com> wrote:

On Thu, Apr 19, 2018 at 12:42 AM, Luca Milanesio <luca.milanesio@gmail.com> wrote:

On 18 Apr 2018, at 23:38, Gert van Dijk <gert...@gmail.com> wrote:

On Thursday, 19 April 2018 00:31:10 UTC+2, lucamilanesio wrote:

Using "Rebase Always" will have Gerrit rebase the change on-submit time, yet allows you to have out-of-date patch sets to be uploaded (and verified). Thus, "Rebase Always" will not solve the problem.

Yes, but in theory only if you have copyAllScoresOnTrivialRebase (defaulted to false). In practice, I believe you are right. The CI will be triggered anyway because you have a new PS pushed

No, it won't fire a patchet-created event for me, but only one of '"type":"change-merged"'.

That's a bug IMHO: if Gerrit creates a new PS and then merge it, it should generate:

- patchset-created

- change-merged

If this is introduced our CI process would need adjustments. On patchset-created event we trigger regression checkin and if the change is already merged it will fail and spam developers with messages. On change-merged we prepare binaries for test runs outside of checkin regression. 

*IF* we introduce an extra patchset-created, then it should respect the label settings for copying the score over.

*IF* there is no copy max score policy, then it cannot merge automatically.

IIUC it would required for projects that has "rebase always" to have copy max score policy set to true. If no, how would we be be able to get commits in for Rebase Always and Cherry Pick? If I have understood correctly both always creates new commits on submit.    

  

That's my idea, however, we are all here to discuss if that would be useful OR not. :-)

Absolutely.

Back to the subject of catching semantically issues. I suspect this requires some serialization with the a regression testing system.  OpenStack has a an with optimistic sequential parallel approach for merging commits. I would love to have time to investigate and use this but I have not had the time. 

Åsmund  

 

Luca.

 

 

*BUT* the validation would be useless because it could provide a -1 when the change has been merged already :-(

And yes, even if you did get a patchset-created event as well it won't help you like you explained.

Not really, it would help to understand what's going on.

Even if the CI feedback won't block the submission, it will be recorded on the change to say "Hey mate, that change broke the build !"

It took a while for us to understand what was going on in Gerrit: I thought that our CI validation scripts were wrong .... a bit of panic :-(

-- 
-- 
To unsubscribe, email repo-discuss+unsubscribe@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en

--- 
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

-- 
-- 
To unsubscribe, email repo-discuss+unsub...@googlegroups.com

[Luca Milanesio]

On 19 Apr 2018, at 09:03, Åsmund Østvold <asm...@gmail.com> wrote:

On Thu, Apr 19, 2018 at 9:32 AM, Luca Milanesio <luca.mi...@gmail.com> wrote:

On 19 Apr 2018, at 08:28, Åsmund Østvold <asm...@gmail.com> wrote:

On Thu, Apr 19, 2018 at 12:42 AM, Luca Milanesio <luca.milanesio@gmail.com> wrote:

On 18 Apr 2018, at 23:38, Gert van Dijk <gert...@gmail.com> wrote:

On Thursday, 19 April 2018 00:31:10 UTC+2, lucamilanesio wrote:

Using "Rebase Always" will have Gerrit rebase the change on-submit time, yet allows you to have out-of-date patch sets to be uploaded (and verified). Thus, "Rebase Always" will not solve the problem.

Yes, but in theory only if you have copyAllScoresOnTrivialRebase (defaulted to false). In practice, I believe you are right. The CI will be triggered anyway because you have a new PS pushed

No, it won't fire a patchet-created event for me, but only one of '"type":"change-merged"'.

That's a bug IMHO: if Gerrit creates a new PS and then merge it, it should generate:

- patchset-created

- change-merged

If this is introduced our CI process would need adjustments. On patchset-created event we trigger regression checkin and if the change is already merged it will fail and spam developers with messages. On change-merged we prepare binaries for test runs outside of checkin regression. 

*IF* we introduce an extra patchset-created, then it should respect the label settings for copying the score over.

*IF* there is no copy max score policy, then it cannot merge automatically.

IIUC it would required for projects that has "rebase always" to have copy max score policy set to true. If no, how would we be be able to get commits in for Rebase Always and Cherry Pick? If I have understood correctly both always creates new commits on submit.    

Yes, that should apply IMHO to every rebase done by gerrit.

The cherry-pick strategy is different: that is *always* broken conceptually because you do not merge the change you verified but only cherry-pick an individual commit.

Talking with other contributors / maintainers, nobody really uses cherry-pick in production though.

Luca.

[Gert van Dijk]

On Thursday, 19 April 2018 10:18:10 UTC+2, lucamilanesio wrote:

Talking with other contributors / maintainers, nobody really uses cherry-pick in production though.

I would not make that assumption really - I have a different view and I learned over the years that development/merge strategies are very much opinionated. Some maintainers really like full control, they want to cherry pick individual changes and don't want to have a system think for them. (Regardless of using Gerrit or not - e.g. plain git-send-email on mailing list.)

[Luca Milanesio]

I remember it was even discussed a few months ago to get rid of the cherry-pick strategy altogether :-O

Do you guys use it?

Luca.

[Åsmund Østvold]

So my project was the oddball then, see high level description other place in this thread. A strictly single commit verification and merge did make our process strict but 11 different projects/repositories all having a working master at all times did actually work VERY well for us. We had bad temported blame wars when things did break before we enforced single commit serialisation merges. With automating the merge and having parallel precheck for quick feedback it worked and there was no possibility to break the 11 master branches from working together.   

 

Luca.

[Martin Fick]

On Thursday, April 19, 2018 07:56:39 AM Luca Milanesio
wrote:

> > On 19 Apr 2018, at 06:48, Duft Markus
> > <Marku...@ssi-schaefer.com> wrote:
> > 3) CI Builds are taking quite some time for us.
> > With database changes, a single CI can take up to half
> > an hour. This is a lot of time. Thinking of the ~200
> > changes per day on that particular project we just
> > cannot affort spending more CI time than necessary, and
> > will rather take the risk is hitting this issue from
> > time to time instead.
> True, in your case the build time makes the option not
> worth the price.

Our builds take over 2 hours. We are able to merge over 200
changes in a day using a batching strategy. 24 hours =
about 12 * 2 hour builds in a day. This works because we
tend to batch around 20 changes in a build. So assuming the
builds pass, that gives about 20 * 12 = 240 changes/day.

So with really smart good batching it is possible to get
"serialized" submit verification with a fairly high
throughput that should never break your build. To achieve
this we also do individual change verification (in parallel)
on every dependency chain before we allow it to be ready for
submit verification. This allows many failures to be caught
way before submit time to help increase the chances that the
"precious" large batch submit verification will pass.
Pipelining (which we do not do) the last stage would further
help increase pass rates and throughput,

[Luca Milanesio]

Hi Martin,
why don't you put together a short presentation at the next Gerrit User Summit?
(BTW: are we planning for it?)

I believe your plugin + experience would be really useful if shared with the community :-)

Luca.

[Martin Fick]

On Thursday, April 19, 2018 05:16:49 PM Luca Milanesio
wrote:

> Hi Martin,
> why don't you put together a short presentation at the
> next Gerrit User Summit? (BTW: are we planning for it?)

That would be fun, I certainly will consider it. No idea
when?

> I believe your plugin + experience would be really useful
> if shared with the community :-)

Sure,

[Duft Markus]

Just out of interest: how do you deal with different manual code review scores then? Suppose change no. 5 in the batch gets a negative review, change 6 does not. Change 6 breaks the build without 5 being in. What happens in this case?

Cheers, Markus

[Martin Fick]

On Thursday, April 19, 2018 04:34:55 PM Duft Markus wrote:
> Just out of interest: how do you deal with different
> manual code review scores then? Suppose change no. 5 in
> the batch gets a negative review, change 6 does not.
> Change 6 breaks the build without 5 being in. What
> happens in this case?

Everything has to be ready (CR+2, V+1...) to make it into
the serialized batch. We then use the "patch-set" and
"approval" lock labels so that they can't change during
serialized CI, exactly so that this can't happen,

[Oswald Buddenhagen]

On Wed, Apr 18, 2018 at 04:27:51PM -0600, Martin Fick wrote:
> This is what the "batch" plugin is designed to help you do
> efficiently. It allows you to merge changes server side for
> serialized verification, and then "submitting" the exact sha1 you
> test. We use it internally for a lot of work.
>

both we (the qt project) and openstack have been doing that for years as
well.


the qt project uses a customized gerrit which introduces the two
additional states STAGED and INTEGRATING. when one presses the "stage"
button, the change is cherry-picked (yes, that merge strategy!) into the
staging branch. later the CI system comes around, puts the changes into
INTEGRATING state, branches a build branch from the staging branch, and
builds it. when the integration succeeds, the target branch is
fast-forwarded and the changes are marked as MERGED. if it fails, the
the staging branch is rebuilt without that batch (which may cause
subsequently staged changes to be unstaged because of conflicts), and
the changes return to NEW.
an important aspect of the whole thing is that gerrit guards that
STAGED and INTEGRATING changes cannot be scored or pushed over.

this process is somewhat crude and we'd like to refine it somewhat. in
particular, re-staging changes which failed only because of misbehaved
other changes in the batch is fairly annoying, so adding a new FAILED
state and basing a nice gui for selective mass restaging on it would be
good to have.

i discussed these process changes with dave b. at the hackathon in march
2016. at the time, the prediction was that upstreaming that specific
workflow has rather low chances. the option to make the workflow
configurable was discarded as waaaay too much effort.


openstack uses a batch integration tool called zuul. it runs on top of a
stock gerrit and is triggered by configurable gerrit events. the big
advantage for them is that they don't have the upgrade pain we have. the
disadvantage is that the system does not guard "busy" changes against
modification, so the system is reactive (it just has to discard affected
integrations).

[Benoit Sigoure]

BTW this whole issue is https://bugs.chromium.org/p/gerrit/issues/detail?id=3751

On Wed, Apr 18, 2018 at 3:31 PM, Luca Milanesio
<luca.mi...@gmail.com> wrote:
> P.S. Happened twice in 10 years of the Gerrit project ... I may say that
> isn't a common scenario after all !

In a larger engineering organization this happens a lot more
frequently. I've seen this happen maybe once a quarter in mine. And
a once-a-quarter build breakage due to this is really not an
acceptable rate.

When Gerrit automatically rebases a change, it should invalidate the
+1-Verified, wait for CI to bless the change again, and then
autosubmit the change. And if by the time Gerrit wants to autosubmit
the change, it needs to be re-rebased again, it should rinse and
repeat the process automatically on its own until it succeeds to merge
or the build fails. This way the process isn't onerous for us slow
and expensive humans, we don't have to manually rebase all the changes
or constantly babysit the ones that are good to go when master is a
moving target.

--
Benoit "tsuna" Sigoure

[Luca Milanesio]

That's why I asked Martin to present his experience at the next Gerrit User Summit :-)

The overall strategy of having a "batching" of commits ready to be submitted and then validating them properly before the final submit, it makes a lot of sense to me.
He has for sure *a lot of changes* and without that strategy, he would have experienced failed builds all the time :-(

The autosubmitter plugin does something similar as well, just not in "batches" but in a reactive way.

Luca.

[Benoit Sigoure]

Right. I personally don't want batching. I want to ensure that each
commit builds+tests on its own, independently of other changes.

--
Benoit "tsuna" Sigoure

[Martin Fick]

Seems like "Fast Forward Only" would satisfy your
requirements then? It forces a rebase and re-verification
before merging,

[Benoit Sigoure]

On Mon, Apr 23, 2018 at 8:22 AM, Martin Fick <mf...@codeaurora.org> wrote:
> On Monday, April 23, 2018 08:04:12 AM Benoit Sigoure wrote:
>> On Mon, Apr 23, 2018 at 7:45 AM, Luca Milanesio
>>
>> <luca.mi...@gmail.com> wrote:
>> >> On 21 Apr 2018, at 06:26, Benoit Sigoure
>> >> <tsun...@gmail.com> wrote:
>> >>
>> > The overall strategy of having a "batching" of commits
>> > ready to be submitted and then validating them properly
>> > before the final submit, it makes a lot of sense to me.
>> > He has for sure *a lot of changes* and without that
>> > strategy, he would have experienced failed builds all
>> > the time :-(
>> >
>> > The autosubmitter plugin does something similar as well,
>> > just not in "batches" but in a reactive way.
>> >
>> > Luca.
>>
>> Right. I personally don't want batching. I want to
>> ensure that each commit builds+tests on its own,
>> independently of other changes.
>
> Seems like "Fast Forward Only" would satisfy your
> requirements then? It forces a rebase and re-verification
> before merging,

Except for the fact that I don't want to require users to manually
rebase their changes. In a large code base with many users and lots
of changes merging all the time, it's not practical.

--
Benoit "tsuna" Sigoure

[Luca Milanesio]

Yes, that's why I proposed to extend the 'autosubmitter' plugin to perform the auto-rebase as well :-)

Luca.

>
> --
> Benoit "tsuna" Sigoure

[Martin Fick]

On Monday, April 23, 2018 08:34:48 AM you wrote:
> On Mon, Apr 23, 2018 at 8:31 AM, Martin Fick
<mf...@codeaurora.org> wrote:

> > On Monday, April 23, 2018 08:27:13 AM you wrote:
> >> On Mon, Apr 23, 2018 at 8:22 AM, Martin Fick
> >
> > <mf...@codeaurora.org> wrote:
> >> > On Monday, April 23, 2018 08:04:12 AM Benoit Sigoure
> >
> > wrote:
> >> >> On Mon, Apr 23, 2018 at 7:45 AM, Luca Milanesio
> >> >>
> >> >> <luca.mi...@gmail.com> wrote:
> >> >> >> On 21 Apr 2018, at 06:26, Benoit Sigoure
> >> >> >
> >> >> >> <tsun...@gmail.com> wrote:
> >> >> > The overall strategy of having a "batching" of
> >> >> > commits ready to be submitted and then validating
> >> >> > them properly before the final submit, it makes a
> >> >> > lot of sense to me.
> >> >> > He has for sure *a lot of changes* and without
> >> >> > that strategy, he would have experienced failed
> >> >> > builds all the time :-(
> >> >> >
> >> >> > The autosubmitter plugin does something similar as
> >> >> > well, just not in "batches" but in a reactive way.
> >> >> >

> >> >> Right. I personally don't want batching. I want to
> >> >> ensure that each commit builds+tests on its own,
> >> >> independently of other changes.
> >> >
> >> > Seems like "Fast Forward Only" would satisfy your
> >> > requirements then? It forces a rebase and
> >> > re-verification before merging,
> >>
> >> Except for the fact that I don't want to require users
> >> to manually rebase their changes. In a large code base
> >> with many users and lots of changes merging all the
> >> time, it's not practical.
> >

> > Then perhaps you want to just use the batch plugin with
> > a single change at a time (no need to actually batch
> > them)? With it you can merge (or cherry-pick) a single
> > change to the current tip, get a ref for verification and
> > then perform submit once verification passes,
>
> (not sure if you replied off list intentionally)

No, sorry. Reposted here so others can follow...

> Ah! Using the batch plugin with a batch size of one! I
> hadn't thought about this.

We use that for our first pass parallel verification so that
we know the change builds against a more recent tip,