Repository navigation
feat(ateapi): send snapshot files and scope from the control plane, remove the snapshot manifest - #1907
Conversation
ateom reports the exact files a checkpoint consists of, but atelet only recorded them in the snapshot manifest. Return them to the caller so the control plane can store the file list itself.
Add snapshot_files to LocalSnapshot and ExternalSnapshot. A pause stores the files that atelet's CheckpointResponse lists on the actor's LocalSnapshot. A suspend from running stores them on its ExternalSnapshot, and a tag copies them from the snapshot it references. This moves the list of files that make up a snapshot into the control plane, so manifest.json can later be removed. A suspend from PAUSED doesn't record files yet, because atelet doesn't report what it uploaded. Nothing reads the lists yet.
ateapi now sends the recorded file lists back to atelet: RestoreRequest.snapshot_files and golden_snapshot_files on restore, and the paused snapshot's files on UploadPausedCheckpointRequest. atelet reports the files it uploaded in UploadPausedCheckpointResponse, after narrowing a FULL capture to its durable-dir tar for a DATA upload. ateapi records that list on the ExternalSnapshot, so the narrowing lives only in atelet. atelet still reads manifest.json for everything else.
When UploadPausedCheckpoint is retried after a previous attempt uploaded every file and pruned the local snapshot, atelet recognized the finished upload by fetching the remote manifest. Check for the uploaded objects themselves instead, so the retry no longer depends on the manifest. ateapi now sends captured_scope. With desired_scope and snapshot_files it determines the files the upload wrote, and atelet looks each one up with a metadata-only lookup (GCS Attrs, S3 HeadObject). The lookups run concurrently, so the check takes about one round trip. The destination is minted once per suspend and each object becomes visible only once fully written, so finding every file means the previous attempt finished, and the retry returns those files like a fresh upload. Any missing file fails the upload as before.
Snapshots no longer carry a manifest.json. The files a snapshot holds are recorded by ateapi on the actor's ExternalSnapshot (and copied onto tags), and every restore and upload request now carries them, so the manifest had no remaining reader. atelet stops writing it at checkpoint and upload, and stops fetching it at restore; the manifest_fetch restore phase is removed from the metrics registry. UploadPausedCheckpointRequest gains sandbox_class, filled from the actor's template, so turning a FULL capture into a DATA upload still applies the per-class rule that used to read the class from the manifest. captured_scope is now required on upload. ateapi labels a restore golden when its snapshot is owned in the golden atespace. An actor still borrowing the golden tag's snapshot keeps the golden label it had before. The on-node sandbox record written at Run/Restore stays; it loses only the fields that existed for the manifest. Snapshots taken before this change have no recorded file list and cannot be restored. Golden tags must be regenerated, and suspended or paused actors from before the upgrade must be cold-booted.
Zoe Zhao (zoez7)
left a comment
There was a problem hiding this comment.
Also the PR description should not say "refactor". This is a behavioral change rather than pure refactoring.
main now takes the DATA snapshot file set from ateom and keeps it in the snapshot manifest, which this branch removes. Record it in ateapi instead: atelet reports data_snapshot_files in CheckpointResponse, the pause records them on LocalSnapshot, and the suspend of a paused actor sends them in UploadPausedCheckpointRequest. A FULL capture uploaded as DATA writes exactly those files; sandbox_class now only labels the upload metrics. Also drop RestoreRequest.golden_snapshot_files, left without a reader by the removal of the DATA_ON_GOLDEN restore scope.
Snapshots no longer carry a manifest: the control plane records each snapshot's files, and restores take the sandbox from the template's SandboxConfig. Remove the manifest_fetch restore phase, stop describing the manifest as part of a snapshot or as where the sandbox version and pause image are pinned, document ExternalSnapshot.snapshotFiles, and say that a restore uses the SandboxConfig as it is at resume time.
A snapshot URI names its owner's atespace, so atelet can tell a golden restore from the URI it already receives: the golden actor's snapshots and the golden tag an actor borrows until its first suspend are owned in the golden atespace. Drop RestoreRequest.from_golden_snapshot, which the control plane set only to carry that bit.
| // +k8s:listType=set | ||
| // +k8s:eachVal=+k8s:format=k8s-path-segment-name | ||
| // +k8s:eachVal=+k8s:maxLength=255 | ||
| repeated string data_snapshot_files = 5; |
There was a problem hiding this comment.
I'm ok with this as a temporary field, we'll need to update this soon with the Actor Lifecycle v2 proposal that represents the layers of snapshot in the api.
| // The sandbox class the pause checkpoint was captured with (e.g. | ||
| // "gvisor"), from the ActorTemplate's sandbox_config. It labels the upload | ||
| // metrics. | ||
| string sandbox_class = 11; |
There was a problem hiding this comment.
This can be removed?
There was a problem hiding this comment.
It only labels atelet's checkpoint-duration histogram for paused uploads (previously read from the manifest). The label is conditionally required, and ateapi's suspend metrics already carry the class from the template, so removed it.
| tele.SnapshotKind = ateattr.SnapshotKindLatest | ||
| scope := actorSnapshotContentScopeToAtelet(src.Scope) | ||
| if src.TemplateReplaced { | ||
| scope = ateletpb.SnapshotScope_SNAPSHOT_SCOPE_DATA |
There was a problem hiding this comment.
When doing a data only restore, does ate-apiserver need to pass the entire "src.SnapshotFiles" or just "DataSnapshotFiles"
There was a problem hiding this comment.
Just the data snapshot files should actually be enough
But on main via the manifest, atelet downloads the whole set and ateom ignores the memory images at DATA scope.
I'll record data_snapshot_files on ExternalSnapshot too, the same temporary field as on LocalSnapshot until Lifecycle v2, and send only those when a FULL snapshot is restored as DATA. but that's another API field and touches suspend, tag and resume, so can i do that in a follow-up ?
There was a problem hiding this comment.
And since this will be updated soon as well ?
The field only labeled atelet's checkpoint-duration histogram for uploads of paused actors. That label is conditionally required, and ateapi's suspend metrics already carry the sandbox class from the template, so the request no longer needs to carry it.
A FULL snapshot restored under a replaced template goes out at DATA scope, but the restore still listed every file, so atelet downloaded the memory images that ateom then ignores. Record the data-scope subset on ExternalSnapshot at suspend, copy it into tags, and send only those files when a FULL snapshot is restored as DATA.
Anish Ramasekar (aramase)
left a comment
There was a problem hiding this comment.
A DATA snapshot with no durable-dir volumes now has zero objects, so tagging it fails in CopyPrefix. Copying snapshot.GetSnapshotFiles() and treating an empty list as success would fix that.
The objectstoresnapshot proto still describes the manifest as the commit marker and should be updated in this PR.
nit: this comment still mentions the manifest.
| // leaves only orphaned files, never a manifest pointing at files that never | ||
| // landed; retries overwrite the deterministic object names. | ||
| func (s *AteomHerder) uploadSnapshot(ctx context.Context, uri resources.SnapshotURI, srcDir string, rec *sandboxAssetsRecord, templateAtespace, templateName string) error { | ||
| // uploadSnapshot uploads snapshotFiles from srcDir to uri, each |
There was a problem hiding this comment.
The commit point is now ateapi's finalize write, but the objectstoresnapshot proto still documents the manifest. Maybe something like "A snapshot is committed only when the control plane records it after UploadSnapshot succeeds; plugins must not infer commit state from object presence."
|
|
||
| tPersist := time.Now() | ||
| sandboxClass, err := s.uploadLocalCheckpointDir(ctx, req, localDir, uri) | ||
| snapshotFiles, err := s.uploadLocalCheckpointDir(ctx, req, localDir, uri) |
There was a problem hiding this comment.
Pruning before ateapi commits is why retries have to check object existence, and the plugin API has no RPC for that. Could we prune after commit instead, so a retry just re-uploads and ObjectExists/StatObject can go?
| } | ||
| return nil | ||
|
|
||
| // A DATA snapshot of an actor without durable-dir volumes has no files; |
There was a problem hiding this comment.
A pre-upgrade DATA snapshot with durable-dir volumes passes this check with no files, then fails late in ateom's untarDurableVolumes after a worker is assigned. Could this require files when the spec has a durable-dir volume, like shouldHaveSnapshots does? A note in docs/upgrade.md would help too.
| @@ -203,8 +204,9 @@ func isPausedOriginSuspend(actor *ateapipb.Actor) bool { | |||
| // the request is keyed by the actor UID, the worker pod UID, and the | |||
| // once-minted snapshot location, so a re-entered workflow re-sends the same | |||
| // semantic request; once atelet's Checkpoint is idempotent on those keys this | |||
There was a problem hiding this comment.
"Fully reentrant with no changes here" no longer holds, since nothing durable records snapshot_files for external checkpoints now. Same comment in workflow_pause.go.
| // +k8s:listType=set | ||
| // +k8s:eachVal=+k8s:format=k8s-path-segment-name | ||
| // +k8s:eachVal=+k8s:maxLength=255 | ||
| repeated string snapshot_files = 4; |
There was a problem hiding this comment.
nit: could the comment say these names are opaque, so clients don't depend on ateom's file layout?
|
I'm not sure if we should actually do this. The actual file structure of checkpoints are hypervisor and storage-specific, so we end up leaking a lot of implementation details into the api. The two primary use cases mentioned in #1835 should be resolved in other ways:
|
…-substrate#2432) Renames the object-store plugin proto package from `objectstoresnapshot.v1` to `objectstore.v1`. It is split out of agent-substrate#2385 as its own refactor, since that PR adds a non-snapshot service to the same API. - Moves `pkg/proto/objectstoresnapshotpb/v1` to `pkg/proto/objectstorepb/v1` and `objectstoresnapshot.proto` to `objectstore.proto`, regenerated with `hack/update/codegen.sh`. - Updates every in-tree caller, the protoredact pinned field names, and `docs/architecture.md`. - Services, messages and fields keep their names. The `cmd/snapshot-plugin` binary, image and manifests are unchanged. Open agent-substrate#1907 will need a rebase. It adds no references to the old package, so only its existing conflicts with main apply. #### Breaking change gRPC method names change from `/objectstoresnapshot.v1.<Service>/<Method>` to `/objectstore.v1.<Service>/<Method>`. Out-of-tree plugins must regenerate from `objectstore.proto` and roll out with atelet and ate-api-server. Please add the `breaking-change` label. - [x] Tests pass - [x] Appropriate changes to documentation are included in the PR
Follow up on #1835
Moving sandbox assets to RestoreRequest - #1866
Why
atelet wrote a
manifest.jsonbeside every snapshot and read it back to learn which files make up the snapshot, what scope it captured, which sandbox class took it, and whose it was. That made snapshot data in object storage an authoritative input to restore. #1866 moved the sandbox binaries and pause image onto the request; this PR moves the rest and deletes the manifest. It also takes a manifest GET off the restore path before any download and the final manifest PUT off suspend.Result: the control plane is the single source of truth for snapshots. atelet takes everything it needs from the request and never reads or writes a manifest.
Where each manifest field comes from now
snapshotFilesCheckpointResponse, stored onExternalSnapshot.snapshot_files/LocalSnapshot.snapshot_files(copied into tags), sent asRestoreRequest.snapshot_files/UploadPausedCheckpointRequest.snapshot_files.dataSnapshotFilesCheckpointResponse.data_snapshot_files, stored onLocalSnapshot.data_snapshot_files, sent asUploadPausedCheckpointRequest.data_snapshot_files. A FULL capture uploaded as DATA writes exactly these files; an empty list isFailedPrecondition. This replaces the manifest field from #2028 and the per-classdurable-dir.tarrule.RestoreRequest/UploadPausedCheckpointRequestcaptured and desired scope, from the actor and its template.sandboxClassate-golden.Breaking change
Snapshots taken before this change have no recorded file list and can't be restored. After upgrading, regenerate golden tags and cold-boot any actors that were suspended or paused before the upgrade.