Repository navigation
ateapi: Remove the PauseActor RPC - #2813
Conversation
f267bb9 to
da44a34
Compare
Dmitry Berkovich (dberkov)
left a comment
Dmitry Berkovich (dberkov)
left a comment
There was a problem hiding this comment.
Please remove pause from kubectl plugin as part of this change.
| calls := fakeCtrl.recordedCalls() | ||
| if slices.Contains(calls, "SuspendActor") || slices.Contains(calls, "PauseActor") { | ||
| t.Errorf("did not expect SuspendActor or PauseActor during iterate in none mode, got: %v", calls) | ||
| if slices.Contains(calls, "SuspendActor") || slices.Contains(calls, "SuspendActorAsync") { |
There was a problem hiding this comment.
🤖 should-fix 🟡 – The fake never records a call under this name. suspendCallName records an async suspend as SuspendActor(async), so this half of the check can never fire and an async suspend in none mode would go unnoticed. Check for "SuspendActor(async)" instead. The PauseActor string it replaces was already dead for the same reason.
There was a problem hiding this comment.
Fixed in 50e3516. The check now looks for SuspendActor(async).
| // recover suspends an actor asynchronously after a failed cycle, so it is cold | ||
| // for the next one. It is load on the server like any other call, so it is | ||
| // recorded, as SuspendActorAsyncRecovery to keep it apart from the measured | ||
| // suspends. An actor |
There was a problem hiding this comment.
🤖 nit 🟢 – The re-wrap left An actor dangling on its own line. Reflow the paragraph.
There was a problem hiding this comment.
Fixed in 50e3516. Reflowed the paragraph.
| // Only crashed or running actors, or suspended actors whose snapshot has | ||
| // not reached storage yet, can be reverted. |
There was a problem hiding this comment.
"whose snapshot has not reached storage yet" isn't quite accurate: isRevertable in workflow_revert.go checks latestSnapshotIsLocal (Locality != ""), not latestSnapshotIsLocalOnly. When atelet finishes uploading a snapshot, applyLatestDurable marks DurableSnapshot.Status = COMPLETED while keeping Locality set until atelet later detaches and deletes its local copy (TestRevertActor_PausedSnapshotAlreadyUploaded tests reverting in that state).
Consider "or suspended actors with a node-local snapshot" (matching the error message in workflow_revert.go:155). Same applies to cmd/kubectl-ate/internal/cmd/revert_actor.go and cmd/kubectl-ate/README.md.
There was a problem hiding this comment.
Fixed in ffe0be8. Reworded to "suspended actors with a node-local snapshot" in the proto, kubectl ate revert actor help, and README, matching the error message in workflow_revert.go.
Also added a sentence explaining why a suspended actor without a node-local snapshot is rejected: it is already at its durable snapshot, so there is nothing to revert.
There was a problem hiding this comment.
kubectl ate pause command was removed in #2805 already.
| // Only crashed or running actors, or suspended actors whose snapshot has | ||
| // not reached storage yet, can be reverted. |
There was a problem hiding this comment.
Fixed in ffe0be8. Reworded to "suspended actors with a node-local snapshot" in the proto, kubectl ate revert actor help, and README, matching the error message in workflow_revert.go.
Also added a sentence explaining why a suspended actor without a node-local snapshot is rejected: it is already at its durable snapshot, so there is nothing to revert.
PauseActor only forwarded to an asynchronous SuspendActor, and SuspendActor now defaults to asynchronous, so the RPC is redundant. Every client in the repository already calls SuspendActor. Remove the RPC and its messages, the handler, request validation, the authorization rule, the apitool exemptions for its undocumented messages, and the tests that called it. The authorization guide listed a can_pause relation that the model never defined; list only SuspendActor and RevertActor there. Rename what was still named after the RPC: - The tests that exercise the asynchronous suspend path are now TestSuspendActorAsync_*, and the workflow ones live in workflow_suspend_async_test.go. - The load generators record their async SuspendActor calls as SuspendActorAsync, SuspendActorAsyncRecovery and SuspendActorAsyncWarmup instead of PauseActor*. The Grafana dashboard queries and the ResumePaused metrics in the benchmarking README match.
The glutton none-mode test looked for an async suspend under a name the fake never records, so it could not catch one; check "SuspendActor(async)", the name suspendCallName records. RevertActor docs in the proto and kubectl-ate still listed paused actors. The state they meant is a suspended actor whose snapshot has not reached storage yet, so say that instead.
RevertActor accepts a SUSPENDED actor whenever its latest snapshot still has a node-local copy, including after the upload to storage completed but before atelet deleted the local copy. Match the RPC, kubectl help, and README wording to that check and to the FailedPrecondition message, and explain why a suspended actor without a node-local snapshot is rejected: it is already at its durable snapshot, so there is nothing to revert.
ffe0be8 to
abdefca
Compare
…pend sweperf always sent a synchronous SuspendActor, ignoring the shared --lifecycle-mode flag that agentsession, ateenv and glutton honor. sweperf now embeds dynconfig.Lifecycle like those classes. With lifecycle_mode=pause it sends an asynchronous SuspendActor, recorded as SuspendActorAsync like the other classes. The default (suspend) keeps today's behavior. lifecycle_mode none and resume_mode implicit are refused, since every sweperf cycle ends in a suspend and resumes with ResumeActor. pause is the existing --lifecycle-mode value for async SuspendActor; PauseActor itself was removed in agent-substrate#2813.
…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.
…rate#3068) sweperf always sent a synchronous SuspendActor and ignored the shared `--lifecycle-mode` flag that agentsession, ateenv and glutton honor. Lifecycle v2 defaults to async suspend, so sweperf couldn't measure the path most users take without a code change. sweperf now embeds `dynconfig.Lifecycle` like the other classes. With `--lifecycle-mode` pause, it sends an asynchronous SuspendActor, recorded as SuspendActorAsync. The default (suspend) keeps today's behavior, so existing runs are unchanged. `--lifecycle-mode` none and `--resume-mode` implicit are refused, since every sweperf cycle ends in a suspend and resumes with ResumeActor. note: pause is the existing `--lifecycle-mode` value for async SuspendActor according to how the other benchmarks do it; was wondering if we should change the naming for all since PauseActor is gone per agent-substrate#2813 AI use: AI-assisted, I reviewed and tested it. - [x] Tests pass - [ ] Docs updated --------- Co-authored-by: Haowei Cai (Roy) <[email protected]>
…t to async (agent-substrate#3097) Rename the `--lifecycle-mode` choices from `suspend` and `pause` to `sync` and `async`, and switch the default in `lifecycle_mode.py` and the boomer `agentsession`, `ateenv`, `glutton`, `durdir`, and `sweperf` user classes to `async` (`DURABILITY_MODE_ASYNCHRONOUS`) to match `Control.SuspendActor`'s default (agent-substrate#2813). - [x] Tests pass - [x] Docs updated
PauseActor only forwarded to an asynchronous SuspendActor. Since #2805 made async the SuspendActor default, the RPC is redundant, and every client in the repository already calls SuspendActor.
PauseActorRPC and its request/response messages, the handler, request validation, the authorization rule, and the apitool exemptions for its messages.TestSuspendActorAsync_*, andworkflow_pause_test.gotoworkflow_suspend_async_test.go.PauseActor,PauseActorRecoveryandPauseActorWarmuptoSuspendActorAsync*, along with the Grafana dashboard queries and the benchmarking README. Dashboards won't line up with runs from before the rename.can_pauserelation the model never defined; it now lists only SuspendActor and RevertActor.Breaking change
PauseActorRPC is gone. Clients calling it getUnimplemented. CallSuspendActorinstead: it suspends asynchronously by default, which is whatPauseActordid.PauseActor,PauseActorRecoveryandPauseActorWarmupare nowSuspendActorAsync,SuspendActorAsyncRecoveryandSuspendActorAsyncWarmup. Saved queries or alerts on the old names need updating.