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/samples/deployment.yaml b/config/samples/deployment.yaml index f3e62cd..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: t4 + 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 @@ -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/docs/architecture.md b/docs/architecture.md index fe43a26..d4aa234 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) @@ -236,6 +238,10 @@ Important details: - 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). + --- ## Components @@ -330,8 +336,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 +498,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..5a25b40 --- /dev/null +++ b/docs/status.md @@ -0,0 +1,246 @@ +# 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; 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: + +- `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..39eac4e 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(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(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(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(id string) { + ctx, cancel := context.WithTimeout(context.Background(), 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/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: