ateapi: UpdateActor allows updating ActorTemplate. - #1365
Conversation
f2be2cd to
5ff83e5
Compare
700b67f to
7ced6b9
Compare
fa79816 to
8f0949c
Compare
Make Actor.actor_template mutable so a suspended Actor can be updated to reference a different ActorTemplate, taking effect on the next resume. UpdateActor resolves a repointed template ref up front, mirroring CreateActor's check. To restore safely across a template swap, a successful resume (including the first boot) records the template (and its UID) the sprint booted with in ActorStatus.current_actor_template(_uid) when it commits RUNNING. The next resume compares that UID against the currently resolved template: when they differ, the snapshot was taken on a different template, so its guest state cannot be layered onto the new template's golden snapshot. The restore wire scope is forced to SNAPSHOT_SCOPE_DATA in that case, and the golden URI is only sent when the scope actually is DATA_ON_GOLDEN. The same scope derivation applies in both the local- and durable-snapshot restore branches. Recording at sprint start rather than at suspend/pause finalization keeps the record correct however the sprint ends, so crashed finalizations need no special casing. The status field mirrors the spec/status split the way StatefulSet's currentRevision does: spec.actor_template is the desired template, status.current_actor_template is the one the guest state is actually built on.
ResumeActor now records the template it restored from in status.current_actor_template and status.current_actor_template_uid, so the resume functional test must expect them in the GetActor response.
The union discriminator was removed from ActorTemplate.volumes, but the repoint-template test fixtures added on this branch still set it, so the controlapi test binary no longer compiled.
gVisor full captures now archive durable-dir bind mounts into durable-dir.tar and atelet can narrow them to a data snapshot, so the data-only restore a repoint forces works from a FULL snapshot. Drop the UpdateActor validation that refused FULL-commit gVisor templates, and let the e2e onCommit:Full case exercise the full repoint lifecycle on gVisor instead of expecting FailedPrecondition.
8f0949c to
3800aab
Compare
| ActorTemplate: refB, | ||
| }}) | ||
| // gVisor cannot restore data-only from a FULL snapshot yet, so the API | ||
| // refuses to repoint an actor whose gVisor templates commit FULL scope. |
There was a problem hiding this comment.
This should work now, we need to untar the durable volumes (from the snapshot) first and then just cold start the sandbox.
There was a problem hiding this comment.
I think that was an older revision of the PR. The newest revision does allow cold start.
| if err != nil { | ||
| return err | ||
| } | ||
| oldTemplate, err := resolveActorTemplate(ctx, s.store, oldVal) |
There was a problem hiding this comment.
If oldTemplate cannot be resolved and the actor has no snapshot (oldVal.GetStatus().GetLatestSnapshot() == nil), should we bypass volume comparison?
There was a problem hiding this comment.
Done. Skipped validation when oldTemplate cannot be resolved , because the actor will not be able to Resume anywya.
| // volumes list must be identical, and containers present in both templates | ||
| // must keep identical mounts, order included; containers added or removed by | ||
| // the new template are unconstrained. | ||
| func validateTemplateVolumesUnchanged(oldTemplate, newTemplate *ateapipb.ActorTemplate) error { |
There was a problem hiding this comment.
We should check sandbox class of old template and new template here and fail if it is not same
An actor can outlive its ActorTemplate. When the stored template ref no longer resolves there is nothing to compare the replacement against, so repointing away from a deleted template was impossible. Treat a not-found old template as skipping the comparison; any other lookup error still fails the update.
Snapshots are not portable across sandbox runtime families, so a repointed actor's saved state could not be restored under a template of another class. Compare the old and new template's sandbox_config.sandbox_class during UpdateActor and reject mismatches with FailedPrecondition. Only the class must match; a different config_name within the same class is still allowed.
| // Update actor template is only allowed while the actor is suspended. | ||
| // The repointed ref must also resolve, mirroring CreateActor's | ||
| // check (same non-atomicity caveat; resume re-resolves and fails | ||
| // cleanly), and the replacement's sandbox class, volumes, and volume |
There was a problem hiding this comment.
I was discussing the statement (volume must be match) with Michelle Au (@msau42) in July , and as I remember, we was thinking allowing adding new volumes, but disallow remove volumes.
Michelle Au (@msau42) - do you remember what you planned to implement?
There was a problem hiding this comment.
My understanding is volumes are immutable for MVP, and we'll explore adding new volumes post MVP.
Let me know if I missed something.
| // must keep identical mounts, order included; containers added or removed by | ||
| // the new template are unconstrained. | ||
| func validateTemplateVolumesUnchanged(oldTemplate, newTemplate *ateapipb.ActorTemplate) error { | ||
| if !slices.EqualFunc(oldTemplate.GetVolumes(), newTemplate.GetVolumes(), func(a, b *ateapipb.Volume) bool { |
There was a problem hiding this comment.
slices.EqualFunc assumes the order must be the same, I think your intention was to check the content and ignore the order.
| var goldenSnapshotURI string | ||
| switch { | ||
| case src.TemplateReplaced: | ||
| scope = ateletpb.SnapshotScope_SNAPSHOT_SCOPE_DATA |
There was a problem hiding this comment.
can it be DATA+Golden if template replaced and golden exist?
| // check (same non-atomicity caveat; resume re-resolves and fails | ||
| // cleanly), and the replacement's sandbox class, volumes, and volume | ||
| // mounts must match the old template's. | ||
| if !proto.Equal(oldVal.GetActorTemplate(), newVal.GetActorTemplate()) { |
There was a problem hiding this comment.
Repointing an actor whose template has no durable-dir volumes silently discards all of its FULL-snapshot state. UpdateActor performs no snapshot-scope/durable-volume check, and the forced-DATA resume cold-boots a fresh guest with no error or event.
Failure scenario: an actor uses a template with onCommit: FULL and zero durable-dir volumes (valid config; such templates exist in demos/). It suspends, uploading a FULL snapshot (memory+rootfs, no durable tar). The user repoints it at a volume-compatible template — accepted. On resume, TemplateReplaced forces SNAPSHOT_SCOPE_DATA (workflow_resume.go:729-730); ateom-gvisor (main.go:945-966) and ateom-microvm (restore.go:132-156) skip the untar (no durable volumes) and plain create+start the containers, ignoring the downloaded memory checkpoint. Resume reports success; the actor's entire persisted state is gone silently.
Note: the earlier in-PR guard (db5ec38) that refused FULL-scope repoints was removed (3800aab) without a durable-volumes-required condition replacing it.
| // different UID on the actor's current template means the actor was | ||
| // repointed since the capture. | ||
| snapshotTemplateUID := snapshot.GetStatus().GetActorTemplateUid() | ||
| src.TemplateReplaced = snapshotTemplateUID != "" && snapshotTemplateUID != actorTemplate.GetMetadata().GetUid() |
There was a problem hiding this comment.
TOCTOU: a repoint racing an in-flight resume is silently absorbed (actual code path: assignWorkerAttempt's version-conflict retry around workflow_resume.go:346 and 551-567 — outside this diff, so anchoring the comment here where TemplateReplaced is computed). The retry refreshes only the actor, not the already-resolved template/snapshot source, and the next suspend then stamps the NEW template UID onto OLD-template guest content, permanently disabling TemplateReplaced detection.
Failure scenario: actor SUSPENDED under template A; ResumeActor takes the lease and loadActorForResume resolves template A and src (TemplateReplaced=false). UpdateActor repoints A→B (the gate at actor.go:287 sees SUSPENDED and commits — it takes no lease). assignWorkerAttempt's UpdateActor hits ErrVersionConflict, re-reads (workflow_resume.go:551-567), sees still-SUSPENDED, and retries with the stale actorTemplate=A and src: atelet restores A's FULL snapshot un-forced while the spec ref says B. The next suspend stamps the new snapshot's ActorTemplateUid from the spec-resolved template B (workflow_suspend.go:386), not Status.CurrentActorTemplateUid (A), so snapshot-UID==template-UID forever after and the actor runs A's guest state under B's identity indefinitely.
| // loadActorForResume resolved a golden URI per the template's onResume | ||
| // configuration, else what the pause captured. | ||
| switch { | ||
| case src.TemplateReplaced: |
There was a problem hiding this comment.
The TemplateReplaced arm in the LOCAL-checkpoint branch fires spuriously off the stale durable LatestSnapshot and downgrades a resume-from-paused to SNAPSHOT_SCOPE_DATA, discarding the paused guest's memory. A local checkpoint is always captured under the current template, so this arm is never correct in this branch.
Failure scenario: suspend under template A (LatestSnapshot records A's UID; written only at workflow_suspend.go:400, never cleared by pause or resume) → repoint to B → resume (correct DATA restore) → pause (local FULL checkpoint captured under B, LocalSnapshotInfo set, LatestSnapshot untouched) → resume: loadActorForResume computes TemplateReplaced = A-UID != B-UID = true (lines 197-198), the local branch is taken (LocalSnapshotInfo != nil, line 682), and the switch sends wire scope DATA for a FULL local checkpoint — atelet either rejects the restore or cold-boots, silently losing the paused session, on every pause/resume cycle until the next suspend rewrites LatestSnapshot.
| if err := validateTemplateVolumesUnchanged(oldTemplate, newTemplate); err != nil { | ||
| return err | ||
| } | ||
| } else if !errors.Is(err, errActorTemplateNotFound) { |
There was a problem hiding this comment.
Deleting the old template waives ALL repoint compatibility checks (sandbox class, volumes, mounts) — and DeleteActorTemplate has no in-use guard — so a suspended actor can be repointed across sandbox families or volume layouts with zero validation. The snapshot's durably-recorded template (actor_template_uid/ObjectRef, and the GCS manifest's SandboxClass) could serve as a fallback anchor.
Failure scenario: an operator deletes gVisor template A (unconditional DELETE, atepg.go:598-603; ServiceImpl.DeleteActorTemplate is a TODO passthrough) while actor X is suspended on it, then repoints X to microvm template B with different volumes: resolveActorTemplate(oldVal) returns errActorTemplateNotFound and both validators are skipped. Nothing re-checks downstream (atelet only cross-checks golden-vs-actor manifest class), so the forced-DATA resume extracts A's data into B's mismatched layout under a different sandbox family — failing at restore time or silently misplacing data.
| if err != nil { | ||
| return err | ||
| } | ||
| oldTemplate, err := resolveActorTemplate(ctx, s.store, oldVal) |
There was a problem hiding this comment.
Chained repoints bypass the mount-layout invariant. Validation anchors on the old spec ref's template instead of the template the snapshot was captured under, and the per-container mount check skips containers absent from either side — so compatibility is non-transitive.
Failure scenario: snapshot captured under A (container main mounts volume data at /data). While still SUSPENDED: repoint A→B where B drops main (passes — removed containers unconstrained, actor.go:368-371 if !ok { continue }); repoint B→C where C re-adds main mounting data at /other (passes — main absent from B). On resume the forced-DATA restore surfaces the durable volume at /other instead of /data — exactly the "restore to the wrong places" condition the validator's own comment says it prevents — while a direct A→C repoint would have been rejected.
Anchoring validation on snapshot.status.actor_template_uid (which this PR's own resume mechanism uses) closes both this and the deleted-template hole.
| @@ -795,6 +808,13 @@ func (w *ActorWorkflow) finalizeRunning(ctx context.Context, actorRef resources. | |||
|
|
|||
| storedActor, err := w.store.UpdateActor(ctx, actorRef, store.PreconditionFrom(latestActor), func(toUpdate *ateapipb.Actor) error { | |||
| toUpdate.Status.State = ateapipb.ActorState_ACTOR_STATE_RUNNING | |||
There was a problem hiding this comment.
finalizeRunning blindly stamps State=RUNNING after a fresh re-read, so a lease-free CRASHED write landing between restore success and finalize is silently overwritten, producing a RUNNING actor with a nil worker assignment.
Failure scenario: restore succeeds; the worker pod dies and DeleteWorker's ensureBoundActorReleased (workflow_worker_delete.go:84-143, takes no actor lease, explicitly handles RESUMING) sets State=CRASHED and clears WorkerAssignment. finalizeRunning then GetActor's the post-crash version, so PreconditionFrom(latestActor) matches, and it writes RUNNING (now also stamping CurrentActorTemplate*) without checking the current state or restoring the assignment — a zombie RUNNING actor with no worker; the store has no CRASHED→RUNNING transition guard.
Pre-existing, but this PR touches the function, and the delete-worker code's comment assumes the optimistic check protects it.
| var scope ateletpb.SnapshotScope | ||
| var goldenSnapshotURI string | ||
| switch { | ||
| case src.TemplateReplaced: |
There was a problem hiding this comment.
When the new template has onResume.fromData: Golden, the TemplateReplaced arm outranks the golden arm and drops the NEW template's own golden URI, sending plain SNAPSHOT_SCOPE_DATA — a cold image boot — even though DATA_ON_GOLDEN with B's golden is exactly what the policy prescribes and is fully compatible.
Failure scenario: actor holds a durable DATA snapshot recorded under old template A; it is repointed to template B (fromData=Golden, golden ready). loadActorForResume sets both TemplateReplaced=true (line 198) and GoldenSnapshotURI=B's golden (lines 224-250 — resolved from the CURRENT template with no TemplateReplaced check, so the flags do co-occur). ensureAteletRestored emits scope=DATA with empty GoldenSnapshotUri in both branches; ateom-microvm's DATA path calls coldBootActorRetrying (restore.go:148-156), so every post-repoint resume pays cold-boot latency and loses the golden's warmed guest state — defeating the template class whose design is to never cold-boot — until a new snapshot is taken under B.
| scope := actorSnapshotContentScopeToAtelet(src.Scope) | ||
| if !src.GoldenSnapshotURI.IsZero() { | ||
| var scope ateletpb.SnapshotScope | ||
| var goldenSnapshotURI string |
There was a problem hiding this comment.
The scope/golden-URI precedence ladder (TemplateReplaced > golden > captured scope) is copy-pasted into both restore branches of ensureAteletRestored with subtly different defaults and shapes, instead of being computed once on resumeSnapshotSource in loadActorForResume where all three inputs are resolved.
Cost: the next scope-affecting rule (e.g. letting a repointed DATA snapshot still ride the new template's golden, per the adjacent finding) must be inserted at the right precedence position in two switches whose default arms already differ (onPause config vs src.Scope) and whose outputs land in different places (req fields vs locals); getting one branch wrong ships divergent wire scopes for local vs durable restores of the same actor. A single helper, e.g. src.wireScope(fallback) (scope, goldenURI), eliminates the bug class.
| return err | ||
| } | ||
| } else if !errors.Is(err, errActorTemplateNotFound) { | ||
| // Skip the validation if old template is not found |
There was a problem hiding this comment.
This comment is attached to the opposite branch it describes. "Skip the validation if old template is not found" sits inside else if !errors.Is(err, errActorTemplateNotFound) directly above return err — the path taken when the old template WAS found but failed to resolve; the skip is the invisible fall-through.
A reader will believe return err implements the skip, or misread resolveActorTemplate's not-found semantics. Restructuring as if err != nil && !errors.Is(err, errActorTemplateNotFound) { return err } with the skip comment on the not-found case is self-documenting.
| t.Run(tt.name, func(t *testing.T) { | ||
| err := validateTemplateVolumesUnchanged(tt.oldTmpl, tt.newTmpl) | ||
| if gotErr := err != nil; gotErr != tt.wantErr { | ||
| t.Fatalf("validateVolumeMountsUnchanged() error = %v, wantErr %v", err, tt.wantErr) |
There was a problem hiding this comment.
Test failure message names a nonexistent function: t.Fatalf("validateVolumeMountsUnchanged() error = ...") while the function under test is validateTemplateVolumesUnchanged — residue of a pre-rename identifier. A failing run points engineers at a function that doesn't exist.
3 participants
Fixes #477 Implements the following actor template update flow: