Repository navigation
atelet: Add snapshot state tracker and return the latest local or durable snapshot - #2677
Conversation
0d74337 to
adb395e
Compare
igooch
left a comment
There was a problem hiding this comment.
A few questions, none blocking.
| defer t.unlock(actorUID) | ||
| a := t.findOrAddLocked(actorUID) | ||
| uuid := snap.Snapshot.GetSnapshotUuid() | ||
| if d := a.LatestDurable; d == nil || d.Snapshot.GetSnapshotUuid() != uuid { |
There was a problem hiding this comment.
🤖 No commit compares generations. CommitPromoted replaces LatestDurable whenever the uuid differs, so a retried promote of an older snapshot would move it back. TestTrackerSerializesEachActor can also end with LatestLocal older than LatestDurable. Should a commit ignore a snapshot older than the one it would replace? Snapshot.generation is already there.
There was a problem hiding this comment.
Not in this PR, for two reasons:
-
The control plane doesn't pass Snapshot.generation to atelet yet (see the TODO on the field). ateapi assigns generations to its own snapshots, but none of its calls to atelet copies them over, so atelet always sees 0 and has nothing to compare.
-
A commit is too late to catch a stale snapshot. By the time a stale Checkpoint commits, it has already pruned the newer local snapshot. The proto's plan is for Checkpoint and PromoteSnapshot to reject a stale generation before doing any work. That would also stop a stale promote from moving LatestDurable back.
I added a TODO on Tracker that says this. The race in TestTrackerSerializesEachActor only exists to exercise the lock, since the control plane never sends one actor's checkpoints concurrently. Its comment now says either commit may land last.
I'll also send a separate PR for passing generation through
There was a problem hiding this comment.
#2690 passes generation through to atelet.
adb395e to
027e7db
Compare
Add the snapshottracker package, which tracks what atelet holds for each actor on the node: its latest local snapshot and its latest durable one. Checkpoint records each deferred or immediate checkpoint, PromoteSnapshot records the snapshot it uploads, and both return the actor's state in their responses, which atelet used to leave unset. Terminate forgets the actor's state. Checkpoint and PromoteSnapshot update the tracker before they delete local checkpoints, so the state never names a deleted snapshot, even if the call then fails. Each actor's state is written atomically to <ActorPath(actorUID)>/snapshot-state.json, through the atomicfile package extracted from writeFileAtomic, and dropped from memory once written. Each actor has its own lock, so one actor's file I/O does not hold up another's. The control plane does not use the returned state yet. Once snapshots can upload in the background, it is how atelet will report that a local snapshot has been uploaded.
027e7db to
993b235
Compare
) Part of agent-substrate#2069 ateapi assigns each actor snapshot a generation, but never passed it to atelet: every `ateletpb.Snapshot` it builds left `generation` unset. That covers the pause and suspend `Checkpoint`, the paused-actor `PromoteSnapshot`, and both `Restore` paths. This sets it from the snapshot each request names, and narrows the TODO on `Snapshot.generation` to say only that atelet does not check it yet. Atelet requires generation to be set but doesn't use the value yet. Once agent-substrate#2677 lands, atelet can reject a `Checkpoint` or `PromoteSnapshot` whose generation is stale, as the `Snapshot.generation` doc describes. That check came up in review on agent-substrate#2677. > It's a good idea to open an issue first for discussion. - [ ] Tests pass - [ ] Appropriate changes to documentation are included in the PR
…nd finalizes (agent-substrate#2766) Part of Actor Lifecycle v2 agent-substrate#2069. atelet's `Checkpoint` and `PromoteSnapshot` return its snapshot state for the actor (agent-substrate#2677). The suspend workflow discarded it. This applies it when a suspend finalizes, with the same rules as a `ReportActorSnapshotState` report (agent-substrate#2622, agent-substrate#2698). No proto changes. - `ensureAteletSuspended` and `ensurePausedSnapshotUploaded` return the state atelet reports. - `ensureSuspendedFinalized` applies it with `applyCheckpointState` before it frees the worker, and records the result in the same update that commits `SUSPENDED`. - A durable snapshot records the fidelity atelet reports. For `Checkpoint` and `PromoteSnapshot`, that's the one requested. - An upload atelet reports finished for an older generation is marked `COMPLETED`, so finalize keeps it rather than pruning it as `PENDING`. - `applyLatestDurable` refuses a reported durable snapshot with no fidelity, rather than recording `UNSPECIFIED`. atelet echoes the fidelity it was asked for, and reports already require one. #### Notes for reviewers - **The state is never stale.** Every attempt that reaches finalize has just called atelet, so a retried suspend applies the state from its own call. - **A conflicting state fails the suspend.** Finalize applies atelet's state before it frees the worker. If the state conflicts with the record, for example a different upload URI, finalize returns the error and the actor stays `SUSPENDING` with its worker, so a retry can call atelet again. - **The lifecycle-op metric's fidelity label** stays the requested fidelity. - **With today's atelet, the record doesn't change.** The applied state matches what finalize already records. It starts to matter once atelet uploads in the background: an async suspend's response can report an older snapshot as uploaded before atelet's report arrives, and finalize then keeps that snapshot as `COMPLETED` instead of pruning it. - [ ] Tests pass - [ ] Appropriate changes to documentation are included in the PR
Part of #2069 and #2571
Add a new snapshot state tracker to atelet. It keeps a record of what the latest local and durable snapshot for an actor that this node has seen. The state is written to disk in the actor directory.
Checkpoint will update the state and return it in its response.
Right now the response is used by ateapi, but once we support asynchronous uploads, it will become important for communicating when a local snapshot has ben uploaded successfully.