Repository navigation
Resume restores an uploaded snapshot when its node is gone - #3094
Dmitry Berkovich (dberkov) merged 2 commits into
Conversation
563f67d to
bb7c324
Compare
atelet can upload a suspended actor's latest local snapshot and die before reporting it. If the node is then deleted, resume crashed the actor, and a revert deleted the complete upload. Resume now releases the deleted node and restores the snapshot on another node, where atelet reads the upload. The snapshot keeps its old locality until then, so resume still picks it. A NOT_FOUND from that restore crashes the actor as before. A successful restore records the snapshot COMPLETED and clears its locality. The runtime fallback applies to it as to a durable snapshot, since the restore reads the upload.
bb7c324 to
0ed8732
Compare
| } | ||
| if err := crashActorVersion(ctx, w.store, actorRef, actor, ateattr.OperationResume, crashMessageLocalSnapshotNodeGone); err != nil { | ||
| stored, err := w.store.UpdateActor(ctx, actorRef, store.PreconditionFrom(actor), func(toUpdate *ateapipb.Actor) error { | ||
| toUpdate.Status.AssignedNode = "" |
There was a problem hiding this comment.
Just for my understanding: recordGoneNode now actually sets AssignedNode = "", this enables the second attempt after detach https://screenshot-v2.corp.google.com/5h18n7ptamph0 to assign a worker elsewhere and continue to resume.
Then after resume, if we successfully resumed, recordRestoredUpload records the latest durable snapshot, if resume fails, we crash the actor
There was a problem hiding this comment.
Yes, that's right. Clearing assigned_node lets the second ensureWorkerAssigned place the actor on any node. A successful restore then records the snapshot COMPLETED in finalizeRunning. A NOT_FOUND from the restore crashes the actor with the gone-node message, and other restore errors are handled as before.
| // recordRestoredUpload records the upload of a snapshot that resume restored | ||
| // on another node after its own node was deleted. The restore proved the | ||
| // upload complete, and the deleted node no longer holds a copy. | ||
| func recordRestoredUpload(ctx context.Context, status *ateapipb.ActorStatus) error { |
There was a problem hiding this comment.
🤖 nit 🟢 – recordRestoredUpload is only covered end to end through ResumeActor. A small table test on the function would cover the no-op cases (snapshot local on the assigned node, durable-only snapshot), the node-gone case, and the error path.
There was a problem hiding this comment.
Added TestRecordRestoredUpload in f268242. It covers both no-op cases, the deleted-node case, and the error path.
| toUpdate.Status.WorkerAssignment.WorkerEpoch = workerEpoch | ||
| toUpdate.Status.WorkerAssignment.WorkerPodIps = slices.Clone(worker.GetStatus().GetIps()) | ||
| return nil | ||
| return recordRestoredUpload(ctx, toUpdate.Status) |
There was a problem hiding this comment.
🤖 should-fix 🟡 – If recordRestoredUpload returns an error, finalizeRunning never commits RUNNING, but the workload is already running on the new worker. Each retry restores again and fails here again, so the actor stays RESUMING.
As far as I can tell, only a drifted record reaches this: UNSPECIFIED fidelity plus a replaced template. The VOLUMES restore succeeds, and then applyLatestDurable rejects the missing fidelity.
I suggest skipping the record in that case. Log the error at Warn and commit RUNNING with the snapshot left PENDING. The next suspend prunes it, so the cost is only that the upload stays unrecorded.
There was a problem hiding this comment.
Good catch. Done in f268242. finalizeRunning logs the error at Warn and commits RUNNING, leaving the entry PENDING for the next suspend to prune. TestFinalizeRunning_UnrecordableUploadStillRuns covers it.
| t.Errorf("snapshots = %v, want %v and %v unchanged", snaps, durable, pending) | ||
| } | ||
|
|
||
| // The crash leaves the actor on node-2, so revert terminates it |
There was a problem hiding this comment.
🤖 nit 🟢 – AGENTS.md asks to avoid contrast pairs. Consider dropping "rather than skipping a deleted node".
A restore from the upload of a snapshot on a deleted node succeeds before finalizeRunning records the upload. If the record rejected it, for example a snapshot with no fidelity, RUNNING never committed and every retry restored again. finalizeRunning now logs the error and commits RUNNING, leaving the entry PENDING for the next suspend to prune. Add a table test for recordRestoredUpload.
Part of #2470, follow-up from 3010.
atelet can upload a suspended actor's latest local snapshot and die before it reports the upload. If the node is then deleted, the upload is complete but still recorded
PENDING. Resume crashed the actor, and a revert then deleted the upload.Resume now releases the deleted node and restores the snapshot on another node. atelet there has no local copy, so it reads the upload. The manifest is written last, so a missing or partial upload returns
NOT_FOUND.recordGoneNodeclearsassigned_node. The snapshot keeps its old locality, sosnapshotToResumestill picks it.NOT_FOUNDfrom that restore crashes the actor with the existing gone-node message.finalizeRunningrecords the restored snapshotCOMPLETEDand clears its locality.canRelaxRuntimelets the snapshot fall back to another runtime, as a durable snapshot can.No proto changes.
Resume claims a worker before it knows the upload exists, so a missing upload costs one placement. A revert still deletes an unreported upload, so the snapshot is lost if its restore fails for another reason or the actor is reverted before it resumes.
AI use: Claude Code helped write the change and tests. I reviewed them.