From 0a739412a552e9468167b6018737479d90a77e8d Mon Sep 17 00:00:00 2001 From: kerthcet Date: Sat, 22 Aug 2026 22:50:11 +0100 Subject: [PATCH] Add region to Get Signed-off-by: kerthcet --- .../controller/nodeclaim_controller_test.go | 4 ++- pkg/provider/aws/aws.go | 30 +++++++++++++++---- pkg/provider/aws/aws_test.go | 10 +++---- pkg/provider/fake/fake.go | 2 +- pkg/provider/fake/fake_test.go | 4 +-- pkg/provider/modal/modal.go | 4 +-- pkg/provider/provider.go | 5 +++- pkg/vnode/handler_test.go | 4 ++- 8 files changed, 44 insertions(+), 19 deletions(-) diff --git a/internal/controller/nodeclaim_controller_test.go b/internal/controller/nodeclaim_controller_test.go index 696a16e..0e17b46 100644 --- a/internal/controller/nodeclaim_controller_test.go +++ b/internal/controller/nodeclaim_controller_test.go @@ -66,7 +66,9 @@ func (f *fakeProvider) Terminate(_ context.Context, id, region string) error { f.regions = append(f.regions, region) return f.terminateErr } -func (f *fakeProvider) Get(context.Context, string) (*provider.Instance, error) { return nil, nil } +func (f *fakeProvider) Get(context.Context, string, string) (*provider.Instance, error) { + return nil, nil +} func (f *fakeProvider) List(context.Context) ([]provider.Instance, error) { return f.list, f.listErr } diff --git a/pkg/provider/aws/aws.go b/pkg/provider/aws/aws.go index 20e8e4c..e0ff5d1 100644 --- a/pkg/provider/aws/aws.go +++ b/pkg/provider/aws/aws.go @@ -545,12 +545,30 @@ func (p *Provider) Terminate(ctx context.Context, instanceID, region string) err return lastErr } -// Get implements provider.Provider. instanceID is a raw EC2 id, which does not -// carry its region, so the lookup sweeps the swept regions and returns the first -// 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. -func (p *Provider) Get(ctx context.Context, instanceID string) (*provider.Instance, error) { +// Get implements provider.Provider. As in Terminate, the region names the endpoint that +// can answer for the instance, and Describe there is the whole call. +// +// Without a region the lookup sweeps and returns the first region's view. 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. An unswept region reads as +// (nil, nil) — indistinguishable from terminated, which is why the region is worth passing. +func (p *Provider) Get(ctx context.Context, instanceID, region string) (*provider.Instance, error) { + if region != "" { + client, err := p.clientFor(ctx, region) + if err != nil { + return nil, err + } + ec2, err := client.DescribeInstance(ctx, instanceID) + if err != nil { + return nil, err + } + if ec2 == nil { + return nil, nil // gone from the region that owns it: terminated + } + inst := p.toInstance(*ec2) + return &inst, nil + } + var lastErr error for _, region := range p.sweepRegions() { client, err := p.clientFor(ctx, region) diff --git a/pkg/provider/aws/aws_test.go b/pkg/provider/aws/aws_test.go index 89081e7..e350f36 100644 --- a/pkg/provider/aws/aws_test.go +++ b/pkg/provider/aws/aws_test.go @@ -415,8 +415,8 @@ func TestGetAndList_NormalizeInstance(t *testing.T) { } p := newTestProvider(f) - // Get takes the raw EC2 id Provision/List hand back; it sweeps regions to find it. - got, err := p.Get(context.Background(), "i-1") + // Get with no region falls back to sweeping for the instance. + got, err := p.Get(context.Background(), "i-1", "") if err != nil { t.Fatalf("Get: %v", err) } @@ -437,7 +437,7 @@ func TestGetAndList_NormalizeInstance(t *testing.T) { } // A missing instance is (nil, nil): absence == terminated per the contract. - missing, err := p.Get(context.Background(), "i-gone") + missing, err := p.Get(context.Background(), "i-gone", testRegion) if err != nil { t.Fatalf("Get(missing): %v", err) } @@ -449,8 +449,8 @@ func TestGetAndList_NormalizeInstance(t *testing.T) { if err != nil { t.Fatalf("List: %v", err) } - // List reports the raw EC2 id; a downstream Terminate re-locates it by sweeping - // regions. + // List reports the raw EC2 id, alongside the Region a downstream Get/Terminate should + // pass back so neither has to search for it. if len(list) != 1 || list[0].ID != "i-1" { t.Fatalf("List = %+v", list) } diff --git a/pkg/provider/fake/fake.go b/pkg/provider/fake/fake.go index 5aab5cd..baf4878 100644 --- a/pkg/provider/fake/fake.go +++ b/pkg/provider/fake/fake.go @@ -134,7 +134,7 @@ func (p *Provider) Terminate(_ context.Context, instanceID, _ string) error { } // Get returns one instance, or (nil, nil) if it no longer exists. -func (p *Provider) Get(_ context.Context, instanceID string) (*provider.Instance, error) { +func (p *Provider) Get(_ context.Context, instanceID, _ string) (*provider.Instance, error) { p.mu.Lock() defer p.mu.Unlock() inst, ok := p.instances[instanceID] diff --git a/pkg/provider/fake/fake_test.go b/pkg/provider/fake/fake_test.go index 7839c92..c9c76d8 100644 --- a/pkg/provider/fake/fake_test.go +++ b/pkg/provider/fake/fake_test.go @@ -63,7 +63,7 @@ func TestProvisionReportsRunningAndLists(t *testing.T) { } // Get reports it Running with the claim recovered. - inst, err := p.Get(ctx, id) + inst, err := p.Get(ctx, id, "") if err != nil || inst == nil { t.Fatalf("Get = (%v, %v), want a live instance", inst, err) } @@ -120,7 +120,7 @@ func TestTerminateIsIdempotent(t *testing.T) { t.Fatalf("Terminate: %v", err) } // Gone from Get/List. - if inst, _ := p.Get(ctx, id); inst != nil { + if inst, _ := p.Get(ctx, id, ""); inst != nil { t.Fatalf("Get after Terminate = %v, want nil (terminated)", inst) } // A repeat Terminate (and terminating an unknown id) is a no-op, not an error. diff --git a/pkg/provider/modal/modal.go b/pkg/provider/modal/modal.go index 8c20709..4490d32 100644 --- a/pkg/provider/modal/modal.go +++ b/pkg/provider/modal/modal.go @@ -406,8 +406,8 @@ func (p *Provider) Exec( return p.client.SandboxExec(ctx, instanceID, cmd, opts) } -// Get implements provider.Provider. -func (p *Provider) Get(ctx context.Context, instanceID string) (*provider.Instance, error) { +// Get implements provider.Provider. The region is ignored, as in Terminate. +func (p *Provider) Get(ctx context.Context, instanceID, _ string) (*provider.Instance, error) { sb, err := p.client.GetSandbox(ctx, instanceID) if err != nil { return nil, err diff --git a/pkg/provider/provider.go b/pkg/provider/provider.go index 35c121a..43470a3 100644 --- a/pkg/provider/provider.go +++ b/pkg/provider/provider.go @@ -78,7 +78,10 @@ type Provider interface { // Get returns the current state of one instance, or (nil, nil) if it no // longer exists (treat absence as terminated). - Get(ctx context.Context, instanceID string) (*Instance, error) + // + // region is as in Terminate, and matters more here: absence is read as terminated, so a + // provider that had to search and searched the wrong place reports a live instance dead. + Get(ctx context.Context, instanceID, region string) (*Instance, error) // List returns every instance Nebula owns on this provider, in as few API calls as // possible (ideally one). This is the engine of the poll loop: since no provider pushes diff --git a/pkg/vnode/handler_test.go b/pkg/vnode/handler_test.go index 6b6d431..9913620 100644 --- a/pkg/vnode/handler_test.go +++ b/pkg/vnode/handler_test.go @@ -119,7 +119,9 @@ func (f *fakeProvider) Terminate(_ context.Context, id, region string) error { return f.terminateErr } -func (f *fakeProvider) Get(context.Context, string) (*provider.Instance, error) { return nil, nil } +func (f *fakeProvider) Get(context.Context, string, string) (*provider.Instance, error) { + return nil, nil +} func (f *fakeProvider) List(context.Context) ([]provider.Instance, error) { return f.list, f.listErr }