Repository navigation
SuspendActor defaults to suspend asynchronously - #2805
Conversation
Zoe Zhao (zoez7)
changed the title
2c7a140 to
f4baead
Compare
A SuspendActor request that leaves options.mode unset now runs an asynchronous suspend, as the SuspendOptions proto comment already documents: the snapshot stays on the worker's node and atelet uploads it in the background. Callers that need the snapshot in object storage before the call returns ask for DURABILITY_MODE_SYNCHRONOUS explicitly. Callers that relied on the old default now say which mode they want: - The template reconciler suspends golden actors synchronously. An async suspend of a golden actor is refused, and the reconciler tags the snapshot straight after. - kubectl ate suspend is synchronous, so it stays distinct from kubectl ate pause. Both build their request in one tested helper. - A worker-requested suspend of an idle actor is asynchronous: the actor is likely to resume on the same node. - The boomer and locust "suspend" lifecycle modes are synchronous, so they keep measuring a durable suspend rather than a pause. - Functional and e2e tests that read a completed durable snapshot, tag, revert, repoint the template, or move the actor to another node right after suspending ask for a synchronous suspend. A new functional test pins what each mode, including an unset one, leaves behind. The API guide now says an unset mode is asynchronous, that an eviction suspend should be synchronous, and that CreateTag fails until an async suspend's upload lands.
A worker-requested suspend now uses SuspendActor's default mode instead of hardcoding async. No worker calls this path yet, so a TODO on RequestActorSuspendRequest tracks letting the Worker choose the mode. A paused Actor stays pinned to its node until atelet detaches its local snapshot, so it cannot resume while that node is full. A TODO in schedulingConstraints tracks detaching it and resuming elsewhere. Tests and the boomer teardown that meant a durable suspend now ask for DURABILITY_MODE_SYNCHRONOUS, so the sync paths keep their coverage.
f4baead to
2e1412d
Compare
| resp, err := apiClient.SuspendActor(ctx, &ateapipb.SuspendActorRequest{ | ||
| Actor: actorRef.ToObjectRef(), | ||
| }) | ||
| resp, err := apiClient.SuspendActor(ctx, buildSuspendActorRequest(actorRef, false)) |
There was a problem hiding this comment.
nit: could you please extend the kubectl plugin to accept new flag in command line. The default behavior supposed to Async if it is not provided
There was a problem hiding this comment.
updated kubectl ate plugin.
kubectl ate suspend actor now takes --mode=async|sync and defaults to async, matching SuspendActor's default: the snapshot stays on the worker's node and atelet uploads it in the background. --mode=sync waits for the snapshot to reach object storage, which an eviction suspend or a tag right after suspending needs. Any other value is a usage error. kubectl ate pause actor still sends an asynchronous suspend. The architecture state diagram now labels which SuspendActor edge is the default, the phase breakdown runbook suspends with --mode=sync so it measures the upload, and the counter demo says where the snapshot goes.
| When the pod gets `SIGTERM`, its worker stops accepting actors and sends `SIGTERM` to the main process of each container of every actor it hosts. Each actor then has 30 minutes, counted from the pod's `SIGTERM`, to be suspended. An actor suspended in that window keeps its state and resumes on another worker like any suspended actor. | ||
|
|
||
| Substrate does not suspend the actor for you. Whatever drives the actor, usually its harness, has to call `SuspendActor` (`kubectl ate suspend`) within the 30 minutes. Only the actor gets the `SIGTERM`, so it has to pass that on, for example on an endpoint the harness polls. | ||
| Substrate does not suspend the actor for you. Whatever drives the actor, usually its harness, has to call `SuspendActor` with `DURABILITY_MODE_SYNCHRONOUS` (`kubectl ate suspend actor --mode=sync`) within the 30 minutes: an asynchronous suspend leaves the snapshot on a node that may be about to go away. Only the actor gets the `SIGTERM`, so it has to pass that on, for example on an endpoint the harness polls. |
There was a problem hiding this comment.
nit: the 30min statement is inaccurate. Upload to durable storage supposed to be faster (5min). Btw the 5min is not a rule. The upload will start during those 5 min. It might start early. And there is no promise it will be completed after 5min. The 30min is when actor is pinned to worker.
There was a problem hiding this comment.
Fixed in 71cb2c1. The 30 minutes in this section is the worker's shutdown budget after SIGTERM (workloadGracePeriod in ateworker, kept below the pod's 1h termination grace period), so the doc now says that explicitly. The async caveat is spelled out separately: atelet schedules the upload around --snapshot-upload-delay (5m default) with no promise on start or completion, and the actor stays pinned to the node until --snapshot-detach-delay (30m default, unrelated to the worker's 30 minutes). If the node goes away before the upload finishes, the snapshot is lost.
There was a problem hiding this comment.
In suspendActor (line 1016, which backs the SuspendResumeActor subtest in TestActorOperations), Options is still unset. pauseActor (line 936) already covers DURABILITY_MODE_ASYNCHRONOUS, so with the new default both subtests run an async suspend and nothing in TestActorOperations exercises synchronous durable suspend + resume.
Should line 1016 pass DURABILITY_MODE_SYNCHRONOUS (and possibly line 961 in pauseActor too)?
There was a problem hiding this comment.
Fixed in e08dd6f. suspendActor now passes DURABILITY_MODE_SYNCHRONOUS, as do the "before deletion" suspends in both pauseActor and suspendActor. The suite now reads "Pausing" as async and "Suspending" as sync, so TestActorOperations covers synchronous suspend + resume again.
There was a problem hiding this comment.
In TestActorTemplateGoldenSnapshot (line 203), SuspendActor on late (which borrows the golden tag snapshot uri) still leaves Options unset before assertSnapshotPresent(t, tc, uri) on line 206.
With the new async default, late's new snapshot stays PENDING, so pruneSnapshots keeps the COMPLETED borrowed golden snapshot on the actor and releaseDroppedSnapshots / releaseReplacedSnapshot is never reached for the tag-owned URI. Passing DURABILITY_MODE_SYNCHRONOUS on line 203 would keep that tag-ownership check covered.
There was a problem hiding this comment.
Fixed in e08dd6f. The SuspendActor on late now passes DURABILITY_MODE_SYNCHRONOUS, so its new snapshot is COMPLETED, the borrowed golden snapshot is dropped, and the tag-ownership check in releaseReplacedSnapshot is exercised before assertSnapshotPresent.
| ateapi_pb2.SuspendActorRequest(actor=self.actor_ref), | ||
| ateapi_pb2.SuspendActorRequest( | ||
| actor=self.actor_ref, | ||
| options=ateapi_pb2.SuspendOptions(mode=ateapi_pb2.DURABILITY_MODE_SYNCHRONOUS), |
There was a problem hiding this comment.
on_stop (lines 107–111 above) also wraps SuspendActor in traced_grpc("SuspendActor", ...), unlike the other Locust files (ate_api.py, sleep.py, kernelmem.py, usermem.py) where on_stop calls SuspendActor without traced_grpc. Because on_stop leaves options unset while run_and_suspend here passes DURABILITY_MODE_SYNCHRONOUS, user teardown now records async suspend latencies under the same "SuspendActor" Locust metric row.
Should on_stop either drop traced_grpc or pass DURABILITY_MODE_SYNCHRONOUS?
There was a problem hiding this comment.
Fixed in e08dd6f. on_stop now passes DURABILITY_MODE_SYNCHRONOUS, which keeps teardown behaving as it did before this change and keeps the "SuspendActor" row to sync latencies only.
There was a problem hiding this comment.
In TestActorLifecycle_WithExternalVolumes (line 1787, // 5. SuspendActor), Options is still unset. Step 3 (// 3. Async SuspendActor, line 1759) already tests DURABILITY_MODE_ASYNCHRONOUS, so step 5 now repeats an async suspend instead of testing a synchronous suspend with external volumes before DeleteActor.
Should step 5 pass DURABILITY_MODE_SYNCHRONOUS?
There was a problem hiding this comment.
Fixed in e08dd6f. Step 5 is now // 5. Sync SuspendActor and passes DURABILITY_MODE_SYNCHRONOUS, so the test covers both modes with external volumes.
| RUNNING --> SUSPENDING : SuspendActor (sync) | ||
| SUSPENDING --> SUSPENDED : checkpoint complete | ||
| RUNNING --> SUSPENDING : SuspendActor (async: keeps the snapshot on the node) | ||
| RUNNING --> SUSPENDING : SuspendActor (async, the default: keeps the snapshot on the node) |
There was a problem hiding this comment.
Further down in ### Phase 3: Hibernation (SuspendActor) (lines 478–493), steps 2, 4, and 5 still describe every SuspendActor as immediately streaming the snapshot to durable storage and deleting the replaced external snapshot. Now that async is the default, should Phase 3 note that immediate upload and release only happen on DURABILITY_MODE_SYNCHRONOUS (or in the background after an async suspend)?
There was a problem hiding this comment.
Fixed in e08dd6f. Phase 3 now says the upload finishes before SuspendActor returns only with DURABILITY_MODE_SYNCHRONOUS, and that the async default leaves the snapshot on the node for atelet to upload in the background. For release: an async suspend keeps the previous external snapshot, since its own is not yet durable, and that snapshot is deleted by the first suspend after a newer snapshot is durable, or by DeleteActor. I also dropped "an Actor owns one snapshot at a time", which no longer holds after an async suspend.
kubectl ate suspend actor now defaults to an asynchronous suspend, which is exactly what pause actor sent, so pause actor is redundant. Remove it along with the pause verb, which had no other subcommand.
The eviction section said an actor must be suspended within 30 minutes and that an asynchronous suspend leaves the snapshot on the node, which read as if the 30 minutes were how long the snapshot stays there. Say that the 30 minutes is the worker's fixed shutdown budget, and spell out what atelet does after an asynchronous suspend: it schedules the upload around --snapshot-upload-delay with no promise on when it starts or finishes, and keeps the actor pinned to the node until --snapshot-detach-delay, so the snapshot is lost if the node goes away before the upload completes.
…fault With SuspendActor defaulting to asynchronous, several tests and the counter demo Locust teardown silently switched to async suspends, leaving synchronous suspend + resume, sync suspend with external volumes, and the golden tag ownership check uncovered. Pass DURABILITY_MODE_SYNCHRONOUS explicitly there. Update the architecture doc's hibernation phase to describe when the upload and the release of the replaced snapshot happen in each mode.
PauseActor only forwarded to an asynchronous SuspendActor. Since agent-substrate#2805 made async the SuspendActor default, the RPC is redundant, and every client in the repository already calls SuspendActor. - Remove the `PauseActor` RPC and its request/response messages, the handler, request validation, the authorization rule, and the apitool exemptions for its messages. - Drop the functional test cases that called the RPC. - Rename tests named after it to `TestSuspendActorAsync_*`, and `workflow_pause_test.go` to `workflow_suspend_async_test.go`. - Rename the benchmark request names `PauseActor`, `PauseActorRecovery` and `PauseActorWarmup` to `SuspendActorAsync*`, along with the Grafana dashboard queries and the benchmarking README. Dashboards won't line up with runs from before the rename. - docs/authorization.md listed a `can_pause` relation the model never defined; it now lists only SuspendActor and RevertActor. ## Breaking change - The `PauseActor` RPC is gone. Clients calling it get `Unimplemented`. Call `SuspendActor` instead: it suspends asynchronously by default, which is what `PauseActor` did. - Benchmark request names `PauseActor`, `PauseActorRecovery` and `PauseActorWarmup` are now `SuspendActorAsync`, `SuspendActorAsyncRecovery` and `SuspendActorAsyncWarmup`. Saved queries or alerts on the old names need updating.
…substrate#2926) Depends on agent-substrate#2883, which makes a synchronous suspend record `locality` and keep `assigned_node`. The docs here describe that behavior after it merges. ## Summary Following the removal of `PauseActor` in agent-substrate#2813 and defaulting `SuspendActor` to asynchronous mode in agent-substrate#2805, update documentation, protobuf comments (and generated code), production Go comments, and test/benchmark comments across the repository that still referred to `PauseActor` or "paused" actors. ### Key Updates - **Documentation (`docs/`)**: - Remove the standalone `Pause` glossary entry in `docs/glossary.md` and update `Actor`, `Checkpoint`, `Restore`, and `Suspend` definitions to describe `SuspendActor`'s two durability modes (`DURABILITY_MODE_SYNCHRONOUS` and `DURABILITY_MODE_ASYNCHRONOUS`, with async as the default) and clarify that snapshots are always uploaded to object storage. - Update `docs/api-guide.md`, `docs/architecture.md`, `docs/api-style-guide.md`, `docs/authorization.md`, `docs/observability.md`, and `docs/metrics/registry/metrics.yaml` to describe synchronous vs. asynchronous `SuspendActor` and node-local snapshot pinning instead of `PauseActor`. - **Protobuf & Generated Code (`pkg/proto/ateapipb/`, `internal/proto/ateletpb/`)**: - Update comments on `SuspendActor`, `RevertActor`, `DeleteSnapshotTag`, `ActorStatus.assigned_node`, `Snapshot.locality`, and `ateletpb` RPCs; regenerate `*.pb.go` and `*_grpc.pb.go`. - **Production Code & Tests (`internal/`, `cmd/ateapi/`, `cmd/atelet/`, `benchmarking/`)**: - Update comments and error message hints referring to `PauseActor` or "paused" actors to reflect asynchronous `SuspendActor` and node-local snapshots.
SuspendActor without
options.modenow runs an asynchronous suspend, as theSuspendOptionsproto comment already documents: the snapshot stays on the worker's node and atelet uploads it in the background. Callers that need the snapshot in object storage before the call returns passDURABILITY_MODE_SYNCHRONOUS.In-repo callers that relied on the old default now set the mode explicitly:
kubectl ate suspend actortakes--mode=async|syncand defaults toasync, matching the API. Pass--mode=syncfor a suspend that waits for object storage.Breaking change
A SuspendActor call that leaves
options.modeunset no longer waits for the snapshot to reach object storage. Callers that tag the snapshot (CreateTag), move the actor to another node, or otherwise need a durable snapshot right after suspending must setoptions.modetoDURABILITY_MODE_SYNCHRONOUS. Until the background upload lands,CreateTagfails withFAILED_PRECONDITION.kubectl ate suspend actorwithout--modeno longer waits for the snapshot to reach object storage either; scripts that tag or migrate right after it need--mode=sync.Part of #2069 and #2571