Repository navigation
Atelet uploads local snapshots asynchronously - #2789
Conversation
Michelle Au (msau42)
changed the title
c099e3b to
cc346fd
Compare
| } | ||
| t.unlock(actorUID) | ||
| if removeOld { | ||
| t.removeSupersededLocalCopy(actorUID, target) |
There was a problem hiding this comment.
🤖 should-fix 🟡 – This deletes the older snapshot's local copy without checking whether a Restore is reading it. Restore opens LocalSnapshotDir(req.snapshot_uuid) directly and restores in place from it, and BeginStart waits only for a detach, not for an upload. Before this PR only one local snapshot existed, so a Restore naming an older UUID fell through to object storage. Now a Restore of the Uploading snapshot (for example after a Checkpoint committed but returned an error, so ateapi still names the previous snapshot) can have its files removed mid-restore.
Suggest BeginStart wait for a background upload of the snapshot being restored, as BeginPromote does, or skip this delete while holds > 0 and leave it to the next prune.
There was a problem hiding this comment.
Fixed: superseded copies are no longer deleted while a Run, Restore, Checkpoint, or PromoteSnapshot holds the actor; the next checkpoint, detach, or terminate removes them. The check and the delete both happen under the actor's lock. I preferred this to making BeginStart wait for the upload, so resume latency doesn't depend on uploads, and it also covers the worker cleaning up a superseded Uploading.
| if !a.keepUploading() || sameUUID(a.nextUploadTarget(), &snap) { | ||
| t.resetBackoffLocked(a) | ||
| } | ||
| t.saveLocked(a) |
There was a problem hiding this comment.
🤖 should-fix 🟡 – When a.Uploading has failed (a.uploadingFailed == true) and CommitLocal commits a newer uploadable snapshot, keepUploading() is false, so LocalSnapshotsToKeep omits a.Uploading and Checkpoint immediately deletes its local directory (cmd/atelet/main.go:1001). However, saveLocked here still writes the old a.Uploading to the state file, and uploadingFailed is not persisted in actorSnapshotsRecord. If atelet restarts before a background worker runs cleanupSupersededUploadingLocked, the reloaded entry has uploadingFailed == false, so keepUploading() becomes true again and nextUploadTarget() tries to upload the snapshot that PruneLocalCheckpoints already deleted.
Suggest clearing a superseded a.Uploading before saveLocked here (or persisting uploadingFailed in actorSnapshotsRecord).
There was a problem hiding this comment.
Fixed: CommitLocal now clears a superseded Uploading in the same save, and the Checkpoint's cleanup deletes its copy. TestTrackerCommitLocalDropsFailedUploading covers the restart case.
The tracker uploads an actor's latest local snapshot in the background once the oldest snapshot not yet durable has waited --snapshot-upload-delay. Checkpoint, Run, and Restore wait only for a detach, not an upload. Detach waits for an upload in progress rather than repeating it. Terminate cancels the upload without waiting for it. A checkpoint deletes older local snapshots only after the new one is committed, so a failed checkpoint leaves the previous snapshot in place. A checkpoint keeps the snapshot a background upload is still reading, which can be older than the latest snapshot. The actor's snapshot state reports the upload in progress and when the next one is due.
cc346fd to
f9698e2
Compare
|
I found a bug in ateapi handling that needs fixing. Will add a second commit. |
|
Decided to fix it as a separate PR: #2795 |
Atelet uploads an actor's latest local snapshot in the background once the oldest snapshot not yet durable has waited --snapshot-upload-delay.
Checkpoint, Run, and Restore wait only for a detach, not an upload. Detach waits for an upload in progress rather than repeating it. Terminate cancels the upload without waiting for it.
A checkpoint deletes older local snapshots only after the new one is committed, so a failed checkpoint leaves the previous snapshot in place.
A checkpoint keeps the snapshot a background upload is still reading, which can be older than the latest snapshot.
Part of #2069 and #2571
Note to reviewers
This PR doesn't update ateapi of the asynchronous upload success. So durable snapshots won't get cleaned up until the actor is deleted. We are addressing this in a followup PR.
Benchmark Results (glutton, 3m, 1 user, 512Mi RAM, 32Mi churn)
Results show we get the same performance as pause, with the benefit of automatically uploading checkpoints to object storage. Out of the 73 local checkpoints, 10 were uploaded to GCS in the background (roughly 1 in 7), and the other 63 were superseded locally before the 15s upload delay fired.
PauseActorp50 / p90 / p99ResumeActorp50 / p90 / p99ac296df67)snapshot-send-obj(cee8dc751, 15s delay)2.2–3.0seach)