From dcbbf5294c9ac5056863ab6f84308a61f617dc45 Mon Sep 17 00:00:00 2001 From: Glenn Pratt Date: Wed, 14 Oct 2026 20:28:40 -0400 Subject: [PATCH] [fix] stop ListRoutes refreshing the instance cache * Read the instance cache without checking its TTL, so route reconciles no longer re-list account instances and VPC IPs about every 20s. * Copy the cache under its read lock, fixing a data race with refreshes. * A node missing from the cache triggers CreateRoute, which refreshes an expired cache via LookupLinode; periodic node controller lookups keep the cache fresh. --- cloud/linode/route_controller.go | 8 ++- cloud/linode/route_controller_test.go | 6 ++ cloud/linode/services/instances.go | 27 +++++++-- cloud/linode/services/instances_test.go | 81 +++++++++++++++++++++++++ 4 files changed, 116 insertions(+), 6 deletions(-) diff --git a/cloud/linode/route_controller.go b/cloud/linode/route_controller.go index 17fd127a..3888e265 100644 --- a/cloud/linode/route_controller.go +++ b/cloud/linode/route_controller.go @@ -276,10 +276,14 @@ func (r *routes) handleInterfaces(ctx context.Context, intfRoutes []string, lino return nil } -// ListRoutes fetches routes configured on all instances which have VPC interfaces +// ListRoutes fetches routes configured on all instances which have VPC interfaces. +// It reads the instance cache without refreshing it, because only instance IDs and labels are +// needed and refreshing here would re-list instances on every reconcile. An instance missing from +// the cache has its routes left out. The route controller then calls CreateRoute, which returns nil +// if the route already exists and refreshes the cache if it has expired. func (r *routes) ListRoutes(ctx context.Context, clusterName string) ([]*cloudprovider.Route, error) { klog.V(4).Infof("Fetching routes configured on the cluster") - instances, err := r.instances.ListAllInstances(ctx) + instances, err := r.instances.ListCachedInstances(ctx) if err != nil { return nil, err } diff --git a/cloud/linode/route_controller_test.go b/cloud/linode/route_controller_test.go index fb76eb23..2d2b9321 100644 --- a/cloud/linode/route_controller_test.go +++ b/cloud/linode/route_controller_test.go @@ -103,6 +103,12 @@ func TestListRoutes(t *testing.T) { defer ctrl.Finish() client := mocks.NewMockClient(ctrl) instanceCache := services.NewInstances(client) + existingK8sCache := registeredK8sNodeCache + defer func() { + registeredK8sNodeCache = existingK8sCache + }() + registeredK8sNodeCache = newK8sNodeCache() + registeredK8sNodeCache.addNodeToCache(node) routeController, err := newRoutes(client, instanceCache) require.NoError(t, err) diff --git a/cloud/linode/services/instances.go b/cloud/linode/services/instances.go index 83c0fc5a..37aafc65 100644 --- a/cloud/linode/services/instances.go +++ b/cloud/linode/services/instances.go @@ -194,6 +194,20 @@ func (nc *nodeCache) refreshInstances(ctx context.Context, client linodeClient.C return nil } +// loadIfNeverPopulated loads the cache only if it has never been populated. Unlike refreshInstances, +// it does not reload a cache that has data but is past its TTL. +func (nc *nodeCache) loadIfNeverPopulated(ctx context.Context, client linodeClient.Client) error { + nc.RLock() + populated := !nc.lastUpdate.IsZero() + nc.RUnlock() + if populated { + return nil + } + + // refreshInstances rechecks under the write lock, so concurrent callers do not load twice. + return nc.refreshInstances(ctx, client) +} + type Instances struct { client linodeClient.Client @@ -270,13 +284,18 @@ func (i *Instances) linodeByID(id int) (*linodego.Instance, error) { return linodeInstance.instance, nil } -// ListAllInstances returns all instances in nodeCache -func (i *Instances) ListAllInstances(ctx context.Context) ([]linodego.Instance, error) { - if err := i.nodeCache.refreshInstances(ctx, i.client); err != nil { +// ListCachedInstances returns the instances as of the last cache refresh. It loads the cache only +// if it has never been loaded and does not check the TTL. The cache is refreshed when LookupLinode +// finds it expired, which the cloud node controller triggers for every node every +// --node-status-update-frequency (5 minutes by default). +func (i *Instances) ListCachedInstances(ctx context.Context) ([]linodego.Instance, error) { + if err := i.nodeCache.loadIfNeverPopulated(ctx, i.client); err != nil { return nil, err } - instances := []linodego.Instance{} + i.nodeCache.RLock() + defer i.nodeCache.RUnlock() + instances := make([]linodego.Instance, 0, len(i.nodeCache.nodes)) for _, linodeInstance := range i.nodeCache.nodes { instances = append(instances, *linodeInstance.instance) } diff --git a/cloud/linode/services/instances_test.go b/cloud/linode/services/instances_test.go index 48c26fcd..8823d061 100644 --- a/cloud/linode/services/instances_test.go +++ b/cloud/linode/services/instances_test.go @@ -6,7 +6,9 @@ import ( "slices" "strconv" "strings" + "sync" "testing" + "time" "github.com/golang/mock/gomock" "github.com/linode/linodego/v2" @@ -575,6 +577,85 @@ func TestRefreshInstancesVPCErrors(t *testing.T) { }) } +func TestListCachedInstances(t *testing.T) { + ctx := t.Context() + listOpts := &linodego.ListOptions{PageSize: linodeClient.MaxPageSize, Filter: "{}"} + cachedInstance := linodego.Instance{ID: 123, Label: "test-instance"} + + t.Run("loads the cache when it has never been populated", func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + c := mocks.NewMockClient(ctrl) + instances := NewInstances(c) + c.EXPECT().ListInstances(gomock.Any(), listOpts).Times(1).Return([]linodego.Instance{cachedInstance}, nil) + + got, err := instances.ListCachedInstances(ctx) + require.NoError(t, err) + assert.Equal(t, []linodego.Instance{cachedInstance}, got) + }) + + t.Run("lists only on the first load, even after the TTL expires", func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + c := mocks.NewMockClient(ctrl) + instances := NewInstances(c) + c.EXPECT().ListInstances(gomock.Any(), listOpts).Times(1).Return([]linodego.Instance{cachedInstance}, nil) + + _, err := instances.ListCachedInstances(ctx) + require.NoError(t, err) + + instances.nodeCache.lastUpdate = time.Now().Add(-time.Hour) + + got, err := instances.ListCachedInstances(ctx) + require.NoError(t, err) + assert.Equal(t, []linodego.Instance{cachedInstance}, got) + }) + + t.Run("retries the initial load when it fails", func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + c := mocks.NewMockClient(ctrl) + instances := NewInstances(c) + apiErr := fmt.Errorf("list instances failed") + c.EXPECT().ListInstances(gomock.Any(), listOpts).Times(2).Return(nil, apiErr) + + _, err := instances.ListCachedInstances(ctx) + require.ErrorIs(t, err, apiErr) + _, err = instances.ListCachedInstances(ctx) + require.ErrorIs(t, err, apiErr) + assert.True(t, instances.nodeCache.lastUpdate.IsZero(), "cache lastUpdate should remain zero after a failed load") + }) + + t.Run("concurrent reads and refreshes are race free", func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + c := mocks.NewMockClient(ctrl) + c.EXPECT().ListInstances(gomock.Any(), listOpts).AnyTimes().Return([]linodego.Instance{cachedInstance}, nil) + instances := NewInstances(c) + // A zero TTL makes every refresh replace the cache map, so reads overlap the replacement. + instances.nodeCache.ttl = 0 + + var wg sync.WaitGroup + for range 4 { + wg.Add(2) + go func() { + defer wg.Done() + for range 20 { + _, err := instances.ListCachedInstances(ctx) + assert.NoError(t, err) + } + }() + go func() { + defer wg.Done() + for range 20 { + assert.NoError(t, instances.nodeCache.refreshInstances(ctx, c)) + } + }() + } + wg.Wait() + }) +} + func TestInstanceShutdown(t *testing.T) { ctx := t.Context() ctrl := gomock.NewController(t)