From 811dc2cd0c762c95e8067dc08d6745aa0b9eda4c Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Tue, 28 Jul 2026 15:09:58 +0200 Subject: [PATCH 1/2] fix(controllers): report total member loss instead of spinning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A cluster 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 with "etcdclient: no available endpoints" and the reconcile requeues every 10s indefinitely. updateStatus is never reached on that path, so Available keeps its last pre-loss value — an empty cluster advertising a healthy quorum to anything gating on it. Park in a terminal AllMembersLost condition instead: 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. Recovery is deliberately left to a human. Rebuilding automatically would mean adopting a new identity on an empty data dir, or — for a cluster created via spec.bootstrap.restore, which is immutable and documented as one-time — silently re-running that restore and rolling data back to create-time state. Nothing on this path mutates clusterID, clusterToken, authEnabled, Pods or PVCs, so surviving volumes stay recoverable by hand and the evidence of what removed the members is preserved. 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 continues to park under the existing DeadlineExceeded arm, unchanged by this commit. Assisted-By: Claude Signed-off-by: Andrei Kvapil --- controllers/etcdcluster_controller.go | 79 +++++++ controllers/etcdcluster_controller_test.go | 233 +++++++++++++++++++++ docs/concepts.md | 3 +- docs/operations.md | 21 ++ 4 files changed, 335 insertions(+), 1 deletion(-) diff --git a/controllers/etcdcluster_controller.go b/controllers/etcdcluster_controller.go index 32ab7632..85635825 100644 --- a/controllers/etcdcluster_controller.go +++ b/controllers/etcdcluster_controller.go @@ -214,6 +214,33 @@ func (r *EtcdClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) desired := cluster.Status.Observed.Replicas + // ── Total member loss ───────────────────────────────────────────── + // A latched ClusterID with no EtcdMember CRs left at all means the data + // plane is gone — something removed every member, and each member's PVC + // is controller-owned by it, so the data went with them. There is no + // recovery this controller can perform: rebuilding would mean adopting a + // new identity on an empty data dir, and scaleUp cannot help either, + // because MemberAdd needs a live member to dial and there is none. + // + // Without this branch the reconcile falls through to scaleUp and spins on + // "etcdclient: no available endpoints" every 10s forever, while the + // status keeps advertising its last pre-loss values — an empty cluster + // reporting Available=True/QuorumHealthy. Park in a terminal condition + // instead: name what happened, drive Available false, and leave the + // destructive call (restore a snapshot, or delete and recreate) to a + // human, as every other unrecoverable state in this controller does. + // + // Scope: this fires for a converged cluster, whose ProgressDeadline has + // been cleared — the state the endless spin was observed in. A cluster + // that loses every member mid-reconcile still has its deadline armed and + // is handled above by handleDeadlineExceeded, which parks it under the + // generic DeadlineExceeded reason. Both are terminal and visible; only + // the specificity of the reason differs, and reusing the existing arm + // keeps this change from altering deadline behaviour. + if cluster.Status.ClusterID != "" && desired > 0 && len(active) == 0 { + return r.handleAllMembersLost(ctx, cluster) + } + // ── Bootstrap ────────────────────────────────────────────────────── // Bootstrap creates a single-member etcd cluster (member -0 only) with // --initial-cluster-state=new. Once ClusterID is latched, scale-up adds @@ -1951,6 +1978,58 @@ func (r *EtcdClusterReconciler) SetupWithManager(mgr ctrl.Manager) error { // that happens we sit in DeadlineExceeded. // // We never auto-pivot during bootstrap, and never silently in steady state. +// handleAllMembersLost parks a cluster whose EtcdMember CRs have all been +// removed while its ClusterID is still latched. +// +// This is a diagnosis, not a repair. The controller deliberately performs no +// recovery: every option — re-bootstrapping onto a fresh data dir, or +// re-running the one-time spec.bootstrap.restore — either discards data or +// silently rolls it back to a snapshot, and both would run unattended against +// a cluster whose members may have been removed by mistake. Nothing here +// mutates ClusterID, ClusterToken, AuthEnabled, Pods or PVCs, so a surviving +// PVC (owner-ref GC skipped, e.g. --cascade=orphan) can still be recovered by +// hand, and the evidence of what happened stays intact. +// +// ReadyMembers is zeroed alongside the conditions: it is the scale +// subresource's status path, and leaving the pre-loss count there would keep +// anything reading it — HPA-style tooling, dashboards, GitOps health — seeing +// a healthy cluster. +// +// Idempotent and parking, matching handleDeadlineExceeded: write only when +// something actually changed, then return without a requeue and let the watch +// wake the controller when a human intervenes. +func (r *EtcdClusterReconciler) handleAllMembersLost( + ctx context.Context, + cluster *lll.EtcdCluster, +) (ctrl.Result, error) { + log := log.FromContext(ctx) + + changed := setClusterCondition(cluster, lll.ClusterAvailable, metav1.ConditionFalse, "AllMembersLost", + fmt.Sprintf("all %d members are gone; no EtcdMember remains to serve or to add a member against. "+ + "Recover by restoring from a snapshot, or delete the cluster and recreate it", + cluster.Status.Observed.Replicas)) + changed = setClusterCondition(cluster, lll.ClusterProgressing, metav1.ConditionFalse, "AllMembersLost", + "every member was lost; manual recovery required") || changed + changed = setClusterCondition(cluster, lll.ClusterDegraded, metav1.ConditionTrue, "AllMembersLost", + "no members remain") || changed + if cluster.Status.ReadyMembers != 0 { + cluster.Status.ReadyMembers = 0 + changed = true + } + + if changed { + log.Error(nil, "cluster lost every member; manual recovery required", + "clusterID", cluster.Status.ClusterID, "desired", cluster.Status.Observed.Replicas) + if err := r.Status().Update(ctx, cluster); err != nil { + if errors.IsConflict(err) { + return ctrl.Result{Requeue: true}, nil + } + return ctrl.Result{}, err + } + } + return ctrl.Result{}, nil +} + func (r *EtcdClusterReconciler) handleDeadlineExceeded( ctx context.Context, cluster *lll.EtcdCluster, diff --git a/controllers/etcdcluster_controller_test.go b/controllers/etcdcluster_controller_test.go index dcf8bc38..e7bdb897 100644 --- a/controllers/etcdcluster_controller_test.go +++ b/controllers/etcdcluster_controller_test.go @@ -12,6 +12,7 @@ package controllers import ( "context" + "crypto/tls" "errors" "fmt" "reflect" @@ -36,6 +37,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" lll "github.com/cozystack/etcd-operator/api/v1alpha2" ) @@ -961,6 +963,237 @@ func TestScaleUp_WaitsForInFlightDeletion(t *testing.T) { } } +// findCond returns the named condition, or nil when it is absent. +func findCond(conds []metav1.Condition, condType string) *metav1.Condition { + for i := range conds { + if conds[i].Type == condType { + return &conds[i] + } + } + return nil +} + +// factoryFailingOnEmptyEndpoints mirrors production: the real factory cannot +// build a client without at least one endpoint to dial. Tests that must not +// reach etcd use this so the assertion means "never dialed", not "dialed a +// fake that happens to answer". +func factoryFailingOnEmptyEndpoints(c EtcdClusterClient) EtcdClientFactory { + return func(_ context.Context, endpoints []string, _ *tls.Config, _, _ string) (EtcdClusterClient, error) { + if len(endpoints) == 0 { + return nil, errors.New("etcdclient: no available endpoints") + } + return c, nil + } +} + +// allMembersLostCluster builds a cluster that has latched a ClusterID and then +// lost every EtcdMember — the state this suite exercises. +func allMembersLostCluster(t *testing.T, deadline *metav1.Time) *lll.EtcdCluster { + t.Helper() + return &lll.EtcdCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "ns"}, + Spec: lll.EtcdClusterSpec{ + Replicas: ptrInt32(3), + Version: "3.5.17", + Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, + }, + Status: lll.EtcdClusterStatus{ + ClusterToken: "ns-test-x", + ClusterID: "deadbeef", + AuthEnabled: true, + ReadyMembers: 3, + Observed: &lll.ObservedClusterSpec{ + Replicas: 3, Version: "3.5.17", Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, + }, + ProgressDeadline: deadline, + Conditions: []metav1.Condition{ + {Type: lll.ClusterAvailable, Status: metav1.ConditionTrue, Reason: "QuorumHealthy", + LastTransitionTime: metav1.Now()}, + }, + }, + } +} + +// TestAllMembersLost_ParksTerminalWithoutDialing covers a cluster whose +// EtcdMember CRs have all been removed while status.clusterID is still +// latched. +// +// Before this branch existed the reconcile fell through to scaleUp, whose +// endpoints derive from the (empty) member set, so the client factory failed +// with "no available endpoints" and the reconcile requeued every 10s forever +// — while Available stayed at its last pre-loss value, advertising a healthy +// quorum for a cluster with nothing running. +// +// The controller cannot repair this state (rebuilding means a new identity on +// an empty data dir), so it parks in a terminal condition and leaves the +// destructive call to a human. Asserted here: Available goes false with a +// reason naming the event, ReadyMembers is zeroed, etcd is never dialed, and +// — critically — nothing is mutated or created. +func TestAllMembersLost_ParksTerminalWithoutDialing(t *testing.T) { + ctx := context.Background() + // ProgressDeadline nil: a converged cluster clears it, which is the + // state this branch is normally reached in. + cluster := allMembersLostCluster(t, nil) + c, _ := newTestClient(t, cluster) + fe := newFakeEtcd(0xdeadbeef) + r := &EtcdClusterReconciler{Client: c, Scheme: testScheme(t), EtcdClientFactory: factoryFailingOnEmptyEndpoints(fe)} + + res, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: "test", Namespace: "ns"}}) + if err != nil { + t.Fatalf("Reconcile: %v", err) + } + if res.RequeueAfter != 0 || res.Requeue { + t.Fatalf("terminal state must park, not requeue; got %+v", res) + } + + got := &lll.EtcdCluster{} + if err := c.Get(ctx, types.NamespacedName{Name: "test", Namespace: "ns"}, got); err != nil { + t.Fatalf("Get: %v", err) + } + avail := findCond(got.Status.Conditions, lll.ClusterAvailable) + if avail == nil || avail.Status != metav1.ConditionFalse || avail.Reason != "AllMembersLost" { + t.Fatalf("Available must go False/AllMembersLost; got %+v", avail) + } + if got.Status.ReadyMembers != 0 { + t.Fatalf("ReadyMembers must be zeroed; got %d", got.Status.ReadyMembers) + } + // Nothing destructive, and nothing that changes the cluster's identity: + // a surviving PVC must stay recoverable by hand, and the evidence of + // what happened must stay intact. + if got.Status.ClusterID != "deadbeef" { + t.Fatalf("ClusterID must not be discarded; got %q", got.Status.ClusterID) + } + if got.Status.ClusterToken != "ns-test-x" { + t.Fatalf("ClusterToken must not be rotated; got %q", got.Status.ClusterToken) + } + if !got.Status.AuthEnabled { + t.Fatalf("AuthEnabled must not be cleared") + } + members := &lll.EtcdMemberList{} + if err := c.List(ctx, members, client.InNamespace("ns")); err != nil { + t.Fatalf("List: %v", err) + } + if len(members.Items) != 0 { + t.Fatalf("no member may be created on this path; got %d", len(members.Items)) + } + if len(fe.addCalls) > 0 { + t.Fatalf("etcd must never be dialed with zero members; got %v", fe.addCalls) + } +} + +// TestAllMembersLost_ArmedDeadlineStillParks pins the scope boundary. A +// cluster that loses every member *before* converging still has its +// ProgressDeadline armed and is claimed by handleDeadlineExceeded, which parks +// it under the generic DeadlineExceeded reason rather than AllMembersLost. +// +// That is deliberate — routing it here instead would change deadline +// behaviour, which this fix has no reason to touch. What matters, and what +// this asserts, is the property the bug was about: whichever arm claims the +// cluster, the reconcile parks instead of spinning on scaleUp forever, and +// Available does not survive as True. +func TestAllMembersLost_ArmedDeadlineStillParks(t *testing.T) { + ctx := context.Background() + expired := metav1.NewTime(metav1.Now().Add(-time.Hour)) + cluster := allMembersLostCluster(t, &expired) + c, _ := newTestClient(t, cluster) + fe := newFakeEtcd(0xdeadbeef) + r := &EtcdClusterReconciler{Client: c, Scheme: testScheme(t), EtcdClientFactory: factoryFailingOnEmptyEndpoints(fe)} + + res, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: "test", Namespace: "ns"}}) + if err != nil { + t.Fatalf("Reconcile: %v", err) + } + if res.RequeueAfter != 0 || res.Requeue { + t.Fatalf("must park rather than spin; got %+v", res) + } + + got := &lll.EtcdCluster{} + if err := c.Get(ctx, types.NamespacedName{Name: "test", Namespace: "ns"}, got); err != nil { + t.Fatalf("Get: %v", err) + } + avail := findCond(got.Status.Conditions, lll.ClusterAvailable) + if avail == nil || avail.Status != metav1.ConditionFalse { + t.Fatalf("Available must not survive as True through total member loss; got %+v", avail) + } + if len(fe.addCalls) > 0 { + t.Fatalf("etcd must never be dialed with zero members; got %v", fe.addCalls) + } +} + +// TestAllMembersLost_NotTriggeredByDormantMember: a paused-then-resumed +// cluster has current==0 with a dormant CR parked alongside. That member still +// owns its PVC and its data — waking it is the correct recovery, and reporting +// the cluster as unrecoverable would be wrong. +func TestAllMembersLost_NotTriggeredByDormantMember(t *testing.T) { + ctx := context.Background() + cluster := allMembersLostCluster(t, nil) + dormant := &lll.EtcdMember{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-dormant", Namespace: "ns", Labels: memberLabels("test", "test-dormant"), + }, + Spec: lll.EtcdMemberSpec{ClusterName: "test", InitialCluster: "x", Bootstrap: true, Dormant: true}, + } + if err := controllerutil.SetControllerReference(cluster, dormant, testScheme(t)); err != nil { + t.Fatalf("SetControllerReference: %v", err) + } + c, _ := newTestClient(t, cluster, dormant) + fe := newFakeEtcd(0xdeadbeef) + r := &EtcdClusterReconciler{Client: c, Scheme: testScheme(t), EtcdClientFactory: factoryFailingOnEmptyEndpoints(fe)} + + if _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: "test", Namespace: "ns"}}); err != nil { + t.Fatalf("Reconcile: %v", err) + } + + got := &lll.EtcdCluster{} + if err := c.Get(ctx, types.NamespacedName{Name: "test", Namespace: "ns"}, got); err != nil { + t.Fatalf("Get: %v", err) + } + if avail := findCond(got.Status.Conditions, lll.ClusterAvailable); avail != nil && avail.Reason == "AllMembersLost" { + t.Fatalf("a dormant member is recoverable; must not report AllMembersLost") + } + gotMember := &lll.EtcdMember{} + if err := c.Get(ctx, types.NamespacedName{Name: "test-dormant", Namespace: "ns"}, gotMember); err != nil { + t.Fatalf("Get member: %v", err) + } + if gotMember.Spec.Dormant { + t.Fatalf("dormant member should have been woken instead") + } +} + +// TestAllMembersLost_NotTriggeredWhileAMemberSurvives: one member left, not +// Ready (e.g. crash-looping). The data plane is degraded, not gone — the +// existing wait-for-Ready path owns this, and a terminal condition here would +// park a cluster that can still recover on its own. +func TestAllMembersLost_NotTriggeredWhileAMemberSurvives(t *testing.T) { + ctx := context.Background() + cluster := allMembersLostCluster(t, nil) + survivor := &lll.EtcdMember{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-alive", Namespace: "ns", Labels: memberLabels("test", "test-alive"), + }, + Spec: lll.EtcdMemberSpec{ClusterName: "test", InitialCluster: "x"}, + Status: lll.EtcdMemberStatus{PodName: "test-alive", MemberID: "abc"}, + } + if err := controllerutil.SetControllerReference(cluster, survivor, testScheme(t)); err != nil { + t.Fatalf("SetControllerReference: %v", err) + } + c, _ := newTestClient(t, cluster, survivor) + fe := newFakeEtcd(0xdeadbeef) + r := &EtcdClusterReconciler{Client: c, Scheme: testScheme(t), EtcdClientFactory: factoryFailingOnEmptyEndpoints(fe)} + + if _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: "test", Namespace: "ns"}}); err != nil { + t.Fatalf("Reconcile: %v", err) + } + + got := &lll.EtcdCluster{} + if err := c.Get(ctx, types.NamespacedName{Name: "test", Namespace: "ns"}, got); err != nil { + t.Fatalf("Get: %v", err) + } + if avail := findCond(got.Status.Conditions, lll.ClusterAvailable); avail != nil && avail.Reason == "AllMembersLost" { + t.Fatalf("a surviving member means the cluster is degraded, not lost") + } +} + // TestScaleDown_PicksMostRecentlyCreatedVictim: with apiserver-assigned // names there is no ordinal to scale down by; victim selection sorts by // CreationTimestamp (newest first). This naturally retires the most diff --git a/docs/concepts.md b/docs/concepts.md index 6cd535ba..b46d11aa 100644 --- a/docs/concepts.md +++ b/docs/concepts.md @@ -417,6 +417,7 @@ The cluster surfaces three conditions: `Available`, `Progressing`, `Degraded`. T | `False` | `ClusterUnreachable` | Discovery couldn't dial etcd (DNS failure, network partition, etcd not yet listening). | | `False` | `BootstrapFailed` | Deadline expired before `clusterID` was latched. Terminal — recovery is delete and recreate. | | `False` | `DeadlineExceeded` | Deadline expired after bootstrap. Terminal — recovery is to edit spec. | +| `False` | `AllMembersLost` | Every `EtcdMember` is gone while `clusterID` is still latched. Terminal — the operator performs no automatic recovery; restore from a snapshot, or delete and recreate. | `Progressing` distinguishes "actively reconciling" from "we hit a wall": @@ -428,7 +429,7 @@ The cluster surfaces three conditions: `Available`, `Progressing`, `Degraded`. T | `True` | `RetryAfterDeadline` | Deadline-exceeded recovery: user edited spec after a steady-state deadline. | | `False` | `Reconciled` | At steady state with the current `observed`. | | `False` | `Paused` | Same as Available; emitted when `desired==0`. | -| `False` | `BootstrapFailed` / `DeadlineExceeded` | Terminal states; see Available. | +| `False` | `BootstrapFailed` / `DeadlineExceeded` / `AllMembersLost` | Terminal states; see Available. | `Degraded` is `True` whenever `Available=True/QuorumAvailable` (partial outage) or `Available=False/QuorumLost`. `False` in healthy or paused states. In other words, `Degraded` means "the cluster is not delivering its full intended capacity right now"; reading `Degraded` alone tells an alerting layer whether to page someone. diff --git a/docs/operations.md b/docs/operations.md index 48649db8..a9409432 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -179,6 +179,27 @@ kubectl edit etcdcluster.etcd-operator.cozystack.io -n The next reconcile notices `spec != observed`, treats your edit as the intervention signal, snapshots the new spec into `observed`, sets a fresh deadline, and resumes. +### `Available=False/AllMembersLost` + +Terminal. Every `EtcdMember` of the cluster is gone while `status.clusterID` is still latched — the data plane no longer exists. Each member's PVC is controller-owned by its `EtcdMember`, so unless owner-ref GC was skipped (`--cascade=orphan`, a CRD replacement, restored-from-backup CRs) the data went with them. + +The operator deliberately performs **no** automatic recovery here, and nothing on this path mutates `clusterID`, `clusterToken`, `authEnabled`, Pods or PVCs. Rebuilding would mean bringing the cluster back under a new identity on an empty data dir — or, for a cluster created via `spec.bootstrap.restore`, silently re-running that one-time restore and rolling the data back to the snapshot it was created from. Both are decisions for a human, and doing either automatically would also destroy the evidence of what removed the members. + +First find out what happened — several clusters entering this state at once points at a systemic cause (a GC misfire, a GitOps prune, a CRD replacement, a cleanup script) that will repeat: + +```sh +kubectl get etcdmember.etcd-operator.cozystack.io -n # expected: none +kubectl get pvc -n -l etcd-operator.cozystack.io/cluster= +kubectl get pods -n -l etcd-operator.cozystack.io/cluster= +kubectl get events -n --sort-by=.lastTimestamp | tail -50 +``` + +Then pick a recovery: + +- **PVCs (or Pods) survived** — the data is still there. Do not delete the cluster: work from the surviving volumes, and stop the old Pods before recreating anything so two etcd clusters never serve the same Service. +- **A snapshot exists** — restore it into a freshly created cluster (see [Restoring a cluster from a snapshot](#restoring-a-cluster-from-a-snapshot)). `spec.bootstrap` is immutable post-create, so this means creating a new `EtcdCluster`, not editing this one. +- **Neither** — the data is gone. Delete the `EtcdCluster` and recreate it to get an empty cluster back. + ### `Progressing=True/WaitingForSeed` The seed `EtcdMember` CR exists but the member controller hasn't yet created its Pod — this is the gap between the cluster controller creating the CR and the member controller's next reconcile pass. `kubectl describe pod` is **not** useful here: there is no Pod yet, so it returns "not found" and obscures the actual state. Inspect the CR and the namespace's events instead: From 7cbad58da6b4a18e00e2bc434c38c1f19de8c5f8 Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Tue, 28 Jul 2026 16:44:12 +0200 Subject: [PATCH 2/2] fix(controllers): don't call it lost while a member is still terminating MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A member whose deletion is in flight (finalizer still running MemberRemove) is filtered out of the active set while its CR is still present, so scaling the last member down — or a self-heal replacement — would be declared a total loss mid-operation and parked terminally. Require the raw member list to be empty as well, leaving those cases to the in-flight-deletion wait that already handles them. Covered by a test that fails without the extra condition. Also extend the Degraded summary in docs/concepts.md, which enumerated only QuorumAvailable and QuorumLost while handleAllMembersLost sets Degraded=True/AllMembersLost too. Assisted-By: Claude Signed-off-by: Andrei Kvapil --- controllers/etcdcluster_controller.go | 11 +++++- controllers/etcdcluster_controller_test.go | 41 ++++++++++++++++++++++ docs/concepts.md | 2 +- 3 files changed, 52 insertions(+), 2 deletions(-) diff --git a/controllers/etcdcluster_controller.go b/controllers/etcdcluster_controller.go index 85635825..b810d365 100644 --- a/controllers/etcdcluster_controller.go +++ b/controllers/etcdcluster_controller.go @@ -237,7 +237,16 @@ func (r *EtcdClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) // generic DeadlineExceeded reason. Both are terminal and visible; only // the specificity of the reason differs, and reusing the existing arm // keeps this change from altering deadline behaviour. - if cluster.Status.ClusterID != "" && desired > 0 && len(active) == 0 { + // + // Both member sets must be empty. `active` alone is not enough: a member + // being deleted (finalizer still running MemberRemove) is filtered out of + // `active` while its CR is very much still there, so a scale-down of the + // last member — or a self-heal replacement — would otherwise be declared + // a total loss mid-flight. Requiring the raw list to be empty too leaves + // those cases to the in-flight-deletion wait below, which is where they + // belong. + if cluster.Status.ClusterID != "" && desired > 0 && + len(active) == 0 && len(memberList.Items) == 0 { return r.handleAllMembersLost(ctx, cluster) } diff --git a/controllers/etcdcluster_controller_test.go b/controllers/etcdcluster_controller_test.go index e7bdb897..91b652d4 100644 --- a/controllers/etcdcluster_controller_test.go +++ b/controllers/etcdcluster_controller_test.go @@ -1160,6 +1160,47 @@ func TestAllMembersLost_NotTriggeredByDormantMember(t *testing.T) { } } +// TestAllMembersLost_NotTriggeredWhileAMemberIsTerminating: the last member is +// being deleted and its finalizer is still running MemberRemove. It is +// filtered out of the active set, but its CR is still there and the deletion +// is expected to complete — scaling down to zero, or replacing a member, must +// not be mistaken for a total loss. The in-flight-deletion wait owns this +// state; parking here would strand a cluster mid-operation. +func TestAllMembersLost_NotTriggeredWhileAMemberIsTerminating(t *testing.T) { + ctx := context.Background() + now := metav1.Now() + cluster := allMembersLostCluster(t, nil) + terminating := &lll.EtcdMember{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-going", Namespace: "ns", Labels: memberLabels("test", "test-going"), + DeletionTimestamp: &now, Finalizers: []string{MemberFinalizer}, + }, + Spec: lll.EtcdMemberSpec{ClusterName: "test", InitialCluster: "x"}, + } + if err := controllerutil.SetControllerReference(cluster, terminating, testScheme(t)); err != nil { + t.Fatalf("SetControllerReference: %v", err) + } + c, _ := newTestClient(t, cluster, terminating) + fe := newFakeEtcd(0xdeadbeef) + r := &EtcdClusterReconciler{Client: c, Scheme: testScheme(t), EtcdClientFactory: factoryFailingOnEmptyEndpoints(fe)} + + res, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: "test", Namespace: "ns"}}) + if err != nil { + t.Fatalf("Reconcile: %v", err) + } + if res.RequeueAfter == 0 { + t.Fatalf("expected the in-flight-deletion wait to requeue; got %+v", res) + } + + got := &lll.EtcdCluster{} + if err := c.Get(ctx, types.NamespacedName{Name: "test", Namespace: "ns"}, got); err != nil { + t.Fatalf("Get: %v", err) + } + if avail := findCond(got.Status.Conditions, lll.ClusterAvailable); avail != nil && avail.Reason == "AllMembersLost" { + t.Fatalf("a member still terminating is not a total loss; must not park") + } +} + // TestAllMembersLost_NotTriggeredWhileAMemberSurvives: one member left, not // Ready (e.g. crash-looping). The data plane is degraded, not gone — the // existing wait-for-Ready path owns this, and a terminal condition here would diff --git a/docs/concepts.md b/docs/concepts.md index b46d11aa..bcfb28bf 100644 --- a/docs/concepts.md +++ b/docs/concepts.md @@ -431,7 +431,7 @@ The cluster surfaces three conditions: `Available`, `Progressing`, `Degraded`. T | `False` | `Paused` | Same as Available; emitted when `desired==0`. | | `False` | `BootstrapFailed` / `DeadlineExceeded` / `AllMembersLost` | Terminal states; see Available. | -`Degraded` is `True` whenever `Available=True/QuorumAvailable` (partial outage) or `Available=False/QuorumLost`. `False` in healthy or paused states. In other words, `Degraded` means "the cluster is not delivering its full intended capacity right now"; reading `Degraded` alone tells an alerting layer whether to page someone. +`Degraded` is `True` whenever `Available=True/QuorumAvailable` (partial outage), `Available=False/QuorumLost`, or `Available=False/AllMembersLost` (every member gone — terminal, see above). `False` in healthy or paused states. In other words, `Degraded` means "the cluster is not delivering its full intended capacity right now"; reading `Degraded` alone tells an alerting layer whether to page someone. All conditions carry `observedGeneration` so consumers can tell whether a condition reflects the latest spec. Status writes are gated on "did anything actually change" — the operator does not bump `resourceVersion` every 30 s just because of the periodic reconcile.