diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index c3762378c..abfb1c906 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -443,7 +443,8 @@ jobs: needs: [changes] if: (needs.changes.outputs.images == 'true') || contains(fromJSON(needs.changes.outputs.ci-jobs || '[]'), 'build-images') runs-on: ubuntu-latest - timeout-minutes: 15 + # Include all image builds, cache exports, artifact upload and cleanup. + timeout-minutes: 30 permissions: contents: read steps: diff --git a/api/internal/handlers/resources_annotations_test.go b/api/internal/handlers/resources_annotations_test.go index 457d5ebfb..a615dc2fe 100644 --- a/api/internal/handlers/resources_annotations_test.go +++ b/api/internal/handlers/resources_annotations_test.go @@ -39,6 +39,7 @@ func TestGenericServerWritesCannotInjectLifecycleAnnotations(t *testing.T) { wipeRequestedAnnotation: "injected", "gameplane.local/wipe-data-completed": "forged", "gameplane.local/restore-guard": "forged", + "operations.gameplane.local/owner": "forged", "gameserver.gameplane.local/stop-requested-at": "forged", "restore.gameplane.local/created-by": "forged", ownerIDAnnotation: "999", @@ -51,7 +52,7 @@ func TestGenericServerWritesCannotInjectLifecycleAnnotations(t *testing.T) { t.Fatalf("write failed: %d %s", response.Code, response.Body) } got := serverAnnotations(t, k, "alpha") - for _, key := range []string{wipeRequestedAnnotation, "gameplane.local/wipe-data-completed", "gameplane.local/restore-guard", "gameserver.gameplane.local/stop-requested-at", "restore.gameplane.local/created-by"} { + for _, key := range []string{wipeRequestedAnnotation, "gameplane.local/wipe-data-completed", "gameplane.local/restore-guard", "operations.gameplane.local/owner", "gameserver.gameplane.local/stop-requested-at", "restore.gameplane.local/created-by"} { if _, found := got[key]; found { t.Errorf("generic write injected %s", key) } @@ -98,6 +99,7 @@ func TestGenericServerUpdatePreservesControllerState(t *testing.T) { wipeRequestedAnnotation: "pending", "gameplane.local/wipe-data-completed": "previous", "gameplane.local/backup-guard": "active-backup", + "operations.gameplane.local/owner": `{"kind":"Backup","name":"copy","uid":"copy-uid","targetUID":"server-uid","generation":1}`, "gameserver.gameplane.local/stop-requested-at": "original-time", "restore.gameplane.local/created-by": "original-restore", } diff --git a/docs/operator-operations.md b/docs/operator-operations.md new file mode 100644 index 000000000..1a90d084e --- /dev/null +++ b/docs/operator-operations.md @@ -0,0 +1,49 @@ +# Backup, restore and wipe ownership + +Gameplane serializes these operations per GameServer. All backups participate, +including backups with `quiesce: false` and CSI VolumeSnapshots. A held operation +also defers idle sleep/wake, restart, and pod-template rollout. Requests remain +pending and reconciliation resumes after the operation's cleanup completes. + +The GameServer annotation `operations.gameplane.local/owner` identifies the +operation kind, name, UID, admitted generation and target UID. A wipe is identified +by the GameServer UID and its request token. The existing backup, restore and wipe +owner annotations remain compatibility markers. A `recovery` array identifies +older owners whose workers or quiesce obligations must be cleaned up first. + +Deleting an operation requests cleanup. Restic Jobs are canceled with foreground +deletion; live and terminating worker pods must drain before the owner is released. +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. + +If an operation waits unexpectedly, inspect the referenced Backup/Restore, its +conditions, Jobs and worker pods. Resolve terminating workers, repository failures +or agent connectivity before retrying. Removing owner annotations or force-removing +finalizers cannot establish that a worker has stopped. A failed or canceled restore +leaves its target suspended for inspection. + +Upgrades recognize existing owner annotations and import backup recovery lineage. +Already-running legacy quiesced copies continue being observed until their workers +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. + +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. + +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 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. diff --git a/operator/api/v1alpha1/backup_types.go b/operator/api/v1alpha1/backup_types.go index d093916f4..0caeed81a 100644 --- a/operator/api/v1alpha1/backup_types.go +++ b/operator/api/v1alpha1/backup_types.go @@ -17,11 +17,10 @@ const ( BackupPhaseFailed BackupPhase = "Failed" ) -// BackupFinalizer blocks a quiesced Backup's deletion until the operator has -// sent the matching unquiesce. Without it, deleting a Backup while -// quiesce-attempted=true drops the unquiesce entirely and the game is left -// with auto-save off indefinitely (F-048). Added only for Backups with -// spec.quiesce=true; see backup_forget.go's finalizeDelete. +// BackupFinalizer blocks deletion until copy workers have drained and any +// matching unquiesce has completed. Every Backup carries it, including raw +// copies and CSI snapshots, so deletion cannot release shared lifecycle +// ownership while a copy is still using the target. See finalizeDelete. const BackupFinalizer = "gameplane.local/backup-finalizer" // BackupSnapshotFinalizer blocks the deletion of a restic-strategy Backup until diff --git a/operator/cmd/main.go b/operator/cmd/main.go index 9b1f125af..191752d14 100644 --- a/operator/cmd/main.go +++ b/operator/cmd/main.go @@ -401,6 +401,7 @@ func main() { AgentCASecretName: agentCASecretName, AgentCASecretNamespace: agentCASecretNamespace, AgentClient: agentClient, + OperationQuiescer: agentClient, PodAttacher: &controller.StopAttachClient{ Config: mgr.GetConfig(), Clientset: kubernetes.NewForConfigOrDie(mgr.GetConfig()), @@ -448,6 +449,7 @@ func main() { Client: mgr.GetClient(), APIReader: mgr.GetAPIReader(), Scheme: mgr.GetScheme(), + OperationQuiescer: agentClient, ResticImage: resticImage, JobBackoffLimit: &backupJobBackoff32, JobActiveDeadlineSeconds: backupJobActiveDeadlineSeconds, diff --git a/operator/internal/controller/backup_controller.go b/operator/internal/controller/backup_controller.go index b350a5448..c579713d9 100644 --- a/operator/internal/controller/backup_controller.go +++ b/operator/internal/controller/backup_controller.go @@ -247,14 +247,12 @@ func (r *BackupReconciler) reconcile(ctx context.Context, req ctrl.Request) (ctr if !b.DeletionTimestamp.IsZero() { return r.finalizeDelete(ctx, &b) } - // Only Backups that might quiesce the world need to block on releasing - // it before deletion; everything else deletes immediately. - if b.Spec.Quiesce && !controllerutil.ContainsFinalizer(&b, gameplanev1alpha1.BackupFinalizer) { + // Every copy owns the source until its workers drain, even without quiesce. + if !controllerutil.ContainsFinalizer(&b, gameplanev1alpha1.BackupFinalizer) { controllerutil.AddFinalizer(&b, gameplanev1alpha1.BackupFinalizer) if err := r.Update(ctx, &b); err != nil { return ctrl.Result{}, err } - return ctrl.Result{}, nil } // Restic Backups own a snapshot in the repository, so hold their deletion @@ -288,6 +286,19 @@ func (r *BackupReconciler) reconcile(ctx context.Context, req ctrl.Request) (ctr b.Status.Phase == gameplanev1alpha1.BackupPhaseFailed { return r.runUnquiesce(ctx, &b) } + // Observe pre-upgrade workers even when multiple legacy quiesced copies + // already overlap. They must finish and unquiesce before new admission; + // trying to acquire exclusive ownership first would deadlock their cleanup. + if legacyBackupOwesUnquiesce(&b) && b.Spec.Strategy != "volume-snapshot" { + var existing batchv1.Job + err := r.backupAPIReader().Get(ctx, client.ObjectKeyFromObject(&b), &existing) + if err != nil && !apierrors.IsNotFound(err) { + return ctrl.Result{}, err + } + if err == nil && metav1.IsControlledBy(&existing, &b) { + return r.mirrorJobStatus(ctx, &b, &existing) + } + } // Resolve the target before building the Job: a Backup against a // GameServer that doesn't exist would produce a pod waiting forever @@ -383,6 +394,9 @@ func (r *BackupReconciler) maybeQuiesce(ctx context.Context, b *gameplanev1alpha } // Persist cleanup intent BEFORE save-off: a crash or a partial agent failure // must still owe save-on even when quiesced-at was never recorded. + if err := r.setLifecycleUnquiesceIntent(ctx, b, true); err != nil { + return err + } patchBackupAnnotations(b, map[string]string{annoQuiesceAttempted: "pending"}) if err := r.Update(ctx, b); err != nil { return err @@ -402,6 +416,9 @@ func (r *BackupReconciler) maybeQuiesce(ctx context.Context, b *gameplanev1alpha patchBackupAnnotations(b, map[string]string{ annoQuiesceAttempted: "unsupported", }) + if err := r.setLifecycleUnquiesceIntent(ctx, b, false); err != nil { + return err + } default: // Hard failure (network, RCON down). Surface as Failed: a // backup taken against an unflushed Minecraft world can be diff --git a/operator/internal/controller/backup_envtest_test.go b/operator/internal/controller/backup_envtest_test.go index 87ce6cc7e..9b037564a 100644 --- a/operator/internal/controller/backup_envtest_test.go +++ b/operator/internal/controller/backup_envtest_test.go @@ -577,6 +577,7 @@ func TestBackup_UsesTemplateSecurityContext(t *testing.T) { if err := k8sClient.Create(context.Background(), gs); err != nil { t.Fatalf("create gameserver: %v", err) } + seedBackupWorkload(t, gs) if err := k8sClient.Create(context.Background(), buildResticRepoSecret(ns, "repo-secret")); err != nil { t.Fatalf("create secret: %v", err) } diff --git a/operator/internal/controller/backup_forget_envtest_test.go b/operator/internal/controller/backup_forget_envtest_test.go index 3a6c642b4..9dbf6bf8e 100644 --- a/operator/internal/controller/backup_forget_envtest_test.go +++ b/operator/internal/controller/backup_forget_envtest_test.go @@ -16,6 +16,7 @@ import ( "k8s.io/client-go/tools/events" "k8s.io/client-go/util/retry" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/manager" gameplanev1alpha1 "github.com/GameplanePanel/gameplane/operator/api/v1alpha1" @@ -149,17 +150,17 @@ func TestBackupForget_FinalizerOnlyOnResticBackups(t *testing.T) { return false, "restic backup has no snapshot finalizer yet" }) - // Once the volume-snapshot Backup is Running the reconciler has been through - // the point where it would have added a finalizer. + // Every strategy gets an operation cleanup finalizer before admission. Do + // not wait for Running: the restic backup may hold the shared server owner. eventually(t, func() (bool, string) { b := getBackup(t, ns, "smp-vs") - if b.Status.Phase != gameplanev1alpha1.BackupPhaseRunning { + if !controllerutil.ContainsFinalizer(b, gameplanev1alpha1.BackupFinalizer) { return false, describeBackupStatus(b) } return true, "" }) - if got := getBackup(t, ns, "smp-vs").Finalizers; len(got) != 0 { - t.Errorf("volume-snapshot backup finalizers = %v, want none", got) + if got := getBackup(t, ns, "smp-vs"); controllerutil.ContainsFinalizer(got, gameplanev1alpha1.BackupSnapshotFinalizer) { + t.Errorf("volume-snapshot backup finalizers = %v, want no restic snapshot finalizer", got.Finalizers) } } diff --git a/operator/internal/controller/backup_forget_test.go b/operator/internal/controller/backup_forget_test.go index 12dd1aa51..ef917dd7f 100644 --- a/operator/internal/controller/backup_forget_test.go +++ b/operator/internal/controller/backup_forget_test.go @@ -16,6 +16,7 @@ import ( "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/controller/controllerutil" gameplanev1alpha1 "github.com/GameplanePanel/gameplane/operator/api/v1alpha1" ) @@ -180,7 +181,7 @@ func TestReconcile_AddsSnapshotFinalizerForResticBackup(t *testing.T) { } // Volume-snapshot and Failed Backups never write a restic snapshot, so they -// must not be held on deletion. +// need operation cleanup but must not be held for restic snapshot deletion. func TestReconcile_NoSnapshotFinalizerWhenNothingToForget(t *testing.T) { volume := forgetBackup() volume.DeletionTimestamp = nil @@ -205,8 +206,11 @@ func TestReconcile_NoSnapshotFinalizerWhenNothingToForget(t *testing.T) { if !ok { t.Fatal("backup vanished") } - if len(got.Finalizers) != 0 { - t.Errorf("finalizers = %v, want none", got.Finalizers) + if !controllerutil.ContainsFinalizer(got, gameplanev1alpha1.BackupFinalizer) { + t.Errorf("finalizers = %v, want operation cleanup finalizer", got.Finalizers) + } + if controllerutil.ContainsFinalizer(got, gameplanev1alpha1.BackupSnapshotFinalizer) { + t.Errorf("finalizers = %v, want no restic snapshot finalizer", got.Finalizers) } }) } diff --git a/operator/internal/controller/backup_guard.go b/operator/internal/controller/backup_guard.go index 4cc09e2f9..cee37734e 100644 --- a/operator/internal/controller/backup_guard.go +++ b/operator/internal/controller/backup_guard.go @@ -3,6 +3,7 @@ package controller import ( "context" "encoding/json" + "errors" "fmt" "time" @@ -61,9 +62,6 @@ func (r *BackupReconciler) backupAPIReader() client.Reader { // the target before claiming it so a crash or a same-name replacement cannot // redirect cleanup to a different game. func (r *BackupReconciler) claimBackupTarget(ctx context.Context, b *gameplanev1alpha1.Backup) (bool, error) { - if !b.Spec.Quiesce { - return true, nil - } legacy, err := r.legacyQuiescedBackups(ctx, b) if err != nil { return false, err @@ -72,10 +70,9 @@ func (r *BackupReconciler) claimBackupTarget(ctx context.Context, b *gameplanev1 // Already-running pre-upgrade copies must be allowed to finish, but // nobody may start a new protected copy until all of their cleanup is // confirmed. Each legacy cleanup also waits for all legacy workers. - if legacyBackupOwesUnquiesce(b) { - return true, nil + if !legacyBackupOwesUnquiesce(b) { + return false, nil } - return false, nil } var gs gameplanev1alpha1.GameServer if err := r.backupAPIReader().Get(ctx, types.NamespacedName{Namespace: b.Namespace, Name: b.Spec.ServerRef.Name}, &gs); err != nil { @@ -98,55 +95,11 @@ func (r *BackupReconciler) claimBackupTarget(ctx context.Context, b *gameplanev1 return false, err } } - owner := backupIdentityJSON(b.Name, b.UID) - guard := gs.Annotations[backupGuardAnnotation] - if guard == owner { - return r.recoverBackupTarget(ctx, b) - } - if guard != "" { - id, err := parseBackupIdentity(guard) - if err != nil { - return false, err - } // Malformed guards fail closed. - var holder gameplanev1alpha1.Backup - err = r.backupAPIReader().Get(ctx, types.NamespacedName{Namespace: b.Namespace, Name: id.Name}, &holder) - if err == nil && holder.UID == id.UID { - return false, nil - } - if err != nil && !apierrors.IsNotFound(err) { - return false, err - } - // Persist recovery intent, then transfer ownership with CAS BEFORE - // touching the agent. Concurrent contenders cannot both call save-on - // while one of them has already started a new protected copy. - patchBackupAnnotations(b, map[string]string{backupRecoveryAnnotation: guard, annoQuiesceAttempted: "pending"}) - if err := r.Update(ctx, b); err != nil { - return false, err - } - // Preserve every predecessor on the surviving GameServer, atomically - // with the owner transfer. A recovering Backup can itself disappear - // before an earlier owner's copy worker has stopped. - lineage, err := parseBackupRecoveryLineage(gs.Annotations[backupRecoveryLineageAnnotation]) - if err != nil { - return false, err - } - found := false - for _, predecessor := range lineage { - if predecessor == id { - found = true - } - } - if !found { - lineage = append(lineage, id) - } - data, _ := json.Marshal(lineage) - gs.Annotations[backupRecoveryLineageAnnotation] = string(data) - } - if gs.Annotations == nil { - gs.Annotations = make(map[string]string) - } - gs.Annotations[backupGuardAnnotation] = owner - if err := r.Update(ctx, &gs); err != nil { + state := b.Annotations[annoQuiesceAttempted] + owesUnquiesce := state != "" && state != "unsupported" && b.Annotations[annoUnquiescedAt] == "" + claimed, err := claimLifecycleOperation(ctx, r.Client, r.backupAPIReader(), &gs, + backupLifecycleIdentity(&gs, b, owesUnquiesce), r.AgentClient) + if err != nil || !claimed { return false, err } return r.recoverBackupTarget(ctx, b) @@ -227,8 +180,19 @@ func (r *BackupReconciler) backupRecoveryIdentities(ctx context.Context, b *game } func (r *BackupReconciler) backupCleanupWorkersLive(ctx context.Context, b *gameplanev1alpha1.Backup, cancel bool) (bool, error) { - if !b.Spec.Quiesce && b.Annotations[annoQuiesceAttempted] == "" { - return false, nil + gs, err := r.ownedBackupTarget(ctx, b) + if err != nil { + return false, err + } + if gs != nil && gs.Annotations[lifecycleOperationAnnotation] != "" { + op, err := parseLifecycleOperation(gs.Annotations[lifecycleOperationAnnotation]) + if err != nil { + return false, err + } + ready, err := recoverLifecycleOperation(ctx, r.Client, r.backupAPIReader(), gs, op, r.AgentClient) + if err != nil || !ready { + return !ready, err + } } busy, err := r.backupWorkersLive(ctx, b.Namespace, backupIdentity{Name: b.Name, UID: b.UID}, cancel) if err != nil || busy { @@ -283,11 +247,26 @@ func (r *BackupReconciler) legacyQuiescedBackups(ctx context.Context, b *gamepla func (r *BackupReconciler) prepareBackupQuiesce(ctx context.Context, b *gameplanev1alpha1.Backup) (ctrl.Result, error) { claimed, err := r.claimBackupTarget(ctx, b) if err != nil { + if errors.Is(err, errLifecycleGenerationChanged) { + _, err = r.fail(ctx, b, err.Error()) + return ctrl.Result{RequeueAfter: time.Nanosecond}, err + } return ctrl.Result{}, err } if !claimed { return ctrl.Result{RequeueAfter: 10 * time.Second}, nil } + gs, err := r.ownedBackupTarget(ctx, b) + if err != nil { + return ctrl.Result{}, err + } + if gs == nil { + return ctrl.Result{RequeueAfter: 5 * time.Second}, nil + } + fenced, err := fenceBackupWorkload(ctx, r.Client, r.backupAPIReader(), gs) + if err != nil || !fenced { + return ctrl.Result{RequeueAfter: 5 * time.Second}, err + } return ctrl.Result{}, r.maybeQuiesce(ctx, b) } @@ -313,6 +292,15 @@ func (r *BackupReconciler) ownedBackupTarget(ctx context.Context, b *gameplanev1 if gs.UID != id.UID || gs.Annotations[backupGuardAnnotation] != backupIdentityJSON(b.Name, b.UID) { return nil, nil } + if value := gs.Annotations[lifecycleOperationAnnotation]; value != "" { + op, err := parseLifecycleOperation(value) + if err != nil { + return nil, err + } + if !sameLifecycleOwner(op.lifecycleIdentity, backupLifecycleIdentity(&gs, b, false)) { + return nil, nil + } + } return &gs, nil } @@ -321,6 +309,9 @@ func (r *BackupReconciler) releaseBackupTarget(ctx context.Context, b *gameplane if err != nil || gs == nil { return err } + if err := clearLifecycleOperation(ctx, r.Client, r.backupAPIReader(), gs, backupLifecycleIdentity(gs, b, false)); err != nil { + return err + } delete(gs.Annotations, backupGuardAnnotation) delete(gs.Annotations, backupRecoveryLineageAnnotation) return r.Update(ctx, gs) diff --git a/operator/internal/controller/backup_guard_test.go b/operator/internal/controller/backup_guard_test.go index ac0e2be97..8b40e1104 100644 --- a/operator/internal/controller/backup_guard_test.go +++ b/operator/internal/controller/backup_guard_test.go @@ -8,6 +8,7 @@ import ( "time" snapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1" + 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" @@ -45,7 +46,12 @@ func guardFixture(t *testing.T, backups ...*gameplanev1alpha1.Backup) (*BackupRe if err := snapshotv1.AddToScheme(s); err != nil { t.Fatal(err) } + if err := appsv1.AddToScheme(s); err != nil { + t.Fatal(err) + } 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}}) for _, b := range backups { objects = append(objects, b) } diff --git a/operator/internal/controller/backup_volumesnapshot.go b/operator/internal/controller/backup_volumesnapshot.go index b454f6fad..d9df66b0e 100644 --- a/operator/internal/controller/backup_volumesnapshot.go +++ b/operator/internal/controller/backup_volumesnapshot.go @@ -38,6 +38,9 @@ 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 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 @@ -69,6 +72,10 @@ func (r *BackupReconciler) reconcileVolumeSnapshot( return r.abortVolumeSnapshot(ctx, b, fmt.Sprintf("volume snapshot creation failed: %v", err)) } + return r.observeVolumeSnapshot(ctx, b, vs) +} + +func (r *BackupReconciler) observeVolumeSnapshot(ctx context.Context, b *gameplanev1alpha1.Backup, vs *snapshotv1.VolumeSnapshot) (ctrl.Result, error) { 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 diff --git a/operator/internal/controller/gameserver_controller.go b/operator/internal/controller/gameserver_controller.go index 40df930ac..96fce2a07 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 { @@ -100,6 +104,8 @@ type GameServerReconciler struct { // during a soft stop. May be nil (or a disabled client) in dev clusters, // in which case the operator falls back to a timed scale-to-zero. AgentClient AgentStopper + // OperationQuiescer recovers orphaned backups before admitting a wipe. + OperationQuiescer AgentQuiescer // PodAttacher runs the module-declared stop sequence over a pod attach // to the game container's stdin, for consoleMode: pty games that @@ -336,6 +342,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 } @@ -385,6 +394,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 } @@ -1196,6 +1208,47 @@ func (r *GameServerReconciler) desiredReplicas( idle idleState, inventory ...*wipeWorkerState, ) (int32, time.Duration, error) { + // Read ownership live before restart acknowledgment or any stop RPC. The + // stop timestamp uses a checked Update, so a claim racing that write wins + // either before the command or after the stop has become visible. + var live gameplanev1alpha1.GameServer + if err := r.wipeReader().Get(ctx, client.ObjectKeyFromObject(gs), &live); err != nil { + return 0, 0, err + } + if live.UID != gs.UID { + return 0, 0, fmt.Errorf("GameServer changed identity before lifecycle decision") + } + if op, held, err := lifecycleOwner(&live); err != nil { + return 0, 0, err + } else if held && op.InitialStartup && len(op.Recovery) == 0 { + if live.Spec.Suspend { + 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" || len(op.Recovery) != 0) { + var ss appsv1.StatefulSet + err := r.wipeReader().Get(ctx, client.ObjectKeyFromObject(gs), &ss) + if apierrors.IsNotFound(err) { + return 0, 5 * time.Second, nil + } + if err != nil { + return 0, 0, err + } + if !metav1.IsControlledBy(&ss, gs) { + return 0, 0, fmt.Errorf("protected StatefulSet belongs to another GameServer") + } + if ss.Spec.Replicas == nil { + return 1, 5 * time.Second, nil + } + return *ss.Spec.Replicas, 5 * time.Second, nil + } else if held && (live.Annotations[restoreGuardAnnotation] != gs.Annotations[restoreGuardAnnotation] || live.Annotations[wipeGuardAnnotation] != gs.Annotations[wipeGuardAnnotation]) { + // This reconcile rendered config from an older GameServer. Use the + // live owner only for stopping; do not replace the caller's snapshot + // and mix a newly-read spec with already-materialized configuration. + return r.softStop(ctx, &live, tmpl) + } if gs.Annotations[wipeGuardAnnotation] != "" || pendingWipe(gs) { replicas, poll, err := r.softStop(ctx, gs, tmpl) if err == nil && r.settledWipeFailure(ctx, gs, wipeInventory(inventory)) { @@ -1458,6 +1511,38 @@ func (r *GameServerReconciler) reconcileStatefulSet( if owner := metav1.GetControllerOf(ss); owner != nil && owner.UID != live.UID { return fmt.Errorf("StatefulSet belongs to a different GameServer identity") } + op, held, err := lifecycleOwner(&live) + if err != nil { + return err + } + if held { + if ss.Annotations == nil { + ss.Annotations = map[string]string{} + } + ss.Annotations[lifecycleOperationAnnotation] = lifecycleOperationJSON(op) + if op.InitialStartup { + actualReplicas = replicas + delete(ss.Annotations, restoreGuardAnnotation) + } + if op.Kind == "Backup" || len(op.Recovery) != 0 { + if !ss.CreationTimestamp.IsZero() || ss.ResourceVersion != "" { + // 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 + // admission a real object to fence against stale creates. + actualReplicas = 0 + replicas = 0 + } + } else { + delete(ss.Annotations, lifecycleOperationAnnotation) + } if live.Annotations[restoreGuardAnnotation] == "" { // Restore cleanup can race a callback that restamps the workload // before the checked GameServer release. That release guarantees @@ -1466,6 +1551,9 @@ func (r *GameServerReconciler) reconcileStatefulSet( delete(ss.Annotations, restoreGuardAnnotation) } for _, annotation := range []string{restoreGuardAnnotation, wipeGuardAnnotation} { + if annotation == restoreGuardAnnotation && held && op.InitialStartup { + continue + } if guard := live.Annotations[annotation]; guard != "" { actualReplicas = 0 if ss.Annotations == nil { @@ -1494,6 +1582,11 @@ func (r *GameServerReconciler) reconcileStatefulSet( } } ss.Spec.Replicas = &actualReplicas + if held && (!ss.CreationTimestamp.IsZero() || ss.ResourceVersion != "") { + // Restore and wipe may stop the existing workload, but rollout + // waits until the operation and its worker cleanup have finished. + return nil + } ss.Spec.ServiceName = gs.Name ss.Spec.Selector = &metav1.LabelSelector{MatchLabels: labels} ss.Spec.Template.Labels = labels diff --git a/operator/internal/controller/gameserver_idle.go b/operator/internal/controller/gameserver_idle.go index 9869ece72..055071975 100644 --- a/operator/internal/controller/gameserver_idle.go +++ b/operator/internal/controller/gameserver_idle.go @@ -292,6 +292,17 @@ func untilNext(next, now time.Time) time.Duration { func (r *GameServerReconciler) reconcileIdle( ctx context.Context, gs *gameplanev1alpha1.GameServer, ) (idleState, *gameplanev1alpha1.IdleStatus, time.Duration, error, error) { + // Keep both sleep and wake intent unchanged while an operation owns the + // workload. In particular, waking a stopped backup source is deferred. + if _, held, err := lifecycleOwner(gs); err != nil { + return idleAwake, nil, 0, nil, err + } else if held { + state := idleAwake + if parseAnnotationTime(gs, IdleAsleepSinceAnnotation) != nil { + state = idleAsleep + } + return state, gs.Status.Idle.DeepCopy(), idleReevaluate, nil, nil + } in := idleInputs{ spec: gs.Spec.Idle, suspended: gs.Spec.Suspend, @@ -349,7 +360,7 @@ func (r *GameServerReconciler) applyIdleTransition( gs.Annotations[IdleWakeCompletedAnnotation] = tok } } - if err := r.Patch(ctx, gs, client.MergeFrom(base)); err != nil { + if err := r.Patch(ctx, gs, client.MergeFromWithOptions(base, client.MergeFromWithOptimisticLock{})); err != nil { return fmt.Errorf("patch idle annotations on %s/%s: %w", gs.Namespace, gs.Name, err) } return nil 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..a74724a8f --- /dev/null +++ b/operator/internal/controller/gameserver_lifecycle_reconcile_test.go @@ -0,0 +1,345 @@ +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() + return &GameServerReconciler{Client: c, APIReader: c, Scheme: s, AgentCASecretName: caSecret.Name, AgentCASecretNamespace: caSecret.Namespace} +} + +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) + switch mode { + case "missing owner": + delete(live.Annotations, lifecycleOperationAnnotation) + case "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") + patchErr := 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_restart.go b/operator/internal/controller/gameserver_restart.go index 33159927d..af1ddf48d 100644 --- a/operator/internal/controller/gameserver_restart.go +++ b/operator/internal/controller/gameserver_restart.go @@ -79,5 +79,5 @@ func (r *GameServerReconciler) ackRestart(ctx context.Context, gs *gameplanev1al } gs.Annotations[RestartCompletedAnnotation] = gs.Annotations[RestartRequestedAnnotation] delete(gs.Annotations, stopRequestedAtAnnotation) - return r.Patch(ctx, gs, client.MergeFrom(base)) + return r.Patch(ctx, gs, client.MergeFromWithOptions(base, client.MergeFromWithOptimisticLock{})) } diff --git a/operator/internal/controller/gameserver_wipe.go b/operator/internal/controller/gameserver_wipe.go index ca96d7d63..e3414336a 100644 --- a/operator/internal/controller/gameserver_wipe.go +++ b/operator/internal/controller/gameserver_wipe.go @@ -51,7 +51,7 @@ func (r *GameServerReconciler) reconcileWipe( if err != nil { return err } - if gs.Annotations[restoreGuardAnnotation] != "" { + if gs.Annotations[restoreGuardAnnotation] != "" || gs.Annotations[backupGuardAnnotation] != "" { return nil } req := gs.Annotations[WipeRequestedAnnotation] @@ -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 f86a1969d..45a6becc4 100644 --- a/operator/internal/controller/gameserver_wipe_guard.go +++ b/operator/internal/controller/gameserver_wipe_guard.go @@ -165,8 +165,12 @@ func (r *GameServerReconciler) ensureWipeGuard(ctx context.Context, gs *gameplan return false, fmt.Errorf("GameServer changed identity before wipe acquisition") } *gs = live - if gs.Annotations[wipeGuardAnnotation] != "" || gs.Annotations[restoreGuardAnnotation] != "" { - return false, nil + if token := gs.Annotations[wipeGuardAnnotation]; token != "" { + _, 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 { @@ -189,20 +193,22 @@ func (r *GameServerReconciler) ensureWipeGuard(ctx context.Context, gs *gameplan if token == "" { token = wipeOrphanGuard } - if gs.Annotations == nil { - gs.Annotations = make(map[string]string) - } - gs.Annotations[wipeGuardAnnotation] = token - // Recovery without a pending request is cleanup, including legacy orphan - // pods with no token. The guard fences live workers without changing the - // current resume/stop intent. Pending and failed matching wipes suspend. - if pendingWipe(gs) { - gs.Spec.Suspend = true + _, err = claimLifecycleOperation(ctx, r.Client, r.wipeReader(), gs, wipeLifecycleIdentity(gs, token), r.OperationQuiescer) + 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 } - if err := r.Update(ctx, gs); err != nil { + op, held, err := lifecycleOwner(gs) + if err != nil { return false, err } - return true, nil + 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 @@ -299,6 +305,9 @@ func (r *GameServerReconciler) releaseWipeGuard(ctx context.Context, gs *gamepla if err := r.clearWipeWorkloadGuard(ctx, &live, token); err != nil { return err } + if err := clearLifecycleOperation(ctx, r.Client, r.wipeReader(), &live, wipeLifecycleIdentity(&live, token)); err != nil { + return err + } if success { live.Annotations[WipeCompletedAnnotation] = token live.Spec.Suspend = false @@ -307,6 +316,7 @@ func (r *GameServerReconciler) releaseWipeGuard(ctx context.Context, gs *gamepla // Transfer only after old Jobs and live pods have drained; next pass // fences the workload for this new token before launching a worker. live.Annotations[wipeGuardAnnotation] = live.Annotations[WipeRequestedAnnotation] + live.Annotations[lifecycleOperationAnnotation] = lifecycleOperationJSON(lifecycleOperation{lifecycleIdentity: wipeLifecycleIdentity(&live, live.Annotations[WipeRequestedAnnotation])}) live.Spec.Suspend = true } else { delete(live.Annotations, wipeGuardAnnotation) 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_envtest_test.go b/operator/internal/controller/helpers_envtest_test.go index 7db1ea7c1..927d58443 100644 --- a/operator/internal/controller/helpers_envtest_test.go +++ b/operator/internal/controller/helpers_envtest_test.go @@ -571,12 +571,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) } } @@ -797,13 +803,32 @@ func (f *fakeLogReader) BackupLogs(_ context.Context, _, _ string) (io.ReadClose return io.NopCloser(strings.NewReader(f.body)), nil } -// seedGameServer creates a bare GameServer so reconcilers that resolve -// Backup/Restore serverRefs find their target. No GameServer reconciler -// runs in these suites, so the object stays inert (no children). +// seedGameServer creates a GameServer and observed workload for backup tests. +// No StatefulSet controller runs in envtest; report its generation explicitly +// so backup admission can exercise the real workload fence. func seedGameServer(t *testing.T, ns, name string) { t.Helper() gs := buildGameServer(ns, name, "seed-template") if err := k8sClient.Create(context.Background(), gs); err != nil { t.Fatalf("seed gameserver %s/%s: %v", ns, name, err) } + seedBackupWorkload(t, gs) +} + +func seedBackupWorkload(t *testing.T, gs *gameplanev1alpha1.GameServer) { + t.Helper() + name, ns := gs.Name, gs.Namespace + one := int32(1) + labels := map[string]string{"app": name} + ss := &appsv1.StatefulSet{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns, OwnerReferences: []metav1.OwnerReference{{APIVersion: gameplanev1alpha1.GroupVersion.String(), Kind: "GameServer", Name: name, UID: gs.UID, Controller: ownerBoolPtr(true)}}}, + Spec: appsv1.StatefulSetSpec{Replicas: &one, ServiceName: name, Selector: &metav1.LabelSelector{MatchLabels: labels}, Template: corev1.PodTemplateSpec{ObjectMeta: metav1.ObjectMeta{Labels: labels}, Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "game", Image: "test/game:fixture"}}}}}, + } + if err := k8sClient.Create(context.Background(), ss); err != nil { + t.Fatalf("seed workload: %v", err) + } + ss.Status.ObservedGeneration = ss.Generation + if err := k8sClient.Status().Update(context.Background(), ss); err != nil { + t.Fatalf("observe workload: %v", err) + } } 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"}, diff --git a/operator/internal/controller/lifecycle_operation.go b/operator/internal/controller/lifecycle_operation.go new file mode 100644 index 000000000..fc769d5e7 --- /dev/null +++ b/operator/internal/controller/lifecycle_operation.go @@ -0,0 +1,428 @@ +package controller + +import ( + "context" + "encoding/json" + "errors" + "fmt" + + appsv1 "k8s.io/api/apps/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" + "sigs.k8s.io/controller-runtime/pkg/client" + + gameplanev1alpha1 "github.com/GameplanePanel/gameplane/operator/api/v1alpha1" +) + +const lifecycleOperationAnnotation = "operations.gameplane.local/owner" + +var errLifecycleGenerationChanged = errors.New("lifecycle operation spec changed after admission") + +// The GameServer is the arbitration object. A wipe has no separate CR, so its +// request token and GameServer UID identify the operation. Generation is pinned +// at admission; subsequent power requests must not change that identity. +type lifecycleIdentity struct { + Kind string `json:"kind"` + Name string `json:"name"` + UID types.UID `json:"uid"` + TargetUID types.UID `json:"targetUID"` + Generation int64 `json:"generation"` + Token string `json:"token,omitempty"` + NeedsUnquiesce bool `json:"needsUnquiesce,omitempty"` + // InitialStartup permits only the first workload of a CSI restore's new + // target (including WaitForFirstConsumer PVCs), while other operations wait. + InitialStartup bool `json:"initialStartup,omitempty"` +} + +type lifecycleOperation struct { + lifecycleIdentity + // Recovery survives deletion of a successor while it is draining an older + // owner's workers. No new work is authorized until every entry is cleaned. + Recovery []lifecycleIdentity `json:"recovery,omitempty"` +} + +func lifecycleOperationJSON(op lifecycleOperation) string { + value, _ := json.Marshal(op) + return string(value) +} + +func parseLifecycleOperation(value string) (lifecycleOperation, error) { + var op lifecycleOperation + if err := json.Unmarshal([]byte(value), &op); err != nil { + return op, fmt.Errorf("decode lifecycle owner: %w", err) + } + for _, id := range append([]lifecycleIdentity{op.lifecycleIdentity}, op.Recovery...) { + if id.Name == "" || id.UID == "" || id.TargetUID == "" || (id.Kind != "Backup" && id.Kind != "Restore" && id.Kind != "Wipe") || (id.Kind == "Wipe" && id.Token == "") { + return op, fmt.Errorf("invalid lifecycle owner identity") + } + if id.TargetUID != op.TargetUID { + return op, fmt.Errorf("lifecycle recovery target changed identity") + } + } + return op, nil +} + +func backupLifecycleIdentity(gs *gameplanev1alpha1.GameServer, b *gameplanev1alpha1.Backup, quiesce bool) lifecycleIdentity { + return lifecycleIdentity{Kind: "Backup", Name: b.Name, UID: b.UID, TargetUID: gs.UID, Generation: b.Generation, NeedsUnquiesce: quiesce} +} + +func restoreLifecycleIdentity(gs *gameplanev1alpha1.GameServer, rs *gameplanev1alpha1.Restore) lifecycleIdentity { + return lifecycleIdentity{Kind: "Restore", Name: rs.Name, UID: rs.UID, TargetUID: gs.UID, Generation: rs.Generation} +} + +func wipeLifecycleIdentity(gs *gameplanev1alpha1.GameServer, token string) lifecycleIdentity { + return lifecycleIdentity{Kind: "Wipe", Name: gs.Name, UID: gs.UID, TargetUID: gs.UID, Generation: gs.Generation, Token: token} +} + +func sameLifecycleOwner(a, b lifecycleIdentity) bool { + return a.Kind == b.Kind && a.Name == b.Name && a.UID == b.UID && a.TargetUID == b.TargetUID && a.Token == b.Token +} + +// Existing guards remain readable during upgrades and provide the compatibility +// markers used by stop/drain cleanup. They are never independent permission to +// start new work. Contradictory legacy guards fail closed until their owners +// complete cleanup; migration must not select one concurrent writer as safe. +func lifecycleOwner(gs *gameplanev1alpha1.GameServer) (lifecycleOperation, bool, error) { + if value := gs.Annotations[lifecycleOperationAnnotation]; value != "" { + op, err := parseLifecycleOperation(value) + if err == nil && op.TargetUID != gs.UID { + err = fmt.Errorf("lifecycle owner belongs to a replaced GameServer") + } + return op, true, err + } + op := lifecycleOperation{} + count := 0 + if value := gs.Annotations[backupGuardAnnotation]; value != "" { + id, err := parseBackupIdentity(value) + if err != nil { + return op, true, err + } + op.lifecycleIdentity = lifecycleIdentity{Kind: "Backup", Name: id.Name, UID: id.UID, TargetUID: gs.UID, NeedsUnquiesce: true} + lineage, err := parseBackupRecoveryLineage(gs.Annotations[backupRecoveryLineageAnnotation]) + if err != nil { + return op, true, err + } + for _, predecessor := range lineage { + op.Recovery = append(op.Recovery, lifecycleIdentity{Kind: "Backup", Name: predecessor.Name, UID: predecessor.UID, TargetUID: gs.UID, NeedsUnquiesce: true}) + } + count++ + } + if value := gs.Annotations[restoreGuardAnnotation]; value != "" { + id, err := parseRestoreIdentity(value) + if err != nil { + return op, true, err + } + op.lifecycleIdentity = lifecycleIdentity{Kind: "Restore", Name: id.Name, UID: id.UID, TargetUID: gs.UID} + count++ + } + if token := gs.Annotations[wipeGuardAnnotation]; token != "" { + op.lifecycleIdentity = wipeLifecycleIdentity(gs, token) + count++ + } + if count > 1 { + return op, true, fmt.Errorf("overlapping legacy lifecycle owners require cleanup before new work") + } + return op, count != 0, nil +} + +func lifecycleOwnerExists(ctx context.Context, reader client.Reader, namespace string, id lifecycleIdentity) (bool, error) { + var obj client.Object + switch id.Kind { + case "Backup": + obj = &gameplanev1alpha1.Backup{} + case "Restore": + obj = &gameplanev1alpha1.Restore{} + case "Wipe": + // Only the GameServer reconciler may cancel/replace a wipe token after + // its workers drain. A removed request is not proof of worker absence. + return true, nil + default: + return false, fmt.Errorf("unknown lifecycle operation %q", id.Kind) + } + err := reader.Get(ctx, client.ObjectKey{Namespace: namespace, Name: id.Name}, obj) + if apierrors.IsNotFound(err) { + return false, nil + } + return err == nil && obj.GetUID() == id.UID, err +} + +// claimLifecycleOperation refreshes gs with the uncached reader and writes the +// shared owner plus compatibility guard in ONE resourceVersion-checked Update. +// Ownership is retained during recovery. A losing claimant performs no cleanup +// or worker creation. No timeout ever permits stealing from an existing owner. +func claimLifecycleOperation(ctx context.Context, c client.Client, reader client.Reader, gs *gameplanev1alpha1.GameServer, want lifecycleIdentity, quiescer AgentQuiescer) (bool, error) { + var live gameplanev1alpha1.GameServer + if err := reader.Get(ctx, client.ObjectKeyFromObject(gs), &live); err != nil { + return false, err + } + if live.UID != want.TargetUID || !live.DeletionTimestamp.IsZero() { + return false, fmt.Errorf("lifecycle target changed identity or is being deleted") + } + *gs = live + op, occupied, err := lifecycleOwner(gs) + if err != nil { + return false, err + } + if occupied && !sameLifecycleOwner(op.lifecycleIdentity, want) { + exists, err := lifecycleOwnerExists(ctx, reader, gs.Namespace, op.lifecycleIdentity) + if err != nil || exists { + return false, err + } + op.Recovery = append(op.Recovery, op.lifecycleIdentity) + // A force-deleted restore may have left partial data. Preserve its + // suspension even when the next operation is a backup. + if op.Kind == "Restore" { + gs.Spec.Suspend = true + } + op.lifecycleIdentity = want + } else if occupied { + if gs.Annotations[lifecycleOperationAnnotation] != "" && op.Kind != "Wipe" && op.Generation != want.Generation { + return false, errLifecycleGenerationChanged + } + if gs.Annotations[lifecycleOperationAnnotation] == "" { + op.lifecycleIdentity = want + } + } else { + if want.Kind != "Wipe" { + workers, err := wipeWorkers(ctx, reader, gs) + if err != nil || pendingWipe(gs) || workers.busy { + return false, err + } + } + // A stop command already sent may still save data and exit. Waiting + // here also lets pending restarts finish instead of freezing mid-stop. + if want.Kind == "Backup" { + pending, err := backupLifecycleTransitionPending(ctx, reader, gs) + if err != nil || pending { + return false, err + } + } + busy, err := legacyLifecycleCopiesLive(ctx, c, reader, gs, want) + if err != nil || busy { + return false, err + } + op = lifecycleOperation{lifecycleIdentity: want} + } + before := gs.DeepCopy() + if gs.Annotations == nil { + gs.Annotations = map[string]string{} + } + gs.Annotations[lifecycleOperationAnnotation] = lifecycleOperationJSON(op) + delete(gs.Annotations, backupGuardAnnotation) + delete(gs.Annotations, restoreGuardAnnotation) + delete(gs.Annotations, wipeGuardAnnotation) + switch want.Kind { + case "Backup": + gs.Annotations[backupGuardAnnotation] = backupIdentityJSON(want.Name, want.UID) + case "Restore": + gs.Annotations[restoreGuardAnnotation] = restoreIdentityJSON(want.Name, want.UID) + if !op.InitialStartup { + gs.Spec.Suspend = true + } + case "Wipe": + gs.Annotations[wipeGuardAnnotation] = want.Token + if pendingWipe(gs) { + gs.Spec.Suspend = true + } + } + // An acquired owner can be re-read many times while its worker runs. + // Avoid needless resourceVersion churn, but always CAS new admission. + if before.Annotations[lifecycleOperationAnnotation] != gs.Annotations[lifecycleOperationAnnotation] || before.Spec.Suspend != gs.Spec.Suspend { + if err := c.Update(ctx, gs); err != nil { + return false, err + } + } + return recoverLifecycleOperation(ctx, c, reader, gs, op, quiescer) +} + +// New code must also respect workers started before shared ownership existed, +// including quiesce=false copies which had no old backup guard at all. +func legacyLifecycleCopiesLive(ctx context.Context, c client.Client, reader client.Reader, gs *gameplanev1alpha1.GameServer, want lifecycleIdentity) (bool, error) { + var backups gameplanev1alpha1.BackupList + if err := reader.List(ctx, &backups, client.InNamespace(gs.Namespace)); err != nil { + return false, err + } + r := &BackupReconciler{Client: c, APIReader: reader, Scheme: c.Scheme()} + for i := range backups.Items { + b := &backups.Items[i] + if b.Spec.ServerRef.Name != gs.Name || (want.Kind == "Backup" && b.UID == want.UID) { + continue + } + if legacyBackupOwesUnquiesce(b) { + return true, nil + } + busy, err := r.backupWorkersLive(ctx, gs.Namespace, backupIdentity{Name: b.Name, UID: b.UID}, false) + if err != nil || busy { + return busy, err + } + } + 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 + var err error + switch predecessor.Kind { + case "Backup": + r := &BackupReconciler{Client: c, APIReader: reader, Scheme: c.Scheme()} + busy, err = r.backupWorkersLive(ctx, gs.Namespace, backupIdentity{Name: predecessor.Name, UID: predecessor.UID}, true) + case "Restore": + r := &RestoreReconciler{Client: c, APIReader: reader, Scheme: c.Scheme()} + busy, err = r.restoreWorkersLive(ctx, gs.Namespace, restoreIdentity{Name: predecessor.Name, UID: predecessor.UID}, true) + default: + return false, fmt.Errorf("unsupported orphan lifecycle owner %q", predecessor.Kind) + } + if err != nil || busy { + 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 + } + } + needsUnquiesce := false + for _, predecessor := range op.Recovery { + needsUnquiesce = needsUnquiesce || predecessor.NeedsUnquiesce + } + if needsUnquiesce { + if quiescer == nil { + return false, fmt.Errorf("cannot recover orphaned quiesce without an agent client") + } + if err := quiescer.Unquiesce(ctx, gs.Namespace, gs.Name); err != nil { + return false, fmt.Errorf("recover orphaned lifecycle quiesce: %w", err) + } + } + if len(op.Recovery) > 0 { + op.Recovery = nil + gs.Annotations[lifecycleOperationAnnotation] = lifecycleOperationJSON(op) + delete(gs.Annotations, backupRecoveryLineageAnnotation) + if err := c.Update(ctx, gs); err != nil { + return false, err + } + } + return true, nil +} + +func backupLifecycleTransitionPending(ctx context.Context, reader client.Reader, gs *gameplanev1alpha1.GameServer) (bool, error) { + if tokenPending(gs, RestartRequestedAnnotation, RestartCompletedAnnotation) { + return true, nil + } + if gs.Annotations[stopRequestedAtAnnotation] == "" { + return false, nil + } + var ss appsv1.StatefulSet + if err := reader.Get(ctx, client.ObjectKeyFromObject(gs), &ss); err != nil { + return true, client.IgnoreNotFound(err) + } + if !metav1.IsControlledBy(&ss, gs) || ss.Spec.Replicas == nil || *ss.Spec.Replicas != 0 || ss.Status.Replicas != 0 || ss.Status.ObservedGeneration < ss.Generation { + return true, nil + } + var pods corev1.PodList + if err := reader.List(ctx, &pods, client.InNamespace(gs.Namespace)); err != nil { + return true, err + } + for i := range pods.Items { + if liveRestorePod(&pods.Items[i]) && (pods.Items[i].Name == gs.Name+"-0" || podMountsRestoreTarget(&pods.Items[i], gs.Name)) { + return true, nil + } + } + return false, nil +} + +// Carry cleanup intent on the surviving arbitration object as well as Backup. +// A force-deleted Backup cannot hide a partial/ambiguous save-off RPC. +func (r *BackupReconciler) setLifecycleUnquiesceIntent(ctx context.Context, b *gameplanev1alpha1.Backup, needed bool) error { + gs, err := r.ownedBackupTarget(ctx, b) + if err != nil || gs == nil || gs.Annotations[lifecycleOperationAnnotation] == "" { + return err + } + op, err := parseLifecycleOperation(gs.Annotations[lifecycleOperationAnnotation]) + if err != nil { + return err + } + if op.NeedsUnquiesce == needed { + return nil + } + op.NeedsUnquiesce = needed + gs.Annotations[lifecycleOperationAnnotation] = lifecycleOperationJSON(op) + return r.Update(ctx, gs) +} + +// fenceBackupWorkload performs a checked write to the workload before a copy +// can start. A stale workload Update either precedes this fence (and is fully +// observed before admission) or conflicts with it. StatefulSet mutation also +// reads the live owner inside its callback and preserves the whole pod template. +func fenceBackupWorkload(ctx context.Context, c client.Client, reader client.Reader, gs *gameplanev1alpha1.GameServer) (bool, error) { + var ss appsv1.StatefulSet + if err := reader.Get(ctx, client.ObjectKeyFromObject(gs), &ss); err != nil { + if apierrors.IsNotFound(err) { + return false, nil // wait for the workload reconciler to create a fenced set + } + return false, err + } + if !metav1.IsControlledBy(&ss, gs) { + return false, fmt.Errorf("backup StatefulSet is not controlled by the bound GameServer") + } + value := gs.Annotations[lifecycleOperationAnnotation] + if value == "" { + return false, fmt.Errorf("backup has no shared lifecycle owner") + } + if ss.Annotations[lifecycleOperationAnnotation] != value { + if ss.Annotations == nil { + ss.Annotations = map[string]string{} + } + ss.Annotations[lifecycleOperationAnnotation] = value + if err := c.Update(ctx, &ss); err != nil { + return false, err + } + } + if ss.Status.ObservedGeneration < ss.Generation || (ss.Status.CurrentRevision != "" && ss.Status.CurrentRevision != ss.Status.UpdateRevision) { + return false, nil + } + return true, nil +} + +// clearLifecycleOperation prepares the release; the caller removes its legacy +// guard and commits the GameServer Update only AFTER worker/agent cleanup and a +// durable completion decision. Release never changes the caller's power intent. +func clearLifecycleOperation(ctx context.Context, c client.Client, reader client.Reader, gs *gameplanev1alpha1.GameServer, want lifecycleIdentity) error { + value := gs.Annotations[lifecycleOperationAnnotation] + if value == "" { + return nil + } + op, err := parseLifecycleOperation(value) + if err != nil { + return err + } + if !sameLifecycleOwner(op.lifecycleIdentity, want) || len(op.Recovery) != 0 { + return fmt.Errorf("cannot release lifecycle ownership before recovery completes") + } + var ss appsv1.StatefulSet + err = reader.Get(ctx, client.ObjectKeyFromObject(gs), &ss) + if err != nil && !apierrors.IsNotFound(err) { + return err + } + if err == nil && metav1.IsControlledBy(&ss, gs) && ss.Annotations[lifecycleOperationAnnotation] == value { + delete(ss.Annotations, lifecycleOperationAnnotation) + if err := c.Update(ctx, &ss); err != nil { + return err + } + } + delete(gs.Annotations, lifecycleOperationAnnotation) + return nil +} diff --git a/operator/internal/controller/lifecycle_operation_envtest_test.go b/operator/internal/controller/lifecycle_operation_envtest_test.go new file mode 100644 index 000000000..7dc623a1a --- /dev/null +++ b/operator/internal/controller/lifecycle_operation_envtest_test.go @@ -0,0 +1,73 @@ +//go:build envtest + +package controller + +import ( + "sync" + "testing" + + 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" +) + +// A real apiserver arbitrates across controller types; no process-local mutex +// or informer timing can allow both claimants to create workers. +func TestLifecycleCASAcrossControllers(t *testing.T) { + ns := newNamespace(t) + seedGameServer(t, ns, "target") + b := buildBackup(ns, "backup", "target", "repo") + b.Spec.Quiesce = false + rs := &gameplanev1alpha1.Restore{ObjectMeta: metav1.ObjectMeta{Name: "restore", Namespace: ns}, Spec: gameplanev1alpha1.RestoreSpec{ServerRef: gameplanev1alpha1.LocalObjectRef{Name: "target"}, BackupRef: gameplanev1alpha1.LocalObjectRef{Name: "source"}}} + if err := k8sClient.Create(t.Context(), b); err != nil { + t.Fatal(err) + } + if err := k8sClient.Create(t.Context(), rs); err != nil { + t.Fatal(err) + } + var gs gameplanev1alpha1.GameServer + if err := k8sClient.Get(t.Context(), client.ObjectKey{Namespace: ns, Name: "target"}, &gs); err != nil { + t.Fatal(err) + } + type outcome struct { + claimed bool + err error + } + results := make(chan outcome, 2) + start := make(chan struct{}) + var ready sync.WaitGroup + ready.Add(2) + for _, want := range []lifecycleIdentity{backupLifecycleIdentity(&gs, b, false), restoreLifecycleIdentity(&gs, rs)} { + go func() { + target := gs.DeepCopy() + ready.Done() + <-start + claimed, err := claimLifecycleOperation(t.Context(), k8sClient, k8sClient, target, want, nil) + results <- outcome{claimed: claimed, err: err} + }() + } + ready.Wait() + close(start) + winners := 0 + for range 2 { + result := <-results + if result.err != nil && !apierrors.IsConflict(result.err) { + t.Fatalf("claim: %v", result.err) + } + if result.claimed { + winners++ + } + } + if winners != 1 { + t.Fatalf("exclusive admission had %d winners", winners) + } + if err := k8sClient.Get(t.Context(), client.ObjectKeyFromObject(&gs), &gs); err != nil { + t.Fatal(err) + } + op, held, err := lifecycleOwner(&gs) + if err != nil || !held || op.TargetUID != gs.UID { + t.Fatalf("owner was not durable: held=%t owner=%+v err=%v", held, op, err) + } +} diff --git a/operator/internal/controller/lifecycle_operation_test.go b/operator/internal/controller/lifecycle_operation_test.go new file mode 100644 index 000000000..b41e61cc7 --- /dev/null +++ b/operator/internal/controller/lifecycle_operation_test.go @@ -0,0 +1,387 @@ +package controller + +import ( + "context" + "fmt" + "testing" + "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" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + gameplanev1alpha1 "github.com/GameplanePanel/gameplane/operator/api/v1alpha1" +) + +func TestLifecycleAllBackupVariantsExcludeRestoreAndWipe(t *testing.T) { + for _, quiesce := range []bool{false, true} { + for _, strategy := range []string{"restic-snapshot", "volume-snapshot"} { + t.Run(fmt.Sprintf("%s/quiesce=%t", strategy, quiesce), func(t *testing.T) { + b := guardBackup("copy") + b.Spec.Quiesce, b.Spec.Strategy = quiesce, strategy + r, _ := guardFixture(t, b) + if claimed, err := r.claimBackupTarget(t.Context(), b); err != nil || !claimed { + t.Fatalf("claim backup: claimed=%t err=%v", claimed, err) + } + var gs gameplanev1alpha1.GameServer + if err := r.Get(t.Context(), client.ObjectKey{Namespace: "ns", Name: "server"}, &gs); err != nil { + t.Fatal(err) + } + rs := &gameplanev1alpha1.Restore{ObjectMeta: metav1.ObjectMeta{Name: "restore", Namespace: "ns", UID: "restore-uid"}} + if err := r.Create(t.Context(), rs); err != nil { + t.Fatal(err) + } + rr := &RestoreReconciler{Client: r.Client, APIReader: r.Client, Scheme: r.Scheme} + if claimed, err := rr.claimRestoreTarget(t.Context(), rs, &gs); err != nil || claimed { + t.Fatalf("restore overlapped backup: claimed=%t err=%v", claimed, err) + } + gs.Annotations[WipeRequestedAnnotation] = "wipe" + if err := r.Update(t.Context(), &gs); err != nil { + t.Fatal(err) + } + gr := &GameServerReconciler{Client: r.Client, APIReader: r.Client, Scheme: r.Scheme} + if changed, err := gr.ensureWipeGuard(t.Context(), &gs); err != nil || changed || gs.Annotations[wipeGuardAnnotation] != "" { + t.Fatalf("wipe overlapped backup: changed=%t err=%v", changed, err) + } + }) + } + } +} + +func TestLifecycleRestoreExcludesBackupCreatedAfterDrainCheck(t *testing.T) { + b := guardBackup("copy") + b.Spec.Quiesce = false + r, _ := guardFixture(t, b) + var gs gameplanev1alpha1.GameServer + if err := r.Get(t.Context(), client.ObjectKey{Namespace: "ns", Name: "server"}, &gs); err != nil { + t.Fatal(err) + } + rs := &gameplanev1alpha1.Restore{ObjectMeta: metav1.ObjectMeta{Name: "restore", Namespace: "ns", UID: "restore-uid"}} + if err := r.Create(t.Context(), rs); err != nil { + t.Fatal(err) + } + rr := &RestoreReconciler{Client: r.Client, APIReader: r.Client, Scheme: r.Scheme} + if claimed, err := rr.claimRestoreTarget(t.Context(), rs, &gs); err != nil || !claimed { + t.Fatalf("claim restore: claimed=%t err=%v", claimed, err) + } + if claimed, err := r.claimBackupTarget(t.Context(), b); err != nil || claimed { + t.Fatalf("unquiesced backup entered restore: claimed=%t err=%v", claimed, err) + } +} + +func TestLifecycleUnquiescedCleanupRetainsOwnerUntilWorkerDrains(t *testing.T) { + b := guardBackup("copy") + b.Spec.Quiesce = false + r, q := guardFixture(t, b) + if claimed, err := r.claimBackupTarget(t.Context(), b); err != nil || !claimed { + t.Fatalf("claim: %t %v", claimed, err) + } + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "orphan", Namespace: "ns", Labels: map[string]string{backupUIDLabel: string(b.UID)}}, Status: corev1.PodStatus{Phase: corev1.PodRunning}} + if err := r.Create(t.Context(), pod); err != nil { + t.Fatal(err) + } + b.Status.Phase = gameplanev1alpha1.BackupPhaseFailed + if err := r.Status().Update(t.Context(), b); err != nil { + t.Fatal(err) + } + if res, err := r.runUnquiesce(t.Context(), b); err != nil || res.RequeueAfter == 0 { + t.Fatalf("cleanup released live worker: result=%+v err=%v", res, err) + } + if err := r.Delete(t.Context(), pod); err != nil { + t.Fatal(err) + } + if _, err := r.runUnquiesce(t.Context(), b); err != nil { + t.Fatal(err) + } + var gs gameplanev1alpha1.GameServer + if err := r.Get(t.Context(), client.ObjectKey{Namespace: "ns", Name: "server"}, &gs); err != nil { + t.Fatal(err) + } + if gs.Annotations[lifecycleOperationAnnotation] != "" || q.unquiesced != 0 { + t.Fatalf("unquiesced cleanup: owner=%q agent calls=%d", gs.Annotations[lifecycleOperationAnnotation], q.unquiesced) + } +} + +func TestLifecycleBackupFencePreservesReplicasAndTemplate(t *testing.T) { + s := wipeScheme(t) + gs := wipeGameServer(false, "") + ss := wipeStoppedSet(gs) + one := int32(1) + ss.Spec.Replicas = &one + ss.Spec.Template.Spec.Containers = []corev1.Container{{Name: "game", Image: "before"}} + b := guardBackup("copy") + b.Spec.ServerRef.Name = gs.Name + gs.Annotations[backupGuardAnnotation] = backupIdentityJSON(b.Name, b.UID) + gs.Annotations[lifecycleOperationAnnotation] = lifecycleOperationJSON(lifecycleOperation{lifecycleIdentity: backupLifecycleIdentity(gs, b, false)}) + c := fake.NewClientBuilder().WithScheme(s).WithObjects(gs, ss, b).Build() + r := &GameServerReconciler{Client: c, APIReader: c, Scheme: s} + stale := gs.DeepCopy() + stale.Annotations = nil + stale.Spec.Suspend = true + if err := r.reconcileStatefulSet(t.Context(), stale, &gameplanev1alpha1.GameTemplate{}, nil, &materializedConfig{}, 0); err != nil { + t.Fatal(err) + } + var current appsv1.StatefulSet + if err := c.Get(t.Context(), client.ObjectKeyFromObject(ss), ¤t); err != nil { + t.Fatal(err) + } + if *current.Spec.Replicas != 1 || current.Spec.Template.Spec.Containers[0].Image != "before" { + t.Fatal("stale reconcile stopped or rolled protected source") + } + if replicas, delay, err := r.desiredReplicas(t.Context(), stale, &gameplanev1alpha1.GameTemplate{}, idleAsleep); err != nil || replicas != 1 || delay == 0 { + t.Fatalf("stop/idle bypassed live owner: replicas=%d delay=%s err=%v", replicas, delay, err) + } +} + +func TestLifecycleConcurrentClaimsUseResourceVersion(t *testing.T) { + b := guardBackup("copy") + b.Spec.Quiesce = false + r, _ := guardFixture(t, b) + intercepted := false + r.Client = interceptor.NewClient(r.Client.(client.WithWatch), interceptor.Funcs{ + Update: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.UpdateOption) error { + if gs, ok := obj.(*gameplanev1alpha1.GameServer); ok && !intercepted && gs.Annotations[lifecycleOperationAnnotation] != "" { + intercepted = true + var current gameplanev1alpha1.GameServer + if err := c.Get(ctx, client.ObjectKeyFromObject(gs), ¤t); err != nil { + return err + } + if current.Annotations == nil { + current.Annotations = map[string]string{} + } + current.Annotations[WipeRequestedAnnotation] = "winner" + current.Annotations[wipeGuardAnnotation] = "winner" + returnAfter := c.Update(ctx, ¤t) + if returnAfter != nil { + return returnAfter + } + } + return c.Update(ctx, obj, opts...) + }, + }) + if claimed, err := r.claimBackupTarget(t.Context(), b); claimed || !apierrors.IsConflict(err) { + t.Fatalf("lost CAS started backup: claimed=%t err=%v", claimed, err) + } + var jobs batchv1.JobList + if err := r.List(t.Context(), &jobs); err != nil || len(jobs.Items) != 0 { + t.Fatalf("losing claimant started workers: jobs=%d err=%v", len(jobs.Items), err) + } +} + +func TestLifecycleIdentityRejectsReplacementAndMalformedOwner(t *testing.T) { + for _, owner := range []string{"broken", lifecycleOperationJSON(lifecycleOperation{lifecycleIdentity: lifecycleIdentity{Kind: "Backup", Name: "other", UID: types.UID("other"), TargetUID: "replaced"}})} { + t.Run(owner, func(t *testing.T) { + b := guardBackup("copy") + r, _ := guardFixture(t, b) + var gs gameplanev1alpha1.GameServer + if err := r.Get(t.Context(), client.ObjectKey{Namespace: "ns", Name: "server"}, &gs); err != nil { + t.Fatal(err) + } + gs.Annotations = map[string]string{lifecycleOperationAnnotation: owner} + if err := r.Update(t.Context(), &gs); err != nil { + t.Fatal(err) + } + if claimed, err := r.claimBackupTarget(t.Context(), b); claimed || err == nil { + t.Fatalf("invalid owner admitted backup: claimed=%t err=%v", claimed, err) + } + }) + } +} + +func TestLifecycleOrphanBackupRecoveryFencesRestoreUntilDrain(t *testing.T) { + b := guardBackup("copy") + b.Spec.Quiesce = false + r, _ := guardFixture(t, b) + if claimed, err := r.claimBackupTarget(t.Context(), b); err != nil || !claimed { + t.Fatalf("claim: %t %v", claimed, err) + } + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "orphan", Namespace: "ns", Labels: map[string]string{backupUIDLabel: string(b.UID)}}, Status: corev1.PodStatus{Phase: corev1.PodRunning}} + if err := r.Create(t.Context(), pod); err != nil { + t.Fatal(err) + } + forceDeleteGuardBackup(t, r, b.Name) + rs := lifecycleRestore("restore", "restore-uid") + if err := r.Create(t.Context(), rs); err != nil { + t.Fatal(err) + } + gs := &gameplanev1alpha1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "server", Namespace: "ns", UID: "server-uid"}} + rr := &RestoreReconciler{Client: r.Client, APIReader: r.Client, Scheme: r.Scheme} + if claimed, err := rr.claimRestoreTarget(t.Context(), rs, gs); err != nil || claimed { + t.Fatalf("restore admitted before orphan drained: %t %v", claimed, err) + } + gr := &GameServerReconciler{Client: r.Client, APIReader: r.Client, Scheme: r.Scheme} + if replicas, _, err := gr.desiredReplicas(t.Context(), gs, &gameplanev1alpha1.GameTemplate{}, idleAwake); err != nil || replicas != 1 { + t.Fatalf("recovery stopped source while copy worker alive: replicas=%d err=%v", replicas, err) + } + if err := r.Delete(t.Context(), pod); err != nil { + t.Fatal(err) + } + if claimed, err := rr.claimRestoreTarget(t.Context(), rs, gs); err != nil || !claimed { + t.Fatalf("restore did not recover after drain: %t %v", claimed, err) + } +} + +func TestLifecycleIdleTransitionConflictsWithNewBackupOwner(t *testing.T) { + b := guardBackup("copy") + r, _ := guardFixture(t, b) + var stale gameplanev1alpha1.GameServer + if err := r.Get(t.Context(), client.ObjectKey{Namespace: "ns", Name: "server"}, &stale); err != nil { + t.Fatal(err) + } + if claimed, err := r.claimBackupTarget(t.Context(), b); err != nil || !claimed { + t.Fatalf("claim: %t %v", claimed, err) + } + gr := &GameServerReconciler{Client: r.Client, APIReader: r.Client, Scheme: r.Scheme} + if err := gr.applyIdleTransition(t.Context(), &stale, idleOutcome{sleep: true}, time.Now()); !apierrors.IsConflict(err) { + t.Fatalf("stale idle transition bypassed ownership CAS: %v", err) + } + var current gameplanev1alpha1.GameServer + if err := r.Get(t.Context(), client.ObjectKeyFromObject(&stale), ¤t); err != nil { + t.Fatal(err) + } + if current.Annotations[IdleAsleepSinceAnnotation] != "" { + t.Fatal("backup acquisition allowed stale sleep") + } +} + +func TestLifecycleBackupFenceConflictsWithStaleWorkloadWrite(t *testing.T) { + b := guardBackup("copy") + r, _ := guardFixture(t, b) + var stale appsv1.StatefulSet + key := client.ObjectKey{Namespace: "ns", Name: "server"} + if err := r.Get(t.Context(), key, &stale); err != nil { + t.Fatal(err) + } + if claimed, err := r.claimBackupTarget(t.Context(), b); err != nil || !claimed { + t.Fatalf("claim: %t %v", claimed, err) + } + gs, err := r.ownedBackupTarget(t.Context(), b) + if err != nil || gs == nil { + t.Fatalf("owned target: %v", err) + } + if ready, err := fenceBackupWorkload(t.Context(), r.Client, r.Client, gs); err != nil || !ready { + t.Fatalf("fence: %t %v", ready, err) + } + zero := int32(0) + stale.Spec.Replicas = &zero + stale.Spec.Template.Spec.Containers = []corev1.Container{{Name: "game", Image: "new-revision"}} + if err := r.Update(t.Context(), &stale); !apierrors.IsConflict(err) { + t.Fatalf("stale workload write defeated fence: %v", err) + } +} + +func TestLifecycleCSIInitialStartupExcludesBackupAndRollout(t *testing.T) { + r, gs, rs := restoreLifecycleFixture(t) + gs.Spec.Suspend = false + gs.Annotations = map[string]string{restoreGuardAnnotation: restoreIdentityJSON(rs.Name, rs.UID)} + if err := r.Update(t.Context(), gs); err != nil { + t.Fatal(err) + } + want := restoreLifecycleIdentity(gs, rs) + want.InitialStartup = true + if ready, err := claimLifecycleOperation(t.Context(), r.Client, r.apiReader(), gs, want, nil); err != nil || !ready { + t.Fatalf("claim initial startup: %t %v", ready, err) + } + var ss appsv1.StatefulSet + if err := r.Get(t.Context(), client.ObjectKeyFromObject(gs), &ss); err != nil { + t.Fatal(err) + } + ss.Spec.Template.Spec.Containers = []corev1.Container{{Name: "game", Image: "snapshot-version"}} + if err := r.Update(t.Context(), &ss); err != nil { + t.Fatal(err) + } + gr := &GameServerReconciler{Client: r.Client, APIReader: r.apiReader(), Scheme: r.Scheme} + replicas, _, err := gr.desiredReplicas(t.Context(), gs, &gameplanev1alpha1.GameTemplate{}, idleAwake) + if err != nil || replicas != 1 { + t.Fatalf("CSI initial startup blocked first consumer: replicas=%d err=%v", replicas, err) + } + if err := gr.reconcileStatefulSet(t.Context(), gs, &gameplanev1alpha1.GameTemplate{}, nil, &materializedConfig{}, replicas); err != nil { + t.Fatal(err) + } + if err := r.Get(t.Context(), client.ObjectKeyFromObject(gs), &ss); err != nil { + t.Fatal(err) + } + if *ss.Spec.Replicas != 1 || ss.Spec.Template.Spec.Containers[0].Image != "snapshot-version" { + t.Fatal("CSI restore either blocked startup or adopted a different template") + } + b := guardBackup("new-copy") + if err := r.Create(t.Context(), b); err != nil { + t.Fatal(err) + } + br := &BackupReconciler{Client: r.Client, APIReader: r.apiReader(), Scheme: r.Scheme} + if claimed, err := br.claimBackupTarget(t.Context(), b); err != nil || claimed { + t.Fatalf("backup entered CSI restore startup: %t %v", claimed, err) + } +} + +func TestLifecycleIndependentServersRemainConcurrent(t *testing.T) { + first, second := guardBackup("first"), guardBackup("second") + second.Spec.ServerRef.Name = "other" + r, _ := guardFixture(t, first, second) + other := &gameplanev1alpha1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "other", Namespace: "ns", UID: "other-uid"}} + if err := r.Create(t.Context(), other); err != nil { + t.Fatal(err) + } + for _, b := range []*gameplanev1alpha1.Backup{first, second} { + if claimed, err := r.claimBackupTarget(t.Context(), b); err != nil || !claimed { + t.Fatalf("independent target %s blocked: %t %v", b.Spec.ServerRef.Name, claimed, err) + } + } +} + +type lifecycleStopper struct{ calls int } + +func (s *lifecycleStopper) Enabled() bool { return true } +func (s *lifecycleStopper) Stop(context.Context, string, string) error { + s.calls++ + return nil +} + +func TestLifecycleBackupDefersRestartAndStopRPC(t *testing.T) { + for _, quiesce := range []bool{false, true} { + t.Run(fmt.Sprintf("quiesce=%t", quiesce), func(t *testing.T) { + b := guardBackup("copy") + b.Spec.Quiesce = quiesce + r, _ := guardFixture(t, b) + if claimed, err := r.claimBackupTarget(t.Context(), b); err != nil || !claimed { + t.Fatalf("claim: %t %v", claimed, err) + } + var gs gameplanev1alpha1.GameServer + key := client.ObjectKey{Namespace: "ns", Name: "server"} + if err := r.Get(t.Context(), key, &gs); err != nil { + t.Fatal(err) + } + gs.Annotations[RestartRequestedAnnotation] = "restart-after-copy" + gs.Spec.Suspend = true + if err := r.Update(t.Context(), &gs); err != nil { + t.Fatal(err) + } + var ss appsv1.StatefulSet + if err := r.Get(t.Context(), key, &ss); err != nil { + t.Fatal(err) + } + ss.Status.Replicas, ss.Status.ReadyReplicas = 1, 1 + if err := r.Update(t.Context(), &ss); err != nil { + t.Fatal(err) + } + stopper := &lifecycleStopper{} + gr := &GameServerReconciler{Client: r.Client, APIReader: r.Client, Scheme: r.Scheme, AgentClient: stopper} + tmpl := &gameplanev1alpha1.GameTemplate{Spec: gameplanev1alpha1.GameTemplateSpec{RCON: &gameplanev1alpha1.RCONSpec{Protocol: "source"}, Capabilities: &gameplanev1alpha1.CapabilitiesSpec{Lifecycle: &gameplanev1alpha1.LifecycleSpec{Stop: []string{"stop"}}}}} + if replicas, poll, err := gr.desiredReplicas(t.Context(), &gs, tmpl, idleAsleep); err != nil || replicas != 1 || poll == 0 { + t.Fatalf("restart changed protected source: replicas=%d poll=%s err=%v", replicas, poll, err) + } + if err := r.Get(t.Context(), key, &gs); err != nil { + t.Fatal(err) + } + if stopper.calls != 0 || gs.Annotations[RestartCompletedAnnotation] != "" || gs.Annotations[stopRequestedAtAnnotation] != "" { + t.Fatal("restart issued or acknowledged a stop during the copy") + } + }) + } +} diff --git a/operator/internal/controller/lifecycle_stop_admission_test.go b/operator/internal/controller/lifecycle_stop_admission_test.go new file mode 100644 index 000000000..caa42dc94 --- /dev/null +++ b/operator/internal/controller/lifecycle_stop_admission_test.go @@ -0,0 +1,131 @@ +package controller + +import ( + "context" + "errors" + "testing" + "time" + + 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" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" +) + +func TestLifecycleBackupAdmissionRequiresObservedStopAndDrainedWriters(t *testing.T) { + t.Parallel() + for _, scenario := range []struct { + name string + want bool + }{ + {"workload missing", false}, + {"foreign workload", false}, + {"replicas unspecified", false}, + {"desired replica remains", false}, + {"observed replica remains", false}, + {"generation unobserved", false}, + {"game pod running", false}, + {"PVC writer running", false}, + {"terminal pod still terminating", false}, + {"restart pending", false}, + {"terminal pod drained", true}, + {"unrelated pod running", true}, + {"stop complete", true}, + } { + t.Run(scenario.name, func(t *testing.T) { + t.Parallel() + gs := wipeGameServer(true, "") + gs.Annotations[stopRequestedAtAnnotation] = time.Now().UTC().Format(time.RFC3339) + ss := wipeStoppedSet(gs) + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: gs.Name + "-0", Namespace: gs.Namespace}, Status: corev1.PodStatus{Phase: corev1.PodRunning}} + var pods []client.Object + switch scenario.name { + case "workload missing": + ss = nil + case "foreign workload": + ss.OwnerReferences = nil + case "replicas unspecified": + ss.Spec.Replicas = nil + case "desired replica remains": + *ss.Spec.Replicas = 1 + case "observed replica remains": + ss.Status.Replicas = 1 + case "generation unobserved": + ss.Status.ObservedGeneration = ss.Generation - 1 + case "game pod running": + pods = append(pods, pod) + case "PVC writer running": + pod.Name = "external-writer" + pod.Spec.Volumes = []corev1.Volume{{Name: "data", VolumeSource: corev1.VolumeSource{PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ClaimName: gs.Name + "-data"}}}} + pods = append(pods, pod) + case "terminal pod still terminating": + pod.Status.Phase = corev1.PodSucceeded + pod.Finalizers = []string{"test/hold"} + now := metav1.Now() + pod.DeletionTimestamp = &now + pods = append(pods, pod) + case "restart pending": + gs.Annotations[RestartRequestedAnnotation] = "restart" + case "terminal pod drained": + pod.Status.Phase = corev1.PodSucceeded + pods = append(pods, pod) + case "unrelated pod running": + pod.Name = "unrelated" + pods = append(pods, pod) + } + b := guardBackup("new-copy") + b.Spec.ServerRef.Name = gs.Name + objects := append([]client.Object{gs, b}, pods...) + if ss != nil { + objects = append(objects, ss) + } + c := fake.NewClientBuilder().WithScheme(wipeScheme(t)).WithObjects(objects...).Build() + want := backupLifecycleIdentity(gs, b, false) + claimed, err := claimLifecycleOperation(t.Context(), c, c, gs, want, nil) + if err != nil || claimed != scenario.want { + t.Fatalf("backup admission=%t, want %t: %v", claimed, scenario.want, err) + } + current := wipeCurrent(t, c, gs) + op, held, err := lifecycleOwner(current) + if err != nil || held != scenario.want || (held && !sameLifecycleOwner(op.lifecycleIdentity, want)) { + t.Fatalf("durable admission disagrees with stop proof: held=%t owner=%+v err=%v", held, op, err) + } + if !current.Spec.Suspend || current.Annotations[stopRequestedAtAnnotation] == "" { + t.Fatal("backup admission changed the caller's stop intent") + } + }) + } +} + +func TestLifecycleStopProofFailsClosedWhenSafetyReadsFail(t *testing.T) { + t.Parallel() + for _, failRead := range []string{"workload", "pods"} { + t.Run(failRead, func(t *testing.T) { + t.Parallel() + gs := wipeGameServer(true, "") + gs.Annotations[stopRequestedAtAnnotation] = time.Now().UTC().Format(time.RFC3339) + cause := errors.New("API unavailable") + c := fake.NewClientBuilder().WithScheme(wipeScheme(t)).WithObjects(gs, wipeStoppedSet(gs)).Build() + reader := interceptor.NewClient(c, interceptor.Funcs{ + Get: func(ctx context.Context, c client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if _, workload := obj.(*appsv1.StatefulSet); workload && failRead == "workload" { + return cause + } + return c.Get(ctx, key, obj, opts...) + }, + List: func(ctx context.Context, c client.WithWatch, list client.ObjectList, opts ...client.ListOption) error { + if _, pods := list.(*corev1.PodList); pods && failRead == "pods" { + return cause + } + return c.List(ctx, list, opts...) + }, + }) + pending, err := backupLifecycleTransitionPending(t.Context(), reader, gs) + if !pending || !errors.Is(err, cause) { + t.Fatalf("failed %s read allowed copy admission: pending=%t err=%v", failRead, pending, 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_controller.go b/operator/internal/controller/restore_controller.go index 4cbf94e10..42cc2a7b2 100644 --- a/operator/internal/controller/restore_controller.go +++ b/operator/internal/controller/restore_controller.go @@ -2,6 +2,7 @@ package controller import ( "context" + "errors" "fmt" "time" @@ -41,6 +42,9 @@ type RestoreReconciler struct { // DefaultBackupJobBackoffLimit / DefaultBackupJobActiveDeadlineSeconds. JobBackoffLimit *int32 JobActiveDeadlineSeconds int64 + // OperationQuiescer completes an orphaned backup's cleanup before this + // restore can acquire the same target. It never quiesces a restore. + OperationQuiescer AgentQuiescer } // apiReader returns the uncached reader for live-consistency checks, @@ -125,7 +129,10 @@ func (r *RestoreReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct } return ctrl.Result{}, err } - if src.Spec.Strategy == "volume-snapshot" && rs.Annotations[restoreTargetAnnotation] == "" { + // Binding the target does not change the restore strategy. CSI's own + // preparation path checks that binding before awaiting startup, repairing + // references and enforcing the deadline on every subsequent reconcile. + if src.Spec.Strategy == "volume-snapshot" { return r.reconcileVolumeSnapshotRestore(ctx, &rs, &src) } @@ -156,6 +163,9 @@ func (r *RestoreReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct } claimed, err := r.claimRestoreTarget(ctx, &rs, &gs) if err != nil { + if errors.Is(err, errLifecycleGenerationChanged) { + return r.fail(ctx, &rs, err.Error()) + } return ctrl.Result{}, err } if !claimed { diff --git a/operator/internal/controller/restore_lifecycle.go b/operator/internal/controller/restore_lifecycle.go index a405b967a..91916ea54 100644 --- a/operator/internal/controller/restore_lifecycle.go +++ b/operator/internal/controller/restore_lifecycle.go @@ -70,7 +70,15 @@ func (r *RestoreReconciler) bindRestoreTarget(ctx context.Context, rs *gameplane // cannot both claim the same GameServer, even across operator processes. Suspend // is written in that same update; a crash cannot leave a lock without suspension. func (r *RestoreReconciler) claimRestoreTarget(ctx context.Context, rs *gameplanev1alpha1.Restore, gs *gameplanev1alpha1.GameServer) (bool, error) { - owner := restoreIdentityJSON(rs.Name, rs.UID) + // Refresh before both compatibility checks and shared-owner admission. + var live gameplanev1alpha1.GameServer + if err := r.apiReader().Get(ctx, client.ObjectKeyFromObject(gs), &live); err != nil { + return false, err + } + if live.UID != gs.UID { + return false, fmt.Errorf("restore target changed identity") + } + *gs = live workers, err := wipeWorkers(ctx, r.apiReader(), gs) if err != nil { return false, err @@ -95,38 +103,7 @@ func (r *RestoreReconciler) claimRestoreTarget(ctx context.Context, rs *gameplan } return false, nil } - if guard := gs.Annotations[restoreGuardAnnotation]; guard != "" && guard != owner { - identity, err := parseRestoreIdentity(guard) - if err != nil { - return false, err - } // Malformed guards fail closed. - var holder gameplanev1alpha1.Restore - err = r.apiReader().Get(ctx, types.NamespacedName{Namespace: gs.Namespace, Name: identity.Name}, &holder) - if err == nil && holder.UID == identity.UID { - return false, nil - } - if err != nil && !apierrors.IsNotFound(err) { - return false, err - } - // Force-deleted owners can leave their Job and pods behind. Cancel the - // old UID's Job and wait for its workers before reclaiming the guard. - busy, err := r.restoreWorkersLive(ctx, gs.Namespace, identity, true) - if err != nil || busy { - return false, err - } - } - if gs.Annotations[restoreGuardAnnotation] == owner && gs.Spec.Suspend { - return true, nil - } - if gs.Annotations == nil { - gs.Annotations = make(map[string]string) - } - gs.Annotations[restoreGuardAnnotation] = owner - gs.Spec.Suspend = true - if err := r.Update(ctx, gs); err != nil { - return false, err - } - return true, nil + return claimLifecycleOperation(ctx, r.Client, r.apiReader(), gs, restoreLifecycleIdentity(gs, rs), r.OperationQuiescer) } func liveRestorePod(pod *corev1.Pod) bool { @@ -280,6 +257,51 @@ func (r *RestoreReconciler) cleanupRestore(ctx context.Context, rs *gameplanev1a return ctrl.Result{}, err } if err == nil && gs.UID == target.UID && gs.Annotations[restoreGuardAnnotation] == restoreIdentityJSON(rs.Name, rs.UID) { + if value := gs.Annotations[lifecycleOperationAnnotation]; value != "" { + op, err := parseLifecycleOperation(value) + 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 + } var ss appsv1.StatefulSet ssErr := r.apiReader().Get(ctx, client.ObjectKeyFromObject(&gs), &ss) if ssErr != nil && !apierrors.IsNotFound(ssErr) { 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..78da66458 --- /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, _, 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/internal/controller/restore_volumesnapshot.go b/operator/internal/controller/restore_volumesnapshot.go index c2d1819f5..00705401c 100644 --- a/operator/internal/controller/restore_volumesnapshot.go +++ b/operator/internal/controller/restore_volumesnapshot.go @@ -76,6 +76,13 @@ func (r *RestoreReconciler) reconcileVolumeSnapshotRestore( "target server %q already exists; volume-snapshot restores create a new server", rs.Spec.ServerRef.Name)) } + ready, err := r.prepareSnapshotRestoreTarget(ctx, rs, &newGS) + if errors.Is(err, errLifecycleGenerationChanged) { + return r.fail(ctx, rs, err.Error()) + } + if err != nil || !ready { + return ctrl.Result{RequeueAfter: 5 * time.Second}, err + } return r.awaitRestoredServer(ctx, rs, src, &newGS) case !apierrors.IsNotFound(err): return ctrl.Result{}, err @@ -94,9 +101,14 @@ func (r *RestoreReconciler) reconcileVolumeSnapshotRestore( created := &gameplanev1alpha1.GameServer{ ObjectMeta: metav1.ObjectMeta{ - Name: rs.Spec.ServerRef.Name, - Namespace: rs.Namespace, - Annotations: map[string]string{annoRestoreCreatedBy: rs.Name}, + Name: rs.Spec.ServerRef.Name, + Namespace: rs.Namespace, + Annotations: map[string]string{ + annoRestoreCreatedBy: rs.Name, + // Reserve the target atomically with creation. Once its UID is + // assigned, bind the full shared owner before permitting startup. + restoreGuardAnnotation: restoreIdentityJSON(rs.Name, rs.UID), + }, }, } orig.Spec.DeepCopyInto(&created.Spec) @@ -211,6 +223,26 @@ func (r *RestoreReconciler) ensureRestoredRefsFromSpec( return nil } +func (r *RestoreReconciler) prepareSnapshotRestoreTarget(ctx context.Context, rs *gameplanev1alpha1.Restore, gs *gameplanev1alpha1.GameServer) (bool, error) { + changed, err := r.bindRestoreTarget(ctx, rs, gs) + if err != nil { + return false, err + } + if changed { + return false, nil + } + target, err := parseRestoreIdentity(rs.Annotations[restoreTargetAnnotation]) + if err != nil { + return false, err + } + if target.UID != gs.UID || target.Name != gs.Name { + return false, fmt.Errorf("CSI restore target changed identity") + } + want := restoreLifecycleIdentity(gs, rs) + want.InitialStartup = true + return claimLifecycleOperation(ctx, r.Client, r.apiReader(), gs, want, r.OperationQuiescer) +} + // awaitRestoredServer drives the Restore to a terminal phase based on the // newly-provisioned server: Running (with its references copied) → // Succeeded, Failed → Failed, otherwise keep polling while it starts up diff --git a/operator/internal/controller/restore_volumesnapshot_dispatch_test.go b/operator/internal/controller/restore_volumesnapshot_dispatch_test.go new file mode 100644 index 000000000..5ca38869b --- /dev/null +++ b/operator/internal/controller/restore_volumesnapshot_dispatch_test.go @@ -0,0 +1,81 @@ +package controller + +import ( + "encoding/json" + "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" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + gameplanev1alpha1 "github.com/GameplanePanel/gameplane/operator/api/v1alpha1" +) + +// Exercise the public dispatch after target binding. Calling the CSI helper +// directly misses a regression that diverted later passes into Restic restore. +func TestVolumeSnapshotRestoreReconcileKeepsBoundStrategy(t *testing.T) { + for _, state := range []string{"running", "repair-reference", "deadline"} { + t.Run(state, func(t *testing.T) { + scheme := scrapeScheme(t) + if err := appsv1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + orig := &gameplanev1alpha1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "source", Namespace: "ns", UID: "source-uid"}} + target := &gameplanev1alpha1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "target", Namespace: "ns", UID: "target-uid", Annotations: map[string]string{annoRestoreCreatedBy: "restore", restoreGuardAnnotation: restoreIdentityJSON("restore", "restore-uid")}}, Spec: gameplanev1alpha1.GameServerSpec{TemplateRef: gameplanev1alpha1.GameTemplateRef{Name: "template"}}} + backup := &gameplanev1alpha1.Backup{ObjectMeta: metav1.ObjectMeta{Name: "backup", Namespace: "ns", UID: "backup-uid"}, Spec: gameplanev1alpha1.BackupSpec{ServerRef: gameplanev1alpha1.LocalObjectRef{Name: orig.Name}, Strategy: "volume-snapshot"}, Status: gameplanev1alpha1.BackupStatus{Phase: gameplanev1alpha1.BackupPhaseSucceeded, SnapshotID: "snapshot", VolumeSnapshotContentName: "content"}} + started := metav1.Now() + rs := &gameplanev1alpha1.Restore{ObjectMeta: metav1.ObjectMeta{Name: "restore", Namespace: "ns", UID: "restore-uid", Finalizers: []string{restoreCleanupFinalizer}, Annotations: map[string]string{restoreTargetAnnotation: restoreIdentityJSON(target.Name, target.UID)}}, Spec: gameplanev1alpha1.RestoreSpec{BackupRef: gameplanev1alpha1.LocalObjectRef{Name: backup.Name}, ServerRef: gameplanev1alpha1.LocalObjectRef{Name: target.Name}}, Status: gameplanev1alpha1.RestoreStatus{Phase: gameplanev1alpha1.RestorePhaseRunning, SnapshotID: "snapshot", StartTime: &started}} + objects := []client.Object{orig, target, backup, rs} + want := gameplanev1alpha1.RestorePhaseRunning + switch state { + case "running": + target.Status.Phase = gameplanev1alpha1.GameServerPhaseRunning + want = gameplanev1alpha1.RestorePhaseSucceeded + case "repair-reference": + plan, err := json.Marshal([]refCopy{{Kind: secretRefKind, OrigName: "source-secret", CopyName: "target-secret"}}) + if err != nil { + t.Fatal(err) + } + rs.Annotations["restore.gameplane.local/copy-plan"] = string(plan) + target.Spec.Env = []corev1.EnvVar{{Name: "SECRET", ValueFrom: &corev1.EnvVarSource{SecretKeyRef: &corev1.SecretKeySelector{LocalObjectReference: corev1.LocalObjectReference{Name: "target-secret"}, Key: "key"}}}} + objects = append(objects, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "source-secret", Namespace: "ns", OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(orig, gameplanev1alpha1.GroupVersion.WithKind("GameServer"))}}, Data: map[string][]byte{"key": []byte("preserved-value")}}) + case "deadline": + started = metav1.NewTime(time.Now().Add(-VolumeSnapshotRestoreDeadline - time.Minute)) + want = gameplanev1alpha1.RestorePhaseFailed + } + cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).WithStatusSubresource(rs, target, backup).Build() + r := &RestoreReconciler{Client: cl, APIReader: cl, Scheme: scheme} + if _, err := r.Reconcile(t.Context(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(rs)}); err != nil { + t.Fatal(err) + } + var got gameplanev1alpha1.Restore + if err := cl.Get(t.Context(), client.ObjectKeyFromObject(rs), &got); err != nil { + t.Fatal(err) + } + if got.Status.Phase != want { + t.Fatalf("bound CSI restore phase = %s, want %s: %s", got.Status.Phase, want, got.Status.Message) + } + if state == "repair-reference" { + var copied corev1.Secret + if err := cl.Get(t.Context(), client.ObjectKey{Namespace: "ns", Name: "target-secret"}, &copied); err != nil { + t.Fatalf("bound CSI restore did not repair reference: %v", err) + } + if !metav1.IsControlledBy(&copied, target) || string(copied.Data["key"]) != "preserved-value" { + t.Fatal("repaired reference lost ownership or data") + } + } + var jobs batchv1.JobList + if err := cl.List(t.Context(), &jobs); err != nil { + t.Fatal(err) + } + if len(jobs.Items) != 0 { + t.Fatal("CSI restore dispatched into Restic Job creation") + } + }) + } +} diff --git a/operator/internal/controller/restore_volumesnapshot_requeue_test.go b/operator/internal/controller/restore_volumesnapshot_requeue_test.go index 78b963678..64ebfb51b 100644 --- a/operator/internal/controller/restore_volumesnapshot_requeue_test.go +++ b/operator/internal/controller/restore_volumesnapshot_requeue_test.go @@ -16,6 +16,7 @@ import ( "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/controller/controllerutil" gameplanev1alpha1 "github.com/GameplanePanel/gameplane/operator/api/v1alpha1" ) @@ -75,7 +76,7 @@ func TestVolumeSnapshotRestore_TransientErrorsRequeueNotFail(t *testing.T) { }, } restore := &gameplanev1alpha1.Restore{ - ObjectMeta: metav1.ObjectMeta{Name: "rs", Namespace: ns}, + ObjectMeta: metav1.ObjectMeta{Name: "rs", Namespace: ns, UID: "uid-restore"}, Spec: gameplanev1alpha1.RestoreSpec{ BackupRef: gameplanev1alpha1.LocalObjectRef{Name: "smp-vs"}, ServerRef: gameplanev1alpha1.LocalObjectRef{Name: restoredName}, @@ -105,6 +106,8 @@ func TestVolumeSnapshotRestore_TransientErrorsRequeueNotFail(t *testing.T) { switch o := obj.(type) { case *gameplanev1alpha1.GameServer: if o.Name == restoredName { + // The fake client does not assign API-server identities. + o.UID = "uid-restored" restoredCreated = true } case *corev1.Secret: @@ -149,13 +152,32 @@ func TestVolumeSnapshotRestore_TransientErrorsRequeueNotFail(t *testing.T) { if err := pass("stale-cache pass"); err != nil { t.Fatalf("stale-cache pass: want requeue without error, got %v", err) } - // Pass 3: cache caught up; the await path re-ensures the copy but the - // write still fails transiently. + // Pass 3: cache caught up; persist the new target's UID and cleanup + // finalizer before claiming its durable lifecycle owner on the next pass. staleRestored = false + if err := pass("bind target pass"); err != nil { + t.Fatalf("bind target pass: unexpected error: %v", err) + } + var bound gameplanev1alpha1.Restore + if err := cl.Get(ctx, client.ObjectKeyFromObject(restore), &bound); err != nil { + t.Fatalf("get bound restore: %v", err) + } + target, err := parseRestoreIdentity(bound.Annotations[restoreTargetAnnotation]) + if err != nil || target.Name != restoredName || target.UID != "uid-restored" { + t.Fatalf("bound target = %+v, error = %v; want restored server identity", target, err) + } + if !controllerutil.ContainsFinalizer(&bound, restoreCleanupFinalizer) { + t.Fatal("bound restore is missing its cleanup finalizer") + } + if failSecretCreates != 1 { + t.Fatalf("binding attempted a copy before admission: remaining failures = %d, want 1", failSecretCreates) + } + // Pass 4: the shared owner is acquired; the await path re-ensures the + // copy but the write still fails transiently. if err := pass("await pass with write error"); err == nil { t.Fatal("await pass: want the transient copy error returned for requeue, got nil") } - // Pass 4: errors cleared; the copy is made. + // Pass 5: errors cleared; the copy is made. if err := pass("await pass"); err != nil { t.Fatalf("await pass: unexpected error: %v", err) } @@ -165,6 +187,10 @@ func TestVolumeSnapshotRestore_TransientErrorsRequeueNotFail(t *testing.T) { if err := cl.Get(ctx, types.NamespacedName{Namespace: ns, Name: restoredName}, &restored); err != nil { t.Fatalf("get restored server: %v", err) } + owner, occupied, err := lifecycleOwner(&restored) + if err != nil || !occupied || !sameLifecycleOwner(owner.lifecycleIdentity, restoreLifecycleIdentity(&restored, restore)) || !owner.InitialStartup { + t.Fatalf("restored lifecycle owner = %+v, occupied = %v, error = %v; want initial-startup restore owner", owner, occupied, err) + } if got := restored.Spec.Env[0].ValueFrom.SecretKeyRef.Name; got != copyName { t.Errorf("restored env secret ref = %q, want copy %q", got, copyName) } diff --git a/operator/specs.md b/operator/specs.md index 23e083513..0cf9fa697 100644 --- a/operator/specs.md +++ b/operator/specs.md @@ -150,7 +150,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:** @@ -218,8 +218,8 @@ Primary reconcilers register with the manager in `cmd/main.go` and handle CRD li - **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. - - Quiesced copies are serialized per target GameServer UID across schedules and on-demand requests. The Backup pins its target in `backup.gameplane.local/target`; the GameServer's `backup.gameplane.local/owner` stores the owning Backup name/UID. Live API reads and resourceVersion updates protect acquisition/release across reconciler restarts. Quiesce cleanup intent is recorded before calling the agent, including partial failures. Cleanup waits for live copy workers; deletion cancels the copy Job with foreground propagation first. Force-deleted owners are recovered under exclusive ownership, draining their workers and restoring saving before the next copy. Pre-upgrade quiesced Backups without a target annotation finish and release all legacy workers before new protected copies may start. - - Orphan recovery preserves all predecessor names/UIDs in the GameServer's `backup.gameplane.local/recovering-owners` annotation, atomically with ownership transfers, so deleting a recovering Backup 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. + - 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. - 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. @@ -238,16 +238,54 @@ Primary reconcilers register with the manager in `cmd/main.go` and handle CRD li ### RestoreReconciler - **Responsibility:** Drive Restore to completion: suspend GameServer, run restic restore Job, resume. - **Key functions:** + - Both restore strategies participate in shared lifecycle exclusion. A CSI restore reserves its new target with a Restore guard in the GameServer Create, then pins the target UID and adds the full owner. Its `initialStartup` mode permits the first workload to run, including PVCs using WaitForFirstConsumer; backup, wipe, restart, idle transitions and template rollout still wait for restore completion. The original server is unaffected. - Pin snapshot ID at observation (immutable during restore). - Restic restores pin the target GameServer name/UID on the Restore and add a cleanup finalizer before acquiring an annotation guard on that GameServer. The guard records the owning Restore name/UID and is acquired with an optimistic resourceVersion update that also sets spec.suspend=true. Other restores targeting that server wait; restores of independent targets may run concurrently. The game workload remains suspended while the guard exists, even if a power request clears spec.suspend; data-wipe reconciliation also waits for the guard to clear. - A nonempty pending data-wipe request blocks guard acquisition, including after wipe failure. Restore status explains this wait. An uncached read of the fixed-name wipe Job identifies a failed current request only when the current GameServer controls it, its token matches, and it reports Failed=True; DataWipe=False alone is insufficient. Confirmed failure reports that restore is blocked and asks the operator to inspect wipe Job logs and resolve the wipe failure. The restore does not cancel or acknowledge the wipe, since an in-flight wipe reconciliation could still create a writer. The wait message clears when restore acquires the target normally. - 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. +### Shared lifecycle exclusion + +`lifecycle_operation.go` is the admission boundary for Backup, Restore and data +wipe. `operations.gameplane.local/owner` on each GameServer stores kind, name, +operation UID, target UID and admitted generation. Wipes use the GameServer UID +plus their existing request token. Acquisition refreshes the target with the +uncached reader and commits the owner and compatibility guard in one checked +resourceVersion Update. Existing owners never expire on time alone. Independent +servers remain concurrent; compatible reader modes are not supported. + +After acquisition, backups fence the owned StatefulSet with a checked metadata +write and wait for its current generation and rollout revision to be observed. +Workload mutation reads ownership inside its callback: backup ownership freezes +replicas and the complete PodTemplate, while restore/wipe may stop the existing +workload but cannot roll it. A missing workload is created stopped so it can be +fenced. Idle sleep/wake and restart acknowledgment use checked writes; a stop RPC +is preceded by a checked durable stop intent which prevents backup admission +until that process drains. Ordinary Services, credentials and status continue +reconciling while operations hold the workload. + +A missing owner is recovered by a successor under exclusive ownership. Recovery +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, 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 +different copy under the old owner. See [operator operation recovery](../docs/operator-operations.md). + ### ModuleSourceReconciler - **Responsibility:** Index module sources and surface available modules into status. - **Key functions:** diff --git a/test/e2e/api_events_roles_scope_e2e_test.go b/test/e2e/api_events_roles_scope_e2e_test.go index 2ab19b2b4..4c3b388c0 100644 --- a/test/e2e/api_events_roles_scope_e2e_test.go +++ b/test/e2e/api_events_roles_scope_e2e_test.go @@ -12,6 +12,9 @@ import ( "strings" "testing" "time" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" ) // TestAPI_EventStreamAndRoleEdits_FollowCallerPermissions checks two @@ -64,19 +67,37 @@ func TestAPI_EventStreamAndRoleEdits_FollowCallerPermissions(t *testing.T) { applyBusyboxTemplate(t, tmpl) createGameServerViaAPI(t, admin, ns, gsName, tmpl) + // /events starts at each collection's current resourceVersion. Drive real + // updates throughout both streams instead of depending on initial replay + // or other parallel tests to happen to mutate a readable resource kind. + changes := func(ctx context.Context) error { + patch, err := json.Marshal(map[string]any{"metadata": map[string]any{"annotations": map[string]string{ + "gameplane.local/e2e-event-sequence": time.Now().UTC().Format(time.RFC3339Nano), + }}}) + if err != nil { + return fmt.Errorf("encode event trigger: %w", err) + } + if _, err := envInstance.Dyn.Resource(gameTemplateGVR).Patch(ctx, tmpl, types.MergePatchType, patch, metav1.PatchOptions{}); err != nil { + return fmt.Errorf("trigger template event: %w", err) + } + if _, err := envInstance.Dyn.Resource(gameServerGVR).Namespace(ns).Patch(ctx, gsName, types.MergePatchType, patch, metav1.PatchOptions{}); err != nil { + return fmt.Errorf("trigger server event: %w", err) + } + return nil + } user := envInstance.APIClient(t, userName, userPW) defer user.Close() t.Run("admin event stream includes templates", func(t *testing.T) { - kinds, found := streamEventKinds(t, admin, ns, func(kind, _ string) bool { return kind == "templates" }, 0) + kinds, found := streamEventKinds(t, admin, ns, func(kind, name string) bool { return kind == "templates" && name == tmpl }, 0, changes) if !found { t.Fatalf("admin stream sent no templates event; kinds=%v", kinds) } }) t.Run("event stream sends only kinds the caller may read", func(t *testing.T) { - kinds, found := streamEventKinds(t, user, ns, func(kind, name string) bool { return kind == "servers" && name == gsName }, 3*time.Second) + kinds, found := streamEventKinds(t, user, ns, func(kind, name string) bool { return kind == "servers" && name == gsName }, 3*time.Second, changes) if !found { t.Fatalf("user stream never sent server %s; kinds=%v", gsName, kinds) } @@ -126,11 +147,41 @@ func TestAPI_EventStreamAndRoleEdits_FollowCallerPermissions(t *testing.T) { // streamEventKinds reads GET /events as cli and returns the set of event // kinds it received. When match reports true for a frame, it keeps // reading for grace (0 = stop at once); otherwise it stops after 30s. +// changes keeps both permitted and forbidden kinds active while connecting and +// reading; its worker is cancelled and joined before the helper returns. // found reports whether match was ever true. -func streamEventKinds(t *testing.T, cli *APIClient, ns string, match func(kind, name string) bool, grace time.Duration) (map[string]bool, bool) { +func streamEventKinds(t *testing.T, cli *APIClient, ns string, match func(kind, name string) bool, grace time.Duration, changes func(context.Context) error) (map[string]bool, bool) { t.Helper() ctx, cancel := context.WithTimeout(t.Context(), 30*time.Second) defer cancel() + changesDone := make(chan struct{}) + var changesErr error + go func() { + defer close(changesDone) + ticker := time.NewTicker(500 * time.Millisecond) + defer ticker.Stop() + for { + if err := changes(ctx); err != nil { + if ctx.Err() == nil { + changesErr = err + } + cancel() + return + } + select { + case <-ctx.Done(): + return + case <-ticker.C: + } + } + }() + defer func() { + cancel() + <-changesDone + if changesErr != nil { + t.Errorf("event trigger failed: %v", changesErr) + } + }() req, err := http.NewRequestWithContext(ctx, http.MethodGet, cli.BaseURL+"/events?namespace="+ns, nil) if err != nil { t.Fatalf("new events request: %v", err)