From a24698b83496403d35ddcda07d7d300d84ecdbe4 Mon Sep 17 00:00:00 2001 From: Andrey Kolkov Date: Tue, 15 Sep 2026 18:42:27 +0400 Subject: [PATCH 1/7] fix(controllers): keep EtcdCluster status truthful when etcd is unreachable On a converged cluster (ClusterID latched, current==desired), the steady-state promote and auth attempts returned their transient requeue directly, short-circuiting the reconcile before updateStatus. When etcd became unreachable the promote path took its MemberList-failure branch and requeued, so the cluster's Available/Degraded conditions and ReadyMembers froze at their last-healthy value. A fully down cluster kept advertising Available=True/QuorumHealthy indefinitely even though every EtcdMember had already flipped Ready=False. updateStatus derives the cluster conditions from the member set, so running it on every converged pass reports the truth on its own. Thread the promote/auth transient requeue through updateStatus via `pending` instead of returning early; the status write always happens and the returned Result carries whichever requeue fires sooner. This matches the existing "log and fall through to updateStatus" stance already used for the TLS-config, credentials, and dial-failure branches in the same block. Fixes #367 Assisted-By: LLM Signed-off-by: Andrey Kolkov --- controllers/etcdcluster_controller.go | 50 ++++++++-- controllers/etcdcluster_controller_test.go | 103 ++++++++++++++++++++- 2 files changed, 142 insertions(+), 11 deletions(-) diff --git a/controllers/etcdcluster_controller.go b/controllers/etcdcluster_controller.go index 68ddbad2..dd180a90 100644 --- a/controllers/etcdcluster_controller.go +++ b/controllers/etcdcluster_controller.go @@ -241,7 +241,7 @@ func (r *EtcdClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) // to discovery to latch ClusterID once etcd answers. if desired == 0 { log.Info("cluster declared paused from the start; not bootstrapping") - return r.updateStatus(ctx, cluster, active) + return r.updateStatus(ctx, cluster, active, nil) } if current == 0 || hasPendingBootstrap(running) { log.Info("bootstrapping single-node cluster") @@ -321,6 +321,15 @@ func (r *EtcdClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) // so scaleUp won't run again on its own). We need a promote attempt // here too. Cheap: list etcd once and try to promote any learner; // no-op if none. + // + // A transient requeue from either the promote or the auth attempt + // (etcd unreachable, learner not yet promotable, auth just latched) + // is threaded into updateStatus via `pending` rather than returned + // here. Returning early would skip updateStatus, freezing the + // cluster's Available/Degraded conditions at their last-healthy value + // while every EtcdMember has already flipped Ready=False — a down + // cluster would keep reporting QuorumHealthy indefinitely. + var pending *ctrl.Result if cluster.Status.ClusterID != "" && len(running) > 0 { endpoints := memberEndpoints(clusterClientScheme(cluster), running, cluster.Namespace) tlsCfg, tlsErr := buildOperatorTLSConfig(ctx, r.Client, cluster) @@ -345,9 +354,7 @@ func (r *EtcdClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) if perr != nil { return ctrl.Result{}, perr } - if res != nil { - return *res, nil - } + pending = res } } // Fall through to updateStatus — the next reconcile will retry @@ -364,7 +371,7 @@ func (r *EtcdClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) if res, err := r.reconcileAuth(ctx, cluster, running); err != nil { return ctrl.Result{}, err } else if res != nil { - return *res, nil + pending = res } // ── Steady state ─────────────────────────────────────────────────── @@ -376,7 +383,7 @@ func (r *EtcdClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) // dormant member with a real PVC exists. updateStatus re-derives // `running` internally for its accounting, so passing `active` // here is the correct shape. - return r.updateStatus(ctx, cluster, active) + return r.updateStatus(ctx, cluster, active, pending) } // ── Bootstrap ──────────────────────────────────────────────────────────── @@ -1331,10 +1338,18 @@ func hasPendingBootstrap(members []lll.EtcdMember) bool { // including any dormant member. It extracts the running subset for the // per-condition accounting and uses the dormant member separately for // the Paused message's PVC name. +// updateStatus recomputes the cluster's cached status fields and conditions +// from the current EtcdMember set and writes them. It is the single exit +// point of a converged reconcile, so callers with a transient requeue to +// honour (promote/auth retries) pass it as `pending` instead of returning +// early: the status write still happens, and the returned Result carries +// whichever requeue fires sooner — `pending` or updateStatus's own steady- +// state cadence. `pending` is nil when there is nothing to thread through. func (r *EtcdClusterReconciler) updateStatus( ctx context.Context, cluster *lll.EtcdCluster, members []lll.EtcdMember, + pending *ctrl.Result, ) (ctrl.Result, error) { desired := cluster.Status.Observed.Replicas running := filterRunningMembers(members) @@ -1489,7 +1504,28 @@ func (r *EtcdClusterReconciler) updateStatus( } } - return ctrl.Result{RequeueAfter: 30 * time.Second}, nil + return soonerRequeue(ctrl.Result{RequeueAfter: 30 * time.Second}, pending), nil +} + +// soonerRequeue returns whichever of the two results asks the controller to +// come back sooner. `base` is updateStatus's own steady-state cadence and +// always requeues; `pending` is an optional transient retry (nil when none). +// A Requeue=true (requeue-now) beats any RequeueAfter delay; between two +// delays the shorter wins. +func soonerRequeue(base ctrl.Result, pending *ctrl.Result) ctrl.Result { + if pending == nil { + return base + } + if pending.Requeue && !base.Requeue { + return *pending + } + if !pending.Requeue && base.Requeue { + return base + } + if pending.RequeueAfter > 0 && pending.RequeueAfter < base.RequeueAfter { + return *pending + } + return base } // pdbMinAvailable returns the eviction floor: quorum (n/2+1) of diff --git a/controllers/etcdcluster_controller_test.go b/controllers/etcdcluster_controller_test.go index ee79cb6e..7364113e 100644 --- a/controllers/etcdcluster_controller_test.go +++ b/controllers/etcdcluster_controller_test.go @@ -590,6 +590,101 @@ func TestTryDiscoverCluster_AuthCredentialsRejected(t *testing.T) { } } +// TestReconcile_UnreachableEtcdDoesNotFreezeStatus pins the fix for #367: +// on a converged cluster (ClusterID latched, current==desired) whose etcd +// has gone unreachable, the steady-state promote attempt returns a transient +// requeue. That requeue must be threaded through updateStatus rather than +// returned early — otherwise the cluster's Available/Degraded conditions +// freeze at their last-healthy value while every member reports Ready=False, +// and a fully down cluster keeps advertising QuorumHealthy. +func TestReconcile_UnreachableEtcdDoesNotFreezeStatus(t *testing.T) { + ctx := context.Background() + cluster := &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: "test", + ClusterID: "deadbeef", + Observed: &lll.ObservedClusterSpec{ + Replicas: 3, + Version: "3.5.17", + Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, + }, + ProgressDeadline: &metav1.Time{Time: metav1.Now().Add(60 * 60 * 1e9)}, + // Stale last-healthy snapshot: this is what must NOT survive a + // reconcile once etcd is unreachable and members are Ready=False. + ReadyMembers: 3, + Conditions: []metav1.Condition{{ + Type: lll.ClusterAvailable, Status: metav1.ConditionTrue, + Reason: "QuorumHealthy", Message: "All members are ready", + LastTransitionTime: metav1.Now(), + }}, + }, + } + objs := []client.Object{cluster} + // Three members, all Ready=False — the member controller has already + // observed the down etcd and flipped them, exactly as reported in #367. + for i := 0; i < 3; i++ { + objs = append(objs, &lll.EtcdMember{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("test-%d", i), + Namespace: "ns", + Labels: memberLabels("test", fmt.Sprintf("test-%d", i)), + }, + Spec: lll.EtcdMemberSpec{ClusterName: "test", Version: "3.5.17", Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, InitialCluster: "x", ClusterToken: "test"}, + Status: lll.EtcdMemberStatus{ + PodName: fmt.Sprintf("test-%d", i), + MemberID: "abc", + Conditions: []metav1.Condition{{ + Type: lll.MemberReady, Status: metav1.ConditionFalse, Reason: "PodNotReady", + LastTransitionTime: metav1.Now(), + }}, + }, + }) + } + c, _ := newTestClient(t, objs...) + // Dialable client whose MemberList errors: this is the etcd-unreachable + // shape (a lazy clientv3 dial succeeds; the RPC is where it fails). + fe := newFakeEtcd(0xdeadbeef) + fe.listErr = errors.New("context deadline exceeded") + r := &EtcdClusterReconciler{ + Client: c, + Scheme: testScheme(t), + EtcdClientFactory: factoryReturning(fe), + } + + res, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: "test", Namespace: "ns"}}) + if err != nil { + t.Fatalf("Reconcile: %v", err) + } + + mustGet(t, c, "test", "ns", cluster) + var available *metav1.Condition + for i := range cluster.Status.Conditions { + if cluster.Status.Conditions[i].Type == lll.ClusterAvailable { + available = &cluster.Status.Conditions[i] + } + } + if available == nil { + t.Fatalf("no Available condition after reconcile") + } + if available.Status != metav1.ConditionFalse { + t.Fatalf("Available = %v/%q, want False (status must not freeze at QuorumHealthy)", available.Status, available.Reason) + } + if cluster.Status.ReadyMembers != 0 { + t.Fatalf("ReadyMembers = %d, want 0 (recomputed from member conditions)", cluster.Status.ReadyMembers) + } + // The promote attempt's 10s transient requeue must survive: it is sooner + // than updateStatus's 30s cadence, so it wins. + if res.RequeueAfter != 10*time.Second { + t.Fatalf("RequeueAfter = %v, want 10s (promote requeue threaded through updateStatus)", res.RequeueAfter) + } +} + // TestUpdateStatus_SurfacesBrokenCount covers reviewer issue #6: the isBroken // stub must have a tested call site so the predicate is actually exercised. // Today it always returns false, so the count must always be 0 — this test @@ -1185,7 +1280,7 @@ func TestUpdateStatus_PausedClusterReportsPausedCondition(t *testing.T) { c, _ := newTestClient(t, cluster, &dormant) r := &EtcdClusterReconciler{Client: c, Scheme: testScheme(t), EtcdClientFactory: factoryReturning(newFakeEtcd(0xdead))} - if _, err := r.updateStatus(ctx, cluster, []lll.EtcdMember{dormant}); err != nil { + if _, err := r.updateStatus(ctx, cluster, []lll.EtcdMember{dormant}, nil); err != nil { t.Fatalf("updateStatus: %v", err) } mustGet(t, c, "test", "ns", cluster) @@ -1241,7 +1336,7 @@ func TestUpdateStatus_PausedFreshZeroMessageDifferentiates(t *testing.T) { c, _ := newTestClient(t, cluster) r := &EtcdClusterReconciler{Client: c, Scheme: testScheme(t), EtcdClientFactory: factoryReturning(newFakeEtcd(0xdead))} - if _, err := r.updateStatus(ctx, cluster, nil); err != nil { + if _, err := r.updateStatus(ctx, cluster, nil, nil); err != nil { t.Fatalf("updateStatus: %v", err) } mustGet(t, c, "test", "ns", cluster) @@ -1301,7 +1396,7 @@ func TestUpdateStatus_PausedMessageHonestForMemoryMember(t *testing.T) { c, _ := newTestClient(t, cluster, dormant) r := &EtcdClusterReconciler{Client: c, Scheme: testScheme(t), EtcdClientFactory: factoryReturning(newFakeEtcd(0xdead))} - if _, err := r.updateStatus(ctx, cluster, []lll.EtcdMember{*dormant}); err != nil { + if _, err := r.updateStatus(ctx, cluster, []lll.EtcdMember{*dormant}, nil); err != nil { t.Fatalf("updateStatus: %v", err) } got := mustGet(t, c, "c", "ns", &lll.EtcdCluster{}) @@ -3154,7 +3249,7 @@ func TestUpdateStatus_SetsScaleSelector(t *testing.T) { c, _ := newTestClient(t, cluster) r := &EtcdClusterReconciler{Client: c, Scheme: testScheme(t), EtcdClientFactory: factoryReturning(newFakeEtcd(0xabc))} - if _, err := r.updateStatus(ctx, cluster, nil); err != nil { + if _, err := r.updateStatus(ctx, cluster, nil, nil); err != nil { t.Fatalf("updateStatus: %v", err) } From 40b21a887faf55501370abcd70dd3c563c1fbd1b Mon Sep 17 00:00:00 2001 From: Andrey Kolkov Date: Tue, 15 Sep 2026 18:52:38 +0400 Subject: [PATCH 2/7] fix(controllers): tidy status-freeze fix per review - Merge the doubled updateStatus doc comment into one paragraph. - Preserve the pre-fix promote-before-auth serialization: only attempt reconcileAuth when the promote step produced no pending requeue, so the auth-enable flip can't race an in-flight promotion. Previously a non-nil auth result could overwrite (and lengthen) a sooner promote requeue. - Add TestSoonerRequeue covering the nil, requeue-now, and shorter/longer delay branches of the requeue-merge helper. Assisted-By: LLM Signed-off-by: Andrey Kolkov --- controllers/etcdcluster_controller.go | 33 +++++++++++++--------- controllers/etcdcluster_controller_test.go | 23 +++++++++++++++ 2 files changed, 43 insertions(+), 13 deletions(-) diff --git a/controllers/etcdcluster_controller.go b/controllers/etcdcluster_controller.go index dd180a90..144731af 100644 --- a/controllers/etcdcluster_controller.go +++ b/controllers/etcdcluster_controller.go @@ -368,10 +368,19 @@ func (r *EtcdClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) // formed). Gating on convergence keeps the auth flip from racing in- // flight scale-up dials. No-op (and skipped) once status.authEnabled // has latched. - if res, err := r.reconcileAuth(ctx, cluster, running); err != nil { - return ctrl.Result{}, err - } else if res != nil { - pending = res + // + // Only attempt auth when the promote step above produced no pending + // requeue: a pending promote means a learner is still unpromoted or + // etcd is unreachable, and the pre-fix flow returned before auth in + // exactly that case. Preserving that promote-before-auth ordering keeps + // the auth-enable flip from racing an in-flight promotion while still + // falling through to updateStatus with the promote requeue. + if pending == nil { + if res, err := r.reconcileAuth(ctx, cluster, running); err != nil { + return ctrl.Result{}, err + } else if res != nil { + pending = res + } } // ── Steady state ─────────────────────────────────────────────────── @@ -1336,15 +1345,13 @@ func hasPendingBootstrap(members []lll.EtcdMember) bool { // updateStatus is called with the full active member list (non-deleted), // including any dormant member. It extracts the running subset for the -// per-condition accounting and uses the dormant member separately for -// the Paused message's PVC name. -// updateStatus recomputes the cluster's cached status fields and conditions -// from the current EtcdMember set and writes them. It is the single exit -// point of a converged reconcile, so callers with a transient requeue to -// honour (promote/auth retries) pass it as `pending` instead of returning -// early: the status write still happens, and the returned Result carries -// whichever requeue fires sooner — `pending` or updateStatus's own steady- -// state cadence. `pending` is nil when there is nothing to thread through. +// per-condition accounting and uses the dormant member separately for the +// Paused message's PVC name. It is the single exit point of a converged +// reconcile, so callers holding a transient requeue (promote/auth retries) +// pass it as `pending` rather than returning early: the status write still +// happens, and the returned Result carries whichever requeue fires sooner — +// `pending` or updateStatus's own steady-state cadence. `pending` is nil +// when there is nothing to thread through. func (r *EtcdClusterReconciler) updateStatus( ctx context.Context, cluster *lll.EtcdCluster, diff --git a/controllers/etcdcluster_controller_test.go b/controllers/etcdcluster_controller_test.go index 7364113e..349b8049 100644 --- a/controllers/etcdcluster_controller_test.go +++ b/controllers/etcdcluster_controller_test.go @@ -685,6 +685,29 @@ func TestReconcile_UnreachableEtcdDoesNotFreezeStatus(t *testing.T) { } } +func TestSoonerRequeue(t *testing.T) { + base := ctrl.Result{RequeueAfter: 30 * time.Second} + cases := []struct { + name string + base ctrl.Result + pending *ctrl.Result + want ctrl.Result + }{ + {"nil pending keeps base", base, nil, base}, + {"shorter delay wins", base, &ctrl.Result{RequeueAfter: 10 * time.Second}, ctrl.Result{RequeueAfter: 10 * time.Second}}, + {"longer delay loses to base", base, &ctrl.Result{RequeueAfter: 40 * time.Second}, base}, + {"requeue-now beats a delay", base, &ctrl.Result{Requeue: true}, ctrl.Result{Requeue: true}}, + {"base requeue-now beats pending delay", ctrl.Result{Requeue: true}, &ctrl.Result{RequeueAfter: 5 * time.Second}, ctrl.Result{Requeue: true}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := soonerRequeue(tc.base, tc.pending); got != tc.want { + t.Fatalf("soonerRequeue = %+v, want %+v", got, tc.want) + } + }) + } +} + // TestUpdateStatus_SurfacesBrokenCount covers reviewer issue #6: the isBroken // stub must have a tested call site so the predicate is actually exercised. // Today it always returns false, so the count must always be 0 — this test From 0aff40d744f04cf1cf774fc7e113fe152ea066d3 Mon Sep 17 00:00:00 2001 From: Andrey Kolkov Date: Tue, 15 Sep 2026 19:12:01 +0400 Subject: [PATCH 3/7] docs(controllers): trim comments on the status-freeze fix Tighten the comments added for the etcd-unreachable status fix to the load-bearing invariant; drop narration and ticket references. Assisted-By: LLM Signed-off-by: Andrey Kolkov --- controllers/etcdcluster_controller.go | 43 ++++++++-------------- controllers/etcdcluster_controller_test.go | 24 +++++------- 2 files changed, 25 insertions(+), 42 deletions(-) diff --git a/controllers/etcdcluster_controller.go b/controllers/etcdcluster_controller.go index 144731af..2ec27740 100644 --- a/controllers/etcdcluster_controller.go +++ b/controllers/etcdcluster_controller.go @@ -322,13 +322,10 @@ func (r *EtcdClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) // here too. Cheap: list etcd once and try to promote any learner; // no-op if none. // - // A transient requeue from either the promote or the auth attempt - // (etcd unreachable, learner not yet promotable, auth just latched) - // is threaded into updateStatus via `pending` rather than returned - // here. Returning early would skip updateStatus, freezing the - // cluster's Available/Degraded conditions at their last-healthy value - // while every EtcdMember has already flipped Ready=False — a down - // cluster would keep reporting QuorumHealthy indefinitely. + // A transient requeue from promote/auth is threaded through + // updateStatus via `pending`, not returned here: an early return skips + // updateStatus, freezing the cluster conditions at their last-healthy + // value while the members already read Ready=False. var pending *ctrl.Result if cluster.Status.ClusterID != "" && len(running) > 0 { endpoints := memberEndpoints(clusterClientScheme(cluster), running, cluster.Namespace) @@ -369,12 +366,9 @@ func (r *EtcdClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) // flight scale-up dials. No-op (and skipped) once status.authEnabled // has latched. // - // Only attempt auth when the promote step above produced no pending - // requeue: a pending promote means a learner is still unpromoted or - // etcd is unreachable, and the pre-fix flow returned before auth in - // exactly that case. Preserving that promote-before-auth ordering keeps - // the auth-enable flip from racing an in-flight promotion while still - // falling through to updateStatus with the promote requeue. + // Skip when a promote requeue is already pending: auth-enable must not + // race an in-flight promotion (a pending promote means a learner is + // unpromoted or etcd is unreachable). if pending == nil { if res, err := r.reconcileAuth(ctx, cluster, running); err != nil { return ctrl.Result{}, err @@ -1343,15 +1337,12 @@ func hasPendingBootstrap(members []lll.EtcdMember) bool { // ── Status ─────────────────────────────────────────────────────────────── -// updateStatus is called with the full active member list (non-deleted), -// including any dormant member. It extracts the running subset for the -// per-condition accounting and uses the dormant member separately for the -// Paused message's PVC name. It is the single exit point of a converged -// reconcile, so callers holding a transient requeue (promote/auth retries) -// pass it as `pending` rather than returning early: the status write still -// happens, and the returned Result carries whichever requeue fires sooner — -// `pending` or updateStatus's own steady-state cadence. `pending` is nil -// when there is nothing to thread through. +// updateStatus takes the full active member list including any dormant member +// (running is extracted for accounting; the dormant one names the Paused- +// message PVC) and writes the recomputed status. A caller holding a transient +// promote/auth requeue passes it as `pending` (nil otherwise) instead of +// returning early, so the status write always happens; the returned Result +// carries whichever of `pending` and the steady-state cadence fires sooner. func (r *EtcdClusterReconciler) updateStatus( ctx context.Context, cluster *lll.EtcdCluster, @@ -1514,11 +1505,9 @@ func (r *EtcdClusterReconciler) updateStatus( return soonerRequeue(ctrl.Result{RequeueAfter: 30 * time.Second}, pending), nil } -// soonerRequeue returns whichever of the two results asks the controller to -// come back sooner. `base` is updateStatus's own steady-state cadence and -// always requeues; `pending` is an optional transient retry (nil when none). -// A Requeue=true (requeue-now) beats any RequeueAfter delay; between two -// delays the shorter wins. +// soonerRequeue returns whichever result requeues sooner: Requeue=true +// (requeue-now) beats any RequeueAfter delay, and between two delays the +// shorter wins. `pending` is nil when there is no transient retry to merge. func soonerRequeue(base ctrl.Result, pending *ctrl.Result) ctrl.Result { if pending == nil { return base diff --git a/controllers/etcdcluster_controller_test.go b/controllers/etcdcluster_controller_test.go index 349b8049..92719764 100644 --- a/controllers/etcdcluster_controller_test.go +++ b/controllers/etcdcluster_controller_test.go @@ -590,13 +590,10 @@ func TestTryDiscoverCluster_AuthCredentialsRejected(t *testing.T) { } } -// TestReconcile_UnreachableEtcdDoesNotFreezeStatus pins the fix for #367: -// on a converged cluster (ClusterID latched, current==desired) whose etcd -// has gone unreachable, the steady-state promote attempt returns a transient -// requeue. That requeue must be threaded through updateStatus rather than -// returned early — otherwise the cluster's Available/Degraded conditions -// freeze at their last-healthy value while every member reports Ready=False, -// and a fully down cluster keeps advertising QuorumHealthy. +// On a converged cluster whose etcd is unreachable, the steady-state promote +// requeue must flow through updateStatus rather than return early: otherwise +// the cluster conditions freeze at their last-healthy value while the members +// already read Ready=False. func TestReconcile_UnreachableEtcdDoesNotFreezeStatus(t *testing.T) { ctx := context.Background() cluster := &lll.EtcdCluster{ @@ -615,8 +612,7 @@ func TestReconcile_UnreachableEtcdDoesNotFreezeStatus(t *testing.T) { Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, }, ProgressDeadline: &metav1.Time{Time: metav1.Now().Add(60 * 60 * 1e9)}, - // Stale last-healthy snapshot: this is what must NOT survive a - // reconcile once etcd is unreachable and members are Ready=False. + // Stale last-healthy snapshot: must NOT survive the reconcile. ReadyMembers: 3, Conditions: []metav1.Condition{{ Type: lll.ClusterAvailable, Status: metav1.ConditionTrue, @@ -626,8 +622,7 @@ func TestReconcile_UnreachableEtcdDoesNotFreezeStatus(t *testing.T) { }, } objs := []client.Object{cluster} - // Three members, all Ready=False — the member controller has already - // observed the down etcd and flipped them, exactly as reported in #367. + // Members already flipped Ready=False by their own controller. for i := 0; i < 3; i++ { objs = append(objs, &lll.EtcdMember{ ObjectMeta: metav1.ObjectMeta{ @@ -647,8 +642,8 @@ func TestReconcile_UnreachableEtcdDoesNotFreezeStatus(t *testing.T) { }) } c, _ := newTestClient(t, objs...) - // Dialable client whose MemberList errors: this is the etcd-unreachable - // shape (a lazy clientv3 dial succeeds; the RPC is where it fails). + // Dialable client whose MemberList errors — the etcd-unreachable shape + // (a lazy clientv3 dial succeeds; the RPC is where it fails). fe := newFakeEtcd(0xdeadbeef) fe.listErr = errors.New("context deadline exceeded") r := &EtcdClusterReconciler{ @@ -678,8 +673,7 @@ func TestReconcile_UnreachableEtcdDoesNotFreezeStatus(t *testing.T) { if cluster.Status.ReadyMembers != 0 { t.Fatalf("ReadyMembers = %d, want 0 (recomputed from member conditions)", cluster.Status.ReadyMembers) } - // The promote attempt's 10s transient requeue must survive: it is sooner - // than updateStatus's 30s cadence, so it wins. + // The promote's 10s requeue survives (sooner than the 30s cadence). if res.RequeueAfter != 10*time.Second { t.Fatalf("RequeueAfter = %v, want 10s (promote requeue threaded through updateStatus)", res.RequeueAfter) } From 73eea79b6316d1ee74426685aa1993b5881b49b7 Mon Sep 17 00:00:00 2001 From: Andrey Kolkov Date: Wed, 16 Sep 2026 14:59:02 +0400 Subject: [PATCH 4/7] fix(controllers): address review on the status-freeze fix - updateStatus withholds the Available/Degraded rewrite while the cluster is Progressing and not yet reconciled: the promote-after-converged pass reaches updateStatus with ready --- controllers/etcdcluster_controller.go | 82 +++++++-- controllers/etcdcluster_controller_test.go | 201 ++++++++++++++++++--- 2 files changed, 241 insertions(+), 42 deletions(-) diff --git a/controllers/etcdcluster_controller.go b/controllers/etcdcluster_controller.go index 2ec27740..95d54118 100644 --- a/controllers/etcdcluster_controller.go +++ b/controllers/etcdcluster_controller.go @@ -19,6 +19,7 @@ package controllers import ( "context" "fmt" + "math" "sort" "strings" "time" @@ -366,9 +367,13 @@ func (r *EtcdClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) // flight scale-up dials. No-op (and skipped) once status.authEnabled // has latched. // - // Skip when a promote requeue is already pending: auth-enable must not - // race an in-flight promotion (a pending promote means a learner is - // unpromoted or etcd is unreachable). + // Skip when a promote requeue is already pending. A pending promote means + // the just-finished MemberList/MemberPromote either found etcd unreachable + // or a learner not yet caught up. reconcileAuth's clientv3 calls + // (AuthStatus/UserAdd/UserGrantRole/AuthEnable) run on the raw reconcile + // context with clientv3's default WaitForReady and no per-call deadline, so + // dialing again against a dead endpoint would block the worker rather than + // error out. Deferring auth to the pass after promote clears avoids that. if pending == nil { if res, err := r.reconcileAuth(ctx, cluster, running); err != nil { return ctrl.Result{}, err @@ -1341,8 +1346,11 @@ func hasPendingBootstrap(members []lll.EtcdMember) bool { // (running is extracted for accounting; the dormant one names the Paused- // message PVC) and writes the recomputed status. A caller holding a transient // promote/auth requeue passes it as `pending` (nil otherwise) instead of -// returning early, so the status write always happens; the returned Result -// carries whichever of `pending` and the steady-state cadence fires sooner. +// returning early, so the status write always happens. `pending` is a status +// input, not just a requeue to forward: while it is non-nil the cluster is not +// settled, so updateStatus withholds the Progressing=False/Reconciled stamp. +// The returned Result carries whichever of `pending` and the steady-state +// cadence fires sooner. func (r *EtcdClusterReconciler) updateStatus( ctx context.Context, cluster *lll.EtcdCluster, @@ -1387,7 +1395,17 @@ func (r *EtcdClusterReconciler) updateStatus( // downstream operators that wait for Available=True). The Paused // branch takes precedence over the health switch below and also // overrides the Reconciled-Progressing override further down. + // While the operator is actively driving toward the target (Progressing= + // True and not yet reconciled — a bootstrap or scale step in flight), the + // ready/desired ratio counts not-yet-joined members against quorum, so the + // health switch below would stamp Degraded on a cluster whose live voters + // are all healthy. Leave Available/Degraded at their last value in that + // window; the steady-state pass (Progressing=False) writes the truthful + // health. This matches the allMembersReady early-return in Reconcile, which + // already suppresses the same write for the earlier learners of the same + // scale-up — so every mid-scale-up window reports alike. paused := desired == 0 + progressing := !paused && clusterProgressing(cluster) && !reconciliationComplete(cluster, running) switch { case paused: // Three flavours of paused: @@ -1424,6 +1442,8 @@ func (r *EtcdClusterReconciler) updateStatus( if setClusterCondition(cluster, lll.ClusterProgressing, metav1.ConditionFalse, "Paused", "") { changed = true } + case progressing: + // Health left as-is; see the comment above the switch. case ready == desired: if setClusterCondition(cluster, lll.ClusterAvailable, metav1.ConditionTrue, "QuorumHealthy", "All members are ready") { changed = true @@ -1458,7 +1478,14 @@ func (r *EtcdClusterReconciler) updateStatus( cluster.Status.ProgressDeadline = nil changed = true } - } else if reconciliationComplete(cluster, running) { + } else if pending == nil && reconciliationComplete(cluster, running) { + // A non-nil `pending` means a promote or auth-enable retry is still + // outstanding: a learner whose MemberPromote keeps being rejected (its + // pod is MemberReady, so reconciliationComplete would otherwise pass + // while it is still a learner), or spec.auth.enabled with reconcileAuth + // looping. Stamping Reconciled and clearing ProgressDeadline there + // would report the cluster settled and let deadline escalation lapse + // while etcd still has an unpromoted voter or auth was never applied. if setClusterCondition(cluster, lll.ClusterProgressing, metav1.ConditionFalse, "Reconciled", "actual state matches status.observed") { changed = true @@ -1505,25 +1532,36 @@ func (r *EtcdClusterReconciler) updateStatus( return soonerRequeue(ctrl.Result{RequeueAfter: 30 * time.Second}, pending), nil } -// soonerRequeue returns whichever result requeues sooner: Requeue=true -// (requeue-now) beats any RequeueAfter delay, and between two delays the -// shorter wins. `pending` is nil when there is no transient retry to merge. +// soonerRequeue returns whichever of base and pending fires sooner. `pending` +// is nil when there is no transient retry to merge. Firing order follows +// controller-runtime v0.21: a positive RequeueAfter takes precedence (Requeue +// is deprecated there and RequeueAfter wins when both are set), a bare +// Requeue=true fires immediately, and a zero Result never fires on its own — +// so soonerRequeue(ctrl.Result{}, pending) yields pending, not the dropped +// retry that an "empty base is immediate" reading would produce. func soonerRequeue(base ctrl.Result, pending *ctrl.Result) ctrl.Result { if pending == nil { return base } - if pending.Requeue && !base.Requeue { - return *pending - } - if !pending.Requeue && base.Requeue { - return base - } - if pending.RequeueAfter > 0 && pending.RequeueAfter < base.RequeueAfter { + if requeueDelay(*pending) < requeueDelay(base) { return *pending } return base } +// requeueDelay is the delay after which a Result triggers the next reconcile, +// used only to order two Results. A zero Result maps to "never". +func requeueDelay(r ctrl.Result) time.Duration { + switch { + case r.RequeueAfter > 0: + return r.RequeueAfter + case r.Requeue: + return 0 + default: + return math.MaxInt64 + } +} + // pdbMinAvailable returns the eviction floor: quorum (n/2+1) of // max(live voters, latched target). The target keeps the floor from // re-basing during churn; the live count covers scale-down. Full @@ -2134,6 +2172,18 @@ func reconciliationComplete(cluster *lll.EtcdCluster, members []lll.EtcdMember) return true } +// clusterProgressing reports whether the Progressing condition is currently +// True — the operator is actively driving toward the observed target +// (bootstrap or a scale step in flight). +func clusterProgressing(cluster *lll.EtcdCluster) bool { + for _, c := range cluster.Status.Conditions { + if c.Type == lll.ClusterProgressing { + return c.Status == metav1.ConditionTrue + } + } + return false +} + func setProgressDeadline(cluster *lll.EtcdCluster, now metav1.Time) { secs := DefaultProgressDeadlineSeconds if cluster.Spec.ProgressDeadlineSeconds != nil { diff --git a/controllers/etcdcluster_controller_test.go b/controllers/etcdcluster_controller_test.go index 92719764..ee424c69 100644 --- a/controllers/etcdcluster_controller_test.go +++ b/controllers/etcdcluster_controller_test.go @@ -27,6 +27,7 @@ import ( policyv1 "k8s.io/api/policy/v1" "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -611,7 +612,7 @@ func TestReconcile_UnreachableEtcdDoesNotFreezeStatus(t *testing.T) { Version: "3.5.17", Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, }, - ProgressDeadline: &metav1.Time{Time: metav1.Now().Add(60 * 60 * 1e9)}, + ProgressDeadline: &metav1.Time{Time: metav1.Now().Add(time.Hour)}, // Stale last-healthy snapshot: must NOT survive the reconcile. ReadyMembers: 3, Conditions: []metav1.Condition{{ @@ -621,26 +622,8 @@ func TestReconcile_UnreachableEtcdDoesNotFreezeStatus(t *testing.T) { }}, }, } - objs := []client.Object{cluster} // Members already flipped Ready=False by their own controller. - for i := 0; i < 3; i++ { - objs = append(objs, &lll.EtcdMember{ - ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf("test-%d", i), - Namespace: "ns", - Labels: memberLabels("test", fmt.Sprintf("test-%d", i)), - }, - Spec: lll.EtcdMemberSpec{ClusterName: "test", Version: "3.5.17", Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, InitialCluster: "x", ClusterToken: "test"}, - Status: lll.EtcdMemberStatus{ - PodName: fmt.Sprintf("test-%d", i), - MemberID: "abc", - Conditions: []metav1.Condition{{ - Type: lll.MemberReady, Status: metav1.ConditionFalse, Reason: "PodNotReady", - LastTransitionTime: metav1.Now(), - }}, - }, - }) - } + objs := append([]client.Object{cluster}, membersAsObjects(scaleUpMembers(t, "test", "ns", 0, 3))...) c, _ := newTestClient(t, objs...) // Dialable client whose MemberList errors — the etcd-unreachable shape // (a lazy clientv3 dial succeeds; the RPC is where it fails). @@ -658,12 +641,7 @@ func TestReconcile_UnreachableEtcdDoesNotFreezeStatus(t *testing.T) { } mustGet(t, c, "test", "ns", cluster) - var available *metav1.Condition - for i := range cluster.Status.Conditions { - if cluster.Status.Conditions[i].Type == lll.ClusterAvailable { - available = &cluster.Status.Conditions[i] - } - } + available := meta.FindStatusCondition(cluster.Status.Conditions, lll.ClusterAvailable) if available == nil { t.Fatalf("no Available condition after reconcile") } @@ -679,6 +657,170 @@ func TestReconcile_UnreachableEtcdDoesNotFreezeStatus(t *testing.T) { } } +// scaleUpMembers builds nReady Ready=True members followed by nNotReady +// Ready=False members for cluster `name` in `ns` — the minimal shared fixture +// for the status tests, standing in for a cluster caught mid-flight (Ready +// voters plus joining members the member controller has not marked Ready). +func scaleUpMembers(t *testing.T, name, ns string, nReady, nNotReady int) []lll.EtcdMember { + t.Helper() + build := func(i int, ready bool) lll.EtcdMember { + mn := fmt.Sprintf("%s-%d", name, i) + status, reason := metav1.ConditionFalse, "PodNotReady" + if ready { + status, reason = metav1.ConditionTrue, "PodReady" + } + return lll.EtcdMember{ + ObjectMeta: metav1.ObjectMeta{Name: mn, Namespace: ns, Labels: memberLabels(name, mn)}, + Spec: lll.EtcdMemberSpec{ClusterName: name, Version: "3.5.17", Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, InitialCluster: "x", ClusterToken: name}, + Status: lll.EtcdMemberStatus{ + PodName: mn, + MemberID: "abc", + IsVoter: ready, + Conditions: []metav1.Condition{{ + Type: lll.MemberReady, Status: status, Reason: reason, LastTransitionTime: metav1.Now(), + }}, + }, + } + } + out := make([]lll.EtcdMember, 0, nReady+nNotReady) + for i := 0; i < nReady; i++ { + out = append(out, build(i, true)) + } + for i := 0; i < nNotReady; i++ { + out = append(out, build(nReady+i, false)) + } + return out +} + +// membersAsObjects adapts a member slice for newTestClient's variadic seed. +func membersAsObjects(members []lll.EtcdMember) []client.Object { + out := make([]client.Object, 0, len(members)) + for i := range members { + out = append(out, &members[i]) + } + return out +} + +// TestUpdateStatus_NoDegradedFlapDuringScaleUp covers review item 1: on the +// converged pass that promotes the last learner, a not-ready promote retry +// (pending) reaches updateStatus with ready0 before the + // deprecated Requeue flag, so a 40s pending loses to the 30s base. + {"pending requeue-after outranks its own requeue flag", base, &ctrl.Result{Requeue: true, RequeueAfter: 40 * time.Second}, base}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { From 738f32eed14b8b26107565e7ffc38145c610cb48 Mon Sep 17 00:00:00 2001 From: Andrey Kolkov Date: Fri, 18 Sep 2026 16:45:27 +0400 Subject: [PATCH 5/7] fix(controllers): floor the progressing status withhold on quorum MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `case progressing` withhold in updateStatus left Available/Degraded at their last value whenever the cluster was Progressing and a member was not Ready, without telling a joining learner from a dead cluster. Progressing latches indefinitely when `pending` never clears (spec.auth.enabled looping on a bad root Secret, or a learner whose MemberPromote keeps being rejected), so a cluster that then lost every pod kept advertising Available=True/QuorumHealthy — the #367 freeze, now with ReadyMembers=0 contradicting Available on one object. Floor the withhold on quorum: hold it only while the ready voters still carry the live voter set (readyVoters > voters/2), counted in the existing member scan alongside `ready`. Once quorum is gone the switch falls through and writes QuorumLost. The joining-member window where live voters hold quorum still reports unchanged, so scale-up does not flap Degraded. The general joining-member denominator stays in #371. Reuse that voter count for the PodDisruptionBudget floor instead of a second scan. Test: all-down-while-progressing writes QuorumLost. Assisted-By: LLM Signed-off-by: Andrey Kolkov --- controllers/etcdcluster_controller.go | 46 +++++++++++++------ controllers/etcdcluster_controller_test.go | 53 ++++++++++++++++++++++ 2 files changed, 85 insertions(+), 14 deletions(-) diff --git a/controllers/etcdcluster_controller.go b/controllers/etcdcluster_controller.go index 95d54118..88755c94 100644 --- a/controllers/etcdcluster_controller.go +++ b/controllers/etcdcluster_controller.go @@ -1362,13 +1362,29 @@ func (r *EtcdClusterReconciler) updateStatus( dormant := findDormantMember(members) ready := int32(0) + // Voter accounting from Status.IsVoter (synced from etcd's MemberList in + // promotePendingLearner; sticky at its last value while etcd is + // unreachable). readyVoters is the live quorum count — it decides both the + // progressing withhold below and the PodDisruptionBudget floor. + voters := int32(0) + readyVoters := int32(0) for _, m := range running { + memberReady := false for _, c := range m.Status.Conditions { if c.Type == lll.MemberReady && c.Status == metav1.ConditionTrue { - ready++ + memberReady = true break } } + if memberReady { + ready++ + } + if m.Status.IsVoter { + voters++ + if memberReady { + readyVoters++ + } + } } changed := false @@ -1404,8 +1420,17 @@ func (r *EtcdClusterReconciler) updateStatus( // health. This matches the allMembersReady early-return in Reconcile, which // already suppresses the same write for the earlier learners of the same // scale-up — so every mid-scale-up window reports alike. + // + // The withhold is floored on quorum: it holds only while the ready voters + // still carry the live voter set (readyVoters > voters/2). Progressing can + // latch indefinitely — spec.auth.enabled looping on a bad root Secret, or a + // learner whose MemberPromote keeps being rejected — so an unfloored + // withhold would freeze Available=True on a cluster that then loses every + // pod, reopening #367. Once quorum is gone the switch falls through and + // writes QuorumLost. The general joining-member denominator is #371. paused := desired == 0 - progressing := !paused && clusterProgressing(cluster) && !reconciliationComplete(cluster, running) + quorumHeld := readyVoters > voters/2 + progressing := !paused && quorumHeld && clusterProgressing(cluster) && !reconciliationComplete(cluster, running) switch { case paused: // Three flavours of paused: @@ -1507,18 +1532,11 @@ func (r *EtcdClusterReconciler) updateStatus( changed = true } - // Reconcile PDB. Voter count comes from members' Status.IsVoter - // (written by this controller from etcd's MemberList in - // promotePendingLearner). On a brand-new cluster pre-bootstrap, no - // member has IsVoter=true yet — voterCount=0 and no PDB is emitted - // until the seed reaches its Status.IsVoter=true pre-stamp. - voterCount := int32(0) - for _, m := range running { - if m.Status.IsVoter { - voterCount++ - } - } - if err := r.reconcilePDB(ctx, cluster, voterCount); err != nil { + // Reconcile PDB. Voter count comes from members' Status.IsVoter (counted + // above). On a brand-new cluster pre-bootstrap, no member has IsVoter=true + // yet — voters=0 and no PDB is emitted until the seed reaches its + // Status.IsVoter=true pre-stamp. + if err := r.reconcilePDB(ctx, cluster, voters); err != nil { log.FromContext(ctx).Error(err, "failed to reconcile PodDisruptionBudget") // Non-fatal: status update still runs; next reconcile retries. } diff --git a/controllers/etcdcluster_controller_test.go b/controllers/etcdcluster_controller_test.go index ee424c69..97f539b9 100644 --- a/controllers/etcdcluster_controller_test.go +++ b/controllers/etcdcluster_controller_test.go @@ -756,6 +756,59 @@ func TestUpdateStatus_NoDegradedFlapDuringScaleUp(t *testing.T) { } } +// TestUpdateStatus_AllDownWhileProgressingWritesQuorumLost is the quorum floor +// on the progressing withhold. A cluster that latches Progressing=True and then +// loses every pod (spec.auth.enabled looping on a bad root Secret, or an +// unpromotable learner — both hold Progressing via the pending gate) must not +// keep its stale Available=True: with no ready voter left, quorum is gone and +// the switch must write QuorumLost. Without the floor this is the #367 freeze +// again, now with ReadyMembers=0 contradicting Available=True on one object. +func TestUpdateStatus_AllDownWhileProgressingWritesQuorumLost(t *testing.T) { + ctx := context.Background() + cluster := &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: "test", + ClusterID: "deadbeef", + Observed: &lll.ObservedClusterSpec{ + Replicas: 3, Version: "3.5.17", Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, + }, + ProgressDeadline: &metav1.Time{Time: metav1.Now().Add(time.Hour)}, + Conditions: []metav1.Condition{ + {Type: lll.ClusterProgressing, Status: metav1.ConditionTrue, Reason: "SpecChanged", LastTransitionTime: metav1.Now()}, + {Type: lll.ClusterAvailable, Status: metav1.ConditionTrue, Reason: "QuorumHealthy", Message: "All members are ready", LastTransitionTime: metav1.Now()}, + {Type: lll.ClusterDegraded, Status: metav1.ConditionFalse, Reason: "QuorumHealthy", LastTransitionTime: metav1.Now()}, + }, + }, + } + // Every pod down: no ready voter holds the live voter set. + members := scaleUpMembers(t, "test", "ns", 0, 3) + c, _ := newTestClient(t, append([]client.Object{cluster}, membersAsObjects(members)...)...) + r := &EtcdClusterReconciler{Client: c, Scheme: testScheme(t), EtcdClientFactory: factoryReturning(newFakeEtcd(0xdead))} + + if _, err := r.updateStatus(ctx, cluster, members, &ctrl.Result{RequeueAfter: 10 * time.Second}); err != nil { + t.Fatalf("updateStatus: %v", err) + } + mustGet(t, c, "test", "ns", cluster) + + av := meta.FindStatusCondition(cluster.Status.Conditions, lll.ClusterAvailable) + if av == nil || av.Status != metav1.ConditionFalse { + t.Fatalf("Available = %+v, want False (quorum lost must break the progressing withhold)", av) + } + deg := meta.FindStatusCondition(cluster.Status.Conditions, lll.ClusterDegraded) + if deg == nil || deg.Status != metav1.ConditionTrue { + t.Fatalf("Degraded = %+v, want True (quorum lost)", deg) + } + if cluster.Status.ReadyMembers != 0 { + t.Fatalf("ReadyMembers = %d, want 0", cluster.Status.ReadyMembers) + } +} + // TestUpdateStatus_WithholdsReconciledWhilePendingRetry covers review item 2: // reconciliationComplete only checks MemberReady, so a learner whose pod is // Ready but whose MemberPromote keeps being rejected (pending set) satisfies From 3b4e3696d1abe5577efa0d59d6a17205c8c6c476 Mon Sep 17 00:00:00 2001 From: Andrey Kolkov Date: Fri, 18 Sep 2026 16:59:08 +0400 Subject: [PATCH 6/7] test(controllers): pin the quorum floor on the sticky-voter down shape The all-down-while-progressing test only drove the fixture shape where down members carry IsVoter=false (voters=0). The shape the fix actually targets is etcd unreachable: syncIsVoter cannot refresh the MemberList, so IsVoter stays sticky-true while only MemberReady flips false (voters=3, readyVoters=0). Table-drive the test over both shapes via a new downVoterMembers helper; the floor writes QuorumLost in each, and dropping it reddens both subcases. Assisted-By: LLM Signed-off-by: Andrey Kolkov --- controllers/etcdcluster_controller_test.go | 119 ++++++++++++++------- 1 file changed, 80 insertions(+), 39 deletions(-) diff --git a/controllers/etcdcluster_controller_test.go b/controllers/etcdcluster_controller_test.go index 97f539b9..11571b22 100644 --- a/controllers/etcdcluster_controller_test.go +++ b/controllers/etcdcluster_controller_test.go @@ -692,6 +692,31 @@ func scaleUpMembers(t *testing.T, name, ns string, nReady, nNotReady int) []lll. return out } +// downVoterMembers builds n members that are voters (IsVoter=true) but not +// Ready — the etcd-unreachable shape, where syncIsVoter cannot refresh the +// MemberList so IsVoter stays at its last value while pod readiness flips off. +// scaleUpMembers cannot express this (it ties IsVoter to readiness). +func downVoterMembers(t *testing.T, name, ns string, n int) []lll.EtcdMember { + t.Helper() + out := make([]lll.EtcdMember, 0, n) + for i := 0; i < n; i++ { + mn := fmt.Sprintf("%s-%d", name, i) + out = append(out, lll.EtcdMember{ + ObjectMeta: metav1.ObjectMeta{Name: mn, Namespace: ns, Labels: memberLabels(name, mn)}, + Spec: lll.EtcdMemberSpec{ClusterName: name, Version: "3.5.17", Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, InitialCluster: "x", ClusterToken: name}, + Status: lll.EtcdMemberStatus{ + PodName: mn, + MemberID: "abc", + IsVoter: true, + Conditions: []metav1.Condition{{ + Type: lll.MemberReady, Status: metav1.ConditionFalse, Reason: "PodNotReady", LastTransitionTime: metav1.Now(), + }}, + }, + }) + } + return out +} + // membersAsObjects adapts a member slice for newTestClient's variadic seed. func membersAsObjects(members []lll.EtcdMember) []client.Object { out := make([]client.Object, 0, len(members)) @@ -764,48 +789,64 @@ func TestUpdateStatus_NoDegradedFlapDuringScaleUp(t *testing.T) { // the switch must write QuorumLost. Without the floor this is the #367 freeze // again, now with ReadyMembers=0 contradicting Available=True on one object. func TestUpdateStatus_AllDownWhileProgressingWritesQuorumLost(t *testing.T) { - ctx := context.Background() - cluster := &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: "test", - ClusterID: "deadbeef", - Observed: &lll.ObservedClusterSpec{ - Replicas: 3, Version: "3.5.17", Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, - }, - ProgressDeadline: &metav1.Time{Time: metav1.Now().Add(time.Hour)}, - Conditions: []metav1.Condition{ - {Type: lll.ClusterProgressing, Status: metav1.ConditionTrue, Reason: "SpecChanged", LastTransitionTime: metav1.Now()}, - {Type: lll.ClusterAvailable, Status: metav1.ConditionTrue, Reason: "QuorumHealthy", Message: "All members are ready", LastTransitionTime: metav1.Now()}, - {Type: lll.ClusterDegraded, Status: metav1.ConditionFalse, Reason: "QuorumHealthy", LastTransitionTime: metav1.Now()}, - }, - }, + // Two shapes of "every pod down while Progressing is latched". The floor + // must break the withhold in both, because readyVoters cannot exceed + // voters/2 when no voter is ready: + // - fixture shape: the down members carry IsVoter=false (voters=0); + // - production shape: etcd is unreachable, so syncIsVoter leaves IsVoter + // sticky-true and only MemberReady flips false (voters=3, readyVoters=0) + // — the exact state the fix targets. + cases := []struct { + name string + members []lll.EtcdMember + }{ + {"down members are non-voters", scaleUpMembers(t, "test", "ns", 0, 3)}, + {"sticky voters, pods not ready", downVoterMembers(t, "test", "ns", 3)}, } - // Every pod down: no ready voter holds the live voter set. - members := scaleUpMembers(t, "test", "ns", 0, 3) - c, _ := newTestClient(t, append([]client.Object{cluster}, membersAsObjects(members)...)...) - r := &EtcdClusterReconciler{Client: c, Scheme: testScheme(t), EtcdClientFactory: factoryReturning(newFakeEtcd(0xdead))} + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + cluster := &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: "test", + ClusterID: "deadbeef", + Observed: &lll.ObservedClusterSpec{ + Replicas: 3, Version: "3.5.17", Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, + }, + ProgressDeadline: &metav1.Time{Time: metav1.Now().Add(time.Hour)}, + Conditions: []metav1.Condition{ + {Type: lll.ClusterProgressing, Status: metav1.ConditionTrue, Reason: "SpecChanged", LastTransitionTime: metav1.Now()}, + {Type: lll.ClusterAvailable, Status: metav1.ConditionTrue, Reason: "QuorumHealthy", Message: "All members are ready", LastTransitionTime: metav1.Now()}, + {Type: lll.ClusterDegraded, Status: metav1.ConditionFalse, Reason: "QuorumHealthy", LastTransitionTime: metav1.Now()}, + }, + }, + } + c, _ := newTestClient(t, append([]client.Object{cluster}, membersAsObjects(tc.members)...)...) + r := &EtcdClusterReconciler{Client: c, Scheme: testScheme(t), EtcdClientFactory: factoryReturning(newFakeEtcd(0xdead))} - if _, err := r.updateStatus(ctx, cluster, members, &ctrl.Result{RequeueAfter: 10 * time.Second}); err != nil { - t.Fatalf("updateStatus: %v", err) - } - mustGet(t, c, "test", "ns", cluster) + if _, err := r.updateStatus(ctx, cluster, tc.members, &ctrl.Result{RequeueAfter: 10 * time.Second}); err != nil { + t.Fatalf("updateStatus: %v", err) + } + mustGet(t, c, "test", "ns", cluster) - av := meta.FindStatusCondition(cluster.Status.Conditions, lll.ClusterAvailable) - if av == nil || av.Status != metav1.ConditionFalse { - t.Fatalf("Available = %+v, want False (quorum lost must break the progressing withhold)", av) - } - deg := meta.FindStatusCondition(cluster.Status.Conditions, lll.ClusterDegraded) - if deg == nil || deg.Status != metav1.ConditionTrue { - t.Fatalf("Degraded = %+v, want True (quorum lost)", deg) - } - if cluster.Status.ReadyMembers != 0 { - t.Fatalf("ReadyMembers = %d, want 0", cluster.Status.ReadyMembers) + av := meta.FindStatusCondition(cluster.Status.Conditions, lll.ClusterAvailable) + if av == nil || av.Status != metav1.ConditionFalse { + t.Fatalf("Available = %+v, want False (quorum lost must break the progressing withhold)", av) + } + deg := meta.FindStatusCondition(cluster.Status.Conditions, lll.ClusterDegraded) + if deg == nil || deg.Status != metav1.ConditionTrue { + t.Fatalf("Degraded = %+v, want True (quorum lost)", deg) + } + if cluster.Status.ReadyMembers != 0 { + t.Fatalf("ReadyMembers = %d, want 0", cluster.Status.ReadyMembers) + } + }) } } From 0c210ad24ca78200462b7465309c6482e9f68cb1 Mon Sep 17 00:00:00 2001 From: Timofei Larkin Date: Tue, 22 Sep 2026 09:52:41 +0300 Subject: [PATCH 7/7] fix(controllers): withhold progressing health only while every voter is Ready The progressing withhold in updateStatus was floored on quorum (readyVoters > voters/2). That keeps a total outage truthful, but a single established voter losing its pod while Progressing is latched still left the cluster reading Available=True/QuorumHealthy "All members are ready" with Degraded=False, because the two surviving voters hold quorum. The state the withhold exists for is "a learner is still joining", and the predicate for that is every established voter being Ready: the only not-Ready members are joiners. Key the withhold on that instead. A not-Ready voter now falls through to the health switch mid-progression and surfaces as Available=True/QuorumAvailable plus Degraded=True/MembersUnhealthy. The all-down shapes still fall through to QuorumLost, and the scale-up flap case (two Ready voters, one joining non-voter) is still withheld. Adds TestUpdateStatus_VoterDownWhileProgressingWritesDegraded for the minority-voter shape. Signed-off-by: Timofei Larkin --- controllers/etcdcluster_controller.go | 25 +++++----- controllers/etcdcluster_controller_test.go | 55 ++++++++++++++++++++++ 2 files changed, 69 insertions(+), 11 deletions(-) diff --git a/controllers/etcdcluster_controller.go b/controllers/etcdcluster_controller.go index 88755c94..4d973f93 100644 --- a/controllers/etcdcluster_controller.go +++ b/controllers/etcdcluster_controller.go @@ -1364,8 +1364,8 @@ func (r *EtcdClusterReconciler) updateStatus( ready := int32(0) // Voter accounting from Status.IsVoter (synced from etcd's MemberList in // promotePendingLearner; sticky at its last value while etcd is - // unreachable). readyVoters is the live quorum count — it decides both the - // progressing withhold below and the PodDisruptionBudget floor. + // unreachable). voters feeds the PodDisruptionBudget floor; readyVoters + // against voters decides the progressing withhold below. voters := int32(0) readyVoters := int32(0) for _, m := range running { @@ -1421,16 +1421,19 @@ func (r *EtcdClusterReconciler) updateStatus( // already suppresses the same write for the earlier learners of the same // scale-up — so every mid-scale-up window reports alike. // - // The withhold is floored on quorum: it holds only while the ready voters - // still carry the live voter set (readyVoters > voters/2). Progressing can - // latch indefinitely — spec.auth.enabled looping on a bad root Secret, or a - // learner whose MemberPromote keeps being rejected — so an unfloored - // withhold would freeze Available=True on a cluster that then loses every - // pod, reopening #367. Once quorum is gone the switch falls through and - // writes QuorumLost. The general joining-member denominator is #371. + // The withhold holds only while every established voter is Ready, i.e. the + // only not-Ready members are joiners that etcd has not promoted yet. A + // not-Ready voter is a real failure and must fall through to the health + // switch even mid-progression: Progressing can latch indefinitely — + // spec.auth.enabled looping on a bad root Secret, or a learner whose + // MemberPromote keeps being rejected — so a withhold keyed on quorum alone + // would hide a minority voter loss under "All members are ready", and a + // withhold with no floor at all would freeze Available=True on a cluster + // that then loses every pod, reopening #367. The general joining-member + // denominator is #371. paused := desired == 0 - quorumHeld := readyVoters > voters/2 - progressing := !paused && quorumHeld && clusterProgressing(cluster) && !reconciliationComplete(cluster, running) + votersHealthy := voters > 0 && readyVoters == voters + progressing := !paused && votersHealthy && clusterProgressing(cluster) && !reconciliationComplete(cluster, running) switch { case paused: // Three flavours of paused: diff --git a/controllers/etcdcluster_controller_test.go b/controllers/etcdcluster_controller_test.go index 11571b22..74ac7caa 100644 --- a/controllers/etcdcluster_controller_test.go +++ b/controllers/etcdcluster_controller_test.go @@ -850,6 +850,61 @@ func TestUpdateStatus_AllDownWhileProgressingWritesQuorumLost(t *testing.T) { } } +// TestUpdateStatus_VoterDownWhileProgressingWritesDegraded pins the shape +// between the flap test and the all-down test: Progressing is latched and +// quorum still holds, but the not-Ready member is an established voter, not a +// joiner. The withhold is for joiners only — a voter that lost its pod is a +// real minority failure and must surface as Degraded=True/MembersUnhealthy +// rather than hide under the stale "All members are ready". +func TestUpdateStatus_VoterDownWhileProgressingWritesDegraded(t *testing.T) { + ctx := context.Background() + cluster := &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: "test", + ClusterID: "deadbeef", + Observed: &lll.ObservedClusterSpec{ + Replicas: 3, Version: "3.5.17", Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, + }, + ProgressDeadline: &metav1.Time{Time: metav1.Now().Add(time.Hour)}, + ReadyMembers: 3, + Conditions: []metav1.Condition{ + {Type: lll.ClusterProgressing, Status: metav1.ConditionTrue, Reason: "SpecChanged", LastTransitionTime: metav1.Now()}, + {Type: lll.ClusterAvailable, Status: metav1.ConditionTrue, Reason: "QuorumHealthy", Message: "All members are ready", LastTransitionTime: metav1.Now()}, + {Type: lll.ClusterDegraded, Status: metav1.ConditionFalse, Reason: "QuorumHealthy", LastTransitionTime: metav1.Now()}, + }, + }, + } + // Two Ready voters plus one not-Ready member that is an established voter + // (sticky IsVoter=true), not a joining learner. + members := scaleUpMembers(t, "test", "ns", 2, 1) + members[2].Status.IsVoter = true + c, _ := newTestClient(t, append([]client.Object{cluster}, membersAsObjects(members)...)...) + r := &EtcdClusterReconciler{Client: c, Scheme: testScheme(t), EtcdClientFactory: factoryReturning(newFakeEtcd(0xdead))} + + if _, err := r.updateStatus(ctx, cluster, members, &ctrl.Result{RequeueAfter: 10 * time.Second}); err != nil { + t.Fatalf("updateStatus: %v", err) + } + mustGet(t, c, "test", "ns", cluster) + + deg := meta.FindStatusCondition(cluster.Status.Conditions, lll.ClusterDegraded) + if deg == nil || deg.Status != metav1.ConditionTrue || deg.Reason != "MembersUnhealthy" { + t.Fatalf("Degraded = %+v, want True/MembersUnhealthy (a down voter is not a joiner)", deg) + } + av := meta.FindStatusCondition(cluster.Status.Conditions, lll.ClusterAvailable) + if av == nil || av.Status != metav1.ConditionTrue || av.Reason != "QuorumAvailable" { + t.Fatalf("Available = %+v, want True/QuorumAvailable (quorum holds on 2/3)", av) + } + if cluster.Status.ReadyMembers != 2 { + t.Fatalf("ReadyMembers = %d, want 2", cluster.Status.ReadyMembers) + } +} + // TestUpdateStatus_WithholdsReconciledWhilePendingRetry covers review item 2: // reconciliationComplete only checks MemberReady, so a learner whose pod is // Ready but whose MemberPromote keeps being rejected (pending set) satisfies