Repository: linode/linode-cloud-controller-manager
Pull request #131 (2023-05-17 16:17:58)

Cache node listings instead of random lookups
### Rationale:

Since we call the individual "get instance" API call very frequently, we could instead call a single "list instances" once and reuse that for all the Linode lookups. This is essentially a client-side cache.

We won't reduce traffic between us and the Linode API (we'll probably increase it a bit), but we'll cut down the number of calls quite dramatically.

### General:

* [x] Have you removed all sensitive information, including but not limited to access keys and passwords?
* [x] Have you checked to ensure there aren't other open or closed [Pull Requests](../../pulls) for the same bug/feature/question?

### Pull Request Guidelines:

1. [x] Does your submission pass tests?
1. [ ] Have you added tests? 
1. [x] Are you addressing a single feature in this PR? 
1. [ ] Are your commits atomic, addressing one change per commit?
1. [x] Are you following the conventions of the language? 
1. [ ] Have you saved your large formatting changes for a different PR, so we can focus on your work?
1. [ ] Have you explained your rationale for why this feature is needed? 
1. [x] Have you linked your PR to an [open issue](https://blog.github.com/2013-05-14-closing-issues-via-pull-requests/)



scmeggitt: Should we add a test that adds multiple nodes to test that it works as well? 

Patch:
diff --git a/cloud/linode/common.go b/cloud/linode/common.go
index 221816b..c4562c3 100644
--- a/cloud/linode/common.go
+++ b/cloud/linode/common.go
@@ -1,14 +1,9 @@
 package linode
 
 import (
-	"context"
 	"fmt"
 	"strconv"
 	"strings"
-
-	"github.com/linode/linodego"
-	"k8s.io/apimachinery/pkg/types"
-	cloudprovider "k8s.io/cloud-provider"
 )
 
 const providerIDPrefix = "linode://"
@@ -31,34 +26,3 @@ func parseProviderID(providerID string) (int, error) {
 	}
 	return id, nil
 }
