[ovirt-devel] Gerrit parallel patch handling and CI (Or, why did my code fail post-merge)

[Nir Soffer]

On Sun, Nov 20, 2016 at 4:07 PM, Barak Korren <bkorren at redhat.com> wrote:
> Hi there,
>
> I would like to address a concernt that had been raised to us by
> multiple developers, and reach an agreement on how (and if)  to remedy
> it.
>
> Lets assume the following situation:
> We have a Git repo in Gerrit with top commit C0 in master.
> On time t0 developers Alice and Bob push patches P1 and P2 respectively
> to master so that we end up with the following situation in git:
> C0 <= P1 (this is Alice`s patch)
> C0 <= P2 (this is Bob`s patch)
>
> On time t1 CI runs for both patches checking the code as it looks for
> each patch. Lets assume CI is successful for both.
>
> On time t2 Alice submits her patch and Gerrit merges it, resulting in
> the following situation in master:
> C0 <= P1
>
> On time t2 Bob submits his patch. Gerrit, seeing master has changed,
> re-bases the patch and merges it, the resulting situation (If the
> rebase is successful) is:
> C0 <= P1 <= P2
>
> This means that the resulting code was never tested in CI.

This makes the CI useless.

To know if a patch actually passed the tests, you have to manually
rebase each patch and wait for the CI - this takes up to 20 minutes
on vdsm CI.

> This, in
> turn, causes various failures to show up post-merge despite having
> pre-merge CI run successfully.
>
> This situation is a result of the way our repos are currently
> configured. Most repos ATM are configured with the "Rebase If
> Necessary" submit type. This means that Gerrit tries to automatically
> rebase patches as mentioned in t2 above.
>
> We could, instead, configure the repos to use the "Fast Forward Only"
> submit type. In that case, when Bob submits on t2, Gerrit refuses to
> merge and asks Bob to rebase (While offering a convenient button to do
> it). When he does, a new patch set gets pushed, and subsequently
> checked by CI.
>
> I recommend we switch all projects to use the "Fast Forward Only" submit type.
>
> Thoughts? Concerns?

We have fast-forward in ioprocess and ovirt-imageio, and we are
happy with this setting.

Another advantage of fast-forward only merges is being able to submit
multiple patches with *one click*. If you submit the top patch in a series,
all patches are submitted.

With the current setting (in vdsm), submitting a series of patches is
a huge pain. Sometimes refreshing the page and submitting the next
patch in the series works, but sometimes you have to rebase again
the next patches in the series, and in the worst cases, you have to
do several rebases in the same series. This when the entire series
was already rebased properly before the submit.

In vdsm we were bitten by this many times, and both Dan and me agree
now that fast-forward is the only way.

I don't think we need to agree on all projects for this, the whole point
of having multiple project is that we don't to agree on every little
detail, the project maintainer can do whatever they want.

Thanks for raising this issue.

Nir