cleanup: remove placement regions

api/v1alpha1/groupversion_info.go

@@ -99,19 +99,6 @@ const (
 	// via util.AcceleratorRequest.
 	AcceleratorTypeLabel = "nebula.inftyai.com/accelerator-type"
 
-	// CapacityTypeAnnotation carries the chosen purchase tier (Spot/OnDemand). It is a
-	// provisioning input the Pod spec cannot express, so the placement controller
-	// writes it when it ungates the Pod and the virtual kubelet — which provisions
-	// solely from the Pod — reads it back on CreatePod. Empty means "use the provider's
-	// default" (Modal is OnDemand-only and ignores it).
-	CapacityTypeAnnotation = "nebula.inftyai.com/capacity-type"
-
-	// RegionAnnotation carries the chosen provider region. Same flow as
-	// CapacityTypeAnnotation: stamped at ungate, read on CreatePod into
-	// ProvisionRequest.Region. Absent means the provider's default region;
-	// region-simple providers (Modal, RunPod) ignore it.
-	RegionAnnotation = "nebula.inftyai.com/region"
-
 	// EndpointAnnotation carries the reachable address of the external instance (a DNS
 	// name, an IP, or a URL, in the provider's own form). It is the only way to reach
 	// the workload, and PodIP cannot hold it — the API server validates PodIP as a

cmd/main.go

@@ -458,10 +458,10 @@ func setupVirtualNodes(mgr ctrl.Manager, blocklist vnode.Blocklister, kubeletSrv
 		if !ok {
 			continue
 		}
-		// The manager's client, so the pool read on the provisioning path hits the shared
-		// cache the controllers already keep warm.
-		pools := vnode.NewCachedPoolReader(mgr.GetClient())
-		if err := mgr.Add(vnode.NewRunner(prov, clientset, blocklist, kubeletSrv, pools)); err != nil {
+		// The manager's client, so the pool and claim reads on the provisioning path hit the
+		// shared cache the controllers already keep warm.
+		cluster := vnode.NewCachedClusterReader(mgr.GetClient())
+		if err := mgr.Add(vnode.NewRunner(prov, clientset, blocklist, kubeletSrv, cluster)); err != nil {
 			return err
 		}
 		setupLog.Info("registered virtual node", "provider", name, "node", vnode.NodeName(name))

internal/controller/nodeclaim_controller.go

@@ -212,7 +212,9 @@ func (r *NodeClaimReconciler) reconcileDelete(ctx context.Context, nc *nebulav1a
 	// already terminated this instance, so this is a redundant no-op; the call is
 	// only load-bearing when DeletePod never ran. Idempotency also makes retries
 	// after a transient error safe.
-	if err := prov.Terminate(ctx, id); err != nil {
+	// spec.Region is written before provisioning and never rewritten, so unlike the region
+	// VK holds in memory it is still readable in the case this backstop exists for.
+	if err := prov.Terminate(ctx, id, nc.Spec.Region); err != nil {
 		log.Error(err, "terminate failed; will retry", "instanceID", id)
 		return ctrl.Result{}, err
 	}

internal/controller/nodeclaim_controller_test.go

@@ -45,6 +45,7 @@ type fakeProvider struct {
 	list         []provider.Instance // what List returns
 	listErr      error               // if set, List fails
 	terminated   []string            // instance ids passed to Terminate, in order
+	regions      []string            // regions passed to Terminate, positionally paired with terminated
 	terminateErr error               // if set, Terminate fails
 	gpus         []string            // accelerators MapAccelerator offers; nil = offer any
 	spot         bool                // Capabilities().SupportsSpot (placement skips Spot without it)
@@ -60,8 +61,9 @@ func (f *fakeProvider) Capabilities() provider.Capabilities {
 func (f *fakeProvider) Provision(context.Context, *corev1.Pod, provider.ProvisionRequest) (provider.ProvisionResult, error) {
 	return provider.ProvisionResult{}, nil
 }
-func (f *fakeProvider) Terminate(_ context.Context, id string) error {
+func (f *fakeProvider) Terminate(_ context.Context, id, region string) error {
 	f.terminated = append(f.terminated, id)
+	f.regions = append(f.regions, region)
 	return f.terminateErr
 }
 func (f *fakeProvider) Get(context.Context, string) (*provider.Instance, error) { return nil, nil }
@@ -478,6 +480,25 @@ func TestReconcileDelete_UsesRecordedInstanceID(t *testing.T) {
 	}
 }
 
+// The backstop runs when VK never did, so the region VK held in memory is gone too. It
+// must pass spec.Region, written before provisioning and never rewritten — otherwise a
+// region-partitioned provider has to search for the instance and can conclude "already
+// gone" about one it never looked for (see provider.Terminate).
+func TestReconcileDelete_PassesTheClaimRegion(t *testing.T) {
+	claim := newClaim("c1", "p1", "default", "uid-1", "fake")
+	claim.Spec.Region = "eu-west-1"
+	claim.Status.InstanceID = "inst-1"
+	deleteClaim(t, claim)
+	prov := &fakeProvider{name: "fake"}
+	r, _ := newClaimReconciler(t, []client.Object{claim}, prov)
+
+	reconcileClaim(t, r, "c1")
+
+	if len(prov.regions) != 1 || prov.regions[0] != "eu-west-1" {
+		t.Fatalf("regions passed to Terminate = %v, want [eu-west-1]", prov.regions)
+	}
+}
+
 func TestReconcileDelete_NoInstanceIsIdempotentNoOp(t *testing.T) {
 	// The happy path: VK's DeletePod already terminated, so List finds nothing.
 	// Terminate is called with "" (idempotent no-op) and the finalizer releases.

internal/controller/pod_placement_controller_test.go

@@ -174,10 +174,6 @@ func TestPlacement_UngatesAndRoutesAndCreatesClaim(t *testing.T) {
 	if got.Spec.NodeSelector[nebulav1alpha1.ProviderLabel] != provider.ProviderModal {
 		t.Fatalf("expected nodeSelector provider=modal, got %v", got.Spec.NodeSelector)
 	}
-	// Capacity tier stamped for the VK handler.
-	if got.Annotations[nebulav1alpha1.CapacityTypeAnnotation] != "OnDemand" {
-		t.Fatalf("expected capacity-type OnDemand, got %q", got.Annotations[nebulav1alpha1.CapacityTypeAnnotation])
-	}
 	// NodeClaim created, pinned to the Pod, on the chosen provider.
 	var nc nebulav1alpha1.NodeClaim
 	if err := c.Get(context.Background(), types.NamespacedName{Name: "default-p1"}, &nc); err != nil {
@@ -186,6 +182,11 @@ func TestPlacement_UngatesAndRoutesAndCreatesClaim(t *testing.T) {
 	if nc.Spec.Provider != provider.ProviderModal || nc.Spec.PodRef.UID != "uid-1" || nc.Spec.PoolRef != "pool-a" {
 		t.Fatalf("unexpected claim spec: %+v", nc.Spec)
 	}
+	// Capacity tier recorded on the CLAIM, which is what the VK handler reads on
+	// CreatePod. Never on the Pod, where it would be patchable after the gate is gone.
+	if nc.Spec.CapacityType != nebulav1alpha1.CapacityOnDemand {
+		t.Fatalf("expected claim capacityType OnDemand, got %q", nc.Spec.CapacityType)
+	}
 	// The request's POOL identity (type:count) is recorded for reporting: H100 with
 	// no explicit count defaults to 1, so the pool is "H100:1".
 	if nc.Spec.Accelerator != "H100:1" {
@@ -442,9 +443,8 @@ func TestPlacement_FailsOverToNextRegionWhenBlocked(t *testing.T) {
 
 	reconcilePod(t, r, "default", "p1")
 
-	got := getPod(t, c, "default", "p1")
-	if got.Annotations[nebulav1alpha1.RegionAnnotation] != "us-west-2" {
-		t.Fatalf("expected failover to us-west-2, got region %q", got.Annotations[nebulav1alpha1.RegionAnnotation])
+	if got := getClaim(t, c, "default-p1").Spec.Region; got != "us-west-2" {
+		t.Fatalf("expected failover to us-west-2, got region %q", got)
 	}
 }
 
@@ -469,21 +469,21 @@ func TestPlacement_CapacityIsOuterAxis(t *testing.T) {
 
 	reconcilePod(t, r, "default", "p1")
 
-	got := getPod(t, c, "default", "p1")
-	if got.Annotations[nebulav1alpha1.CapacityTypeAnnotation] != "OnDemand" {
-		t.Fatalf("expected the walk to drop to OnDemand, got %q", got.Annotations[nebulav1alpha1.CapacityTypeAnnotation])
+	if got := getClaim(t, c, "default-p1").Spec.CapacityType; got != nebulav1alpha1.CapacityOnDemand {
+		t.Fatalf("expected the walk to drop to OnDemand, got %q", got)
 	}
 	// ...and to the FIRST provider (runpod), since OnDemand is walked provider-first.
+	got := getPod(t, c, "default", "p1")
 	if got.Spec.NodeSelector[nebulav1alpha1.ProviderLabel] != "runpod" {
 		t.Fatalf("expected first provider runpod at the OnDemand tier, got %v", got.Spec.NodeSelector)
 	}
 }
 
 func TestPlacement_ExpandsRegionGroupIntoConcreteCandidates(t *testing.T) {
 	// The pool declares a GROUP token, not a region. Placement must walk the concrete
-	// regions the provider expands it into — and must stamp a CONCRETE one on the Pod,
-	// never the token: RegionAnnotation feeds ProvisionRequest.Region, which the
-	// adapter turns into a regional API endpoint, and "us" is not one.
+	// regions the provider expands it into — and must record a CONCRETE one on the claim,
+	// never the token: the claim's region feeds ProvisionRequest.Region, which the adapter
+	// turns into a regional API endpoint, and "us" is not one.
 	pod := gatedPod("p1", "default", "uid-1", "pool-a", "H100")
 	pool := poolWithRegions("pool-a", []nebulav1alpha1.CapacityType{nebulav1alpha1.CapacityOnDemand},
 		provider.ProviderModal, "us")
@@ -506,8 +506,7 @@ func TestPlacement_ExpandsRegionGroupIntoConcreteCandidates(t *testing.T) {
 
 	reconcilePod(t, r, "default", "p1")
 
-	got := getPod(t, c, "default", "p1")
-	if region := got.Annotations[nebulav1alpha1.RegionAnnotation]; region != "us-west-2" {
+	if region := getClaim(t, c, "default-p1").Spec.Region; region != "us-west-2" {
 		t.Fatalf("expected the group to expand and fail over to us-west-2, got %q", region)
 	}
 }
@@ -560,9 +559,8 @@ func TestPlacement_SkipsSpotWhenProviderHasNoSpotTier(t *testing.T) {
 	if hasGateNamed(got) {
 		t.Fatal("expected the Pod placed at the OnDemand tier")
 	}
-	if got.Annotations[nebulav1alpha1.CapacityTypeAnnotation] != string(nebulav1alpha1.CapacityOnDemand) {
-		t.Fatalf("expected the Spot candidate skipped for OnDemand, got %q",
-			got.Annotations[nebulav1alpha1.CapacityTypeAnnotation])
+	if tier := getClaim(t, c, "default-p1").Spec.CapacityType; tier != nebulav1alpha1.CapacityOnDemand {
+		t.Fatalf("expected the Spot candidate skipped for OnDemand, got %q", tier)
 	}
 }
 
@@ -629,6 +627,38 @@ func TestPlacement_CopiesNoEgressPolicyOntoThePod(t *testing.T) {
 	}
 }
 
+func TestPlacement_CopiesNothingNebulaOwnedOntoThePod(t *testing.T) {
+	pod := gatedPod("p1", "default", "uid-1", "pool-a", "H100")
+	pool := poolWithRegions("pool-a", []nebulav1alpha1.CapacityType{nebulav1alpha1.CapacitySpot},
+		provider.ProviderModal, "us-east-1")
+	// A pool with something to say on every axis that used to ride the Pod.
+	pool.Spec.Egress = &nebulav1alpha1.EgressPolicy{Mode: nebulav1alpha1.EgressBlocked}
+	pool.Spec.Failover = &nebulav1alpha1.FailoverPolicy{BlocklistTTL: metav1.Duration{Duration: time.Hour}}
+	prov := &fakeProvider{name: provider.ProviderModal, gpus: []string{"H100"}, spot: true, egress: true}
+	r, c := newPlacementReconciler(t, []client.Object{pod, pool}, prov)
+
+	reconcilePod(t, r, "default", "p1")
+
+	got := getPod(t, c, "default", "p1")
+	if hasGateNamed(got) {
+		t.Fatal("expected the Pod placed")
+	}
+	// Swept by prefix rather than key by key: the claim is that NO Nebula-owned annotation is
+	// written, so a copy under a fresh spelling is the same bug and fails here too.
+	for k, v := range got.Annotations {
+		if strings.HasPrefix(k, nebulav1alpha1.GroupVersion.Group+"/") {
+			t.Errorf("annotation %s=%q was stamped on the Pod; a provisioning input on the Pod "+
+				"is patchable between ungate and CreatePod", k, v)
+		}
+	}
+	// ...and the decision really is recorded, on the claim the handler reads.
+	nc := getClaim(t, c, "default-p1")
+	if nc.Spec.CapacityType != nebulav1alpha1.CapacitySpot || nc.Spec.Region != "us-east-1" {
+		t.Errorf("claim records tier %q region %q, want Spot/us-east-1",
+			nc.Spec.CapacityType, nc.Spec.Region)
+	}
+}
+
 func TestPlacement_RestrictedPoolStaysGatedWhenNoProviderEnforcesEgress(t *testing.T) {
 	// The provider cannot enforce the policy, so there is no candidate and the Pod stays
 	// visibly unplaceable. This is the whole point of the capability gate: placing it

internal/controller/pod_placement_helpers.go

@@ -354,44 +354,29 @@ func (r *PodPlacementReconciler) ensureClaim(ctx context.Context, pod *corev1.Po
 	return false, nil // stale claim for a prior Pod; wait for the backstop
 }
 
-// place stamps the routing decision onto the Pod and removes the gate, atomically
-// from the Pod's perspective (one Update). After this, the scheduler is free to
-// bind the Pod to the chosen provider's virtual node.
+// place writes the one thing the Pod itself needs — the nodeSelector that routes it to the
+// chosen provider's virtual node — and removes the gate, atomically from the Pod's
+// perspective (one Update). After this, the scheduler is free to bind it.
 //
-// It takes the DECISION, not the pool: nothing from the pool's own spec is copied onto the
-// Pod any more. The egress policy and the failover TTL both used to be stamped here for the
-// VK handler to read back, which made them patchable by whoever owns the Pod; the handler
-// reads both from the NodePool at provision time instead.
+// Nothing else about the decision goes on the Pod. The provisioning inputs (capacity tier,
+// region) are already on the NodeClaim ensureClaim wrote a moment ago, and the pool's policy
+// (egress, failover TTL) stays on the NodePool. All of it used to be stamped here for the VK
+// handler to read back, which made every one of them patchable between ungate and CreatePod
+// by whoever the policy constrains; the handler reads cluster state instead.
 func (r *PodPlacementReconciler) place(ctx context.Context, pod *corev1.Pod, p placement) error {
 	// Route to the provider's virtual node.
 	if pod.Spec.NodeSelector == nil {
 		pod.Spec.NodeSelector = map[string]string{}
 	}
 	pod.Spec.NodeSelector[nebulav1alpha1.ProviderLabel] = p.provider
 
-	// Carry the capacity tier and region the VK handler reads on CreatePod (inputs
-	// that are not otherwise on the Pod). Skip each when empty (provider default).
-	if p.capacityType != "" {
-		setAnnotation(pod, nebulav1alpha1.CapacityTypeAnnotation, string(p.capacityType))
-	}
-	if p.region != "" {
-		setAnnotation(pod, nebulav1alpha1.RegionAnnotation, p.region)
-	}
 	// Remove our gate, releasing the Pod to the scheduler. Preserve any other
 	// gates a different controller may hold.
 	pod.Spec.SchedulingGates = removeGate(pod.Spec.SchedulingGates, nebulav1alpha1.ProviderSelectionGate)
 
 	return r.Update(ctx, pod)
 }
 
-// setAnnotation sets one annotation on the Pod, allocating the map on first use.
-func setAnnotation(pod *corev1.Pod, key, value string) {
-	if pod.Annotations == nil {
-		pod.Annotations = map[string]string{}
-	}
-	pod.Annotations[key] = value
-}
-
 // removeGate returns gates with the named gate removed, preserving order.
 func removeGate(gates []corev1.PodSchedulingGate, name string) []corev1.PodSchedulingGate {
 	out := gates[:0]

pkg/provider/aws/aws.go

@@ -359,18 +359,9 @@ func (p *Provider) sweepRegions() []string {
 	return out
 }
 
-// clientFor returns the Client for region (defaulting an empty region), building
-// and caching it on first use. Construction is serialized under mu: a region's
-// Client is built at most once, and a concurrent caller for the same or another
-// region waits — acceptable because a build is a rare one-time, per-region event
-// (an AMI + subnet resolution), not a hot path. A build failure is NOT cached, so
-// a transient resolution error (throttle, a not-yet-enabled region) is retried on
-// the next call rather than poisoning the region permanently.
+// clientFor returns the Client for region, building and caching it on first use.
 func (p *Provider) clientFor(ctx context.Context, region string) (Client, error) {
 	if region == "" {
-		// No region to build a client for. Unreachable on the normal path (every
-		// request carries a region), so this only guards a legacy unqualified instance
-		// id reaching Terminate/Get — surface it rather than silently guessing.
 		return nil, fmt.Errorf("aws: no region for client: %w", ErrConfig)
 	}
 	p.mu.Lock()
@@ -511,30 +502,24 @@ func (p *Provider) Provision(
 	return provider.ProvisionResult{InstanceID: id, Reserved: true}, nil
 }
 
-// Terminate implements provider.Provider. Idempotent by the Client contract. The
-// instanceID is a raw EC2 id, which does not carry its region, so teardown sweeps
-// the swept regions and terminates the instance wherever it lives. This is safe
-// and cheap: TerminateInstance is idempotent and a wrong-region lookup returns
-// InvalidInstanceID.NotFound, which the Client maps to nil — so terminating in a
-// region the instance is not in is a harmless no-op. It stops at the first region
-// that actually owns the instance.
+// Terminate implements provider.Provider. Idempotent by the Client contract.
 //
-// A legacy region-qualified id ("<region>/i-...") from before this change is still
-// honored: splitID peels the region off and it routes straight to that region.
-func (p *Provider) Terminate(ctx context.Context, instanceID string) error {
+// An EC2 id is only reachable through its own region's endpoint, so the region picks the
+// client — and terminating there needs no Describe first, since TerminateInstance is
+// idempotent and maps InvalidInstanceID.NotFound to nil.
+func (p *Provider) Terminate(ctx context.Context, instanceID, region string) error {
 	if instanceID == "" {
 		return nil // nothing provisioned yet; treat as already gone
 	}
-	// Back-compat: a legacy qualified id routes directly to its region.
-	if region, rawID := splitID(instanceID); region != "" {
+	if region != "" {
 		client, err := p.clientFor(ctx, region)
 		if err != nil {
 			return err
 		}
-		return client.TerminateInstance(ctx, rawID)
+		return client.TerminateInstance(ctx, instanceID)
 	}
 
-	// Raw id: sweep the regions and terminate wherever it lives. Confirm ownership
+	// Sweep the regions and terminate wherever it lives. Confirm ownership
 	// with a Describe first so we only issue TerminateInstance against the region
 	// that actually has it — and so a region whose client cannot be built does not
 	// mask a successful terminate elsewhere.
@@ -565,25 +550,7 @@ func (p *Provider) Terminate(ctx context.Context, instanceID string) error {
 // region's view of the instance. A per-region client-build/describe error is
 // tolerated and the sweep continues; only if every region errored (and none held
 // the instance) is that error surfaced.
-//
-// A legacy region-qualified id ("<region>/i-...") routes directly to its region.
 func (p *Provider) Get(ctx context.Context, instanceID string) (*provider.Instance, error) {
-	if region, rawID := splitID(instanceID); region != "" {
-		client, err := p.clientFor(ctx, region)
-		if err != nil {
-			return nil, err
-		}
-		ec2, err := client.DescribeInstance(ctx, rawID)
-		if err != nil {
-			return nil, err
-		}
-		if ec2 == nil {
-			return nil, nil // absent => terminated, per interface contract
-		}
-		inst := p.toInstance(*ec2)
-		return &inst, nil
-	}
-
 	var lastErr error
 	for _, region := range p.sweepRegions() {
 		client, err := p.clientFor(ctx, region)
@@ -726,24 +693,6 @@ func findByClaim(ctx context.Context, client Client, claimName string) (*EC2Inst
 	return nil, nil
 }
 
-// idSep separates the region prefix from the raw EC2 id in a legacy region-qualified
-// instance id ("<region>/<ec2-id>"). "/" cannot appear in either a region name or
-// an EC2 instance id, so it is an unambiguous delimiter. Current ids are raw EC2
-// ids; this exists only so splitID can still route ids recorded before that change.
-const idSep = "/"
-
-// splitID recognizes a LEGACY region-qualified id ("<region>/i-..."): it returns
-// the region and the raw EC2 id. A current, raw EC2 id (no separator) yields an
-// empty region, signaling the caller to locate the instance by sweeping regions
-// instead. It is the one remaining reader of the old format, kept so ids persisted
-// on a NodeClaim before the id stopped being qualified still terminate correctly.
-func splitID(instanceID string) (region, rawID string) {
-	if i := strings.Index(instanceID, idSep); i >= 0 {
-		return instanceID[:i], instanceID[i+len(idSep):]
-	}
-	return "", instanceID
-}
-
 // instanceSpecFromPod reads the workload off the Pod (source of truth) and the
 // accelerator type (from the AcceleratorTypeLabel), maps it to an EC2 instance
 // type via the catalog, and stamps the claim tag, capacity tier, and region.