From 2b6087e9a16b6c87a2a5fdd2fc67d8481823a16a Mon Sep 17 00:00:00 2001 From: Pengfei Ni Date: Mon, 4 Mar 2019 14:22:35 +0800 Subject: [PATCH 1/4] Revert "Add GetInstanceID interface for cloudprovider" This reverts commit a9758b2b15d02feffb94d5359ec0c8cc796baa75. --- cluster-autoscaler/cloudprovider/cloud_provider.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/cluster-autoscaler/cloudprovider/cloud_provider.go b/cluster-autoscaler/cloudprovider/cloud_provider.go index 0187c2178b1..8eb252efe31 100644 --- a/cluster-autoscaler/cloudprovider/cloud_provider.go +++ b/cluster-autoscaler/cloudprovider/cloud_provider.go @@ -56,9 +56,6 @@ type CloudProvider interface { // GetResourceLimiter returns struct containing limits (max, min) for resources (cores, memory etc.). GetResourceLimiter() (*ResourceLimiter, error) - // GetInstanceID gets the instance ID for the specified node. - GetInstanceID(node *apiv1.Node) string - // Cleanup cleans up open resources before the cloud provider is destroyed, i.e. go routines etc. Cleanup() error From b721438315ad6b7820ff4cdbe5cd7f80fdbe16d2 Mon Sep 17 00:00:00 2001 From: Pengfei Ni Date: Mon, 4 Mar 2019 14:23:13 +0800 Subject: [PATCH 2/4] Revert "Use cloudProvider.GetInstanceID() to get unregistered nodes" This reverts commit f4ef957ecd32f29852ef52fcbe17856add501bb8. --- cluster-autoscaler/clusterstate/clusterstate.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cluster-autoscaler/clusterstate/clusterstate.go b/cluster-autoscaler/clusterstate/clusterstate.go index 02e5b2017a0..390ec2f6eee 100644 --- a/cluster-autoscaler/clusterstate/clusterstate.go +++ b/cluster-autoscaler/clusterstate/clusterstate.go @@ -281,7 +281,7 @@ func (csr *ClusterStateRegistry) UpdateNodes(nodes []*apiv1.Node, nodeInfosForGr if err != nil { return err } - notRegistered := getNotRegisteredNodes(csr.cloudProvider, nodes, cloudProviderNodeInstances, currentTime) + notRegistered := getNotRegisteredNodes(nodes, cloudProviderNodeInstances, currentTime) csr.Lock() defer csr.Unlock() @@ -932,10 +932,10 @@ func getCloudProviderNodeInstances(cloudProvider cloudprovider.CloudProvider) (m } // Calculates which of the existing cloud provider nodes are not registered in Kubernetes. -func getNotRegisteredNodes(cloudProvider cloudprovider.CloudProvider, allNodes []*apiv1.Node, cloudProviderNodeInstances map[string][]cloudprovider.Instance, time time.Time) []UnregisteredNode { +func getNotRegisteredNodes(allNodes []*apiv1.Node, cloudProviderNodeInstances map[string][]cloudprovider.Instance, time time.Time) []UnregisteredNode { registered := sets.NewString() for _, node := range allNodes { - registered.Insert(cloudProvider.GetInstanceID(node)) + registered.Insert(node.Spec.ProviderID) } notRegistered := make([]UnregisteredNode, 0) for _, instances := range cloudProviderNodeInstances { From ceee6a4515a4207a8736da824918a9c34ba8f48d Mon Sep 17 00:00:00 2001 From: Pengfei Ni Date: Mon, 4 Mar 2019 14:23:28 +0800 Subject: [PATCH 3/4] Revert "Implement GetInstanceID for other cloud providers" This reverts commit 2e2aab6e28d9ecca13579fe0e9a32faabf51043d. --- .../alicloud/alicloud_cloud_provider.go | 5 ----- .../cloudprovider/aws/aws_cloud_provider.go | 5 ----- .../baiducloud/baiducloud_cloud_provider.go | 5 ----- .../cloudprovider/gce/gce_cloud_provider.go | 5 ----- .../cloudprovider/gke/gke_cloud_provider.go | 5 ----- .../cloudprovider/kubemark/kubemark_linux.go | 5 ----- .../cloudprovider/kubemark/kubemark_other.go | 5 ----- .../cloudprovider/mocks/CloudProvider.go | 14 -------------- .../cloudprovider/test/test_cloud_provider.go | 5 ----- 9 files changed, 54 deletions(-) diff --git a/cluster-autoscaler/cloudprovider/alicloud/alicloud_cloud_provider.go b/cluster-autoscaler/cloudprovider/alicloud/alicloud_cloud_provider.go index d3dcdbc838d..07c8a7a6bce 100644 --- a/cluster-autoscaler/cloudprovider/alicloud/alicloud_cloud_provider.go +++ b/cluster-autoscaler/cloudprovider/alicloud/alicloud_cloud_provider.go @@ -140,11 +140,6 @@ func (ali *aliCloudProvider) Cleanup() error { return nil } -// GetInstanceID gets the instance ID for the specified node. -func (ali *aliCloudProvider) GetInstanceID(node *apiv1.Node) string { - return node.Spec.ProviderID -} - // AliRef contains a reference to ECS instance or . type AliRef struct { ID string diff --git a/cluster-autoscaler/cloudprovider/aws/aws_cloud_provider.go b/cluster-autoscaler/cloudprovider/aws/aws_cloud_provider.go index 62fe6184577..bddecab4ed9 100644 --- a/cluster-autoscaler/cloudprovider/aws/aws_cloud_provider.go +++ b/cluster-autoscaler/cloudprovider/aws/aws_cloud_provider.go @@ -127,11 +127,6 @@ func (aws *awsCloudProvider) Refresh() error { return aws.awsManager.Refresh() } -// GetInstanceID gets the instance ID for the specified node. -func (aws *awsCloudProvider) GetInstanceID(node *apiv1.Node) string { - return node.Spec.ProviderID -} - // AwsRef contains a reference to some entity in AWS world. type AwsRef struct { Name string diff --git a/cluster-autoscaler/cloudprovider/baiducloud/baiducloud_cloud_provider.go b/cluster-autoscaler/cloudprovider/baiducloud/baiducloud_cloud_provider.go index d6e16efd4be..f99e60c131f 100644 --- a/cluster-autoscaler/cloudprovider/baiducloud/baiducloud_cloud_provider.go +++ b/cluster-autoscaler/cloudprovider/baiducloud/baiducloud_cloud_provider.go @@ -196,11 +196,6 @@ func (baiducloud *baiducloudCloudProvider) Refresh() error { return nil } -// GetInstanceID gets the instance ID for the specified node. -func (baiducloud *baiducloudCloudProvider) GetInstanceID(node *apiv1.Node) string { - return node.Spec.ProviderID -} - // BaiducloudRef contains a reference to some entity in baiducloud world. type BaiducloudRef struct { Name string diff --git a/cluster-autoscaler/cloudprovider/gce/gce_cloud_provider.go b/cluster-autoscaler/cloudprovider/gce/gce_cloud_provider.go index 80974b37c67..7741c612809 100644 --- a/cluster-autoscaler/cloudprovider/gce/gce_cloud_provider.go +++ b/cluster-autoscaler/cloudprovider/gce/gce_cloud_provider.go @@ -114,11 +114,6 @@ func (gce *GceCloudProvider) Refresh() error { return gce.gceManager.Refresh() } -// GetInstanceID gets the instance ID for the specified node. -func (gce *GceCloudProvider) GetInstanceID(node *apiv1.Node) string { - return node.Spec.ProviderID -} - // GceRef contains s reference to some entity in GCE world. type GceRef struct { Project string diff --git a/cluster-autoscaler/cloudprovider/gke/gke_cloud_provider.go b/cluster-autoscaler/cloudprovider/gke/gke_cloud_provider.go index 76cc7fb8d68..a6ce8f3b3f5 100644 --- a/cluster-autoscaler/cloudprovider/gke/gke_cloud_provider.go +++ b/cluster-autoscaler/cloudprovider/gke/gke_cloud_provider.go @@ -213,11 +213,6 @@ func (gke *GkeCloudProvider) GetNodeLocations() []string { return gke.gkeManager.GetNodeLocations() } -// GetInstanceID gets the instance ID for the specified node. -func (gke *GkeCloudProvider) GetInstanceID(node *apiv1.Node) string { - return node.Spec.ProviderID -} - // MigSpec contains information about what machines in a MIG look like. type MigSpec struct { MachineType string diff --git a/cluster-autoscaler/cloudprovider/kubemark/kubemark_linux.go b/cluster-autoscaler/cloudprovider/kubemark/kubemark_linux.go index eddeeb13c7a..8b556cf6662 100644 --- a/cluster-autoscaler/cloudprovider/kubemark/kubemark_linux.go +++ b/cluster-autoscaler/cloudprovider/kubemark/kubemark_linux.go @@ -135,11 +135,6 @@ func (kubemark *KubemarkCloudProvider) Refresh() error { return nil } -// GetInstanceID gets the instance ID for the specified node. -func (kubemark *KubemarkCloudProvider) GetInstanceID(node *apiv1.Node) string { - return node.Spec.ProviderID -} - // Cleanup cleans up all resources before the cloud provider is removed func (kubemark *KubemarkCloudProvider) Cleanup() error { return nil diff --git a/cluster-autoscaler/cloudprovider/kubemark/kubemark_other.go b/cluster-autoscaler/cloudprovider/kubemark/kubemark_other.go index 47af4256565..f7b0afd32cb 100644 --- a/cluster-autoscaler/cloudprovider/kubemark/kubemark_other.go +++ b/cluster-autoscaler/cloudprovider/kubemark/kubemark_other.go @@ -85,11 +85,6 @@ func (kubemark *KubemarkCloudProvider) Refresh() error { return cloudprovider.ErrNotImplemented } -// GetInstanceID gets the instance ID for the specified node. -func (kubemark *KubemarkCloudProvider) GetInstanceID(node *apiv1.Node) string { - return "" -} - // Cleanup cleans up all resources before the cloud provider is removed func (kubemark *KubemarkCloudProvider) Cleanup() error { return cloudprovider.ErrNotImplemented diff --git a/cluster-autoscaler/cloudprovider/mocks/CloudProvider.go b/cluster-autoscaler/cloudprovider/mocks/CloudProvider.go index ce4cdd186f9..6a6c9d948d2 100644 --- a/cluster-autoscaler/cloudprovider/mocks/CloudProvider.go +++ b/cluster-autoscaler/cloudprovider/mocks/CloudProvider.go @@ -201,17 +201,3 @@ func (_m *CloudProvider) Refresh() error { return r0 } - -// GetInstanceID gets the instance ID for the specified node. -func (_m *CloudProvider) GetInstanceID(node *v1.Node) string { - ret := _m.Called() - - var r0 string - if rf, ok := ret.Get(0).(func() string); ok { - r0 = rf() - } else { - r0 = ret.Get(0).(string) - } - - return r0 -} diff --git a/cluster-autoscaler/cloudprovider/test/test_cloud_provider.go b/cluster-autoscaler/cloudprovider/test/test_cloud_provider.go index 97e2a65a5df..92818131e82 100644 --- a/cluster-autoscaler/cloudprovider/test/test_cloud_provider.go +++ b/cluster-autoscaler/cloudprovider/test/test_cloud_provider.go @@ -224,11 +224,6 @@ func (tcp *TestCloudProvider) Refresh() error { return nil } -// GetInstanceID gets the instance ID for the specified node. -func (tcp *TestCloudProvider) GetInstanceID(node *apiv1.Node) string { - return node.Spec.ProviderID -} - // TestNodeGroup is a node group used by TestCloudProvider. type TestNodeGroup struct { sync.Mutex From 75ea002bdaa72cb0c46c70be65144f96d6f3ec37 Mon Sep 17 00:00:00 2001 From: Pengfei Ni Date: Mon, 4 Mar 2019 14:48:14 +0800 Subject: [PATCH 4/4] Convert resource group name in Azure providerID to lower case --- .../cloudprovider/azure/azure_agent_pool.go | 16 ++++-- .../cloudprovider/azure/azure_cache.go | 6 +- .../azure/azure_cloud_provider.go | 6 -- .../azure/azure_container_service_pool.go | 10 +++- .../cloudprovider/azure/azure_fakes.go | 2 +- .../cloudprovider/azure/azure_scale_set.go | 11 +++- .../cloudprovider/azure/azure_util.go | 18 +++++- .../cloudprovider/azure/azure_util_test.go | 55 +++++++++++++++++++ 8 files changed, 105 insertions(+), 19 deletions(-) diff --git a/cluster-autoscaler/cloudprovider/azure/azure_agent_pool.go b/cluster-autoscaler/cloudprovider/azure/azure_agent_pool.go index 6c9a7336016..8378442c8c6 100644 --- a/cluster-autoscaler/cloudprovider/azure/azure_agent_pool.go +++ b/cluster-autoscaler/cloudprovider/azure/azure_agent_pool.go @@ -133,7 +133,11 @@ func (as *AgentPool) GetVMIndexes() ([]int, map[int]string, error) { } indexes = append(indexes, index) - indexToVM[index] = "azure://" + strings.ToLower(*instance.ID) + resourceID, err := convertResourceGroupNameToLower("azure://" + *instance.ID) + if err != nil { + return nil, nil, err + } + indexToVM[index] = resourceID } sortedIndexes := sort.IntSlice(indexes) @@ -398,9 +402,13 @@ func (as *AgentPool) Nodes() ([]cloudprovider.Instance, error) { continue } - // To keep consistent with providerID from kubernetes cloud provider, do not convert ID to lower case. - name := "azure://" + strings.ToLower(*instance.ID) - nodes = append(nodes, cloudprovider.Instance{Id: name}) + // To keep consistent with providerID from kubernetes cloud provider, convert + // resourceGroupName in the ID to lower case. + resourceID, err := convertResourceGroupNameToLower("azure://" + *instance.ID) + if err != nil { + return nil, err + } + nodes = append(nodes, cloudprovider.Instance{Id: resourceID}) } return nodes, nil diff --git a/cluster-autoscaler/cloudprovider/azure/azure_cache.go b/cluster-autoscaler/cloudprovider/azure/azure_cache.go index 6f4fa56bb9b..9a598bd8490 100644 --- a/cluster-autoscaler/cloudprovider/azure/azure_cache.go +++ b/cluster-autoscaler/cloudprovider/azure/azure_cache.go @@ -118,7 +118,11 @@ func (m *asgCache) FindForInstance(instance *azureRef, vmType string) (cloudprov m.mutex.Lock() defer m.mutex.Unlock() - inst := azureRef{Name: strings.ToLower(instance.Name)} + resourceID, err := convertResourceGroupNameToLower(instance.Name) + if err != nil { + return nil, err + } + inst := azureRef{Name: resourceID} if m.notInRegisteredAsg[inst] { // We already know we don't own this instance. Return early and avoid // additional calls. diff --git a/cluster-autoscaler/cloudprovider/azure/azure_cloud_provider.go b/cluster-autoscaler/cloudprovider/azure/azure_cloud_provider.go index 15dd6aa5735..2a112ecd61f 100644 --- a/cluster-autoscaler/cloudprovider/azure/azure_cloud_provider.go +++ b/cluster-autoscaler/cloudprovider/azure/azure_cloud_provider.go @@ -19,7 +19,6 @@ package azure import ( "io" "os" - "strings" "k8s.io/klog" @@ -111,11 +110,6 @@ func (azure *AzureCloudProvider) Refresh() error { return azure.azureManager.Refresh() } -// GetInstanceID gets the instance ID for the specified node. -func (azure *AzureCloudProvider) GetInstanceID(node *apiv1.Node) string { - return strings.ToLower(node.Spec.ProviderID) -} - // azureRef contains a reference to some entity in Azure world. type azureRef struct { Name string diff --git a/cluster-autoscaler/cloudprovider/azure/azure_container_service_pool.go b/cluster-autoscaler/cloudprovider/azure/azure_container_service_pool.go index cb382baa453..00ea08957b3 100644 --- a/cluster-autoscaler/cloudprovider/azure/azure_container_service_pool.go +++ b/cluster-autoscaler/cloudprovider/azure/azure_container_service_pool.go @@ -270,7 +270,7 @@ func (agentPool *ContainerServiceAgentPool) SetNodeCount(count int) (err error) func (agentPool *ContainerServiceAgentPool) GetProviderID(name string) string { //TODO: come with a generic way to make it work with provider id formats // in different version of k8s. - return "azure://" + strings.ToLower(name) + return "azure://" + name } //GetName extracts the name of the node (a format which underlying cloud service understands) @@ -419,7 +419,13 @@ func (agentPool *ContainerServiceAgentPool) GetNodes() ([]string, error) { for _, node := range vmList { klog.V(5).Infof("Node Name: %s, ID: %s", *node.Name, *node.ID) if agentPool.IsContainerServiceNode(node.Tags) { - providerID := agentPool.GetProviderID(*node.ID) + providerID, err := convertResourceGroupNameToLower(agentPool.GetProviderID(*node.ID)) + if err != nil { + // This shouldn't happen. Log a waring message for tracking. + klog.Warningf("GetNodes.convertResourceGroupNameToLower failed with error: %v", err) + continue + } + klog.V(5).Infof("Returning back the providerID: %s", providerID) nodeArray = append(nodeArray, providerID) } diff --git a/cluster-autoscaler/cloudprovider/azure/azure_fakes.go b/cluster-autoscaler/cloudprovider/azure/azure_fakes.go index 395f01a4c0b..dd1f532bb88 100644 --- a/cluster-autoscaler/cloudprovider/azure/azure_fakes.go +++ b/cluster-autoscaler/cloudprovider/azure/azure_fakes.go @@ -30,7 +30,7 @@ import ( ) const ( - fakeVirtualMachineScaleSetVMID = "/subscriptions/test-subscription-id/resourcegroups/test-asg/providers/microsoft.compute/virtualmachinescalesets/agents/virtualmachines/0" + fakeVirtualMachineScaleSetVMID = "/subscriptions/test-subscription-id/resourceGroups/test-asg/providers/Microsoft.Compute/virtualMachineScaleSets/agents/virtualMachines/0" ) // VirtualMachineScaleSetsClientMock mocks for VirtualMachineScaleSetsClient. diff --git a/cluster-autoscaler/cloudprovider/azure/azure_scale_set.go b/cluster-autoscaler/cloudprovider/azure/azure_scale_set.go index 72b555afba3..7118aa93563 100644 --- a/cluster-autoscaler/cloudprovider/azure/azure_scale_set.go +++ b/cluster-autoscaler/cloudprovider/azure/azure_scale_set.go @@ -211,7 +211,14 @@ func (scaleSet *ScaleSet) GetScaleSetVms() ([]string, error) { continue } - allVMs = append(allVMs, *vm.ID) + resourceID, err := convertResourceGroupNameToLower(*vm.ID) + if err != nil { + // This shouldn't happen. Log a waring message for tracking. + klog.Warningf("GetScaleSetVms.convertResourceGroupNameToLower failed with error: %v", err) + continue + } + + allVMs = append(allVMs, resourceID) } return allVMs, nil @@ -456,7 +463,7 @@ func (scaleSet *ScaleSet) Nodes() ([]cloudprovider.Instance, error) { instances := make([]cloudprovider.Instance, 0, len(vms)) for i := range vms { - name := "azure://" + strings.ToLower(vms[i]) + name := "azure://" + vms[i] instances = append(instances, cloudprovider.Instance{Id: name}) } diff --git a/cluster-autoscaler/cloudprovider/azure/azure_util.go b/cluster-autoscaler/cloudprovider/azure/azure_util.go index 61892118a32..edb44940ac7 100644 --- a/cluster-autoscaler/cloudprovider/azure/azure_util.go +++ b/cluster-autoscaler/cloudprovider/azure/azure_util.go @@ -78,9 +78,10 @@ const ( ) var ( - vmnameLinuxRegexp = regexp.MustCompile(k8sLinuxVMNamingFormat) - vmnameWindowsRegexp = regexp.MustCompile(k8sWindowsVMNamingFormat) - oldvmnameWindowsRegexp = regexp.MustCompile(k8sWindowsOldVMNamingFormat) + vmnameLinuxRegexp = regexp.MustCompile(k8sLinuxVMNamingFormat) + vmnameWindowsRegexp = regexp.MustCompile(k8sWindowsVMNamingFormat) + oldvmnameWindowsRegexp = regexp.MustCompile(k8sWindowsOldVMNamingFormat) + azureResourceGroupNameRE = regexp.MustCompile(`.*/subscriptions/(?:.*)/resourceGroups/(.+)/providers/(?:.*)`) ) //AzUtil consists of utility functions which utilizes clients to different services. @@ -628,3 +629,14 @@ func isSuccessHTTPResponse(resp *http.Response, err error) (isSuccess bool, real // This shouldn't happen, it only ensures all exceptions are handled. return false, fmt.Errorf("failed with unknown error") } + +// convertResourceGroupNameToLower converts the resource group name in the resource ID to be lowered. +func convertResourceGroupNameToLower(resourceID string) (string, error) { + matches := azureResourceGroupNameRE.FindStringSubmatch(resourceID) + if len(matches) != 2 { + return "", fmt.Errorf("%q isn't in Azure resource ID format", resourceID) + } + + resourceGroup := matches[1] + return strings.Replace(resourceID, resourceGroup, strings.ToLower(resourceGroup), 1), nil +} diff --git a/cluster-autoscaler/cloudprovider/azure/azure_util_test.go b/cluster-autoscaler/cloudprovider/azure/azure_util_test.go index c93cb0f3825..05efdb0f0fe 100644 --- a/cluster-autoscaler/cloudprovider/azure/azure_util_test.go +++ b/cluster-autoscaler/cloudprovider/azure/azure_util_test.go @@ -189,3 +189,58 @@ func TestIsSuccessResponse(t *testing.T) { assert.Equal(t, test.expectedError, realError, "[%s] expected: %v, saw: %v", test.name, realError, test.expectedError) } } +func TestConvertResourceGroupNameToLower(t *testing.T) { + tests := []struct { + desc string + resourceID string + expected string + expectError bool + }{ + { + desc: "empty string should report error", + resourceID: "", + expectError: true, + }, + { + desc: "resourceID not in Azure format should report error", + resourceID: "invalid-id", + expectError: true, + }, + { + desc: "providerID not in Azure format should report error", + resourceID: "azure://invalid-id", + expectError: true, + }, + { + desc: "resource group name in VM providerID should be converted", + resourceID: "azure:///subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myResourceGroupName/providers/Microsoft.Compute/virtualMachines/k8s-agent-AAAAAAAA-0", + expected: "azure:///subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myresourcegroupname/providers/Microsoft.Compute/virtualMachines/k8s-agent-AAAAAAAA-0", + }, + { + desc: "resource group name in VM resourceID should be converted", + resourceID: "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myResourceGroupName/providers/Microsoft.Compute/virtualMachines/k8s-agent-AAAAAAAA-0", + expected: "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myresourcegroupname/providers/Microsoft.Compute/virtualMachines/k8s-agent-AAAAAAAA-0", + }, + { + desc: "resource group name in VMSS providerID should be converted", + resourceID: "azure:///subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myResourceGroupName/providers/Microsoft.Compute/virtualMachineScaleSets/myScaleSetName/virtualMachines/156", + expected: "azure:///subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myresourcegroupname/providers/Microsoft.Compute/virtualMachineScaleSets/myScaleSetName/virtualMachines/156", + }, + { + desc: "resource group name in VMSS resourceID should be converted", + resourceID: "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myResourceGroupName/providers/Microsoft.Compute/virtualMachineScaleSets/myScaleSetName/virtualMachines/156", + expected: "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myresourcegroupname/providers/Microsoft.Compute/virtualMachineScaleSets/myScaleSetName/virtualMachines/156", + }, + } + + for _, test := range tests { + real, err := convertResourceGroupNameToLower(test.resourceID) + if test.expectError { + assert.NotNil(t, err, test.desc) + continue + } + + assert.Nil(t, err, test.desc) + assert.Equal(t, test.expected, real, test.desc) + } +}