Test-infra: Tide stuck for build-pipeline repo (trouble merging?)

[bobcatfish]

What happened:

In the build-pipeline repo, Tide doesn't seem to be merging https://github.com/knative/build-pipeline/pull/362

The status of the checks are all green:

And tide itself seems to be happy enough:

The status looks like this:

The labels seem to be in order:

But the merge does not happen.

@adrcunha reported in https://slack-redir.net/link?url=https%3A%2F%2Fgithub.com%2Fknative%2Fbuild-pipeline%2Fpull%2F362%23issuecomment-453301099 that in the Tide logs it complains that it can't rebase the PR

What you expected to happen:

• If the PR can't be merged, then github and/or Tide should surface the problem (probably the Tide status shouldn't be green)
• Or if the PR _can_ be merged (no idea why it can't be :S ) then Tide should merge it

How to reproduce it (as minimally and precisely as possible):

I wish I could tell you! Not sure why Tide can't rebase this

Please provide links to example occurrences, if any:

https://github.com/knative/build-pipeline/pull/362

But we might need to merge this manually to unblock everyone 😅

Anything else we need to know?:

[bobcatfish]

More deets:

The repo had both "squash and merge" and "rebase" as available merge options, but we only use "rebase" (via tide) and never squash, so i turned off the squash option and now github is showing what i guess is the same error tide is having:

However tide is still green and trying to merge 🤔 So I'm guessing that tide isn't anticipating this rebase error correctly (probably most people use the squash functionality so this doesnt happen often?)

[adrcunha]

From Tide logs:

E  {"client":"github","level":"info","component":"tide","msg":"Merge(knative, build-pipeline, 362, {  db93ff2a269aceb4dba37baec69575a18f40a9d0 rebase})"} 
E  {"org":"knative","component":"tide","sha":"db93ff2a269aceb4dba37baec69575a18f40a9d0","error":"This branch can't be rebased","msg":"Merge failed: PR is unmergable. Do the Tide merge requirements match the GitHub settings for the repo?","repo":"build-pipeline","base-sha":"8557df41d57a785bc8a04df0e603a… 
E  {"controller":"sync","msg":"Subpool synced.","targets":[362],"level":"info","repo":"build-pipeline","branch":"master","base-sha":"8557df41d57a785bc8a04df0e603ab1d07ef3f34","action":"MERGE","org":"knative","component":"tide"} 

Cluster is running gcr.io/k8s-prow/tide:v20190109-716fe27

[stevekuznetsov]

Does tide end up merging other possible PRs or does it crash-loop on this one forever?

[stevekuznetsov]

/cc @cjwagner
/kind bug
/area prow/tide

[adrcunha]

Crash-looping forever. Still stuck as you can see in https://prow.knative.dev/tide (at least until the offending PR gets rebased and the merge succeeds).

[cjwagner]

Yeah Tide can get stuck on PRs if there is a PR that it thinks should be mergeable, but isn't for some reason. I think this is the first time we've hit this specific case because most repos don't use the rebase merge strategy.

We have an issue open for making Tide eventually give up on a PR that can't be merged for some reason: https://github.com/kubernetes/test-infra/issues/10001

In the meantime if this happens the work around is /holding the offending PR until it is mergeable.

[cjwagner]

Crash-looping forever.

You don't mean this literally, right? Tide shouldn't be crashing from this.

[stevekuznetsov]

Yeah, sorry -- it will just fail to do the merge and try again forever

[adrcunha]

Crash-looping forever.
You don't mean this literally, right? Tide shouldn't be crashing from this.

Correct. Read it as "looping forever with the 'This branch can't be rebased' error".

[stevekuznetsov]

I wonder if in these cases tide itself should add the hold label and a comment explaining what went wrong

[bobcatfish]

Just a quick bump, running into this again with https://github.com/knative/build-pipeline/pull/430 , git says "This branch cannot be rebased due to conflicts" but tide appears to be trying to merge it ("MERGE: #430") and it's blocking anything else from merging.

For now I'll try putting a /hold on that PR and hopefully that'll unblock the flow

[stevekuznetsov]

So to be clear here GitHub is telling us the PR is merge-able but in reality it's not due to the configured merge strategy?

[bobcatfish]

So to be clear here GitHub is telling us the PR is merge-able but in reality it's not due to the configured merge strategy?

A bit of confusion @stevekuznetsov :

Assuming Prow is configured to do a rebase to merge:

• If the only merge strategy allowed in the repo is "rebase", github will tell us it can't merge, but tide will try to anyway
• If the repo allows both "rebase" and "merge", github will _not_ tell us it can't merge (since the merge strategy would work, though "rebase" wouldnt) and tide will still try to anyway

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[stevekuznetsov]

/remove-lifecycle stale
@bobcatfish sorry, this got lost in my notifications.

I'm not sure there is ever a case where the PR branch cannot be automatically rebased but _can_ be merged? Wouldn't you get conflicts in both cases?

[bobcatfish]

Hey @stevekuznetsov !

I'm not sure there is ever a case where the PR branch cannot be automatically rebased but can be merged? Wouldn't you get conflicts in both cases?

I'm pretty sure you can end up in this state: each commit is the application of a diff to a parent. So let's say your branch has commits:

• A (parent SOME_PREVIOUS_HEAD)
• B (parent A)
• C (parent B)

If you use the merge strategy, a new commit is made that has 2 parents, one of which is master's HEAD, the other is the branch's head:

There _can_ be a conflict here, but it would be between basically applying the net result of all the commits (i.e. using C as a base) to master's HEAD. (:sweat_smile: very hard for me to explain this, probably because my understanding of how 2 parent commits works is incomplete! :sweat_smile: )

If you rebase, what happens is that the commits themselves are edited, so our list of commits above would need to become:

• A (parent MASTERS_CURRENT_HEAD)
• B (parent A)
• C (parent B)

Since the base of each commit changes, there is the potential for conflicts when trying to apply the diff to a new base commit - for each commit! (vs the "net result" when using the merge strategy)

Some more sources of info (not sure if I explained that very clearly :sweat_smile: ), including where I stole that image from XD:

• https://help.github.com/en/articles/about-merge-methods-on-github
• https://www.atlassian.com/git/tutorials/using-branches/git-merge
• https://www.atlassian.com/git/tutorials/merging-vs-rebasing

[stevekuznetsov]

Not sure that makes sense. In the diagram above, you'd really have:

master:  A -- B -- C ------------ M
feature:            `-- D -- E --'

Since D and E are children of C in the first place, there's no way that you'd be able to merge but not rebase as they already diff on top of the commit you'd be rebasing on in the first place. My git fu isn't the best but that is my best understanding.

Unpacking your earlier comment:

If the only merge strategy allowed in the repo is "rebase", github will tell us it can't merge, but tide will try to anyway

So if we have a case where tide is attempting to merge something that is in the API as not mergeable that is a bug, but that is only ever the case if the merge actually cannot happen -- if there will be conflicts. This is different from Prow being misconfigured as to the strategy to use.

I think we might need a more concrete example to move forward with this since it's not really obvious to me what is going on here.

[bobcatfish]

Since D and E are children of C in the first place, there's no way that you'd be able to merge but not rebase as they already diff on top of the commit you'd be rebasing on in the first place.

I think your diagram is a good representation of the merge strategy:

We go from this:
master:  A -- B -- C ------------ M
feature:             `-- D -- E --`

To this (where N is a merge commit):
master:  A -- B -- C ------------ M---N
feature:            \- D -- E -------/

But afaik, the rebase strategy is more like this:

We start with this state:
master:  A -- B -- C ------------ M
feature:            `-- D -- E --'

And then we try to go to this:
master:  A -- B -- C ------------ M-- D -- E --

D and E are no longer children of C, they are rebased such that D becomes a child of M (master's former HEAD) - which is why we can end up with conflicts that have not been resolved.

Here is how you can reproduce it (probably not _exactly_ what github is doing, but pretty close I think!)

1. Create a git repo, commit a file with some text in it
2. Create a branch (B) from that commit
3. In master, commit some changes to the text
4. In branch (B), make different changes to the same text
5. Merge master into branch (B) <-- at this point there should be a conflict to resolve - note that when you resolve it, this happens _in the merge commit_ (and so at this point, merging using the "merge" strategy would succeed with no conflicts)
6. Now, from the branch, try to rebase onto master <-- at this point, you should run into a conflict with the first commit in (B), b/c though you resolved this _in your merge commit_, it's still not resolved when rebasing _this specific commit_ onto master

If that's not clear I can try to update this with an exact step by step reproduction :)

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[bobcatfish]

@stevekuznetsov did my explanation make sense? I think this is a legit issue folks can run into, tho not terribly urgent.

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

[k8s-ci-robot]

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.