From 5fcb62a0f45bee14364246da29c4aaef76d1b9b7 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Fri, 28 Aug 2026 16:20:00 +0100 Subject: [PATCH 01/12] solve the image build errror for big images Signed-off-by: kerthcet --- pkg/provider/errors.go | 13 +++++++-- pkg/provider/errors_test.go | 12 ++++++++ pkg/provider/modal/client.go | 16 +++++++++++ pkg/provider/modal/modal.go | 18 ++++++++---- pkg/provider/modal/modal_test.go | 48 ++++++++++++++++++++++++++++++++ 5 files changed, 98 insertions(+), 9 deletions(-) diff --git a/pkg/provider/errors.go b/pkg/provider/errors.go index 2b03579..305dec3 100644 --- a/pkg/provider/errors.go +++ b/pkg/provider/errors.go @@ -58,6 +58,13 @@ var ( // other Pods perfectly well, since the blocklist key carries no Pod, image or credential // identity. A failure that belongs to one request cannot be recorded against a candidate. ErrImagePull = errors.New("provider: cannot pull image") + // ErrImageBuild: the provider could not PRODUCE the image it was asked to run — + // distinct from ErrImagePull because pulling is only one of the ways it fails. + // + // REQUEST-scoped like ErrImagePull, and for the same reason: WHICH image to run belongs + // to the Pod, so it blocklists NOTHING — no other region, tier or provider produces an + // image this one just refused to produce. + ErrImageBuild = errors.New("provider: cannot build image") ) // ClassifyError maps a provision error to the BlockScope it should be blocklisted at, @@ -143,8 +150,8 @@ const ( // it is meaningful, because the next Pod asking for the same candidate would fail too. catCapacity // catRequest: the provider refused this request because of something about the REQUEST - // itself — today only an image it cannot pull. A decision, so a rejection, but it says - // nothing about the candidate and must not blocklist one. + // itself — today an image it cannot pull or cannot build. A decision, so a rejection, but + // it says nothing about the candidate and must not blocklist one. catRequest ) @@ -155,7 +162,7 @@ func categorize(err error) failureCategory { switch { case errors.Is(err, ErrAuth): return catAuth - case errors.Is(err, ErrImagePull): + case errors.Is(err, ErrImagePull), errors.Is(err, ErrImageBuild): return catRequest case errors.Is(err, ErrNoCapacity), errors.Is(err, ErrUnsupportedAccelerator), errors.Is(err, ErrQuota): diff --git a/pkg/provider/errors_test.go b/pkg/provider/errors_test.go index 330c854..32a5417 100644 --- a/pkg/provider/errors_test.go +++ b/pkg/provider/errors_test.go @@ -55,6 +55,17 @@ func TestClassifyError(t *testing.T) { fmt.Errorf("modal: unsupported image pull credential: %w", ErrImagePull), BlockScope{}, }, + // Same scope as a pull failure, and it must stay that way: the image is the Pod's, + // and a builder that refused this one has nothing to say about the candidate. The + // wrapped case is the shape Modal produces — the SDK's own verdict, then the label + // (see modal.sdkClient.buildImage) — and it is the one that regressed to DenyAll when the + // registry's "unauthorized" text was left to the heuristics. + {"image-build sentinel blocks nothing", ErrImageBuild, BlockScope{}}, + { + "wrapped image-build sentinel blocks nothing", + fmt.Errorf("modal: RemoteError: unauthorized: authentication required: %w", ErrImageBuild), + BlockScope{}, + }, {"wrapped sentinel", fmt.Errorf("provision failed: %w", ErrNoCapacity), capacityScope}, {"string unauthorized", fmt.Errorf("HTTP 401 unauthorized"), BlockScope{DenyAll: true}}, {"string quota", fmt.Errorf("account limit exceeded"), capacityScope}, @@ -116,6 +127,7 @@ func TestIsRejection(t *testing.T) { // decide, and a Pod whose credential it cannot use must fail with that reason rather // than retry it forever. Blocking and rejecting are separate questions. {"image-pull sentinel", ErrImagePull, true}, + {"image-build sentinel", ErrImageBuild, true}, // The failures this predicate exists for. {"deadline exceeded", context.DeadlineExceeded, false}, diff --git a/pkg/provider/modal/client.go b/pkg/provider/modal/client.go index 34d5414..9858d5b 100644 --- a/pkg/provider/modal/client.go +++ b/pkg/provider/modal/client.go @@ -143,6 +143,12 @@ func (c *sdkClient) CreateSandbox(ctx context.Context, spec SandboxSpec) (string if err != nil { return "", Credential{}, err } + // Built HERE rather than implicitly inside Sandboxes.Create, so a build failure can be + // told apart from a create failure and labelled; see buildImage. + image, err = c.buildImage(ctx, app, image) + if err != nil { + return "", Credential{}, err + } probe, err := modalProbe(spec.ReadinessProbe) if err != nil { @@ -226,6 +232,16 @@ func (c *sdkClient) imageFor(ctx context.Context, spec SandboxSpec) (*modal.Imag } } +// buildImage hydrates the image, which is where Modal pulls it into its own cache. +// EVERY failure is labelled ErrImageBuild. +func (c *sdkClient) buildImage(ctx context.Context, app *modal.App, image *modal.Image) (*modal.Image, error) { + built, err := image.Build(ctx, app, nil) + if err != nil { + return nil, fmt.Errorf("modal: image build: %w: %w", err, provider.ErrImageBuild) + } + return built, nil +} + // registrySecret builds a lazily-hydrated ephemeral Modal Secret for these values. // // Per sandbox, and cached nowhere: the credential belongs to the Pod, is re-read from the diff --git a/pkg/provider/modal/modal.go b/pkg/provider/modal/modal.go index b7f66b5..819a714 100644 --- a/pkg/provider/modal/modal.go +++ b/pkg/provider/modal/modal.go @@ -66,6 +66,11 @@ import ( // different ceiling sets spec.activeDeadlineSeconds, which maps straight through. const defaultSandboxTimeout = 24 * time.Hour +// provisionTimeout overrides the vnode handler's generic 90s Provision deadline, because +// Provision here BLOCKS on the image build (see sdkClient.buildImage): a cold multi-GB +// image can take minutes to pull into Modal's cache. +const provisionTimeout = 5 * time.Minute + // compile-time assertions that Provider satisfies the interfaces. LogStreamer and // Executor are the optional halves: they are what make `kubectl logs` and `kubectl exec` // work here, and asserting them separately is the point — a provider is free to serve @@ -323,12 +328,13 @@ func (p *Provider) ExpandRegions(declared []string) []string { // trait is set the way it is. func (p *Provider) Capabilities() provider.Capabilities { return provider.Capabilities{ - SupportsStop: false, // create/terminate only - SupportsSpot: false, // no user-facing preemptible tier - SupportsEgressPolicy: true, // outbound allowlists on the sandbox itself - NativeTags: true, // sandbox tags carry identity - PreemptionNotice: 0, // no push; poll-based detection - PollInterval: 0, // OnDemand-only (never preempts) → the default cadence is fine + SupportsStop: false, // create/terminate only + SupportsSpot: false, // no user-facing preemptible tier + SupportsEgressPolicy: true, // outbound allowlists on the sandbox itself + NativeTags: true, // sandbox tags carry identity + PreemptionNotice: 0, // no push; poll-based detection + PollInterval: 0, // OnDemand-only (never preempts) → the default cadence is fine + ProvisionTimeout: provisionTimeout, // Provision blocks on the image build } } diff --git a/pkg/provider/modal/modal_test.go b/pkg/provider/modal/modal_test.go index d3c65b9..b4cec77 100644 --- a/pkg/provider/modal/modal_test.go +++ b/pkg/provider/modal/modal_test.go @@ -465,6 +465,13 @@ func TestCapabilities(t *testing.T) { if caps.SupportsStop || caps.SupportsSpot || caps.PreemptionNotice != 0 || !caps.NativeTags { t.Fatalf("unexpected caps: %+v", caps) } + // Must be stated, and must be well above the handler's generic 90s: Provision blocks on + // the image build here, so at the default a cold multi-GB image never finishes pulling + // and the Pod dies on a deadline instead of a verdict. Zero would silently restore that. + if caps.ProvisionTimeout < 5*time.Minute { + t.Fatalf("ProvisionTimeout = %v, want at least 5m to cover a cold image build", + caps.ProvisionTimeout) + } if p.Name() != provider.ProviderModal { t.Fatalf("name = %q", p.Name()) } @@ -947,6 +954,47 @@ func TestClassifyProvisionError_ConfinesToFailingRegion(t *testing.T) { } } +// TestClassifyProvisionError_ImageBuildNeverDeniesTheProvider is the regression test for the +// worst blast radius this adapter can produce. Modal relays the REGISTRY's words in a build +// failure, and a private registry's 401 says "unauthorized" — the same word the shared auth +// heuristic promotes to DenyAll, which is meant for OUR workspace credentials being wrong. +// Unlabelled, one Pod naming an image it cannot pull would fence off the entire provider for +// the blocklist TTL, failing over every other workload for something none of them did. +func TestClassifyProvisionError_ImageBuildNeverDeniesTheProvider(t *testing.T) { + p := newTestProvider(&fakeClient{}) + + // The shape buildImage produces: Modal's verdict, carrying the registry's text, wrapped + // with the sentinel. Spelled as the verdict's rendered text rather than the SDK type, to + // keep this file SDK-free — under test is the label, not how buildImage obtained one. + verdict := errors.New("RemoteError: Image build for im-1 failed with the exception:\n" + + "unauthorized: authentication required") + err := fmt.Errorf("modal: image build: %w: %w", verdict, provider.ErrImageBuild) + + // Blocks nothing at all: the image is a property of the request, and no region or tier + // builds an image Modal refused to build. + if got := p.ClassifyProvisionError(err, "H100:1", "us-east"); got != (provider.BlockScope{}) { + t.Fatalf("an image build failure must block nothing, got %+v", got) + } + // Still a rejection, so the Pod fails with the reason instead of retrying an image that + // will never build. Blocking and rejecting are separate questions. + if !provider.IsRejection(err) { + t.Error("an image build failure must be a rejection") + } + // The registry's text survives for whoever has to fix the Secret. + if !strings.Contains(err.Error(), "authentication required") { + t.Errorf("err = %q, want the registry's reason preserved", err) + } + // Named for what actually failed. A build fails on more than pulls — a layer it cannot + // unpack, a manifest it rejects — so ErrImagePull would assert a pull problem the + // adapter has not established, and send whoever reads it to check a credential. + if !errors.Is(err, provider.ErrImageBuild) { + t.Errorf("err must identify as ErrImageBuild, got %v", err) + } + if errors.Is(err, provider.ErrImagePull) { + t.Error("a build failure must not claim to be a pull failure") + } +} + func TestProvision_CarriesDeclaredPorts(t *testing.T) { for _, tc := range []struct { name string From f79ff703518b33f2883f764c65b5c4bd8e4c7a22 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Fri, 28 Aug 2026 17:14:41 +0100 Subject: [PATCH 02/12] change the provision timeout Signed-off-by: kerthcet --- pkg/provider/modal/modal.go | 8 ++++---- pkg/provider/modal/modal_test.go | 10 +++++----- pkg/vnode/handler.go | 12 ++++++++---- 3 files changed, 17 insertions(+), 13 deletions(-) diff --git a/pkg/provider/modal/modal.go b/pkg/provider/modal/modal.go index 819a714..f3b092b 100644 --- a/pkg/provider/modal/modal.go +++ b/pkg/provider/modal/modal.go @@ -66,10 +66,10 @@ import ( // different ceiling sets spec.activeDeadlineSeconds, which maps straight through. const defaultSandboxTimeout = 24 * time.Hour -// provisionTimeout overrides the vnode handler's generic 90s Provision deadline, because -// Provision here BLOCKS on the image build (see sdkClient.buildImage): a cold multi-GB -// image can take minutes to pull into Modal's cache. -const provisionTimeout = 5 * time.Minute +// provisionTimeout raises the vnode handler's generic Provision deadline, because Provision +// here BLOCKS on the image build (see sdkClient.buildImage): a cold image is pulled into +// Modal's cache on this call, and the create leg only gets what the build leaves it. +const provisionTimeout = 90 * time.Second // compile-time assertions that Provider satisfies the interfaces. LogStreamer and // Executor are the optional halves: they are what make `kubectl logs` and `kubectl exec` diff --git a/pkg/provider/modal/modal_test.go b/pkg/provider/modal/modal_test.go index b4cec77..7bde94a 100644 --- a/pkg/provider/modal/modal_test.go +++ b/pkg/provider/modal/modal_test.go @@ -465,11 +465,11 @@ func TestCapabilities(t *testing.T) { if caps.SupportsStop || caps.SupportsSpot || caps.PreemptionNotice != 0 || !caps.NativeTags { t.Fatalf("unexpected caps: %+v", caps) } - // Must be stated, and must be well above the handler's generic 90s: Provision blocks on - // the image build here, so at the default a cold multi-GB image never finishes pulling - // and the Pod dies on a deadline instead of a verdict. Zero would silently restore that. - if caps.ProvisionTimeout < 5*time.Minute { - t.Fatalf("ProvisionTimeout = %v, want at least 5m to cover a cold image build", + // Must be STATED, and above the handler's generic default: Provision blocks on the image + // build here, so this adapter needs more than a provider whose create just returns an id. + // Zero is the regression to guard — it silently reverts to that default. + if caps.ProvisionTimeout < 90*time.Second { + t.Fatalf("ProvisionTimeout = %v, want at least 90s to cover an image build", caps.ProvisionTimeout) } if p.Name() != provider.ProviderModal { diff --git a/pkg/vnode/handler.go b/pkg/vnode/handler.go index 6700687..266c490 100644 --- a/pkg/vnode/handler.go +++ b/pkg/vnode/handler.go @@ -65,10 +65,14 @@ type Blocklister interface { } // defaultProvisionTimeout bounds one Provision call, so a hung backend cannot pin a -// pod-controller worker forever. Deliberately generous — a backstop, not a tuning -// knob. A provider that needs longer (AWS sweeping zones) raises it via -// Capabilities.ProvisionTimeout. -const defaultProvisionTimeout = 90 * time.Second +// pod-controller worker forever. A backstop, not a tuning knob: it is what a provider gets +// when it declares nothing, and an API that accepts a create takes seconds, not minutes. +// Short on purpose — the cost of waiting is a pinned worker and a Pod learning nothing, +// while the cost of giving up early is one retry of an idempotent call. +// +// A provider that genuinely needs longer raises it via Capabilities.ProvisionTimeout: AWS +// sweeps a region's zones, Modal blocks on an image build. +const defaultProvisionTimeout = 30 * time.Second // Handler bridges one provider into the virtual kubelet: CreatePod provisions an // external instance, DeletePod terminates it. This is the "VK owns provisioning" From b70e89d80c48d2502362cb57cb759467a6ce2a99 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Fri, 28 Aug 2026 20:06:45 +0100 Subject: [PATCH 03/12] error handling Signed-off-by: kerthcet --- pkg/provider/errors.go | 29 +++++++++++------------------ pkg/provider/errors_test.go | 22 ++++++++++++++++++++++ pkg/provider/modal/client.go | 30 +++--------------------------- 3 files changed, 36 insertions(+), 45 deletions(-) diff --git a/pkg/provider/errors.go b/pkg/provider/errors.go index 305dec3..f4c00f6 100644 --- a/pkg/provider/errors.go +++ b/pkg/provider/errors.go @@ -155,10 +155,16 @@ const ( catRequest ) -// categorize buckets a provision error, sentinels first and string heuristics after. +// categorize buckets a provision error: cancellation first, then sentinels, then string +// heuristics. func categorize(err error) failureCategory { - // Sentinels first. An adapter that wrapped one has made an explicit decision, and - // it outranks anything the raw message text happens to contain. + msg := strings.ToLower(err.Error()) + + if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) || + containsAny(msg, "context deadline exceeded", "context canceled") { + return catUnattributable + } + switch { case errors.Is(err, ErrAuth): return catAuth @@ -169,23 +175,10 @@ func categorize(err error) failureCategory { return catCapacity } - msg := strings.ToLower(err.Error()) - - // Transport and timeout markers are checked BEFORE the category heuristics, - // because they are the failures those heuristics most reliably MISREAD: a gRPC - // status renders as "rpc error: code = Unavailable desc = ...", whose - // "unavailable" would otherwise match the capacity bucket below and turn "we could - // not reach the provider" into "the provider has no capacity" — the exact - // misattribution IsRejection exists to prevent. A deadline is unattributable for a - // second reason too: our own ProvisionTimeout can fire on a call the provider went - // on to honour, so the instance may well exist. - switch { - case errors.Is(err, context.DeadlineExceeded), errors.Is(err, context.Canceled): - return catUnattributable - case containsAny(msg, + if containsAny(msg, "rpc error", "connection refused", "connection reset", "broken pipe", "no such host", "i/o timeout", "eof", "tls handshake", - "service unavailable", "bad gateway", "gateway timeout", "internal server error"): + "service unavailable", "bad gateway", "gateway timeout", "internal server error") { return catUnattributable } diff --git a/pkg/provider/errors_test.go b/pkg/provider/errors_test.go index 32a5417..58de33e 100644 --- a/pkg/provider/errors_test.go +++ b/pkg/provider/errors_test.go @@ -145,6 +145,28 @@ func TestIsRejection(t *testing.T) { // adapter that classified a gRPC error itself is not second-guessed. {"grpc unavailable wrapping a sentinel", fmt.Errorf("rpc error: code = Unavailable desc = no gpu: %w", ErrNoCapacity), true}, + + // OUR clock is the one thing a sentinel does NOT win over. An adapter labels the + // failure it saw and cannot see whose deadline fired, so a ProvisionTimeout mid-call + // must stay retryable — failing the Pod here would reap the attempt that was warming + // the provider's cache for the retry (see modal.sdkClient.buildImage). + {"deadline wrapped in an image-build label", + fmt.Errorf("modal: image build: %w: %w", context.DeadlineExceeded, ErrImageBuild), false}, + {"cancellation wrapped in a capacity label", + fmt.Errorf("sweep: %w: %w", context.Canceled, ErrNoCapacity), false}, + // The form errors.Is CANNOT see: grpc-go turns a dead context into a status error + // that wraps nothing, and it is the likelier arrival — an SDK blocked in Recv finds + // out from gRPC, not from its own ctx.Err() poll. + {"grpc deadline wrapping an image-build label", + fmt.Errorf("modal: image build: %w: %w", + errors.New("rpc error: code = DeadlineExceeded desc = context deadline exceeded"), + ErrImageBuild), false}, + // A verdict Modal actually reached still rejects: the label is only overridden when + // the context is what died. + {"image-build label on a remote verdict", + fmt.Errorf("modal: image build: %w: %w", + errors.New("Image build for im-1 failed with the exception:\nunauthorized"), + ErrImageBuild), true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/provider/modal/client.go b/pkg/provider/modal/client.go index 9858d5b..ea65991 100644 --- a/pkg/provider/modal/client.go +++ b/pkg/provider/modal/client.go @@ -190,7 +190,7 @@ func (c *sdkClient) CreateSandbox(ctx context.Context, spec SandboxSpec) (string if err != nil { return "", Credential{}, err } - return sb.SandboxID, c.mintCredential(ctx, sb, spec), nil + return sb.SandboxID, c.mintCredential(ctx, sb, firstPort(spec.Ports)), nil } // imageFor resolves the sandbox's image, attaching pull credentials when the spec carries @@ -233,7 +233,6 @@ func (c *sdkClient) imageFor(ctx context.Context, spec SandboxSpec) (*modal.Imag } // buildImage hydrates the image, which is where Modal pulls it into its own cache. -// EVERY failure is labelled ErrImageBuild. func (c *sdkClient) buildImage(ctx context.Context, app *modal.App, image *modal.Image) (*modal.Image, error) { built, err := image.Build(ctx, app, nil) if err != nil { @@ -259,34 +258,11 @@ func (c *sdkClient) registrySecret(ctx context.Context, kv map[string]string) (* // would hand over every workload's token. Not in memory, which is not durable. A // credential belongs in an access-controlled Secret, and this layer has no cluster // access, so it hands the pair up to the virtual kubelet, which writes it. -// -// Minting is one-shot: every CreateConnectToken call mints a FRESH token, with no -// read-back. A caller that drops the return value has lost it for the sandbox's life. -// That is also why this cannot move to the read path — observe would hand out a token -// that changed every tick. (The endpoint lives on the Pod annotation instead, so Modal -// reports no observed endpoint at all; see observe.) -// -// It can run this early because the RPC needs only the sandbox id and port — no task id, -// no running container, no booted GPU (contrast Tunnels, which needs the container up). -// So the credential is in hand while the sandbox is still queued. -// -// Every workload gets one: an authenticated URL is the only general way to reach a -// NeoCloud instance, and a workload with nothing to serve just leaves it unused. The URL -// routes to the first of spec.Ports, or Modal's default 8080 if none are declared. -// -// TODO: a Sandbox is reached by identity (`kubectl exec sbx-alice`), not by address, so it -// should not get a credential — it does today because it arrives here as an ordinary Pod. -// -// Best-effort: a sandbox that exists must be reported and reclaimed whether or not it got -// a credential, so a failure returns the zero Credential and the instance is simply -// unreachable. Returning the error would fail a Provision whose sandbox is already -// running, leaking a paid instance to save an address. The text is dropped rather than -// logged because it can echo the request. -func (c *sdkClient) mintCredential(ctx context.Context, sb *modal.Sandbox, spec SandboxSpec) Credential { +func (c *sdkClient) mintCredential(ctx context.Context, sb *modal.Sandbox, port int) Credential { creds, err := sb.CreateConnectToken(ctx, &modal.SandboxCreateConnectTokenParams{ // Derived from the exposed set rather than carried separately, so the routed // port cannot name one the sandbox was never told to accept traffic on. - Port: firstPort(spec.Ports), + Port: port, }) if err != nil || creds == nil || creds.Token == "" { return Credential{} From 6580af2c07e32bc06fc27d6c3f1db4e46813c364 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Fri, 28 Aug 2026 21:43:01 +0100 Subject: [PATCH 04/12] support to create credential for second retry Signed-off-by: kerthcet --- pkg/provider/modal/client.go | 17 ++++++ pkg/provider/modal/modal.go | 41 +++++++++---- pkg/provider/modal/modal_test.go | 99 +++++++++++++++++++++++++++++--- 3 files changed, 137 insertions(+), 20 deletions(-) diff --git a/pkg/provider/modal/client.go b/pkg/provider/modal/client.go index ea65991..390dc50 100644 --- a/pkg/provider/modal/client.go +++ b/pkg/provider/modal/client.go @@ -270,6 +270,23 @@ func (c *sdkClient) mintCredential(ctx context.Context, sb *modal.Sandbox, port return Credential{URL: creds.URL, Token: creds.Token} } +// MintConnectCredential implements Client. FromID attaches to a live sandbox and creates +// nothing, which is what lets a credential be minted for one this process never created. +// +// Unlike the create path it ERRORS on an empty credential: this call exists only to produce +// one, so nothing downstream can do anything useful with a zero value. +func (c *sdkClient) MintConnectCredential(ctx context.Context, id string, port int) (Credential, error) { + sb, err := c.mc.Sandboxes.FromID(ctx, id, nil) + if err != nil { + return Credential{}, fmt.Errorf("modal: attach sandbox %s: %w", id, err) + } + cred := c.mintCredential(ctx, sb, port) + if cred.Token == "" { + return Credential{}, fmt.Errorf("modal: sandbox %s: no connect credential minted", id) + } + return cred, nil +} + // modalProbe maps a Pod readinessProbe onto Modal's Probe. Modal supports only // TCP and Exec probes, so an HTTPGet probe degrades to a TCP probe on its port // (readiness ≈ the port accepting connections). Returns (nil, nil) when p is nil diff --git a/pkg/provider/modal/modal.go b/pkg/provider/modal/modal.go index f3b092b..4df6098 100644 --- a/pkg/provider/modal/modal.go +++ b/pkg/provider/modal/modal.go @@ -86,11 +86,14 @@ var ( // real implementation (Modal SDK/HTTP) and a fake (tests) are interchangeable. type Client interface { // CreateSandbox launches one sandbox from spec and returns its Modal id plus the - // connect credential minted for it. The credential is returned HERE and nowhere - // else — minting is one-shot and there is no read-back, so a caller that drops it - // has lost it for the sandbox's life. Zero when none could be minted; see - // sdkClient.mintCredential. + // connect credential minted for it. Minting is one-shot and there is no read-back, so + // a caller that drops the credential can only get another from MintConnectCredential. + // Zero when none could be minted; see sdkClient.mintCredential. CreateSandbox(ctx context.Context, spec SandboxSpec) (id string, cred Credential, err error) + // MintConnectCredential mints a NEW credential for a sandbox that already exists, + // which Modal allows from the id alone. Every call returns a different token and none + // can be revoked, so it is only safe where nothing holds the previous one. + MintConnectCredential(ctx context.Context, id string, port int) (Credential, error) // TerminateSandbox terminates a sandbox by id. Must be idempotent: // terminating an already-gone sandbox returns nil. TerminateSandbox(ctx context.Context, id string) error @@ -366,18 +369,34 @@ func (p *Provider) Provision( // the poll loop reports Running has capacity, one still queued does not. That is more // information than a create can return, so report it rather than a flat false. // - // It carries NO credential, per the interface contract: the original cannot be - // re-read, and minting a second would hand the consumer a token that changed on every - // retry. That leaves a real gap — if the first create succeeded but its credential - // never reached a Secret, nothing recovers it. Closing it needs cluster access this - // layer does not have. + // Its credential is minted anew, but ONLY when the Pod carries no endpoint: that + // annotation and the credential Secret are written in the same pass (see + // vnode.Handler.CreatePod), so no endpoint means the first token reached nobody and the + // sandbox is unreachable. Where one exists, minting would strand the consumer holding + // it — a new token revokes nothing and the old one keeps working. if existing, err := p.findByClaim(ctx, req.ClaimName); err != nil { return provider.ProvisionResult{}, err } else if existing != nil { - return provider.ProvisionResult{ + res := provider.ProvisionResult{ InstanceID: existing.ID, Reserved: existing.State == provider.InstanceRunning, - }, nil + } + if pod.Annotations[nebulav1alpha1.EndpointAnnotation] == "" { + port := 0 + if len(pod.Spec.Containers) > 0 { + port = firstPort(containerPorts(&pod.Spec.Containers[0])) + } + cred, err := p.client.MintConnectCredential(ctx, existing.ID, port) + if err != nil { + // Fail the whole provision rather than report an unreachable sandbox as + // provisioned. The error is not a rejection, so the Pod stays Provisioning + // and VK retries; the retry adopts this same sandbox by its claim tag and + // mints again, which is safe precisely because no endpoint was published. + return provider.ProvisionResult{}, err + } + res.ConnectURL, res.ConnectToken = cred.URL, cred.Token + } + return res, nil } spec, err := p.sandboxSpecFromPod(pod, req) diff --git a/pkg/provider/modal/modal_test.go b/pkg/provider/modal/modal_test.go index 7bde94a..5ebb1d5 100644 --- a/pkg/provider/modal/modal_test.go +++ b/pkg/provider/modal/modal_test.go @@ -49,6 +49,15 @@ type fakeClient struct { cred Credential // credential CreateSandbox returns; zero = none minted terminated []string + // The mint path: what MintConnectCredential returns, and what it was asked for. + // mintCnt is the assertion that matters most — minting when a token is already held + // is the one mistake nothing can undo. + mintCred Credential + mintErr error + mintCnt int + mintID string + mintPort int + // logs is the stream SandboxLogs hands back, and logsFor records the id it was // asked for — the one thing the adapter decides on that path. logs string @@ -76,6 +85,15 @@ func (f *fakeClient) CreateSandbox(_ context.Context, spec SandboxSpec) (string, return id, f.cred, nil } +func (f *fakeClient) MintConnectCredential(_ context.Context, id string, port int) (Credential, error) { + f.mintCnt++ + f.mintID, f.mintPort = id, port + if f.mintErr != nil { + return Credential{}, f.mintErr + } + return f.mintCred, nil +} + func (f *fakeClient) TerminateSandbox(_ context.Context, id string) error { f.terminated = append(f.terminated, id) return nil @@ -1092,34 +1110,97 @@ func TestProvision_NoCredentialWhenNoneMinted(t *testing.T) { } } -// An idempotent re-Provision carries NO credential. The original was minted once and -// cannot be re-read, and minting a second one here would hand the consumer a token -// that changes on every retry. -func TestProvision_IdempotentReturnsNoCredential(t *testing.T) { - f := &fakeClient{ +// adoptable returns a client holding one Running sandbox tagged for claim-a, so a +// Provision for that claim takes the idempotent branch instead of creating. +func adoptable() *fakeClient { + return &fakeClient{ sandboxes: []Sandbox{{ ID: "sb-existing", Tags: map[string]string{ClaimTagKey: "claim-a"}, Status: statusRunning, }}, - cred: Credential{URL: "https://x.modal.host", Token: "tok-abc"}, + cred: Credential{URL: "https://created.modal.host", Token: "tok-created"}, + mintCred: Credential{URL: "https://minted.modal.host", Token: "tok-minted"}, } +} + +// An adopted sandbox whose Pod ALREADY carries an endpoint carries no credential: a token +// was published for it, and minting a second would leave the consumer holding one the +// Secret no longer matches, with the original still valid and unrevokable. +func TestProvision_IdempotentKeepsPublishedCredential(t *testing.T) { + f := adoptable() p := newTestProvider(f) - res, err := p.Provision(context.Background(), gpuPod("claim-a", "H100", 1), - provider.ProvisionRequest{ClaimName: "claim-a"}) + pod := gpuPod("claim-a", "H100", 1) + pod.Annotations = map[string]string{ + nebulav1alpha1.EndpointAnnotation: "https://published.modal.host", + } + + res, err := p.Provision(context.Background(), pod, provider.ProvisionRequest{ClaimName: "claim-a"}) if err != nil { t.Fatalf("Provision: %v", err) } if res.InstanceID != "sb-existing" { t.Fatalf("InstanceID = %q, want sb-existing", res.InstanceID) } + if f.mintCnt != 0 { + t.Fatalf("minted %d credentials for a Pod that already has an endpoint; want 0", f.mintCnt) + } if res.ConnectURL != "" || res.ConnectToken != "" { - t.Fatalf("an adopted sandbox must carry no credential, got url=%q token set=%t", + t.Fatalf("an adopted sandbox with a published endpoint must carry no credential, got url=%q token set=%t", res.ConnectURL, res.ConnectToken != "") } } +// The recovery case: the first attempt created the sandbox but never published its +// address, so nothing holds the original token and the adopted sandbox is unreachable. +// It gets a fresh credential, routed to the port the POD declares. +func TestProvision_IdempotentMintsWhenNoEndpointPublished(t *testing.T) { + f := adoptable() + p := newTestProvider(f) + + pod := gpuPod("claim-a", "H100", 1) // no endpoint annotation + pod.Spec.Containers[0].Ports = []corev1.ContainerPort{{ContainerPort: 9000}} + + res, err := p.Provision(context.Background(), pod, provider.ProvisionRequest{ClaimName: "claim-a"}) + if err != nil { + t.Fatalf("Provision: %v", err) + } + if f.createCnt != 0 { + t.Fatalf("created %d sandboxes; recovery must adopt, not create", f.createCnt) + } + if f.mintCnt != 1 || f.mintID != "sb-existing" { + t.Fatalf("mint calls = %d for id %q, want 1 for sb-existing", f.mintCnt, f.mintID) + } + if f.mintPort != 9000 { + t.Fatalf("minted for port %d, want the Pod's declared 9000", f.mintPort) + } + if res.ConnectURL != "https://minted.modal.host" || res.ConnectToken != "tok-minted" { + t.Fatalf("credential = %q/%q, want the freshly minted pair", res.ConnectURL, res.ConnectToken) + } +} + +// A failed mint FAILS the Provision, rather than reporting an unreachable sandbox as +// provisioned. It is not a rejection, so the handler leaves the Pod provisioning and VK +// retries — and the retry adopts this same sandbox and mints again. +func TestProvision_IdempotentFailsWhenMintFails(t *testing.T) { + f := adoptable() + f.mintErr = errors.New("boom") + p := newTestProvider(f) + + res, err := p.Provision(context.Background(), gpuPod("claim-a", "H100", 1), + provider.ProvisionRequest{ClaimName: "claim-a"}) + if err == nil { + t.Fatal("Provision succeeded; a sandbox that could not be given a credential is unreachable") + } + if res.InstanceID != "" { + t.Fatalf("InstanceID = %q, want empty so nothing reads a half-provisioned result", res.InstanceID) + } + if provider.IsRejection(err) { + t.Fatal("a mint failure must not be a rejection; it would fail the Pod and blocklist the provider") + } +} + // Modal reports NO observed endpoint. Its address is the connect URL, published from // the create path onto the Pod's annotation, where it persists; re-deriving it per tick // would be a round trip for a value the API server already holds. The alternative — From 34c090421865545f7332bfb263f69a6f6007a3b1 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Sat, 29 Aug 2026 15:33:34 +0100 Subject: [PATCH 05/12] add log Signed-off-by: kerthcet --- pkg/provider/modal/client.go | 33 +++++++++++++++++++------------- pkg/provider/modal/modal.go | 4 +++- pkg/provider/modal/modal_test.go | 25 +++++++++++++----------- 3 files changed, 37 insertions(+), 25 deletions(-) diff --git a/pkg/provider/modal/client.go b/pkg/provider/modal/client.go index 390dc50..b652b08 100644 --- a/pkg/provider/modal/client.go +++ b/pkg/provider/modal/client.go @@ -190,7 +190,11 @@ func (c *sdkClient) CreateSandbox(ctx context.Context, spec SandboxSpec) (string if err != nil { return "", Credential{}, err } - return sb.SandboxID, c.mintCredential(ctx, sb, firstPort(spec.Ports)), nil + cred, err := c.mintCredential(ctx, sb, firstPort(spec.Ports)) + if err != nil { + return "", Credential{}, err + } + return sb.SandboxID, cred, nil } // imageFor resolves the sandbox's image, attaching pull credentials when the spec carries @@ -258,33 +262,36 @@ func (c *sdkClient) registrySecret(ctx context.Context, kv map[string]string) (* // would hand over every workload's token. Not in memory, which is not durable. A // credential belongs in an access-controlled Secret, and this layer has no cluster // access, so it hands the pair up to the virtual kubelet, which writes it. -func (c *sdkClient) mintCredential(ctx context.Context, sb *modal.Sandbox, port int) Credential { +// +// A failure is REPORTED, never swallowed into a zero credential. Minting is one-shot with +// no read-back, so a dropped error loses the credential of a sandbox that exists and is +// billing — silently, since a caller handed an empty pair has nothing to log. +func (c *sdkClient) mintCredential(ctx context.Context, sb *modal.Sandbox, port int) (Credential, error) { creds, err := sb.CreateConnectToken(ctx, &modal.SandboxCreateConnectTokenParams{ // Derived from the exposed set rather than carried separately, so the routed // port cannot name one the sandbox was never told to accept traffic on. Port: port, }) - if err != nil || creds == nil || creds.Token == "" { - return Credential{} + if err != nil { + return Credential{}, fmt.Errorf("modal: mint connect credential for sandbox %s on port %d: %w", + sb.SandboxID, port, err) + } + // A token-less success is the same outcome as an error — an address with nothing to + // authenticate against it — so it is reported as one. + if creds == nil || creds.Token == "" { + return Credential{}, fmt.Errorf("modal: sandbox %s: connect credential minted without a token", sb.SandboxID) } - return Credential{URL: creds.URL, Token: creds.Token} + return Credential{URL: creds.URL, Token: creds.Token}, nil } // MintConnectCredential implements Client. FromID attaches to a live sandbox and creates // nothing, which is what lets a credential be minted for one this process never created. -// -// Unlike the create path it ERRORS on an empty credential: this call exists only to produce -// one, so nothing downstream can do anything useful with a zero value. func (c *sdkClient) MintConnectCredential(ctx context.Context, id string, port int) (Credential, error) { sb, err := c.mc.Sandboxes.FromID(ctx, id, nil) if err != nil { return Credential{}, fmt.Errorf("modal: attach sandbox %s: %w", id, err) } - cred := c.mintCredential(ctx, sb, port) - if cred.Token == "" { - return Credential{}, fmt.Errorf("modal: sandbox %s: no connect credential minted", id) - } - return cred, nil + return c.mintCredential(ctx, sb, port) } // modalProbe maps a Pod readinessProbe onto Modal's Probe. Modal supports only diff --git a/pkg/provider/modal/modal.go b/pkg/provider/modal/modal.go index 4df6098..480955b 100644 --- a/pkg/provider/modal/modal.go +++ b/pkg/provider/modal/modal.go @@ -88,7 +88,9 @@ type Client interface { // CreateSandbox launches one sandbox from spec and returns its Modal id plus the // connect credential minted for it. Minting is one-shot and there is no read-back, so // a caller that drops the credential can only get another from MintConnectCredential. - // Zero when none could be minted; see sdkClient.mintCredential. + // A sandbox that could not be given one is unreachable, so a failed mint is an ERROR + // with no id, not a zero credential — the sandbox may exist, and the claim tag is what + // reclaims it (see sdkClient.CreateSandbox). CreateSandbox(ctx context.Context, spec SandboxSpec) (id string, cred Credential, err error) // MintConnectCredential mints a NEW credential for a sandbox that already exists, // which Modal allows from the id alone. Every call returns a different token and none diff --git a/pkg/provider/modal/modal_test.go b/pkg/provider/modal/modal_test.go index 5ebb1d5..0be2928 100644 --- a/pkg/provider/modal/modal_test.go +++ b/pkg/provider/modal/modal_test.go @@ -46,7 +46,7 @@ type fakeClient struct { createCnt int createErr error createID string - cred Credential // credential CreateSandbox returns; zero = none minted + cred Credential // credential CreateSandbox returns alongside its id terminated []string // The mint path: what MintConnectCredential returns, and what it was asked for. @@ -1091,22 +1091,25 @@ func TestProvision_ReturnsMintedCredential(t *testing.T) { } } -// A sandbox that minted nothing yields no credential rather than an error: it still -// exists, still costs money, and must still be reported and reclaimed. -func TestProvision_NoCredentialWhenNoneMinted(t *testing.T) { - f := &fakeClient{createID: "sb-1"} // zero cred +// The create leg's half of TestProvision_IdempotentFailsWhenMintFails: a sandbox that came +// up but could not be given a credential is unreachable, so the create reports a failure +// and no id rather than a provisioned sandbox with an empty credential — which nothing +// downstream would revisit, since the Pod succeeded. Not a rejection, so the Pod stays +// provisioning and the retry adopts the sandbox by its claim tag. +func TestProvision_FailsWhenCreateCannotMint(t *testing.T) { + f := &fakeClient{createID: "sb-1", createErr: errors.New("mint connect credential: boom")} p := newTestProvider(f) res, err := p.Provision(context.Background(), gpuPod("claim-a", "H100", 1), provider.ProvisionRequest{ClaimName: "claim-a"}) - if err != nil { - t.Fatalf("Provision: %v", err) + if err == nil { + t.Fatal("Provision succeeded; a sandbox that could not be given a credential is unreachable") } - if res.InstanceID != "sb-1" { - t.Fatalf("InstanceID = %q, want sb-1 even with no credential", res.InstanceID) + if res.InstanceID != "" { + t.Fatalf("InstanceID = %q, want empty so nothing reads a half-provisioned result", res.InstanceID) } - if res.ConnectURL != "" || res.ConnectToken != "" { - t.Fatalf("expected no credential, got url=%q token set=%t", res.ConnectURL, res.ConnectToken != "") + if provider.IsRejection(err) { + t.Fatal("a mint failure must not be a rejection; it would fail the Pod and blocklist the provider") } } From e9a3f4f88d0b07579bfc8a7653fc88247d9f7213 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Sat, 29 Aug 2026 22:24:48 +0100 Subject: [PATCH 06/12] increase the timeout Signed-off-by: kerthcet --- pkg/provider/aws/aws_test.go | 21 +++--- pkg/provider/errors.go | 100 ++++++++++++++++--------- pkg/provider/errors_test.go | 48 ++++++------ pkg/provider/modal/client.go | 11 ++- pkg/provider/modal/modal.go | 35 +++------ pkg/provider/modal/modal_test.go | 124 ++++++++++++++----------------- pkg/vnode/handler.go | 48 +++--------- pkg/vnode/handler_test.go | 50 ++++++------- 8 files changed, 206 insertions(+), 231 deletions(-) diff --git a/pkg/provider/aws/aws_test.go b/pkg/provider/aws/aws_test.go index e42b02a..e9b14c2 100644 --- a/pkg/provider/aws/aws_test.go +++ b/pkg/provider/aws/aws_test.go @@ -559,18 +559,17 @@ func TestClassifyProvisionError(t *testing.T) { {"wrapped capacity is regional", fmt.Errorf("run: %w", provider.ErrNoCapacity), onDemandRegional}, {"spot capacity blocks only Spot in region", spotNoCapacity, spotRegional}, {"string no-capacity is regional OnDemand", stringNoCapacity, onDemandRegional}, - // An unrecognized error is confined to this accelerator + tier + region, NOT - // DenyAll: a whole-provider block on an unidentifiable failure is too broad, so - // failover routes around the one failing candidate instead. - {"unknown is regional OnDemand", fmt.Errorf("weird transient blip"), onDemandRegional}, - // InvalidFleetConfiguration, in a CreateFleet per-override error, means the - // instance type is not offered in that subnet's AZ — a zone-local availability - // gap classifyEC2Error maps to no-capacity, so it blocks only this - // accelerator/tier/region (a sibling AZ or region may still serve it), never - // DenyAll. - {"invalid fleet config is regional OnDemand", + // An error we cannot attribute blocks NOTHING, so no region is stamped either: a + // transport failure or an unknown code is no evidence against this candidate. + {"unknown blocks nothing", fmt.Errorf("weird transient blip"), provider.BlockScope{}}, + // The RAW code reaches here only if it escaped translation, and unrecognized means + // unattributable, so it blocks nothing. The real path never presents it this way: + // classifyEC2Error wraps InvalidFleetConfiguration as no-capacity first (it means + // "not offered in this subnet's AZ" — see translate.go), which lands on the + // ErrNoCapacity rows above and blocks this accelerator/tier/region. + {"raw invalid fleet config blocks nothing", &smithy.GenericAPIError{Code: "InvalidFleetConfiguration", Message: "not supported in AZ"}, - onDemandRegional}, + provider.BlockScope{}}, {"nil", nil, provider.BlockScope{}}, // An image pull credential this adapter cannot honour is a fact about the POD, so it // blocks nothing. Region must NOT be stamped on: a scope carrying only a region diff --git a/pkg/provider/errors.go b/pkg/provider/errors.go index f4c00f6..eb130fc 100644 --- a/pkg/provider/errors.go +++ b/pkg/provider/errors.go @@ -65,15 +65,23 @@ var ( // to the Pod, so it blocklists NOTHING — no other region, tier or provider produces an // image this one just refused to produce. ErrImageBuild = errors.New("provider: cannot build image") + // ErrCredential: the instance came up but could not be given the credential that makes it + // reachable. Minting is create-only with no read-back, so it cannot be repeated for that + // instance — the adapter destroys it and reports this. + ErrCredential = errors.New("provider: cannot mint connect credential") ) // ClassifyError maps a provision error to the BlockScope it should be blocklisted at, // checking the shared sentinels first and falling back to string heuristics for raw API // messages. The rule it encodes: a narrow failure must not disqualify other accelerators, or -// other regions. Only auth widens to the whole provider via DenyAll; a failure that belongs to -// the REQUEST rather than the candidate blocks nothing at all; everything else — including an -// unrecognized error — is scoped to the failing accelerator/tier/region so failover can route -// around it. +// other regions. Only auth widens to the whole provider via DenyAll; a capacity refusal is +// scoped to the failing accelerator/tier/region so failover can route around it; and two cases +// block nothing at all — a failure that belongs to the REQUEST rather than the candidate, and +// one that could not be attributed to either. +// +// The zero scope is the ONLY signal for "block nothing": recordBlock no-ops on it, so callers +// classify every failure and act on the result instead of pre-filtering with a second +// predicate. IsRejection answers a different question and does not gate this one. // // This is the single place the SHARED part of a scope is derived, so every adapter delegates // here and then only adds what is provider-specific (AWS adds its region). Nothing assembles @@ -118,17 +126,22 @@ func ClassifyError(err error, capacityType nebulav1alpha1.CapacityType, accelera // Still a rejection, so the Pod fails with the reason rather than retrying forever — // the two questions are separate. See IsRejection. return BlockScope{} - default: - // An unrecognized error is scoped like capacity (this accelerator + tier, and - // per region once the adapter confines it), NOT DenyAll. A DenyAll on an - // unknown error fences off the WHOLE provider — every region and accelerator — - // which is far too broad a blast radius for a failure we can't even identify - // (e.g. a transient malformed-request blip in one region). Failover past the - // one failing candidate is the safer default; the TTL still bounds it. + case catUnattributable: + // Nothing is blocklisted, and for a different reason than catRequest above: there the + // candidate is known to be innocent, here nothing at all was learned about it. A + // cancelled context or a dropped connection is no evidence against a candidate, so + // recording one would exclude a provider for a failure that was never its doing. // - // This answers "how widely, IF we block", not "should we block": a caller that - // cannot attribute the failure to the request at all should not be filing a block - // in the first place — see IsRejection. + // This zero is what makes the scope the SINGLE answer to "should anything be blocked": + // callers hand every failure to recordBlock and let the scope decide, rather than + // gating on a second predicate that could disagree with it. + return BlockScope{} + default: + // A category added to the enum but not handled above. Scoped like capacity (this + // accelerator + tier, and per region once the adapter confines it) rather than zero, + // because zero is a CLAIM — that the candidate is fine — which an unreasoned-about + // category has not earned. NOT DenyAll either: that fences off every region and + // accelerator on the provider, far too wide a blast radius to reach by omission. return capacityScope() } } @@ -139,15 +152,17 @@ type failureCategory int const ( // catUnattributable: nothing in the error says what the provider decided, because - // it may not have decided anything — a transport failure, a timeout, an - // unparseable API blip. Kept distinct from the scope ClassifyError ultimately - // returns for it. + // it may not have decided anything — a transport failure, a cancellation, an + // unparseable API blip. The only category IsRejection answers false for, and it + // blocklists nothing: no candidate can be held responsible for a failure nobody + // could attribute to it. catUnattributable failureCategory = iota // catAuth: credentials or authorization failed, so nothing on the provider works. catAuth // catCapacity: the provider refused this request because of something about the - // CANDIDATE — no capacity, quota exhausted, an accelerator it does not offer. Blocking - // it is meaningful, because the next Pod asking for the same candidate would fail too. + // CANDIDATE — no capacity, quota exhausted, an accelerator it does not offer, or the whole + // provision budget spent without a usable instance. Blocking it is meaningful, because the + // next Pod asking for the same candidate would fail the same way. catCapacity // catRequest: the provider refused this request because of something about the REQUEST // itself — today an image it cannot pull or cannot build. A decision, so a rejection, but @@ -155,20 +170,30 @@ const ( catRequest ) -// categorize buckets a provision error: cancellation first, then sentinels, then string -// heuristics. +// categorize buckets a provision error: the deadline and cancellation checks first — they +// outrank any label an adapter attached, since an adapter cannot see whose clock fired — then +// the sentinels, then string heuristics. func categorize(err error) failureCategory { msg := strings.ToLower(err.Error()) - if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) || - containsAny(msg, "context deadline exceeded", "context canceled") { + // A deadline is a capacity failure, not an unknown: the candidate was given the whole + // provision budget and did not produce a usable instance. + if errors.Is(err, context.DeadlineExceeded) || containsAny(msg, "context deadline exceeded") { + return catCapacity + } + + // Cancellation stays unattributable, unlike the deadline above: it means WE stopped asking + // — a manager shutdown, a leader handoff — and the provider may well have accepted the + // request. Nothing about the candidate was learned, so failing the Pod or blocklisting + // would punish it for our own exit. + if errors.Is(err, context.Canceled) || containsAny(msg, "context canceled") { return catUnattributable } switch { case errors.Is(err, ErrAuth): return catAuth - case errors.Is(err, ErrImagePull), errors.Is(err, ErrImageBuild): + case errors.Is(err, ErrImagePull), errors.Is(err, ErrImageBuild), errors.Is(err, ErrCredential): return catRequest case errors.Is(err, ErrNoCapacity), errors.Is(err, ErrUnsupportedAccelerator), errors.Is(err, ErrQuota): @@ -197,19 +222,22 @@ func categorize(err error) failureCategory { // IsRejection reports whether err is a provider DECISION about this request — "no // capacity", "over quota", "bad credentials", "I do not offer that accelerator" — as // opposed to a failure to find out what the provider would have decided: a transport -// error, a timeout, a 503, an unparseable response. +// error, a cancellation, a 503, an unparseable response. +// +// A provision DEADLINE counts as a decision, though nobody spoke it: the candidate was given +// the entire provision budget and produced no usable instance, which is as good a refusal as +// one it words. Treating it as unknown left the Pod provisioning behind an attempt nothing +// re-enters. A cancellation is the opposite — that is us stopping, not the candidate failing. +// +// It does NOT decide what happens to the Pod, and no longer gates the blocklist. A provision +// failure is terminal either way (see vnode.Handler.CreatePod), and what may be blocklisted is +// answered by ClassifyError alone, whose zero scope means "nothing". Two mechanisms for one +// question could disagree; one cannot. // -// The distinction exists because the two call for opposite handling and the costs are -// asymmetric. A rejection is authoritative, so the Pod is failed with the reason — correct, -// because the provider said no. Whether a CANDIDATE is also blocklisted is a separate -// question, answered by ClassifyError: a capacity rejection blocks one and failover routes -// around it, while a request-scoped one (an unusable image credential) blocks nothing, because -// the candidate did nothing wrong. An unattributable failure is authoritative about nothing, -// so the same writes -// stamp a terminal status onto a request the provider may have accepted (leaving a -// paid instance running behind a Pod that is about to be reaped) and fence off a -// candidate that never misbehaved. Retrying costs a request and Provision is -// idempotent on ClaimName; acting on a guess costs an instance. +// What it still separates is DIAGNOSIS: a rejection is a placement problem — the provider was +// reached and said no — while an unattributable failure is an integration problem, and the two +// are fixed by different people. That is the split metrics reports on (see +// metrics.provisionReason). // // A nil error is not a rejection. func IsRejection(err error) bool { diff --git a/pkg/provider/errors_test.go b/pkg/provider/errors_test.go index 58de33e..4c7e5df 100644 --- a/pkg/provider/errors_test.go +++ b/pkg/provider/errors_test.go @@ -70,10 +70,9 @@ func TestClassifyError(t *testing.T) { {"string unauthorized", fmt.Errorf("HTTP 401 unauthorized"), BlockScope{DenyAll: true}}, {"string quota", fmt.Errorf("account limit exceeded"), capacityScope}, {"string capacity", fmt.Errorf("no capacity available"), capacityScope}, - // An unrecognized error is scoped like capacity (this accelerator + tier), NOT - // DenyAll: a DenyAll would fence off the whole provider on a failure we can't - // even identify, so failover past the one failing candidate is the safer default. - {"unknown capacity-scoped", fmt.Errorf("weird transient blip"), capacityScope}, + // An error we cannot attribute blocks NOTHING: it is no evidence against the + // candidate, and the zero scope is what keeps recordBlock from acting on it. + {"unknown blocks nothing", fmt.Errorf("weird transient blip"), BlockScope{}}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -129,9 +128,14 @@ func TestIsRejection(t *testing.T) { {"image-pull sentinel", ErrImagePull, true}, {"image-build sentinel", ErrImageBuild, true}, - // The failures this predicate exists for. - {"deadline exceeded", context.DeadlineExceeded, false}, - {"wrapped deadline", fmt.Errorf("provision: %w", context.DeadlineExceeded), false}, + // A deadline REJECTS: the candidate had the whole provision budget and produced no + // usable instance, so the Pod fails and the candidate is blocked for the TTL, sending + // the next attempt somewhere else instead of spending another full budget here. + {"deadline exceeded", context.DeadlineExceeded, true}, + {"wrapped deadline", fmt.Errorf("provision: %w", context.DeadlineExceeded), true}, + + // The failures this predicate exists for. Cancellation is OUR exit, not the + // candidate's failure, so it stays retryable however the deadline is treated. {"canceled", context.Canceled, false}, {"connection refused", errors.New("dial tcp 10.0.0.1:443: connect: connection refused"), false}, {"eof", errors.New("unexpected EOF"), false}, @@ -146,12 +150,12 @@ func TestIsRejection(t *testing.T) { {"grpc unavailable wrapping a sentinel", fmt.Errorf("rpc error: code = Unavailable desc = no gpu: %w", ErrNoCapacity), true}, - // OUR clock is the one thing a sentinel does NOT win over. An adapter labels the - // failure it saw and cannot see whose deadline fired, so a ProvisionTimeout mid-call - // must stay retryable — failing the Pod here would reap the attempt that was warming - // the provider's cache for the retry (see modal.sdkClient.buildImage). + // OUR clock outranks any label a sentinel carries: an adapter reports the failure it + // saw and cannot see whose deadline fired. So a build that ran out of budget is a + // capacity rejection, not the zero-scope image failure its label suggests — the + // candidate spent the whole budget and delivered nothing. {"deadline wrapped in an image-build label", - fmt.Errorf("modal: image build: %w: %w", context.DeadlineExceeded, ErrImageBuild), false}, + fmt.Errorf("modal: image build: %w: %w", context.DeadlineExceeded, ErrImageBuild), true}, {"cancellation wrapped in a capacity label", fmt.Errorf("sweep: %w: %w", context.Canceled, ErrNoCapacity), false}, // The form errors.Is CANNOT see: grpc-go turns a dead context into a status error @@ -160,7 +164,7 @@ func TestIsRejection(t *testing.T) { {"grpc deadline wrapping an image-build label", fmt.Errorf("modal: image build: %w: %w", errors.New("rpc error: code = DeadlineExceeded desc = context deadline exceeded"), - ErrImageBuild), false}, + ErrImageBuild), true}, // A verdict Modal actually reached still rejects: the label is only overridden when // the context is what died. {"image-build label on a remote verdict", @@ -177,20 +181,16 @@ func TestIsRejection(t *testing.T) { } } -// ClassifyError answers "how widely, IF we block" and keeps its capacity-shaped -// default for an unattributable error: a caller that has decided to block something -// still wants the narrow blast radius. IsRejection is the separate question of -// whether to block at all, so the two must not be collapsed. -func TestClassifyError_UnattributableStillScopesNarrow(t *testing.T) { +// The scope is the single answer to "should anything be blocked", so an unattributable +// error must classify to ZERO — callers hand every failure to recordBlock and rely on this +// to record nothing. A narrow-but-non-empty scope here would fence off a candidate for a +// transport failure that was never its doing. +func TestClassifyError_UnattributableBlocksNothing(t *testing.T) { got := ClassifyError( errors.New("rpc error: code = Unavailable desc = transport is closing"), nebulav1alpha1.CapacitySpot, "H100:8") - if got.DenyAll { - t.Fatalf("an unattributable error must never widen to DenyAll, got %+v", got) - } - if got.Accelerator == nil || *got.Accelerator != "H100:8" || - got.CapacityType != nebulav1alpha1.CapacitySpot { - t.Fatalf("expected a Spot/H100:8-scoped block, got %+v", got) + if got != (BlockScope{}) { + t.Fatalf("an unattributable error must block nothing, got %+v", got) } } diff --git a/pkg/provider/modal/client.go b/pkg/provider/modal/client.go index b652b08..f2ce18e 100644 --- a/pkg/provider/modal/client.go +++ b/pkg/provider/modal/client.go @@ -265,7 +265,9 @@ func (c *sdkClient) registrySecret(ctx context.Context, kv map[string]string) (* // // A failure is REPORTED, never swallowed into a zero credential. Minting is one-shot with // no read-back, so a dropped error loses the credential of a sandbox that exists and is -// billing — silently, since a caller handed an empty pair has nothing to log. +// billing — silently, since a caller handed an empty pair has nothing to log. It is tagged +// provider.ErrCredential, which fails the Pod terminally (blocklisting nothing) so its owner +// recreates it — the only recovery, since this sandbox can never be given a credential. func (c *sdkClient) mintCredential(ctx context.Context, sb *modal.Sandbox, port int) (Credential, error) { creds, err := sb.CreateConnectToken(ctx, &modal.SandboxCreateConnectTokenParams{ // Derived from the exposed set rather than carried separately, so the routed @@ -273,13 +275,14 @@ func (c *sdkClient) mintCredential(ctx context.Context, sb *modal.Sandbox, port Port: port, }) if err != nil { - return Credential{}, fmt.Errorf("modal: mint connect credential for sandbox %s on port %d: %w", - sb.SandboxID, port, err) + return Credential{}, fmt.Errorf("modal: mint connect credential for sandbox %s on port %d: %w: %w", + sb.SandboxID, port, err, provider.ErrCredential) } // A token-less success is the same outcome as an error — an address with nothing to // authenticate against it — so it is reported as one. if creds == nil || creds.Token == "" { - return Credential{}, fmt.Errorf("modal: sandbox %s: connect credential minted without a token", sb.SandboxID) + return Credential{}, fmt.Errorf("modal: sandbox %s: connect credential minted without a token: %w", + sb.SandboxID, provider.ErrCredential) } return Credential{URL: creds.URL, Token: creds.Token}, nil } diff --git a/pkg/provider/modal/modal.go b/pkg/provider/modal/modal.go index 480955b..49786bd 100644 --- a/pkg/provider/modal/modal.go +++ b/pkg/provider/modal/modal.go @@ -68,8 +68,11 @@ const defaultSandboxTimeout = 24 * time.Hour // provisionTimeout raises the vnode handler's generic Provision deadline, because Provision // here BLOCKS on the image build (see sdkClient.buildImage): a cold image is pulled into -// Modal's cache on this call, and the create leg only gets what the build leaves it. -const provisionTimeout = 90 * time.Second +// Modal's cache on this call, and the create and credential legs only get what the build +// leaves them. Generous on purpose — at 90s a slow build starved the mint, which is the last +// call of the three and the one with no second chance: it is one-shot with no read-back, so a +// sandbox that misses it is unreachable for good and the Pod fails. +const provisionTimeout = 15 * time.Minute // compile-time assertions that Provider satisfies the interfaces. LogStreamer and // Executor are the optional halves: they are what make `kubectl logs` and `kubectl exec` @@ -371,34 +374,16 @@ func (p *Provider) Provision( // the poll loop reports Running has capacity, one still queued does not. That is more // information than a create can return, so report it rather than a flat false. // - // Its credential is minted anew, but ONLY when the Pod carries no endpoint: that - // annotation and the credential Secret are written in the same pass (see - // vnode.Handler.CreatePod), so no endpoint means the first token reached nobody and the - // sandbox is unreachable. Where one exists, minting would strand the consumer holding - // it — a new token revokes nothing and the old one keeps working. + // No credential comes back, per the Provider contract: minting is one-shot with no + // read-back, and a fresh token revokes nothing, so re-minting for a sandbox whose token is + // already published strands the consumer holding it. if existing, err := p.findByClaim(ctx, req.ClaimName); err != nil { return provider.ProvisionResult{}, err } else if existing != nil { - res := provider.ProvisionResult{ + return provider.ProvisionResult{ InstanceID: existing.ID, Reserved: existing.State == provider.InstanceRunning, - } - if pod.Annotations[nebulav1alpha1.EndpointAnnotation] == "" { - port := 0 - if len(pod.Spec.Containers) > 0 { - port = firstPort(containerPorts(&pod.Spec.Containers[0])) - } - cred, err := p.client.MintConnectCredential(ctx, existing.ID, port) - if err != nil { - // Fail the whole provision rather than report an unreachable sandbox as - // provisioned. The error is not a rejection, so the Pod stays Provisioning - // and VK retries; the retry adopts this same sandbox by its claim tag and - // mints again, which is safe precisely because no endpoint was published. - return provider.ProvisionResult{}, err - } - res.ConnectURL, res.ConnectToken = cred.URL, cred.Token - } - return res, nil + }, nil } spec, err := p.sandboxSpecFromPod(pod, req) diff --git a/pkg/provider/modal/modal_test.go b/pkg/provider/modal/modal_test.go index 0be2928..ab63d8d 100644 --- a/pkg/provider/modal/modal_test.go +++ b/pkg/provider/modal/modal_test.go @@ -464,7 +464,7 @@ func TestClassifyProvisionError(t *testing.T) { {"string no capacity", fmt.Errorf("no capacity available in region"), onDemand}, // An unrecognized error is scoped like capacity, not DenyAll: a whole-provider // block on an unidentifiable failure is too broad; failover routes around it. - {"unknown capacity-scoped", fmt.Errorf("weird transient blip"), onDemand}, + {"unknown blocks nothing", fmt.Errorf("weird transient blip"), provider.BlockScope{}}, {"nil", nil, provider.BlockScope{}}, } for _, tt := range tests { @@ -1091,13 +1091,16 @@ func TestProvision_ReturnsMintedCredential(t *testing.T) { } } -// The create leg's half of TestProvision_IdempotentFailsWhenMintFails: a sandbox that came -// up but could not be given a credential is unreachable, so the create reports a failure -// and no id rather than a provisioned sandbox with an empty credential — which nothing -// downstream would revisit, since the Pod succeeded. Not a rejection, so the Pod stays -// provisioning and the retry adopts the sandbox by its claim tag. +// A sandbox that came up but could not be given a credential is unreachable, and nothing +// revisits it: minting is one-shot with no read-back, and the idempotent branch below hands +// back no credential. So the create reports a failure and no id, and reports it as a +// REJECTION — the Pod fails terminally and its owner recreates it, which is the only recovery +// left. The scope stays zero, because the candidate served the request correctly. func TestProvision_FailsWhenCreateCannotMint(t *testing.T) { - f := &fakeClient{createID: "sb-1", createErr: errors.New("mint connect credential: boom")} + f := &fakeClient{ + createID: "sb-1", + createErr: fmt.Errorf("modal: mint connect credential for sandbox sb-1: %w", provider.ErrCredential), + } p := newTestProvider(f) res, err := p.Provision(context.Background(), gpuPod("claim-a", "H100", 1), @@ -1108,8 +1111,35 @@ func TestProvision_FailsWhenCreateCannotMint(t *testing.T) { if res.InstanceID != "" { t.Fatalf("InstanceID = %q, want empty so nothing reads a half-provisioned result", res.InstanceID) } - if provider.IsRejection(err) { - t.Fatal("a mint failure must not be a rejection; it would fail the Pod and blocklist the provider") + if !provider.IsRejection(err) { + t.Fatal("a mint failure must be a rejection; otherwise the Pod sits provisioning and is never re-provisioned") + } + if scope := provider.ClassifyError(err, nebulav1alpha1.CapacityOnDemand, "H100:1"); scope != + (provider.BlockScope{}) { + t.Fatalf("BlockScope = %+v, want zero; the request failed, not the candidate", scope) + } +} + +// The production shape of the same failure: the image build eats the provision budget and the +// mint — last of the three legs — dies on the deadline, so the error carries BOTH the sentinel +// and a deadline. It must still classify as a rejection: the sentinel says the outcome is +// known, while a bare deadline would mean "we cannot say", leaving the Pod provisioning +// forever behind a sandbox that can never be given a credential. +func TestProvision_MintDeadlineIsStillARejection(t *testing.T) { + f := &fakeClient{ + createID: "sb-1", + createErr: fmt.Errorf("modal: mint connect credential for sandbox sb-1: %w: %w", + fmt.Errorf("rpc error: code = DeadlineExceeded: %w", context.DeadlineExceeded), provider.ErrCredential), + } + p := newTestProvider(f) + + _, err := p.Provision(context.Background(), gpuPod("claim-a", "H100", 1), + provider.ProvisionRequest{ClaimName: "claim-a"}) + if err == nil { + t.Fatal("Provision succeeded; the sandbox has no credential") + } + if !provider.IsRejection(err) { + t.Fatal("a timed-out mint must be a rejection; the deadline is how it failed, not whether we know it did") } } @@ -1127,80 +1157,34 @@ func adoptable() *fakeClient { } } -// An adopted sandbox whose Pod ALREADY carries an endpoint carries no credential: a token -// was published for it, and minting a second would leave the consumer holding one the -// Secret no longer matches, with the original still valid and unrevokable. -func TestProvision_IdempotentKeepsPublishedCredential(t *testing.T) { +// An adopted sandbox NEVER carries a credential, whatever the Pod looks like. Its token was +// minted once, at create, and cannot be read back; a second one would leave the consumer +// holding a token the Secret no longer matches, with the original still valid and unrevokable. +// The instance id and its state are all the branch reports. +func TestProvision_IdempotentReturnsNoCredential(t *testing.T) { f := adoptable() p := newTestProvider(f) - pod := gpuPod("claim-a", "H100", 1) - pod.Annotations = map[string]string{ - nebulav1alpha1.EndpointAnnotation: "https://published.modal.host", - } - - res, err := p.Provision(context.Background(), pod, provider.ProvisionRequest{ClaimName: "claim-a"}) + res, err := p.Provision(context.Background(), gpuPod("claim-a", "H100", 1), + provider.ProvisionRequest{ClaimName: "claim-a"}) if err != nil { t.Fatalf("Provision: %v", err) } if res.InstanceID != "sb-existing" { t.Fatalf("InstanceID = %q, want sb-existing", res.InstanceID) } - if f.mintCnt != 0 { - t.Fatalf("minted %d credentials for a Pod that already has an endpoint; want 0", f.mintCnt) - } - if res.ConnectURL != "" || res.ConnectToken != "" { - t.Fatalf("an adopted sandbox with a published endpoint must carry no credential, got url=%q token set=%t", - res.ConnectURL, res.ConnectToken != "") - } -} - -// The recovery case: the first attempt created the sandbox but never published its -// address, so nothing holds the original token and the adopted sandbox is unreachable. -// It gets a fresh credential, routed to the port the POD declares. -func TestProvision_IdempotentMintsWhenNoEndpointPublished(t *testing.T) { - f := adoptable() - p := newTestProvider(f) - - pod := gpuPod("claim-a", "H100", 1) // no endpoint annotation - pod.Spec.Containers[0].Ports = []corev1.ContainerPort{{ContainerPort: 9000}} - - res, err := p.Provision(context.Background(), pod, provider.ProvisionRequest{ClaimName: "claim-a"}) - if err != nil { - t.Fatalf("Provision: %v", err) + if !res.Reserved { + t.Fatal("Reserved = false for an adopted Running sandbox, which has its capacity") } if f.createCnt != 0 { - t.Fatalf("created %d sandboxes; recovery must adopt, not create", f.createCnt) - } - if f.mintCnt != 1 || f.mintID != "sb-existing" { - t.Fatalf("mint calls = %d for id %q, want 1 for sb-existing", f.mintCnt, f.mintID) - } - if f.mintPort != 9000 { - t.Fatalf("minted for port %d, want the Pod's declared 9000", f.mintPort) - } - if res.ConnectURL != "https://minted.modal.host" || res.ConnectToken != "tok-minted" { - t.Fatalf("credential = %q/%q, want the freshly minted pair", res.ConnectURL, res.ConnectToken) - } -} - -// A failed mint FAILS the Provision, rather than reporting an unreachable sandbox as -// provisioned. It is not a rejection, so the handler leaves the Pod provisioning and VK -// retries — and the retry adopts this same sandbox and mints again. -func TestProvision_IdempotentFailsWhenMintFails(t *testing.T) { - f := adoptable() - f.mintErr = errors.New("boom") - p := newTestProvider(f) - - res, err := p.Provision(context.Background(), gpuPod("claim-a", "H100", 1), - provider.ProvisionRequest{ClaimName: "claim-a"}) - if err == nil { - t.Fatal("Provision succeeded; a sandbox that could not be given a credential is unreachable") + t.Fatalf("created %d sandboxes; the claim tag must be adopted, not duplicated", f.createCnt) } - if res.InstanceID != "" { - t.Fatalf("InstanceID = %q, want empty so nothing reads a half-provisioned result", res.InstanceID) + if f.mintCnt != 0 { + t.Fatalf("minted %d credentials for an adopted sandbox; want 0", f.mintCnt) } - if provider.IsRejection(err) { - t.Fatal("a mint failure must not be a rejection; it would fail the Pod and blocklist the provider") + if res.ConnectURL != "" || res.ConnectToken != "" { + t.Fatalf("an adopted sandbox must carry no credential, got url=%q token set=%t", + res.ConnectURL, res.ConnectToken != "") } } diff --git a/pkg/vnode/handler.go b/pkg/vnode/handler.go index 266c490..bc0dc2f 100644 --- a/pkg/vnode/handler.go +++ b/pkg/vnode/handler.go @@ -367,37 +367,15 @@ func (h *Handler) CreatePod(ctx context.Context, pod *corev1.Pod) error { // blocklisted for it. metrics.ObserveProvision(labels, time.Since(callStart), err) if err != nil { - // An error the provider never attributed to this request — a transport failure, - // our own timeout, a 503 — is not a rejection (see provider.IsRejection). The - // request may even have been accepted, so failing the Pod would reap it out from - // under a paid instance whose id we never learned, and blocklisting would fence - // off a candidate that never misbehaved. - // - // So leave it NON-terminal at the Provisioning already stamped, with the error as - // its message, and return the error for VK to retry with backoff. Provision is - // idempotent on ClaimName, so the retry adopts whatever the failed attempt created. - // - // Deliberately NOT stored: a tracked pod with no instance id would be read as - // absent from List and written the very Terminated this branch avoids. - if !provider.IsRejection(err) { - log.Error(err, "provision failed with an error the provider did not attribute "+ - "to this request; Pod left provisioning for retry, nothing blocklisted") - h.markStatus(pod, corev1.PodPending, reasonProvisioning, "retrying: "+err.Error()) - h.emit(pod) - return err - } - - log.Error(err, "provision rejected by the provider; Pod marked Failed for failover") - // Record the failure so placement fails over to the next candidate (zone → region - // → tier) instead of hot-looping here. The provider narrows its own error into a - // BlockScope (a Spot shortage in one region blocks only that; auth/quota blocks - // the whole provider). + log.Error(err, "provision failed; Pod marked Failed") h.recordBlock(ctx, pod, req.Region, blocklistTTLOf(pool), err) - // Surface the failure so placement can fail over, and return the error so the pod - // controller retries with backoff. + // Surface the failure on the Pod so placement can fail over, and return the error for + // VK's own accounting — not for a retry, which the store below suppresses. h.markStatus(pod, corev1.PodFailed, reasonProvisionFailed, err.Error()) - // Zero start: terminal, so it never reaches Running and has no ready-duration, which - // is also why the placement it would be filed under is not worth carrying. + // Stored to SUPPRESS the create: tracked means GetPod returns non-nil, so VK takes the + // update branch instead of provisioning again. + // + // Safe only because Pod with empty instanceID will be terminated on the next poll tick. h.store(pod, claim, "", time.Time{}, placement{}) h.emit(pod) return err @@ -459,17 +437,15 @@ func (h *Handler) persistCredential(ctx context.Context, pod *corev1.Pod, url, t h.createConnectSecret(ctx, pod, url, token) } -// UpdatePod is a no-op: an instance's shape is immutable once provisioned (recovery -// from any change is delete-and-recreate). We still refresh the tracked copy so GetPod -// reflects the latest metadata. +// UpdatePod is a no-op on the instance: its shape is immutable once provisioned (recovery +// from any change is delete-and-recreate). It only refreshes the tracked copy, which is +// what GetPod serves and the poll loop re-emits. func (h *Handler) UpdatePod(_ context.Context, pod *corev1.Pod) error { h.mu.Lock() defer h.mu.Unlock() if tp, ok := h.tracked[key(pod.Namespace, pod.Name)]; ok { - // Keep what WE own and the API server does not know yet: the status we compute, - // and the endpoint — possibly an address minted at create whose patch has not - // landed, so the incoming Pod lacks it. Dropping it would discard the only copy, - // since a minted URL is never re-observed. Everything else is adopted. + // Copy the status and endpoint just in case the update failed, + // so we do not lose them in the tracked copy. status := tp.pod.Status endpoint := tp.pod.Annotations[nebulav1alpha1.EndpointAnnotation] tp.pod = pod.DeepCopy() diff --git a/pkg/vnode/handler_test.go b/pkg/vnode/handler_test.go index 9913620..8280a1e 100644 --- a/pkg/vnode/handler_test.go +++ b/pkg/vnode/handler_test.go @@ -436,21 +436,20 @@ func TestCreatePod_ProvisionErrorSurfaces(t *testing.T) { } } -// An error the provider never attributed to the request is not a rejection, so it -// must not be acted on like one: no terminal status (the request may have been -// accepted, and a Failed Pod is reaped out from under a paid instance), no blocklist -// entry against a candidate that never misbehaved, and no tracking (a tracked pod with -// no instance id is written Terminated by the very next poll tick). The Pod stays -// Provisioning with the error as its message and VK retries with backoff. -func TestCreatePod_UnattributableErrorLeavesPodProvisioning(t *testing.T) { +// An error the provider never attributed to the request still FAILS the Pod: leaving it +// provisioning promised a retry that does not exist (VK re-enters CreatePod only while +// GetPod reports nothing, so an attempt that created an instance was never re-driven), and +// a Pod hung there forever is worse than one whose owner can replace it. +// +// What the attribution still decides is the BLOCKLIST: nothing may be recorded against a +// candidate that never misbehaved. +func TestCreatePod_UnattributableErrorFailsPodWithoutBlocklisting(t *testing.T) { provErr := errors.New("rpc error: code = Unavailable desc = transport is closing") - // A non-empty classifyScope proves the guard runs BEFORE classification: a provider - // willing to hand back a blockable scope still must not have one recorded. - accel := "H100:1" - fp := &fakeProvider{ - provisionErr: provErr, - classifyScope: provider.BlockScope{Accelerator: &accel}, - } + // The zero classifyScope is what a real adapter returns for this error (see + // provider.ClassifyError), and asserting on it end-to-end is the point: nothing here + // pre-filters which errors may be blocked, so the scope is the only thing standing + // between a transport failure and a block against an innocent candidate. + fp := &fakeProvider{provisionErr: provErr, classifyScope: provider.BlockScope{}} bl := &recordingBlocklist{} h := NewHandler(fp, nil, bl, openCluster()) @@ -467,9 +466,9 @@ func TestCreatePod_UnattributableErrorLeavesPodProvisioning(t *testing.T) { if !errors.Is(err, provErr) { t.Fatalf("CreatePod must return the provision error for VK to back off, got %v", err) } - if pod.Status.Phase != corev1.PodPending || pod.Status.Reason != reasonProvisioning { - t.Fatalf("status = %s/%s, want the non-terminal %s/%s", - pod.Status.Phase, pod.Status.Reason, corev1.PodPending, reasonProvisioning) + if pod.Status.Phase != corev1.PodFailed || pod.Status.Reason != reasonProvisionFailed { + t.Fatalf("status = %s/%s, want %s/%s", + pod.Status.Phase, pod.Status.Reason, corev1.PodFailed, reasonProvisionFailed) } if !strings.Contains(pod.Status.Message, provErr.Error()) { t.Fatalf("expected the error surfaced as the Pod message, got %q", pod.Status.Message) @@ -477,17 +476,18 @@ func TestCreatePod_UnattributableErrorLeavesPodProvisioning(t *testing.T) { if bl.calls != 0 { t.Fatalf("expected no blocklist entry for an unattributable error, got %d", bl.calls) } - if len(h.tracked) != 0 { - t.Fatalf("expected the Pod left untracked, got %d tracked", len(h.tracked)) + + // Tracked, so GetPod returns non-nil and VK stops trying to create: the Pod is terminal + // and nothing here should provision again for it. + if len(h.tracked) != 1 { + t.Fatalf("expected the failed Pod tracked to suppress a re-create, got %d tracked", len(h.tracked)) } mu.Lock() defer mu.Unlock() - // Both emits are the same non-terminal status: the pre-call stamp and the retry - // message. Neither may be Failed. - for _, e := range emitted { - if strings.HasPrefix(e, string(corev1.PodFailed)) { - t.Fatalf("emitted a terminal status %q for an unattributable error: %v", e, emitted) - } + // The terminal status must actually reach VK, not just the local copy — a SandboxSet + // replaces the box off the back of this emit. + if len(emitted) == 0 || !strings.HasPrefix(emitted[len(emitted)-1], string(corev1.PodFailed)) { + t.Fatalf("expected the last emit to carry the terminal status, got %v", emitted) } } From 3e984ee26b85f56eddb063afc08045e1ba9dc2a4 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Sat, 29 Aug 2026 22:42:24 +0100 Subject: [PATCH 07/12] update config Signed-off-by: kerthcet --- cmd/main.go | 15 ++++++++++++++- pkg/vnode/node.go | 7 ++----- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index c964697..8cee8f0 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -61,6 +61,15 @@ import ( // a fallback for managerNamespace when POD_NAMESPACE is unset. const defaultNamespace = "nebula-system" +// restConfigQPS and restConfigBurst size the client-go bucket shared by every API call in the +// process — each controller's, and the virtual kubelet's status pushes. controller-runtime's +// default 20/30 is what binds first at fleet scale, and it binds invisibly: throttled calls +// wait in our own process, so it reads as API-server or provider slowness. +const ( + restConfigQPS = 50 + restConfigBurst = 100 +) + var ( scheme = runtime.NewScheme() setupLog = ctrl.Log.WithName("setup") @@ -207,7 +216,11 @@ func main() { }) } - mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{ + restConfig := ctrl.GetConfigOrDie() + restConfig.QPS = restConfigQPS + restConfig.Burst = restConfigBurst + + mgr, err := ctrl.NewManager(restConfig, ctrl.Options{ Scheme: scheme, Metrics: metricsServerOptions, WebhookServer: webhookServer, diff --git a/pkg/vnode/node.go b/pkg/vnode/node.go index 027c1ca..65185a2 100644 --- a/pkg/vnode/node.go +++ b/pkg/vnode/node.go @@ -46,11 +46,8 @@ const informerResync = time.Minute // podSyncWorkers is how many workers the pod controller runs per queue. VK serializes // work per pod key, so distinct pods provision in parallel while one key never runs // twice — without this, a single slow provision blocks pods that would succeed -// instantly. Modest, to bound concurrent bursts against a provider's rate limits. -// -// Worthless on its own: the workers pull from queues whose ADMISSION is rate limited, so -// the ceiling is podQueueRate below, not this. See podQueueRateLimiter. -const podSyncWorkers = 8 +// instantly. +const podSyncWorkers = 32 // podQueueRate and podQueueBurst size the token bucket that admits work into each of the // pod controller's queues. From 67fc365c17221bf8a4b17052067d8cead17aaf25 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Sat, 29 Aug 2026 22:58:57 +0100 Subject: [PATCH 08/12] remove isrejected() Signed-off-by: kerthcet --- docs/add-a-provider.md | 26 ++++--- docs/architecture.md | 16 +++-- docs/metrics.md | 13 ++-- pkg/metrics/provision.go | 25 ++----- pkg/metrics/provision_test.go | 24 +++---- pkg/provider/errors.go | 39 ++-------- pkg/provider/errors_test.go | 115 ++++++++++-------------------- pkg/provider/modal/modal.go | 4 +- pkg/provider/modal/modal_test.go | 25 +++---- pkg/provider/registryauth.go | 4 +- pkg/provider/registryauth_test.go | 11 +-- pkg/vnode/metrics_test.go | 17 +++-- 12 files changed, 122 insertions(+), 197 deletions(-) diff --git a/docs/add-a-provider.md b/docs/add-a-provider.md index 544cd21..58f33d8 100644 --- a/docs/add-a-provider.md +++ b/docs/add-a-provider.md @@ -34,17 +34,21 @@ claim identity). Do not duplicate Pod fields onto the request. ### Wrap the errors your `Provision` returns -`ClassifyProvisionError` decides *how widely* to blocklist, but a separate predicate — -`provider.IsRejection` — decides *whether to blocklist at all*, and whether the Pod is -failed. It answers: did the provider make a decision about this request ("no capacity", -"over quota", "bad credentials"), or did we merely fail to find out what it would have -decided (a transport error, a timeout, a 503)? - -Only a **decision** is acted on. An unattributable failure leaves the Pod -non-terminal at `Provisioning` for the pod controller to retry, and records nothing — -because failing a Pod there would stamp a terminal verdict on a request the provider may -well have accepted, reaping the Pod out from under a paid instance whose id was never -returned. +`provider.ClassifyError` — which `ClassifyProvisionError` delegates to before adding +anything provider-specific — is the *single* answer to what a failure blocklists, and its +**zero scope means "block nothing"**. Three outcomes are possible: a decision about the +candidate ("no capacity", "over quota", "I do not offer that accelerator") is scoped to +that accelerator and tier so failover routes around it; bad credentials widen to the whole +provider; and two cases block nothing at all — a failure that belongs to the *request* +rather than the candidate (an image it cannot pull or build), and one that could not be +attributed to either (a transport error, a 503, an unparseable response). + +The Pod fails either way, so failover can pick a different candidate rather than sit behind +an attempt nothing re-enters. What your wrapping decides is whether a candidate is fenced +off for the blocklist TTL — and getting it wrong is expensive in both directions: an +unwrapped image failure whose text says `unauthorized` reads as auth and fences off your +entire provider, while an unwrapped capacity refusal lets the next Pod fail exactly the +same way. What this asks of an adapter: **wrap every error your `Provision` path returns with the matching sentinel** (`fmt.Errorf("...: %w", provider.ErrNoCapacity)`). A wrapped sentinel diff --git a/docs/architecture.md b/docs/architecture.md index 8bcf78f..2056798 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -601,13 +601,15 @@ Components designed to degrade without leaks: suppresses both a duplicate `CreatePod` and a premature `DeletePod` until the provider answers. Conflating the two would let one failed list mark a healthy Pod `Failed` for reaping while the paid instance kept running behind a zero instance id. -- **Provider unreachable during `Provision`.** Distinguished from a *rejection* - (`provider.IsRejection`). Only a rejection — no capacity, quota, auth, unsupported - accelerator — fails the Pod and files a blocklist entry. A transport error, timeout - or 503 leaves the Pod non-terminal at `Provisioning` with the error as its message - and records nothing, because it is not evidence about the request: the provider may - have accepted it. `Provision` is idempotent on `ClaimName`, so the retry adopts - whatever the failed attempt created rather than doubling it. +- **Provider unreachable during `Provision`.** Every provision failure fails the Pod, so + placement can fail over rather than sit behind an attempt nothing re-enters. What the + failure's *kind* still decides is the blocklist, and `provider.ClassifyError` is the + single answer: a transport error, timeout or 503 files nothing, because it is not + evidence about the candidate — the provider may well have accepted the request. Only a + decision about the candidate (no capacity, quota, unsupported accelerator) files an + entry, and only auth widens it to the whole provider. `Provision` is idempotent on + `ClaimName`, so a re-provision adopts whatever the failed attempt created rather than + doubling it. Leader election (`LeaderElectionID: nebula.inftyai.com`) keeps a single active manager reconciling controllers and owning virtual-node leases. diff --git a/docs/metrics.md b/docs/metrics.md index 201bf01..86db61d 100644 --- a/docs/metrics.md +++ b/docs/metrics.md @@ -105,8 +105,7 @@ The failure `reason` is a coarse, closed set, mapped from the shared sentinels i | `auth` | `ErrAuth` — credentials or permissions. | | `unsupported_accelerator` | `ErrUnsupportedAccelerator` — the request cannot be honoured here at all. | | `timeout` | The `Provision` call hit its own deadline without a capacity cause. | -| `unreachable` | The provider never told us what it decided. | -| `other` | The provider *did* reject the request, but the adapter returned a raw API error without wrapping a sentinel, so the category was unavailable. | +| `other` | No sentinel, so the category was unavailable — either the adapter returned a raw API error without wrapping one, or the provider never told us what it decided at all. | Fine-grained detail is deliberately *not* here: it stays where it is already available (the Pod's `Failed` status message and the `vnode-handler` error log). These labels exist @@ -149,11 +148,11 @@ Five label values are load-bearing: - **`region`** is the provider's own token, not necessarily one region. For a provider that collapses every declared region into a single candidate (Modal) it is the joined form — the same value `NodeClaimSpec.Region` carries. -- **`reason="unreachable"`** means the provider never told us what it decided: a transport - failure, a 503, an unparseable response. It is the one failure reason for which Nebula - deliberately does *not* fail the Pod or blocklist the candidate (see - `provider.IsRejection`), so a spike here alongside flat `capacity`/`auth` series is a - network or provider-outage problem, not a placement one. +- **`reason="other"`** covers both an adapter that did not wrap its errors and a provider + that never told us what it decided (a transport failure, a 503). Neither blocklists the + candidate on its own, and the two are separated in the `vnode-handler` error log rather + than in the label. A spike here alongside flat `capacity`/`auth` series is the shape of a + network or provider outage. Cardinality is bounded by *configuration*, not by workload: providers x regions x tiers x accelerator pools, all of which come from NodePools and provider catalogs. Nothing diff --git a/pkg/metrics/provision.go b/pkg/metrics/provision.go index 7deb6f6..4470939 100644 --- a/pkg/metrics/provision.go +++ b/pkg/metrics/provision.go @@ -45,19 +45,12 @@ const ( ReasonAuth = "auth" ReasonUnsupported = "unsupported_accelerator" ReasonTimeout = "timeout" - // ReasonUnreachable: the provider never told us what it decided — a transport - // failure, a 503, an unparseable response. Kept separate from every other reason - // because it is the one that is NOT about capacity, credentials or the request: it - // says the integration itself is unhealthy, and it is the only reason for which - // Nebula deliberately does not fail the Pod or blocklist the candidate (see - // provider.IsRejection). A spike here alongside flat capacity/auth series is a - // network or provider-outage signal, not a placement one. - ReasonUnreachable = "unreachable" - // ReasonOther: the provider DID reject the request, but not through a sentinel, so - // the category is unavailable. In practice that means the adapter returned a raw API - // error without wrapping it, which makes a sustained rate on this series a to-do - // rather than an incident: wrap the condition in the adapter (see - // docs/add-a-provider.md) and the failure moves onto its real category. + // ReasonOther: the failure carried no sentinel, so its category is unavailable — either + // the adapter returned a raw API error without wrapping it, or the provider never told + // us what it decided at all (a transport failure, a 503, an unparseable response). A + // sustained rate here is a to-do rather than an incident: wrap the condition in the + // adapter (see docs/add-a-provider.md) and the failure moves onto its real category. + // Which of the two it was is in the vnode-handler error log. ReasonOther = "other" ) @@ -175,12 +168,6 @@ func FailureReason(err error) string { // of the two. case errors.Is(err, context.DeadlineExceeded): return ReasonTimeout - // Everything left is either a rejection whose category we could not name, or a - // failure to reach the provider at all. Splitting them is the whole point of having - // this label: the first is a placement problem, the second an integration one, and - // they are fixed by completely different people. - case !provider.IsRejection(err): - return ReasonUnreachable default: return ReasonOther } diff --git a/pkg/metrics/provision_test.go b/pkg/metrics/provision_test.go index 4122d2e..14a4a5a 100644 --- a/pkg/metrics/provision_test.go +++ b/pkg/metrics/provision_test.go @@ -27,7 +27,7 @@ import ( // The reason label is a CLOSED set, so this pins every mapping into it. The label is // what an operator reads to decide whether a provisioning problem is theirs (quota, -// credentials), the provider's (capacity, unreachable) or neither. +// credentials), the provider's (capacity) or neither. func TestFailureReason(t *testing.T) { tests := []struct { name string @@ -46,17 +46,17 @@ func TestFailureReason(t *testing.T) { {"capacity beats timeout", fmt.Errorf("%w: %w", provider.ErrNoCapacity, context.DeadlineExceeded), ReasonCapacity}, {"bare timeout", context.DeadlineExceeded, ReasonTimeout}, - // The split that matters operationally: an integration outage must not read as a - // capacity shortfall, or the failure series points at the wrong problem entirely. - {"grpc transport", errors.New("rpc error: code = Unavailable desc = transport is closing"), ReasonUnreachable}, - {"connection refused", errors.New("dial tcp: connect: connection refused"), ReasonUnreachable}, - {"unrecognized", errors.New("weird transient blip"), ReasonUnreachable}, + // What matters here is that a transport failure does NOT read as a capacity + // shortfall, which would point the failure series at the wrong problem entirely. + // "Unavailable" in a gRPC status text is the misread this guards. + {"grpc transport", errors.New("rpc error: code = Unavailable desc = transport is closing"), ReasonOther}, + {"connection refused", errors.New("dial tcp: connect: connection refused"), ReasonOther}, + {"unrecognized", errors.New("weird transient blip"), ReasonOther}, - // An unwrapped provider message is recognized as a REJECTION (so not - // "unreachable"), but its category is unavailable here: FailureReason matches - // sentinels only, on purpose, rather than re-running the string heuristics. So it - // lands on "other", which is precisely the signal that an adapter is not wrapping - // its errors — actionable, unlike a guess at the category. + // An unwrapped provider message lands on "other" too: FailureReason matches + // sentinels only, on purpose, rather than re-running the string heuristics. That is + // precisely the signal that an adapter is not wrapping its errors — actionable, + // unlike a guess at the category. {"unwrapped rejection", errors.New("InsufficientInstanceCapacity"), ReasonOther}, } for _, tt := range tests { @@ -93,7 +93,7 @@ func TestObserveProvision_CountersStayInStep(t *testing.T) { // A success must never touch the failure-reason counter, whatever the reason. allReasons := []string{ ReasonCapacity, ReasonAuth, ReasonQuota, - ReasonUnsupported, ReasonTimeout, ReasonUnreachable, ReasonOther, + ReasonUnsupported, ReasonTimeout, ReasonOther, } for _, reason := range allReasons { if reason == ReasonCapacity { diff --git a/pkg/provider/errors.go b/pkg/provider/errors.go index eb130fc..a634a35 100644 --- a/pkg/provider/errors.go +++ b/pkg/provider/errors.go @@ -81,7 +81,7 @@ var ( // // The zero scope is the ONLY signal for "block nothing": recordBlock no-ops on it, so callers // classify every failure and act on the result instead of pre-filtering with a second -// predicate. IsRejection answers a different question and does not gate this one. +// predicate. // // This is the single place the SHARED part of a scope is derived, so every adapter delegates // here and then only adds what is provider-specific (AWS adds its region). Nothing assembles @@ -123,8 +123,8 @@ func ClassifyError(err error, capacityType nebulav1alpha1.CapacityType, accelera // must therefore not decorate this scope (see ClassifyProvisionError in each // adapter): adding a region would make it non-empty and install a region-wide block. // - // Still a rejection, so the Pod fails with the reason rather than retrying forever — - // the two questions are separate. See IsRejection. + // The Pod still fails with the reason rather than retrying forever; that is the + // caller's doing (see vnode.Handler.CreatePod), not this scope's. return BlockScope{} case catUnattributable: // Nothing is blocklisted, and for a different reason than catRequest above: there the @@ -146,16 +146,14 @@ func ClassifyError(err error, capacityType nebulav1alpha1.CapacityType, accelera } } -// failureCategory is the internal classification ClassifyError and IsRejection both -// drive off, so the two can never disagree about whether an error was recognized. +// failureCategory is the internal classification ClassifyError drives off. type failureCategory int const ( // catUnattributable: nothing in the error says what the provider decided, because // it may not have decided anything — a transport failure, a cancellation, an - // unparseable API blip. The only category IsRejection answers false for, and it - // blocklists nothing: no candidate can be held responsible for a failure nobody - // could attribute to it. + // unparseable API blip. It blocklists nothing: no candidate can be held responsible + // for a failure nobody could attribute to it. catUnattributable failureCategory = iota // catAuth: credentials or authorization failed, so nothing on the provider works. catAuth @@ -219,31 +217,6 @@ func categorize(err error) failureCategory { } } -// IsRejection reports whether err is a provider DECISION about this request — "no -// capacity", "over quota", "bad credentials", "I do not offer that accelerator" — as -// opposed to a failure to find out what the provider would have decided: a transport -// error, a cancellation, a 503, an unparseable response. -// -// A provision DEADLINE counts as a decision, though nobody spoke it: the candidate was given -// the entire provision budget and produced no usable instance, which is as good a refusal as -// one it words. Treating it as unknown left the Pod provisioning behind an attempt nothing -// re-enters. A cancellation is the opposite — that is us stopping, not the candidate failing. -// -// It does NOT decide what happens to the Pod, and no longer gates the blocklist. A provision -// failure is terminal either way (see vnode.Handler.CreatePod), and what may be blocklisted is -// answered by ClassifyError alone, whose zero scope means "nothing". Two mechanisms for one -// question could disagree; one cannot. -// -// What it still separates is DIAGNOSIS: a rejection is a placement problem — the provider was -// reached and said no — while an unattributable failure is an integration problem, and the two -// are fixed by different people. That is the split metrics reports on (see -// metrics.provisionReason). -// -// A nil error is not a rejection. -func IsRejection(err error) bool { - return err != nil && categorize(err) != catUnattributable -} - // containsAny reports whether s contains any of subs. func containsAny(s string, subs ...string) bool { for _, sub := range subs { diff --git a/pkg/provider/errors_test.go b/pkg/provider/errors_test.go index 4c7e5df..5b0ef8e 100644 --- a/pkg/provider/errors_test.go +++ b/pkg/provider/errors_test.go @@ -48,7 +48,7 @@ func TestClassifyError(t *testing.T) { // An unusable image credential belongs to the POD. The blocklist key carries no Pod, // image or credential identity, so ANY scope here would exclude the candidate for // unrelated Pods that pull perfectly well — hence the zero scope, which blocks - // nothing. Still a rejection: see TestIsRejection. + // nothing. {"image-pull sentinel blocks nothing", ErrImagePull, BlockScope{}}, { "wrapped image-pull sentinel blocks nothing", @@ -73,6 +73,42 @@ func TestClassifyError(t *testing.T) { // An error we cannot attribute blocks NOTHING: it is no evidence against the // candidate, and the zero scope is what keeps recordBlock from acting on it. {"unknown blocks nothing", fmt.Errorf("weird transient blip"), BlockScope{}}, + + // A DEADLINE is scoped like capacity: the candidate had the whole provision budget + // and produced no usable instance, so the next attempt should go elsewhere instead + // of spending another full budget here. Cancellation is the opposite — that is US + // stopping, no evidence against the candidate — so it blocks nothing. + {"deadline exceeded", context.DeadlineExceeded, capacityScope}, + {"wrapped deadline", fmt.Errorf("provision: %w", context.DeadlineExceeded), capacityScope}, + {"canceled blocks nothing", context.Canceled, BlockScope{}}, + + // OUR clock outranks any label a sentinel carries: an adapter reports the failure it + // saw and cannot see whose deadline fired. So a build that ran out of budget is a + // capacity failure, not the zero-scope image failure its label suggests. + {"deadline wrapped in an image-build label", + fmt.Errorf("modal: image build: %w: %w", context.DeadlineExceeded, ErrImageBuild), capacityScope}, + {"cancellation wrapped in a capacity label", + fmt.Errorf("sweep: %w: %w", context.Canceled, ErrNoCapacity), BlockScope{}}, + // The form errors.Is CANNOT see: grpc-go turns a dead context into a status error + // that wraps nothing, and it is the likelier arrival — an SDK blocked in Recv finds + // out from gRPC, not from its own ctx.Err() poll. + {"grpc deadline wrapping an image-build label", + fmt.Errorf("modal: image build: %w: %w", + errors.New("rpc error: code = DeadlineExceeded desc = context deadline exceeded"), + ErrImageBuild), capacityScope}, + // ...but a verdict Modal actually reached keeps the label's zero scope: the override + // fires only when the context is what died, so a registry refusal must not fence off + // the accelerator. + {"image-build label on a remote verdict", + fmt.Errorf("modal: image build: %w: %w", + errors.New("Image build for im-1 failed with the exception:\nunauthorized"), + ErrImageBuild), BlockScope{}}, + // A gRPC status renders "Unavailable" in its text, which the capacity heuristic + // would otherwise match — the misread the transport check precedes it for. A sentinel + // the adapter wrapped still wins over the raw text, so an adapter that classified a + // gRPC error itself is not second-guessed. + {"grpc unavailable wrapping a sentinel", + fmt.Errorf("rpc error: code = Unavailable desc = no gpu: %w", ErrNoCapacity), capacityScope}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -104,83 +140,6 @@ func TestClassifyError_EmptyAcceleratorStaysNil(t *testing.T) { } } -// IsRejection separates a provider DECISION about the request from a failure to -// learn what it would have decided. The vnode handler fails the Pod and blocklists -// the candidate only for the former, so a transport error misclassified as a -// rejection stamps a terminal status on a request that may have been accepted. -func TestIsRejection(t *testing.T) { - tests := []struct { - name string - err error - want bool - }{ - {"nil is not a rejection", nil, false}, - {"auth sentinel", ErrAuth, true}, - {"no-capacity sentinel", ErrNoCapacity, true}, - {"quota sentinel", ErrQuota, true}, - {"unsupported sentinel", ErrUnsupportedAccelerator, true}, - {"wrapped sentinel", fmt.Errorf("create sandbox: %w", ErrNoCapacity), true}, - {"string capacity", errors.New("InsufficientInstanceCapacity"), true}, - {"string auth", errors.New("HTTP 401 unauthorized"), true}, - // Blocks nothing (see TestClassifyError), yet still a rejection: the provider DID - // decide, and a Pod whose credential it cannot use must fail with that reason rather - // than retry it forever. Blocking and rejecting are separate questions. - {"image-pull sentinel", ErrImagePull, true}, - {"image-build sentinel", ErrImageBuild, true}, - - // A deadline REJECTS: the candidate had the whole provision budget and produced no - // usable instance, so the Pod fails and the candidate is blocked for the TTL, sending - // the next attempt somewhere else instead of spending another full budget here. - {"deadline exceeded", context.DeadlineExceeded, true}, - {"wrapped deadline", fmt.Errorf("provision: %w", context.DeadlineExceeded), true}, - - // The failures this predicate exists for. Cancellation is OUR exit, not the - // candidate's failure, so it stays retryable however the deadline is treated. - {"canceled", context.Canceled, false}, - {"connection refused", errors.New("dial tcp 10.0.0.1:443: connect: connection refused"), false}, - {"eof", errors.New("unexpected EOF"), false}, - {"http 503", errors.New("503 Service Unavailable"), false}, - {"unrecognized", errors.New("weird transient blip"), false}, - - // A gRPC status renders "Unavailable" in its text, which the capacity heuristic - // would otherwise match — this is the misread the transport check precedes it for. - {"grpc unavailable", errors.New("rpc error: code = Unavailable desc = transport is closing"), false}, - // ...but a sentinel the adapter wrapped still wins over the raw text, so an - // adapter that classified a gRPC error itself is not second-guessed. - {"grpc unavailable wrapping a sentinel", - fmt.Errorf("rpc error: code = Unavailable desc = no gpu: %w", ErrNoCapacity), true}, - - // OUR clock outranks any label a sentinel carries: an adapter reports the failure it - // saw and cannot see whose deadline fired. So a build that ran out of budget is a - // capacity rejection, not the zero-scope image failure its label suggests — the - // candidate spent the whole budget and delivered nothing. - {"deadline wrapped in an image-build label", - fmt.Errorf("modal: image build: %w: %w", context.DeadlineExceeded, ErrImageBuild), true}, - {"cancellation wrapped in a capacity label", - fmt.Errorf("sweep: %w: %w", context.Canceled, ErrNoCapacity), false}, - // The form errors.Is CANNOT see: grpc-go turns a dead context into a status error - // that wraps nothing, and it is the likelier arrival — an SDK blocked in Recv finds - // out from gRPC, not from its own ctx.Err() poll. - {"grpc deadline wrapping an image-build label", - fmt.Errorf("modal: image build: %w: %w", - errors.New("rpc error: code = DeadlineExceeded desc = context deadline exceeded"), - ErrImageBuild), true}, - // A verdict Modal actually reached still rejects: the label is only overridden when - // the context is what died. - {"image-build label on a remote verdict", - fmt.Errorf("modal: image build: %w: %w", - errors.New("Image build for im-1 failed with the exception:\nunauthorized"), - ErrImageBuild), true}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := IsRejection(tt.err); got != tt.want { - t.Fatalf("IsRejection(%v) = %v, want %v", tt.err, got, tt.want) - } - }) - } -} - // The scope is the single answer to "should anything be blocked", so an unattributable // error must classify to ZERO — callers hand every failure to recordBlock and rely on this // to record nothing. A narrow-but-non-empty scope here would fence off a candidate for a diff --git a/pkg/provider/modal/modal.go b/pkg/provider/modal/modal.go index 49786bd..04d6141 100644 --- a/pkg/provider/modal/modal.go +++ b/pkg/provider/modal/modal.go @@ -69,9 +69,7 @@ const defaultSandboxTimeout = 24 * time.Hour // provisionTimeout raises the vnode handler's generic Provision deadline, because Provision // here BLOCKS on the image build (see sdkClient.buildImage): a cold image is pulled into // Modal's cache on this call, and the create and credential legs only get what the build -// leaves them. Generous on purpose — at 90s a slow build starved the mint, which is the last -// call of the three and the one with no second chance: it is one-shot with no read-back, so a -// sandbox that misses it is unreachable for good and the Pod fails. +// leaves them. const provisionTimeout = 15 * time.Minute // compile-time assertions that Provider satisfies the interfaces. LogStreamer and diff --git a/pkg/provider/modal/modal_test.go b/pkg/provider/modal/modal_test.go index ab63d8d..15e49d6 100644 --- a/pkg/provider/modal/modal_test.go +++ b/pkg/provider/modal/modal_test.go @@ -993,10 +993,10 @@ func TestClassifyProvisionError_ImageBuildNeverDeniesTheProvider(t *testing.T) { if got := p.ClassifyProvisionError(err, "H100:1", "us-east"); got != (provider.BlockScope{}) { t.Fatalf("an image build failure must block nothing, got %+v", got) } - // Still a rejection, so the Pod fails with the reason instead of retrying an image that - // will never build. Blocking and rejecting are separate questions. - if !provider.IsRejection(err) { - t.Error("an image build failure must be a rejection") + // The label is what keeps it at zero: the registry's "unauthorized" text left to the + // heuristics reads as auth and fences off the whole provider. + if !errors.Is(err, provider.ErrImageBuild) { + t.Error("an image build failure must carry the ErrImageBuild label") } // The registry's text survives for whoever has to fix the Secret. if !strings.Contains(err.Error(), "authentication required") { @@ -1111,8 +1111,8 @@ func TestProvision_FailsWhenCreateCannotMint(t *testing.T) { if res.InstanceID != "" { t.Fatalf("InstanceID = %q, want empty so nothing reads a half-provisioned result", res.InstanceID) } - if !provider.IsRejection(err) { - t.Fatal("a mint failure must be a rejection; otherwise the Pod sits provisioning and is never re-provisioned") + if !errors.Is(err, provider.ErrCredential) { + t.Fatal("a mint failure must carry the ErrCredential label, or its text is left to the heuristics") } if scope := provider.ClassifyError(err, nebulav1alpha1.CapacityOnDemand, "H100:1"); scope != (provider.BlockScope{}) { @@ -1122,10 +1122,10 @@ func TestProvision_FailsWhenCreateCannotMint(t *testing.T) { // The production shape of the same failure: the image build eats the provision budget and the // mint — last of the three legs — dies on the deadline, so the error carries BOTH the sentinel -// and a deadline. It must still classify as a rejection: the sentinel says the outcome is -// known, while a bare deadline would mean "we cannot say", leaving the Pod provisioning -// forever behind a sandbox that can never be given a credential. -func TestProvision_MintDeadlineIsStillARejection(t *testing.T) { +// and a deadline. OUR clock outranks the label: the candidate spent the whole budget and +// delivered nothing, so it is blocked for the TTL and the next attempt goes elsewhere instead +// of spending another full budget here. +func TestProvision_MintDeadlineBlocksTheCandidate(t *testing.T) { f := &fakeClient{ createID: "sb-1", createErr: fmt.Errorf("modal: mint connect credential for sandbox sb-1: %w: %w", @@ -1138,8 +1138,9 @@ func TestProvision_MintDeadlineIsStillARejection(t *testing.T) { if err == nil { t.Fatal("Provision succeeded; the sandbox has no credential") } - if !provider.IsRejection(err) { - t.Fatal("a timed-out mint must be a rejection; the deadline is how it failed, not whether we know it did") + scope := provider.ClassifyError(err, nebulav1alpha1.CapacityOnDemand, "H100:1") + if scope == (provider.BlockScope{}) || scope.DenyAll { + t.Fatalf("BlockScope = %+v, want an accelerator-scoped block for a spent budget", scope) } } diff --git a/pkg/provider/registryauth.go b/pkg/provider/registryauth.go index fdea026..936b977 100644 --- a/pkg/provider/registryauth.go +++ b/pkg/provider/registryauth.go @@ -95,8 +95,8 @@ func (a *RegistryAuth) Validate() error { // Unsupported is the error an adapter returns for a credential kind it cannot honour, named // so the message says which provider refused. Shared so the ErrImagePull wrap cannot be -// forgotten: unwrapped, the same refusal reads as unattributable and the Pod retries against -// a provider that will never accept it (see ErrImagePull, IsRejection). +// forgotten: unwrapped, the same refusal reads as unattributable and the failure is reported +// as an integration problem rather than the request's own (see ErrImagePull). func (a *RegistryAuth) Unsupported(providerName string) error { return fmt.Errorf("%s: unsupported image pull credential %s: %w", providerName, a, ErrImagePull) } diff --git a/pkg/provider/registryauth_test.go b/pkg/provider/registryauth_test.go index 4ea19aa..6298513 100644 --- a/pkg/provider/registryauth_test.go +++ b/pkg/provider/registryauth_test.go @@ -20,6 +20,8 @@ import ( "errors" "strings" "testing" + + nebulav1alpha1 "github.com/InftyAI/Nebula/api/v1alpha1" ) func TestRegistryAuthValidate(t *testing.T) { @@ -94,10 +96,11 @@ func TestRegistryAuthUnsupported(t *testing.T) { if !errors.Is(err, ErrImagePull) { t.Errorf("err = %v, want it to wrap ErrImagePull", err) } - // A refusal must be a rejection, not an unattributable failure: the latter leaves the - // Pod retrying against a provider that will never accept the credential. - if !IsRejection(err) { - t.Errorf("IsRejection(%v) = false, want true", err) + // The refusal is the POD's, so it must blocklist nothing: the candidate accepts other + // Pods' credentials perfectly well, and "unsupported credential" text left to the + // heuristics would read as auth and fence off the whole provider. + if scope := ClassifyError(err, nebulav1alpha1.CapacityOnDemand, "H100:1"); scope != (BlockScope{}) { + t.Errorf("BlockScope = %+v, want zero", scope) } if !strings.Contains(err.Error(), "aws") { t.Errorf("err = %q, want the refusing provider named", err) diff --git a/pkg/vnode/metrics_test.go b/pkg/vnode/metrics_test.go index 1429c7b..c75b474 100644 --- a/pkg/vnode/metrics_test.go +++ b/pkg/vnode/metrics_test.go @@ -128,15 +128,15 @@ func TestCreatePod_RecordsRejectionReason(t *testing.T) { } // An unreachable provider is still a counted ATTEMPT — the call happened and failed — -// even though the handler deliberately does not fail the Pod or blocklist anything for -// it. reason="unreachable" is what distinguishes an integration outage from a capacity -// shortfall, which is the whole reason the two are not both "other". -func TestCreatePod_UnreachableProviderCountedSeparately(t *testing.T) { +// even though the handler blocklists nothing for it. It counts as reason="other", and what +// this pins is that it does NOT count as capacity: "Unavailable" in a gRPC status text must +// not read as a shortfall, or the failure series points at the wrong problem. +func TestCreatePod_UnreachableProviderNotCountedAsCapacity(t *testing.T) { failure := labelsFor("result", metrics.ResultFailure) - unreachable := labelsFor("reason", metrics.ReasonUnreachable) + other := labelsFor("reason", metrics.ReasonOther) capacity := labelsFor("reason", metrics.ReasonCapacity) beforeAttempts := testutil.ToFloat64(metrics.ProvisionAttempts.With(failure)) - beforeUnreachable := testutil.ToFloat64(metrics.ProvisionFailures.With(unreachable)) + beforeOther := testutil.ToFloat64(metrics.ProvisionFailures.With(other)) beforeCapacity := testutil.ToFloat64(metrics.ProvisionFailures.With(capacity)) fp := &fakeProvider{provisionErr: errors.New("rpc error: code = Unavailable desc = transport is closing")} @@ -148,10 +148,9 @@ func TestCreatePod_UnreachableProviderCountedSeparately(t *testing.T) { if got := testutil.ToFloat64(metrics.ProvisionAttempts.With(failure)) - beforeAttempts; got != 1 { t.Fatalf("failure attempts delta = %v, want 1", got) } - if got := testutil.ToFloat64(metrics.ProvisionFailures.With(unreachable)) - beforeUnreachable; got != 1 { - t.Fatalf("unreachable failures delta = %v, want 1", got) + if got := testutil.ToFloat64(metrics.ProvisionFailures.With(other)) - beforeOther; got != 1 { + t.Fatalf("other failures delta = %v, want 1", got) } - // "Unavailable" in the gRPC status text must not be read as a capacity shortfall. if got := testutil.ToFloat64(metrics.ProvisionFailures.With(capacity)) - beforeCapacity; got != 0 { t.Fatalf("capacity failures delta = %v, want 0 for a transport error", got) } From 2298f63c28c2d0f51b85537a92aaf5ae06540b43 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Sat, 29 Aug 2026 23:56:35 +0100 Subject: [PATCH 09/12] reduce timeout Signed-off-by: kerthcet --- pkg/provider/modal/modal.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/provider/modal/modal.go b/pkg/provider/modal/modal.go index 04d6141..914d1a7 100644 --- a/pkg/provider/modal/modal.go +++ b/pkg/provider/modal/modal.go @@ -70,7 +70,7 @@ const defaultSandboxTimeout = 24 * time.Hour // here BLOCKS on the image build (see sdkClient.buildImage): a cold image is pulled into // Modal's cache on this call, and the create and credential legs only get what the build // leaves them. -const provisionTimeout = 15 * time.Minute +const provisionTimeout = 10 * time.Minute // compile-time assertions that Provider satisfies the interfaces. LogStreamer and // Executor are the optional halves: they are what make `kubectl logs` and `kubectl exec` From 0bdd0c0b281550dc4b601d28ce4dba41edd5b188 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Sun, 30 Aug 2026 21:13:32 +0100 Subject: [PATCH 10/12] address comment Signed-off-by: kerthcet --- docs/architecture.md | 8 ++-- docs/metrics.md | 2 +- pkg/metrics/provision.go | 22 +++++------ pkg/metrics/provision_test.go | 9 +++++ pkg/provider/aws/aws_test.go | 4 +- pkg/provider/errors.go | 40 +++++++------------ pkg/provider/errors_test.go | 56 ++++++++++++++++---------- pkg/provider/modal/client.go | 23 ++++++----- pkg/provider/modal/modal_test.go | 65 +++++++++++++------------------ pkg/provider/registryauth.go | 14 +++---- pkg/provider/registryauth_test.go | 10 ++--- pkg/vnode/handler.go | 5 ++- 12 files changed, 131 insertions(+), 127 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 2056798..6fd467c 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -604,10 +604,10 @@ Components designed to degrade without leaks: - **Provider unreachable during `Provision`.** Every provision failure fails the Pod, so placement can fail over rather than sit behind an attempt nothing re-enters. What the failure's *kind* still decides is the blocklist, and `provider.ClassifyError` is the - single answer: a transport error, timeout or 503 files nothing, because it is not - evidence about the candidate — the provider may well have accepted the request. Only a - decision about the candidate (no capacity, quota, unsupported accelerator) files an - entry, and only auth widens it to the whole provider. `Provision` is idempotent on + single answer: a transport error or 503 files nothing, because it is not evidence about + the candidate — the provider may well have accepted the request. An exhausted provision + timeout is candidate-scoped; other candidate decisions (no capacity, quota, unsupported + accelerator) also file an entry, and only auth widens it to the whole provider. `ClaimName`, so a re-provision adopts whatever the failed attempt created rather than doubling it. diff --git a/docs/metrics.md b/docs/metrics.md index 86db61d..dfeac5a 100644 --- a/docs/metrics.md +++ b/docs/metrics.md @@ -87,7 +87,7 @@ What the external call cost, and how it failed. | --- | --- | --- | | `nebula_provision_attempts_total{result}` | counter | Provisioning volume and error rate, per candidate. | | `nebula_provision_failures_total{reason}` | counter | *Why* provisioning fails. | -| `nebula_provision_duration_seconds{result}` | histogram | Latency of the `Provision` call alone. AWS sweeps a region's availability zones inside it, so a capacity shortage shows up as latency *here*; Modal returns as soon as the sandbox is accepted and the wait moves to the next metric. | +| `nebula_provision_duration_seconds{result}` | histogram | Latency of the `Provision` call alone. What lands here is provider-specific: AWS sweeps a region's availability zones inside the call, so a capacity shortage shows up as latency *here*; Modal builds the image inside it, so a build-cache miss does. Bucketed to 600s, the largest `ProvisionTimeout` — so `+Inf` means a call outran its own deadline. | | `nebula_instance_ready_duration_seconds` | histogram | The whole user-visible wait, from `CreatePod` to the first poll tick reporting `Running` — including provider-side queueing, image pull, GPU attach and up to one poll interval of detection lag. | `nebula_provision_failures_total` deliberately overlaps diff --git a/pkg/metrics/provision.go b/pkg/metrics/provision.go index 4470939..b2c6f32 100644 --- a/pkg/metrics/provision.go +++ b/pkg/metrics/provision.go @@ -17,7 +17,6 @@ limitations under the License. package metrics import ( - "context" "errors" "time" @@ -76,17 +75,19 @@ var ( }, withExtra("reason")) // ProvisionDuration measures the provider's Provision call alone — not the - // wait for the instance to become usable. The two differ enormously and for - // different reasons: AWS sweeps a region's availability zones inside this call - // (so a capacity shortage shows up as latency HERE), while Modal returns as soon - // as the sandbox is accepted and the wait moves to InstanceReadyDuration. - // Bucketed out to 300s because the call is bounded by - // Capabilities.ProvisionTimeout, which AWS raises above the 90s default. + // wait for the instance to become usable. What lands here is provider-specific + // and worth knowing per provider: AWS sweeps a region's availability zones + // inside the call (so a capacity shortage shows up as latency HERE), Modal + // builds the image inside it (so a cache miss does). + // + // The top bucket tracks the largest Capabilities.ProvisionTimeout — Modal's 10 + // minutes. A lower ceiling would bury every slow build in +Inf; at 600s, +Inf + // means one thing only: a call outran its own deadline. ProvisionDuration = prometheus.NewHistogramVec(prometheus.HistogramOpts{ Name: "nebula_provision_duration_seconds", Help: "Latency of the provider's Provision call, by provider, region, capacity type, " + "accelerator type, accelerator count and outcome.", - Buckets: []float64{0.5, 1, 2.5, 5, 10, 20, 30, 45, 60, 90, 120, 180, 300}, + Buckets: []float64{0.5, 1, 2.5, 5, 10, 20, 30, 45, 60, 90, 120, 180, 300, 420, 600}, }, withExtra("result")) // InstanceReadyDuration measures the whole user-visible wait: from the moment @@ -163,10 +164,7 @@ func FailureReason(err error) string { return ReasonUnsupported case errors.Is(err, provider.ErrNoCapacity): return ReasonCapacity - // Checked after the sentinels: a provider that hits its own deadline while - // sweeping for capacity may wrap both, and the capacity cause is the more useful - // of the two. - case errors.Is(err, context.DeadlineExceeded): + case provider.IsDeadline(err): return ReasonTimeout default: return ReasonOther diff --git a/pkg/metrics/provision_test.go b/pkg/metrics/provision_test.go index 14a4a5a..1130a56 100644 --- a/pkg/metrics/provision_test.go +++ b/pkg/metrics/provision_test.go @@ -45,6 +45,15 @@ func TestFailureReason(t *testing.T) { // the capacity cause is the more useful of the two, so the sentinels come first. {"capacity beats timeout", fmt.Errorf("%w: %w", provider.ErrNoCapacity, context.DeadlineExceeded), ReasonCapacity}, {"bare timeout", context.DeadlineExceeded, ReasonTimeout}, + // The form errors.Is CANNOT see, and the one a timed-out Modal build actually arrives + // in: grpc-go renders an expired context as a status error wrapping nothing, under the + // adapter's image label. This read as "other" until FailureReason moved onto + // provider.IsDeadline — a real timeout reported as an unwrapped-adapter to-do, which is + // the one thing "other" must not mean. + {"grpc deadline under an image label", + fmt.Errorf("modal: image build: %w: %w", + errors.New("rpc error: code = DeadlineExceeded desc = context deadline exceeded"), + provider.ErrImage), ReasonTimeout}, // What matters here is that a transport failure does NOT read as a capacity // shortfall, which would point the failure series at the wrong problem entirely. diff --git a/pkg/provider/aws/aws_test.go b/pkg/provider/aws/aws_test.go index e9b14c2..c9e0809 100644 --- a/pkg/provider/aws/aws_test.go +++ b/pkg/provider/aws/aws_test.go @@ -574,9 +574,9 @@ func TestClassifyProvisionError(t *testing.T) { // An image pull credential this adapter cannot honour is a fact about the POD, so it // blocks nothing. Region must NOT be stamped on: a scope carrying only a region // fences off every accelerator and tier there, on behalf of one Pod. - {"image pull blocks nothing", provider.ErrImagePull, provider.BlockScope{}}, + {"image pull blocks nothing", provider.ErrImage, provider.BlockScope{}}, {"wrapped image pull blocks nothing", - fmt.Errorf("aws: unsupported image pull credential: %w", provider.ErrImagePull), + fmt.Errorf("aws: unsupported image pull credential: %w", provider.ErrImage), provider.BlockScope{}}, } for _, tt := range tests { diff --git a/pkg/provider/errors.go b/pkg/provider/errors.go index a634a35..7fbfb3e 100644 --- a/pkg/provider/errors.go +++ b/pkg/provider/errors.go @@ -48,27 +48,11 @@ var ( // nothing about the same request in another. The adapter confines it to the // failing region (see aws.ClassifyProvisionError). Transient until quota frees up. ErrQuota = errors.New("provider: quota exceeded") - // ErrImagePull: the image could not be pulled — a credential the provider cannot honour, - // one it was not given, or a registry that refused it. - // - // REQUEST-scoped, and the only sentinel that is: it describes the Pod, not the candidate. - // So it blocklists NOTHING (see ClassifyError). Both alternatives are wrong — ErrAuth - // widens to DenyAll, fencing off the whole provider because one Pod named a role it - // cannot assume, and a capacity scope evicts an accelerator/tier/region that is serving - // other Pods perfectly well, since the blocklist key carries no Pod, image or credential - // identity. A failure that belongs to one request cannot be recorded against a candidate. - ErrImagePull = errors.New("provider: cannot pull image") - // ErrImageBuild: the provider could not PRODUCE the image it was asked to run — - // distinct from ErrImagePull because pulling is only one of the ways it fails. - // - // REQUEST-scoped like ErrImagePull, and for the same reason: WHICH image to run belongs - // to the Pod, so it blocklists NOTHING — no other region, tier or provider produces an - // image this one just refused to produce. - ErrImageBuild = errors.New("provider: cannot build image") - // ErrCredential: the instance came up but could not be given the credential that makes it - // reachable. Minting is create-only with no read-back, so it cannot be repeated for that - // instance — the adapter destroys it and reports this. - ErrCredential = errors.New("provider: cannot mint connect credential") + // ErrImage: the provider could not obtain the image it was asked to run — a credential it + // cannot honour, one it was not given, a registry that refused it, or a build that failed. + // ONE sentinel for all of those because pulling and building are not separable outcomes: + // the provider pulls the image as part of building it (see modal.sdkClient.buildImage). + ErrImage = errors.New("provider: cannot obtain image") ) // ClassifyError maps a provision error to the BlockScope it should be blocklisted at, @@ -146,6 +130,12 @@ func ClassifyError(err error, capacityType nebulav1alpha1.CapacityType, accelera } } +// IsDeadline reports whether err is a provision deadline that fired. +func IsDeadline(err error) bool { + return err != nil && (errors.Is(err, context.DeadlineExceeded) || + strings.Contains(strings.ToLower(err.Error()), "context deadline exceeded")) +} + // failureCategory is the internal classification ClassifyError drives off. type failureCategory int @@ -176,14 +166,14 @@ func categorize(err error) failureCategory { // A deadline is a capacity failure, not an unknown: the candidate was given the whole // provision budget and did not produce a usable instance. - if errors.Is(err, context.DeadlineExceeded) || containsAny(msg, "context deadline exceeded") { + if IsDeadline(err) { return catCapacity } // Cancellation stays unattributable, unlike the deadline above: it means WE stopped asking // — a manager shutdown, a leader handoff — and the provider may well have accepted the - // request. Nothing about the candidate was learned, so failing the Pod or blocklisting - // would punish it for our own exit. + // request. Nothing about the candidate was learned, so blocklisting would punish it for + // our own exit. Only the block scope; the caller still fails the Pod. if errors.Is(err, context.Canceled) || containsAny(msg, "context canceled") { return catUnattributable } @@ -191,7 +181,7 @@ func categorize(err error) failureCategory { switch { case errors.Is(err, ErrAuth): return catAuth - case errors.Is(err, ErrImagePull), errors.Is(err, ErrImageBuild), errors.Is(err, ErrCredential): + case errors.Is(err, ErrImage): return catRequest case errors.Is(err, ErrNoCapacity), errors.Is(err, ErrUnsupportedAccelerator), errors.Is(err, ErrQuota): diff --git a/pkg/provider/errors_test.go b/pkg/provider/errors_test.go index 5b0ef8e..6c3c7df 100644 --- a/pkg/provider/errors_test.go +++ b/pkg/provider/errors_test.go @@ -45,25 +45,22 @@ func TestClassifyError(t *testing.T) { {"quota sentinel", ErrQuota, capacityScope}, {"no-capacity sentinel", ErrNoCapacity, capacityScope}, {"unsupported sentinel", ErrUnsupportedAccelerator, capacityScope}, - // An unusable image credential belongs to the POD. The blocklist key carries no Pod, - // image or credential identity, so ANY scope here would exclude the candidate for - // unrelated Pods that pull perfectly well — hence the zero scope, which blocks - // nothing. - {"image-pull sentinel blocks nothing", ErrImagePull, BlockScope{}}, + // An image the provider cannot obtain belongs to the POD. The blocklist key carries no + // Pod, image or credential identity, so ANY scope here would exclude the candidate for + // unrelated Pods that pull perfectly well — hence the zero scope, which blocks nothing. + {"image sentinel blocks nothing", ErrImage, BlockScope{}}, { - "wrapped image-pull sentinel blocks nothing", - fmt.Errorf("modal: unsupported image pull credential: %w", ErrImagePull), + "unsupported pull credential blocks nothing", + fmt.Errorf("modal: unsupported image pull credential: %w", ErrImage), BlockScope{}, }, - // Same scope as a pull failure, and it must stay that way: the image is the Pod's, - // and a builder that refused this one has nothing to say about the candidate. The - // wrapped case is the shape Modal produces — the SDK's own verdict, then the label - // (see modal.sdkClient.buildImage) — and it is the one that regressed to DenyAll when the - // registry's "unauthorized" text was left to the heuristics. - {"image-build sentinel blocks nothing", ErrImageBuild, BlockScope{}}, + // The one that matters: a build refused by the REGISTRY, which is the shape Modal + // produces — the SDK's own verdict, then the label (see modal.sdkClient.buildImage). + // This regressed to DenyAll once, when the registry's "unauthorized" text was left to + // the heuristics and one Pod's bad Secret fenced off the whole provider. { - "wrapped image-build sentinel blocks nothing", - fmt.Errorf("modal: RemoteError: unauthorized: authentication required: %w", ErrImageBuild), + "registry refusal during a build blocks nothing", + fmt.Errorf("modal: RemoteError: unauthorized: authentication required: %w", ErrImage), BlockScope{}, }, {"wrapped sentinel", fmt.Errorf("provision failed: %w", ErrNoCapacity), capacityScope}, @@ -85,24 +82,41 @@ func TestClassifyError(t *testing.T) { // OUR clock outranks any label a sentinel carries: an adapter reports the failure it // saw and cannot see whose deadline fired. So a build that ran out of budget is a // capacity failure, not the zero-scope image failure its label suggests. - {"deadline wrapped in an image-build label", - fmt.Errorf("modal: image build: %w: %w", context.DeadlineExceeded, ErrImageBuild), capacityScope}, + {"deadline wrapped in an image label", + fmt.Errorf("modal: image build: %w: %w", context.DeadlineExceeded, ErrImage), capacityScope}, {"cancellation wrapped in a capacity label", fmt.Errorf("sweep: %w: %w", context.Canceled, ErrNoCapacity), BlockScope{}}, // The form errors.Is CANNOT see: grpc-go turns a dead context into a status error // that wraps nothing, and it is the likelier arrival — an SDK blocked in Recv finds // out from gRPC, not from its own ctx.Err() poll. - {"grpc deadline wrapping an image-build label", + {"grpc deadline wrapping an image label", fmt.Errorf("modal: image build: %w: %w", errors.New("rpc error: code = DeadlineExceeded desc = context deadline exceeded"), - ErrImageBuild), capacityScope}, + ErrImage), capacityScope}, // ...but a verdict Modal actually reached keeps the label's zero scope: the override // fires only when the context is what died, so a registry refusal must not fence off // the accelerator. - {"image-build label on a remote verdict", + {"image label on a remote verdict", fmt.Errorf("modal: image build: %w: %w", errors.New("Image build for im-1 failed with the exception:\nunauthorized"), - ErrImageBuild), BlockScope{}}, + ErrImage), BlockScope{}}, + // A mint failure carries no label, so it is scoped by its cause. That is the point: the + // mint uses OUR provider credentials, so an auth or rate-limit refusal there is + // provider-wide, and a request scope would fence off nothing while every replacement + // Pod picked the same broken provider. + {"mint failure on an auth refusal", + fmt.Errorf("modal: mint connect credential for sandbox sb-1: %w", + errors.New("unauthorized")), BlockScope{DenyAll: true}}, + {"mint failure on a rate limit", + fmt.Errorf("modal: mint connect credential for sandbox sb-1: %w", + errors.New("429 rate limit exceeded")), capacityScope}, + // Only a mint failure that names nothing recognizable stays at zero. + {"mint failure on a transport error", + fmt.Errorf("modal: mint connect credential for sandbox sb-1: %w", + errors.New("rpc error: code = Unavailable")), BlockScope{}}, + {"mint failure with no cause", errors.New("modal: connect credential minted without a token"), + BlockScope{}}, + // A gRPC status renders "Unavailable" in its text, which the capacity heuristic // would otherwise match — the misread the transport check precedes it for. A sentinel // the adapter wrapped still wins over the raw text, so an adapter that classified a diff --git a/pkg/provider/modal/client.go b/pkg/provider/modal/client.go index f2ce18e..967f604 100644 --- a/pkg/provider/modal/client.go +++ b/pkg/provider/modal/client.go @@ -232,7 +232,7 @@ func (c *sdkClient) imageFor(ctx context.Context, spec SandboxSpec) (*modal.Imag default: // Refuse rather than pull anonymously; see provider.RegistryAuth. - return nil, fmt.Errorf("modal: unsupported image pull credential: %w", provider.ErrImagePull) + return nil, fmt.Errorf("modal: unsupported image pull credential: %w", provider.ErrImage) } } @@ -240,7 +240,7 @@ func (c *sdkClient) imageFor(ctx context.Context, spec SandboxSpec) (*modal.Imag func (c *sdkClient) buildImage(ctx context.Context, app *modal.App, image *modal.Image) (*modal.Image, error) { built, err := image.Build(ctx, app, nil) if err != nil { - return nil, fmt.Errorf("modal: image build: %w: %w", err, provider.ErrImageBuild) + return nil, fmt.Errorf("modal: image build: %w: %w", err, provider.ErrImage) } return built, nil } @@ -265,9 +265,11 @@ func (c *sdkClient) registrySecret(ctx context.Context, kv map[string]string) (* // // A failure is REPORTED, never swallowed into a zero credential. Minting is one-shot with // no read-back, so a dropped error loses the credential of a sandbox that exists and is -// billing — silently, since a caller handed an empty pair has nothing to log. It is tagged -// provider.ErrCredential, which fails the Pod terminally (blocklisting nothing) so its owner -// recreates it — the only recovery, since this sandbox can never be given a credential. +// billing — silently, since a caller handed an empty pair has nothing to log. The Pod fails +// terminally and its owner recreates it — the only recovery, since this sandbox can never be +// given a credential. The API error is passed through unlabelled, because it is what decides +// the blocklist scope: an "unauthorized" here is OUR Modal credential, so it must fence the +// provider off rather than be scoped to one request. func (c *sdkClient) mintCredential(ctx context.Context, sb *modal.Sandbox, port int) (Credential, error) { creds, err := sb.CreateConnectToken(ctx, &modal.SandboxCreateConnectTokenParams{ // Derived from the exposed set rather than carried separately, so the routed @@ -275,14 +277,15 @@ func (c *sdkClient) mintCredential(ctx context.Context, sb *modal.Sandbox, port Port: port, }) if err != nil { - return Credential{}, fmt.Errorf("modal: mint connect credential for sandbox %s on port %d: %w: %w", - sb.SandboxID, port, err, provider.ErrCredential) + return Credential{}, fmt.Errorf("modal: mint connect credential for sandbox %s on port %d: %w", + sb.SandboxID, port, err) } // A token-less success is the same outcome as an error — an address with nothing to - // authenticate against it — so it is reported as one. + // authenticate against it — so it is reported as one. Nothing to wrap: Modal reported + // no failure, so there is no cause to attribute and the candidate is not blocklisted. if creds == nil || creds.Token == "" { - return Credential{}, fmt.Errorf("modal: sandbox %s: connect credential minted without a token: %w", - sb.SandboxID, provider.ErrCredential) + return Credential{}, fmt.Errorf("modal: sandbox %s: connect credential minted without a token", + sb.SandboxID) } return Credential{URL: creds.URL, Token: creds.Token}, nil } diff --git a/pkg/provider/modal/modal_test.go b/pkg/provider/modal/modal_test.go index 15e49d6..099ef23 100644 --- a/pkg/provider/modal/modal_test.go +++ b/pkg/provider/modal/modal_test.go @@ -486,9 +486,9 @@ func TestCapabilities(t *testing.T) { // Must be STATED, and above the handler's generic default: Provision blocks on the image // build here, so this adapter needs more than a provider whose create just returns an id. // Zero is the regression to guard — it silently reverts to that default. - if caps.ProvisionTimeout < 90*time.Second { - t.Fatalf("ProvisionTimeout = %v, want at least 90s to cover an image build", - caps.ProvisionTimeout) + if caps.ProvisionTimeout != provisionTimeout { + t.Fatalf("ProvisionTimeout = %v, want %v to cover a cold image build", + caps.ProvisionTimeout, provisionTimeout) } if p.Name() != provider.ProviderModal { t.Fatalf("name = %q", p.Name()) @@ -966,9 +966,9 @@ func TestClassifyProvisionError_ConfinesToFailingRegion(t *testing.T) { // credential belongs to one POD, so it blocks nothing. Stamping the region here would // make the scope non-empty and fence off every accelerator in us-east because one Pod // named a role Modal cannot assume. - if got := p.ClassifyProvisionError(provider.ErrImagePull, "H100:1", "us-east"); got != + if got := p.ClassifyProvisionError(provider.ErrImage, "H100:1", "us-east"); got != (provider.BlockScope{}) { - t.Fatalf("an image-pull rejection must block nothing, got %+v", got) + t.Fatalf("an image failure must block nothing, got %+v", got) } } @@ -986,7 +986,7 @@ func TestClassifyProvisionError_ImageBuildNeverDeniesTheProvider(t *testing.T) { // keep this file SDK-free — under test is the label, not how buildImage obtained one. verdict := errors.New("RemoteError: Image build for im-1 failed with the exception:\n" + "unauthorized: authentication required") - err := fmt.Errorf("modal: image build: %w: %w", verdict, provider.ErrImageBuild) + err := fmt.Errorf("modal: image build: %w: %w", verdict, provider.ErrImage) // Blocks nothing at all: the image is a property of the request, and no region or tier // builds an image Modal refused to build. @@ -995,22 +995,14 @@ func TestClassifyProvisionError_ImageBuildNeverDeniesTheProvider(t *testing.T) { } // The label is what keeps it at zero: the registry's "unauthorized" text left to the // heuristics reads as auth and fences off the whole provider. - if !errors.Is(err, provider.ErrImageBuild) { - t.Error("an image build failure must carry the ErrImageBuild label") + if !errors.Is(err, provider.ErrImage) { + t.Error("an image build failure must carry the ErrImage label") } - // The registry's text survives for whoever has to fix the Secret. + // The registry's text survives for whoever has to fix the Secret. It is the only thing + // that says WHICH of the image failures this was, now that one sentinel covers them all. if !strings.Contains(err.Error(), "authentication required") { t.Errorf("err = %q, want the registry's reason preserved", err) } - // Named for what actually failed. A build fails on more than pulls — a layer it cannot - // unpack, a manifest it rejects — so ErrImagePull would assert a pull problem the - // adapter has not established, and send whoever reads it to check a credential. - if !errors.Is(err, provider.ErrImageBuild) { - t.Errorf("err must identify as ErrImageBuild, got %v", err) - } - if errors.Is(err, provider.ErrImagePull) { - t.Error("a build failure must not claim to be a pull failure") - } } func TestProvision_CarriesDeclaredPorts(t *testing.T) { @@ -1093,13 +1085,14 @@ func TestProvision_ReturnsMintedCredential(t *testing.T) { // A sandbox that came up but could not be given a credential is unreachable, and nothing // revisits it: minting is one-shot with no read-back, and the idempotent branch below hands -// back no credential. So the create reports a failure and no id, and reports it as a -// REJECTION — the Pod fails terminally and its owner recreates it, which is the only recovery -// left. The scope stays zero, because the candidate served the request correctly. +// back no credential. So the create reports a failure and no id, and the Pod fails terminally +// for its owner to recreate — the only recovery left. This mint named no cause, so it blocks +// nothing; one that names an auth or rate-limit refusal does fence the provider (see +// provider.ClassifyError). func TestProvision_FailsWhenCreateCannotMint(t *testing.T) { f := &fakeClient{ createID: "sb-1", - createErr: fmt.Errorf("modal: mint connect credential for sandbox sb-1: %w", provider.ErrCredential), + createErr: errors.New("modal: sandbox sb-1: connect credential minted without a token"), } p := newTestProvider(f) @@ -1111,9 +1104,6 @@ func TestProvision_FailsWhenCreateCannotMint(t *testing.T) { if res.InstanceID != "" { t.Fatalf("InstanceID = %q, want empty so nothing reads a half-provisioned result", res.InstanceID) } - if !errors.Is(err, provider.ErrCredential) { - t.Fatal("a mint failure must carry the ErrCredential label, or its text is left to the heuristics") - } if scope := provider.ClassifyError(err, nebulav1alpha1.CapacityOnDemand, "H100:1"); scope != (provider.BlockScope{}) { t.Fatalf("BlockScope = %+v, want zero; the request failed, not the candidate", scope) @@ -1121,15 +1111,14 @@ func TestProvision_FailsWhenCreateCannotMint(t *testing.T) { } // The production shape of the same failure: the image build eats the provision budget and the -// mint — last of the three legs — dies on the deadline, so the error carries BOTH the sentinel -// and a deadline. OUR clock outranks the label: the candidate spent the whole budget and -// delivered nothing, so it is blocked for the TTL and the next attempt goes elsewhere instead -// of spending another full budget here. +// mint — last of the three legs — dies on the deadline. The candidate spent the whole budget +// and delivered nothing, so it is blocked for the TTL and the next attempt goes elsewhere +// instead of spending another full budget here. func TestProvision_MintDeadlineBlocksTheCandidate(t *testing.T) { f := &fakeClient{ createID: "sb-1", - createErr: fmt.Errorf("modal: mint connect credential for sandbox sb-1: %w: %w", - fmt.Errorf("rpc error: code = DeadlineExceeded: %w", context.DeadlineExceeded), provider.ErrCredential), + createErr: fmt.Errorf("modal: mint connect credential for sandbox sb-1: %w", + fmt.Errorf("rpc error: code = DeadlineExceeded: %w", context.DeadlineExceeded)), } p := newTestProvider(f) @@ -1669,13 +1658,13 @@ func TestCheckRegistryAuth(t *testing.T) { } // Well-formedness is the shared check's, but it must reach the caller through here — - // and as ErrImagePull, so it fails this Pod rather than blocklisting the whole provider + // and as ErrImage, so it fails this Pod rather than blocklisting the whole provider // the way ErrAuth would. err := checkRegistryAuth(&provider.RegistryAuth{ AWSRole: &provider.AWSRoleAuth{RoleARN: arn}, // no region }) - if !errors.Is(err, provider.ErrImagePull) { - t.Fatalf("err = %v, want ErrImagePull", err) + if !errors.Is(err, provider.ErrImage) { + t.Fatalf("err = %v, want ErrImage", err) } if !strings.HasPrefix(err.Error(), "modal: ") { t.Errorf("err = %q, want the adapter's prefix", err) @@ -1683,8 +1672,8 @@ func TestCheckRegistryAuth(t *testing.T) { // A kind Modal is not wired for must not silently become an anonymous pull. err = checkRegistryAuth(&provider.RegistryAuth{Registry: "gcr.io"}) - if !errors.Is(err, provider.ErrImagePull) { - t.Fatalf("err = %v, want ErrImagePull for an unwired kind", err) + if !errors.Is(err, provider.ErrImage) { + t.Fatalf("err = %v, want ErrImage for an unwired kind", err) } } @@ -1727,8 +1716,8 @@ func TestProvision_CarriesRegistryAuth(t *testing.T) { ClaimName: "claim-bad", RegistryAuth: &provider.RegistryAuth{AWSRole: &provider.AWSRoleAuth{RoleARN: "arn:x"}}, }) - if !errors.Is(err, provider.ErrImagePull) { - t.Fatalf("err = %v, want ErrImagePull", err) + if !errors.Is(err, provider.ErrImage) { + t.Fatalf("err = %v, want ErrImage", err) } if f2.createCnt != 0 { t.Errorf("CreateSandbox called %d times, want 0", f2.createCnt) diff --git a/pkg/provider/registryauth.go b/pkg/provider/registryauth.go index 936b977..eb7dbbf 100644 --- a/pkg/provider/registryauth.go +++ b/pkg/provider/registryauth.go @@ -76,29 +76,29 @@ func (a *RegistryAuth) Validate() error { return nil // no credential is not a malformed one; an anonymous pull is legal case a.AWSRole != nil && a.Basic != nil: return fmt.Errorf("registry auth for %q sets two kinds at once: %w", - a.Registry, ErrImagePull) + a.Registry, ErrImage) case a.AWSRole != nil: if a.AWSRole.RoleARN == "" || a.AWSRole.Region == "" { return fmt.Errorf("AWS role auth for %q needs both a role ARN and a region: %w", - a.Registry, ErrImagePull) + a.Registry, ErrImage) } case a.Basic != nil: if a.Basic.Username == "" || a.Basic.Password == "" { return fmt.Errorf("basic auth for %q needs both a username and a password: %w", - a.Registry, ErrImagePull) + a.Registry, ErrImage) } default: - return fmt.Errorf("registry auth for %q sets no credential: %w", a.Registry, ErrImagePull) + return fmt.Errorf("registry auth for %q sets no credential: %w", a.Registry, ErrImage) } return nil } // Unsupported is the error an adapter returns for a credential kind it cannot honour, named -// so the message says which provider refused. Shared so the ErrImagePull wrap cannot be +// so the message says which provider refused. Shared so the ErrImage wrap cannot be // forgotten: unwrapped, the same refusal reads as unattributable and the failure is reported -// as an integration problem rather than the request's own (see ErrImagePull). +// as an integration problem rather than the request's own (see ErrImage). func (a *RegistryAuth) Unsupported(providerName string) error { - return fmt.Errorf("%s: unsupported image pull credential %s: %w", providerName, a, ErrImagePull) + return fmt.Errorf("%s: unsupported image pull credential %s: %w", providerName, a, ErrImage) } // String names the role ARN — an identifier, and what makes "cannot assume this role" diff --git a/pkg/provider/registryauth_test.go b/pkg/provider/registryauth_test.go index 6298513..f31bcea 100644 --- a/pkg/provider/registryauth_test.go +++ b/pkg/provider/registryauth_test.go @@ -72,10 +72,10 @@ func TestRegistryAuthValidate(t *testing.T) { if err == nil { t.Fatal("Validate() = nil, want an error") } - // The sentinel is the contract: ErrImagePull scopes the block to this Pod's + // The sentinel is the contract: ErrImage scopes the block to this Pod's // request, where ErrAuth would fence off the entire provider. - if !errors.Is(err, ErrImagePull) { - t.Errorf("Validate() = %v, want it to wrap ErrImagePull", err) + if !errors.Is(err, ErrImage) { + t.Errorf("Validate() = %v, want it to wrap ErrImage", err) } if errors.Is(err, ErrAuth) { t.Errorf("Validate() = %v, must NOT wrap ErrAuth (it widens to DenyAll)", err) @@ -93,8 +93,8 @@ func TestRegistryAuthUnsupported(t *testing.T) { Basic: &BasicAuth{Username: "bot", Password: "hunter2"}, }).Unsupported("aws") - if !errors.Is(err, ErrImagePull) { - t.Errorf("err = %v, want it to wrap ErrImagePull", err) + if !errors.Is(err, ErrImage) { + t.Errorf("err = %v, want it to wrap ErrImage", err) } // The refusal is the POD's, so it must blocklist nothing: the candidate accepts other // Pods' credentials perfectly well, and "unsupported credential" text left to the diff --git a/pkg/vnode/handler.go b/pkg/vnode/handler.go index bc0dc2f..1dd57c4 100644 --- a/pkg/vnode/handler.go +++ b/pkg/vnode/handler.go @@ -300,8 +300,9 @@ func (h *Handler) CreatePod(ctx context.Context, pod *corev1.Pod) error { // would reap a workload over a race, and would run recordBlock's failover machinery over // a Pod-spec problem no other provider or region can fix. // - // Deliberately NOT stored, as in the transport-failure branch below: a tracked pod with - // no instance id reads as absent from List and gets written Terminated. + // Deliberately NOT stored: tracking a pod makes GetPod return non-nil, which suppresses + // the retry this branch wants. (The provision-failure branch below stores for exactly + // that reason — it does not want one.) env, err := resolveEnv(ctx, h.client, pod) if err != nil { log.Error(err, "cannot resolve the Pod's environment; nothing provisioned, retrying") From 09ce2a175d728659dc9dca4c9c24ee835515bd3f Mon Sep 17 00:00:00 2001 From: kerthcet Date: Sun, 30 Aug 2026 21:20:00 +0100 Subject: [PATCH 11/12] fix comment Signed-off-by: kerthcet --- pkg/provider/errors.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/provider/errors.go b/pkg/provider/errors.go index 7fbfb3e..60c00f9 100644 --- a/pkg/provider/errors.go +++ b/pkg/provider/errors.go @@ -50,8 +50,6 @@ var ( ErrQuota = errors.New("provider: quota exceeded") // ErrImage: the provider could not obtain the image it was asked to run — a credential it // cannot honour, one it was not given, a registry that refused it, or a build that failed. - // ONE sentinel for all of those because pulling and building are not separable outcomes: - // the provider pulls the image as part of building it (see modal.sdkClient.buildImage). ErrImage = errors.New("provider: cannot obtain image") ) From 094ac50e7af9dd67c9cbf194a1e646007ce68172 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Sun, 30 Aug 2026 21:51:01 +0100 Subject: [PATCH 12/12] fix comment Signed-off-by: kerthcet --- pkg/metrics/provision_test.go | 15 ++++---- pkg/provider/aws/aws_test.go | 8 ++-- pkg/provider/errors.go | 32 +++++++++++++--- pkg/provider/errors_test.go | 64 ++++++++++++++++++++----------- pkg/provider/modal/client.go | 4 +- pkg/provider/modal/modal_test.go | 56 +++++++++++++++++---------- pkg/provider/registryauth.go | 23 +++++------ pkg/provider/registryauth_test.go | 18 ++++----- 8 files changed, 136 insertions(+), 84 deletions(-) diff --git a/pkg/metrics/provision_test.go b/pkg/metrics/provision_test.go index 1130a56..27359e2 100644 --- a/pkg/metrics/provision_test.go +++ b/pkg/metrics/provision_test.go @@ -46,14 +46,13 @@ func TestFailureReason(t *testing.T) { {"capacity beats timeout", fmt.Errorf("%w: %w", provider.ErrNoCapacity, context.DeadlineExceeded), ReasonCapacity}, {"bare timeout", context.DeadlineExceeded, ReasonTimeout}, // The form errors.Is CANNOT see, and the one a timed-out Modal build actually arrives - // in: grpc-go renders an expired context as a status error wrapping nothing, under the - // adapter's image label. This read as "other" until FailureReason moved onto - // provider.IsDeadline — a real timeout reported as an unwrapped-adapter to-do, which is - // the one thing "other" must not mean. - {"grpc deadline under an image label", - fmt.Errorf("modal: image build: %w: %w", - errors.New("rpc error: code = DeadlineExceeded desc = context deadline exceeded"), - provider.ErrImage), ReasonTimeout}, + // in: grpc-go renders an expired context as a status error wrapping nothing. This read + // as "other" until FailureReason moved onto provider.IsDeadline — a real timeout + // reported as an unwrapped-adapter to-do, which is the one thing "other" must not mean. + {"grpc deadline during an image build", + fmt.Errorf("modal: image build: %w", + errors.New("rpc error: code = DeadlineExceeded desc = context deadline exceeded")), + ReasonTimeout}, // What matters here is that a transport failure does NOT read as a capacity // shortfall, which would point the failure series at the wrong problem entirely. diff --git a/pkg/provider/aws/aws_test.go b/pkg/provider/aws/aws_test.go index c9e0809..9ea5217 100644 --- a/pkg/provider/aws/aws_test.go +++ b/pkg/provider/aws/aws_test.go @@ -574,9 +574,11 @@ func TestClassifyProvisionError(t *testing.T) { // An image pull credential this adapter cannot honour is a fact about the POD, so it // blocks nothing. Region must NOT be stamped on: a scope carrying only a region // fences off every accelerator and tier there, on behalf of one Pod. - {"image pull blocks nothing", provider.ErrImage, provider.BlockScope{}}, - {"wrapped image pull blocks nothing", - fmt.Errorf("aws: unsupported image pull credential: %w", provider.ErrImage), + {"unsupported image pull blocks nothing", + (&provider.RegistryAuth{Registry: "ghcr.io"}).Unsupported("aws"), + provider.BlockScope{}}, + {"malformed image pull blocks nothing", + (&provider.RegistryAuth{Registry: "ghcr.io", Basic: &provider.BasicAuth{Username: "u"}}).Validate(), provider.BlockScope{}}, } for _, tt := range tests { diff --git a/pkg/provider/errors.go b/pkg/provider/errors.go index 60c00f9..aba8213 100644 --- a/pkg/provider/errors.go +++ b/pkg/provider/errors.go @@ -48,9 +48,6 @@ var ( // nothing about the same request in another. The adapter confines it to the // failing region (see aws.ClassifyProvisionError). Transient until quota frees up. ErrQuota = errors.New("provider: quota exceeded") - // ErrImage: the provider could not obtain the image it was asked to run — a credential it - // cannot honour, one it was not given, a registry that refused it, or a build that failed. - ErrImage = errors.New("provider: cannot obtain image") ) // ClassifyError maps a provision error to the BlockScope it should be blocklisted at, @@ -179,13 +176,35 @@ func categorize(err error) failureCategory { switch { case errors.Is(err, ErrAuth): return catAuth - case errors.Is(err, ErrImage): - return catRequest case errors.Is(err, ErrNoCapacity), errors.Is(err, ErrUnsupportedAccelerator), errors.Is(err, ErrQuota): return catCapacity } + // An image the provider cannot obtain belongs to the REQUEST. This MUST precede the auth + // checks below, because both failures render the same word: a registry's "unauthorized" is + // the Pod's own pull credential, and reading it as ours would fence the whole provider for + // one bad Secret. Two phrase families, no sentinel: + // - "image pull credential": ours (see RegistryAuth.Validate and Unsupported). + // - "image build for": the Modal SDK's remote build verdict, the only thing it says when + // the build itself reached a decision. An API error from the same call does NOT carry + // it, which is what keeps an expired workspace token classifiable as auth below. + if containsAny(msg, "image pull credential", "image build for") { + return catRequest + } + + // A gRPC status code is a VERDICT, so it outranks the transport catch-all below — "rpc + // error" alone would file an expired provider credential as unattributable, fencing nothing + // while every replacement Pod retried the same broken provider. + if strings.Contains(msg, "rpc error") { + switch { + case containsAny(msg, "code = unauthenticated", "code = permissiondenied"): + return catAuth + case strings.Contains(msg, "code = resourceexhausted"): + return catCapacity + } + } + if containsAny(msg, "rpc error", "connection refused", "connection reset", "broken pipe", "no such host", "i/o timeout", "eof", "tls handshake", @@ -194,7 +213,8 @@ func categorize(err error) failureCategory { } switch { - case containsAny(msg, "unauthorized", "forbidden", "authentication", "invalid token", "api key"): + case containsAny(msg, "unauthorized", "forbidden", "authentication", + "unauthenticated", "invalid token", "api key"): return catAuth case containsAny(msg, "quota", "limit exceeded", "rate limit"): return catCapacity diff --git a/pkg/provider/errors_test.go b/pkg/provider/errors_test.go index 6c3c7df..cb68d72 100644 --- a/pkg/provider/errors_test.go +++ b/pkg/provider/errors_test.go @@ -48,21 +48,46 @@ func TestClassifyError(t *testing.T) { // An image the provider cannot obtain belongs to the POD. The blocklist key carries no // Pod, image or credential identity, so ANY scope here would exclude the candidate for // unrelated Pods that pull perfectly well — hence the zero scope, which blocks nothing. - {"image sentinel blocks nothing", ErrImage, BlockScope{}}, + // Recognized by PHRASE, not a sentinel: these two families are the contract, so the + // wording in RegistryAuth and the Modal SDK's build verdict cannot drift out from under + // the classifier without one of these rows failing. { "unsupported pull credential blocks nothing", - fmt.Errorf("modal: unsupported image pull credential: %w", ErrImage), + (&RegistryAuth{Registry: "ghcr.io"}).Unsupported("modal"), BlockScope{}, }, - // The one that matters: a build refused by the REGISTRY, which is the shape Modal - // produces — the SDK's own verdict, then the label (see modal.sdkClient.buildImage). - // This regressed to DenyAll once, when the registry's "unauthorized" text was left to - // the heuristics and one Pod's bad Secret fenced off the whole provider. + { + "malformed pull credential blocks nothing", + (&RegistryAuth{Registry: "ghcr.io", Basic: &BasicAuth{Username: "u"}}).Validate(), + BlockScope{}, + }, + // The one that matters: a build refused by the REGISTRY, in the exact shape Modal + // produces — modal.RemoteError's text, wrapped by buildImage. This regressed to DenyAll + // once, when the registry's "unauthorized" was left to the heuristics and one Pod's bad + // Secret fenced off the whole provider. { "registry refusal during a build blocks nothing", - fmt.Errorf("modal: RemoteError: unauthorized: authentication required: %w", ErrImage), + fmt.Errorf("modal: image build: %w", errors.New( + "RemoteError: Image build for im-1 failed with the exception:\n"+ + "unauthorized: authentication required")), BlockScope{}, }, + // The counterpart, and the reason the phrase has to be specific: the SAME call can fail + // because MODAL refused us. That carries no build verdict, so it must reach the auth + // classification and fence the provider — otherwise an expired workspace token blocks + // nothing and every replacement Pod retries it. + { + "modal refusing us during a build fences the provider", + fmt.Errorf("modal: image build: %w", errors.New( + "rpc error: code = Unauthenticated desc = invalid token")), + BlockScope{DenyAll: true}, + }, + { + "modal rate-limiting us during a build is capacity-scoped", + fmt.Errorf("modal: image build: %w", errors.New( + "rpc error: code = ResourceExhausted desc = too many requests")), + capacityScope, + }, {"wrapped sentinel", fmt.Errorf("provision failed: %w", ErrNoCapacity), capacityScope}, {"string unauthorized", fmt.Errorf("HTTP 401 unauthorized"), BlockScope{DenyAll: true}}, {"string quota", fmt.Errorf("account limit exceeded"), capacityScope}, @@ -79,27 +104,20 @@ func TestClassifyError(t *testing.T) { {"wrapped deadline", fmt.Errorf("provision: %w", context.DeadlineExceeded), capacityScope}, {"canceled blocks nothing", context.Canceled, BlockScope{}}, - // OUR clock outranks any label a sentinel carries: an adapter reports the failure it - // saw and cannot see whose deadline fired. So a build that ran out of budget is a - // capacity failure, not the zero-scope image failure its label suggests. - {"deadline wrapped in an image label", - fmt.Errorf("modal: image build: %w: %w", context.DeadlineExceeded, ErrImage), capacityScope}, + // OUR clock outranks whatever the failure looks like: an adapter reports what it saw and + // cannot see whose deadline fired. So a build that ran out of budget is a capacity + // failure, not the zero-scope image failure its wrapper suggests. + {"deadline during an image build", + fmt.Errorf("modal: image build: %w", context.DeadlineExceeded), capacityScope}, {"cancellation wrapped in a capacity label", fmt.Errorf("sweep: %w: %w", context.Canceled, ErrNoCapacity), BlockScope{}}, // The form errors.Is CANNOT see: grpc-go turns a dead context into a status error // that wraps nothing, and it is the likelier arrival — an SDK blocked in Recv finds // out from gRPC, not from its own ctx.Err() poll. - {"grpc deadline wrapping an image label", - fmt.Errorf("modal: image build: %w: %w", - errors.New("rpc error: code = DeadlineExceeded desc = context deadline exceeded"), - ErrImage), capacityScope}, - // ...but a verdict Modal actually reached keeps the label's zero scope: the override - // fires only when the context is what died, so a registry refusal must not fence off - // the accelerator. - {"image label on a remote verdict", - fmt.Errorf("modal: image build: %w: %w", - errors.New("Image build for im-1 failed with the exception:\nunauthorized"), - ErrImage), BlockScope{}}, + {"grpc deadline during an image build", + fmt.Errorf("modal: image build: %w", + errors.New("rpc error: code = DeadlineExceeded desc = context deadline exceeded")), + capacityScope}, // A mint failure carries no label, so it is scoped by its cause. That is the point: the // mint uses OUR provider credentials, so an auth or rate-limit refusal there is // provider-wide, and a request scope would fence off nothing while every replacement diff --git a/pkg/provider/modal/client.go b/pkg/provider/modal/client.go index 967f604..b6d4557 100644 --- a/pkg/provider/modal/client.go +++ b/pkg/provider/modal/client.go @@ -232,7 +232,7 @@ func (c *sdkClient) imageFor(ctx context.Context, spec SandboxSpec) (*modal.Imag default: // Refuse rather than pull anonymously; see provider.RegistryAuth. - return nil, fmt.Errorf("modal: unsupported image pull credential: %w", provider.ErrImage) + return nil, a.Unsupported("modal") } } @@ -240,7 +240,7 @@ func (c *sdkClient) imageFor(ctx context.Context, spec SandboxSpec) (*modal.Imag func (c *sdkClient) buildImage(ctx context.Context, app *modal.App, image *modal.Image) (*modal.Image, error) { built, err := image.Build(ctx, app, nil) if err != nil { - return nil, fmt.Errorf("modal: image build: %w: %w", err, provider.ErrImage) + return nil, fmt.Errorf("modal: image build: %w", err) } return built, nil } diff --git a/pkg/provider/modal/modal_test.go b/pkg/provider/modal/modal_test.go index 099ef23..8fb3d3b 100644 --- a/pkg/provider/modal/modal_test.go +++ b/pkg/provider/modal/modal_test.go @@ -966,7 +966,8 @@ func TestClassifyProvisionError_ConfinesToFailingRegion(t *testing.T) { // credential belongs to one POD, so it blocks nothing. Stamping the region here would // make the scope non-empty and fence off every accelerator in us-east because one Pod // named a role Modal cannot assume. - if got := p.ClassifyProvisionError(provider.ErrImage, "H100:1", "us-east"); got != + unusable := (&provider.RegistryAuth{Registry: "ghcr.io"}).Unsupported("modal") + if got := p.ClassifyProvisionError(unusable, "H100:1", "us-east"); got != (provider.BlockScope{}) { t.Fatalf("an image failure must block nothing, got %+v", got) } @@ -981,28 +982,37 @@ func TestClassifyProvisionError_ConfinesToFailingRegion(t *testing.T) { func TestClassifyProvisionError_ImageBuildNeverDeniesTheProvider(t *testing.T) { p := newTestProvider(&fakeClient{}) - // The shape buildImage produces: Modal's verdict, carrying the registry's text, wrapped - // with the sentinel. Spelled as the verdict's rendered text rather than the SDK type, to - // keep this file SDK-free — under test is the label, not how buildImage obtained one. + // The shape buildImage produces: Modal's own verdict, carrying the registry's text. + // Spelled as the verdict's rendered text rather than the SDK type, to keep this file + // SDK-free — under test is the classification, not how buildImage obtained the error. verdict := errors.New("RemoteError: Image build for im-1 failed with the exception:\n" + "unauthorized: authentication required") - err := fmt.Errorf("modal: image build: %w: %w", verdict, provider.ErrImage) + err := fmt.Errorf("modal: image build: %w", verdict) // Blocks nothing at all: the image is a property of the request, and no region or tier // builds an image Modal refused to build. if got := p.ClassifyProvisionError(err, "H100:1", "us-east"); got != (provider.BlockScope{}) { t.Fatalf("an image build failure must block nothing, got %+v", got) } - // The label is what keeps it at zero: the registry's "unauthorized" text left to the - // heuristics reads as auth and fences off the whole provider. - if !errors.Is(err, provider.ErrImage) { - t.Error("an image build failure must carry the ErrImage label") + // The verdict's own phrasing is what keeps it at zero, now that no sentinel does. If the + // SDK ever rewords this line, the registry's "unauthorized" falls through to the auth + // heuristic and fences the provider — so pin the phrase the classifier depends on. + if !strings.Contains(err.Error(), "Image build for") { + t.Errorf("err = %q, want the SDK's build-verdict phrasing preserved", err) } - // The registry's text survives for whoever has to fix the Secret. It is the only thing - // that says WHICH of the image failures this was, now that one sentinel covers them all. + // The registry's text survives for whoever has to fix the Secret. if !strings.Contains(err.Error(), "authentication required") { t.Errorf("err = %q, want the registry's reason preserved", err) } + + // The counterpart, and why the phrase must be specific: the SAME call fails this way when + // MODAL refuses US. No build verdict, so it must NOT be excused as the request's problem — + // an expired workspace token has to fence the provider, or every replacement Pod retries it. + refused := fmt.Errorf("modal: image build: %w", + errors.New("rpc error: code = Unauthenticated desc = invalid token")) + if got := p.ClassifyProvisionError(refused, "H100:1", "us-east"); !got.DenyAll { + t.Fatalf("Modal refusing our credentials must fence the provider, got %+v", got) + } } func TestProvision_CarriesDeclaredPorts(t *testing.T) { @@ -1657,14 +1667,15 @@ func TestCheckRegistryAuth(t *testing.T) { t.Fatalf("basic: %v", err) } - // Well-formedness is the shared check's, but it must reach the caller through here — - // and as ErrImage, so it fails this Pod rather than blocklisting the whole provider - // the way ErrAuth would. + // Well-formedness is the shared check's, but it must reach the caller through here — and + // scope the block to this Pod rather than fencing the whole provider the way an auth + // reading would. err := checkRegistryAuth(&provider.RegistryAuth{ AWSRole: &provider.AWSRoleAuth{RoleARN: arn}, // no region }) - if !errors.Is(err, provider.ErrImage) { - t.Fatalf("err = %v, want ErrImage", err) + if got := provider.ClassifyError(err, nebulav1alpha1.CapacityOnDemand, "H100:1"); got != + (provider.BlockScope{}) { + t.Fatalf("err = %v, BlockScope = %+v, want zero", err, got) } if !strings.HasPrefix(err.Error(), "modal: ") { t.Errorf("err = %q, want the adapter's prefix", err) @@ -1672,8 +1683,12 @@ func TestCheckRegistryAuth(t *testing.T) { // A kind Modal is not wired for must not silently become an anonymous pull. err = checkRegistryAuth(&provider.RegistryAuth{Registry: "gcr.io"}) - if !errors.Is(err, provider.ErrImage) { - t.Fatalf("err = %v, want ErrImage for an unwired kind", err) + if err == nil { + t.Fatal("checkRegistryAuth() = nil for an unwired kind, want an error") + } + if got := provider.ClassifyError(err, nebulav1alpha1.CapacityOnDemand, "H100:1"); got != + (provider.BlockScope{}) { + t.Fatalf("err = %v, BlockScope = %+v, want zero for an unwired kind", err, got) } } @@ -1716,8 +1731,9 @@ func TestProvision_CarriesRegistryAuth(t *testing.T) { ClaimName: "claim-bad", RegistryAuth: &provider.RegistryAuth{AWSRole: &provider.AWSRoleAuth{RoleARN: "arn:x"}}, }) - if !errors.Is(err, provider.ErrImage) { - t.Fatalf("err = %v, want ErrImage", err) + if got := provider.ClassifyError(err, nebulav1alpha1.CapacityOnDemand, "H100:1"); got != + (provider.BlockScope{}) { + t.Fatalf("err = %v, BlockScope = %+v, want zero", err, got) } if f2.createCnt != 0 { t.Errorf("CreateSandbox called %d times, want 0", f2.createCnt) diff --git a/pkg/provider/registryauth.go b/pkg/provider/registryauth.go index eb7dbbf..9f6196d 100644 --- a/pkg/provider/registryauth.go +++ b/pkg/provider/registryauth.go @@ -74,31 +74,32 @@ func (a *RegistryAuth) Validate() error { switch { case a == nil: return nil // no credential is not a malformed one; an anonymous pull is legal + // Every message says "image pull credential" on purpose: that phrase is what + // ClassifyError matches to scope the failure to this Pod instead of the candidate. case a.AWSRole != nil && a.Basic != nil: - return fmt.Errorf("registry auth for %q sets two kinds at once: %w", - a.Registry, ErrImage) + return fmt.Errorf("image pull credential for %q sets two kinds at once", a.Registry) case a.AWSRole != nil: if a.AWSRole.RoleARN == "" || a.AWSRole.Region == "" { - return fmt.Errorf("AWS role auth for %q needs both a role ARN and a region: %w", - a.Registry, ErrImage) + return fmt.Errorf("image pull credential for %q (AWS role) needs both a role ARN and a region", + a.Registry) } case a.Basic != nil: if a.Basic.Username == "" || a.Basic.Password == "" { - return fmt.Errorf("basic auth for %q needs both a username and a password: %w", - a.Registry, ErrImage) + return fmt.Errorf("image pull credential for %q (basic auth) needs both a username and a password", + a.Registry) } default: - return fmt.Errorf("registry auth for %q sets no credential: %w", a.Registry, ErrImage) + return fmt.Errorf("image pull credential for %q sets no credential", a.Registry) } return nil } // Unsupported is the error an adapter returns for a credential kind it cannot honour, named -// so the message says which provider refused. Shared so the ErrImage wrap cannot be -// forgotten: unwrapped, the same refusal reads as unattributable and the failure is reported -// as an integration problem rather than the request's own (see ErrImage). +// so the message says which provider refused. Shared so the phrase ClassifyError keys on +// ("image pull credential") cannot be reworded away in one adapter: without it the same +// refusal reads as OUR auth failing and fences the whole provider. func (a *RegistryAuth) Unsupported(providerName string) error { - return fmt.Errorf("%s: unsupported image pull credential %s: %w", providerName, a, ErrImage) + return fmt.Errorf("%s: unsupported image pull credential %s", providerName, a) } // String names the role ARN — an identifier, and what makes "cannot assume this role" diff --git a/pkg/provider/registryauth_test.go b/pkg/provider/registryauth_test.go index f31bcea..693eefc 100644 --- a/pkg/provider/registryauth_test.go +++ b/pkg/provider/registryauth_test.go @@ -17,7 +17,6 @@ limitations under the License. package provider import ( - "errors" "strings" "testing" @@ -72,13 +71,13 @@ func TestRegistryAuthValidate(t *testing.T) { if err == nil { t.Fatal("Validate() = nil, want an error") } - // The sentinel is the contract: ErrImage scopes the block to this Pod's - // request, where ErrAuth would fence off the entire provider. - if !errors.Is(err, ErrImage) { - t.Errorf("Validate() = %v, want it to wrap ErrImage", err) - } - if errors.Is(err, ErrAuth) { - t.Errorf("Validate() = %v, must NOT wrap ErrAuth (it widens to DenyAll)", err) + // The resulting SCOPE is the contract, and the message text is how it is + // reached: a malformed credential is this Pod's problem, so it must blocklist + // nothing. Reword these messages without "image pull credential" and the same + // refusal reads as auth and fences the entire provider — which is what this + // asserts, since there is no sentinel left to carry the intent. + if scope := ClassifyError(err, nebulav1alpha1.CapacityOnDemand, "H100:1"); scope != (BlockScope{}) { + t.Errorf("Validate() = %v, BlockScope = %+v, want zero", err, scope) } if strings.Contains(err.Error(), "hunter2") { t.Errorf("Validate() = %q, must not leak the password", err) @@ -93,9 +92,6 @@ func TestRegistryAuthUnsupported(t *testing.T) { Basic: &BasicAuth{Username: "bot", Password: "hunter2"}, }).Unsupported("aws") - if !errors.Is(err, ErrImage) { - t.Errorf("err = %v, want it to wrap ErrImage", err) - } // The refusal is the POD's, so it must blocklist nothing: the candidate accepts other // Pods' credentials perfectly well, and "unsupported credential" text left to the // heuristics would read as auth and fence off the whole provider.