Merge pull request #681 from feiskyer/az

Improve azure node's providerID handling

cluster-autoscaler/cloudprovider/azure/azure_agent_pool.go

@@ -20,7 +20,6 @@ import (
 	"fmt"
 	"math/rand"
 	"sort"
-	"strings"
 	"sync"
 	"time"
 
@@ -130,7 +129,7 @@ func (as *AgentPool) GetVMIndexes() ([]int, map[int]string, error) {
 		}
 
 		indexes = append(indexes, index)
-		indexToVM[index] = "azure://" + strings.ToLower(*instance.ID)
+		indexToVM[index] = "azure://" + *instance.ID
 	}
 
 	sortedIndexes := sort.IntSlice(indexes)
@@ -286,7 +285,7 @@ func (as *AgentPool) Belongs(node *apiv1.Node) (bool, error) {
 	glog.V(6).Infof("Check if node belongs to this agent pool: AgentPool:%v, node:%v\n", as, node)
 
 	ref := &azureRef{
-		Name: strings.ToLower(node.Spec.ProviderID),
+		Name: node.Spec.ProviderID,
 	}
 
 	targetAsg, err := as.manager.GetAsgForInstance(ref)
@@ -365,7 +364,7 @@ func (as *AgentPool) DeleteNodes(nodes []*apiv1.Node) error {
 		}
 
 		ref := &azureRef{
-			Name: strings.ToLower(node.Spec.ProviderID),
+			Name: node.Spec.ProviderID,
 		}
 		refs = append(refs, ref)
 	}

cluster-autoscaler/cloudprovider/azure/azure_cache.go