-
-func linodeFilterListOptions(targetLabel string) *linodego.ListOptions {
-	jsonFilter := fmt.Sprintf(`{"label":%q}`, targetLabel)
-	return linodego.NewListOptions(0, jsonFilter)
-}
-
-func linodeByName(ctx context.Context, client Client, nodeName types.NodeName) (*linodego.Instance, error) {
-	linodes, err := client.ListInstances(ctx, linodeFilterListOptions(string(nodeName)))
-	if err != nil {
-		return nil, err
-	}
-
-	if len(linodes) == 0 {
-		return nil, cloudprovider.InstanceNotFound
-	} else if len(linodes) > 1 {
-		return nil, fmt.Errorf("Multiple instances found with name %v", nodeName)
-	}
-
-	return &linodes[0], nil
-}
-
-func linodeByID(ctx context.Context, client Client, id int) (*linodego.Instance, error) {
-	instance, err := client.GetInstance(ctx, id)
-	if err != nil {
-		return nil, err
-	}
-	if instance == nil {
-		return nil, fmt.Errorf("linode not found with id %v", id)
-	}
-	return instance, nil
-}
diff --git a/cloud/linode/instances.go b/cloud/linode/instances.go
index 3402159..c803b32 100644
--- a/cloud/linode/instances.go
+++ b/cloud/linode/instances.go
@@ -3,8 +3,10 @@ package linode
 import (
 	"context"
 	"fmt"
-	"net/http"
+	"os"
 	"strconv"
+	"sync"
+	"time"
 
 	"github.com/linode/linode-cloud-controller-manager/sentry"
 	"github.com/linode/linodego"
@@ -13,12 +15,56 @@ import (
 	cloudprovider "k8s.io/cloud-provider"
 )
 
+type nodeCache struct {
+	sync.RWMutex
+	nodes      map[int]*linodego.Instance
+	lastUpdate time.Time
+	ttl        time.Duration
+}
+
+// refreshInstances conditionally loads all instances from the Linode API and caches them.
+// It does not refresh if the last update happened less than `nodeCache.ttl` ago.
+func (nc *nodeCache) refreshInstances(ctx context.Context, client Client) error {
+	nc.Lock()
+	defer nc.Unlock()
+
+	if time.Since(nc.lastUpdate) < nc.ttl {
+		return nil
+	}
+
+	instances, err := client.ListInstances(ctx, nil)
+	if err != nil {
+		return err
+	}
+	nc.nodes = make(map[int]*linodego.Instance)
+	for _, instance := range instances {
+		instance := instance
+		nc.nodes[instance.ID] = &instance
+	}
+	nc.lastUpdate = time.Now()
+
+	return nil
+}
+
 type instances struct {
 	client Client
+
+	nodeCache *nodeCache
 }
 
 func newInstances(client Client) cloudprovider.InstancesV2 {
-	return &instances{client}
+	var timeout int
+	if raw, ok := os.LookupEnv("LINODE_INSTANCE_CACHE_TTL"); ok {
+		timeout, _ = strconv.Atoi(raw)
+	}
+	if timeout == 0 {
+		timeout = 15
+	}
+
+	return &instances{client, &nodeCache{
+		nodes: make(map[int]*linodego.Instance),
+		ttl:   time.Duration(timeout) * time.Second,
+	}}
 }
 
 type instanceNoIPAddressesError struct {
@@ -29,7 +75,33 @@ func (e instanceNoIPAddressesError) Error() string {
 	return fmt.Sprintf("instance %d has no IP addresses", e.id)
 }
 
+func (i *instances) linodeByName(nodeName types.NodeName) (*linodego.Instance, error) {
+	i.nodeCache.RLock()
+	defer i.nodeCache.RUnlock()
+	for _, node := range i.nodeCache.nodes {
+		if node.Label == string(nodeName) {
+			return node, nil
+		}
+	}
+
+	return nil, cloudprovider.InstanceNotFound
+}
+
+func (i *instances) linodeByID(id int) (*linodego.Instance, error) {
+	i.nodeCache.RLock()
+	defer i.nodeCache.RUnlock()
+	instance, ok := i.nodeCache.nodes[id]
+	if !ok {
+		return nil, cloudprovider.InstanceNotFound
+	}
+	return instance, nil
+}
+
 func (i *instances) lookupLinode(ctx context.Context, node *v1.Node) (*linodego.Instance, error) {
+	if err := i.nodeCache.refreshInstances(ctx, i.client); err != nil {
+		return nil, err
+	}
+
 	providerID := node.Spec.ProviderID
 	nodeName := types.NodeName(node.Name)
 
@@ -44,16 +116,16 @@ func (i *instances) lookupLinode(ctx context.Context, node *v1.Node) (*linodego.
 		}
 		sentry.SetTag(ctx, "linode_id", strconv.Itoa(id))
 
-		return linodeByID(ctx, i.client, id)
+		return i.linodeByID(id)
 	}
 
-	return linodeByName(ctx, i.client, nodeName)
+	return i.linodeByName(nodeName)
 }
 
 func (i *instances) InstanceExists(ctx context.Context, node *v1.Node) (bool, error) {
 	ctx = sentry.SetHubOnContext(ctx)
 	if _, err := i.lookupLinode(ctx, node); err != nil {
-		if apiError, ok := err.(*linodego.Error); ok && apiError.Code == http.StatusNotFound {
+		if err == cloudprovider.InstanceNotFound {
 			return false, nil
 		}
 		sentry.CaptureError(ctx, err)


Test patch:
diff --git a/cloud/linode/instances_test.go b/cloud/linode/instances_test.go
index f42068a..acfba0a 100644
--- a/cloud/linode/instances_test.go
+++ b/cloud/linode/instances_test.go
@@ -2,10 +2,8 @@ package linode
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"net"
-	"net/http"
 	"strconv"
 	"testing"
 
@@ -33,23 +31,11 @@ func TestInstanceExists(t *testing.T) {
 	defer ctrl.Finish()
 
 	client := NewMockClient(ctrl)
-	instances := newInstances(client)
-
-	t.Run("should propagate generic api error", func(t *testing.T) {
-		node := nodeWithProviderID(providerIDPrefix + "123")
-		expectedErr := errors.New("some error")
-		client.EXPECT().GetInstance(gomock.Any(), 123).Times(1).Return(nil, expectedErr)
-
-		exists, err := instances.InstanceExists(ctx, node)
-		assert.ErrorIs(t, err, expectedErr)
-		assert.False(t, exists)
-	})
 
 	t.Run("should return false if linode does not exist (by providerID)", func(t *testing.T) {
+		instances := newInstances(client)
 		node := nodeWithProviderID(providerIDPrefix + "123")
-		client.EXPECT().GetInstance(gomock.Any(), 123).Times(1).Return(nil, &linodego.Error{
-			Code: http.StatusNotFound,
-		})
+		client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{}, nil)
 
 		exists, err := instances.InstanceExists(ctx, node)
 		assert.NoError(t, err)
@@ -57,13 +43,10 @@ func TestInstanceExists(t *testing.T) {
 	})
 
 	t.Run("should return false if linode does not exist (by name)", func(t *testing.T) {
+		instances := newInstances(client)
 		name := "some-name"
 		node := nodeWithName(name)
-		notFound := &linodego.Error{
-			Code: http.StatusNotFound,
-		}
-		filter := linodeFilterListOptions(name)
-		client.EXPECT().ListInstances(gomock.Any(), filter).Times(1).Return(nil, notFound)
+		client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{}, nil)
 
 		exists, err := instances.InstanceExists(ctx, node)
 		assert.NoError(t, err)
@@ -71,12 +54,13 @@ func TestInstanceExists(t *testing.T) {
 	})
 
 	t.Run("should return true if linode exists (by provider)", func(t *testing.T) {
+		instances := newInstances(client)
 		node := nodeWithProviderID(providerIDPrefix + "123")
-		client.EXPECT().GetInstance(gomock.Any(), 123).Times(1).Return(&linodego.Instance{
-			ID:     123,
-			Label:  "mock",
-			Region: "us-east",
-			Type:   "g6-standard-2",
+		client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{
+			{ID: 123,
+				Label:  "mock",
+				Region: "us-east",
+				Type:   "g6-standard-2"},
 		}, nil)
 
 		exists, err := instances.InstanceExists(ctx, node)
@@ -85,10 +69,11 @@ func TestInstanceExists(t *testing.T) {
 	})
 
 	t.Run("should return true if linode exists (by name)", func(t *testing.T) {
+		instances := newInstances(client)
 		name := "some-name"
 		node := nodeWithName(name)
 
-		client.EXPECT().ListInstances(gomock.Any(), linodeFilterListOptions(name)).Times(1).Return([]linodego.Instance{
+		client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{
 			{ID: 123, Label: name},
 		}, nil)
 
@@ -104,13 +89,12 @@ func TestMetadataRetrieval(t *testing.T) {
 	defer ctrl.Finish()
 
 	client := NewMockClient(ctrl)
-	instances := newInstances(client)
 
 	t.Run("errors when linode does not exist (by name)", func(t *testing.T) {
+		instances := newInstances(client)
 		name := "does-not-exist"
 		node := nodeWithName(name)
-		filter := linodeFilterListOptions(name)
-		client.EXPECT().ListInstances(gomock.Any(), filter).Times(1).Return([]linodego.Instance{}, nil)
+		client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{}, nil)
 
 		meta, err := instances.InstanceMetadata(ctx, node)
 		assert.ErrorIs(t, err, cloudprovider.InstanceNotFound)
@@ -118,18 +102,19 @@ func TestMetadataRetrieval(t *testing.T) {
 	})
 
 	t.Run("fails when linode does not exist (by provider)", func(t *testing.T) {
+		instances := newInstances(client)
 		id := 456302
 		providerID := providerIDPrefix + strconv.Itoa(id)
 		node := nodeWithProviderID(providerID)
-		getInstanceErr := &linodego.Error{Code: http.StatusNotFound}
-		client.EXPECT().GetInstance(gomock.Any(), id).Times(1).Return(nil, getInstanceErr)
+		client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{}, nil)
 		meta, err := instances.InstanceMetadata(ctx, node)
 
-		assert.ErrorIs(t, err, getInstanceErr)
+		assert.ErrorIs(t, err, cloudprovider.InstanceNotFound)
 		assert.Nil(t, meta)
 	})
 
 	t.Run("should return data when linode is found (by name)", func(t *testing.T) {
+		instances := newInstances(client)
 		id := 123
 		name := "mock-instance"
 		node := nodeWithName(name)
@@ -137,8 +122,7 @@ func TestMetadataRetrieval(t *testing.T) {
 		privateIPv4 := net.ParseIP("192.168.133.65")
 		linodeType := "g6-standard-1"
 		region := "us-east"
-		filter := linodeFilterListOptions(name)
-		client.EXPECT().ListInstances(gomock.Any(), filter).Times(1).Return([]linodego.Instance{
+		client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{
 			{ID: id, Label: name, Type: linodeType, Region: region, IPv4: []*net.IP{&publicIPv4, &privateIPv4}},
 		}, nil)
 
@@ -184,6 +168,7 @@ func TestMetadataRetrieval(t *testing.T) {
 
 	for _, test := range ipTests {
 		t.Run(fmt.Sprintf("addresses are retrieved - %s", test.name), func(t *testing.T) {
+			instances := newInstances(client)
 			id := 192910
 			name := "my-instance"
 			providerID := providerIDPrefix + strconv.Itoa(id)
@@ -200,8 +185,8 @@ func TestMetadataRetrieval(t *testing.T) {
 
 			linodeType := "g6-standard-1"
 			region := "us-east"
-			client.EXPECT().GetInstance(gomock.Any(), id).Times(1).Return(&linodego.Instance{
-				ID: id, Label: name, Type: linodeType, Region: region, IPv4: ips,
+			client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{
+				{ID: id, Label: name, Type: linodeType, Region: region, IPv4: ips},
 			}, nil)
 
 			meta, err := instances.InstanceMetadata(ctx, node)
@@ -225,19 +210,23 @@ func TestMalformedProviders(t *testing.T) {
 	defer ctrl.Finish()
 
 	client := NewMockClient(ctrl)
-	instances := newInstances(client)
 
 	t.Run("fails on malformed providerID", func(t *testing.T) {
+		instances := newInstances(client)
 		providerID := "bogus://bogus"
 		node := nodeWithProviderID(providerID)
+		client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{}, nil)
+
 		meta, err := instances.InstanceMetadata(ctx, node)
 		assert.ErrorIs(t, err, invalidProviderIDError{providerID})
 		assert.Nil(t, meta)
 	})
 
 	t.Run("fails on non-numeric providerID", func(t *testing.T) {
+		instances := newInstances(client)
 		providerID := providerIDPrefix + "abc"
 		node := nodeWithProviderID(providerID)
+		client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{}, nil)
 		meta, err := instances.InstanceMetadata(ctx, node)
 
 		assert.ErrorIs(t, err, invalidProviderIDError{providerID})
@@ -251,12 +240,12 @@ func TestInstanceShutdown(t *testing.T) {
 	defer ctrl.Finish()
 
 	client := NewMockClient(ctrl)
-	instances := newInstances(client)
 
 	t.Run("fails when instance not found (by provider)", func(t *testing.T) {
+		instances := newInstances(client)
 		id := 12345
 		node := nodeWithProviderID(providerIDPrefix + strconv.Itoa(id))
-		client.EXPECT().GetInstance(gomock.Any(), id).Times(1).Return(nil, linodego.Error{Code: http.StatusNotFound})
+		client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{}, nil)
 		shutdown, err := instances.InstanceShutdown(ctx, node)
 
 		assert.Error(t, err)
@@ -264,13 +253,10 @@ func TestInstanceShutdown(t *testing.T) {
 	})
 
 	t.Run("fails when instance not found (by name)", func(t *testing.T) {
+		instances := newInstances(client)
 		name := "some-name"
 		node := nodeWithName(name)
-		notFound := &linodego.Error{
-			Code: http.StatusNotFound,
-		}
-		filter := linodeFilterListOptions(name)
-		client.EXPECT().ListInstances(gomock.Any(), filter).Times(1).Return(nil, notFound)
+		client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{}, nil)
 		shutdown, err := instances.InstanceShutdown(ctx, node)
 
 		assert.Error(t, err)
@@ -278,10 +264,11 @@ func TestInstanceShutdown(t *testing.T) {
 	})
 
 	t.Run("returns true when instance is shut down", func(t *testing.T) {
+		instances := newInstances(client)
 		id := 12345
 		node := nodeWithProviderID(providerIDPrefix + strconv.Itoa(id))
-		client.EXPECT().GetInstance(gomock.Any(), id).Times(1).Return(&linodego.Instance{
-			ID: id, Label: "offline-linode", Status: linodego.InstanceOffline,
+		client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{
+			{ID: id, Label: "offline-linode", Status: linodego.InstanceOffline},
 		}, nil)
 		shutdown, err := instances.InstanceShutdown(ctx, node)
 
@@ -290,11 +277,11 @@ func TestInstanceShutdown(t *testing.T) {
 	})
 
 	t.Run("returns true when instance is shutting down", func(t *testing.T) {
+		instances := newInstances(client)
 		id := 12345
 		node := nodeWithProviderID(providerIDPrefix + strconv.Itoa(id))
-		client.EXPECT().GetInstance(gomock.Any(), id).Times(1).Return(&linodego.Instance{
-			ID: id, Label: "shutting-down-linode", Status: linodego.InstanceShuttingDown,
-		}, nil)
+		client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{
+			{ID: id, Label: "shutting-down-linode", Status: linodego.InstanceShuttingDown}}, nil)
 		shutdown, err := instances.InstanceShutdown(ctx, node)
 
 		assert.NoError(t, err)
@@ -302,10 +289,11 @@ func TestInstanceShutdown(t *testing.T) {
 	})
 
 	t.Run("returns false when instance is running", func(t *testing.T) {
+		instances := newInstances(client)
 		id := 12345
 		node := nodeWithProviderID(providerIDPrefix + strconv.Itoa(id))
-		client.EXPECT().GetInstance(gomock.Any(), id).Times(1).Return(&linodego.Instance{
-			ID: id, Label: "running-linode", Status: linodego.InstanceRunning,
+		client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{
+			{ID: id, Label: "running-linode", Status: linodego.InstanceRunning},
 		}, nil)
 		shutdown, err := instances.InstanceShutdown(ctx, node)