From 4da46f1c1ad1c2e52347a27c2ab16e99fb80a0ec Mon Sep 17 00:00:00 2001 From: Israel Fruchter Date: Mon, 10 Aug 2026 19:35:20 +0300 Subject: [PATCH] docs(minicloud): correct the merge runbook against what the merges did MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three things the runbook got wrong or did not know, all found while executing it: A GitHub stack object is separate from the base refs and was the actual merge blocker. 15617's base was already master, yet the button stayed unavailable and retargeting 15618 was refused outright — stack #15619 had to be dissolved with 'gh stack unstack'. The runbook had assumed base refs were the whole story. jenkins/precommit does not necessarily clear when the base becomes master. On 15618 the stacked-PR baseHash artifact was masking a real ruff-format failure in two of its own files; both look the same from outside, so the changed files have to be checked directly. 'git checkout -B ' fails when that branch is checked out in another worktree, and the following rebase then runs on the wrong branch. Use a scratch name and push with an explicit HEAD: refspec. --- docs/plans/minicloud-stack-merge-runbook.md | 120 ++++++++++++++------ 1 file changed, 83 insertions(+), 37 deletions(-) diff --git a/docs/plans/minicloud-stack-merge-runbook.md b/docs/plans/minicloud-stack-merge-runbook.md index 82c3e89645e..7b5af10e341 100644 --- a/docs/plans/minicloud-stack-merge-runbook.md +++ b/docs/plans/minicloud-stack-merge-runbook.md @@ -3,14 +3,15 @@ How to land [#15617](https://github.com/scylladb/scylla-cluster-tests/pull/15617) on its own and restack the rest, one PR at a time. Nothing here merges more than one PR. -Verified 2026-08-10 against `upstream/master` = `db9425d6e1`. +Written 2026-08-10 before the first merge; updated as the merges landed with what they actually +did. Steps 1 and 2 are complete (15617 and 15618 are on master) — Step 3 is the remaining one. ## The stack -| PR | branch | base today | commits of its own | topology it adds | +| PR | branch | base | commits of its own | topology it adds | |---|---|---|---|---| -| [#15617](https://github.com/scylladb/scylla-cluster-tests/pull/15617) | `feature/minicloud-integration` | `master` | 8 | core SCT support, local `scripts/run-minicloud-test.sh` | -| [#15618](https://github.com/scylladb/scylla-cluster-tests/pull/15618) | `feature/minicloud-pipelines` | `feature/minicloud-integration` | 8 | Jenkins jobs on a nested-virt **sct-runner** | +| [#15617](https://github.com/scylladb/scylla-cluster-tests/pull/15617) | `feature/minicloud-integration` | **merged to `master`** | 9 | core SCT support, local `scripts/run-minicloud-test.sh` | +| [#15618](https://github.com/scylladb/scylla-cluster-tests/pull/15618) | `feature/minicloud-pipelines` | **merged to `master`** | 8 | Jenkins jobs on a nested-virt **sct-runner** | | [#15668](https://github.com/scylladb/scylla-cluster-tests/pull/15668) | `feature/minicloud-local-agent` | `feature/minicloud-pipelines` | 2 | the **local KVM agent** alternative | All three head branches live on `scylladb/scylla-cluster-tests` (required for native GitHub @@ -45,44 +46,65 @@ The reverse is **not** true: 15618's own commits touch ten files 15617 also touc `sdcm/utils/minicloud/{bootstrap,config,gcp,manager,networking}.py`. 15618 and 15668 therefore stay stacked and are restacked after each merge; they cannot be reordered ahead of 15617. -## Step 1 — merge 15617 +## Step 1 — merge 15617 (done 2026-08-10) -Remaining gates, as of writing: +Merged by rebase; its 9 commits are on master as `7e4f498419`..`b214d08781`, every patch replayed +unchanged — `git range-diff af8b09ced2~9..af8b09ced2 7e4f498419^..b214d08781` shows all nine as +`=`. Do not check with a plain `git diff` against the PR head: master had moved 19 files under the +PR by merge time, two of them files this PR also touches, so the *trees* differ even though the +*patches* do not. Gates it had to clear: -- `jenkins/precommit` ✅ (build #21) -- `continuous-integration/jenkins/pr-head` — must go green -- **one maintainer approval** — `reviewDecision` is `REVIEW_REQUIRED`. All 13 review threads - (8 from dimakr, 5 from the copilot bot) are answered and resolved; each fix was re-verified - present in `139b5499db`. +- the four required contexts: `jenkins/precommit`, `jenkins/unittests`, + `jenkins/lint_test_cases`, `label` +- **one maintainer approval** (`required_approving_review_count: 1`). All 13 review threads + (8 from dimakr, 5 from the copilot bot) were answered and resolved, each fix re-verified present + in the head rather than taken from the reply. -Merge with **Rebase and merge**. Master has zero merge commits in its last 200 and no squash -`(#NNNNN)` suffixes in its last 100, so the convention is a rebase that replays the 8 commits -individually. +Merge with **Rebase and merge**: master has zero merge commits in its last 200 and no squash +`(#NNNNN)` suffixes in its last 100. -Do not touch 15618/15668 until this is in — GitHub auto-retargets 15618 to `master` when -`feature/minicloud-integration` is deleted, and that retarget alone leaves its diff wrong (it will -show 15617's changes again until the rebase below). +Do not touch 15618/15668 until this is in. -## Step 2 — restack 15618 onto master +**A GitHub *stack object* is a separate thing from the base refs, and it blocks the merge.** These +PRs were bound into stack #15619 (size 3); while it existed, GitHub refused even to retarget a +member — `Cannot change the base branch because the pull request is part of a stack` — and the +merge button stayed unavailable on 15617 despite its base already being `master`. Dissolve it +first, which leaves every base ref untouched: -Dry-run verified clean, and the resulting tree is byte-identical to today's `pr/15618`, under both -rebase-merge and squash-merge semantics. +```bash +gh api graphql -f query='{repository(owner:"scylladb",name:"scylla-cluster-tests"){ + pullRequest(number:15617){stack{number size}}}}' # read the stack number +gh stack unstack 15619 +``` + +Reversible with `gh stack link 15617 15618 15668` if the UI grouping is wanted back. Admins can +then merge past a missing approval with `gh pr merge 15617 --rebase --admin` (`enforce_admins` is +false on `master`). + +## Step 2 — restack 15618 onto master (done 2026-08-10; merged 2026-08-11) + +Rebase was clean and the resulting diff is 8 commits / 32 files, as predicted. Two caveats that +only showed up in the doing are recorded below: the precommit failure, and the branch-name clash. ```bash git fetch upstream master \ 'refs/pull/15618/head:refs/remotes/pr/15618' \ 'refs/pull/15668/head:refs/remotes/pr/15668' --force -# The old base commit — 15617's head as it was before the merge. Capture it BEFORE deleting -# the branch, or read it from the PR's "merged commit" reference afterwards. -OLD_BASE=139b5499db +# 15617's head as it was at merge time — not the SHA it had when this runbook was written, which +# is why it is read rather than remembered. Capture it BEFORE the branch is deleted, or recover +# it from the PR's merged-commit reference afterwards. +OLD_BASE=af8b09ced2 -git checkout -B feature/minicloud-pipelines pr/15618 +# Use a scratch branch name: `git checkout -B feature/minicloud-pipelines` fails with +# "already used by worktree at ..." if that branch is checked out in another worktree, and the +# rebase then silently runs on whatever branch you were on. +git checkout -B restack18 pr/15618 git rebase --onto upstream/master $OLD_BASE git rev-list --count upstream/master..HEAD # expect 8 -git push --force-with-lease=feature/minicloud-pipelines:$(git rev-parse pr/15618) \ - upstream feature/minicloud-pipelines +git push --force-with-lease=feature/minicloud-pipelines:$(git ls-remote upstream \ + refs/heads/feature/minicloud-pipelines | cut -f1) upstream HEAD:feature/minicloud-pipelines ``` Pin `--force-with-lease` to that separately-read SHA rather than the bare form; a chained fetch @@ -92,27 +114,51 @@ Then, on the PR: confirm the base is `master` (GitHub usually retargets it autom `gh pr edit 15618 --base master`), and check the diff is 8 commits / ~1365 insertions across 32 files — not 40+ files, which would mean the rebase did not take. -**`jenkins/precommit` should flip from fail to pass here.** It fails today only because Jenkins' -github-branch-source derives `baseHash` from a merge ref it cannot reach for a stacked PR; a -`master`-based PR has a reachable one. If it still fails after the restack, that is a real failure -and needs reading. +**`jenkins/precommit` does not necessarily clear on its own here — always read it after the +restack.** The stacking failure and a real failure look identical from the outside, and on 15618 +the first was hiding the second. Its console showed both at once: + +``` +fatal: ambiguous argument 'origin/master..HEAD': unknown revision or path not in the working tree. +ruff-format..............................................................Failed +``` + +The first line is the stacking artifact — Jenkins' github-branch-source derives `baseHash` from a +merge ref it cannot reach for a stacked PR, so `origin/master..HEAD` does not resolve. The second +was genuine: two files in 15618's own commits failed `ruff format --check` while passing at master. +So check the changed files directly rather than trusting the base change to fix it: -Review and merge 15618 on its own. Note in the PR that the only end-to-end evidence is the staging -AMI artifact job (#24); the GCE image job and the longevity job have not been linked. +```bash +.venv/bin/ruff format --check $(git diff --name-only upstream/master..HEAD | grep '\.py$') +.venv/bin/ruff check $(git diff --name-only upstream/master..HEAD | grep '\.py$') +``` + +Fix with `git commit --fixup=` per owning commit, then +`GIT_SEQUENCE_EDITOR=true git rebase -i --autosquash upstream/master` — no fixup commits left in +the history — and restack the PR above it again on the new head. + +Merged 2026-08-11 by rebase; its 8 commits are on master as `72f889859b`..`ae5f85a892`, again with +every patch replayed unchanged (`git range-diff` all `=`). The only end-to-end evidence noted in +the PR is the staging AMI artifact job (#24); the GCE image job and the longevity job were not +linked. ## Step 3 — restack 15668 onto master -Same shape, after 15618 lands: +Same shape, now that 15618 has landed: ```bash -OLD_BASE=fb0626590d # 15618's head before its merge +# 15618's head at merge time — read from the PR's merged headRefOid, not remembered. 15668's two +# commits sit exactly on it (`git rev-parse pr/15668~2` gives the same SHA). +OLD_BASE=1729aa537e -git checkout -B feature/minicloud-local-agent pr/15668 +git checkout -B restack68 pr/15668 git rebase --onto upstream/master $OLD_BASE git rev-list --count upstream/master..HEAD # expect 2 -git push --force-with-lease=feature/minicloud-local-agent:$(git rev-parse pr/15668) \ - upstream feature/minicloud-local-agent +git push --force-with-lease=feature/minicloud-local-agent:$(git ls-remote upstream \ + refs/heads/feature/minicloud-local-agent | cut -f1) upstream HEAD:feature/minicloud-local-agent + +# Note: 15668 must be restacked again after ANY force-push to 15618, not just after its merge. ``` 15668 also carries the one acknowledged blocker for the whole effort: no Jenkins node serves