Avoid unnecessary ListManagedInstances calls

Author: tetianakh

cluster-autoscaler/cloudprovider/gce/cache.go

@@ -181,6 +181,15 @@ func (gc *GceCache) GetMigInstancesUpdateTime(migRef GceRef) (time.Time, bool) {
 	return timestamp, found
 }
 
+// IsMigInstancesCacheEmpty returns true if instances cache for the given MIG is empty.
+func (gc *GceCache) IsMigInstancesCacheEmpty(migRef GceRef) bool {
+	gc.cacheMutex.Lock()
+	defer gc.cacheMutex.Unlock()
+
+	_, found := gc.instances[migRef]
+	return !found
+}
+
 // GetMigForInstance returns the cached MIG for instance GceRef
 func (gc *GceCache) GetMigForInstance(instanceRef GceRef) (GceRef, bool) {
 	gc.cacheMutex.Lock()

cluster-autoscaler/cloudprovider/gce/cache_test.go

@@ -18,6 +18,9 @@ package gce
 
 import (
 	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
 )
 
 func TestMachineCache(t *testing.T) {
@@ -113,3 +116,35 @@ func TestListManagedInstancesResultsCache(t *testing.T) {
 		t.Errorf("Expected listManagedInstancesResultsCache to be empty, but it still contains %d entries", cacheSize)
 	}
 }
+
+func TestMigInstancesCache(t *testing.T) {
+	c := NewGceCache()
+	migRef := GceRef{
+		Project: "project",
+		Zone:    "us-test1",
+		Name:    "mig",
+	}
+
+	// Pecondition: Cache is empty before it is set
+	assert.True(t, c.IsMigInstancesCacheEmpty(migRef), "Expected MIG instaces cache to be empty before SetMigInstances was called")
+
+	// When: set MIG instances cache
+	instances := make([]GceInstance, 0)
+	instances = append(instances, GceInstance{NumericId: 123})
+	instances = append(instances, GceInstance{NumericId: 456})
+	c.SetMigInstances(migRef, instances, time.Now())
+
+	// Then: can retrieve instances from cache by MIG reference
+	gotInstances, found := c.GetMigInstances(migRef)
+	assert.False(t, c.IsMigInstancesCacheEmpty(migRef), "Expected MIG instaces cache to not be empty after SetMigInstances was called")
+	assert.True(t, found, "Expected MIG insatcnes cache to be populated")
+	assert.Equal(t, gotInstances, instances)
+
+	// When: Invalidate the cache
+	c.InvalidateAllMigInstances()
+
+	// Then: Cache is empty
+	_, found = c.GetMigInstances(migRef)
+	assert.False(t, found, "Expected MIG instaces cache to be empty after InvalidateAllMigInstances was called")
+	assert.True(t, c.IsMigInstancesCacheEmpty(migRef))
+}

cluster-autoscaler/cloudprovider/gce/gce_manager_test.go

@@ -686,13 +686,15 @@ func TestGetMigForInstance(t *testing.T) {
 
 	setupTestDefaultPool(g, false)
 	g.cache.InvalidateAllMigBasenames()
+	g.cache.InvalidateAllInstancesToMig()
+	g.cache.InvalidateAllMigInstances()
 
 	server.On("handle", "/projects/project1/zones/us-central1-b/instanceGroupManagers").Return(
 		buildListInstanceGroupManagersResponse(
 			buildListInstanceGroupManagersResponsePart(defaultPoolMigName, zoneB, 7),
 			buildListInstanceGroupManagersResponsePart(extraPoolMigName, zoneB, 8),
 		)).Once()
-	server.On("handle", "/projects/project1/zones/us-central1-b/instanceGroupManagers/gke-cluster-1-default-pool/listManagedInstances").Return(buildFourRunningInstancesOnDefaultMigManagedInstancesResponse(zoneB)).Twice()
+	server.On("handle", "/projects/project1/zones/us-central1-b/instanceGroupManagers/gke-cluster-1-default-pool/listManagedInstances").Return(buildFourRunningInstancesOnDefaultMigManagedInstancesResponse(zoneB)).Once()
 	gceRef1 := GceRef{
 		Project: projectId,
 		Zone:    zoneB,

cluster-autoscaler/cloudprovider/gce/mig_info_provider.go

@@ -41,7 +41,7 @@ type MigInfoProvider interface {
 	GetMigForInstance(instanceRef GceRef) (Mig, error)
 	// RegenerateMigInstancesCache regenerates MIGs to instances mapping cache
 	RegenerateMigInstancesCache() error
-	// GetMigTargetSize returns target size for given MIG ref
+	// GetMigTargetSize returns target size of a MIG
 	GetMigTargetSize(migRef GceRef) (int64, error)
 	// GetMigActualSize returns current number of instances in a given MIG
 	GetMigActualSize(migRef GceRef) (int64, error)
@@ -112,6 +112,7 @@ func (c *cachingMigInfoProvider) GetMigInstances(migRef GceRef) ([]GceInstance,
 		return instances, nil
 	}
 
+	// MIG is not in the cache.
 	err := c.fillMigInstances(migRef)
 	if err != nil {
 		return nil, err
@@ -135,16 +136,24 @@ func (c *cachingMigInfoProvider) GetMigForInstance(instanceRef GceRef) (Mig, err
 		return nil, nil
 	}
 
+	// Cache is cleared every loop.
+	// If it's not empty, it's been refreshed this loop, and we don't want to refresh it again.
+	if !c.cache.IsMigInstancesCacheEmpty(mig.GceRef()) {
+		c.cache.MarkInstanceMigUnknown(instanceRef)
+		return nil, nil
+	}
+
 	err = c.fillMigInstances(mig.GceRef())
 	if err != nil {
 		return nil, err
 	}
-
+	// Check in the cache again after it's been refilled
 	mig, found, err = c.getCachedMigForInstance(instanceRef)
 	if !found {
 		c.cache.MarkInstanceMigUnknown(instanceRef)
 	}
 	return mig, err
+
 }
 
 func (c *cachingMigInfoProvider) getCachedMigForInstance(instanceRef GceRef) (Mig, bool, error) {

cluster-autoscaler/cloudprovider/gce/mig_info_provider_test.go

@@ -1551,24 +1551,14 @@ func TestGetMigMachineType(t *testing.T) {
 	}
 }
 
-func TestMultipleGetMigInstanceCallsLimited(t *testing.T) {
+func TestMultipleFillMigInstancesCallsLimited(t *testing.T) {
 	mig := &gceMig{
 		gceRef: GceRef{
 			Project: "project",
 			Zone:    "zone",
 			Name:    "base-instance-name",
 		},
 	}
-	instance := GceInstance{
-		Instance: cloudprovider.Instance{Id: "gce://project/zone/base-instance-name-abcd"}, NumericId: 1111,
-	}
-	instanceRef, err := GceRefFromProviderId(instance.Id)
-	assert.Nil(t, err)
-	instance2 := GceInstance{
-		Instance: cloudprovider.Instance{Id: "gce://project/zone/base-instance-name-abcd2"}, NumericId: 222,
-	}
-	instanceRef2, err := GceRefFromProviderId(instance2.Id)
-	assert.Nil(t, err)
 	now := time.Now()
 	for name, tc := range map[string]struct {
 		refreshRateDuration              time.Duration
@@ -1617,10 +1607,10 @@ func TestMultipleGetMigInstanceCallsLimited(t *testing.T) {
 				timeProvider:                   ft,
 			}
 			ft.now = tc.firstCallTime
-			_, err = provider.GetMigForInstance(instanceRef)
+			err := provider.fillMigInstances(mig.GceRef())
 			assert.NoError(t, err)
 			ft.now = tc.secondCallTime
-			_, err = provider.GetMigForInstance(instanceRef2)
+			err = provider.fillMigInstances(mig.GceRef())
 			assert.NoError(t, err)
 			assert.Equal(t, tc.expectedCallsToFetchMigInstances, callCounter[mig.GceRef()])
 		})