From 9c934aea2dcf9972fe5f9c9c87c125073187f238 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Sun, 27 Sep 2026 23:40:24 +0100 Subject: [PATCH] Add FindByClaim as interface to provider Signed-off-by: kerthcet --- internal/controller/nodeclaim_controller.go | 50 ++++++++----------- .../controller/nodeclaim_controller_test.go | 31 ++++++++++-- pkg/provider/aws/aws.go | 30 ++++++++--- pkg/provider/aws/aws_test.go | 26 ++++++++++ pkg/provider/fake/fake.go | 13 +++++ pkg/provider/modal/modal.go | 6 +-- pkg/provider/provider.go | 7 +++ pkg/vnode/handler_test.go | 11 ++++ 8 files changed, 128 insertions(+), 46 deletions(-) diff --git a/internal/controller/nodeclaim_controller.go b/internal/controller/nodeclaim_controller.go index bb19cfa..336bdad 100644 --- a/internal/controller/nodeclaim_controller.go +++ b/internal/controller/nodeclaim_controller.go @@ -76,7 +76,7 @@ const podReasonInitializing = nebulav1alpha1.PodReasonInitializing // sees no Pod and no delete event, so it never calls Terminate. The claim is cluster-scoped, // so it outlives the namespaced Pod and is reconciled level-triggered on every restart. When // the served Pod is gone it self-deletes, and its finalizer reclaims the instance (resolve -// provider → find by claim name via List → Terminate) independent of VK liveness. +// provider → find by claim name → Terminate) independent of VK liveness. // // Self-delete is guarded by placementGracePeriod, but only for a claim that has never // observed its Pod, so cache lag never tears down a live workload. @@ -161,7 +161,7 @@ func (r *NodeClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( } // Grace elapsed and the Pod never appeared: nothing was ever provisioned for - // this claim. Delete it; the backstop List finds no instance and the finalizer + // this claim. Delete it; the backstop finds no instance and the finalizer // releases cleanly. log.Info("served Pod never appeared within grace period; deleting orphaned claim", "pod", nc.Spec.PodRef.Name) @@ -199,18 +199,18 @@ func (r *NodeClaimReconciler) reconcileDelete(ctx context.Context, nc *nebulav1a return r.releaseFinalizer(ctx, nc) } - // Find the instance by its Pod-derived claim name. We cannot rely on - // status.InstanceID: VK tracks the id in memory, so if VK died (the very case - // this backstop exists for) that id was never persisted anywhere we can read. - // Re-deriving the claim name from PodRef and matching it against List is the - // only way to reclaim an instance VK has forgotten about. If DeletePod already - // ran, List simply won't contain it and findInstanceID returns "" (a no-op - // Terminate). - claim := util.ClaimName(nc.Spec.PodRef.Namespace, nc.Spec.PodRef.Name) - id, err := r.findInstanceID(ctx, prov, claim, nc.Status.InstanceID) - if err != nil { - log.Error(err, "listing provider instances for teardown; will retry") - return ctrl.Result{}, err + // status.InstanceID can be empty: VK died before the id reached the Pod (the very + // case this backstop exists for), or a failed Provision created an instance without + // ever returning its id. The Pod-derived claim name is then the only way to reclaim + // it. If DeletePod already ran, nothing matches and id stays "" (a no-op Terminate). + id := nc.Status.InstanceID + if id == "" { + claim := util.ClaimName(nc.Spec.PodRef.Namespace, nc.Spec.PodRef.Name) + var err error + if id, err = r.findInstanceID(ctx, prov, claim, nc.Spec.Region); err != nil { + log.Error(err, "looking up the claim's instance for teardown; will retry") + return ctrl.Result{}, err + } } // Terminate is idempotent (terminating an already-gone or empty instance @@ -231,24 +231,14 @@ func (r *NodeClaimReconciler) reconcileDelete(ctx context.Context, nc *nebulav1a return r.releaseFinalizer(ctx, nc) } -// findInstanceID resolves the provider instance id to terminate. It prefers a -// recorded status.InstanceID, then falls back to matching the Pod-derived claim -// name against provider.List(). Returns "" when no instance exists (already gone -// or never provisioned), which Terminate treats as a no-op. -func (r *NodeClaimReconciler) findInstanceID(ctx context.Context, prov provider.Provider, claim, recordedID string) (string, error) { - if recordedID != "" { - return recordedID, nil - } - instances, err := prov.List(ctx) - if err != nil { +// findInstanceID returns the id of claim's live instance, or "" when none exists +// (already gone or never provisioned), which Terminate treats as a no-op. +func (r *NodeClaimReconciler) findInstanceID(ctx context.Context, prov provider.Provider, claim, region string) (string, error) { + inst, err := prov.FindByClaim(ctx, claim, region) + if err != nil || inst == nil { return "", err } - for _, inst := range instances { - if inst.ClaimName == claim { - return inst.ID, nil - } - } - return "", nil // no live instance for this claim + return inst.ID, nil } // releaseFinalizer removes the terminate finalizer, allowing the API server to diff --git a/internal/controller/nodeclaim_controller_test.go b/internal/controller/nodeclaim_controller_test.go index 2410f28..292d391 100644 --- a/internal/controller/nodeclaim_controller_test.go +++ b/internal/controller/nodeclaim_controller_test.go @@ -39,12 +39,13 @@ import ( // fakeProvider is a minimal provider.Provider. On the happy path the NodeClaim // controller never touches a provider (VK owns provisioning), but the teardown -// backstop calls List/Terminate on the deletion path, so this records those. +// backstop calls FindByClaim/Terminate on the deletion path, so this records those. type fakeProvider struct { name string - list []provider.Instance // what List returns - listErr error // if set, List fails + list []provider.Instance // what List returns, and what FindByClaim searches + listErr error // if set, List and FindByClaim fail + findRegions []string // regions passed to FindByClaim, one per call 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 @@ -73,6 +74,18 @@ func (f *fakeProvider) Get(context.Context, string, string) (*provider.Instance, func (f *fakeProvider) List(context.Context) ([]provider.Instance, error) { return f.list, f.listErr } +func (f *fakeProvider) FindByClaim(_ context.Context, claim, region string) (*provider.Instance, error) { + f.findRegions = append(f.findRegions, region) + if f.listErr != nil { + return nil, f.listErr + } + for _, inst := range f.list { + if inst.ClaimName == claim { + return &inst, nil + } + } + return nil, nil +} func (f *fakeProvider) Offerings(context.Context) ([]provider.Offering, error) { return nil, nil } func (f *fakeProvider) MapAccelerator(c string, _ int32) ([]string, bool) { if f.gpus == nil { @@ -465,8 +478,10 @@ func TestReconcile_NeverObservedPodDeletedAfterGrace(t *testing.T) { func TestReconcileDelete_TerminatesInstanceByClaimName(t *testing.T) { // The backstop: on the deletion path, the instance is found by its - // Pod-derived claim name via List and terminated before the finalizer drops. + // Pod-derived claim name and terminated before the finalizer drops. The lookup + // gets spec.Region for the same reason Terminate does (see the test below). claim := newClaim("c1", "p1", "default", "uid-1", "fake") + claim.Spec.Region = "eu-west-1" deleteClaim(t, claim) // set deletionTimestamp; finalizer keeps it alive prov := &fakeProvider{ name: "fake", @@ -479,6 +494,9 @@ func TestReconcileDelete_TerminatesInstanceByClaimName(t *testing.T) { if len(prov.terminated) != 1 || prov.terminated[0] != "inst-1" { t.Fatalf("expected Terminate(inst-1), got %v", prov.terminated) } + if len(prov.findRegions) != 1 || prov.findRegions[0] != "eu-west-1" { + t.Fatalf("regions passed to FindByClaim = %v, want [eu-west-1]", prov.findRegions) + } if claimExists(t, c, "c1") { t.Fatal("expected finalizer released and claim gone after teardown") } @@ -486,7 +504,7 @@ func TestReconcileDelete_TerminatesInstanceByClaimName(t *testing.T) { func TestReconcileDelete_UsesRecordedInstanceID(t *testing.T) { // When status.InstanceID is set (VK wrote it), the backstop terminates it - // directly without needing a List lookup. + // directly without a provider lookup. claim := newClaim("c1", "p1", "default", "uid-1", "fake") claim.Status.InstanceID = "inst-recorded" deleteClaim(t, claim) @@ -498,6 +516,9 @@ func TestReconcileDelete_UsesRecordedInstanceID(t *testing.T) { if len(prov.terminated) != 1 || prov.terminated[0] != "inst-recorded" { t.Fatalf("expected Terminate(inst-recorded) from recorded id, got %v", prov.terminated) } + if len(prov.findRegions) != 0 { + t.Fatalf("FindByClaim called %d times with a recorded id, want 0", len(prov.findRegions)) + } } // The backstop runs when VK never did, so the region VK held in memory is gone too. It diff --git a/pkg/provider/aws/aws.go b/pkg/provider/aws/aws.go index ef83b61..abcc5a0 100644 --- a/pkg/provider/aws/aws.go +++ b/pkg/provider/aws/aws.go @@ -498,7 +498,7 @@ func (p *Provider) Provision( // create). A claim is placed in exactly one region per attempt, so scanning the // 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 { + if existing, err := client.FindInstance(ctx, req.ClaimName); err != nil { return provider.ProvisionResult{}, err } else if existing != nil { return provider.ProvisionResult{InstanceID: existing.ID, Reserved: true}, nil @@ -608,6 +608,27 @@ func (p *Provider) Get(ctx context.Context, instanceID, region string) (*provide return nil, lastErr } +// FindByClaim implements provider.Provider. An empty region finds nothing rather than +// sweeping: Provision cannot launch without one (see clientFor), so no instance exists. +// +// Only pending/running instances match (see sdkClient.FindInstance), so a STOPPED instance +// whose id was never recorded is not found — acceptable, as it bills EBS only, not compute. +func (p *Provider) FindByClaim(ctx context.Context, claimName, region string) (*provider.Instance, error) { + if region == "" { + return nil, nil + } + client, err := p.clientFor(ctx, region) + if err != nil { + return nil, err + } + ec2, err := client.FindInstance(ctx, claimName) + if err != nil || ec2 == nil { + return nil, err + } + inst := p.toInstance(*ec2) + return &inst, nil +} + // List implements provider.Provider. It FANS OUT across every region sweepRegions // yields (the NodePool-declared set unioned with regions already provisioned into) // and concatenates the results, so the poll loop and the NodeClaim teardown backstop @@ -718,13 +739,6 @@ func (p *Provider) ClassifyProvisionError(err error, accelerator, region string) return scope } -// findByClaim returns the instance in this client's region tagged with claimName, -// or nil if none. It scans one region's client (the launch target), since a claim -// is placed in exactly one region per provision attempt. -func findByClaim(ctx context.Context, client Client, claimName string) (*EC2Instance, error) { - return client.FindInstance(ctx, claimName) -} - // 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. diff --git a/pkg/provider/aws/aws_test.go b/pkg/provider/aws/aws_test.go index 4921d33..7a9014e 100644 --- a/pkg/provider/aws/aws_test.go +++ b/pkg/provider/aws/aws_test.go @@ -541,6 +541,32 @@ func TestTerminate_RegionOutsideTheSweep(t *testing.T) { } } +func TestFindByClaim(t *testing.T) { + f := &fakeClient{ + instances: []EC2Instance{{ + ID: "i-1", Tags: map[string]string{ClaimTagKey: "claim-a"}, State: stateRunning, Region: testRegion, + }}, + } + p := newTestProvider(f) + + got, err := p.FindByClaim(context.Background(), "claim-a", testRegion) + if err != nil || got == nil || got.ID != "i-1" { + t.Fatalf("FindByClaim = %+v, %v; want i-1", got, err) + } + if got, err := p.FindByClaim(context.Background(), "claim-b", testRegion); err != nil || got != nil { + t.Fatalf("FindByClaim(unknown claim) = %+v, %v; want nil, nil", got, err) + } + // No region means Provision never launched, so nothing to find — and no error, which + // would wedge the NodeClaim finalizer on a claim that owns nothing. + if got, err := p.FindByClaim(context.Background(), "claim-a", ""); err != nil || got != nil { + t.Fatalf("FindByClaim(no region) = %+v, %v; want nil, nil", got, err) + } + // A lookup, not a List: every instance in the region comes back with status checks. + if f.listCnt != 0 { + t.Errorf("ListInstances called %d times, want 0", f.listCnt) + } +} + // TestSweepRegions_CoversEveryPoolsPlacement pins that the sweep covers every region // placement can provision into, per pool. The unconstrained pool is the case that breaks if // declarations are flattened before resolving: appended to ["us"] it would vanish, leaving diff --git a/pkg/provider/fake/fake.go b/pkg/provider/fake/fake.go index 32b662b..062e9cb 100644 --- a/pkg/provider/fake/fake.go +++ b/pkg/provider/fake/fake.go @@ -232,6 +232,19 @@ func (p *Provider) List(_ context.Context) ([]provider.Instance, error) { return out, nil } +// FindByClaim returns the instance recorded for claimName, or (nil, nil) if none. +func (p *Provider) FindByClaim(_ context.Context, claimName, _ string) (*provider.Instance, error) { + p.mu.Lock() + defer p.mu.Unlock() + for _, inst := range p.instances { + if inst.ClaimName == claimName { + out := *inst + return &out, nil + } + } + return nil, nil +} + // ClassifyProvisionError never really fails to provision, so any error it is // asked to classify is treated as a whole-provider block on OnDemand — the same // shared derivation the real adapters use. diff --git a/pkg/provider/modal/modal.go b/pkg/provider/modal/modal.go index 85dbf2e..9dd272b 100644 --- a/pkg/provider/modal/modal.go +++ b/pkg/provider/modal/modal.go @@ -472,7 +472,7 @@ func (p *Provider) Provision( // No credential comes back, per the Provider contract: minting is one-shot with no // read-back, and a fresh token revokes nothing, so re-minting for a sandbox whose token is // already published strands the consumer holding it. - if existing, err := p.findByClaim(ctx, req.ClaimName); err != nil { + if existing, err := p.FindByClaim(ctx, req.ClaimName, ""); err != nil { return provider.ProvisionResult{}, err } else if existing != nil { return provider.ProvisionResult{ @@ -594,8 +594,8 @@ func (p *Provider) ClassifyProvisionError(err error, accelerator, region string) return scope } -// findByClaim returns the sandbox tagged with claimName, or nil if none. -func (p *Provider) findByClaim(ctx context.Context, claimName string) (*provider.Instance, error) { +// FindByClaim implements provider.Provider. The region is ignored, as in Terminate. +func (p *Provider) FindByClaim(ctx context.Context, claimName, _ string) (*provider.Instance, error) { sb, err := p.client.FindSandbox(ctx, claimName) if err != nil || sb == nil { return nil, err diff --git a/pkg/provider/provider.go b/pkg/provider/provider.go index 6320787..02d60eb 100644 --- a/pkg/provider/provider.go +++ b/pkg/provider/provider.go @@ -89,6 +89,13 @@ type Provider interface { // changing state here. List(ctx context.Context) ([]Instance, error) + // FindByClaim returns the live instance for claimName, or (nil, nil) if none. It is the + // lookup for an instance whose id was never recorded, so it should cost one filtered + // call where the provider has one, not a List. + // + // region is the claim's (NodeClaim.Spec.Region), the one Provision launched into. + FindByClaim(ctx context.Context, claimName, region string) (*Instance, error) + // --- Catalog --------------------------------------------------------- // Offerings returns the price/availability rows this provider can serve, feeding the diff --git a/pkg/vnode/handler_test.go b/pkg/vnode/handler_test.go index 9cc312e..4a52497 100644 --- a/pkg/vnode/handler_test.go +++ b/pkg/vnode/handler_test.go @@ -125,6 +125,17 @@ func (f *fakeProvider) Get(context.Context, string, string) (*provider.Instance, func (f *fakeProvider) List(context.Context) ([]provider.Instance, error) { return f.list, f.listErr } +func (f *fakeProvider) FindByClaim(_ context.Context, claim, _ string) (*provider.Instance, error) { + if f.listErr != nil { + return nil, f.listErr + } + for _, inst := range f.list { + if inst.ClaimName == claim { + return &inst, nil + } + } + return nil, nil +} func (f *fakeProvider) Offerings(context.Context) ([]provider.Offering, error) { return nil, nil } func (f *fakeProvider) MapAccelerator(c string, _ int32) ([]string, bool) { return []string{c}, true } func (f *fakeProvider) ResolveRegions(declared, _ []string) []string { return declared }