fix(controllers): report total member loss instead of spinning

[Andrei Kvapil (kvaps)]

Reworked after review. The first revision made re-bootstrap automatic. Review showed that to be the wrong call — it silently re-ran the immutable one-time spec.bootstrap.restore, left status.authEnabled stranded, kept Available=True through the whole event, and its "data is not discarded" claim was wrong (after adoption a member's PVC is controller-owned by it, with blockOwnerDeletion). This revision performs no recovery at all — it only reports.

Problem

An EtcdCluster whose EtcdMember CRs have all been removed — while status.clusterID is still latched — never recovers and never says so.

current(0) < desired routes into scaleUp(), whose endpoints derive from the (empty) member set, so the client factory fails before anything else:

INFO  scaling up   {"current": 0, "desired": 3}
ERROR cannot connect to etcd for scale-up  {"endpoints": [], "error": "etcdclient: no available endpoints"}

The reconcile requeues every 10s indefinitely; the only member-creating path is gated on ClusterID == "", which no longer holds. updateStatus is never reached on this path, so Available keeps its last pre-loss value — an empty cluster advertising a healthy quorum to anything gating on it (alerting, GitOps health, platform readiness).

Why this is not a corner case

This state is reachable straight from the project's own migration path, and the field data is unambiguous about how long it can hide.

On a cluster where cmd/etcd-migrate ran, the EtcdMember CRs of seven namespaces were subsequently removed. From that moment the operator was wedged in the loop above. managedFields on the EtcdCluster objects record it precisely: one entry from manager=etcd-migrate at migration time, and then nothing until an operator with this patch touched them twelve days later. No reconcile ever landed a status write. Throughout, every one of those clusters reported Available=True/QuorumAvailable, 2/3 members ready.

For most of that window the etcd pods were still running and serving — the data was there, and a snapshot would have saved it. Nobody took one, because nothing indicated a problem. The loss was only discovered when a routine node reboot removed the last running pods.

A terminal condition here would have surfaced the broken state on day one, while recovery was still possible. That is the entire value of this patch: it does not repair anything, it stops a destroyed data plane from looking healthy.

Fix

Park in a terminal AllMembersLost condition: Available=False with a message naming the event and the recovery options, Progressing=False, Degraded=True, and ReadyMembers zeroed so the scale subresource stops reporting a healthy count. Idempotent and parking, matching handleDeadlineExceeded.

Recovery is deliberately left to a human, because every automatic option is destructive: re-bootstrapping means a new identity on an empty data dir, and for a cluster created via spec.bootstrap.restore (immutable, documented as one-time) it would silently re-run that restore, rolling data back to create-time state and then reporting healthy.

Nothing on this path mutates clusterID, clusterToken, authEnabled, Pods or PVCs — a PVC that survived owner-ref GC stays recoverable by hand, and the evidence of what removed the members is preserved. That matters: several clusters entering this state at once points at a systemic cause worth finding, and auto-recovery would paper over it.

Guards. The branch requires both the active set and the raw member list to be empty, so a member whose deletion is still in flight (finalizer running MemberRemove) leaves the cluster to the existing in-flight-deletion wait instead of being declared lost mid-operation. A dormant member is woken as before, and a single surviving member keeps the cluster in its normal degraded path.

Scope. This fires for a converged cluster, whose ProgressDeadline has been cleared — the state the endless spin was observed in. A cluster that loses its members mid-reconcile still has its deadline armed and keeps parking under the existing DeadlineExceeded arm; that path is untouched, so deadline behaviour does not change. Both outcomes are terminal and visible — only the specificity of the reason differs.

Tests

• TestAllMembersLost_ParksTerminalWithoutDialing — parks instead of requeuing, Available=False/AllMembersLost, ReadyMembers=0, and asserts nothing is mutated or created: clusterID, clusterToken, authEnabled unchanged, no EtcdMember created, etcd never dialed. Fails on main with terminal state must park, not requeue; got {RequeueAfter:10s} — the bug itself.
• TestAllMembersLost_ArmedDeadlineStillParks — pins the scope boundary above: with an armed+expired deadline the cluster still parks and Available does not survive as True.
• TestAllMembersLost_NotTriggeredByDormantMember — a paused cluster's dormant member owns its PVC and its data; it must be woken, not declared lost.
• TestAllMembersLost_NotTriggeredWhileAMemberIsTerminating — the last member being deleted must not be mistaken for a total loss. Fails without the raw-list guard.
• TestAllMembersLost_NotTriggeredWhileAMemberSurvives — one surviving not-Ready member means degraded, not lost.

Tests that must not reach etcd use factoryFailingOnEmptyEndpoints, which errors on an empty endpoint list exactly like the real factory, so "never dialed" is asserted against production behaviour rather than a fake that always answers.

go test ./... and go vet ./... are green.

Docs

docs/concepts.md: AllMembersLost added to the Available and Progressing reason tables, and to the Degraded summary. docs/operations.md: a new Available=False/AllMembersLost section — how to find out what removed the members (several clusters at once = systemic cause), and the three recovery paths depending on whether PVCs, a snapshot, or neither survived.

Summary by CodeRabbit

• Bug Fixes
  • Prevented automated scale-up/recovery when all active members are gone while the cluster remains latched, now placing the cluster into terminal Available=False/AllMembersLost.
• Tests
  • Added end-to-end reconcile coverage for the AllMembersLost terminal scenario, including deadline-armed behavior and cases where a dormant/terminating/surviving member prevents terminal reporting.
• Documentation
  • Documented AllMembersLost in conditions and added an operations runbook describing terminal state meaning and manual recovery options.

[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

The controller detects a latched cluster with no EtcdMember resources, marks it terminal with AllMembersLost, preserves cluster identity, avoids automatic recovery, and documents manual recovery procedures. Tests cover deadline precedence and dormant, terminating, or surviving member exceptions.

Changes

Member-loss terminal handling

Layer / File(s)	Summary
Terminal detection and status handling controllers/etcdcluster_controller.go	Reconciliation routes total member loss to a handler that sets terminal availability, progress, degradation, and readiness conditions, updates status when needed, and does not requeue.
Terminal behavior validation controllers/etcdcluster_controller_test.go	Tests verify status parking, identity preservation, blocked etcd dialing, no member creation, deadline precedence, and non-triggering with dormant, terminating, or surviving members.
Terminal state documentation docs/concepts.md, docs/operations.md	Documentation defines the AllMembersLost conditions, diagnostic commands, and manual recovery paths.

Sequence Diagram(s)

sequenceDiagram
  participant Reconcile
  participant Handler
  participant ClusterStatus
  Reconcile->>Handler: detect latched ClusterID with zero members
  Handler->>ClusterStatus: set AllMembersLost conditions and ReadyMembers=0
  Handler-->>Reconcile: return without requeue

Loading

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

Suggested reviewers: lllamnyp, androndo

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
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.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main controller change: handling total member loss instead of looping.

✨ 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/rebootstrap-when-every-member-lost

---

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 `@controllers/etcdcluster_controller.go`:
- Around line 312-325: Move the empty-member recovery handled by the
active-member logic before the deadline-expiry check that invokes
handleDeadlineExceeded, or explicitly exempt that state from the gate. Ensure
expired deadlines still clear the stale ClusterID, reset progress, persist
status, and requeue; add a test covering empty members with a past deadline.

🪄 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: 7a01a8dd-cd35-4c90-92a0-32631c09acf0

📥 Commits

Reviewing files that changed from the base of the PR and between 8207298 and 14128aa.

📒 Files selected for processing (2)

• controllers/etcdcluster_controller.go
• controllers/etcdcluster_controller_test.go

[Andrey Kolkov (androndo)]

NOT LGTM

Reviewed fix/rebootstrap-when-every-member-lost → main (merge base 8207298): 29 added lines in controllers/etcdcluster_controller.go, 69 in the test. go vet ./... and go test ./controllers/... are green as claimed.

The diagnosis in the PR body is correct and well-written: current(0) < desired with status.clusterID latched really does dead-end in scaleUp → RequeueAfter: 10s forever (etcdcluster_controller.go:814-818), and the only member-creating path is gated on ClusterID == "". That part is solid.

The fix is not. It makes the most destructive recovery an etcd operator can perform — abandoning the cluster's identity and starting from an empty data dir — fully automatic, unsignalled, and un-opt-outable. I wrote four throwaway probe tests against this branch; all four confirmed concrete defects. Field corroboration also exists: a custom build of this branch running on a dev cluster took exactly this path and came up on fresh empty PVCs, wiping the application's data.

Blocking

1. Silently re-runs the immutable, documented-as-one-time spec.bootstrap.restore (verified)

bootstrap() sets Restore: restoreForSeed(cluster) (etcdcluster_controller.go:470), which returns cluster.Spec.Bootstrap.Restore unconditionally. Clearing status.clusterID re-enters that path. Probe output:

seed=test-75xjz bootstrap=true restore=&{Source:{S3:0x... PVC:<nil>}}
PROBE CONFIRMED: re-bootstrap seed re-runs the immutable one-time restore from "ancient-snapshot.db"

A cluster originally created by restoring snapshot X will, on total member loss, come back seeded from snapshot X — a silent point-in-time rollback to whatever the data looked like at create time, then reported healthy. This is worse than coming back empty, because stale-but-plausible data does not announce itself.

The user cannot prevent it: api/v1alpha2/etcdcluster_types.go:438-439 makes spec.bootstrap CEL-immutable post-create. And the project already understands this exact hazard — the rule at :440 rejects bootstrap.restore + medium=Memory precisely because "any seed Pod restart re-restores the snapshot — reverting writes". This PR reintroduces that failure mode at cluster scope, for PVC clusters, unattended.

2. Auth-enabled clusters are left permanently without auth, and end up strictly worse off than before (verified)

status.authEnabled is not reset. Probe:

after re-bootstrap: clusterID="" authEnabled=true

Two consequences:

• reconcileAuth short-circuits on if cluster.Status.AuthEnabled { return nil, nil } (:1156), so the root user is never provisioned and auth enable never runs against the brand-new etcd. The recovered cluster serves anonymously, forever.
• resolveEtcdCredentials gates on status.authEnabled, not spec.auth.enabled (etcd_client.go:113-118), so it hands root credentials to every dial against an etcd that has auth off. Those dials fail, discovery never latches a new clusterID, and the freshly-armed 600 s deadline then routes into the ClusterID == "" arm of handleDeadlineExceeded (:1990-2009) — terminal BootstrapFailed, "delete the cluster and recreate to recover".

That is a regression this PR introduces: before it, the cluster spun on a recoverable state with its clusterID intact; after it, the identity is destroyed and the cluster is parked in the one state the docs describe as unrecoverable.

3. Available=True/QuorumHealthy survives the whole event (verified)

Available=True/QuorumHealthy readyMembers=3 clusterID=""
PROBE CONFIRMED: cluster lost every member and re-bootstrapped, yet still advertises Available=True/QuorumHealthy (readyMembers=3)

updateStatus is never reached on this path, so Available and readyMembers keep their last pre-loss values through the loss, the re-bootstrap, and the empty seed coming up. Anything gating on Available — alerting, GitOps health, the platform's own readiness — sees green throughout a total data loss. updateStatus's own comment (:1399-1405) says this must not happen: "An empty cluster cannot serve a single request, so reporting Available=True/QuorumHealthy would mislead anything gating on the Available condition." The branch must set Available=False with a reason naming what happened, and zero readyMembers.

4. The safety argument in the comment is false in both directions, and the surviving-Pod case is a split brain

The comment claims "Data is not discarded — any PVC left behind by the old members stays put".

• Ordinary deletion: the PVC is data-<member.Name> and controller-owned by the EtcdMember (etcdmember_controller.go:360, :403), so it is cascade-GC'd with the member. Nothing is "left behind". The consoling "an operator can still restore from a snapshot" rests on a snapshot that may not exist.
• The only case where PVCs do survive is when the owner-ref GC did not run (--cascade=orphan, lost/restored CRs, deleted-and-recreated CRD) — and in that same case the old Pods survive too, still serving the real data, still carrying LabelCluster. Both Services select on LabelCluster alone (:1841-1858), so those pods stay in the <cluster> and <cluster>-client endpoint sets alongside the new empty seed. Clients then read either the populated etcd or the empty one at random.

Compounding it: status.clusterToken is not rotated, and deriveClusterToken is <ns>-<name>-<uid> (helpers.go:521-523), i.e. identical across incarnations. --initial-cluster-token is etcd's cross-cluster contamination guard; reusing it for a deliberately-unrelated new cluster in the same namespace removes exactly that protection.

At minimum the branch must refuse to fire while any Pod or PVC carrying LabelCluster: <cluster> still exists, and must rotate the token when it does.

5. The branch is unreachable in the state the PR says it is for (verified)

The inline comment justifies re-arming with "the progress deadline, which has long expired by the time we get here". That cannot be true where the branch runs, and where it is true the branch never runs:

clusterID="deadbeef" members=0 conds=[Progressing=False/DeadlineExceeded, Available=False/DeadlineExceeded]
PROBE CONFIRMED: with an armed+expired deadline the re-bootstrap branch is unreachable; ClusterID still "deadbeef", members=0

if !complete && deadlineExpired(cluster, now) at :198 short-circuits into handleDeadlineExceeded long before line 312. A converged cluster has ProgressDeadline == nil (cleared at :1482-1485), which is the only reason the branch fires at all. So: the re-arm is genuinely needed (otherwise bootstrap would be unbounded), but the stated reason is wrong, and a cluster that lost its members before converging — deadline armed, not yet cleared — gets no recovery at all. Fix the comment, and decide deliberately whether the mid-reconcile case is in or out of scope.

6. No safety gate, where the analogous destructive path has one

etcdmember_controller.go:1049-1064 replaces a crash-looping member only when r.clusterHasQuorumWithout(ctx, member) holds, explicitly "so a cluster-wide outage never cascades into mass deletion", with the finalizer's quorum gate as belt-and-braces. This PR performs a strictly more destructive action — discarding the cluster's identity — with no gate, no opt-in, and no persistent record. handleDeadlineExceeded's doc states the project's stance outright: "We never auto-pivot during bootstrap, and never silently in steady state." This is a silent auto-pivot in steady state.

7. Documentation drift

Every one of these now contradicts the code:

• docs/concepts.md:89 — "Once latched, discovery is never run again."
• docs/concepts.md:91 — "Once clusterID is set, the operator never re-reads spec.bootstrap for any decision — the seed is, from that point on, just a regular member."
• docs/operations.md:281 — "Restore is a first-bootstrap-only path… You cannot restore into an existing, already-bootstrapped cluster (spec.bootstrap is immutable post-create)."
• api/v1alpha2/etcdcluster_types.go:312-313 and :493-497 — "Consulted only at first bootstrap (while status.clusterID is unset)."
• docs/concepts.md Progressing-reason table (~line 424) is exhaustive and omits the new ReBootstrapping reason.
• Nothing in docs/operations.md tells an operator this can happen or what its data implications are.

8. Test is happy-path only in a heavily-tested area

TestScaleUp_EveryMemberLostReBootstraps asserts only that the latch clears and one seed appears. Missing, all cheap to add: a dormant-only cluster (active=[dormant], current=0 < desired) must not clear the latch; a cluster with one surviving not-Ready member must not; all members Terminating must not; plus the restore, auth, and condition cases above.

Also, factoryReturning(fe) never fails, so the test cannot reproduce the "no available endpoints" factory error that is the entire motivation. A factory that errors when endpoints is empty would make the test mirror production and would let the len(fe.addCalls) == 0 assertion mean something stronger.

On the approach

Worth settling before iterating on the code. Two questions the PR does not answer:

What deleted the members? Several EtcdClusters wedging identically points at a systemic cause — a GC misfire, a Flux prune, a CRD replacement, a bad cleanup script. If the operator or the platform did it, that is the bug to fix; auto-recovery here would only paper over it while destroying the evidence and the data.

Is automatic the right answer? The rest of this controller deliberately routes unrecoverable states to loud terminal conditions and leaves the destructive call to a human (BootstrapFailed, DeadlineExceeded, "delete and recreate" in five places across the docs). The smallest fix that solves the reported symptom — an operator with no idea why the cluster is dead and no signal in status — is a terminal Available=False / AllMembersLost condition whose message says what happened and what to do (restore from a snapshot, or delete and recreate). That is a few lines, has no data-loss surface, and closes the "the status gave no hint that the data plane was gone" complaint that the PR body actually leads with.

If unattended re-bootstrap is genuinely wanted, it needs to be opt-in (a spec field or annotation) and to carry, at minimum: a refusal while any LabelCluster Pod or PVC survives; status.authEnabled reset and status.clusterToken rotated; spec.bootstrap.restore deliberately not re-applied; Available driven false with an accurate reason; and a durable status marker so the event is auditable after the fact.

Reproducing the probes

Each is 20-30 lines: copy TestScaleUp_EveryMemberLostReBootstraps and vary one thing — add Spec.Bootstrap.Restore and assert the recreated seed's Spec.Restore == nil; set Status.AuthEnabled: true and assert it flips false; set ProgressDeadline an hour in the past and assert the latch clears; pre-seed Available=True/QuorumHealthy and assert it does not survive. Each fails on the current branch.

Unrelated: the red E2E is not this PR's fault

TestPVCMemberCrashLoopSelfHeal failing in run 30297109488 is a pre-existing flake in the test itself, not a regression here — this branch touches neither etcdmember_controller.go nor any e2e file. The same test failed identically (same 940.9 s timeout) on chore/codeowners, a branch whose entire diff is 8 lines of .github/CODEOWNERS, and then passed on a re-run of that same branch.

Root cause: the test picks its victim as original[0], the alphabetically-first member (kube.List returns name-sorted items), while member names are apiserver-assigned random suffixes. Roughly one run in three the seed sorts first, and the seed is permanently excluded from self-heal by the !member.Spec.Bootstrap gate at etcdmember_controller.go:1058 (Spec.Bootstrap is set at creation and never cleared) — so the test waits out its full 15 minutes. Evidence across three runs:

Run	Members	Added as learner	Seed	Victim	Result
this PR	p6bvx, v9kr8, vtm7h	v9kr8, vtm7h	p6bvx	p6bvx	FAIL 940 s
chore/codeowners	9xf86, bst2x, fph75	bst2x, fph75	9xf86	9xf86	FAIL 940 s
chore/codeowners re-run	—	—	not the victim	h7gtw	PASS 250 s

Fixed separately in #345 (pick a non-bootstrap victim). Worth noting what the flake incidentally surfaces, since it is adjacent to this PR's subject: a seed member whose data dir is destroyed never recovers — quorum among the other two holds, but the seed crash-loops forever, excluded from self-heal, with Spec.Bootstrap never cleared. That is a narrower, well-bounded gap in the same problem space, and a better target than unattended re-bootstrap.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

docs/concepts.md (1)

420-432: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Degraded summary just below is now incomplete.

Line 434 enumerates Degraded=True as only Available=True/QuorumAvailable or Available=False/QuorumLost, but handleAllMembersLost also sets Degraded=True/AllMembersLost. Worth extending that sentence so alerting readers get the full set.

🤖 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 `@docs/concepts.md` around lines 420 - 432, Update the Degraded summary near
the Progressing table to include the AllMembersLost reason emitted by
handleAllMembersLost, alongside the existing QuorumAvailable and QuorumLost
cases. Keep the documented behavior and alerting guidance otherwise unchanged.

🧹 Nitpick comments (1)

controllers/etcdcluster_controller_test.go (1)

1017-1082: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider a case for members that exist but are terminating.

The suite pins dormant and surviving-member exemptions but not the DeletionTimestamp-set window, which is the boundary flagged on controllers/etcdcluster_controller.go Line 240. A second-reconcile idempotency assertion (no further status write, still parked) would also be cheap here.

🤖 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 `@controllers/etcdcluster_controller_test.go` around lines 1017 - 1082, The
test coverage for TestAllMembersLost_ParksTerminalWithoutDialing should include
EtcdMember resources that still exist but have DeletionTimestamp set, verifying
this terminating-member window follows the intended exemption rather than being
treated as all members lost. Add a second Reconcile assertion for idempotency,
confirming the cluster remains parked with no requeue and no additional status
mutation.

🤖 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/etcdcluster_controller.go`:
- Around line 240-242: Update the AllMembersLost branch in the controller
reconcile flow to require both len(active) == 0 and the raw member list length
== 0 before calling handleAllMembersLost. Preserve the existing desired and
ClusterID conditions so clusters with terminating members follow the
deletion-wait path instead of being parked.

---

Outside diff comments:
In `@docs/concepts.md`:
- Around line 420-432: Update the Degraded summary near the Progressing table to
include the AllMembersLost reason emitted by handleAllMembersLost, alongside the
existing QuorumAvailable and QuorumLost cases. Keep the documented behavior and
alerting guidance otherwise unchanged.

---

Nitpick comments:
In `@controllers/etcdcluster_controller_test.go`:
- Around line 1017-1082: The test coverage for
TestAllMembersLost_ParksTerminalWithoutDialing should include EtcdMember
resources that still exist but have DeletionTimestamp set, verifying this
terminating-member window follows the intended exemption rather than being
treated as all members lost. Add a second Reconcile assertion for idempotency,
confirming the cluster remains parked with no requeue and no additional status
mutation.

🪄 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: ed960b7d-6527-4dd7-addf-e6ecf39ec86b

📥 Commits

Reviewing files that changed from the base of the PR and between 14128aa and 811dc2c.

📒 Files selected for processing (4)

• controllers/etcdcluster_controller.go
• controllers/etcdcluster_controller_test.go
• docs/concepts.md
• docs/operations.md

[Andrei Kvapil (kvaps)]

Thanks — the review was right on every count, and the branch has been reworked accordingly. All automatic recovery is gone; the change now only reports.

What it does: parks the cluster in a terminal AllMembersLost condition (Available=False with the event and the recovery options named, Progressing=False, Degraded=True, ReadyMembers zeroed). Nothing on the path mutates clusterID, clusterToken, authEnabled, Pods or PVCs.

Against the blockers:

1. spec.bootstrap.restore re-run — gone with bootstrap(); nothing re-enters that path.
2. authEnabled stranded — nothing touches it; the cluster keeps its identity and its auth state.
3. Available=True/QuorumHealthy surviving — inverted: driving Available false with an accurate reason is now the entire point of the change. ReadyMembers is zeroed alongside, since it is the scale subresource's status path.
4. False safety claim / split brain / token reuse — the claim is gone from the comment; no volumes, pods or tokens are touched, so neither split brain nor token reuse can arise here.
5. Branch unreachable where the PR said it was — decided deliberately: the branch now sits after the deadline check, so it fires for a converged cluster (ProgressDeadline cleared) — the state the endless spin was actually observed in. A cluster that loses its members mid-reconcile keeps parking under the existing DeadlineExceeded arm, untouched. TestAllMembersLost_ArmedDeadlineStillParks pins that boundary, and the misleading comment about re-arming is gone.
6. No safety gate — no destructive action left to gate. The guards that remain are about not mis-reporting: dormant members are woken as before, a surviving not-Ready member keeps the normal degraded path, and a member still terminating is left to the in-flight-deletion wait (that last one came from CodeRabbit on the reworked branch — active alone would have declared a mid-flight scale-down a total loss).
7. Docs drift — AllMembersLost added to the Available, Progressing and Degraded tables in concepts.md, plus a new Available=False/AllMembersLost section in operations.md covering how to find what removed the members and the three recovery paths (PVCs survived / snapshot exists / neither).
8. Happy-path-only test — five tests now: the terminal case asserting nothing is mutated or created and etcd is never dialed, the armed-deadline boundary, and three negatives (dormant, terminating, surviving member). factoryFailingOnEmptyEndpoints errors on an empty endpoint list exactly like the real factory, so "never dialed" is asserted against production behaviour.

On the two framing questions:

What deleted the members — investigated and written up: postmortem. Short version: the volumes were reclaimed as a cascade behind EtcdMembers that something removed by hand after an in-place migration. EtcdCluster was never deleted and no Helm rollback happened, so the automation is ruled out; the actual actor could not be identified because the apiserver audit log rotates within hours at its configured limits. #347 addresses the mechanism that made a member deletion fatal to data in the first place.

Is automatic the right answer — no, and that is precisely what this revision concedes. Worth adding one piece of field data for the "loud terminal condition" case: managedFields on the affected EtcdClusters show one entry from the migration tool and then nothing until a patched operator touched them twelve days later. No status write ever landed, and all of them reported Available=True/QuorumAvailable, 2/3 members ready throughout. For most of that window the pods were still serving and a snapshot would have saved the data — nobody took one because nothing indicated a problem.

[Timofei Larkin (lllamnyp)]

Changes requested — direction, not implementation.

On #347 I've proposed solving the member-deletion problem at the API boundary instead of in the controllers: a ValidatingAdmissionPolicy denying DELETE on etcdmembers to anything but the operator's ServiceAccount, the garbage collector, and the namespace controller (break-glass via an explicit annotation), plus finally documenting that deleting an EtcdMember destroys its data volume. See the review there for the full argument.

If that direction goes forward, it voids this PR: with member deletion guarded at admission, "every EtcdMember gone while status.clusterID is still latched" stops being reachable through any supported path, and this branch would be a terminal condition for a state the platform prevents. I'd rather not carry both.

To be clear about what I'm not disputing: the diagnosis is correct, the report-only rework was the right call, and the test suite is solid. If the admission-guard approach is rejected on #347, I'm happy to come back to this — but it would then need one fix first: the branch can false-positive during cmd/etcd-migrate runs. The tool prefills status.clusterID before creating the EtcdMember CRs (cmd/etcd-migrate/apply_adopt.go, step 1 vs. step 2), and the members it creates carry no owner reference to the EtcdCluster (internal/migrate/adopt.go), so a reconcile landing in that window parks a freshly-adopted, healthy cluster in AllMembersLost — and since the handler returns without a requeue and the Owns(&EtcdMember{}) watch only maps owner-referenced members, nothing wakes it until the manager's resync. Either the park needs a slow-cadence requeue instead of none, or the migrator needs to stamp the same controller owner reference the operator does.

Parking this pending the outcome on #347.

[Andrei Kvapil (kvaps)]

Agreed — parking this behind #348, which implements the admission guard you proposed. If that lands, this PR is for a state the platform prevents, and I'd rather not carry both either.

Two things worth recording before it goes quiet.

The migrate-window false positive is real, but narrower than it looks. You're right about the ordering — apply_adopt.go creates the EtcdCluster with prefilled status.clusterID in step 1, before any EtcdMember exists, and those members carry no owner reference to the cluster, so Owns(&EtcdMember{}) would not wake a parked cluster either. What limits it: the tool refuses to run unless both operator Deployments are scaled to zero — main.go prints ✓ both operator Deployments are down, and skipping that check needs an explicit --skip-controller-check. So no reconcile lands in the window on a normal run; it takes misuse or an operator that came back up mid-migration. Still worth fixing rather than arguing about, and the fix is cheap: a slow-cadence requeue instead of none. That is also the right shape independently — a terminal park with no requeue cannot notice that a human repaired the state, which is a bad property for a condition whose whole purpose is to be seen and acted on.

What I'd keep from this, if anything. The signal was worth something beyond the deletion accident: on the affected clusters managedFields show one entry from the migration tool and then nothing for twelve days, while every one of them reported Available=True/QuorumAvailable, 2/3 members ready with zero members. Admission control closes the path that got them there; it does not make the operator notice when it is spinning on an impossible scaleUp. If that ever bites again through a route the guard doesn't cover — an operator bug, a break-glass deletion, a member-CR restore from a stale backup — the diagnosis is the same no available endpoints loop with a green status. Not arguing to land it now; noting the gap so it's on record if we see it a second time.

Closing thought on sequencing: #348 is small and reversible, so it should go first regardless of what happens to this branch and #347.