From aa108186c20a608a8216be9cbb188ab06028cf90 Mon Sep 17 00:00:00 2001 From: Andrey Kolkov Date: Tue, 15 Sep 2026 19:19:39 +0400 Subject: [PATCH 1/3] fix(controllers): replace EtcdMember Pods that reach a terminal phase MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A graceful node shutdown SIGTERMs etcd, which exits 0, and the kubelet moves the Pod to a terminal phase (Succeeded, or Failed on a non-zero exit) without deleting it. The kubelet never restarts containers in a terminal Pod, and the operator manages bare Pods rather than a StatefulSet, so ensurePod — which only recreates on NotFound — treated the Succeeded Pod as present and left the member down permanently. This was seen in production after all nodes of a cluster were rebooted at once: every etcd Pod was Succeeded and etcd never recovered until the Pods were deleted by hand. The crash-loop detector didn't help (it needs 5+ restarts; a clean exit has zero) and neither did the pod-loss detector (it fires only on a missing or UID-changed Pod). Delete an owned Pod that has reached a terminal phase and has no deletionTimestamp, then requeue. The next reconcile recreates it: a PVC-backed member resumes from its data dir with the same member ID; a memory-backed member falls into the existing pod-loss path once the Pod is gone (its tmpfs data was lost, so it is replaced, not recreated). A Pod already terminating is left to finish, so a manual restart, drain, or eviction is untouched. Not quorum-gated: a whole-cluster reboot lands every member here at once and all must be free to recreate. Fixes #368 Assisted-By: LLM Signed-off-by: Andrey Kolkov --- controllers/etcdmember_controller.go | 46 ++++++ controllers/etcdmember_controller_test.go | 184 ++++++++++++++++++++++ 2 files changed, 230 insertions(+) diff --git a/controllers/etcdmember_controller.go b/controllers/etcdmember_controller.go index d116cf86..f35dfe17 100644 --- a/controllers/etcdmember_controller.go +++ b/controllers/etcdmember_controller.go @@ -161,6 +161,23 @@ func (r *EtcdMemberReconciler) Reconcile(ctx context.Context, req ctrl.Request) } } + // A Pod that reached a terminal phase (Succeeded/Failed) without a + // deletionTimestamp will not come back on its own: the kubelet does not + // restart containers in a terminal Pod, and the operator manages bare + // Pods, not a StatefulSet. Graceful node shutdown is the trap — the + // kubelet SIGTERMs etcd, it exits 0, the Pod goes Succeeded, and the + // member stays down until the Pod is deleted. Delete it so the next + // reconcile recreates it: a PVC-backed member resumes from its data dir + // with the same member ID; a memory-backed member falls into the pod-loss + // path above once the Pod is gone. Not quorum-gated — a whole-cluster + // reboot lands every member here at once and all must recreate. + if deleted, err := r.deleteTerminalPod(ctx, member); err != nil { + log.Error(err, "failed to delete terminal-phase pod") + return ctrl.Result{}, err + } else if deleted { + return ctrl.Result{RequeueAfter: 2 * time.Second}, nil + } + if err := r.ensurePVC(ctx, member); err != nil { log.Error(err, "failed to ensure PVC") return ctrl.Result{}, err @@ -190,6 +207,35 @@ func (r *EtcdMemberReconciler) memoryMemberPodLost(ctx context.Context, member * return string(pod.UID) != member.Status.PodUID, nil } +// podInTerminalPhase reports whether the Pod has run to a terminal phase +// (Succeeded or Failed) and so will never be restarted by the kubelet. +func podInTerminalPhase(pod *corev1.Pod) bool { + return pod.Status.Phase == corev1.PodSucceeded || pod.Status.Phase == corev1.PodFailed +} + +// deleteTerminalPod deletes the member's Pod when it has reached a terminal +// phase without a deletionTimestamp, reporting whether it issued the delete. +// A Pod already terminating is left to finish (a manual restart, drain, or +// eviction is on its way to a clean reschedule). Only a Pod this member owns +// is touched. +func (r *EtcdMemberReconciler) deleteTerminalPod(ctx context.Context, member *lll.EtcdMember) (bool, error) { + pod := &corev1.Pod{} + err := r.Get(ctx, types.NamespacedName{Namespace: member.Namespace, Name: member.Name}, pod) + if errors.IsNotFound(err) { + return false, nil + } + if err != nil { + return false, err + } + if !podOwnedBy(pod, member) || pod.DeletionTimestamp != nil || !podInTerminalPhase(pod) { + return false, nil + } + if err := r.Delete(ctx, pod); err != nil && !errors.IsNotFound(err) { + return false, err + } + return true, nil +} + // ── Deletion ───────────────────────────────────────────────────────────── func (r *EtcdMemberReconciler) handleDeletion(ctx context.Context, member *lll.EtcdMember) (ctrl.Result, error) { diff --git a/controllers/etcdmember_controller_test.go b/controllers/etcdmember_controller_test.go index 2568dcb3..34f5be4e 100644 --- a/controllers/etcdmember_controller_test.go +++ b/controllers/etcdmember_controller_test.go @@ -2263,6 +2263,190 @@ func TestReconcile_MemoryMemberStablePodIsNotLost(t *testing.T) { } } +func TestPodInTerminalPhase(t *testing.T) { + cases := []struct { + phase corev1.PodPhase + want bool + }{ + {corev1.PodRunning, false}, + {corev1.PodPending, false}, + {corev1.PodUnknown, false}, + {corev1.PodSucceeded, true}, + {corev1.PodFailed, true}, + } + for _, tc := range cases { + pod := &corev1.Pod{Status: corev1.PodStatus{Phase: tc.phase}} + if got := podInTerminalPhase(pod); got != tc.want { + t.Fatalf("podInTerminalPhase(%s) = %v, want %v", tc.phase, got, tc.want) + } + } +} + +// A PVC-backed member whose Pod reached a terminal phase (graceful node +// shutdown: etcd caught SIGTERM, exited 0, the Pod went Succeeded) must have +// that Pod deleted and recreated against the same PVC — the kubelet will not +// restart a terminal Pod on its own, so without this the member stays down. +func TestReconcile_ReplacesTerminalPhasePod(t *testing.T) { + ctx := context.Background() + tru := true + + member := &lll.EtcdMember{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-0", Namespace: "ns", UID: types.UID("member-uid"), + Labels: memberLabels("test", "test-0"), + Finalizers: []string{MemberFinalizer}, + }, + Spec: lll.EtcdMemberSpec{ + ClusterName: "test", Version: "3.5.17", Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, + InitialCluster: "x", ClusterToken: "ns-test-x", Bootstrap: true, + }, + Status: lll.EtcdMemberStatus{PodName: "test-0", PodUID: "old-uid", PVCName: "data-test-0"}, + } + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-0", Namespace: "ns", UID: types.UID("old-uid"), + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: "etcd-operator.cozystack.io/v1alpha2", Kind: "EtcdMember", + Name: "test-0", UID: types.UID("member-uid"), Controller: &tru, BlockOwnerDeletion: &tru, + }}, + }, + Status: corev1.PodStatus{Phase: corev1.PodSucceeded}, + } + pvc := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "data-test-0", Namespace: "ns", + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: "etcd-operator.cozystack.io/v1alpha2", Kind: "EtcdMember", + Name: "test-0", UID: types.UID("member-uid"), Controller: &tru, BlockOwnerDeletion: &tru, + }}, + }, + } + c, _ := newTestClient(t, member, pod, pvc) + r := &EtcdMemberReconciler{Client: c, Scheme: testScheme(t), EtcdClientFactory: factoryReturning(newFakeEtcd(0xdead))} + req := ctrl.Request{NamespacedName: types.NamespacedName{Name: "test-0", Namespace: "ns"}} + + // Pass 1: the terminal Pod is deleted. + if _, err := r.Reconcile(ctx, req); err != nil { + t.Fatalf("Reconcile (pass 1): %v", err) + } + if err := c.Get(ctx, types.NamespacedName{Namespace: "ns", Name: "test-0"}, &corev1.Pod{}); !apierrors.IsNotFound(err) { + t.Fatalf("terminal Pod must be deleted; got err=%v", err) + } + + // Pass 2: a fresh Pod is recreated against the existing PVC. + if _, err := r.Reconcile(ctx, req); err != nil { + t.Fatalf("Reconcile (pass 2): %v", err) + } + fresh := mustGet(t, c, "test-0", "ns", &corev1.Pod{}) + if fresh.UID == types.UID("old-uid") { + t.Fatalf("Pod must be recreated with a new UID; still old-uid") + } + gotPVC := mustGet(t, c, "data-test-0", "ns", &corev1.PersistentVolumeClaim{}) + if !pvcOwnedBy(gotPVC, member) { + t.Fatalf("PVC must be preserved and still owned by the member; got %+v", gotPVC.OwnerReferences) + } +} + +// A memory-backed member's terminal Pod is deleted too, converting the +// "Succeeded, same UID" state into the Pod-gone state the pod-loss path +// already handles: data is lost with the tmpfs, so the member is replaced +// rather than recreated in place. +func TestReconcile_MemoryMemberTerminalPodTriggersReplacement(t *testing.T) { + ctx := context.Background() + tru := true + + member := &lll.EtcdMember{ + ObjectMeta: metav1.ObjectMeta{ + Name: "m-1", Namespace: "ns", UID: types.UID("mu"), + Labels: memberLabels("test", "m-1"), + Finalizers: []string{MemberFinalizer}, + }, + Spec: lll.EtcdMemberSpec{ + ClusterName: "test", Version: "3.5.17", + Storage: lll.StorageSpec{Size: quickQty(t, "1Gi"), Medium: lll.StorageMediumMemory}, + InitialCluster: "m-1=" + peerURL("http", "m-1", "test", "ns"), + ClusterToken: "ns-test-x", Bootstrap: true, + }, + Status: lll.EtcdMemberStatus{PodName: "m-1", PodUID: "stable-uid"}, + } + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "m-1", Namespace: "ns", UID: types.UID("stable-uid"), + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: "etcd-operator.cozystack.io/v1alpha2", Kind: "EtcdMember", + Name: "m-1", UID: types.UID("mu"), Controller: &tru, BlockOwnerDeletion: &tru, + }}, + }, + Status: corev1.PodStatus{Phase: corev1.PodSucceeded}, + } + c, _ := newTestClient(t, member, pod) + r := &EtcdMemberReconciler{Client: c, Scheme: testScheme(t), EtcdClientFactory: factoryReturning(newFakeEtcd(0xdead))} + req := ctrl.Request{NamespacedName: types.NamespacedName{Name: "m-1", Namespace: "ns"}} + + // Pass 1: terminal Pod deleted (memory pod-loss saw a same-UID Pod, so it + // did not fire yet). + if _, err := r.Reconcile(ctx, req); err != nil { + t.Fatalf("Reconcile (pass 1): %v", err) + } + if err := c.Get(ctx, types.NamespacedName{Namespace: "ns", Name: "m-1"}, &corev1.Pod{}); !apierrors.IsNotFound(err) { + t.Fatalf("terminal Pod must be deleted; got err=%v", err) + } + + // Pass 2: Pod now gone → member is deleted for replacement, and no fresh + // tmpfs-backed Pod is created. + if _, err := r.Reconcile(ctx, req); err != nil { + t.Fatalf("Reconcile (pass 2): %v", err) + } + got := &lll.EtcdMember{} + err := c.Get(ctx, types.NamespacedName{Name: "m-1", Namespace: "ns"}, got) + switch { + case apierrors.IsNotFound(err): + case err != nil: + t.Fatalf("Get(member): %v", err) + case got.DeletionTimestamp.IsZero(): + t.Fatalf("memory member must be marked for deletion after its Pod is lost") + } + if err := c.Get(ctx, types.NamespacedName{Namespace: "ns", Name: "m-1"}, &corev1.Pod{}); !apierrors.IsNotFound(err) { + t.Fatalf("no fresh Pod must be created for a memory member being replaced; got err=%v", err) + } +} + +// A Pod already terminating (deletionTimestamp set — manual restart, drain, +// eviction) must be left to finish, not re-deleted as a terminal Pod. +func TestDeleteTerminalPod_SkipsPodBeingDeleted(t *testing.T) { + ctx := context.Background() + tru := true + + member := &lll.EtcdMember{ + ObjectMeta: metav1.ObjectMeta{Name: "test-0", Namespace: "ns", UID: types.UID("member-uid")}, + } + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-0", Namespace: "ns", UID: types.UID("old-uid"), + Finalizers: []string{"keep/terminating"}, + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: "etcd-operator.cozystack.io/v1alpha2", Kind: "EtcdMember", + Name: "test-0", UID: types.UID("member-uid"), Controller: &tru, BlockOwnerDeletion: &tru, + }}, + }, + Status: corev1.PodStatus{Phase: corev1.PodSucceeded}, + } + c, _ := newTestClient(t, member, pod) + // Stamp a deletionTimestamp: the finalizer keeps the Pod present. + if err := c.Delete(ctx, pod); err != nil { + t.Fatalf("Delete(pod): %v", err) + } + r := &EtcdMemberReconciler{Client: c, Scheme: testScheme(t)} + + deleted, err := r.deleteTerminalPod(ctx, member) + if err != nil { + t.Fatalf("deleteTerminalPod: %v", err) + } + if deleted { + t.Fatalf("a Pod already terminating must not be treated as a terminal Pod to delete") + } +} + // TestUpdateStatus_MemoryMemberLeavesPVCNameEmpty: even after a full // reconcile pass, a memory member's Status.PVCName must stay empty so // downstream consumers (the EtcdCluster's Paused message in particular, From 22d5d0520712d2264ca45a5828579e9746170c23 Mon Sep 17 00:00:00 2001 From: Andrey Kolkov Date: Tue, 22 Sep 2026 12:12:56 +0400 Subject: [PATCH 2/3] fix(controllers): flip MemberReady=False when a terminal Pod is replaced Deleting a terminal-phase Pod returned before any status write. When the re-creation then failed (missing TLS Secret, quota) updateStatus never ran, so the member kept advertising MemberReady=True with no Pod and inflated the cluster's readyMembers count that the crash-loop quorum gate reads. Set Ready=False with reason PodReplacing in the same pass, keeping the phase and reason the Pod died with, and log the delete like the other destructive paths in Reconcile. Status.PodUID is preserved so the memory pod-loss gate still fires on the next pass. Pin the ownership conjunct of the delete guard with its own test. Assisted-By: LLM Signed-off-by: Andrey Kolkov --- controllers/etcdmember_controller.go | 51 ++++++++--- controllers/etcdmember_controller_test.go | 106 +++++++++++++++++++++- 2 files changed, 144 insertions(+), 13 deletions(-) diff --git a/controllers/etcdmember_controller.go b/controllers/etcdmember_controller.go index f35dfe17..f3011535 100644 --- a/controllers/etcdmember_controller.go +++ b/controllers/etcdmember_controller.go @@ -171,10 +171,26 @@ func (r *EtcdMemberReconciler) Reconcile(ctx context.Context, req ctrl.Request) // with the same member ID; a memory-backed member falls into the pod-loss // path above once the Pod is gone. Not quorum-gated — a whole-cluster // reboot lands every member here at once and all must recreate. - if deleted, err := r.deleteTerminalPod(ctx, member); err != nil { + // + // Ready flips to False here, not in updateStatus: if re-creation then + // fails (missing TLS Secret, quota) updateStatus never runs and the + // member would keep advertising Ready with no Pod, inflating the + // cluster's readyMembers that the crash-loop quorum gate reads. + // Status.PodUID is left as is — the memory pod-loss gate above needs it. + terminal, err := r.deleteTerminalPod(ctx, member) + if err != nil { log.Error(err, "failed to delete terminal-phase pod") return ctrl.Result{}, err - } else if deleted { + } + if terminal != nil { + log.Info("deleted terminal-phase pod for recreation", + "phase", terminal.Status.Phase, "reason", terminal.Status.Reason, "podUID", terminal.UID) + if setMemberCondition(member, lll.MemberReady, metav1.ConditionFalse, "PodReplacing", + terminalPodMessage(terminal)) { + if err := r.Status().Update(ctx, member); err != nil { + return ctrl.Result{}, err + } + } return ctrl.Result{RequeueAfter: 2 * time.Second}, nil } @@ -214,26 +230,37 @@ func podInTerminalPhase(pod *corev1.Pod) bool { } // deleteTerminalPod deletes the member's Pod when it has reached a terminal -// phase without a deletionTimestamp, reporting whether it issued the delete. -// A Pod already terminating is left to finish (a manual restart, drain, or -// eviction is on its way to a clean reschedule). Only a Pod this member owns -// is touched. -func (r *EtcdMemberReconciler) deleteTerminalPod(ctx context.Context, member *lll.EtcdMember) (bool, error) { +// phase without a deletionTimestamp and returns the Pod it deleted, or nil +// when there was nothing to delete. A Pod already terminating is left to +// finish (a manual restart, drain, or eviction is on its way to a clean +// reschedule). Only a Pod this member owns is touched. +func (r *EtcdMemberReconciler) deleteTerminalPod(ctx context.Context, member *lll.EtcdMember) (*corev1.Pod, error) { pod := &corev1.Pod{} err := r.Get(ctx, types.NamespacedName{Namespace: member.Namespace, Name: member.Name}, pod) if errors.IsNotFound(err) { - return false, nil + return nil, nil } if err != nil { - return false, err + return nil, err } if !podOwnedBy(pod, member) || pod.DeletionTimestamp != nil || !podInTerminalPhase(pod) { - return false, nil + return nil, nil } if err := r.Delete(ctx, pod); err != nil && !errors.IsNotFound(err) { - return false, err + return nil, err + } + return pod, nil +} + +// terminalPodMessage is the MemberReady=False message for a Pod deleted in a +// terminal phase; it keeps the phase and reason the Pod died with, which the +// fresh Pod's status no longer carries. +func terminalPodMessage(pod *corev1.Pod) string { + msg := fmt.Sprintf("pod reached terminal phase %s", pod.Status.Phase) + if pod.Status.Reason != "" { + msg += " (" + pod.Status.Reason + ")" } - return true, nil + return msg + "; deleted for recreation" } // ── Deletion ───────────────────────────────────────────────────────────── diff --git a/controllers/etcdmember_controller_test.go b/controllers/etcdmember_controller_test.go index 34f5be4e..834cb622 100644 --- a/controllers/etcdmember_controller_test.go +++ b/controllers/etcdmember_controller_test.go @@ -2442,11 +2442,115 @@ func TestDeleteTerminalPod_SkipsPodBeingDeleted(t *testing.T) { if err != nil { t.Fatalf("deleteTerminalPod: %v", err) } - if deleted { + if deleted != nil { t.Fatalf("a Pod already terminating must not be treated as a terminal Pod to delete") } } +// A terminal Pod of the same name that belongs to a different EtcdMember +// (a leftover from a previous generation awaiting GC) is not ours to delete. +func TestDeleteTerminalPod_SkipsPodOwnedByAnotherMember(t *testing.T) { + ctx := context.Background() + tru := true + + member := &lll.EtcdMember{ + ObjectMeta: metav1.ObjectMeta{Name: "test-0", Namespace: "ns", UID: types.UID("member-uid")}, + } + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-0", Namespace: "ns", UID: types.UID("old-uid"), + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: "etcd-operator.cozystack.io/v1alpha2", Kind: "EtcdMember", + Name: "test-0", UID: types.UID("previous-member-uid"), Controller: &tru, BlockOwnerDeletion: &tru, + }}, + }, + Status: corev1.PodStatus{Phase: corev1.PodFailed}, + } + c, _ := newTestClient(t, member, pod) + r := &EtcdMemberReconciler{Client: c, Scheme: testScheme(t)} + + deleted, err := r.deleteTerminalPod(ctx, member) + if err != nil { + t.Fatalf("deleteTerminalPod: %v", err) + } + if deleted != nil { + t.Fatalf("a terminal Pod owned by another EtcdMember must not be deleted") + } + if err := c.Get(ctx, types.NamespacedName{Namespace: "ns", Name: "test-0"}, &corev1.Pod{}); err != nil { + t.Fatalf("the other member's Pod must still exist; got err=%v", err) + } +} + +// Deleting the terminal Pod must flip MemberReady to False in the same pass. +// If re-creation then fails (here: the referenced TLS Secret is gone) the +// running flow never reaches updateStatus, and a member left at Ready=True +// with no Pod would inflate the cluster's readyMembers count that the +// crash-loop quorum gate reads. +func TestReconcile_TerminalPodDeleteFlipsReadyFalse(t *testing.T) { + ctx := context.Background() + tru := true + owner := []metav1.OwnerReference{{ + APIVersion: "etcd-operator.cozystack.io/v1alpha2", Kind: "EtcdMember", + Name: "test-0", UID: types.UID("member-uid"), Controller: &tru, BlockOwnerDeletion: &tru, + }} + + member := &lll.EtcdMember{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-0", Namespace: "ns", UID: types.UID("member-uid"), + Labels: memberLabels("test", "test-0"), + Finalizers: []string{MemberFinalizer}, + }, + Spec: lll.EtcdMemberSpec{ + ClusterName: "test", Version: "3.5.17", Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, + InitialCluster: "x", ClusterToken: "ns-test-x", Bootstrap: true, + TLS: &lll.EtcdMemberTLS{ClientServerSecretRef: &corev1.LocalObjectReference{Name: "missing-tls"}}, + }, + Status: lll.EtcdMemberStatus{PodName: "test-0", PodUID: "old-uid", PVCName: "data-test-0", MemberID: "abc"}, + } + setMemberCondition(member, lll.MemberReady, metav1.ConditionTrue, "PodReady", "etcd member is ready") + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "test-0", Namespace: "ns", UID: types.UID("old-uid"), OwnerReferences: owner}, + Status: corev1.PodStatus{Phase: corev1.PodFailed, Reason: "Terminated"}, + } + pvc := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{Name: "data-test-0", Namespace: "ns", OwnerReferences: owner}, + } + c, _ := newTestClient(t, member, pod, pvc) + r := &EtcdMemberReconciler{Client: c, Scheme: testScheme(t), EtcdClientFactory: factoryReturning(newFakeEtcd(0xdead))} + req := ctrl.Request{NamespacedName: types.NamespacedName{Name: "test-0", Namespace: "ns"}} + + readyCond := func() metav1.Condition { + got := mustGet(t, c, "test-0", "ns", &lll.EtcdMember{}) + for _, cond := range got.Status.Conditions { + if cond.Type == lll.MemberReady { + return cond + } + } + t.Fatalf("MemberReady condition missing: %+v", got.Status.Conditions) + return metav1.Condition{} + } + + // Pass 1: Pod deleted, Ready=False persisted with the phase it died in. + if _, err := r.Reconcile(ctx, req); err != nil { + t.Fatalf("Reconcile (pass 1): %v", err) + } + if cond := readyCond(); cond.Status != metav1.ConditionFalse || cond.Reason != "PodReplacing" || + !strings.Contains(cond.Message, "Failed") || !strings.Contains(cond.Message, "Terminated") { + t.Fatalf("after deleting the terminal Pod want Ready=False/PodReplacing naming Failed (Terminated); got %+v", cond) + } + if got := mustGet(t, c, "test-0", "ns", &lll.EtcdMember{}); got.Status.PodUID != "old-uid" { + t.Fatalf("Status.PodUID must be preserved for the memory pod-loss gate; got %q", got.Status.PodUID) + } + + // Pass 2: re-creation fails on the missing Secret; Ready must stay False. + if _, err := r.Reconcile(ctx, req); err == nil { + t.Fatalf("Reconcile (pass 2): expected the missing TLS Secret to block Pod creation") + } + if cond := readyCond(); cond.Status != metav1.ConditionFalse { + t.Fatalf("Ready must stay False while re-creation fails; got %+v", cond) + } +} + // TestUpdateStatus_MemoryMemberLeavesPVCNameEmpty: even after a full // reconcile pass, a memory member's Status.PVCName must stay empty so // downstream consumers (the EtcdCluster's Paused message in particular, From 16f07b29d1f9bbe3f929e768c85a225aed214ab7 Mon Sep 17 00:00:00 2001 From: Andrey Kolkov Date: Tue, 22 Sep 2026 14:21:49 +0400 Subject: [PATCH 3/3] fix(controllers): write MemberReady=False before deleting a terminal Pod The condition was written after the Pod delete, so a conflicting status write (the cluster controller patches member status too) returned an error with the Pod already gone; the retry found nothing terminal and the flip was lost. Split the delete into a predicate and the delete itself, and persist the condition first so a failed write leaves the terminal Pod in place as the retry trigger. Trim the comments to the reasons. Assisted-By: LLM Signed-off-by: Andrey Kolkov --- controllers/etcdmember_controller.go | 59 ++++------ controllers/etcdmember_controller_test.go | 135 ++++++++++++++++------ 2 files changed, 122 insertions(+), 72 deletions(-) diff --git a/controllers/etcdmember_controller.go b/controllers/etcdmember_controller.go index f3011535..f3e87112 100644 --- a/controllers/etcdmember_controller.go +++ b/controllers/etcdmember_controller.go @@ -161,36 +161,27 @@ func (r *EtcdMemberReconciler) Reconcile(ctx context.Context, req ctrl.Request) } } - // A Pod that reached a terminal phase (Succeeded/Failed) without a - // deletionTimestamp will not come back on its own: the kubelet does not - // restart containers in a terminal Pod, and the operator manages bare - // Pods, not a StatefulSet. Graceful node shutdown is the trap — the - // kubelet SIGTERMs etcd, it exits 0, the Pod goes Succeeded, and the - // member stays down until the Pod is deleted. Delete it so the next - // reconcile recreates it: a PVC-backed member resumes from its data dir - // with the same member ID; a memory-backed member falls into the pod-loss - // path above once the Pod is gone. Not quorum-gated — a whole-cluster - // reboot lands every member here at once and all must recreate. - // - // Ready flips to False here, not in updateStatus: if re-creation then - // fails (missing TLS Secret, quota) updateStatus never runs and the - // member would keep advertising Ready with no Pod, inflating the - // cluster's readyMembers that the crash-loop quorum gate reads. - // Status.PodUID is left as is — the memory pod-loss gate above needs it. - terminal, err := r.deleteTerminalPod(ctx, member) - if err != nil { - log.Error(err, "failed to delete terminal-phase pod") + // The kubelet never restarts a Pod in a terminal phase (a graceful node + // shutdown leaves etcd Succeeded) and nothing else replaces a bare Pod. + // Delete it so the next pass recreates it: PVC-backed resumes with the + // same member ID, memory-backed takes the pod-loss path above. Not + // quorum-gated: a whole-cluster reboot lands every member here at once. + // Ready=False is written before the delete so a failed write leaves the + // Pod in place as the retry trigger; PodUID stays for the pod-loss gate. + if pod, err := r.terminalPod(ctx, member); err != nil { return ctrl.Result{}, err - } - if terminal != nil { - log.Info("deleted terminal-phase pod for recreation", - "phase", terminal.Status.Phase, "reason", terminal.Status.Reason, "podUID", terminal.UID) + } else if pod != nil { if setMemberCondition(member, lll.MemberReady, metav1.ConditionFalse, "PodReplacing", - terminalPodMessage(terminal)) { + terminalPodMessage(pod)) { if err := r.Status().Update(ctx, member); err != nil { return ctrl.Result{}, err } } + log.Info("deleting terminal-phase pod for recreation", + "phase", pod.Status.Phase, "reason", pod.Status.Reason, "podUID", pod.UID) + if err := r.Delete(ctx, pod); err != nil && !errors.IsNotFound(err) { + return ctrl.Result{}, err + } return ctrl.Result{RequeueAfter: 2 * time.Second}, nil } @@ -223,18 +214,14 @@ func (r *EtcdMemberReconciler) memoryMemberPodLost(ctx context.Context, member * return string(pod.UID) != member.Status.PodUID, nil } -// podInTerminalPhase reports whether the Pod has run to a terminal phase -// (Succeeded or Failed) and so will never be restarted by the kubelet. +// podInTerminalPhase reports whether the kubelet will never restart the Pod. func podInTerminalPhase(pod *corev1.Pod) bool { return pod.Status.Phase == corev1.PodSucceeded || pod.Status.Phase == corev1.PodFailed } -// deleteTerminalPod deletes the member's Pod when it has reached a terminal -// phase without a deletionTimestamp and returns the Pod it deleted, or nil -// when there was nothing to delete. A Pod already terminating is left to -// finish (a manual restart, drain, or eviction is on its way to a clean -// reschedule). Only a Pod this member owns is touched. -func (r *EtcdMemberReconciler) deleteTerminalPod(ctx context.Context, member *lll.EtcdMember) (*corev1.Pod, error) { +// terminalPod returns the member's own Pod when it sits in a terminal phase +// and is not already terminating (drain, eviction, manual delete), else nil. +func (r *EtcdMemberReconciler) terminalPod(ctx context.Context, member *lll.EtcdMember) (*corev1.Pod, error) { pod := &corev1.Pod{} err := r.Get(ctx, types.NamespacedName{Namespace: member.Namespace, Name: member.Name}, pod) if errors.IsNotFound(err) { @@ -246,15 +233,11 @@ func (r *EtcdMemberReconciler) deleteTerminalPod(ctx context.Context, member *ll if !podOwnedBy(pod, member) || pod.DeletionTimestamp != nil || !podInTerminalPhase(pod) { return nil, nil } - if err := r.Delete(ctx, pod); err != nil && !errors.IsNotFound(err) { - return nil, err - } return pod, nil } -// terminalPodMessage is the MemberReady=False message for a Pod deleted in a -// terminal phase; it keeps the phase and reason the Pod died with, which the -// fresh Pod's status no longer carries. +// terminalPodMessage keeps the phase and reason the Pod died with; the +// replacement Pod's status will not carry them. func terminalPodMessage(pod *corev1.Pod) string { msg := fmt.Sprintf("pod reached terminal phase %s", pod.Status.Phase) if pod.Status.Reason != "" { diff --git a/controllers/etcdmember_controller_test.go b/controllers/etcdmember_controller_test.go index 834cb622..41d35bcb 100644 --- a/controllers/etcdmember_controller_test.go +++ b/controllers/etcdmember_controller_test.go @@ -24,9 +24,12 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" lll "github.com/cozystack/etcd-operator/api/v1alpha2" ) @@ -2282,10 +2285,8 @@ func TestPodInTerminalPhase(t *testing.T) { } } -// A PVC-backed member whose Pod reached a terminal phase (graceful node -// shutdown: etcd caught SIGTERM, exited 0, the Pod went Succeeded) must have -// that Pod deleted and recreated against the same PVC — the kubelet will not -// restart a terminal Pod on its own, so without this the member stays down. +// A PVC-backed member's terminal Pod is deleted and recreated against the +// same PVC. func TestReconcile_ReplacesTerminalPhasePod(t *testing.T) { ctx := context.Background() tru := true @@ -2347,10 +2348,8 @@ func TestReconcile_ReplacesTerminalPhasePod(t *testing.T) { } } -// A memory-backed member's terminal Pod is deleted too, converting the -// "Succeeded, same UID" state into the Pod-gone state the pod-loss path -// already handles: data is lost with the tmpfs, so the member is replaced -// rather than recreated in place. +// A memory-backed member's terminal Pod is deleted so the pod-loss path +// replaces the member instead of recreating it on an empty tmpfs. func TestReconcile_MemoryMemberTerminalPodTriggersReplacement(t *testing.T) { ctx := context.Background() tru := true @@ -2383,8 +2382,7 @@ func TestReconcile_MemoryMemberTerminalPodTriggersReplacement(t *testing.T) { r := &EtcdMemberReconciler{Client: c, Scheme: testScheme(t), EtcdClientFactory: factoryReturning(newFakeEtcd(0xdead))} req := ctrl.Request{NamespacedName: types.NamespacedName{Name: "m-1", Namespace: "ns"}} - // Pass 1: terminal Pod deleted (memory pod-loss saw a same-UID Pod, so it - // did not fire yet). + // Pass 1: terminal Pod deleted; pod-loss saw the same UID and stayed quiet. if _, err := r.Reconcile(ctx, req); err != nil { t.Fatalf("Reconcile (pass 1): %v", err) } @@ -2392,8 +2390,7 @@ func TestReconcile_MemoryMemberTerminalPodTriggersReplacement(t *testing.T) { t.Fatalf("terminal Pod must be deleted; got err=%v", err) } - // Pass 2: Pod now gone → member is deleted for replacement, and no fresh - // tmpfs-backed Pod is created. + // Pass 2: Pod gone, member deleted for replacement, no fresh Pod. if _, err := r.Reconcile(ctx, req); err != nil { t.Fatalf("Reconcile (pass 2): %v", err) } @@ -2411,9 +2408,8 @@ func TestReconcile_MemoryMemberTerminalPodTriggersReplacement(t *testing.T) { } } -// A Pod already terminating (deletionTimestamp set — manual restart, drain, -// eviction) must be left to finish, not re-deleted as a terminal Pod. -func TestDeleteTerminalPod_SkipsPodBeingDeleted(t *testing.T) { +// A Pod already terminating is left to finish. +func TestTerminalPod_SkipsPodBeingDeleted(t *testing.T) { ctx := context.Background() tru := true @@ -2438,18 +2434,17 @@ func TestDeleteTerminalPod_SkipsPodBeingDeleted(t *testing.T) { } r := &EtcdMemberReconciler{Client: c, Scheme: testScheme(t)} - deleted, err := r.deleteTerminalPod(ctx, member) + got, err := r.terminalPod(ctx, member) if err != nil { - t.Fatalf("deleteTerminalPod: %v", err) + t.Fatalf("terminalPod: %v", err) } - if deleted != nil { - t.Fatalf("a Pod already terminating must not be treated as a terminal Pod to delete") + if got != nil { + t.Fatalf("a Pod already terminating must not be reported as terminal") } } -// A terminal Pod of the same name that belongs to a different EtcdMember -// (a leftover from a previous generation awaiting GC) is not ours to delete. -func TestDeleteTerminalPod_SkipsPodOwnedByAnotherMember(t *testing.T) { +// A same-name terminal Pod owned by another EtcdMember is not ours. +func TestTerminalPod_SkipsPodOwnedByAnotherMember(t *testing.T) { ctx := context.Background() tru := true @@ -2469,23 +2464,18 @@ func TestDeleteTerminalPod_SkipsPodOwnedByAnotherMember(t *testing.T) { c, _ := newTestClient(t, member, pod) r := &EtcdMemberReconciler{Client: c, Scheme: testScheme(t)} - deleted, err := r.deleteTerminalPod(ctx, member) + got, err := r.terminalPod(ctx, member) if err != nil { - t.Fatalf("deleteTerminalPod: %v", err) + t.Fatalf("terminalPod: %v", err) } - if deleted != nil { - t.Fatalf("a terminal Pod owned by another EtcdMember must not be deleted") - } - if err := c.Get(ctx, types.NamespacedName{Namespace: "ns", Name: "test-0"}, &corev1.Pod{}); err != nil { - t.Fatalf("the other member's Pod must still exist; got err=%v", err) + if got != nil { + t.Fatalf("a terminal Pod owned by another EtcdMember must not be reported") } } -// Deleting the terminal Pod must flip MemberReady to False in the same pass. -// If re-creation then fails (here: the referenced TLS Secret is gone) the -// running flow never reaches updateStatus, and a member left at Ready=True -// with no Pod would inflate the cluster's readyMembers count that the -// crash-loop quorum gate reads. +// Replacing the terminal Pod flips MemberReady=False in the same pass, so a +// member whose re-creation then fails (missing TLS Secret) does not sit at +// Ready=True with no Pod. func TestReconcile_TerminalPodDeleteFlipsReadyFalse(t *testing.T) { ctx := context.Background() tru := true @@ -2551,6 +2541,83 @@ func TestReconcile_TerminalPodDeleteFlipsReadyFalse(t *testing.T) { } } +// The Ready=False write goes before the delete: when it fails, the terminal +// Pod must still be there to re-trigger the replacement on the retry. +func TestReconcile_TerminalPodStatusWriteFailureKeepsPod(t *testing.T) { + ctx := context.Background() + tru := true + owner := []metav1.OwnerReference{{ + APIVersion: "etcd-operator.cozystack.io/v1alpha2", Kind: "EtcdMember", + Name: "test-0", UID: types.UID("member-uid"), Controller: &tru, BlockOwnerDeletion: &tru, + }} + member := &lll.EtcdMember{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-0", Namespace: "ns", UID: types.UID("member-uid"), + Labels: memberLabels("test", "test-0"), + Finalizers: []string{MemberFinalizer}, + }, + Spec: lll.EtcdMemberSpec{ + ClusterName: "test", Version: "3.5.17", Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, + InitialCluster: "x", ClusterToken: "ns-test-x", Bootstrap: true, + }, + Status: lll.EtcdMemberStatus{PodName: "test-0", PodUID: "old-uid", PVCName: "data-test-0", MemberID: "abc"}, + } + setMemberCondition(member, lll.MemberReady, metav1.ConditionTrue, "PodReady", "etcd member is ready") + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "test-0", Namespace: "ns", UID: types.UID("old-uid"), OwnerReferences: owner}, + Status: corev1.PodStatus{Phase: corev1.PodSucceeded}, + } + pvc := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{Name: "data-test-0", Namespace: "ns", OwnerReferences: owner}, + } + s := testScheme(t) + failOnce := true + c := fake.NewClientBuilder(). + WithScheme(s). + WithObjects(member, pod, pvc). + WithStatusSubresource(&lll.EtcdCluster{}, &lll.EtcdMember{}). + WithInterceptorFuncs(interceptor.Funcs{ + SubResourceUpdate: func(ctx context.Context, cl client.Client, sub string, obj client.Object, opts ...client.SubResourceUpdateOption) error { + if _, isMember := obj.(*lll.EtcdMember); isMember && sub == "status" && failOnce { + failOnce = false + return apierrors.NewConflict( + schema.GroupResource{Group: lll.GroupVersion.Group, Resource: "etcdmembers"}, + obj.GetName(), errors.New("simulated concurrent status writer")) + } + return cl.SubResource(sub).Update(ctx, obj, opts...) + }, + }). + Build() + r := &EtcdMemberReconciler{Client: c, Scheme: s, EtcdClientFactory: factoryReturning(newFakeEtcd(0xdead))} + req := ctrl.Request{NamespacedName: types.NamespacedName{Name: "test-0", Namespace: "ns"}} + + // Pass 1: the status write conflicts; nothing may be deleted. + if _, err := r.Reconcile(ctx, req); err == nil || !apierrors.IsConflict(err) { + t.Fatalf("Reconcile (pass 1): want the status conflict surfaced; got %v", err) + } + if got := mustGet(t, c, "test-0", "ns", &corev1.Pod{}); got.UID != types.UID("old-uid") { + t.Fatalf("terminal Pod must survive a failed status write; got UID %q", got.UID) + } + + // Pass 2: the write goes through and the Pod is deleted. + if _, err := r.Reconcile(ctx, req); err != nil { + t.Fatalf("Reconcile (pass 2): %v", err) + } + if err := c.Get(ctx, types.NamespacedName{Namespace: "ns", Name: "test-0"}, &corev1.Pod{}); !apierrors.IsNotFound(err) { + t.Fatalf("terminal Pod must be deleted on the retry; got err=%v", err) + } + got := mustGet(t, c, "test-0", "ns", &lll.EtcdMember{}) + for _, cond := range got.Status.Conditions { + if cond.Type == lll.MemberReady { + if cond.Status != metav1.ConditionFalse || cond.Reason != "PodReplacing" { + t.Fatalf("want Ready=False/PodReplacing after the retry; got %+v", cond) + } + return + } + } + t.Fatalf("MemberReady condition missing: %+v", got.Status.Conditions) +} + // TestUpdateStatus_MemoryMemberLeavesPVCNameEmpty: even after a full // reconcile pass, a memory member's Status.PVCName must stay empty so // downstream consumers (the EtcdCluster's Paused message in particular,