From 2e07a5233f503b8d3db992d720d5040dfc0bd966 Mon Sep 17 00:00:00 2001 From: Baylem Date: Tue, 10 Nov 2026 20:56:38 -0500 Subject: [PATCH 1/2] fix: require verified backup pause and resume acknowledgments Signed-off-by: Baylem --- agent/cmd/main.go | 47 +++--- agent/internal/quiesce/quiesce.go | 134 ++++++++++++------ agent/internal/quiesce/quiesce_test.go | 5 +- agent/internal/quiesce/verified_test.go | 109 ++++++++++++++ agent/internal/rcon/commands.go | 39 +++++ agent/internal/rcon/commands_test.go | 55 +++++++ agent/openapi.yaml | 49 +++++++ agent/specs.md | 3 +- docs/module-authoring.md | 14 +- operator/internal/agent/client.go | 49 ++++--- operator/internal/agent/client_test.go | 22 ++- .../backup_container_restart_test.go | 12 +- .../internal/controller/backup_controller.go | 43 +++--- .../internal/controller/backup_guard_test.go | 11 ++ .../internal/controller/backup_preparation.go | 69 ++++++++- .../controller/backup_stopped_test.go | 2 +- .../controller/lifecycle_operation.go | 58 ++++---- operator/specs.md | 2 +- test/e2e/gamebot_helpers_e2e_test.go | 15 +- test/e2e/minecraft_bot_e2e_test.go | 53 ++++++- 20 files changed, 630 insertions(+), 161 deletions(-) create mode 100644 agent/internal/quiesce/verified_test.go create mode 100644 agent/internal/rcon/commands.go create mode 100644 agent/internal/rcon/commands_test.go diff --git a/agent/cmd/main.go b/agent/cmd/main.go index f1ca077f9..3900c18f2 100644 --- a/agent/cmd/main.go +++ b/agent/cmd/main.go @@ -138,36 +138,39 @@ func main() { modsSpec = capSpec.Mods } - var rconClient interface { - Exec(cmd string) (string, error) - } + var transport rcon.Executor switch { case !rconEnabled: // The game declares no RCON (e.g. consoleMode pty/none). Dialing // 127.0.0.1:25575 would just fail on every request; the Disabled // client makes that explicit so handlers degrade cleanly. - rconClient = rcon.Disabled{} + transport = rcon.Disabled{} case strings.EqualFold(rconProtocol, "telnet"): - rconClient = rcon.NewTelnet(rconHost, rconPort, rcon.PasswordFromFile(rconPassFile)) + transport = rcon.NewTelnet(rconHost, rconPort, rcon.PasswordFromFile(rconPassFile)) case strings.EqualFold(rconProtocol, "websocket"): - rconClient = rcon.NewWebSocket(rconHost, rconPort, rcon.PasswordFromFile(rconPassFile)) + transport = rcon.NewWebSocket(rconHost, rconPort, rcon.PasswordFromFile(rconPassFile)) case strings.EqualFold(rconProtocol, "battleye"): - rconClient = rcon.NewBattlEye(rconHost, rconPort, rcon.PasswordFromFile(rconPassFile)) + transport = rcon.NewBattlEye(rconHost, rconPort, rcon.PasswordFromFile(rconPassFile)) case strings.EqualFold(rconProtocol, "satisfactory"): - rconClient = rcon.NewSatisfactory(rconHost, rconPort, rcon.PasswordFromFile(rconPassFile)) + transport = rcon.NewSatisfactory(rconHost, rconPort, rcon.PasswordFromFile(rconPassFile)) case strings.EqualFold(rconProtocol, "palworld"): - rconClient = rcon.NewPalworld(rconHost, rconPort, rcon.PasswordFromFile(rconPassFile)) + transport = rcon.NewPalworld(rconHost, rconPort, rcon.PasswordFromFile(rconPassFile)) case strings.EqualFold(rconProtocol, "nuclearoption"): - rconClient = rcon.NewNuclearOption(rconHost, rconPort, rcon.PasswordFromFile(rconPassFile)) + transport = rcon.NewNuclearOption(rconHost, rconPort, rcon.PasswordFromFile(rconPassFile)) case strings.EqualFold(rconProtocol, "rest"): - rconClient = rcon.NewREST(rconHost, rconPort, rcon.PasswordFromFile(rconPassFile), rcon.WithGame(gameName)) + transport = rcon.NewREST(rconHost, rconPort, rcon.PasswordFromFile(rconPassFile), rcon.WithGame(gameName)) case strings.EqualFold(rconProtocol, "cli"): - rconClient = rcon.NewCLI(rconHost, rconPort, rcon.PasswordFromFile(rconPassFile), rcon.WithPipePath(cliPipe)) + transport = rcon.NewCLI(rconHost, rconPort, rcon.PasswordFromFile(rconPassFile), rcon.WithPipePath(cliPipe)) default: // "source", empty, or anything unrecognized: back-compat default. - rconClient = rcon.New(rconHost, rconPort, rcon.PasswordFromFile(rconPassFile)) + transport = rcon.New(rconHost, rconPort, rcon.PasswordFromFile(rconPassFile)) } + commands := rcon.NewCommands(transport) + rconClient := transport + if quiesceSpec != nil { + rconClient = commands + } r := chi.NewRouter() r.Use(middleware.RequestID, middleware.Recoverer) @@ -197,7 +200,7 @@ func main() { protected.Group(func(bounded chi.Router) { bounded.Use(middleware.Timeout(30 * time.Second)) players.Mount(bounded, rconClient, gameName, playerActions) - quiesce.Mount(bounded, rconClient, gameName, quiesceSpec) + quiesce.Mount(bounded, commands, gameName, quiesceSpec) lifecycle.Mount(bounded, rconClient, gameName, lifecycleSpec) actions.Mount(bounded, rconClient, gameName, actionSpecs) statusSource.Mount(bounded) @@ -213,11 +216,17 @@ func main() { }) // Backups bind save-off/save-on to the process they prepared. The service // may already route to a replacement pod, which must reject the old UID. - r.Route("/v1/pods/{uid}", func(protected chi.Router) { - protected.Use(authCheck.Middleware, target.Guard(envOr("GAMEPLANE_POD_UID", "")), middleware.Timeout(30*time.Second)) - protected.Get("/identity", func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusNoContent) }) - quiesce.Mount(protected, rconClient, gameName, quiesceSpec) - }) + for _, version := range []string{"v1", "v2"} { + r.Route("/"+version+"/pods/{uid}", func(protected chi.Router) { + protected.Use(authCheck.Middleware, target.Guard(envOr("GAMEPLANE_POD_UID", "")), middleware.Timeout(30*time.Second)) + protected.Get("/identity", func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusNoContent) }) + if version == "v2" { + quiesce.MountVerified(protected, commands, gameName, quiesceSpec) + } else { + quiesce.Mount(protected, commands, gameName, quiesceSpec) + } + }) + } srv := &http.Server{ Addr: addr, diff --git a/agent/internal/quiesce/quiesce.go b/agent/internal/quiesce/quiesce.go index 7d77d2589..13a3f2952 100644 --- a/agent/internal/quiesce/quiesce.go +++ b/agent/internal/quiesce/quiesce.go @@ -15,18 +15,19 @@ import ( "log/slog" "net/http" "regexp" + "slices" + "strings" "github.com/go-chi/chi/v5" "github.com/GameplanePanel/gameplane/agent/internal/caps" "github.com/GameplanePanel/gameplane/agent/internal/httpjson" + "github.com/GameplanePanel/gameplane/agent/internal/rcon" ) // Rcon is the slice of *rcon.Client we actually use. Defined here as // an interface so tests can swap in a recording fake. -type Rcon interface { - Exec(cmd string) (string, error) -} +type Rcon = rcon.Executor // Quiescer encapsulates the game-specific RCON sequence for pausing // writes (Quiesce) and resuming them (Unquiesce). Implementations @@ -53,54 +54,98 @@ func Pick(spec *caps.Quiesce) Quiescer { type response struct { Quiesced bool `json:"quiesced"` + Outcome string `json:"outcome,omitempty"` Reason string `json:"reason,omitempty"` } -// Mount registers POST /quiesce and POST /unquiesce on r. -func Mount(r chi.Router, rc Rcon, game string, spec *caps.Quiesce) { - q := Pick(spec) - r.Post("/quiesce", func(w http.ResponseWriter, _ *http.Request) { - if !q.Supported() { - httpjson.Write(w, http.StatusOK, response{Quiesced: false, Reason: "game does not support quiesce"}) - return - } - if err := q.Quiesce(rc); err != nil { - slog.Warn("quiesce failed", "game", game, "err", err) - httpjson.Write(w, http.StatusBadGateway, response{Quiesced: false, Reason: "rcon error"}) - return - } - httpjson.Write(w, http.StatusOK, response{Quiesced: true}) - }) - r.Post("/unquiesce", func(w http.ResponseWriter, _ *http.Request) { - if !q.Supported() { - httpjson.Write(w, http.StatusOK, response{Quiesced: false, Reason: "game does not support quiesce"}) - return - } - if err := q.Unquiesce(rc); err != nil { - // Unquiesce failure is more dangerous than quiesce failure - // (the game is now stuck with auto-save off), but we still - // must not block the controller — surface 502 so the - // operator can record an event. - slog.Warn("unquiesce failed", "game", game, "err", err) - httpjson.Write(w, http.StatusBadGateway, response{Quiesced: true, Reason: "unquiesce rcon error"}) - return - } - httpjson.Write(w, http.StatusOK, response{Quiesced: false}) - }) +// Mount preserves the legacy declaration-driven endpoints for older clients. +func Mount(r chi.Router, rc *rcon.Commands, game string, spec *caps.Quiesce) { + mount(r, rc, game, Pick(spec), false) +} + +// MountVerified accepts only a sequence for which every game reply can be verified. +// Unsupported pause is safe to degrade because no command has been issued. +func MountVerified(r chi.Router, rc *rcon.Commands, game string, spec *caps.Quiesce) { + var q Quiescer = unsupportedQuiescer{} + if game == "minecraft-java" && spec != nil && + slices.Equal(spec.Quiesce, []string{"save-off", "save-all flush"}) && + slices.Equal(spec.Unquiesce, []string{"save-on"}) { + declared := newDeclaredQuiescer(spec) + declared.rollbackFirst = true + q = declared + } + mount(r, rc, game, q, true) +} + +func mount(r chi.Router, rc *rcon.Commands, game string, q Quiescer, verified bool) { + handle := func(path, outcome string, paused bool, run func(Rcon) error) { + r.Post(path, func(w http.ResponseWriter, req *http.Request) { + if !q.Supported() { + httpjson.Write(w, http.StatusOK, response{Outcome: "unsupported", Reason: "game does not support quiesce"}) + return + } + err := rc.Sequence(req.Context(), func(transport rcon.Executor) error { + if verified { + transport = minecraftReplies{transport} + } + return run(transport) + }) + if err != nil { + slog.Warn("backup command failed", "path", path, "game", game, "err", err) + httpjson.Write(w, http.StatusBadGateway, response{Quiesced: !paused, Reason: "backup command failed"}) + return + } + ack := response{Quiesced: paused} + if verified { + ack.Outcome = outcome + } + httpjson.Write(w, http.StatusOK, ack) + }) + } + handle("/quiesce", "paused", true, q.Quiesce) + handle("/unquiesce", "resumed", false, q.Unquiesce) +} + +type minecraftReplies struct{ Rcon } + +// Vanilla Java 1.21.4 en_us commands.save.* messages. RCON can concatenate +// the two flush messages with no separator. Never accept a success substring +// embedded in an error, or a reply to the opposite command. +func (m minecraftReplies) Exec(cmd string) (string, error) { + reply, err := m.Rcon.Exec(cmd) + if err != nil { + return "", err + } + out := strings.TrimSpace(reply) + var ok bool + switch cmd { + case "save-off": + ok = out == "Automatic saving is now disabled" || out == "Saving is already turned off" + case "save-on": + ok = out == "Automatic saving is now enabled" || out == "Saving is already turned on" + case "save-all flush": + out = strings.TrimSpace(strings.TrimPrefix(out, "Saving the game (this may take a moment!)")) + ok = out == "Saved the game" + } + if !ok { + return "", fmt.Errorf("game did not acknowledge %q", cmd) + } + return reply, nil } // --- Declared (module-driven) ------------------------------------------ type declaredQuiescer struct { - quiesce []string - unquiesce []string - failRE *regexp.Regexp + quiesce []string + unquiesce []string + failRE *regexp.Regexp + rollbackFirst bool } // newDeclaredQuiescer compiles the declared sequences. An invalid // FailurePattern is logged and ignored — the commands still run, the // output check just can't. -func newDeclaredQuiescer(spec *caps.Quiesce) Quiescer { +func newDeclaredQuiescer(spec *caps.Quiesce) declaredQuiescer { q := declaredQuiescer{quiesce: spec.Quiesce, unquiesce: spec.Unquiesce} if spec.FailurePattern != "" { re, err := regexp.Compile("(?i)" + spec.FailurePattern) @@ -122,10 +167,9 @@ func (q declaredQuiescer) Quiesce(rc Rcon) error { err = fmt.Errorf("quiesce command %q reported failure", cmd) } if err != nil { - // Roll auto-save (or whatever the game paused) back on so we - // don't leave the world frozen — but only if any pausing - // command already ran. - if i > 0 { + // A timed-out save-off may already have reached the game. + // The verified save-on contract accepts "already on" on retry. + if i > 0 || q.rollbackFirst { _ = q.Unquiesce(rc) } return err @@ -137,7 +181,11 @@ func (q declaredQuiescer) Quiesce(rc Rcon) error { func (q declaredQuiescer) Unquiesce(rc Rcon) error { var firstErr error for _, cmd := range q.unquiesce { - if _, err := rc.Exec(cmd); err != nil && firstErr == nil { + out, err := rc.Exec(cmd) + if err == nil && q.failRE != nil && q.failRE.MatchString(out) { + err = fmt.Errorf("unquiesce command %q reported failure", cmd) + } + if err != nil && firstErr == nil { firstErr = err } } diff --git a/agent/internal/quiesce/quiesce_test.go b/agent/internal/quiesce/quiesce_test.go index 62aca43a4..64671914c 100644 --- a/agent/internal/quiesce/quiesce_test.go +++ b/agent/internal/quiesce/quiesce_test.go @@ -12,6 +12,7 @@ import ( "github.com/go-chi/chi/v5" "github.com/GameplanePanel/gameplane/agent/internal/caps" + "github.com/GameplanePanel/gameplane/agent/internal/rcon" "github.com/GameplanePanel/gameplane/agent/internal/target" ) @@ -83,7 +84,7 @@ func TestUnsupportedQuiescerNoOp(t *testing.T) { func newTestRouter(rc Rcon, spec *caps.Quiesce) *chi.Mux { r := chi.NewRouter() - Mount(r, rc, "testgame", spec) + Mount(r, rcon.NewCommands(rc), "testgame", spec) return r } @@ -94,7 +95,7 @@ func TestPodBoundRoutesRejectReplacementBeforeRCON(t *testing.T) { r := chi.NewRouter() r.Route("/v1/pods/{uid}", func(bound chi.Router) { bound.Use(target.Guard(actual)) - Mount(bound, rc, "test", minecraftQuiesceSpec()) + Mount(bound, rcon.NewCommands(rc), "test", minecraftQuiesceSpec()) }) for _, path := range []string{"quiesce", "unquiesce"} { response := httptest.NewRecorder() diff --git a/agent/internal/quiesce/verified_test.go b/agent/internal/quiesce/verified_test.go new file mode 100644 index 000000000..6bd8d0765 --- /dev/null +++ b/agent/internal/quiesce/verified_test.go @@ -0,0 +1,109 @@ +package quiesce + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "reflect" + "sync" + "testing" + + "github.com/go-chi/chi/v5" + + "github.com/GameplanePanel/gameplane/agent/internal/rcon" +) + +func TestMinecraftPositiveCommandResponses(t *testing.T) { + for _, tc := range []struct { + cmd, reply string + ok bool + }{ + {"save-off", "Automatic saving is now disabled", true}, + {"save-off", "Saving is already turned off", true}, + {"save-on", "Automatic saving is now enabled", true}, + {"save-on", "Saving is already turned on", true}, + {"save-all flush", "Saving the game (this may take a moment!)Saved the game", true}, + {"save-all flush", "Saving the game (this may take a moment!)\nSaved the game", true}, + {"save-all flush", "Saved the game", true}, + {"save-all flush", "Saving the game (this may take a moment!)", false}, + {"save-all flush", "Unable to save the game (is there enough disk space?)", false}, + {"save-on", "Automatic saving is now disabled", false}, + {"save-on", "", false}, + {"save-on", "Unknown or incomplete command", false}, + {"save-on", "Error: Automatic saving is now enabled", false}, + } { + t.Run(tc.cmd+"/"+tc.reply, func(t *testing.T) { + m := minecraftReplies{&fakeRcon{respond: func(string) (string, error) { return tc.reply, nil }}} + if _, err := m.Exec(tc.cmd); (err == nil) != tc.ok { + t.Fatalf("reply %q: %v", tc.reply, err) + } + }) + } +} + +func TestVerifiedSequenceBlocksCleanupUntilRollbackFinishes(t *testing.T) { + reached, release := make(chan struct{}), make(chan struct{}) + var once sync.Once + rc := &fakeRcon{respond: func(cmd string) (string, error) { + switch cmd { + case "save-off": + return "Automatic saving is now disabled", nil + case "save-all flush": + once.Do(func() { close(reached) }) + <-release + return "", context.DeadlineExceeded + case "save-on": + return "Saving is already turned on", nil + } + return "", errors.New("unexpected command") + }} + commands := rcon.NewCommands(rc) + router := chi.NewRouter() + MountVerified(router, commands, "minecraft-java", minecraftQuiesceSpec()) + pause := make(chan int, 1) + go func() { + w := httptest.NewRecorder() + router.ServeHTTP(w, httptest.NewRequest(http.MethodPost, "/quiesce", nil)) + pause <- w.Code + }() + <-reached + ctx, cancel := context.WithCancel(t.Context()) + cancel() + w := httptest.NewRecorder() + router.ServeHTTP(w, httptest.NewRequestWithContext(ctx, http.MethodPost, "/quiesce", nil)) + if w.Code != http.StatusBadGateway { + t.Errorf("canceled pause: %d", w.Code) + } + resumed := make(chan *httptest.ResponseRecorder, 1) + go func() { + w := httptest.NewRecorder() + router.ServeHTTP(w, httptest.NewRequest(http.MethodPost, "/unquiesce", nil)) + resumed <- w + }() + close(release) + if status := <-pause; status != http.StatusBadGateway { + t.Errorf("timed-out pause: %d", status) + } + w = <-resumed + if w.Code != http.StatusOK || decodeResponse(t, w.Body.Bytes()).Outcome != "resumed" { + t.Fatalf("cleanup response: %d %s", w.Code, w.Body) + } + want := []string{"save-off", "save-all flush", "save-on", "save-on"} + if got := rc.calls(); !reflect.DeepEqual(got, want) { + t.Fatalf("commands = %v, want %v", got, want) + } +} + +func TestVerifiedRejectsUnqualifiedDeclarations(t *testing.T) { + rc := &fakeRcon{} + r := chi.NewRouter() + MountVerified(r, rcon.NewCommands(rc), "other-game", minecraftQuiesceSpec()) + for _, path := range []string{"/quiesce", "/unquiesce"} { + w := httptest.NewRecorder() + r.ServeHTTP(w, httptest.NewRequestWithContext(t.Context(), http.MethodPost, path, nil)) + if got := decodeResponse(t, w.Body.Bytes()); got.Outcome != "unsupported" || got.Quiesced || len(rc.calls()) != 0 { + t.Fatalf("%s = %+v, calls %v", path, got, rc.calls()) + } + } +} diff --git a/agent/internal/rcon/commands.go b/agent/internal/rcon/commands.go new file mode 100644 index 000000000..7b5babc05 --- /dev/null +++ b/agent/internal/rcon/commands.go @@ -0,0 +1,39 @@ +package rcon + +import "context" + +// Executor is the command transport shared by all handlers for one game. +type Executor interface { + Exec(string) (string, error) +} + +// Commands admits either one command or one complete backup sequence at a time. +type Commands struct { + transport Executor + busy chan struct{} +} + +func NewCommands(transport Executor) *Commands { + return &Commands{transport: transport, busy: make(chan struct{}, 1)} +} + +func (c *Commands) Exec(command string) (string, error) { + c.busy <- struct{}{} + defer func() { <-c.busy }() + return c.transport.Exec(command) +} + +// Sequence gives run exclusive use of the underlying transport, including +// any compensating commands. A canceled waiter must never start a late pause. +func (c *Commands) Sequence(ctx context.Context, run func(Executor) error) error { + select { + case <-ctx.Done(): + return ctx.Err() + case c.busy <- struct{}{}: + } + defer func() { <-c.busy }() + if err := ctx.Err(); err != nil { + return err + } + return run(c.transport) +} diff --git a/agent/internal/rcon/commands_test.go b/agent/internal/rcon/commands_test.go new file mode 100644 index 000000000..6f8c9c540 --- /dev/null +++ b/agent/internal/rcon/commands_test.go @@ -0,0 +1,55 @@ +package rcon + +import ( + "context" + "errors" + "slices" + "testing" + "testing/synctest" +) + +type recordCommand func(string) (string, error) + +func (f recordCommand) Exec(command string) (string, error) { return f(command) } + +func TestCommandsExcludeOtherCallersFromSequence(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + var calls []string + c := NewCommands(recordCommand(func(s string) (string, error) { calls = append(calls, s); return s, nil })) + release := make(chan struct{}) + go func() { + err := c.Sequence(t.Context(), func(rc Executor) error { + _, _ = rc.Exec("first") + <-release + _, _ = rc.Exec("second") + return nil + }) + if err != nil { + t.Error(err) + } + }() + synctest.Wait() + go func() { + if out, err := c.Exec("console"); out != "console" || err != nil { + t.Errorf("console: %q, %v", out, err) + } + }() + ctx, cancel := context.WithCancel(t.Context()) + go func() { + if err := c.Sequence(ctx, func(Executor) error { t.Error("ran canceled sequence"); return nil }); !errors.Is(err, context.Canceled) { + t.Errorf("canceled sequence: %v", err) + } + }() + synctest.Wait() + cancel() + synctest.Wait() + if !slices.Equal(calls, []string{"first"}) { + t.Fatalf("sequence interleaved: %v", calls) + } + close(release) + synctest.Wait() + if !slices.Equal(calls, []string{"first", "second", "console"}) { + t.Fatalf("calls=%v", calls) + } + }) +} diff --git a/agent/openapi.yaml b/agent/openapi.yaml index 481eee593..49b94061e 100644 --- a/agent/openapi.yaml +++ b/agent/openapi.yaml @@ -376,3 +376,52 @@ paths: quiesced: { type: boolean, description: "false on success" } reason: { type: string } "502": { description: RCON unavailable; the game may still be quiesced — retry } + + /v2/pods/{uid}/identity: + parameters: + - &podUID + name: uid + in: path + required: true + schema: { type: string } + get: + summary: Verify the pod UID and availability of the verified backup protocol + responses: + "204": { description: Matching pod UID } + "404": { description: Missing or mismatched pod UID } + /v2/pods/{uid}/quiesce: + parameters: [*podUID] + post: + summary: Pause with verified game command replies + responses: + "200": + description: Paused or unsupported + content: + application/json: + schema: + type: object + required: [outcome, quiesced] + properties: + outcome: { type: string, enum: [paused, unsupported] } + quiesced: { type: boolean, description: "true only when paused" } + reason: { type: string } + "502": { description: Transport failure or unacknowledged command } + "404": { description: Missing or mismatched pod UID } + /v2/pods/{uid}/unquiesce: + parameters: [*podUID] + post: + summary: Resume with verified game command replies + responses: + "200": + description: Only resumed confirms completion of cleanup + content: + application/json: + schema: + type: object + required: [outcome, quiesced] + properties: + outcome: { type: string, enum: [resumed, unsupported] } + quiesced: { type: boolean, enum: [false] } + reason: { type: string } + "502": { description: Transport failure or unacknowledged command } + "404": { description: Missing or mismatched pod UID } diff --git a/agent/specs.md b/agent/specs.md index 2ee465f71..40935f565 100644 --- a/agent/specs.md +++ b/agent/specs.md @@ -187,8 +187,9 @@ Resource usage env vars (set by the operator): | `/players/banned` | GET | Currently banned players; response: `[ { name, reason?, source? } ]`; 502 if RCON unavailable | | `/quiesce` | POST | Pause auto-saves before snapshot; response: `{ quiesced, reason? }` (boolean, reason optional); unsupported games return `quiesced: false` with a reason | | `/unquiesce` | POST | Resume auto-saves after snapshot; response: `{ quiesced, reason? }` | -| `/v1/pods/{uid}/identity` | GET | Operator preflight: 204 only when `{uid}` matches `GAMEPLANE_POD_UID`; never mutates game state | +| `/v2/pods/{uid}/identity`, `/v1/pods/{uid}/identity` | GET | Operator preflight: 204 only when `{uid}` matches `GAMEPLANE_POD_UID`; never mutates game state | | `/v1/pods/{uid}/quiesce`, `/v1/pods/{uid}/unquiesce` | POST | Pod-bound backup RPCs with the same response contract as the legacy routes; reject missing/mismatched Pod UID with 404 before RCON | +| `/v2/pods/{uid}/quiesce`, `/v2/pods/{uid}/unquiesce` | POST | Verified backup RPCs. HTTP 200 requires `outcome: paused` with `quiesced: true`, or `outcome: resumed` with `quiesced: false`. The shipped Minecraft Java sequence checks each vanilla English RCON reply, including already-on/off retries. Unsupported sequences return `outcome: unsupported`, `quiesced: false` and a reason without issuing commands. The whole sequence, including rollback, excludes other agent RCON calls. | | `/lifecycle/stop` | POST | Run stop sequence before scale-to-zero; RCON connection drop is the expected outcome | | `/actions/run` | POST | Execute module-declared operator action; request: `{ id, params }`; response: `{ ok, raw? }` | | `/status` | GET | Live game metrics (module-declared); response: bare JSON array `[ { id, displayName?, value, unit? } ]` | diff --git a/docs/module-authoring.md b/docs/module-authoring.md index a6fb80600..08a2886ec 100644 --- a/docs/module-authoring.md +++ b/docs/module-authoring.md @@ -1214,13 +1214,13 @@ capabilities: From `modules/palworld/template.yaml` (note: Palworld uses the REST admin API `protocol: palworld`, not RCON, so the players list is fetched via the REST endpoint, not a console command). -- The quiesce sequence runs in order; any command error — or output - matching `failurePattern` (case-insensitive) — aborts the backup. If - at least one earlier command in the sequence already succeeded, - `unquiesce` then runs best-effort so the game is never left paused; - a failure on the *first* command means nothing was paused yet, so - `unquiesce` is skipped. Games that can't quiesce simply omit the - block; backups proceed without pausing. +- The operator uses the verified agent endpoint for live backups. For + `minecraft-java`, declare the sequence above. The agent checks the English + vanilla response to each command, including the already-on and already-off + replies on retries. A rejected flush triggers a save-on attempt. Unrecognized + command sequences report unsupported before issuing a command. The legacy + agent endpoints still execute module-declared sequences and check their + optional failure pattern on both pause and resume. - `lifecycle.stop` runs before the pod is scaled down (stop button, restarts): the operator issues the sequence — over RCON when the template has it, otherwise over a stdin pod-attach for `consoleMode: diff --git a/operator/internal/agent/client.go b/operator/internal/agent/client.go index 2b1c06923..2857461f0 100644 --- a/operator/internal/agent/client.go +++ b/operator/internal/agent/client.go @@ -16,6 +16,7 @@ import ( "net/http" "net/url" "os" + "strings" "time" ) @@ -103,7 +104,8 @@ func agentURL(namespace, server, path string) string { } type quiesceResponse struct { - Quiesced bool `json:"quiesced"` + Quiesced *bool `json:"quiesced"` + Outcome string `json:"outcome"` Reason string `json:"reason,omitempty"` } @@ -129,7 +131,7 @@ func (c *Client) CheckPod(ctx context.Context, namespace, server, podUID string) if c.Disabled || podUID == "" { return fmt.Errorf("agent: pod identity check requires an enabled client and pod UID") } - req, err := http.NewRequestWithContext(ctx, http.MethodGet, agentURL(namespace, server, "/v1/pods/"+url.PathEscape(podUID)+"/identity"), nil) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, agentURL(namespace, server, "/v2/pods/"+url.PathEscape(podUID)+"/identity"), nil) if err != nil { return err } @@ -158,7 +160,7 @@ func (c *Client) callPodQuiesce(ctx context.Context, namespace, server, podUID, if c.Disabled || podUID == "" { return fmt.Errorf("agent: pod-bound quiesce requires an enabled client and pod UID") } - return c.callQuiesce(ctx, namespace, server, "/v1/pods/"+url.PathEscape(podUID)+path, expectQuiesced) + return c.callQuiesce(ctx, namespace, server, "/v2/pods/"+url.PathEscape(podUID)+path, expectQuiesced) } // expectQuiesced is what the response should report on a successful @@ -177,25 +179,40 @@ func (c *Client) callQuiesce(ctx context.Context, namespace, server, path string return fmt.Errorf("agent: %s: %w", path, err) } defer func() { _ = resp.Body.Close() }() - body, _ := io.ReadAll(io.LimitReader(resp.Body, 16<<10)) + const maxReply = 16 << 10 + body, err := io.ReadAll(io.LimitReader(resp.Body, maxReply+1)) + if err != nil { + return fmt.Errorf("agent: %s: read response: %w", path, err) + } + if len(body) > maxReply { + return fmt.Errorf("agent: %s: oversized response", path) + } if resp.StatusCode != http.StatusOK { return fmt.Errorf("agent: %s: status %d", path, resp.StatusCode) } var parsed quiesceResponse - if len(body) > 0 { - if err := json.Unmarshal(body, &parsed); err != nil { - return fmt.Errorf("agent: %s: decode response: %w", path, err) - } - } - // Detect the unsupported-game case via the canonical reason string - // so we can report it distinctly without leaking it as a hard error. - if !parsed.Quiesced && expectQuiesced { - if parsed.Reason == "game does not support quiesce" { - return ErrUnsupported - } - return fmt.Errorf("agent: %s: refused (%s)", path, parsed.Reason) + if err := json.Unmarshal(body, &parsed); err != nil { + return fmt.Errorf("agent: %s: decode response: %w", path, err) + } + if parsed.Quiesced == nil { + return fmt.Errorf("agent: %s: missing quiesced field", path) + } + verified := strings.HasPrefix(path, "/v2/pods/") + if expectQuiesced && !*parsed.Quiesced && parsed.Reason == "game does not support quiesce" && + (!verified || parsed.Outcome == "unsupported") { + return ErrUnsupported + } + if *parsed.Quiesced != expectQuiesced || parsed.Reason != "" { + return fmt.Errorf("agent: %s: refused or contradictory acknowledgment", path) + } + want := "resumed" + if expectQuiesced { + want = "paused" + } + if verified && parsed.Outcome != want { + return fmt.Errorf("agent: %s: missing or contradictory outcome", path) } return nil } diff --git a/operator/internal/agent/client_test.go b/operator/internal/agent/client_test.go index 7d04dc9d1..987912bf7 100644 --- a/operator/internal/agent/client_test.go +++ b/operator/internal/agent/client_test.go @@ -129,14 +129,14 @@ func TestPodBoundQuiesceNeverFallsBackToLegacyRoute(t *testing.T) { c, closeServer := newMockClient(t, func(w http.ResponseWriter, r *http.Request) { paths = append(paths, r.URL.Path) w.WriteHeader(status) - _, _ = w.Write([]byte(`{"quiesced":true}`)) + _, _ = w.Write([]byte(`{"quiesced":true,"outcome":"paused"}`)) }) defer closeServer() err := c.QuiescePod(t.Context(), "ns", "server", "original-pod") if (err == nil) != (status == http.StatusOK) { t.Fatalf("status=%d error=%v", status, err) } - if len(paths) != 1 || paths[0] != "/v1/pods/original-pod/quiesce" { + if len(paths) != 1 || paths[0] != "/v2/pods/original-pod/quiesce" { t.Fatalf("unbound mutation/fallback: %v", paths) } }) @@ -145,10 +145,10 @@ func TestPodBoundQuiesceNeverFallsBackToLegacyRoute(t *testing.T) { func TestPodBoundUnquiesceAndDisabledClient(t *testing.T) { c, closeServer := newMockClient(t, func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path != "/v1/pods/original-pod/unquiesce" { + if r.URL.Path != "/v2/pods/original-pod/unquiesce" { t.Errorf("wrong cleanup target: %s", r.URL.Path) } - _, _ = w.Write([]byte(`{"quiesced":false}`)) + _, _ = w.Write([]byte(`{"quiesced":false,"outcome":"resumed"}`)) }) defer closeServer() if err := c.UnquiescePod(t.Context(), "ns", "server", "original-pod"); err != nil { @@ -303,7 +303,7 @@ func TestCheckPodRequiresIdentityEndpoint(t *testing.T) { calls := 0 c, cleanup := newMockClient(t, func(w http.ResponseWriter, req *http.Request) { calls++ - if req.Method != http.MethodGet || req.URL.Path != "/v1/pods/original/identity" { + if req.Method != http.MethodGet || req.URL.Path != "/v2/pods/original/identity" { t.Errorf("unexpected preflight request: %s %s", req.Method, req.URL.Path) } w.WriteHeader(status) @@ -339,3 +339,15 @@ type errTransport struct{} func (errTransport) RoundTrip(*http.Request) (*http.Response, error) { return nil, errors.New("dial fail") } + +func TestPodBoundQuiesceRequiresDirectionalAcknowledgment(t *testing.T) { + for _, body := range []string{"", "not json", "null", "{}", `{"outcome":"resumed"}`, `{"quiesced":null,"outcome":"resumed"}`, `{"quiesced":true,"outcome":"resumed"}`, `{"quiesced":false,"outcome":"paused"}`, `{"quiesced":false}`, `{"quiesced":false,"outcome":"unsupported","reason":"game does not support quiesce"}`, `{"quiesced":false,"outcome":"resumed","reason":"rcon error"}`, `{"quiesced":false,"outcome":"resumed"}{}`, strings.Repeat(" ", 16<<10+1)} { + t.Run(body[:min(64, len(body))], func(t *testing.T) { + c, closeServer := newMockClient(t, func(w http.ResponseWriter, _ *http.Request) { _, _ = w.Write([]byte(body)) }) + defer closeServer() + if err := c.UnquiescePod(t.Context(), "ns", "game", "pod"); err == nil || errors.Is(err, ErrUnsupported) { + t.Fatalf("invalid response discharged cleanup: %v", err) + } + }) + } +} diff --git a/operator/internal/controller/backup_container_restart_test.go b/operator/internal/controller/backup_container_restart_test.go index 5043340b2..e95929f46 100644 --- a/operator/internal/controller/backup_container_restart_test.go +++ b/operator/internal/controller/backup_container_restart_test.go @@ -109,8 +109,16 @@ func TestBackupGameContainerChangesAbortAndDrain(t *testing.T) { if err := r.Get(t.Context(), client.ObjectKeyFromObject(&gs), &gs); err != nil { t.Fatal(err) } - if gs.Annotations[lifecycleOperationAnnotation] != "" || (stage != "before-quiesce" && q.unquiesced != 1) { - t.Fatalf("drained copy did not finish cleanup: owner=%q, agent=%+v", gs.Annotations[lifecycleOperationAnnotation], q) + unidentified := stage != "before-quiesce" && (name == "waiting" || name == "missing-id" || name == "missing-status") + if (gs.Annotations[lifecycleOperationAnnotation] != "") != unidentified || q.unquiesced != 0 { + t.Fatalf("wrong cleanup after worker drain: owner=%q, agent=%+v", gs.Annotations[lifecycleOperationAnnotation], q) + } + if stage != "before-quiesce" { + b = guardGetBackup(t, r, "first") + condition := meta.FindStatusCondition(b.Status.Conditions, "Unquiesced") + if condition == nil || (unidentified && condition.Status != metav1.ConditionFalse) || (!unidentified && condition.Reason != "SourceProcessGone") { + t.Fatalf("wrong original-process cleanup condition: %+v", condition) + } } }) } diff --git a/operator/internal/controller/backup_controller.go b/operator/internal/controller/backup_controller.go index 694429e53..9f61106a8 100644 --- a/operator/internal/controller/backup_controller.go +++ b/operator/internal/controller/backup_controller.go @@ -404,7 +404,8 @@ func (r *BackupReconciler) maybeQuiesce(ctx context.Context, b *gameplanev1alpha } return nil } - if state := b.Annotations[annoQuiesceAttempted]; state != "" && state != "pending" { + state := b.Annotations[annoQuiesceAttempted] + if state != "" && state != "pending" { return nil } // Verify the versioned endpoint before creating the first cleanup debt. @@ -447,9 +448,9 @@ func (r *BackupReconciler) maybeQuiesce(ctx context.Context, b *gameplanev1alpha annoQuiesceAttempted: "true", annoQuiescedAt: now, }) - case errors.Is(err, agent.ErrUnsupported): - // Game has no quiesce equivalent. Mark attempted and proceed — - // games like Valheim write to disk consistently anyway. + case errors.Is(err, agent.ErrUnsupported) && state != "pending": + // Only a first, explicitly unsupported operation proves that no + // command was issued. An ambiguous earlier attempt still owes resume. patchBackupAnnotations(b, map[string]string{ annoQuiesceAttempted: "unsupported", }) @@ -689,7 +690,7 @@ func (r *BackupReconciler) setUnquiescedCondition( cond.Reason = "Unquiesced" if b.Annotations[backupCleanupOutcomeAnnotation] == "source-gone" { cond.Reason = "SourceProcessGone" - cond.Message = "original process no longer exists; no unquiesce RPC was sent" + cond.Message = "original game process no longer exists" } } else { cond.Status = metav1.ConditionFalse @@ -805,11 +806,11 @@ func (r *BackupReconciler) unquiesceTargetGone(ctx context.Context, b *gameplane if err != nil { return false, err } - var podUID types.UID - if preparation != nil { - podUID = preparation.PodUID + if preparation == nil { + preparation = &backupPreparation{} } - return backupQuiesceProcessGone(ctx, r.backupAPIReader(), b.Namespace, targetName, podUID) + state, err := backupGameState(ctx, r.backupAPIReader(), b.Namespace, targetName, preparation.PodUID, preparation.GameContainerID, preparation.GameRestartCount) + return state == gameGone, err } // unquiesceRetryExpired records (on first call) when finalizeDelete's @@ -897,30 +898,20 @@ func (r *BackupReconciler) maybeUnquiesce(ctx context.Context, b *gameplanev1alp return fmt.Errorf("legacy quiesce cleanup is blocked by a protected backup") } } - if targetName != "" { - gone, err := r.unquiesceTargetGone(ctx, b) - if err != nil { - return err - } - if gone { - patchBackupAnnotations(b, map[string]string{backupCleanupOutcomeAnnotation: "source-gone"}) - return r.recordBackupUnquiesced(ctx, b) - } - } - if !agentQuiesceAvailable(r.AgentClient) { - return fmt.Errorf("cannot release quiesced world without an agent client") - } preparation, err := parseBackupPreparation(b) if err != nil { return err } - var podUID types.UID - if preparation != nil { - podUID = preparation.PodUID + if preparation == nil { + preparation = &backupPreparation{} } - if err := callBackupQuiesce(ctx, r.AgentClient, b.Namespace, targetName, podUID, true); err != nil { + gone, err := resumeBackupProcess(ctx, r.backupAPIReader(), r.AgentClient, b.Namespace, targetName, preparation.PodUID, preparation.GameContainerID, preparation.GameRestartCount) + if err != nil { return err } + if gone { + patchBackupAnnotations(b, map[string]string{backupCleanupOutcomeAnnotation: "source-gone"}) + } return r.recordBackupUnquiesced(ctx, b) } diff --git a/operator/internal/controller/backup_guard_test.go b/operator/internal/controller/backup_guard_test.go index c995b8b38..e1edd39f8 100644 --- a/operator/internal/controller/backup_guard_test.go +++ b/operator/internal/controller/backup_guard_test.go @@ -485,6 +485,17 @@ func TestBackupQuiesceIntentSurvivesPostAgentWriteFailure(t *testing.T) { if b.Annotations[annoQuiesceAttempted] != "pending" { t.Fatal("cleanup intent was not durable before save-off") } + // A fresh reconciler reads the pending intent. An unsupported retry + // describes that retry only; the previous save-off may have succeeded. + r = &BackupReconciler{Client: base, Scheme: r.Scheme, AgentClient: q, LogReader: r.LogReader} + q.quiesceErr = agent.ErrUnsupported + if err := r.maybeQuiesce(t.Context(), b); err == nil { + t.Fatal("unsupported retry erased an ambiguous pause") + } + b = guardGetBackup(t, r, "first") + if b.Annotations[annoQuiesceAttempted] != "pending" || b.Status.Phase != gameplanev1alpha1.BackupPhaseFailed { + t.Fatalf("lost pending cleanup across restart: %+v", b) + } if err := r.Delete(context.Background(), b); err != nil { t.Fatal(err) } diff --git a/operator/internal/controller/backup_preparation.go b/operator/internal/controller/backup_preparation.go index 1993d0500..e2c4b746a 100644 --- a/operator/internal/controller/backup_preparation.go +++ b/operator/internal/controller/backup_preparation.go @@ -289,16 +289,77 @@ func (r *BackupReconciler) backupSourceDrained(ctx context.Context, b *gameplane // unready pod, or terminating pod with the same UID is never proof of exit. // Empty UID is legacy state: only absence or an observed terminal pod suffices. func backupQuiesceProcessGone(ctx context.Context, reader client.Reader, namespace, server string, uid types.UID) (bool, error) { + state, err := backupGameState(ctx, reader, namespace, server, uid, "", nil) + return state == gameGone, err +} + +type gameState int + +const ( + gameUnknown gameState = iota + gameOriginal + gameGone +) + +// A bound game container may restart without replacing the pod. Missing +// kubelet status is ambiguous. Never send save-on to an unidentified game. +func backupGameState(ctx context.Context, reader client.Reader, namespace, server string, uid types.UID, containerID string, restarts *int32) (gameState, error) { var pod corev1.Pod err := reader.Get(ctx, client.ObjectKey{Namespace: namespace, Name: server + "-0"}, &pod) if apierrors.IsNotFound(err) { - return true, nil + return gameGone, nil } if err != nil { - return false, err + return gameUnknown, err } if uid != "" && pod.UID != uid { - return true, nil + return gameGone, nil + } + if pod.DeletionTimestamp.IsZero() && (pod.Status.Phase == corev1.PodSucceeded || pod.Status.Phase == corev1.PodFailed) { + return gameGone, nil + } + // Pre-upgrade owners can lack a container fingerprint. Preserve their + // pod-bound cleanup rules and never invent a fingerprint from current state. + if uid == "" || (containerID == "" && restarts == nil) { + return gameOriginal, nil + } + for _, game := range pod.Status.ContainerStatuses { + if game.Name != "game" || game.ContainerID == "" { + continue + } + if game.State.Terminated != nil { + return gameGone, nil + } + if game.State.Running != nil { + if (containerID != "" && game.ContainerID != containerID) || (restarts != nil && game.RestartCount > *restarts) { + return gameGone, nil + } + if restarts == nil || game.RestartCount == *restarts { + return gameOriginal, nil + } + } + } + return gameUnknown, nil +} + +// Confirm the same process both sides of the verified RPC. Cleanup callers +// record a replacement as process exit, independently of a successful reply. +func resumeBackupProcess(ctx context.Context, reader client.Reader, q AgentQuiescer, ns, server string, uid types.UID, containerID string, restarts *int32) (bool, error) { + if server == "" { + return false, fmt.Errorf("cleanup target has no server name") + } + check := func() (bool, error) { + state, err := backupGameState(ctx, reader, ns, server, uid, containerID, restarts) + if err == nil && state == gameUnknown { + err = fmt.Errorf("original game container state is unavailable") + } + return state == gameGone, err + } + if gone, err := check(); gone || err != nil { + return gone, err + } + if err := callBackupQuiesce(ctx, q, ns, server, uid, true); err != nil { + return false, err } - return pod.DeletionTimestamp.IsZero() && (pod.Status.Phase == corev1.PodSucceeded || pod.Status.Phase == corev1.PodFailed), nil + return check() } diff --git a/operator/internal/controller/backup_stopped_test.go b/operator/internal/controller/backup_stopped_test.go index a61c43712..ab3d5a522 100644 --- a/operator/internal/controller/backup_stopped_test.go +++ b/operator/internal/controller/backup_stopped_test.go @@ -212,7 +212,7 @@ func TestBackupLiveTimeoutRetainsOwnerUntilOriginalPodGone(t *testing.T) { t.Fatal(err) } op, _, err := lifecycleOwner(&gs) - if err != nil || op.QuiescePodUID != "game-pod-uid" || !op.NeedsUnquiesce { + if err != nil || op.QuiescePodUID != "game-pod-uid" || op.QuiesceContainerID != backupRunningGameStatus().ContainerID || op.QuiesceRestartCount == nil || *op.QuiesceRestartCount != 0 || !op.NeedsUnquiesce { t.Fatalf("cleanup identity not durable: %+v %v", op, err) } guardPass(t, r, "first") diff --git a/operator/internal/controller/lifecycle_operation.go b/operator/internal/controller/lifecycle_operation.go index 20aff9bbb..261c06cee 100644 --- a/operator/internal/controller/lifecycle_operation.go +++ b/operator/internal/controller/lifecycle_operation.go @@ -11,6 +11,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" gameplanev1alpha1 "github.com/GameplanePanel/gameplane/operator/api/v1alpha1" @@ -27,15 +28,17 @@ type lifecycleIdentity struct { // PreservePower allows v2 restores to stop with the guard, without altering user suspend intent. PreservePower bool `json:"preservePower,omitempty"` - StoppedBackup *stoppedBackup `json:"stoppedBackup,omitempty"` - Kind string `json:"kind"` - Name string `json:"name"` - UID types.UID `json:"uid"` - TargetUID types.UID `json:"targetUID"` - Generation int64 `json:"generation"` - Token string `json:"token,omitempty"` - NeedsUnquiesce bool `json:"needsUnquiesce,omitempty"` - QuiescePodUID types.UID `json:"quiescePodUID,omitempty"` + StoppedBackup *stoppedBackup `json:"stoppedBackup,omitempty"` + Kind string `json:"kind"` + Name string `json:"name"` + UID types.UID `json:"uid"` + TargetUID types.UID `json:"targetUID"` + Generation int64 `json:"generation"` + Token string `json:"token,omitempty"` + NeedsUnquiesce bool `json:"needsUnquiesce,omitempty"` + QuiescePodUID types.UID `json:"quiescePodUID,omitempty"` + QuiesceContainerID string `json:"quiesceContainerID,omitempty"` + QuiesceRestartCount *int32 `json:"quiesceRestartCount,omitempty"` // InitialStartup permits only the first workload of a CSI restore's new // target (including WaitForFirstConsumer PVCs), while other operations wait. InitialStartup bool `json:"initialStartup,omitempty"` @@ -323,17 +326,7 @@ func recoverLifecycleOperation(ctx context.Context, c client.Client, reader clie if !predecessor.NeedsUnquiesce { continue } - gone, err := backupQuiesceProcessGone(ctx, reader, gs.Namespace, gs.Name, predecessor.QuiescePodUID) - if err != nil { - return false, err - } - if gone { - continue - } - if !agentQuiesceAvailable(quiescer) { - return false, fmt.Errorf("cannot recover orphaned quiesce without an agent client") - } - if err := callBackupQuiesce(ctx, quiescer, gs.Namespace, gs.Name, predecessor.QuiescePodUID, true); err != nil { + if _, err := resumeBackupProcess(ctx, reader, quiescer, gs.Namespace, gs.Name, predecessor.QuiescePodUID, predecessor.QuiesceContainerID, predecessor.QuiesceRestartCount); err != nil { return false, fmt.Errorf("recover orphaned lifecycle quiesce: %w", err) } } @@ -385,23 +378,30 @@ func (r *BackupReconciler) setLifecycleUnquiesceIntent(ctx context.Context, b *g if err != nil { return err } - var podUID types.UID if needed { - preparation, err := parseBackupPreparation(b) + prep, err := parseBackupPreparation(b) if err != nil { return err } - if preparation != nil { - podUID = preparation.PodUID + if prep != nil { + if op.NeedsUnquiesce && op.QuiescePodUID != "" && op.QuiescePodUID != prep.PodUID { + return fmt.Errorf("cleanup pod identity changed") + } + if op.NeedsUnquiesce && op.QuiesceContainerID != "" && + (op.QuiesceContainerID != prep.GameContainerID || !ptr.Equal(op.QuiesceRestartCount, prep.GameRestartCount)) { + return fmt.Errorf("cleanup game incarnation changed") + } + if op.NeedsUnquiesce && op.QuiescePodUID == prep.PodUID && op.QuiesceContainerID == prep.GameContainerID && ptr.Equal(op.QuiesceRestartCount, prep.GameRestartCount) { + return nil + } + op.QuiescePodUID = prep.PodUID + op.QuiesceContainerID = prep.GameContainerID + op.QuiesceRestartCount = prep.GameRestartCount } - } - if op.NeedsUnquiesce == needed && (!needed || op.QuiescePodUID == podUID) { + } else if !op.NeedsUnquiesce { return nil } op.NeedsUnquiesce = needed - if needed { - op.QuiescePodUID = podUID - } gs.Annotations[lifecycleOperationAnnotation] = lifecycleOperationJSON(op) return r.Update(ctx, gs) } diff --git a/operator/specs.md b/operator/specs.md index ad0254e01..6dcea63c7 100644 --- a/operator/specs.md +++ b/operator/specs.md @@ -430,7 +430,7 @@ Primary reconcilers register with the manager in `cmd/main.go` and handle CRD li - Preparation defaults to 10m and live quiesce attempts to 5m (`--backup-preparation-timeout`, `--backup-quiesce-timeout`; matching Helm `operator.backupPreparationTimeout`, `operator.backupQuiesceTimeout`). Deadlines are annotations written once and survive restart/configuration changes, including when an attempted quiesce is unsupported. Copies requested raw, without an agent, or from an already-stopped source retain the full Job deadline. The waiting shell validates token expiry and bounds Restic with `timeout -s TERM -k 5`. On expiry/deletion, invalidation and token revocation precede foreground cancellation; actual live/terminating workers still block save-on and owner release. A timeout is not evidence of drainage. Existing pre-upgrade Jobs retain their observation protocol. - Initial source/fence waits share an existing preparation deadline. CSI captures and older Jobs without one persist a separate two-minute source-startup deadline after acquiring lifecycle ownership; queue age and restarts do not spend or reset that budget. Expired unavailable sources fail visibly and enter normal cleanup. A foreign same-name worker is awaited before acquiring ownership or quiescing. Existing owned workers continue through their original gate/completion checks without resolving the template again; template resolution failure when constructing a new worker aborts and releases only after drainage and save-on. - With quiesce and an agent client enabled, preparation is recorded once in `backup.gameplane.local/preparation`, bound to the target UID and StatefulSet UID/generation. `already-stopped` requires uncached proof of desired and observed zero replicas, caught-up observed generation, and no live game/PVC user pods or pending writer Jobs. Current copy workers are excluded by Job ownership. This path sends no agent calls and preserves suspend/idle/wake intent under the shared workload fence. It applies equally to restic and CSI copies. - - Live preparation also pins the game Pod UID. The operator checks the agent's read-only `/v1/pods/{uid}/identity` endpoint before recording the first cleanup intent, then uses pod-bound quiesce/unquiesce routes; the agent rejects a different UID before RCON. A missing endpoint or identity mismatch fails closed without a legacy-route fallback. Ambiguous quiesce errors retain cleanup intent and the original Pod UID on both the Backup and shared owner. Only confirmed save-on or uncached evidence that the original pod is absent/replaced/terminal permits cleanup; an HTTP timeout, unready pod, or terminating pod is insufficient. Legacy attempts without a UID retire only on absence/terminal proof and never adopt a new UID. `Unquiesced=True, Reason=SourceProcessGone` distinguishes process exit from an actual save-on RPC. + - Live preparation also pins the game Pod UID. The operator checks the agent's read-only `/v2/pods/{uid}/identity` endpoint before recording the first cleanup intent, then uses pod-bound quiesce/unquiesce routes; the agent rejects a different UID before RCON. A missing endpoint or identity mismatch fails closed without a legacy-route fallback. Ambiguous quiesce errors retain cleanup intent and the original Pod UID on both the Backup and shared owner. The v2 response must include the matching pause/resume outcome and boolean. A resume checks the pinned container before and after the RPC. The GameServer owner carries the original container ID and restart count through ownership transfer. Only a confirmed save-on or uncached evidence of original process exit (including replacement within the same Pod) permits cleanup; an HTTP timeout, unready pod, or terminating pod is insufficient. Legacy attempts without a UID retire only on absence/terminal proof and never adopt a new UID. `Unquiesced=True, Reason=SourceProcessGone` distinguishes process exit from an actual save-on RPC. - Live source proof also records the running `game` container's ContainerID and RestartCount, independently of the agent container. It is checked before and after quiesce and on each copy reconcile, including completion. A changed incarnation or missing/non-running game status after binding invalidates the capture even when the Pod UID and phase remain unchanged. Initial unavailable container evidence waits within the existing persisted startup/preparation deadline. In-progress legacy live preparations without this proof fail conservatively without backfilling an incarnation after quiesce; they remain parseable for cleanup, and completed historical manifests are unchanged. Observed workload/process replacement fails the Backup before it can report a restorable success. Owner recovery retains the pod UID even if the Backup is force-deleted, drains predecessor workers, and never sends its cleanup to a successor pod. - Imported legacy recovery records and backups without a recorded Pod UID cannot call unbound save-on, even if an agent is available. Their cleanup intent remains durable until an uncached read proves the game Pod absent or terminal and not terminating; a live or unready replacement is not exit proof. - Upgrade the operator and agent image together and allow existing GameServer pods to roll out the new agent and `GAMEPLANE_POD_UID` DownwardAPI env. Older agents fail the identity preflight before cleanup debt, so the workload can continue its upgrade after terminal cleanup releases the owner. Retry failed copies after rollout; preflight never bypasses protection. This does not automatically stop a running game. diff --git a/test/e2e/gamebot_helpers_e2e_test.go b/test/e2e/gamebot_helpers_e2e_test.go index 31f28c4a3..58aee501b 100644 --- a/test/e2e/gamebot_helpers_e2e_test.go +++ b/test/e2e/gamebot_helpers_e2e_test.go @@ -158,13 +158,14 @@ type gameBotSpec struct { RCON map[string]any // spec.rcon ConsoleMode string // spec.consoleMode Actions []any // spec.capabilities.actions; nil means no actions + Quiesce map[string]any // optional backup declaration Control pathAControl // Path A: drive the server through Gameplane (optional) } // runGameBotTest creates the GameTemplate + GameServer, waits for // status.phase == Running, then runs the in-cluster probe. It expects // the probe to reach ExpectDepth or the test fails. -func runGameBotTest(t *testing.T, s gameBotSpec) { +func runGameBotTest(t *testing.T, s gameBotSpec) (string, string) { t.Helper() // Enforcement: verify the test name suffix matches the asserted depth. @@ -253,6 +254,7 @@ func runGameBotTest(t *testing.T, s gameBotSpec) { if s.Control.Mode != "" { runGameBotPathA(t, gsName, ns, s.Control) } + return gsName, ns } // createGameBotServer creates the GameTemplate and the GameServer described by s @@ -323,10 +325,15 @@ func createGameBotServer(t *testing.T, env *Env, s gameBotSpec) (gsName, namespa if s.ConsoleMode != "" { spec["consoleMode"] = s.ConsoleMode } + capabilities := map[string]any{} if s.Actions != nil { - spec["capabilities"] = map[string]any{ - "actions": s.Actions, - } + capabilities["actions"] = s.Actions + } + if s.Quiesce != nil { + capabilities["quiesce"] = s.Quiesce + } + if len(capabilities) > 0 { + spec["capabilities"] = capabilities } tmpl := &unstructured.Unstructured{Object: map[string]any{ diff --git a/test/e2e/minecraft_bot_e2e_test.go b/test/e2e/minecraft_bot_e2e_test.go index 5d7253ec4..3ca6be3f6 100644 --- a/test/e2e/minecraft_bot_e2e_test.go +++ b/test/e2e/minecraft_bot_e2e_test.go @@ -3,6 +3,10 @@ package e2e import ( + "fmt" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "strings" "testing" "time" @@ -45,7 +49,11 @@ import ( func TestGameServer_MinecraftJavaBot_Joined(t *testing.T) { skipUnlessGameInScope(t, "minecraft-java") - runGameBotTest(t, minecraftBotSpec()) + spec := minecraftBotSpec() + spec.Env["PAUSE_WHEN_EMPTY_SECONDS"] = "-1" + spec.Quiesce = map[string]any{"quiesce": []any{"save-off", "save-all flush"}, "unquiesce": []any{"save-on"}} + gs, ns := runGameBotTest(t, spec) + verifyMinecraftBackupResumesAutosave(t, ns, gs) } // minecraftBotSpec is the trimmed vanilla Minecraft Java 1.21.4 GameServer used by @@ -139,3 +147,46 @@ func minecraftBotSpec() gameBotSpec { }, } } + +// Write a new command-storage marker after recovery. Only an automated save +// can put it on disk: this probe issues neither save-all nor a server stop. +func verifyMinecraftBackupResumesAutosave(t *testing.T, ns, gs string) { + t.Helper() + ensureResticRepo(t) + name := gs + "-autosave" + b := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "gameplane.local/v1alpha1", "kind": "Backup", + "metadata": map[string]any{"namespace": ns, "name": name}, + "spec": map[string]any{"serverRef": map[string]any{"name": gs}, "repoRef": map[string]any{"name": "e2e-restic-creds", "key": "repo"}, "quiesce": true}, + }} + backups := envInstance.Dyn.Resource(backupGVR).Namespace(ns) + if _, err := backups.Create(t.Context(), b, metav1.CreateOptions{}); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { deleteBackupCR(t, ns, name) }) + waitBackupSucceeded(t, ns, name, 8*time.Minute) + envInstance.Eventually(t, time.Minute, func() (bool, string) { + got, err := backups.Get(t.Context(), name, metav1.GetOptions{}) + if err != nil { + return false, err.Error() + } + a := got.GetAnnotations() + return a["backup.gameplane.local/quiesce-attempted"] == "true" && + a["backup.gameplane.local/unquiesced-at"] != "" && a["backup.gameplane.local/cleanup-outcome"] != "source-gone", "waiting for pause and resume of the live game" + }) + exec := func(args ...string) (string, error) { + return envInstance.Kubectl(t.Context(), append([]string{"exec", "-n", ns, "pod/" + gs + "-0", "-c", "game", "--"}, args...)...) + } + // An old command storage file cannot satisfy a new marker. + marker := fmt.Sprintf("gameplane_autosave_%d", time.Now().UnixNano()) + if out, err := exec("rcon-cli", "data modify storage gpaudit:backup marker set value "+marker); err != nil || !strings.Contains(out, "Modified storage") { + t.Fatalf("set post-backup marker: %s (%v)", out, err) + } + if out, err := exec("rcon-cli", "tick sprint 7000t"); err != nil { + t.Fatalf("advance vanilla autosave ticks: %s (%v)", out, err) + } + envInstance.Eventually(t, 7*time.Minute, func() (bool, string) { + out, err := exec("gzip", "-dc", "/data/world/data/command_storage_gpaudit.dat") + return err == nil && strings.Contains(out, marker), "waiting for automatic persistence of the post-backup marker" + }) +} From 9a4c316920aef2a53d12aa150ff482b09e90a535 Mon Sep 17 00:00:00 2001 From: Baylem Date: Tue, 10 Nov 2026 20:59:25 -0500 Subject: [PATCH 2/2] test: cover backup cleanup handoff across operator restart Signed-off-by: Baylem --- agent/internal/quiesce/verified_test.go | 28 ++++++++ agent/internal/rcon/commands.go | 2 + docs/operator-operations.md | 6 ++ .../internal/controller/backup_guard_test.go | 67 +++++++++++-------- test/e2e/minecraft_bot_e2e_test.go | 5 +- 5 files changed, 78 insertions(+), 30 deletions(-) diff --git a/agent/internal/quiesce/verified_test.go b/agent/internal/quiesce/verified_test.go index 6bd8d0765..411ff8fe4 100644 --- a/agent/internal/quiesce/verified_test.go +++ b/agent/internal/quiesce/verified_test.go @@ -107,3 +107,31 @@ func TestVerifiedRejectsUnqualifiedDeclarations(t *testing.T) { } } } + +func TestVerifiedRejectedResumeCannotAcknowledgeAutosave(t *testing.T) { + rc := &fakeRcon{respond: func(cmd string) (string, error) { + switch cmd { + case "save-off": + return "Saving is already turned off", nil + case "save-all flush": + return "Saved the game", nil + } + return "Unknown or incomplete command", nil + }} + router := chi.NewRouter() + MountVerified(router, rcon.NewCommands(rc), "minecraft-java", minecraftQuiesceSpec()) + for _, tc := range []struct { + path string + status int + outcome string + }{ + {"/quiesce", http.StatusOK, "paused"}, {"/quiesce", http.StatusOK, "paused"}, + {"/unquiesce", http.StatusBadGateway, ""}, + } { + w := httptest.NewRecorder() + router.ServeHTTP(w, httptest.NewRequestWithContext(t.Context(), http.MethodPost, tc.path, nil)) + if got := decodeResponse(t, w.Body.Bytes()); w.Code != tc.status || got.Outcome != tc.outcome || !got.Quiesced { + t.Fatalf("%s: %d %+v", tc.path, w.Code, got) + } + } +} diff --git a/agent/internal/rcon/commands.go b/agent/internal/rcon/commands.go index 7b5babc05..14d1be824 100644 --- a/agent/internal/rcon/commands.go +++ b/agent/internal/rcon/commands.go @@ -13,10 +13,12 @@ type Commands struct { busy chan struct{} } +// NewCommands shares one admission queue across clients of a transport. func NewCommands(transport Executor) *Commands { return &Commands{transport: transport, busy: make(chan struct{}, 1)} } +// Exec admits one command when no sequence is in progress. func (c *Commands) Exec(command string) (string, error) { c.busy <- struct{}{} defer func() { <-c.busy }() diff --git a/docs/operator-operations.md b/docs/operator-operations.md index 9a123bdc1..9cb3f17cf 100644 --- a/docs/operator-operations.md +++ b/docs/operator-operations.md @@ -129,6 +129,12 @@ or agent connectivity before retrying. Removing owner annotations or force-remov finalizers cannot establish that a worker has stopped. A failed or canceled restore leaves its target suspended for inspection. +Before upgrading from agents with only the v1 backup endpoint, let active live +backups finish and check their `Unquiesced` condition. The new operator checks +for v2 support before starting a new pause. If an upgrade interrupts an existing +pause, the operator retains the owner until it receives a verified resume or +observes the original process exit. An older agent cannot supply that acknowledgment. + Upgrades recognize existing owner annotations and import backup recovery lineage. If that legacy record lacks the original Pod UID, cleanup cannot safely address save-on to a running Pod. Its owner remains held until an uncached read proves diff --git a/operator/internal/controller/backup_guard_test.go b/operator/internal/controller/backup_guard_test.go index e1edd39f8..25baa5ec6 100644 --- a/operator/internal/controller/backup_guard_test.go +++ b/operator/internal/controller/backup_guard_test.go @@ -316,34 +316,45 @@ func TestBackupGuardRolloutWaitsForLegacyWorkersAndCleanup(t *testing.T) { } func TestBackupGuardOrphanRecoveryWaitsForWorkers(t *testing.T) { - r, q := guardFixture(t, guardBackup("first"), guardBackup("second")) - guardPass(t, r, "first") - b := guardGetBackup(t, r, "first") - b.Finalizers = nil - if err := r.Update(context.Background(), b); err != nil { - t.Fatal(err) - } - if err := r.Delete(context.Background(), b); err != nil { - t.Fatal(err) - } - var job batchv1.Job - if err := r.Get(context.Background(), types.NamespacedName{Namespace: "ns", Name: "first"}, &job); err != nil { - t.Fatal(err) - } - pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "orphan", Namespace: "ns", Labels: job.Spec.Template.Labels}, Status: corev1.PodStatus{Phase: corev1.PodRunning}} - if err := r.Create(context.Background(), pod); err != nil { - t.Fatal(err) - } - guardPass(t, r, "second") - if q.quiesced != 1 || q.unquiesced != 0 { - t.Fatalf("reclaimed before orphan drain: %+v", q) - } - if err := r.Delete(context.Background(), pod); err != nil { - t.Fatal(err) - } - guardPass(t, r, "second") - if q.quiesced != 2 || q.unquiesced != 1 { - t.Fatalf("orphan owner not recovered: %+v", q) + for _, replaced := range []bool{false, true} { + t.Run(fmt.Sprintf("game-replaced-%t", replaced), func(t *testing.T) { + r, q := guardFixture(t, guardBackup("first"), guardBackup("second")) + guardPass(t, r, "first") + b := guardGetBackup(t, r, "first") + b.Finalizers = nil + if err := r.Update(context.Background(), b); err != nil { + t.Fatal(err) + } + if err := r.Delete(context.Background(), b); err != nil { + t.Fatal(err) + } + var job batchv1.Job + if err := r.Get(context.Background(), types.NamespacedName{Namespace: "ns", Name: "first"}, &job); err != nil { + t.Fatal(err) + } + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "orphan", Namespace: "ns", Labels: job.Spec.Template.Labels}, Status: corev1.PodStatus{Phase: corev1.PodRunning}} + if err := r.Create(context.Background(), pod); err != nil { + t.Fatal(err) + } + guardPass(t, r, "second") + if q.quiesced != 1 || q.unquiesced != 0 { + t.Fatalf("reclaimed before orphan drain: %+v", q) + } + if err := r.Delete(context.Background(), pod); err != nil { + t.Fatal(err) + } + wantResume := 1 + if replaced { + changeBackupGamePod(t, r, backupGameChanges()["container-id"]) + wantResume = 0 + } + // Recovery must use the persisted predecessor identity after restart. + r = &BackupReconciler{Client: r.Client, Scheme: r.Scheme, AgentClient: q, LogReader: r.LogReader} + guardPass(t, r, "second") + if q.quiesced != 2 || q.unquiesced != wantResume { + t.Fatalf("orphan owner not recovered: %+v", q) + } + }) } } diff --git a/test/e2e/minecraft_bot_e2e_test.go b/test/e2e/minecraft_bot_e2e_test.go index 3ca6be3f6..9689e7a3b 100644 --- a/test/e2e/minecraft_bot_e2e_test.go +++ b/test/e2e/minecraft_bot_e2e_test.go @@ -4,12 +4,13 @@ package e2e import ( "fmt" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "strings" "testing" "time" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "github.com/GameplanePanel/gameplane/test/e2e/internal/protocol/joindepth" )