Repository navigation
atelet: detached actors no longer pin sandbox assets in the node cache - #2772
Eric Curtin (ericcurtin) wants to merge 2 commits into
Conversation
|
Dmitry Berkovich (@dberkov) ILLUM1N0X PTAL when you get a chance. Thank you! |
atelet's `DetachActor` returned UNIMPLEMENTED. Without it, ate-api-server can't make a suspended actor's latest snapshot durable and free it from the node on demand. Resuming the actor on another node needs that. The handler validates the request and runs the snapshot tracker's `Detach`. That's the same detach atelet runs on its own after `--snapshot-detach-delay`. The handler returns the actor's new snapshot state. #### API changes - `Atelet.DetachActor` (atelet internal) is now implemented. The proto comment drops its "returns UNIMPLEMENTED" TODO. No fields change. #### Not in this PR - Detach deletes the actor's local snapshots but not the rest of the actor's directory. The detach atelet runs on its own does the same. Removing the whole directory overlaps agent-substrate#2742 and agent-substrate#2772, and belongs in the tracker so both paths get it. - Nothing in ate-api-server calls `DetachActor` yet. Resuming elsewhere will be a follow-up.
igooch
left a comment
igooch
left a comment
There was a problem hiding this comment.
Looks right. The removal sits after the prune and before LatestLocal is cleared, so a failed removal is retried with the detach, and Run and Restore re-stage the assets for a later resume on the node.
On ordering with #2871, which has detach remove the whole actor directory. I'd land this first. It is small, and it reclaims the only part of a detached actor's leftovers that pins disk. #2871 then drops the removeSandboxAssets call in its rebase and keeps the nlink test from here as a regression check.
This needs a rebase before it can merge. The conflict is one comment block in finishDetach, against the recordDetach hunk from #2815.
Bowei Du (bowei)
added
area/node
area/storage
kind/bug
babec6e to
f58cadf
Compare
|
Merged main; it applies cleanly now, so no conflict is left. igooch thank you for the review. |
| // call; a worker's detach makes a report of what it changed due, while a | ||
| // Detach call returns the new state and cancels any pending report. A detach | ||
| // Forget abandons leaves the actor alone, and fails. | ||
| // snapshots and sandbox asset links. background reports whether a worker runs |
There was a problem hiding this comment.
🤖 nit 🟢 – Other docs still describe a detach as deleting only the local snapshots. docs/sandbox-asset-cache.md says when an actor's links are made and kept, but not that a detach drops them. In docs/metrics/registry/metrics.yaml, the atelet.snapshot.detach.duration note says a failed detach keeps the local snapshots, which no longer holds when only the link removal fails. The Detach doc at line 42 and the Tracker doc in tracker.go could mention the links too.
| } | ||
| st, ok := fi.Sys().(*syscall.Stat_t) | ||
| if !ok { | ||
| t.Skip("link counts are unavailable on this platform") |
There was a problem hiding this comment.
🤖 nit 🟢 – This skip is unreachable, since os.Stat returns a *syscall.Stat_t wherever this file compiles. If it ever fired, the test would skip its link-count checks instead of failing. Calling syscall.Stat and failing on error, as allocatedBytes in filecache_test.go does, drops the branch.
|
Eric Curtin (@ericcurtin) looks like this PRs functionality mostly merged in #2871. Are you OK with closing this PR? |
|
Agreed, #2871 already removes the sandbox asset links on detach, so closing this. |
Detach now deletes the actor's sandbox asset hard links, so the asset cache can evict them.
Fixes #2742