ateapi: resolve the resume boot source and wire scope once in loadActorForResume - #1492
ateapi: resolve the resume boot source and wire scope once in loadActorForResume#1492Zoe Zhao (zoez7) wants to merge 1 commit into
Conversation
a98f590 to
429a5f3
Compare
429a5f3 to
8b36b58
Compare
|
|
||
| // Rule 1: an explicit boot request discards any captured guest state and | ||
| // carries the actor's durable data alone (none: cold boot from the spec). | ||
| if boot { |
There was a problem hiding this comment.
Rule 1 changes the boot=true semantics from "skip the golden fallback" to "force a data-only restore": a suspended actor with a FULL durable snapshot (or a paused actor with an onPause=FULL checkpoint) previously restored at FULL scope with guest memory intact, and now silently drops all guest state. The tests assert the new behavior deliberately, but the public contract still describes the old one — pkg/proto/ateapipb/ateapi.proto ("If true, skip golden snapshot and boot the workload from scratch.") and the kubectl-ate resume-actor --boot help text are unchanged. If the new semantics are intended, those docs need updating and the change deserves a call-out beyond "refactor"; external callers get state-destroying behavior under an unchanged wire contract otherwise.
| onResumeGolden := actorTemplate.GetSnapshotsConfig().GetOnResume().GetFromData() == ateapipb.ResumeSource_RESUME_SOURCE_GOLDEN | ||
| // Rule 2: An actor whose template was updated will cold boot. | ||
| builtOnTemplateUID := actor.GetStatus().GetCurrentActorTemplateUid() | ||
| if external.GetSnapshotUri() != "" && builtOnTemplateUID != "" && builtOnTemplateUID != actorTemplate.GetMetadata().GetUid() { |
There was a problem hiding this comment.
This guard keys on external.GetSnapshotUri() != "", so a repointed PAUSED actor with only a local checkpoint skips Rule 2 entirely: run (stamps CurrentActorTemplateUid) → pause (writes only LocalSnapshotInfo, no external snapshot) → template deleted and recreated under the same name with a new UID → resume falls through to Rule 3 and restores the replaced template's guest state at FULL scope. That's exactly what the new doc says must not happen ("a mismatch ... means the snapshot's guest state must not be restored"), and it's asymmetric — the same actor with a leftover durable snapshot correctly gets DATA. The gap predates this PR, but since this rewrites the exact condition and codifies the invariant, the guard should key on any snapshot (local or external), not the external URI.
| } | ||
| src.SnapshotURI = goldenURI | ||
| // A golden snapshot is always FULL scope. | ||
| src.WireScope = ateletpb.SnapshotScope_SNAPSHOT_SCOPE_FULL |
There was a problem hiding this comment.
Two new decision branches have no test coverage: this Rule 5 success path (golden becomes src.SnapshotURI with WireScope hardcoded FULL — TestLoadActorForResume_GoldenFallbackRejectsNonFullGolden only covers the rejection path), and Rule 3's paused + onPause=DATA + non-Golden-policy DATA branch (line 235 — every paused test case uses the GOLDEN policy, seeds a durable snapshot, or passes boot=true). A regression in either (e.g. the golden fallback losing the FULL wire scope) would pass the suite unnoticed.
|
|
||
| // Rule 3: a paused actor restores its local pause checkpoint, which the | ||
| // template's onPause scope describes. | ||
| if actor.GetStatus().GetLocalSnapshotInfo() != nil { |
There was a problem hiding this comment.
Rules 3 and 4 are ~15-line copy-paste twins differing only in the scope source (template onPause vs external.GetContentScope()), and both hand-write the DATA→SNAPSHOT_SCOPE_DATA-else-FULL mapping that actorSnapshotContentScopeToAtelet (converter.go) already provides. Rule 3's raw GetOnPause() == DATA also diverges from the effectiveContentScope(...) convention used in workflow_pause.go and workflow_suspend.go (equivalent today, drifts if normalization grows). A shared helper like func(scope, onResumeGolden, tmpl) (goldenURI, wireScope, error) collapses both blocks to ~3 lines and keeps the next golden-ride policy change from having to land twice.
| } | ||
|
|
||
| onResumeGolden := actorTemplate.GetSnapshotsConfig().GetOnResume().GetFromData() == ateapipb.ResumeSource_RESUME_SOURCE_GOLDEN | ||
| // Rule 2: An actor whose template was updated will cold boot. |
There was a problem hiding this comment.
"will cold boot" misdescribes this branch: it keeps src.SnapshotURI and sets WireScope=DATA — a data-only restore of the durable snapshot, not a cold boot (which in this file means a RunRequest with no snapshot). It also contradicts the function doc three lines up ("the restore carries the actor's durable data alone").
| req.GoldenSnapshotUri = src.GoldenSnapshotURI.String() | ||
| default: | ||
| req.Scope = actorSnapshotContentScopeToAtelet(actorTemplate.GetSnapshotsConfig().GetOnPause()) | ||
| req.Scope = src.WireScope |
There was a problem hiding this comment.
The PR centralizes WireScope/golden resolution in resumeSnapshotSource, but the restore kind is still re-derived three separate ways in ensureAteletRestored: local-vs-external from actor.GetStatus().GetLocalSnapshotInfo() here, cold-boot from !src.SnapshotURI.IsZero() below, and a third derivation for the telemetry SnapshotKind — coherent with the rule list only by hand-mirrored ordering (Rules 1–2 return with src.SnapshotURI populated for paused actors even though this branch ignores it). Carrying a resolved Kind (local/external/coldBoot) in resumeSnapshotSource would collapse all three and would have made the Rule 2/Rule 3 guard gap structurally impossible.
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { |
There was a problem hiding this comment.
This reproduces ~40 lines of the TestLoadActorForResume_OnGoldenDataResume seeding harness nearly verbatim (seedOpts branches, actorState toggle, seedWorkflowActor, MustCreateAtespace, the template literal with OnResumeConfig and conditional GoldenSnapshotStatus), with only the OnPause field and golden scope differing — and the template is hand-built a third time in TestLoadActorForResume_TemplateReplaced. A shared seeding helper, or a boot column in the existing table, would absorb these cases and keep the copies from drifting when the seeding shape changes.
…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.
8b36b58 to
fb186be
Compare
2 participants
Refactored
loadActorForResumeto the following 5 steps:bootis true, boot from data-only from whatever snapshot exists (none: cold boot from the spec), never use golden.