Repository navigation
atelet: Clean up whole actor directory on detach - #2871
Michelle Au (msau42) merged 1 commit into
Conversation
igooch
left a comment
igooch
left a comment
There was a problem hiding this comment.
I'm for removing the whole directory at detach. I have one question inline, and two small asks here.
Please say in the PR description what happens to a pending detach report when atelet restarts. Today the report is saved in snapshot-state.json, so it survives a restart. With this PR it lives only in memory. That may be fine, but the description should say so. The inline comment has the details.
Please update the DetachActorResponse comment in atelet.proto. It says latest_durable is set "if atelet knows one". After this PR, atelet forgets the actor once the detach report is sent, so calling DetachActor again returns an empty state. #2598 will call DetachActor, and it needs to treat an empty state as already detached.
On merge order with related PRs:
- If #2772 merges first, drop its
removeSandboxAssetscall when you rebase, sinceRemoveActorDirsalready deletes that directory. Keep itsnlinktest. If this PR merges first, #2772 can be closed. - If #2746 merges first,
RemoveActorDirsalso needs to delete/var/lib/ate/snapshots/<uid>/(ateletpath.ActorSnapshotPath). If this PR merges first, #2746 adds that when it rebases.
Bowei Du (bowei)
added
area/node
area/storage
kind/bug
e288d18 to
37b3a97
Compare
c3ca6e7 to
2e9adce
Compare
2e9adce to
bb801e6
Compare
5d2f943 to
5a62856
Compare
igooch
left a comment
igooch
left a comment
There was a problem hiding this comment.
Agreed on choice of the Option B shape. I found one bug in the forget path and one merge-order dependency on #2797. Details inline.
5a62856 to
81dad7f
Compare
igooch
left a comment
igooch
left a comment
There was a problem hiding this comment.
🤖 Both comments are addressed, thanks.
The new commit message doesn't match the diff. Neither main nor this PR has a RemoveActorData, and it leaves out the actordirs move, the --snapshot-report-workers >= 1 check and the cleanUpAfterCheckpoint change. Could you restore the
previous message and add a line for the !a.Started guard?
Otherwise, looks good on top of #2797.
81dad7f to
d6e7f68
Compare
Addressed! |
25d59ae to
c383eed
Compare
Move ResetDir, ResetActorDirs, ResetActorDirsKeepingVolumes, RemoveActorFilesExceptState, and RemoveActorDirs into cmd/atelet/internal/actordirs so both atelet and snapshottracker can call them. When Detach runs (or background upload finishes with DeleteLocalOnUpload), call actordirs.RemoveActorFilesExceptState to delete the actor's snapshot directory tree and all files and subdirectories under the actor's directory except snapshot-state.json, refusing to delete through a populated external volume mount directory. When the detach report is acknowledged or refused with NotFound and the actor has not started again (!a.Started), delete snapshot-state.json and remove the empty actor root directory. Keep a pending detach report due in CommittedCheckpointState when LatestLocal is nil so a retried Checkpoint after a background detach still cleans up snapshot-state.json and the actor directory once the detach report completes. Require --snapshot-report-workers >= 1 in validateSnapshotWorkerFlags and remove the reporting-disabled detached state removal branch in queueReportLocked and enqueueSavedReportLocked. When Checkpoint cleans up with no local snapshot kept on the node (such as UPLOAD_MODE_IMMEDIATE without a local copy, or a retried Checkpoint after Detach), call actordirs.RemoveActorFilesExceptState instead of actordirs.ResetActorDirs so empty subdirectories are not recreated.
c383eed to
1119116
Compare
What this PR does and why we need it ?
When detaching an actor (either via background timer detach or explicit
DetachActorRPC),ateletpreviously only pruned entries inside/var/lib/ate/snapshots/<uid>/local-checkpoint/, leaving the actor's snapshot directory (/var/lib/ate/snapshots/<uid>) and actor directory (/var/lib/ate/actors/<uid>) on the node indefinitely. Becausesandbox-assets/holds hard links into the sandbox asset cache (st_nlink > 1), leaving the actor directory also permanently pinned sandbox binaries in the cache after detach.Changes
ResetDir,ResetActorDirs,ResetActorDirsKeepingVolumes,RemoveActorFilesExceptState, andRemoveActorDirsintocmd/atelet/internal/actordirssoAteletandsnapshottracker.Trackershare directory cleanup logic (preserving populated external volume mount directories, removingateletpath.ActorSnapshotPath, and handling read-only bundle upper directories viaimagecache.RemoveAllWritable).Tracker.finishDetachto callactordirs.RemoveActorFilesExceptState(removing/var/lib/ate/snapshots/<uid>and all entries under/var/lib/ate/actors/<uid>exceptsnapshot-state.json) whileopDetachis active, and mark a detach report due (markReportDueLocked) for both background and foregroundDetachcalls.snapshot-state.json(withReportDueSince) on disk until the detach report finishes so a retriedDetachActorRPC still seesLatestDurableand a pending detach report survives anateletrestart.snapshot-state.jsonand the empty/var/lib/ate/actors/<uid>directory (viaremoveDetachedActorStateinfinishReportDueLocked) once!Started && LatestLocal == nil && Uploading == niland the control plane acknowledges the detach report or refuses it withNotFound. On give-up or any other refusal (Unimplemented,FailedPrecondition,InvalidArgument),snapshot-state.jsonstays on disk withReportDueSincecleared soDetachActorkeeps returningLatestDurable.--snapshot-report-workers >= 1invalidateSnapshotWorkerFlagsso background detach reports are always sent.cleanUpAfterCheckpoint, callactordirs.RemoveActorFilesExceptStateinstead ofactordirs.ResetActorDirswhen no local snapshot is kept on the node, so empty subdirectories are not recreated.atelet.proto(ActorSnapshotState.latest_durableandDetachActorResponse.state) thatDetachActorqueues a background detach report and forgets the actor once acknowledged.