test(e2e): pick a non-seed victim in the self-heal test - #345
test(e2e): pick a non-seed victim in the self-heal test#345Andrey Kolkov (androndo) wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe self-healing end-to-end test selects a non-bootstrap member as its victim and reports the victim Pod’s etcd restart count and readiness when the recovery wait condition fails. ChangesSelf-healing test reliability
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
4fb0401 to
9e572e6
Compare
Andrei Kvapil (kvaps)
left a comment
Andrei Kvapil (kvaps)
left a comment
There was a problem hiding this comment.
LGTM.
Correctly identifies the root cause: original[0] picks the alphabetically-first member while names are apiserver-assigned random suffixes, so the seed became the victim in roughly one run in three — and the seed is permanently excluded from self-heal by the !member.Spec.Bootstrap gate, so the test then waited out its full 15m for a deletion that was never coming. Verified against three runs (this branch's own history, plus #343 where it failed identically at 940.9s and passed on a re-run).
The replacement picks deterministically and asserts the single-seed invariant rather than silently proceeding on a wrongly-shaped cluster, and the failure message now reports actual restart state instead of unconditionally claiming "not yet past threshold" — which is what made the real failure mode unreadable.
One forward-looking note, no change requested here: the comment at the PVC assertion says the volume is GC'd via owner-ref. #347 removes that owner reference and reclaims the volume explicitly in this exact path instead, so the assertion keeps passing but the comment goes stale. I'll fix the wording in #347 once this lands, to keep the two from conflicting.
9e572e6 to
ff888ee
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/member_selfheal_test.go`:
- Line 209: Update selfHealReplaceableMember to sort the eligible members
deterministically before returning eligible[0]. Preserve the existing
eligibility filtering and selection behavior, adding only the minimal ordering
logic needed to make the chosen non-bootstrap member independent of kube.List()
order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 587675a2-1e13-4575-855c-07b645c9059c
📒 Files selected for processing (1)
test/e2e/member_selfheal_test.go
| if len(eligible) == 0 { | ||
| t.Fatalf("no non-bootstrap member to corrupt; seeds=%v", seeds) | ||
| } | ||
| return eligible[0] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files matching test filename =="
fd -a 'member_selfheal_test.go' . || true
file="$(fd 'member_selfheal_test.go' . | head -n 1 || true)"
if [ -n "${file:-}" ]; then
echo "== line count =="
wc -l "$file"
echo "== relevant helper and imports around candidate =="
sed -n '1,260p' "$file" | nl -ba | sed -n '1,260p'
fi
echo "== usages/selectors for eligible helper =="
rg -n "eligible|GetEligible|eligibleMembers|List\\(" test/e2e/member_selfheal_test.go test/e2e 2>/dev/null || true
echo "== controller-runtime dependency refs =="
rg -n "controller-runtime|sigs.k8s.io/controller-runtime" go.mod go.sum vendor 2>/dev/null || trueRepository: cozystack/etcd-operator
Length of output: 401
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="./test/e2e/member_selfheal_test.go"
echo "== imports and top of file =="
sed -n '1,60p' "$file" | cat -n
echo "== helper and lines 180-220 =="
sed -n '180,225p' "$file" | cat -n
echo "== usages in test/e2e member_selfheal_test.go =="
rg -n "eligible|GetEl|^/\*|List\\(" "$file" || true
echo "== dependency version refs =="
sed -n '/controller-runtime/,+5p' go.mod 2>/dev/null || true
rg -n "controller-runtime|sigs.k8s.io/controller-runtime" go.mod go.sum 2>/dev/null | head -n 20 || trueRepository: cozystack/etcd-operator
Length of output: 6341
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== kube definitions/usages in e2e =="
rg -n "var kube|kube :=|controllerruntime|client\\.New|NewClient" test go.mod 2>/dev/null || true
echo "== Kubernetes controller-runtime List behavior docs/source if vendored or Go module cache =="
(GOPATH="$HOME/go" go env GOPATH) 2>/dev/null || true
for p in "$HOME/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.21.0/pkg/client/client.go" \
"$HOME/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.21.0/pkg/cache/cache.go"; do
echo "-- $p --"
if [ -f "$p" ]; then
rg -n "func .*List\\(|Cache|List\\(" "$p" | head -n 40
else
echo "missing"
fi
done
echo "== deterministic sort probe for eligible selection =="
python3 - <<'PY'
eligible = ["member-b", "member-a", "member-c"]
print("unsorted first:", eligible[0])
eligible.sort()
print("sorted first:", eligible[0])
print("require_sort:", eligible[0] != ["member-b", "member-a", "member-c"][0])
PYRepository: cozystack/etcd-operator
Length of output: 3641
Sort eligible members before selecting eligible[0].
selfHealReplaceableMember builds an unsorted slice from kube.List(), so slicing eligible[0] can pick different non-bootstrap victims depending on listing order. Sorting keeps only the deterministic helper needed.
Proposed fix
+import "sort" // add to the existing import block
+
...
+ sort.Strings(eligible)
return eligible[0]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return eligible[0] | |
| sort.Strings(eligible) | |
| return eligible[0] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/member_selfheal_test.go` at line 209, Update
selfHealReplaceableMember to sort the eligible members deterministically before
returning eligible[0]. Preserve the existing eligibility filtering and selection
behavior, adding only the minimal ordering logic needed to make the chosen
non-bootstrap member independent of kube.List() order.
|
The diagnosis in this PR is correct and it is the thing I built on, so I want to be specific about that before proposing something different. The root cause here — Where I ended up differing is the conclusion. This PR reads the seed exclusion as a property of the operator to be worked around; I think it is the bug, and I have opened #354 and #355 as a stack that removes it. The exclusion is safe to delete. The guard was protecting the bootstrap window — deleting the seed while the cluster is still forming destroys the only copy of a cluster nothing else has joined yet — but it was written as a permanent property of a member. The window turns out to be protected already, by the quorum gate on the very next line. Two things already in the tree confirm that a cluster with no Why the test change specifically stops being the one we want. Once the operator is fixed, selecting a non-seed victim means the suite no longer exercises the seed at all — and the seed is the member that was uniquely unrecoverable. A future change that reintroduces the exclusion would pass CI unnoticed. Inverting the selection to target the seed deliberately gets the identical determinism and the identical flake fix, and additionally fails on That inversion carries over the substance of this PR rather than discarding it: deterministic victim selection instead of list order, the assertion that exactly one A second issue surfaced while establishing why the exclusion existed, and is the upper layer of the stack: Suggested disposition, though it is your call as the author. I would close this in favour of the stack, since the inverted test covers everything here plus the regression it guards. The one argument for landing this first is timing: it is test-only and safe, the flake is costing CI runs now, and the stack is larger and carries an e2e that has not yet been run against a live cluster. If you would rather stop the bleeding immediately, landing this and then taking the inversion as part of #354 is a perfectly reasonable order — the two touch the same file and will conflict, so it is worth deciding the sequence rather than discovering it at merge time. Either way, thanks for chasing this down to the gate; the CI evidence made the underlying bug straightforward to argue. |
TestPVCMemberCrashLoopSelfHeal corrupted `original[0]`, the alphabetically-first member, because List returns items name-sorted. Member names are apiserver-assigned random suffixes, so the bootstrap seed sorted first — and became the victim — in roughly one run in three. The seed is permanently excluded from self-heal: the caller of etcdContainerStuck gates on !member.Spec.Bootstrap, and Spec.Bootstrap is set once at creation and never cleared. So on those runs the test waited out its full 15m timeout for a deletion that was by design never coming. Pick the victim by Spec.Bootstrap == false instead, and assert the single-seed invariant while we are there. The seed-exclusion contract itself is already covered by the unit test TestUpdateStatus_KeepsStuckBootstrapMember; this test covers the replaceable case, so it must choose a replaceable member deterministically. Also fix the timeout message, which claimed "crash-loop not yet past threshold" unconditionally. In both observed failures the victim was well past the threshold (7 restarts, threshold 5) and held back by the bootstrap gate, so the message pointed at the wrong cause. Report the actual restart count and readiness instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com>
ff888ee to
1ec6646
Compare
Pull request was closed
3 participants
Summary
TestPVCMemberCrashLoopSelfHealfails roughly one run in three, for a reasonbuilt into the test rather than into the operator. It picks its victim by index
and can land on the bootstrap seed, which is permanently excluded from the
self-heal path — so the test waits out its full 15-minute timeout for a deletion
that is by design never coming. This makes the victim selection explicit.
Root cause
The test chose
original[0]:Listreturns items name-sorted, and member names are apiserver-assigned randomsuffixes (
etcd-p6bvx,etcd-v9kr8, …). Sooriginal[0]is simply whichevermember's random suffix sorts first — the seed in about a third of runs.
The seed can never be self-healed.
etcdContainerStuck's caller gates on it:and
Spec.Bootstrapis set once, at member creation(
controllers/etcdcluster_controller.go:463), and never cleared. When the victimis the seed the member is therefore never deleted, and the test burns its whole
15m budget before failing.
Observed across three consecutive E2E runs — the seed is the member that was
not added as a learner:
fix/rebootstrap-when-every-member-lost)chore/codeowners)chore/codeowners, re-run)Note the second row:
chore/codeownersis an 8-line change to.github/CODEOWNERSwith no Go code at all, and it failed identically. The940.9 s duration in both failures is the 15m wait plus setup — the test never
had a chance, it just had to time out first.
What changed
selfHealReplaceableMemberpicks a member withSpec.Bootstrap == false, so thevictim is always one the self-heal path can actually act on. It also asserts the
single-seed invariant (the cluster controller finds the seed by
Spec.Bootstrapand assumes exactly one) rather than silently selecting from a wrongly-shaped
cluster.
The timeout message is fixed too. It claimed:
In both failures the victim was well past the threshold — 7 restarts against
dataLossRestartThreshold = 5— and held back by the bootstrap gate instead. Themessage sent the one real failure this test has produced looking in the wrong
place. It now reports the actual restart count and readiness:
Test coverage
No new test: this is the test, and the fix is in how it selects its subject.
The two behaviours involved are each covered where they belong —
flaky):
TestUpdateStatus_KeepsStuckBootstrapMember, unit, already onmain.TestPVCMemberCrashLoopSelfHeal, which now reliably exercises it instead ofcovering it two runs out of three.
Verified locally:
go vet ./...,go vet -tags e2e ./test/e2e/...,gofmt -lclean, and
go test ./controllers/...green. The E2E itself needs kind +cert-manager + Kamaji and runs in CI here. Worth stating plainly: a single green
E2E run cannot by itself prove a 1-in-3 flake is gone — the argument is that the
victim is now selected by the very predicate the self-heal path gates on, so the
failing case is unreachable by construction rather than unlikely.
Follow-up (not in this PR)
The flake incidentally documents a real gap: a seed member whose data dir is
destroyed never recovers. Quorum among the remaining members holds, but the
seed crash-loops forever — excluded from self-heal, with
Spec.Bootstrapnevercleared and no condition saying so. Two candidate fixes, both narrow:
Spec.Bootstraponcestatus.clusterIDhas latched —docs/concepts.md:91already asserts that after latching "the seed is, from that point on, just a
regular member", which is not currently true of this field; or
clusterHasQuorumWithoutholds, which is thecondition that makes replacement safe in the first place.
Happy to open that separately once this lands.
Summary by CodeRabbit