diff --git a/deploy/crd/felis.lolicon.best_minecraftservers.yaml b/deploy/crd/felis.lolicon.best_minecraftservers.yaml index 3a98e0d..1afe293 100644 --- a/deploy/crd/felis.lolicon.best_minecraftservers.yaml +++ b/deploy/crd/felis.lolicon.best_minecraftservers.yaml @@ -123,10 +123,6 @@ spec: lifecycle: description: Lifecycle tunes graceful shutdown (spec §7). properties: - preStopSaveAndStop: - description: PreStopSaveAndStop enables the operator-injected - RCON save+stop preStop. - type: boolean terminationGracePeriodSeconds: description: TerminationGracePeriodSeconds is the pod grace period (default 300). diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index cf7ab85..a952623 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -256,7 +256,7 @@ What holds the world, in order: ``` A restore, backup or file write refused with `409 not_stopped` although the -panel shows `Stopped` means the game pod is still terminating (its preStop save +panel shows `Stopped` means the game pod is still terminating (its shutdown save can take a while); retry once `kubectl -n minecraft get pods -l felis.lolicon.best/server=` shows nothing. diff --git a/internal/apis/felis/v1alpha1/minecraftserver_types.go b/internal/apis/felis/v1alpha1/minecraftserver_types.go index 2c16c64..2094169 100644 --- a/internal/apis/felis/v1alpha1/minecraftserver_types.go +++ b/internal/apis/felis/v1alpha1/minecraftserver_types.go @@ -197,13 +197,12 @@ type StorageSpec struct { StorageClassName string `json:"storageClassName,omitempty"` } -// LifecycleSpec tunes graceful shutdown (spec §7). The operator injects a -// preStop RCON "save-all flush; stop" hook when PreStopSaveAndStop is set. +// LifecycleSpec tunes graceful shutdown (spec §7). Before scaling a server with +// RCON to zero the operator runs "save-all flush" over RCON; the grace period is +// then the time the server has to finish its own shutdown save after SIGTERM. type LifecycleSpec struct { // TerminationGracePeriodSeconds is the pod grace period (default 300). TerminationGracePeriodSeconds int64 `json:"terminationGracePeriodSeconds,omitempty"` - // PreStopSaveAndStop enables the operator-injected RCON save+stop preStop. - PreStopSaveAndStop bool `json:"preStopSaveAndStop,omitempty"` } // StartupSpec bounds the Starting phase (spec §5). diff --git a/internal/operator/builders.go b/internal/operator/builders.go index a31354c..7bf2123 100644 --- a/internal/operator/builders.go +++ b/internal/operator/builders.go @@ -108,18 +108,6 @@ func rconAddress(server *v1alpha1.MinecraftServer) string { return fmt.Sprintf("%s.%s.svc.cluster.local:%d", server.Name, server.Namespace, rconPort(server)) } -// preStopScript is the operator-injected graceful-shutdown sequence (spec §7): -// flush the world, then stop the server, both over RCON. It relies on rcon-cli -// being present in the Felis base image and reading the RCON_* env injected -// alongside it. -func preStopScript(server *v1alpha1.MinecraftServer) string { - port := rconPort(server) - return fmt.Sprintf( - `rcon-cli --port %d --password "$RCON_PASSWORD" save-all flush; rcon-cli --port %d --password "$RCON_PASSWORD" stop`, - port, port, - ) -} - // buildHeadlessService backs the StatefulSet's stable network identity. func buildHeadlessService(server *v1alpha1.MinecraftServer) *corev1.Service { svc := &corev1.Service{ @@ -198,9 +186,10 @@ func readinessProbe(server *v1alpha1.MinecraftServer) *corev1.Probe { return probe } -// buildStatefulSet renders the workload for replicas in {0,1}. It is where -// graceful shutdown is injected: the pod gets terminationGracePeriodSeconds and -// (when enabled) a preStop RCON save+stop hook. +// buildStatefulSet renders the workload for replicas in {0,1}. Its half of +// graceful shutdown is terminationGracePeriodSeconds, the time the server gets to +// save on SIGTERM; the reconciler flushes the world over RCON before it scales to +// zero (saveBeforeStop). func buildStatefulSet(server *v1alpha1.MinecraftServer, replicas int32, felisImage string) (*appsv1.StatefulSet, error) { storageSize := server.Spec.Storage.Size if storageSize == "" { @@ -245,15 +234,6 @@ func buildStatefulSet(server *v1alpha1.MinecraftServer, replicas int32, felisIma container.Ports = append(container.Ports, corev1.ContainerPort{ Name: "rcon", ContainerPort: rconPort(server), Protocol: corev1.ProtocolTCP, }) - if server.Spec.Lifecycle.PreStopSaveAndStop { - container.Lifecycle = &corev1.Lifecycle{ - PreStop: &corev1.LifecycleHandler{ - Exec: &corev1.ExecAction{ - Command: []string{"/bin/sh", "-c", preStopScript(server)}, - }, - }, - } - } } // Every server first hands its world volume to the game uid (prepareDataInitContainer), diff --git a/internal/operator/prober.go b/internal/operator/prober.go index 755b562..78c4fb3 100644 --- a/internal/operator/prober.go +++ b/internal/operator/prober.go @@ -20,21 +20,52 @@ type PlayerCount struct { Known bool } -// Prober reports whether a server's RCON endpoint is reachable and accepts the -// password, and best-effort returns its current player tally. A nil error is the -// loader-agnostic readiness gate (spec §5); the PlayerCount is advisory and has -// Known=false (with a nil error) whenever the tally could not be sampled. It is an -// interface so the reconciler can be tested without a live server. +// Prober is the operator's RCON channel into a running server. It is an interface +// so the reconciler can be tested without a live server. +// +// Probe reports whether the endpoint is reachable and accepts the password, and +// best-effort returns the current player tally. A nil error is the loader-agnostic +// readiness gate (spec §5); the PlayerCount is advisory and has Known=false (with +// a nil error) whenever the tally could not be sampled. +// +// Save flushes the world to disk (`save-all flush`) and returns once the server +// has answered, i.e. once the save is done. The reconciler runs it right before +// scaling a server to zero (spec §7). type Prober interface { Probe(ctx context.Context, addr, password string) (PlayerCount, error) + Save(ctx context.Context, addr, password string) error } // RconProber is the production Prober: a successful Dial (TCP connect + auth) // is the readiness gate; on that same connection it then runs `list` to sample // the player tally before closing. type RconProber struct { - // Timeout bounds a single probe. Defaults to 5s. + // Timeout bounds a single probe, and the connect+auth step of a save. + // Defaults to 5s. Timeout time.Duration + // SaveTimeout bounds the wait for `save-all flush` to answer. Defaults to + // defaultSaveTimeout. + SaveTimeout time.Duration +} + +// defaultSaveTimeout is how long a stop waits for the pre-stop save. The server +// answers `save-all flush` only after every loaded chunk is written, which takes +// seconds on a large world. Past this the stop goes ahead anyway: SIGTERM runs the +// server's own shutdown save within the pod's grace period, so the explicit save +// only moves most of that work ahead of the kill deadline. +const defaultSaveTimeout = 30 * time.Second + +// boundTimeout returns d (or def when d is unset), shortened to ctx's deadline. +func boundTimeout(ctx context.Context, d, def time.Duration) time.Duration { + if d <= 0 { + d = def + } + if dl, ok := ctx.Deadline(); ok { + if remaining := time.Until(dl); remaining > 0 && remaining < d { + d = remaining + } + } + return d } // Probe dials addr and authenticates with password, honoring the smaller of the @@ -43,15 +74,7 @@ type RconProber struct { // a failed or unparseable `list` yields an unknown PlayerCount, never a probe error, // so a transient count-read hiccup can never flap a healthy server out of Ready. func (p RconProber) Probe(ctx context.Context, addr, password string) (PlayerCount, error) { - timeout := p.Timeout - if timeout <= 0 { - timeout = 5 * time.Second - } - if dl, ok := ctx.Deadline(); ok { - if remaining := time.Until(dl); remaining > 0 && remaining < timeout { - timeout = remaining - } - } + timeout := boundTimeout(ctx, p.Timeout, 5*time.Second) conn, err := rcon.Dial(addr, password, timeout) if err != nil { return PlayerCount{}, err @@ -71,6 +94,22 @@ func (p RconProber) Probe(ctx context.Context, addr, password string) (PlayerCou return pc, nil } +// Save runs `save-all flush` and waits for its reply. The reply text is not +// checked: vanilla, Paper and the modded loaders word it differently, and any +// reply at all means the command ran to completion on the server thread. +func (p RconProber) Save(ctx context.Context, addr, password string) error { + conn, err := rcon.Dial(addr, password, boundTimeout(ctx, p.Timeout, 5*time.Second)) + if err != nil { + return err + } + defer conn.Close() + if err := conn.SetDeadline(time.Now().Add(boundTimeout(ctx, p.SaveTimeout, defaultSaveTimeout))); err != nil { + return err + } + _, err = conn.Execute("save-all flush") + return err +} + // listReplyPatterns match the `list` replies of the loaders Felis runs, tried in // order against the reply with § color codes stripped: // diff --git a/internal/operator/prober_internal_test.go b/internal/operator/prober_internal_test.go index 54feb48..e2c3548 100644 --- a/internal/operator/prober_internal_test.go +++ b/internal/operator/prober_internal_test.go @@ -1,6 +1,13 @@ package operator -import "testing" +import ( + "context" + "encoding/binary" + "io" + "net" + "testing" + "time" +) func TestParseListReply(t *testing.T) { cases := []struct { @@ -82,3 +89,73 @@ func TestParseListReply(t *testing.T) { }) } } + +// serveFakeRcon accepts one RCON connection, accepts any password, and answers +// each command after delay. Every command body is sent on the returned channel. +func serveFakeRcon(t *testing.T, delay time.Duration) (string, <-chan string) { + t.Helper() + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + t.Cleanup(func() { ln.Close() }) + cmds := make(chan string, 4) + go func() { + conn, err := ln.Accept() + if err != nil { + return + } + defer conn.Close() + for { + var hdr [12]byte + if _, err := io.ReadFull(conn, hdr[:]); err != nil { + return + } + size := int32(binary.LittleEndian.Uint32(hdr[0:])) + id := int32(binary.LittleEndian.Uint32(hdr[4:])) + typ := int32(binary.LittleEndian.Uint32(hdr[8:])) + rest := make([]byte, size-8) + if _, err := io.ReadFull(conn, rest); err != nil { + return + } + reply := "" + if typ == 3 { // auth: answer with an auth response carrying the same id + typ = 2 + } else { + cmds <- string(rest[:len(rest)-2]) + time.Sleep(delay) + typ, reply = 0, "Saved the game" + } + out := binary.LittleEndian.AppendUint32(nil, uint32(4+4+len(reply)+2)) + out = binary.LittleEndian.AppendUint32(out, uint32(id)) + out = binary.LittleEndian.AppendUint32(out, uint32(typ)) + out = append(append(out, reply...), 0, 0) + if _, err := conn.Write(out); err != nil { + return + } + } + }() + return ln.Addr().String(), cmds +} + +func TestRconProberSaveFlushes(t *testing.T) { + addr, cmds := serveFakeRcon(t, 0) + if err := (RconProber{}).Save(context.Background(), addr, "pw"); err != nil { + t.Fatalf("Save: %v", err) + } + if got := <-cmds; got != "save-all flush" { + t.Errorf("command = %q, want save-all flush", got) + } +} + +func TestRconProberSaveTimesOut(t *testing.T) { + addr, _ := serveFakeRcon(t, time.Second) + start := time.Now() + err := (RconProber{SaveTimeout: 100 * time.Millisecond}).Save(context.Background(), addr, "pw") + if err == nil { + t.Fatal("Save returned nil for a reply slower than SaveTimeout") + } + if elapsed := time.Since(start); elapsed > 900*time.Millisecond { + t.Errorf("Save took %v, want it bounded by SaveTimeout", elapsed) + } +} diff --git a/internal/operator/reconciler.go b/internal/operator/reconciler.go index 3d253a2..4bb1387 100644 --- a/internal/operator/reconciler.go +++ b/internal/operator/reconciler.go @@ -1,7 +1,8 @@ // Package operator reconciles MinecraftServer objects (spec §4, §5, §7). The // CRD is the lifecycle source-of-truth; this controller renders the -// StatefulSet/Service/PVC from it, gates readiness on an RCON probe, and injects -// graceful shutdown. It never reads or writes business-layer (Postgres) fields. +// StatefulSet/Service/PVC from it, gates readiness on an RCON probe, and flushes +// the world over RCON before scaling a server down. It never reads or writes +// business-layer (Postgres) fields. package operator import ( @@ -25,6 +26,7 @@ import ( "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) @@ -96,9 +98,17 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { // and a watch is what lets a deleted Secret be noticed at all (a quiet // Running server otherwise produces no events). Owns(&corev1.Secret{}). + WithOptions(controller.Options{MaxConcurrentReconciles: maxConcurrentReconciles}). Complete(r) } +// maxConcurrentReconciles lets that many servers reconcile at once. A reconcile +// blocks on RCON (up to 5s for a probe, and up to defaultSaveTimeout for the +// save ahead of a stop), so with controller-runtime's default of one, a single +// large world saving would stall every other server's start, stop and readiness. +// The same server is never reconciled twice at once regardless. +const maxConcurrentReconciles = 4 + // Reconcile drives a single MinecraftServer toward spec.desiredState. func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { var server v1alpha1.MinecraftServer @@ -302,8 +312,11 @@ func (r *Reconciler) reconcileStopped(ctx context.Context, server *v1alpha1.Mine return ctrl.Result{}, err } - // Scaling to zero triggers each pod's preStop RCON save+stop (spec §7). + // Graceful shutdown (spec §7): flush the world over RCON, then scale to zero, + // which sends the server SIGTERM and so its own shutdown save within the + // grace period. if sts.Spec.Replicas == nil || *sts.Spec.Replicas != 0 { + r.saveBeforeStop(ctx, server, &sts) zero := int32(0) sts.Spec.Replicas = &zero if err := r.Update(ctx, &sts); err != nil { @@ -323,6 +336,37 @@ func (r *Reconciler) reconcileStopped(ctx context.Context, server *v1alpha1.Mine return ctrl.Result{}, r.patchStatus(ctx, server) } +// saveBeforeStop runs `save-all flush` on a server that is about to be scaled to +// zero. The SIGTERM that follows makes the server save again on its way out, but +// that save races the grace period: a large world killed mid-save rolls back to +// whatever was last flushed. Flushing first leaves the shutdown save with almost +// nothing to write. +// +// It is best-effort and never holds up the stop. A server without RCON, or with no +// ready pod (still booting, or already terminating), has nothing to flush it with, +// and a failed save still leaves the shutdown save; holding a stop the user asked +// for over it would only keep the server up. A conflict on the Update that follows +// re-runs it on the next reconcile, which is harmless: a second flush right after +// the first writes nothing. +func (r *Reconciler) saveBeforeStop(ctx context.Context, server *v1alpha1.MinecraftServer, sts *appsv1.StatefulSet) { + if !server.Spec.Rcon.Enabled || sts.Status.ReadyReplicas == 0 { + return + } + logger := ctrl.LoggerFrom(ctx) + password, err := r.rconPassword(ctx, server) + if err != nil { + logger.Info("skipping pre-stop world save: RCON password unavailable", "error", err.Error()) + return + } + start := time.Now() + if err := r.Prober.Save(ctx, rconAddress(server), password); err != nil { + logger.Info("pre-stop world save failed; stopping anyway, the server saves again on SIGTERM", + "error", err.Error(), "elapsed", time.Since(start).Round(time.Millisecond).String()) + return + } + logger.Info("pre-stop world save done", "elapsed", time.Since(start).Round(time.Millisecond).String()) +} + // maintenanceHold reports whether a restore, backup or file write holds the // server's world volume (internal/maintenance) while its pod is about to be // created. felis-api already refuses a wake in that state; this is the same rule diff --git a/internal/operator/reconciler_test.go b/internal/operator/reconciler_test.go index 14f0c0d..b30043e 100644 --- a/internal/operator/reconciler_test.go +++ b/internal/operator/reconciler_test.go @@ -3,6 +3,7 @@ package operator_test import ( "context" "errors" + "slices" "strings" "testing" "time" @@ -27,12 +28,22 @@ import ( type fakeProber struct { err error players operator.PlayerCount + saveErr error + // saves, when set, records each Save's address. + saves *[]string } func (f fakeProber) Probe(context.Context, string, string) (operator.PlayerCount, error) { return f.players, f.err } +func (f fakeProber) Save(_ context.Context, addr, _ string) error { + if f.saves != nil { + *f.saves = append(*f.saves, addr) + } + return f.saveErr +} + func newScheme(t *testing.T) *runtime.Scheme { t.Helper() scheme := runtime.NewScheme() @@ -61,7 +72,6 @@ func runningServer() *v1alpha1.MinecraftServer { Storage: v1alpha1.StorageSpec{Size: "10Gi"}, FallbackServer: "lobby", Motd: v1alpha1.MotdSpec{Running: "up", Stopped: "down", Starting: "booting"}, - Lifecycle: v1alpha1.LifecycleSpec{PreStopSaveAndStop: true}, Rcon: v1alpha1.RconSpec{ Enabled: true, Port: 25575, @@ -171,18 +181,16 @@ func TestReconcileRunning_CreatesWorkloadAndInjectsGracefulShutdown(t *testing.T t.Errorf("headless service ClusterIP = %q, want None", hl.Spec.ClusterIP) } - // StatefulSet exists with graceful-shutdown injection. + // StatefulSet exists with the shutdown grace period. The pre-stop save runs + // from the operator over RCON, so the pod carries no preStop hook (none of the + // game images ship an RCON client to run one with). sts := getSTS(t, c, "survival") if got := sts.Spec.Template.Spec.TerminationGracePeriodSeconds; got == nil || *got != 300 { t.Errorf("terminationGracePeriodSeconds = %v, want 300", got) } container := sts.Spec.Template.Spec.Containers[0] - if container.Lifecycle == nil || container.Lifecycle.PreStop == nil || container.Lifecycle.PreStop.Exec == nil { - t.Fatal("expected preStop exec hook to be injected") - } - preStop := strings.Join(container.Lifecycle.PreStop.Exec.Command, " ") - if !strings.Contains(preStop, "save-all flush") || !strings.Contains(preStop, "stop") { - t.Errorf("preStop hook missing save/stop sequence: %q", preStop) + if container.Lifecycle != nil { + t.Errorf("container lifecycle = %+v, want none", container.Lifecycle) } // RCON password is sourced from the Secret, never inlined. var sawRconPassword bool @@ -327,6 +335,93 @@ func TestReconcileStopped_ScalesRunningWorkloadDown(t *testing.T) { } } +// stopRunningServer takes survival to Running with a ready pod, then flips it to +// Stopped and reconciles once. +func stopRunningServer(t *testing.T, r *operator.Reconciler, c client.Client) { + t.Helper() + reconcile(t, r, "survival") + markPodReady(t, c, "survival") + reconcile(t, r, "survival") + server := getServer(t, c, "survival") + server.Spec.DesiredState = v1alpha1.DesiredStopped + if err := c.Update(context.Background(), server); err != nil { + t.Fatalf("flip desiredState: %v", err) + } + reconcile(t, r, "survival") +} + +func TestReconcileStopped_SavesBeforeScalingDown(t *testing.T) { + var saves []string + r, c := newReconciler(t, fakeProber{saves: &saves}, runningServer(), rconSecret()) + + stopRunningServer(t, r, c) + + if want := []string{"survival.minecraft.svc.cluster.local:25575"}; !slices.Equal(saves, want) { + t.Errorf("saves = %v, want %v", saves, want) + } + if sts := getSTS(t, c, "survival"); sts.Spec.Replicas == nil || *sts.Spec.Replicas != 0 { + t.Errorf("replicas = %v, want 0 after stop", sts.Spec.Replicas) + } + + // Once scaled to zero the next reconcile only waits for the pod to go; it + // does not flush again. + reconcile(t, r, "survival") + if len(saves) != 1 { + t.Errorf("saves after second reconcile = %d, want 1", len(saves)) + } +} + +func TestReconcileStopped_FailedSaveStillStops(t *testing.T) { + var saves []string + prober := fakeProber{saves: &saves, saveErr: errors.New("i/o timeout")} + r, c := newReconciler(t, prober, runningServer(), rconSecret()) + + stopRunningServer(t, r, c) + + if len(saves) != 1 { + t.Fatalf("saves = %d, want 1 attempt", len(saves)) + } + if sts := getSTS(t, c, "survival"); sts.Spec.Replicas == nil || *sts.Spec.Replicas != 0 { + t.Errorf("replicas = %v, want 0: a failed save must not hold the stop", sts.Spec.Replicas) + } + if server := getServer(t, c, "survival"); server.Status.Phase != v1alpha1.PhaseStopping { + t.Errorf("phase = %s, want Stopping", server.Status.Phase) + } +} + +func TestReconcileStopped_SkipsSaveWithoutReadyPodOrRcon(t *testing.T) { + t.Run("pod not ready", func(t *testing.T) { + var saves []string + r, c := newReconciler(t, fakeProber{saves: &saves}, runningServer(), rconSecret()) + reconcile(t, r, "survival") // StatefulSet at replicas=1, pod never ready + server := getServer(t, c, "survival") + server.Spec.DesiredState = v1alpha1.DesiredStopped + if err := c.Update(context.Background(), server); err != nil { + t.Fatalf("flip desiredState: %v", err) + } + reconcile(t, r, "survival") + if len(saves) != 0 { + t.Errorf("saves = %v, want none for a pod that never became ready", saves) + } + if sts := getSTS(t, c, "survival"); sts.Spec.Replicas == nil || *sts.Spec.Replicas != 0 { + t.Errorf("replicas = %v, want 0", sts.Spec.Replicas) + } + }) + t.Run("rcon disabled", func(t *testing.T) { + var saves []string + s := runningServer() + s.Spec.Rcon = v1alpha1.RconSpec{} + r, c := newReconciler(t, fakeProber{saves: &saves}, s) + stopRunningServer(t, r, c) + if len(saves) != 0 { + t.Errorf("saves = %v, want none without RCON", saves) + } + if sts := getSTS(t, c, "survival"); sts.Spec.Replicas == nil || *sts.Spec.Replicas != 0 { + t.Errorf("replicas = %v, want 0", sts.Spec.Replicas) + } + }) +} + // --- idle auto-stop tests (spec §8) --------------------------------------- // TestIdleAutoStop_EmptyServerGetsTimestamp verifies that the first Running diff --git a/internal/rcon/rcon.go b/internal/rcon/rcon.go index 8f29f59..1c771de 100644 --- a/internal/rcon/rcon.go +++ b/internal/rcon/rcon.go @@ -3,8 +3,8 @@ // The operator uses it for two purposes: // - Readiness probing: a successful Dial (TCP connect + auth) is the // loader-agnostic "RCON 探通" gate. A status ping is never sufficient. -// - Graceful shutdown: Execute("save-all flush") then Execute("stop") from -// the operator-injected preStop hook. +// - Graceful shutdown: Execute("save-all flush") right before the operator +// scales a server to zero. // // Multi-packet responses (a single command whose reply exceeds one ~4 KiB // packet) are not reassembled; Phase-1 commands ("list", "save-all", "stop") diff --git a/internal/rcon/rcon_test.go b/internal/rcon/rcon_test.go index 5280b43..33424a3 100644 --- a/internal/rcon/rcon_test.go +++ b/internal/rcon/rcon_test.go @@ -157,7 +157,8 @@ func TestDialUnreachable(t *testing.T) { } func TestExecuteGracefulShutdownSequence(t *testing.T) { - // Mirrors the operator preStop hook: save then stop. + // A save followed by a second command on the same connection: the reply + // ids must line up across consecutive Executes. f := startFakeRCON(t, "pw", map[string]string{ "save-all flush": "Saved the game", "stop": "Stopping the server",