Page MenuHomePhabricator

Zuul merger should reject merge when Gerrit repository has Allow Content Merge set to false
Open, Needs TriagePublic

Description

jenkins-bot no longer votes V-1 when a patch with C+2 can be rebased and passes all tests, but can't be submitted according to Gerrit (has the "Merge Conflict" status).

I think Gerrit for some reason uses a dumber merge algorithm that the "real" Git, so Jenkins manages to merge/rebase the changes, but it can't be submitted in Gerrit.

Examples from today:

The patch just sits there with C+2 V+2 until a human notices the issue (which is much harder to spot when there are no negative votes) and clicks "Rebase" and votes C+2 again.


Coments for https://gerrit.wikimedia.org/r/c/VisualEditor/VisualEditor/+/474533 with UTC on 2018-11-26:

ActorMessageTime
Bartosz DziewońskiPatch Set 4: Code-Review+219:12
jenkins-botPatch Set 4: -Verified Starting gate-and-submit jobs.19:12
jenkins-botPatch Set 4: Verified+2 Gate pipeline build succeeded19:16
Bartosz DziewońskiRemoved Code-Review+2 by Bartosz Dziewoński19:22
Bartosz DziewońskiPatch Set 5: Patch Set 4 was rebased19:23
Bartosz DziewońskiPatch Set 5: Code-Review+219:23
jenkins-botPatch Set 5: Starting gate-and-submit jobs.19:23
jenkins-botPatch Set 5: Verified+2 Gate pipeline build succeeded.19:25
jenkins-botChange has been successfully merged by jenkins-bot19:25

The Gerrit repository has:

SettingValue
Submit TypeMerge if Necessary
Allow content mergeINHERIT (false)

Event Timeline

One of the first thing Zuul does it attempting to merge the patchset against the tip of the branch.

The merge has worked as well as the test and indeed Gerrit rejected it when trying to do gerrit review --verified 2 --submit:

2018-11-26 19:16:39,061 DEBUG zuul.reporter.gerrit.Reporter: Report change <Change 0x7efdb41386d0 474533,4>, params {'verified': 2, 'submit': True}, message: Gate pipeline build succeeded.

- visualeditor-npm6-browser-node-6-docker https://integration.wikimedia.org/ci/job/visualeditor-npm6-browser-node-6-docker/298/console : SUCCESS in 3m 40s
- visualeditor-npm6-run-doc-node-6-docker https://integration.wikimedia.org/ci/job/visualeditor-npm6-run-doc-node-6-docker/292/console : SUCCESS in 1m 14s

2018-11-26 19:16:39,432 ERROR zuul.DependentPipelineManager: Exception while reporting:
Traceback (most recent call last):
  File "/usr/share/python/zuul/local/lib/python2.7/site-packages/zuul/scheduler.py", line 1768, in _reportItem
    ret = self.sendReport(actions, self.pipeline.source, item)
  File "/usr/share/python/zuul/local/lib/python2.7/site-packages/zuul/scheduler.py", line 1293, in sendReport
    ret = reporter.report(source, self.pipeline, item)
  File "/usr/share/python/zuul/local/lib/python2.7/site-packages/zuul/reporter/gerrit.py", line 39, in report
    message, self.reporter_config)
  File "/usr/share/python/zuul/local/lib/python2.7/site-packages/zuul/connection/gerrit.py", line 282, in review
    out, err = self._ssh(cmd)
  File "/usr/share/python/zuul/local/lib/python2.7/site-packages/zuul/connection/gerrit.py", line 387, in _ssh
    raise Exception("Gerrit error executing %s" % command)
Exception: Gerrit error executing gerrit review --project VisualEditor/VisualEditor --message "Gate pipeline build succeeded.

- visualeditor-npm6-browser-node-6-docker https://integration.wikimedia.org/ci/job/visualeditor-npm6-browser-node-6-docker/298/console : SUCCESS in 3m 40s
- visualeditor-npm6-run-doc-node-6-docker https://integration.wikimedia.org/ci/job/visualeditor-npm6-run-doc-node-6-docker/292/console : SUCCESS in 1m 14s
" --verified 2 --submit 474533,4

Unfortunately Zuul logging configuration does not capture Gerrit error message (something we should fix probably).

On the zuul merger:

2018-11-26 19:12:47,876 DEBUG zuul.MergeServer: Got merge job: 58d663b7ad6d4471825ad8c1bbccded3
2018-11-26 19:12:47,877 DEBUG zuul.Merger: Merging for change 474533,4.
2018-11-26 19:12:47,877 DEBUG zuul.Merger: Processing refspec refs/changes/33/474533/4 for project VisualEditor/VisualEditor / master ref Z0a4a105f973343d1961afac9f5daa074
2018-11-26 19:12:48,759 DEBUG zuul.Merger: Unable to find commit for ref master/Z0a4a105f973343d1961afac9f5daa074
2018-11-26 19:12:48,759 DEBUG zuul.Merger: No base commit found for (u'VisualEditor/VisualEditor', u'master')
2018-11-26 19:12:48,759 DEBUG zuul.Repo: Resetting repository /srv/zuul/git/VisualEditor/VisualEditor
2018-11-26 19:12:48,760 DEBUG zuul.Repo: Updating repository /srv/zuul/git/VisualEditor/VisualEditor
2018-11-26 19:12:54,047 DEBUG zuul.Repo: Checking out b5f36f1ea4b18d77820dbe617bfa25d5b6cd925c
2018-11-26 19:12:55,349 DEBUG zuul.Repo: Merging refs/changes/33/474533/4 with args ['-s', 'resolve', 'FETCH_HEAD']
2018-11-26 19:12:57,630 DEBUG zuul.Repo: CreateZuulRef master/Z0a4a105f973343d1961afac9f5daa074 at ea1a2197048ffcdf9564844ec2de978c48b8eacf on <git.Repo "/srv/zuul/git/VisualEditor/VisualEditor/.git">

