Merge pull request kubernetes#1769 from feiskyer/cleanup-get-instance-id

Cluster Autoscaler: Cleanup GetInstanceID() interface

Author: k8s-ci-robot

Diff for: 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 for: 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 for: 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 for: 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 for: 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 for: 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 for: 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 for: 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 for: 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 for: 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)
+	}
+}

Diff for: 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 for: 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
 

Diff for: 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 for: 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 for: 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 for: 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 for: 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 for: 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

Diff for: 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 {