From 9e2c2ea62de2f5a417e989f98fcdbcc4ba993800 Mon Sep 17 00:00:00 2001 From: Baylem Date: Wed, 21 Oct 2026 12:35:20 -0400 Subject: [PATCH 01/12] fix: prepare backup workers before quiesce and bound CSI captures Signed-off-by: Baylem --- .github/workflows/ci.yaml | 12 +- charts/gameplane/templates/operator.yaml | 17 + charts/gameplane/values.yaml | 7 + docs/operator-operations.md | 61 ++- i18n/locales/en/app.json | 32 ++ operator/cmd/main.go | 16 + operator/config/rbac/role.yaml | 8 + operator/hack/condition-ids/main.go | 10 + .../internal/controller/backup_budgets.go | 122 +++++ .../controller/backup_budgets_test.go | 490 ++++++++++++++++++ .../internal/controller/backup_controller.go | 48 +- .../controller/backup_envtest_test.go | 23 +- operator/internal/controller/backup_forget.go | 15 + operator/internal/controller/backup_guard.go | 56 +- .../internal/controller/backup_guard_test.go | 61 ++- .../internal/controller/backup_manifest.go | 27 +- .../controller/backup_manifest_test.go | 40 ++ .../controller/backup_snapshot_cleanup.go | 215 ++++++++ .../controller/backup_snapshot_source.go | 49 ++ .../controller/backup_snapshot_source_test.go | 69 +++ .../controller/backup_stopped_test.go | 8 +- .../controller/backup_volumesnapshot.go | 75 ++- .../backup_volumesnapshot_envtest_test.go | 6 + operator/internal/controller/backup_worker.go | 330 ++++++++++++ .../controller/gameserver_controller.go | 7 + .../controller/helpers_envtest_test.go | 47 ++ .../internal/controller/restore_controller.go | 9 +- operator/specs.md | 12 +- test/e2e/backup_preparation_e2e_test.go | 158 ++++++ test/e2e/buckets.sh | 8 +- test/e2e/specs.md | 11 + 31 files changed, 1963 insertions(+), 86 deletions(-) create mode 100644 operator/internal/controller/backup_budgets.go create mode 100644 operator/internal/controller/backup_budgets_test.go create mode 100644 operator/internal/controller/backup_snapshot_cleanup.go create mode 100644 operator/internal/controller/backup_snapshot_source.go create mode 100644 operator/internal/controller/backup_snapshot_source_test.go create mode 100644 operator/internal/controller/backup_worker.go create mode 100644 test/e2e/backup_preparation_e2e_test.go diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index a902708f0..d2c52e592 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -1666,7 +1666,7 @@ jobs: # (operator does zero logins and is wait-dominated, so it runs # wide; the api buckets stay within the login limiter's budget). # The arm64 job below uses the same steps and per-bucket settings. - bucket: [operator, api-auth, api-roles, api-rbac, api-agent, api-mods, telemetry, optional-components, management-placement] + bucket: [operator, api-auth, api-roles, api-rbac, api-agent, api-mods, telemetry, optional-components, management-placement, backup-preparation] arch: [amd64] include: - bucket: operator @@ -1677,6 +1677,10 @@ jobs: parallel: 1 test_timeout: 15m job_timeout: 60 + - bucket: backup-preparation + parallel: 1 + test_timeout: 15m + job_timeout: 60 - bucket: api-auth parallel: 4 test_timeout: 15m @@ -1725,7 +1729,7 @@ jobs: arch: ${{ matrix.arch }} - name: bring up cluster + helm install env: - GAMEPLANE_E2E_PROFILE: ${{ matrix.bucket == 'management-placement' && 'management' || 'default' }} + GAMEPLANE_E2E_PROFILE: ${{ (matrix.bucket == 'management-placement' || matrix.bucket == 'backup-preparation') && 'management' || 'default' }} run: ./deploy/kind/e2e.sh up "$CLUSTER" e2e "$GAMEPLANE_E2E_PROFILE" - name: run go e2e (${{ matrix.bucket }}) run: | @@ -2518,7 +2522,7 @@ jobs: 'go-e2e-unit': (n) => n === 'go (test/e2e)', 'api-postgres': (n) => n === 'api (postgres)', 'e2e-buckets': (n) => n === 'e2e bucket coverage', - 'e2e-go': (n) => /^e2e (operator|api-auth|api-roles|api-rbac|api-agent|api-mods|telemetry|optional-components) \//.test(n) && n.includes('/ amd64 '), + 'e2e-go': (n) => /^e2e (operator|api-auth|api-roles|api-rbac|api-agent|api-mods|telemetry|optional-components|management-placement|backup-preparation) \//.test(n) && n.includes('/ amd64 '), 'e2e-go-arm64': (n) => /^e2e (operator|api-auth|api-roles|api-rbac|api-agent|api-mods|telemetry|optional-components) \//.test(n) && n.includes('/ arm64 '), 'e2e-multicluster': (n) => n.startsWith('e2e multicluster /') && n.includes('/ amd64 '), 'e2e-multicluster-arm64': (n) => n.startsWith('e2e multicluster /') && n.includes('/ arm64 '), @@ -2671,7 +2675,7 @@ jobs: const bucketSet = new Set(); for (const j of apiJobs) { const n = j.name || ''; - const m = n.match(/^e2e (operator|api-auth|api-roles|api-rbac|api-agent|api-mods|telemetry|optional-components) \//); + const m = n.match(/^e2e (operator|api-auth|api-roles|api-rbac|api-agent|api-mods|telemetry|optional-components|management-placement|backup-preparation) \//); if (m) bucketSet.add(m[1]); if (/^e2e multicluster \//.test(n)) bucketSet.add('multicluster'); if (/^e2e upgrade \//.test(n)) bucketSet.add('upgrade'); diff --git a/charts/gameplane/templates/operator.yaml b/charts/gameplane/templates/operator.yaml index 444a60936..a5fb29951 100644 --- a/charts/gameplane/templates/operator.yaml +++ b/charts/gameplane/templates/operator.yaml @@ -82,6 +82,11 @@ rules: - apiGroups: ["snapshot.storage.k8s.io"] resources: [volumesnapshots, volumesnapshotcontents] verbs: [get, list, watch] + # Abort reclamation is restricted in the controller to recorded snapshot + # references and exact content UIDs; VolumeSnapshotContent is cluster-scoped. + - apiGroups: [snapshot.storage.k8s.io] + resources: [volumesnapshotcontents] + verbs: [delete] # The GameServer controller registers Owns(&networkingv1.NetworkPolicy{}) # unconditionally (regardless of networkPolicies.enabled), and the # manager's cache is cluster-scoped (no Cache/DefaultNamespaces option @@ -291,6 +296,18 @@ spec: {{- if .Values.operator.resticImage }} - --restic-image={{ .Values.operator.resticImage }} {{- end }} + {{- with .Values.operator.backupPreparationTimeout }} + - --backup-preparation-timeout={{ . }} + {{- end }} + {{- with .Values.operator.backupQuiesceTimeout }} + - --backup-quiesce-timeout={{ . }} + {{- end }} + {{- with .Values.operator.backupCSICaptureTimeout }} + - --backup-csi-capture-timeout={{ . }} + {{- end }} + {{- if and (hasKey .Values.operator "backupHardSameNode") (ne .Values.operator.backupHardSameNode nil) }} + - --backup-hard-same-node={{ .Values.operator.backupHardSameNode }} + {{- end }} {{- /* Omit the flags when the keys are absent/null (e.g. --reuse-values from a release predating them) so the operator's defaults (2 / 86400) apply. Not `| default`: an explicit 0 backoff is diff --git a/charts/gameplane/values.yaml b/charts/gameplane/values.yaml index 9346f8e00..94915da4a 100644 --- a/charts/gameplane/values.yaml +++ b/charts/gameplane/values.yaml @@ -122,6 +122,13 @@ operator: # pod cannot hold a Backup/Restore in flight forever. Defaults: 2 and 86400 (24h). backupJobBackoffLimit: 2 backupJobActiveDeadlineSeconds: 86400 + # Preparation (scheduling, image pull, PVC mount, repository/lock probe) + # precedes save-off. Quiesce expiry cancels and drains the copy before save-on. + backupPreparationTimeout: 10m + backupQuiesceTimeout: 5m + backupCSICaptureTimeout: 10m + # Prefer the observed game's node by default; opt in to a hard requirement. + backupHardSameNode: false # Mount a directory of module bundles into the operator so local-type # ModuleSources can index it (homelab: drop module dirs on disk and # they appear in the catalog). Provide exactly one of existingClaim diff --git a/docs/operator-operations.md b/docs/operator-operations.md index 88366705f..a93e11ed7 100644 --- a/docs/operator-operations.md +++ b/docs/operator-operations.md @@ -53,8 +53,9 @@ deletion; live and terminating worker pods must drain before the owner is releas Quiesced backups also owe a successful save-on call. The existing bounded Backup deletion retry may release its finalizer, but an unconfirmed save-on leaves the durable owner behind for recovery by the next operation. Time alone never releases -ownership. A pending CSI capture retains the current capture-drain behavior; a -separate CSI timeout change is tracked in issue #719. +ownership. CSI cancellation or expiry first invalidates the capture, then allows +save-on independently of driver reclamation. A late-ready aborted capture remains +unrestorable. If an operation waits unexpectedly, inspect the referenced Backup/Restore, its conditions, Jobs and worker pods. Resolve terminating workers, repository failures @@ -67,13 +68,61 @@ Already-running legacy quiesced copies continue being observed until their worke finish and cleanup succeeds. Legacy raw workers also block new admission. Conflicting legacy guards fail closed until their existing operations finish cleanup. Keep a single active operator version during upgrade; older binaries do not honor the -shared owner. No new CRD fields or Kubernetes permissions are required. +shared owner. CSI abort reclamation adds cluster-scoped delete permission for +VolumeSnapshotContents; runtime checks restrict it to recorded, UID-verified +contents with `Delete` policy. CSI restores create a new target and reserve it for the Restore. Initial startup is allowed under that reservation so storage using WaitForFirstConsumer can bind. Other operations wait until the restore completes. Existing data-PVC restores continue to require an observed stopped workload and drained writers. -Stopped-source preparation (#716), Restic preparation and quiesce deadlines (#717), -CSI capture deadlines (#719), and staged template adoption (#731) are separate -changes building on this ownership boundary. +Stopped-source proof, bounded copy preparation and CSI invalidation build on this +ownership boundary. Staged template adoption is tracked separately in #731. + +## Backup preparation and capture budgets + +Restic workers now schedule, pull their image, mount the source and validate the +repository before the agent is asked to disable saving. A Ready worker waits for +an expiring authorization bound to its pod UID, Backup UID and observed source. +The preferred node is the game's current node; `operator.backupHardSameNode: true` +makes that placement mandatory. Source tolerations and `operator.backupResources` +apply to preparation and copy containers. Stopped sources leave node selection +to the scheduler and volume affinity. + +The install settings are `operator.backupPreparationTimeout: 10m`, +`operator.backupQuiesceTimeout: 5m`, and `operator.backupCSICaptureTimeout: 10m`. +Equivalent operator flags use kebab-case names. The recorded deadlines remain +fixed across restarts or later configuration edits. Restic's existing Job +backoff/deadline settings remain separate; raw and already-stopped copies use +the full Job deadline. A live quiesce attempt retains the short budget even if +the agent reports the command unsupported. Increase the live quiesce budget +deliberately for worlds that cannot finish within five minutes, or use a +supported CSI capture strategy. + +Preparation failure leaves saving enabled. Quiesce expiry marks the copy failed, +revokes authorization and cancels its Job. Saving is restored only after all +live or terminating workers have stopped. The worker itself also stops Restic +at the token deadline (TERM, then KILL after five seconds), including when the +operator is unavailable. This bounds copying; save-on still requires the operator +and agent to recover. `CaptureReady` describes preparation/authorization or a +pending drain. Resolve stuck mounts, node failures or terminating workers before +retrying; removing owner annotations is not proof that copying stopped. + +Custom Restic images must provide the restic CLI (including `snapshots --retry-lock`), +POSIX `/bin/sh`, `date +%s`, `cat`, `touch`, `sleep`, and `timeout -s TERM -k 5`. +The pinned `restic/restic:0.17.1` release uses Alpine/BusyBox and supports this +contract. The gate runs under the existing non-root, read-only-root-filesystem +security context. Node/operator clocks must be synchronized for absolute token +expiry. No worker receives Kubernetes API credentials. + +CSI expiry and Backup deletion invalidate an incomplete capture before save-on. +The `snapshot-aborted` marker is terminal even if a delayed driver later reports +readiness. Normal restore and new raw GameServer PVC references reject such +captures. `SnapshotCleaned` reports reclamation progress and records snapshot +and content names/UIDs; `Retain` content and unverified replacements require +administrator action. A slow driver can keep Backup deletion pending for storage +cleanup after saving and game ownership have already been restored. Inspect the +condition/event identities before deleting retained storage manually. Existing +PVCs remain usable after source Backup retention; administrator-managed external +snapshots keep their direct-CRD behavior. diff --git a/i18n/locales/en/app.json b/i18n/locales/en/app.json index 466a55d4a..317ebce27 100644 --- a/i18n/locales/en/app.json +++ b/i18n/locales/en/app.json @@ -155,6 +155,22 @@ "defaultMessage": "Requested {requestKind, select, addressAndPool {address ''{address}'' from pool ''{pool}''} address {address ''{address}''} other {pool ''{pool}''}}: waiting for the LoadBalancer Service that carries the request to exist.", "description": "AddressAssignment condition before the load-balancer Service exists." }, + "condition.captureready.snapshotcapturepending": { + "defaultMessage": "Waiting for CSI capture within its persisted deadline.", + "description": "Bounded CSI capture is pending." + }, + "condition.captureready.workerauthorized": { + "defaultMessage": "The prepared worker is authorized to capture data within its deadline.", + "description": "Backup copy authorization granted after preparation." + }, + "condition.captureready.workerdrainpending": { + "defaultMessage": "The copy is invalid. Waiting for workers to stop before restoring saving.", + "description": "Backup cancellation awaiting worker drainage." + }, + "condition.captureready.workerpreparing": { + "defaultMessage": "Waiting for worker scheduling, source mount and repository preparation.", + "description": "Backup preparation before saving is disabled." + }, "condition.completed.failed": { "defaultMessage": "The backup or restore failed.", "description": "Completed condition on a Backup or Restore that failed. The message holds the English detail." @@ -579,6 +595,22 @@ "defaultMessage": "The capture sidecar was told to stop.", "description": "SidecarStopped condition after a stop request." }, + "condition.snapshotcleaned.snapshotcleanupcomplete": { + "defaultMessage": "Aborted snapshot resources are absent.", + "description": "Snapshot reclamation completed." + }, + "condition.snapshotcleaned.snapshotcleanuppending": { + "defaultMessage": "Aborted snapshot reclamation is pending. {detail}", + "description": "Aborted snapshot deletion has not completed; detail includes resource identities." + }, + "condition.snapshotcleaned.snapshotcleanupretained": { + "defaultMessage": "Snapshot content requires administrator reclamation. {detail}", + "description": "Retained or replacement snapshot content is left for an administrator." + }, + "condition.snapshotcleaned.snapshotcleanupunverified": { + "defaultMessage": "A snapshot resource was replaced. It was not deleted. {detail}", + "description": "Snapshot identity verification refused deletion." + }, "condition.snapshotforgotten.forgetabandoned": { "defaultMessage": "The snapshot was not removed from the backup repository when the backup was deleted.", "description": "SnapshotForgotten condition when the forget step was abandoned." diff --git a/operator/cmd/main.go b/operator/cmd/main.go index 933925528..8d4afb1a6 100644 --- a/operator/cmd/main.go +++ b/operator/cmd/main.go @@ -150,6 +150,10 @@ func main() { captureDefaultMaxSizeBytes int64 backupJobBackoffLimit int64 backupJobActiveDeadlineSeconds int64 + backupPreparationTimeout time.Duration + backupQuiesceTimeout time.Duration + backupCSICaptureTimeout time.Duration + backupHardSameNode bool agentResourcesJSON string sentinelResourcesJSON string tunnelResourcesJSON string @@ -181,6 +185,10 @@ func main() { "BackoffLimit for the restic backup and restore Jobs (retries before the Job is marked Failed). Must be >= 0. Defaults to 2.") flag.Int64Var(&backupJobActiveDeadlineSeconds, "backup-job-active-deadline-seconds", controller.DefaultBackupJobActiveDeadlineSeconds, "ActiveDeadlineSeconds for the restic backup and restore Jobs. Must be > 0. Defaults to 86400 (24 hours).") + flag.DurationVar(&backupPreparationTimeout, "backup-preparation-timeout", controller.DefaultBackupPreparationTimeout, "Maximum worker preparation time before copy authorization; must be positive.") + flag.DurationVar(&backupQuiesceTimeout, "backup-quiesce-timeout", controller.DefaultBackupQuiesceTimeout, "Maximum live quiesced Restic copy duration; expiry cancels and drains workers before save-on.") + flag.DurationVar(&backupCSICaptureTimeout, "backup-csi-capture-timeout", controller.DefaultBackupCSICaptureTimeout, "Maximum CSI capture duration before invalidation and save-on; must be positive.") + flag.BoolVar(&backupHardSameNode, "backup-hard-same-node", false, "Require Restic workers on the observed game node instead of the default preferred placement.") flag.StringVar(&sentinelImage, "sentinel-image", controller.DefaultSentinelImage, "Image for the wake sentinel pod that holds advertised ports while a server is asleep. "+ "Point at a private registry mirror for air-gapped installs.") @@ -300,6 +308,10 @@ func main() { setupLog.Error(nil, "invalid --backup-job-active-deadline-seconds value (must be > 0)", "value", backupJobActiveDeadlineSeconds) os.Exit(1) } + if backupPreparationTimeout <= 0 || backupQuiesceTimeout <= 0 || backupCSICaptureTimeout <= 0 { + setupLog.Error(nil, "backup preparation, quiesce and CSI capture timeouts must be positive") + os.Exit(1) + } // Validate capture retention flags while they are still the int64 type // flag.Int64Var naturally produces. The minimum of 60 seconds aligns with @@ -455,6 +467,10 @@ func main() { JobBackoffLimit: &backupJobBackoff32, JobActiveDeadlineSeconds: backupJobActiveDeadlineSeconds, EventRecorder: mgr.GetEventRecorder("backup-controller"), + PreparationTimeout: backupPreparationTimeout, + QuiesceTimeout: backupQuiesceTimeout, + CSICaptureTimeout: backupCSICaptureTimeout, + HardSameNode: backupHardSameNode, }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to set up controller", "controller", "Backup") os.Exit(1) diff --git a/operator/config/rbac/role.yaml b/operator/config/rbac/role.yaml index e9d3d9337..3bef4d7a1 100644 --- a/operator/config/rbac/role.yaml +++ b/operator/config/rbac/role.yaml @@ -152,6 +152,14 @@ rules: - snapshot.storage.k8s.io resources: - volumesnapshotcontents + verbs: + - delete + - get + - list + - watch +- apiGroups: + - snapshot.storage.k8s.io + resources: - volumesnapshots verbs: - get diff --git a/operator/hack/condition-ids/main.go b/operator/hack/condition-ids/main.go index b361725db..1bf749157 100644 --- a/operator/hack/condition-ids/main.go +++ b/operator/hack/condition-ids/main.go @@ -51,6 +51,8 @@ var guardCalls = map[string]int{ "markPending": 2, "failWithReason": 2, "setSnapshotForgottenCondition": 3, + "setBackupProgress": 2, + "snapshotCleanupCondition": 3, } // guardReturnFuncs are the functions whose first string result is a reason. @@ -67,6 +69,14 @@ var guardSkipFiles = map[string]bool{ // rows is the English reference for every condition reason. var rows = []row{ + {types: []string{"CaptureReady"}, reason: "WorkerPreparing", english: "Waiting for worker scheduling, source mount and repository preparation.", description: "Backup preparation before saving is disabled."}, + {types: []string{"CaptureReady"}, reason: "WorkerAuthorized", english: "The prepared worker is authorized to capture data within its deadline.", description: "Backup copy authorization granted after preparation."}, + {types: []string{"CaptureReady"}, reason: "WorkerDrainPending", english: "The copy is invalid. Waiting for workers to stop before restoring saving.", description: "Backup cancellation awaiting worker drainage."}, + {types: []string{"CaptureReady"}, reason: "SnapshotCapturePending", english: "Waiting for CSI capture within its persisted deadline.", description: "Bounded CSI capture is pending."}, + {types: []string{"SnapshotCleaned"}, reason: "SnapshotCleanupPending", english: "Aborted snapshot reclamation is pending. {detail}", description: "Aborted snapshot deletion has not completed; detail includes resource identities."}, + {types: []string{"SnapshotCleaned"}, reason: "SnapshotCleanupUnverified", english: "A snapshot resource was replaced. It was not deleted. {detail}", description: "Snapshot identity verification refused deletion."}, + {types: []string{"SnapshotCleaned"}, reason: "SnapshotCleanupRetained", english: "Snapshot content requires administrator reclamation. {detail}", description: "Retained or replacement snapshot content is left for an administrator."}, + {types: []string{"SnapshotCleaned"}, reason: "SnapshotCleanupComplete", english: "Aborted snapshot resources are absent.", description: "Snapshot reclamation completed."}, // GameServer: Ready {types: []string{"Ready"}, reason: "Running", english: "The server is running and the agent is reporting heartbeats.", description: "Ready condition while the game server is up."}, {types: []string{"Ready"}, reason: "Pending", english: "The server is waiting to start.", description: "Ready condition while the game server is being created."}, diff --git a/operator/internal/controller/backup_budgets.go b/operator/internal/controller/backup_budgets.go new file mode 100644 index 000000000..ca0acf1d2 --- /dev/null +++ b/operator/internal/controller/backup_budgets.go @@ -0,0 +1,122 @@ +package controller + +import ( + "context" + "fmt" + "time" + + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + + gameplanev1alpha1 "github.com/GameplanePanel/gameplane/operator/api/v1alpha1" +) + +const ( + DefaultBackupPreparationTimeout = 10 * time.Minute + DefaultBackupQuiesceTimeout = 5 * time.Minute + DefaultBackupCSICaptureTimeout = 10 * time.Minute + backupPreparationDeadlineAnnotation = "backup.gameplane.local/preparation-deadline" + backupQuiesceDeadlineAnnotation = "backup.gameplane.local/quiesce-deadline" + backupCSIDeadlineAnnotation = "backup.gameplane.local/csi-deadline" + backupCopyAbortAnnotation = "backup.gameplane.local/copy-aborted" +) + +func backupTimeout(configured, fallback time.Duration) time.Duration { + if configured > 0 { + return configured + } + return fallback +} + +// Write once before the bounded action. Reconfiguration and restarts cannot +// extend an admitted attempt. Malformed stored state fails closed. +func (r *BackupReconciler) backupDeadline(ctx context.Context, b *gameplanev1alpha1.Backup, key string, duration time.Duration) (time.Time, error) { + if value := b.Annotations[key]; value != "" { + deadline, err := time.Parse(time.RFC3339Nano, value) + if err != nil { + return time.Time{}, fmt.Errorf("invalid persisted backup deadline %s: %w", key, err) + } + return deadline, nil + } + deadline := time.Now().UTC().Add(duration) + patchBackupAnnotations(b, map[string]string{key: deadline.Format(time.RFC3339Nano)}) + return deadline, r.Update(ctx, b) +} + +func (r *BackupReconciler) setBackupProgress(ctx context.Context, b *gameplanev1alpha1.Backup, reason, message string) error { + status := metav1.ConditionFalse + if reason == "WorkerAuthorized" { + status = metav1.ConditionTrue + } + conditions := upsertCondition(b.Status.Conditions, metav1.Condition{Type: "CaptureReady", Status: status, + Reason: reason, Message: message, ObservedGeneration: b.Generation}) + if sameConditions(b.Status.Conditions, conditions) { + return nil + } + b.Status.Conditions = conditions + return r.Status().Update(ctx, b) +} + +// Persist invalidation before revoking authorization or cancelling workers. +// A stale mounted token can still be observed until projection catches up, so +// neither token revocation nor a deadline substitutes for actual worker drain. +func (r *BackupReconciler) abortResticCopy(ctx context.Context, b *gameplanev1alpha1.Backup, message string) (ctrl.Result, error) { + if message == "" { + message = "backup copy invalidated" + } + if b.Annotations[backupCopyAbortAnnotation] == "" { + patchBackupAnnotations(b, map[string]string{backupCopyAbortAnnotation: message}) + if err := r.Update(ctx, b); err != nil { + return ctrl.Result{}, err + } + } + message = b.Annotations[backupCopyAbortAnnotation] + if b.Status.Phase != gameplanev1alpha1.BackupPhaseFailed || b.Status.SnapshotID != "" || b.Status.Message != message { + b.Status.SnapshotID = "" + if _, err := r.fail(ctx, b, message); err != nil { + return ctrl.Result{}, err + } + } + return r.cleanupResticCopy(ctx, b) +} + +func (r *BackupReconciler) cleanupResticCopy(ctx context.Context, b *gameplanev1alpha1.Backup) (ctrl.Result, error) { + if err := r.revokeBackupStart(ctx, b); err != nil { + return ctrl.Result{}, err + } + busy, err := r.backupCleanupWorkersLive(ctx, b, true) + if err != nil { + return ctrl.Result{}, err + } + if busy { + if err := r.setBackupProgress(ctx, b, "WorkerDrainPending", "copy invalidated; waiting for workers to stop before restoring saving"); err != nil { + return ctrl.Result{}, err + } + return ctrl.Result{RequeueAfter: 5 * time.Second}, nil + } + if meta.RemoveStatusCondition(&b.Status.Conditions, "CaptureReady") { + if err := r.Status().Update(ctx, b); err != nil { + return ctrl.Result{}, err + } + } + return r.runUnquiesce(ctx, b) +} + +func (r *BackupReconciler) revokeBackupStart(ctx context.Context, b *gameplanev1alpha1.Backup) error { + var gate corev1.ConfigMap + err := r.backupAPIReader().Get(ctx, client.ObjectKey{Namespace: b.Namespace, Name: backupStartConfigMapName(b)}, &gate) + if err != nil && !apierrors.IsNotFound(err) { + return err + } + if err == nil && metav1.IsControlledBy(&gate, b) && gate.Data[backupStartTokenKey] != "" { + gate.Data[backupStartTokenKey] = "" + if err := r.Update(ctx, &gate); err != nil { + return err + } + } + return nil +} diff --git a/operator/internal/controller/backup_budgets_test.go b/operator/internal/controller/backup_budgets_test.go new file mode 100644 index 000000000..bedf6c357 --- /dev/null +++ b/operator/internal/controller/backup_budgets_test.go @@ -0,0 +1,490 @@ +package controller + +import ( + "context" + "fmt" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" + "time" + + snapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1" + batchv1 "k8s.io/api/batch/v1" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + gameplanev1alpha1 "github.com/GameplanePanel/gameplane/operator/api/v1alpha1" +) + +func budgetPass(t *testing.T, r *BackupReconciler, name string) { + t.Helper() + for range 4 { + if _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: types.NamespacedName{Namespace: "ns", Name: name}}); err != nil { + t.Fatal(err) + } + } +} + +// Model the Job controller and kubelet explicitly: no worker may be authorized +// merely because the Job exists or an informer has an old Ready observation. +func budgetReadyWorker(t *testing.T, r *BackupReconciler, name, uid string) *corev1.Pod { + t.Helper() + ctx := context.Background() + var job batchv1.Job + if err := r.Get(ctx, client.ObjectKey{Namespace: "ns", Name: name}, &job); err != nil { + t.Fatal(err) + } + if job.UID == "" { + job.UID = types.UID(name + "-job-uid") + if err := r.Update(ctx, &job); err != nil { + t.Fatal(err) + } + } + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: name + "-worker", Namespace: "ns", UID: types.UID(uid), Labels: job.Spec.Template.Labels, + OwnerReferences: []metav1.OwnerReference{{APIVersion: "batch/v1", Kind: "Job", Name: job.Name, UID: job.UID, Controller: ownerBoolPtr(true)}}}, + Spec: job.Spec.Template.Spec, Status: corev1.PodStatus{Phase: corev1.PodRunning, Conditions: []corev1.PodCondition{{Type: corev1.PodReady, Status: corev1.ConditionTrue}}}} + pod.Spec.NodeName = "node-a" + if err := r.Create(ctx, pod); err != nil { + t.Fatal(err) + } + return pod +} + +func TestBackupWorkerPreparesBeforeQuiesceAndPinsAuthorization(t *testing.T) { + r, q := guardFixtureWorkers(t, false, guardBackup("prepared")) + budgetPass(t, r, "prepared") + if q.quiesced != 0 { + t.Fatal("save-off preceded worker readiness") + } + b := guardGetBackup(t, r, "prepared") + if b.Status.CaptureManifest != nil { + t.Fatal("capture metadata recorded before worker preparation") + } + var gate corev1.ConfigMap + if err := r.Get(context.Background(), client.ObjectKey{Namespace: "ns", Name: backupStartConfigMapName(b)}, &gate); err != nil { + t.Fatal(err) + } + if gate.Data[backupStartTokenKey] != "" { + t.Fatal("unprepared worker received start authorization") + } + worker := budgetReadyWorker(t, r, b.Name, "prepared-worker-uid") + budgetPass(t, r, b.Name) + b = guardGetBackup(t, r, b.Name) + if q.quiesced != 1 || b.Status.CaptureManifest == nil || b.Status.CaptureManifest.State != "Recorded" || b.Status.CaptureManifest.QuiesceOutcome != "confirmed" { + t.Fatalf("prepared copy not quiesced/recorded: %+v, %+v", q, b.Status) + } + if volumes := b.Status.CaptureManifest.Volumes; len(volumes) != 1 || volumes[0].ClaimName != "server-data" || !volumes[0].Included || volumes[0].Classification != "data" { + t.Fatalf("prepared worker lost capture-time coverage: %+v", volumes) + } + if err := r.Get(context.Background(), client.ObjectKeyFromObject(&gate), &gate); err != nil { + t.Fatal(err) + } + if !strings.Contains(gate.Data[backupStartTokenKey], string(worker.UID)) || !strings.Contains(gate.Data[backupStartTokenKey], string(b.UID)) { + t.Fatalf("unbound authorization: %q", gate.Data[backupStartTokenKey]) + } + deadline := b.Annotations[backupQuiesceDeadlineAnnotation] + r = &BackupReconciler{Client: r.Client, Scheme: r.Scheme, AgentClient: q, QuiesceTimeout: time.Hour} + budgetPass(t, r, b.Name) + if got := guardGetBackup(t, r, b.Name).Annotations[backupQuiesceDeadlineAnnotation]; got != deadline { + t.Fatal("restart extended admitted quiesce deadline") + } +} + +func TestBackupPreparationDeadlineNeverDisablesSaving(t *testing.T) { + r, q := guardFixtureWorkers(t, false, guardBackup("expired")) + budgetPass(t, r, "expired") + b := guardGetBackup(t, r, "expired") + patchBackupAnnotations(b, map[string]string{backupPreparationDeadlineAnnotation: time.Now().Add(-time.Minute).UTC().Format(time.RFC3339Nano)}) + if err := r.Update(context.Background(), b); err != nil { + t.Fatal(err) + } + budgetPass(t, r, b.Name) + b = guardGetBackup(t, r, b.Name) + if b.Status.Phase != gameplanev1alpha1.BackupPhaseFailed || q.quiesced != 0 { + t.Fatalf("expired preparation disabled saving: %+v %+v", b.Status, q) + } +} + +func TestBackupRawAndStoppedCopiesKeepFullJobBudget(t *testing.T) { + for _, mode := range []string{"raw", "no-agent", "stopped"} { + t.Run(mode, func(t *testing.T) { + b := guardBackup("first") + b.Spec.Quiesce = mode != "raw" + r, q := guardFixture(t, b) + if mode == "no-agent" { + r.AgentClient = nil + } + if mode == "stopped" { + r, q = stoppedBackupFixture(t, true) + } + r.QuiesceTimeout, r.JobActiveDeadlineSeconds = time.Second, 7200 + budgetPass(t, r, "first") + b = guardGetBackup(t, r, "first") + deadline, err := time.Parse(time.RFC3339Nano, b.Annotations[backupQuiesceDeadlineAnnotation]) + if err != nil || time.Until(deadline) < time.Hour || b.Annotations[backupAuthorizedWorkerAnnotation] == "" || q.quiesced != 0 { + t.Fatalf("non-quiesced copy lost its full budget/authorization: %v %+v %+v", err, b.Annotations, q) + } + }) + } +} + +func TestBackupQueuedCopyDoesNotSpendPreparationBudget(t *testing.T) { + r, _ := guardFixture(t, guardBackup("first"), guardBackup("second")) + budgetPass(t, r, "first") + budgetPass(t, r, "second") + second := guardGetBackup(t, r, "second") + if second.Annotations[backupPreparationDeadlineAnnotation] != "" || second.Annotations[backupAuthorizedWorkerAnnotation] != "" { + t.Fatal("queued Backup spent preparation budget before owning the source") + } +} + +func TestBackupQuiesceDeadlineDrainsBeforeSaveOn(t *testing.T) { + r, q := guardFixtureWorkers(t, false, guardBackup("expired")) + budgetPass(t, r, "expired") + worker := budgetReadyWorker(t, r, "expired", "worker-uid") + budgetPass(t, r, "expired") + b := guardGetBackup(t, r, "expired") + patchBackupAnnotations(b, map[string]string{backupQuiesceDeadlineAnnotation: time.Now().Add(-time.Minute).UTC().Format(time.RFC3339Nano)}) + if err := r.Update(context.Background(), b); err != nil { + t.Fatal(err) + } + budgetPass(t, r, b.Name) + b = guardGetBackup(t, r, b.Name) + if b.Status.Phase != gameplanev1alpha1.BackupPhaseFailed || q.unquiesced != 0 { + t.Fatalf("deadline bypassed worker drain: %+v %+v", b.Status, q) + } + if err := r.Delete(context.Background(), worker); err != nil { + t.Fatal(err) + } + budgetPass(t, r, b.Name) + if q.unquiesced != 1 { + t.Fatalf("save-on not retried after drain: %+v", q) + } +} + +func TestBackupDeletionRevokesAuthorizationBeforeTerminatingWorkerDrains(t *testing.T) { + r, q := guardFixtureWorkers(t, false, guardBackup("cancel")) + budgetPass(t, r, "cancel") + worker := budgetReadyWorker(t, r, "cancel", "worker-uid") + budgetPass(t, r, "cancel") + worker.Finalizers = []string{"test/hold"} + if err := r.Update(t.Context(), worker); err != nil { + t.Fatal(err) + } + if err := r.Delete(t.Context(), worker); err != nil { + t.Fatal(err) + } + b := guardGetBackup(t, r, "cancel") + if err := r.Delete(t.Context(), b); err != nil { + t.Fatal(err) + } + budgetPass(t, r, "cancel") + var gate corev1.ConfigMap + if err := r.Get(t.Context(), client.ObjectKey{Namespace: b.Namespace, Name: backupStartConfigMapName(b)}, &gate); err != nil { + t.Fatal(err) + } + if gate.Data[backupStartTokenKey] != "" || q.unquiesced != 0 { + t.Fatal("deletion retained authorization or bypassed terminating worker") + } + if err := r.Get(t.Context(), client.ObjectKeyFromObject(worker), worker); err != nil { + t.Fatal(err) + } + worker.Finalizers = nil + if err := r.Update(t.Context(), worker); err != nil { + t.Fatal(err) + } + budgetPass(t, r, "cancel") + if q.unquiesced != 1 { + t.Fatal("deletion did not restore saving after drain") + } +} + +func TestBackupRecoveryRevokesTokenBeforeCancellingWorkers(t *testing.T) { + r, _ := guardFixtureWorkers(t, false, guardBackup("orphan")) + budgetPass(t, r, "orphan") + budgetReadyWorker(t, r, "orphan", "orphan-worker") + budgetPass(t, r, "orphan") + b := guardGetBackup(t, r, "orphan") + deletionObserved := false + r.Client = interceptor.NewClient(r.Client.(client.WithWatch), interceptor.Funcs{Delete: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.DeleteOption) error { + if _, ok := obj.(*batchv1.Job); ok { + var gate corev1.ConfigMap + if err := c.Get(ctx, client.ObjectKey{Namespace: b.Namespace, Name: backupStartConfigMapName(b)}, &gate); err != nil { + t.Fatal(err) + } + if gate.Data[backupStartTokenKey] != "" { + t.Fatal("recovery cancelled Job before revoking authorization") + } + deletionObserved = true + } + return c.Delete(ctx, obj, opts...) + }}) + busy, err := r.backupWorkersLive(t.Context(), b.Namespace, backupIdentity{Name: b.Name, UID: b.UID}, true) + if err != nil || !busy || !deletionObserved { + t.Fatalf("recovery bypassed worker cancellation/drain: %t %t %v", busy, deletionObserved, err) + } +} + +func TestBackupCSIDeadlineInvalidatesLateReady(t *testing.T) { + for _, status := range []*snapshotv1.VolumeSnapshotStatus{nil, {ReadyToUse: ownerBoolPtr(false)}} { + b := guardBackup("snapshot") + b.Spec.Strategy = "volume-snapshot" + r, q := guardFixture(t, b) + budgetPass(t, r, b.Name) + b = guardGetBackup(t, r, b.Name) + patchBackupAnnotations(b, map[string]string{backupCSIDeadlineAnnotation: time.Now().Add(-time.Minute).UTC().Format(time.RFC3339Nano)}) + if err := r.Update(context.Background(), b); err != nil { + t.Fatal(err) + } + var vs snapshotv1.VolumeSnapshot + if err := r.Get(context.Background(), client.ObjectKeyFromObject(b), &vs); err != nil { + t.Fatal(err) + } + vs.Status = status + if err := r.Status().Update(context.Background(), &vs); err != nil { + t.Fatal(err) + } + budgetPass(t, r, b.Name) + b = guardGetBackup(t, r, b.Name) + if b.Annotations[backupSnapshotAbortAnnotation] != "true" || b.Status.Phase != gameplanev1alpha1.BackupPhaseFailed || b.Status.SnapshotID != "" || q.unquiesced != 1 { + t.Fatalf("CSI abort did not invalidate before cleanup: %+v %+v", b, q) + } + vs.Status = &snapshotv1.VolumeSnapshotStatus{ReadyToUse: ownerBoolPtr(true)} + if _, err := r.observeVolumeSnapshot(context.Background(), b, &vs); err != nil { + t.Fatal(err) + } + if got := guardGetBackup(t, r, b.Name); got.Status.Phase != gameplanev1alpha1.BackupPhaseFailed || got.Status.SnapshotID != "" { + t.Fatal("late readiness revived aborted capture") + } + } +} + +func TestBackupWorkerDoesNotTrustCachedReadinessOrReplacement(t *testing.T) { + for _, replacement := range []bool{false, true} { + t.Run(fmt.Sprintf("replacement-%t", replacement), func(t *testing.T) { + r, q := guardFixtureWorkers(t, false, guardBackup("copy")) + budgetPass(t, r, "copy") + worker := budgetReadyWorker(t, r, "copy", "first-worker") + if replacement { + q.afterQuiesce = func() { + if err := r.Delete(t.Context(), worker); err != nil { + t.Fatal(err) + } + budgetReadyWorker(t, r, "copy", "replacement-worker") + } + } else { + r.APIReader = interceptor.NewClient(r.Client.(client.WithWatch), interceptor.Funcs{List: func(ctx context.Context, cl client.WithWatch, list client.ObjectList, opts ...client.ListOption) error { + if err := cl.List(ctx, list, opts...); err != nil { + return err + } + if pods, ok := list.(*corev1.PodList); ok { + for i := range pods.Items { + if pods.Items[i].UID == worker.UID { + pods.Items[i].Status.Conditions = nil + } + } + } + return nil + }}) + } + if _, err := r.Reconcile(t.Context(), ctrl.Request{NamespacedName: client.ObjectKey{Namespace: "ns", Name: "copy"}}); err != nil { + t.Fatal(err) + } + b := guardGetBackup(t, r, "copy") + var gate corev1.ConfigMap + if err := r.Get(t.Context(), client.ObjectKey{Namespace: "ns", Name: backupStartConfigMapName(b)}, &gate); err != nil { + t.Fatal(err) + } + if gate.Data[backupStartTokenKey] != "" { + t.Fatal("stale readiness granted copy authorization") + } + if !replacement && q.quiesced != 0 { + t.Fatal("cached Ready disabled saving") + } + if replacement && q.quiesced != 1 { + t.Fatal("fixture did not replace worker during RPC") + } + }) + } +} + +func TestPreparedBackupPlacementAndResources(t *testing.T) { + for _, hard := range []bool{false, true} { + r := &BackupReconciler{HardSameNode: hard, BackupResources: &corev1.ResourceRequirements{}} + b := guardBackup("copy") + source := &backupWorkerSource{NodeName: "node-a", Tolerations: []corev1.Toleration{{Key: "games", Operator: corev1.TolerationOpExists, Effect: corev1.TaintEffectNoSchedule}}} + spec := r.buildPreparedBackupPodSpec(b, &gameplanev1alpha1.GameTemplate{}, source, time.Now().Add(time.Minute)) + if spec.AutomountServiceAccountToken == nil || *spec.AutomountServiceAccountToken || len(spec.Tolerations) != 1 || spec.Tolerations[0].Key != "games" { + t.Fatal("worker credentials or tolerations contract lost") + } + if spec.Affinity == nil || len(spec.Affinity.NodeAffinity.PreferredDuringSchedulingIgnoredDuringExecution) != 1 { + t.Fatal("same-node preference missing") + } + if (spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution != nil) != hard { + t.Fatal("hard placement opt-in not respected") + } + if len(spec.Containers[0].Resources.Requests) != 0 || spec.Containers[0].ReadinessProbe == nil { + t.Fatal("resource clearing or preparation probe missing") + } + source.NodeName = "" + if stopped := r.buildPreparedBackupPodSpec(b, &gameplanev1alpha1.GameTemplate{}, source, time.Now()); stopped.Affinity != nil { + t.Fatal("stopped source invented a game node") + } + } +} + +// Execute the real gate script in CI with only its mount paths redirected to +// temp directories. A fake restic records whether source capture was invoked. +func TestBackupWorkerScriptRejectsExpiredAndReplayedTokens(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("worker image uses a POSIX shell; executed by Linux CI") + } + for _, tokenKind := range []string{"valid", "expired", "other-worker", "other-backup", "other-source", "repo-failure"} { + t.Run(tokenKind, func(t *testing.T) { + dir := t.TempDir() + restic := "#!/bin/sh\nif [ \"$1\" = snapshots ]; then [ \"$REPO_FAIL\" != true ]; exit $?; fi\nprintf copied > \"$COPY_MARKER\"\n" + if err := os.WriteFile(filepath.Join(dir, "restic"), []byte(restic), 0o700); err != nil { + t.Fatal(err) + } + backup, pod, source, expiry := "backup", "worker", "source", time.Now().Add(time.Minute).Unix() + switch tokenKind { + case "expired": + expiry = time.Now().Add(-time.Minute).Unix() + case "other-worker": + pod = "old-worker" + case "other-backup": + backup = "old-backup" + case "other-source": + source = "old-source" + } + tokenFile, prepared, copied := filepath.Join(dir, "token"), filepath.Join(dir, "prepared"), filepath.Join(dir, "copied") + if err := os.WriteFile(tokenFile, fmt.Appendf(nil, "v1 %s %s %s %d\n", backup, pod, source, expiry), 0o600); err != nil { + t.Fatal(err) + } + script := strings.NewReplacer("/data", dir, "/start/start", tokenFile, "/tmp/prepared", prepared).Replace(backupWorkerScript) + ctx, cancel := context.WithTimeout(t.Context(), 4*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, "/bin/sh", "-c", script, "backup-worker", "backup", dir) + cmd.Env = append(os.Environ(), "PATH="+dir+":"+os.Getenv("PATH"), "BACKUP_UID=backup", "POD_UID=worker", "SOURCE_ID=source", fmt.Sprintf("PREPARATION_EXPIRES=%d", time.Now().Add(time.Second).Unix()), "COPY_MARKER="+copied, fmt.Sprintf("REPO_FAIL=%t", tokenKind == "repo-failure")) + output, err := cmd.CombinedOutput() + if ctx.Err() != nil { + t.Fatalf("gate did not terminate: %s", output) + } + _, copiedErr := os.Stat(copied) + if tokenKind == "valid" { + if err != nil || copiedErr != nil { + t.Fatalf("valid token did not start copy: %v %s", err, output) + } + } else if err == nil || copiedErr == nil { + t.Fatalf("invalid token/preparation started capture: %v %s", err, output) + } + if tokenKind == "repo-failure" { + if _, err := os.Stat(prepared); err == nil { + t.Fatal("repository failure became Ready") + } + } + }) + } +} + +func TestBackupCSIAbortReclaimsOnlyVerifiedDeleteContent(t *testing.T) { + for _, mode := range []string{"delete", "retain", "foreign-reference", "replacement-snapshot", "delete-error"} { + t.Run(mode, func(t *testing.T) { + b := guardBackup("snapshot") + b.Spec.Strategy = "volume-snapshot" + r, q := guardFixture(t, b) + budgetPass(t, r, b.Name) + b = guardGetBackup(t, r, b.Name) + var vs snapshotv1.VolumeSnapshot + if err := r.Get(t.Context(), client.ObjectKeyFromObject(b), &vs); err != nil { + t.Fatal(err) + } + contentName := "content" + content := &snapshotv1.VolumeSnapshotContent{ObjectMeta: metav1.ObjectMeta{Name: contentName, UID: "content-uid"}, Spec: snapshotv1.VolumeSnapshotContentSpec{DeletionPolicy: snapshotv1.VolumeSnapshotContentDelete, + VolumeSnapshotRef: corev1.ObjectReference{Name: vs.Name, Namespace: vs.Namespace, UID: vs.UID}}} + if mode == "retain" { + content.Spec.DeletionPolicy = snapshotv1.VolumeSnapshotContentRetain + } + if mode == "foreign-reference" { + content.Spec.VolumeSnapshotRef.UID = "foreign-snapshot" + } + if err := r.Create(t.Context(), content); err != nil { + t.Fatal(err) + } + vs.Status = &snapshotv1.VolumeSnapshotStatus{BoundVolumeSnapshotContentName: &contentName} + if err := r.Status().Update(t.Context(), &vs); err != nil { + t.Fatal(err) + } + if mode == "delete" { + r.Client = interceptor.NewClient(r.Client.(client.WithWatch), interceptor.Funcs{Delete: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.DeleteOption) error { + options := &client.DeleteOptions{} + for _, option := range opts { + option.ApplyToDelete(options) + } + if options.Preconditions == nil || options.Preconditions.UID == nil || *options.Preconditions.UID != obj.GetUID() { + t.Fatalf("cleanup deletion lacks exact UID precondition: %+v", options) + } + return c.Delete(ctx, obj, opts...) + }}) + } + if mode == "replacement-snapshot" { + if err := r.Delete(t.Context(), &vs); err != nil { + t.Fatal(err) + } + vs.UID, vs.ResourceVersion = "replacement-uid", "" + if err := r.Create(t.Context(), &vs); err != nil { + t.Fatal(err) + } + } + if mode == "delete-error" { + r.Client = interceptor.NewClient(r.Client.(client.WithWatch), interceptor.Funcs{Delete: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.DeleteOption) error { + if _, ok := obj.(*snapshotv1.VolumeSnapshot); ok { + return fmt.Errorf("driver deletion unavailable") + } + return c.Delete(ctx, obj, opts...) + }}) + } + patchBackupAnnotations(b, map[string]string{backupCSIDeadlineAnnotation: time.Now().Add(-time.Minute).UTC().Format(time.RFC3339Nano)}) + if err := r.Update(t.Context(), b); err != nil { + t.Fatal(err) + } + budgetPass(t, r, b.Name) + b = guardGetBackup(t, r, b.Name) + if q.unquiesced != 1 || b.Status.Phase != gameplanev1alpha1.BackupPhaseFailed || b.Status.SnapshotID != "" { + t.Fatalf("reclamation blocked invalidation/save-on: %+v %+v", b.Status, q) + } + condition := meta.FindStatusCondition(b.Status.Conditions, "SnapshotCleaned") + if condition == nil { + t.Fatal("missing cleanup condition") + } + err := r.Get(t.Context(), client.ObjectKeyFromObject(content), content) + switch mode { + case "delete": + if !apierrors.IsNotFound(err) || condition.Status != metav1.ConditionTrue { + t.Fatalf("owned Delete content was not reclaimed: %v %+v", err, condition) + } + case "retain": + if err != nil || condition.Reason != "SnapshotCleanupRetained" || !strings.Contains(condition.Message, "content-uid") { + t.Fatalf("Retain identity was lost: %v %+v", err, condition) + } + case "foreign-reference", "replacement-snapshot": + if err != nil || condition.Reason != "SnapshotCleanupUnverified" { + t.Fatalf("foreign resource was touched: %v %+v", err, condition) + } + case "delete-error": + if condition.Reason != "SnapshotCleanupPending" || !strings.Contains(condition.Message, "ns/snapshot") || !strings.Contains(condition.Message, string(vs.UID)) { + t.Fatalf("delete failure not visible: %+v", condition) + } + } + }) + } +} diff --git a/operator/internal/controller/backup_controller.go b/operator/internal/controller/backup_controller.go index 5be2457c8..dab5f3ef8 100644 --- a/operator/internal/controller/backup_controller.go +++ b/operator/internal/controller/backup_controller.go @@ -127,6 +127,10 @@ type BackupReconciler struct { // DefaultBackupJobBackoffLimit / DefaultBackupJobActiveDeadlineSeconds. JobBackoffLimit *int32 JobActiveDeadlineSeconds int64 + PreparationTimeout time.Duration + QuiesceTimeout time.Duration + CSICaptureTimeout time.Duration + HardSameNode bool // SnapshotScrapeGracePeriod bounds how long after the restic Job // completes the reconciler keeps retrying to read the snapshot id from // the pod logs before failing the Backup. Zero uses @@ -235,7 +239,7 @@ func resticImageOrDefault(image string) string { // +kubebuilder:rbac:groups="",resources=events,verbs=create;patch // +kubebuilder:rbac:groups=events.k8s.io,resources=events,verbs=create;patch // +kubebuilder:rbac:groups=snapshot.storage.k8s.io,resources=volumesnapshots,verbs=get;list;watch -// +kubebuilder:rbac:groups=snapshot.storage.k8s.io,resources=volumesnapshotcontents,verbs=get;list;watch +// +kubebuilder:rbac:groups=snapshot.storage.k8s.io,resources=volumesnapshotcontents,verbs=get;list;watch;delete func (r *BackupReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { return requeueOnConflict(r.reconcile(ctx, req)) @@ -250,6 +254,12 @@ func (r *BackupReconciler) reconcile(ctx context.Context, req ctrl.Request) (ctr if !b.DeletionTimestamp.IsZero() { return r.finalizeDelete(ctx, &b) } + if b.Annotations[backupCopyAbortAnnotation] != "" { + return r.abortResticCopy(ctx, &b, b.Annotations[backupCopyAbortAnnotation]) + } + if b.Annotations[backupSnapshotAbortAnnotation] == "true" { + return r.abortVolumeSnapshot(ctx, &b, "volume snapshot capture was aborted") + } // Every copy owns the source until its workers drain, even without quiesce. if !controllerutil.ContainsFinalizer(&b, gameplanev1alpha1.BackupFinalizer) { controllerutil.AddFinalizer(&b, gameplanev1alpha1.BackupFinalizer) @@ -292,6 +302,12 @@ func (r *BackupReconciler) reconcile(ctx context.Context, req ctrl.Request) (ctr // is outstanding, so this stays cheap on the common path. if b.Status.Phase == gameplanev1alpha1.BackupPhaseSucceeded || b.Status.Phase == gameplanev1alpha1.BackupPhaseFailed { + if b.Status.Phase == gameplanev1alpha1.BackupPhaseFailed && b.Spec.Strategy == "volume-snapshot" { + return r.abortVolumeSnapshot(ctx, &b, b.Status.Message) + } + if b.Status.Phase == gameplanev1alpha1.BackupPhaseFailed && b.Annotations[backupPreparationDeadlineAnnotation] != "" { + return r.abortResticCopy(ctx, &b, b.Status.Message) + } return r.runUnquiesce(ctx, &b) } // Observe pre-upgrade workers even when multiple legacy quiesced copies @@ -364,26 +380,7 @@ func (r *BackupReconciler) reconcile(ctx context.Context, req ctrl.Request) (ctr b.Spec.RepoRef.Name, strings.Join(missing, ", "))) } - if res, err := r.prepareBackupQuiesce(ctx, &b); err != nil || res.RequeueAfter > 0 { - return res, err - } - - backoff, deadline := resolveJobLimits(r.JobBackoffLimit, r.JobActiveDeadlineSeconds) - job := &batchv1.Job{ObjectMeta: metav1.ObjectMeta{Name: b.Name, Namespace: b.Namespace}} - if _, err := controllerutil.CreateOrUpdate(ctx, r.Client, job, func() error { - if job.CreationTimestamp.IsZero() { - job.Spec.BackoffLimit = &backoff - job.Spec.ActiveDeadlineSeconds = &deadline - job.Spec.Template.Labels = map[string]string{backupRestoreJobLabel: backupRestoreJobValue, backupUIDLabel: string(b.UID)} - job.Spec.Template.Spec = r.buildBackupPodSpec(&b, &tmpl) - job.Spec.Template.Spec.RestartPolicy = corev1.RestartPolicyNever - } - return controllerutil.SetControllerReference(&b, job, r.Scheme) - }); err != nil { - return ctrl.Result{}, err - } - - return r.mirrorJobStatus(ctx, &b, job) + return r.reconcilePreparedRestic(ctx, &b, &tmpl) } // maybeQuiesce calls the agent's /quiesce endpoint once per Backup. @@ -581,6 +578,9 @@ func (r *BackupReconciler) mirrorJobStatus( cond.Reason = string(phase) } newConds := upsertCondition(b.Status.Conditions, cond) + if phase == gameplanev1alpha1.BackupPhaseSucceeded || phase == gameplanev1alpha1.BackupPhaseFailed { + meta.RemoveStatusCondition(&newConds, "CaptureReady") + } newConds = upsertCondition(newConds, backupConsistencyCondition(b)) if !sameConditions(b.Status.Conditions, newConds) { b.Status.Conditions = newConds @@ -779,6 +779,11 @@ func (r *BackupReconciler) finalizeUnquiesce(ctx context.Context, b *gameplanev1 return ctrl.Result{}, err } } + if b.Annotations[backupSnapshotAbortAnnotation] == "true" { + if res, err := r.cleanupAbortedSnapshot(ctx, b); err != nil || res.RequeueAfter > 0 { + return res, err + } + } controllerutil.RemoveFinalizer(b, gameplanev1alpha1.BackupFinalizer) if err := r.Update(ctx, b); err != nil { return ctrl.Result{}, err @@ -958,6 +963,7 @@ func (r *BackupReconciler) readResticSummary(ctx context.Context, b *gameplanev1 func (r *BackupReconciler) fail(ctx context.Context, b *gameplanev1alpha1.Backup, msg string) (ctrl.Result, error) { now := metav1.Now() b.Status.Phase = gameplanev1alpha1.BackupPhaseFailed + meta.RemoveStatusCondition(&b.Status.Conditions, "CaptureReady") b.Status.ObservedGeneration = b.Generation if b.Status.Message != msg { b.Status.Message = msg diff --git a/operator/internal/controller/backup_envtest_test.go b/operator/internal/controller/backup_envtest_test.go index f577563a6..a2b6d9aa3 100644 --- a/operator/internal/controller/backup_envtest_test.go +++ b/operator/internal/controller/backup_envtest_test.go @@ -56,7 +56,7 @@ func TestBackup_CreatesJobWithExpectedSpec(t *testing.T) { if !strings.HasPrefix(c.Image, "restic/restic:") { t.Errorf("image = %q, want restic/restic:*", c.Image) } - if got, want := c.Args, []string{"backup", "/data", "--json", "--retry-lock", "10m", "--tag", "gameplane"}; !equalStrings(got, want) { + if got, want := c.Args, []string{backupWorkerScript, "backup-worker", "backup", "/data", "--json", "--retry-lock", "10m", "--tag", "gameplane"}; !equalStrings(got, want) { t.Errorf("args = %v, want %v", got, want) } @@ -365,6 +365,7 @@ func TestBackup_PassesTagsToRestic(t *testing.T) { return false, "job not yet created" } want := []string{"backup", "/data", "--json", "--retry-lock", "10m", "--tag", "gameplane", "--tag", "nightly", "--tag", "preupgrade"} + want = append([]string{backupWorkerScript, "backup-worker"}, want...) if !equalStrings(j.Spec.Template.Spec.Containers[0].Args, want) { return false, "args = " + strings.Join(j.Spec.Template.Spec.Containers[0].Args, " ") } @@ -372,11 +373,9 @@ func TestBackup_PassesTagsToRestic(t *testing.T) { }) } -// TestBackup_QuiesceAnnotationSetBeforeJob verifies that with a real -// agent client (faked) and spec.quiesce=true (the default), the -// controller calls Quiesce and stamps the quiesced-at annotation -// before the restic Job appears. -func TestBackup_QuiesceAnnotationSetBeforeJob(t *testing.T) { +// The Job must prepare before saving is disabled; authorization follows the +// durable quiesce annotation and cannot be granted to an unready worker. +func TestBackup_QuiesceAfterWorkerPreparation(t *testing.T) { ns := newNamespace(t) fa := &fakeAgent{} startMgr(t, ns, withBackupReconciler(backupReconcilerOpts{agent: fa})) @@ -404,6 +403,16 @@ func TestBackup_QuiesceAnnotationSetBeforeJob(t *testing.T) { if err := k8sClient.Create(context.Background(), b); err != nil { t.Fatalf("create backup: %v", err) } + var copyJob *batchv1.Job + eventually(t, func() (bool, string) { + var ok bool + copyJob, ok = getJob(t, ns, "smp-q") + return ok, "job not yet created" + }) + if fa.quiesceCount() != 0 { + t.Fatal("save-off happened before worker preparation") + } + prepareEnvtestBackupWorker(t, copyJob) eventually(t, func() (bool, string) { got := getBackup(t, ns, "smp-q") @@ -416,7 +425,7 @@ func TestBackup_QuiesceAnnotationSetBeforeJob(t *testing.T) { return true, "" }) - // Job exists and was created at-or-after the quiesce call. + // The prepared Job remains the authorized copy. eventually(t, func() (bool, string) { _, ok := getJob(t, ns, "smp-q") return ok, "job not yet created" diff --git a/operator/internal/controller/backup_forget.go b/operator/internal/controller/backup_forget.go index 2d97d9410..5d73b8170 100644 --- a/operator/internal/controller/backup_forget.go +++ b/operator/internal/controller/backup_forget.go @@ -112,6 +112,21 @@ func wantsSnapshotFinalizer(b *gameplanev1alpha1.Backup) bool { // forgets the restic snapshot. Unquiesce goes first because it is time // critical: a world left with auto-save off is worse than an orphaned snapshot. func (r *BackupReconciler) finalizeDelete(ctx context.Context, b *gameplanev1alpha1.Backup) (ctrl.Result, error) { + if b.Annotations[backupPreparationDeadlineAnnotation] != "" { + if err := r.revokeBackupStart(ctx, b); err != nil { + return ctrl.Result{}, err + } + } + if b.Spec.Strategy == "volume-snapshot" && b.Status.Phase != gameplanev1alpha1.BackupPhaseSucceeded { + if err := r.recordSnapshotAbort(ctx, b, "CSI capture cancelled by Backup deletion"); err != nil { + return ctrl.Result{}, err + } + } + if b.Annotations[backupPreparationDeadlineAnnotation] != "" && b.Status.Phase != gameplanev1alpha1.BackupPhaseSucceeded { + if res, err := r.abortResticCopy(ctx, b, "copy cancelled by Backup deletion"); err != nil || res.RequeueAfter > 0 { + return res, err + } + } if controllerutil.ContainsFinalizer(b, gameplanev1alpha1.BackupFinalizer) { res, err := r.finalizeUnquiesce(ctx, b) if err != nil || res.RequeueAfter > 0 { diff --git a/operator/internal/controller/backup_guard.go b/operator/internal/controller/backup_guard.go index ea0f8d84b..e78dda59d 100644 --- a/operator/internal/controller/backup_guard.go +++ b/operator/internal/controller/backup_guard.go @@ -252,7 +252,7 @@ func (r *BackupReconciler) legacyQuiescedBackups(ctx context.Context, b *gamepla return legacy, nil } -func (r *BackupReconciler) prepareBackupQuiesce(ctx context.Context, b *gameplanev1alpha1.Backup) (ctrl.Result, error) { +func (r *BackupReconciler) prepareBackupWorkload(ctx context.Context, b *gameplanev1alpha1.Backup) (ctrl.Result, error) { claimed, err := r.claimBackupTarget(ctx, b) if err != nil { if errors.Is(err, errLifecycleGenerationChanged) { @@ -283,12 +283,23 @@ func (r *BackupReconciler) prepareBackupQuiesce(ctx context.Context, b *gameplan if err != nil || !ready { return ctrl.Result{RequeueAfter: 5 * time.Second}, err } + return ctrl.Result{}, nil +} + +func (r *BackupReconciler) prepareBackupQuiesce(ctx context.Context, b *gameplanev1alpha1.Backup) (ctrl.Result, error) { + if res, err := r.prepareBackupWorkload(ctx, b); err != nil || res.RequeueAfter > 0 { + return res, err + } + gs, err := r.ownedBackupTarget(ctx, b) + if err != nil || gs == nil { + return ctrl.Result{RequeueAfter: 5 * time.Second}, err + } if err := r.maybeQuiesce(ctx, b); err != nil { return ctrl.Result{}, err } // The Service RPC may overlap a pod replacement. Never start a copy on a // different process from the one pinned before the request. - ready, err = r.prepareBackupSource(ctx, b, gs) + ready, err := r.prepareBackupSource(ctx, b, gs) if errors.Is(err, errBackupSourceChanged) { _, err = r.fail(ctx, b, err.Error()) return ctrl.Result{RequeueAfter: time.Nanosecond}, err @@ -356,6 +367,14 @@ func (r *BackupReconciler) releaseBackupTarget(ctx context.Context, b *gameplane // world while a deleting/failed Job still has a restic worker mounted. The // UID label keeps orphaned workers visible even after their Job disappears. func (r *BackupReconciler) backupWorkersLive(ctx context.Context, namespace string, id backupIdentity, cancel bool) (bool, error) { + if cancel { + // Recovery can run after the Backup disappeared. The immutable owner + // UID still identifies its gate, so revoke it before touching workers. + owner := &gameplanev1alpha1.Backup{ObjectMeta: metav1.ObjectMeta{Name: id.Name, Namespace: namespace, UID: id.UID}} + if err := r.revokeBackupStart(ctx, owner); err != nil { + return true, err + } + } var jobs batchv1.JobList if err := r.backupAPIReader().List(ctx, &jobs, client.InNamespace(namespace)); err != nil { return false, err @@ -398,13 +417,13 @@ func (r *BackupReconciler) backupWorkersLive(ctx context.Context, namespace stri if busy { return true, nil } - return r.backupSnapshotCopyLive(ctx, namespace, id) + return r.backupSnapshotCopyLive(ctx, namespace, id, cancel) } -// CSI captures have no Job or Pod. On normal deletion retain quiesce until -// capture completion/error instead of deleting the pending VolumeSnapshot; -// owner-reference GC will delete it after the Backup's finalizer clears. -func (r *BackupReconciler) backupSnapshotCopyLive(ctx context.Context, namespace string, id backupIdentity) (bool, error) { +// Normal captures remain live until ready/error. Cancellation first invalidates +// the Backup; orphan recovery marks its UID-bound snapshot invalid before +// save-on, since no surviving Backup can expose it as a restorable capture. +func (r *BackupReconciler) backupSnapshotCopyLive(ctx context.Context, namespace string, id backupIdentity, cancel bool) (bool, error) { scheme := r.Scheme if scheme == nil { scheme = r.Client.Scheme() @@ -432,6 +451,29 @@ func (r *BackupReconciler) backupSnapshotCopyLive(ctx context.Context, namespace if owner == nil || owner.Kind != "Backup" || owner.UID != id.UID { return false, nil } + if cancel { + if b.UID == id.UID && b.Status.Phase != gameplanev1alpha1.BackupPhaseSucceeded { + return false, r.recordSnapshotAbort(ctx, &b, "CSI capture cancelled during lifecycle recovery") + } + if b.UID != id.UID { + if vs.Annotations == nil { + vs.Annotations = map[string]string{} + } + if vs.Annotations[backupSnapshotAbortAnnotation] != "true" { + vs.Annotations[backupSnapshotAbortAnnotation] = "true" + if err := r.Update(ctx, &vs); err != nil { + return true, err + } + } + uid := vs.UID + if uid != "" && vs.DeletionTimestamp.IsZero() { + if err := r.Delete(ctx, &vs, &client.DeleteOptions{Preconditions: &metav1.Preconditions{UID: &uid}}); err != nil && !apierrors.IsNotFound(err) { + ctrl.LoggerFrom(ctx).Error(err, "orphaned aborted snapshot requires reclamation", "snapshot", client.ObjectKeyFromObject(&vs), "uid", uid) + } + } + return false, nil + } + } if vs.Status != nil && ((vs.Status.ReadyToUse != nil && *vs.Status.ReadyToUse) || (vs.Status.Error != nil && vs.Status.Error.Message != nil)) { return false, nil } diff --git a/operator/internal/controller/backup_guard_test.go b/operator/internal/controller/backup_guard_test.go index 44afc6713..d94790665 100644 --- a/operator/internal/controller/backup_guard_test.go +++ b/operator/internal/controller/backup_guard_test.go @@ -58,6 +58,12 @@ func guardBackup(name string) *gameplanev1alpha1.Backup { } func guardFixture(t *testing.T, backups ...*gameplanev1alpha1.Backup) (*BackupReconciler, *guardQuiescer) { + return guardFixtureWorkers(t, true, backups...) +} + +// Most guard tests model a healthy Job controller/kubelet. Budget tests opt +// out and advance worker scheduling/readiness explicitly. +func guardFixtureWorkers(t *testing.T, automatic bool, backups ...*gameplanev1alpha1.Backup) (*BackupReconciler, *guardQuiescer) { t.Helper() s := scrapeScheme(t) if err := snapshotv1.AddToScheme(s); err != nil { @@ -74,6 +80,31 @@ func guardFixture(t *testing.T, backups ...*gameplanev1alpha1.Backup) (*BackupRe objects = append(objects, b) } cl := fake.NewClientBuilder().WithScheme(s).WithObjects(objects...).WithStatusSubresource(&gameplanev1alpha1.Backup{}, &gameplanev1alpha1.GameServer{}, &batchv1.Job{}, &snapshotv1.VolumeSnapshot{}).Build() + cl = interceptor.NewClient(cl, interceptor.Funcs{Create: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.CreateOption) error { + if obj.GetUID() == "" { + obj.SetUID(types.UID(obj.GetName() + "-generated-uid")) + } + if err := c.Create(ctx, obj, opts...); err != nil { + return err + } + job, ok := obj.(*batchv1.Job) + if !automatic || !ok || job.Annotations[backupWorkerProtocolAnnotation] != "v1" { + return nil + } + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: job.Name + "-automatic", Namespace: job.Namespace, UID: types.UID(job.Name + "-automatic-uid"), Labels: job.Spec.Template.Labels, + OwnerReferences: []metav1.OwnerReference{{APIVersion: "batch/v1", Kind: "Job", Name: job.Name, UID: job.UID, Controller: ownerBoolPtr(true)}}}, Spec: job.Spec.Template.Spec, + Status: corev1.PodStatus{Phase: corev1.PodRunning, Conditions: []corev1.PodCondition{{Type: corev1.PodReady, Status: corev1.ConditionTrue}}}} + pod.Spec.NodeName = "node-a" + return c.Create(ctx, pod) + }, Delete: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.DeleteOption) error { + if err := c.Delete(ctx, obj, opts...); err != nil { + return err + } + if job, ok := obj.(*batchv1.Job); automatic && ok { + return client.IgnoreNotFound(c.Delete(ctx, &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: job.Name + "-automatic", Namespace: job.Namespace}})) + } + return nil + }}) q := &guardQuiescer{} return &BackupReconciler{Client: cl, Scheme: s, AgentClient: q, LogReader: scrapeLogReader{body: `{"message_type":"summary","snapshot_id":"abc123"}`}}, q } @@ -81,6 +112,14 @@ func guardFixture(t *testing.T, backups ...*gameplanev1alpha1.Backup) (*BackupRe func guardPass(t *testing.T, r *BackupReconciler, name string) { t.Helper() for i := 0; i < 3; i++ { + // Model a completed worker after tests advance the Job's terminal + // status. Explicitly created live/terminating pods are never removed. + var job batchv1.Job + if err := r.Get(context.Background(), client.ObjectKey{Namespace: "ns", Name: name}, &job); err == nil && (job.Status.Succeeded > 0 || jobPermanentlyFailed(&job)) { + if err := r.Delete(context.Background(), &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: name + "-automatic"}}); err != nil && !apierrors.IsNotFound(err) { + t.Fatal(err) + } + } if _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: types.NamespacedName{Namespace: "ns", Name: name}}); err != nil { t.Fatal(err) } @@ -525,7 +564,7 @@ func TestBackupGuardRecoveryLineageSurvivesRepeatedOwnerDeletion(t *testing.T) { } } -func TestBackupGuardPendingSnapshotCleanupWaitsForCapture(t *testing.T) { +func TestBackupGuardPendingSnapshotCancellationInvalidatesBeforeSaveOn(t *testing.T) { for _, mode := range []string{"delete", "orphan", "legacy"} { t.Run(mode, func(t *testing.T) { a := guardBackup("snapshot") @@ -546,6 +585,19 @@ func TestBackupGuardPendingSnapshotCleanupWaitsForCapture(t *testing.T) { guardPass(t, r, "snapshot") } guardPass(t, r, "next") + if mode != "legacy" { + if q.unquiesced != 1 { + t.Fatalf("cancelled CSI capture did not restore saving: %+v", q) + } + var jobs batchv1.JobList + if err := r.List(context.Background(), &jobs); err != nil { + t.Fatal(err) + } + if len(jobs.Items) != 1 { + t.Fatal("next copy did not start after explicit CSI abort") + } + return + } if q.unquiesced != 0 { t.Fatalf("save-on during pending CSI capture: %+v", q) } @@ -600,11 +652,8 @@ func TestBackupSnapshotAmbiguousCreateStillCleansUp(t *testing.T) { t.Fatalf("ambiguous create lost abort cleanup: %+v", q) } var vs snapshotv1.VolumeSnapshot - if err := r.Get(context.Background(), types.NamespacedName{Namespace: "ns", Name: "snapshot"}, &vs); err != nil { - t.Fatal(err) - } - if vs.Status != nil && vs.Status.ReadyToUse != nil && *vs.Status.ReadyToUse { - t.Fatal("expected still-pending ambiguous capture") + if err := r.Get(context.Background(), types.NamespacedName{Namespace: "ns", Name: "snapshot"}, &vs); !apierrors.IsNotFound(err) { + t.Fatalf("ambiguous aborted capture was not reclaimed: %v", err) } } diff --git a/operator/internal/controller/backup_manifest.go b/operator/internal/controller/backup_manifest.go index 682a2214c..624550e97 100644 --- a/operator/internal/controller/backup_manifest.go +++ b/operator/internal/controller/backup_manifest.go @@ -15,8 +15,8 @@ import ( gameplanev1alpha1 "github.com/GameplanePanel/gameplane/operator/api/v1alpha1" ) -// Called after lifecycle ownership and the workload fence, before creating the -// copy worker. Requeues and later template edits never rewrite this manifest. +// Called after lifecycle ownership and the workload fence, before a copy is +// authorized. Requeues and later template edits never rewrite this manifest. func (r *BackupReconciler) recordBackupManifest(ctx context.Context, b *gameplanev1alpha1.Backup, gs *gameplanev1alpha1.GameServer) error { if b.Status.CaptureManifest != nil { return nil @@ -27,10 +27,25 @@ func (r *BackupReconciler) recordBackupManifest(ctx context.Context, b *gameplan worker = &snapshotv1.VolumeSnapshot{} } if err := reader.Get(ctx, client.ObjectKeyFromObject(b), worker); err == nil { - // Existing workers predate this metadata. Current layout cannot prove - // what they captured, even if their requested quiesce was successful. - b.Status.CaptureManifest = &gameplanev1alpha1.BackupCaptureManifest{State: "LegacyUnknown"} - return r.Status().Update(ctx, b) + // Current prepared workers exist before capture but cannot read data + // without authorization. Its durable annotation is written before the + // projected token and never cleared, including during cancellation. + // All other existing workers retain unknown historical coverage. + waiting := false + if job, ok := worker.(*batchv1.Job); ok && metav1.IsControlledBy(job, b) && job.Annotations[backupWorkerProtocolAnnotation] == "v1" && + b.Annotations[backupPreparationDeadlineAnnotation] != "" && b.Annotations[backupAuthorizedWorkerAnnotation] == "" && + job.Status.Succeeded == 0 && !jobPermanentlyFailed(job) { + var gate corev1.ConfigMap + if err := reader.Get(ctx, client.ObjectKey{Namespace: b.Namespace, Name: backupStartConfigMapName(b)}, &gate); err == nil { + waiting = metav1.IsControlledBy(&gate, b) && gate.Data[backupStartTokenKey] == "" + } else if !apierrors.IsNotFound(err) { + return err + } + } + if !waiting { + b.Status.CaptureManifest = &gameplanev1alpha1.BackupCaptureManifest{State: "LegacyUnknown"} + return r.Status().Update(ctx, b) + } } else if !apierrors.IsNotFound(err) { return err } diff --git a/operator/internal/controller/backup_manifest_test.go b/operator/internal/controller/backup_manifest_test.go index 5ccb1f0c7..006482c9e 100644 --- a/operator/internal/controller/backup_manifest_test.go +++ b/operator/internal/controller/backup_manifest_test.go @@ -101,6 +101,46 @@ func TestBackupManifestDoesNotInventHistoryForExistingWorker(t *testing.T) { } } +func TestBackupManifestDoesNotInventHistoryForAuthorizedPreparedWorker(t *testing.T) { + for _, mode := range []string{"authorized", "projected-token", "missing-gate", "foreign-gate"} { + t.Run(mode, func(t *testing.T) { + b := guardBackup("copy") + b.Annotations = map[string]string{backupPreparationDeadlineAnnotation: "persisted-before-preparation"} + r, _ := guardFixtureWorkers(t, false, b) + job := &batchv1.Job{ObjectMeta: metav1.ObjectMeta{Name: b.Name, Namespace: b.Namespace, Annotations: map[string]string{backupWorkerProtocolAnnotation: "v1"}, + OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(b, gameplanev1alpha1.GroupVersion.WithKind("Backup"))}}} + if err := r.Create(t.Context(), job); err != nil { + t.Fatal(err) + } + gate := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: backupStartConfigMapName(b), Namespace: b.Namespace, + OwnerReferences: job.OwnerReferences}, Data: map[string]string{backupStartTokenKey: ""}} + if mode == "authorized" { + b.Annotations[backupAuthorizedWorkerAnnotation] = "previous-worker" + if err := r.Update(t.Context(), b); err != nil { + t.Fatal(err) + } + } + if mode == "projected-token" { + gate.Data[backupStartTokenKey] = "previous-authorization" + } + if mode == "foreign-gate" { + gate.OwnerReferences = nil + } + if mode != "missing-gate" { + if err := r.Create(t.Context(), gate); err != nil { + t.Fatal(err) + } + } + if err := r.recordBackupManifest(t.Context(), b, &gameplanev1alpha1.GameServer{}); err != nil { + t.Fatal(err) + } + if b.Status.CaptureManifest == nil || b.Status.CaptureManifest.State != "LegacyUnknown" || len(b.Status.CaptureManifest.Volumes) != 0 { + t.Fatalf("invented history for potentially started copy: %+v", b.Status.CaptureManifest) + } + }) + } +} + func TestBackupConsistencyDescribesAchievedResult(t *testing.T) { for _, tc := range []struct { strategy, outcome, reason string diff --git a/operator/internal/controller/backup_snapshot_cleanup.go b/operator/internal/controller/backup_snapshot_cleanup.go new file mode 100644 index 000000000..6e07b7c0c --- /dev/null +++ b/operator/internal/controller/backup_snapshot_cleanup.go @@ -0,0 +1,215 @@ +package controller + +import ( + "context" + "encoding/json" + "fmt" + "strings" + "time" + + snapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + + gameplanev1alpha1 "github.com/GameplanePanel/gameplane/operator/api/v1alpha1" +) + +const backupSnapshotIdentityAnnotation = "backup.gameplane.local/snapshot-identity" +const backupSnapshotAbortMessageAnnotation = "backup.gameplane.local/snapshot-abort-reason" + +type backupSnapshotIdentity struct { + UID types.UID `json:"uid"` + Contents map[string]types.UID `json:"contents,omitempty"` +} + +func (r *BackupReconciler) rememberBackupSnapshot(ctx context.Context, b *gameplanev1alpha1.Backup, vs *snapshotv1.VolumeSnapshot) error { + var id backupSnapshotIdentity + if value := b.Annotations[backupSnapshotIdentityAnnotation]; value != "" { + if err := json.Unmarshal([]byte(value), &id); err != nil { + return fmt.Errorf("invalid snapshot identity: %w", err) + } + } + if !metav1.IsControlledBy(vs, b) || (id.UID != "" && id.UID != vs.UID) { + return fmt.Errorf("VolumeSnapshot %s/%s identity changed", vs.Namespace, vs.Name) + } + if id.UID == vs.UID { + return nil + } + id.UID = vs.UID + encoded, err := json.Marshal(id) + if err != nil { + return err + } + patchBackupAnnotations(b, map[string]string{backupSnapshotIdentityAnnotation: string(encoded)}) + return r.Update(ctx, b) +} + +func (r *BackupReconciler) recordSnapshotAbort(ctx context.Context, b *gameplanev1alpha1.Backup, message string) error { + if b.Annotations[backupSnapshotAbortAnnotation] != "true" { + patchBackupAnnotations(b, map[string]string{backupSnapshotAbortAnnotation: "true", backupSnapshotAbortMessageAnnotation: message}) + if err := r.Update(ctx, b); err != nil { + return err + } + } + if stored := b.Annotations[backupSnapshotAbortMessageAnnotation]; stored != "" { + message = stored + } + if b.Status.Phase == gameplanev1alpha1.BackupPhaseFailed && b.Status.SnapshotID == "" && b.Status.VolumeSnapshotContentName == "" && b.Status.Message == message { + return nil + } + b.Status.SnapshotID, b.Status.VolumeSnapshotContentName = "", "" + _, err := r.fail(ctx, b, message) + return err +} + +func (r *BackupReconciler) snapshotCleanupCondition(ctx context.Context, b *gameplanev1alpha1.Backup, status metav1.ConditionStatus, reason, message string) error { + conditions := upsertCondition(b.Status.Conditions, metav1.Condition{Type: "SnapshotCleaned", Status: status, Reason: reason, Message: message, ObservedGeneration: b.Generation}) + if sameConditions(b.Status.Conditions, conditions) { + return nil + } + b.Status.Conditions = conditions + if status != metav1.ConditionTrue { + r.recordEvent(b, corev1.EventTypeWarning, reason, message) + } + return r.Status().Update(ctx, b) +} + +// Cleanup is independent of save-on once the capture is durably invalidated. +// The original snapshot UID and each content UID are retained on the Backup; +// resource replacements are reported, never adopted or deleted. +func (r *BackupReconciler) cleanupAbortedSnapshot(ctx context.Context, b *gameplanev1alpha1.Backup) (ctrl.Result, error) { + res, err := r.reclaimAbortedSnapshot(ctx, b) + if err == nil { + return res, nil + } + if conditionErr := r.snapshotCleanupCondition(ctx, b, metav1.ConditionFalse, "SnapshotCleanupPending", err.Error()); conditionErr != nil { + return ctrl.Result{}, conditionErr + } + return ctrl.Result{RequeueAfter: 10 * time.Second}, nil +} + +func (r *BackupReconciler) reclaimAbortedSnapshot(ctx context.Context, b *gameplanev1alpha1.Backup) (ctrl.Result, error) { + var id backupSnapshotIdentity + if value := b.Annotations[backupSnapshotIdentityAnnotation]; value != "" { + if err := json.Unmarshal([]byte(value), &id); err != nil { + return ctrl.Result{}, err + } + } + var vs snapshotv1.VolumeSnapshot + err := r.backupAPIReader().Get(ctx, client.ObjectKeyFromObject(b), &vs) + if meta.IsNoMatchError(err) { + return ctrl.Result{}, fmt.Errorf("snapshot API unavailable; inspect VolumeSnapshot %s/%s and recorded identity %s", b.Namespace, b.Name, b.Annotations[backupSnapshotIdentityAnnotation]) + } + if err != nil && !apierrors.IsNotFound(err) { + return ctrl.Result{}, fmt.Errorf("read VolumeSnapshot %s/%s (recorded UID %s): %w", b.Namespace, b.Name, id.UID, err) + } + present := err == nil + if present { + if !metav1.IsControlledBy(&vs, b) || (id.UID != "" && id.UID != vs.UID) { + return ctrl.Result{}, r.snapshotCleanupCondition(ctx, b, metav1.ConditionFalse, "SnapshotCleanupUnverified", fmt.Sprintf("refusing replacement VolumeSnapshot %s/%s UID %s; original UID %s", vs.Namespace, vs.Name, vs.UID, id.UID)) + } + if id.UID == "" { + if vs.UID == "" { + return ctrl.Result{}, fmt.Errorf("VolumeSnapshot %s/%s has no verifiable UID", vs.Namespace, vs.Name) + } + if err := r.rememberBackupSnapshot(ctx, b, &vs); err != nil { + return ctrl.Result{}, err + } + id.UID = vs.UID + } + if vs.Annotations == nil { + vs.Annotations = map[string]string{} + } + if vs.Annotations[backupSnapshotAbortAnnotation] != "true" { + vs.Annotations[backupSnapshotAbortAnnotation] = "true" + if err := r.Update(ctx, &vs); err != nil { + return ctrl.Result{}, err + } + } + } + var contents snapshotv1.VolumeSnapshotContentList + if err := r.backupAPIReader().List(ctx, &contents); err != nil { + return ctrl.Result{}, fmt.Errorf("list contents for VolumeSnapshot %s/%s UID %s: %w", b.Namespace, b.Name, id.UID, err) + } + if id.Contents == nil { + id.Contents = map[string]types.UID{} + } + var retained []string + unverified := false + boundName := "" + if present && vs.Status != nil && vs.Status.BoundVolumeSnapshotContentName != nil { + boundName = *vs.Status.BoundVolumeSnapshotContentName + } + var owned []*snapshotv1.VolumeSnapshotContent + for i := range contents.Items { + content := &contents.Items[i] + ref := content.Spec.VolumeSnapshotRef + if known, ok := id.Contents[content.Name]; ok && known != content.UID { + unverified = true + retained = append(retained, fmt.Sprintf("replacement content %s UID %s (original %s)", content.Name, content.UID, known)) + continue + } + if id.UID == "" || ref.UID != id.UID || ref.Name != b.Name || ref.Namespace != b.Namespace { + if _, known := id.Contents[content.Name]; known || content.Name == boundName { + // Retain the rejected candidate's UID so deleting the valid + // snapshot cannot erase the administrator's follow-up identity. + id.Contents[content.Name] = content.UID + unverified = true + retained = append(retained, fmt.Sprintf("unverified content %s UID %s references snapshot %s/%s UID %s", content.Name, content.UID, ref.Namespace, ref.Name, ref.UID)) + } + continue + } + id.Contents[content.Name] = content.UID + if content.Spec.DeletionPolicy == snapshotv1.VolumeSnapshotContentRetain { + retained = append(retained, fmt.Sprintf("Retain content %s UID %s for snapshot UID %s", content.Name, content.UID, id.UID)) + } else { + owned = append(owned, content) + } + } + encoded, err := json.Marshal(id) + if err != nil { + return ctrl.Result{}, err + } + if string(encoded) != b.Annotations[backupSnapshotIdentityAnnotation] { + patchBackupAnnotations(b, map[string]string{backupSnapshotIdentityAnnotation: string(encoded)}) + if err := r.Update(ctx, b); err != nil { + return ctrl.Result{}, err + } + } + if present && vs.DeletionTimestamp.IsZero() { + uid := vs.UID + if err := r.Delete(ctx, &vs, &client.DeleteOptions{Preconditions: &metav1.Preconditions{UID: &uid}}); err != nil && !apierrors.IsNotFound(err) { + return ctrl.Result{}, fmt.Errorf("delete VolumeSnapshot %s/%s UID %s: %w", vs.Namespace, vs.Name, uid, err) + } + } + for _, content := range owned { + if !content.DeletionTimestamp.IsZero() { + continue + } + uid := content.UID + if uid == "" { + return ctrl.Result{}, fmt.Errorf("content %s has no verifiable UID", content.Name) + } + if err := r.Delete(ctx, content, &client.DeleteOptions{Preconditions: &metav1.Preconditions{UID: &uid}}); err != nil && !apierrors.IsNotFound(err) { + return ctrl.Result{}, fmt.Errorf("delete VolumeSnapshotContent %s UID %s for snapshot UID %s: %w", content.Name, uid, id.UID, err) + } + } + if len(retained) > 0 { + if unverified { + return ctrl.Result{}, r.snapshotCleanupCondition(ctx, b, metav1.ConditionFalse, "SnapshotCleanupUnverified", strings.Join(retained, "; ")+"; administrator verification required") + } + return ctrl.Result{}, r.snapshotCleanupCondition(ctx, b, metav1.ConditionFalse, "SnapshotCleanupRetained", strings.Join(retained, "; ")+"; administrator reclamation required") + } + if present || len(owned) > 0 { + if err := r.snapshotCleanupCondition(ctx, b, metav1.ConditionFalse, "SnapshotCleanupPending", fmt.Sprintf("deletion requested for snapshot %s/%s UID %s and contents %v", b.Namespace, b.Name, id.UID, id.Contents)); err != nil { + return ctrl.Result{}, err + } + return ctrl.Result{RequeueAfter: 10 * time.Second}, nil + } + return ctrl.Result{}, r.snapshotCleanupCondition(ctx, b, metav1.ConditionTrue, "SnapshotCleanupComplete", "aborted snapshot resources are absent") +} diff --git a/operator/internal/controller/backup_snapshot_source.go b/operator/internal/controller/backup_snapshot_source.go new file mode 100644 index 000000000..160619c22 --- /dev/null +++ b/operator/internal/controller/backup_snapshot_source.go @@ -0,0 +1,49 @@ +package controller + +import ( + "context" + "encoding/json" + "fmt" + + snapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + gameplanev1alpha1 "github.com/GameplanePanel/gameplane/operator/api/v1alpha1" +) + +// A raw GameServer dataSource must not revive an aborted operator capture while +// a CSI driver still exposes its object. Run only before creating a new PVC: +// existing volumes remain usable after retention deletes their source Backup. +// External snapshots without a Gameplane Backup owner keep the direct-CRD +// administrator contract; API authorization is enforced separately. +func validateBackupSnapshotSource(ctx context.Context, reader client.Reader, namespace, name string) error { + var snapshot snapshotv1.VolumeSnapshot + if err := reader.Get(ctx, client.ObjectKey{Namespace: namespace, Name: name}, &snapshot); err != nil { + return fmt.Errorf("resolve snapshot source: %w", err) + } + if snapshot.Annotations[backupSnapshotAbortAnnotation] == "true" { + return fmt.Errorf("snapshot %s was aborted and cannot seed a volume", name) + } + owner := metav1.GetControllerOf(&snapshot) + if owner == nil || owner.APIVersion != gameplanev1alpha1.GroupVersion.String() || owner.Kind != "Backup" { + return nil + } + var backup gameplanev1alpha1.Backup + if err := reader.Get(ctx, client.ObjectKey{Namespace: namespace, Name: owner.Name}, &backup); err != nil { + return fmt.Errorf("resolve snapshot Backup owner: %w", err) + } + if owner.UID == "" || owner.UID != backup.UID || !backup.DeletionTimestamp.IsZero() || !snapshot.DeletionTimestamp.IsZero() || + backup.Annotations[backupSnapshotAbortAnnotation] == "true" || backup.Annotations[backupCopyAbortAnnotation] != "" || + backup.Status.Phase != gameplanev1alpha1.BackupPhaseSucceeded || backup.Status.SnapshotID != snapshot.Name || + snapshot.Status == nil || snapshot.Status.ReadyToUse == nil || !*snapshot.Status.ReadyToUse { + return fmt.Errorf("snapshot %s is not a completed, valid capture of its current Backup owner", name) + } + if value := backup.Annotations[backupSnapshotIdentityAnnotation]; value != "" { + var id backupSnapshotIdentity + if err := json.Unmarshal([]byte(value), &id); err != nil || id.UID == "" || id.UID != snapshot.UID { + return fmt.Errorf("snapshot %s differs from the recorded capture UID", name) + } + } + return nil +} diff --git a/operator/internal/controller/backup_snapshot_source_test.go b/operator/internal/controller/backup_snapshot_source_test.go new file mode 100644 index 000000000..2daa8f82d --- /dev/null +++ b/operator/internal/controller/backup_snapshot_source_test.go @@ -0,0 +1,69 @@ +package controller + +import ( + "encoding/json" + "testing" + + snapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + gameplanev1alpha1 "github.com/GameplanePanel/gameplane/operator/api/v1alpha1" +) + +func TestSnapshotPVCRefusesIncompleteAbortedAndReplacedCapture(t *testing.T) { + for _, mode := range []string{"valid", "pending", "running", "failed", "abort-before-status", "copy-aborted", "snapshot-aborted", "owner-replaced", "snapshot-replaced", "not-ready", "external"} { + t.Run(mode, func(t *testing.T) { + b := guardBackup("snapshot") + b.Spec.Strategy = "volume-snapshot" + b.Status.Phase, b.Status.SnapshotID = gameplanev1alpha1.BackupPhaseSucceeded, b.Name + identity, err := json.Marshal(backupSnapshotIdentity{UID: "snapshot-uid"}) + if err != nil { + t.Fatal(err) + } + b.Annotations = map[string]string{backupSnapshotIdentityAnnotation: string(identity)} + vs := &snapshotv1.VolumeSnapshot{ObjectMeta: metav1.ObjectMeta{Name: b.Name, Namespace: b.Namespace, UID: "snapshot-uid", OwnerReferences: []metav1.OwnerReference{{APIVersion: gameplanev1alpha1.GroupVersion.String(), Kind: "Backup", Name: b.Name, UID: b.UID, Controller: ownerBoolPtr(true)}}}, Status: &snapshotv1.VolumeSnapshotStatus{ReadyToUse: ownerBoolPtr(true)}} + switch mode { + case "pending": + b.Status.Phase = gameplanev1alpha1.BackupPhasePending + case "running": + b.Status.Phase = gameplanev1alpha1.BackupPhaseRunning + case "failed": + b.Status.Phase = gameplanev1alpha1.BackupPhaseFailed + case "abort-before-status": + b.Annotations[backupSnapshotAbortAnnotation] = "true" + case "copy-aborted": + b.Annotations[backupCopyAbortAnnotation] = "cancelled" + case "snapshot-aborted": + vs.Annotations = map[string]string{backupSnapshotAbortAnnotation: "true"} + case "owner-replaced": + vs.OwnerReferences[0].UID = "previous-backup" + case "snapshot-replaced": + vs.UID = "replacement-snapshot" + case "not-ready": + vs.Status = nil + case "external": + vs.OwnerReferences = nil + vs.Status = nil + } + r, _ := guardFixture(t, b) + if err := r.Create(t.Context(), vs); err != nil { + t.Fatal(err) + } + gs := &gameplanev1alpha1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "restored", Namespace: b.Namespace, UID: "restored-uid"}, Spec: gameplanev1alpha1.GameServerSpec{Storage: &gameplanev1alpha1.GameStorageSpec{DataSource: &gameplanev1alpha1.GameDataSource{Kind: "VolumeSnapshot", Name: b.Name}}}} + gr := &GameServerReconciler{Client: r.Client, APIReader: r.Client, Scheme: r.Scheme} + err = gr.reconcilePVC(t.Context(), gs, &gameplanev1alpha1.GameTemplate{}) + allowed := mode == "valid" || mode == "external" + if (err == nil) != allowed { + t.Fatalf("PVC source validation mode=%s err=%v", mode, err) + } + var pvc corev1.PersistentVolumeClaim + getErr := r.Get(t.Context(), client.ObjectKey{Namespace: b.Namespace, Name: "restored-data"}, &pvc) + if !allowed && !apierrors.IsNotFound(getErr) { + t.Fatalf("invalid source created PVC: %v", getErr) + } + }) + } +} diff --git a/operator/internal/controller/backup_stopped_test.go b/operator/internal/controller/backup_stopped_test.go index eb07407b8..914752522 100644 --- a/operator/internal/controller/backup_stopped_test.go +++ b/operator/internal/controller/backup_stopped_test.go @@ -200,8 +200,12 @@ func TestBackupLiveTimeoutRetainsOwnerUntilOriginalPodGone(t *testing.T) { t.Fatalf("timeout did not fail closed: %+v", first.Status) } var job batchv1.Job - if err := r.Get(t.Context(), client.ObjectKey{Namespace: "ns", Name: "first"}, &job); !apierrors.IsNotFound(err) { - t.Fatal("live timeout created copy") + var gate corev1.ConfigMap + if err := r.Get(t.Context(), client.ObjectKey{Namespace: "ns", Name: backupStartConfigMapName(first)}, &gate); err != nil { + t.Fatal(err) + } + if gate.Data[backupStartTokenKey] != "" { + t.Fatal("live timeout authorized prepared worker") } var gs gameplanev1alpha1.GameServer if err := r.Get(t.Context(), client.ObjectKey{Namespace: "ns", Name: "server"}, &gs); err != nil { diff --git a/operator/internal/controller/backup_volumesnapshot.go b/operator/internal/controller/backup_volumesnapshot.go index e90a0b6c9..4eea866d1 100644 --- a/operator/internal/controller/backup_volumesnapshot.go +++ b/operator/internal/controller/backup_volumesnapshot.go @@ -2,11 +2,13 @@ package controller import ( "context" + "errors" "fmt" "time" snapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -29,7 +31,7 @@ func (r *BackupReconciler) reconcileVolumeSnapshot( ctx context.Context, b *gameplanev1alpha1.Backup, gs *gameplanev1alpha1.GameServer, ) (ctrl.Result, error) { if b.Annotations[backupSnapshotAbortAnnotation] == "true" { - return r.failVolumeSnapshot(ctx, b, "volume snapshot aborted after an API or creation failure") + return r.abortVolumeSnapshot(ctx, b, "volume snapshot capture was aborted") } // An uncached read verifies that the snapshot API is served before // disabling autosave. Missing CRDs (including NoMatch) and authorization @@ -38,22 +40,43 @@ func (r *BackupReconciler) reconcileVolumeSnapshot( if err := r.backupAPIReader().Get(ctx, types.NamespacedName{Namespace: b.Namespace, Name: b.Name}, &existing); err != nil && !apierrors.IsNotFound(err) { return r.abortVolumeSnapshot(ctx, b, fmt.Sprintf("volume snapshot API unavailable: %v", err)) } + if existing.Name != "" { + if err := r.rememberBackupSnapshot(ctx, b, &existing); err != nil { + return r.abortVolumeSnapshot(ctx, b, err.Error()) + } + } + if !legacyBackupOwesUnquiesce(b) { + if res, err := r.prepareBackupWorkload(ctx, b); err != nil || res.RequeueAfter > 0 { + return res, err + } + } + deadline, err := r.backupDeadline(ctx, b, backupCSIDeadlineAnnotation, backupTimeout(r.CSICaptureTimeout, DefaultBackupCSICaptureTimeout)) + if err != nil { + return r.abortVolumeSnapshot(ctx, b, err.Error()) + } + if !time.Now().Before(deadline) { + return r.abortVolumeSnapshot(ctx, b, "CSI capture deadline exceeded; incomplete snapshot invalidated") + } if legacyBackupOwesUnquiesce(b) && metav1.IsControlledBy(&existing, b) { return r.observeVolumeSnapshot(ctx, b, &existing) } - // Quiesce first (idempotent across requeues). A CSI snapshot is only - // crash-consistent; flushing the game to disk first (RCON save-all, - // etc.) makes it application-consistent. No-op when the backup opts out - // or no agent mTLS is configured. + // The durable capture budget starts before any save-off RPC. Quiesce + // confirmation describes the agent action, not universal game consistency. if res, err := r.prepareBackupQuiesce(ctx, b); err != nil || res.RequeueAfter > 0 { return res, err } + if !time.Now().Before(deadline) { + return r.abortVolumeSnapshot(ctx, b, "CSI capture deadline exceeded during quiesce") + } pvcName := gs.Name + "-data" vs := &snapshotv1.VolumeSnapshot{ ObjectMeta: metav1.ObjectMeta{Name: b.Name, Namespace: b.Namespace}, } if _, err := controllerutil.CreateOrUpdate(ctx, r.Client, vs, func() error { + if !vs.CreationTimestamp.IsZero() && !metav1.IsControlledBy(vs, b) { + return fmt.Errorf("VolumeSnapshot is not owned by this Backup") + } // Source + class are immutable on a VolumeSnapshot, so only set // them on first creation; SetControllerReference is idempotent. if vs.CreationTimestamp.IsZero() { @@ -71,16 +94,28 @@ func (r *BackupReconciler) reconcileVolumeSnapshot( // independently until save-on is confirmed. return r.abortVolumeSnapshot(ctx, b, fmt.Sprintf("volume snapshot creation failed: %v", err)) } + if err := r.rememberBackupSnapshot(ctx, b, vs); err != nil { + return r.abortVolumeSnapshot(ctx, b, err.Error()) + } return r.observeVolumeSnapshot(ctx, b, vs) } func (r *BackupReconciler) observeVolumeSnapshot(ctx context.Context, b *gameplanev1alpha1.Backup, vs *snapshotv1.VolumeSnapshot) (ctrl.Result, error) { + if b.Annotations[backupSnapshotAbortAnnotation] == "true" { + return r.abortVolumeSnapshot(ctx, b, "volume snapshot capture was aborted") + } + if value := b.Annotations[backupCSIDeadlineAnnotation]; value != "" { + deadline, err := time.Parse(time.RFC3339Nano, value) + if err != nil || !time.Now().Before(deadline) { + return r.abortVolumeSnapshot(ctx, b, "CSI capture deadline exceeded; late readiness is invalid") + } + } switch { case vs.Status != nil && vs.Status.Error != nil && vs.Status.Error.Message != nil: // The CSI driver rejected the snapshot. Surface it and unquiesce so // the game doesn't stay frozen. - return r.failVolumeSnapshot(ctx, b, fmt.Sprintf("volume snapshot failed: %s", *vs.Status.Error.Message)) + return r.abortVolumeSnapshot(ctx, b, fmt.Sprintf("volume snapshot failed: %s", *vs.Status.Error.Message)) case vs.Status != nil && vs.Status.ReadyToUse != nil && *vs.Status.ReadyToUse: return r.completeVolumeSnapshot(ctx, b, vs) default: @@ -93,11 +128,15 @@ func (r *BackupReconciler) observeVolumeSnapshot(ctx context.Context, b *gamepla // created a snapshot. Persist that abort decision before terminal cleanup so // worker-drain checks cannot trap this failed/nonrestorable Backup forever. func (r *BackupReconciler) abortVolumeSnapshot(ctx context.Context, b *gameplanev1alpha1.Backup, msg string) (ctrl.Result, error) { - patchBackupAnnotations(b, map[string]string{backupSnapshotAbortAnnotation: "true"}) - if err := r.Update(ctx, b); err != nil { + if err := r.recordSnapshotAbort(ctx, b, msg); err != nil { return ctrl.Result{}, err } - return r.failVolumeSnapshot(ctx, b, msg) + unquiesce, unquiesceErr := r.runUnquiesce(ctx, b) + cleanup, cleanupErr := r.cleanupAbortedSnapshot(ctx, b) + if unquiesce.RequeueAfter > 0 && (cleanup.RequeueAfter == 0 || unquiesce.RequeueAfter < cleanup.RequeueAfter) { + cleanup = unquiesce + } + return cleanup, errors.Join(unquiesceErr, cleanupErr) } // markVolumeSnapshotRunning flips the Backup to Running on first observation @@ -124,6 +163,9 @@ func (r *BackupReconciler) markVolumeSnapshotRunning( return ctrl.Result{}, err } } + if err := r.setBackupProgress(ctx, b, "SnapshotCapturePending", "waiting for CSI capture; deadline "+b.Annotations[backupCSIDeadlineAnnotation]); err != nil { + return ctrl.Result{}, err + } return ctrl.Result{RequeueAfter: 10 * time.Second}, nil } @@ -132,9 +174,13 @@ func (r *BackupReconciler) markVolumeSnapshotRunning( func (r *BackupReconciler) completeVolumeSnapshot( ctx context.Context, b *gameplanev1alpha1.Backup, vs *snapshotv1.VolumeSnapshot, ) (ctrl.Result, error) { + if b.Annotations[backupSnapshotAbortAnnotation] == "true" { + return r.abortVolumeSnapshot(ctx, b, "volume snapshot capture was aborted") + } if b.Status.Phase != gameplanev1alpha1.BackupPhaseSucceeded { now := metav1.Now() b.Status.Phase = gameplanev1alpha1.BackupPhaseSucceeded + meta.RemoveStatusCondition(&b.Status.Conditions, "CaptureReady") b.Status.ObservedGeneration = b.Generation b.Status.SnapshotID = vs.Name if vs.Status.BoundVolumeSnapshotContentName != nil { @@ -167,14 +213,3 @@ func (r *BackupReconciler) completeVolumeSnapshot( } return r.runUnquiesce(ctx, b) } - -// failVolumeSnapshot marks the Backup Failed and still runs the terminal -// unquiesce so a quiesced game doesn't stay frozen after a snapshot error. -func (r *BackupReconciler) failVolumeSnapshot( - ctx context.Context, b *gameplanev1alpha1.Backup, msg string, -) (ctrl.Result, error) { - if _, err := r.fail(ctx, b, msg); err != nil { - return ctrl.Result{}, err - } - return r.runUnquiesce(ctx, b) -} diff --git a/operator/internal/controller/backup_volumesnapshot_envtest_test.go b/operator/internal/controller/backup_volumesnapshot_envtest_test.go index 6e8bb7e01..d182eb696 100644 --- a/operator/internal/controller/backup_volumesnapshot_envtest_test.go +++ b/operator/internal/controller/backup_volumesnapshot_envtest_test.go @@ -9,8 +9,10 @@ import ( "testing" "time" + snapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" gameplanev1alpha1 "github.com/GameplanePanel/gameplane/operator/api/v1alpha1" @@ -210,6 +212,10 @@ func TestRestore_VolumeSnapshotRejectsExistingTarget(t *testing.T) { func TestGameServer_DataSourceSeedsPVC(t *testing.T) { ns := newNamespace(t) startMgr(t, ns, withGameServerReconciler(t, ns)) + sourcePVC := "external-source-data" + if err := k8sClient.Create(context.Background(), &snapshotv1.VolumeSnapshot{ObjectMeta: metav1.ObjectMeta{Name: "snap-1", Namespace: ns}, Spec: snapshotv1.VolumeSnapshotSpec{Source: snapshotv1.VolumeSnapshotSource{PersistentVolumeClaimName: &sourcePVC}}}); err != nil { + t.Fatal(err) + } tmpl := buildGameTemplate(uniqueName("minecraft")) if err := k8sClient.Create(context.Background(), tmpl); err != nil { diff --git a/operator/internal/controller/backup_worker.go b/operator/internal/controller/backup_worker.go new file mode 100644 index 000000000..b90074b41 --- /dev/null +++ b/operator/internal/controller/backup_worker.go @@ -0,0 +1,330 @@ +package controller + +import ( + "context" + "crypto/sha256" + "encoding/json" + "errors" + "fmt" + "strconv" + "time" + + appsv1 "k8s.io/api/apps/v1" + batchv1 "k8s.io/api/batch/v1" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "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/controllerutil" + + gameplanev1alpha1 "github.com/GameplanePanel/gameplane/operator/api/v1alpha1" +) + +const ( + backupWorkerSourceAnnotation = "backup.gameplane.local/worker-source" + backupAuthorizedWorkerAnnotation = "backup.gameplane.local/authorized-worker" + backupStartTokenKey = "start" + backupWorkerProtocolAnnotation = "backup.gameplane.local/worker-protocol" +) + +// The waiting worker never reads source files until the operator authorizes +// this exact Backup/pod/source generation. Repository preparation takes a +// shared restic lock; later lock contention is bounded by the copy timeout. +// Values and tags are arguments/environment, never interpolated shell code. +const backupWorkerScript = `set -eu +for utility in timeout date cat touch sleep restic; do command -v "$utility" >/dev/null; done +timeout -s TERM -k 5 1 /bin/sh -c ':' +test -d /data && test -r /data && test -x /data +restic snapshots --json --retry-lock 10m >/dev/null +touch /tmp/prepared +while :; do + now=$(date +%s) + if [ -r /start/start ]; then + token=$(cat /start/start) + if [ -n "$token" ]; then + read -r version backup pod source expires extra < 0 { + return res, err + } + return r.mirrorJobStatus(ctx, b, &job) + } + // Queueing behind another lifecycle owner is not worker preparation and + // must not consume its budget before this Backup can start preparing. + if b.Annotations[backupPreparationDeadlineAnnotation] == "" { + claimed, err := r.claimBackupTarget(ctx, b) + if err != nil { + if errors.Is(err, errLifecycleGenerationChanged) { + return r.abortResticCopy(ctx, b, err.Error()) + } + return ctrl.Result{}, err + } + if !claimed { + return ctrl.Result{RequeueAfter: 10 * time.Second}, nil + } + } + preparation, err := r.backupDeadline(ctx, b, backupPreparationDeadlineAnnotation, backupTimeout(r.PreparationTimeout, DefaultBackupPreparationTimeout)) + if err != nil { + return r.abortResticCopy(ctx, b, err.Error()) + } + if b.Annotations[backupAuthorizedWorkerAnnotation] == "" && time.Now().After(preparation) { + return r.abortResticCopy(ctx, b, "worker preparation deadline exceeded before copy authorization") + } + if value := b.Annotations[backupQuiesceDeadlineAnnotation]; value != "" { + deadline, err := time.Parse(time.RFC3339Nano, value) + if err != nil { + return r.abortResticCopy(ctx, b, "invalid persisted quiesce deadline") + } + finishedInTime := job.Status.Succeeded > 0 && job.Status.CompletionTime != nil && !job.Status.CompletionTime.After(deadline) + if time.Now().After(deadline) && !finishedInTime { + return r.abortResticCopy(ctx, b, "maximum quiesce duration exceeded; copy invalidated") + } + } + if res, err := r.prepareBackupWorkload(ctx, b); err != nil || res.RequeueAfter > 0 { + return res, err + } + source, err := r.prepareBackupWorkerSource(ctx, b) + if err != nil { + return r.abortResticCopy(ctx, b, err.Error()) + } + gate := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: backupStartConfigMapName(b), Namespace: b.Namespace}} + if _, err := controllerutil.CreateOrUpdate(ctx, r.Client, gate, func() error { + if !gate.CreationTimestamp.IsZero() && !metav1.IsControlledBy(gate, b) { + return fmt.Errorf("copy authorization ConfigMap is not owned by this Backup") + } + if gate.Data == nil { + gate.Data = map[string]string{backupStartTokenKey: ""} + } + return controllerutil.SetControllerReference(b, gate, r.Scheme) + }); err != nil { + return ctrl.Result{}, err + } + if job.Name == "" { + backoff, deadline := resolveJobLimits(r.JobBackoffLimit, r.JobActiveDeadlineSeconds) + job = batchv1.Job{ObjectMeta: metav1.ObjectMeta{Name: b.Name, Namespace: b.Namespace, Annotations: map[string]string{backupWorkerProtocolAnnotation: "v1"}}, Spec: batchv1.JobSpec{BackoffLimit: &backoff, ActiveDeadlineSeconds: &deadline, + Template: corev1.PodTemplateSpec{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{backupRestoreJobLabel: backupRestoreJobValue, backupUIDLabel: string(b.UID)}}, Spec: r.buildPreparedBackupPodSpec(b, tmpl, source, preparation)}}} + job.Spec.Template.Spec.RestartPolicy = corev1.RestartPolicyNever + if err := controllerutil.SetControllerReference(b, &job, r.Scheme); err != nil { + return ctrl.Result{}, err + } + if err := r.Create(ctx, &job); err != nil { + return ctrl.Result{}, err + } + } + if jobPermanentlyFailed(&job) { + return r.abortResticCopy(ctx, b, "backup worker reported Failed") + } + if job.Status.Succeeded > 0 { + if b.Annotations[backupAuthorizedWorkerAnnotation] == "" { + return r.abortResticCopy(ctx, b, "backup worker completed without copy authorization") + } + return r.mirrorJobStatus(ctx, b, &job) + } + worker, err := r.preparedBackupWorker(ctx, b, &job) + if err != nil { + return ctrl.Result{}, err + } + if worker == nil { + if err := r.setBackupProgress(ctx, b, "WorkerPreparing", "waiting for a scheduled worker with mounted source and validated repository"); err != nil { + return ctrl.Result{}, err + } + return ctrl.Result{RequeueAfter: 2 * time.Second}, nil + } + // Raw and already-stopped copies have no save-off window. Their tokens use + // the full Job deadline, preserving long-copy support independently. + _, copySeconds := resolveJobLimits(r.JobBackoffLimit, r.JobActiveDeadlineSeconds) + if job.Spec.ActiveDeadlineSeconds != nil && *job.Spec.ActiveDeadlineSeconds > 0 { + copySeconds = *job.Spec.ActiveDeadlineSeconds + } + const maxDurationSeconds = int64((1<<63 - 1) / time.Second) + if copySeconds > maxDurationSeconds { + copySeconds = maxDurationSeconds + } + copyBudget := time.Duration(copySeconds) * time.Second + protected := b.Spec.Quiesce && agentQuiesceAvailable(r.AgentClient) + if prepared, err := parseBackupPreparation(b); err != nil { + return ctrl.Result{}, err + } else if prepared != nil && prepared.Outcome == "already-stopped" { + protected = false + } + deadlineKey := backupQuiesceDeadlineAnnotation + if protected { + copyBudget = backupTimeout(r.QuiesceTimeout, DefaultBackupQuiesceTimeout) + } + deadline, err := r.backupDeadline(ctx, b, deadlineKey, copyBudget) + if err != nil { + return ctrl.Result{}, err + } + if res, err := r.prepareBackupQuiesce(ctx, b); err != nil || res.RequeueAfter > 0 { + return res, err + } + if _, err := r.prepareBackupWorkerSource(ctx, b); err != nil { + return r.abortResticCopy(ctx, b, err.Error()) + } + // Re-read readiness after the RPC. A replacement worker cannot inherit + // the first pod's authorization, including during a Job retry. + current, err := r.preparedBackupWorker(ctx, b, &job) + if err != nil { + return ctrl.Result{}, err + } + if current == nil || current.UID != worker.UID { + return ctrl.Result{RequeueAfter: 2 * time.Second}, nil + } + if !time.Now().Before(deadline) { + return r.abortResticCopy(ctx, b, "maximum quiesce duration exceeded before start authorization") + } + if b.Annotations[backupAuthorizedWorkerAnnotation] != string(worker.UID) { + patchBackupAnnotations(b, map[string]string{backupAuthorizedWorkerAnnotation: string(worker.UID)}) + if err := r.Update(ctx, b); err != nil { + return ctrl.Result{}, err + } + } + token := fmt.Sprintf("v1 %s %s %s %d\n", b.UID, worker.UID, backupWorkerSourceID(b), deadline.Unix()) + if gate.Data[backupStartTokenKey] != token { + gate.Data[backupStartTokenKey] = token + if err := r.Update(ctx, gate); err != nil { + return ctrl.Result{}, err + } + } + if err := r.setBackupProgress(ctx, b, "WorkerAuthorized", "prepared worker authorized; copy deadline "+deadline.UTC().Format(time.RFC3339)); err != nil { + return ctrl.Result{}, err + } + return r.mirrorJobStatus(ctx, b, &job) +} diff --git a/operator/internal/controller/gameserver_controller.go b/operator/internal/controller/gameserver_controller.go index 5f681a789..d6aecf5e8 100644 --- a/operator/internal/controller/gameserver_controller.go +++ b/operator/internal/controller/gameserver_controller.go @@ -574,6 +574,13 @@ func (r *GameServerReconciler) reconcilePVC( // size is >= the snapshot's restoreSize by construction (the // snapshot came from a PVC of that size). if gs.Spec.Storage != nil && gs.Spec.Storage.DataSource != nil { + reader := r.APIReader + if reader == nil { + reader = r.Client + } + if err := validateBackupSnapshotSource(ctx, reader, gs.Namespace, gs.Spec.Storage.DataSource.Name); err != nil { + return err + } apiGroup := "snapshot.storage.k8s.io" pvc.Spec.DataSource = &corev1.TypedLocalObjectReference{ APIGroup: &apiGroup, diff --git a/operator/internal/controller/helpers_envtest_test.go b/operator/internal/controller/helpers_envtest_test.go index fbb6dccb0..1e4dcfd78 100644 --- a/operator/internal/controller/helpers_envtest_test.go +++ b/operator/internal/controller/helpers_envtest_test.go @@ -469,6 +469,25 @@ func patchJobStatus(t *testing.T, ns, name string, mut func(s *batchv1.JobStatus t.Fatalf("get job %s/%s: %v", ns, name, err) } mut(&job.Status) + if job.Annotations[backupWorkerProtocolAnnotation] == "v1" { + if job.Status.Succeeded > 0 || job.Status.Active > 0 { + prepareEnvtestBackupWorker(t, &job) + } + if job.Status.Succeeded > 0 || job.Status.Failed > 0 { + var worker corev1.Pod + if err := k8sClient.Get(context.Background(), client.ObjectKey{Namespace: ns, Name: name + "-test-worker"}, &worker); err == nil { + worker.Status.Phase = corev1.PodSucceeded + if job.Status.Failed > 0 { + worker.Status.Phase = corev1.PodFailed + } + if err := k8sClient.Status().Update(context.Background(), &worker); err != nil { + t.Fatal(err) + } + } else if !apierrors.IsNotFound(err) { + t.Fatal(err) + } + } + } now := metav1.Now() @@ -495,6 +514,34 @@ func patchJobStatus(t *testing.T, ns, name string, mut func(s *batchv1.JobStatus } } +// envtest has neither Job controller nor kubelet. This models their actual +// readiness contract and waits for operator authorization before reporting a +// successful copy. It never grants a token itself. +func prepareEnvtestBackupWorker(t *testing.T, job *batchv1.Job) { + t.Helper() + ctx := context.Background() + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: job.Name + "-test-worker", Namespace: job.Namespace, Labels: job.Spec.Template.Labels, + OwnerReferences: []metav1.OwnerReference{{APIVersion: "batch/v1", Kind: "Job", Name: job.Name, UID: job.UID, Controller: ownerBoolPtr(true)}}}, Spec: job.Spec.Template.Spec} + pod.Spec.NodeName = "test-node" + if err := k8sClient.Create(ctx, pod); err != nil { + if !apierrors.IsAlreadyExists(err) { + t.Fatal(err) + } + if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(pod), pod); err != nil { + t.Fatal(err) + } + } + pod.Status.Phase = corev1.PodRunning + pod.Status.Conditions = []corev1.PodCondition{{Type: corev1.PodReady, Status: corev1.ConditionTrue}} + if err := k8sClient.Status().Update(ctx, pod); err != nil { + t.Fatal(err) + } + eventually(t, func() (bool, string) { + backup := getBackup(t, job.Namespace, job.Name) + return backup.Annotations[backupAuthorizedWorkerAnnotation] == string(pod.UID), "prepared worker is not yet authorized: " + describeBackupStatus(backup) + }) +} + // upsertJobCondition sets/replaces the JobCondition of type t to // Status: True in conds, returning the updated slice. Unlike // upsertCondition (helpers.go), which operates on the CRD condition diff --git a/operator/internal/controller/restore_controller.go b/operator/internal/controller/restore_controller.go index a67a4447d..3d0417015 100644 --- a/operator/internal/controller/restore_controller.go +++ b/operator/internal/controller/restore_controller.go @@ -80,13 +80,13 @@ func (r *RestoreReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct // the rug out from under us mid-restore. if rs.Status.SnapshotID == "" { var src gameplanev1alpha1.Backup - if err := r.Get(ctx, types.NamespacedName{Name: rs.Spec.BackupRef.Name, Namespace: rs.Namespace}, &src); err != nil { + if err := r.apiReader().Get(ctx, types.NamespacedName{Name: rs.Spec.BackupRef.Name, Namespace: rs.Namespace}, &src); err != nil { if apierrors.IsNotFound(err) { return r.fail(ctx, &rs, fmt.Sprintf("source backup %q not found", rs.Spec.BackupRef.Name)) } return ctrl.Result{}, err } - if src.Status.Phase == gameplanev1alpha1.BackupPhaseFailed { + if src.Status.Phase == gameplanev1alpha1.BackupPhaseFailed || src.Annotations[backupSnapshotAbortAnnotation] == "true" || src.Annotations[backupCopyAbortAnnotation] != "" { // Terminal: a Failed backup will never grow a snapshotID, so // waiting would leave the Restore Pending forever. return r.fail(ctx, &rs, fmt.Sprintf( @@ -125,7 +125,7 @@ func (r *RestoreReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct // touch an existing server — they provision a new one seeded from the // CSI snapshot — so they branch off here, before the suspend/Job flow. var src gameplanev1alpha1.Backup - if err := r.Get(ctx, types.NamespacedName{Name: rs.Spec.BackupRef.Name, Namespace: rs.Namespace}, &src); err != nil { + if err := r.apiReader().Get(ctx, types.NamespacedName{Name: rs.Spec.BackupRef.Name, Namespace: rs.Namespace}, &src); err != nil { if apierrors.IsNotFound(err) { return r.fail(ctx, &rs, fmt.Sprintf("source backup %q disappeared", rs.Spec.BackupRef.Name)) } @@ -135,6 +135,9 @@ func (r *RestoreReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct // preparation path checks that binding before awaiting startup, repairing // references and enforcing the deadline on every subsequent reconcile. if src.Spec.Strategy == "volume-snapshot" { + if src.Annotations[backupSnapshotAbortAnnotation] == "true" || src.Status.Phase != gameplanev1alpha1.BackupPhaseSucceeded || src.Status.SnapshotID != rs.Status.SnapshotID { + return r.fail(ctx, &rs, "source CSI capture is no longer valid") + } return r.reconcileVolumeSnapshotRestore(ctx, &rs, &src) } diff --git a/operator/specs.md b/operator/specs.md index 7762c4d2e..da629cbc1 100644 --- a/operator/specs.md +++ b/operator/specs.md @@ -231,14 +231,16 @@ Primary reconcilers register with the manager in `cmd/main.go` and handle CRD li ### BackupReconciler - **Responsibility:** Drive Backup to completion: coordinate quiesce (if agent available), spawn restic Job, track snapshot ID. - **Key functions:** - - Quiesce orchestration: call agent Quiesce before Job, Unquiesce after completion. + - Restic preparation precedes quiesce: claim/fence the source, persist a preparation deadline, create the waiting worker, and require an uncached Running/Ready pod controlled by the exact Job UID. The worker mounts the PVC, initializes/probes the repository with a shared lock, then waits for an owned ConfigMap start token. Only after readiness does the operator persist the quiesce deadline, perform the pod-bound RPC and source postcheck, record the capture manifest, and grant the Backup UID/worker UID/source-fingerprint/expiry-bound token. Replacement pods cannot reuse it. Workers have no service-account token, use the shared backup resource settings and source tolerations, and prefer the observed game node; hard same-node affinity is opt-in. + - 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. - 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. - Source proof is checked before and after quiesce and on each copy reconcile. 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. - 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. - Every copy, including `quiesce=false` and CSI snapshots, acquires the shared lifecycle owner before starting. The Backup pins its target in `backup.gameplane.local/target`; `backup.gameplane.local/owner` remains a compatibility marker. The shared owner is described below. Quiesce cleanup intent is recorded on both the Backup and the surviving GameServer before calling the agent, including partial failures. Cleanup waits for live copy workers; deletion cancels the copy Job with foreground propagation first. Every Backup carries the cleanup finalizer. Pre-upgrade quiesced workers are observed through completion even when overlapping legacy copies cannot acquire the new exclusive owner. - - Orphan recovery preserves predecessor operation identities in the shared owner's `recovery` array, atomically with ownership transfer. Existing `backup.gameplane.local/recovering-owners` lineage is imported during migration. Deleting a recovering owner cannot hide an earlier worker. Normal deletion, orphan recovery, and legacy cleanup also wait for pending owned CSI captures to report ready or an explicit error; VolumeSnapshots are reclaimed by owner-reference GC after cleanup. - - VolumeSnapshot API availability is checked before quiescing. Snapshot creation errors fail the Backup and run unquiesce cleanup immediately instead of retrying creation indefinitely with saving disabled. API/creation failures persist `backup.gameplane.local/snapshot-aborted` to allow cleanup without waiting for a failed/nonrestorable capture. Failed unquiesce remains visible and retried. A transport failure can leave a CSI snapshot object despite a Failed Backup; this does not make that Backup restorable. + - Orphan recovery preserves predecessor operation identities in the shared owner's `recovery` array, atomically with ownership transfer. Existing `backup.gameplane.local/recovering-owners` lineage is imported during migration. Deleting a recovering owner cannot hide an earlier worker. Normal CSI observation waits for ready/error, but cancellation durably invalidates incomplete captures before save-on. Force-deleted Backup recovery marks the exact owned snapshot aborted and requests UID-bound deletion; a missing Backup cannot expose that snapshot as a valid Gameplane capture. + - CSI capture has a persisted deadline before quiesce, default 10m (`--backup-csi-capture-timeout`, Helm `operator.backupCSICaptureTimeout`), independent of Restic. API/creation errors, deadline expiry and deletion persist `backup.gameplane.local/snapshot-aborted` as a terminal decision, clear snapshot identity and fail the Backup before save-on. Late readiness cannot revive it. Snapshot/content identity is retained separately for reclamation. Cleanup verifies snapshot ownership and recorded UID, content reference UID/name/namespace and content UID, and deletes with UID preconditions. `Retain` or unverified resources remain for administrator action with names/UIDs in `SnapshotCleaned` conditions/events. Cleanup errors retry independently of save-on; deletion retains the Backup finalizer for pending reclamation but does not retain the game owner after confirmed save-on. Cluster RBAC adds content delete for verified `Delete`-policy resources. + - New PVCs referencing a Gameplane Backup-owned VolumeSnapshot require uncached successful-owner/snapshotID/UID/readiness checks and reject abort markers, including raw GameServer dataSource references. Existing PVCs do not depend on retained source metadata. External snapshots without a Gameplane Backup owner preserve the administrator direct-CRD contract; API authorization is separate. - Restic Job: mount data PVC, run restic backup to configured destination (S3, B2, rest-server, local fs). Job carries BackoffLimit and ActiveDeadlineSeconds from the reconciler's JobBackoffLimit / JobActiveDeadlineSeconds fields (operator flags --backup-job-backoff-limit / --backup-job-active-deadline-seconds, Helm operator.backupJobBackoffLimit / operator.backupJobActiveDeadlineSeconds; defaults 2 and 86400, applied when unset; spec 018 OD-027); only a Failed=True Job condition (set by jobPermanentlyFailed check) marks the Backup Failed. Pod SecurityContext takes FSGroup from the target GameTemplate's spec.security.fsGroup (fallback 65532 if template missing). - Snapshot tracking: parse restic JSON output from container logs to extract snapshot ID. - Capture metadata: after ownership, workload fencing and quiesce admission, record `status.captureManifest` once before creating the first copy worker. It pins the GameServer UID, strategy, observed Pod/StatefulSet mount layout, PVC names/UIDs, included primary claim and excluded extra/mod/retained claims. `storage.extra[].backupRole` optionally declares `data` or `rebuildable`; absent declarations remain unclassified and excluded. Later template edits never rewrite the manifest. Existing workers and old terminal Backups have unknown historical coverage, never reconstructed from the current template. @@ -436,6 +438,10 @@ different copy under the old owner. See [operator operation recovery](../docs/op | `--agent-image-pull-policy` | string | `` | ImagePullPolicy override (Always/IfNotPresent/Never); empty leaves unset. | | `--config-init-image` | string | `controller.DefaultConfigInitImage` | Init container for rendering config files onto data volume. | | `--restic-image` | string | `controller.DefaultResticImage` | restic backup/restore Job image. | +| `--backup-preparation-timeout` | duration | `10m` | Maximum preparation before copy authorization. | +| `--backup-quiesce-timeout` | duration | `5m` | Live quiesced Restic copy budget; drainage precedes save-on. | +| `--backup-csi-capture-timeout` | duration | `10m` | CSI capture budget before terminal invalidation. | +| `--backup-hard-same-node` | bool | `false` | Require the observed game node instead of preferring it. | | `--backup-job-backoff-limit` | int64 | 2 | BackoffLimit of restic backup/restore Jobs; must be >= 0. | | `--backup-job-active-deadline-seconds` | int64 | 86400 | ActiveDeadlineSeconds of restic backup/restore Jobs; must be > 0. | | `--sentinel-image` | string | `controller.DefaultSentinelImage` | Wake-on-connect sentinel container image. | diff --git a/test/e2e/backup_preparation_e2e_test.go b/test/e2e/backup_preparation_e2e_test.go new file mode 100644 index 000000000..c1f4d8070 --- /dev/null +++ b/test/e2e/backup_preparation_e2e_test.go @@ -0,0 +1,158 @@ +//go:build e2e + +package e2e + +import ( + "context" + "fmt" + "regexp" + "testing" + "time" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" +) + +// Real scheduler, RWO PVC and repository failure/recovery on two schedulable +// nodes. Kind's local-path volume supplies node affinity; attachable CSI +// drivers still need their own multi-attach qualification. Zero API logins. +func TestBackup_PreparationAndSameNodeCopy(t *testing.T) { + ctx := context.Background() + nodes, err := envInstance.K8s.CoreV1().Nodes().List(ctx, metav1.ListOptions{}) + if err != nil { + t.Fatal(err) + } + eligible := 0 + for _, node := range nodes.Items { + ready, tainted := false, node.Spec.Unschedulable + for _, condition := range node.Status.Conditions { + ready = ready || (condition.Type == corev1.NodeReady && condition.Status == corev1.ConditionTrue) + } + for _, taint := range node.Spec.Taints { + tainted = tainted || taint.Effect == corev1.TaintEffectNoSchedule || taint.Effect == corev1.TaintEffectNoExecute + } + if ready && !tainted { + eligible++ + } + } + if eligible < 2 { + t.Fatal("backup preparation requires the management Kind profile with two schedulable workers") + } + ns, gs, template, name := "gameplane-games", "e2e-prepared-source", "e2e-prepared-template", "e2e-prepared-copy" + ensureResticRepo(t) + applyBusyboxTemplate(t, template) + applyBusyboxGameServer(t, ns, gs, template) + waitPVCBound(t, ns, gs+"-data", 90*time.Second) + var sourceNode string + envInstance.Eventually(t, 2*time.Minute, func() (bool, string) { + pod, err := envInstance.K8s.CoreV1().Pods(ns).Get(ctx, gs+"-0", metav1.GetOptions{}) + if err != nil { + return false, err.Error() + } + for _, status := range pod.Status.ContainerStatuses { + if status.Name == "game" && status.Ready { + sourceNode = pod.Spec.NodeName + return sourceNode != "", "source has no node" + } + } + return false, "game container not ready" + }) + pvc, err := envInstance.K8s.CoreV1().PersistentVolumeClaims(ns).Get(ctx, gs+"-data", metav1.GetOptions{}) + if err != nil || len(pvc.Spec.AccessModes) != 1 || pvc.Spec.AccessModes[0] != corev1.ReadWriteOnce { + t.Fatalf("fixture is not RWO: %v %+v", err, pvc) + } + if out, err := envInstance.KubectlExec(t, ns, "pod/"+gs+"-0", "sh", "-c", "echo prepared-copy-marker > /data/prepared-marker.txt && sync"); err != nil { + t.Fatalf("write marker: %v %s", err, out) + } + repo, err := envInstance.K8s.CoreV1().Secrets(ns).Get(ctx, "e2e-restic-creds", metav1.GetOptions{}) + if err != nil { + t.Fatal(err) + } + repo = repo.DeepCopy() + repo.ObjectMeta = metav1.ObjectMeta{Name: "e2e-prepared-repo", Namespace: ns} + validURL := append([]byte(nil), repo.Data["repo"]...) + repo.Data["repo"] = []byte("rest:http://127.0.0.1:1") + if _, err := envInstance.K8s.CoreV1().Secrets(ns).Create(ctx, repo, metav1.CreateOptions{}); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = envInstance.K8s.CoreV1().Secrets(ns).Delete(ctx, repo.Name, metav1.DeleteOptions{}) }) + backup := &unstructured.Unstructured{Object: map[string]any{"apiVersion": "gameplane.local/v1alpha1", "kind": "Backup", "metadata": map[string]any{"name": name, "namespace": ns}, + "spec": map[string]any{"serverRef": map[string]any{"name": gs}, "repoRef": map[string]any{"name": repo.Name, "key": "repo"}, "quiesce": true}}} + if _, err := envInstance.Dyn.Resource(backupGVR).Namespace(ns).Create(ctx, backup, metav1.CreateOptions{}); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { deleteBackupCR(t, ns, name) }) + var failedWorker *corev1.Pod + envInstance.Eventually(t, 2*time.Minute, func() (bool, string) { + got, err := envInstance.Dyn.Resource(backupGVR).Namespace(ns).Get(ctx, name, metav1.GetOptions{}) + if err != nil { + return false, err.Error() + } + if got.GetAnnotations()["backup.gameplane.local/quiesce-attempted"] != "" || got.GetAnnotations()["backup.gameplane.local/authorized-worker"] != "" { + t.Fatal("repository preparation failure disabled saving or authorized copy") + } + pods, err := envInstance.K8s.CoreV1().Pods(ns).List(ctx, metav1.ListOptions{LabelSelector: "job-name=" + name}) + if err != nil { + return false, err.Error() + } + for i := range pods.Items { + pod := &pods.Items[i] + if pod.Spec.NodeName != "" && pod.Spec.NodeName != sourceNode { + t.Fatalf("RWO worker scheduled away from source: %s vs %s", pod.Spec.NodeName, sourceNode) + } + for _, status := range pod.Status.InitContainerStatuses { + if status.Name == "restic-init" && ((status.State.Terminated != nil && status.State.Terminated.ExitCode != 0) || (status.LastTerminationState.Terminated != nil && status.LastTerminationState.Terminated.ExitCode != 0)) { + failedWorker = pod + return true, "" + } + } + } + return false, "waiting for actual repository preparation failure" + }) + repo, err = envInstance.K8s.CoreV1().Secrets(ns).Get(ctx, repo.Name, metav1.GetOptions{}) + if err != nil { + t.Fatal(err) + } + repo.Data["repo"] = validURL + if _, err := envInstance.K8s.CoreV1().Secrets(ns).Update(ctx, repo, metav1.UpdateOptions{}); err != nil { + t.Fatal(err) + } + uid := failedWorker.UID + if err := envInstance.K8s.CoreV1().Pods(ns).Delete(ctx, failedWorker.Name, metav1.DeleteOptions{Preconditions: &metav1.Preconditions{UID: &uid}}); err != nil { + t.Fatal(err) + } + var snapshotID string + envInstance.Eventually(t, 5*time.Minute, func() (bool, string) { + got, err := envInstance.Dyn.Resource(backupGVR).Namespace(ns).Get(ctx, name, metav1.GetOptions{}) + if err != nil { + return false, err.Error() + } + phase, _, _ := unstructured.NestedString(got.Object, "status", "phase") + if phase == "Failed" { + t.Fatalf("copy failed after repository recovery: %+v", got.Object["status"]) + } + snapshotID, _, _ = unstructured.NestedString(got.Object, "status", "snapshotID") + return phase == "Succeeded" && snapshotID != "", "phase=" + phase + }) + pods, err := envInstance.K8s.CoreV1().Pods(ns).List(ctx, metav1.ListOptions{LabelSelector: "job-name=" + name}) + if err != nil { + t.Fatal(err) + } + completed := false + for _, pod := range pods.Items { + if pod.Status.Phase == corev1.PodSucceeded { + completed = true + if pod.Spec.NodeName != sourceNode || pod.Spec.AutomountServiceAccountToken == nil || *pod.Spec.AutomountServiceAccountToken { + t.Fatal("completed worker violated placement or credential boundary") + } + } + } + if !completed { + t.Fatal("no completed real restic worker") + } + if !regexp.MustCompile(`^[0-9a-f]{8,64}$`).MatchString(snapshotID) { + t.Fatalf("unexpected snapshot identity %q", snapshotID) + } + runResticJob(t, fmt.Sprintf("restic restore %s --target /tmp/prepared-restore --retry-lock 5m && test \"$(cat /tmp/prepared-restore/data/prepared-marker.txt)\" = prepared-copy-marker", snapshotID)) +} diff --git a/test/e2e/buckets.sh b/test/e2e/buckets.sh index 519e56643..7d397a176 100755 --- a/test/e2e/buckets.sh +++ b/test/e2e/buckets.sh @@ -398,10 +398,15 @@ TestManagementPlacementAndDisruption EOF } +bucket_backup_preparation() { cat <<'EOF' +TestBackup_PreparationAndSameNodeCopy +EOF +} + unbucketed() { :; } bucket_names() { - printf '%s\n' operator api-auth api-roles api-rbac api-agent api-mods ratelimit bot-fast bot-heavy multicluster upgrade telemetry capture-overhead optional-components management-placement + printf '%s\n' operator api-auth api-roles api-rbac api-agent api-mods ratelimit bot-fast bot-heavy multicluster upgrade telemetry capture-overhead optional-components management-placement backup-preparation } list_bucket() { @@ -421,6 +426,7 @@ list_bucket() { capture-overhead) bucket_capture_overhead ;; optional-components) bucket_optional_components ;; management-placement) bucket_management_placement ;; + backup-preparation) bucket_backup_preparation ;; *) echo "unknown bucket: $1 (known: $(bucket_names | tr '\n' ' '))" >&2 exit 2 diff --git a/test/e2e/specs.md b/test/e2e/specs.md index 75fff7eb7..7649e8b1d 100644 --- a/test/e2e/specs.md +++ b/test/e2e/specs.md @@ -51,6 +51,17 @@ a cordon, readiness of the surviving replica, and singleton API eviction. Node labels and cordons are restored in cleanup. This validates scheduler/PDB behavior, not application HA or storage migration. +### Backup preparation acceptance + +The `backup-preparation` bucket also uses the `management` Kind profile and zero +API logins. It verifies a real RWO copy on the game's node across two schedulable +workers, repository preparation failure before quiesce/authorization, retry after +repository recovery, token-gated Restic execution and restoration of a marker. +Kind local-path PV affinity supplies the storage placement constraint; this is +not an attachable CSI driver or multi-attach qualification. Unit/envtest regressions +cover stale worker/source identity, persisted budgets, delayed drainage, CSI +abort/reclamation, retained/replaced content and raw snapshot-source rejection. + ## Non-goals / boundaries - Does **not** replace the unit or envtest tiers for any other module — it exercises integration behavior, not the exhaustive branch coverage those tiers own (this module carries no `.testcoverage.yml` coverage gate). From cc3cf86a7366f686a7d7844994adb50dfcb50526 Mon Sep 17 00:00:00 2001 From: Baylem Date: Wed, 21 Oct 2026 17:10:55 -0400 Subject: [PATCH 02/12] test: exercise backup preparation and asynchronous cleanup Signed-off-by: Baylem --- operator/internal/controller/backup_budgets_test.go | 12 +++++++++--- operator/internal/controller/backup_worker.go | 4 ++-- test/e2e/backup_preparation_e2e_test.go | 9 ++++++--- test/e2e/specs.md | 2 +- 4 files changed, 18 insertions(+), 9 deletions(-) diff --git a/operator/internal/controller/backup_budgets_test.go b/operator/internal/controller/backup_budgets_test.go index 77715ca19..c8e35001f 100644 --- a/operator/internal/controller/backup_budgets_test.go +++ b/operator/internal/controller/backup_budgets_test.go @@ -476,7 +476,7 @@ func TestBackupWorkerScriptRejectsExpiredAndReplayedTokens(t *testing.T) { if err := os.WriteFile(tokenFile, fmt.Appendf(nil, "v1 %s %s %s %d\n", backup, pod, source, expiry), 0o600); err != nil { t.Fatal(err) } - script := strings.NewReplacer("/data", dir, "/start/start", tokenFile, "/tmp/prepared", prepared).Replace(backupWorkerScript) + script := strings.NewReplacer("/data", dir, "/start/start", tokenFile, "/tmp/prepared", prepared).Replace(resticWorkerScript) ctx, cancel := context.WithTimeout(t.Context(), 4*time.Second) defer cancel() cmd := exec.CommandContext(ctx, "/bin/sh", "-c", script, "backup-worker", "backup", dir) @@ -575,8 +575,14 @@ func TestBackupCSIAbortReclaimsOnlyVerifiedDeleteContent(t *testing.T) { err := r.Get(t.Context(), client.ObjectKeyFromObject(content), content) switch mode { case "delete": - if !apierrors.IsNotFound(err) || condition.Status != metav1.ConditionTrue { - t.Fatalf("owned Delete content was not reclaimed: %v %+v", err, condition) + if !apierrors.IsNotFound(err) || condition.Status != metav1.ConditionFalse || condition.Reason != "SnapshotCleanupPending" { + t.Fatalf("deletion request was not recorded before observing absence: %v %+v", err, condition) + } + budgetPass(t, r, b.Name) + b = guardGetBackup(t, r, b.Name) + condition = meta.FindStatusCondition(b.Status.Conditions, "SnapshotCleaned") + if condition == nil || condition.Status != metav1.ConditionTrue || condition.Reason != "SnapshotCleanupComplete" { + t.Fatalf("observed resource absence did not complete reclamation: %+v", condition) } case "retain": if err != nil || condition.Reason != "SnapshotCleanupRetained" || !strings.Contains(condition.Message, "content-uid") { diff --git a/operator/internal/controller/backup_worker.go b/operator/internal/controller/backup_worker.go index 0e60aa368..f25417d3a 100644 --- a/operator/internal/controller/backup_worker.go +++ b/operator/internal/controller/backup_worker.go @@ -33,7 +33,7 @@ const ( // this exact Backup/pod/source generation. Repository preparation takes a // shared restic lock; later lock contention is bounded by the copy timeout. // Values and tags are arguments/environment, never interpolated shell code. -const backupWorkerScript = `set -eu +const resticWorkerScript = `set -eu for utility in timeout date cat touch sleep restic; do command -v "$utility" >/dev/null; done timeout -s TERM -k 5 1 /bin/sh -c ':' test -d /data && test -r /data && test -x /data @@ -131,7 +131,7 @@ func (r *BackupReconciler) buildPreparedBackupPodSpec(b *gameplanev1alpha1.Backu spec.Volumes = append(spec.Volumes, corev1.Volume{Name: "start", VolumeSource: corev1.VolumeSource{ConfigMap: &corev1.ConfigMapVolumeSource{LocalObjectReference: corev1.LocalObjectReference{Name: backupStartConfigMapName(b)}}}}) worker := &spec.Containers[0] worker.Command = []string{"/bin/sh", "-c"} - worker.Args = append([]string{backupWorkerScript, "backup-worker"}, worker.Args...) + worker.Args = append([]string{resticWorkerScript, "backup-worker"}, worker.Args...) worker.Env = append(worker.Env, corev1.EnvVar{Name: "BACKUP_UID", Value: string(b.UID)}, corev1.EnvVar{Name: "POD_UID", ValueFrom: &corev1.EnvVarSource{FieldRef: &corev1.ObjectFieldSelector{FieldPath: "metadata.uid"}}}, corev1.EnvVar{Name: "SOURCE_ID", Value: backupWorkerSourceID(b)}, diff --git a/test/e2e/backup_preparation_e2e_test.go b/test/e2e/backup_preparation_e2e_test.go index a1db76a9e..fd5ec305c 100644 --- a/test/e2e/backup_preparation_e2e_test.go +++ b/test/e2e/backup_preparation_e2e_test.go @@ -73,8 +73,11 @@ func TestBackup_PreparationAndSameNodeCopy(t *testing.T) { } repo = repo.DeepCopy() repo.ObjectMeta = metav1.ObjectMeta{Name: "prepared-repo-" + suffix, Namespace: ns} - validURL := append([]byte(nil), repo.Data["repo"]...) - repo.Data["repo"] = []byte("rest:http://127.0.0.1:1") + // Authentication failure against the warm repository is deterministic. + // A refused HTTP endpoint exercises Restic's network retry budget and may + // keep the init container running longer than this preparation assertion. + validPassword := append([]byte(nil), repo.Data["password"]...) + repo.Data["password"] = []byte("incorrect-repository-password-" + suffix) if _, err := envInstance.K8s.CoreV1().Secrets(ns).Create(ctx, repo, metav1.CreateOptions{}); err != nil { t.Fatal(err) } @@ -120,7 +123,7 @@ func TestBackup_PreparationAndSameNodeCopy(t *testing.T) { if err != nil { t.Fatal(err) } - repo.Data["repo"] = validURL + repo.Data["password"] = validPassword if _, err := envInstance.K8s.CoreV1().Secrets(ns).Update(ctx, repo, metav1.UpdateOptions{}); err != nil { t.Fatal(err) } diff --git a/test/e2e/specs.md b/test/e2e/specs.md index afdd21a22..a145c4e73 100644 --- a/test/e2e/specs.md +++ b/test/e2e/specs.md @@ -76,7 +76,7 @@ not application HA or storage migration. The `backup-preparation` bucket also uses the `management` Kind profile and zero API logins. It verifies a real RWO copy on the game's node across two schedulable -workers, repository preparation failure before quiesce/authorization, retry after +workers, repository authentication failure before quiesce/authorization, retry after repository recovery, token-gated Restic execution and restoration of a marker. Kind local-path PV affinity supplies the storage placement constraint; this is not an attachable CSI driver or multi-attach qualification. Unit/envtest regressions From 9c268d80ce16a1fb91ab148ae13b7eb1b1567b30 Mon Sep 17 00:00:00 2001 From: Baylem Date: Wed, 21 Oct 2026 17:15:02 -0400 Subject: [PATCH 03/12] test: update envtest references to the Restic worker script Signed-off-by: Baylem --- operator/internal/controller/backup_envtest_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/operator/internal/controller/backup_envtest_test.go b/operator/internal/controller/backup_envtest_test.go index a2b6d9aa3..fd226d489 100644 --- a/operator/internal/controller/backup_envtest_test.go +++ b/operator/internal/controller/backup_envtest_test.go @@ -56,7 +56,7 @@ func TestBackup_CreatesJobWithExpectedSpec(t *testing.T) { if !strings.HasPrefix(c.Image, "restic/restic:") { t.Errorf("image = %q, want restic/restic:*", c.Image) } - if got, want := c.Args, []string{backupWorkerScript, "backup-worker", "backup", "/data", "--json", "--retry-lock", "10m", "--tag", "gameplane"}; !equalStrings(got, want) { + if got, want := c.Args, []string{resticWorkerScript, "backup-worker", "backup", "/data", "--json", "--retry-lock", "10m", "--tag", "gameplane"}; !equalStrings(got, want) { t.Errorf("args = %v, want %v", got, want) } @@ -365,7 +365,7 @@ func TestBackup_PassesTagsToRestic(t *testing.T) { return false, "job not yet created" } want := []string{"backup", "/data", "--json", "--retry-lock", "10m", "--tag", "gameplane", "--tag", "nightly", "--tag", "preupgrade"} - want = append([]string{backupWorkerScript, "backup-worker"}, want...) + want = append([]string{resticWorkerScript, "backup-worker"}, want...) if !equalStrings(j.Spec.Template.Spec.Containers[0].Args, want) { return false, "args = " + strings.Join(j.Spec.Template.Spec.Containers[0].Args, " ") } From c62ce58d5211e77a33667135de5712182899db1e Mon Sep 17 00:00:00 2001 From: Baylem Date: Wed, 21 Oct 2026 22:53:11 -0400 Subject: [PATCH 04/12] fix(backups): persist snapshot cleanup condition transitions Signed-off-by: Baylem --- operator/internal/controller/backup_budgets_test.go | 9 ++++++++- operator/internal/controller/helpers.go | 4 +++- operator/internal/controller/helpers_extra_test.go | 13 +++++++++++++ 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/operator/internal/controller/backup_budgets_test.go b/operator/internal/controller/backup_budgets_test.go index c8e35001f..4645605e1 100644 --- a/operator/internal/controller/backup_budgets_test.go +++ b/operator/internal/controller/backup_budgets_test.go @@ -563,7 +563,11 @@ func TestBackupCSIAbortReclaimsOnlyVerifiedDeleteContent(t *testing.T) { if err := r.Update(t.Context(), b); err != nil { t.Fatal(err) } - budgetPass(t, r, b.Name) + // First request deletion, then separately observe its completion. + // A single pass must not claim backend reclamation was observed. + if _, err := r.Reconcile(t.Context(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(b)}); err != nil { + t.Fatal(err) + } b = guardGetBackup(t, r, b.Name) if q.unquiesced != 1 || b.Status.Phase != gameplanev1alpha1.BackupPhaseFailed || b.Status.SnapshotID != "" { t.Fatalf("reclamation blocked invalidation/save-on: %+v %+v", b.Status, q) @@ -578,6 +582,9 @@ func TestBackupCSIAbortReclaimsOnlyVerifiedDeleteContent(t *testing.T) { if !apierrors.IsNotFound(err) || condition.Status != metav1.ConditionFalse || condition.Reason != "SnapshotCleanupPending" { t.Fatalf("deletion request was not recorded before observing absence: %v %+v", err, condition) } + if err := r.Get(t.Context(), client.ObjectKeyFromObject(&vs), &vs); !apierrors.IsNotFound(err) { + t.Fatalf("snapshot remains after deletion: %v", err) + } budgetPass(t, r, b.Name) b = guardGetBackup(t, r, b.Name) condition = meta.FindStatusCondition(b.Status.Conditions, "SnapshotCleaned") diff --git a/operator/internal/controller/helpers.go b/operator/internal/controller/helpers.go index be11001c0..8bd3890d6 100644 --- a/operator/internal/controller/helpers.go +++ b/operator/internal/controller/helpers.go @@ -13,8 +13,10 @@ import ( ) // upsertCondition replaces a condition by Type if present, otherwise -// appends. Sets LastTransitionTime to now when the status flips. +// appends. Sets LastTransitionTime to now when the status flips. Preserve the +// input slice so callers can compare old and new conditions before persisting. func upsertCondition(conds []metav1.Condition, c metav1.Condition) []metav1.Condition { + conds = append([]metav1.Condition(nil), conds...) now := metav1.Now() for i := range conds { if conds[i].Type != c.Type { diff --git a/operator/internal/controller/helpers_extra_test.go b/operator/internal/controller/helpers_extra_test.go index 85a8d2ccf..ea6ab4ce0 100644 --- a/operator/internal/controller/helpers_extra_test.go +++ b/operator/internal/controller/helpers_extra_test.go @@ -53,6 +53,19 @@ func TestUpsertCondition_StatusFlip_BumpsTimestamp(t *testing.T) { } } +func TestUpsertConditionPreservesComparisonBase(t *testing.T) { + for _, status := range []metav1.ConditionStatus{metav1.ConditionFalse, metav1.ConditionTrue} { + t.Run(string(status), func(t *testing.T) { + original := metav1.Condition{Type: "SnapshotCleaned", Status: metav1.ConditionFalse, Reason: "Pending", LastTransitionTime: metav1.Now()} + conditions := []metav1.Condition{original} + updated := upsertCondition(conditions, metav1.Condition{Type: original.Type, Status: status, Reason: "Updated"}) + if conditions[0] != original || sameConditions(conditions, updated) { + t.Fatalf("upsert hid the change from status persistence: before=%+v after=%+v", conditions, updated) + } + }) + } +} + func TestSameConditions(t *testing.T) { a := []metav1.Condition{ {Type: "Ready", Status: metav1.ConditionTrue, Reason: "Up"}, From ed8a24017aa7b8f0feba6916634416e3dae3608d Mon Sep 17 00:00:00 2001 From: Baylem Date: Thu, 22 Oct 2026 20:31:39 -0400 Subject: [PATCH 05/12] fix(operator): wait for source scheduling before binding backup worker Signed-off-by: Baylem --- docs/operator-operations.md | 10 ++- .../controller/backup_budgets_test.go | 75 +++++++++++++++++++ .../internal/controller/backup_guard_test.go | 2 +- operator/internal/controller/backup_worker.go | 16 +++- .../controller/helpers_envtest_test.go | 11 +++ operator/specs.md | 4 +- test/e2e/failure_paths_e2e_test.go | 3 +- 7 files changed, 114 insertions(+), 7 deletions(-) diff --git a/docs/operator-operations.md b/docs/operator-operations.md index a93e11ed7..ba1372686 100644 --- a/docs/operator-operations.md +++ b/docs/operator-operations.md @@ -13,8 +13,8 @@ already stopped and its writers drained before capture; the backup did not stop it. Failed or unfinished copies, including completed Jobs still awaiting a snapshot ID, cannot report achieved quiesce or stopped-source consistency. -`status.captureManifest` is recorded once before the first copy worker is -created, after lifecycle ownership and quiesce preparation. It lists the primary +`status.captureManifest` is recorded once before copy authorization or CSI +capture, after lifecycle ownership and quiesce preparation. It lists the primary `-data` claim as included and every other observed persistent claim as uncovered, including extra volumes, nested mod/loader mounts and retained server-owned claims. It records claim UIDs, observed mount paths, source layout @@ -90,6 +90,12 @@ makes that placement mandatory. Source tolerations and `operator.backupResources apply to preparation and copy containers. Stopped sources leave node selection to the scheduler and volume affinity. +When the source StatefulSet requests a running workload, initial preparation +waits for its owned pod to be created and assigned a node before binding the +worker to that source. This wait uses the preparation budget and does not disable +saving. A pod or node replacement after binding still fails the copy. Raw backups +of intentionally stopped workloads remain eligible without a source pod. + The install settings are `operator.backupPreparationTimeout: 10m`, `operator.backupQuiesceTimeout: 5m`, and `operator.backupCSICaptureTimeout: 10m`. Equivalent operator flags use kebab-case names. The recorded deadlines remain diff --git a/operator/internal/controller/backup_budgets_test.go b/operator/internal/controller/backup_budgets_test.go index 4645605e1..b483e7538 100644 --- a/operator/internal/controller/backup_budgets_test.go +++ b/operator/internal/controller/backup_budgets_test.go @@ -243,6 +243,81 @@ func TestBackupRawAndStoppedCopiesKeepFullJobBudget(t *testing.T) { } } +func TestBackupRawSourceWaitsForInitialScheduling(t *testing.T) { + for _, initiallyAbsent := range []bool{false, true} { + t.Run(fmt.Sprintf("absent=%t", initiallyAbsent), func(t *testing.T) { + b := guardBackup("starting") + b.Spec.Quiesce = false + r, q := guardFixtureWorkers(t, false, b) + var pod corev1.Pod + if err := r.Get(t.Context(), client.ObjectKey{Namespace: "ns", Name: "server-0"}, &pod); err != nil { + t.Fatal(err) + } + pod.Spec.NodeName = "" + if err := r.Update(t.Context(), &pod); err != nil { + t.Fatal(err) + } + if initiallyAbsent { + if err := r.Delete(t.Context(), &pod); err != nil { + t.Fatal(err) + } + } + budgetPass(t, r, b.Name) + b = guardGetBackup(t, r, b.Name) + var job batchv1.Job + if err := r.Get(t.Context(), client.ObjectKeyFromObject(b), &job); !apierrors.IsNotFound(err) || b.Annotations[backupWorkerSourceAnnotation] != "" || q.quiesced != 0 { + t.Fatalf("starting source was bound or quiesced: job=%v annotations=%v calls=%+v", err, b.Annotations, q) + } + pod.Spec.NodeName = "node-a" + if initiallyAbsent { + pod.ResourceVersion = "" + if err := r.Create(t.Context(), &pod); err != nil { + t.Fatal(err) + } + } else if err := r.Update(t.Context(), &pod); err != nil { + t.Fatal(err) + } + budgetPass(t, r, b.Name) + b = guardGetBackup(t, r, b.Name) + if err := r.Get(t.Context(), client.ObjectKeyFromObject(b), &job); err != nil || b.Status.Phase == gameplanev1alpha1.BackupPhaseFailed { + t.Fatalf("normal source scheduling failed backup: job=%v status=%+v", err, b.Status) + } + // Once captured in the worker's immutable environment, the source + // may not silently change to a different pod at the same name. + if err := r.Delete(t.Context(), &pod); err != nil { + t.Fatal(err) + } + pod.UID, pod.ResourceVersion = "replacement-pod", "" + if err := r.Create(t.Context(), &pod); err != nil { + t.Fatal(err) + } + budgetPass(t, r, b.Name) + b = guardGetBackup(t, r, b.Name) + if b.Status.Phase != gameplanev1alpha1.BackupPhaseFailed || b.Annotations[backupAuthorizedWorkerAnnotation] != "" { + t.Fatalf("replacement source escaped immutable binding: %+v", b) + } + }) + } +} + +func TestBackupPreparedWorkerFailurePreservesJobMessage(t *testing.T) { + r, _ := guardFixtureWorkers(t, false, guardBackup("failed-worker")) + budgetPass(t, r, "failed-worker") + var job batchv1.Job + if err := r.Get(t.Context(), client.ObjectKey{Namespace: "ns", Name: "failed-worker"}, &job); err != nil { + t.Fatal(err) + } + job.Status.Conditions = []batchv1.JobCondition{{Type: batchv1.JobFailed, Status: corev1.ConditionTrue}} + if err := r.Status().Update(t.Context(), &job); err != nil { + t.Fatal(err) + } + budgetPass(t, r, job.Name) + b := guardGetBackup(t, r, job.Name) + if b.Status.Phase != gameplanev1alpha1.BackupPhaseFailed || b.Status.Message != "backup job reported Failed" || b.Annotations[backupCopyAbortAnnotation] != b.Status.Message { + t.Fatalf("prepared worker lost failure compatibility/invalidation: %+v", b) + } +} + func TestBackupQueuedCopyDoesNotSpendPreparationBudget(t *testing.T) { r, _ := guardFixture(t, guardBackup("first"), guardBackup("second")) budgetPass(t, r, "first") diff --git a/operator/internal/controller/backup_guard_test.go b/operator/internal/controller/backup_guard_test.go index e3f8ae74f..63c9a2c36 100644 --- a/operator/internal/controller/backup_guard_test.go +++ b/operator/internal/controller/backup_guard_test.go @@ -75,7 +75,7 @@ func guardFixtureWorkers(t *testing.T, automatic bool, backups ...*gameplanev1al objects := []client.Object{&gameplanev1alpha1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "server", Namespace: "ns", UID: "server-uid"}, Spec: gameplanev1alpha1.GameServerSpec{TemplateRef: gameplanev1alpha1.GameTemplateRef{Name: "template"}}}, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "repo", Namespace: "ns"}, Data: map[string][]byte{"repo": []byte("repo"), "password": []byte("password")}}} one := int32(1) objects = append(objects, &appsv1.StatefulSet{ObjectMeta: metav1.ObjectMeta{Name: "server", Namespace: "ns", UID: "workload-uid", OwnerReferences: []metav1.OwnerReference{{APIVersion: gameplanev1alpha1.GroupVersion.String(), Kind: "GameServer", Name: "server", UID: "server-uid", Controller: ownerBoolPtr(true)}}}, Spec: appsv1.StatefulSetSpec{Replicas: &one}}) - objects = append(objects, &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "server-0", Namespace: "ns", UID: "game-pod-uid", OwnerReferences: []metav1.OwnerReference{{APIVersion: "apps/v1", Kind: "StatefulSet", Name: "server", UID: "workload-uid", Controller: ownerBoolPtr(true)}}}, Status: corev1.PodStatus{Phase: corev1.PodRunning}}) + objects = append(objects, &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "server-0", Namespace: "ns", UID: "game-pod-uid", OwnerReferences: []metav1.OwnerReference{{APIVersion: "apps/v1", Kind: "StatefulSet", Name: "server", UID: "workload-uid", Controller: ownerBoolPtr(true)}}}, Spec: corev1.PodSpec{NodeName: "node-a"}, Status: corev1.PodStatus{Phase: corev1.PodRunning}}) for _, b := range backups { objects = append(objects, b) } diff --git a/operator/internal/controller/backup_worker.go b/operator/internal/controller/backup_worker.go index f25417d3a..61d55055a 100644 --- a/operator/internal/controller/backup_worker.go +++ b/operator/internal/controller/backup_worker.go @@ -79,6 +79,7 @@ func backupWorkerSourceID(b *gameplanev1alpha1.Backup) string { // Raw copies retain their no-agent behavior while pinning any observed game // pod for placement and rejecting a replacement before authorization. The // stronger stopped/live proof remains #716's preparation for protected copies. +// A nil source means the initial workload has not reached a stable node yet. func (r *BackupReconciler) prepareBackupWorkerSource(ctx context.Context, b *gameplanev1alpha1.Backup) (*backupWorkerSource, error) { var source backupWorkerSource var pod corev1.Pod @@ -107,6 +108,13 @@ func (r *BackupReconciler) prepareBackupWorkerSource(ctx context.Context, b *gam } return &previous, nil } + // The StatefulSet controller and scheduler may still be establishing the + // first pod. Do not freeze an absent UID or empty node into the worker's + // immutable source ID and then mistake normal startup for replacement. + // An intentionally stopped workload remains eligible without any pod. + if (err == nil && pod.Spec.NodeName == "") || (apierrors.IsNotFound(err) && (ss.Spec.Replicas == nil || *ss.Spec.Replicas != 0)) { + return nil, nil + } encoded, err := json.Marshal(source) if err != nil { return nil, err @@ -238,6 +246,12 @@ func (r *BackupReconciler) reconcilePreparedRestic(ctx context.Context, b *gamep if err != nil { return r.abortResticCopy(ctx, b, err.Error()) } + if source == nil { + if err := r.setBackupProgress(ctx, b, "WorkerPreparing", "waiting for the source workload to be created and scheduled"); err != nil { + return ctrl.Result{}, err + } + return ctrl.Result{RequeueAfter: 5 * time.Second}, nil + } gate := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: backupStartConfigMapName(b), Namespace: b.Namespace}} if _, err := controllerutil.CreateOrUpdate(ctx, r.Client, gate, func() error { if !gate.CreationTimestamp.IsZero() && !metav1.IsControlledBy(gate, b) { @@ -263,7 +277,7 @@ func (r *BackupReconciler) reconcilePreparedRestic(ctx context.Context, b *gamep } } if jobPermanentlyFailed(&job) { - return r.abortResticCopy(ctx, b, "backup worker reported Failed") + return r.abortResticCopy(ctx, b, "backup job reported Failed") } if job.Status.Succeeded > 0 { if b.Annotations[backupAuthorizedWorkerAnnotation] == "" { diff --git a/operator/internal/controller/helpers_envtest_test.go b/operator/internal/controller/helpers_envtest_test.go index 1917daa19..0060b6886 100644 --- a/operator/internal/controller/helpers_envtest_test.go +++ b/operator/internal/controller/helpers_envtest_test.go @@ -879,4 +879,15 @@ func seedBackupWorkload(t *testing.T, gs *gameplanev1alpha1.GameServer) { if err := k8sClient.Status().Update(context.Background(), ss); err != nil { t.Fatalf("observe workload: %v", err) } + // Envtest has no StatefulSet controller or scheduler. Model their initial + // pod placement before a worker binds its immutable source identity. + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: name + "-0", Namespace: ns, OwnerReferences: []metav1.OwnerReference{{APIVersion: "apps/v1", Kind: "StatefulSet", Name: name, UID: ss.UID, Controller: ownerBoolPtr(true)}}}, + Spec: corev1.PodSpec{NodeName: "fixture-node", Containers: []corev1.Container{{Name: "game", Image: "test/game:fixture"}}}} + if err := k8sClient.Create(context.Background(), pod); err != nil { + t.Fatalf("seed scheduled source pod: %v", err) + } + pod.Status.Phase = corev1.PodRunning + if err := k8sClient.Status().Update(context.Background(), pod); err != nil { + t.Fatalf("observe running source pod: %v", err) + } } diff --git a/operator/specs.md b/operator/specs.md index e17d30b84..88d065060 100644 --- a/operator/specs.md +++ b/operator/specs.md @@ -295,7 +295,7 @@ Primary reconcilers register with the manager in `cmd/main.go` and handle CRD li ### BackupReconciler - **Responsibility:** Drive Backup to completion: coordinate quiesce (if agent available), spawn restic Job, track snapshot ID. - **Key functions:** - - Restic preparation precedes quiesce: claim/fence the source, persist a preparation deadline, create the waiting worker, and require an uncached Running/Ready pod controlled by the exact Job UID. The worker mounts the PVC, initializes/probes the repository with a shared lock, then waits for an owned ConfigMap start token. Only after readiness does the operator persist the quiesce deadline, perform the pod-bound RPC and source postcheck, record the capture manifest, and grant the Backup UID/worker UID/source-fingerprint/expiry-bound token. Replacement pods cannot reuse it. Workers have no service-account token, use the shared backup resource settings and source tolerations, and prefer the observed game node; hard same-node affinity is opt-in. + - Restic preparation precedes quiesce: claim/fence the source, persist a preparation deadline, create the waiting worker, and require an uncached Running/Ready pod controlled by the exact Job UID. Before binding the first worker, a source StatefulSet requesting replicas must have an owned pod assigned to a node; initial creation/scheduling waits within the preparation budget, while a replacement after binding fails the copy. An intentionally stopped raw source remains eligible without a pod. The worker mounts the PVC, initializes/probes the repository with a shared lock, then waits for an owned ConfigMap start token. Only after readiness does the operator persist the quiesce deadline, perform the pod-bound RPC and source postcheck, record the capture manifest, and grant the Backup UID/worker UID/source-fingerprint/expiry-bound token. Replacement pods cannot reuse it. Workers have no service-account token, use the shared backup resource settings and source tolerations, and prefer the observed game node; hard same-node affinity is opt-in. - 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. - 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. @@ -307,7 +307,7 @@ Primary reconcilers register with the manager in `cmd/main.go` and handle CRD li - New PVCs referencing a Gameplane Backup-owned VolumeSnapshot require uncached successful-owner/snapshotID/UID/readiness checks and reject abort markers, including raw GameServer dataSource references. Existing PVCs do not depend on retained source metadata. External snapshots without a Gameplane Backup owner preserve the administrator direct-CRD contract; API authorization is separate. - Restic Job: mount data PVC, run restic backup to configured destination (S3, B2, rest-server, local fs). Job carries BackoffLimit and ActiveDeadlineSeconds from the reconciler's JobBackoffLimit / JobActiveDeadlineSeconds fields (operator flags --backup-job-backoff-limit / --backup-job-active-deadline-seconds, Helm operator.backupJobBackoffLimit / operator.backupJobActiveDeadlineSeconds; defaults 2 and 86400, applied when unset; spec 018 OD-027); only a Failed=True Job condition (set by jobPermanentlyFailed check) marks the Backup Failed. Pod SecurityContext takes FSGroup from the target GameTemplate's spec.security.fsGroup (fallback 65532 if template missing). - Snapshot tracking: parse restic JSON output from container logs to extract snapshot ID. - - Capture metadata: after ownership, workload fencing and quiesce admission, record `status.captureManifest` once before creating the first copy worker. It pins the GameServer UID, strategy, observed Pod/StatefulSet mount layout, PVC names/UIDs, included primary claim and excluded extra/mod/retained claims. `storage.extra[].backupRole` optionally declares `data` or `rebuildable`; absent declarations remain unclassified and excluded. Later template edits never rewrite the manifest. Existing workers and old terminal Backups have unknown historical coverage, never reconstructed from the current template. + - Capture metadata: after ownership, workload fencing and quiesce admission, record `status.captureManifest` once before authorizing the prepared copy worker or starting CSI capture. It pins the GameServer UID, strategy, observed Pod/StatefulSet mount layout, PVC names/UIDs, included primary claim and excluded extra/mod/retained claims. `storage.extra[].backupRole` optionally declares `data` or `rebuildable`; absent declarations remain unclassified and excluded. Later template edits never rewrite the manifest. Legacy workers and old terminal Backups have unknown historical coverage, never reconstructed from the current template. - Achieved consistency: the `Consistency` condition is separate from copy phase and volume coverage. It stays Unknown until a usable snapshot identity is recorded; failed copies report `CaptureFailed`. Agent-confirmed quiesce, already-stopped/drained sources, unsupported/no-agent raw copy and unquiesced CSI snapshots have distinct reasons. Quiesce depends on the module commands and does not imply that excluded volumes were captured. The API exposes the stored status and the backup drawer shows consistency and included/excluded claims. `StoppedSource` records an observation, without adding a stop/copy/restart policy or multi-volume backup. - Snapshot cleanup on delete (`backup_forget.go`): restic-strategy Backups carry `BackupSnapshotFinalizer` (`gameplane.local/backup-snapshot-finalizer`, added on first reconcile, also for pre-existing non-Failed Backups; not added for volume-snapshot or Failed Backups). `finalizeDelete` runs the unquiesce stage (`BackupFinalizer`) first, then `finalizeSnapshot`: it deletes the Backup's own still-running Job (foreground, awaited), waits while a non-terminal Restore pins the Backup or its snapshot id, then runs Job `-forget` (`restic unlock || true; restic forget --prune --retry-lock 10m`, backoff 3, deadline 1800s, 50m/128Mi requests and 512Mi memory limit, no PVC; the snapshot id is passed through env `FORGET_SNAPSHOT_ID` and must match `^[0-9a-f]{8,64}$`). Success sets `SnapshotForgotten=True` and a Normal `SnapshotForgotten` event; a Failed Job, a missing/incomplete repo Secret, a terminating namespace, or `maxSnapshotForgetWait` (45m, from annotation `backup.gameplane.local/forget-started-at`) sets `SnapshotForgotten=False` with a Warning `SnapshotForgetAbandoned` event and releases the Backup, leaving the snapshot in the repository. A Backup with no recorded snapshot id is released with `SnapshotForgetSkipped` when it may have written one. Backup Jobs run `restic backup /data --json --retry-lock 10m ...`. RBAC markers on `backup_controller.go` grant `backups` update/patch, `backups/status`, `backups/finalizers` and `jobs` create/update/patch/delete. Tests: `backup_forget_test.go`, `backup_forget_envtest_test.go`, `retention_terminating_test.go`; e2e `TestBackup_DeleteForgetsSnapshot` and the extended `TestBackupSchedule_RetentionTrimsPast`. Spec: `specs/done_020-backup-snapshot-forget/`. - Status: report phase (Pending/Running/Succeeded/Failed), error details. diff --git a/test/e2e/failure_paths_e2e_test.go b/test/e2e/failure_paths_e2e_test.go index 6cf00a068..e497340a4 100644 --- a/test/e2e/failure_paths_e2e_test.go +++ b/test/e2e/failure_paths_e2e_test.go @@ -123,7 +123,8 @@ func TestBackup_FailsOnBadCredentials(t *testing.T) { } phase, _, _ := unstructured.NestedString(got.Object, "status", "phase") if phase != "Succeeded" { - return false, "seed phase=" + phase + message, _, _ := unstructured.NestedString(got.Object, "status", "message") + return false, "seed phase=" + phase + ": " + message } return true, "" }) From c02531a9c34ed8c0c6a847e5371f9c4f9b8d150a Mon Sep 17 00:00:00 2001 From: Baylem Date: Thu, 22 Oct 2026 20:33:52 -0400 Subject: [PATCH 06/12] fix(operator): retain failed restore ownership until startup drains Signed-off-by: Baylem --- docs/operator-operations.md | 7 + .../controller/gameserver_controller.go | 11 +- .../controller/lifecycle_operation.go | 16 ++ .../internal/controller/restore_lifecycle.go | 32 ++++ .../restore_startup_cleanup_test.go | 181 ++++++++++++++++++ operator/specs.md | 2 +- 6 files changed, 246 insertions(+), 3 deletions(-) create mode 100644 operator/internal/controller/restore_startup_cleanup_test.go diff --git a/docs/operator-operations.md b/docs/operator-operations.md index ba1372686..e4cbe8643 100644 --- a/docs/operator-operations.md +++ b/docs/operator-operations.md @@ -77,6 +77,13 @@ is allowed under that reservation so storage using WaitForFirstConsumer can bind Other operations wait until the restore completes. Existing data-PVC restores continue to require an observed stopped workload and drained writers. +If initial startup fails or the Restore is deleted, cleanup first retires its +startup permission and suspends the target. The operation remains held until the +StatefulSet has observed zero replicas and every live or terminating game/PVC +writer has drained. Recovery of a force-deleted Restore applies the same check +before a successor can proceed, preserving the existing pod template while +requesting zero replicas. A successful Restore leaves its initial workload running. + Stopped-source proof, bounded copy preparation and CSI invalidation build on this ownership boundary. Staged template adoption is tracked separately in #731. diff --git a/operator/internal/controller/gameserver_controller.go b/operator/internal/controller/gameserver_controller.go index 97a438512..9ff7f336e 100644 --- a/operator/internal/controller/gameserver_controller.go +++ b/operator/internal/controller/gameserver_controller.go @@ -1254,6 +1254,8 @@ func (r *GameServerReconciler) desiredReplicas( return 0, 5 * time.Second, nil } return 1, 5 * time.Second, nil + } else if held && lifecycleRecoveryNeedsStop(op) { + return 0, 5 * time.Second, nil } else if held && (op.Kind == "Backup" || op.Baseline || len(op.Recovery) != 0) { var ss appsv1.StatefulSet err := r.wipeReader().Get(ctx, client.ObjectKeyFromObject(gs), &ss) @@ -1556,8 +1558,13 @@ func (r *GameServerReconciler) reconcileStatefulSet( } if op.Kind == "Backup" || len(op.Recovery) != 0 { if !ss.CreationTimestamp.IsZero() || ss.ResourceVersion != "" { - // Preserve replicas AND the complete pod template, including - // config hashes, images, init containers and mount topology. + // Orphaned restores may leave a running initial workload. + // Stop it while preserving its complete pod template. Other + // protected operations also preserve the replica count. + if lifecycleRecoveryNeedsStop(op) { + zero := int32(0) + ss.Spec.Replicas = &zero + } return nil } // No workload exists yet. Create it stopped; this gives backup diff --git a/operator/internal/controller/lifecycle_operation.go b/operator/internal/controller/lifecycle_operation.go index 09acf54d3..6445aa927 100644 --- a/operator/internal/controller/lifecycle_operation.go +++ b/operator/internal/controller/lifecycle_operation.go @@ -268,6 +268,15 @@ func legacyLifecycleCopiesLive(ctx context.Context, c client.Client, reader clie return false, nil } +func lifecycleRecoveryNeedsStop(op lifecycleOperation) bool { + for _, predecessor := range op.Recovery { + if predecessor.Kind == "Restore" { + return true + } + } + return false +} + func recoverLifecycleOperation(ctx context.Context, c client.Client, reader client.Reader, gs *gameplanev1alpha1.GameServer, op lifecycleOperation, quiescer AgentQuiescer) (bool, error) { for _, predecessor := range op.Recovery { var busy bool @@ -286,6 +295,13 @@ func recoverLifecycleOperation(ctx context.Context, c client.Client, reader clie return false, err } } + if lifecycleRecoveryNeedsStop(op) { + r := &RestoreReconciler{Client: c, APIReader: reader, Scheme: c.Scheme()} + stopped, _, err := r.restoreTargetStopped(ctx, gs) + if err != nil || !stopped { + return false, err + } + } for _, predecessor := range op.Recovery { if !predecessor.NeedsUnquiesce { continue diff --git a/operator/internal/controller/restore_lifecycle.go b/operator/internal/controller/restore_lifecycle.go index f54d09756..91916ea54 100644 --- a/operator/internal/controller/restore_lifecycle.go +++ b/operator/internal/controller/restore_lifecycle.go @@ -262,11 +262,43 @@ func (r *RestoreReconciler) cleanupRestore(ctx context.Context, rs *gameplanev1a if err != nil { return ctrl.Result{}, err } + if !sameLifecycleOwner(op.lifecycleIdentity, restoreLifecycleIdentity(&gs, rs)) { + return ctrl.Result{}, fmt.Errorf("restore cleanup target has a different lifecycle owner") + } + if !success && (op.InitialStartup || !gs.Spec.Suspend) { + // A CSI restore may have started the game to bind its PVC. + // Retire that permission before asking the workload to stop; + // a replacement operation must not overlap the old process. + op.InitialStartup = false + gs.Spec.Suspend = true + gs.Annotations[lifecycleOperationAnnotation] = lifecycleOperationJSON(op) + if err := r.Update(ctx, &gs); err != nil { + return ctrl.Result{}, err + } + } ready, err := recoverLifecycleOperation(ctx, r.Client, r.apiReader(), &gs, op, r.OperationQuiescer) if err != nil || !ready { return ctrl.Result{RequeueAfter: 5 * time.Second}, err } } + if !success { + if !gs.Spec.Suspend { + gs.Spec.Suspend = true + if err := r.Update(ctx, &gs); err != nil { + return ctrl.Result{}, err + } + } + stopped, progressed, err := r.restoreTargetStopped(ctx, &gs) + if err != nil { + return ctrl.Result{}, err + } + if progressed { + return ctrl.Result{Requeue: true}, nil + } + if !stopped { + return ctrl.Result{RequeueAfter: 5 * time.Second}, nil + } + } if err := clearLifecycleOperation(ctx, r.Client, r.apiReader(), &gs, restoreLifecycleIdentity(&gs, rs)); err != nil { return ctrl.Result{}, err } diff --git a/operator/internal/controller/restore_startup_cleanup_test.go b/operator/internal/controller/restore_startup_cleanup_test.go new file mode 100644 index 000000000..5ac8e40e1 --- /dev/null +++ b/operator/internal/controller/restore_startup_cleanup_test.go @@ -0,0 +1,181 @@ +package controller + +import ( + "testing" + + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + gameplanev1alpha1 "github.com/GameplanePanel/gameplane/operator/api/v1alpha1" +) + +func startupCleanupFixture(t *testing.T) (*RestoreReconciler, *gameplanev1alpha1.GameServer, *gameplanev1alpha1.Restore, *appsv1.StatefulSet, *corev1.Pod) { + t.Helper() + r, gs, rs := restoreLifecycleFixture(t) + rs.Annotations = map[string]string{restoreTargetAnnotation: restoreIdentityJSON(gs.Name, gs.UID)} + rs.Finalizers = []string{restoreCleanupFinalizer} + if err := r.Update(t.Context(), rs); err != nil { + t.Fatal(err) + } + op := lifecycleOperation{lifecycleIdentity: restoreLifecycleIdentity(gs, rs)} + op.InitialStartup = true + gs.Spec.Suspend = false + gs.Annotations = map[string]string{restoreGuardAnnotation: restoreIdentityJSON(rs.Name, rs.UID), lifecycleOperationAnnotation: lifecycleOperationJSON(op)} + if err := r.Update(t.Context(), gs); err != nil { + t.Fatal(err) + } + ss := &appsv1.StatefulSet{} + if err := r.Get(t.Context(), client.ObjectKeyFromObject(gs), ss); err != nil { + t.Fatal(err) + } + one := int32(1) + ss.Spec.Replicas = &one + ss.Spec.Template.Spec.Containers = []corev1.Container{{Name: "game", Image: "restored-image"}} + if err := r.Update(t.Context(), ss); err != nil { + t.Fatal(err) + } + ss.Status.Replicas = 1 + ss.Status.ObservedGeneration = ss.Generation + if err := r.Status().Update(t.Context(), ss); err != nil { + t.Fatal(err) + } + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: gs.Name + "-0", Namespace: gs.Namespace, UID: "startup-pod", Finalizers: []string{"test/hold"}, OwnerReferences: []metav1.OwnerReference{{APIVersion: "apps/v1", Kind: "StatefulSet", Name: ss.Name, UID: ss.UID, Controller: ownerBoolPtr(true)}}}, Status: corev1.PodStatus{Phase: corev1.PodRunning}} + if err := r.Create(t.Context(), pod); err != nil { + t.Fatal(err) + } + return r, gs, rs, ss, pod +} + +func TestRestoreStartupCleanupDrainsBeforeRelease(t *testing.T) { + for _, outcome := range []string{"failed", "deleted", "succeeded"} { + t.Run(outcome, func(t *testing.T) { + r, gs, rs, ss, pod := startupCleanupFixture(t) + rs.Status.Phase = gameplanev1alpha1.RestorePhaseFailed + if outcome == "succeeded" { + rs.Status.Phase = gameplanev1alpha1.RestorePhaseSucceeded + } + if err := r.Status().Update(t.Context(), rs); err != nil { + t.Fatal(err) + } + if outcome == "deleted" { + if err := r.Delete(t.Context(), rs); err != nil { + t.Fatal(err) + } + if err := r.Get(t.Context(), client.ObjectKeyFromObject(rs), rs); err != nil { + t.Fatal(err) + } + } + res, err := r.cleanupRestore(t.Context(), rs) + if err != nil { + t.Fatal(err) + } + gs = lifecycleServer(t, r) + if outcome == "succeeded" { + if gs.Spec.Suspend || gs.Annotations[lifecycleOperationAnnotation] != "" || res.RequeueAfter != 0 { + t.Fatalf("successful startup was stopped: %+v %+v", gs, res) + } + return + } + op, held, err := lifecycleOwner(gs) + if err != nil || !held || op.InitialStartup || !gs.Spec.Suspend || res.RequeueAfter == 0 { + t.Fatalf("failed startup released or retained permission to start: %+v %+v %v", gs, res, err) + } + // An observed scale-down does not prove a terminating Pod drained. + zero := int32(0) + ss.Spec.Replicas = &zero + if err := r.Update(t.Context(), ss); err != nil { + t.Fatal(err) + } + ss.Status.Replicas, ss.Status.ObservedGeneration = 0, ss.Generation + if err := r.Status().Update(t.Context(), ss); err != nil { + t.Fatal(err) + } + if err := r.Delete(t.Context(), pod); err != nil { + t.Fatal(err) + } + if res, err := r.cleanupRestore(t.Context(), rs); err != nil || res.RequeueAfter == 0 || lifecycleServer(t, r).Annotations[lifecycleOperationAnnotation] == "" { + t.Fatalf("terminating startup Pod lost exclusion: %+v %v", res, err) + } + if err := r.Get(t.Context(), client.ObjectKeyFromObject(pod), pod); err != nil { + t.Fatal(err) + } + pod.Finalizers = nil + if err := r.Update(t.Context(), pod); err != nil { + t.Fatal(err) + } + for range 2 { + if _, err := r.cleanupRestore(t.Context(), rs); err != nil { + t.Fatal(err) + } + } + gs = lifecycleServer(t, r) + if !gs.Spec.Suspend || gs.Annotations[lifecycleOperationAnnotation] != "" || gs.Annotations[restoreGuardAnnotation] != "" { + t.Fatalf("drained failed startup did not settle suspended: %+v", gs) + } + }) + } +} + +func TestOrphanRestoreStartupDrainsBeforeNextOwnerProceeds(t *testing.T) { + r, gs, rs, ss, pod := startupCleanupFixture(t) + rs.Finalizers = nil + if err := r.Update(t.Context(), rs); err != nil { + t.Fatal(err) + } + if err := r.Delete(t.Context(), rs); err != nil { + t.Fatal(err) + } + b := guardBackup("next") + if err := r.Create(t.Context(), b); err != nil { + t.Fatal(err) + } + want := backupLifecycleIdentity(gs, b, false) + if ready, err := claimLifecycleOperation(t.Context(), r.Client, r.apiReader(), gs, want, nil); err != nil || ready { + t.Fatalf("orphan startup admitted another operation: %t %v", ready, err) + } + gr := &GameServerReconciler{Client: r.Client, APIReader: r.apiReader(), Scheme: r.Scheme} + if replicas, _, err := gr.desiredReplicas(t.Context(), gs, &gameplanev1alpha1.GameTemplate{}, idleAwake); err != nil || replicas != 0 { + t.Fatalf("orphan recovery did not request zero replicas: %d %v", replicas, err) + } + // Even a stale callback asking for one replica must stop the old process + // without changing the template before the successor owns clean data. + if err := gr.reconcileStatefulSet(t.Context(), gs, &gameplanev1alpha1.GameTemplate{}, nil, &materializedConfig{}, 1); err != nil { + t.Fatal(err) + } + if err := r.Get(t.Context(), client.ObjectKeyFromObject(ss), ss); err != nil { + t.Fatal(err) + } + if *ss.Spec.Replicas != 0 || ss.Spec.Template.Spec.Containers[0].Image != "restored-image" { + t.Fatalf("orphan recovery did not preserve the stopped template: %+v", ss.Spec) + } + ss.Status.Replicas, ss.Status.ObservedGeneration = 0, ss.Generation + if err := r.Status().Update(t.Context(), ss); err != nil { + t.Fatal(err) + } + if ready, err := claimLifecycleOperation(t.Context(), r.Client, r.apiReader(), gs, want, nil); err != nil || ready { + t.Fatalf("observed scale-down bypassed live startup Pod: %t %v", ready, err) + } + pod.Finalizers = nil + if err := r.Update(t.Context(), pod); err != nil { + t.Fatal(err) + } + if err := r.Delete(t.Context(), pod); err != nil { + t.Fatal(err) + } + ready := false + for range 2 { + var err error + ready, err = claimLifecycleOperation(t.Context(), r.Client, r.apiReader(), gs, want, nil) + if err != nil { + t.Fatal(err) + } + if ready { + break + } + } + if !ready { + t.Fatal("drained predecessor did not permit successor") + } +} diff --git a/operator/specs.md b/operator/specs.md index 88d065060..e6beff4a0 100644 --- a/operator/specs.md +++ b/operator/specs.md @@ -331,7 +331,7 @@ Primary reconcilers register with the manager in `cmd/main.go` and handle CRD li - Suspend: wait for Suspended/Stopped status plus the actual workload: the owned StatefulSet must exist, request zero replicas, report zero replicas, and have observed its current generation. An absent StatefulSet waits for workload reconciliation; a missing template or broken workload must be repaired before restore can proceed. No live or terminating game pod or pod mounting the data PVC may remain. Nonterminal Jobs mounting that PVC must also finish before a new restore Job starts. These checks use the uncached API reader and run again immediately before creating the Job. - The stopped StatefulSet also receives the owning Restore guard. A successful fence write immediately requeues the Restore to repeat the workload and writer checks; only external drainage waits use the polling delay. Its mutation callback honors that guard even when the GameServer cache snapshot predates acquisition, fencing delayed scale-up writes with the StatefulSet resourceVersion. Cleanup removes both guards after workers drain. - Restore Job: mount snapshot (from VolumeSnapshot or restic), run restic restore. Job carries BackoffLimit and ActiveDeadlineSeconds from the reconciler's JobBackoffLimit / JobActiveDeadlineSeconds fields (operator flags --backup-job-backoff-limit / --backup-job-active-deadline-seconds, Helm operator.backupJobBackoffLimit / operator.backupJobActiveDeadlineSeconds; defaults 2 and 86400, applied when unset; spec 018 OD-027); Job failure is recognized only from a Failed=True condition. Legacy in-flight Jobs without the pinned target annotation fail the Restore and are canceled/drained; operators can start a fresh Restore after cleanup. Pod SecurityContext takes FSGroup from the target GameTemplate's spec.security.fsGroup (fallback 65532 if template missing). - - Resume: once the restore Job succeeds and its live pods have drained, persist Succeeded before releasing the matching guard and clearing suspend in the same pass. This ordering survives interrupted status writes or operator restarts; it does not wait for the GameServer to reach Running again. Failure/deletion cancels active owned Jobs with foreground deletion and retains the guard/finalizer until Jobs and live worker pods drain, then leaves the target suspended. A missing or recreated guard owner can be recovered only after its UID's orphan Jobs and workers drain. Target recreation fails the old restore; cleanup never changes the replacement server. + - Resume: once the restore Job succeeds and its live pods have drained, persist Succeeded before releasing the matching guard and clearing suspend in the same pass. This ordering survives interrupted status writes or operator restarts; it does not wait for the GameServer to reach Running again. Failure/deletion cancels active owned Jobs with foreground deletion, retires CSI initial-startup permission and suspends the target while retaining the guard/finalizer. Release additionally requires an owned StatefulSet with desired/observed zero replicas at its current generation, no live or terminating game/PVC writer pods and no pending PVC writer Jobs. Orphan recovery applies the same stopped-target proof before admitting a successor, preserving the template while forcing zero replicas. Successful CSI restores keep their initial workload running. Target recreation fails the old restore; cleanup never changes the replacement server. - Status: report phase (Pending/Suspending/Running/Succeeded/Failed). `RestorePhaseResuming` is declared on the type but never assigned by this reconciler (F-260). - Volume-snapshot restore reference handling: When a volume-snapshot restore stands up a new GameServer from the original server's spec, any references in spec.env (SecretKeyRef, ConfigMapKeyRef) and spec.networking.tunnel.credentialsSecretRef that are owned by the original server must be copied to new objects owned by the restored server, with rewritten names, because the restored server does not own the originals. Ownership of a source object is an OwnerReference to the original server (Kind, Name, UID) and is re-verified against the source object immediately before each copy, not just once during planning. An object that already exists under a copy name is accepted only if the restored server controls it (a controller OwnerReference, `controller: true`); otherwise the restore fails. The restore fails before the new GameServer is created if the original server does not own every referenced object. The references are rewritten in the new GameServer's spec when it is created, and the copies are made right after creation and re-ensured on every reconcile pass. Only an ownership refusal fails the restore; transient errors (stale cache reads, write conflicts, a referenced object not yet visible in the cache) requeue it. A restore's 10-minute deadline starts when the restore first plans the new server (before it is created) and covers both creating the server with its reference copies and waiting for it to reach Running; it is enforced even while a transient error (such as a referenced object that stays missing) keeps recurring, so the restore cannot run past the limit, and once it expires the restore fails with a deadline message. From 2bc395d224ab29c86ec5e02fcfe333b6c46b8676 Mon Sep 17 00:00:00 2001 From: Baylem Date: Fri, 23 Oct 2026 00:19:18 -0400 Subject: [PATCH 07/12] test: reuse scheduled backup source fixture Signed-off-by: Baylem --- .../internal/controller/backup_envtest_test.go | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) diff --git a/operator/internal/controller/backup_envtest_test.go b/operator/internal/controller/backup_envtest_test.go index fd226d489..e67328880 100644 --- a/operator/internal/controller/backup_envtest_test.go +++ b/operator/internal/controller/backup_envtest_test.go @@ -8,11 +8,9 @@ import ( "testing" "time" - appsv1 "k8s.io/api/apps/v1" batchv1 "k8s.io/api/batch/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" gameplanev1alpha1 "github.com/GameplanePanel/gameplane/operator/api/v1alpha1" ) @@ -379,21 +377,9 @@ func TestBackup_QuiesceAfterWorkerPreparation(t *testing.T) { ns := newNamespace(t) fa := &fakeAgent{} startMgr(t, ns, withBackupReconciler(backupReconcilerOpts{agent: fa})) + // The shared fixture supplies the observed workload and its scheduled, + // running source Pod; envtest has no StatefulSet controller or kubelet. seedGameServer(t, ns, "smp") - // envtest has no StatefulSet controller or kubelet: provide the observed - // running source whose immutable identity is bound before the agent RPC. - var workload appsv1.StatefulSet - if err := k8sClient.Get(context.Background(), types.NamespacedName{Namespace: ns, Name: "smp"}, &workload); err != nil { - t.Fatal(err) - } - pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "smp-0", Namespace: ns, OwnerReferences: []metav1.OwnerReference{{APIVersion: "apps/v1", Kind: "StatefulSet", Name: workload.Name, UID: workload.UID, Controller: ownerBoolPtr(true)}}}, Spec: workload.Spec.Template.Spec} - if err := k8sClient.Create(context.Background(), pod); err != nil { - t.Fatal(err) - } - pod.Status.Phase = corev1.PodRunning - if err := k8sClient.Status().Update(context.Background(), pod); err != nil { - t.Fatal(err) - } if err := k8sClient.Create(context.Background(), buildResticRepoSecret(ns, "repo")); err != nil { t.Fatalf("create secret: %v", err) From 60af93ba561eb4b730e00f0aee00b47c9514f062 Mon Sep 17 00:00:00 2001 From: Baylem Date: Fri, 23 Oct 2026 00:19:17 -0400 Subject: [PATCH 08/12] test: preserve controller updates in lifecycle fixtures Signed-off-by: Baylem --- .../controller/helpers_envtest_test.go | 18 ++++++++++++------ .../controller/networkcapture_envtest_test.go | 19 +++++++++++++------ .../restore_startup_cleanup_test.go | 4 ++-- 3 files changed, 27 insertions(+), 14 deletions(-) diff --git a/operator/internal/controller/helpers_envtest_test.go b/operator/internal/controller/helpers_envtest_test.go index 0060b6886..0a18108ad 100644 --- a/operator/internal/controller/helpers_envtest_test.go +++ b/operator/internal/controller/helpers_envtest_test.go @@ -617,12 +617,18 @@ func markBackupSucceeded(t *testing.T, ns, name, snapshotID, size string) { // reconciler waits for Suspended/Stopped before advancing to Running. func markGameServerPhase(t *testing.T, ns, name string, phase gameplanev1alpha1.GameServerPhase) { t.Helper() - var gs gameplanev1alpha1.GameServer - if err := k8sClient.Get(context.Background(), types.NamespacedName{Namespace: ns, Name: name}, &gs); err != nil { - t.Fatalf("get gameserver: %v", err) - } - gs.Status.Phase = phase - if err := k8sClient.Status().Update(context.Background(), &gs); err != nil { + // The Restore reconciler also writes the target's ownership annotations. + // Refetch after conflicts so this simulated status observation preserves + // concurrent controller writes without turning them into fixture failures. + ctx := t.Context() + if err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + var gs gameplanev1alpha1.GameServer + if err := k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: name}, &gs); err != nil { + return err + } + gs.Status.Phase = phase + return k8sClient.Status().Update(ctx, &gs) + }); err != nil { t.Fatalf("status update gameserver: %v", err) } } diff --git a/operator/internal/controller/networkcapture_envtest_test.go b/operator/internal/controller/networkcapture_envtest_test.go index f7cf85007..5c5d2e12d 100644 --- a/operator/internal/controller/networkcapture_envtest_test.go +++ b/operator/internal/controller/networkcapture_envtest_test.go @@ -510,12 +510,19 @@ func TestNetworkCapture_UserStopCallsSidecar(t *testing.T) { // Emulate the API's StopNetworkCapture: patch status directly to // Completed with its exact message, bypassing the reconciler. - nc := getNetworkCapture(t, ns, "cap-stop-001") - now := metav1.NewTime(time.Now().UTC()) - nc.Status.Phase = gameplanev1alpha1.CapturePhaseCompleted - nc.Status.CompletionTime = &now - nc.Status.Message = "stopped by user request" - if err := k8sClient.Status().Update(context.Background(), nc); err != nil { + // The active reconciler may refresh capture progress between Get and Update. + ctx := t.Context() + if err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + var nc gameplanev1alpha1.NetworkCapture + if err := k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: "cap-stop-001"}, &nc); err != nil { + return err + } + now := metav1.NewTime(time.Now().UTC()) + nc.Status.Phase = gameplanev1alpha1.CapturePhaseCompleted + nc.Status.CompletionTime = &now + nc.Status.Message = "stopped by user request" + return k8sClient.Status().Update(ctx, &nc) + }); err != nil { t.Fatalf("patch capture to user-stopped: %v", err) } diff --git a/operator/internal/controller/restore_startup_cleanup_test.go b/operator/internal/controller/restore_startup_cleanup_test.go index 5ac8e40e1..78da66458 100644 --- a/operator/internal/controller/restore_startup_cleanup_test.go +++ b/operator/internal/controller/restore_startup_cleanup_test.go @@ -51,7 +51,7 @@ func startupCleanupFixture(t *testing.T) (*RestoreReconciler, *gameplanev1alpha1 func TestRestoreStartupCleanupDrainsBeforeRelease(t *testing.T) { for _, outcome := range []string{"failed", "deleted", "succeeded"} { t.Run(outcome, func(t *testing.T) { - r, gs, rs, ss, pod := startupCleanupFixture(t) + r, _, rs, ss, pod := startupCleanupFixture(t) rs.Status.Phase = gameplanev1alpha1.RestorePhaseFailed if outcome == "succeeded" { rs.Status.Phase = gameplanev1alpha1.RestorePhaseSucceeded @@ -71,7 +71,7 @@ func TestRestoreStartupCleanupDrainsBeforeRelease(t *testing.T) { if err != nil { t.Fatal(err) } - gs = lifecycleServer(t, r) + gs := lifecycleServer(t, r) if outcome == "succeeded" { if gs.Spec.Suspend || gs.Annotations[lifecycleOperationAnnotation] != "" || res.RequeueAfter != 0 { t.Fatalf("successful startup was stopped: %+v %+v", gs, res) From 3b9ab20803ddbf8476dddbdd5fa5d0deabddad5e Mon Sep 17 00:00:00 2001 From: Baylem Date: Fri, 23 Oct 2026 00:19:17 -0400 Subject: [PATCH 09/12] test: synchronize closed RCON response fixtures Signed-off-by: Baylem --- agent/internal/rcon/nuclearoption_test.go | 29 +++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/agent/internal/rcon/nuclearoption_test.go b/agent/internal/rcon/nuclearoption_test.go index 248c6a7fb..9061d2c94 100644 --- a/agent/internal/rcon/nuclearoption_test.go +++ b/agent/internal/rcon/nuclearoption_test.go @@ -1275,18 +1275,47 @@ type pipeFrameCase struct { forbidErr string // substring the error must NOT contain (e.g. a detail suffix) } +type responseDeadlineConn struct { + net.Conn + deadlineSet chan<- struct{} +} + +func (c *responseDeadlineConn) SetDeadline(deadline time.Time) error { + if err := c.Conn.SetDeadline(deadline); err != nil { + return err + } + select { + case c.deadlineSet <- struct{}{}: + default: + } + return nil +} + // runPipeFrameCases sends each case's raw frame back to the client over net.Pipe // and checks the body or the error. func runPipeFrameCases(t *testing.T, tests []pipeFrameCase) { t.Helper() for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + deadlineSet := make(chan struct{}, 1) client := newPipeClient(t, func(remote net.Conn) { if _, _, err := readNuclearOptionRequest(remote); err != nil { return } + if len(tt.frame) == 0 { + // Exec sets its response deadline after sending the request. + // Closing first can fail SetDeadline instead of the status read. + select { + case <-deadlineSet: + case <-t.Context().Done(): + return + } + } writeFrameSegments(remote, tt.frame, tt.oneByte) }) + if len(tt.frame) == 0 { + client.conn = &responseDeadlineConn{Conn: client.conn, deadlineSet: deadlineSet} + } body, err := client.Exec("test-command") if tt.wantErr != "" { From a35332ef485d3f6e3743c2f9cda45411e1e46c3a Mon Sep 17 00:00:00 2001 From: Baylem Date: Sat, 24 Oct 2026 10:53:50 -0400 Subject: [PATCH 10/12] test: verify shared PID survives restricted capture Signed-off-by: Baylem --- test/e2e/restricted_workloads_e2e_test.go | 6 ++++++ test/e2e/specs.md | 2 ++ 2 files changed, 8 insertions(+) diff --git a/test/e2e/restricted_workloads_e2e_test.go b/test/e2e/restricted_workloads_e2e_test.go index 823b1d403..703e5991c 100644 --- a/test/e2e/restricted_workloads_e2e_test.go +++ b/test/e2e/restricted_workloads_e2e_test.go @@ -74,6 +74,9 @@ func TestRestrictedWorkloads_NodeCaptureAndTokenRotation(t *testing.T) { if pod.Spec.AutomountServiceAccountToken == nil || *pod.Spec.AutomountServiceAccountToken || len(pod.Spec.InitContainers) != 1 || len(pod.Spec.EphemeralContainers) != 0 { t.Fatalf("unexpected restricted pod: %+v", pod.Spec) } + if pod.Spec.ShareProcessNamespace == nil || !*pod.Spec.ShareProcessNamespace { + t.Fatal("restricted profile must retain shared PID for agent metrics") + } for _, c := range append(pod.Spec.Containers, pod.Spec.InitContainers...) { sc := c.SecurityContext if sc == nil || sc.RunAsNonRoot == nil || !*sc.RunAsNonRoot || sc.AllowPrivilegeEscalation == nil || *sc.AllowPrivilegeEscalation || sc.Capabilities == nil || len(sc.Capabilities.Add) != 0 || len(sc.Capabilities.Drop) != 1 || sc.Capabilities.Drop[0] != "ALL" || sc.SeccompProfile == nil || sc.SeccompProfile.Type != corev1.SeccompProfileTypeRuntimeDefault { @@ -237,6 +240,9 @@ func TestRestrictedWorkloads_NodeCaptureAndTokenRotation(t *testing.T) { if live.UID != pod.UID || len(live.Spec.EphemeralContainers) != 0 { t.Fatal("capture restarted or injected into game pod") } + if live.Spec.ShareProcessNamespace == nil || !*live.Spec.ShareProcessNamespace { + t.Fatal("capture changed the shared PID profile required for agent metrics") + } for i, c := range live.Status.ContainerStatuses { if c.RestartCount != pod.Status.ContainerStatuses[i].RestartCount { t.Fatal("capture restarted game/agent container") diff --git a/test/e2e/specs.md b/test/e2e/specs.md index 4b19583f7..898ba175d 100644 --- a/test/e2e/specs.md +++ b/test/e2e/specs.md @@ -18,6 +18,8 @@ opt-in, traffic-bearing PCAP download, unchanged game Pod UID/restart counts, worker cleanup on completion/revocation, and cross-namespace resource cleanup after GameServer deletion. Local work compiles test sources only; CI executes the drill and all unit/chart regressions. +The drill explicitly verifies that shared PID remains enabled for agent metrics +after the Restricted upgrade and after capture completion and revocation. The `test/e2e` module is Gameplane's end-to-end test suite: it stands up a real (kind-based) Kubernetes cluster, installs the Helm chart, and drives the operator/api/agent/sentinel stack as real components rather than mocks — CRDs applied through the typed/dynamic client, HTTP calls through a real port-forward, RCON/wire-protocol joins against real game server containers. Where the other modules' test tiers (unit, envtest) prove a component's own logic in isolation, this module proves the whole product works together on a cluster shaped like the ones users actually run. From 318742554148026d90032b016b6e0da32c0d866a Mon Sep 17 00:00:00 2001 From: Baylem Date: Sat, 24 Oct 2026 11:00:07 -0400 Subject: [PATCH 11/12] fix: allow guarded wipe recovery to drain workloads Signed-off-by: Baylem --- .../controller/gameserver_controller.go | 10 + .../gameserver_lifecycle_reconcile_test.go | 365 ++++++++++++++++++ .../internal/controller/gameserver_wipe.go | 10 +- .../controller/gameserver_wipe_guard.go | 23 +- operator/specs.md | 10 +- 5 files changed, 411 insertions(+), 7 deletions(-) create mode 100644 operator/internal/controller/gameserver_lifecycle_reconcile_test.go diff --git a/operator/internal/controller/gameserver_controller.go b/operator/internal/controller/gameserver_controller.go index 9ff7f336e..002da6c2f 100644 --- a/operator/internal/controller/gameserver_controller.go +++ b/operator/internal/controller/gameserver_controller.go @@ -38,6 +38,10 @@ import ( gameplanev1alpha1 "github.com/GameplanePanel/gameplane/operator/api/v1alpha1" ) +// Lifecycle annotation conflicts need a fresh reconcile, not an error backoff. +// Give concurrent writers and the informer cache a bounded interval to settle. +const lifecycleConflictRetry = time.Second + // GameServerReconciler reconciles a GameServer object into a StatefulSet, // Service, and PVC. The agent sidecar is injected at pod-spec build time. type GameServerReconciler struct { @@ -360,6 +364,9 @@ func (r *GameServerReconciler) Reconcile(ctx context.Context, req ctrl.Request) // single status patch further down. idle, idleStatus, idleRequeue, idleScheduleErr, err := r.reconcileIdle(ctx, &gs) if err != nil { + if apierrors.IsConflict(err) { + return ctrl.Result{RequeueAfter: lifecycleConflictRetry}, nil + } logger.Error(err, "reconcile idle") return ctrl.Result{}, err } @@ -409,6 +416,9 @@ func (r *GameServerReconciler) Reconcile(ctx context.Context, req ctrl.Request) replicas, stopRequeue, err := r.desiredReplicas(ctx, &gs, &tmpl, idle, &wipeState) if err != nil { + if apierrors.IsConflict(err) { + return ctrl.Result{RequeueAfter: lifecycleConflictRetry}, nil + } logger.Error(err, "compute desired replicas") return ctrl.Result{}, err } diff --git a/operator/internal/controller/gameserver_lifecycle_reconcile_test.go b/operator/internal/controller/gameserver_lifecycle_reconcile_test.go new file mode 100644 index 000000000..3be7f0010 --- /dev/null +++ b/operator/internal/controller/gameserver_lifecycle_reconcile_test.go @@ -0,0 +1,365 @@ +package controller + +import ( + "context" + "crypto/rand" + "crypto/rsa" + "crypto/x509" + "encoding/pem" + "errors" + "fmt" + "reflect" + "testing" + "time" + + "github.com/go-logr/logr" + appsv1 "k8s.io/api/apps/v1" + batchv1 "k8s.io/api/batch/v1" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/util/cert" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + "sigs.k8s.io/controller-runtime/pkg/log" + + gameplanev1alpha1 "github.com/GameplanePanel/gameplane/operator/api/v1alpha1" +) + +func lifecycleGameServerFixture(t *testing.T, gs *gameplanev1alpha1.GameServer, extras ...client.Object) *GameServerReconciler { + t.Helper() + s := wipeScheme(t) + if err := clientgoscheme.AddToScheme(s); err != nil { + t.Fatal(err) + } + tmpl := &gameplanev1alpha1.GameTemplate{ObjectMeta: metav1.ObjectMeta{Name: gs.Spec.TemplateRef.Name}, Spec: gameplanev1alpha1.GameTemplateSpec{Image: "example.invalid/game:unit"}} + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatal(err) + } + ca, err := cert.NewSelfSignedCACert(cert.Config{CommonName: "lifecycle-reconcile-test"}, key) + if err != nil { + t.Fatal(err) + } + caSecret := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "agent-ca", Namespace: gs.Namespace}, Data: map[string][]byte{ + "ca.crt": pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: ca.Raw}), + "ca.key": pem.EncodeToMemory(&pem.Block{Type: "RSA PRIVATE KEY", Bytes: x509.MarshalPKCS1PrivateKey(key)}), + }} + objects := append([]client.Object{gs, tmpl, caSecret}, extras...) + c := fake.NewClientBuilder().WithScheme(s).WithObjects(objects...). + WithStatusSubresource(gs, &appsv1.StatefulSet{}, &batchv1.Job{}).Build() + r := &GameServerReconciler{Client: c, APIReader: c, Scheme: s, AgentCASecretName: caSecret.Name, AgentCASecretNamespace: caSecret.Namespace} + // These regressions start after template admission so their first public + // pass exercises lifecycle recovery or the intended annotation conflict. + revision, err := r.saveTemplateSnapshot(t.Context(), gs, tmpl) + if err != nil { + t.Fatal(err) + } + if gs.Annotations == nil { + gs.Annotations = map[string]string{} + } + gs.Annotations[effectiveTemplateAnnotation] = revision + if err := c.Update(t.Context(), gs); err != nil { + t.Fatal(err) + } + op, held, err := lifecycleOwner(gs) + if err != nil { + t.Fatal(err) + } + if err := r.reportTemplateRevision(t.Context(), gs, revision, op, held); err != nil { + t.Fatal(err) + } + return r +} + +func requireNoWipeJob(t *testing.T, r *GameServerReconciler, gs *gameplanev1alpha1.GameServer) { + t.Helper() + var job batchv1.Job + if err := r.Get(t.Context(), client.ObjectKey{Namespace: gs.Namespace, Name: gs.Name + "-wipe"}, &job); !apierrors.IsNotFound(err) { + t.Fatalf("wipe launched before recovery completed: %v", err) + } +} + +func TestGameServerWipeReconcileDrainsOrphanRestoreStartup(t *testing.T) { + gs := wipeGameServer(false, "wipe") + predecessor := lifecycleIdentity{Kind: "Restore", Name: "deleted", UID: "deleted-restore", TargetUID: gs.UID, InitialStartup: true} + gs.Annotations[lifecycleOperationAnnotation] = lifecycleOperationJSON(lifecycleOperation{lifecycleIdentity: predecessor}) + gs.Annotations[restoreGuardAnnotation] = restoreIdentityJSON(predecessor.Name, predecessor.UID) + ss := wipeStoppedSet(gs) + one := int32(1) + ss.Spec.Replicas, ss.Status.Replicas = &one, 1 + ss.Spec.Template.Spec.Containers = []corev1.Container{{Name: "game", Image: "restored-image"}} + originalTemplate := ss.Spec.Template.DeepCopy() + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: gs.Name + "-0", Namespace: gs.Namespace, UID: "startup", Finalizers: []string{"test/hold"}, + OwnerReferences: []metav1.OwnerReference{{APIVersion: "apps/v1", Kind: "StatefulSet", Name: ss.Name, UID: ss.UID, Controller: ownerBoolPtr(true)}}}, Status: corev1.PodStatus{Phase: corev1.PodRunning}} + r := lifecycleGameServerFixture(t, gs, ss, pod) + request := ctrl.Request{NamespacedName: client.ObjectKeyFromObject(gs)} + // Admission persists the successor. The next public pass must reach the + // StatefulSet controller instead of waiting for its own scale-down. + for range 2 { + if _, err := r.Reconcile(t.Context(), request); err != nil { + t.Fatal(err) + } + } + current := wipeCurrent(t, r.Client, gs) + op, held, err := lifecycleOwner(current) + if err != nil || !held || op.Kind != "Wipe" || len(op.Recovery) != 1 || op.Recovery[0].UID != predecessor.UID || !current.Spec.Suspend { + t.Fatalf("orphan recovery lost its identity or exclusion: %+v %v", current, err) + } + if err := r.Get(t.Context(), client.ObjectKeyFromObject(ss), ss); err != nil { + t.Fatal(err) + } + if ss.Spec.Replicas == nil || *ss.Spec.Replicas != 0 || !reflect.DeepEqual(ss.Spec.Template, *originalTemplate) { + t.Fatalf("recovery did not scale down with the original template: %+v", ss.Spec) + } + requireNoWipeJob(t, r, gs) + ss.Status.Replicas, ss.Status.ObservedGeneration = 0, ss.Generation + if err := r.Status().Update(t.Context(), ss); err != nil { + t.Fatal(err) + } + if err := r.Delete(t.Context(), pod); err != nil { + t.Fatal(err) + } + if _, err := r.Reconcile(t.Context(), request); err != nil { + t.Fatal(err) + } + requireNoWipeJob(t, r, gs) + current = wipeCurrent(t, r.Client, gs) + op, held, err = lifecycleOwner(current) + if err != nil || !held || len(op.Recovery) != 1 { + t.Fatalf("terminating startup pod lost the recovery fence: %+v %v", current, err) + } + if err := r.Get(t.Context(), client.ObjectKeyFromObject(pod), pod); err != nil { + t.Fatal(err) + } + pod.Finalizers = nil + if err := r.Update(t.Context(), pod); err != nil { + t.Fatal(err) + } + for range 3 { + if _, err := r.Reconcile(t.Context(), request); err != nil { + t.Fatal(err) + } + } + var job batchv1.Job + if err := r.Get(t.Context(), client.ObjectKey{Namespace: gs.Namespace, Name: gs.Name + "-wipe"}, &job); err != nil { + t.Fatalf("drained predecessor did not permit wipe: %v", err) + } + current = wipeCurrent(t, r.Client, gs) + op, held, err = lifecycleOwner(current) + if err != nil || !held || op.Kind != "Wipe" || len(op.Recovery) != 0 || !current.Spec.Suspend || job.Labels[wipeTokenLabel] != "wipe" { + t.Fatalf("wipe launch lost exclusive ownership: %+v %v", current, err) + } +} + +func TestGameServerWipeReconcileWaitsForPredecessorCleanup(t *testing.T) { + gs := wipeGameServer(true, "wipe") + gs.Annotations[wipeGuardAnnotation] = "wipe" + predecessor := lifecycleIdentity{Kind: "Backup", Name: "deleted", UID: "deleted-backup", TargetUID: gs.UID, NeedsUnquiesce: true} + gs.Annotations[lifecycleOperationAnnotation] = lifecycleOperationJSON(lifecycleOperation{lifecycleIdentity: wipeLifecycleIdentity(gs, "wipe"), Recovery: []lifecycleIdentity{predecessor}}) + // A labelled worker still counts even if its Job and PVC mount metadata + // are gone. A stopped target alone does not complete predecessor cleanup. + worker := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "orphan-copy", Namespace: gs.Namespace, Labels: map[string]string{backupUIDLabel: string(predecessor.UID)}}, Status: corev1.PodStatus{Phase: corev1.PodRunning}} + gamePod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: gs.Name + "-0", Namespace: gs.Namespace, UID: "original-game"}, Status: corev1.PodStatus{Phase: corev1.PodRunning}} + r := lifecycleGameServerFixture(t, gs, wipeStoppedSet(gs), worker, gamePod) + cleanupErr := errors.New("save-on unavailable") + quiescer := &scrapeQuiescer{unquiesceErr: cleanupErr} + r.OperationQuiescer = quiescer + request := ctrl.Request{NamespacedName: client.ObjectKeyFromObject(gs)} + if _, err := r.Reconcile(t.Context(), request); err != nil { + t.Fatal(err) + } + requireNoWipeJob(t, r, gs) + if err := r.Delete(t.Context(), worker); err != nil { + t.Fatal(err) + } + if _, err := r.Reconcile(t.Context(), request); !errors.Is(err, cleanupErr) { + t.Fatalf("unfinished save-on did not retain its error: %v", err) + } + requireNoWipeJob(t, r, gs) + current := wipeCurrent(t, r.Client, gs) + op, held, err := lifecycleOwner(current) + if err != nil || !held || len(op.Recovery) != 1 || !current.Spec.Suspend { + t.Fatalf("unfinished save-on lost its recovery fence: %+v %v", current, err) + } + quiescer.unquiesceErr = nil + if _, err := r.Reconcile(t.Context(), request); err != nil { + t.Fatal(err) + } + requireNoWipeJob(t, r, gs) + if err := r.Delete(t.Context(), gamePod); err != nil { + t.Fatal(err) + } + for range 2 { + if _, err := r.Reconcile(t.Context(), request); err != nil { + t.Fatal(err) + } + } + var job batchv1.Job + if err := r.Get(t.Context(), client.ObjectKey{Namespace: gs.Namespace, Name: gs.Name + "-wipe"}, &job); err != nil { + t.Fatalf("completed save-on did not permit wipe: %v", err) + } + if quiescer.unquiesced != 1 { + t.Fatalf("save-on completed %d times, want once", quiescer.unquiesced) + } +} + +func TestWipeRecoveryRetainsCleanupFences(t *testing.T) { + for _, mode := range []string{"cancel", "replace", "success"} { + t.Run(mode, func(t *testing.T) { + gs := wipeGameServer(true, "wipe") + gs.Annotations[wipeGuardAnnotation] = "wipe" + predecessor := lifecycleIdentity{Kind: "Backup", Name: "deleted", UID: "deleted-backup", TargetUID: gs.UID} + gs.Annotations[lifecycleOperationAnnotation] = lifecycleOperationJSON(lifecycleOperation{lifecycleIdentity: wipeLifecycleIdentity(gs, "wipe"), Recovery: []lifecycleIdentity{predecessor}}) + ss := wipeStoppedSet(gs) + worker := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "orphan-copy", Namespace: gs.Namespace, Labels: map[string]string{backupUIDLabel: string(predecessor.UID)}}, Status: corev1.PodStatus{Phase: corev1.PodRunning}} + objects := []client.Object{gs, ss, worker} + switch mode { + case "cancel": + delete(gs.Annotations, WipeRequestedAnnotation) + case "replace": + gs.Annotations[WipeRequestedAnnotation] = "replacement" + case "success": + gs.Annotations[wipeSuccessAnnotation] = "wipe" + objects = append(objects, &batchv1.Job{ObjectMeta: metav1.ObjectMeta{Name: gs.Name + "-wipe", Namespace: gs.Namespace, Labels: map[string]string{wipeTokenLabel: "wipe"}, + OwnerReferences: []metav1.OwnerReference{{APIVersion: gameplanev1alpha1.GroupVersion.String(), Kind: "GameServer", Name: gs.Name, UID: gs.UID, Controller: ownerBoolPtr(true)}}}, Status: batchv1.JobStatus{Succeeded: 1}}) + } + s := wipeScheme(t) + c := fake.NewClientBuilder().WithScheme(s).WithObjects(objects...).Build() + r := &GameServerReconciler{Client: c, APIReader: c, Scheme: s} + if err := r.reconcileWipe(t.Context(), gs, &gameplanev1alpha1.GameTemplate{}); err != nil { + t.Fatal(err) + } + current := wipeCurrent(t, c, gs) + op, held, err := lifecycleOwner(current) + if err != nil || !held || op.Token != "wipe" || len(op.Recovery) != 1 || current.Annotations[wipeGuardAnnotation] != "wipe" || current.Annotations[WipeCompletedAnnotation] != "" || !current.Spec.Suspend { + t.Fatalf("cleanup changed unfinished recovery: %+v %v", current, err) + } + if err := c.Get(t.Context(), client.ObjectKeyFromObject(ss), ss); err != nil || ss.Annotations[wipeGuardAnnotation] != "wipe" { + t.Fatalf("cleanup removed the workload fence: %+v %v", ss, err) + } + if mode == "success" { + var job batchv1.Job + if err := c.Get(t.Context(), client.ObjectKey{Namespace: gs.Namespace, Name: gs.Name + "-wipe"}, &job); err != nil || !job.DeletionTimestamp.IsZero() { + t.Fatalf("success cleanup ran before predecessor drained: %+v %v", job, err) + } + } else { + requireNoWipeJob(t, r, gs) + } + }) + } +} + +func TestWipeLaunchRevalidatesDurableOwnerAfterStopProof(t *testing.T) { + for _, mode := range []string{"recovery", "other owner", "missing owner", "malformed owner"} { + t.Run(mode, func(t *testing.T) { + gs := wipeGameServer(true, "wipe") + gs.Annotations[wipeGuardAnnotation] = "wipe" + gs.Annotations[lifecycleOperationAnnotation] = lifecycleOperationJSON(lifecycleOperation{lifecycleIdentity: wipeLifecycleIdentity(gs, "wipe")}) + s := wipeScheme(t) + c := fake.NewClientBuilder().WithScheme(s).WithObjects(gs, wipeStoppedSet(gs)).Build() + observedWorkload, changedOwner := false, false + reader := interceptor.NewClient(c, interceptor.Funcs{Get: func(ctx context.Context, cl client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if err := cl.Get(ctx, key, obj, opts...); err != nil { + return err + } + if _, ok := obj.(*appsv1.StatefulSet); ok { + observedWorkload = true + } + live, ok := obj.(*gameplanev1alpha1.GameServer) + if !ok || !observedWorkload || changedOwner { + return nil + } + changedOwner = true + op := lifecycleOperation{lifecycleIdentity: wipeLifecycleIdentity(live, "wipe")} + switch mode { + case "recovery": + op.Recovery = []lifecycleIdentity{{Kind: "Backup", Name: "predecessor", UID: "predecessor", TargetUID: gs.UID, NeedsUnquiesce: true}} + case "other owner": + op.lifecycleIdentity = lifecycleIdentity{Kind: "Restore", Name: "replacement", UID: "replacement", TargetUID: gs.UID} + } + live.Annotations[lifecycleOperationAnnotation] = lifecycleOperationJSON(op) + if mode == "missing owner" { + delete(live.Annotations, lifecycleOperationAnnotation) + } else if mode == "malformed owner" { + live.Annotations[lifecycleOperationAnnotation] = "invalid" + } + return cl.Update(ctx, live) + }}) + r := &GameServerReconciler{Client: c, APIReader: reader, Scheme: s} + err := r.reconcileWipe(t.Context(), gs, &gameplanev1alpha1.GameTemplate{}) + if mode == "malformed owner" { + if err == nil { + t.Fatal("malformed owner did not fail closed") + } + } else if err != nil { + t.Fatal(err) + } + if !changedOwner { + t.Fatal("did not reach the live owner read after stop proof") + } + requireNoWipeJob(t, r, gs) + }) + } +} + +type lifecycleErrorLog struct{ errors *[]error } + +func (*lifecycleErrorLog) Init(logr.RuntimeInfo) {} +func (*lifecycleErrorLog) Enabled(int) bool { return true } +func (*lifecycleErrorLog) Info(int, string, ...any) {} +func (l *lifecycleErrorLog) WithValues(...any) logr.LogSink { return l } +func (l *lifecycleErrorLog) WithName(string) logr.LogSink { return l } +func (l *lifecycleErrorLog) Error(err error, _ string, _ ...any) { *l.errors = append(*l.errors, err) } + +func TestGameServerLifecyclePatchConflictsRetryQuietly(t *testing.T) { + for _, transition := range []string{"idle", "restart"} { + for _, conflict := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/conflict=%t", transition, conflict), func(t *testing.T) { + gs := wipeGameServer(false, "") + if transition == "idle" { + // Disabling idle while asleep requests an annotation clear. + gs.Annotations[IdleAsleepSinceAnnotation] = time.Now().Add(-time.Hour).UTC().Format(time.RFC3339) + } else { + gs.Annotations[RestartRequestedAnnotation] = "restart" + } + r := lifecycleGameServerFixture(t, gs) + cause := errors.New("API unavailable") + var patchErr error = cause + if conflict { + patchErr = apierrors.NewConflict(schema.GroupResource{Group: "gameplane.local", Resource: "gameservers"}, gs.Name, cause) + } + patchErr = fmt.Errorf("write response: %w", patchErr) + patches := 0 + r.Client = interceptor.NewClient(r.Client.(client.WithWatch), interceptor.Funcs{Patch: func(ctx context.Context, c client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + if _, ok := obj.(*gameplanev1alpha1.GameServer); ok { + patches++ + return patchErr + } + return c.Patch(ctx, obj, patch, opts...) + }}) + var logged []error + ctx := log.IntoContext(t.Context(), logr.New(&lifecycleErrorLog{errors: &logged})) + result, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(gs)}) + if patches != 1 { + t.Fatalf("lifecycle patch was not reached exactly once: %d", patches) + } + if conflict { + if err != nil || result != (ctrl.Result{RequeueAfter: time.Second}) || len(logged) != 0 { + t.Fatalf("expected quiet one-second conflict retry: result=%+v error=%v logs=%v", result, err, logged) + } + } else if !errors.Is(err, cause) || result != (ctrl.Result{}) || len(logged) != 1 || !errors.Is(logged[0], cause) { + t.Fatalf("real error lost its identity/log/backoff: result=%+v error=%v logs=%v", result, err, logged) + } + var ss appsv1.StatefulSet + if err := r.Get(t.Context(), client.ObjectKeyFromObject(gs), &ss); !apierrors.IsNotFound(err) { + t.Fatalf("failed lifecycle patch still mutated the workload: %v", err) + } + }) + } + } +} diff --git a/operator/internal/controller/gameserver_wipe.go b/operator/internal/controller/gameserver_wipe.go index c59ecf998..e3414336a 100644 --- a/operator/internal/controller/gameserver_wipe.go +++ b/operator/internal/controller/gameserver_wipe.go @@ -74,6 +74,11 @@ func (r *GameServerReconciler) reconcileWipe( if changed { return nil } + if guard != "" { + if ready, err := wipeLifecycleReady(gs, guard); err != nil || !ready { + return err + } + } if guard == "" { if len(state.jobs) == 0 { return nil @@ -137,9 +142,12 @@ func (r *GameServerReconciler) reconcileWipe( if err := r.wipeReader().Get(ctx, client.ObjectKeyFromObject(gs), &live); err != nil { return client.IgnoreNotFound(err) } - if live.UID != gs.UID || live.Annotations[wipeGuardAnnotation] != guard || live.Annotations[WipeRequestedAnnotation] != guard || !pendingWipe(&live) || live.Annotations[restoreGuardAnnotation] != "" { + if live.UID != gs.UID || live.Annotations[wipeGuardAnnotation] != guard || live.Annotations[WipeRequestedAnnotation] != guard || !pendingWipe(&live) || live.Annotations[restoreGuardAnnotation] != "" || live.Annotations[backupGuardAnnotation] != "" { return nil } + if ready, err := wipeLifecycleReady(&live, guard); err != nil || !ready { + return err + } return r.createWipeJob(ctx, &live, tmpl, jobName, guard) } diff --git a/operator/internal/controller/gameserver_wipe_guard.go b/operator/internal/controller/gameserver_wipe_guard.go index bdf00bfc8..45a6becc4 100644 --- a/operator/internal/controller/gameserver_wipe_guard.go +++ b/operator/internal/controller/gameserver_wipe_guard.go @@ -166,10 +166,11 @@ func (r *GameServerReconciler) ensureWipeGuard(ctx context.Context, gs *gameplan } *gs = live if token := gs.Annotations[wipeGuardAnnotation]; token != "" { - ready, err := claimLifecycleOperation(ctx, r.Client, r.wipeReader(), gs, wipeLifecycleIdentity(gs, token), r.OperationQuiescer) - // Upgrading an existing guard does not require another pass. Only - // unfinished predecessor cleanup must prevent this wipe proceeding. - return err == nil && !ready, err + _, err := claimLifecycleOperation(ctx, r.Client, r.wipeReader(), gs, wipeLifecycleIdentity(gs, token), r.OperationQuiescer) + // Existing ownership must not skip workload reconciliation: orphaned + // Restore recovery may need it to scale down the initial startup pod. + // reconcileWipe separately blocks cleanup and launch until recovery ends. + return false, err } state := wipeInventory(inventory) if err := r.loadWipeWorkers(ctx, gs, state); err != nil { @@ -196,6 +197,20 @@ func (r *GameServerReconciler) ensureWipeGuard(ctx context.Context, gs *gameplan return err == nil && gs.Annotations[wipeGuardAnnotation] == token, err } +// wipeLifecycleReady requires the durable owner, not just its compatibility +// guard. Pending predecessors still own cleanup obligations and cannot be +// discarded by wipe completion, cancellation, replacement, or a new worker. +func wipeLifecycleReady(gs *gameplanev1alpha1.GameServer, token string) (bool, error) { + if gs.Annotations[lifecycleOperationAnnotation] == "" { + return false, nil + } + op, held, err := lifecycleOwner(gs) + if err != nil { + return false, err + } + return held && sameLifecycleOwner(op.lifecycleIdentity, wipeLifecycleIdentity(gs, token)) && len(op.Recovery) == 0, nil +} + // Stamp the owned workload with a checked write before workers launch. A stale // workload update then conflicts, while callbacks reading the fence stay at 0. func (r *GameServerReconciler) wipeTargetStopped(ctx context.Context, gs *gameplanev1alpha1.GameServer) (bool, error) { diff --git a/operator/specs.md b/operator/specs.md index e6beff4a0..f297ea1da 100644 --- a/operator/specs.md +++ b/operator/specs.md @@ -228,7 +228,7 @@ Primary reconcilers register with the manager in `cmd/main.go` and handle CRD li - Node affinity: honor spec.nodeSelector and spec.affinity preferences. - Ingress NetworkPolicy: enforce per-template advertised ports, admit CIDRs from `--game-ingress-from-cidr`. - Fixed-name child cleanup: Delete fixed-name objects (`-config` Secret, `-files` Secret, `-rcon` Secret, `-auto` BackupSchedule, `-wipe` Job) only when the GameServer controls them via OwnerReference; unowned objects with these names stay in place (same rule as game-ingress NetworkPolicy). Wipe cleanup carries the verified Job UID as a delete precondition; a foreign Job cannot acknowledge a pending wipe or resume its server. - - **Conflict requeue:** When reconcileStatefulSet or reconcileWipe encounters an apierrors.IsConflict error (concurrent update races), requeue without error logging — conflicts are expected and transient. The Module, NetworkCapture and Backup reconcilers apply the same rule to every error they return, via `requeueOnConflict` (`conflict.go`). + - **Conflict requeue:** When reconcileStatefulSet or reconcileWipe encounters an apierrors.IsConflict error (concurrent update races), requeue without error logging — conflicts are expected and transient. Idle-transition and restart-acknowledgment conflicts, including wrapped errors, also return before logging and retry after one second with fresh state. Other errors retain their identity, error logging and controller-runtime backoff. The Module, NetworkCapture and Backup reconcilers apply the conflict rule to every error they return, via `requeueOnConflict` (`conflict.go`). - Load-balancer address management: translate spec.networking.addressPool / .address preferences onto the Service based on the cluster's address-manager flavor (MetalLB, Cilium, or none), report assignment status via the AddressAssignment condition. - **Network capture foundation** (**Phase 2 Foundational**): Pre-provision a `captures` emptyDir (1 GiB) unconditionally on every game pod's StatefulSet template (required because pod.spec.volumes is immutable on running pods; a rolling restart of all existing game pods is incurred once on upgrade). Extend the `-agent` Service with a second numeric ServicePort 9091 for the capture sidecar's control endpoint. RBAC markers grant `pods/ephemeralcontainers` access (get, list, watch, patch, update) and full CRUD on `networkcaptures` resources. The capture sidecar injection as an ephemeral container is implemented: `buildCaptureEphemeralContainer` (`gameserver_controller.go`) is the single source of truth for the container spec, injected eagerly by `GameServerReconciler.reconcileCapture` when `spec.capture.enabled` is set, with `NetworkCaptureReconciler.injectCaptureContainer` as an idempotent fallback for the case where its own cache is ahead of the GameServer reconciler's write. - **Split concerns:** @@ -360,7 +360,13 @@ lineage is retained on the GameServer across repeated force deletion. Owned Jobs are canceled with UID preconditions and foreground propagation; independently inventoried live or terminating worker pods and pending CSI captures retain the guard. Saving is restored only after recovery workers drain. The workload remains -frozen during recovery even when the successor is a Restore or wipe. Terminal +frozen during recovery even when the successor is a Restore or wipe, except +that an orphaned Restore's initial workload must scale to zero with its template +preserved. An existing wipe guard does not skip this workload reconciliation. +Wipe cancellation, replacement, success cleanup and worker creation wait until +all predecessor workers and quiesce cleanup finish. Immediately before worker +creation, an uncached read must still show the same durable wipe owner with no +pending recovery; the compatibility guard alone cannot authorize a launch. Terminal state and existing cleanup intent are preserved before releasing the owner; failed/canceled restores remain suspended. Generation changes to active Backup or Restore specs fail the operation and enter cleanup rather than authorizing a From ee8b145b3981937cb6a10ddc44b13e963b9e33f3 Mon Sep 17 00:00:00 2001 From: Baylem Date: Sun, 25 Oct 2026 19:12:40 -0400 Subject: [PATCH 12/12] fix: carry reviewed security and chart prerequisites Signed-off-by: Baylem --- charts/gameplane/templates/NOTES.txt | 6 ++ .../templates/operator-strategy-migrate.yaml | 2 +- charts/gameplane/templates/operator.yaml | 4 +- charts/gameplane/templates/validate.yaml | 3 - charts/tests/check_workloads.py | 9 ++- docs/production-placement.md | 7 +- docs/release-notes.md | 5 +- .../controller/gameserver_files_test.go | 11 ++- .../controller/networkcapture_node.go | 48 +++++++++--- .../controller/networkcapture_node_test.go | 74 ++++++++++++++++++- operator/specs.md | 7 +- test/e2e/backup_stopped_e2e_test.go | 7 +- 12 files changed, 154 insertions(+), 29 deletions(-) diff --git a/charts/gameplane/templates/NOTES.txt b/charts/gameplane/templates/NOTES.txt index 6f5d24139..bac02868a 100644 --- a/charts/gameplane/templates/NOTES.txt +++ b/charts/gameplane/templates/NOTES.txt @@ -4,6 +4,12 @@ persistence and dashboard blocks are absent. */ -}} {{- $r := .Values.api.telemetry.receiver }} +{{- if and (eq (include "gameplane.operatorEnabled" .) "true") (gt (int .Values.operator.replicas) 1) (not .Values.operator.leaderElect) }} + +Operator leader election is enabled automatically because operator.replicas is +greater than one. The saved leaderElect=false value remains compatible with +upgrades; all operator replicas coordinate through the leader-election Lease. +{{- end }} {{- if .Values.api.standalone }} Standalone central panel: no local game cluster or Kubernetes API credentials. diff --git a/charts/gameplane/templates/operator-strategy-migrate.yaml b/charts/gameplane/templates/operator-strategy-migrate.yaml index e376fca4b..c6b51ef85 100644 --- a/charts/gameplane/templates/operator-strategy-migrate.yaml +++ b/charts/gameplane/templates/operator-strategy-migrate.yaml @@ -1,6 +1,6 @@ {{- /* SSA cannot remove a server-defaulted rollingUpdate field. Patch it before Helm applies Recreate, including installations that manage CRDs externally. */ -}} -{{- if and .Release.IsUpgrade (eq (include "gameplane.operatorEnabled" .) "true") (not .Values.operator.leaderElect) }} +{{- if and .Release.IsUpgrade (eq (include "gameplane.operatorEnabled" .) "true") (not (or .Values.operator.leaderElect (gt (int .Values.operator.replicas) 1))) }} {{- $existing := lookup "apps/v1" "Deployment" .Release.Namespace "gameplane-operator" }} {{- if $existing }} {{- $name := printf "%s-op-roll" .Release.Name }} diff --git a/charts/gameplane/templates/operator.yaml b/charts/gameplane/templates/operator.yaml index 142d2840a..86af4cece 100644 --- a/charts/gameplane/templates/operator.yaml +++ b/charts/gameplane/templates/operator.yaml @@ -244,7 +244,7 @@ metadata: labels: {{- include "gameplane.labels" . | nindent 4 }} spec: replicas: {{ .Values.operator.replicas }} - {{- if not .Values.operator.leaderElect }} + {{- if not (or .Values.operator.leaderElect (gt (int .Values.operator.replicas) 1)) }} # Without leader election, prevent a rolling update from overlapping writers. strategy: type: Recreate @@ -279,7 +279,7 @@ spec: imagePullPolicy: {{ .Values.image.pullPolicy }} args: {{- include "gameplane.auxiliaryResourceArgs" .Values.operator | nindent 12 }} - {{- if .Values.operator.leaderElect }} + {{- if or .Values.operator.leaderElect (gt (int .Values.operator.replicas) 1) }} - --leader-elect {{- end }} - --agent-image={{ include "gameplane.agentImage" . }} diff --git a/charts/gameplane/templates/validate.yaml b/charts/gameplane/templates/validate.yaml index d29173bc8..12e28a855 100644 --- a/charts/gameplane/templates/validate.yaml +++ b/charts/gameplane/templates/validate.yaml @@ -13,9 +13,6 @@ {{- if not (regexMatch "^v1\\.[0-9]+$" (.Values.podSecurity.version | default "v1.34")) }} {{- fail "podSecurity.version must pin a Kubernetes minor such as v1.34" }} {{- end }} -{{- if and (eq (include "gameplane.operatorEnabled" .) "true") (gt (int .Values.operator.replicas) 1) (not .Values.operator.leaderElect) }} -{{- fail "operator.leaderElect must be enabled with more than one replica" }} -{{- end }} {{- if .Values.api.standalone }} {{- if ne (include "gameplane.apiEnabled" .) "true" }} {{- fail "api.standalone requires api.enabled=true" }} diff --git a/charts/tests/check_workloads.py b/charts/tests/check_workloads.py index a4c433528..9d8e4f860 100644 --- a/charts/tests/check_workloads.py +++ b/charts/tests/check_workloads.py @@ -39,6 +39,8 @@ def check_operator_migration(): assert not any(obj["kind"] == "Job" for obj in render(values, release=release, chart=chart)) values["operator"]["leaderElect"] = True assert not any(obj["kind"] == "Job" for obj in render(values, upgrade=True, release=release, chart=chart)) + values["operator"].update(replicas=2, leaderElect=False) + assert not any(obj["kind"] == "Job" for obj in render(values, upgrade=True, release=release, chart=chart)) def main(): @@ -81,7 +83,10 @@ def main(): assert overrides["--agent-resources"] == {"requests": {"cpu": "80m"}} render({"api": {"pdb": {"enabled": True, "minAvailable": 1}}}, failure="api.pdb must allow the singleton API to be evicted") render({"api": {"pdb": {"enabled": True, "maxUnavailable": 0}}}, failure="api.pdb must allow the singleton API to be evicted") - render({"operator": {"replicas": 2, "leaderElect": False}}, failure="operator.leaderElect must be enabled") + upgraded = render({"operator": {"replicas": 2, "leaderElect": False}}, upgrade=True) + operator = one(upgraded, "Deployment", "gameplane-operator") + assert operator["spec"]["strategy"]["type"] == "RollingUpdate" + assert "--leader-elect" in operator["spec"]["template"]["spec"]["containers"][0]["args"] single = render({"operator": {"leaderElect": False}}) assert one(single, "Deployment", "gameplane-operator")["spec"]["strategy"] == {"type": "Recreate", "rollingUpdate": None} removed = render({"operator": {"agentResources": None, "tunnelResources": {}}}) @@ -100,12 +105,14 @@ def main(): old_chart = Path(directory) / "chart" shutil.copytree(CHART, old_chart) old_values = yaml.safe_load((old_chart / "values.yaml").read_text()) + old_values["operator"].update(replicas=2, leaderElect=False) for key in ["agentResources", "sentinelResources", "tunnelResources", "backupResources", "pdb", *placement]: old_values["operator"].pop(key, None) (old_chart / "values.yaml").write_text(yaml.safe_dump(old_values)) old = render(chart=old_chart) args = one(old, "Deployment", "gameplane-operator")["spec"]["template"]["spec"]["containers"][0]["args"] assert not any(arg.startswith("--tunnel-resources=") for arg in args) + assert "--leader-elect" in args if __name__ == "__main__": diff --git a/docs/production-placement.md b/docs/production-placement.md index 175d23f3a..9bdd26dab 100644 --- a/docs/production-placement.md +++ b/docs/production-placement.md @@ -82,9 +82,10 @@ cause API downtime. A PDB is not an availability guarantee and does not prevent node failures. The persistent telemetry receiver is also a singleton; avoid a zero-unavailable budget unless intentionally accepting a blocked drain. -Keep operator leader election enabled with redundant replicas and rolling -updates. The chart rejects multiple replicas with leader election disabled; -with one replica and election disabled it uses Recreate to avoid overlapping +Redundant operator replicas use leader election and rolling updates. The chart +automatically enables election when replicas exceed one, even when an older +release retained `leaderElect: false`; upgrades continue rendering and install +notes explain the effective setting. With one replica and election disabled it uses Recreate to avoid overlapping reconcilers during upgrades. API replica/process restrictions remain unchanged. When switching an existing operator to Recreate, a pre-upgrade hook clears the diff --git a/docs/release-notes.md b/docs/release-notes.md index f3da01f11..ff07383fa 100644 --- a/docs/release-notes.md +++ b/docs/release-notes.md @@ -16,6 +16,9 @@ keys use the defaults even with `--reuse-values`. Chart-managed pods now accept placement, topology-spread and priority settings, with optional PDBs for Deployments. API remains a singleton and its PDB uses `maxUnavailable: 1`; node drains can cause API downtime. Operator replicas greater -than one require leader election. Disabling election on a singleton uses Recreate. +than one automatically enable leader election, including upgrades retaining +`leaderElect: false` through `--reuse-values`; that old setting no longer fails +chart rendering. Install notes explain the effective behavior. Disabling +election on a singleton uses Recreate. See [production placement](production-placement.md) for upgrade controls and a worker-pool example. diff --git a/operator/internal/controller/gameserver_files_test.go b/operator/internal/controller/gameserver_files_test.go index 4eb17f521..d1d530332 100644 --- a/operator/internal/controller/gameserver_files_test.go +++ b/operator/internal/controller/gameserver_files_test.go @@ -102,6 +102,13 @@ func TestBuildConfigInitContainer_GroupWritableWithFSGroup(t *testing.T) { if !strings.Contains(arg, "stat -c %g") || !strings.Contains(arg, "stat -c %a") { t.Errorf("already-correct ownership and modes must be checked before mutation: %q", arg) } + if !strings.Contains(arg, `'/data/'"$p"`) { + t.Errorf("copied paths must keep the relative path quoted: %q", arg) + } + groupCheck, groupChange := strings.Index(arg, "stat -c %g"), strings.Index(arg, "chgrp ") + if groupCheck < 0 || groupChange < 0 || groupCheck >= groupChange { + t.Errorf("group ownership must be checked before chgrp: %q", arg) + } // Verify the command contains "find ./*" if !strings.Contains(arg, "find ./*") { @@ -122,7 +129,7 @@ func TestBuildConfigInitContainer_ReusesSharedFilesWithoutOwnership(t *testing.T t.Fatal(err) } } - source := filepath.Join(staging, "cfg", "profile.txt") + source := filepath.Join(staging, "cfg", "profile with spaces.txt") if err := os.WriteFile(source, []byte("first"), 0o600); err != nil { t.Fatal(err) } @@ -160,7 +167,7 @@ func TestBuildConfigInitContainer_ReusesSharedFilesWithoutOwnership(t *testing.T if out, err := command.CombinedOutput(); err != nil { t.Fatalf("repeat copy required ownership of writable files: %v %s", err, out) } - if got, err := os.ReadFile(filepath.Join(data, "cfg", "profile.txt")); err != nil || string(got) != "updated" { + if got, err := os.ReadFile(filepath.Join(data, "cfg", "profile with spaces.txt")); err != nil || string(got) != "updated" { t.Fatalf("config was not replaced: %q %v", got, err) } if info, err := os.Stat(unrelated); err != nil || info.Mode().Perm() != 0o600 { diff --git a/operator/internal/controller/networkcapture_node.go b/operator/internal/controller/networkcapture_node.go index ab6231797..944fbe10b 100644 --- a/operator/internal/controller/networkcapture_node.go +++ b/operator/internal/controller/networkcapture_node.go @@ -301,6 +301,31 @@ func (r *NetworkCaptureReconciler) nodeFail(ctx context.Context, nc *gameplanev1 return r.fail(ctx, nc, message) } +// A recorded annotation identifies cleanup obligations, not permission to +// create privileged resources. Revalidate deployment authority on every pass. +func (r *NetworkCaptureReconciler) validateNodeCaptureNamespace(ctx context.Context, nc *gameplanev1alpha1.NetworkCapture) (string, error) { + if bound := nc.Annotations[nodeCaptureNamespaceAnnotation]; bound != "" && bound != r.DiagnosticsNamespace { + return "node capture annotation must match the configured diagnostics namespace", nil + } + if r.DiagnosticsNamespace == "" { + return "node diagnostics require cluster deployment opt-in and a configured diagnostics namespace", nil + } + if r.DiagnosticsNamespace == nc.Namespace || r.DiagnosticsNamespace == r.AgentCASecretNamespace { + return "node diagnostics namespace must be separate from the games and control-plane namespaces", nil + } + var namespace corev1.Namespace + if err := r.nodeReader().Get(ctx, types.NamespacedName{Name: r.DiagnosticsNamespace}, &namespace); err != nil { + if apierrors.IsNotFound(err) { + return "configured diagnostics namespace no longer exists", nil + } + return "", err + } + if namespace.Labels["pod-security.kubernetes.io/enforce"] != "privileged" { + return "diagnostics namespace must explicitly permit privileged node diagnostics", nil + } + return "", nil +} + func (r *NetworkCaptureReconciler) reconcileNodeCapture(ctx context.Context, nc *gameplanev1alpha1.NetworkCapture) (ctrl.Result, error) { if nc.DeletionTimestamp != nil { clean, err := r.cleanupNodeCapture(ctx, nc) @@ -316,6 +341,13 @@ func (r *NetworkCaptureReconciler) reconcileNodeCapture(ctx context.Context, nc controllerutil.RemoveFinalizer(nc, nodeCaptureFinalizer) return ctrl.Result{}, r.Update(ctx, nc) } + namespaceFailure, err := r.validateNodeCaptureNamespace(ctx, nc) + if err != nil { + return ctrl.Result{}, err + } + if namespaceFailure != "" && !isTerminalCapturePhase(nc.Status.Phase) { + return r.nodeFail(ctx, nc, namespaceFailure) + } if isTerminalCapturePhase(nc.Status.Phase) { drained, err := r.deleteNodeWorkers(ctx, nc) if err != nil { @@ -332,7 +364,9 @@ func (r *NetworkCaptureReconciler) reconcileNodeCapture(ctx context.Context, nc if time.Now().Before(nc.Status.CompletionTime.Add(time.Duration(ttl) * time.Second)) { // Retained artifacts need certificate rotation even after the target // game Pod has gone away. No target or worker is recreated here. - if ns := nc.Annotations[nodeCaptureNamespaceAnnotation]; ns != "" { + // Revoked namespace authority permits cleanup, never fresh writes + // such as certificate rotation. Preserve the original retention clock. + if ns := nc.Annotations[nodeCaptureNamespaceAnnotation]; ns != "" && namespaceFailure == "" { var anchor corev1.ConfigMap if err := r.nodeReader().Get(ctx, types.NamespacedName{Namespace: ns, Name: nodeCaptureName(nc)}, &anchor); err != nil { if !apierrors.IsNotFound(err) { @@ -409,17 +443,7 @@ func (r *NetworkCaptureReconciler) reconcileNodeCapture(ctx context.Context, nc if bound := nc.Annotations[capturePodUIDAnnotation]; bound != "" && (bound != string(pod.UID) || nc.Annotations[nodeCaptureNodeAnnotation] != pod.Spec.NodeName || nc.Annotations[nodeCaptureContainerAnnotation] != containerID) { return r.nodeFail(ctx, nc, "target Pod, node or container identity changed") } - if nc.Annotations[nodeCaptureNamespaceAnnotation] == "" { - if r.DiagnosticsNamespace == gs.Namespace { - return r.nodeFail(ctx, nc, "node diagnostics namespace must be separate from the games namespace") - } - var namespace corev1.Namespace - if err := r.nodeReader().Get(ctx, types.NamespacedName{Name: r.DiagnosticsNamespace}, &namespace); err != nil { - return ctrl.Result{}, err - } - if namespace.Labels["pod-security.kubernetes.io/enforce"] != "privileged" { - return r.nodeFail(ctx, nc, "diagnostics namespace must explicitly permit privileged node diagnostics") - } + if nc.Annotations[capturePodUIDAnnotation] == "" || !controllerutil.ContainsFinalizer(nc, nodeCaptureFinalizer) { var captures gameplanev1alpha1.NetworkCaptureList if err := r.nodeReader().List(ctx, &captures, client.InNamespace(nc.Namespace)); err != nil { return ctrl.Result{}, err diff --git a/operator/internal/controller/networkcapture_node_test.go b/operator/internal/controller/networkcapture_node_test.go index ddd9a333d..8ff025291 100644 --- a/operator/internal/controller/networkcapture_node_test.go +++ b/operator/internal/controller/networkcapture_node_test.go @@ -63,6 +63,77 @@ func nodeReconcilerFixture(t *testing.T, objects ...client.Object) *NetworkCaptu return &NetworkCaptureReconciler{Client: c, APIReader: c, Scheme: scheme, CaptureEnabled: true, RestrictedWorkloads: true, DiagnosticsNamespace: "diagnostics"} } +func TestNodeCaptureValidatesNamespaceOnEveryReconcile(t *testing.T) { + for _, test := range []struct { + name, configured, recorded, enforcement, controlPlane, want string + }{ + {"prefilled-foreign", "diagnostics", "foreign", "privileged", "system", "configured diagnostics namespace"}, + {"same-as-games", "games", "games", "privileged", "system", "separate"}, + {"same-as-control-plane", "system", "system", "privileged", "system", "separate"}, + {"psa-revoked", "diagnostics", "diagnostics", "restricted", "system", "explicitly permit privileged"}, + {"psa-omitted", "diagnostics", "diagnostics", "", "system", "explicitly permit privileged"}, + } { + t.Run(test.name, func(t *testing.T) { + nc := &gameplanev1alpha1.NetworkCapture{ObjectMeta: metav1.ObjectMeta{Name: "cap", Namespace: "games", UID: "capture-uid", Annotations: map[string]string{nodeCaptureNamespaceAnnotation: test.recorded}}, Status: gameplanev1alpha1.NetworkCaptureStatus{Phase: gameplanev1alpha1.CapturePhaseRunning}} + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: test.configured, Labels: map[string]string{"pod-security.kubernetes.io/enforce": test.enforcement}}} + r := nodeReconcilerFixture(t, nc, ns) + r.DiagnosticsNamespace = test.configured + r.AgentCASecretNamespace = test.controlPlane + if _, err := r.reconcileNodeCapture(t.Context(), nc); err != nil { + t.Fatal(err) + } + var live gameplanev1alpha1.NetworkCapture + if err := r.Get(t.Context(), client.ObjectKeyFromObject(nc), &live); err != nil { + t.Fatal(err) + } + if live.Status.Phase != gameplanev1alpha1.CapturePhaseFailed || !strings.Contains(live.Status.Message, test.want) { + t.Fatalf("namespace authorization bypassed: %+v", live.Status) + } + var anchors corev1.ConfigMapList + if err := r.List(t.Context(), &anchors); err != nil || len(anchors.Items) != 0 { + t.Fatalf("invalid namespace acquired artifacts: %v %+v", err, anchors) + } + }) + } +} + +func TestNodeCaptureNamespaceRevocationDrainsBeforeFailure(t *testing.T) { + nc := &gameplanev1alpha1.NetworkCapture{ObjectMeta: metav1.ObjectMeta{Name: "cap", Namespace: "games", UID: "capture-uid", Annotations: map[string]string{nodeCaptureNamespaceAnnotation: "diagnostics"}}, Status: gameplanev1alpha1.NetworkCaptureStatus{Phase: gameplanev1alpha1.CapturePhaseRunning}} + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "diagnostics", Labels: map[string]string{"pod-security.kubernetes.io/enforce": "restricted"}}} + worker := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "worker", Namespace: ns.Name, Labels: map[string]string{nodeCaptureUIDLabel: string(nc.UID), nodeCaptureComponentLabel: "worker"}}} + r := nodeReconcilerFixture(t, nc, ns, worker) + if result, err := r.reconcileNodeCapture(t.Context(), nc); err != nil || result.RequeueAfter == 0 { + t.Fatalf("revocation did not wait for worker drain: %+v %v", result, err) + } + if nc.Status.Phase != gameplanev1alpha1.CapturePhaseRunning { + t.Fatal("failure published before worker drain") + } + if _, err := r.reconcileNodeCapture(t.Context(), nc); err != nil { + t.Fatal(err) + } + if nc.Status.Phase != gameplanev1alpha1.CapturePhaseFailed { + t.Fatalf("revocation not reported: %+v", nc.Status) + } +} + +func TestNodeCaptureRevokedNamespacePreservesTerminalRetention(t *testing.T) { + completed := metav1.NewTime(time.Now().Add(-time.Minute)) + nc := &gameplanev1alpha1.NetworkCapture{ObjectMeta: metav1.ObjectMeta{Name: "cap", Namespace: "games", UID: "capture-uid", Annotations: map[string]string{nodeCaptureNamespaceAnnotation: "diagnostics"}}, Status: gameplanev1alpha1.NetworkCaptureStatus{Phase: gameplanev1alpha1.CapturePhaseFailed, CompletionTime: &completed}} + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "diagnostics", Labels: map[string]string{"pod-security.kubernetes.io/enforce": "restricted"}}} + anchor := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: nodeCaptureName(nc), Namespace: ns.Name, Labels: map[string]string{nodeCaptureUIDLabel: string(nc.UID)}}} + r := nodeReconcilerFixture(t, nc, ns, anchor) + if result, err := r.reconcileNodeCapture(t.Context(), nc); err != nil || result.RequeueAfter == 0 { + t.Fatalf("revocation interrupted terminal retention: %+v %v", result, err) + } + if !nc.Status.CompletionTime.Equal(&completed) { + t.Fatal("revocation restarted the retention clock") + } + var secrets corev1.SecretList + if err := r.List(t.Context(), &secrets); err != nil || len(secrets.Items) != 0 { + t.Fatalf("revoked namespace received certificate writes: %v %+v", err, secrets) + } +} + type staleCaptureListClient struct{ client.Client } func (c staleCaptureListClient) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { @@ -141,7 +212,8 @@ func TestNodeCaptureRefusesWithoutServerOptIn(t *testing.T) { gs := &gameplanev1alpha1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "game", Namespace: "games", UID: "server-uid"}, Spec: gameplanev1alpha1.GameServerSpec{Capture: &gameplanev1alpha1.CaptureConfiguration{Enabled: true}}} nc := &gameplanev1alpha1.NetworkCapture{ObjectMeta: metav1.ObjectMeta{Name: "cap", Namespace: "games", UID: "capture-uid"}} nc.Spec.ServerRef.Name = gs.Name - r := nodeReconcilerFixture(t, gs, nc) + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "diagnostics", Labels: map[string]string{"pod-security.kubernetes.io/enforce": "privileged"}}} + r := nodeReconcilerFixture(t, gs, nc, ns) if _, err := r.reconcileNodeCapture(t.Context(), nc); err != nil { t.Fatal(err) } diff --git a/operator/specs.md b/operator/specs.md index f297ea1da..6fe4d52c1 100644 --- a/operator/specs.md +++ b/operator/specs.md @@ -22,7 +22,12 @@ Existing volumes still need compatible ownership before enabling Restricted. Restricted Pods never receive the legacy capture ephemeral container. `spec.capture.nodeDiagnostic` additionally requires cluster capture enablement and `--diagnostics-namespace`. The dedicated namespace must explicitly enforce -privileged and be separate from games/control plane. Live owner UIDs establish +privileged and be separate from games/control plane. Every reconcile checks +the recorded namespace against the configured namespace and rechecks live PSA +labels and namespace separation. Revocation drains workers before failure; +terminal/deleting captures retain cleanup but cannot rotate certificates in an +unauthorized namespace. A prefilled namespace annotation alone never skips +target binding or cleanup finalization. Live owner UIDs establish GameServer→StatefulSet→Pod provenance; Pod UID, node and exact containerd ID are pinned before worker creation. A target change drains work before releasing the capture lock. A bounded, no-retry Job has hostPID and only SYS_ADMIN, diff --git a/test/e2e/backup_stopped_e2e_test.go b/test/e2e/backup_stopped_e2e_test.go index d469a635d..e02258db2 100644 --- a/test/e2e/backup_stopped_e2e_test.go +++ b/test/e2e/backup_stopped_e2e_test.go @@ -21,7 +21,10 @@ import ( func TestBackup_StoppedCopiesRemainRestorable(t *testing.T) { t.Parallel() ctx := context.Background() - ns, gs, tmpl := "gameplane-games", "e2e-stopped-backup", "e2e-stopped-backup-template" + suffix := time.Now().UnixNano() + ns := "gameplane-games" + gs := fmt.Sprintf("e2e-stopped-backup-%d", suffix) + tmpl := fmt.Sprintf("e2e-stopped-backup-template-%d", suffix) ensureResticRepo(t) applyBusyboxTemplate(t, tmpl) applyBusyboxGameServer(t, ns, gs, tmpl) @@ -56,7 +59,7 @@ func TestBackup_StoppedCopiesRemainRestorable(t *testing.T) { return apierrors.IsNotFound(err), fmt.Sprintf("pod absence: %v", err) }) for i := 1; i <= 2; i++ { - name := fmt.Sprintf("e2e-stopped-copy-%d", i) + name := fmt.Sprintf("e2e-stopped-copy-%d-%d", suffix, i) backup := &unstructured.Unstructured{Object: map[string]any{ "apiVersion": "gameplane.local/v1alpha1", "kind": "Backup", "metadata": map[string]any{"name": name, "namespace": ns},