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
📝 WalkthroughWalkthroughThe self-healing end-to-end test now 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 Suggested labels: 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.
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>
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.
2 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