From 82b5a606f75ab791e9d47a798b4f6b498cbd9d00 Mon Sep 17 00:00:00 2001 From: Lemon-miaow Date: Wed, 23 Sep 2026 04:23:59 +0800 Subject: [PATCH] =?UTF-8?q?fix(operator):=20end=20the=20start=20attempt=20?= =?UTF-8?q?on=20success=20=E2=80=94=20stale=20anchor=20caused=20false=20St?= =?UTF-8?q?artupTimeout?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found live while validating the RCON-secret heal: a server that had already recovered to Ready was marked Failed(StartupTimeout) minutes later, the moment an unrelated pod rollout briefly dropped readyReplicas. The anchor (status.startRequestedAt) was never cleared on success, so its 300s budget kept ticking under a healthy server and any later blip spent it. markRunningReady now clears the anchor: every start-or-recovery attempt gets its own budget. It also flips ConditionProvisioned back to True — markFailed sets it False and nothing ever reset it, leaving a permanent failure flag on recovered servers that every conditions consumer would read. Unit tests pin both: anchor cleared on Ready, Provisioned recovers from Failed to Running. --- internal/operator/reconciler.go | 9 ++++ internal/operator/reconciler_test.go | 63 ++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+) diff --git a/internal/operator/reconciler.go b/internal/operator/reconciler.go index 81a96e0..e0ee795 100644 --- a/internal/operator/reconciler.go +++ b/internal/operator/reconciler.go @@ -488,10 +488,19 @@ func (r *Reconciler) markRunningReady(server *v1alpha1.MinecraftServer, players metrics.StartDurationSeconds.Observe(t.Sub(server.Status.StartRequestedAt.Time).Seconds()) } } + // The start attempt is over the moment it succeeds: clear the anchor so a + // later pod blip runs on a fresh startup budget instead of inheriting a + // stale one. Found live: a server that had already recovered was marked + // StartupTimeout minutes later because the old anchor was still ticking + // underneath, and the Failed condition outlived the recovery. + server.Status.StartRequestedAt = nil server.Status.Endpoint = v1alpha1.EndpointStatus{Mode: v1alpha1.EndpointDirect, Address: endpointAddress} server.Status.LiveMotd = server.Spec.Motd.Running r.setCondition(server, v1alpha1.ConditionRconReached, metav1.ConditionTrue, "Probed", "RCON probe succeeded") r.setCondition(server, v1alpha1.ConditionReady, metav1.ConditionTrue, "RconReached", "server is accepting RCON") + // markFailed flips Provisioned to False; a recovered server must flip it + // back, or every consumer of the conditions sees a permanent failure flag. + r.setCondition(server, v1alpha1.ConditionProvisioned, metav1.ConditionTrue, "Provisioned", "server is provisioned and accepting RCON") } func (r *Reconciler) markStopping(server *v1alpha1.MinecraftServer) { diff --git a/internal/operator/reconciler_test.go b/internal/operator/reconciler_test.go index f8c2de4..86fd3e0 100644 --- a/internal/operator/reconciler_test.go +++ b/internal/operator/reconciler_test.go @@ -657,6 +657,69 @@ func TestReconcileRunning_StartupTimeoutConvertsToFailed(t *testing.T) { _ = res } +// TestReconcileRunning_ReadyClearsStartAnchor pins the recovery hygiene: once a +// server is Ready the start anchor must clear, so a later pod blip can never +// inherit the stale anchor and be judged StartupTimeout (found live: the +// operator marked an already-recovered server Failed minutes after recovery). +func TestReconcileRunning_ReadyClearsStartAnchor(t *testing.T) { + r, c := newReconciler(t, fakeProber{}, runningServer(), rconSecret()) + base := time.Date(2026, 7, 1, 10, 0, 0, 0, time.UTC) + clock := base + r.Now = func() metav1.Time { return metav1.NewTime(clock) } + + reconcile(t, r, "survival") // Starting: anchors startRequestedAt + if s := getServer(t, c, "survival"); s.Status.StartRequestedAt == nil { + t.Fatal("StartRequestedAt should anchor on the first Starting pass") + } + + clock = base.Add(10 * time.Second) + markPodReady(t, c, "survival") + reconcile(t, r, "survival") // Running + + s := getServer(t, c, "survival") + if s.Status.Phase != v1alpha1.PhaseRunning { + t.Fatalf("phase = %s, want Running", s.Status.Phase) + } + if s.Status.StartRequestedAt != nil { + t.Fatalf("StartRequestedAt = %v after Ready, want nil", s.Status.StartRequestedAt) + } +} + +// TestReconcileRunning_ProvisionedRecovers: markFailed flips Provisioned to +// False, and a recovered server must flip it back — a permanent False misleads +// every consumer of the conditions (kubectl waits, monitoring) forever. +func TestReconcileRunning_ProvisionedRecovers(t *testing.T) { + srv := runningServer() + srv.Spec.Startup.TimeoutSeconds = 30 + r, c := newReconciler(t, fakeProber{}, srv, rconSecret()) + base := time.Date(2026, 7, 1, 10, 0, 0, 0, time.UTC) + clock := base + r.Now = func() metav1.Time { return metav1.NewTime(clock) } + + reconcile(t, r, "survival") // Starting + clock = base.Add(31 * time.Second) + reconcile(t, r, "survival") // past timeout → Failed + + s := getServer(t, c, "survival") + if s.Status.Phase != v1alpha1.PhaseFailed { + t.Fatalf("phase = %s, want Failed", s.Status.Phase) + } + if isConditionTrue(s, v1alpha1.ConditionProvisioned) { + t.Fatal("Provisioned must be False after Failed") + } + + markPodReady(t, c, "survival") + reconcile(t, r, "survival") // recovers to Running + + s = getServer(t, c, "survival") + if s.Status.Phase != v1alpha1.PhaseRunning { + t.Fatalf("phase = %s, want Running after recovery", s.Status.Phase) + } + if !isConditionTrue(s, v1alpha1.ConditionProvisioned) { + t.Fatal("Provisioned must flip back to True after recovery") + } +} + // --- helpers --------------------------------------------------------------- func getSTSErr(c client.Client, name string) (*appsv1.StatefulSet, error) {