diff --git a/cmd/ateapi/internal/actoridentity/actoridentity.go b/cmd/ateapi/internal/actoridentity/actoridentity.go index 963c2e8077..e9c83410ea 100644 --- a/cmd/ateapi/internal/actoridentity/actoridentity.go +++ b/cmd/ateapi/internal/actoridentity/actoridentity.go @@ -384,9 +384,9 @@ func (s *Server) authorizeActor(ctx context.Context, caller *ateletCaller, actor } // The worker must still agree that it is hosting this actor. - if assigned := worker.GetAssignment().GetActor(); resources.ActorRefFromObjectRef(assigned) != actorRef { + if assignedActorUID := worker.GetAssignment().GetActorUid(); assignedActorUID != actor.GetMetadata().GetUid() { return nil, deny("worker is no longer assigned to the actor", - slog.String("workerAssignment", assigned.GetAtespace()+"/"+assigned.GetName())) + slog.String("assignedActorUID", assignedActorUID)) } return actor, nil diff --git a/cmd/ateapi/internal/actoridentity/actoridentity_test.go b/cmd/ateapi/internal/actoridentity/actoridentity_test.go index 68796a2e94..d6982fa311 100644 --- a/cmd/ateapi/internal/actoridentity/actoridentity_test.go +++ b/cmd/ateapi/internal/actoridentity/actoridentity_test.go @@ -180,6 +180,8 @@ type actorFixture struct { noPlacement bool // noWorker skips seeding the worker record entirely. noWorker bool + // mismatchedUID simulates a worker assigned to an actor with the same name/atespace but a different UID. + mismatchedUID bool } // seedActor writes an actor, and normally its hosting worker, into st. @@ -201,7 +203,8 @@ func seedActor(t *testing.T, ctx context.Context, st store.Interface, f actorFix WorkerPodUid: "worker-uid", } } - if _, err := st.CreateActor(ctx, actor); err != nil { + created, err := st.CreateActor(ctx, actor) + if err != nil { t.Fatalf("seed actor: %v", err) } @@ -212,6 +215,10 @@ func seedActor(t *testing.T, ctx context.Context, st store.Interface, f actorFix if assigned == (resources.ActorRef{}) { assigned = actorRef } + assignedActorUID := created.GetMetadata().GetUid() + if f.mismatchedUID || assigned != actorRef { + assignedActorUID = "other-actor-uid" + } worker := &ateapipb.Worker{ WorkerNamespace: testPodNS, WorkerPool: testPool, @@ -219,7 +226,10 @@ func seedActor(t *testing.T, ctx context.Context, st store.Interface, f actorFix WorkerPodUid: "worker-uid", NodeName: f.workerNode, State: ateapipb.Worker_STATE_ACTIVE, - Assignment: &ateapipb.Assignment{Actor: assigned.ToObjectRef()}, + Assignment: &ateapipb.Assignment{ + Actor: assigned.ToObjectRef(), + ActorUid: assignedActorUID, + }, } if f.unassigned { worker.Assignment = nil @@ -319,6 +329,14 @@ func TestMintCertAuthorization(t *testing.T) { }, wantCode: codes.PermissionDenied, }, + "worker is assigned to an actor with same name and atespace but different UID": { + fixture: actorFixture{ + status: ateapipb.Actor_STATUS_RUNNING, + workerNode: testNode, + mismatchedUID: true, + }, + wantCode: codes.PermissionDenied, + }, "hosting worker record is missing": { fixture: actorFixture{ status: ateapipb.Actor_STATUS_RUNNING, diff --git a/cmd/ateapi/internal/controlapi/crash.go b/cmd/ateapi/internal/controlapi/crash.go index f4e13740ef..44daa4c798 100644 --- a/cmd/ateapi/internal/controlapi/crash.go +++ b/cmd/ateapi/internal/controlapi/crash.go @@ -123,7 +123,7 @@ func releaseWorker(ctx context.Context, st store.Interface, actor *ateapipb.Acto return sandboxClass, nil } // Only free it if it still belongs to us - if resources.ActorRefFromObjectRef(wass.GetActor()) != resources.ActorRefFromActor(actor) { + if wass.GetActorUid() != actor.GetMetadata().GetUid() { slog.WarnContext(ctx, "Worker already assigned to another Actor", slog.String("worker", podUid)) return sandboxClass, nil } diff --git a/cmd/ateapi/internal/controlapi/crash_test.go b/cmd/ateapi/internal/controlapi/crash_test.go index 462af5dae3..4726019956 100644 --- a/cmd/ateapi/internal/controlapi/crash_test.go +++ b/cmd/ateapi/internal/controlapi/crash_test.go @@ -62,8 +62,17 @@ func seedWorker(t *testing.T, ctx context.Context, st store.Interface, actorRef WorkerPod: "pod", } if actorRef != (resources.ActorRef{}) { - worker.Assignment = &ateapipb.Assignment{ - Actor: actorRef.ToObjectRef(), + actor, err := st.GetActor(ctx, actorRef) + if err != nil { + worker.Assignment = &ateapipb.Assignment{ + Actor: &ateapipb.ObjectRef{Atespace: actorRef.Atespace, Name: actorRef.Name}, + ActorUid: "synthetic-" + actorRef.Name, + } + } else { + worker.Assignment = &ateapipb.Assignment{ + Actor: &ateapipb.ObjectRef{Atespace: actor.GetMetadata().GetAtespace(), Name: actor.GetMetadata().GetName()}, + ActorUid: actor.GetMetadata().GetUid(), + } } } if err := st.CreateWorker(ctx, worker); err != nil { @@ -161,7 +170,45 @@ func TestCrashActor(t *testing.T) { t.Fatalf("GetWorker() = %v, want nil", gerr) } if got := worker.GetAssignment().GetActor().GetName(); got != "actor-2" { - t.Errorf("worker assigned actor = %q, want %q", got, "actor-2") + t.Errorf("worker assigned actor name = %q, want %q", got, "actor-2") + } + if got := worker.GetAssignment().GetActorUid(); got != "synthetic-actor-2" { + t.Errorf("worker assigned actor uid = %q, want %q", got, "synthetic-actor-2") + } + }, + }, + { + name: "keeps worker assigned to previous incarnation of same actor", + seed: true, + setup: func(t *testing.T, ctx context.Context, st store.Interface) { + // Create a worker assigned to the same actorRef, but with a stale UID + worker := &ateapipb.Worker{ + WorkerNamespace: "ns", + WorkerPool: "pool", + WorkerPod: "pod", + Assignment: &ateapipb.Assignment{ + Actor: &ateapipb.ObjectRef{Atespace: actorRef.Atespace, Name: actorRef.Name}, + ActorUid: "stale-incarnation-uid", + }, + } + if err := st.CreateWorker(ctx, worker); err != nil { + t.Fatalf("CreateWorker: %v", err) + } + }, + check: func(t *testing.T, ctx context.Context, st store.Interface, err error) { + if err != nil { + t.Fatalf("crashActor() = %v, want nil", err) + } + assertCrashed(t, ctx, st, actorRef) + worker, gerr := st.GetWorker(ctx, "ns", "pool", "pod") + if gerr != nil { + t.Fatalf("GetWorker() = %v, want nil", gerr) + } + if got := worker.GetAssignment().GetActor().GetName(); got != actorRef.Name { + t.Errorf("worker assigned actor name = %q, want %q", got, actorRef.Name) + } + if got := worker.GetAssignment().GetActorUid(); got != "stale-incarnation-uid" { + t.Errorf("worker assigned actor uid = %q, want %q", got, "stale-incarnation-uid") } }, }, diff --git a/cmd/ateapi/internal/controlapi/functional_test.go b/cmd/ateapi/internal/controlapi/functional_test.go index 84bb5c92aa..66b2f73076 100644 --- a/cmd/ateapi/internal/controlapi/functional_test.go +++ b/cmd/ateapi/internal/controlapi/functional_test.go @@ -1710,6 +1710,7 @@ func TestResumeActor(t *testing.T) { Name: name, Atespace: testAtespace, }, + ActorUid: getResp.GetMetadata().GetUid(), }, Ip: "127.0.0.1", NodeName: "node1", diff --git a/cmd/ateapi/internal/controlapi/syncer.go b/cmd/ateapi/internal/controlapi/syncer.go index 77919b439f..d547c4d2b0 100644 --- a/cmd/ateapi/internal/controlapi/syncer.go +++ b/cmd/ateapi/internal/controlapi/syncer.go @@ -345,16 +345,20 @@ func (s *WorkerPoolSyncer) releaseActorOnDeadWorker(ctx context.Context, namespa } return err } - if worker.Assignment == nil { + if worker.Assignment == nil || worker.Assignment.GetActor() == nil { return nil } - actor, err := s.persistence.GetActor(ctx, resources.ActorRefFromObjectRef(worker.Assignment.Actor)) + actorRef := resources.ActorRefFromObjectRef(worker.Assignment.GetActor()) + actor, err := s.persistence.GetActor(ctx, actorRef) if err != nil { if errors.Is(err, store.ErrNotFound) { return nil } return err } + if actor.GetMetadata().GetUid() != worker.Assignment.GetActorUid() { + return nil + } // Skip if a concurrent SuspendActor already cleared the pointer. assignment := actor.GetWorkerAssignment() if assignment.GetWorkerNamespace() != namespace || assignment.GetWorkerPod() != podName { diff --git a/cmd/ateapi/internal/controlapi/syncer_test.go b/cmd/ateapi/internal/controlapi/syncer_test.go index 1569d5f10a..33fae9b5c4 100644 --- a/cmd/ateapi/internal/controlapi/syncer_test.go +++ b/cmd/ateapi/internal/controlapi/syncer_test.go @@ -256,7 +256,7 @@ func TestSyncer_DeleteBoundWorker_ClearsActor(t *testing.T) { t.Fatalf("worker row not materialised: %v", err) } actorName := "actor-orphan" - if _, err := persistence.CreateActor(ctx, &ateapipb.Actor{ + createdActor, err := persistence.CreateActor(ctx, &ateapipb.Actor{ Metadata: &ateapipb.ResourceMetadata{Name: actorName, Atespace: "team-orphan"}, ActorTemplateNamespace: ns, ActorTemplateName: "tmpl", Status: ateapipb.Actor_STATUS_RUNNING, WorkerAssignment: &ateapipb.WorkerAssignment{ @@ -264,7 +264,8 @@ func TestSyncer_DeleteBoundWorker_ClearsActor(t *testing.T) { }, InProgressSnapshot: "gs://snapshots/partial", LatestSnapshot: &ateapipb.ObjectRef{Atespace: "team-orphan", Name: "last"}, - }); err != nil { + }) + if err != nil { t.Fatalf("create actor: %v", err) } w, _ := persistence.GetWorker(ctx, ns, pool, pod) @@ -273,10 +274,8 @@ func TestSyncer_DeleteBoundWorker_ClearsActor(t *testing.T) { Namespace: ns, Name: "tmpl", }, - Actor: &ateapipb.ObjectRef{ - Name: actorName, - Atespace: "team-orphan", - }, + Actor: &ateapipb.ObjectRef{Atespace: createdActor.GetMetadata().GetAtespace(), Name: createdActor.GetMetadata().GetName()}, + ActorUid: createdActor.GetMetadata().GetUid(), } if err := persistence.UpdateWorker(ctx, w, w.Version); err != nil { t.Fatalf("update worker: %v", err) @@ -560,13 +559,14 @@ func TestReconcileDeadWorker(t *testing.T) { ns, pool, pod := "ns-rdw", "pool1", "worker-rdw" atespace, actorID := "team-rdw", "actor-rdw" - if _, err := persistence.CreateActor(ctx, &ateapipb.Actor{ + createdActor, err := persistence.CreateActor(ctx, &ateapipb.Actor{ Metadata: &ateapipb.ResourceMetadata{Name: actorID, Atespace: atespace}, ActorTemplateNamespace: ns, ActorTemplateName: "tmpl", Status: ateapipb.Actor_STATUS_RUNNING, WorkerAssignment: &ateapipb.WorkerAssignment{ WorkerNamespace: ns, WorkerPool: pool, WorkerPod: pod, WorkerPodIp: "10.0.0.5", }, - }); err != nil { + }) + if err != nil { t.Fatalf("create actor: %v", err) } if err := persistence.CreateWorker(ctx, &ateapipb.Worker{ @@ -575,7 +575,8 @@ func TestReconcileDeadWorker(t *testing.T) { State: ateapipb.Worker_STATE_DRAINING, Assignment: &ateapipb.Assignment{ ActorTemplate: &ateapipb.KubeNamespacedObjectRef{Namespace: ns, Name: "tmpl"}, - Actor: &ateapipb.ObjectRef{Name: actorID, Atespace: atespace}, + Actor: &ateapipb.ObjectRef{Atespace: createdActor.GetMetadata().GetAtespace(), Name: createdActor.GetMetadata().GetName()}, + ActorUid: createdActor.GetMetadata().GetUid(), }, }); err != nil { t.Fatalf("create worker: %v", err) @@ -596,6 +597,57 @@ func TestReconcileDeadWorker(t *testing.T) { } } +func TestReconcileDeadWorker_IgnoresStaleIncarnationAssignment(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + persistence, cleanup := storetest.SetupTestStore(t) + defer cleanup() + + s := &WorkerPoolSyncer{persistence: persistence} + + ns, pool, pod := "ns-rdw", "pool1", "worker-rdw" + atespace, actorID := "team-rdw", "actor-rdw" + createdActor, err := persistence.CreateActor(ctx, &ateapipb.Actor{ + Metadata: &ateapipb.ResourceMetadata{Name: actorID, Atespace: atespace}, ActorTemplateNamespace: ns, ActorTemplateName: "tmpl", + Status: ateapipb.Actor_STATUS_RUNNING, + WorkerAssignment: &ateapipb.WorkerAssignment{ + WorkerNamespace: ns, WorkerPool: pool, WorkerPod: pod, WorkerPodIp: "10.0.0.5", + }, + }) + if err != nil { + t.Fatalf("create actor: %v", err) + } + if err := persistence.CreateWorker(ctx, &ateapipb.Worker{ + WorkerNamespace: ns, WorkerPool: pool, WorkerPod: pod, + WorkerPodUid: "uid-rdw", + State: ateapipb.Worker_STATE_DRAINING, + Assignment: &ateapipb.Assignment{ + ActorTemplate: &ateapipb.KubeNamespacedObjectRef{Namespace: ns, Name: "tmpl"}, + Actor: &ateapipb.ObjectRef{Atespace: createdActor.GetMetadata().GetAtespace(), Name: createdActor.GetMetadata().GetName()}, + ActorUid: "old-incarnation-uid", + }, + }); err != nil { + t.Fatalf("create worker: %v", err) + } + + if err := s.reconcileDeadWorker(ctx, ns, pool, pod); err != nil { + t.Fatalf("reconcileDeadWorker = %v, want nil", err) + } + // The dead worker should be deleted. + if _, err := persistence.GetWorker(ctx, ns, pool, pod); !errors.Is(err, store.ErrNotFound) { + t.Errorf("worker not deleted: err=%v", err) + } + // Because ActorUid did not match, the new actor must remain RUNNING. + got, err := persistence.GetActor(ctx, resources.ActorRef{Name: actorID, Atespace: atespace}) + if err != nil { + t.Fatalf("get actor: %v", err) + } + if got.GetStatus() != ateapipb.Actor_STATUS_RUNNING { + t.Errorf("actor status = %v, want RUNNING (should ignore dead worker assigned to stale incarnation)", got.GetStatus()) + } +} + // TestSyncer_ReconcileOrphanedWorkers verifies the startup reconcile: a stored // worker whose pod no longer exists is cleaned up (and its actor released), while // a worker whose pod is still live is preserved. This is the durability backstop @@ -640,13 +692,14 @@ func TestSyncer_ReconcileOrphanedWorkers(t *testing.T) { // An orphan worker (no pod) whose actor is still RUNNING must be cleaned up. atespace, actorID := "team-recon", "actor-recon" - if _, err := persistence.CreateActor(ctx, &ateapipb.Actor{ + createdActor, err := persistence.CreateActor(ctx, &ateapipb.Actor{ Metadata: &ateapipb.ResourceMetadata{Name: actorID, Atespace: atespace}, ActorTemplateNamespace: ns, ActorTemplateName: "tmpl", Status: ateapipb.Actor_STATUS_RUNNING, WorkerAssignment: &ateapipb.WorkerAssignment{ WorkerNamespace: ns, WorkerPool: pool, WorkerPod: "worker-orphan", WorkerPodIp: "10.0.0.10", }, - }); err != nil { + }) + if err != nil { t.Fatalf("create actor: %v", err) } if err := persistence.CreateWorker(ctx, &ateapipb.Worker{ @@ -655,7 +708,8 @@ func TestSyncer_ReconcileOrphanedWorkers(t *testing.T) { State: ateapipb.Worker_STATE_DRAINING, Assignment: &ateapipb.Assignment{ ActorTemplate: &ateapipb.KubeNamespacedObjectRef{Namespace: ns, Name: "tmpl"}, - Actor: &ateapipb.ObjectRef{Name: actorID, Atespace: atespace}, + Actor: &ateapipb.ObjectRef{Atespace: createdActor.GetMetadata().GetAtespace(), Name: createdActor.GetMetadata().GetName()}, + ActorUid: createdActor.GetMetadata().GetUid(), }, }); err != nil { t.Fatalf("create orphan worker: %v", err) @@ -719,7 +773,7 @@ func TestReleaseActorOnDeadWorker_StatusTransitions(t *testing.T) { s := &WorkerPoolSyncer{persistence: persistence} atespace, actorID := "team-status", "actor-status" - if _, err := persistence.CreateActor(ctx, &ateapipb.Actor{ + createdActor, err := persistence.CreateActor(ctx, &ateapipb.Actor{ Metadata: &ateapipb.ResourceMetadata{Name: actorID, Atespace: atespace}, ActorTemplateNamespace: ns, ActorTemplateName: "tmpl", @@ -727,7 +781,8 @@ func TestReleaseActorOnDeadWorker_StatusTransitions(t *testing.T) { WorkerAssignment: &ateapipb.WorkerAssignment{ WorkerNamespace: ns, WorkerPool: pool, WorkerPod: pod, WorkerPodIp: ip, WorkerPodUid: "uid", }, - }); err != nil { + }) + if err != nil { t.Fatalf("create actor: %v", err) } if err := persistence.CreateWorker(ctx, &ateapipb.Worker{ @@ -737,7 +792,8 @@ func TestReleaseActorOnDeadWorker_StatusTransitions(t *testing.T) { State: ateapipb.Worker_STATE_ACTIVE, Assignment: &ateapipb.Assignment{ ActorTemplate: &ateapipb.KubeNamespacedObjectRef{Namespace: ns, Name: "tmpl"}, - Actor: &ateapipb.ObjectRef{Name: actorID, Atespace: atespace}, + Actor: &ateapipb.ObjectRef{Atespace: createdActor.GetMetadata().GetAtespace(), Name: createdActor.GetMetadata().GetName()}, + ActorUid: createdActor.GetMetadata().GetUid(), }, }); err != nil { t.Fatalf("create worker: %v", err) diff --git a/cmd/ateapi/internal/controlapi/workflow_pause.go b/cmd/ateapi/internal/controlapi/workflow_pause.go index 356caa7e21..a29e65caed 100644 --- a/cmd/ateapi/internal/controlapi/workflow_pause.go +++ b/cmd/ateapi/internal/controlapi/workflow_pause.go @@ -233,12 +233,11 @@ func (s *FinalizePausedStep) Execute(ctx context.Context, input *PauseInput, sta } slog.Warn("Worker already gone during finalize pause, skipping release", "worker", assignment.GetWorkerPod()) } else { - // TODO(dberkov) - what if worker does not belong to this actor? nodeName = worker.GetNodeName() // Only free it if it still belongs to us if wass := worker.Assignment; wass != nil { - if resources.ActorRefFromObjectRef(wass.Actor) == input.ActorRef { + if wass.GetActorUid() == latestActor.GetMetadata().GetUid() { worker.Assignment = nil err = s.store.UpdateWorker(ctx, worker, worker.Version) if err != nil { diff --git a/cmd/ateapi/internal/controlapi/workflow_resume.go b/cmd/ateapi/internal/controlapi/workflow_resume.go index ef4004c62c..c3ec2b14b0 100644 --- a/cmd/ateapi/internal/controlapi/workflow_resume.go +++ b/cmd/ateapi/internal/controlapi/workflow_resume.go @@ -319,7 +319,7 @@ func (s *AssignWorkerStep) Execute(ctx context.Context, input *ResumeInput, stat if worker.Assignment == nil { continue } - if resources.ActorRefFromObjectRef(worker.Assignment.Actor) != input.ActorRef { + if worker.Assignment.GetActorUid() != state.Actor.GetMetadata().GetUid() { continue } if s.scheduler.Applies(worker, constraints) { @@ -366,7 +366,11 @@ func (s *AssignWorkerStep) Execute(ctx context.Context, input *ResumeInput, stat Namespace: state.Actor.GetActorTemplateNamespace(), Name: state.Actor.GetActorTemplateName(), }, - Actor: input.ActorRef.ToObjectRef(), + Actor: &ateapipb.ObjectRef{ + Atespace: state.Actor.GetMetadata().GetAtespace(), + Name: state.Actor.GetMetadata().GetName(), + }, + ActorUid: state.Actor.GetMetadata().GetUid(), } if err := s.store.UpdateWorker(ctx, assignedWorker, assignedWorker.Version); err != nil { @@ -504,8 +508,8 @@ func (s *CallAteletRestoreStep) CheckPrerequisite(ctx context.Context, input *Re return status.Errorf(codes.FailedPrecondition, "Assigned worker is nil") } // Verify if the worker is still assigned to the same Actor. - assigned := state.Worker.GetAssignment().GetActor() - if resources.ActorRefFromObjectRef(assigned) != input.ActorRef { + assignedActorUID := state.Worker.GetAssignment().GetActorUid() + if assignedActorUID != state.Actor.GetMetadata().GetUid() { slog.ErrorContext(ctx, "crashing actor because its assigned worker no longer belongs to it", slog.String("worker", state.Worker.GetWorkerPod()), slog.Any("assignment", state.Worker.GetAssignment())) diff --git a/cmd/ateapi/internal/controlapi/workflow_resume_test.go b/cmd/ateapi/internal/controlapi/workflow_resume_test.go index c4c2d26d7a..f97f3fd16b 100644 --- a/cmd/ateapi/internal/controlapi/workflow_resume_test.go +++ b/cmd/ateapi/internal/controlapi/workflow_resume_test.go @@ -74,7 +74,8 @@ func TestAssignWorkerStep_SkipsWorkerAssignedInOtherAtespace(t *testing.T) { SandboxClass: "gvisor", State: ateapipb.Worker_STATE_ACTIVE, Assignment: &ateapipb.Assignment{ - Actor: &ateapipb.ObjectRef{Atespace: "team-b", Name: "shared"}, + Actor: &ateapipb.ObjectRef{Atespace: "team-b", Name: "shared"}, + ActorUid: "team-b-actor-uid", }, } if err := persistence.CreateWorker(ctx, worker); err != nil { @@ -91,7 +92,7 @@ func TestAssignWorkerStep_SkipsWorkerAssignedInOtherAtespace(t *testing.T) { step := &AssignWorkerStep{store: persistence, workerCache: wc, scheduler: scheduling.New(wc)} state := &ResumeState{ Actor: &ateapipb.Actor{ - Metadata: &ateapipb.ResourceMetadata{Atespace: "team-a", Name: "shared"}, + Metadata: &ateapipb.ResourceMetadata{Atespace: "team-a", Name: "shared", Uid: "actor-uid"}, }, ActorTemplate: &atev1alpha1.ActorTemplate{ Spec: atev1alpha1.ActorTemplateSpec{SandboxClass: atev1alpha1.SandboxClassGvisor}, @@ -106,6 +107,9 @@ func TestAssignWorkerStep_SkipsWorkerAssignedInOtherAtespace(t *testing.T) { if err != nil { t.Fatalf("GetWorker: %v", err) } + if got := stored.GetAssignment().GetActorUid(); got != "team-b-actor-uid" { + t.Errorf("worker assignment uid = %q, want %q (assignment: %v)", got, "team-b-actor-uid", stored.GetAssignment()) + } if got := stored.GetAssignment().GetActor().GetAtespace(); got != "team-b" { t.Errorf("worker assignment atespace = %q, want %q (assignment: %v)", got, "team-b", stored.GetAssignment()) } @@ -119,6 +123,14 @@ func TestAssignWorkerStep_ReleasesIneligibleStaleWorkerInBackground(t *testing.T ctx := context.Background() persistence := newTestPersistence(t) + actor, err := persistence.CreateActor(ctx, &ateapipb.Actor{ + Metadata: &ateapipb.ResourceMetadata{Atespace: "team-a", Name: "id1"}, + Status: ateapipb.Actor_STATUS_SUSPENDED, + }) + if err != nil { + t.Fatalf("CreateActor: %v", err) + } + // stale-pod is claimed by this actor from a failed attempt but its sandbox // class no longer matches the template; free-pod is eligible and free. stale := &ateapipb.Worker{ @@ -128,7 +140,8 @@ func TestAssignWorkerStep_ReleasesIneligibleStaleWorkerInBackground(t *testing.T SandboxClass: "microvm", State: ateapipb.Worker_STATE_ACTIVE, Assignment: &ateapipb.Assignment{ - Actor: &ateapipb.ObjectRef{Atespace: "team-a", Name: "id1"}, + Actor: &ateapipb.ObjectRef{Atespace: "team-a", Name: "id1"}, + ActorUid: actor.GetMetadata().GetUid(), }, } free := &ateapipb.Worker{ @@ -144,14 +157,6 @@ func TestAssignWorkerStep_ReleasesIneligibleStaleWorkerInBackground(t *testing.T } } - actor, err := persistence.CreateActor(ctx, &ateapipb.Actor{ - Metadata: &ateapipb.ResourceMetadata{Atespace: "team-a", Name: "id1"}, - Status: ateapipb.Actor_STATUS_SUSPENDED, - }) - if err != nil { - t.Fatalf("CreateActor: %v", err) - } - cacheCtx, cancel := context.WithCancel(ctx) defer cancel() wc := workercache.New(persistence, time.Minute) @@ -231,7 +236,8 @@ func TestAssignWorkerStep_RetryAfterConflictPicksFreshWorker(t *testing.T) { // its stored version past the failed attempt's snapshot. claimed := proto.Clone(beforeClaim).(*ateapipb.Worker) claimed.Assignment = &ateapipb.Assignment{ - Actor: &ateapipb.ObjectRef{Atespace: "team-a", Name: "other"}, + Actor: &ateapipb.ObjectRef{Atespace: "team-a", Name: "other"}, + ActorUid: "other-actor-uid", } if err := persistence.UpdateWorker(ctx, claimed, claimed.GetVersion()); err != nil { t.Fatalf("UpdateWorker (concurrent claim): %v", err) @@ -256,7 +262,8 @@ func TestAssignWorkerStep_RetryAfterConflictPicksFreshWorker(t *testing.T) { // contested worker mutated with our assignment, at the pre-claim version. stale := proto.Clone(beforeClaim).(*ateapipb.Worker) stale.Assignment = &ateapipb.Assignment{ - Actor: &ateapipb.ObjectRef{Atespace: "team-a", Name: "id1"}, + Actor: &ateapipb.ObjectRef{Atespace: "team-a", Name: "id1"}, + ActorUid: actor.GetMetadata().GetUid(), } step := &AssignWorkerStep{store: persistence, workerCache: wc, scheduler: scheduling.New(wc)} state := &ResumeState{ @@ -277,15 +284,15 @@ func TestAssignWorkerStep_RetryAfterConflictPicksFreshWorker(t *testing.T) { if err != nil { t.Fatalf("GetWorker(contested-pod): %v", err) } - if got := storedContested.GetAssignment().GetActor().GetName(); got != "other" { - t.Errorf("contested worker assignment = %v, want to remain with actor %q", storedContested.GetAssignment(), "other") + if got := storedContested.GetAssignment().GetActorUid(); got != "other-actor-uid" { + t.Errorf("contested worker assignment = %v, want to remain with actor %q", storedContested.GetAssignment(), "other-actor-uid") } storedFallback, err := persistence.GetWorker(ctx, "worker-ns", "pool", "fallback-pod") if err != nil { t.Fatalf("GetWorker(fallback-pod): %v", err) } - if got := storedFallback.GetAssignment().GetActor().GetName(); got != "id1" { - t.Errorf("fallback worker assignment = %v, want actor %q", storedFallback.GetAssignment(), "id1") + if got := storedFallback.GetAssignment().GetActorUid(); got != actor.GetMetadata().GetUid() { + t.Errorf("fallback worker assignment = %v, want actor uid %q", storedFallback.GetAssignment(), actor.GetMetadata().GetUid()) } storedActor, err := persistence.GetActor(ctx, resources.ActorRef{Atespace: "team-a", Name: "id1"}) @@ -566,11 +573,11 @@ func TestResumeSteps_CheckPrerequisite(t *testing.T) { // CallAteletRestoreStep's worker checks pass; this test only // verifies status gating. state := &ResumeState{ - Actor: &ateapipb.Actor{Status: st}, + Actor: &ateapipb.Actor{Status: st, Metadata: &ateapipb.ResourceMetadata{Name: "id1", Uid: "actor-uid-1"}}, Worker: &ateapipb.Worker{ SandboxClass: string(atev1alpha1.SandboxClassGvisor), State: ateapipb.Worker_STATE_ACTIVE, - Assignment: &ateapipb.Assignment{Actor: &ateapipb.ObjectRef{Name: "id1"}}, + Assignment: &ateapipb.Assignment{Actor: &ateapipb.ObjectRef{Atespace: "team-a", Name: "id1"}, ActorUid: "actor-uid-1"}, }, ActorTemplate: &atev1alpha1.ActorTemplate{Spec: atev1alpha1.ActorTemplateSpec{SandboxClass: atev1alpha1.SandboxClassGvisor}}, } @@ -664,10 +671,16 @@ func TestResumeActor_CrashesOnMissingWorkerAssignment(t *testing.T) { // which is not ours — must not be written. func TestCallAteletRestoreStep_CheckPrerequisite_WorkerOwnership(t *testing.T) { ownAssignment := &ateapipb.Assignment{ - Actor: &ateapipb.ObjectRef{Atespace: "team-a", Name: "shared"}, + Actor: &ateapipb.ObjectRef{Atespace: "team-a", Name: "shared"}, + ActorUid: "own-actor-uid", } otherAssignment := &ateapipb.Assignment{ - Actor: &ateapipb.ObjectRef{Atespace: "team-b", Name: "shared"}, + Actor: &ateapipb.ObjectRef{Atespace: "team-b", Name: "shared"}, + ActorUid: "other-actor-uid", + } + staleIncarnationAssignment := &ateapipb.Assignment{ + Actor: &ateapipb.ObjectRef{Atespace: "team-a", Name: "shared"}, + ActorUid: "stale-incarnation-uid", } tests := []struct { @@ -691,6 +704,14 @@ func TestCallAteletRestoreStep_CheckPrerequisite_WorkerOwnership(t *testing.T) { wantActorStatus: ateapipb.Actor_STATUS_CRASHED, wantAssignment: otherAssignment, }, + { + name: "crashes actor and leaves worker untouched when assigned to previous incarnation of same actor", + sandboxClass: "gvisor", + assignment: staleIncarnationAssignment, + wantCode: codes.Aborted, + wantActorStatus: ateapipb.Actor_STATUS_CRASHED, + wantAssignment: staleIncarnationAssignment, + }, { name: "crashes actor and leaves worker untouched when assignment is cleared", sandboxClass: "gvisor", @@ -745,7 +766,7 @@ func TestCallAteletRestoreStep_CheckPrerequisite_WorkerOwnership(t *testing.T) { step := &CallAteletRestoreStep{store: persistence, scheduler: scheduling.New(nil)} state := &ResumeState{ Actor: &ateapipb.Actor{ - Metadata: &ateapipb.ResourceMetadata{Atespace: "team-a", Name: "shared"}, + Metadata: &ateapipb.ResourceMetadata{Atespace: "team-a", Name: "shared", Uid: "own-actor-uid"}, Status: ateapipb.Actor_STATUS_RESUMING, }, Worker: seeded, diff --git a/cmd/ateapi/internal/controlapi/workflow_suspend.go b/cmd/ateapi/internal/controlapi/workflow_suspend.go index 8722a0bcca..cf63b15c7f 100644 --- a/cmd/ateapi/internal/controlapi/workflow_suspend.go +++ b/cmd/ateapi/internal/controlapi/workflow_suspend.go @@ -247,7 +247,7 @@ func (s *FinalizeSuspendedStep) Execute(ctx context.Context, input *SuspendInput } else { // Only free it if it still belongs to us if wass := worker.Assignment; wass != nil { - if resources.ActorRefFromObjectRef(wass.Actor) == input.ActorRef { + if wass.GetActorUid() == latestActor.GetMetadata().GetUid() { worker.Assignment = nil err = s.store.UpdateWorker(ctx, worker, worker.Version) if err != nil { diff --git a/cmd/ateapi/internal/controlapi/workflow_suspend_test.go b/cmd/ateapi/internal/controlapi/workflow_suspend_test.go index e035606f6e..cb64b887a8 100644 --- a/cmd/ateapi/internal/controlapi/workflow_suspend_test.go +++ b/cmd/ateapi/internal/controlapi/workflow_suspend_test.go @@ -298,6 +298,7 @@ func TestFinalizeSuspendedStep_ReleasesOnlyOwnWorker(t *testing.T) { tests := []struct { name string assignmentAtespace string + mismatchedUID bool wantReleased bool }{ { @@ -310,6 +311,12 @@ func TestFinalizeSuspendedStep_ReleasesOnlyOwnWorker(t *testing.T) { assignmentAtespace: "team-b", wantReleased: false, }, + { + name: "keeps worker assigned to previous incarnation of same actor", + assignmentAtespace: "team-a", + mismatchedUID: true, + wantReleased: false, + }, } for _, tt := range tests { @@ -317,18 +324,6 @@ func TestFinalizeSuspendedStep_ReleasesOnlyOwnWorker(t *testing.T) { ctx := context.Background() persistence := newTestPersistence(t) - worker := &ateapipb.Worker{ - WorkerNamespace: "worker-ns", - WorkerPool: "pool", - WorkerPod: "pod-1", - Assignment: &ateapipb.Assignment{ - Actor: &ateapipb.ObjectRef{Atespace: tt.assignmentAtespace, Name: "shared"}, - }, - } - if err := persistence.CreateWorker(ctx, worker); err != nil { - t.Fatalf("CreateWorker: %v", err) - } - actor := &ateapipb.Actor{ Metadata: &ateapipb.ResourceMetadata{Atespace: "team-a", Name: "shared"}, Status: ateapipb.Actor_STATUS_SUSPENDING, @@ -339,10 +334,28 @@ func TestFinalizeSuspendedStep_ReleasesOnlyOwnWorker(t *testing.T) { }, InProgressSnapshot: "snapshot-1", } - if _, err := persistence.CreateActor(ctx, actor); err != nil { + created, err := persistence.CreateActor(ctx, actor) + if err != nil { t.Fatalf("CreateActor: %v", err) } + uid := created.GetMetadata().GetUid() + if tt.assignmentAtespace != "team-a" || tt.mismatchedUID { + uid = "other-actor-uid-b" + } + worker := &ateapipb.Worker{ + WorkerNamespace: "worker-ns", + WorkerPool: "pool", + WorkerPod: "pod-1", + Assignment: &ateapipb.Assignment{ + Actor: &ateapipb.ObjectRef{Atespace: tt.assignmentAtespace, Name: "shared"}, + ActorUid: uid, + }, + } + if err := persistence.CreateWorker(ctx, worker); err != nil { + t.Fatalf("CreateWorker: %v", err) + } + step := &FinalizeSuspendedStep{store: persistence} input := &SuspendInput{ActorRef: resources.ActorRef{Atespace: "team-a", Name: "shared"}} state := &SuspendState{ActorTemplate: &atev1alpha1.ActorTemplate{Spec: atev1alpha1.ActorTemplateSpec{SnapshotsConfig: atev1alpha1.SnapshotsConfig{Location: "gs://snapshots"}}}} diff --git a/cmd/ateapi/internal/scheduling/scheduling_test.go b/cmd/ateapi/internal/scheduling/scheduling_test.go index 52062e47c5..6f306f17d0 100644 --- a/cmd/ateapi/internal/scheduling/scheduling_test.go +++ b/cmd/ateapi/internal/scheduling/scheduling_test.go @@ -260,7 +260,8 @@ func withState(state ateapipb.Worker_State) func(*ateapipb.Worker) { func assigned(atespace, name string) func(*ateapipb.Worker) { return func(w *ateapipb.Worker) { w.Assignment = &ateapipb.Assignment{ - Actor: &ateapipb.ObjectRef{Atespace: atespace, Name: name}, + Actor: &ateapipb.ObjectRef{Atespace: atespace, Name: name}, + ActorUid: atespace + "/" + name, } } } diff --git a/cmd/ateapi/internal/store/ateredis/ateredis_test.go b/cmd/ateapi/internal/store/ateredis/ateredis_test.go index a1626ad67f..a187e3c5d5 100644 --- a/cmd/ateapi/internal/store/ateredis/ateredis_test.go +++ b/cmd/ateapi/internal/store/ateredis/ateredis_test.go @@ -341,6 +341,7 @@ func TestUpdateWorker_Success(t *testing.T) { Actor: &ateapipb.ObjectRef{ Name: "actor-1", }, + ActorUid: "actor-1-uid", } if err := s.UpdateWorker(ctx, worker, 1); err != nil { @@ -691,14 +692,20 @@ func TestUpdateWorker_Conflict(t *testing.T) { } // Update instance 1 - worker1.Assignment = &ateapipb.Assignment{Actor: &ateapipb.ObjectRef{Name: "actor-1"}} + worker1.Assignment = &ateapipb.Assignment{ + Actor: &ateapipb.ObjectRef{Atespace: "team-a", Name: "actor-1"}, + ActorUid: "actor-1-uid", + } err = s.UpdateWorker(ctx, worker1, worker1.Version) if err != nil { t.Fatalf("UpdateWorker failed: %v", err) } // Try to update instance 2 - worker2.Assignment = &ateapipb.Assignment{Actor: &ateapipb.ObjectRef{Name: "actor-2"}} + worker2.Assignment = &ateapipb.Assignment{ + Actor: &ateapipb.ObjectRef{Atespace: "team-a", Name: "actor-2"}, + ActorUid: "actor-2-uid", + } err = s.UpdateWorker(ctx, worker2, worker2.Version) if !errors.Is(err, store.ErrVersionConflict) { t.Errorf("expected ErrVersionConflict, got %v", err) diff --git a/cmd/ateapi/internal/workercache/workercache_test.go b/cmd/ateapi/internal/workercache/workercache_test.go index cda1b8aea0..b47008e81f 100644 --- a/cmd/ateapi/internal/workercache/workercache_test.go +++ b/cmd/ateapi/internal/workercache/workercache_test.go @@ -92,7 +92,10 @@ func TestCache_UpdatedEvent_NewerVersionApplied(t *testing.T) { } updated := makeWorker("ns", "pod1", 2) - updated.Assignment = &ateapipb.Assignment{Actor: &ateapipb.ObjectRef{Name: "actor-1"}} + updated.Assignment = &ateapipb.Assignment{ + Actor: &ateapipb.ObjectRef{Atespace: "team-a", Name: "actor-1"}, + ActorUid: "actor-1-uid", + } fs.send(store.WorkerEvent{Type: store.WorkerEventUpdated, Worker: updated}) eventually(t, func() bool { @@ -101,7 +104,7 @@ func TestCache_UpdatedEvent_NewerVersionApplied(t *testing.T) { return false } wass := workers[0].Assignment - return wass.Actor.Name == "actor-1" + return wass.Actor.Name == "actor-1" && wass.ActorUid == "actor-1-uid" }, 2*time.Second) got, _ := c.Workers() @@ -122,7 +125,10 @@ func TestCache_UpdatedEvent_OlderVersionIgnored(t *testing.T) { // Send a stale update followed by a sentinel we can detect. stale := makeWorker("ns", "pod1", 3) - stale.Assignment = &ateapipb.Assignment{Actor: &ateapipb.ObjectRef{Name: "stale-actor"}} + stale.Assignment = &ateapipb.Assignment{ + Actor: &ateapipb.ObjectRef{Atespace: "team-a", Name: "stale-actor"}, + ActorUid: "stale-actor-uid", + } fs.send(store.WorkerEvent{Type: store.WorkerEventUpdated, Worker: stale}) sentinel := makeWorker("ns", "pod2", 1) diff --git a/internal/resources/validate.go b/internal/resources/validate.go index 09c985df71..54582a9fef 100644 --- a/internal/resources/validate.go +++ b/internal/resources/validate.go @@ -263,17 +263,11 @@ func ValidateAssignment(assignment *ateapipb.Assignment, fldPath *field.Path) fi if val, fldPath := assignment.Actor, fldPath.Child("actor"); val == nil { errs = append(errs, field.Required(fldPath, "")) } else { - if val, fldPath := assignment.Actor.Name, fldPath.Child("name"); val == "" { - errs = append(errs, field.Required(fldPath, "")) - } else { - errs = append(errs, ValidateResourceName(val, fldPath)...) - } + errs = append(errs, ValidateObjectRef(val, fldPath)...) + } - if val, fldPath := assignment.Actor.Atespace, fldPath.Child("atespace"); val == "" { - errs = append(errs, field.Required(fldPath, "")) - } else { - errs = append(errs, ValidateResourceName(val, fldPath)...) - } + if val, fldPath := assignment.ActorUid, fldPath.Child("actor_uid"); val == "" { + errs = append(errs, field.Required(fldPath, "")) } return errs diff --git a/internal/resources/validate_test.go b/internal/resources/validate_test.go index b25de1d55c..57be7a74f5 100644 --- a/internal/resources/validate_test.go +++ b/internal/resources/validate_test.go @@ -266,9 +266,10 @@ func TestValidateWorker(t *testing.T) { Name: "actor-template", }, Actor: &ateapipb.ObjectRef{ - Name: "actor-id", - Atespace: "actor-atespace", + Atespace: "actor-ns", + Name: "actor", }, + ActorUid: "actor-uid", }, Ip: "10.0.0.1", WorkerPodUid: "123e4567-e89b-12d3-a456-426614174000", @@ -283,9 +284,10 @@ func TestValidateWorker(t *testing.T) { WorkerPod: "pod-1", Assignment: &ateapipb.Assignment{ Actor: &ateapipb.ObjectRef{ - Name: "actor-id", - Atespace: "actor-atespace", + Atespace: "actor-ns", + Name: "actor", }, + ActorUid: "actor-uid", }, Ip: "10.0.0.1", WorkerPodUid: "123e4567-e89b-12d3-a456-426614174000", @@ -303,9 +305,10 @@ func TestValidateWorker(t *testing.T) { Name: "actor-template", }, Actor: &ateapipb.ObjectRef{ - Name: "actor-id", - Atespace: "actor-atespace", + Atespace: "actor-ns", + Name: "actor", }, + ActorUid: "actor-uid", }, Ip: "10.0.0.1", WorkerPodUid: "123e4567-e89b-12d3-a456-426614174000", @@ -323,9 +326,10 @@ func TestValidateWorker(t *testing.T) { Namespace: "actor-ns", }, Actor: &ateapipb.ObjectRef{ - Name: "actor-id", - Atespace: "actor-atespace", + Atespace: "actor-ns", + Name: "actor", }, + ActorUid: "actor-uid", }, Ip: "10.0.0.1", WorkerPodUid: "123e4567-e89b-12d3-a456-426614174000", @@ -343,6 +347,7 @@ func TestValidateWorker(t *testing.T) { Name: "actor-template", Namespace: "actor-ns", }, + ActorUid: "actor-uid", }, Ip: "10.0.0.1", WorkerPodUid: "123e4567-e89b-12d3-a456-426614174000", @@ -361,8 +366,9 @@ func TestValidateWorker(t *testing.T) { Namespace: "actor-ns", }, Actor: &ateapipb.ObjectRef{ - Atespace: "actor-atespace", + Atespace: "actor-ns", }, + ActorUid: "actor-uid", }, Ip: "10.0.0.1", WorkerPodUid: "123e4567-e89b-12d3-a456-426614174000", @@ -381,14 +387,36 @@ func TestValidateWorker(t *testing.T) { Namespace: "actor-ns", }, Actor: &ateapipb.ObjectRef{ - Name: "actor-id", + Name: "actor", }, + ActorUid: "actor-uid", }, Ip: "10.0.0.1", WorkerPodUid: "123e4567-e89b-12d3-a456-426614174000", NodeName: "node-1.example.com", }, wantMsg: "worker.assignment.actor.atespace: Required value", + }, { + name: "partially assigned worker, missing actor_uid", + worker: &ateapipb.Worker{ + WorkerNamespace: "ns-1", + WorkerPool: "pool-1", + WorkerPod: "pod-1", + Assignment: &ateapipb.Assignment{ + ActorTemplate: &ateapipb.KubeNamespacedObjectRef{ + Name: "actor-template", + Namespace: "actor-ns", + }, + Actor: &ateapipb.ObjectRef{ + Atespace: "actor-ns", + Name: "actor", + }, + }, + Ip: "10.0.0.1", + WorkerPodUid: "123e4567-e89b-12d3-a456-426614174000", + NodeName: "node-1.example.com", + }, + wantMsg: "worker.assignment.actor_uid: Required value", }, { name: "missing worker_namespace", worker: &ateapipb.Worker{ diff --git a/pkg/proto/ateapipb/ateapi.pb.go b/pkg/proto/ateapipb/ateapi.pb.go index fdae6906fc..83ad5bfb50 100644 --- a/pkg/proto/ateapipb/ateapi.pb.go +++ b/pkg/proto/ateapipb/ateapi.pb.go @@ -2543,6 +2543,7 @@ type Assignment struct { state protoimpl.MessageState `protogen:"open.v1"` ActorTemplate *KubeNamespacedObjectRef `protobuf:"bytes,1,opt,name=actor_template,json=actorTemplate,proto3" json:"actor_template,omitempty"` Actor *ObjectRef `protobuf:"bytes,2,opt,name=actor,proto3" json:"actor,omitempty"` + ActorUid string `protobuf:"bytes,3,opt,name=actor_uid,json=actorUid,proto3" json:"actor_uid,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache } @@ -2591,6 +2592,13 @@ func (x *Assignment) GetActor() *ObjectRef { return nil } +func (x *Assignment) GetActorUid() string { + if x != nil { + return x.ActorUid + } + return "" +} + type KubeNamespacedObjectRef struct { state protoimpl.MessageState `protogen:"open.v1"` Namespace string `protobuf:"bytes,1,opt,name=namespace,proto3" json:"namespace,omitempty"` @@ -3147,11 +3155,12 @@ const file_ateapi_proto_rawDesc = "" + "\x05State\x12\x15\n" + "\x11STATE_UNSPECIFIED\x10\x00\x12\x10\n" + "\fSTATE_ACTIVE\x10\x01\x12\x12\n" + - "\x0eSTATE_DRAINING\x10\x02\"}\n" + + "\x0eSTATE_DRAINING\x10\x02\"\x9a\x01\n" + "\n" + "Assignment\x12F\n" + "\x0eactor_template\x18\x01 \x01(\v2\x1f.ateapi.KubeNamespacedObjectRefR\ractorTemplate\x12'\n" + - "\x05actor\x18\x02 \x01(\v2\x11.ateapi.ObjectRefR\x05actor\"K\n" + + "\x05actor\x18\x02 \x01(\v2\x11.ateapi.ObjectRefR\x05actor\x12\x1b\n" + + "\tactor_uid\x18\x03 \x01(\tR\bactorUid\"K\n" + "\x17KubeNamespacedObjectRef\x12\x1c\n" + "\tnamespace\x18\x01 \x01(\tR\tnamespace\x12\x12\n" + "\x04name\x18\x02 \x01(\tR\x04name\"\x13\n" + diff --git a/pkg/proto/ateapipb/ateapi.proto b/pkg/proto/ateapipb/ateapi.proto index bc576cfc93..5281c0b1b4 100644 --- a/pkg/proto/ateapipb/ateapi.proto +++ b/pkg/proto/ateapipb/ateapi.proto @@ -461,6 +461,7 @@ message Worker { message Assignment { KubeNamespacedObjectRef actor_template = 1; ObjectRef actor = 2; + string actor_uid = 3; } message KubeNamespacedObjectRef {