From 2f920cf1c9279dfb64b3d4c7c3c27c4669b35edf Mon Sep 17 00:00:00 2001 From: Lemon-miaow Date: Sun, 27 Sep 2026 04:03:15 +0800 Subject: [PATCH] =?UTF-8?q?fix(operator):=20=E6=9C=8D=E5=8A=A1=E5=99=A8?= =?UTF-8?q?=E6=9C=AA=E5=B0=B1=E7=BB=AA=E6=97=B6=E6=94=B9=E4=BA=86=E9=85=8D?= =?UTF-8?q?=E7=BD=AE=EF=BC=8Coperator=20=E6=8C=89=E6=96=B0=E6=A8=A1?= =?UTF-8?q?=E6=9D=BF=E9=87=8D=E5=BB=BA=20pod-0=EF=BC=8C=E4=B8=8D=E5=86=8D?= =?UTF-8?q?=E5=8D=A1=E5=9C=A8=E6=97=A7=E9=95=9C=E5=83=8F=E4=B8=8A?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- cmd/felis/operator.go | 5 +- docs/troubleshooting.md | 18 ++- internal/operator/reconciler.go | 68 +++++++++ internal/operator/stalepod_test.go | 217 +++++++++++++++++++++++++++++ internal/platform/rbac.go | 16 ++- internal/platform/rbac_test.go | 10 +- 6 files changed, 320 insertions(+), 14 deletions(-) create mode 100644 internal/operator/stalepod_test.go diff --git a/cmd/felis/operator.go b/cmd/felis/operator.go index ad8f87e..1f96868 100644 --- a/cmd/felis/operator.go +++ b/cmd/felis/operator.go @@ -120,7 +120,10 @@ func cmdOperator(args []string, _, stderr io.Writer) int { Jobs: mgr.GetAPIReader(), // Uncached too: RCON Secrets are read by name, so the Role grants // secrets:get without the list/watch an informer would need. - Secrets: mgr.GetAPIReader(), + Secrets: mgr.GetAPIReader(), + // And pods: pod-0 is read by name, so the Role grants pods:get and no + // namespace-wide pod informer runs. + Pods: mgr.GetAPIReader(), Recorder: mgr.GetEventRecorderFor("felis-operator"), Watch: watch, } diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 56322ca..cb9a777 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -40,7 +40,7 @@ concrete host. ## 1. Server is stuck in `Starting` and never becomes `Running` `MinecraftServer.status.phase` stays `Starting`. A start that never succeeds is -requeued every 5s until one of the two startup budgets expires, then escalated +requeued every 2s until one of the two startup budgets expires, then escalated to `Failed` — `StartupTimeout` if the pod never passed TCP readiness, `ReadinessTimeout` if the RCON probe never succeeded. Both default to **300s** when `spec.startup.timeoutSeconds` / `spec.startup.readinessTimeoutSeconds` are @@ -62,13 +62,16 @@ kubectl get minecraftserver -o jsonpath='{.status.conditions}' ``` `markStarting` writes the same reason to both `Ready=False` and -`RconReached=False`. The reason is exactly one of: +`RconReached=False`. The reason is one of: | `status.conditions[].reason` | Meaning | Requeue | |---|---|---| -| `PodNotReady` | Pod not TCP-ready yet (`status.readyReplicas < 1`) | 5s | +| `PodNotReady` | Pod not TCP-ready yet (`status.readyReplicas < 1`) | 2s | +| `SpecChanged` | The spec changed while the pod was not ready; the operator recreated the pod from the new template (§1a) | 2s | +| `AutoRestart` / `StartRetried` | A timed-out start was retried, automatically or from the panel, by recreating the pod | 2s | +| `ServiceAddressPending` | The client Service has no ClusterIP yet | 2s | | `RconSecretUnavailable` | RCON secret missing or malformed | 10s | -| `RconNotReachable` | RCON dial/auth failed | 5s | +| `RconNotReachable` | RCON dial/auth failed | 2s | [GO-TESTED for the reason set.] @@ -88,6 +91,13 @@ kubectl describe pod # look at Events + container State not exist, or the registry is unreachable. `spec.image` is copied verbatim into the container with **zero validation** by the operator. Fix the image reference, or see §7 (registry reachability) and §6 (build push target). + Editing the spec is enough, even once the server is `Failed` with its retries + spent: the StatefulSet (OrderedReady) never rolls a pod that is not ready, so + the operator deletes a not-ready `-0` made from an older template itself + and the StatefulSet recreates it from the new one. The server goes back to + `Starting` with reason `SpecChanged`, a fresh start timeout and its restart + budget at zero, and the MinecraftServer gets a `PodReplaced` Event. + [GO-TESTED: `TestSpecChangeReplacesAPodThatGaveUp`, `TestStalePodReplacement`.] - **PVC `Pending`** → `kubectl get pvc -l app.kubernetes.io/name=`. A nonexistent `spec.storage.storageClassName`, or a request larger than any class can satisfy, leaves the PVC unbound. The operator does **not** error on this diff --git a/internal/operator/reconciler.go b/internal/operator/reconciler.go index b6b51e5..2ab8d33 100644 --- a/internal/operator/reconciler.go +++ b/internal/operator/reconciler.go @@ -130,6 +130,11 @@ type Reconciler struct { // mirror with the database URL among them) and grow with them. Nil falls back // to the embedded client. Secrets client.Reader + // Pods reads a server's pod-0 by name, to replace one left on an old template + // (replaceStalePod). The manager's uncached API reader as well, so the + // operator holds pods:get and no pod informer. Nil falls back to the embedded + // client. + Pods client.Reader // Recorder puts an Event on the MinecraftServer at each phase change, pod // recreation, idle stop and RCON Secret provision, so `kubectl describe` // shows the timeline the status alone overwrites. Nil records none. @@ -330,6 +335,18 @@ func (r *Reconciler) reconcileRunning(ctx context.Context, server *v1alpha1.Mine // The pod must first pass its tcpSocket readiness (readyReplicas >= 1). if current.Status.ReadyReplicas < 1 { + if replaced, err := r.replaceStalePod(ctx, server, ¤t); err != nil { + return ctrl.Result{}, err + } else if replaced { + // A new spec is a new start: its own timeout and restart budget. + server.Status.AutoRestarts = 0 + server.Status.StartRequestedAt = nil + r.markStarting(server, "SpecChanged", "the spec changed while the pod was not ready; recreated the pod from the new spec") + if err := r.patchStatus(ctx, server); err != nil { + return ctrl.Result{}, err + } + return ctrl.Result{RequeueAfter: requeueStarting}, nil + } r.markStarting(server, "PodNotReady", "waiting for pod TCP readiness") if r.startupTimedOut(server) { r.markFailed(server, v1alpha1.ReasonStartupTimeout, "pod did not become ready within startup timeout") @@ -677,6 +694,57 @@ func (r *Reconciler) secretReader() client.Reader { return r.Client } +func (r *Reconciler) podReader() client.Reader { + if r.Pods != nil { + return r.Pods + } + return r.Client +} + +// replaceStalePod deletes a pod-0 that is not ready and was made from an older +// template than the StatefulSet's current one, for the StatefulSet to recreate +// from the new spec. The StatefulSet controller does not do it: under +// OrderedReady it rolls a pod only once that pod is Running and Ready, so a +// server that cannot start keeps crash-looping on the image or settings that +// broke it however its spec is corrected, until the next timed-out retry +// deletes the pod, or forever once the retries are spent. A ready pod is left +// to the controller's own rolling update, and one already terminating to its +// deletion. It acts only on a StatefulSet status that has observed the current +// template, so the update revision it compares against is the current one. +func (r *Reconciler) replaceStalePod(ctx context.Context, server *v1alpha1.MinecraftServer, sts *appsv1.StatefulSet) (bool, error) { + update := sts.Status.UpdateRevision + if update == "" || sts.Status.ObservedGeneration < sts.Generation { + return false, nil + } + var pod corev1.Pod + if err := r.podReader().Get(ctx, types.NamespacedName{Namespace: server.Namespace, Name: server.Name + "-0"}, &pod); err != nil { + return false, client.IgnoreNotFound(err) + } + if pod.DeletionTimestamp != nil || pod.Labels[appsv1.ControllerRevisionHashLabelKey] == update || podReady(&pod) { + return false, nil + } + // The UID precondition: a pod the StatefulSet already recreated is not + // deleted in its place. + if err := r.Delete(ctx, &pod, client.Preconditions{UID: &pod.UID}); err != nil { + if apierrors.IsNotFound(err) || apierrors.IsConflict(err) { + return false, nil + } + return false, err + } + log.FromContext(ctx).Info("recreated a pod left on an old spec", "pod", pod.Name, "revision", pod.Labels[appsv1.ControllerRevisionHashLabelKey], "updateRevision", update) + r.event(server, corev1.EventTypeNormal, "PodReplaced", "the spec changed while the pod was not ready; recreated the pod from the new spec") + return true, nil +} + +func podReady(pod *corev1.Pod) bool { + for _, c := range pod.Status.Conditions { + if c.Type == corev1.PodReady { + return c.Status == corev1.ConditionTrue + } + } + return false +} + func (r *Reconciler) rconPassword(ctx context.Context, server *v1alpha1.MinecraftServer) (string, error) { ref := server.Spec.Rcon.SecretRef if ref.Name == "" || ref.Key == "" { diff --git a/internal/operator/stalepod_test.go b/internal/operator/stalepod_test.go new file mode 100644 index 0000000..37e1552 --- /dev/null +++ b/internal/operator/stalepod_test.go @@ -0,0 +1,217 @@ +package operator_test + +import ( + "context" + "errors" + "testing" + "time" + + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + + "felis.lolicon.best/internal/apis/felis/v1alpha1" +) + +// podAt is survival-0 as the StatefulSet controller made it from revision rev. +func podAt(rev string) *corev1.Pod { + p := gamePod() + p.UID = types.UID("pod-" + rev) + p.Labels = map[string]string{appsv1.ControllerRevisionHashLabelKey: rev} + return p +} + +func withReady(p *corev1.Pod, status corev1.ConditionStatus) *corev1.Pod { + p.Status.Conditions = []corev1.PodCondition{{Type: corev1.PodReady, Status: status}} + return p +} + +// observeTemplate stands in for the StatefulSet controller: it has seen the +// current template, which it names revision rev, and the pod is not ready. +func observeTemplate(t *testing.T, c client.Client, rev string) { + t.Helper() + sts := getSTS(t, c, "survival") + sts.Status.ObservedGeneration = sts.Generation + sts.Status.UpdateRevision = rev + sts.Status.Replicas = 1 + sts.Status.ReadyReplicas = 0 + if err := c.Status().Update(context.Background(), sts); err != nil { + t.Fatalf("update sts status: %v", err) + } +} + +func editImage(t *testing.T, c client.Client, image string) { + t.Helper() + s := getServer(t, c, "survival") + s.Spec.Image = image + if err := c.Update(context.Background(), s); err != nil { + t.Fatalf("edit image: %v", err) + } +} + +func readyReason(s *v1alpha1.MinecraftServer) string { + if c := meta.FindStatusCondition(s.Status.Conditions, v1alpha1.ConditionReady); c != nil { + return c.Reason + } + return "" +} + +// A server that gave up on a broken image is not stuck on it: once its spec is +// corrected, the pod still crash-looping on the old template is recreated from +// the new one, and the corrected start gets its own timeout and restarts. +func TestSpecChangeReplacesAPodThatGaveUp(t *testing.T) { + srv := runningServer() + srv.Spec.Startup.TimeoutSeconds = 30 + r, c := newReconciler(t, fakeProber{}, srv, rconSecret(), podAt("survival-old")) + base := time.Date(2026, 7, 1, 10, 0, 0, 0, time.UTC) + clock := base + r.Now = func() metav1.Time { return metav1.NewTime(clock) } + at := func(offset time.Duration) *v1alpha1.MinecraftServer { + clock = base.Add(offset) + reconcile(t, r, "survival") + return getServer(t, c, "survival") + } + + // Spend the three automatic restarts (autorestart_test.go pins the schedule), + // each pod coming back on the same broken template. + at(0) + observeTemplate(t, c, "survival-old") + for _, due := range []time.Duration{90 * time.Second, 240 * time.Second, 510 * time.Second} { + at(due) + if err := c.Create(context.Background(), podAt("survival-old")); err != nil { + t.Fatal(err) + } + } + s := at(time.Hour) + if !v1alpha1.StartGaveUp(&s.Status) { + t.Fatalf("setup: not given up: phase=%s restarts=%d", s.Status.Phase, s.Status.AutoRestarts) + } + + // The admin corrects the image; the StatefulSet takes the new template but, + // the pod never having been ready, leaves the pod as it is. + editImage(t, c, "registry.internal/felis/paper:fixed") + s = at(time.Hour + time.Second) + if !podPresent(t, c) { + t.Fatal("the pod was deleted before the StatefulSet had observed the new template") + } + if img := getSTS(t, c, "survival").Spec.Template.Spec.Containers[0].Image; img != "registry.internal/felis/paper:fixed" { + t.Fatalf("setup: template image = %q", img) + } + observeTemplate(t, c, "survival-new") + + s = at(time.Hour + 2*time.Second) + if podPresent(t, c) { + t.Fatal("the pod on the old template was left in place") + } + if s.Status.Phase != v1alpha1.PhaseStarting || readyReason(s) != "SpecChanged" { + t.Fatalf("phase=%s reason=%q, want Starting SpecChanged", s.Status.Phase, readyReason(s)) + } + if s.Status.AutoRestarts != 0 { + t.Fatalf("autoRestarts = %d, want the budget back at 0", s.Status.AutoRestarts) + } + if s.Status.StartRequestedAt == nil || !s.Status.StartRequestedAt.Time.Equal(clock) { + t.Fatalf("startRequestedAt = %v, want the start re-anchored at %v", s.Status.StartRequestedAt, clock) + } + + // The StatefulSet recreates the pod from the new revision; that one is left + // to start, and the new start times out on its own clock. + if err := c.Create(context.Background(), podAt("survival-new")); err != nil { + t.Fatal(err) + } + s = at(time.Hour + 20*time.Second) + if !podPresent(t, c) || s.Status.Phase != v1alpha1.PhaseStarting { + t.Fatalf("pod present=%v phase=%s, want the new pod kept while it starts", podPresent(t, c), s.Status.Phase) + } + s = at(time.Hour + 33*time.Second) + if s.Status.Phase != v1alpha1.PhaseFailed || s.Status.AutoRestarts != 0 { + t.Fatalf("phase=%s restarts=%d, want Failed with the budget untouched at the new timeout", s.Status.Phase, s.Status.AutoRestarts) + } +} + +// Which pods a Starting server's pass replaces once the StatefulSet has a newer +// template, and which it leaves alone. +func TestStalePodReplacement(t *testing.T) { + terminating := podAt("survival-old") + terminating.DeletionTimestamp = &metav1.Time{Time: time.Date(2026, 6, 25, 11, 59, 0, 0, time.UTC)} + terminating.Finalizers = []string{"test.felis/hold"} + + for _, tc := range []struct { + name string + pod *corev1.Pod + observed bool + replaced bool + }{ + {"old template, not ready", podAt("survival-old"), true, true}, + {"old template, readiness false", withReady(podAt("survival-old"), corev1.ConditionFalse), true, true}, + {"current template", podAt("survival-new"), true, false}, + {"old template but ready: the StatefulSet rolls it", withReady(podAt("survival-old"), corev1.ConditionTrue), true, false}, + {"already terminating", terminating, true, false}, + {"template not yet observed", podAt("survival-old"), false, false}, + {"no pod", nil, true, false}, + } { + t.Run(tc.name, func(t *testing.T) { + objs := []client.Object{runningServer(), rconSecret()} + if tc.pod != nil { + objs = append(objs, tc.pod.DeepCopy()) + } + r, c := newReconciler(t, fakeProber{}, objs...) + reconcile(t, r, "survival") + observeTemplate(t, c, "survival-new") + if !tc.observed { + sts := getSTS(t, c, "survival") + sts.Generation = 2 + if err := c.Update(context.Background(), sts); err != nil { + t.Fatal(err) + } + sts = getSTS(t, c, "survival") + if sts.Status.ObservedGeneration >= sts.Generation { + t.Fatalf("setup: observedGeneration %d is not behind generation %d", sts.Status.ObservedGeneration, sts.Generation) + } + } + + res := reconcile(t, r, "survival") + s := getServer(t, c, "survival") + if gone := tc.pod != nil && !podPresent(t, c); gone != tc.replaced { + t.Fatalf("pod deleted = %v, want %v", gone, tc.replaced) + } + want := "PodNotReady" + if tc.replaced { + want = "SpecChanged" + } + if readyReason(s) != want || s.Status.Phase != v1alpha1.PhaseStarting { + t.Fatalf("phase=%s reason=%q, want Starting %s", s.Status.Phase, readyReason(s), want) + } + if res.RequeueAfter != 2*time.Second { + t.Fatalf("requeue = %v, want the Starting pace", res.RequeueAfter) + } + }) + } +} + +// noCachedPods stands in for the manager's cached client, which has no pod +// informer: any pod read through it fails. +type noCachedPods struct{ client.Client } + +func (c noCachedPods) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if _, ok := obj.(*corev1.Pod); ok { + return errors.New("pods are not cached") + } + return c.Client.Get(ctx, key, obj, opts...) +} + +// The pod is read through Reconciler.Pods alone, so the operator runs with +// pods:get and no pod informer. +func TestStalePodIsReadThroughTheUncachedReader(t *testing.T) { + r, c := newReconciler(t, fakeProber{}, runningServer(), rconSecret(), podAt("survival-old")) + r.Client = noCachedPods{c} + r.Pods = c + reconcile(t, r, "survival") + observeTemplate(t, c, "survival-new") + reconcile(t, r, "survival") + if podPresent(t, c) { + t.Fatal("the pod on the old template was left in place") + } +} diff --git a/internal/platform/rbac.go b/internal/platform/rbac.go index ba21d6a..741aecd 100644 --- a/internal/platform/rbac.go +++ b/internal/platform/rbac.go @@ -153,9 +153,12 @@ func APIBuildRole(p Params) *rbacv1.Role { // call fails closed with a 403), and reads RCON Secrets by name, uncached. Jobs are list-only, // through the manager's uncached API reader: before scaling a server up from zero // the operator checks that no restore/backup/file-write Job holds its world -// (internal/maintenance). Pods are delete-only: a start that timed out is retried -// by deleting its pod for the StatefulSet to recreate (bounded, three attempts; -// internal/operator.recoverFailedStart). Events are create/patch only, for the +// (internal/maintenance). Pods are get and delete, by name through the uncached +// reader: a start that timed out is retried by deleting its pod for the +// StatefulSet to recreate (bounded, three attempts; +// internal/operator.recoverFailedStart), and a pod that is not ready and was made +// from an older template is read and replaced the same way +// (internal/operator.replaceStalePod). Events are create/patch only, for the // timeline it records on each server. It never touches PVCs or finalizers, so // none appear here. func OperatorRole(p Params) *rbacv1.Role { @@ -175,9 +178,10 @@ func OperatorRole(p Params) *rbacv1.Role { // list only: an uncached List (no informer, so no watch) of the world-volume // maintenance Jobs; the operator never creates or deletes a Job. rule([]string{groupBatch}, []string{"jobs"}, []string{"list"}), - // delete only, through the direct client: no read of pods is needed to - // remove the one named -0. - rule([]string{groupCore}, []string{"pods"}, []string{"delete"}), + // get and delete by name of the one named -0: get through the + // uncached API reader (Reconciler.Pods), to see whether it is ready and which + // template revision made it. No list/watch, so there is no pod informer. + rule([]string{groupCore}, []string{"pods"}, []string{"get", "delete"}), // The Events the reconciler records on MinecraftServers (phase changes, // pod recreation, idle stop): the recorder creates one and patches its // count when the same Event repeats. diff --git a/internal/platform/rbac_test.go b/internal/platform/rbac_test.go index 5f7859b..1756f94 100644 --- a/internal/platform/rbac_test.go +++ b/internal/platform/rbac_test.go @@ -215,13 +215,17 @@ func TestOperatorRole_ScopeExact(t *testing.T) { t.Errorf("operator secrets rule must be get+create only, found %s", v) } } - // Pods are delete-only: the bounded retry of a timed-out start. + // Pods are read and deleted by name: the bounded retry of a timed-out start, + // and the replacement of a not-ready pod left on an old template. if !hasRule(op, groupCore, "pods", "delete") { t.Error("operator must have pods:delete (auto-restart of a timed-out start)") } - for _, v := range []string{"get", "list", "watch", "create", "update", "patch", "deletecollection", "*"} { + if !hasRule(op, groupCore, "pods", "get") { + t.Error("operator must have pods:get (replacing a not-ready pod left on an old template)") + } + for _, v := range []string{"list", "watch", "create", "update", "patch", "deletecollection", "*"} { if hasRule(op, groupCore, "pods", v) { - t.Errorf("operator pods rule must be delete-only, found %s", v) + t.Errorf("operator pods rule must be get+delete only, found %s", v) } } // The world-volume lock check lists Jobs uncached; it never writes one.