fix(controllers): derive --initial-cluster-state from phase, not from the seed

[Timofei Larkin (lllamnyp)]

Second layer of a stack. Depends on #354, which makes the seed eligible for
crash-loop self-heal; this one makes the seed fail loudly in the case that
self-heal cannot otherwise see.

The bug

buildPod rendered etcd's --initial-cluster-state straight off spec.bootstrap:

clusterState := "new"
if !member.Spec.Bootstrap { clusterState = "existing" }

spec.bootstrap is set once at seed creation and never cleared, so the seed's
Pod was built with =new for the entire life of the cluster, not just for
its first boot. Its --initial-cluster is likewise frozen at the bootstrap
value, which names only itself.

Why that is usually invisible, and occasionally very bad

etcd honours --initial-cluster-state only when the data dir is empty. So it is
inert on ordinary restarts, and inert on a corrupt data dir — etcd fails to
boot either way, the Pod crash-loops, and self-heal replaces the member (that is
the path #336 added and #354 extends to the seed).

It is not inert when the seed's data dir comes back empty with the PVC
binding intact: a re-provisioned volume, a PV restored blank, node-local storage
lost on a node reimage. There the two cases diverge sharply:

• a non-seed member gets =existing against a stale --initial-cluster, fails
  loudly (member count is unequal), crash-loops, and is self-healed;
• the seed got =new against an --initial-cluster naming only itself. That is
  a complete, internally consistent bootstrap instruction, so etcd does not
  error. It forms a fresh one-member cluster on the empty dir and reports
  healthy.

The second outcome is worse than the crash-loop it resembles, because it is
quiet. Two things are certain here, and one is not — worth separating.

Certain: the member goes Ready on an empty data dir. Its readiness probe is
/health, which a healthy one-member cluster answers 200. <cluster>-client
selects every member Pod with no role filter, so it immediately takes a share of
client traffic — serving reads from an empty keyspace and accepting writes the
rest of the cluster knows nothing about. Neither self-heal trigger sees any of
it: the Status.PodUID check needs a lost Pod, etcdContainerStuck needs a
not-ready container.

Certain: etcd's own guard against strangers does not fire.
EtcdServer.Process rejects a raft message whose m.To is not the local member
ID (cannot process message to mismatch member) — the check that would normally
fence off a member from a different cluster. Member IDs are derived
deterministically (computeMemberID = sha1 over sorted peer URLs + cluster
token; NewClusterFromURLsMap passes a nil timestamp, so bootstrap IDs carry no
entropy), and nothing about this member changed, so its ID is identical to the
one the survivors still have on file. Their messages address it correctly and go
straight into raft. Same story one level up for the cluster ID and the peer
transport's X-Etcd-Cluster-ID check. The collision is precisely what removes
the loud failure.

Not established: how long the divergence lasts. Once those messages reach
raft, the surviving leader's higher term and mismatched log should pull the
member back into line, most likely via InstallSnapshot. If so the window is
short. That is expected behaviour rather than something observed here, and it is
deliberately not what this PR rests on — a brief window is still wrong reads
served to clients, and a write acknowledged in that window is silently discarded
when the snapshot lands, which is an acknowledged-write loss rather than a stale
read.

The case for =existing does not depend on the divergence being durable. It
rests on not issuing a bootstrap instruction to a member that is not
bootstrapping: =existing fails immediately and deterministically into a
designed, tested recovery path, instead of relying on raft to repair a state the
operator should never have created.

One likely objection: the operator already randomises the cluster token, so
shouldn't the IDs differ? deriveClusterToken (helpers.go:521) is
<namespace>-<cluster>-<uid>, and its entropy is the EtcdCluster's UID — so it
varies across incarnations, which is what stops a stale PVC from a previous
same-named cluster rejoining. A member rebooting inside one incarnation has the
same object, hence the same UID, hence the same token; spec.clusterToken is
copied from status.clusterToken at creation and never rewritten afterwards (no
assignment to it exists outside the two creation-time struct literals). The token
defends the boundary between clusters, not between boots.

The fix

Ask a phase question instead of an identity question. Emit =new only while
nothing yet says this cluster exists — two independent signals, both of which
must agree:

clusterState := "existing"
if member.Spec.Bootstrap && member.Status.MemberID == "" && !clusterFormed {
    clusterState = "new"
}

clusterFormed is cluster.Status.ClusterID != "", read in ensurePod via the
existing clusterFor helper and passed in, which keeps buildPod a pure
function of its arguments.

Requiring both is strictly safer than either alone and cannot misfire:
either signal being set proves the cluster formed, which makes an empty data dir
data loss rather than a pending bootstrap — and =existing failing loudly is
precisely the outcome we want, because #354 then replaces the member.

spec.bootstrap keeps its identity role. It still anchors seed discovery before
clusterID is latched, and it remains a permanent record of which member
bootstrapped; it simply no longer steers ongoing behaviour. Clearing the field
was the obvious alternative and was rejected: it records phase in spec (where
status.clusterID already answers the question with no write at all), it is
correct only if the clearing write is ordered right — a seed cleared before its
Pod ever starts would boot =existing against a self-only --initial-cluster
and never form the cluster — and it destroys the only record of which member was
the seed, which still explains that member's otherwise-anomalous
--initial-cluster.

Why it is safe to roll out

• No spec writes, no Pod restarts. ensurePod never re-templates an
  existing Pod, so running seeds keep their current argv and converge to
  =existing whenever their Pod is next recreated. Nothing moves on upgrade.
• Restore is unaffected. internal/agent/restore.go:140 calls
  snapshot.Restore with InitialCluster/InitialClusterToken, producing a
  complete data dir with a WAL — etcd ignores --initial-cluster-state entirely
  on that path.
• Pause/resume improves. A dormant seed's Pod is recreated on resume; today
  it returns with =new, so a PVC that lost data during the pause would
  silently re-bootstrap. =existing makes that loud.

Residual, deliberately not closed: a seed that formed the cluster while
both memberID and clusterID are still unset — a seconds-wide window — and
loses its data dir inside it would still re-bootstrap. That is today's behaviour
rather than a regression; closing it fully needs a persisted "has booted once"
marker, which is not worth the field.

Tests

TestBuildPod_InitialClusterState — table over the six meaningful states.
Note that nothing asserted this flag before, so this closes a live coverage
gap rather than adjusting existing expectations. The three seed-with-a-formed-
cluster rows fail against the old derivation.

TestEnsurePod_FormedClusterGivesSeedExistingState — the integration half:
proves ensurePod actually reads the parent cluster's clusterID rather than
just accepting a bool. Also fails against the old derivation.

TestSeedDataDirLossDoesNotRebootstrap (e2e, new file) — wipes rather than
corrupts the seed's data dir, because empty-vs-corrupt is exactly where the two
paths diverge and corruption only reaches the already-covered crash-loop. It
writes a sentinel key through the client Service first, then asserts that the
wiped seed is replaced and that the sentinel survives on every ready
member afterwards — checking all of them, so a single member answering out of an
empty store is caught rather than averaged away by whichever endpoint the Service
happens to pick.

Those two are the load-bearing assertions, and both are deterministic: a seed
that re-bootstrapped stays Ready and is therefore never replaced, so the wait
times out regardless of what raft does afterwards. The test additionally probes
the seed for the sentinel while waiting; catching it serving without the sentinel
fast-fails with the precise diagnosis instead of burning the full 15-minute
budget. That probe is opportunistic — missing it proves nothing, since the
surviving leader may already have snapshotted the member back into consistency —
but it cannot produce a false failure, as it fires only on a successful read
returning something other than the value we wrote.

The rest of the diff is mechanical: buildPod gained a parameter, so its ~24
existing test call sites pass false. Two namespace-bound e2e helpers were
generalised (readyMembersIsIn, seedMemberIn) so the new suite can use its own
namespace; the existing wrappers are unchanged for their callers.

Docs

concepts.md replaces the "Known gap: a seed that re-bootstraps" section added
in #354 with "Bootstrap state is a phase, not a member", documenting the derivation
and keeping the failure mode on record as the reason for it.

Verification

go build ./..., go vet -tags e2e ./test/e2e/ and go test ./controllers/
are green. Both new unit tests were confirmed to fail against the previous
derivation before the fix was applied.

The e2e has now run on CI's kind cluster and passes:
--- PASS: TestSeedDataDirLossDoesNotRebootstrap (246.70s), in a suite
finishing in 1018s against a 45m budget. The kamaji-datastore job runs the
whole test/e2e package unfiltered despite its name, so the new test is
covered there.

The wipe helper's verification step has since been tightened: the ephemeral
container's exit code now reflects whether the data dir is actually empty
afterwards. Previously the final ls -A always exited 0, so a partial wipe
would have been reported as success and surfaced much later as a seed that
simply starts fine. The check also fails closed when the data dir cannot be
read at all, which an emptiness test alone treats as "empty".

[gemini-code-assist]

Caution

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

[coderabbitai]

📝 Walkthrough

Walkthrough

Changes

Seed recovery state

Layer / File(s)	Summary
Derive initial cluster state controllers/etcdmember_controller.go, controllers/etcdmember_controller_test.go, controllers/restore_initcontainer_test.go	The controller reads cluster formation status and passes it to buildPod. Tests cover state selection and updated calls.
Document formation phases docs/concepts.md	The documentation defines bootstrap metadata, formation phases, and recovery with --initial-cluster-state=existing.
Validate seed self-healing test/e2e/member_selfheal_test.go, test/e2e/seed_rebootstrap_test.go	End-to-end tests wipe seed data, detect rebootstrap or replacement, and verify data retention across ready members.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant EtcdMemberController
  participant EtcdCluster
  participant buildPod
  participant EtcdPod
  participant SelfHealTest
  EtcdMemberController->>EtcdCluster: read cluster status
  EtcdMemberController->>buildPod: pass clusterFormed
  buildPod->>EtcdPod: set initial cluster state
  SelfHealTest->>EtcdPod: wipe seed data directory
  EtcdPod-->>SelfHealTest: report readiness and recovery

Loading

Possibly related PRs

• cozystack/etcd-operator#345: Changes seed self-healing behavior and shared end-to-end test helpers.
• cozystack/etcd-operator#354: Addresses bootstrap-seed recovery behavior used by this change.

Suggested reviewers: androndo

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%.
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.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: deriving --initial-cluster-state from cluster phase instead of seed identity.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/seed-initial-cluster-state

---

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.

[Andrey Kolkov (androndo)]

LGTM. The fix correctly reframes --initial-cluster-state as a phase question (Bootstrap && MemberID == "" && !clusterFormed) rather than a frozen identity property, so a seed that loses its data dir no longer re-bootstraps a fresh cluster.

Two safety claims verified:

• MemberID is never cleared (only assigned once at the latch), so a formed seed always forces =existing independently.
• The swallowed clusterFor error is safe: a transient failure collapses clusterFormed to false, which can only yield =new when MemberID == "" too — the early-bootstrap window where =new is already correct.

Coverage is complete: TestBuildPod_InitialClusterState (6-row table over all signal combinations), TestEnsurePod_FormedClusterGivesSeedExistingState, and the TestSeedDataDirLossDoesNotRebootstrap e2e. Docs updated in lockstep. build/vet/unit tests green.

[Andrey Kolkov (androndo)]

Branch review — LGTM

Reviewed fix/seed-initial-cluster-state @ da5d6d9 against fix/seed-member-not-special (stacked on #354, single commit).

What the change does

Replaces buildPod's identity-based derivation of --initial-cluster-state (previously new iff spec.bootstrap, frozen for the cluster's life) with a phase question: emit =new only when member.Spec.Bootstrap && member.Status.MemberID == "" && !clusterFormed, where clusterFormed comes from the parent's status.clusterID read in ensurePod. This stops a seed that lost its data dir from re-bootstrapping a fresh cluster.

No blocking findings

Both safety claims the fix rests on hold:

• MemberID is never cleared — only assigned once (the latch at etcdmember_controller.go:1105). A formed seed always has a non-empty MemberID, which independently forces =existing even if the clusterFormed signal is stale.
• The swallowed clusterFor error is safe (etcdmember_controller.go:485). A transient API error collapses clusterFormed to false, but that only yields =new when MemberID == "" too — the early-bootstrap window where =new is already correct.

Coverage

• TestBuildPod_InitialClusterState — 6-row table over both outcomes / all four signal combinations (closes a real prior gap; the flag was never asserted before).
• TestEnsurePod_FormedClusterGivesSeedExistingState — proves ensurePod actually reads the parent's clusterID.
• TestSeedDataDirLossDoesNotRebootstrap (e2e) — wipes the seed's data dir, asserts replacement + sentinel survival on all 3 members.
• ~24 existing buildPod call sites correctly pass false, reproducing prior behaviour exactly (no test drift).

Docs (concepts.md) updated in lockstep — the old "Known gap: a seed that re-bootstraps" section from #354 is replaced with "Bootstrap state is a phase, not a member", matching the code. No dangling anchor references.

Verification

go build ./..., go vet -tags e2e ./test/e2e/, and full go test ./controllers/ — all green. (The e2e needs a live cluster and was not run, consistent with the PR description.)

Nothing found outside the diff.

[coderabbitai]

Actionable comments posted: 4

🤖 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 `@controllers/etcdmember_controller.go`:
- Around line 628-647: Update the explanatory comment above clusterState in
buildPod to state that --initial-cluster-state=new is gated first by
member.Spec.Bootstrap, then by the two phase signals: empty
member.Status.MemberID and !clusterFormed. Preserve the existing code and
explicitly retain the scale-up behavior where non-bootstrap members use
"existing".

In `@docs/concepts.md`:
- Line 93: Update the derivation documentation for --initial-cluster-state,
including the “new” case at the referenced conditions section, to state that
spec.bootstrap=true is required in addition to the empty status.memberID and
absent status.clusterID conditions. Preserve the explanation that spec.bootstrap
identifies the bootstrap member while the status fields describe cluster phase.
- Line 181: Update the self-heal reference in the paragraph around “Either
signal being set...” to use the existing heading anchor `#crash-loop-self-heal`,
matching the “Crash-loop self-heal” section and other reference in the document.

In `@test/e2e/seed_rebootstrap_test.go`:
- Around line 187-226: Update the wipe-data command in wipeMemberDataDir so a
failed or incomplete rm operation causes the ephemeral container to exit
nonzero; do not let sync or ls mask the removal status. Preserve the existing
completion and exit-code validation in the waitFor callback, and ensure the
command explicitly verifies that /var/lib/etcd is empty before succeeding.

🪄 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: 27a45e03-0f47-4e86-9808-fe729b908f19

📥 Commits

Reviewing files that changed from the base of the PR and between fbadc9f and 2f82f25.

📒 Files selected for processing (6)

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