From 0673eb00bf63bf86e0dc8162d26e5f3757d12237 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Miela=C5=84czuk?= Date: Sun, 28 Jan 2024 00:41:30 +0000 Subject: [PATCH] Introduce GceInstance that extends cloudprovider.Instance with NumericId. --- .../gce/autoscaling_gce_client.go | 29 ++-- .../gce/autoscaling_gce_client_test.go | 125 ++++++++++++----- cluster-autoscaler/cloudprovider/gce/cache.go | 14 +- .../cloudprovider/gce/gce_cloud_provider.go | 10 +- .../gce/gce_cloud_provider_test.go | 64 +++++---- .../cloudprovider/gce/gce_manager.go | 4 +- .../cloudprovider/gce/gce_manager_test.go | 2 +- .../cloudprovider/gce/mig_info_provider.go | 5 +- .../gce/mig_info_provider_test.go | 127 +++++++++--------- 9 files changed, 236 insertions(+), 144 deletions(-) diff --git a/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client.go b/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client.go index 39c5364e7b61..a477b7273fe2 100644 --- a/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client.go +++ b/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client.go @@ -88,6 +88,12 @@ var ( } ) +// GceInstance extends cloudprovider.Instance with GCE specific numeric id. +type GceInstance struct { + cloudprovider.Instance + NumericId uint64 +} + // AutoscalingGceClient is used for communicating with GCE API. type AutoscalingGceClient interface { // reading resources @@ -96,7 +102,7 @@ type AutoscalingGceClient interface { FetchAllMigs(zone string) ([]*gce.InstanceGroupManager, error) FetchMigTargetSize(GceRef) (int64, error) FetchMigBasename(GceRef) (string, error) - FetchMigInstances(GceRef) ([]cloudprovider.Instance, error) + FetchMigInstances(GceRef) ([]GceInstance, error) FetchMigTemplateName(migRef GceRef) (string, error) FetchMigTemplate(migRef GceRef, templateName string) (*gce.InstanceTemplate, error) FetchMigsWithName(zone string, filter *regexp.Regexp) ([]string, error) @@ -306,7 +312,7 @@ func (client *autoscalingGceClientV1) DeleteInstances(migRef GceRef, instances [ return client.waitForOp(op, migRef.Project, migRef.Zone, true) } -func (client *autoscalingGceClientV1) FetchMigInstances(migRef GceRef) ([]cloudprovider.Instance, error) { +func (client *autoscalingGceClientV1) FetchMigInstances(migRef GceRef) ([]GceInstance, error) { registerRequest("instance_group_managers", "list_managed_instances") b := newInstanceListBuilder(migRef) err := client.gceService.InstanceGroupManagers.ListManagedInstances(migRef.Project, migRef.Zone, migRef.Name).Pages(context.Background(), b.loadPage) @@ -321,7 +327,7 @@ type instanceListBuilder struct { migRef GceRef errorCodeCounts map[string]int errorLoggingQuota *klogx.Quota - infos []cloudprovider.Instance + infos []GceInstance } func newInstanceListBuilder(migRef GceRef) *instanceListBuilder { @@ -334,7 +340,7 @@ func newInstanceListBuilder(migRef GceRef) *instanceListBuilder { func (i *instanceListBuilder) loadPage(page *gce.InstanceGroupManagersListManagedInstancesResponse) error { if i.infos == nil { - i.infos = make([]cloudprovider.Instance, 0, len(page.ManagedInstances)) + i.infos = make([]GceInstance, 0, len(page.ManagedInstances)) } for _, gceInstance := range page.ManagedInstances { ref, err := ParseInstanceUrlRef(gceInstance.Instance) @@ -348,12 +354,15 @@ func (i *instanceListBuilder) loadPage(page *gce.InstanceGroupManagersListManage return nil } -func (i *instanceListBuilder) gceInstanceToInstance(ref GceRef, gceInstance *gce.ManagedInstance) cloudprovider.Instance { - instance := cloudprovider.Instance{ - Id: ref.ToProviderId(), - Status: &cloudprovider.InstanceStatus{ - State: getInstanceState(gceInstance.CurrentAction), +func (i *instanceListBuilder) gceInstanceToInstance(ref GceRef, gceInstance *gce.ManagedInstance) GceInstance { + instance := GceInstance{ + Instance: cloudprovider.Instance{ + Id: ref.ToProviderId(), + Status: &cloudprovider.InstanceStatus{ + State: getInstanceState(gceInstance.CurrentAction), + }, }, + NumericId: gceInstance.Id, } if instance.Status.State != cloudprovider.InstanceCreating { @@ -395,7 +404,7 @@ func (i *instanceListBuilder) gceInstanceToInstance(ref GceRef, gceInstance *gce return instance } -func (i *instanceListBuilder) build() []cloudprovider.Instance { +func (i *instanceListBuilder) build() []GceInstance { klogx.V(4).Over(i.errorLoggingQuota).Infof("Got %v other GCE instances being created with lastAttemptErrors", -i.errorLoggingQuota.Left()) if len(i.errorCodeCounts) > 0 { klog.Warningf("Spotted following instance creation error codes: %#v", i.errorCodeCounts) diff --git a/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client_test.go b/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client_test.go index 617f74036fe5..ad44678404c2 100644 --- a/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client_test.go +++ b/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client_test.go @@ -237,13 +237,14 @@ func TestFetchMigInstancesInstanceUrlHandling(t *testing.T) { name string lmiResponse gce_api.InstanceGroupManagersListManagedInstancesResponse lmiPageResponses map[string]gce_api.InstanceGroupManagersListManagedInstancesResponse - wantInstances []cloudprovider.Instance + wantInstances []GceInstance }{ { name: "all instances good", lmiResponse: gce_api.InstanceGroupManagersListManagedInstancesResponse{ ManagedInstances: []*gce_api.ManagedInstance{ { + Id: 2, Instance: fmt.Sprintf(goodInstanceUrlTempl, 2), CurrentAction: "CREATING", LastAttempt: &gce_api.ManagedInstanceLastAttempt{ @@ -251,6 +252,7 @@ func TestFetchMigInstancesInstanceUrlHandling(t *testing.T) { }, }, { + Id: 42, Instance: fmt.Sprintf(goodInstanceUrlTempl, 42), CurrentAction: "CREATING", LastAttempt: &gce_api.ManagedInstanceLastAttempt{ @@ -259,14 +261,20 @@ func TestFetchMigInstancesInstanceUrlHandling(t *testing.T) { }, }, }, - wantInstances: []cloudprovider.Instance{ + wantInstances: []GceInstance{ { - Id: "gce://myprojid/myzone/myinst_2", - Status: &cloudprovider.InstanceStatus{State: cloudprovider.InstanceCreating}, + Instance: cloudprovider.Instance{ + Id: "gce://myprojid/myzone/myinst_2", + Status: &cloudprovider.InstanceStatus{State: cloudprovider.InstanceCreating}, + }, + NumericId: 2, }, { - Id: "gce://myprojid/myzone/myinst_42", - Status: &cloudprovider.InstanceStatus{State: cloudprovider.InstanceCreating}, + Instance: cloudprovider.Instance{ + Id: "gce://myprojid/myzone/myinst_42", + Status: &cloudprovider.InstanceStatus{State: cloudprovider.InstanceCreating}, + }, + NumericId: 42, }, }, }, @@ -275,6 +283,7 @@ func TestFetchMigInstancesInstanceUrlHandling(t *testing.T) { lmiResponse: gce_api.InstanceGroupManagersListManagedInstancesResponse{ ManagedInstances: []*gce_api.ManagedInstance{ { + Id: 2, Instance: fmt.Sprintf(goodInstanceUrlTempl, 2), CurrentAction: "CREATING", LastAttempt: &gce_api.ManagedInstanceLastAttempt{ @@ -282,6 +291,7 @@ func TestFetchMigInstancesInstanceUrlHandling(t *testing.T) { }, }, { + Id: 42, Instance: fmt.Sprintf(goodInstanceUrlTempl, 42), CurrentAction: "CREATING", LastAttempt: &gce_api.ManagedInstanceLastAttempt{ @@ -295,6 +305,7 @@ func TestFetchMigInstancesInstanceUrlHandling(t *testing.T) { "foo": { ManagedInstances: []*gce_api.ManagedInstance{ { + Id: 123, Instance: fmt.Sprintf(goodInstanceUrlTempl, 123), CurrentAction: "CREATING", LastAttempt: &gce_api.ManagedInstanceLastAttempt{ @@ -302,6 +313,7 @@ func TestFetchMigInstancesInstanceUrlHandling(t *testing.T) { }, }, { + Id: 456, Instance: fmt.Sprintf(goodInstanceUrlTempl, 456), CurrentAction: "CREATING", LastAttempt: &gce_api.ManagedInstanceLastAttempt{ @@ -311,22 +323,34 @@ func TestFetchMigInstancesInstanceUrlHandling(t *testing.T) { }, }, }, - wantInstances: []cloudprovider.Instance{ + wantInstances: []GceInstance{ { - Id: "gce://myprojid/myzone/myinst_2", - Status: &cloudprovider.InstanceStatus{State: cloudprovider.InstanceCreating}, + Instance: cloudprovider.Instance{ + Id: "gce://myprojid/myzone/myinst_2", + Status: &cloudprovider.InstanceStatus{State: cloudprovider.InstanceCreating}, + }, + NumericId: 2, }, { - Id: "gce://myprojid/myzone/myinst_42", - Status: &cloudprovider.InstanceStatus{State: cloudprovider.InstanceCreating}, + Instance: cloudprovider.Instance{ + Id: "gce://myprojid/myzone/myinst_42", + Status: &cloudprovider.InstanceStatus{State: cloudprovider.InstanceCreating}, + }, + NumericId: 42, }, { - Id: "gce://myprojid/myzone/myinst_123", - Status: &cloudprovider.InstanceStatus{State: cloudprovider.InstanceCreating}, + Instance: cloudprovider.Instance{ + Id: "gce://myprojid/myzone/myinst_123", + Status: &cloudprovider.InstanceStatus{State: cloudprovider.InstanceCreating}, + }, + NumericId: 123, }, { - Id: "gce://myprojid/myzone/myinst_456", - Status: &cloudprovider.InstanceStatus{State: cloudprovider.InstanceCreating}, + Instance: cloudprovider.Instance{ + Id: "gce://myprojid/myzone/myinst_456", + Status: &cloudprovider.InstanceStatus{State: cloudprovider.InstanceCreating}, + }, + NumericId: 456, }, }, }, @@ -335,6 +359,7 @@ func TestFetchMigInstancesInstanceUrlHandling(t *testing.T) { lmiResponse: gce_api.InstanceGroupManagersListManagedInstancesResponse{ ManagedInstances: []*gce_api.ManagedInstance{ { + Id: 2, Instance: fmt.Sprintf(goodInstanceUrlTempl, 2), CurrentAction: "CREATING", LastAttempt: &gce_api.ManagedInstanceLastAttempt{ @@ -342,6 +367,7 @@ func TestFetchMigInstancesInstanceUrlHandling(t *testing.T) { }, }, { + Id: 42, Instance: fmt.Sprintf(goodInstanceUrlTempl, 42), CurrentAction: "CREATING", LastAttempt: &gce_api.ManagedInstanceLastAttempt{ @@ -355,6 +381,7 @@ func TestFetchMigInstancesInstanceUrlHandling(t *testing.T) { "foo": { ManagedInstances: []*gce_api.ManagedInstance{ { + Id: 123, Instance: fmt.Sprintf(goodInstanceUrlTempl, 123), CurrentAction: "CREATING", LastAttempt: &gce_api.ManagedInstanceLastAttempt{ @@ -362,6 +389,7 @@ func TestFetchMigInstancesInstanceUrlHandling(t *testing.T) { }, }, { + Id: 456, Instance: fmt.Sprintf(goodInstanceUrlTempl, 456), CurrentAction: "CREATING", LastAttempt: &gce_api.ManagedInstanceLastAttempt{ @@ -374,6 +402,7 @@ func TestFetchMigInstancesInstanceUrlHandling(t *testing.T) { "bar": { ManagedInstances: []*gce_api.ManagedInstance{ { + Id: 789, Instance: fmt.Sprintf(goodInstanceUrlTempl, 789), CurrentAction: "CREATING", LastAttempt: &gce_api.ManagedInstanceLastAttempt{ @@ -381,6 +410,7 @@ func TestFetchMigInstancesInstanceUrlHandling(t *testing.T) { }, }, { + Id: 666, Instance: fmt.Sprintf(goodInstanceUrlTempl, 666), CurrentAction: "CREATING", LastAttempt: &gce_api.ManagedInstanceLastAttempt{ @@ -390,30 +420,48 @@ func TestFetchMigInstancesInstanceUrlHandling(t *testing.T) { }, }, }, - wantInstances: []cloudprovider.Instance{ + wantInstances: []GceInstance{ { - Id: "gce://myprojid/myzone/myinst_2", - Status: &cloudprovider.InstanceStatus{State: cloudprovider.InstanceCreating}, + Instance: cloudprovider.Instance{ + Id: "gce://myprojid/myzone/myinst_2", + Status: &cloudprovider.InstanceStatus{State: cloudprovider.InstanceCreating}, + }, + NumericId: 2, }, { - Id: "gce://myprojid/myzone/myinst_42", - Status: &cloudprovider.InstanceStatus{State: cloudprovider.InstanceCreating}, + Instance: cloudprovider.Instance{ + Id: "gce://myprojid/myzone/myinst_42", + Status: &cloudprovider.InstanceStatus{State: cloudprovider.InstanceCreating}, + }, + NumericId: 42, }, { - Id: "gce://myprojid/myzone/myinst_123", - Status: &cloudprovider.InstanceStatus{State: cloudprovider.InstanceCreating}, + Instance: cloudprovider.Instance{ + Id: "gce://myprojid/myzone/myinst_123", + Status: &cloudprovider.InstanceStatus{State: cloudprovider.InstanceCreating}, + }, + NumericId: 123, }, { - Id: "gce://myprojid/myzone/myinst_456", - Status: &cloudprovider.InstanceStatus{State: cloudprovider.InstanceCreating}, + Instance: cloudprovider.Instance{ + Id: "gce://myprojid/myzone/myinst_456", + Status: &cloudprovider.InstanceStatus{State: cloudprovider.InstanceCreating}, + }, + NumericId: 456, }, { - Id: "gce://myprojid/myzone/myinst_789", - Status: &cloudprovider.InstanceStatus{State: cloudprovider.InstanceCreating}, + Instance: cloudprovider.Instance{ + Id: "gce://myprojid/myzone/myinst_789", + Status: &cloudprovider.InstanceStatus{State: cloudprovider.InstanceCreating}, + }, + NumericId: 789, }, { - Id: "gce://myprojid/myzone/myinst_666", - Status: &cloudprovider.InstanceStatus{State: cloudprovider.InstanceCreating}, + Instance: cloudprovider.Instance{ + Id: "gce://myprojid/myzone/myinst_666", + Status: &cloudprovider.InstanceStatus{State: cloudprovider.InstanceCreating}, + }, + NumericId: 666, }, }, }, @@ -422,6 +470,7 @@ func TestFetchMigInstancesInstanceUrlHandling(t *testing.T) { lmiResponse: gce_api.InstanceGroupManagersListManagedInstancesResponse{ ManagedInstances: []*gce_api.ManagedInstance{ { + Id: 99999, Instance: badInstanceUrl, CurrentAction: "CREATING", LastAttempt: &gce_api.ManagedInstanceLastAttempt{ @@ -429,6 +478,7 @@ func TestFetchMigInstancesInstanceUrlHandling(t *testing.T) { }, }, { + Id: 42, Instance: fmt.Sprintf(goodInstanceUrlTempl, 42), CurrentAction: "CREATING", LastAttempt: &gce_api.ManagedInstanceLastAttempt{ @@ -437,10 +487,13 @@ func TestFetchMigInstancesInstanceUrlHandling(t *testing.T) { }, }, }, - wantInstances: []cloudprovider.Instance{ + wantInstances: []GceInstance{ { - Id: "gce://myprojid/myzone/myinst_42", - Status: &cloudprovider.InstanceStatus{State: cloudprovider.InstanceCreating}, + Instance: cloudprovider.Instance{ + Id: "gce://myprojid/myzone/myinst_42", + Status: &cloudprovider.InstanceStatus{State: cloudprovider.InstanceCreating}, + }, + NumericId: 42, }, }, }, @@ -456,6 +509,7 @@ func TestFetchMigInstancesInstanceUrlHandling(t *testing.T) { }, }, { + Id: 42, Instance: fmt.Sprintf(goodInstanceUrlTempl, 42), CurrentAction: "CREATING", LastAttempt: &gce_api.ManagedInstanceLastAttempt{ @@ -464,10 +518,13 @@ func TestFetchMigInstancesInstanceUrlHandling(t *testing.T) { }, }, }, - wantInstances: []cloudprovider.Instance{ + wantInstances: []GceInstance{ { - Id: "gce://myprojid/myzone/myinst_42", - Status: &cloudprovider.InstanceStatus{State: cloudprovider.InstanceCreating}, + Instance: cloudprovider.Instance{ + Id: "gce://myprojid/myzone/myinst_42", + Status: &cloudprovider.InstanceStatus{State: cloudprovider.InstanceCreating}, + }, + NumericId: 42, }, }, }, diff --git a/cluster-autoscaler/cloudprovider/gce/cache.go b/cluster-autoscaler/cloudprovider/gce/cache.go index 818f5d7aada9..0b9df2500268 100644 --- a/cluster-autoscaler/cloudprovider/gce/cache.go +++ b/cluster-autoscaler/cloudprovider/gce/cache.go @@ -57,7 +57,7 @@ type GceCache struct { // Cache content. migs map[GceRef]Mig - instances map[GceRef][]cloudprovider.Instance + instances map[GceRef][]GceInstance instancesUpdateTime map[GceRef]time.Time instancesToMig map[GceRef]GceRef instancesFromUnknownMig map[GceRef]bool @@ -74,7 +74,7 @@ type GceCache struct { func NewGceCache() *GceCache { return &GceCache{ migs: map[GceRef]Mig{}, - instances: map[GceRef][]cloudprovider.Instance{}, + instances: map[GceRef][]GceInstance{}, instancesUpdateTime: map[GceRef]time.Time{}, instancesToMig: map[GceRef]GceRef{}, instancesFromUnknownMig: map[GceRef]bool{}, @@ -144,7 +144,7 @@ func (gc *GceCache) GetMigs() []Mig { } // GetMigInstances returns the cached instances for a given MIG GceRef -func (gc *GceCache) GetMigInstances(migRef GceRef) ([]cloudprovider.Instance, bool) { +func (gc *GceCache) GetMigInstances(migRef GceRef) ([]GceInstance, bool) { gc.cacheMutex.Lock() defer gc.cacheMutex.Unlock() @@ -152,7 +152,7 @@ func (gc *GceCache) GetMigInstances(migRef GceRef) ([]cloudprovider.Instance, bo if found { klog.V(5).Infof("Instances cache hit for %s", migRef) } - return append([]cloudprovider.Instance{}, instances...), found + return append([]GceInstance{}, instances...), found } // GetMigInstancesUpdateTime returns the timestamp when the cached instances @@ -194,12 +194,12 @@ func (gc *GceCache) IsMigUnknownForInstance(instanceRef GceRef) bool { } // SetMigInstances sets instances for a given Mig ref -func (gc *GceCache) SetMigInstances(migRef GceRef, instances []cloudprovider.Instance, timeNow time.Time) error { +func (gc *GceCache) SetMigInstances(migRef GceRef, instances []GceInstance, timeNow time.Time) error { gc.cacheMutex.Lock() defer gc.cacheMutex.Unlock() gc.removeMigInstances(migRef) - gc.instances[migRef] = append([]cloudprovider.Instance{}, instances...) + gc.instances[migRef] = append([]GceInstance{}, instances...) gc.instancesUpdateTime[migRef] = timeNow for _, instance := range instances { instanceRef, err := GceRefFromProviderId(instance.Id) @@ -227,7 +227,7 @@ func (gc *GceCache) InvalidateAllMigInstances() { defer gc.cacheMutex.Unlock() klog.V(5).Infof("Mig instances cache invalidated") - gc.instances = make(map[GceRef][]cloudprovider.Instance) + gc.instances = make(map[GceRef][]GceInstance) gc.instancesUpdateTime = make(map[GceRef]time.Time) } diff --git a/cluster-autoscaler/cloudprovider/gce/gce_cloud_provider.go b/cluster-autoscaler/cloudprovider/gce/gce_cloud_provider.go index fa5c8a625883..d159c21d9f13 100644 --- a/cluster-autoscaler/cloudprovider/gce/gce_cloud_provider.go +++ b/cluster-autoscaler/cloudprovider/gce/gce_cloud_provider.go @@ -318,7 +318,15 @@ func (mig *gceMig) Debug() string { // Nodes returns a list of all nodes that belong to this node group. func (mig *gceMig) Nodes() ([]cloudprovider.Instance, error) { - return mig.gceManager.GetMigNodes(mig) + gceInstances, err := mig.gceManager.GetMigNodes(mig) + if err != nil { + return nil, err + } + instances := make([]cloudprovider.Instance, len(gceInstances), len(gceInstances)) + for i, inst := range gceInstances { + instances[i] = inst.Instance + } + return instances, nil } // Exist checks if the node group really exists on the cloud provider side. diff --git a/cluster-autoscaler/cloudprovider/gce/gce_cloud_provider_test.go b/cluster-autoscaler/cloudprovider/gce/gce_cloud_provider_test.go index 9e40a2d1e921..964e65b1744b 100644 --- a/cluster-autoscaler/cloudprovider/gce/gce_cloud_provider_test.go +++ b/cluster-autoscaler/cloudprovider/gce/gce_cloud_provider_test.go @@ -58,9 +58,9 @@ func (m *gceManagerMock) GetMigForInstance(instance GceRef) (Mig, error) { return args.Get(0).(*gceMig), args.Error(1) } -func (m *gceManagerMock) GetMigNodes(mig Mig) ([]cloudprovider.Instance, error) { +func (m *gceManagerMock) GetMigNodes(mig Mig) ([]GceInstance, error) { args := m.Called(mig) - return args.Get(0).([]cloudprovider.Instance), args.Error(1) + return args.Get(0).([]GceInstance), args.Error(1) } func (m *gceManagerMock) Refresh() error { @@ -297,18 +297,24 @@ func TestMig(t *testing.T) { // Test DecreaseTargetSize. gceManagerMock.On("GetMigSize", mock.AnythingOfType("*gce.gceMig")).Return(int64(3), nil).Once() gceManagerMock.On("GetMigNodes", mock.AnythingOfType("*gce.gceMig")).Return( - []cloudprovider.Instance{ + []GceInstance{ { - Id: "gce://project1/us-central1-b/gke-cluster-1-default-pool-f7607aac-9j4g", - Status: &cloudprovider.InstanceStatus{ - State: cloudprovider.InstanceRunning, + Instance: cloudprovider.Instance{ + Id: "gce://project1/us-central1-b/gke-cluster-1-default-pool-f7607aac-9j4g", + Status: &cloudprovider.InstanceStatus{ + State: cloudprovider.InstanceRunning, + }, }, + NumericId: 1, }, { - Id: "gce://project1/us-central1-b/gke-cluster-1-default-pool-f7607aac-dck1", - Status: &cloudprovider.InstanceStatus{ - State: cloudprovider.InstanceRunning, + Instance: cloudprovider.Instance{ + Id: "gce://project1/us-central1-b/gke-cluster-1-default-pool-f7607aac-dck1", + Status: &cloudprovider.InstanceStatus{ + State: cloudprovider.InstanceRunning, + }, }, + NumericId: 222, }, }, nil).Once() gceManagerMock.On("SetMigSize", mock.AnythingOfType("*gce.gceMig"), int64(2)).Return(nil).Once() @@ -324,18 +330,24 @@ func TestMig(t *testing.T) { // Test DecreaseTargetSize - fail on deleting existing nodes. gceManagerMock.On("GetMigSize", mock.AnythingOfType("*gce.gceMig")).Return(int64(3), nil).Once() gceManagerMock.On("GetMigNodes", mock.AnythingOfType("*gce.gceMig")).Return( - []cloudprovider.Instance{ + []GceInstance{ { - Id: "gce://project1/us-central1-b/gke-cluster-1-default-pool-f7607aac-9j4g", - Status: &cloudprovider.InstanceStatus{ - State: cloudprovider.InstanceRunning, + Instance: cloudprovider.Instance{ + Id: "gce://project1/us-central1-b/gke-cluster-1-default-pool-f7607aac-9j4g", + Status: &cloudprovider.InstanceStatus{ + State: cloudprovider.InstanceRunning, + }, }, + NumericId: 1111, }, { - Id: "gce://project1/us-central1-b/gke-cluster-1-default-pool-f7607aac-dck1", - Status: &cloudprovider.InstanceStatus{ - State: cloudprovider.InstanceRunning, + Instance: cloudprovider.Instance{ + Id: "gce://project1/us-central1-b/gke-cluster-1-default-pool-f7607aac-dck1", + Status: &cloudprovider.InstanceStatus{ + State: cloudprovider.InstanceRunning, + }, }, + NumericId: 333, }, }, nil).Once() err = mig1.DecreaseTargetSize(-2) @@ -395,18 +407,24 @@ func TestMig(t *testing.T) { // Test Nodes. gceManagerMock.On("GetMigNodes", mock.AnythingOfType("*gce.gceMig")).Return( - []cloudprovider.Instance{ + []GceInstance{ { - Id: "gce://project1/us-central1-b/gke-cluster-1-default-pool-f7607aac-9j4g", - Status: &cloudprovider.InstanceStatus{ - State: cloudprovider.InstanceRunning, + Instance: cloudprovider.Instance{ + Id: "gce://project1/us-central1-b/gke-cluster-1-default-pool-f7607aac-9j4g", + Status: &cloudprovider.InstanceStatus{ + State: cloudprovider.InstanceRunning, + }, }, + NumericId: 1, }, { - Id: "gce://project1/us-central1-b/gke-cluster-1-default-pool-f7607aac-dck1", - Status: &cloudprovider.InstanceStatus{ - State: cloudprovider.InstanceRunning, + Instance: cloudprovider.Instance{ + Id: "gce://project1/us-central1-b/gke-cluster-1-default-pool-f7607aac-dck1", + Status: &cloudprovider.InstanceStatus{ + State: cloudprovider.InstanceRunning, + }, }, + NumericId: 2, }, }, nil).Once() nodes, err := mig1.Nodes() diff --git a/cluster-autoscaler/cloudprovider/gce/gce_manager.go b/cluster-autoscaler/cloudprovider/gce/gce_manager.go index a7f565f6abf8..db1329037e27 100644 --- a/cluster-autoscaler/cloudprovider/gce/gce_manager.go +++ b/cluster-autoscaler/cloudprovider/gce/gce_manager.go @@ -82,7 +82,7 @@ type GceManager interface { // GetMigs returns list of registered MIGs. GetMigs() []Mig // GetMigNodes returns mig nodes. - GetMigNodes(mig Mig) ([]cloudprovider.Instance, error) + GetMigNodes(mig Mig) ([]GceInstance, error) // GetMigForInstance returns MIG to which the given instance belongs. GetMigForInstance(instance GceRef) (Mig, error) // GetMigTemplateNode returns a template node for MIG. @@ -286,7 +286,7 @@ func (m *gceManagerImpl) GetMigForInstance(instance GceRef) (Mig, error) { } // GetMigNodes returns mig nodes. -func (m *gceManagerImpl) GetMigNodes(mig Mig) ([]cloudprovider.Instance, error) { +func (m *gceManagerImpl) GetMigNodes(mig Mig) ([]GceInstance, error) { return m.migInfoProvider.GetMigInstances(mig.GceRef()) } diff --git a/cluster-autoscaler/cloudprovider/gce/gce_manager_test.go b/cluster-autoscaler/cloudprovider/gce/gce_manager_test.go index 609e6af91f06..953c9789bf28 100644 --- a/cluster-autoscaler/cloudprovider/gce/gce_manager_test.go +++ b/cluster-autoscaler/cloudprovider/gce/gce_manager_test.go @@ -333,7 +333,7 @@ func newTestGceManager(t *testing.T, testServerURL string, regional bool) *gceMa cache := &GceCache{ migs: make(map[GceRef]Mig), - instances: make(map[GceRef][]cloudprovider.Instance), + instances: make(map[GceRef][]GceInstance), instancesUpdateTime: make(map[GceRef]time.Time), instancesToMig: make(map[GceRef]GceRef), instancesFromUnknownMig: make(map[GceRef]bool), diff --git a/cluster-autoscaler/cloudprovider/gce/mig_info_provider.go b/cluster-autoscaler/cloudprovider/gce/mig_info_provider.go index abee9c4660ce..b96a477bf1af 100644 --- a/cluster-autoscaler/cloudprovider/gce/mig_info_provider.go +++ b/cluster-autoscaler/cloudprovider/gce/mig_info_provider.go @@ -26,7 +26,6 @@ import ( "time" gce "google.golang.org/api/compute/v1" - "k8s.io/autoscaler/cluster-autoscaler/cloudprovider" "k8s.io/client-go/util/workqueue" klog "k8s.io/klog/v2" ) @@ -34,7 +33,7 @@ import ( // MigInfoProvider allows obtaining information about MIGs type MigInfoProvider interface { // GetMigInstances returns instances for a given MIG ref - GetMigInstances(migRef GceRef) ([]cloudprovider.Instance, error) + GetMigInstances(migRef GceRef) ([]GceInstance, error) // GetMigForInstance returns MIG ref for a given instance GetMigForInstance(instanceRef GceRef) (Mig, error) // RegenerateMigInstancesCache regenerates MIGs to instances mapping cache @@ -89,7 +88,7 @@ func NewCachingMigInfoProvider(cache *GceCache, migLister MigLister, gceClient A } // GetMigInstances returns instances for a given MIG ref -func (c *cachingMigInfoProvider) GetMigInstances(migRef GceRef) ([]cloudprovider.Instance, error) { +func (c *cachingMigInfoProvider) GetMigInstances(migRef GceRef) ([]GceInstance, error) { instances, found := c.cache.GetMigInstances(migRef) if found { return instances, nil diff --git a/cluster-autoscaler/cloudprovider/gce/mig_info_provider_test.go b/cluster-autoscaler/cloudprovider/gce/mig_info_provider_test.go index adf2240329fb..6c2ea6dc75e6 100644 --- a/cluster-autoscaler/cloudprovider/gce/mig_info_provider_test.go +++ b/cluster-autoscaler/cloudprovider/gce/mig_info_provider_test.go @@ -51,7 +51,7 @@ type mockAutoscalingGceClient struct { fetchMigs func(string) ([]*gce.InstanceGroupManager, error) fetchMigTargetSize func(GceRef) (int64, error) fetchMigBasename func(GceRef) (string, error) - fetchMigInstances func(GceRef) ([]cloudprovider.Instance, error) + fetchMigInstances func(GceRef) ([]GceInstance, error) fetchMigTemplateName func(GceRef) (string, error) fetchMigTemplate func(GceRef, string) (*gce.InstanceTemplate, error) fetchMachineType func(string, string) (*gce.MachineType, error) @@ -77,7 +77,7 @@ func (client *mockAutoscalingGceClient) FetchMigBasename(migRef GceRef) (string, return client.fetchMigBasename(migRef) } -func (client *mockAutoscalingGceClient) FetchMigInstances(migRef GceRef) ([]cloudprovider.Instance, error) { +func (client *mockAutoscalingGceClient) FetchMigInstances(migRef GceRef) ([]GceInstance, error) { return client.fetchMigInstances(migRef) } @@ -123,13 +123,13 @@ func (client *mockAutoscalingGceClient) CreateInstances(_ GceRef, _ string, _ in func TestFillMigInstances(t *testing.T) { migRef := GceRef{Project: "test", Zone: "zone-A", Name: "some-mig"} - oldInstances := []cloudprovider.Instance{ - {Id: "gce://test/zone-A/some-mig-old-instance-1"}, - {Id: "gce://test/zone-A/some-mig-old-instance-2"}, + oldInstances := []GceInstance{ + {Instance: cloudprovider.Instance{Id: "gce://test/zone-A/some-mig-old-instance-1"}, NumericId: 1}, + {Instance: cloudprovider.Instance{Id: "gce://test/zone-A/some-mig-old-instance-2"}, NumericId: 2}, } - newInstances := []cloudprovider.Instance{ - {Id: "gce://test/zone-A/some-mig-new-instance-1"}, - {Id: "gce://test/zone-A/some-mig-new-instance-2"}, + newInstances := []GceInstance{ + {Instance: cloudprovider.Instance{Id: "gce://test/zone-A/some-mig-new-instance-1"}, NumericId: 3}, + {Instance: cloudprovider.Instance{Id: "gce://test/zone-A/some-mig-new-instance-2"}, NumericId: 4}, } timeNow := time.Now() @@ -140,13 +140,13 @@ func TestFillMigInstances(t *testing.T) { name string cache *GceCache wantClientCalls int - wantInstances []cloudprovider.Instance + wantInstances []GceInstance wantUpdateTime time.Time }{ { name: "No instances in cache", cache: &GceCache{ - instances: map[GceRef][]cloudprovider.Instance{}, + instances: map[GceRef][]GceInstance{}, instancesUpdateTime: map[GceRef]time.Time{}, instancesToMig: map[GceRef]GceRef{}, }, @@ -157,7 +157,7 @@ func TestFillMigInstances(t *testing.T) { { name: "Old instances in cache", cache: &GceCache{ - instances: map[GceRef][]cloudprovider.Instance{migRef: oldInstances}, + instances: map[GceRef][]GceInstance{migRef: oldInstances}, instancesUpdateTime: map[GceRef]time.Time{migRef: timeOld}, instancesToMig: map[GceRef]GceRef{}, }, @@ -168,7 +168,7 @@ func TestFillMigInstances(t *testing.T) { { name: "Recently updated instances in cache", cache: &GceCache{ - instances: map[GceRef][]cloudprovider.Instance{migRef: oldInstances}, + instances: map[GceRef][]GceInstance{migRef: oldInstances}, instancesUpdateTime: map[GceRef]time.Time{migRef: timeRecent}, instancesToMig: map[GceRef]GceRef{}, }, @@ -204,8 +204,9 @@ func TestFillMigInstances(t *testing.T) { } func TestMigInfoProviderGetMigForInstance(t *testing.T) { - instance := cloudprovider.Instance{ - Id: "gce://project/us-test1/base-instance-name-abcd", + instance := GceInstance{ + Instance: cloudprovider.Instance{Id: "gce://project/us-test1/base-instance-name-abcd"}, + NumericId: 777, } instanceRef, err := GceRefFromProviderId(instance.Id) assert.Nil(t, err) @@ -214,7 +215,7 @@ func TestMigInfoProviderGetMigForInstance(t *testing.T) { name string instanceRef GceRef cache *GceCache - fetchMigInstances func(GceRef) ([]cloudprovider.Instance, error) + fetchMigInstances func(GceRef) ([]GceInstance, error) fetchMigBasename func(GceRef) (string, error) expectedMig Mig expectedErr error @@ -253,12 +254,12 @@ func TestMigInfoProviderGetMigForInstance(t *testing.T) { name: "mig from cache fill", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instances: map[GceRef][]cloudprovider.Instance{}, + instances: map[GceRef][]GceInstance{}, instancesUpdateTime: map[GceRef]time.Time{}, instancesToMig: map[GceRef]GceRef{}, migBaseNameCache: map[GceRef]string{mig.GceRef(): "base-instance-name"}, }, - fetchMigInstances: fetchMigInstancesConst([]cloudprovider.Instance{instance}), + fetchMigInstances: fetchMigInstancesConst([]GceInstance{instance}), expectedMig: mig, expectedCachedMigRef: mig.GceRef(), expectedCached: true, @@ -267,12 +268,12 @@ func TestMigInfoProviderGetMigForInstance(t *testing.T) { name: "mig and basename from cache fill", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instances: map[GceRef][]cloudprovider.Instance{}, + instances: map[GceRef][]GceInstance{}, instancesUpdateTime: map[GceRef]time.Time{}, instancesToMig: map[GceRef]GceRef{}, migBaseNameCache: map[GceRef]string{}, }, - fetchMigInstances: fetchMigInstancesConst([]cloudprovider.Instance{instance}), + fetchMigInstances: fetchMigInstancesConst([]GceInstance{instance}), fetchMigBasename: fetchMigBasenameConst("base-instance-name"), expectedMig: mig, expectedCachedMigRef: mig.GceRef(), @@ -282,12 +283,12 @@ func TestMigInfoProviderGetMigForInstance(t *testing.T) { name: "unknown mig from cache fill", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instances: map[GceRef][]cloudprovider.Instance{}, + instances: map[GceRef][]GceInstance{}, instancesUpdateTime: map[GceRef]time.Time{}, instancesFromUnknownMig: map[GceRef]bool{}, migBaseNameCache: map[GceRef]string{mig.GceRef(): "base-instance-name"}, }, - fetchMigInstances: fetchMigInstancesConst([]cloudprovider.Instance{}), + fetchMigInstances: fetchMigInstancesConst([]GceInstance{}), expectedCachedMigUnknown: true, }, { @@ -297,7 +298,7 @@ func TestMigInfoProviderGetMigForInstance(t *testing.T) { migBaseNameCache: map[GceRef]string{mig.GceRef(): "different-base-instance-name"}, instancesToMig: map[GceRef]GceRef{}, }, - fetchMigInstances: fetchMigInstancesConst([]cloudprovider.Instance{instance}), + fetchMigInstances: fetchMigInstancesConst([]GceInstance{instance}), }, { name: "fetch instances error during cache fill", @@ -349,18 +350,18 @@ func TestMigInfoProviderGetMigForInstance(t *testing.T) { func TestGetMigInstances(t *testing.T) { oldRefreshTime := time.Now().Add(-time.Hour) newRefreshTime := time.Now() - instances := []cloudprovider.Instance{ - {Id: "gce://project/us-test1/base-instance-name-abcd"}, - {Id: "gce://project/us-test1/base-instance-name-efgh"}, + instances := []GceInstance{ + {Instance: cloudprovider.Instance{Id: "gce://project/us-test1/base-instance-name-abcd"}, NumericId: 7}, + {Instance: cloudprovider.Instance{Id: "gce://project/us-test1/base-instance-name-efgh"}, NumericId: 88}, } testCases := []struct { name string cache *GceCache - fetchMigInstances func(GceRef) ([]cloudprovider.Instance, error) - expectedInstances []cloudprovider.Instance + fetchMigInstances func(GceRef) ([]GceInstance, error) + expectedInstances []GceInstance expectedErr error - expectedCachedInstances []cloudprovider.Instance + expectedCachedInstances []GceInstance expectedCached bool expectedRefreshTime time.Time expectedRefreshed bool @@ -369,7 +370,7 @@ func TestGetMigInstances(t *testing.T) { name: "instances in cache", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instances: map[GceRef][]cloudprovider.Instance{mig.GceRef(): instances}, + instances: map[GceRef][]GceInstance{mig.GceRef(): instances}, instancesUpdateTime: map[GceRef]time.Time{mig.GceRef(): oldRefreshTime}, }, expectedInstances: instances, @@ -382,7 +383,7 @@ func TestGetMigInstances(t *testing.T) { name: "instances cache fill", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instances: map[GceRef][]cloudprovider.Instance{}, + instances: map[GceRef][]GceInstance{}, instancesUpdateTime: map[GceRef]time.Time{}, instancesToMig: map[GceRef]GceRef{}, }, @@ -397,12 +398,12 @@ func TestGetMigInstances(t *testing.T) { name: "error during instances cache fill", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instances: map[GceRef][]cloudprovider.Instance{}, + instances: map[GceRef][]GceInstance{}, instancesUpdateTime: map[GceRef]time.Time{}, }, fetchMigInstances: fetchMigInstancesFail, expectedErr: errFetchMigInstances, - expectedCachedInstances: []cloudprovider.Instance{}, + expectedCachedInstances: []GceInstance{}, }, } @@ -441,13 +442,13 @@ func TestRegenerateMigInstancesCache(t *testing.T) { }, } - instances := []cloudprovider.Instance{ - {Id: "gce://project/us-test1/base-instance-name-abcd"}, - {Id: "gce://project/us-test1/base-instance-name-efgh"}, + instances := []GceInstance{ + {Instance: cloudprovider.Instance{Id: "gce://project/us-test1/base-instance-name-abcd"}, NumericId: 1}, + {Instance: cloudprovider.Instance{Id: "gce://project/us-test1/base-instance-name-efgh"}, NumericId: 2}, } - otherInstances := []cloudprovider.Instance{ - {Id: "gce://project/us-test1/other-base-instance-name-abcd"}, - {Id: "gce://project/us-test1/other-base-instance-name-efgh"}, + otherInstances := []GceInstance{ + {Instance: cloudprovider.Instance{Id: "gce://project/us-test1/other-base-instance-name-abcd"}}, + {Instance: cloudprovider.Instance{Id: "gce://project/us-test1/other-base-instance-name-efgh"}}, } var instancesRefs, otherInstancesRefs []GceRef @@ -465,20 +466,20 @@ func TestRegenerateMigInstancesCache(t *testing.T) { testCases := []struct { name string cache *GceCache - fetchMigInstances func(GceRef) ([]cloudprovider.Instance, error) + fetchMigInstances func(GceRef) ([]GceInstance, error) expectedErr error - expectedMigInstances map[GceRef][]cloudprovider.Instance + expectedMigInstances map[GceRef][]GceInstance expectedInstancesToMig map[GceRef]GceRef }{ { name: "fill empty cache for one mig", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instances: map[GceRef][]cloudprovider.Instance{}, + instances: map[GceRef][]GceInstance{}, instancesToMig: map[GceRef]GceRef{}, }, fetchMigInstances: fetchMigInstancesConst(instances), - expectedMigInstances: map[GceRef][]cloudprovider.Instance{ + expectedMigInstances: map[GceRef][]GceInstance{ mig.GceRef(): instances, }, expectedInstancesToMig: map[GceRef]GceRef{ @@ -493,14 +494,14 @@ func TestRegenerateMigInstancesCache(t *testing.T) { mig.GceRef(): mig, otherMig.GceRef(): otherMig, }, - instances: map[GceRef][]cloudprovider.Instance{}, + instances: map[GceRef][]GceInstance{}, instancesToMig: map[GceRef]GceRef{}, }, - fetchMigInstances: fetchMigInstancesMapping(map[GceRef][]cloudprovider.Instance{ + fetchMigInstances: fetchMigInstancesMapping(map[GceRef][]GceInstance{ mig.GceRef(): instances, otherMig.GceRef(): otherInstances, }), - expectedMigInstances: map[GceRef][]cloudprovider.Instance{ + expectedMigInstances: map[GceRef][]GceInstance{ mig.GceRef(): instances, otherMig.GceRef(): otherInstances, }, @@ -517,7 +518,7 @@ func TestRegenerateMigInstancesCache(t *testing.T) { migs: map[GceRef]Mig{ mig.GceRef(): mig, }, - instances: map[GceRef][]cloudprovider.Instance{ + instances: map[GceRef][]GceInstance{ mig.GceRef(): instances, otherMig.GceRef(): otherInstances, }, @@ -528,11 +529,11 @@ func TestRegenerateMigInstancesCache(t *testing.T) { otherInstancesRefs[1]: otherMig.GceRef(), }, }, - fetchMigInstances: fetchMigInstancesMapping(map[GceRef][]cloudprovider.Instance{ + fetchMigInstances: fetchMigInstancesMapping(map[GceRef][]GceInstance{ mig.GceRef(): instances, otherMig.GceRef(): otherInstances, }), - expectedMigInstances: map[GceRef][]cloudprovider.Instance{ + expectedMigInstances: map[GceRef][]GceInstance{ mig.GceRef(): instances, }, expectedInstancesToMig: map[GceRef]GceRef{ @@ -546,7 +547,7 @@ func TestRegenerateMigInstancesCache(t *testing.T) { migs: map[GceRef]Mig{ mig.GceRef(): mig, }, - instances: map[GceRef][]cloudprovider.Instance{ + instances: map[GceRef][]GceInstance{ mig.GceRef(): instances, }, instancesToMig: map[GceRef]GceRef{ @@ -555,7 +556,7 @@ func TestRegenerateMigInstancesCache(t *testing.T) { }, }, fetchMigInstances: fetchMigInstancesConst(otherInstances), - expectedMigInstances: map[GceRef][]cloudprovider.Instance{ + expectedMigInstances: map[GceRef][]GceInstance{ mig.GceRef(): otherInstances, }, expectedInstancesToMig: map[GceRef]GceRef{ @@ -569,7 +570,7 @@ func TestRegenerateMigInstancesCache(t *testing.T) { migs: map[GceRef]Mig{ mig.GceRef(): mig, }, - instances: map[GceRef][]cloudprovider.Instance{}, + instances: map[GceRef][]GceInstance{}, instancesToMig: map[GceRef]GceRef{}, }, fetchMigInstances: fetchMigInstancesFail, @@ -1048,13 +1049,13 @@ func TestMultipleGetMigInstanceCallsLimited(t *testing.T) { Name: "base-instance-name", }, } - instance := cloudprovider.Instance{ - Id: "gce://project/zone/base-instance-name-abcd", + 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 := cloudprovider.Instance{ - Id: "gce://project/zone/base-instance-name-abcd2", + instance2 := GceInstance{ + Instance: cloudprovider.Instance{Id: "gce://project/zone/base-instance-name-abcd2"}, NumericId: 222, } instanceRef2, err := GceRefFromProviderId(instance2.Id) assert.Nil(t, err) @@ -1127,7 +1128,7 @@ func (f *fakeTime) Now() time.Time { func emptyCache() *GceCache { return &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instances: make(map[GceRef][]cloudprovider.Instance), + instances: make(map[GceRef][]GceInstance), instancesUpdateTime: make(map[GceRef]time.Time), migTargetSizeCache: make(map[GceRef]int64), migBaseNameCache: make(map[GceRef]string), @@ -1147,25 +1148,25 @@ func fetchMigsConst(migs []*gce.InstanceGroupManager) func(string) ([]*gce.Insta } } -func fetchMigInstancesFail(_ GceRef) ([]cloudprovider.Instance, error) { +func fetchMigInstancesFail(_ GceRef) ([]GceInstance, error) { return nil, errFetchMigInstances } -func fetchMigInstancesConst(instances []cloudprovider.Instance) func(GceRef) ([]cloudprovider.Instance, error) { - return func(GceRef) ([]cloudprovider.Instance, error) { +func fetchMigInstancesConst(instances []GceInstance) func(GceRef) ([]GceInstance, error) { + return func(GceRef) ([]GceInstance, error) { return instances, nil } } -func fetchMigInstancesWithCounter(instances []cloudprovider.Instance, migCounter map[GceRef]int) func(GceRef) ([]cloudprovider.Instance, error) { - return func(ref GceRef) ([]cloudprovider.Instance, error) { +func fetchMigInstancesWithCounter(instances []GceInstance, migCounter map[GceRef]int) func(GceRef) ([]GceInstance, error) { + return func(ref GceRef) ([]GceInstance, error) { migCounter[ref] = migCounter[ref] + 1 return instances, nil } } -func fetchMigInstancesMapping(instancesMapping map[GceRef][]cloudprovider.Instance) func(GceRef) ([]cloudprovider.Instance, error) { - return func(migRef GceRef) ([]cloudprovider.Instance, error) { +func fetchMigInstancesMapping(instancesMapping map[GceRef][]GceInstance) func(GceRef) ([]GceInstance, error) { + return func(migRef GceRef) ([]GceInstance, error) { return instancesMapping[migRef], nil } }