[Based on 1492] Resume with Golden+DurDir when ActorTemplate updated. - #1461
[Based on 1492] Resume with Golden+DurDir when ActorTemplate updated.#1461Zoe Zhao (zoez7) wants to merge 2 commits into
Conversation
4fc2e0d to
212e512
Compare
Zoe Zhao (zoez7)
changed the title
Zoe Zhao (zoez7)
changed the title
89b99ad to
749c13a
Compare
4bb2d19 to
3c5e736
Compare
2edc462 to
b3dbb2d
Compare
| // applies only to data-only restores (Data pause scope, or a durable | ||
| // Data snapshot); valid Full snapshots restore from their own content. | ||
| dataOnly := false | ||
| if templateReplaced { |
There was a problem hiding this comment.
nit: it is very difficult to read the code, could you please try avoiding elseif?
| templateReplaced = snapshotTemplateUID != "" && snapshotTemplateUID != targetTemplateUID | ||
| } | ||
| } else if goldenRef := actorTemplate.GetStatus().GetGoldenSnapshotStatus().GetGoldenSnapshot(); goldenRef != nil && !boot { | ||
| snapshot, err := w.store.GetActorSnapshot(ctx, resources.ActorSnapshotRefFromObjectRef(goldenRef)) |
There was a problem hiding this comment.
If a stamped actor has no durable snapshot (LatestSnapshot == nil — reachable when a worker delete lands mid-suspend and ensureSuspendedFinalized commits SUSPENDED with no snapshot record), this fallback sets src.SnapshotURI to the template's golden. When that actor is also repointed and the new template's policy is COLD_BOOT, dataOnly becomes true below and the restore goes out as Scope=DATA carrying the golden's URI — the golden actor's durable-dir contents get restored as this actor's own data (pre-PR this state produced a FULL golden restore). With the policy unset, the same state emits a degenerate DATA_ON_GOLDEN with SnapshotUri == GoldenSnapshotUri (double manifest fetch). The repoint branch should probably not treat a golden-sourced src.SnapshotURI as actor data.
| src.TemplateReplaced = snapshotTemplateUID != "" && snapshotTemplateUID != actorTemplate.GetMetadata().GetUid() | ||
| capturedScope = snapshot.GetStatus().GetContentScope() | ||
| if currActorTemplateUID == "" { | ||
| // This is the codepath for actors that were created from a snapshot, and hasn't been resumed yet. |
There was a problem hiding this comment.
This fallback trusts the snapshot's recorded ActorTemplateUid, but workflow_suspend.go (~line 410) stamps that field from the freshly-resolved template rather than actor.Status.CurrentActorTemplateUid. Suspend a paused actor across a template delete/recreate and the snapshot records the NEW UID over the OLD template's guest bytes; an actor created from that snapshot has no stamp, lands here, sees the UIDs equal, and full-restores the replaced template's guest state — the exact bug class this PR fixes. Stamping the snapshot from CurrentActorTemplateUid (when set) would close this and also let the clone-creation guard in actor.go reject it.
| if boot || actorTemplate.GetSnapshotsConfig().GetOnResume().GetFromData() == ateapipb.ResumeSource_RESUME_SOURCE_COLD_BOOT { | ||
| dataOnly = true | ||
| } else if src.GoldenSnapshotURI, err = w.resolveGoldenSnapshotURI(ctx, actorTemplate, | ||
| "a repointed actor's resume requires the new ActorTemplate's golden snapshot, which is not available"); err != nil { |
There was a problem hiding this comment.
A terminally failed golden capture makes this a permanent failure: the template reconciler treats a non-empty ErrorMessage as terminal and never retries, so every default resume of a repointed (or same-name-recreated) template's actors returns FailedPrecondition — including router-driven resumes on live traffic — until an operator passes boot=true per-resume, edits the onResume policy, or recreates the template. It also fires transiently on every repoint-then-resume during the async golden-capture window. Consider checking golden status at repoint time in UpdateActor to surface this early, retrying failed goldens, or at least documenting the escape hatch.
| dataOnly := false | ||
| if templateReplaced { | ||
| if boot || actorTemplate.GetSnapshotsConfig().GetOnResume().GetFromData() == ateapipb.ResumeSource_RESUME_SOURCE_COLD_BOOT { | ||
| dataOnly = true |
There was a problem hiding this comment.
Unset fromData and explicit COLD_BOOT diverge here — the first and only place they differ. The proto contract says UNSPECIFIED selects the documented default, and docs/glossary.md documents ColdBoot as that default; yet after a repoint the unset template hard-fails without a golden while the explicit-ColdBoot one resumes data-only, and when a golden exists they restore different guest state (golden memory vs cold boot). Either treat UNSPECIFIED as ColdBoot in this branch, or update the from_data proto comment and the glossary to document the new repoint default.
| targetTemplateUID := actorTemplate.GetMetadata().GetUid() | ||
| currActorTemplateUID := actor.GetStatus().GetCurrentActorTemplateUid() | ||
| if currActorTemplateUID != "" { | ||
| templateReplaced = currActorTemplateUID != targetTemplateUID |
There was a problem hiding this comment.
Behavioral note: stamp-based detection marks a paused actor repointed even when its template is deleted and re-applied byte-identically (delete+create is the only way to edit an immutable template). Previously the local FULL pause checkpoint restored with memory intact; now the paused guest memory is silently discarded in favor of the recreated template's golden — or resume fails FailedPrecondition until the fresh golden capture completes. If intended (it is test-encoded), this deserves a release note; otherwise consider a spec-equivalence escape hatch.
| }}); err != nil { | ||
| t.Fatalf("failed to create reference Actor: %v", err) | ||
| } | ||
| defer func() { |
There was a problem hiding this comment.
Both deferred DeleteActor calls (this one and the main actor's at ~line 101) lack AnyState and discard the error, but the test now ends with both actors RUNNING (Step 9 resumes the main actor; the reference actor is never suspended). workflow_delete.go only permits deletion from SUSPENDED/CRASHED without AnyState, so cleanup silently fails with FailedPrecondition and leaks two actor records per subtest into the shared 'demo' atespace (actors.atespace is ON DELETE RESTRICT, so leaked rows block any future DeleteAtespace). Use the suspend-then-delete pattern from demo_test.go:100 or AnyState: true, and log the error.
| // combined restore. missingMsg is the FailedPrecondition message when the | ||
| // template records no golden snapshot. | ||
| func (w *ActorWorkflow) resolveGoldenSnapshotURI(ctx context.Context, actorTemplate *ateapipb.ActorTemplate, missingMsg string) (resources.SnapshotURI, error) { | ||
| goldenRef := actorTemplate.GetStatus().GetGoldenSnapshotStatus().GetGoldenSnapshot() |
There was a problem hiding this comment.
The golden-fallback branch above (~lines 215-228) still inlines this exact sequence (GetActorSnapshot, ErrNotFound→DataLoss, error wrap, validateGoldenSnapshotScope, ParseSnapshotURI), and on the fallback+repointed path this helper then re-fetches, re-validates, and re-parses the same golden ActorSnapshot — a redundant second store round trip on the resume path. Having the fallback branch call the helper (returning the snapshot or its ContentScope) and the repoint branch reuse the already-resolved URI would remove both the duplication and the double fetch.
…orForResume The restore step derived the wire scope twice, once per restore branch, each with a different fallback (the template's onPause scope for a local pause checkpoint, the durable snapshot's captured scope otherwise), from the raw inputs resumeSnapshotSource carried (Scope, TemplateReplaced). Every input is already in hand when loadActorForResume resolves the boot source, so resolve everything there: the boot-source decision becomes an explicit first-match rule list (explicit boot / repointed / paused / no snapshot / own durable snapshot), the struct carries the resolved WireScope, and ensureAteletRestored only consumes it, attaching the golden URI only when the scope is DATA_ON_GOLDEN. The golden snapshot's validation and location parsing, previously duplicated between the no-snapshot golden fallback and the Golden data-resume policy, collapse into resolveGoldenSnapshot. GoldenSnapshotURI becomes GoldenForDataSnapshotURI: it only ever names the guest half of a data-only combined restore, never the boot snapshot itself. Two deliberate behavior changes ride along: - An explicit boot request now always restores data-only: it discards any captured guest state and carries the actor's durable data alone, where it previously restored a Full capture at Full scope (or rode the golden under the Golden policy). Boot is the operator's escape hatch to a clean guest, so it must not depend on stored guest state. - A paused actor whose template records an unusable golden snapshot no longer fails its resume: the local checkpoint restore never touches the golden fallback, which only applies to actors with no snapshot of their own.
A repointed actor's snapshot holds guest state captured under the replaced template, so it must never be restored as-is. Previously every repoint fell back to a data-only restore, discarding the new template's golden snapshot entirely. ateapi now resolves a repointed resume against the new template, based on the actor's last capture: - A Full (or legacy Unspecified) capture — durable snapshot or pause checkpoint — rides the new template's golden snapshot combined with the actor's durable data (DATA_ON_GOLDEN), and fails the resume with FailedPrecondition when that golden is not yet available instead of silently discarding the guest state. - A Data-scoped capture holds no guest state to lose, so it follows the onResume policy like any data resume: data-only cold boot by default, riding the golden under the Golden policy. - An actor with no snapshot at all restores the new template's golden as its own Full content, and cold boots from the spec when the template has none. atelet's Restore narrows the actor's half of a DATA_ON_GOLDEN combine to its durable data before staging: a FULL actor snapshot also lists guest files whose names would shadow the golden's in the combined set, which would resurrect the replaced template's guest state. An actor with no durable data now contributes nothing and boots the golden alone. The updatetemplate e2e suite covers the repoint lifecycle end to end: the resume after a repoint restores the durable data on the new template's golden, and a later snapshot taken under the new template leaves the repoint path behind. The counter demo gains a boot UUID that lives only in guest memory, so the test can tell a restored guest from a fresh boot.
b3dbb2d to
75b6b67
Compare
Zoe Zhao (zoez7)
changed the title
2 participants
Before this PR, if actor's template has changed, the next resume cold boot the actor with DATA only. After this PR, the next resume will do:
If the DurDir does not exist, after the ActorTemplate update, the next Resume will use the golden snapshot of the new ActorTemplate.
Tested: Added a e2e test where the counter demo now reports a boot uuid that lives only in guest memory (restore from golden preserves it). The
TestUpdateTemplateLifecyclee2e test uses it to verify the repointed actor restores template B's golden memory image.Fixes #477