fix(controllers): stop exempting the bootstrap seed from self-heal

[Timofei Larkin (lllamnyp)]

Rebased onto main after #352.

The bug

Crash-loop self-heal is gated on !member.Spec.Bootstrap:

// controllers/etcdmember_controller.go
if !member.Spec.Bootstrap &&
    etcdContainerStuck(pod) &&
    r.clusterHasQuorumWithout(ctx, member) {

Spec.Bootstrap is set once, when the cluster controller creates the seed
(etcdcluster_controller.go:434), and is never cleared anywhere in the tree. So
the member that bootstrapped the cluster carries the exemption for the entire
life of the cluster — long after bootstrap is over and it has become an
ordinary voter indistinguishable from its peers.

The result: a seed whose data dir is lost or corrupted crash-loops forever. The
one mechanism the operator has for recovering exactly that failure refuses to
look at it, permanently, because of what the member was months ago.

This also contradicts what the docs already promise in three places — that the
seed flag is "the discovery anchor and is otherwise just historical metadata"
(concepts.md), that "once clusterID is set, the operator never re-reads
spec.bootstrap for any decision" (concepts.md), and that the seed "has no
permanent special role and can be removed like any other member"
(operations.md). The self-heal gate was the one place the code disagreed.

Why the guard existed, and why the predicate was wrong

The guard is protecting something real. Deleting the seed while the cluster is
still forming would destroy the only copy of a cluster that no other member has
joined yet — there is nothing to gap-fill from, and the finalizer's
MemberRemove has no peers to run against.

But that is a property of the phase, not of the member. "The cluster is
still bootstrapping" is true for a minute and then false forever. "This member
bootstrapped the cluster" is true forever. Gating on the second to protect the
first is what makes the exemption permanent — the guard has no way to expire
because the fact it reads never changes.

Why dropping it is safe

The bootstrap window is protected already, by the quorum gate standing
immediately next to it, at no extra cost:

clusterHasQuorumWithout requires readyOthers >= desired/2+1, reading
readyOthers from cluster.Status.ReadyMembers. While status.clusterID is
unlatched, Reconcile routes into bootstrap() / tryDiscoverCluster() and
never reaches updateStatus — so ReadyMembers is 0 for the whole
bootstrap window. Zero never satisfies the gate, at any replica count. The seed
cannot be self-healed during bootstrap whether or not anyone checks
spec.bootstrap.

The same arithmetic permanently protects a 1-replica cluster's only member:
desired/2+1 == 1, and readyOthers after subtracting the member itself is
0. It is never deleted at any point in the cluster's life. (The stale-high
ReadyMembers case is what the existing subtract-yourself logic in that
function handles, and there is a test for it.)

So this removes a redundant guard, not a load-bearing one. spec.bootstrap
keeps every other job it has — it is still how bootstrap(),
tryDiscoverCluster() and hasPendingBootstrap() locate the seed, all of which
run only while clusterID is unlatched.

Two things already in production confirm that a cluster with no
Bootstrap=true member is a normal, supported state:

• Every adopted cluster is one. internal/migrate/adopt.go:283 creates
  every member with Bootstrap: false and pre-latches status.clusterID. No
  migrated cluster has a seed at all.
• The memory pod-loss self-heal already deletes seeds. The check at
  etcdmember_controller.go:149 has never consulted spec.bootstrap; a
  memory-backed seed whose Pod is lost is deleted and gap-filled today.

Production impact

Not CI-only. To hit it, a seed's data dir has to become unreadable while the
cluster membership has moved on — a volume lost on node failure, a corrupt
bbolt/WAL header, a storage backend hiccup. That is precisely the scenario
#336 was written for; it just declines to handle it on one member in every
cluster.

How it presents: readyMembers sits at 2/3 indefinitely. The Pod accumulates
restarts well past dataLossRestartThreshold (the failing CI runs reached 7
against a threshold of 5) with no operator log line explaining why nothing is
being done, because the gate rejects the member before the log.Info that
announces a replacement. Nothing distinguishes it from a member that simply has
not crossed the threshold yet.

Severity is worse than one lost member: the cluster is stuck at 2/3 with no
redundancy left — one more failure loses quorum — and it stays there until a
human deletes the EtcdMember by hand. It also interacts badly with the
max-learners=1 constraint described in #352: a wedged member occupying the
single learner slot blocks further replacement, so an unhealable seed can
prevent recovery of other members too.

What changed

Drop the !member.Spec.Bootstrap conjunct. With #352 having removed the
memory-medium exclusion, the gate is now entirely state-based — nothing in it
names a member:

if etcdContainerStuck(pod) &&
    r.clusterHasQuorumWithout(ctx, member) {

etcdContainerStuck still covers not-ready, past restart threshold, not
OOMKilled and Pod-not-terminating; the quorum gate is unchanged.

Tests. TestUpdateStatus_KeepsStuckBootstrapMember pinned the old
behaviour; it is replaced by three tests that pin the new predicate:

• TestUpdateStatus_ReplacesStuckSeedAfterBootstrap — a formed cluster's stuck
  seed is replaced like any other member. Fails on main with expected a formed cluster's seed to be self-healed like any other member — the bug
  itself.
• TestUpdateStatus_KeepsStuckSeedDuringBootstrap — mid-bootstrap
  (ReadyMembers=0), the seed is left alone. This is the guard the old
  conjunct was reaching for, now pinned against the mechanism that actually
  provides it.
• TestUpdateStatus_KeepsStuckSoleMember — a 1-replica cluster's only member is
  never deleted, tested against the stale-high ReadyMembers worst case.

E2E. TestPVCMemberCrashLoopSelfHeal now corrupts the seed deliberately,
via a selfHealSeedMember helper that also asserts the single-seed invariant
the cluster controller relies on.

This is worth spelling out, because the same test is the subject of #345. That
PR diagnoses a real ~1-in-3 flake correctly: the test picked original[0] from
a name-sorted list, member names are apiserver-assigned random suffixes, so it
landed on the exempt seed about a third of the time and burned its full 15-minute
timeout waiting for a deletion that could never come. Its fix is to select a
non-seed victim. Both changes make the victim deterministic and both end the
flake; the difference is that avoiding the seed leaves the operator behaviour
untouched, while targeting it turns the test into a regression guard for the bug
— it would have failed on main for the right reason. #345's other improvement,
the misleading timeout message, is carried here too: it claimed "crash-loop not
yet past threshold" when the member was well past it and held back by a gate, and
now names both possibilities.

Docs. concepts.md and operations.md gain a bullet stating the
phase-versus-identity rule explicitly, and drop the last "non-bootstrap"
qualifier from the crash-loop description.

A related gap this does not close

buildPod also reads spec.bootstrap past latch, to choose
--initial-cluster-state=new vs =existing. Since the field is never cleared,
a re-created seed Pod is handed =new for the life of the cluster, and the
seed's --initial-cluster is frozen at bootstrap listing only itself.

etcd consults both flags only when the data dir is empty, so this is inert on
ordinary restarts and inert on a corrupt data dir — that just fails to boot and
is now self-healed. It is not inert when the seed's data dir returns empty
with the PVC binding intact (re-provisioned volume, PV restored blank,
node-local storage lost on reimage). A non-seed member in that state gets
=existing against a stale --initial-cluster and fails loudly. The seed gets
=new against an --initial-cluster naming only itself — a complete, internally
consistent bootstrap instruction. etcd does not error; it forms a fresh
one-member cluster on the empty dir and comes up ready.

Ready is the problem: no self-heal trigger can see it. The Status.PodUID check
needs a lost Pod, etcdContainerStuck needs a not-ready container. Meanwhile
<cluster>-client selects every member Pod with no role filter, so a share of
client traffic reaches a member serving an empty keyspace.

Worth noting: etcd derives both cluster ID and member ID from the initial
peer-URL set plus --initial-cluster-token, all unchanged here — so the
re-bootstrapped seed is expected to return under the same cluster ID rather
than being rejected on a mismatch. That is reasoned from etcd's ID derivation,
not from an observed incident, and is flagged as such in the docs.

The likely fix keeps spec.bootstrap as an immutable origin record and
re-derives the flag from whether the member has ever joined
(member.Spec.Bootstrap && member.Status.MemberID == ""), rather than clearing
the field. Left to its own change — it alters what etcd is told at boot and
deserves a separate bisect point.

Not a one-off

This is the third instance of one shape: a self-heal exclusion that leaves a
member permanently unrecoverable. #352 removed the memory-medium exclusion
from this same gate for the same reason — a wedged member stays wedged forever.
#343 covers the terminal case where every member is gone and the cluster
silently reports healthy.

Each exclusion was written for a real hazard and expressed as a permanent
property of the member rather than a condition that can clear. The general rule
worth adopting: self-heal may be gated on cluster state, never on member
identity — state can expire, identity cannot, and an exclusion that cannot
expire is an unrecoverable member waiting for the right failure. With this
change the gate holds to that rule exactly: etcdContainerStuck and the quorum
gate are both pure state.

Note docs/member-rollout-design.md (on the unmerged
design/mutable-settings-rollout branch) tells the rollout engine to "prefer a
non-seed" victim and cites this gate as the reason. That rationale goes away
once this lands — a non-leader preference still stands on its own, but the seed
no longer needs avoiding.

Verification

go build ./..., go vet -tags e2e ./test/e2e/ and go test ./controllers/
are green on the rebased branch. The new seed test was confirmed to fail against
current main's gate before the fix was applied. No cluster was touched.

Summary by CodeRabbit

• Bug Fixes

  • Crash-looping bootstrap members can now be automatically replaced after cluster formation when restart and quorum safeguards are satisfied.
  • Bootstrap members remain protected during initial discovery and when replacing them would compromise a single-member cluster.

• Documentation

  • Clarified bootstrap-member behavior, recovery conditions, and a known data-reset scenario.

• Tests

  • Added coverage for bootstrap-window protection, post-bootstrap recovery, quorum handling, and deterministic end-to-end validation.

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 188b3967-4bbc-4cb8-8e73-ab4f50a9bb09

📥 Commits

Reviewing files that changed from the base of the PR and between 70c93f8 and cf4615a.

📒 Files selected for processing (1)

• docs/concepts.md

🚧 Files skipped from review as they are similar to previous changes (1)

• docs/concepts.md

---

📝 Walkthrough

Walkthrough

Bootstrap seeds can undergo crash-loop replacement after quorum forms. Tests cover bootstrap-window and single-member protections. Documentation describes the updated behavior and the seed re-bootstrap gap. E2E coverage targets the bootstrap seed deterministically.

Changes

Bootstrap seed self-healing

Layer / File(s)	Summary
Controller eligibility and unit coverage controllers/etcdmember_controller.go, controllers/etcdmember_controller_test.go	The controller no longer excludes bootstrap seeds unconditionally. Tests cover replacement after quorum, bootstrap-window retention, and sole-member retention.
Seed lifecycle documentation docs/concepts.md, docs/operations.md	Documentation describes seed discovery, replacement eligibility, quorum protection, and the empty-data-directory behavior.
Deterministic E2E seed validation test/e2e/member_selfheal_test.go	The E2E test selects the bootstrap seed and reports restart-threshold and quorum-gate diagnostics.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• cozystack/etcd-operator#345: Related E2E victim-selection and diagnostic changes with opposite bootstrap-seed handling.
• cozystack/etcd-operator#347: Related crash-loop self-healing logic in EtcdMemberReconciler.
• cozystack/etcd-operator#352: Related crash-loop self-healing eligibility changes and tests.

Suggested reviewers: androndo

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: the bootstrap seed no longer receives an inappropriate self-healing exemption.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/seed-member-not-special

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[coderabbitai]

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 `@docs/concepts.md`:
- Line 91: Update the crash-loop self-heal reference in the documented seed
behavior to use the existing heading fragment `#crash-loop-self-heal` instead of
`#crash-loop-self-heal-pvc-members`.

🪄 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: 7f3dcf48-683e-4c5c-8a71-faed0537828f

📥 Commits

Reviewing files that changed from the base of the PR and between 24f3892 and 70c93f8.

📒 Files selected for processing (5)

• controllers/etcdmember_controller.go
• controllers/etcdmember_controller_test.go
• docs/concepts.md
• docs/operations.md
• test/e2e/member_selfheal_test.go

[Andrey Kolkov (androndo)]

LGTM.

Reviewed fix/seed-member-not-special at cf4615a against main.

The core change is a one-conjunct deletion of !member.Spec.Bootstrap from the self-heal gate, making replacement purely state-driven (etcdContainerStuck(pod) && clusterHasQuorumWithout(...)). Verified the load-bearing safety claims independently:

• Bootstrap window is protected by the quorum gate alone. While Status.ClusterID == "", Reconcile returns via bootstrap()/tryDiscoverCluster() and never reaches the ready-counting updateStatus, so ReadyMembers stays 0 and readyOthers >= desired/2+1 can never pass. The only path that does reach it (desired == 0) short-circuits in clusterHasQuorumWithout anyway.
• 1-replica sole member stays protected (desired/2+1 == 1, readyOthers → 0 after self-subtraction), pinned by TestUpdateStatus_KeepsStuckSoleMember at the stale-high worst case.
• No regression — the only changed behavior is a formed cluster's stuck seed now healing instead of wedging permanently.

Tests define the full contract (replace-after-bootstrap / keep-during-bootstrap / keep-sole-member); build, vet, and go test ./controllers/ green. Docs kept in sync and their claims confirmed against code; anchors now resolve. The 'seed re-bootstrap on empty data-dir' gap is pre-existing, correctly documented, and correctly scoped to a separate change.

The dead-anchor issue from the first pass is resolved.