@@ -20,7 +20,6 @@ import (
 	"fmt"
 	"reflect"
 	"regexp"
-	"strings"
 	"sync"
 	"time"
 
@@ -29,7 +28,7 @@ import (
 	"k8s.io/autoscaler/cluster-autoscaler/cloudprovider"
 )
 
-var virtualMachineRE = regexp.MustCompile(`^azure://(?:.*)/providers/microsoft.compute/virtualmachines/(.+)$`)
+var virtualMachineRE = regexp.MustCompile(`^azure://(?:.*)/providers/Microsoft.Compute/virtualMachines/(.+)$`)
 
 type asgCache struct {
 	registeredAsgs     []cloudprovider.NodeGroup
@@ -172,8 +171,7 @@ func (m *asgCache) regenerate() error {
 		}
 
 		for _, instance := range instances {
-			// Convert to lower because instance.ID is in different in different API calls (e.g. GET and LIST).
-			ref := azureRef{Name: strings.ToLower(instance)}
+			ref := azureRef{Name: instance}
 			newCache[ref] = nsg
 		}
 	}

cluster-autoscaler/cloudprovider/azure/azure_client.go

@@ -43,6 +43,7 @@ type VirtualMachineScaleSetsClient interface {
 
 // VirtualMachineScaleSetVMsClient defines needed functions for azure compute.VirtualMachineScaleSetVMsClient.
 type VirtualMachineScaleSetVMsClient interface {
+	Get(resourceGroupName string, VMScaleSetName string, instanceID string) (result compute.VirtualMachineScaleSetVM, err error)
 	List(resourceGroupName string, virtualMachineScaleSetName string, filter string, selectParameter string, expand string) (result compute.VirtualMachineScaleSetVMListResult, err error)
 	ListNextResults(lastResults compute.VirtualMachineScaleSetVMListResult) (result compute.VirtualMachineScaleSetVMListResult, err error)
 }

cluster-autoscaler/cloudprovider/azure/azure_cloud_provider.go

@@ -17,8 +17,6 @@ limitations under the License.
 package azure
 
 import (
-	"strings"
-
 	"github.com/golang/glog"
 
 	apiv1 "k8s.io/api/core/v1"
@@ -74,7 +72,7 @@ func (azure *AzureCloudProvider) NodeGroups() []cloudprovider.NodeGroup {
 func (azure *AzureCloudProvider) NodeGroupForNode(node *apiv1.Node) (cloudprovider.NodeGroup, error) {
 	glog.V(6).Infof("Searching for node group for the node: %s, %s\n", node.Spec.ExternalID, node.Spec.ProviderID)
 	ref := &azureRef{
-		Name: strings.ToLower(node.Spec.ProviderID),
+		Name: node.Spec.ProviderID,
 	}
 
 	return azure.azureManager.GetAsgForInstance(ref)

cluster-autoscaler/cloudprovider/azure/azure_fakes.go

@@ -123,6 +123,20 @@ type VirtualMachineScaleSetVMsClientMock struct {
 	mock.Mock
 }
 
+// Get gets a VirtualMachineScaleSetVM by VMScaleSetName and instanceID.
+func (m *VirtualMachineScaleSetVMsClientMock) Get(resourceGroupName string, VMScaleSetName string, instanceID string) (result compute.VirtualMachineScaleSetVM, err error) {
+	ID := fakeVirtualMachineScaleSetVMID
+	vmID := "123E4567-E89B-12D3-A456-426655440000"
+	properties := compute.VirtualMachineScaleSetVMProperties{
+		VMID: &vmID,
+	}
+	return compute.VirtualMachineScaleSetVM{
+		ID:                                 &ID,
+		InstanceID:                         &instanceID,
+		VirtualMachineScaleSetVMProperties: &properties,
+	}, nil
+}
+
 // List gets a list of VirtualMachineScaleSetVMs.
 func (m *VirtualMachineScaleSetVMsClientMock) List(resourceGroupName string, virtualMachineScaleSetName string, filter string, selectParameter string, expand string) (result compute.VirtualMachineScaleSetVMListResult, err error) {
 	value := make([]compute.VirtualMachineScaleSetVM, 1)

cluster-autoscaler/cloudprovider/azure/azure_scale_set.go

@@ -18,11 +18,12 @@ package azure
 
 import (
 	"fmt"
-	"strings"
+	"net/http"
 	"sync"
 	"time"
 
 	"github.com/Azure/azure-sdk-for-go/arm/compute"
+	"github.com/Azure/go-autorest/autorest"
 	"github.com/golang/glog"
 
 	apiv1 "k8s.io/api/core/v1"
@@ -169,8 +170,11 @@ func (scaleSet *ScaleSet) IncreaseSize(delta int) error {
 }
 
 // GetScaleSetVms returns list of nodes for the given scale set.
+// Note that the list results is not used directly because their resource ID format
+// is not consistent with Get results.
+// TODO(feiskyer): use list results directly after the issue fixed in Azure VMSS API.
 func (scaleSet *ScaleSet) GetScaleSetVms() ([]compute.VirtualMachineScaleSetVM, error) {
-	allVMs := make([]compute.VirtualMachineScaleSetVM, 0)
+	instanceIDs := make([]string, 0)
 	resourceGroup := scaleSet.manager.config.ResourceGroup
 	result, err := scaleSet.manager.azClient.virtualMachineScaleSetVMsClient.List(resourceGroup, scaleSet.Name, "", "", "")
 	if err != nil {
@@ -180,7 +184,9 @@ func (scaleSet *ScaleSet) GetScaleSetVms() ([]compute.VirtualMachineScaleSetVM,
 
 	moreResults := (result.Value != nil && len(*result.Value) > 0)
 	for moreResults {
-		allVMs = append(allVMs, *result.Value...)
+		for _, vm := range *result.Value {
+			instanceIDs = append(instanceIDs, *vm.InstanceID)
+		}
 		moreResults = false
 
 		result, err = scaleSet.manager.azClient.virtualMachineScaleSetVMsClient.ListNextResults(result)
@@ -190,6 +196,22 @@ func (scaleSet *ScaleSet) GetScaleSetVms() ([]compute.VirtualMachineScaleSetVM,
 		moreResults = (result.Value != nil && len(*result.Value) > 0)
 	}
 
+	allVMs := make([]compute.VirtualMachineScaleSetVM, 0)
+	for _, instanceID := range instanceIDs {
+		vm, err := scaleSet.manager.azClient.virtualMachineScaleSetVMsClient.Get(resourceGroup, scaleSet.Name, instanceID)
+		if err != nil {
+			if v, ok := err.(autorest.DetailedError); ok && v.StatusCode == http.StatusNotFound {
+				glog.Warningf("Couldn't find VirtualMachineScaleSetVM by (%s,%s), assuming it has been removed", scaleSet.Name, instanceID)
+				continue
+			}
+
+			glog.Errorf("Failed to get VirtualMachineScaleSetVM by (%s,%s), error: %v", scaleSet.Name, instanceID, err)
+			return nil, err
+		}
+
+		allVMs = append(allVMs, vm)
+	}
+
 	return allVMs, nil
 }
 
@@ -226,7 +248,7 @@ func (scaleSet *ScaleSet) Belongs(node *apiv1.Node) (bool, error) {
 	glog.V(6).Infof("Check if node belongs to this scale set: scaleset:%v, node:%v\n", scaleSet, node)
 
 	ref := &azureRef{
-		Name: strings.ToLower(node.Spec.ProviderID),
+		Name: node.Spec.ProviderID,
 	}
 
 	targetAsg, err := scaleSet.manager.GetAsgForInstance(ref)
@@ -308,7 +330,7 @@ func (scaleSet *ScaleSet) DeleteNodes(nodes []*apiv1.Node) error {
 		}
 
 		ref := &azureRef{
-			Name: strings.ToLower(node.Spec.ProviderID),
+			Name: node.Spec.ProviderID,
 		}
 		refs = append(refs, ref)
 	}
@@ -343,28 +365,9 @@ func (scaleSet *ScaleSet) Nodes() ([]string, error) {
 		if len(*vms[i].ID) == 0 {
 			continue
 		}
-		// To keep consistent with providerID from kubernetes cloud provider, do not convert ID to lower case.
 		name := "azure://" + *vms[i].ID
 		result = append(result, name)
 	}
 
 	return result, nil
 }
-
-func (scaleSet *ScaleSet) getInstanceIDs() (map[azureRef]string, error) {
-	vms, err := scaleSet.GetScaleSetVms()
-	if err != nil {
-		return nil, err
-	}
-
-	result := make(map[azureRef]string)
-	for i := range vms {
-		// Convert to lower because instance.ID is in different in different API calls (e.g. GET and LIST).
-		ref := azureRef{
-			Name: "azure://" + strings.ToLower(*vms[i].ID),
-		}
-		result[ref] = *vms[i].InstanceID
-	}
-
-	return result, nil
-}