From ed807f1be1c59404eca759dfed31b175a4887f11 Mon Sep 17 00:00:00 2001 From: Philip Su Date: Wed, 16 Sep 2026 15:27:24 +0000 Subject: [PATCH 1/2] fix(task): replace-pod judges staleness per pod, not by equal STS revisions Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- internal/task/replace_pod.go | 10 ++++------ internal/task/replace_pod_test.go | 23 +++++++++++++++++++++++ 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/internal/task/replace_pod.go b/internal/task/replace_pod.go index 51d2e638..0c721cf8 100644 --- a/internal/task/replace_pod.go +++ b/internal/task/replace_pod.go @@ -59,18 +59,16 @@ func (e *replacePodExecution) Execute(ctx context.Context) error { } // Revision gate runs before the selector/replica guards: a not-yet-observed - // or not-yet-populated revision is a transient wait, and an already-rolled - // StatefulSet is a complete no-op — neither should reach pod deletion. + // or not-yet-populated revision is a transient wait and must not reach pod + // deletion. Staleness is judged per pod against UpdateRevision, never from + // CurrentRevision == UpdateRevision: a rollback to a revision still in + // history makes those equal while the pods carry the abandoned revision. if sts.Status.ObservedGeneration < sts.Generation { return nil } if sts.Status.UpdateRevision == "" { return nil } - if sts.Status.CurrentRevision == sts.Status.UpdateRevision { - e.complete() - return nil - } if err := guardSelectorAndReplicas(node, sts); err != nil { return err diff --git a/internal/task/replace_pod_test.go b/internal/task/replace_pod_test.go index 58d202ae..12124eed 100644 --- a/internal/task/replace_pod_test.go +++ b/internal/task/replace_pod_test.go @@ -246,6 +246,29 @@ func TestReplacePod_AlreadyAtUpdateRevision_NoOp(t *testing.T) { types.NamespacedName{Name: currentPod.Name, Namespace: currentPod.Namespace}, got)).To(Succeed()) } +// Rollback to a revision still in history: the StatefulSet reports +// CurrentRevision == UpdateRevision while the pod still carries the abandoned +// revision. The task must delete that pod rather than treating the equal +// revisions as an already-rolled StatefulSet. +func TestReplacePod_RollbackWithEqualRevisions_DeletesStalePod(t *testing.T) { + g := NewWithT(t) + node := replacePodNode() + sts := stsForReplace("rev-a", "rev-a") + stalePod := podForReplace("rev-b", false) + + cfg := replacePodCfg(t, node, sts, stalePod) + exec := newReplacePodExec(t, cfg) + + g.Expect(exec.Execute(context.Background())).To(Succeed()) + g.Expect(exec.Status(context.Background())).To(Equal(ExecutionComplete)) + + got := &corev1.Pod{} + err := cfg.KubeClient.Get(context.Background(), + types.NamespacedName{Name: stalePod.Name, Namespace: stalePod.Namespace}, got) + g.Expect(apierrors.IsNotFound(err)).To(BeTrue(), + "expected rolled-back pod to be deleted, got err=%v", err) +} + // Pod already terminating (deletionTimestamp present) → task skips it. // We assert the pod's finalizer wasn't stripped (i.e. task didn't double-delete). func TestReplacePod_TerminatingPod_Skipped(t *testing.T) { From 29b3140692ffa3e541c0369d68f7efb19877cf35 Mon Sep 17 00:00:00 2001 From: Philip Su Date: Wed, 16 Sep 2026 15:35:01 +0000 Subject: [PATCH 2/2] replace-pod: UID-precondition pod deletes; fix stale envtest faker comment Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- .../controller/seinetwork/envtest/sts_status_faker.go | 4 ++-- internal/task/replace_pod.go | 10 +++++++--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/internal/controller/seinetwork/envtest/sts_status_faker.go b/internal/controller/seinetwork/envtest/sts_status_faker.go index 8fc4b96a..60aaa98e 100644 --- a/internal/controller/seinetwork/envtest/sts_status_faker.go +++ b/internal/controller/seinetwork/envtest/sts_status_faker.go @@ -32,8 +32,8 @@ type StatusFaker struct { // // - status.observedGeneration = .Generation // - status.currentRevision = "stub-rev" -// - status.updateRevision = "stub-rev" (matches currentRevision so -// ReplacePod's "rollout complete" branch fires immediately) +// - status.updateRevision = "stub-rev" (non-empty, so ReplacePod passes +// its revision gate; envtest pods carry no revision hash, so none are deleted) // - status.updatedReplicas = *spec.replicas (ObserveImage's gate) // - status.readyReplicas = *spec.replicas // - status.replicas = *spec.replicas diff --git a/internal/task/replace_pod.go b/internal/task/replace_pod.go index 0c721cf8..1f6df74b 100644 --- a/internal/task/replace_pod.go +++ b/internal/task/replace_pod.go @@ -149,10 +149,14 @@ func (e *replacePodExecution) ownedPods(ctx context.Context, node *seiv1alpha1.S return owned, nil } -// deletePod deletes the pod, tolerating an already-gone pod (NotFound) so the -// task is idempotent across reconciles. +// deletePod deletes exactly the observed pod: the UID precondition stops a +// stale cache entry for the stable pod name from deleting its recreated +// replacement. An already-gone pod (NotFound) or a UID mismatch (Conflict) is +// tolerated so the task is idempotent across reconciles. func (e *replacePodExecution) deletePod(ctx context.Context, pod *corev1.Pod) error { - if err := e.cfg.KubeClient.Delete(ctx, pod); err != nil && !apierrors.IsNotFound(err) { + uid := pod.UID + err := e.cfg.KubeClient.Delete(ctx, pod, client.Preconditions{UID: &uid}) + if err != nil && !apierrors.IsNotFound(err) && !apierrors.IsConflict(err) { return fmt.Errorf("deleting pod %q: %w", pod.Name, err) } return nil