From 623f132a0b80838d93909574d60a42846f07844a Mon Sep 17 00:00:00 2001 From: Edwinhr716 Date: Tue, 20 Feb 2024 17:55:39 +0000 Subject: [PATCH 01/12] made changes to support MIGs that use regional instance templates --- .../gce/autoscaling_gce_client.go | 23 ++++++++----- cluster-autoscaler/cloudprovider/gce/cache.go | 21 +++++++----- .../cloudprovider/gce/mig_info_provider.go | 32 ++++++++++--------- 3 files changed, 45 insertions(+), 31 deletions(-) diff --git a/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client.go b/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client.go index 12d88fb183c5..688da0552f19 100644 --- a/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client.go +++ b/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client.go @@ -103,8 +103,8 @@ type AutoscalingGceClient interface { FetchMigTargetSize(GceRef) (int64, error) FetchMigBasename(GceRef) (string, error) FetchMigInstances(GceRef) ([]GceInstance, error) - FetchMigTemplateName(migRef GceRef) (string, error) - FetchMigTemplate(migRef GceRef, templateName string) (*gce.InstanceTemplate, error) + FetchMigTemplateName(migRef GceRef) (InstanceTemplateNameType, error) + FetchMigTemplate(migRef GceRef, templateName string, regional bool) (*gce.InstanceTemplate, error) FetchMigsWithName(zone string, filter *regexp.Regexp) ([]string, error) FetchZones(region string) ([]string, error) FetchAvailableCpuPlatforms() (map[string][]string, error) @@ -580,26 +580,33 @@ func (client *autoscalingGceClientV1) FetchAvailableCpuPlatforms() (map[string][ return availableCpuPlatforms, nil } -func (client *autoscalingGceClientV1) FetchMigTemplateName(migRef GceRef) (string, error) { +func (client *autoscalingGceClientV1) FetchMigTemplateName(migRef GceRef) (InstanceTemplateNameType, error) { registerRequest("instance_group_managers", "get") igm, err := client.gceService.InstanceGroupManagers.Get(migRef.Project, migRef.Zone, migRef.Name).Do() if err != nil { if err, ok := err.(*googleapi.Error); ok { if err.Code == http.StatusNotFound { - return "", errors.NewAutoscalerError(errors.NodeGroupDoesNotExistError, "%s", err.Error()) + return InstanceTemplateNameType{}, errors.NewAutoscalerError(errors.NodeGroupDoesNotExistError, "%s", err.Error()) } } - return "", err + return InstanceTemplateNameType{}, err } templateUrl, err := url.Parse(igm.InstanceTemplate) + regional, _ := regexp.MatchString("(/projects/.*[A-Za-z0-9]+.*/regions/)", templateUrl.String()) if err != nil { - return "", err + return InstanceTemplateNameType{}, err } _, templateName := path.Split(templateUrl.EscapedPath()) - return templateName, nil + return InstanceTemplateNameType{templateName, regional}, nil } -func (client *autoscalingGceClientV1) FetchMigTemplate(migRef GceRef, templateName string) (*gce.InstanceTemplate, error) { +func (client *autoscalingGceClientV1) FetchMigTemplate(migRef GceRef, templateName string, regional bool) (*gce.InstanceTemplate, error) { + if regional { + zoneHyphenIndex := strings.LastIndex(migRef.Zone, "-") + region := migRef.Zone[:zoneHyphenIndex] + registerRequest("region_instance_templates", "get") + return client.gceService.RegionInstanceTemplates.Get(migRef.Project, region, templateName).Do() + } registerRequest("instance_templates", "get") return client.gceService.InstanceTemplates.Get(migRef.Project, templateName).Do() } diff --git a/cluster-autoscaler/cloudprovider/gce/cache.go b/cluster-autoscaler/cloudprovider/gce/cache.go index 1d46e4f353d5..379c43a43cf2 100644 --- a/cluster-autoscaler/cloudprovider/gce/cache.go +++ b/cluster-autoscaler/cloudprovider/gce/cache.go @@ -33,6 +33,11 @@ type MachineTypeKey struct { MachineTypeName string } +type InstanceTemplateNameType struct { + Name string + Regional bool +} + // GceCache is used for caching cluster resources state. // // It is needed to: @@ -67,7 +72,7 @@ type GceCache struct { migTargetSizeCache map[GceRef]int64 migBaseNameCache map[GceRef]string listManagedInstancesResultsCache map[GceRef]string - instanceTemplateNameCache map[GceRef]string + instanceTemplateNameCache map[GceRef]InstanceTemplateNameType instanceTemplatesCache map[GceRef]*gce.InstanceTemplate kubeEnvCache map[GceRef]KubeEnv } @@ -85,7 +90,7 @@ func NewGceCache() *GceCache { migTargetSizeCache: map[GceRef]int64{}, migBaseNameCache: map[GceRef]string{}, listManagedInstancesResultsCache: map[GceRef]string{}, - instanceTemplateNameCache: map[GceRef]string{}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{}, instanceTemplatesCache: map[GceRef]*gce.InstanceTemplate{}, kubeEnvCache: map[GceRef]KubeEnv{}, } @@ -334,23 +339,23 @@ func (gc *GceCache) InvalidateAllMigTargetSizes() { } // GetMigInstanceTemplateName returns the cached instance template ref for a mig GceRef -func (gc *GceCache) GetMigInstanceTemplateName(ref GceRef) (string, bool) { +func (gc *GceCache) GetMigInstanceTemplateName(ref GceRef) (InstanceTemplateNameType, bool) { gc.cacheMutex.Lock() defer gc.cacheMutex.Unlock() - templateName, found := gc.instanceTemplateNameCache[ref] + instanceTemplateNameType, found := gc.instanceTemplateNameCache[ref] if found { klog.V(5).Infof("Instance template names cache hit for %s", ref) } - return templateName, found + return instanceTemplateNameType, found } // SetMigInstanceTemplateName sets instance template ref for a mig GceRef -func (gc *GceCache) SetMigInstanceTemplateName(ref GceRef, templateName string) { +func (gc *GceCache) SetMigInstanceTemplateName(ref GceRef, instanceTemplateNameType InstanceTemplateNameType) { gc.cacheMutex.Lock() defer gc.cacheMutex.Unlock() - gc.instanceTemplateNameCache[ref] = templateName + gc.instanceTemplateNameCache[ref] = instanceTemplateNameType } // InvalidateMigInstanceTemplateName clears the instance template ref cache for a mig GceRef @@ -370,7 +375,7 @@ func (gc *GceCache) InvalidateAllMigInstanceTemplateNames() { defer gc.cacheMutex.Unlock() klog.V(5).Infof("Instance template names cache invalidated") - gc.instanceTemplateNameCache = map[GceRef]string{} + gc.instanceTemplateNameCache = map[GceRef]InstanceTemplateNameType{} } // GetMigInstanceTemplate returns the cached gce.InstanceTemplate for a mig GceRef diff --git a/cluster-autoscaler/cloudprovider/gce/mig_info_provider.go b/cluster-autoscaler/cloudprovider/gce/mig_info_provider.go index bd7c61c2ef8e..6bfb9db42874 100644 --- a/cluster-autoscaler/cloudprovider/gce/mig_info_provider.go +++ b/cluster-autoscaler/cloudprovider/gce/mig_info_provider.go @@ -21,6 +21,7 @@ import ( "fmt" "net/url" "path" + "regexp" "strings" "sync" "time" @@ -43,7 +44,7 @@ type MigInfoProvider interface { // GetMigBasename returns basename for given MIG ref GetMigBasename(migRef GceRef) (string, error) // GetMigInstanceTemplateName returns instance template name for given MIG ref - GetMigInstanceTemplateName(migRef GceRef) (string, error) + GetMigInstanceTemplateName(migRef GceRef) (InstanceTemplateNameType, error) // GetMigInstanceTemplate returns instance template for given MIG ref GetMigInstanceTemplate(migRef GceRef) (*gce.InstanceTemplate, error) // GetMigKubeEnv returns kube-env for given MIG ref @@ -243,44 +244,44 @@ func (c *cachingMigInfoProvider) GetMigBasename(migRef GceRef) (string, error) { return basename, nil } -func (c *cachingMigInfoProvider) GetMigInstanceTemplateName(migRef GceRef) (string, error) { +func (c *cachingMigInfoProvider) GetMigInstanceTemplateName(migRef GceRef) (InstanceTemplateNameType, error) { c.migInfoMutex.Lock() defer c.migInfoMutex.Unlock() - templateName, found := c.cache.GetMigInstanceTemplateName(migRef) + instanceTemplateNameType, found := c.cache.GetMigInstanceTemplateName(migRef) if found { - return templateName, nil + return instanceTemplateNameType, nil } err := c.fillMigInfoCache() - templateName, found = c.cache.GetMigInstanceTemplateName(migRef) + instanceTemplateNameType, found = c.cache.GetMigInstanceTemplateName(migRef) if err == nil && found { - return templateName, nil + return instanceTemplateNameType, nil } // fallback to querying for single mig - templateName, err = c.gceClient.FetchMigTemplateName(migRef) + instanceTemplateNameType, err = c.gceClient.FetchMigTemplateName(migRef) if err != nil { c.migLister.HandleMigIssue(migRef, err) - return "", err + return InstanceTemplateNameType{}, err } - c.cache.SetMigInstanceTemplateName(migRef, templateName) - return templateName, nil + c.cache.SetMigInstanceTemplateName(migRef, instanceTemplateNameType) + return instanceTemplateNameType, nil } func (c *cachingMigInfoProvider) GetMigInstanceTemplate(migRef GceRef) (*gce.InstanceTemplate, error) { - templateName, err := c.GetMigInstanceTemplateName(migRef) + instanceTemplateNameType, err := c.GetMigInstanceTemplateName(migRef) if err != nil { return nil, err } template, found := c.cache.GetMigInstanceTemplate(migRef) - if found && template.Name == templateName { + if found && template.Name == instanceTemplateNameType.Name { return template, nil } - klog.V(2).Infof("Instance template of mig %v changed to %v", migRef.Name, templateName) - template, err = c.gceClient.FetchMigTemplate(migRef, templateName) + klog.V(2).Infof("Instance template of mig %v changed to %v", migRef.Name, instanceTemplateNameType.Name) + template, err = c.gceClient.FetchMigTemplate(migRef, instanceTemplateNameType.Name, instanceTemplateNameType.Regional) if err != nil { return nil, err } @@ -363,7 +364,8 @@ func (c *cachingMigInfoProvider) fillMigInfoCache() error { templateUrl, err := url.Parse(zoneMig.InstanceTemplate) if err == nil { _, templateName := path.Split(templateUrl.EscapedPath()) - c.cache.SetMigInstanceTemplateName(zoneMigRef, templateName) + regional, _ := regexp.MatchString("(/projects/.*[A-Za-z0-9]+.*/regions/)", templateUrl.String()) + c.cache.SetMigInstanceTemplateName(zoneMigRef, InstanceTemplateNameType{templateName, regional}) } } } From b57335157a8d326195b2ad27a6d7cd08cdfee5b3 Mon Sep 17 00:00:00 2001 From: Edwinhr716 Date: Tue, 20 Feb 2024 22:33:08 +0000 Subject: [PATCH 02/12] modified current unit tests to support the new modifications --- .../cloudprovider/gce/gce_manager_test.go | 2 +- .../gce/mig_info_provider_test.go | 50 +++++++++---------- 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/cluster-autoscaler/cloudprovider/gce/gce_manager_test.go b/cluster-autoscaler/cloudprovider/gce/gce_manager_test.go index b4ddba0b4b87..55256acca4ab 100644 --- a/cluster-autoscaler/cloudprovider/gce/gce_manager_test.go +++ b/cluster-autoscaler/cloudprovider/gce/gce_manager_test.go @@ -344,7 +344,7 @@ func newTestGceManager(t *testing.T, testServerURL string, regional bool) *gceMa {"us-central1-f", "n1-standard-1"}: {Name: "n1-standard-1", CPU: 1, Memory: 1}, }, migTargetSizeCache: map[GceRef]int64{}, - instanceTemplateNameCache: map[GceRef]string{}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{}, instanceTemplatesCache: map[GceRef]*gce.InstanceTemplate{}, kubeEnvCache: map[GceRef]KubeEnv{}, migBaseNameCache: map[GceRef]string{}, diff --git a/cluster-autoscaler/cloudprovider/gce/mig_info_provider_test.go b/cluster-autoscaler/cloudprovider/gce/mig_info_provider_test.go index a7e9e311766f..e6b11099879d 100644 --- a/cluster-autoscaler/cloudprovider/gce/mig_info_provider_test.go +++ b/cluster-autoscaler/cloudprovider/gce/mig_info_provider_test.go @@ -53,7 +53,7 @@ type mockAutoscalingGceClient struct { fetchMigTargetSize func(GceRef) (int64, error) fetchMigBasename func(GceRef) (string, error) fetchMigInstances func(GceRef) ([]GceInstance, error) - fetchMigTemplateName func(GceRef) (string, error) + fetchMigTemplateName func(GceRef) (InstanceTemplateNameType, error) fetchMigTemplate func(GceRef, string) (*gce.InstanceTemplate, error) fetchMachineType func(string, string) (*gce.MachineType, error) fetchListManagedInstancesResults func(GceRef) (string, error) @@ -87,12 +87,12 @@ func (client *mockAutoscalingGceClient) FetchMigInstances(migRef GceRef) ([]GceI return client.fetchMigInstances(migRef) } -func (client *mockAutoscalingGceClient) FetchMigTemplateName(migRef GceRef) (string, error) { +func (client *mockAutoscalingGceClient) FetchMigTemplateName(migRef GceRef) (InstanceTemplateNameType, error) { return client.fetchMigTemplateName(migRef) } -func (client *mockAutoscalingGceClient) FetchMigTemplate(migRef GceRef, templateName string) (*gce.InstanceTemplate, error) { - return client.fetchMigTemplate(migRef, templateName) +func (client *mockAutoscalingGceClient) FetchMigTemplate(migRef GceRef, templateName string, regional bool) (*gce.InstanceTemplate, error) { + return client.fetchMigTemplate(migRef, templateName, regional) } func (client *mockAutoscalingGceClient) FetchMigsWithName(_ string, _ *regexp.Regexp) ([]string, error) { @@ -859,7 +859,7 @@ func TestGetMigInstanceTemplateName(t *testing.T) { name string cache *GceCache fetchMigs func(string) ([]*gce.InstanceGroupManager, error) - fetchMigTemplateName func(GceRef) (string, error) + fetchMigTemplateName func(GceRef) (InstanceTemplateNameType, error) expectedTemplateName string expectedErr error }{ @@ -867,7 +867,7 @@ func TestGetMigInstanceTemplateName(t *testing.T) { name: "template name in cache", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]string{mig.GceRef(): templateName}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): InstanceTemplateNameType{templateName, false}}, }, expectedTemplateName: templateName, }, @@ -881,14 +881,14 @@ func TestGetMigInstanceTemplateName(t *testing.T) { name: "cache fill without mig, fallback success", cache: emptyCache(), fetchMigs: fetchMigsConst([]*gce.InstanceGroupManager{}), - fetchMigTemplateName: fetchMigTemplateNameConst(templateName), + fetchMigTemplateName: fetchMigTemplateNameConst(InstanceTemplateNameType{templateName, false}), expectedTemplateName: templateName, }, { name: "cache fill failure, fallback success", cache: emptyCache(), fetchMigs: fetchMigsFail, - fetchMigTemplateName: fetchMigTemplateNameConst(templateName), + fetchMigTemplateName: fetchMigTemplateNameConst(InstanceTemplateNameType{templateName, false}), expectedTemplateName: templateName, }, { @@ -944,8 +944,8 @@ func TestGetMigInstanceTemplate(t *testing.T) { name string cache *GceCache fetchMigs func(string) ([]*gce.InstanceGroupManager, error) - fetchMigTemplateName func(GceRef) (string, error) - fetchMigTemplate func(GceRef, string) (*gce.InstanceTemplate, error) + fetchMigTemplateName func(GceRef) (InstanceTemplateNameType, error) + fetchMigTemplate func(GceRef, string, bool) (*gce.InstanceTemplate, error) expectedTemplate *gce.InstanceTemplate expectedCachedTemplate *gce.InstanceTemplate expectedErr error @@ -954,7 +954,7 @@ func TestGetMigInstanceTemplate(t *testing.T) { name: "template in cache", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]string{mig.GceRef(): templateName}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): InstanceTemplateNameType{templateName, false}}, instanceTemplatesCache: map[GceRef]*gce.InstanceTemplate{mig.GceRef(): template}, }, expectedTemplate: template, @@ -964,7 +964,7 @@ func TestGetMigInstanceTemplate(t *testing.T) { name: "cache without template, fetch success", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]string{mig.GceRef(): templateName}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): InstanceTemplateNameType{templateName, false}}, instanceTemplatesCache: make(map[GceRef]*gce.InstanceTemplate), }, fetchMigTemplate: fetchMigTemplateConst(template), @@ -975,7 +975,7 @@ func TestGetMigInstanceTemplate(t *testing.T) { name: "cache with old template, fetch success", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]string{mig.GceRef(): templateName}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): InstanceTemplateNameType{templateName, false}}, instanceTemplatesCache: map[GceRef]*gce.InstanceTemplate{mig.GceRef(): oldTemplate}, }, fetchMigTemplate: fetchMigTemplateConst(template), @@ -986,7 +986,7 @@ func TestGetMigInstanceTemplate(t *testing.T) { name: "cache without template, fetch failure", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]string{mig.GceRef(): templateName}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): InstanceTemplateNameType{templateName, false}}, instanceTemplatesCache: make(map[GceRef]*gce.InstanceTemplate), }, fetchMigTemplate: fetchMigTemplateFail, @@ -996,7 +996,7 @@ func TestGetMigInstanceTemplate(t *testing.T) { name: "cache with old template, fetch failure", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]string{mig.GceRef(): templateName}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): InstanceTemplateNameType{templateName, false}}, instanceTemplatesCache: map[GceRef]*gce.InstanceTemplate{mig.GceRef(): oldTemplate}, }, fetchMigTemplate: fetchMigTemplateFail, @@ -1251,7 +1251,7 @@ func TestGetMigMachineType(t *testing.T) { }, } cache := &GceCache{ - instanceTemplateNameCache: map[GceRef]string{mig.GceRef(): "template"}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): InstanceTemplateNameType{"template", false}}, instanceTemplatesCache: map[GceRef]*gce.InstanceTemplate{ mig.GceRef(): { Name: "template", @@ -1377,7 +1377,7 @@ func emptyCache() *GceCache { migTargetSizeCache: make(map[GceRef]int64), migBaseNameCache: make(map[GceRef]string), listManagedInstancesResultsCache: make(map[GceRef]string), - instanceTemplateNameCache: make(map[GceRef]string), + instanceTemplateNameCache: make(map[GceRef]InstanceTemplateNameType), instanceTemplatesCache: make(map[GceRef]*gce.InstanceTemplate), instancesFromUnknownMig: make(map[GceRef]bool), } @@ -1436,22 +1436,22 @@ func fetchMigBasenameConst(basename string) func(GceRef) (string, error) { } } -func fetchMigTemplateNameFail(_ GceRef) (string, error) { - return "", errFetchMigTemplateName +func fetchMigTemplateNameFail(_ GceRef) (InstanceTemplateNameType, error) { + return InstanceTemplateNameType{}, errFetchMigTemplateName } -func fetchMigTemplateNameConst(templateName string) func(GceRef) (string, error) { - return func(GceRef) (string, error) { - return templateName, nil +func fetchMigTemplateNameConst(instanceTemplateNameType InstanceTemplateNameType) func(GceRef) (InstanceTemplateNameType, error) { + return func(GceRef) (InstanceTemplateNameType, error) { + return instanceTemplateNameType, nil } } -func fetchMigTemplateFail(_ GceRef, _ string) (*gce.InstanceTemplate, error) { +func fetchMigTemplateFail(_ GceRef, _ string, _ bool) (*gce.InstanceTemplate, error) { return nil, errFetchMigTemplate } -func fetchMigTemplateConst(template *gce.InstanceTemplate) func(GceRef, string) (*gce.InstanceTemplate, error) { - return func(GceRef, string) (*gce.InstanceTemplate, error) { +func fetchMigTemplateConst(template *gce.InstanceTemplate) func(GceRef, string, bool) (*gce.InstanceTemplate, error) { + return func(GceRef, string, bool) (*gce.InstanceTemplate, error) { return template, nil } } From da30b66f085235b6592fdf8f5528527f1590d7d1 Mon Sep 17 00:00:00 2001 From: Edwinhr716 Date: Tue, 27 Feb 2024 00:12:37 +0000 Subject: [PATCH 03/12] added comment to InstanceTemplateNameType --- cluster-autoscaler/cloudprovider/gce/cache.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cluster-autoscaler/cloudprovider/gce/cache.go b/cluster-autoscaler/cloudprovider/gce/cache.go index 379c43a43cf2..166c59bf3ffe 100644 --- a/cluster-autoscaler/cloudprovider/gce/cache.go +++ b/cluster-autoscaler/cloudprovider/gce/cache.go @@ -33,6 +33,8 @@ type MachineTypeKey struct { MachineTypeName string } +// InstanceTemplateNameType is used to store the name, and +// whether or not the instance template is regional type InstanceTemplateNameType struct { Name string Regional bool From 9916027073f0f76cd491bedf4597ff80b4319ba1 Mon Sep 17 00:00:00 2001 From: Edwinhr716 Date: Tue, 27 Feb 2024 00:23:01 +0000 Subject: [PATCH 04/12] Ran hack/go-fmtupdate.h on mig_info_provider_test.go --- .../cloudprovider/gce/mig_info_provider_test.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/cluster-autoscaler/cloudprovider/gce/mig_info_provider_test.go b/cluster-autoscaler/cloudprovider/gce/mig_info_provider_test.go index e6b11099879d..85b84b731662 100644 --- a/cluster-autoscaler/cloudprovider/gce/mig_info_provider_test.go +++ b/cluster-autoscaler/cloudprovider/gce/mig_info_provider_test.go @@ -867,7 +867,7 @@ func TestGetMigInstanceTemplateName(t *testing.T) { name: "template name in cache", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): InstanceTemplateNameType{templateName, false}}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): {templateName, false}}, }, expectedTemplateName: templateName, }, @@ -954,7 +954,7 @@ func TestGetMigInstanceTemplate(t *testing.T) { name: "template in cache", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): InstanceTemplateNameType{templateName, false}}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): {templateName, false}}, instanceTemplatesCache: map[GceRef]*gce.InstanceTemplate{mig.GceRef(): template}, }, expectedTemplate: template, @@ -964,7 +964,7 @@ func TestGetMigInstanceTemplate(t *testing.T) { name: "cache without template, fetch success", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): InstanceTemplateNameType{templateName, false}}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): {templateName, false}}, instanceTemplatesCache: make(map[GceRef]*gce.InstanceTemplate), }, fetchMigTemplate: fetchMigTemplateConst(template), @@ -975,7 +975,7 @@ func TestGetMigInstanceTemplate(t *testing.T) { name: "cache with old template, fetch success", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): InstanceTemplateNameType{templateName, false}}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): {templateName, false}}, instanceTemplatesCache: map[GceRef]*gce.InstanceTemplate{mig.GceRef(): oldTemplate}, }, fetchMigTemplate: fetchMigTemplateConst(template), @@ -986,7 +986,7 @@ func TestGetMigInstanceTemplate(t *testing.T) { name: "cache without template, fetch failure", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): InstanceTemplateNameType{templateName, false}}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): {templateName, false}}, instanceTemplatesCache: make(map[GceRef]*gce.InstanceTemplate), }, fetchMigTemplate: fetchMigTemplateFail, @@ -996,7 +996,7 @@ func TestGetMigInstanceTemplate(t *testing.T) { name: "cache with old template, fetch failure", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): InstanceTemplateNameType{templateName, false}}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): {templateName, false}}, instanceTemplatesCache: map[GceRef]*gce.InstanceTemplate{mig.GceRef(): oldTemplate}, }, fetchMigTemplate: fetchMigTemplateFail, @@ -1251,7 +1251,7 @@ func TestGetMigMachineType(t *testing.T) { }, } cache := &GceCache{ - instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): InstanceTemplateNameType{"template", false}}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): {"template", false}}, instanceTemplatesCache: map[GceRef]*gce.InstanceTemplate{ mig.GceRef(): { Name: "template", From 52be5d2dc4049b5cd6b5748ee5c1ac1bcb962425 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Wr=C3=B3blewski?= Date: Thu, 15 Feb 2024 14:58:07 +0000 Subject: [PATCH 05/12] Use KubeEnv in gce/templates.go --- .../cloudprovider/gce/kube_env.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/cluster-autoscaler/cloudprovider/gce/kube_env.go b/cluster-autoscaler/cloudprovider/gce/kube_env.go index a772948eed32..dfbc8e0fb492 100644 --- a/cluster-autoscaler/cloudprovider/gce/kube_env.go +++ b/cluster-autoscaler/cloudprovider/gce/kube_env.go @@ -71,3 +71,22 @@ func (ke KubeEnv) Var(name string) (string, bool) { val, found := ke.env[name] return val, found } + +// ParseKubeEnv parses kube-env from its string representation +func ParseKubeEnv(kubeEnvValue string) (KubeEnv, error) { + kubeEnv := make(map[string]string) + err := yaml.Unmarshal([]byte(kubeEnvValue), &kubeEnv) + if err != nil { + return nil, fmt.Errorf("error unmarshalling kubeEnv: %v", err) + } + return kubeEnv, nil +} + +// Var extracts variable from KubeEnv +func (ke KubeEnv) Var(name string) (string, bool) { + if ke == nil { + return "", false + } + val, found := ke[name] + return val, found +} From d348ae858b167e82435f0130aa0cc777e5a40893 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Wr=C3=B3blewski?= Date: Fri, 16 Feb 2024 13:09:36 +0000 Subject: [PATCH 06/12] Add templateName to kube-env to ensure that correct value is cached --- cluster-autoscaler/cloudprovider/gce/kube_env.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/cluster-autoscaler/cloudprovider/gce/kube_env.go b/cluster-autoscaler/cloudprovider/gce/kube_env.go index dfbc8e0fb492..4cf1354baee4 100644 --- a/cluster-autoscaler/cloudprovider/gce/kube_env.go +++ b/cluster-autoscaler/cloudprovider/gce/kube_env.go @@ -73,20 +73,20 @@ func (ke KubeEnv) Var(name string) (string, bool) { } // ParseKubeEnv parses kube-env from its string representation -func ParseKubeEnv(kubeEnvValue string) (KubeEnv, error) { - kubeEnv := make(map[string]string) - err := yaml.Unmarshal([]byte(kubeEnvValue), &kubeEnv) +func ParseKubeEnv(templateName, kubeEnvValue string) (KubeEnv, error) { + env := make(map[string]string) + err := yaml.Unmarshal([]byte(kubeEnvValue), &env) if err != nil { - return nil, fmt.Errorf("error unmarshalling kubeEnv: %v", err) + return KubeEnv{}, fmt.Errorf("error unmarshalling kubeEnv: %v", err) } - return kubeEnv, nil + return KubeEnv{templateName: templateName, env: env}, nil } // Var extracts variable from KubeEnv func (ke KubeEnv) Var(name string) (string, bool) { - if ke == nil { + if ke.env == nil { return "", false } - val, found := ke[name] + val, found := ke.env[name] return val, found } From 145d28622fc7aa1b89dfeff6261322116075574b Mon Sep 17 00:00:00 2001 From: Edwinhr716 Date: Wed, 28 Feb 2024 18:37:30 +0000 Subject: [PATCH 07/12] rebased and resolved conflicts --- .../gce/autoscaling_gce_client.go | 6 +++++- .../cloudprovider/gce/mig_info_provider.go | 4 ++-- .../gce/mig_info_provider_test.go | 18 +++++++++--------- 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client.go b/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client.go index 688da0552f19..f24c05729dba 100644 --- a/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client.go +++ b/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client.go @@ -592,10 +592,14 @@ func (client *autoscalingGceClientV1) FetchMigTemplateName(migRef GceRef) (Insta return InstanceTemplateNameType{}, err } templateUrl, err := url.Parse(igm.InstanceTemplate) - regional, _ := regexp.MatchString("(/projects/.*[A-Za-z0-9]+.*/regions/)", templateUrl.String()) if err != nil { return InstanceTemplateNameType{}, err } + regional, err := regexp.MatchString("(/projects/.*[A-Za-z0-9]+.*/regions/)", templateUrl.String()) + if err != nil { + return InstanceTemplateNameType{}, err + } + _, templateName := path.Split(templateUrl.EscapedPath()) return InstanceTemplateNameType{templateName, regional}, nil } diff --git a/cluster-autoscaler/cloudprovider/gce/mig_info_provider.go b/cluster-autoscaler/cloudprovider/gce/mig_info_provider.go index 6bfb9db42874..b0a7b59ce9e3 100644 --- a/cluster-autoscaler/cloudprovider/gce/mig_info_provider.go +++ b/cluster-autoscaler/cloudprovider/gce/mig_info_provider.go @@ -290,13 +290,13 @@ func (c *cachingMigInfoProvider) GetMigInstanceTemplate(migRef GceRef) (*gce.Ins } func (c *cachingMigInfoProvider) GetMigKubeEnv(migRef GceRef) (KubeEnv, error) { - templateName, err := c.GetMigInstanceTemplateName(migRef) + instanceTemplateNameType, err := c.GetMigInstanceTemplateName(migRef) if err != nil { return KubeEnv{}, err } kubeEnv, kubeEnvFound := c.cache.GetMigKubeEnv(migRef) - if kubeEnvFound && kubeEnv.templateName == templateName { + if kubeEnvFound && kubeEnv.templateName == instanceTemplateNameType.Name { return kubeEnv, nil } diff --git a/cluster-autoscaler/cloudprovider/gce/mig_info_provider_test.go b/cluster-autoscaler/cloudprovider/gce/mig_info_provider_test.go index 85b84b731662..4ecb4a1d828a 100644 --- a/cluster-autoscaler/cloudprovider/gce/mig_info_provider_test.go +++ b/cluster-autoscaler/cloudprovider/gce/mig_info_provider_test.go @@ -1075,8 +1075,8 @@ func TestGetMigInstanceKubeEnv(t *testing.T) { name string cache *GceCache fetchMigs func(string) ([]*gce.InstanceGroupManager, error) - fetchMigTemplateName func(GceRef) (string, error) - fetchMigTemplate func(GceRef, string) (*gce.InstanceTemplate, error) + fetchMigTemplateName func(GceRef) (InstanceTemplateNameType, error) + fetchMigTemplate func(GceRef, string, bool) (*gce.InstanceTemplate, error) expectedKubeEnv KubeEnv expectedCachedKubeEnv KubeEnv expectedErr error @@ -1085,7 +1085,7 @@ func TestGetMigInstanceKubeEnv(t *testing.T) { name: "kube-env in cache", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]string{mig.GceRef(): templateName}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): {templateName, false}}, kubeEnvCache: map[GceRef]KubeEnv{mig.GceRef(): kubeEnv}, }, expectedKubeEnv: kubeEnv, @@ -1095,7 +1095,7 @@ func TestGetMigInstanceKubeEnv(t *testing.T) { name: "cache without kube-env, template in cache", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]string{mig.GceRef(): templateName}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): {templateName, false}}, instanceTemplatesCache: map[GceRef]*gce.InstanceTemplate{mig.GceRef(): template}, kubeEnvCache: make(map[GceRef]KubeEnv), }, @@ -1106,7 +1106,7 @@ func TestGetMigInstanceKubeEnv(t *testing.T) { name: "cache without kube-env, fetch success", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]string{mig.GceRef(): templateName}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): {templateName, false}}, instanceTemplatesCache: make(map[GceRef]*gce.InstanceTemplate), kubeEnvCache: make(map[GceRef]KubeEnv), }, @@ -1118,7 +1118,7 @@ func TestGetMigInstanceKubeEnv(t *testing.T) { name: "cache with old kube-env, new template cached", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]string{mig.GceRef(): templateName}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): {templateName, false}}, instanceTemplatesCache: map[GceRef]*gce.InstanceTemplate{mig.GceRef(): template}, kubeEnvCache: map[GceRef]KubeEnv{mig.GceRef(): oldKubeEnv}, }, @@ -1129,7 +1129,7 @@ func TestGetMigInstanceKubeEnv(t *testing.T) { name: "cache with old kube-env, fetch success", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]string{mig.GceRef(): templateName}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): {templateName, false}}, instanceTemplatesCache: map[GceRef]*gce.InstanceTemplate{mig.GceRef(): oldTemplate}, kubeEnvCache: map[GceRef]KubeEnv{mig.GceRef(): oldKubeEnv}, }, @@ -1141,7 +1141,7 @@ func TestGetMigInstanceKubeEnv(t *testing.T) { name: "cache without kube-env, fetch failure", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]string{mig.GceRef(): templateName}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): {templateName, false}}, instanceTemplatesCache: make(map[GceRef]*gce.InstanceTemplate), kubeEnvCache: make(map[GceRef]KubeEnv), }, @@ -1152,7 +1152,7 @@ func TestGetMigInstanceKubeEnv(t *testing.T) { name: "cache with old kube-env, fetch failure", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]string{mig.GceRef(): templateName}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): {templateName, false}}, instanceTemplatesCache: map[GceRef]*gce.InstanceTemplate{mig.GceRef(): oldTemplate}, kubeEnvCache: map[GceRef]KubeEnv{mig.GceRef(): oldKubeEnv}, }, From fd8b89e943da0294fe24db3f37c6996cd69d5497 Mon Sep 17 00:00:00 2001 From: Edwinhr716 Date: Wed, 28 Feb 2024 19:21:02 +0000 Subject: [PATCH 08/12] added fix for unit tests --- .../cloudprovider/gce/mig_info_provider_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cluster-autoscaler/cloudprovider/gce/mig_info_provider_test.go b/cluster-autoscaler/cloudprovider/gce/mig_info_provider_test.go index 4ecb4a1d828a..380ad7f4a8ae 100644 --- a/cluster-autoscaler/cloudprovider/gce/mig_info_provider_test.go +++ b/cluster-autoscaler/cloudprovider/gce/mig_info_provider_test.go @@ -916,14 +916,14 @@ func TestGetMigInstanceTemplateName(t *testing.T) { migLister := NewMigLister(tc.cache) provider := NewCachingMigInfoProvider(tc.cache, migLister, client, mig.GceRef().Project, 1, 0*time.Second) - templateName, err := provider.GetMigInstanceTemplateName(mig.GceRef()) - cachedTemplateName, found := tc.cache.GetMigInstanceTemplateName(mig.GceRef()) + instanceTemplateNameType, err := provider.GetMigInstanceTemplateName(mig.GceRef()) + cachedInstanceTemplateNameType, found := tc.cache.GetMigInstanceTemplateName(mig.GceRef()) assert.Equal(t, tc.expectedErr, err) assert.Equal(t, tc.expectedErr == nil, found) if tc.expectedErr == nil { - assert.Equal(t, tc.expectedTemplateName, templateName) - assert.Equal(t, tc.expectedTemplateName, cachedTemplateName) + assert.Equal(t, tc.expectedTemplateName, instanceTemplateNameType.Name) + assert.Equal(t, tc.expectedTemplateName, cachedInstanceTemplateNameType.Name) } }) } From cd3ab2c4179a4b248645d78037705efa878f7185 Mon Sep 17 00:00:00 2001 From: Edwinhr716 Date: Wed, 28 Feb 2024 22:03:30 +0000 Subject: [PATCH 09/12] changed InstanceTemplateNameType to InstanceTemplateName --- .../gce/autoscaling_gce_client.go | 14 ++-- cluster-autoscaler/cloudprovider/gce/cache.go | 20 +++--- .../cloudprovider/gce/gce_manager_test.go | 2 +- .../cloudprovider/gce/mig_info_provider.go | 34 ++++----- .../gce/mig_info_provider_test.go | 71 +++++++++++-------- 5 files changed, 76 insertions(+), 65 deletions(-) diff --git a/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client.go b/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client.go index f24c05729dba..64b85272b19a 100644 --- a/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client.go +++ b/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client.go @@ -103,7 +103,7 @@ type AutoscalingGceClient interface { FetchMigTargetSize(GceRef) (int64, error) FetchMigBasename(GceRef) (string, error) FetchMigInstances(GceRef) ([]GceInstance, error) - FetchMigTemplateName(migRef GceRef) (InstanceTemplateNameType, error) + FetchMigTemplateName(migRef GceRef) (InstanceTemplateName, error) FetchMigTemplate(migRef GceRef, templateName string, regional bool) (*gce.InstanceTemplate, error) FetchMigsWithName(zone string, filter *regexp.Regexp) ([]string, error) FetchZones(region string) ([]string, error) @@ -580,28 +580,28 @@ func (client *autoscalingGceClientV1) FetchAvailableCpuPlatforms() (map[string][ return availableCpuPlatforms, nil } -func (client *autoscalingGceClientV1) FetchMigTemplateName(migRef GceRef) (InstanceTemplateNameType, error) { +func (client *autoscalingGceClientV1) FetchMigTemplateName(migRef GceRef) (InstanceTemplateName, error) { registerRequest("instance_group_managers", "get") igm, err := client.gceService.InstanceGroupManagers.Get(migRef.Project, migRef.Zone, migRef.Name).Do() if err != nil { if err, ok := err.(*googleapi.Error); ok { if err.Code == http.StatusNotFound { - return InstanceTemplateNameType{}, errors.NewAutoscalerError(errors.NodeGroupDoesNotExistError, "%s", err.Error()) + return InstanceTemplateName{}, errors.NewAutoscalerError(errors.NodeGroupDoesNotExistError, "%s", err.Error()) } } - return InstanceTemplateNameType{}, err + return InstanceTemplateName{}, err } templateUrl, err := url.Parse(igm.InstanceTemplate) if err != nil { - return InstanceTemplateNameType{}, err + return InstanceTemplateName{}, err } regional, err := regexp.MatchString("(/projects/.*[A-Za-z0-9]+.*/regions/)", templateUrl.String()) if err != nil { - return InstanceTemplateNameType{}, err + return InstanceTemplateName{}, err } _, templateName := path.Split(templateUrl.EscapedPath()) - return InstanceTemplateNameType{templateName, regional}, nil + return InstanceTemplateName{templateName, regional}, nil } func (client *autoscalingGceClientV1) FetchMigTemplate(migRef GceRef, templateName string, regional bool) (*gce.InstanceTemplate, error) { diff --git a/cluster-autoscaler/cloudprovider/gce/cache.go b/cluster-autoscaler/cloudprovider/gce/cache.go index 166c59bf3ffe..d31f20dd37b9 100644 --- a/cluster-autoscaler/cloudprovider/gce/cache.go +++ b/cluster-autoscaler/cloudprovider/gce/cache.go @@ -33,9 +33,9 @@ type MachineTypeKey struct { MachineTypeName string } -// InstanceTemplateNameType is used to store the name, and +// InstanceTemplateName is used to store the name, and // whether or not the instance template is regional -type InstanceTemplateNameType struct { +type InstanceTemplateName struct { Name string Regional bool } @@ -74,7 +74,7 @@ type GceCache struct { migTargetSizeCache map[GceRef]int64 migBaseNameCache map[GceRef]string listManagedInstancesResultsCache map[GceRef]string - instanceTemplateNameCache map[GceRef]InstanceTemplateNameType + instanceTemplateNameCache map[GceRef]InstanceTemplateName instanceTemplatesCache map[GceRef]*gce.InstanceTemplate kubeEnvCache map[GceRef]KubeEnv } @@ -92,7 +92,7 @@ func NewGceCache() *GceCache { migTargetSizeCache: map[GceRef]int64{}, migBaseNameCache: map[GceRef]string{}, listManagedInstancesResultsCache: map[GceRef]string{}, - instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateName{}, instanceTemplatesCache: map[GceRef]*gce.InstanceTemplate{}, kubeEnvCache: map[GceRef]KubeEnv{}, } @@ -341,23 +341,23 @@ func (gc *GceCache) InvalidateAllMigTargetSizes() { } // GetMigInstanceTemplateName returns the cached instance template ref for a mig GceRef -func (gc *GceCache) GetMigInstanceTemplateName(ref GceRef) (InstanceTemplateNameType, bool) { +func (gc *GceCache) GetMigInstanceTemplateName(ref GceRef) (InstanceTemplateName, bool) { gc.cacheMutex.Lock() defer gc.cacheMutex.Unlock() - instanceTemplateNameType, found := gc.instanceTemplateNameCache[ref] + instanceTemplateName, found := gc.instanceTemplateNameCache[ref] if found { klog.V(5).Infof("Instance template names cache hit for %s", ref) } - return instanceTemplateNameType, found + return instanceTemplateName, found } // SetMigInstanceTemplateName sets instance template ref for a mig GceRef -func (gc *GceCache) SetMigInstanceTemplateName(ref GceRef, instanceTemplateNameType InstanceTemplateNameType) { +func (gc *GceCache) SetMigInstanceTemplateName(ref GceRef, instanceTemplateName InstanceTemplateName) { gc.cacheMutex.Lock() defer gc.cacheMutex.Unlock() - gc.instanceTemplateNameCache[ref] = instanceTemplateNameType + gc.instanceTemplateNameCache[ref] = instanceTemplateName } // InvalidateMigInstanceTemplateName clears the instance template ref cache for a mig GceRef @@ -377,7 +377,7 @@ func (gc *GceCache) InvalidateAllMigInstanceTemplateNames() { defer gc.cacheMutex.Unlock() klog.V(5).Infof("Instance template names cache invalidated") - gc.instanceTemplateNameCache = map[GceRef]InstanceTemplateNameType{} + gc.instanceTemplateNameCache = map[GceRef]InstanceTemplateName{} } // GetMigInstanceTemplate returns the cached gce.InstanceTemplate for a mig GceRef diff --git a/cluster-autoscaler/cloudprovider/gce/gce_manager_test.go b/cluster-autoscaler/cloudprovider/gce/gce_manager_test.go index 55256acca4ab..207c04deb9b3 100644 --- a/cluster-autoscaler/cloudprovider/gce/gce_manager_test.go +++ b/cluster-autoscaler/cloudprovider/gce/gce_manager_test.go @@ -344,7 +344,7 @@ func newTestGceManager(t *testing.T, testServerURL string, regional bool) *gceMa {"us-central1-f", "n1-standard-1"}: {Name: "n1-standard-1", CPU: 1, Memory: 1}, }, migTargetSizeCache: map[GceRef]int64{}, - instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateName{}, instanceTemplatesCache: map[GceRef]*gce.InstanceTemplate{}, kubeEnvCache: map[GceRef]KubeEnv{}, migBaseNameCache: map[GceRef]string{}, diff --git a/cluster-autoscaler/cloudprovider/gce/mig_info_provider.go b/cluster-autoscaler/cloudprovider/gce/mig_info_provider.go index b0a7b59ce9e3..38a632832d5a 100644 --- a/cluster-autoscaler/cloudprovider/gce/mig_info_provider.go +++ b/cluster-autoscaler/cloudprovider/gce/mig_info_provider.go @@ -44,7 +44,7 @@ type MigInfoProvider interface { // GetMigBasename returns basename for given MIG ref GetMigBasename(migRef GceRef) (string, error) // GetMigInstanceTemplateName returns instance template name for given MIG ref - GetMigInstanceTemplateName(migRef GceRef) (InstanceTemplateNameType, error) + GetMigInstanceTemplateName(migRef GceRef) (InstanceTemplateName, error) // GetMigInstanceTemplate returns instance template for given MIG ref GetMigInstanceTemplate(migRef GceRef) (*gce.InstanceTemplate, error) // GetMigKubeEnv returns kube-env for given MIG ref @@ -244,44 +244,44 @@ func (c *cachingMigInfoProvider) GetMigBasename(migRef GceRef) (string, error) { return basename, nil } -func (c *cachingMigInfoProvider) GetMigInstanceTemplateName(migRef GceRef) (InstanceTemplateNameType, error) { +func (c *cachingMigInfoProvider) GetMigInstanceTemplateName(migRef GceRef) (InstanceTemplateName, error) { c.migInfoMutex.Lock() defer c.migInfoMutex.Unlock() - instanceTemplateNameType, found := c.cache.GetMigInstanceTemplateName(migRef) + instanceTemplateName, found := c.cache.GetMigInstanceTemplateName(migRef) if found { - return instanceTemplateNameType, nil + return instanceTemplateName, nil } err := c.fillMigInfoCache() - instanceTemplateNameType, found = c.cache.GetMigInstanceTemplateName(migRef) + instanceTemplateName, found = c.cache.GetMigInstanceTemplateName(migRef) if err == nil && found { - return instanceTemplateNameType, nil + return instanceTemplateName, nil } // fallback to querying for single mig - instanceTemplateNameType, err = c.gceClient.FetchMigTemplateName(migRef) + instanceTemplateName, err = c.gceClient.FetchMigTemplateName(migRef) if err != nil { c.migLister.HandleMigIssue(migRef, err) - return InstanceTemplateNameType{}, err + return InstanceTemplateName{}, err } - c.cache.SetMigInstanceTemplateName(migRef, instanceTemplateNameType) - return instanceTemplateNameType, nil + c.cache.SetMigInstanceTemplateName(migRef, instanceTemplateName) + return instanceTemplateName, nil } func (c *cachingMigInfoProvider) GetMigInstanceTemplate(migRef GceRef) (*gce.InstanceTemplate, error) { - instanceTemplateNameType, err := c.GetMigInstanceTemplateName(migRef) + instanceTemplateName, err := c.GetMigInstanceTemplateName(migRef) if err != nil { return nil, err } template, found := c.cache.GetMigInstanceTemplate(migRef) - if found && template.Name == instanceTemplateNameType.Name { + if found && template.Name == instanceTemplateName.Name { return template, nil } - klog.V(2).Infof("Instance template of mig %v changed to %v", migRef.Name, instanceTemplateNameType.Name) - template, err = c.gceClient.FetchMigTemplate(migRef, instanceTemplateNameType.Name, instanceTemplateNameType.Regional) + klog.V(2).Infof("Instance template of mig %v changed to %v", migRef.Name, instanceTemplateName.Name) + template, err = c.gceClient.FetchMigTemplate(migRef, instanceTemplateName.Name, instanceTemplateName.Regional) if err != nil { return nil, err } @@ -290,13 +290,13 @@ func (c *cachingMigInfoProvider) GetMigInstanceTemplate(migRef GceRef) (*gce.Ins } func (c *cachingMigInfoProvider) GetMigKubeEnv(migRef GceRef) (KubeEnv, error) { - instanceTemplateNameType, err := c.GetMigInstanceTemplateName(migRef) + instanceTemplateName, err := c.GetMigInstanceTemplateName(migRef) if err != nil { return KubeEnv{}, err } kubeEnv, kubeEnvFound := c.cache.GetMigKubeEnv(migRef) - if kubeEnvFound && kubeEnv.templateName == instanceTemplateNameType.Name { + if kubeEnvFound && kubeEnv.templateName == instanceTemplateName.Name { return kubeEnv, nil } @@ -365,7 +365,7 @@ func (c *cachingMigInfoProvider) fillMigInfoCache() error { if err == nil { _, templateName := path.Split(templateUrl.EscapedPath()) regional, _ := regexp.MatchString("(/projects/.*[A-Za-z0-9]+.*/regions/)", templateUrl.String()) - c.cache.SetMigInstanceTemplateName(zoneMigRef, InstanceTemplateNameType{templateName, regional}) + c.cache.SetMigInstanceTemplateName(zoneMigRef, InstanceTemplateName{templateName, regional}) } } } diff --git a/cluster-autoscaler/cloudprovider/gce/mig_info_provider_test.go b/cluster-autoscaler/cloudprovider/gce/mig_info_provider_test.go index 380ad7f4a8ae..f01d928c8dbf 100644 --- a/cluster-autoscaler/cloudprovider/gce/mig_info_provider_test.go +++ b/cluster-autoscaler/cloudprovider/gce/mig_info_provider_test.go @@ -53,7 +53,7 @@ type mockAutoscalingGceClient struct { fetchMigTargetSize func(GceRef) (int64, error) fetchMigBasename func(GceRef) (string, error) fetchMigInstances func(GceRef) ([]GceInstance, error) - fetchMigTemplateName func(GceRef) (InstanceTemplateNameType, error) + fetchMigTemplate func(GceRef, string, bool) (*gce.InstanceTemplate, error) fetchMigTemplate func(GceRef, string) (*gce.InstanceTemplate, error) fetchMachineType func(string, string) (*gce.MachineType, error) fetchListManagedInstancesResults func(GceRef) (string, error) @@ -87,7 +87,7 @@ func (client *mockAutoscalingGceClient) FetchMigInstances(migRef GceRef) ([]GceI return client.fetchMigInstances(migRef) } -func (client *mockAutoscalingGceClient) FetchMigTemplateName(migRef GceRef) (InstanceTemplateNameType, error) { +func (client *mockAutoscalingGceClient) FetchMigTemplateName(migRef GceRef) (InstanceTemplateName, error) { return client.fetchMigTemplateName(migRef) } @@ -859,7 +859,7 @@ func TestGetMigInstanceTemplateName(t *testing.T) { name string cache *GceCache fetchMigs func(string) ([]*gce.InstanceGroupManager, error) - fetchMigTemplateName func(GceRef) (InstanceTemplateNameType, error) + fetchMigTemplateName func(GceRef) (InstanceTemplateName, error) expectedTemplateName string expectedErr error }{ @@ -867,7 +867,7 @@ func TestGetMigInstanceTemplateName(t *testing.T) { name: "template name in cache", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): {templateName, false}}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateName{mig.GceRef(): {templateName, false}}, }, expectedTemplateName: templateName, }, @@ -881,14 +881,14 @@ func TestGetMigInstanceTemplateName(t *testing.T) { name: "cache fill without mig, fallback success", cache: emptyCache(), fetchMigs: fetchMigsConst([]*gce.InstanceGroupManager{}), - fetchMigTemplateName: fetchMigTemplateNameConst(InstanceTemplateNameType{templateName, false}), + fetchMigTemplateName: fetchMigTemplateNameConst(InstanceTemplateName{templateName, false}), expectedTemplateName: templateName, }, { name: "cache fill failure, fallback success", cache: emptyCache(), fetchMigs: fetchMigsFail, - fetchMigTemplateName: fetchMigTemplateNameConst(InstanceTemplateNameType{templateName, false}), + fetchMigTemplateName: fetchMigTemplateNameConst(InstanceTemplateName{templateName, false}), expectedTemplateName: templateName, }, { @@ -916,14 +916,14 @@ func TestGetMigInstanceTemplateName(t *testing.T) { migLister := NewMigLister(tc.cache) provider := NewCachingMigInfoProvider(tc.cache, migLister, client, mig.GceRef().Project, 1, 0*time.Second) - instanceTemplateNameType, err := provider.GetMigInstanceTemplateName(mig.GceRef()) - cachedInstanceTemplateNameType, found := tc.cache.GetMigInstanceTemplateName(mig.GceRef()) + instanceTemplateName, err := provider.GetMigInstanceTemplateName(mig.GceRef()) + cachedInstanceTemplateName, found := tc.cache.GetMigInstanceTemplateName(mig.GceRef()) assert.Equal(t, tc.expectedErr, err) assert.Equal(t, tc.expectedErr == nil, found) if tc.expectedErr == nil { - assert.Equal(t, tc.expectedTemplateName, instanceTemplateNameType.Name) - assert.Equal(t, tc.expectedTemplateName, cachedInstanceTemplateNameType.Name) + assert.Equal(t, tc.expectedTemplateName, instanceTemplateName.Name) + assert.Equal(t, tc.expectedTemplateName, cachedInstanceTemplateName.Name) } }) } @@ -944,7 +944,7 @@ func TestGetMigInstanceTemplate(t *testing.T) { name string cache *GceCache fetchMigs func(string) ([]*gce.InstanceGroupManager, error) - fetchMigTemplateName func(GceRef) (InstanceTemplateNameType, error) + fetchMigTemplateName func(GceRef) (InstanceTemplateName, error) fetchMigTemplate func(GceRef, string, bool) (*gce.InstanceTemplate, error) expectedTemplate *gce.InstanceTemplate expectedCachedTemplate *gce.InstanceTemplate @@ -954,7 +954,7 @@ func TestGetMigInstanceTemplate(t *testing.T) { name: "template in cache", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): {templateName, false}}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateName{mig.GceRef(): {templateName, false}}, instanceTemplatesCache: map[GceRef]*gce.InstanceTemplate{mig.GceRef(): template}, }, expectedTemplate: template, @@ -964,7 +964,7 @@ func TestGetMigInstanceTemplate(t *testing.T) { name: "cache without template, fetch success", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): {templateName, false}}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateName{mig.GceRef(): {templateName, false}}, instanceTemplatesCache: make(map[GceRef]*gce.InstanceTemplate), }, fetchMigTemplate: fetchMigTemplateConst(template), @@ -975,7 +975,7 @@ func TestGetMigInstanceTemplate(t *testing.T) { name: "cache with old template, fetch success", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): {templateName, false}}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateName{mig.GceRef(): {templateName, false}}, instanceTemplatesCache: map[GceRef]*gce.InstanceTemplate{mig.GceRef(): oldTemplate}, }, fetchMigTemplate: fetchMigTemplateConst(template), @@ -986,7 +986,7 @@ func TestGetMigInstanceTemplate(t *testing.T) { name: "cache without template, fetch failure", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): {templateName, false}}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateName{mig.GceRef(): {templateName, false}}, instanceTemplatesCache: make(map[GceRef]*gce.InstanceTemplate), }, fetchMigTemplate: fetchMigTemplateFail, @@ -996,7 +996,7 @@ func TestGetMigInstanceTemplate(t *testing.T) { name: "cache with old template, fetch failure", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): {templateName, false}}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateName{mig.GceRef(): {templateName, false}}, instanceTemplatesCache: map[GceRef]*gce.InstanceTemplate{mig.GceRef(): oldTemplate}, }, fetchMigTemplate: fetchMigTemplateFail, @@ -1075,7 +1075,7 @@ func TestGetMigInstanceKubeEnv(t *testing.T) { name string cache *GceCache fetchMigs func(string) ([]*gce.InstanceGroupManager, error) - fetchMigTemplateName func(GceRef) (InstanceTemplateNameType, error) + fetchMigTemplateName func(GceRef) (InstanceTemplateName, error) fetchMigTemplate func(GceRef, string, bool) (*gce.InstanceTemplate, error) expectedKubeEnv KubeEnv expectedCachedKubeEnv KubeEnv @@ -1085,7 +1085,7 @@ func TestGetMigInstanceKubeEnv(t *testing.T) { name: "kube-env in cache", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): {templateName, false}}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateName{mig.GceRef(): {templateName, false}}, kubeEnvCache: map[GceRef]KubeEnv{mig.GceRef(): kubeEnv}, }, expectedKubeEnv: kubeEnv, @@ -1095,7 +1095,7 @@ func TestGetMigInstanceKubeEnv(t *testing.T) { name: "cache without kube-env, template in cache", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): {templateName, false}}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateName{mig.GceRef(): {templateName, false}}, instanceTemplatesCache: map[GceRef]*gce.InstanceTemplate{mig.GceRef(): template}, kubeEnvCache: make(map[GceRef]KubeEnv), }, @@ -1106,7 +1106,7 @@ func TestGetMigInstanceKubeEnv(t *testing.T) { name: "cache without kube-env, fetch success", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): {templateName, false}}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateName{mig.GceRef(): {templateName, false}}, instanceTemplatesCache: make(map[GceRef]*gce.InstanceTemplate), kubeEnvCache: make(map[GceRef]KubeEnv), }, @@ -1118,7 +1118,7 @@ func TestGetMigInstanceKubeEnv(t *testing.T) { name: "cache with old kube-env, new template cached", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): {templateName, false}}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateName{mig.GceRef(): {templateName, false}}, instanceTemplatesCache: map[GceRef]*gce.InstanceTemplate{mig.GceRef(): template}, kubeEnvCache: map[GceRef]KubeEnv{mig.GceRef(): oldKubeEnv}, }, @@ -1129,7 +1129,7 @@ func TestGetMigInstanceKubeEnv(t *testing.T) { name: "cache with old kube-env, fetch success", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): {templateName, false}}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateName{mig.GceRef(): {templateName, false}}, instanceTemplatesCache: map[GceRef]*gce.InstanceTemplate{mig.GceRef(): oldTemplate}, kubeEnvCache: map[GceRef]KubeEnv{mig.GceRef(): oldKubeEnv}, }, @@ -1141,7 +1141,7 @@ func TestGetMigInstanceKubeEnv(t *testing.T) { name: "cache without kube-env, fetch failure", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): {templateName, false}}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateName{mig.GceRef(): {templateName, false}}, instanceTemplatesCache: make(map[GceRef]*gce.InstanceTemplate), kubeEnvCache: make(map[GceRef]KubeEnv), }, @@ -1152,7 +1152,7 @@ func TestGetMigInstanceKubeEnv(t *testing.T) { name: "cache with old kube-env, fetch failure", cache: &GceCache{ migs: map[GceRef]Mig{mig.GceRef(): mig}, - instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): {templateName, false}}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateName{mig.GceRef(): {templateName, false}}, instanceTemplatesCache: map[GceRef]*gce.InstanceTemplate{mig.GceRef(): oldTemplate}, kubeEnvCache: map[GceRef]KubeEnv{mig.GceRef(): oldKubeEnv}, }, @@ -1251,7 +1251,7 @@ func TestGetMigMachineType(t *testing.T) { }, } cache := &GceCache{ - instanceTemplateNameCache: map[GceRef]InstanceTemplateNameType{mig.GceRef(): {"template", false}}, + instanceTemplateNameCache: map[GceRef]InstanceTemplateName{mig.GceRef(): {"template", false}}, instanceTemplatesCache: map[GceRef]*gce.InstanceTemplate{ mig.GceRef(): { Name: "template", @@ -1371,6 +1371,7 @@ func (f *fakeTime) Now() time.Time { func emptyCache() *GceCache { return &GceCache{ +<<<<<<< HEAD migs: map[GceRef]Mig{mig.GceRef(): mig}, instances: make(map[GceRef][]GceInstance), instancesUpdateTime: make(map[GceRef]time.Time), @@ -1380,6 +1381,16 @@ func emptyCache() *GceCache { instanceTemplateNameCache: make(map[GceRef]InstanceTemplateNameType), instanceTemplatesCache: make(map[GceRef]*gce.InstanceTemplate), instancesFromUnknownMig: make(map[GceRef]bool), +======= + migs: map[GceRef]Mig{mig.GceRef(): mig}, + instances: make(map[GceRef][]GceInstance), + instancesUpdateTime: make(map[GceRef]time.Time), + migTargetSizeCache: make(map[GceRef]int64), + migBaseNameCache: make(map[GceRef]string), + instanceTemplateNameCache: make(map[GceRef]InstanceTemplateName), + instanceTemplatesCache: make(map[GceRef]*gce.InstanceTemplate), + instancesFromUnknownMig: make(map[GceRef]bool), +>>>>>>> 2aeec4097 (changed InstanceTemplateNameType to InstanceTemplateName) } } @@ -1436,13 +1447,13 @@ func fetchMigBasenameConst(basename string) func(GceRef) (string, error) { } } -func fetchMigTemplateNameFail(_ GceRef) (InstanceTemplateNameType, error) { - return InstanceTemplateNameType{}, errFetchMigTemplateName +func fetchMigTemplateNameFail(_ GceRef) (InstanceTemplateName, error) { + return InstanceTemplateName{}, errFetchMigTemplateName } -func fetchMigTemplateNameConst(instanceTemplateNameType InstanceTemplateNameType) func(GceRef) (InstanceTemplateNameType, error) { - return func(GceRef) (InstanceTemplateNameType, error) { - return instanceTemplateNameType, nil +func fetchMigTemplateNameConst(instanceTemplateName InstanceTemplateName) func(GceRef) (InstanceTemplateName, error) { + return func(GceRef) (InstanceTemplateName, error) { + return instanceTemplateName, nil } } From 5345defdbc2380744fe99aa97da8481c0b31bc02 Mon Sep 17 00:00:00 2001 From: Edwinhr716 Date: Thu, 7 Mar 2024 19:35:06 +0000 Subject: [PATCH 10/12] separated url parser to its own function, created unit test for the function --- .../gce/autoscaling_gce_client.go | 2 +- .../cloudprovider/gce/gce_url.go | 5 +++ .../cloudprovider/gce/gce_url_test.go | 31 +++++++++++++++++++ .../cloudprovider/gce/mig_info_provider.go | 9 ++++-- 4 files changed, 43 insertions(+), 4 deletions(-) diff --git a/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client.go b/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client.go index 64b85272b19a..cbd2eff6a492 100644 --- a/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client.go +++ b/cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client.go @@ -595,7 +595,7 @@ func (client *autoscalingGceClientV1) FetchMigTemplateName(migRef GceRef) (Insta if err != nil { return InstanceTemplateName{}, err } - regional, err := regexp.MatchString("(/projects/.*[A-Za-z0-9]+.*/regions/)", templateUrl.String()) + regional, err := IsInstanceTemplateRegional(templateUrl.String()) if err != nil { return InstanceTemplateName{}, err } diff --git a/cluster-autoscaler/cloudprovider/gce/gce_url.go b/cluster-autoscaler/cloudprovider/gce/gce_url.go index 7b0ab135a46c..863f996002a1 100644 --- a/cluster-autoscaler/cloudprovider/gce/gce_url.go +++ b/cluster-autoscaler/cloudprovider/gce/gce_url.go @@ -78,6 +78,11 @@ func GenerateMigUrl(domainUrl string, ref GceRef) string { return fmt.Sprintf(migUrlTemplate, ref.Project, ref.Zone, ref.Name) } +// IsInstanceTemplateRegional verifies if the instance template is regional or global +func IsInstanceTemplateRegional(templateUrl string) (bool, error) { + return regexp.MatchString("(/projects/.*[A-Za-z0-9]+.*/regions/)", templateUrl) +} + func parseGceUrl(url, expectedResource string) (project string, zone string, name string, err error) { reg := regexp.MustCompile(fmt.Sprintf("https://.*/projects/.*/zones/.*/%s/.*", expectedResource)) errMsg := fmt.Errorf("wrong url: expected format /projects//zones//%s/, got %s", expectedResource, url) diff --git a/cluster-autoscaler/cloudprovider/gce/gce_url_test.go b/cluster-autoscaler/cloudprovider/gce/gce_url_test.go index 1317ef76e605..feee3b2432ae 100644 --- a/cluster-autoscaler/cloudprovider/gce/gce_url_test.go +++ b/cluster-autoscaler/cloudprovider/gce/gce_url_test.go @@ -263,3 +263,34 @@ func TestParseMigUrl(t *testing.T) { }) } } + +func TestIsInstanceTemplateRegional(t *testing.T) { + tests := []struct { + name string + templateUrl string + expectRegional bool + wantErr error + }{ + { + name: "Has regional instance url", + templateUrl: "https://www.googleapis.com/compute/v1/projects/test-project/regions/us-central1/instanceTemplates/instance-template", + expectRegional: true, + }, + { + name: "Has global instance url", + templateUrl: "https://www.googleapis.com/compute/v1/projects/test-project/global/instanceTemplates/instance-template", + expectRegional: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + regional, err := IsInstanceTemplateRegional(tt.templateUrl) + assert.Equal(t, tt.wantErr, err) + if tt.wantErr != nil { + return + } + assert.Equal(t, tt.expectRegional, regional) + }) + } +} diff --git a/cluster-autoscaler/cloudprovider/gce/mig_info_provider.go b/cluster-autoscaler/cloudprovider/gce/mig_info_provider.go index 38a632832d5a..ad407bec1b10 100644 --- a/cluster-autoscaler/cloudprovider/gce/mig_info_provider.go +++ b/cluster-autoscaler/cloudprovider/gce/mig_info_provider.go @@ -21,7 +21,6 @@ import ( "fmt" "net/url" "path" - "regexp" "strings" "sync" "time" @@ -364,8 +363,12 @@ func (c *cachingMigInfoProvider) fillMigInfoCache() error { templateUrl, err := url.Parse(zoneMig.InstanceTemplate) if err == nil { _, templateName := path.Split(templateUrl.EscapedPath()) - regional, _ := regexp.MatchString("(/projects/.*[A-Za-z0-9]+.*/regions/)", templateUrl.String()) - c.cache.SetMigInstanceTemplateName(zoneMigRef, InstanceTemplateName{templateName, regional}) + regional, err := IsInstanceTemplateRegional(templateUrl.String()) + if err != nil { + klog.Errorf("Error parsing instance template url: %v; err=%v ", templateUrl.String(), err) + } else { + c.cache.SetMigInstanceTemplateName(zoneMigRef, InstanceTemplateName{templateName, regional}) + } } } } From f8b4727c1eac1a76596b16cafdbd0b99c0b4580c Mon Sep 17 00:00:00 2001 From: Edwinhr716 Date: Thu, 7 Mar 2024 19:35:06 +0000 Subject: [PATCH 11/12] separated url parser to its own function, created unit test for the function --- cluster-autoscaler/cloudprovider/gce/gce_url.go | 1 - 1 file changed, 1 deletion(-) diff --git a/cluster-autoscaler/cloudprovider/gce/gce_url.go b/cluster-autoscaler/cloudprovider/gce/gce_url.go index 863f996002a1..72fc3e774550 100644 --- a/cluster-autoscaler/cloudprovider/gce/gce_url.go +++ b/cluster-autoscaler/cloudprovider/gce/gce_url.go @@ -78,7 +78,6 @@ func GenerateMigUrl(domainUrl string, ref GceRef) string { return fmt.Sprintf(migUrlTemplate, ref.Project, ref.Zone, ref.Name) } -// IsInstanceTemplateRegional verifies if the instance template is regional or global func IsInstanceTemplateRegional(templateUrl string) (bool, error) { return regexp.MatchString("(/projects/.*[A-Za-z0-9]+.*/regions/)", templateUrl) } From 771e9327d29852631f80787696277ade086628ba Mon Sep 17 00:00:00 2001 From: Edwinhr716 Date: Thu, 7 Mar 2024 20:09:00 +0000 Subject: [PATCH 12/12] added unit test with regional MIG --- .../cloudprovider/gce/gce_url.go | 1 + .../cloudprovider/gce/kube_env.go | 19 ------------ .../gce/mig_info_provider_test.go | 30 ++++++++++--------- 3 files changed, 17 insertions(+), 33 deletions(-) diff --git a/cluster-autoscaler/cloudprovider/gce/gce_url.go b/cluster-autoscaler/cloudprovider/gce/gce_url.go index 72fc3e774550..80ef91207ff0 100644 --- a/cluster-autoscaler/cloudprovider/gce/gce_url.go +++ b/cluster-autoscaler/cloudprovider/gce/gce_url.go @@ -78,6 +78,7 @@ func GenerateMigUrl(domainUrl string, ref GceRef) string { return fmt.Sprintf(migUrlTemplate, ref.Project, ref.Zone, ref.Name) } +// IsInstanceTemplateRegional determines whether or not an instance template is regional based on the url func IsInstanceTemplateRegional(templateUrl string) (bool, error) { return regexp.MatchString("(/projects/.*[A-Za-z0-9]+.*/regions/)", templateUrl) } diff --git a/cluster-autoscaler/cloudprovider/gce/kube_env.go b/cluster-autoscaler/cloudprovider/gce/kube_env.go index 4cf1354baee4..a772948eed32 100644 --- a/cluster-autoscaler/cloudprovider/gce/kube_env.go +++ b/cluster-autoscaler/cloudprovider/gce/kube_env.go @@ -71,22 +71,3 @@ func (ke KubeEnv) Var(name string) (string, bool) { val, found := ke.env[name] return val, found } - -// ParseKubeEnv parses kube-env from its string representation -func ParseKubeEnv(templateName, kubeEnvValue string) (KubeEnv, error) { - env := make(map[string]string) - err := yaml.Unmarshal([]byte(kubeEnvValue), &env) - if err != nil { - return KubeEnv{}, fmt.Errorf("error unmarshalling kubeEnv: %v", err) - } - return KubeEnv{templateName: templateName, env: env}, nil -} - -// Var extracts variable from KubeEnv -func (ke KubeEnv) Var(name string) (string, bool) { - if ke.env == nil { - return "", false - } - val, found := ke.env[name] - return val, found -} diff --git a/cluster-autoscaler/cloudprovider/gce/mig_info_provider_test.go b/cluster-autoscaler/cloudprovider/gce/mig_info_provider_test.go index f01d928c8dbf..838a36ba6134 100644 --- a/cluster-autoscaler/cloudprovider/gce/mig_info_provider_test.go +++ b/cluster-autoscaler/cloudprovider/gce/mig_info_provider_test.go @@ -53,8 +53,8 @@ type mockAutoscalingGceClient struct { fetchMigTargetSize func(GceRef) (int64, error) fetchMigBasename func(GceRef) (string, error) fetchMigInstances func(GceRef) ([]GceInstance, error) + fetchMigTemplateName func(GceRef) (InstanceTemplateName, error) fetchMigTemplate func(GceRef, string, bool) (*gce.InstanceTemplate, error) - fetchMigTemplate func(GceRef, string) (*gce.InstanceTemplate, error) fetchMachineType func(string, string) (*gce.MachineType, error) fetchListManagedInstancesResults func(GceRef) (string, error) } @@ -852,7 +852,13 @@ func TestGetMigInstanceTemplateName(t *testing.T) { instanceGroupManager := &gce.InstanceGroupManager{ Zone: mig.GceRef().Zone, Name: mig.GceRef().Name, - InstanceTemplate: templateName, + InstanceTemplate: "https://www.googleapis.com/compute/v1/projects/test-project/global/instanceTemplates/template-name", + } + + instanceGroupManagerRegional := &gce.InstanceGroupManager{ + Zone: mig.GceRef().Zone, + Name: mig.GceRef().Name, + InstanceTemplate: "https://www.googleapis.com/compute/v1/projects/test-project/regions/us-central1/instanceTemplates/template-name", } testCases := []struct { @@ -861,6 +867,7 @@ func TestGetMigInstanceTemplateName(t *testing.T) { fetchMigs func(string) ([]*gce.InstanceGroupManager, error) fetchMigTemplateName func(GceRef) (InstanceTemplateName, error) expectedTemplateName string + expectedRegion bool expectedErr error }{ { @@ -877,6 +884,12 @@ func TestGetMigInstanceTemplateName(t *testing.T) { fetchMigs: fetchMigsConst([]*gce.InstanceGroupManager{instanceGroupManager}), expectedTemplateName: templateName, }, + { + name: "target size from cache fill, regional", + cache: emptyCache(), + fetchMigs: fetchMigsConst([]*gce.InstanceGroupManager{instanceGroupManagerRegional}), + expectedTemplateName: templateName, + }, { name: "cache fill without mig, fallback success", cache: emptyCache(), @@ -1371,26 +1384,15 @@ func (f *fakeTime) Now() time.Time { func emptyCache() *GceCache { return &GceCache{ -<<<<<<< HEAD migs: map[GceRef]Mig{mig.GceRef(): mig}, instances: make(map[GceRef][]GceInstance), instancesUpdateTime: make(map[GceRef]time.Time), migTargetSizeCache: make(map[GceRef]int64), migBaseNameCache: make(map[GceRef]string), listManagedInstancesResultsCache: make(map[GceRef]string), - instanceTemplateNameCache: make(map[GceRef]InstanceTemplateNameType), + instanceTemplateNameCache: make(map[GceRef]InstanceTemplateName), instanceTemplatesCache: make(map[GceRef]*gce.InstanceTemplate), instancesFromUnknownMig: make(map[GceRef]bool), -======= - migs: map[GceRef]Mig{mig.GceRef(): mig}, - instances: make(map[GceRef][]GceInstance), - instancesUpdateTime: make(map[GceRef]time.Time), - migTargetSizeCache: make(map[GceRef]int64), - migBaseNameCache: make(map[GceRef]string), - instanceTemplateNameCache: make(map[GceRef]InstanceTemplateName), - instanceTemplatesCache: make(map[GceRef]*gce.InstanceTemplate), - instancesFromUnknownMig: make(map[GceRef]bool), ->>>>>>> 2aeec4097 (changed InstanceTemplateNameType to InstanceTemplateName) } }