Repository navigation
Revert and delete no longer hang when an actor's node is deleted - #2486
Conversation
|
I'm not an ATE developer, I just happened to run into this issue over the weekend so I'm taking an interest in it! My only thought about this PR is: it's a very targeted solution. It takes exactly one flow (deleting an actor) and solves it precisely - but should we consider something more comprehensive? I don't know what that might look like and the answer might be "no," but I think it's worth considering. |
|
I agree there’s a broader recovery-policy question here. This change handles the shared termination step used by both revert and delete, so those operations can finish when the assigned Node object is gone. A missing atelet on a node that still exists remains retryable. A more comprehensive design would need to cover recovery and reassignment, fencing when a machine remains alive after its Node object is deleted, and orphan cleanup. I’d keep this fix focused while discussing that broader policy separately. Is there a particular recovery flow you think should be included here? |
|
No, it was more of a "code smell" kind of situation. Julian Gutierrez Oschmann (@juli4n) and/or Scott Funkenhauser (@sfunkenhauser) might have follow-up suggestions. |
3f91e31 to
fe3d2d2
Compare
Bowei Du (bowei)
added
area/api-machinery
area/node
area/reliability
kind/bug
| // The node left the cluster, and its workload went with it. Waiting | ||
| // for its atelet would block revert and delete forever. |
There was a problem hiding this comment.
The PR description says a deleted Node object with the machine still running leaves atelet's state behind, so "its workload went with it" isn't always true. Could the comment state the trade instead? Something like:
// A deleted Node object counts as the node being gone for good. If the
// machine still runs, whatever atelet holds for the actor stays there.| // for its atelet would block revert and delete forever. | ||
| slog.WarnContext(ctx, "assigned node no longer exists, skipping atelet terminate request", | ||
| slog.String("node", nodeName), slog.Any("actor", actorRef)) | ||
| return nil |
There was a problem hiding this comment.
After this returns, revert runs ensureVolumesDetached and delete runs ensureVolumesDetachedForDelete, both with the same gone node name. For CSI that becomes ControllerUnpublishVolume on a node that no longer exists. The CSI spec says a driver should return OK there, but not every driver does. Did your kind run include an actor with external volumes? If not, an actor with volumes could still get stuck at the detach step.
| // A node that left the cluster takes its workload with it, so revert and | ||
| // delete must not wait for its atelet. A node that is still there may get its | ||
| // atelet back, and a failed lookup proves nothing, so both stay retryable. | ||
| func TestEnsureAteletTerminated_NodeGone(t *testing.T) { |
There was a problem hiding this comment.
This tests the helper on its own. The bug in #2470 is that the workflows never finish, so could we add a test at the workflow level as well? For example, a PAUSED actor whose node is gone reverts to SUSPENDED with assigned_node cleared, and delete --any-state removes it. That would catch a later step that dials the node again, and it would keep the fix if ensureAteletTerminated gets restructured.
fe3d2d2 to
ccf5a85
Compare
|
Thanks for the review. I rebased on main, changed "Fixes" to "Part of #2470", and took your wording for the comment. No, my kind run had no actor with external volumes. Revert and delete still call the plugin's I added a test that runs the whole revert and delete for a paused and a crashed actor. It covers a gone node and a kept node, and a volume whose detach returns nil, NotFound or another error. The volume cases use a mock plugin, not a real CSI driver. Without the fix, every gone node case fails. Reusing |
igooch
left a comment
igooch
left a comment
There was a problem hiding this comment.
#2883 merged today and narrows what this PR fixes. On main, revert already forgets an unreachable node when canDetachWithoutAtelet holds. That needs no worker assignment, no crash, and a COMPLETED durable copy of the latest local snapshot. detachFromFullNode does the same for resume. This PR still fixes the cases that check excludes. Those are a CRASHED actor, a paused actor whose upload never completed, and every delete. Could the first paragraph of the description say that, so it matches main?
The branch is also behind main, and #2881 and #2883 both touched workflow_revert.go.
Once rebased, this looks good to me.
| // atelet (#2470). If the Node object is gone, both finish, and the revert | ||
| // clears assigned_node. If it still exists, both fail with ErrNoAteletOnNode | ||
| // and leave the actor REVERTING or DELETING for a retry. A volume detach on |
There was a problem hiding this comment.
The paused actor here has a PENDING upload. Could the comment say so? On main, a paused actor with a COMPLETED upload passes canDetachWithoutAtelet, so revert finishes on a kept node too.
Since agent-substrate#2222, revert and delete call atelet Terminate whenever the actor has an AssignedNode. A PAUSED or CRASHED actor keeps that node. When the node left the cluster, the dial fails with ErrNoAteletOnNode on every try, so the actor stays in REVERTING or DELETING, and even delete --any-state cannot remove it. When the dial finds no atelet, ensureAteletTerminated now asks the dialer's new NodeGone whether the node still exists. NodeGone reads the node live from the API server, not from an informer, so a stale cache cannot skip a terminate. It reports the node as gone only on NotFound and when no atelet pod appeared meanwhile. A deleted node counts as nothing left to terminate. A node that exists and a failed lookup keep the current retry, and the lookup error is added to the returned error. ate-api-server gets get on nodes. Fixes agent-substrate#2470
Run the whole revert and delete workflows for a paused and a crashed actor whose node has no atelet, with the Node object gone and kept, and with an external volume whose detach on the gone node returns OK, NotFound or another error. Also reword the comment on the skipped terminate: a deleted Node object counts as gone for good, and if the machine still runs, what atelet holds for the actor stays there.
2f1dd7c to
fd16355
Compare
…#3010) agent-substrate#2883 lets resume forget an unreachable node when a paused actor's latest snapshot is durable. When that upload is still PENDING, the actor stays pinned. If the node was deleted, every resume returns `Unavailable`, and nothing says the snapshot is lost. Now, when resume finds no atelet on the pinned node and `NodeGone` reports the Node object deleted, it reads the actor again. If the upload was recorded during the lookup, it detaches as usual. Otherwise it crashes the actor. The crash keeps `assigned_node` and the PENDING snapshot. `RevertActor` then skips the terminate, deletes the pending objects, and returns the actor to its last completed durable snapshot. A node that still exists, or a failed Node lookup, keeps the current retry. There are two known limits. The check runs only after resume finds room on another node, so with no room anywhere resume still returns `RESOURCE_EXHAUSTED`. And a Node object deleted while the machine keeps running crashes the actor even though its checkpoint is still on that machine. That's the same trade agent-substrate#2486 makes. A PENDING copy may already be fully uploaded if atelet died before reporting it. Resume still crashes the actor, and revert deletes that copy. Recovering it needs ate-api-server to check for the manifest, which is a follow-up. No proto changes. `ResumeActor` can now crash the actor and return `FAILED_PRECONDITION` in this case, and `docs/api-guide.md` says so. Part of agent-substrate#2470 Supersedes agent-substrate#3005. Its "upload completes during lookup" regression case is adapted here. AI use: Claude Code helped write the change and tests. I reviewed them. - [ ] Tests pass - [x] Docs updated
After a node is deleted, revert can still hang for a CRASHED actor or a paused actor whose latest upload is PENDING. Delete can hang for any actor still bound to that node. #2883 already lets revert and resume forget an unreachable node when the actor has no worker assignment or crash and the latest local snapshot has a COMPLETED durable copy.
When no atelet is indexed on the assigned node,
ensureAteletTerminatedusesNodeGoneto check the Node through the live Kubernetes API. It skips termination only on NotFound, after checking again that no atelet appeared. Existing nodes and lookup failures stay retryable. The API server gainsgetpermission on nodes.External volumes still go through
DetachVolume: nil and NotFound succeed, other errors propagate. Kubernetes Node deletion does not fence a machine that is still running; resources on such a machine may remain.Rebased onto main
872ab2077. The whole-workflow test comment now identifies its paused actor's PENDING upload. Validation:TestNodeGonetests pass with-race -count=3 -shuffle=onand Docker-backed database tests enabled.cmd/ateworker-microvm/internal/child:TestReapedWhenItDiesOnItsOwnandTestStop. Both reportunsupported operationon macOS and reproduce from unchanged main872ab2077; the non-Linux pidfd implementation returnserrors.ErrUnsupported.hack/verify-all.shpasses (using local Python 3.13 and clang-format 23.1.3).git diff --checkpasses.Earlier kind runs exercised real worker deletion, paused/crashed revert and delete, and the kept-node control. Those live runs were not repeated for this rebase. Current volume tests use a mock plugin; no real CSI-driver validation is claimed.
Part of #2470. Resume without a completed latest upload remains separate.
AI assistance: Claude Code helped write the original code and tests. Codex clarified the fixture comment, rebased the branch, reviewed the integration, and ran validation.