Crash actor when Ateom Checkpoint or Restore fails; Also when the error is not marked as retriable - #1220
Conversation
45b92f9 to
d74e54c
Compare
f13c469 to
7bf42da
Compare
515b756 to
267a268
Compare
…ever An ateom RPC failure the classifier cannot map to a known retriable reason previously left the actor retrying forever. atelet now wraps the Checkpoint boundary in CrashUnlessRetriable: any ateom.Checkpoint failure crashes the actor, except Unavailable transport errors, which stay retriable because an ateom restart can heal them.
The test expected the rendered counter manifest to contain "type: ExternalVolumeTemplate", but the Volume union has never carried a type discriminator field, and the API style guide now explicitly forbids one: the set member is the discriminator. The rendered manifest also parses strictly as an ActorTemplate, which would reject such an unknown field. Remove the stale expectation so the test matches what the template actually renders.
267a268 to
1dba017
Compare
Zoe Zhao (zoez7)
changed the title
| rc, mainErr := ategcs.Open(ctx, s.gcsClient, url) | ||
| if mainErr != nil { | ||
| return nil, fmt.Errorf("anonymous open failed (%v); main client open failed: %w", anonErr, mainErr) | ||
| return nil, fmt.Errorf("anonymous open failed (%w); main client open failed: %w", anonErr, mainErr) |
There was a problem hiding this comment.
Anon-client %w puts the wrong error in the unwrap chain. The %v→%w change here means a transient-tagged anonymous-leg failure can mask a definitive main-client failure: if anonymous egress to storage.googleapis.com is blocked (dial refused → *net.OpError → tagged ReasonTransientObjectStorage) while the main client gets a definitive 404 (ReasonFailedGetExternalObject), the combined error satisfies errors.Is(err, ReasonTransientObjectStorage) via the anon leg. run()/checkpoint()/restore() then MarkRetriable and return Unavailable, so the control plane retries a permanently missing asset forever — the exact infinite-retry pathology this PR exists to eliminate. errors.AsType[Reason] also reports the anon leg's reason first (DFS over multi-%w). Pre-PR, %v kept anonErr out of the chain; consider keeping it out, or ensuring the main-client error's reason wins.
| } | ||
| if regErr, ok := errors.AsType[*transport.Error](err); ok { | ||
| if regErr.Temporary() { | ||
| return fmt.Errorf("%w: %w", ateerrors.ReasonTransientImageRegistry, err) |
There was a problem hiding this comment.
transport.Error.Temporary() classifies on parsed JSON body codes, not the HTTP status. In go-containerregistry v0.21.7, when the body parses into Errors, Temporary() ignores StatusCode and requires every code ∈ {BLOB_UPLOAD_INVALID, TOOMANYREQUESTS, UNKNOWN, UNAVAILABLE}. Two failure modes:
- Artifact Registry returns 403 with
{"errors":[{"code":"DENIED"}]}during IAM/token propagation →Temporary()==false→ taggedReasonFailedGetExternalObject→ actor crashes even though a retry would succeed. (ggcr's own retry layer even retries 503s regardless of body, so a 503-with-DENIED-body is retried by the SDK and then declared "definitive" here.) - A 404 with body code
UNKNOWNis taggedReasonTransientImageRegistry→ infinite retry of a missing image.
Classify on StatusCode (like ategcs retryableHTTPStatus) instead.
| // reschedule the actor onto a different node when this happens. | ||
| return nil, fmt.Errorf("while resetting actor dirs: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
External-volume mount failures (just below, and in run() ~line 493) are neither transient-tagged nor MarkRetriable'd. A routine CSI ABORTED ("operation pending for volume", i.e. a NodeStage/NodePublish still in flight) or UNAVAILABLE (driver pod restarting) — codes the CSI spec defines as retry-me — now permanently crashes an actor whose snapshot is intact. The status survives the %w wraps (internal/volume/csi/plugin.go:195,213; cmd/atelet/volumes.go:47), and CrashUnlessRetriable's pass-through set is only Canceled/DeadlineExceeded/InvalidArgument/FailedPrecondition, so the crash directive is appended and ateapi's maybeCrashActor sets terminal ACTOR_STATE_CRASHED. Pre-PR the untagged error passed CrashIfReason unchanged and the workflow retried. Consider passing through ABORTED/UNAVAILABLE from the CSI plugin or marking these retriable.
| errors.Is(err, syscall.ECONNRESET) || | ||
| errors.Is(err, syscall.ECONNREFUSED) || | ||
| errors.Is(err, syscall.EPIPE) | ||
| } |
There was a problem hiding this comment.
The hand-rolled transient allowlist is narrower than the SDKs' own retry predicates. isTransientBackendErr here (and isNetworkErr in internal/imagecache/imagecache.go) misses fault classes the SDKs call retryable: TLS handshake timeout, url.Error-wrapped io.EOF, net.ErrClosed, temporary DNS errors, "http2: client connection lost". Example: Restore fetches the snapshot manifest over a reused connection the server closed → url.Error{Err: io.EOF} — not a *net.OpError, not ErrUnexpectedEOF, no errno — so tagTransientErr passes it through untagged and CrashUnlessRetriable crashes the actor on a one-shot blip. cloud.google.com/go/storage's exported ShouldRetry and ggcr's defaultRetryPredicate both retry these exact classes; GCS uploads without preconditions get no SDK-internal retry at all, making UploadPausedCheckpoint the most exposed path. Suggest delegating to storage.ShouldRetry rather than maintaining a narrower copy.
P.S. - storage retry on 429 has been added to #1416 for GCS only
| // workload RPC (never at dialAteom, which dials lazily). | ||
| func markAteomTransportRetriable(err error) error { | ||
| if status.Code(err) == codes.Unavailable { | ||
| return ateerrors.MarkRetriable(err) |
There was a problem hiding this comment.
codes.Unavailable does not imply "the workload never saw the request". gRPC also returns Unavailable when the connection dies after the handler ran: if ateom is OOM-killed during CheckpointWorkload after destroying the sandbox (ateom-gvisor checkpoints, then tears down before replying), grpc-go closes all active streams with Unavailable — atelet marks it retriable, and the control plane retries Checkpoint against a torn-down workload, which then fails unclassified and crashes as UNKNOWN with the real cause (interrupted checkpoint) invisible.
The reverse also holds: grpc-go's http2ErrConvTab maps RST_STREAM/stream-level breakage to codes.Internal, which now gets the crash directive even when the request was never processed — crashing a healthy actor during routine ateom restarts.
| // Not crashing the actor, because terminal errors here indicate problems with atelet, | ||
| // node or the disk itself. | ||
| if err := resetActorDirs(actorUID); err != nil { | ||
| // TODO: return an error code that signals the control plane to |
There was a problem hiding this comment.
Deleting the carve-out makes a node-local fault permanently destroy actors with intact snapshots. The removed comment ("Not crashing the actor, because terminal errors here indicate problems with atelet, node or the disk itself") was deliberate, and this TODO admits the correct behavior (reschedule to another node) isn't implemented. If one node's disk fills or turns read-only, every actor resumed onto it fails resetActorDirs before anything runs; the error reaches CrashUnlessRetriable, which returns DataLoss + ActorCrashedMetadata, and ateapi sets terminal ACTOR_STATE_CRASHED and releases the worker — actors whose snapshots are fully intact in GCS are permanently destroyed. Pre-PR they stayed retriable. Suggest keeping this path retriable (or non-crashing) until rescheduling exists.
| if errors.As(err, &grpcErr) { | ||
| st := grpcErr.GRPCStatus() | ||
| switch st.Code() { | ||
| case codes.Canceled, codes.DeadlineExceeded, |
There was a problem hiding this comment.
The InvalidArgument/FailedPrecondition pass-through can't distinguish request validation from downstream deterministic rejections. ateom-microvm/gvisor return InvalidArgument for deterministic spec-projection rejections and FailedPrecondition from checkpoint preconditions; restore() itself returns FailedPrecondition for a golden/actor sandbox-class mismatch after fetching manifests. errors.As finds the wrapped status ("while calling ateom.CheckpointWorkload: %w"), the boundary returns it uncrashed, maybeCrashActor doesn't crash, and every subsequent resume re-runs the identical deterministic failure — the fail-open pathology this PR's contract claims to remove. Fix at the right depth: run validateXRequest outside the crash-wrapped inner function instead of inferring "request-level" from codes.
| if ActorCrashRequested(err) { | ||
| return err | ||
| } | ||
| if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { |
There was a problem hiding this comment.
Checking the error chain instead of the RPC's own ctx lets library-internal timeouts escape unclassified. net/http's timeoutError implements Is(context.DeadlineExceeded) even while the RPC context is alive, so a storage-client HTTP transport timeout during UploadPausedCheckpoint passes through raw: tagTransientErr deliberately skips context-matching errors, the ReasonTransientObjectStorage check misses, and the control plane gets a bare DeadlineExceeded with neither a crash directive nor a TRANSIENT classification — exactly the ambiguous unclassified state this PR exists to eliminate. (TestCheckpointExternalUploadFailureCrashes pins a workaround at Checkpoint's persist leg only; every other timeout path lacks it.) Gating the pass-through on ctx.Err() != nil closes the hole in one place.
|
|
||
| func FetchLocalFileFromGCSWithZstd(ctx context.Context, client ObjectStorage, gsURL string, localFilePath string) (err error) { | ||
| func FetchLocalFileFromGCSWithZstd(ctx context.Context, client ObjectStorage, gsURL string, localFilePath string) error { | ||
| return tagTransientErr(fetchLocalFileFromGCSWithZstd(ctx, client, gsURL, localFilePath)) |
There was a problem hiding this comment.
Pre-existing inverted rc.Close defer in fetchLocalFileFromGCSWithZstd (~line 366) now destroys the transient tag this wrapper adds. The defer reads if err != nil { err = closeErr } — inverted relative to its two sibling defers: it overwrites a real, transient-classifiable download error with the close error, and on success drops closeErr. Example: a snapshot download dies mid-stream with ECONNRESET (would be tagged ReasonTransientObjectStorage); rc.Close() on the broken stream also errors with a non-classifiable error, the defer replaces the real error, tagTransientErr finds nothing to tag, and CrashUnlessRetriable crashes the actor as UNKNOWN for a network blip. Harmless before this PR (untagged == retriable); should be flipped to match the siblings: if err == nil { err = closeErr }.
| // Best-effort: the snapshot is already persisted, so failing (and thereby | ||
| // crashing) here would lose a healthy actor over cleanup. Run and Restore | ||
| // reset these dirs again before reusing them. | ||
| resetActorDirsBestEffort(ctx, actorUID) |
There was a problem hiding this comment.
If the actor never returns to this node, nothing else cleans the checkpoint-state dir. The justification "Run and Restore reset these dirs again before reuse" only holds when the actor comes back: the delete workflow skips Terminate when the worker assignment is already released (workflow_delete.go:115-119), and atelet has no actor-dir GC or startup sweep. So if resetActorDirs fails once here (e.g. EBUSY from a leaked mount), the multi-GB memory-ranges files persist on the hostPath for the node's lifetime — no metric, no retry. Accumulated per suspended actor this drives the node toward ENOSPC, which under the new regime permanently crashes other actors whose Restores hit the full disk. Consider a startup sweep or GC for orphaned actor dirs, plus a metric on cleanup failure.
3 participants
Fixes #292 When Ateom Checkpoint or Restore fails, the Actor will be marked as CRASHED.
Also crashes an actor for any state that we don't know for sure the Actor's data is not corrupted. (i.e. Fail-closed by default). If we later discover specific error classes that are safe to retry, we can explicitly exempt them via targeted transient error classification (similar to GCS / Image Registry transient error classification today).
A follow up to this: We should consider return a error code that signals "Pick another worker" if the current worker cannot be scheduled, but the actor snapshot themselves are intact, today the actor are stuck with retrying with the same worker.