Repository navigation
ateworker-microvm, atelet: classify pre-serving restore failures as Unavailable - #2744
Conversation
26ad7f3 to
6bdaf43
Compare
6bdaf43 to
49f8038
Compare
| // attempt leaves no state behind and is safe for the caller to retry (re-staging | ||
| // the snapshot if it was dropped above). When the caller's own context ended, | ||
| // wrap ctx.Err() so the server reports the context status instead. | ||
| func restoreGuestReadyError(ctx context.Context, step string, err error) error { |
There was a problem hiding this comment.
Nothing bounds this. A guest that dies on every resume (kernel panic after Resume → errGuestStopped from dialAgentRetry, or a corrupt memory image) now cycles RESUMING → Unavailable → re-stage on every router flight, where today it crashes once and shows up as CRASHED. coldBootActorRetrying bounds the same errGuestStopped case with coldBootAttempts. Either exclude errGuestStopped here (the VMM exited, that is not scheduling contention) or bound the attempts somewhere; otherwise a permanently broken snapshot is only visible as 503s.
Bowei Du (bowei)
added
area/microVM
area/node
area/reliability
kind/bug
49f8038 to
e49632d
Compare
Walter Fender (cheftako)
left a comment
Walter Fender (cheftako)
left a comment
There was a problem hiding this comment.
🤖 Review of the latest push (e49632d). The worker-side cleanup reorder looks right, and the second commit addresses Aditya Shantanu (@aditya-shantanu)'s unbounded-retry comment (errGuestStopped is no longer Unavailable, and atelet gives up after 3 straight failures). I found two bugs on the atelet side, and the retry classification is wider than the description says. Details are inline.
What looks right
- Moving the failure-cleanup defer up next to
PrepareEgressmakes it run last: after the CH and virtiofsd kills and after the untar drain. That matchesteardownActor. Before,UnmountAllUnderran while virtiofsd was still alive. - Failures between
PrepareEgressand the network setup now also deactivate the tunnel. restoreGuestReadyErrorformats the cause with%vwhen the caller's context ended, so a childDeadlineExceededcan't hide the caller'sCanceled.
Not inline
- A transport
Unavailablefrom a worker pod that is restarting counts toward the 3-failure limit. That's minor and still much better than today's crash on the first failure. - The kernel/image check from #2699 still runs before any side effect. 👍
Tested: the new tests pass with -race. In ./cmd/atelet, TestAteletServerTLSConfigReloadsCACertsWithoutRestart fails for me, but it fails the same way on the PR's base commit, so it's unrelated.
| // If the restore fails after mounting external volumes, unmount them so a | ||
| // retried Restore's resetActorDirs can remove the empty mountpoint | ||
| // directories without hitting EBUSY/ENOTEMPTY. | ||
| defer func() { |
There was a problem hiding this comment.
🤖 should-fix 🟡 – This defer fires on any later error from Restore, including writeSandboxRecord failing after RestoreWorkload succeeded. At that point the guest is running and using the CSI mount, so this unmounts a volume that is in use. (The SandboxAssetsDir cleanup above has the same scope, but removing hardlinks under a running binary is harmless; an unmount isn't.)
Please limit it to failures up to and including the worker call, e.g. with a flag set once RestoreWorkload returns OK. Related, and smaller: when atelet's own context is cancelled, this unmounts right away while the worker may still be tearing down CH and virtiofsd.
There was a problem hiding this comment.
Done. It now unmounts only if RestoreWorkload returned an error while atelet was still waiting; workers return only after their own cleanup. Gap: a transport error still unmounts. Test: TestRestoreKeepsVolumesMountedWhileTheGuestMayUseThem.
| tMount := time.Now() | ||
| mountErr := s.mountExternalVolumes(ctx, actorUID, req.GetSpec().GetVolumes()) | ||
| dMount = time.Since(tMount) | ||
| if mountErr != nil { |
There was a problem hiding this comment.
🤖 nit 🟢 – mountExternalVolumes returns on the first failed mount, before the cleanup defer below is registered. Any volumes mounted before that one stay mounted, and the next restore's resetActorDirs hits the EBUSY this change is meant to avoid. unmountExternalVolumes already tolerates NotFound, so registering the defer before the mount call covers it.
There was a problem hiding this comment.
Done, with TestRestoreMountFailureUnmountsVolumesMountedBeforeIt.
| return fmt.Errorf("while calling worker RestoreWorkload: %w: %w", err, ctx.Err()) | ||
| } | ||
| switch status.Code(err) { | ||
| case codes.Unavailable, codes.Canceled, codes.DeadlineExceeded: |
There was a problem hiding this comment.
🤖 should-fix 🟡 – Treating worker Canceled/DeadlineExceeded as retryable is wider than the doc comment's "the worker rolled back the restore before the workload served any traffic". The worker's InternalServerUnaryInterceptor turns any error wrapping a child context.DeadlineExceeded into codes.DeadlineExceeded (via status.FromContextError). That covers internal timeouts at any step, in either worker, gVisor included. restoreGuestReadyError is the only code that actually checks whether the failure was before serving.
It also conflicts with the apierror.Unavailable contract from #2072 ("Never use it for a failure partway through"). I'd suggest mapping only codes.Unavailable. The microvm worker already returns it when draining, and could return it from cancelStartups too, if worker shutdown is the Canceled case you're after.
There was a problem hiding this comment.
Done. The reseed path still retries: apierror.FromError checks for an apierror first. cancelStartups is left for a follow-up.
| // Any other error passes through uncounted: errors from atelet's own steps | ||
| // (snapshot download, asset fetch) are not counted, and a context error means | ||
| // this attempt was cut short rather than that the restore failed. | ||
| func (s *Atelet) boundRetryableRestoreError(actorUID string, err error) error { |
There was a problem hiding this comment.
🤖 should-fix 🟡 – The count resets only on success, on giving up, and on Terminate. So it outlives the streak it is meant to measure:
- An actor fails
Unavailabletwice, then fails withInternal(or atelet fails before the worker call) and crashes. Its count stays at 2.crashActoronly releases the worker and doesn't call ateletTerminate, so nothing clears it. If the actor is later restored on this node, its first transient failure crashes it. - Entries for actors that move to another node are never removed.
Resetting on any worker result that isn't Unavailable fixes the first case. Keying by actor UID plus snapshot UUID would also help with the second. A test for "crash in between, then restore again" would cover it.
There was a problem hiding this comment.
Done. Success or any crashing error now resets it, under the actor lock; context errors don't. It's keyed to the snapshot UUID. Pre-lock errors don't reset it.
| } | ||
|
|
||
| // restoreGuestReadyError classifies a failure to resume the restored micro-VM or | ||
| // reach its kata-agent before the workload serves any traffic. Because the |
There was a problem hiding this comment.
🤖 nit 🟢 – "Before the workload serves any traffic" only holds for inbound traffic. After Resume, the guest runs with egress active for up to ~15s of dialing and reseeding, and when the reseed is what failed, it ran on CRNG state shared with its clones. The retry then replays from the same snapshot. Retrying is still the right call here, but please make the comment (and the one on workerRestoreError) say inbound, so the next reader doesn't assume the guest never ran.
There was a problem hiding this comment.
Done. Small correction: outbound TCP isn't live yet. atunnel closes it until tunnel.Activate, so only DNS and volume writes get through.
| if cleanupErr := s.tunnel.Deactivate(cleanupCtx, p.attribution()); cleanupErr != nil { | ||
| slog.WarnContext(cleanupCtx, "Failed to deactivate actor networking after Restore failure", slog.Any("err", cleanupErr)) | ||
| } | ||
| // Sweep any per-sandbox host-side mounts and state after the VMM and |
There was a problem hiding this comment.
🤖 should-fix 🟡 – cleanupSandboxState and os.RemoveAll(rootfsUpperDir) in the failure path are new behavior, and there is no test for them. The order is load-bearing: the RemoveAll is only safe because this defer runs after the background untar is drained and after the CH and virtiofsd kills (LIFO), and after cleanupSandboxState drops the merged overlays. If a future edit moves a defer above this one, the upper gets removed under a live untar or overlay. Please say that in this comment, and add a test that a failure after buildActorContainers leaves no upper dir and no bundle overlays behind.
There was a problem hiding this comment.
The comment now explains the order. TestCleanupFailedRestoreRootfs tests the helper, not the defer order.
73da250 to
1c02fb2
Compare
9f5bd3f to
9999c2f
Compare
|
Thanks, PTAL at |
| // (a later step failing), or when atelet stopped waiting first, the volumes | ||
| // stay mounted, and a retried Restore here then fails in resetActorDirs, | ||
| // crashing the actor. | ||
| unmountVolumesOnFailure := true |
There was a problem hiding this comment.
🤖 should-fix 🟡 – Please rebase first. On main, #2746 made Restore call resetActorDirsKeepingVolumes, which leaves the external volume mount points alone ("publishing it again is a no-op"). So a retried Restore no longer hits EBUSY on a still-mounted volume, and this defer no longer has a reason to exist.
I checked by merging main into this branch and disabling this defer. TestRestoreStopsRetryingAfterConsecutiveUnavailableFailures still passes, including the retry after a failed mount. Only the two "unmounted" assertions fail (TestRestoreUnavailableFailureUnmountsExternalVolumes, TestRestoreMountFailureUnmountsVolumesMountedBeforeIt).
Dropping it also removes the transport-error case, where atelet unmounts while the guest may still be live. I'd remove unmountVolumesOnFailure, this comment and those tests. If a crashed actor leaking a published volume is the concern, that's a separate change.
There was a problem hiding this comment.
Rebased onto main and removed unmountVolumesOnFailure, its comment, and the three external-volume unmount tests.
| if errors.Is(err, errGuestStopped) { | ||
| return fmt.Errorf("while %s: %w", step, err) | ||
| } | ||
| return apierror.Unavailable("while %s: %w", step, err) |
There was a problem hiding this comment.
🤖 should-fix 🟡 – apierror.Unavailable says "the request was not acted on ... Never use it for a failure partway through." Your doc comment above says the guest ran, so the code no longer keeps that guarantee. So either consider creating a new error such as apierror.Restored "Transient error has occurred and the state has been restored to make retrying safe", return an apierror.Internal error equivalent to the former or update the contract in internal/apierror/apierror.go to cover a rolled-back attempt, or the next reader will trust it.
Question on external volumes: durable-dir volumes are reset and re-untarred on the retry, but CSI volumes keep whatever the failed run wrote. The retry then resumes guest memory from before those writes, including the page cache and the app's in-memory view of the files. Can a workload get confused or corrupt data that way? If you've considered this and the window is acceptable, a line saying so would help. Otherwise, should Unavailable be limited to failures where nothing could have written yet?
There was a problem hiding this comment.
Updated apierror.Unavailable's doc comment in internal/apierror/apierror.go to cover a rolled-back attempt, and documented the CSI volume window on restoreGuestReadyError: the retry re-extracts the rootfs upper and durable-dir volumes from the snapshot, whereas a CSI volume is outside the snapshot and keeps any background writes made during the post-Resume dial/reseed window (before the wakeup probe or any inbound traffic connects) while guest memory and its page cache resume from the snapshot. We accept that narrow window over crashing the actor on a transient host stall.
| // again; restoring a different snapshot then starts the count over. The counts | ||
| // live in memory: an atelet restart starts every actor over, which only allows | ||
| // a few more retries. | ||
| type restoreFailureCounter struct { |
There was a problem hiding this comment.
🤖 nit 🟢 – This doc comment is long for a map of counters. The paths before the lock and the "actor that leaves this node" case could be one sentence: "Entries for actors that left this node stay until the actor is restored or terminated here again." The reset rules are already documented on endsRestoreFailureStreak.
There was a problem hiding this comment.
Done.
| }) | ||
| } | ||
|
|
||
| func TestBoundRetryableRestoreError(t *testing.T) { |
There was a problem hiding this comment.
🤖 nit 🟢 – TestRestoreStopsRetryingAfterConsecutiveUnavailableFailures already drives the same cases through Restore: giving up, the reset after giving up, the reset on success, on a crash and on a failed mount, and a canceled attempt. Only the per-snapshot keying is missing from it. I'd add that as one more step there, and drop this test and TestEndsRestoreFailureStreak, which restates the switch.
There was a problem hiding this comment.
Done. Added the second-snapshot step to TestRestoreStopsRetryingAfterConsecutiveUnavailableFailures and removed TestBoundRetryableRestoreError and TestEndsRestoreFailureStreak.
| func TestCleanupFailedRestoreRootfs(t *testing.T) { | ||
| t.Run("removes the rootfs upper dir", func(t *testing.T) { | ||
| dirs := failedRestoreRootfs(t, false) | ||
| (&AteWorkerService{}).cleanupFailedRestoreRootfs(t.Context(), "restore-cleanup-test", dirs) |
There was a problem hiding this comment.
🤖 nit 🟢 – cleanupFailedRestoreRootfs calls kata.CleanupSandboxState. When run as root, that detaches mounts and removes everything under /run/kata-containers/shared/sandboxes/<id> and /run/vc/vm/<id>. It also SIGKILLs any cloud-hypervisor or virtiofsd process whose command line contains the id. With a fixed id that touches real host paths. A fresh uuid.NewString() per test keeps it from matching anything real.
There was a problem hiding this comment.
Done.
…navailable When a micro-VM restore fails to resume the guest, reach the kata-agent, or reseed the guest CRNG before serving traffic, RestoreWorkload's defers tear down all worker-local state (VMM, virtiofsd, shared sandbox mounts, rootfs upper, bundle overlays, network, and hosted-actor slot). Report apierror.Unavailable for those pre-serving readiness failures, unmount any external volumes in Atelet.Restore on failure, and propagate worker Unavailable/timeout errors as apierror.Unavailable so ateapi keeps the actor in ACTOR_STATE_RESUMING and retries the restore instead of crashing the actor.
A restore that fails the same way every time would otherwise keep the actor RESUMING forever, visible only as 503s, where before it crashed once. - ateworker-microvm: a guest that stopped (errGuestStopped: cloud-hypervisor exited, e.g. a guest kernel panic after resume or a corrupt memory image) is reported as an error, not Unavailable. It is not host contention and would most likely stop the guest again on retry; coldBootActorRetrying bounds the same case. - atelet: count each actor's consecutive retryable (Unavailable) worker restore failures, and report the third as Internal so the control plane crashes the actor. A successful restore or a Terminate clears the count.
Address review feedback on the retryable restore path: - atelet maps only a worker codes.Unavailable to apierror.Unavailable. A worker-side Canceled or DeadlineExceeded can come from an internal timeout at any step of the restore, so it now crashes the actor as before. - The streak of consecutive retryable restore failures ends when Restore succeeds or fails with an error that crashes the actor, and records the snapshot it is for, so a count left by a restore that crashed the actor, or by another snapshot, no longer crashes the first transient failure of a later restore. An error that keeps the actor RESUMING (a context error, or another Unavailable) leaves the streak as is. The reset runs while the actor's lock is held. - The external-volume unmount on Restore failure is registered before mounting, so a mount failing partway through the list unmounts the ones mounted before it. Once the worker call starts, it unmounts only if RestoreWorkload returned an error while atelet was still waiting: after a successful worker call, or when atelet stopped waiting, the sandbox may still be using the volumes. - The worker's failure cleanup of the rootfs upper dir and bundle overlays moves into cleanupFailedRestoreRootfs, with a test (the overlay case is root-gated), and its defer documents why it must stay registered first. - restoreGuestReadyError says the restore is retryable before the workload served inbound traffic, and what the resumed guest could do until then: query DNS and write to its volumes, but not open outbound TCP connections.
…refine Unavailable contract Address follow-up review feedback after rebasing onto main (agent-substrate#2746): - Atelet.Restore now calls resetActorDirsKeepingVolumes (agent-substrate#2746), which leaves external volume mount points in place across retries, so remove unmountVolumesOnFailure and its unmount tests. - Update the apierror.Unavailable doc comment to cover an attempt whose state was rolled back so retrying is safe, and document the post-Resume CSI volume window on restoreGuestReadyError. - Shorten the restoreFailureCounter doc comment, fold the per-snapshot streak assertion into TestRestoreStopsRetryingAfterConsecutiveUnavailableFailures, and drop TestBoundRetryableRestoreError and TestEndsRestoreFailureStreak. - Use a fresh uuid.NewString() per subtest in TestCleanupFailedRestoreRootfs so running as root never touches a fixed sandbox path or matches a real process.
9999c2f to
b9733a0
Compare
|
Rebased onto |
Walter Fender (cheftako)
left a comment
Walter Fender (cheftako)
left a comment
There was a problem hiding this comment.
Looks good. Just a couple of items we can follow up with.
| // Unavailable reports that the request was not acted on, or that the attempt | ||
| // was rolled back so retrying it, here or elsewhere, is safe. Never use it for | ||
| // a failure that leaves partial state behind. |
There was a problem hiding this comment.
🤖 nit 🟢 – This still contradicts restoreGuestReadyError. That function documents that CSI volume writes from the failed attempt stay behind, which is partial state. Saying what "rolled back" covers would make the two agree. For example:
| // Unavailable reports that the request was not acted on, or that the attempt | |
| // was rolled back so retrying it, here or elsewhere, is safe. Never use it for | |
| // a failure that leaves partial state behind. | |
| // Unavailable reports that the request was not acted on, or that this server | |
| // rolled back the attempt, so retrying it, here or elsewhere, is safe. Never | |
| // use it for a failure that leaves partial state on this server. Effects | |
| // outside it, such as writes to external storage, may remain. |
| // or write to its filesystems. The retry re-extracts the rootfs upper and | ||
| // durable-dir volumes from the snapshot, whereas a CSI volume is outside the | ||
| // snapshot and keeps any writes made during that window while guest memory and | ||
| // its page cache resume from the snapshot; we accept that narrow window over | ||
| // crashing the actor on a transient host stall. When the reseed is what failed, | ||
| // the guest also ran on CRNG state shared with the snapshot's other clones. |
There was a problem hiding this comment.
🤖 nit 🟢 – That sentence piles up clauses around a semicolon. Also, "narrow" is generous: with reseedTimeout at 60s, the window can run to about 75s. Maybe:
| // or write to its filesystems. The retry re-extracts the rootfs upper and | |
| // durable-dir volumes from the snapshot, whereas a CSI volume is outside the | |
| // snapshot and keeps any writes made during that window while guest memory and | |
| // its page cache resume from the snapshot; we accept that narrow window over | |
| // crashing the actor on a transient host stall. When the reseed is what failed, | |
| // the guest also ran on CRNG state shared with the snapshot's other clones. | |
| // or write to its filesystems. The retry re-extracts the rootfs upper and | |
| // durable-dir volumes from the snapshot. A CSI volume is outside the snapshot, | |
| // so it keeps any writes from that window while guest memory and its page | |
| // cache resume from before them. That risk is accepted over crashing the actor | |
| // on a transient host stall. When the reseed is what failed, the guest also | |
| // ran on CRNG state shared with the snapshot's other clones. |
When a micro-VM restore in
restoreMemoryFidelityfails to resume the guest, reachkata-agent, or reseed the guest CRNG before serving any inbound traffic, the worker's deferred cleanup already stops cloud-hypervisor and virtiofsd and unhosts the actor. However,restoreMemoryFidelityreturned a plainfmt.Errorf(which the worker reports ascodes.Internal, orDeadlineExceededwhen the cause was a timeout) andAtelet.Restorewrapped all workerRestoreWorkloaderrors withfmt.Errorf(codes.Internal). Soateapi'shandleAteletErrortreated transient post-resume guest failures as fatal and moved the actor toACTOR_STATE_CRASHEDinstead of keepingACTOR_STATE_RESUMINGforActorResumerto retry.This PR:
apierror.Unavailabledoc comment ininternal/apierror/apierror.goto cover an attempt that was rolled back so retrying it is safe,restoreGuestReadyErrorincmd/ateworker-microvm/restore.go, so pre-serving guest readiness failures (client.Resume,dialAgentRetry,reseedGuestCRNG) reportapierror.Unavailablewhen the caller's context is still active. When it wrapsctx.Err(), it formats the cause with%v, so a childcontext.DeadlineExceededdoes not shadow a callercontext.Canceled. "Pre-serving" means before inbound traffic (not even the wakeup probe has connected yet). AfterResume, the guest and its workload do run until the agent is dialed and the CRNG reseeded: atunnel's egress proxy closes their outbound TCP connections untiltunnel.Activate, and the retry re-extracts the rootfs upper and durable-dir volumes from the snapshot, whereas any background writes made to a CSI volume during that window stay while guest memory and its page cache resume from the snapshot,restoreMemoryFidelityfailure cleanupdeferright afterPrepareEgress, so it runs last (after the untar drain and the cloud-hypervisor and virtiofsd kills). It now sweeps the sandbox state, removes the rootfs upper dir, and detaches the bundle rootfs overlays (cleanupFailedRestoreRootfs, mirroringteardownActor), so nothing stays mounted across a retry. Two side effects:buildActorContainersthroughstageMergedRootfs, which used to leave the bundle overlays mounted, now detach them too;teardownActor, the sweep removes/run/vc/vm/<id>, so a failed restore no longer leaves the guest's console, serial and virtiofsd logs on the node,workerRestoreErrorincmd/atelet/main.go(resolving theTODOinAtelet.Restore). It propagates a workercodes.Unavailable, which a transport error also arrives as, asapierror.Unavailable, soateapipreservesACTOR_STATE_RESUMINGandWorkerAssignmentfor retry. Other worker errors still crash the actor, including a worker-sideCanceledorDeadlineExceeded, which can come from any step,RESUMINGforever:errGuestStopped, e.g. a guest kernel panic after resume or a corrupt memory image) is reported as an error rather thanUnavailable(coldBootActorRetryinglikewise caps retries of that case);ateletreports an actor's third consecutive retryable worker failure to restore the same snapshot asInternal;Terminateends the streak, while an error that keeps the actorRESUMING(a context error, or anotherUnavailable) leaves it as is, andcmd/ateworker-microvm/restore_test.go,cmd/ateworker-microvm/restore_linux_test.go(the overlay case is root-gated), andcmd/atelet/lifecycle_test.go.How I tested it
go test -race ./internal/apierror/... ./cmd/ateworker-microvm/... ./cmd/atelet/...go buildandgo vetat each commit, andgo vet ./...on the headThe root-gated
TestCleanupFailedRestoreRootfsoverlay case, run in a user and mount namespace (unshare -r -m)./hack/verify/gofmt.shand./hack/verify/boilerplate.shTests pass
Appropriate changes to documentation are included in the PR