[fix] stop ListRoutes refreshing the instance cache

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
 	}

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)
 

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)
 	}

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)