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/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..6fd467c 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 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. 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..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 @@ -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..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" @@ -45,19 +44,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" ) @@ -83,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 @@ -170,17 +164,8 @@ 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 - // 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..27359e2 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 @@ -45,18 +45,26 @@ 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. 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}, - // 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 +101,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/aws/aws_test.go b/pkg/provider/aws/aws_test.go index e42b02a..9ea5217 100644 --- a/pkg/provider/aws/aws_test.go +++ b/pkg/provider/aws/aws_test.go @@ -559,25 +559,26 @@ 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 // fences off every accelerator and tier there, on behalf of one Pod. - {"image pull blocks nothing", provider.ErrImagePull, provider.BlockScope{}}, - {"wrapped image pull blocks nothing", - fmt.Errorf("aws: unsupported image pull credential: %w", provider.ErrImagePull), + {"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 2b03579..aba8213 100644 --- a/pkg/provider/errors.go +++ b/pkg/provider/errors.go @@ -48,25 +48,19 @@ 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") ) // 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. // // 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 @@ -108,82 +102,119 @@ 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{} - 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() } } -// failureCategory is the internal classification ClassifyError and IsRejection both -// drive off, so the two can never disagree about whether an error was recognized. +// 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 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. 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 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 ) -// categorize buckets a provision error, sentinels first and string heuristics after. +// 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 { - // 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()) + + // 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 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 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 + } + switch { case errors.Is(err, ErrAuth): return catAuth - case errors.Is(err, ErrImagePull): - return catRequest case errors.Is(err, ErrNoCapacity), errors.Is(err, ErrUnsupportedAccelerator), errors.Is(err, ErrQuota): return catCapacity } - msg := strings.ToLower(err.Error()) + // 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 + } - // 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, + // 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", - "service unavailable", "bad gateway", "gateway timeout", "internal server error"): + "service unavailable", "bad gateway", "gateway timeout", "internal server error") { return catUnattributable } 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 @@ -194,28 +225,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 timeout, a 503, an unparseable response. -// -// 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. -// -// 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 330c854..cb68d72 100644 --- a/pkg/provider/errors_test.go +++ b/pkg/provider/errors_test.go @@ -45,24 +45,102 @@ 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. Still a rejection: see TestIsRejection. - {"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. + // 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. { - "wrapped image-pull sentinel blocks nothing", - fmt.Errorf("modal: unsupported image pull credential: %w", ErrImagePull), + "unsupported pull credential blocks nothing", + (&RegistryAuth{Registry: "ghcr.io"}).Unsupported("modal"), BlockScope{}, }, + { + "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: 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}, {"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{}}, + + // 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 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 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 + // 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 + // 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) { @@ -94,69 +172,16 @@ 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}, - - // The failures this predicate exists for. - {"deadline exceeded", context.DeadlineExceeded, false}, - {"wrapped deadline", fmt.Errorf("provision: %w", context.DeadlineExceeded), false}, - {"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}, - } - 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) - } - }) - } -} - -// 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 34d5414..b6d4557 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 { @@ -184,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, spec), 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 @@ -222,8 +232,17 @@ 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, a.Unsupported("modal") + } +} + +// buildImage hydrates the image, which is where Modal pulls it into its own cache. +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", err) } + return built, nil } // registrySecret builds a lazily-hydrated ephemeral Modal Secret for these values. @@ -244,38 +263,41 @@ func (c *sdkClient) registrySecret(ctx context.Context, kv map[string]string) (* // 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 { +// 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. 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 // 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{} + 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. 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", + sb.SandboxID) + } + 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. +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) } - return Credential{URL: creds.URL, Token: creds.Token} + 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 b7f66b5..914d1a7 100644 --- a/pkg/provider/modal/modal.go +++ b/pkg/provider/modal/modal.go @@ -66,6 +66,12 @@ import ( // different ceiling sets spec.activeDeadlineSeconds, which maps straight through. 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. +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` // work here, and asserting them separately is the point — a provider is free to serve @@ -81,11 +87,16 @@ 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. + // 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 + // 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 @@ -323,12 +334,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 } } @@ -360,11 +372,9 @@ 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. + // 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 { diff --git a/pkg/provider/modal/modal_test.go b/pkg/provider/modal/modal_test.go index d3c65b9..8fb3d3b 100644 --- a/pkg/provider/modal/modal_test.go +++ b/pkg/provider/modal/modal_test.go @@ -46,9 +46,18 @@ 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. + // 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 @@ -446,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 { @@ -465,6 +483,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 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 != 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()) } @@ -941,9 +966,52 @@ 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 != + unusable := (&provider.RegistryAuth{Registry: "ghcr.io"}).Unsupported("modal") + if got := p.ClassifyProvisionError(unusable, "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) + } +} + +// 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 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", 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 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. + 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) } } @@ -1025,37 +1093,76 @@ 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 +// 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 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: errors.New("modal: sandbox sb-1: connect credential minted without a token"), + } 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 scope := provider.ClassifyError(err, nebulav1alpha1.CapacityOnDemand, "H100:1"); scope != + (provider.BlockScope{}) { + t.Fatalf("BlockScope = %+v, want zero; the request failed, not the candidate", scope) } } -// 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) { +// 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. 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", + fmt.Errorf("rpc error: code = DeadlineExceeded: %w", context.DeadlineExceeded)), + } + 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") + } + 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) + } +} + +// 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 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) res, err := p.Provision(context.Background(), gpuPod("claim-a", "H100", 1), @@ -1066,6 +1173,15 @@ func TestProvision_IdempotentReturnsNoCredential(t *testing.T) { if res.InstanceID != "sb-existing" { t.Fatalf("InstanceID = %q, want sb-existing", res.InstanceID) } + 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; the claim tag must be adopted, not duplicated", f.createCnt) + } + if f.mintCnt != 0 { + t.Fatalf("minted %d credentials for an adopted sandbox; want 0", f.mintCnt) + } if res.ConnectURL != "" || res.ConnectToken != "" { t.Fatalf("an adopted sandbox must carry no credential, got url=%q token set=%t", res.ConnectURL, res.ConnectToken != "") @@ -1551,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 ErrImagePull, 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.ErrImagePull) { - t.Fatalf("err = %v, want ErrImagePull", 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) @@ -1566,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.ErrImagePull) { - t.Fatalf("err = %v, want ErrImagePull 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) } } @@ -1610,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.ErrImagePull) { - t.Fatalf("err = %v, want ErrImagePull", 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 fdea026..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, ErrImagePull) + 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, ErrImagePull) + 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, ErrImagePull) + 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, ErrImagePull) + 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 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). +// 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, ErrImagePull) + 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 4ea19aa..693eefc 100644 --- a/pkg/provider/registryauth_test.go +++ b/pkg/provider/registryauth_test.go @@ -17,9 +17,10 @@ limitations under the License. package provider import ( - "errors" "strings" "testing" + + nebulav1alpha1 "github.com/InftyAI/Nebula/api/v1alpha1" ) func TestRegistryAuthValidate(t *testing.T) { @@ -70,13 +71,13 @@ 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 - // 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, 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) @@ -91,13 +92,11 @@ 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) - } - // 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/handler.go b/pkg/vnode/handler.go index 6700687..1dd57c4 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" @@ -296,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") @@ -363,37 +368,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 @@ -455,17 +438,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) } } 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) } 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.