Or in short, Zuul attempts to do: git merge -s resolve. From git-merge(1) manual page:

git-merge [-s <strategy>]

MERGE STRATEGIES

resolve
This can only resolve two heads (i.e. the current branch and another branch you pulled from) using a 3-way merge algorithm. It tries to carefully detect criss-cross merge ambiguities and is considered generally safe and fast.

Which I guess automatically resolves change that happened in the same file. VisualEditor/VisualEditor has Allow content merge set to false which rejects such conflicts.

Quick solution: change Allow content merge to true to make Gerrit more permissive. That is usually acceptable and with good test coverage it is not an issue, then it makes it harder to find potentially conflicting changes hitting the same files since most would no more be flagged as being in merge conflict.

For Zuul , I don't think it supports the content merge flag. When it is not allowed, I think it should switch to the octopus merge strategy which apparently does not attempt any resolution. From the man page:

octopus
This resolves cases with more than two heads, but refuses to do a complex merge that needs manual resolution. It is primarily meant to be used for bundling topic branch heads together. This is the default merge strategy when pulling or merging more than one branch.

TLDR:

  • In Gerrit set Allow Content Merge = True
  • Later, teach Zuul to use git-merge -s octopus when content merge is not allowed.

Slightly related is T131008#3307337

@matmarex next time it happens can you run and paste here the result of:

ssh gerrit.wikimedia.org 'gerrity query change:XXXXX --submit-records`

Wwhere XXXX is the change number. I am specially interested in the status field and the submitRecords -> status.

hashar renamed this task from jenkins-bot no longer votes V-1 when a patch with C+2 can be rebased and passes all tests, but can't be submitted according to Gerrit to Zuul merger should reject merge when Gerrit repository has Allow Content Merge set to false.Nov 27 2018, 11:15 AM

Related thread on zuul-discuss mailling list http://lists.zuul-ci.org/pipermail/zuul-discuss/2019-January/000687.html and my replies pointing to our task:

http://lists.zuul-ci.org/pipermail/zuul-discuss/2019-January/000690.html


On 09/01/2019 18:13, James E. Blair wrote:

Currently the supported merge modes[1] in Zuul are "merge",
"merge-resolve", and "cherry-pick".

In Gerrit, "rebase if necessary" attempts a fast-forward and if that
fails, rebases the change, "rebase always" simply always rebases the
change. The differences between "rebase always" and "cherry pick" in
Gerrit only relate to Gerrit's own dependency handling ("cherry pick"
ignores dependencies and "rebase always" does not). That doesn't apply
to Zuul, so if you use "cherry-pick" as the merge mode for a project,
you should get behavior similar to Gerrit's "rebase always". That's not
the same as "rebase if necessary", but perhaps it's closer than the
merge strategies.

It should be possible to implement "rebase if necessary" in Zuul -- I
think it would just attempt a fast forward, and, if that fails,
cherry-pick (a rebase of a single patch is the same as a cherry-pick).

[1]https://zuul-ci.org/docs/zuul/user/config.html#attr-project.merge-mode

Hello,

TLDR: switch to -s recursive

zuul-merger defaults to use the resolve strategy which would handle a
trivial conflict in a file. However Gerrit behavior is slightly
different. The code is the mergeStrategyName function in
gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java

Since Gerrit 2.10, the default is to use JGit recursive merge
(controlled by core.useRecursiveMerge defaulting to true).

Most probably zuul-merger should be switched to use the recursive
strategy. In practice I don't think it changes much.

Gerrit also have the per project option "Allow Content Merge" which in
project.config corresponds to:

[submit]
     mergeContent = false

When true, Gerrit uses the recursive strategy. However when it is false,
Gerrit uses JGit SIMPLE_TWO_WAY_IN_CORE strategy:

"Simple strategy to merge paths, without simultaneous edits"

The option would cause Gerrit to reject the submit of a change if it has
to do a conflict resolution, even if it is a trivial one.

To elaborate, on a repository that does not allow content merge and
given the topology:

(master)
|
B  C
| /
A

With B and C both altering the same file. The zuul-merger will attempt
to merge C in master and succeeds. It uses the resolve strategy which
would most probably solve the conflict between B and C.

Then Zuul asks Gerrit to submit the change, the merge get rejected due
to the SIMPLE_TWO_WAY_IN_CORE and the change fails to merge. The change
did get the Verified +2 and gate success message, but it is not
submitted and marked in merge conflict.

Unfortunately, git does not seem to have the equivalent of jgit
SIMPLE_TWO_WAY_IN_CORE strategy. Though maybe the octopus strategy would
reject it:

> octopus
> This resolves cases with more than two heads, but refuses to do a
complex merge that needs manual resolution. It is primarily meant to be
used for bundling topic branch heads together. This is the default merge
strategy when pulling or merging more than one branch.

A last barrier would be to have Zuul to detect the Gerrit reporter is
willing to submit, then check the change mergeability and reject the
reporting action when Gerrit says the change is not submittable. But
that seems prone to errors/race conditions. Forcing octopus might be
working, though I haven't tested that hypothesis.

More food, Zuul could query Gerrit for the project submit type and thus
adjust automatically between cherry-pick/rebase only etc :-)

Related: https://phabricator.wikimedia.org/T210442#4776011