From 5ab931b8b5099aa4bd9bc1332d99cd7d81a2de9c Mon Sep 17 00:00:00 2001 From: kerthcet Date: Tue, 11 Aug 2026 16:17:01 +0100 Subject: [PATCH 1/6] [modal] fix: make instance not ready until readiness prob passed Signed-off-by: kerthcet --- README.md | 3 + api/v1alpha1/groupversion_info.go | 45 +++ api/v1alpha1/nodeclaim_types.go | 57 ++-- api/v1alpha1/nodepool_types.go | 3 +- .../bases/nebula.inftyai.com_nodepools.yaml | 5 +- config/manager/kustomization.yaml | 4 +- config/samples/deployment.yaml | 25 +- config/samples/nodepool.yaml | 20 +- docs/architecture.md | 66 ++--- docs/status.md | 259 +++++++++++++++++ internal/controller/nodeclaim_controller.go | 54 ++-- .../controller/nodeclaim_controller_test.go | 41 ++- internal/controller/nodepool_controller.go | 10 +- pkg/provider/modal/client.go | 263 ++++++++++++++++-- pkg/provider/modal/modal.go | 88 +++++- pkg/provider/modal/modal_test.go | 114 +++++++- pkg/vnode/handler.go | 42 ++- pkg/vnode/handler_test.go | 108 ++++++- pkg/vnode/status.go | 54 ++-- 19 files changed, 1049 insertions(+), 212 deletions(-) create mode 100644 docs/status.md diff --git a/README.md b/README.md index bccf94d..e93f0f6 100644 --- a/README.md +++ b/README.md @@ -7,6 +7,7 @@ [![Go Reference](https://pkg.go.dev/badge/github.com/InftyAI/Nebula.svg)](https://pkg.go.dev/github.com/InftyAI/Nebula) [![License](https://img.shields.io/badge/License-Apache_2.0-blue.svg)](LICENSE) ![Go Version](https://img.shields.io/badge/go-1.24-00ADD8?logo=go&logoColor=white) +[![Discord](https://img.shields.io/badge/Discord-Join%20us-5865F2?logo=discord&logoColor=white)](https://discord.gg/7WTUuFqyS6) Run GPU workloads on any NeoCloud or hyperscaler through one Kubernetes API. @@ -96,6 +97,8 @@ placement controller owns those. - See [config/samples](config/samples) for example NodePools and a runnable workload. - See [docs/add-a-provider.md](docs/add-a-provider.md) to add a provider backend. - See [docs/architecture.md](docs/architecture.md) for design details. +- See [docs/status.md](docs/status.md) for how instance lifecycle becomes Pod and + NodeClaim status, per provider. ## License diff --git a/api/v1alpha1/groupversion_info.go b/api/v1alpha1/groupversion_info.go index 7978f96..7f9ded8 100644 --- a/api/v1alpha1/groupversion_info.go +++ b/api/v1alpha1/groupversion_info.go @@ -150,3 +150,48 @@ const ( // VK liveness (see docs/architecture.md §3). TerminateInstanceFinalizer = "nebula.inftyai.com/terminate-instance" ) + +// Pod status reasons the virtual kubelet stamps on the Pods it reports, projecting +// the external instance's lifecycle onto standard Pod status (pkg/vnode/status.go +// is the only writer). +// +// They live here, not privately in pkg/vnode, for two reasons. They are a CONTRACT +// between packages: the Pod phase is lossy — Provisioning and "booting" both +// surface as PodPending — so the reason is the only thing separating "no instance +// exists yet" from "an instance exists and is coming up", and the NodeClaim +// controller keys its teardown guard off exactly that distinction (see +// desiredPhase). A rename on the writing side that the reading side did not follow +// would still compile, still pass tests, and silently leak paid instances: every +// booting instance would read as Provisioning, so a Pod that vanished mid-boot +// would be left running behind the cache-lag grace window. And they are user-facing +// — operators match on status.reason in jsonpath and alerts — so every value is +// public API whether or not Nebula's own code currently reads it. That is why the +// whole set is here rather than the subset with in-tree readers: these are the +// values status.reason can take, and a reader should find them in one place. +const ( + // PodReasonProvisioning: a provider Provision call has been issued but the + // instance does not yet exist — we are still allocating it (e.g. EC2 + // RunInstances in flight). Set on CreatePod, before the first poll observes + // the instance. + PodReasonProvisioning = "Provisioning" + // PodReasonInitializing: the instance EXISTS at the provider but is not yet + // reachable — it is booting (EC2 "pending"), running-but-not-yet-passing its + // reachability checks (running, <2/2, EC2's own "Initializing" status), or a Modal + // sandbox whose readiness probe has not passed. It mirrors that EC2 status-check + // term. Provisioning is done; the instance is coming up. Distinct from + // Provisioning so a Pod stuck here points at a slow boot / failing status checks, + // not a stuck allocation — and so the NodeClaim controller can tell that an + // instance exists. The virtual kubelet stamps it only for an instance it observed + // in the provider's List, which is what makes it trustworthy as that evidence. + PodReasonInitializing = "Initializing" + // PodReasonRunning: the provider reports the instance running. + PodReasonRunning = "Running" + // PodReasonProvisionFailed: the provider rejected or failed the Provision call. + PodReasonProvisionFailed = "ProvisionFailed" + // PodReasonFailed: the provider reports the instance in a failed state. + PodReasonFailed = "Failed" + // PodReasonTerminated: the instance is gone from the provider (torn down, + // reclaimed, or exited). Disappearance alone does not say WHY, so this is the + // neutral term rather than "Preempted". + PodReasonTerminated = "Terminated" +) diff --git a/api/v1alpha1/nodeclaim_types.go b/api/v1alpha1/nodeclaim_types.go index 3e7a998..131fe8f 100644 --- a/api/v1alpha1/nodeclaim_types.go +++ b/api/v1alpha1/nodeclaim_types.go @@ -72,20 +72,22 @@ type PodReference struct { // NodeClaimPhase is the coarse, user-facing lifecycle state. // // The NodeClaim is a passive teardown ledger, not a status mirror: it does NOT -// track finer workload runtime status (CPU/logs/restarts) — the Pod is the -// source of truth for that (see pkg/vnode/status.go). It tracks only the coarse -// states that matter to its own job as a ledger, keyed off the served Pod's -// phase/reason: Provisioning (allocating — instance does not exist yet), -// Initializing (instance exists and is booting but not yet reachable), Bound -// (instance running — the guard the teardown backstop trusts), and Terminated -// (instance gone). Finer states (e.g. Preempted) are deliberately absent: -// preemption cannot be detected — the provider contract's InstanceState has no -// Preempted value, and an absent instance only tells us it is gone, not why. -// Reintroduce a phase only when something actually sets it. +// track finer workload runtime status (CPU/logs/restarts/readiness) — the Pod is +// the source of truth for that (see pkg/vnode/status.go). It tracks only the +// coarse states that matter to its own job as a ledger, keyed off the served +// Pod's phase/reason: Provisioning (instance does not exist yet), Bound (an +// instance EXISTS at the provider — the guard the teardown backstop trusts), and +// Terminated (instance gone). Finer states (e.g. Preempted) are deliberately +// absent: preemption cannot be detected — the provider contract's InstanceState +// has no Preempted value, and an absent instance only tells us it is gone, not +// why. Reintroduce a phase only when something actually sets it. // -// Only Bound is a teardown guard: neither Provisioning nor Initializing earns the -// "trust a later disappearance" trust, because until the instance is confirmed up -// an absent Pod may be cache lag rather than a real teardown. +// The ledger's question is EXISTENCE, not readiness: what the backstop must know +// is whether there is an instance out there to reclaim. A booting instance and a +// serving one are equally real — equally billable, equally in need of teardown — +// so both are Bound, and readiness is left entirely to the Pod. (This is why +// there is no Initializing phase: it would be a readiness distinction on an +// object that does not track readiness.) type NodeClaimPhase string const ( @@ -93,19 +95,24 @@ const ( // instance does not yet exist — provisioning is still allocating it. The claim // does NOT earn the Bound teardown guard here: a Pod that vanishes while still // provisioning is treated as possible cache lag (grace window), not a real - // teardown, because we never confirmed the instance was actually up. + // teardown, because we never confirmed an instance was actually created. NodeClaimProvisioning NodeClaimPhase = "Provisioning" - // NodeClaimInitializing: the external instance EXISTS at the provider but is not - // yet reachable (e.g. EC2 is "pending", or "running" but its 2/2 status checks - // have not passed). The served Pod is Pending with reason Initializing (see - // pkg/vnode/status.go). Like Provisioning it does NOT earn the Bound guard — the - // instance is not yet confirmed up — but it is surfaced as a distinct phase so - // "allocating" and "booting" are distinguishable on the ledger. - NodeClaimInitializing NodeClaimPhase = "Initializing" - // NodeClaimBound: the served Pod has been observed running (present and not in - // a terminal phase). This is the durable guard the backstop trusts — a Bound - // claim whose Pod later disappears is a real teardown, not cache lag. The claim - // does not track finer workload status; the Pod is the source of truth for that. + // NOTE: there is deliberately no "Initializing" phase. It used to mean "the + // instance exists but is not reachable yet" and did NOT earn the teardown guard, + // which stranded a real, billable instance behind the grace window whenever its + // Pod vanished mid-boot. Existence is what the ledger tracks, so that state is + // now Bound; readiness lives on the Pod alone. + // + // NodeClaimBound: an external instance EXISTS at the provider for this claim. + // This is the durable guard the backstop trusts — a Bound claim whose Pod later + // disappears is a real teardown, not cache lag, so it is reclaimed immediately + // rather than after the grace window. + // + // Existence, NOT readiness: an instance that is booting (EC2 "pending", or + // running with its 2/2 status checks still pending; a Modal sandbox whose + // readiness probe has not passed) is Bound, because it is just as real and just + // as billable as one that is serving. Whether the workload is actually usable is + // the Pod's Ready condition, not this phase. NodeClaimBound NodeClaimPhase = "Bound" // NodeClaimTerminating: the served Pod is being deleted (its DeletionTimestamp // is set) but the external instance may not be reclaimed yet — teardown is in diff --git a/api/v1alpha1/nodepool_types.go b/api/v1alpha1/nodepool_types.go index a954007..738064f 100644 --- a/api/v1alpha1/nodepool_types.go +++ b/api/v1alpha1/nodepool_types.go @@ -167,7 +167,8 @@ const ( // NodePoolStatus surfaces the current placement picture for observability. type NodePoolStatus struct { - // Placed counts running instances per provider, for at-a-glance balance. + // Placed counts existing instances per provider (booting included), for + // at-a-glance balance. // +optional Placed map[string]int32 `json:"placed,omitempty"` diff --git a/config/crd/bases/nebula.inftyai.com_nodepools.yaml b/config/crd/bases/nebula.inftyai.com_nodepools.yaml index ddfa71f..c7feaa0 100644 --- a/config/crd/bases/nebula.inftyai.com_nodepools.yaml +++ b/config/crd/bases/nebula.inftyai.com_nodepools.yaml @@ -246,8 +246,9 @@ spec: additionalProperties: format: int32 type: integer - description: Placed counts running instances per provider, for at-a-glance - balance. + description: |- + Placed counts existing instances per provider (booting included), for + at-a-glance balance. type: object type: object type: object diff --git a/config/manager/kustomization.yaml b/config/manager/kustomization.yaml index 086bc1e..d0cdc06 100644 --- a/config/manager/kustomization.yaml +++ b/config/manager/kustomization.yaml @@ -4,5 +4,5 @@ apiVersion: kustomize.config.k8s.io/v1beta1 kind: Kustomization images: - name: controller - newName: inftyai/nebula-controller - newTag: latest + newName: nebula + newTag: dev diff --git a/config/samples/deployment.yaml b/config/samples/deployment.yaml index f3e62cd..02b2966 100644 --- a/config/samples/deployment.yaml +++ b/config/samples/deployment.yaml @@ -28,7 +28,7 @@ metadata: labels: app.kubernetes.io/managed-by: nebula spec: - replicas: 2 + replicas: 8 selector: matchLabels: app: gpu-workload-sample @@ -38,7 +38,7 @@ spec: app: gpu-workload-sample nebula.inftyai.com/enabled: "true" nebula.inftyai.com/nodepool: sample - nebula.inftyai.com/accelerator-type: t4 + nebula.inftyai.com/accelerator-type: l4 spec: # Do NOT set nodeName or a provider nodeSelector yourself — the placement # controller fills the nodeSelector in when it ungates the Pod. Setting @@ -57,6 +57,25 @@ spec: # this output, read it on the PROVIDER side (the Modal dashboard or # `modal app logs`), not via kubectl. command: ["sh", "-c", "nvidia-smi --query-gpu=index,name,memory.total --format=csv || echo 'no nvidia-smi'; sleep 3600"] + # The readiness bar for the EXTERNAL instance. On Modal this is the only + # thing that lets Nebula tell "still coming up" (queued, pulling the image, + # attaching the GPU) from "up and serving": the cheap poll signal answers + # only "has the process exited?", so WITHOUT a probe the Pod — and its + # Deployment's ready count — goes Running the moment the sandbox is created. + # With one, it stays Pending/Initializing until the probe passes. + # + # An EXEC probe, because this sample serves nothing and declares no ports: + # tcpSocket/httpGet need a numeric port, and a named or missing one omits + # the probe entirely. `nvidia-smi -L` is a real bar here — it passes only + # once the GPU is actually attached and the driver answers. + # + # Only exec and tcpSocket reach Modal (httpGet degrades to tcpSocket on its + # port); periodSeconds maps to the probe interval, and the other timing + # fields are the kubelet's, which never runs here. + readinessProbe: + exec: + command: ["nvidia-smi", "-L"] + periodSeconds: 5 resources: requests: cpu: "1" @@ -65,4 +84,4 @@ spec: # GPU count. Standard extended resource, so the scheduler's fit check # and provisioning read the same number. 8 => 8x the accelerator-type # above. - nvidia.com/gpu: "1" + nvidia.com/gpu: "8" diff --git a/config/samples/nodepool.yaml b/config/samples/nodepool.yaml index 8503656..a741081 100644 --- a/config/samples/nodepool.yaml +++ b/config/samples/nodepool.yaml @@ -7,17 +7,17 @@ metadata: spec: # strategy (the inner, provider-ranking axis). providers: - - name: aws - regions: - - us-east-1 - - us-west-1 - - ap-south-1 - - ap-northeast-1 - - eu-central-1 - - eu-west-1 - - ca-central-1 - - sa-east-1 - name: modal + # - name: aws + # regions: + # - us-east-1 + # - us-west-1 + # - ap-south-1 + # - ap-northeast-1 + # - eu-central-1 + # - eu-west-1 + # - ca-central-1 + # - sa-east-1 # - name: runpod # Outer axis: try OnDemand on every provider first, fall back to Spot. capacityTypes: diff --git a/docs/architecture.md b/docs/architecture.md index fe43a26..8d9b1b8 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -11,7 +11,9 @@ seam is meant to support more backends, but this document describes the implementation that is in the repository today. Planned or partial work is called out in [Current implementation status](#current-implementation-status). For deployment -and credential setup, see [docs/deploy.md](deploy.md). +and credential setup, see [docs/deploy.md](deploy.md); for how an instance's +lifecycle becomes Pod and NodeClaim status (including each provider's own status +mapping), see [docs/status.md](status.md). - [Goals and Non-Goals](#goals-and-non-goals) - [System Overview](#system-overview) @@ -199,42 +201,24 @@ Follow one GPU Pod from creation to teardown: ### Status Flow -Pod status and NodeClaim status come from different sources. The Pod is the -runtime surface users watch. The NodeClaim is the coarse ledger that protects -teardown. - -``` -Pod.status, written by pkg/vnode - CreatePod success -> Pending / Provisioning - CreatePod error -> Failed / ProvisionFailed - List sees Pending -> Pending / Initializing - List sees Running -> Running / Ready=True / endpoint annotation - List sees Failed -> Failed / Failed - List misses instance -> Failed / Terminated - DeletePod success -> Succeeded / Terminated - -NodeClaim.status, written by NodeClaim controller - present Pod, no instance yet -> Provisioning - present Pod, initializing instance -> Initializing - present Running Pod -> Bound - present deleting Pod -> Terminating - present terminal Pod -> Terminated - absent after Bound/Terminating -> delete self -> terminate finalizer - absent before Bound -> wait placementGracePeriod, then delete -``` - -Important details: - -- `Bound` is the teardown guard. Once a claim has seen a Running Pod, a later Pod - disappearance is trusted as real teardown, not cache lag. -- `Provisioning` and `Initializing` do not earn the guard. If the Pod is absent - before `Bound`, the controller waits `placementGracePeriod` (15 seconds) before - deleting an orphaned claim. +Once a Pod is placed, its status is driven by the poll loop rather than the +placement path: the virtual kubelet projects the external instance's lifecycle +onto standard Pod status, and the NodeClaim controller derives its coarse ledger +phase from the served Pod. + +The full mapping — Pod phase/reason and the claim phase each produces, plus each +provider's own status vocabulary and the limits of what is observable — lives in +[docs/status.md](status.md). Two properties matter for the placement flow +described above: + +- `Bound` means an instance EXISTS at the provider, not that the workload is + ready. It is the teardown guard: once a claim is `Bound`, a later Pod + disappearance is trusted as real teardown rather than informer cache lag, and is + reclaimed with no grace period. Only `Provisioning` (nothing created yet) waits + out `placementGracePeriod`. - `NodeClaimStatus.InstanceID` is recorded on a best-effort basis. The finalizer - prefers it when present, but can still recover by matching provider instances - by claim name through `List()`. -- NodeClaim does not mirror logs, restarts, container state, or fine-grained - runtime health. Those belong on the Pod. + prefers it when present, but can still recover by matching provider instances by + claim name through `List()`. --- @@ -330,8 +314,8 @@ Reconcile behavior: - add the terminate finalizer before doing anything else; - fetch the served Pod by namespace/name and UID; -- set coarse phase from the served Pod: `Provisioning`, `Initializing`, `Bound`, - `Terminating`, or `Terminated`; +- set coarse phase from the served Pod: `Provisioning`, `Bound`, `Terminating`, or + `Terminated`; - best-effort record `status.instanceID` by matching the provider instance by claim name; - when a previously observed Pod disappears, delete the claim so the finalizer @@ -492,9 +476,9 @@ status: instanceID: i-0123456789abcdef0 ``` -Valid phases are `Provisioning`, `Initializing`, `Bound`, `Terminating`, and -`Terminated`. The claim deliberately does not duplicate PodSpec and does not -mirror fine-grained runtime status. +Valid phases are `Provisioning`, `Bound`, `Terminating`, and `Terminated`. The +claim deliberately does not duplicate PodSpec and does not mirror fine-grained +runtime status — `Bound` answers existence, not readiness. ### Sandbox and SandboxSet diff --git a/docs/status.md b/docs/status.md new file mode 100644 index 0000000..983762b --- /dev/null +++ b/docs/status.md @@ -0,0 +1,259 @@ +# Status + +How an external instance's lifecycle becomes Kubernetes status. There are three +layers, and each one deliberately knows less than the one below it: + +``` +provider API (EC2 states, Modal exit codes, ...) + | each adapter's toState + v +provider.InstanceState Pending | Running | Terminated | Failed + | pkg/vnode/status.go applyState + v +Pod.status phase + reason + Ready condition + | internal/controller desiredPhase + v +NodeClaim.status.phase Provisioning | Bound | Terminating | Terminated +``` + +The narrowing is the point. `InstanceState` is a closed four-value set so the +control plane never branches on a provider name, and the NodeClaim reads only the +Pod — never the provider — so there is exactly one place where provider vocabulary +enters the system. + +- [Pod and NodeClaim status](#pod-and-nodeclaim-status) +- [Provider mappings](#provider-mappings) + - [AWS](#aws) + - [Modal](#modal) + - [fake](#fake) +- [What is not observable](#what-is-not-observable) + +--- + +## Pod and NodeClaim status + +Pod status and NodeClaim status come from different sources. The Pod is the +runtime surface users watch. The NodeClaim is the coarse ledger that protects +teardown. + +| Pod phase | reason | writer | instance exists? | claim phase | +|---|---|---|---|---| +| `Pending` | `Provisioning` | `CreatePod` | no | `Provisioning` (grace applies) | +| `Pending` | `Initializing` | `applyState` ← `InstancePending` | yes | `Bound` | +| `Running` | `Running` | `applyState` ← `InstanceRunning` | yes | `Bound` | +| `Failed` | `ProvisionFailed` | `CreatePod` | no | `Terminated` (via `isTerminal`) | +| `Failed` | `Failed` | `applyState` ← `InstanceFailed` | yes | `Terminated` | +| `Failed` | `Terminated` | `applyState` ← `InstanceTerminated` | gone | `Terminated` | +| `Succeeded` | `Terminated` | `DeletePod` | gone | `Terminated` | + +`Running` also sets `Ready=True` and the endpoint annotation. A Pod carrying a +`DeletionTimestamp` maps to `Terminating` from any non-terminal phase. When the +served Pod is ABSENT: after `Bound`/`Terminating`, the claim deletes itself and +the terminate finalizer runs; before `Bound`, it waits `placementGracePeriod` +first. + +Note the two writers. `CreatePod` writes the rows where no instance exists, plus +the first `Initializing` — it publishes `Provisioning` *before* calling the +provider and `Initializing` as soon as the call returns an id, so each reason +covers exactly the window in which it is true: `Provisioning` is the blocking +allocation call itself, and `Initializing` starts the moment an instance exists. +Every other non-terminal row comes from `applyState`, driven by the poll loop, +and is therefore only reachable for an instance the provider actually returned +from `List()`. + +The ordering matters in both directions. Writing `Provisioning` after the call +described a state already over, so it was effectively unobservable, while leaving +`Initializing` to the first tick would hold the claim at `Provisioning` — and so +behind the `placementGracePeriod` — for up to 15 seconds after a billable instance +existed. + +The pre-call write is emitted but NOT tracked, and that distinction is load-bearing: +the poll loop maps a tracked Pod absent from `List()` to `Terminated`, and during +the provider call the instance is legitimately absent, so tracking it there would +let a concurrent tick write `Failed`/`Terminated` over a provision that goes on to +succeed — unrecoverably, since Pod phases are terminal-sticky and the claim +reclaims on that phase. + +`Initializing` asserts EXISTENCE, not boot progress. On AWS the instance is also +genuinely booting, because an instant fleet allocates capacity synchronously. A +Modal sandbox may still be queued — but reporting `Provisioning` for it would be +worse: the id, and with it the reclaim obligation, already exists, and a queued +sandbox bills. + +Important details: + +- `Bound` means an instance EXISTS at the provider, not that the workload is + ready. It is the teardown guard: a later Pod disappearance is trusted as real + teardown, not cache lag, and is reclaimed with no grace period. +- A booting instance is `Bound` too — just as real and just as billable as a + serving one. There is no `Initializing` claim phase; readiness lives on the Pod + alone. Note `Pending` carries two reasons: the phase alone cannot separate + "nothing exists yet" from "exists and booting", so the claim keys off + `status.reason`, which is why the reasons are declared once as `PodReason*` in + `api/v1alpha1`. +- Only `Provisioning` fails to earn the guard, because nothing exists yet. If the + Pod is absent then, the controller waits `placementGracePeriod` (15 seconds) + before deleting an orphaned claim. +- `NodeClaimStatus.InstanceID` is recorded on a best-effort basis. The finalizer + prefers it when present, but can still recover by matching provider instances + by claim name through `List()`. +- NodeClaim does not mirror logs, restarts, container state, or fine-grained + runtime health. Those belong on the Pod. + +--- + +## Provider mappings + +Every adapter narrows its own vocabulary to `InstanceState` in a `toState` +function. Two rules hold across all of them: + +- **`Running` means reachable, not merely started.** A provider reports + `InstanceRunning` only once the instance has passed whatever readiness bar it + can observe, because `InstanceRunning` becomes Pod `Running` + `Ready=True`, + which is what Kubernetes counts toward a Deployment's ready replicas. Advancing + early would report a Deployment as serving while none of its boxes can be + reached. +- **Unknown states map to `Pending`, never to a terminal state.** A status string + the adapter does not recognize means the poll loop keeps watching. Guessing + `Terminated` would strand a live, billing instance. + +### AWS + +Two independent axes: the EC2 instance-state name from `DescribeInstances`, and +the 2/2 reachability checks, which need a separate `DescribeInstanceStatus` call +folded in by `List` (`StatusChecksPassed`). + +| `ec2State` | checks | `InstanceState` | Pod | +|---|---|---|---| +| `pending` | — | `Pending` | `Pending` / `Initializing` | +| `running` | not ok | `Pending` | `Pending` / `Initializing` | +| `running` | 2/2 ok | `Running` | `Running` / `Ready=True` | +| `shutting-down`, `terminated` | — | `Terminated` | `Failed` / `Terminated` | +| `stopping`, `stopped` | — | `Terminated` | `Failed` / `Terminated` | +| anything else | — | `Pending` | `Pending` / `Initializing` | + +- EC2 flips an instance to `running` a minute or two before its checks clear, so + `running` alone is not reachable — hence the second call. If + `DescribeInstanceStatus` fails (commonly a missing `ec2:DescribeInstanceStatus` + IAM grant, which is a *separate* permission from `DescribeInstances`), `List` + still returns the instances with `StatusChecksPassed` false, so they hold at + `Pending`. That failure is logged precisely because it is otherwise invisible: + every healthy instance stays stuck at `Pending` forever. +- `stopped` is treated as gone, not as a distinct state: a stopped instance is not + serving the workload, and the ledger's recovery model is delete-and-recreate. +- **There is no queueing.** Provisioning uses a `CreateFleet` *instant* request, + which is synchronous — the response carries either an instance id or the + reason it could not launch. A capacity shortfall is an error + (`ErrNoCapacity`/`ErrSpotCapacity`) that drives AZ/region/tier failover, not a + pending instance. So an AWS instance that exists is always allocated; `pending` + vs `running`-without-checks are both "booting", and both are `Bound`. +- **No `Failed` case.** Impaired status checks and `StateReason` are not consumed, + so an instance that failed to boot currently reads as `Terminated` (looks like a + clean teardown) or holds at `Pending`. See + [What is not observable](#what-is-not-observable). + +### Modal + +One sandbox per NodeClaim. Modal exposes far less than EC2, so the adapter has +only two signals and has to record a third fact itself. + +| signal | `InstanceState` | Pod | +|---|---|---| +| `Poll` → nil (live), readiness confirmed | `Running` | `Running` / `Ready=True` | +| `Poll` → nil (live), readiness not confirmed | `Pending` | `Pending` / `Initializing` | +| `Poll` → exit `0`, `137`, `124` | `Terminated` | `Failed` / `Terminated` | +| `Poll` → any other exit code | `Failed` | `Failed` / `Failed` | +| absent from `List` | `Terminated` | `Failed` / `Terminated` | + +- `Poll` answers exactly one question: **has the process exited?** It returns + `nil` for a sandbox that is queued, pulling its image, attaching GPUs, booting, + or up-but-not-ready — all of it. Readiness cannot come from `Poll`. +- Readiness comes from `WaitUntilReady`, which **blocks** and returns early only + to say "ready" — never to say "not ready". Its timeout is a budget, not a hint, + and the cost is dominated by per-call setup (task-id lookup plus a fresh TLS + gRPC dial to the task's own router), measured at ~16s cold versus ~100ms warm. + It therefore cannot be called on the read path: `observe` runs once per sandbox + per poll tick, so a truthful budget would serialize ~16s per sandbox, and a + short budget returns a deadline regardless of the truth. Instead one background + waiter per sandbox does the blocking call and latches the result; `observe` + reads the latch without blocking. The latch is set only on a CONFIRMED answer + (`err == nil`, or `FailedPrecondition` meaning "no readiness probe configured"), + so an ambiguous error can never promote a sandbox that is still coming up. +- The latch does **not demote**: a probe that passes and later starts failing + leaves the sandbox `Running`. `Poll` still catches process exit, so death is + observed; sickness is not. +- Readiness only exists if the sandbox was created with a probe, and Modal's + control plane does not expose that fact back through the Go SDK — so the adapter + records it at create time in the `ProbeTagKey` tag. The tag tracks whether Modal + actually RECEIVED a probe, not whether the Pod declared one: a Pod probe with a + named port or an unsupported handler cannot be expressed as a Modal probe and is + dropped, and tagging those would claim a readiness signal that does not exist. A + sandbox with no probe is reported ready as soon as it is live, which is the + honest answer — Modal has no readiness concept without one. +- Pod probes map onto Modal's exec and TCP probes only. `httpGet` degrades to a + TCP probe on its port; `periodSeconds` becomes the probe interval. +- **Exit codes are lossy.** The control plane's result carries eight statuses + (`SUCCESS`, `FAILURE`, `INIT_FAILURE`, `INTERNAL_FAILURE`, `TERMINATED`, + `TIMEOUT`, `IDLE_TIMEOUT`, unspecified) and the SDK collapses all of them into + one int before the adapter sees it. The split above is therefore inference, and + deliberately conservative: only a clean exit plus the two codes Modal + substitutes for a non-exit outcome (`137` terminated, `124` timeout) count as + gone; any other nonzero exit is a failure. A workload that genuinely exits `137` + is indistinguishable from a Modal termination, so this can understate a failure + but never invent one — and it cannot affect teardown, which reclaims by asking + the provider what exists. +- **Modal DOES queue**, unlike AWS: `Sandboxes.Create` returns an id immediately + and the sandbox then waits for capacity, potentially for minutes on a large GPU + shape. It is `Bound` and billing throughout. See below for why that is not + reported distinctly. + +### fake + +The in-memory e2e provider reports `InstanceRunning` as soon as an instance is +created. It exists to exercise the placement and teardown paths without a real +backend, so it has no boot or readiness phase to model. + +--- + +## What is not observable + +Documented so the coarseness is not mistaken for a bug. + +**Modal: queued vs. booting.** A sandbox waiting for capacity and one actively +starting up are both reported `Pending` / `Initializing`, and the Pod's reason +flips from `Provisioning` to `Initializing` at the first poll tick (≤15s) whether +or not anything changed in the sandbox. Every public signal was measured and none +carries the boundary: + +| signal | distinguishes queued from booting? | +|---|---| +| `Poll` | No — both are the unspecified status → `nil` | +| `WaitUntilReady` | No — one bit, and only ever the positive one | +| `SandboxGetTaskId` | No — a task id is assigned ~400ms after create, long before boot | + +Modal's control plane does know (`SandboxInfo` carries the task state and a +`ReadyAt` stamp) but the Go SDK builds its sandbox objects from ids alone and +keeps the control-plane client private. If a future SDK exposes `SandboxInfo`, +both this gap and `ProbeTagKey` and the readiness latch all become unnecessary. + +The mislabel is cosmetic today: the only consumer of the reason is the claim's +teardown guard, and both `Provisioning` and `Initializing` reclaim correctly — +`Provisioning` after the grace window, `Initializing` immediately — because +teardown resolves the instance from `List()`, not from the reason. + +**AWS: failed vs. gone.** `toState` has no `InstanceFailed` case, so an instance +that failed to boot is reported as `Terminated` or holds at `Pending`. Unlike the +Modal exit-code inference, the signal here is available and authoritative: +`DescribeInstanceStatus` reports `impaired` for both status checks, and the +instance carries a `StateReason` that separates "we stopped it" from "it died". + +**Neither provider reports preemption.** `InstanceState` has no `Preempted` +value, and an instance's disappearance says only that it is gone, not why — so +`Terminated` is the neutral, accurate answer rather than a claim about a provider +reclaim. This is also why `NodeClaimPhase` has no `Preempted`. + +**Readiness has no intermediate rung.** `applyState` welds phase, the `Ready` +condition, and container readiness into one atomic write, so in Nebula +`PodRunning` implies `Ready=True` implies all containers ready. The readiness bar +lives entirely in each adapter's `toState`; a Pod is never `Running` but +not-ready. diff --git a/internal/controller/nodeclaim_controller.go b/internal/controller/nodeclaim_controller.go index ddf0450..2e12cad 100644 --- a/internal/controller/nodeclaim_controller.go +++ b/internal/controller/nodeclaim_controller.go @@ -48,18 +48,18 @@ import ( // exists at the API server by the time this claim reconciles — only cache // propagation, not Pod creation, is being waited on. A Pod watch re-enqueues the // claim the moment the cache catches up, so this is just the backstop for a -// missed event; seconds are plenty. Once the Pod has been observed running +// missed event; seconds are plenty. Once an instance has been confirmed to exist // (Phase set to Bound) a later disappearance is trusted immediately — no grace — // because we KNOW the Pod existed and is now gone. const placementGracePeriod = 15 * time.Second // podReasonInitializing is the Pod status.Reason the virtual node stamps while the -// external instance exists but is not yet reachable (see pkg/vnode/status.go -// reasonInitializing). The claim keys off it to distinguish Initializing from -// Provisioning, since both share the Pending phase. Kept in sync with vnode's -// value by hand — it is a stable, user-facing reason string, not worth an exported -// package coupling. -const podReasonInitializing = "Initializing" +// external instance EXISTS but is not yet reachable. It is the claim's evidence of +// existence for a Pod that is still Pending: Provisioning and "booting" share the +// Pending phase, and only the reason separates them — so this must be the SAME +// string the virtual node writes, which is why it aliases the shared API constant +// rather than repeating the literal. +const podReasonInitializing = nebulav1alpha1.PodReasonInitializing // NodeClaimReconciler reconciles a NodeClaim object. // @@ -134,10 +134,10 @@ func (r *NodeClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( if pod != nil { // The workload is present. We do not mirror the Pod's fine-grained runtime // status, but we DO reflect the coarse phase that matters to the ledger. - // desiredPhase maps the served Pod onto the claim phase (Terminated / Bound / - // Initializing / Provisioning); markPhase persists it (and the instance id) - // idempotently. An empty desiredPhase means "hold the current phase, just keep - // the id fresh" — the Bound-flap guard (see desiredPhase). + // desiredPhase maps the served Pod onto the claim phase (Terminated / + // Terminating / Bound / Provisioning); markPhase persists it (and the instance + // id) idempotently. An empty desiredPhase means "hold the current phase, just + // keep the id fresh" — the Bound hold (see desiredPhase). return ctrl.Result{}, r.markPhase(ctx, &nc, r.desiredPhase(&nc, pod)) } @@ -286,15 +286,19 @@ func (r *NodeClaimReconciler) provider(name string) (provider.Provider, bool) { // Bound (or Provisioning) Pod that starts deleting is not stranded on its prior // phase. Terminal wins over it: an already-gone instance is Terminated, not // merely terminating. -// - Bound already => "" (hold). Bound is the durable teardown guard, and a -// transient Running->Pending flap (e.g. a status-check blip) must NOT strip it -// — losing it would make a later disappearance read as cache lag and skip -// teardown. Returning "" holds the phase while markPhase still refreshes the id. -// - Running => Bound. The instance is confirmed up; earn the guard. -// - Pending with reason Initializing => Initializing (instance exists, booting). -// - Otherwise => Provisioning (still allocating; instance does not exist yet). -// -// Neither Initializing nor Provisioning earns the Bound guard. +// - Bound already => "" (hold). Bound is the durable teardown guard, and it must +// never be stripped: losing it would make a later disappearance read as cache +// lag and skip teardown of a live instance. Returning "" holds the phase while +// markPhase still refreshes the id. +// - An instance EXISTS => Bound. Two Pod shapes prove existence, and the claim +// treats them identically because its question is existence, not readiness: +// Running (up and past its readiness bar) and Pending/Initializing (created and +// booting — vnode stamps that reason only for an instance it observed in the +// provider's List). A booting GPU box is just as real, and just as billable, as +// a serving one; if its Pod vanishes it must be reclaimed with no grace. +// - Otherwise => Provisioning. The instance does not exist yet (the Provision +// call may still be in flight), so a vanished Pod may be cache lag and the +// grace window applies. func (r *NodeClaimReconciler) desiredPhase(nc *nebulav1alpha1.NodeClaim, pod *corev1.Pod) nebulav1alpha1.NodeClaimPhase { switch { case isTerminal(pod.Status.Phase): @@ -303,10 +307,9 @@ func (r *NodeClaimReconciler) desiredPhase(nc *nebulav1alpha1.NodeClaim, pod *co return nebulav1alpha1.NodeClaimTerminating case r.wasBound(nc): return "" // hold Bound (or Terminated); never downgrade - case pod.Status.Phase == corev1.PodRunning: + case pod.Status.Phase == corev1.PodRunning, + pod.Status.Reason == podReasonInitializing: return nebulav1alpha1.NodeClaimBound - case pod.Status.Reason == podReasonInitializing: - return nebulav1alpha1.NodeClaimInitializing default: return nebulav1alpha1.NodeClaimProvisioning } @@ -355,8 +358,9 @@ func (r *NodeClaimReconciler) recordInstanceID(ctx context.Context, nc *nebulav1 return true } -// wasBound reports whether the served Pod has ever been observed running for this -// claim. A Terminated claim was necessarily Bound first, so it also counts. +// wasBound reports whether an external instance has ever been confirmed to exist +// for this claim — the fact the teardown backstop keys off. A Terminated claim was +// necessarily Bound first, so it also counts. func (r *NodeClaimReconciler) wasBound(nc *nebulav1alpha1.NodeClaim) bool { return nc.Status.Phase == nebulav1alpha1.NodeClaimBound || nc.Status.Phase == nebulav1alpha1.NodeClaimTerminated diff --git a/internal/controller/nodeclaim_controller_test.go b/internal/controller/nodeclaim_controller_test.go index e3b2b9c..4326612 100644 --- a/internal/controller/nodeclaim_controller_test.go +++ b/internal/controller/nodeclaim_controller_test.go @@ -233,11 +233,14 @@ func TestReconcile_PendingPodDoesNotMarkBound(t *testing.T) { } } -func TestReconcile_InitializingPodMarksInitializing(t *testing.T) { - // A served Pod that is Pending with reason Initializing (instance exists and is - // booting, e.g. EC2 running but <2/2 checks) must move the claim to the distinct - // Initializing phase — NOT Provisioning (which means still allocating) and NOT - // Bound (the guard is earned only when running). +func TestReconcile_BootingPodEarnsBound(t *testing.T) { + // A served Pod that is Pending with reason Initializing means the instance EXISTS + // and is booting (e.g. EC2 running but <2/2 checks, or a Modal sandbox whose + // readiness probe has not passed). vnode stamps that reason only for an instance + // it observed in the provider's List, so it is positive evidence of existence — + // and existence, not readiness, is what the claim tracks. It must earn Bound: the + // box is real and billable, so if its Pod later vanishes it has to be reclaimed + // immediately rather than waiting out the grace window. pod := newPod("p1", "default", "uid-1", corev1.PodPending) pod.Status.Reason = podReasonInitializing claim := newClaim("c1", "p1", "default", "uid-1", "fake") @@ -250,14 +253,38 @@ func TestReconcile_InitializingPodMarksInitializing(t *testing.T) { reconcileClaim(t, r, "c1") got := getClaim(t, c, "c1") - if got.Status.Phase != nebulav1alpha1.NodeClaimInitializing { - t.Fatalf("expected phase Initializing, got %q", got.Status.Phase) + if got.Status.Phase != nebulav1alpha1.NodeClaimBound { + t.Fatalf("expected phase Bound for a booting instance, got %q", got.Status.Phase) } if got.Status.InstanceID != "inst-1" { t.Fatalf("expected instance id captured, got %q", got.Status.InstanceID) } } +func TestReconcile_BootingPodGoneIsTornDownWithoutGrace(t *testing.T) { + // The point of the change: a claim whose instance was confirmed to exist while + // still BOOTING, and whose Pod then disappears, must self-delete immediately so + // the finalizer reclaims the instance. Previously such a claim sat at + // Initializing, which did not earn the guard, so a real billable GPU box was left + // running behind the cache-lag grace window. + claim := newClaim("c1", "p1", "default", "uid-1", "fake") + claim.Status.Phase = nebulav1alpha1.NodeClaimBound // earned while booting + prov := &fakeProvider{ + name: "fake", + list: []provider.Instance{{ID: "inst-1", ClaimName: "default-p1"}}, + } + // No Pod object: the served Pod is gone. + r, c := newClaimReconciler(t, []client.Object{claim}, prov) + + reconcileClaim(t, r, "c1") + + got := &nebulav1alpha1.NodeClaim{} + err := c.Get(context.Background(), types.NamespacedName{Name: "c1"}, got) + if err == nil && got.DeletionTimestamp.IsZero() { + t.Fatal("expected the claim to be deleted (no grace) once its instance was known to exist") + } +} + func TestReconcile_BoundClaimDoesNotDowngradeOnStatusFlap(t *testing.T) { // A claim already Bound whose Pod briefly drops back to Pending (a status-check // flap surfacing as reason Initializing) must NOT downgrade: Bound is the durable diff --git a/internal/controller/nodepool_controller.go b/internal/controller/nodepool_controller.go index 4cf788d..dc48782 100644 --- a/internal/controller/nodepool_controller.go +++ b/internal/controller/nodepool_controller.go @@ -124,10 +124,12 @@ func (r *NodePoolReconciler) validate(pool *nebulav1alpha1.NodePool) (reason, ms // live placement picture — so it is fully recomputed each reconcile rather than // incremented, which keeps it correct after missed events. // -// "Placed" is a claim whose served Pod has been observed (phase Bound). The -// claim no longer mirrors the Pod's finer runtime status (the Pod is the source -// of truth for that), so Bound is the claim-level signal that an instance is -// live for the workload. +// "Placed" is a claim with an instance at the provider (phase Bound). That counts +// a BOOTING instance as placed, which is the intent: this is a capacity picture, +// and an instance still coming up already occupies quota and already bills. The +// claim does not mirror the Pod's finer runtime status (the Pod is the source of +// truth for readiness), so Bound is the claim-level signal that an instance exists +// for the workload. func (r *NodePoolReconciler) refreshPlaced(ctx context.Context, pool *nebulav1alpha1.NodePool) error { var claims nebulav1alpha1.NodeClaimList if err := r.List(ctx, &claims); err != nil { diff --git a/pkg/provider/modal/client.go b/pkg/provider/modal/client.go index f5701de..94e54fd 100644 --- a/pkg/provider/modal/client.go +++ b/pkg/provider/modal/client.go @@ -21,9 +21,12 @@ import ( "fmt" "strconv" "strings" + "sync" "time" modal "github.com/modal-labs/modal-client/go" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/intstr" @@ -45,6 +48,35 @@ type sdkClient struct { // endpointTimeout bounds the best-effort tunnel lookup used to report an // instance's reachable address; tunnels only exist once the sandbox is up. endpointTimeout time.Duration + + // readyTimeout is the budget one background readiness waiter gets. It is a real + // budget, not a hint: WaitUntilReady BLOCKS and returns early only to say + // "ready" — never to say "not ready" — so a budget below the call's own setup + // cost cannot produce an answer, it can only produce a deadline. Setup dominates + // (getCommandRouter polls for the task id, then dials a fresh TLS gRPC + // connection to the task's own router), measured at ~16s cold, so this must be + // comfortably above that. It is affordable precisely because the wait no longer + // runs on the read path. + readyTimeout time.Duration + + // Readiness latch. WaitUntilReady is a one-shot blocking WAIT, but observe is a + // level-triggered READ that must answer from state on every poll tick; wrapping + // the wait in a short timeout to fake a read is what made a timeout masquerade + // as "not ready". So the wait happens once, in the background, and its result is + // latched here for observe to read. + // + // ready is latched on CONFIRMED readiness only — the default is not-ready, so an + // ambiguous error can no longer promote a sandbox that is still coming up. + // waiting dedupes waiters so repeated ticks don't pile up goroutines on one + // sandbox. Both are keyed by sandbox id and dropped by forgetReady when the + // sandbox goes away, so neither grows without bound. + // + // The latch does not DEMOTE: a probe that passes and later starts failing leaves + // the sandbox Running. Poll still observes process exit, so death is caught; + // sickness is not. Demotion would need a re-armed waiter and a staleness stamp. + readyMu sync.Mutex + ready map[string]bool + waiting map[string]struct{} } // compile-time assertion that sdkClient satisfies the adapter's Client seam. @@ -76,6 +108,9 @@ func NewSDKClient(ctx context.Context, appName string) (*Provider, error) { mc: mc, appName: appName, endpointTimeout: 5 * time.Second, + readyTimeout: 30 * time.Second, + ready: make(map[string]bool), + waiting: make(map[string]struct{}), }, cat), nil } @@ -130,9 +165,38 @@ func (c *sdkClient) CreateSandbox(ctx context.Context, spec SandboxSpec) (string // create an invalid Modal TCP probe on port 0. So a named port omits the probe // rather than emitting a bogus one. func modalProbe(p *corev1.Probe) (*modal.Probe, error) { - if p == nil { + plan, ok := planProbe(p) + if !ok { return nil, nil } + if plan.exec != nil { + return modal.NewExecProbe(plan.exec, &modal.ExecProbeParams{Interval: plan.interval}) + } + return modal.NewTCPProbe(plan.port, &modal.TCPProbeParams{Interval: plan.interval}) +} + +// probePlan is the SDK-free decision behind modalProbe: WHETHER a Pod probe maps +// to a Modal probe, and if so with what. Exactly one of exec/port is set. +type probePlan struct { + exec []string + port int + interval time.Duration +} + +// planProbe decides whether a Pod readinessProbe maps onto a Modal probe, +// reporting ok=false when it does not (nil probe, no handler, or a named port — +// see modalProbe for why a named port cannot be resolved here). +// +// This is split out from modalProbe so the CREATE path and the ProbeTagKey gate +// cannot disagree: the tag asserts "Modal received a probe for this sandbox", and +// deriving it from `pod.Spec.Containers[0].ReadinessProbe != nil` made that a lie +// for every probe shape modalProbe drops — those sandboxes advertised a probe +// Modal never got, so the readiness wait was asked about a probe that did not +// exist. One predicate, two callers, no drift. +func planProbe(p *corev1.Probe) (probePlan, bool) { + if p == nil { + return probePlan{}, false + } // PeriodSeconds maps to the probe interval; zero leaves the SDK default (the // SDK constructors reject a zero interval). var interval time.Duration @@ -141,19 +205,15 @@ func modalProbe(p *corev1.Probe) (*modal.Probe, error) { } switch { case p.Exec != nil && len(p.Exec.Command) > 0: - return modal.NewExecProbe(p.Exec.Command, &modal.ExecProbeParams{Interval: interval}) + return probePlan{exec: p.Exec.Command, interval: interval}, true case p.TCPSocket != nil: - if port, ok := numericPort(p.TCPSocket.Port); ok { - return modal.NewTCPProbe(port, &modal.TCPProbeParams{Interval: interval}) - } - return nil, nil // named port: unsupported here (see doc) + port, ok := numericPort(p.TCPSocket.Port) + return probePlan{port: port, interval: interval}, ok case p.HTTPGet != nil: - if port, ok := numericPort(p.HTTPGet.Port); ok { - return modal.NewTCPProbe(port, &modal.TCPProbeParams{Interval: interval}) - } - return nil, nil // named port: unsupported here (see doc) + port, ok := numericPort(p.HTTPGet.Port) + return probePlan{port: port, interval: interval}, ok default: - return nil, nil + return probePlan{}, false } } @@ -174,6 +234,11 @@ func numericPort(p intstr.IntOrString) (int, bool) { // TerminateSandbox implements Client. Idempotent: a sandbox that no longer // exists resolves to a not-found from FromID, which we treat as already gone. func (c *sdkClient) TerminateSandbox(ctx context.Context, id string) error { + // Drop any latched readiness up front, so it is released even on the error + // paths below: this sandbox is on its way out either way, and a live waiter on + // it is now pointless work. + c.forgetReady(id) + sb, err := c.mc.Sandboxes.FromID(ctx, id, &modal.SandboxFromIDParams{}) if err != nil { if isNotFound(err) { @@ -235,36 +300,40 @@ func (c *sdkClient) ListSandboxes(ctx context.Context) ([]Sandbox, error) { // Tunnels). Tag/tunnel/poll errors are tolerated so a single flaky sandbox // doesn't fail the whole List — the poll loop will re-observe next tick. // -// observe is a POINT-IN-TIME read and never blocks: it reports the status as -// currently known and returns immediately. It deliberately does not call -// WaitUntilReady (the only Modal readiness signal), because that blocks until the -// probe first passes and would stall the List for as long as a sandbox takes to -// come up. +// observe is a BOUNDED read: every call it makes carries a short deadline, so it +// returns promptly even for a sandbox that is still coming up. It must be, since +// it runs once per sandbox inside the List iteration. func (c *sdkClient) observe(ctx context.Context, sb *modal.Sandbox) Sandbox { out := Sandbox{ID: sb.SandboxID} - // Tags carry Nebula identity (ClaimTagKey), recovered by toInstance. + // Tags carry Nebula identity (ClaimTagKey), recovered by toInstance, and + // probe-ness (ProbeTagKey), read by isReady below — so this must precede the + // status block. if tags, err := sb.GetTags(ctx, &modal.SandboxGetTagsParams{}); err == nil { out.Tags = tags } - // Status. Poll (== sandboxWait(0)) is the only cheap point-in-time signal: it - // reports whether the sandbox PROCESS HAS EXITED — a non-nil exit code means it - // is gone (terminated), nil means it is still live. Poll cannot tell "still - // scheduling" (queued, image pull, GPU attach, container boot) apart from - // "running and serving" — both read as nil — and Modal exposes no cheap - // readiness readback (WaitUntilReady blocks, so we do not call it here). So a - // live sandbox is reported "running" as soon as its process exists, folding the - // brief startup window into running rather than blocking to confirm readiness. + // Status. Poll (== sandboxWait(0)) reports whether the sandbox PROCESS HAS + // EXITED — a non-nil exit code means it is gone (terminated), nil means it is + // still live. Poll alone cannot tell "still scheduling" (queued, image pull, + // GPU attach, container boot) apart from "running and serving": both read as + // nil. So liveness comes from Poll, and readiness — the part that decides + // whether the Pod may be advanced to Running — is read from the latch by + // isReady, which never blocks (see there). if code, err := sb.Poll(ctx, &modal.SandboxPollParams{}); err == nil { - if code != nil { - out.Status = statusTerminated - } else { + switch { + case code != nil: + out.Status = exitStatus(*code) + c.forgetReady(sb.SandboxID) + case c.observeReady(ctx, sb.SandboxID, out.Tags): out.Status = statusRunning + default: + out.Status = statusInitializing } } // Endpoint is only meaningful once running; look it up best-effort. + // TODO: why we need the tunnel. if out.Status == statusRunning { tctx, cancel := context.WithTimeout(ctx, c.endpointTimeout) if tunnels, err := sb.Tunnels(tctx, c.endpointTimeout, &modal.SandboxTunnelsParams{}); err == nil { @@ -278,6 +347,144 @@ func (c *sdkClient) observe(ctx context.Context, sb *modal.Sandbox) Sandbox { return out } +// Exit codes Modal substitutes for a non-exit outcome, since Poll conforms to the +// subprocess API and has only an int to say it with (see getReturnCode in the +// SDK). Both mean "Modal ended this sandbox", not "the workload failed": +// +// sandboxExitTerminated the sandbox was terminated — by our own Terminate on the +// teardown path, or by Modal. +// sandboxExitTimeout the sandbox hit its configured Timeout. +// +// They are the conventional signal-derived codes (128+SIGKILL, 128-4), which is +// exactly why they are AMBIGUOUS: a workload that genuinely exits 137 is +// indistinguishable from a Modal termination. See exitStatus for why that is +// tolerable here. +const ( + sandboxExitTerminated = 137 + sandboxExitTimeout = 124 +) + +// exitStatus classifies an exited sandbox from its Poll exit code, splitting "it +// failed" from "it is gone". +// +// The distinction is worth recovering because Poll is LOSSY in a way that +// flattens the two: the control plane's GenericResult carries eight statuses +// (SUCCESS, FAILURE, INIT_FAILURE, INTERNAL_FAILURE, TERMINATED, TIMEOUT, +// IDLE_TIMEOUT), and getReturnCode collapses every one of them into a single int +// before we ever see it. Treating any exit as terminated therefore reported a +// sandbox that never came up — a bad image, an unavailable GPU, an OOM at init — +// as "the instance is gone", which reads like a clean teardown and hides the +// failure from whoever has to fix it. +// +// The mapping is deliberately conservative, because the collapse cannot be +// undone: only the two codes Modal SUBSTITUTES for a non-exit outcome, plus a +// clean 0, count as terminated; everything else is the workload's own nonzero +// exit and counts as failed. The ambiguity is real but harmless in the direction +// it errs — a workload exiting exactly 137 is read as terminated rather than +// failed, so an odd exit code can understate a failure. It cannot invent one, and +// it cannot affect teardown either way: both states are terminal for the claim, +// which reclaims by asking the provider what exists, not by reading this. +func exitStatus(code int) string { + switch code { + case 0, sandboxExitTerminated, sandboxExitTimeout: + return statusTerminated + default: + return statusFailed + } +} + +// observeReady reports whether a live sandbox may be treated as Running, WITHOUT +// making a network call: it reads the latch and, on a miss, starts the one +// background waiter that will fill it. This is what keeps observe bounded — the +// cost per sandbox per tick is a mutex, not a ~16s blocking wait, so List does +// not degrade with the number of sandboxes. +// +// A sandbox with no probe (no ProbeTagKey) is ready by definition: Modal has no +// readiness concept without one, so there is nothing to wait for and asking would +// error. This folds the startup window into Running for exactly those sandboxes +// Nebula cannot observe — including any created before ProbeTagKey existed. +// +// A miss reports NOT ready. That is the point of the inversion: the previous code +// defaulted to ready and let an ambiguous error promote a sandbox that was still +// booting, which flapped the Pod between Running and Pending (and permanently +// latched its NodeClaim to Bound off one spurious tick). Not-ready is the safe +// default because it is self-correcting — the waiter promotes it within one +// budget — whereas a wrong Running is not. +// It takes no context on purpose: there is nothing here to cancel, which is the +// property that makes it safe to call once per sandbox inside List. +func (c *sdkClient) observeReady(ctx context.Context, id string, tags map[string]string) bool { + if tags[ProbeTagKey] != probeTagValue { + return true + } + + c.readyMu.Lock() + ready, waiting := c.ready[id], false + if !ready { + if _, waiting = c.waiting[id]; !waiting { + c.waiting[id] = struct{}{} + } + } + c.readyMu.Unlock() + + if !ready && !waiting { + go c.awaitReady(ctx, id) + } + return ready +} + +// awaitReady runs the one blocking readiness wait for a sandbox and latches the +// result. It runs in its own goroutine, off the poll loop, which is what lets the +// wait have a budget big enough to actually reach an answer. +// +// It takes its own context rather than inheriting the caller's: the caller is +// List, whose context is cancelled the moment List returns — long before this +// wait could finish. The sandbox is re-resolved by id for the same reason, since +// the *modal.Sandbox the List iterator yielded belongs to that finished call. +// +// Only a CONFIRMED answer latches: +// +// - err == nil: the probe passed. +// - FailedPrecondition: Modal's "sandbox does not have a readiness probe +// configured". A definitive no-probe answer, so it latches ready for the same +// reason a missing ProbeTagKey does. It is reachable when the tag and the +// actual probe disagree (see sandboxSpecFromPod), and latching keeps that from +// costing a full wait every tick. +// +// Anything else — a deadline, a transient API failure, a not-found — latches +// nothing and just clears the in-flight marker, so the next tick retries. Note +// the deliberate absence of error classification: the four shapes an expired +// deadline can take (*status.Error with code DeadlineExceeded, +// context.deadlineExceededError, modal.TimeoutError, modal.SandboxTimeoutError — +// and errors.Is(grpcErr, context.DeadlineExceeded) is FALSE for the first) no +// longer need telling apart, because none of them can promote a sandbox now. +func (c *sdkClient) awaitReady(ctx context.Context, id string) { + ctx, cancel := context.WithTimeout(ctx, c.readyTimeout) + defer cancel() + + confirmed := false + if sb, err := c.mc.Sandboxes.FromID(ctx, id, &modal.SandboxFromIDParams{}); err == nil { + err := sb.WaitUntilReady(ctx, c.readyTimeout, &modal.SandboxWaitUntilReadyParams{}) + confirmed = err == nil || status.Code(err) == codes.FailedPrecondition + } + + c.readyMu.Lock() + defer c.readyMu.Unlock() + delete(c.waiting, id) + if confirmed { + c.ready[id] = true + } +} + +// forgetReady drops a sandbox's latch state. Called when the sandbox is observed +// terminated or explicitly terminated, so the maps track live sandboxes only and +// a recycled id can never inherit a stale ready. +func (c *sdkClient) forgetReady(id string) { + c.readyMu.Lock() + defer c.readyMu.Unlock() + delete(c.ready, id) + delete(c.waiting, id) +} + // gpuReservation renders Modal's GPU reservation string. Modal expresses count // as a "type:count" suffix (e.g. "A100:2"); a count of 0/1 needs no suffix, and // an empty type means a CPU-only sandbox. diff --git a/pkg/provider/modal/modal.go b/pkg/provider/modal/modal.go index b719bf9..b8a4f03 100644 --- a/pkg/provider/modal/modal.go +++ b/pkg/provider/modal/modal.go @@ -111,8 +111,8 @@ type SandboxSpec struct { // ReadinessProbe, when non-nil, is the Pod's first-container readinessProbe // carried through so the Client can configure Modal's own readiness probe at // create time. Modal enforces the probe internally (it gates its own traffic - // routing on it); Nebula does not read the result back — observe reports status - // from the cheap point-in-time Poll signal and never blocks on WaitUntilReady. + // routing on it), and observe reads the result back so a live-but-not-yet-ready + // sandbox is reported statusInitializing rather than statusRunning. // We only ever pass a user-supplied probe; the adapter never fabricates one. ReadinessProbe *corev1.Probe } @@ -130,6 +130,31 @@ type Sandbox struct { // name-encoding hack is needed. const ClaimTagKey = "nebula.inftyai.com/claim" +// ProbeTagKey records that a sandbox was created WITH a readiness probe; +// probeTagValue is the only value it is ever set to. +// +// Nebula has to carry this fact itself. observe needs it to tell "no probe, so +// there is nothing to wait for" from "the probe has not passed yet" — and +// WaitUntilReady errors on the former, so it cannot simply be attempted. The Pod +// cannot answer it either: the read path (Provider.List) takes no Pod, and the +// Pod that GetPod synthesizes when re-adopting after a VK restart carries only a +// namespace and name, hence no spec and no readinessProbe. +// +// Modal's control plane does return it (SandboxInfo.ReadinessProbe, next to a +// ReadyAt stamp), but the Go SDK builds its *Sandbox from a list response with +// info.GetId() alone and keeps the control-plane client private, so neither field +// is reachable through the public API. If a future SDK exposes SandboxInfo, this +// tag and the WaitUntilReady call in observe both become unnecessary. +// +// A tag is the right carrier: observe already fetches tags before it reads status +// (so the gate costs no extra call), and tags live with the sandbox at Modal, so +// probe-ness is recovered exactly the way identity is — even for a sandbox this +// process never created. +const ( + ProbeTagKey = "nebula.inftyai.com/readiness-probe" + probeTagValue = "true" +) + // Provider is the Modal implementation of provider.Provider. It embeds // catalog.Base for the generic catalog methods (Name, Offerings, and the // identity MapAccelerator — Modal names its GPUs exactly like Nebula's canonical @@ -269,6 +294,17 @@ func (p *Provider) sandboxSpecFromPod(pod *corev1.Pod, req provider.ProvisionReq } } + tags := map[string]string{ClaimTagKey: req.ClaimName} + // Record probe-ness alongside identity so observe can recover it later; see + // ProbeTagKey for why this cannot be re-derived at observation time. The tag + // tracks whether Modal will actually RECEIVE a probe, not merely whether the Pod + // declares one — planProbe drops shapes Modal cannot express (a named port, an + // unsupported handler), and tagging those would claim a readiness signal that + // does not exist. + if _, ok := planProbe(c.ReadinessProbe); ok { + tags[ProbeTagKey] = probeTagValue + } + spec := SandboxSpec{ Image: c.Image, Command: append(append([]string{}, c.Command...), c.Args...), @@ -277,7 +313,7 @@ func (p *Provider) sandboxSpecFromPod(pod *corev1.Pod, req provider.ProvisionReq MemoryMiB: memoryMiB(&c), Ports: containerPorts(&c), Timeout: sandboxTimeout(pod), - Tags: map[string]string{ClaimTagKey: req.ClaimName}, + Tags: tags, ReadinessProbe: c.ReadinessProbe, } @@ -373,26 +409,52 @@ func (p *Provider) toInstance(sb Sandbox) provider.Instance { } } -// Sandbox status strings observe produces (and toState consumes). observe -// derives status from Poll, so it only ever emits statusRunning (live) or -// statusTerminated (process exited); an unset status ("") means Poll errored. +// Sandbox status strings observe produces (and toState consumes). An unset +// status ("") means Poll errored. const ( - statusRunning = "running" + // statusRunning: the sandbox process is live AND, when a readiness probe is + // configured, that probe has passed. + statusRunning = "running" + // statusInitializing: the process is live but its readiness probe has not + // passed yet — the sandbox is queued, pulling its image, attaching a GPU, or + // booting. Only ever produced for a sandbox carrying ProbeTagKey, since Modal + // has no readiness signal without a probe. + statusInitializing = "initializing" + // statusTerminated: the process exited in a way that is NOT a failure — it ran + // to completion (exit 0), or Modal reclaimed it (our own Terminate, a sandbox + // timeout). "Gone", with no claim about why. statusTerminated = "terminated" + // statusFailed: the process exited nonzero — it crashed, or never came up at + // all (a bad image, an unavailable GPU, an OOM at init: Modal's INIT_FAILURE). + // Distinct from terminated because "it failed" and "it is gone" are different + // facts to put in front of an operator: a sandbox that never started reported as + // terminated reads like a clean teardown, which is the opposite of what happened. + statusFailed = "failed" ) // toState maps the status strings observe produces to the provider-agnostic -// lifecycle state. observe emits only statusRunning (live) or statusTerminated -// (process exited); "ready" is also accepted as a live synonym. Anything else — -// including the empty string observe leaves when Poll itself errors — maps to -// Pending, so the poll loop keeps watching rather than declaring a premature -// terminal state. +// lifecycle state. +// +// A live sandbox is only reported Running once its readiness probe has passed. +// Modal's cheap Poll signal cannot see readiness at all — it answers "has the +// process exited?", so a sandbox that is still scheduling reads exactly like one +// that is serving — and reporting Running that early advances the Pod, and the +// owning Deployment's ready replicas, before the box can be reached. This mirrors +// the AWS adapter holding a running-but-unchecked instance at Pending until its +// 2/2 EC2 status checks clear. +// +// statusInitializing needs no case of its own: it falls to the default, which is +// where every unrecognized status — including the empty string observe leaves when +// Poll errors — maps to Pending, so the poll loop keeps watching rather than +// declaring a premature terminal state. func toState(modalStatus string) provider.InstanceState { switch strings.ToLower(modalStatus) { - case statusRunning, "ready": + case statusRunning: return provider.InstanceRunning case statusTerminated: return provider.InstanceTerminated + case statusFailed: + return provider.InstanceFailed default: return provider.InstancePending } diff --git a/pkg/provider/modal/modal_test.go b/pkg/provider/modal/modal_test.go index 4885ece..0966aad 100644 --- a/pkg/provider/modal/modal_test.go +++ b/pkg/provider/modal/modal_test.go @@ -318,17 +318,25 @@ func TestOfferings(t *testing.T) { } func TestToState(t *testing.T) { - // The status→state mapping is load-bearing. observe emits only "running"/ - // "ready" (live), "terminated" (exited), or "" (Poll errored); everything else, - // including the empty string, must map to Pending so the poll loop keeps - // watching rather than declaring a premature terminal state. + // The status→state mapping is load-bearing. observe emits "running" (live AND + // ready), "initializing" (live but the readiness probe has not passed), + // "terminated" (exited), or "" (Poll errored); everything else, including the + // empty string, must map to Pending so the poll loop keeps watching rather than + // declaring a premature terminal state. cases := map[string]provider.InstanceState{ - "running": provider.InstanceRunning, - "ready": provider.InstanceRunning, - "pending": provider.InstancePending, - "": provider.InstancePending, // Poll errored, status left unset - "terminated": provider.InstanceTerminated, - "weird-new": provider.InstancePending, // unknown => keep watching, not terminal + "running": provider.InstanceRunning, + // The whole point of the readiness work: a live-but-not-ready sandbox must NOT + // reach Running, or the Pod (and its Deployment's ready count) advances while + // the box is still queued/pulling/booting. + "initializing": provider.InstancePending, + "pending": provider.InstancePending, + "": provider.InstancePending, // Poll errored, status left unset + "terminated": provider.InstanceTerminated, + // A sandbox that exited nonzero — crashed, or never came up (INIT_FAILURE). + // Must NOT read as terminated: "gone" looks like a clean teardown and hides + // the failure. + "failed": provider.InstanceFailed, + "weird-new": provider.InstancePending, // unknown => keep watching, not terminal } for in, want := range cases { if got := toState(in); got != want { @@ -337,6 +345,92 @@ func TestToState(t *testing.T) { } } +// TestExitStatus pins the exit-code classification. Poll collapses eight +// control-plane statuses into one int (see exitStatus), so this split is inference +// and its direction matters: only a clean exit and the two codes Modal SUBSTITUTES +// for a non-exit outcome are "gone"; any other nonzero exit is a real failure. +func TestExitStatus(t *testing.T) { + cases := map[int]string{ + 0: statusTerminated, // ran to completion + 137: statusTerminated, // Modal terminated it (our Terminate, or Modal's) + 124: statusTerminated, // sandbox Timeout elapsed + 1: statusFailed, // the workload crashed + 2: statusFailed, + 127: statusFailed, // command not found + // The case this whole split exists for: a sandbox that never started (bad + // image, no GPU available, OOM at init) must surface as failed, not gone. + 139: statusFailed, + } + for code, want := range cases { + if got := exitStatus(code); got != want { + t.Fatalf("exitStatus(%d) = %q, want %q", code, got, want) + } + } +} + +// TestToInstance_ReadinessGatesRunning pins the end-to-end consequence through the +// public surface: only a "running" sandbox becomes InstanceRunning, which is what +// applyState turns into PodRunning + Ready=True. +func TestToInstance_ReadinessGatesRunning(t *testing.T) { + p := newTestProvider(&fakeClient{}) + cases := []struct { + status string + want provider.InstanceState + }{ + {statusRunning, provider.InstanceRunning}, + {statusInitializing, provider.InstancePending}, + {statusTerminated, provider.InstanceTerminated}, + } + for _, tc := range cases { + got := p.toInstance(Sandbox{ID: "sb-1", Status: tc.status}) + if got.State != tc.want { + t.Errorf("toInstance(status=%q).State = %q, want %q", tc.status, got.State, tc.want) + } + } +} + +// TestProvision_ProbeTagStampedOnlyWithProbe: the tag is how observe recovers +// probe-ness after a restart (it cannot be re-derived — see ProbeTagKey), so it +// must be present exactly when the Pod carries a readinessProbe. Stamping it on a +// probe-less sandbox would make observe call WaitUntilReady, which errors on one. +func TestProvision_ProbeTagStampedOnlyWithProbe(t *testing.T) { + probe := &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + Exec: &corev1.ExecAction{Command: []string{"true"}}, + }, + } + for _, tc := range []struct { + name string + probe *corev1.Probe + want bool + }{ + {"with probe", probe, true}, + {"without probe", nil, false}, + } { + t.Run(tc.name, func(t *testing.T) { + f := &fakeClient{createID: "sb-1"} + p := newTestProvider(f) + pod := gpuPod("claim-a", "H100", 1) + pod.Spec.Containers[0].ReadinessProbe = tc.probe + + if _, err := p.Provision(context.Background(), pod, provider.ProvisionRequest{ClaimName: "claim-a"}); err != nil { + t.Fatalf("Provision: %v", err) + } + _, present := f.lastSpec.Tags[ProbeTagKey] + if present != tc.want { + t.Errorf("%s tag present = %v, want %v (tags=%v)", ProbeTagKey, present, tc.want, f.lastSpec.Tags) + } + if tc.want && f.lastSpec.Tags[ProbeTagKey] != probeTagValue { + t.Errorf("%s = %q, want %q", ProbeTagKey, f.lastSpec.Tags[ProbeTagKey], probeTagValue) + } + // Identity must survive alongside it. + if f.lastSpec.Tags[ClaimTagKey] != "claim-a" { + t.Errorf("%s = %q, want claim-a", ClaimTagKey, f.lastSpec.Tags[ClaimTagKey]) + } + }) + } +} + func TestProvision_ReadinessProbeCarriedThrough(t *testing.T) { f := &fakeClient{createID: "sb-1"} p := newTestProvider(f) diff --git a/pkg/vnode/handler.go b/pkg/vnode/handler.go index b309837..119f2f4 100644 --- a/pkg/vnode/handler.go +++ b/pkg/vnode/handler.go @@ -203,6 +203,25 @@ func (h *Handler) CreatePod(ctx context.Context, pod *corev1.Pod) error { log.Info("provisioning external instance", "capacityType", req.CapacityType, "region", req.Region, "timeout", timeout.String()) + + // Report Provisioning BEFORE the call, not after it. Provision blocks until the + // provider has created the instance (seconds for Modal, longer for AWS, which + // creates a launch template and runs an instant fleet), and that wait is the only + // window in which "no instance exists yet" is actually true. Stamping it + // afterwards described a state that was already over, so Provisioning was + // effectively unobservable and indistinguishable from Initializing except by + // timing. + // + // Emit WITHOUT store: the pod must NOT be tracked yet. reconcileOnce treats a + // tracked pod that is absent from List() as Terminated, and during this window it + // is legitimately absent — so tracking it here lets a concurrent poll tick write + // Failed/Terminated over a provision that is still succeeding. That write is + // unrecoverable (Pod phases are terminal-sticky) and the claim would reclaim on + // it. Emitting alone is safe: notify only pushes status, and persistEndpoint + // no-ops on an endpoint-less pod, so nothing needs the tracking entry yet. + h.markStatus(pod, corev1.PodPending, reasonProvisioning, "allocating external instance") + h.emit(pod) + id, err := h.prov.Provision(ctx, pod, req) if err != nil { log.Error(err, "provision failed; Pod marked Failed for failover") @@ -221,9 +240,28 @@ func (h *Handler) CreatePod(ctx context.Context, pod *corev1.Pod) error { return err } - // Record the instance id before reporting success; teardown relies on it. + // The instance now EXISTS at the provider — listable, terminable, and billable — + // so report Initializing rather than leaving the Pod at Provisioning until the + // first poll tick. That closes a real gap: the claim reads Initializing as its + // proof an instance exists, and until that reason lands the claim stays + // Provisioning, so a Pod deleted in the window would be reclaimed only after the + // placement grace period even though something was already running. + // + // "Initializing" is deliberately about EXISTENCE, not boot progress. For AWS it is + // also literally booting (an instant fleet allocates capacity synchronously), but a + // Modal sandbox may still be queued for a GPU. Reporting Provisioning for it would + // be worse, not better: it would mean the id — and the reclaim obligation — exists + // while the Pod still claims nothing does, and a queued sandbox bills. The + // queued/booting split is genuinely unobservable through Modal's public API (see + // docs/status.md), so both collapse into the one honest statement available: it + // exists and is not yet ready. This matches applyState's InstancePending case, so + // the first poll tick confirms this status rather than changing it. + // + // markStatus precedes store because store deep-copies: storing first would track a + // Pod still carrying Provisioning, and GetPodStatus would serve that stale reason + // until a poll tick overwrote it. log.Info("external instance provisioned", "instanceID", id) - h.markStatus(pod, corev1.PodPending, reasonProvisioning, "provisioning external instance") + h.markStatus(pod, corev1.PodPending, reasonInitializing, "external instance is initializing") h.store(pod, claim, id) h.emit(pod) return nil diff --git a/pkg/vnode/handler_test.go b/pkg/vnode/handler_test.go index e1ff35c..a46b5ea 100644 --- a/pkg/vnode/handler_test.go +++ b/pkg/vnode/handler_test.go @@ -55,12 +55,21 @@ type fakeProvider struct { classifyScope provider.BlockScope classifyAccel string classifyRegion string + // provisionHook runs inside Provision, before it returns, so a test can observe + // the status the handler published for the window in which the provider call is + // still in flight. + provisionHook func() } func (f *fakeProvider) Name() string { return "fake" } func (f *fakeProvider) Capabilities() provider.Capabilities { return f.capabilities } func (f *fakeProvider) Provision(_ context.Context, _ *corev1.Pod, req provider.ProvisionRequest) (string, error) { + // Outside the lock: the hook reads Handler state, and holding f.mu here would + // deadlock a hook that touches the provider. + if f.provisionHook != nil { + f.provisionHook() + } f.mu.Lock() defer f.mu.Unlock() f.provisionCnt++ @@ -416,21 +425,102 @@ func TestNotify_PersistsEndpointAnnotationOnce(t *testing.T) { } } +// The two reasons at phase Pending mean different things — "no instance exists +// yet" versus "it exists and is not yet ready" — so each has to be reported in +// its own window. Provisioning is only true while the provider call is in flight, +// and Initializing must be published the moment the call returns an id, because +// the NodeClaim reads that reason as its proof that a billable instance exists. +func TestCreatePod_ProvisioningWhileInFlightThenInitializing(t *testing.T) { + fp := &fakeProvider{provisionID: "inst-1"} + h := NewHandler(fp, nil, nil) + + var mu sync.Mutex + var emitted []string + h.NotifyPods(context.Background(), func(p *corev1.Pod) { + mu.Lock() + emitted = append(emitted, string(p.Status.Phase)+"/"+p.Status.Reason) + mu.Unlock() + }) + + fp.provisionHook = func() { + mu.Lock() + defer mu.Unlock() + if len(emitted) != 1 || emitted[0] != string(corev1.PodPending)+"/"+reasonProvisioning { + t.Errorf("while Provision is in flight, expected a single %q emit, got %v", + reasonProvisioning, emitted) + } + } + + if err := h.CreatePod(context.Background(), testPod("default", "p1")); err != nil { + t.Fatalf("CreatePod: %v", err) + } + + // The instance now exists, so the reason must ALREADY have advanced — waiting for + // the first poll tick would hold the claim at Provisioning (and so behind the + // placement grace period) while the instance bills. + got, err := h.GetPod(context.Background(), "default", "p1") + if err != nil { + t.Fatalf("GetPod: %v", err) + } + if got.Status.Phase != corev1.PodPending { + t.Fatalf("expected phase Pending, got %q", got.Status.Phase) + } + if got.Status.Reason != reasonInitializing { + t.Fatalf("after Provision returned, expected reason %q, got %q", reasonInitializing, got.Status.Reason) + } +} + +// A Pod must NOT be tracked while its Provision call is still in flight. The poll +// loop maps a tracked pod that is absent from List() to Terminated, and in that +// window it is legitimately absent — so tracking it early lets a concurrent tick +// write Failed/Terminated over a provision that goes on to succeed. Pod phases are +// terminal-sticky and the claim reclaims on that phase, so the write is +// unrecoverable: the instance is provisioned and then immediately torn down. +func TestCreatePod_PollTickDuringProvisionDoesNotTerminate(t *testing.T) { + fp := &fakeProvider{provisionID: "inst-1"} + h := NewHandler(fp, nil, nil) + + var mu sync.Mutex + var emitted []string + h.NotifyPods(context.Background(), func(p *corev1.Pod) { + mu.Lock() + emitted = append(emitted, string(p.Status.Phase)+"/"+p.Status.Reason) + mu.Unlock() + }) + + // A tick lands mid-provision, when the provider genuinely has no instance yet. + fp.provisionHook = func() { h.reconcileOnce(context.Background()) } + + if err := h.CreatePod(context.Background(), testPod("default", "p1")); err != nil { + t.Fatalf("CreatePod: %v", err) + } + + mu.Lock() + defer mu.Unlock() + for _, s := range emitted { + if s == string(corev1.PodFailed)+"/"+reasonTerminated { + t.Fatalf("a poll tick during Provision reported the pod Terminated: %v", emitted) + } + } + if got, _ := h.GetPod(context.Background(), "default", "p1"); got.Status.Reason != reasonInitializing { + t.Fatalf("expected %q after a successful provision, got %q", reasonInitializing, got.Status.Reason) + } +} + func TestReconcileOnce_NotifiesOnProvisioningToInitializing(t *testing.T) { - // The Pod starts at "Provisioning" (phase Pending), and the instance comes up - // but has not yet passed its readiness checks => InstancePending, which maps to - // the "Initializing" reason at the SAME phase (Pending). A phase-only change - // check would swallow this and strand the Pod on the stale "Provisioning" - // reason; the reason must move and a notification must fire. + // The instance comes up but has not yet passed its readiness checks => + // InstancePending, which maps to the "Initializing" reason at phase Pending. The + // Pod is already Initializing when CreatePod returns, so what this pins is the + // notification: a phase-only change check would swallow a same-phase reason move + // and strand the Pod on a stale reason. fp := &fakeProvider{provisionID: "inst-1"} h := NewHandler(fp, nil, nil) pod := testPod("default", "p1") _ = h.CreatePod(context.Background(), pod) - // Sanity: CreatePod left it at the Provisioning reason (still Pending). - if got, _ := h.GetPod(context.Background(), "default", "p1"); got.Status.Reason != reasonProvisioning { - t.Fatalf("precondition: expected %q, got %q", reasonProvisioning, got.Status.Reason) - } + // Force the stale reason back on, so the tick below has a change to report. + h.markStatus(pod, corev1.PodPending, reasonProvisioning, "allocating external instance") + h.store(pod, "default-p1", "inst-1") var mu sync.Mutex var notified []*corev1.Pod diff --git a/pkg/vnode/status.go b/pkg/vnode/status.go index c4b0c76..4adf782 100644 --- a/pkg/vnode/status.go +++ b/pkg/vnode/status.go @@ -22,36 +22,24 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + nebulav1alpha1 "github.com/InftyAI/Nebula/api/v1alpha1" "github.com/InftyAI/Nebula/pkg/provider" ) -// Pod status reasons the virtual node stamps on the Pod it reports. The Pod is -// the source of truth for the external instance's runtime state (the NodeClaim -// is a passive ledger and does not mirror this), so these live here in the -// vnode, not on the API type. +// Pod status reasons the virtual node stamps on the Pod it reports. The Pod is the +// source of truth for the external instance's runtime state (the NodeClaim is a +// passive ledger and does not mirror it), and this package is the only writer — but +// the reason strings themselves are a shared contract (the NodeClaim controller +// reads them, and operators match on them), so they are declared once in +// api/v1alpha1 rather than privately here. See the const block there for what each +// value means and why the set is public. const ( - // reasonProvisioning: a provider Provision call has been issued but the - // instance does not yet exist — we are still allocating it (e.g. EC2 - // RunInstances in flight). Set on CreatePod, before the first poll observes - // the instance. - reasonProvisioning = "Provisioning" - // reasonInitializing: the instance EXISTS at the provider but is not yet - // reachable — it is booting (EC2 "pending") or running-but-not-yet-passing its - // reachability checks (running, <2/2, EC2's own "Initializing" status). It - // mirrors that EC2 status-check term. Provisioning is done; the instance is - // coming up. Distinct from Provisioning so a Pod stuck here points at a slow - // boot / failing status checks, not a stuck allocation. - reasonInitializing = "Initializing" - // reasonRunning: the provider reports the instance running. - reasonRunning = "Running" - // reasonProvisionFailed: the provider rejected or failed the Provision call. - reasonProvisionFailed = "ProvisionFailed" - // reasonFailed: the provider reports the instance in a failed state. - reasonFailed = "Failed" - // reasonTerminated: the instance is gone from the provider (torn down, - // reclaimed, or exited). Disappearance alone does not say WHY, so this is the - // neutral term rather than "Preempted". - reasonTerminated = "Terminated" + reasonProvisioning = nebulav1alpha1.PodReasonProvisioning + reasonInitializing = nebulav1alpha1.PodReasonInitializing + reasonRunning = nebulav1alpha1.PodReasonRunning + reasonProvisionFailed = nebulav1alpha1.PodReasonProvisionFailed + reasonFailed = nebulav1alpha1.PodReasonFailed + reasonTerminated = nebulav1alpha1.PodReasonTerminated ) // applyState maps a provider Instance state onto the Pod status the virtual node @@ -64,10 +52,16 @@ const ( // Terminated -> PodFailed (instance gone: torn down or reclaimed out-of-band) // // Running already means "reachable": a provider only reports InstanceRunning once -// the instance has passed its readiness bar (for AWS, the 2/2 EC2 status checks — -// see toState), so reaching Running is the point at which the Pod is both Running -// and Ready. Ready is the condition Kubernetes counts toward a Deployment's ready -// replicas. +// the instance has passed its readiness bar — for AWS the 2/2 EC2 status checks, +// for Modal its readiness probe when the sandbox was created with one (see each +// adapter's toState). So reaching Running is the point at which the Pod is both +// Running and Ready. Ready is the condition Kubernetes counts toward a +// Deployment's ready replicas, which is why holding it back until the instance is +// genuinely reachable matters. +// +// The bar is only as good as the provider's signal: a Modal sandbox created +// WITHOUT a readiness probe has no observable readiness at all, so it reaches +// Running as soon as its process is live. func applyState(pod *corev1.Pod, state provider.InstanceState, endpoint string, now metav1.Time) { switch state { case provider.InstanceRunning: From b928f58542b50665d617a0c85b3cc452f622d473 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Tue, 11 Aug 2026 16:31:36 +0100 Subject: [PATCH 2/6] revert some change Signed-off-by: kerthcet --- docs/status.md | 41 +++++---------- pkg/vnode/handler.go | 42 +-------------- pkg/vnode/handler_test.go | 108 ++++---------------------------------- 3 files changed, 25 insertions(+), 166 deletions(-) diff --git a/docs/status.md b/docs/status.md index 983762b..5a25b40 100644 --- a/docs/status.md +++ b/docs/status.md @@ -52,33 +52,20 @@ served Pod is ABSENT: after `Bound`/`Terminating`, the claim deletes itself and the terminate finalizer runs; before `Bound`, it waits `placementGracePeriod` first. -Note the two writers. `CreatePod` writes the rows where no instance exists, plus -the first `Initializing` — it publishes `Provisioning` *before* calling the -provider and `Initializing` as soon as the call returns an id, so each reason -covers exactly the window in which it is true: `Provisioning` is the blocking -allocation call itself, and `Initializing` starts the moment an instance exists. -Every other non-terminal row comes from `applyState`, driven by the poll loop, -and is therefore only reachable for an instance the provider actually returned -from `List()`. - -The ordering matters in both directions. Writing `Provisioning` after the call -described a state already over, so it was effectively unobservable, while leaving -`Initializing` to the first tick would hold the claim at `Provisioning` — and so -behind the `placementGracePeriod` — for up to 15 seconds after a billable instance -existed. - -The pre-call write is emitted but NOT tracked, and that distinction is load-bearing: -the poll loop maps a tracked Pod absent from `List()` to `Terminated`, and during -the provider call the instance is legitimately absent, so tracking it there would -let a concurrent tick write `Failed`/`Terminated` over a provision that goes on to -succeed — unrecoverably, since Pod phases are terminal-sticky and the claim -reclaims on that phase. - -`Initializing` asserts EXISTENCE, not boot progress. On AWS the instance is also -genuinely booting, because an instant fleet allocates capacity synchronously. A -Modal sandbox may still be queued — but reporting `Provisioning` for it would be -worse: the id, and with it the reclaim obligation, already exists, and a queued -sandbox bills. +Note the two writers. `CreatePod` writes the rows where no instance exists; every +other non-terminal row comes from `applyState`, driven by the poll loop, and is +therefore only reachable for an instance the provider actually returned from +`List()`. + +`Provisioning` is written *after* `Provision` returns, so today it is barely +observable: by the time it lands the instance already exists, and the first poll +tick (≤15s) replaces it with `Initializing`. That makes the two reasons hard to +tell apart in practice even though they mean different things — "nothing exists +yet" versus "it exists and is not yet ready". Fixing this is not simply a matter of +writing `Provisioning` earlier: a Pod must not be tracked before its instance +exists, because the poll loop maps a tracked Pod absent from `List()` to +`Terminated`, which is unrecoverable (Pod phases are terminal-sticky and the claim +reclaims on that phase). Important details: diff --git a/pkg/vnode/handler.go b/pkg/vnode/handler.go index 119f2f4..b309837 100644 --- a/pkg/vnode/handler.go +++ b/pkg/vnode/handler.go @@ -203,25 +203,6 @@ func (h *Handler) CreatePod(ctx context.Context, pod *corev1.Pod) error { log.Info("provisioning external instance", "capacityType", req.CapacityType, "region", req.Region, "timeout", timeout.String()) - - // Report Provisioning BEFORE the call, not after it. Provision blocks until the - // provider has created the instance (seconds for Modal, longer for AWS, which - // creates a launch template and runs an instant fleet), and that wait is the only - // window in which "no instance exists yet" is actually true. Stamping it - // afterwards described a state that was already over, so Provisioning was - // effectively unobservable and indistinguishable from Initializing except by - // timing. - // - // Emit WITHOUT store: the pod must NOT be tracked yet. reconcileOnce treats a - // tracked pod that is absent from List() as Terminated, and during this window it - // is legitimately absent — so tracking it here lets a concurrent poll tick write - // Failed/Terminated over a provision that is still succeeding. That write is - // unrecoverable (Pod phases are terminal-sticky) and the claim would reclaim on - // it. Emitting alone is safe: notify only pushes status, and persistEndpoint - // no-ops on an endpoint-less pod, so nothing needs the tracking entry yet. - h.markStatus(pod, corev1.PodPending, reasonProvisioning, "allocating external instance") - h.emit(pod) - id, err := h.prov.Provision(ctx, pod, req) if err != nil { log.Error(err, "provision failed; Pod marked Failed for failover") @@ -240,28 +221,9 @@ func (h *Handler) CreatePod(ctx context.Context, pod *corev1.Pod) error { return err } - // The instance now EXISTS at the provider — listable, terminable, and billable — - // so report Initializing rather than leaving the Pod at Provisioning until the - // first poll tick. That closes a real gap: the claim reads Initializing as its - // proof an instance exists, and until that reason lands the claim stays - // Provisioning, so a Pod deleted in the window would be reclaimed only after the - // placement grace period even though something was already running. - // - // "Initializing" is deliberately about EXISTENCE, not boot progress. For AWS it is - // also literally booting (an instant fleet allocates capacity synchronously), but a - // Modal sandbox may still be queued for a GPU. Reporting Provisioning for it would - // be worse, not better: it would mean the id — and the reclaim obligation — exists - // while the Pod still claims nothing does, and a queued sandbox bills. The - // queued/booting split is genuinely unobservable through Modal's public API (see - // docs/status.md), so both collapse into the one honest statement available: it - // exists and is not yet ready. This matches applyState's InstancePending case, so - // the first poll tick confirms this status rather than changing it. - // - // markStatus precedes store because store deep-copies: storing first would track a - // Pod still carrying Provisioning, and GetPodStatus would serve that stale reason - // until a poll tick overwrote it. + // Record the instance id before reporting success; teardown relies on it. log.Info("external instance provisioned", "instanceID", id) - h.markStatus(pod, corev1.PodPending, reasonInitializing, "external instance is initializing") + h.markStatus(pod, corev1.PodPending, reasonProvisioning, "provisioning external instance") h.store(pod, claim, id) h.emit(pod) return nil diff --git a/pkg/vnode/handler_test.go b/pkg/vnode/handler_test.go index a46b5ea..e1ff35c 100644 --- a/pkg/vnode/handler_test.go +++ b/pkg/vnode/handler_test.go @@ -55,21 +55,12 @@ type fakeProvider struct { classifyScope provider.BlockScope classifyAccel string classifyRegion string - // provisionHook runs inside Provision, before it returns, so a test can observe - // the status the handler published for the window in which the provider call is - // still in flight. - provisionHook func() } func (f *fakeProvider) Name() string { return "fake" } func (f *fakeProvider) Capabilities() provider.Capabilities { return f.capabilities } func (f *fakeProvider) Provision(_ context.Context, _ *corev1.Pod, req provider.ProvisionRequest) (string, error) { - // Outside the lock: the hook reads Handler state, and holding f.mu here would - // deadlock a hook that touches the provider. - if f.provisionHook != nil { - f.provisionHook() - } f.mu.Lock() defer f.mu.Unlock() f.provisionCnt++ @@ -425,102 +416,21 @@ func TestNotify_PersistsEndpointAnnotationOnce(t *testing.T) { } } -// The two reasons at phase Pending mean different things — "no instance exists -// yet" versus "it exists and is not yet ready" — so each has to be reported in -// its own window. Provisioning is only true while the provider call is in flight, -// and Initializing must be published the moment the call returns an id, because -// the NodeClaim reads that reason as its proof that a billable instance exists. -func TestCreatePod_ProvisioningWhileInFlightThenInitializing(t *testing.T) { - fp := &fakeProvider{provisionID: "inst-1"} - h := NewHandler(fp, nil, nil) - - var mu sync.Mutex - var emitted []string - h.NotifyPods(context.Background(), func(p *corev1.Pod) { - mu.Lock() - emitted = append(emitted, string(p.Status.Phase)+"/"+p.Status.Reason) - mu.Unlock() - }) - - fp.provisionHook = func() { - mu.Lock() - defer mu.Unlock() - if len(emitted) != 1 || emitted[0] != string(corev1.PodPending)+"/"+reasonProvisioning { - t.Errorf("while Provision is in flight, expected a single %q emit, got %v", - reasonProvisioning, emitted) - } - } - - if err := h.CreatePod(context.Background(), testPod("default", "p1")); err != nil { - t.Fatalf("CreatePod: %v", err) - } - - // The instance now exists, so the reason must ALREADY have advanced — waiting for - // the first poll tick would hold the claim at Provisioning (and so behind the - // placement grace period) while the instance bills. - got, err := h.GetPod(context.Background(), "default", "p1") - if err != nil { - t.Fatalf("GetPod: %v", err) - } - if got.Status.Phase != corev1.PodPending { - t.Fatalf("expected phase Pending, got %q", got.Status.Phase) - } - if got.Status.Reason != reasonInitializing { - t.Fatalf("after Provision returned, expected reason %q, got %q", reasonInitializing, got.Status.Reason) - } -} - -// A Pod must NOT be tracked while its Provision call is still in flight. The poll -// loop maps a tracked pod that is absent from List() to Terminated, and in that -// window it is legitimately absent — so tracking it early lets a concurrent tick -// write Failed/Terminated over a provision that goes on to succeed. Pod phases are -// terminal-sticky and the claim reclaims on that phase, so the write is -// unrecoverable: the instance is provisioned and then immediately torn down. -func TestCreatePod_PollTickDuringProvisionDoesNotTerminate(t *testing.T) { - fp := &fakeProvider{provisionID: "inst-1"} - h := NewHandler(fp, nil, nil) - - var mu sync.Mutex - var emitted []string - h.NotifyPods(context.Background(), func(p *corev1.Pod) { - mu.Lock() - emitted = append(emitted, string(p.Status.Phase)+"/"+p.Status.Reason) - mu.Unlock() - }) - - // A tick lands mid-provision, when the provider genuinely has no instance yet. - fp.provisionHook = func() { h.reconcileOnce(context.Background()) } - - if err := h.CreatePod(context.Background(), testPod("default", "p1")); err != nil { - t.Fatalf("CreatePod: %v", err) - } - - mu.Lock() - defer mu.Unlock() - for _, s := range emitted { - if s == string(corev1.PodFailed)+"/"+reasonTerminated { - t.Fatalf("a poll tick during Provision reported the pod Terminated: %v", emitted) - } - } - if got, _ := h.GetPod(context.Background(), "default", "p1"); got.Status.Reason != reasonInitializing { - t.Fatalf("expected %q after a successful provision, got %q", reasonInitializing, got.Status.Reason) - } -} - func TestReconcileOnce_NotifiesOnProvisioningToInitializing(t *testing.T) { - // The instance comes up but has not yet passed its readiness checks => - // InstancePending, which maps to the "Initializing" reason at phase Pending. The - // Pod is already Initializing when CreatePod returns, so what this pins is the - // notification: a phase-only change check would swallow a same-phase reason move - // and strand the Pod on a stale reason. + // The Pod starts at "Provisioning" (phase Pending), and the instance comes up + // but has not yet passed its readiness checks => InstancePending, which maps to + // the "Initializing" reason at the SAME phase (Pending). A phase-only change + // check would swallow this and strand the Pod on the stale "Provisioning" + // reason; the reason must move and a notification must fire. fp := &fakeProvider{provisionID: "inst-1"} h := NewHandler(fp, nil, nil) pod := testPod("default", "p1") _ = h.CreatePod(context.Background(), pod) - // Force the stale reason back on, so the tick below has a change to report. - h.markStatus(pod, corev1.PodPending, reasonProvisioning, "allocating external instance") - h.store(pod, "default-p1", "inst-1") + // Sanity: CreatePod left it at the Provisioning reason (still Pending). + if got, _ := h.GetPod(context.Background(), "default", "p1"); got.Status.Reason != reasonProvisioning { + t.Fatalf("precondition: expected %q, got %q", reasonProvisioning, got.Status.Reason) + } var mu sync.Mutex var notified []*corev1.Pod From c08637c96b9483774a627d8825493e73fcbc0a3b Mon Sep 17 00:00:00 2001 From: kerthcet Date: Tue, 11 Aug 2026 16:38:44 +0100 Subject: [PATCH 3/6] fix yaml Signed-off-by: kerthcet --- config/manager/kustomization.yaml | 4 ++-- config/samples/deployment.yaml | 2 +- config/samples/nodepool.yaml | 20 ++++++++++---------- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/config/manager/kustomization.yaml b/config/manager/kustomization.yaml index d0cdc06..086bc1e 100644 --- a/config/manager/kustomization.yaml +++ b/config/manager/kustomization.yaml @@ -4,5 +4,5 @@ apiVersion: kustomize.config.k8s.io/v1beta1 kind: Kustomization images: - name: controller - newName: nebula - newTag: dev + newName: inftyai/nebula-controller + newTag: latest diff --git a/config/samples/deployment.yaml b/config/samples/deployment.yaml index 02b2966..18b9cf4 100644 --- a/config/samples/deployment.yaml +++ b/config/samples/deployment.yaml @@ -28,7 +28,7 @@ metadata: labels: app.kubernetes.io/managed-by: nebula spec: - replicas: 8 + replicas: 2 selector: matchLabels: app: gpu-workload-sample diff --git a/config/samples/nodepool.yaml b/config/samples/nodepool.yaml index a741081..8503656 100644 --- a/config/samples/nodepool.yaml +++ b/config/samples/nodepool.yaml @@ -7,17 +7,17 @@ metadata: spec: # strategy (the inner, provider-ranking axis). providers: + - name: aws + regions: + - us-east-1 + - us-west-1 + - ap-south-1 + - ap-northeast-1 + - eu-central-1 + - eu-west-1 + - ca-central-1 + - sa-east-1 - name: modal - # - name: aws - # regions: - # - us-east-1 - # - us-west-1 - # - ap-south-1 - # - ap-northeast-1 - # - eu-central-1 - # - eu-west-1 - # - ca-central-1 - # - sa-east-1 # - name: runpod # Outer axis: try OnDemand on every provider first, fall back to Spot. capacityTypes: From ff8152ddf944547df5988eef9cd7019aee77fb2c Mon Sep 17 00:00:00 2001 From: kerthcet Date: Tue, 11 Aug 2026 16:43:57 +0100 Subject: [PATCH 4/6] fix status flow Signed-off-by: kerthcet --- docs/architecture.md | 52 +++++++++++++++++++++++++++++++------------- 1 file changed, 37 insertions(+), 15 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 8d9b1b8..d4aa234 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -201,24 +201,46 @@ Follow one GPU Pod from creation to teardown: ### Status Flow -Once a Pod is placed, its status is driven by the poll loop rather than the -placement path: the virtual kubelet projects the external instance's lifecycle -onto standard Pod status, and the NodeClaim controller derives its coarse ledger -phase from the served Pod. +Pod status and NodeClaim status come from different sources. The Pod is the +runtime surface users watch. The NodeClaim is the coarse ledger that protects +teardown. + +``` +Pod.status, written by pkg/vnode + CreatePod success -> Pending / Provisioning + CreatePod error -> Failed / ProvisionFailed + List sees Pending -> Pending / Initializing + List sees Running -> Running / Ready=True / endpoint annotation + List sees Failed -> Failed / Failed + List misses instance -> Failed / Terminated + DeletePod success -> Succeeded / Terminated + +NodeClaim.status, written by NodeClaim controller + present Pod, no instance yet -> Provisioning + present Pod, initializing instance -> Initializing + present Running Pod -> Bound + present deleting Pod -> Terminating + present terminal Pod -> Terminated + absent after Bound/Terminating -> delete self -> terminate finalizer + absent before Bound -> wait placementGracePeriod, then delete +``` + +Important details: + +- `Bound` is the teardown guard. Once a claim has seen a Running Pod, a later Pod + disappearance is trusted as real teardown, not cache lag. +- `Provisioning` and `Initializing` do not earn the guard. If the Pod is absent + before `Bound`, the controller waits `placementGracePeriod` (15 seconds) before + deleting an orphaned claim. +- `NodeClaimStatus.InstanceID` is recorded on a best-effort basis. The finalizer + prefers it when present, but can still recover by matching provider instances + by claim name through `List()`. +- NodeClaim does not mirror logs, restarts, container state, or fine-grained + runtime health. Those belong on the Pod. The full mapping — Pod phase/reason and the claim phase each produces, plus each provider's own status vocabulary and the limits of what is observable — lives in -[docs/status.md](status.md). Two properties matter for the placement flow -described above: - -- `Bound` means an instance EXISTS at the provider, not that the workload is - ready. It is the teardown guard: once a claim is `Bound`, a later Pod - disappearance is trusted as real teardown rather than informer cache lag, and is - reclaimed with no grace period. Only `Provisioning` (nothing created yet) waits - out `placementGracePeriod`. -- `NodeClaimStatus.InstanceID` is recorded on a best-effort basis. The finalizer - prefers it when present, but can still recover by matching provider instances by - claim name through `List()`. +[docs/status.md](status.md). --- From 3ef25d723029402b13325923950b17a662ba43aa Mon Sep 17 00:00:00 2001 From: kerthcet Date: Tue, 11 Aug 2026 17:48:38 +0100 Subject: [PATCH 5/6] fix lint Signed-off-by: kerthcet --- config/samples/deployment.yaml | 4 ++-- pkg/provider/modal/client.go | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/config/samples/deployment.yaml b/config/samples/deployment.yaml index 18b9cf4..b1353a3 100644 --- a/config/samples/deployment.yaml +++ b/config/samples/deployment.yaml @@ -28,7 +28,7 @@ metadata: labels: app.kubernetes.io/managed-by: nebula spec: - replicas: 2 + replicas: 8 selector: matchLabels: app: gpu-workload-sample @@ -38,7 +38,7 @@ spec: app: gpu-workload-sample nebula.inftyai.com/enabled: "true" nebula.inftyai.com/nodepool: sample - nebula.inftyai.com/accelerator-type: l4 + nebula.inftyai.com/accelerator-type: a100-80gb spec: # Do NOT set nodeName or a provider nodeSelector yourself — the placement # controller fills the nodeSelector in when it ungates the Pod. Setting diff --git a/pkg/provider/modal/client.go b/pkg/provider/modal/client.go index 94e54fd..f5f09b2 100644 --- a/pkg/provider/modal/client.go +++ b/pkg/provider/modal/client.go @@ -427,7 +427,7 @@ func (c *sdkClient) observeReady(ctx context.Context, id string, tags map[string c.readyMu.Unlock() if !ready && !waiting { - go c.awaitReady(ctx, id) + go c.awaitReady(id) } return ready } @@ -457,8 +457,8 @@ func (c *sdkClient) observeReady(ctx context.Context, id string, tags map[string // context.deadlineExceededError, modal.TimeoutError, modal.SandboxTimeoutError — // and errors.Is(grpcErr, context.DeadlineExceeded) is FALSE for the first) no // longer need telling apart, because none of them can promote a sandbox now. -func (c *sdkClient) awaitReady(ctx context.Context, id string) { - ctx, cancel := context.WithTimeout(ctx, c.readyTimeout) +func (c *sdkClient) awaitReady(id string) { + ctx, cancel := context.WithTimeout(context.Background(), c.readyTimeout) defer cancel() confirmed := false From 8a6611beae38ac96ce852374f6cbdd432e7101a1 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Tue, 11 Aug 2026 17:51:28 +0100 Subject: [PATCH 6/6] fix lint Signed-off-by: kerthcet --- pkg/provider/modal/client.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/provider/modal/client.go b/pkg/provider/modal/client.go index f5f09b2..39eac4e 100644 --- a/pkg/provider/modal/client.go +++ b/pkg/provider/modal/client.go @@ -325,7 +325,7 @@ func (c *sdkClient) observe(ctx context.Context, sb *modal.Sandbox) Sandbox { case code != nil: out.Status = exitStatus(*code) c.forgetReady(sb.SandboxID) - case c.observeReady(ctx, sb.SandboxID, out.Tags): + case c.observeReady(sb.SandboxID, out.Tags): out.Status = statusRunning default: out.Status = statusInitializing @@ -412,7 +412,7 @@ func exitStatus(code int) string { // budget — whereas a wrong Running is not. // It takes no context on purpose: there is nothing here to cancel, which is the // property that makes it safe to call once per sandbox inside List. -func (c *sdkClient) observeReady(ctx context.Context, id string, tags map[string]string) bool { +func (c *sdkClient) observeReady(id string, tags map[string]string) bool { if tags[ProbeTagKey] != probeTagValue { return true }