diff --git a/api/v1alpha1/groupversion_info.go b/api/v1alpha1/groupversion_info.go index 7f9ded8..0f93c10 100644 --- a/api/v1alpha1/groupversion_info.go +++ b/api/v1alpha1/groupversion_info.go @@ -169,10 +169,13 @@ const ( // 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: capacity has not been allocated yet. Stamped by CreatePod + // before it calls Provision, and HELD if Provision returns an id without reserving + // capacity — a Modal sandbox the control plane accepted but that is still queued + // for a GPU. So the instance may exist (and then must be reclaimed) even under this + // reason; what has not happened is the allocation. Replaced by Initializing as soon + // as capacity is committed: at once for a provider that allocates synchronously + // (AWS), otherwise when 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 @@ -181,8 +184,10 @@ const ( // 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. + // instance exists. The virtual kubelet stamps it only on EVIDENCE of existence: + // either the provider observed the instance in its List, or Provision reported it + // reserved (capacity committed, not merely requested). That is what makes it + // trustworthy for the claim to key Bound off. PodReasonInitializing = "Initializing" // PodReasonRunning: the provider reports the instance running. PodReasonRunning = "Running" diff --git a/config/samples/deployment.yaml b/config/samples/deployment.yaml index b1353a3..8293eba 100644 --- a/config/samples/deployment.yaml +++ b/config/samples/deployment.yaml @@ -38,7 +38,7 @@ spec: app: gpu-workload-sample nebula.inftyai.com/enabled: "true" nebula.inftyai.com/nodepool: sample - nebula.inftyai.com/accelerator-type: a100-80gb + nebula.inftyai.com/accelerator-type: a100-40gb 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/config/samples/nodepool.yaml b/config/samples/nodepool.yaml index 8503656..8295ac8 100644 --- a/config/samples/nodepool.yaml +++ b/config/samples/nodepool.yaml @@ -17,7 +17,7 @@ spec: - eu-west-1 - ca-central-1 - sa-east-1 - - name: modal + # - name: modal # - 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 5fbdd3c..d4aa234 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -476,10 +476,6 @@ status: When `capacityTypes` is omitted, the API defaults it to `{OnDemand, Spot}`. The listed order is the fallback order the placement controller walks. -A pool must list between one and eight providers. The upper bound keeps the -provider-by-region candidate expansion bounded while performance at larger -provider counts remains unproven. - ### NodeClaim (`nc`) ```yaml diff --git a/docs/status.md b/docs/status.md index 5a25b40..9599838 100644 --- a/docs/status.md +++ b/docs/status.md @@ -38,7 +38,9 @@ teardown. | Pod phase | reason | writer | instance exists? | claim phase | |---|---|---|---|---| -| `Pending` | `Provisioning` | `CreatePod` | no | `Provisioning` (grace applies) | +| `Pending` | `Provisioning` | `CreatePod`, before `Provision` | no | `Provisioning` (grace applies) | +| `Pending` | `Provisioning` | held, after an UNRESERVED `Provision` | yes, unreserved | `Provisioning` (grace applies) | +| `Pending` | `Initializing` | `CreatePod`, after a RESERVED `Provision` | yes | `Bound` | | `Pending` | `Initializing` | `applyState` ← `InstancePending` | yes | `Bound` | | `Running` | `Running` | `applyState` ← `InstanceRunning` | yes | `Bound` | | `Failed` | `ProvisionFailed` | `CreatePod` | no | `Terminated` (via `isTerminal`) | @@ -52,20 +54,34 @@ 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). +Note the two writers. `CreatePod` writes the rows around the `Provision` call; +every other non-terminal row comes from `applyState`, driven by the poll loop, and +is therefore only reachable for an instance the provider returned from `List()`. + +`Provisioning` is written *before* `Provision` is called. That call can run for +minutes (AWS sweeps a region's zones on a capacity error, bounded at 2), and until +it returns this is the only explanation the Pod carries. + +Emitting it early is safe; *tracking* it early is not. 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), so a tick landing mid- +provision would tear down the instance the call is about to return. `CreatePod` +therefore emits without storing, and stores only once `Provision` returns. + +`Provision` returns `(id, reserved, error)`. `reserved` means the provider committed +capacity, not merely accepted the request: AWS always does (`CreateFleet` with +`FleetTypeInstant` is synchronous), a fresh Modal sandbox never does (the GPU may +still be queued). Only a reserved instance advances to `Initializing`; an unreserved +id holds at `Provisioning`, which is still exactly true — the id is real and must be +reclaimed, but nothing is allocated. Either way the Pod is now tracked: `reserved` +constrains what the status may claim, not what is owed, and says nothing about +readiness — that is `applyState`'s job one tick later. + +That hold is short-lived by design. One poll tick later the same queued sandbox +reads `Initializing`, because a provider that queues cannot tell queued from booting +(see below). For that window the claim is `Provisioning` rather than `Bound`, so an +orphan waits the grace period instead of being reclaimed at once — teardown still +resolves the instance from `List()`, so nothing leaks. Important details: @@ -78,9 +94,11 @@ Important details: "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. +- Only `Provisioning` fails to earn the guard. If the Pod is absent then, the + controller waits `placementGracePeriod` (15 seconds) before deleting an orphaned + claim. An unreserved instance reads `Provisioning` even though it exists, so it + gets the grace window rather than immediate reclaim — safe either way, because + teardown resolves the instance from `List()`, not from the reason. - `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()`. @@ -133,7 +151,8 @@ folded in by `List` (`StatusChecksPassed`). 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`. + vs `running`-without-checks are both "booting", and both are `Bound`. This is why + `Provision` always returns `reserved` — the Pod can go straight to `Initializing`. - **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 @@ -191,14 +210,16 @@ only two signals and has to record a third fact itself. 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. + shape. It is `Bound` and billing throughout. So `Provision` returns + `reserved=false` for a fresh sandbox and the Pod holds at `Provisioning` until the + first poll tick. An *adopted* sandbox has been observed, so a `running` one is + known to be reserved. See below for why queued 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. +created, and so always reserves. It exists to exercise the placement and teardown +paths without a real backend, so it has no boot or readiness phase to model. --- @@ -207,10 +228,10 @@ backend, so it has no boot or readiness phase to model. 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: +starting up are both reported `Pending` / `Initializing`: the Pod reads +`Provisioning` until the first poll tick (≤15s), then `Initializing` for the rest of +both, so that one reason covers queueing and booting alike. Every public signal was +measured and none carries the boundary: | signal | distinguishes queued from booting? | |---|---| diff --git a/internal/controller/nodeclaim_controller.go b/internal/controller/nodeclaim_controller.go index 2e12cad..5eebee3 100644 --- a/internal/controller/nodeclaim_controller.go +++ b/internal/controller/nodeclaim_controller.go @@ -293,12 +293,15 @@ func (r *NodeClaimReconciler) provider(name string) (provider.Provider, bool) { // - 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. +// booting — vnode stamps that reason only on evidence of existence, either an +// instance observed in the provider's List or a reserved Provision). 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. No capacity has been allocated (the Provision call +// may still be in flight, or it returned an unreserved id — a Modal sandbox still +// queued for a GPU), so a vanished Pod may be cache lag and the grace window +// applies. An unreserved instance does exist, so this understates it for one poll +// tick; the grace path still reclaims by asking the provider what exists. func (r *NodeClaimReconciler) desiredPhase(nc *nebulav1alpha1.NodeClaim, pod *corev1.Pod) nebulav1alpha1.NodeClaimPhase { switch { case isTerminal(pod.Status.Phase): diff --git a/internal/controller/nodeclaim_controller_test.go b/internal/controller/nodeclaim_controller_test.go index 4326612..447349c 100644 --- a/internal/controller/nodeclaim_controller_test.go +++ b/internal/controller/nodeclaim_controller_test.go @@ -50,8 +50,8 @@ type fakeProvider struct { func (f *fakeProvider) Name() string { return f.name } func (f *fakeProvider) Capabilities() provider.Capabilities { return provider.Capabilities{} } -func (f *fakeProvider) Provision(context.Context, *corev1.Pod, provider.ProvisionRequest) (string, error) { - return "", nil +func (f *fakeProvider) Provision(context.Context, *corev1.Pod, provider.ProvisionRequest) (string, bool, error) { + return "", false, nil } func (f *fakeProvider) Terminate(_ context.Context, id string) error { f.terminated = append(f.terminated, id) diff --git a/pkg/provider/aws/aws.go b/pkg/provider/aws/aws.go index d2f9a14..8268d1f 100644 --- a/pkg/provider/aws/aws.go +++ b/pkg/provider/aws/aws.go @@ -382,42 +382,51 @@ func (p *Provider) Offerings(ctx context.Context) ([]provider.Offering, error) { // EC2 id. Terminate/Get do not need the region encoded in it — they locate the // instance by sweeping the swept regions (the same set List covers), since a // wrong-region lookup is a harmless no-op (see Terminate/Get). -func (p *Provider) Provision(ctx context.Context, pod *corev1.Pod, req provider.ProvisionRequest) (string, error) { +// Every id this returns is RESERVED. RunInstance launches through a CreateFleet +// *instant* request, which is synchronous: the response carries either an instance +// id — meaning EC2 already found capacity in some (type, AZ) cell and the instance +// is booting on real hardware — or the reason it could not launch, which becomes an +// error driving AZ/region/tier failover. There is no queued state for an EC2 +// instance to sit in, so the reserved return is unconditionally true on success. +func (p *Provider) Provision( + ctx context.Context, pod *corev1.Pod, req provider.ProvisionRequest, +) (string, bool, error) { if pod == nil { - return "", errors.New("aws: nil pod") + return "", false, errors.New("aws: nil pod") } if req.ClaimName == "" { - return "", errors.New("aws: empty ClaimName in ProvisionRequest") + return "", false, errors.New("aws: empty ClaimName in ProvisionRequest") } region := req.Region client, err := p.clientFor(ctx, region) if err != nil { - return "", err + return "", false, err } // Idempotency: if an instance already carries this claim tag IN THIS REGION, // return it rather than launching a second (guards a retry after a partial // create). A claim is placed in exactly one region per attempt, so scanning the - // target region's client is sufficient. + // target region's client is sufficient. It is reserved for the same reason a + // fresh launch is: it only exists because some earlier instant fleet succeeded. if existing, err := findByClaim(ctx, client, req.ClaimName); err != nil { - return "", err + return "", false, err } else if existing != nil { - return existing.ID, nil + return existing.ID, true, nil } spec, err := p.instanceSpecFromPod(pod, req) if err != nil { - return "", err + return "", false, err } // The Provision deadline is enforced generically by the vnode handler (from // Capabilities.ProvisionTimeout), so RunInstance simply honors ctx as it fails // over across zones — no adapter-local WithTimeout here. id, err := client.RunInstance(ctx, spec) if err != nil { - return "", err + return "", false, err } - return id, nil + return id, true, nil } // Terminate implements provider.Provider. Idempotent by the Client contract. The diff --git a/pkg/provider/aws/aws_test.go b/pkg/provider/aws/aws_test.go index 2083394..e4a9c8e 100644 --- a/pkg/provider/aws/aws_test.go +++ b/pkg/provider/aws/aws_test.go @@ -164,7 +164,7 @@ func TestProvision_MapsAcceleratorToInstanceType(t *testing.T) { f := &fakeClient{runID: "i-1"} p := newTestProvider(f) - id, err := p.Provision(context.Background(), gpuPod("H100", 8), provider.ProvisionRequest{ + id, reserved, err := p.Provision(context.Background(), gpuPod("H100", 8), provider.ProvisionRequest{ ClaimName: "claim-a", CapacityType: nebulav1alpha1.CapacityOnDemand, Region: "us-west-2", @@ -177,6 +177,11 @@ func TestProvision_MapsAcceleratorToInstanceType(t *testing.T) { if id != "i-1" { t.Fatalf("id = %q, want i-1", id) } + // An instant fleet is synchronous, so an id means EC2 already allocated capacity: + // AWS never hands back an instance that is still queued. + if !reserved { + t.Fatal("reserved = false; an instant fleet only returns an id once capacity is allocated") + } // AWS requests by instance type: H100 must resolve to its accelerator_id as the // primary (fleet) instance type. if got := primaryType(f.lastSpec); got != "p5.48xlarge" { @@ -211,7 +216,7 @@ func TestProvision_LowercaseAcceleratorLabel(t *testing.T) { // A user may write the accelerator-type label in any case; it must resolve to // the canonical catalog row (and thus the right instance type). - if _, err := p.Provision(context.Background(), gpuPod("h100", 8), provider.ProvisionRequest{ + if _, _, err := p.Provision(context.Background(), gpuPod("h100", 8), provider.ProvisionRequest{ ClaimName: "claim-lc", Region: testRegion, }); err != nil { @@ -246,7 +251,7 @@ func TestProvision_CountSelectsInstanceType(t *testing.T) { f := &fakeClient{runID: "i-t4"} p := newTestProvider(f) req := provider.ProvisionRequest{ClaimName: "claim-t4", Region: testRegion} - if _, err := p.Provision(context.Background(), gpuPod("T4", tc.count), req); err != nil { + if _, _, err := p.Provision(context.Background(), gpuPod("T4", tc.count), req); err != nil { t.Fatalf("Provision(T4 x%d): %v", tc.count, err) } if got := primaryType(f.lastSpec); got != tc.wantType { @@ -261,7 +266,7 @@ func TestProvision_UnsupportedCountIsError(t *testing.T) { // T4 x2 has no instance type (there is no 2-GPU T4 shape): must error rather // than silently picking the x1 or x8 row. req := provider.ProvisionRequest{ClaimName: "claim-t4x2", Region: testRegion} - if _, err := p.Provision(context.Background(), gpuPod("T4", 2), req); err == nil { + if _, _, err := p.Provision(context.Background(), gpuPod("T4", 2), req); err == nil { t.Fatal("expected an error for an unsupported (accelerator, count) pair") } if f.runCnt != 0 { @@ -273,7 +278,7 @@ func TestProvision_SpotSetsMarketOption(t *testing.T) { f := &fakeClient{runID: "i-spot"} p := newTestProvider(f) - if _, err := p.Provision(context.Background(), gpuPod("H100", 8), provider.ProvisionRequest{ + if _, _, err := p.Provision(context.Background(), gpuPod("H100", 8), provider.ProvisionRequest{ ClaimName: "claim-spot", CapacityType: nebulav1alpha1.CapacitySpot, Region: testRegion, @@ -293,7 +298,7 @@ func TestProvision_EmptyRegionIsError(t *testing.T) { // Provision errors rather than silently guessing. In production every request // carries a region (admission requires each aws pool to list ≥1; placement stamps // it), so this only guards a malformed request. - if _, err := p.Provision(context.Background(), gpuPod("H100", 8), provider.ProvisionRequest{ + if _, _, err := p.Provision(context.Background(), gpuPod("H100", 8), provider.ProvisionRequest{ ClaimName: "claim-def", }); err == nil { t.Fatal("expected an error for a request with no region") @@ -310,7 +315,7 @@ func TestProvision_NoAcceleratorIsError(t *testing.T) { // EC2 GPU provisioning is by instance type; a Pod with no accelerator has no // instance type to launch, so it must error rather than silently guessing. req := provider.ProvisionRequest{ClaimName: "claim-cpu", Region: testRegion} - if _, err := p.Provision(context.Background(), gpuPod("", 0), req); err == nil { + if _, _, err := p.Provision(context.Background(), gpuPod("", 0), req); err == nil { t.Fatal("expected an error for a Pod requesting no accelerator") } if f.runCnt != 0 { @@ -329,7 +334,7 @@ func TestProvision_Idempotent(t *testing.T) { } p := newTestProvider(f) - id, err := p.Provision(context.Background(), gpuPod("H100", 8), + id, _, err := p.Provision(context.Background(), gpuPod("H100", 8), provider.ProvisionRequest{ClaimName: "claim-a", Region: testRegion}) if err != nil { t.Fatalf("Provision: %v", err) @@ -347,7 +352,7 @@ func TestProvision_UnsupportedAccelerator(t *testing.T) { f := &fakeClient{} p := newTestProvider(f) req := provider.ProvisionRequest{ClaimName: "claim-x", Region: testRegion} - if _, err := p.Provision(context.Background(), gpuPod("TPU-v4", 1), req); err == nil { + if _, _, err := p.Provision(context.Background(), gpuPod("TPU-v4", 1), req); err == nil { t.Fatal("expected error for unsupported accelerator") } } diff --git a/pkg/provider/fake/fake.go b/pkg/provider/fake/fake.go index 36fbfec..a4e7f0d 100644 --- a/pkg/provider/fake/fake.go +++ b/pkg/provider/fake/fake.go @@ -84,12 +84,16 @@ func (p *Provider) Capabilities() provider.Capabilities { // Provision records one instance for the claim and reports it Running at once. // Idempotent on ClaimName: a repeat returns the existing instance's id rather // than creating a second (matching the real adapters' contract). -func (p *Provider) Provision(_ context.Context, pod *corev1.Pod, req provider.ProvisionRequest) (string, error) { +// +// It reports the instance RESERVED, which is honest for an in-memory backend: the +// instance is Running the moment it is recorded, so there is no queueing to model +// (and none of the capacity the reserved flag exists to describe). +func (p *Provider) Provision(_ context.Context, pod *corev1.Pod, req provider.ProvisionRequest) (string, bool, error) { if pod == nil { - return "", fmt.Errorf("fake: nil pod") + return "", false, fmt.Errorf("fake: nil pod") } if req.ClaimName == "" { - return "", fmt.Errorf("fake: empty ClaimName in ProvisionRequest") + return "", false, fmt.Errorf("fake: empty ClaimName in ProvisionRequest") } p.mu.Lock() @@ -97,7 +101,7 @@ func (p *Provider) Provision(_ context.Context, pod *corev1.Pod, req provider.Pr for _, inst := range p.instances { if inst.ClaimName == req.ClaimName { - return inst.ID, nil // idempotent reuse + return inst.ID, true, nil // idempotent reuse } } @@ -110,7 +114,7 @@ func (p *Provider) Provision(_ context.Context, pod *corev1.Pod, req provider.Pr Endpoint: fmt.Sprintf("fake://%s", id), CapacityType: req.CapacityType, } - return id, nil + return id, true, nil } // Terminate forgets the instance. Idempotent: terminating an already-gone (or diff --git a/pkg/provider/fake/fake_test.go b/pkg/provider/fake/fake_test.go index 9981515..2c7312c 100644 --- a/pkg/provider/fake/fake_test.go +++ b/pkg/provider/fake/fake_test.go @@ -38,7 +38,7 @@ func TestProvisionReportsRunningAndLists(t *testing.T) { p := New() ctx := context.Background() - id, err := p.Provision(ctx, testPod(), provider.ProvisionRequest{ + id, reserved, err := p.Provision(ctx, testPod(), provider.ProvisionRequest{ ClaimName: "claim-a", CapacityType: nebulav1alpha1.CapacityOnDemand, }) @@ -48,6 +48,12 @@ func TestProvisionReportsRunningAndLists(t *testing.T) { if id == "" { t.Fatal("expected a non-empty instance id") } + // The fake has no queueing to model — an instance is Running the moment it is + // created — so it always reserves. Reporting false would make the fake exercise + // the Modal-shaped path and leave Pods at Provisioning forever. + if !reserved { + t.Fatal("reserved = false; the fake allocates synchronously and is Running immediately") + } // Get reports it Running with the claim recovered. inst, err := p.Get(ctx, id) @@ -76,11 +82,11 @@ func TestProvisionIdempotentOnClaim(t *testing.T) { ctx := context.Background() req := provider.ProvisionRequest{ClaimName: "claim-a"} - id1, err := p.Provision(ctx, testPod(), req) + id1, _, err := p.Provision(ctx, testPod(), req) if err != nil { t.Fatalf("Provision #1: %v", err) } - id2, err := p.Provision(ctx, testPod(), req) + id2, _, err := p.Provision(ctx, testPod(), req) if err != nil { t.Fatalf("Provision #2: %v", err) } @@ -96,7 +102,7 @@ func TestTerminateIsIdempotent(t *testing.T) { p := New() ctx := context.Background() - id, err := p.Provision(ctx, testPod(), provider.ProvisionRequest{ClaimName: "claim-a"}) + id, _, err := p.Provision(ctx, testPod(), provider.ProvisionRequest{ClaimName: "claim-a"}) if err != nil { t.Fatalf("Provision: %v", err) } diff --git a/pkg/provider/modal/client.go b/pkg/provider/modal/client.go index 39eac4e..9f755b7 100644 --- a/pkg/provider/modal/client.go +++ b/pkg/provider/modal/client.go @@ -264,7 +264,10 @@ func (c *sdkClient) GetSandbox(ctx context.Context, id string) (*Sandbox, error) } return nil, err } - out := c.observe(ctx, sb) + out, err := c.observe(ctx, sb) + if err != nil { + return nil, err + } return &out, nil } @@ -290,28 +293,48 @@ func (c *sdkClient) ListSandboxes(ctx context.Context) ([]Sandbox, error) { if err != nil { return nil, err } - out = append(out, c.observe(ctx, sb)) + observed, err := c.observe(ctx, sb) + if err != nil { + return nil, err + } + out = append(out, observed) } return out, nil } // observe normalizes a live SDK *Sandbox into the adapter-level Sandbox view: // status (from Poll), tags (from GetTags), and a best-effort endpoint (from -// 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. +// Tunnels). Poll and tunnel errors are tolerated so a single flaky sandbox doesn't +// fail the whole read — the poll loop will re-observe next tick. A TAG error is +// not: see below. // // 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 { +func (c *sdkClient) observe(ctx context.Context, sb *modal.Sandbox) (Sandbox, error) { out := Sandbox{ID: sb.SandboxID} // Tags carry Nebula identity (ClaimTagKey), recovered by toInstance, and - // probe-ness (ProbeTagKey), read by isReady below — so this must precede the + // probe-ness (ProbeTagKey), read by observeReady below — so this must precede the // status block. - if tags, err := sb.GetTags(ctx, &modal.SandboxGetTagsParams{}); err == nil { - out.Tags = tags + // + // A read failure is NOT "no tags", and it cannot be tolerated like the others: + // without ClaimTagKey the sandbox has no recoverable identity, and every way of + // reporting it anyway is wrong. An empty ClaimName matches no claim, and dropping + // the sandbox is indistinguishable from that — the poll loop reads both as "this + // claim's instance is gone" and reports the Pod Terminated, which is terminal-sticky + // and makes the NodeClaim reclaim a live sandbox. A missing ProbeTagKey would also + // make observeReady report a still-booting sandbox ready. + // + // So the error propagates and fails the whole read. Callers already treat that as + // "do not act on a half-known fleet": the poll loop leaves statuses untouched and + // retries next tick, re-adoption falls through to NotFound, and the teardown + // backstop requeues. A stalled tick is recoverable; a stuck terminal phase is not. + tags, err := sb.GetTags(ctx, &modal.SandboxGetTagsParams{}) + if err != nil { + return out, fmt.Errorf("modal: read tags for sandbox %s: %w", sb.SandboxID, err) } + out.Tags = tags // 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 @@ -344,7 +367,7 @@ func (c *sdkClient) observe(ctx context.Context, sb *modal.Sandbox) Sandbox { } cancel() } - return out + return out, nil } // Exit codes Modal substitutes for a non-exit outcome, since Poll conforms to the diff --git a/pkg/provider/modal/modal.go b/pkg/provider/modal/modal.go index b8a4f03..8126581 100644 --- a/pkg/provider/modal/modal.go +++ b/pkg/provider/modal/modal.go @@ -188,27 +188,49 @@ func (p *Provider) Capabilities() provider.Capabilities { // Provision implements provider.Provider. The Pod is the source of truth for // the workload; req carries only the claim identity and capacity tier. -func (p *Provider) Provision(ctx context.Context, pod *corev1.Pod, req provider.ProvisionRequest) (string, error) { +// +// A fresh sandbox is NEVER reserved. Sandboxes.Create returns as soon as Modal's +// control plane accepts the sandbox: the id is real (listable, terminable, and +// carrying the full teardown obligation), but the GPU may still be queued — +// potentially for minutes on a large shape. So the id and the capacity are two +// separate facts here, unlike AWS, and reporting reserved=false is what lets the +// Pod stay at the provisioning reason instead of claiming to be initializing. +// +// The queued→running transition is then observed the same way readiness is, through +// the poll loop's List: the sandbox reads statusInitializing until it is live. +func (p *Provider) Provision( + ctx context.Context, pod *corev1.Pod, req provider.ProvisionRequest, +) (string, bool, error) { if pod == nil { - return "", errors.New("modal: nil pod") + return "", false, errors.New("modal: nil pod") } if req.ClaimName == "" { - return "", errors.New("modal: empty ClaimName in ProvisionRequest") + return "", false, errors.New("modal: empty ClaimName in ProvisionRequest") } // Idempotency: if a sandbox already carries this claim tag, return it rather // than creating a second (guards against a retry after a partial create). + // + // Unlike a fresh create, this one has been OBSERVED, so its state is known: a + // sandbox the poll loop reports live (Pending once it is up, or Running once + // ready) has necessarily been allocated capacity, whereas one still queued is + // not yet reserved. That is strictly more information than a create can return, + // so use it rather than flatly reporting false. if existing, err := p.findByClaim(ctx, req.ClaimName); err != nil { - return "", err + return "", false, err } else if existing != nil { - return existing.ID, nil + return existing.ID, existing.State == provider.InstanceRunning, nil } spec, err := p.sandboxSpecFromPod(pod, req) if err != nil { - return "", err + return "", false, err + } + id, err := p.client.CreateSandbox(ctx, spec) + if err != nil { + return "", false, err } - return p.client.CreateSandbox(ctx, spec) + return id, false, nil } // Terminate implements provider.Provider. Idempotent by the Client contract. diff --git a/pkg/provider/modal/modal_test.go b/pkg/provider/modal/modal_test.go index 0966aad..3fbd899 100644 --- a/pkg/provider/modal/modal_test.go +++ b/pkg/provider/modal/modal_test.go @@ -118,7 +118,7 @@ func TestProvision_GPUPod(t *testing.T) { f := &fakeClient{createID: "sb-1"} p := newTestProvider(f) - id, err := p.Provision(context.Background(), gpuPod("claim-a", "H100", 2), provider.ProvisionRequest{ + id, reserved, err := p.Provision(context.Background(), gpuPod("claim-a", "H100", 2), provider.ProvisionRequest{ ClaimName: "claim-a", CapacityType: nebulav1alpha1.CapacityOnDemand, }) @@ -128,6 +128,12 @@ func TestProvision_GPUPod(t *testing.T) { if id != "sb-1" { t.Fatalf("id = %q, want sb-1", id) } + // Create only means the control plane ACCEPTED the sandbox — the GPU may still be + // queued — so a fresh sandbox is never reserved. Claiming otherwise would let the + // Pod report Initializing while nothing has been allocated. + if reserved { + t.Fatal("reserved = true; a freshly created sandbox may still be queued for capacity") + } if f.lastSpec.GPU != "H100" || f.lastSpec.GPUCount != 2 { t.Fatalf("spec GPU=%q count=%d, want H100/2", f.lastSpec.GPU, f.lastSpec.GPUCount) } @@ -149,7 +155,7 @@ func TestProvision_LowercaseGPUAnnotation(t *testing.T) { // A user may write the accelerator-type label in any case (e.g. "h100"). It must // resolve to the canonical catalog accelerator ("H100") so the provisioned // sandbox — and any downstream key (blocklist/catalog) — uses one casing. - _, err := p.Provision(context.Background(), gpuPod("claim-lc", "h100", 1), provider.ProvisionRequest{ + _, _, err := p.Provision(context.Background(), gpuPod("claim-lc", "h100", 1), provider.ProvisionRequest{ ClaimName: "claim-lc", CapacityType: nebulav1alpha1.CapacityOnDemand, }) @@ -177,7 +183,7 @@ func TestProvision_MapsResourcesPortsAndTimeout(t *testing.T) { deadline := int64(3600) pod.Spec.ActiveDeadlineSeconds = &deadline - if _, err := p.Provision(context.Background(), pod, provider.ProvisionRequest{ClaimName: "claim-res"}); err != nil { + if _, _, err := p.Provision(context.Background(), pod, provider.ProvisionRequest{ClaimName: "claim-res"}); err != nil { t.Fatalf("Provision: %v", err) } if f.lastSpec.CPU != 2.5 { @@ -201,7 +207,7 @@ func TestProvision_DefaultsTimeoutWhenNoDeadline(t *testing.T) { // No activeDeadlineSeconds: the adapter must still set a non-zero timeout, else // Modal applies its 5-minute default and the workload dies almost immediately. req := provider.ProvisionRequest{ClaimName: "claim-dt"} - if _, err := p.Provision(context.Background(), gpuPod("claim-dt", "H100", 1), req); err != nil { + if _, _, err := p.Provision(context.Background(), gpuPod("claim-dt", "H100", 1), req); err != nil { t.Fatalf("Provision: %v", err) } if f.lastSpec.Timeout != defaultSandboxTimeout { @@ -213,7 +219,7 @@ func TestProvision_CPUOnly(t *testing.T) { f := &fakeClient{} p := newTestProvider(f) - _, err := p.Provision(context.Background(), gpuPod("claim-cpu", "", 0), provider.ProvisionRequest{ + _, _, err := p.Provision(context.Background(), gpuPod("claim-cpu", "", 0), provider.ProvisionRequest{ ClaimName: "claim-cpu", CapacityType: nebulav1alpha1.CapacityOnDemand, }) @@ -236,7 +242,7 @@ func TestProvision_Idempotent(t *testing.T) { p := newTestProvider(f) req := provider.ProvisionRequest{ClaimName: "claim-a"} - id, err := p.Provision(context.Background(), gpuPod("claim-a", "H100", 1), req) + id, reserved, err := p.Provision(context.Background(), gpuPod("claim-a", "H100", 1), req) if err != nil { t.Fatalf("Provision: %v", err) } @@ -246,13 +252,44 @@ func TestProvision_Idempotent(t *testing.T) { if f.createCnt != 0 { t.Fatalf("CreateSandbox called %d times, want 0 (idempotent)", f.createCnt) } + // An adopted sandbox has been OBSERVED, unlike a fresh create, so its state is + // known: this one is running, which means capacity was necessarily allocated. + if !reserved { + t.Fatal("reserved = false for an adopted RUNNING sandbox; observed state proves capacity was allocated") + } +} + +// A sandbox adopted while still coming up is NOT reserved: "initializing" is what +// Modal reports for both a queued sandbox and a booting one, so it does not prove +// capacity was allocated. Only a running sandbox does. +func TestProvision_IdempotentInitializingIsNotReserved(t *testing.T) { + f := &fakeClient{ + sandboxes: []Sandbox{{ + ID: "sb-existing", + Tags: map[string]string{ClaimTagKey: "claim-a"}, + Status: statusInitializing, + }}, + } + p := newTestProvider(f) + + id, reserved, err := p.Provision(context.Background(), gpuPod("claim-a", "H100", 1), + provider.ProvisionRequest{ClaimName: "claim-a"}) + if err != nil { + t.Fatalf("Provision: %v", err) + } + if id != "sb-existing" { + t.Fatalf("id = %q, want sb-existing (idempotent reuse)", id) + } + if reserved { + t.Fatal("reserved = true for an adopted INITIALIZING sandbox; it may still be queued") + } } func TestProvision_UnsupportedAccelerator(t *testing.T) { f := &fakeClient{} p := newTestProvider(f) req := provider.ProvisionRequest{ClaimName: "claim-x"} - _, err := p.Provision(context.Background(), gpuPod("claim-x", "TPU-v4", 1), req) + _, _, err := p.Provision(context.Background(), gpuPod("claim-x", "TPU-v4", 1), req) if err == nil { t.Fatal("expected error for unsupported accelerator") } @@ -413,7 +450,7 @@ func TestProvision_ProbeTagStampedOnlyWithProbe(t *testing.T) { 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 { + if _, _, err := p.Provision(context.Background(), pod, provider.ProvisionRequest{ClaimName: "claim-a"}); err != nil { t.Fatalf("Provision: %v", err) } _, present := f.lastSpec.Tags[ProbeTagKey] @@ -443,7 +480,7 @@ func TestProvision_ReadinessProbeCarriedThrough(t *testing.T) { } pod.Spec.Containers[0].ReadinessProbe = probe - if _, err := p.Provision(context.Background(), pod, provider.ProvisionRequest{ClaimName: "claim-a"}); err != nil { + if _, _, err := p.Provision(context.Background(), pod, provider.ProvisionRequest{ClaimName: "claim-a"}); err != nil { t.Fatalf("Provision: %v", err) } // The probe is carried onto the spec so the Client can configure Modal's own @@ -458,7 +495,7 @@ func TestProvision_NoProbeLeavesSpecUnset(t *testing.T) { p := newTestProvider(f) req := provider.ProvisionRequest{ClaimName: "claim-a"} - if _, err := p.Provision(context.Background(), gpuPod("claim-a", "H100", 1), req); err != nil { + if _, _, err := p.Provision(context.Background(), gpuPod("claim-a", "H100", 1), req); err != nil { t.Fatalf("Provision: %v", err) } if f.lastSpec.ReadinessProbe != nil { diff --git a/pkg/provider/provider.go b/pkg/provider/provider.go index 093f3cb..0b5a207 100644 --- a/pkg/provider/provider.go +++ b/pkg/provider/provider.go @@ -54,11 +54,32 @@ type Provider interface { // nvidia.com/gpu resource (via util.AcceleratorRequest; the type is then // translated via MapAccelerator). The request carries // only what the Pod cannot express: the optimizer-chosen capacity tier and - // the claim identity. It returns the provider instance id on success. + // the claim identity. + // + // It returns the provider instance id plus whether that instance is RESERVED: + // whether the provider has committed actual capacity to it, as opposed to + // merely accepting the request. The two are genuinely different guarantees and + // an id alone cannot express either one: + // + // - AWS reserves. CreateFleet with an *instant* request is synchronous, so an + // id means EC2 found capacity and the instance is booting on real hardware; + // a shortfall is an error, never a pending instance. + // - Modal does not. Sandboxes.Create returns as soon as the control plane + // accepts the sandbox; the GPU may still be queued, for minutes on a large + // shape. + // + // Callers use it to report honest status: an unreserved instance is not yet + // initializing, so its Pod stays at the provisioning reason. Note that reserved + // says nothing about readiness — a reserved instance is still booting, and + // readiness is observed later through List/Get. An id with reserved=false still + // carries the full teardown obligation, since the instance exists as far as the + // provider is concerned; a provider that cannot distinguish the two should + // return true, matching the pre-existing assumption. + // // Idempotency: if an instance already exists for req.ClaimName (encoded in // the provider's naming scheme, since most providers lack tags), return that // id instead of creating a second. - Provision(ctx context.Context, pod *corev1.Pod, req ProvisionRequest) (instanceID string, err error) + Provision(ctx context.Context, pod *corev1.Pod, req ProvisionRequest) (instanceID string, reserved bool, err error) // Terminate destroys the instance by id. Must be idempotent: terminating an // already-gone instance returns nil (so the NodeClaim finalizer can retry diff --git a/pkg/vnode/handler.go b/pkg/vnode/handler.go index b309837..132dfe8 100644 --- a/pkg/vnode/handler.go +++ b/pkg/vnode/handler.go @@ -116,9 +116,18 @@ type Handler struct { // blocklist-less wiring simple. blocklist Blocklister - mu sync.Mutex - tracked map[string]*trackedPod // key: namespace/name - notify func(*corev1.Pod) + mu sync.Mutex + + // tracked is the poll loop's work list and what GetPod/GetPodStatus serve, keyed + // by namespace/name. + // + // INVARIANT: only pods whose instance the provider has acknowledged (Provision + // returned an id) or that are already terminal. Never one whose Provision call is + // still in flight — reconcileOnce maps a tracked pod absent from List() to + // Terminated, which for a live provision is a wrong, unrecoverable write. + tracked map[string]*trackedPod + + notify func(*corev1.Pod) // nowFn and pollEvery are seams for tests. nowFn func() metav1.Time @@ -203,7 +212,14 @@ 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()) - id, err := h.prov.Provision(ctx, pod, req) + + // Report Provisioning BEFORE the call. It can run for minutes (AWS sweeps the + // region's zones on a capacity error), and until it returns this is the only + // explanation the Pod carries. Emit but do NOT store — see the tracked invariant. + h.markStatus(pod, corev1.PodPending, reasonProvisioning, "allocating external instance") + h.emit(pod) + + id, reserved, err := h.prov.Provision(ctx, pod, req) if err != nil { log.Error(err, "provision failed; Pod marked Failed for failover") // Record the failure on the shared blocklist so placement fails over to the @@ -221,9 +237,20 @@ func (h *Handler) CreatePod(ctx context.Context, pod *corev1.Pod) error { return err } - // Record the instance id before reporting success; teardown relies on it. - log.Info("external instance provisioned", "instanceID", id) - h.markStatus(pod, corev1.PodPending, reasonProvisioning, "provisioning external instance") + // Only a RESERVED instance advances: capacity is committed and it is booting. An + // unreserved id (a Modal sandbox accepted but still queued for a GPU) is left at + // the Provisioning stamped above, which is exactly true — the id is real and must + // be reclaimed, but nothing is allocated yet. One poll tick later the queued + // sandbox reads Initializing too, since Modal cannot tell queued from booting (see + // docs/status.md). + // + // store runs either way: an id means the instance exists. markStatus precedes it + // because store deep-copies — storing first would track a copy without the status + // just written. + log.Info("external instance provisioned", "instanceID", id, "reserved", reserved) + if reserved { + 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..15615a1 100644 --- a/pkg/vnode/handler_test.go +++ b/pkg/vnode/handler_test.go @@ -19,6 +19,8 @@ package vnode import ( "context" "errors" + "slices" + "strings" "sync" "testing" "time" @@ -37,17 +39,22 @@ import ( // fakeProvider records lifecycle calls and returns canned results so each // Handler branch can be driven deterministically. type fakeProvider struct { - mu sync.Mutex - provisionID string - provisionErr error - provisionCnt int - lastReq provider.ProvisionRequest - terminateCnt int - terminateID string - terminateErr error - list []provider.Instance - listErr error - capabilities provider.Capabilities + mu sync.Mutex + provisionID string + // provisionReserved is the reserved return: whether the provider committed + // capacity, not merely accepted the request. It defaults to FALSE (the Modal-like + // case), so a test that cares about the reserved path must opt in — the zero value + // should not silently assert the stronger guarantee. + provisionReserved bool + provisionErr error + provisionCnt int + lastReq provider.ProvisionRequest + terminateCnt int + terminateID string + terminateErr error + list []provider.Instance + listErr error + capabilities provider.Capabilities // classifyScope is what ClassifyProvisionError returns for a failure; the zero // value (empty scope) means "not blocklistable". classifyAccel/classifyRegion // record what the handler passed in, so a test can assert it resolved them off the @@ -55,17 +62,27 @@ type fakeProvider struct { classifyScope provider.BlockScope classifyAccel string classifyRegion string + // provisionHook runs inside Provision, before it returns, so a test can observe + // what the handler published for the window in which the 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) { +func (f *fakeProvider) Provision( + _ context.Context, _ *corev1.Pod, req provider.ProvisionRequest, +) (string, bool, 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++ f.lastReq = req - return f.provisionID, f.provisionErr + return f.provisionID, f.provisionReserved, f.provisionErr } func (f *fakeProvider) Terminate(_ context.Context, id string) error { @@ -139,6 +156,145 @@ func TestCreatePod_ProvisionsAndTracks(t *testing.T) { } } +func TestCreatePod_ReservedAdvancesToInitializing(t *testing.T) { + // reserved=true (AWS: an instant fleet only returns an id once capacity is + // allocated) means the instance is committed and booting, so the Pod may leave + // Provisioning immediately instead of waiting a whole poll tick for the same news. + fp := &fakeProvider{provisionID: "inst-1", provisionReserved: true} + h := NewHandler(fp, nil, nil) + pod := testPod("default", "p1") + + if err := h.CreatePod(context.Background(), pod); err != nil { + t.Fatalf("CreatePod: %v", err) + } + if pod.Status.Reason != reasonInitializing { + t.Fatalf("reason = %q, want %q for a reserved instance", pod.Status.Reason, reasonInitializing) + } + if pod.Status.Phase != corev1.PodPending { + t.Fatalf("phase = %q, want Pending (Initializing is a Pending reason)", pod.Status.Phase) + } + // The tracked copy must carry the advanced status: store deep-copies, so a + // markStatus after it would be invisible to GetPod and to the poll loop. + got, err := h.GetPod(context.Background(), "default", "p1") + if err != nil { + t.Fatalf("GetPod: %v", err) + } + if got.Status.Reason != reasonInitializing { + t.Fatalf("tracked reason = %q, want %q (markStatus must precede store)", got.Status.Reason, reasonInitializing) + } +} + +func TestCreatePod_UnreservedStaysProvisioning(t *testing.T) { + // reserved=false (Modal: Create returns on control-plane acceptance, the GPU may + // still be queued) means nothing has been allocated yet, so Provisioning is still + // exactly true. Advancing to Initializing here would claim a commitment the + // provider has not made. + fp := &fakeProvider{provisionID: "sb-1", provisionReserved: false} + h := NewHandler(fp, nil, nil) + pod := testPod("default", "p1") + + if err := h.CreatePod(context.Background(), pod); err != nil { + t.Fatalf("CreatePod: %v", err) + } + if pod.Status.Reason != reasonProvisioning { + t.Fatalf("reason = %q, want %q for an unreserved instance", pod.Status.Reason, reasonProvisioning) + } + // It is still TRACKED: an id was returned, so the instance exists and carries the + // full teardown obligation regardless of whether capacity was committed. + got, err := h.GetPod(context.Background(), "default", "p1") + if err != nil { + t.Fatalf("an unreserved instance must still be tracked: %v", err) + } + if got.Status.Reason != reasonProvisioning { + t.Fatalf("tracked reason = %q, want %q", got.Status.Reason, reasonProvisioning) + } +} + +func TestCreatePod_EmitsProvisioningWhileProvisionInFlight(t *testing.T) { + // Provision can run for minutes (AWS sweeps a region's zones on a capacity error), + // and until it returns Provisioning is the only explanation the Pod carries — so it + // must be emitted BEFORE the call, not after, where it would describe a window that + // has already closed. + // + // Emitting an untracked Pod is what makes this safe: the tracked invariant forbids + // storing one mid-provision, not reporting one. + fp := &fakeProvider{provisionID: "inst-1", provisionReserved: true} + h := NewHandler(fp, nil, nil) + + var mu sync.Mutex + var reasons []string + h.NotifyPods(context.Background(), func(p *corev1.Pod) { + mu.Lock() + reasons = append(reasons, p.Status.Reason) + mu.Unlock() + }) + + var inFlight []string + fp.provisionHook = func() { + mu.Lock() + inFlight = slices.Clone(reasons) + mu.Unlock() + } + + if err := h.CreatePod(context.Background(), testPod("default", "p1")); err != nil { + t.Fatalf("CreatePod: %v", err) + } + + mu.Lock() + defer mu.Unlock() + if !slices.Equal(inFlight, []string{reasonProvisioning}) { + t.Fatalf("emitted %v while Provision was in flight, want exactly [%s]", inFlight, reasonProvisioning) + } + if !slices.Equal(reasons, []string{reasonProvisioning, reasonInitializing}) { + t.Fatalf("emitted %v, want [%s %s]", reasons, reasonProvisioning, reasonInitializing) + } +} + +func TestCreatePod_PollTickDuringProvisionEmitsNoTerminalStatus(t *testing.T) { + // The tracked invariant, asserted end to end. reconcileOnce maps a tracked pod + // that is absent from List() to Terminated, so tracking a pod whose Provision is + // still in flight reports Failed/Terminated over a SUCCEEDING provision. + // + // The assertion is on what was EMITTED, not on the final tracked status: the store + // after a successful Provision overwrites the tracked entry, so the end state looks + // correct either way. The damage is the emit — VK writes it to the API server, where + // Pod phases are terminal-sticky, and the NodeClaim then reclaims a live instance. + fp := &fakeProvider{provisionID: "inst-1", provisionReserved: true} + 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 has nothing to list 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 strings.HasPrefix(s, string(corev1.PodFailed)) || strings.HasPrefix(s, string(corev1.PodSucceeded)) { + t.Fatalf("emitted %v; a tick during a succeeding Provision must not report a terminal phase", emitted) + } + } + // Exactly CreatePod's own two emits: the mid-provision tick had nothing to report + // because the pod was not yet tracked. + want := []string{ + string(corev1.PodPending) + "/" + reasonProvisioning, + string(corev1.PodPending) + "/" + reasonInitializing, + } + if !slices.Equal(emitted, want) { + t.Fatalf("emitted %v, want %v", emitted, want) + } +} + func TestCreatePod_ProvisionErrorSurfaces(t *testing.T) { fp := &fakeProvider{provisionErr: errors.New("no capacity")} h := NewHandler(fp, nil, nil) @@ -483,6 +639,45 @@ func TestReconcileOnce_AbsentInstanceIsTerminated(t *testing.T) { } } +func TestReconcileOnce_ListErrorLeavesStatusUntouched(t *testing.T) { + // A List error must not advance anything. It means the fleet is half-known, and + // the only unsafe reading is "absent" — which maps to Terminated, a terminal + // phase the Pod can never leave. + // + // This is load-bearing beyond transport failures: the Modal adapter deliberately + // FAILS List when a sandbox's tags are unreadable, because a sandbox with no + // recoverable ClaimName cannot be reported at all (an empty claim and an omitted + // sandbox both read as absent here). That choice is only safe because of this. + fp := &fakeProvider{provisionID: "inst-1", provisionReserved: true} + h := NewHandler(fp, nil, nil) + _ = h.CreatePod(context.Background(), testPod("default", "p1")) + + var mu sync.Mutex + var emitted int + h.NotifyPods(context.Background(), func(*corev1.Pod) { + mu.Lock() + emitted++ + mu.Unlock() + }) + + fp.list, fp.listErr = nil, errors.New("tags unreadable") + h.reconcileOnce(context.Background()) + + got, err := h.GetPod(context.Background(), "default", "p1") + if err != nil { + t.Fatalf("GetPod: %v", err) + } + if got.Status.Phase != corev1.PodPending || got.Status.Reason != reasonInitializing { + t.Fatalf("status = %s/%s after a failed List, want the pre-tick Pending/%s", + got.Status.Phase, got.Status.Reason, reasonInitializing) + } + mu.Lock() + defer mu.Unlock() + if emitted != 0 { + t.Fatalf("emitted %d notifications on a failed List, want 0", emitted) + } +} + func TestNewHandler_PollIntervalFromCapabilities(t *testing.T) { // A provider that declares a cadence overrides the default. custom := &fakeProvider{capabilities: provider.Capabilities{PollInterval: 5 * time.Second}}