Merge pull request #68921 from feiskyer/powerstate

Fix nodes power state on Azure

pkg/cloudprovider/providers/azure/azure_fakes.go

@@ -894,6 +894,6 @@ func (f *fakeVMSet) GetDataDisks(nodeName types.NodeName) ([]compute.DataDisk, e
 	return nil, fmt.Errorf("unimplemented")
 }
 
-func (f *fakeVMSet) GetProvisioningStateByNodeName(name string) (string, error) {
+func (f *fakeVMSet) GetPowerStatusByNodeName(name string) (string, error) {
 	return "", fmt.Errorf("unimplemented")
 }

pkg/cloudprovider/providers/azure/azure_instances.go

@@ -28,6 +28,12 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 )
 
+const (
+	vmPowerStatePrefix      = "PowerState/"
+	vmPowerStateStopped     = "stopped"
+	vmPowerStateDeallocated = "deallocated"
+)
+
 // NodeAddresses returns the addresses of the specified instance.
 func (az *Cloud) NodeAddresses(ctx context.Context, name types.NodeName) ([]v1.NodeAddress, error) {
 	// Returns nil for unmanaged nodes because azure cloud provider couldn't fetch information for them.
@@ -148,12 +154,13 @@ func (az *Cloud) InstanceShutdownByProviderID(ctx context.Context, providerID st
 		return false, err
 	}
 
-	provisioningState, err := az.vmSet.GetProvisioningStateByNodeName(string(nodeName))
+	powerStatus, err := az.vmSet.GetPowerStatusByNodeName(string(nodeName))
 	if err != nil {
 		return false, err
 	}
+	glog.V(5).Infof("InstanceShutdownByProviderID gets power status %q for node %q", powerStatus, nodeName)
 
-	return strings.ToLower(provisioningState) == "stopped" || strings.ToLower(provisioningState) == "deallocated", nil
+	return strings.ToLower(powerStatus) == vmPowerStateStopped || strings.ToLower(powerStatus) == vmPowerStateDeallocated, nil
 }
 
 // getComputeMetadata gets compute information from instance metadata.

pkg/cloudprovider/providers/azure/azure_instances_test.go

@@ -24,23 +24,41 @@ import (
 	"testing"
 
 	"github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2018-04-01/compute"
+	"github.com/Azure/go-autorest/autorest/to"
 	"k8s.io/apimachinery/pkg/types"
 )
 
-func setTestVirtualMachines(c *Cloud, vmList []string) {
+// setTestVirtualMachines sets test virtual machine with powerstate.
+func setTestVirtualMachines(c *Cloud, vmList map[string]string) {
 	virtualMachineClient := c.VirtualMachinesClient.(*fakeAzureVirtualMachinesClient)
 	store := map[string]map[string]compute.VirtualMachine{
 		"rg": make(map[string]compute.VirtualMachine),
 	}
 
-	for i := range vmList {
+	for nodeName, powerState := range vmList {
-		nodeName := vmList[i]
+		instanceID := fmt.Sprintf("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Compute/virtualMachines/%s", nodeName)
-		instanceID := fmt.Sprintf("/subscriptions/script/resourceGroups/rg/providers/Microsoft.Compute/virtualMachines/%s", nodeName)
+		vm := compute.VirtualMachine{
-		store["rg"][nodeName] = compute.VirtualMachine{
 			Name:     &nodeName,
 			ID:       &instanceID,
 			Location: &c.Location,
 		}
+		if powerState != "" {
+			status := []compute.InstanceViewStatus{
+				{
+					Code: to.StringPtr(powerState),
+				},
+				{
+					Code: to.StringPtr("ProvisioningState/succeeded"),
+				},
+			}
+			vm.VirtualMachineProperties = &compute.VirtualMachineProperties{
+				InstanceView: &compute.VirtualMachineInstanceView{
+					Statuses: &status,
+				},
+			}
+		}
+
+		store["rg"][nodeName] = vm
 	}
 
 	virtualMachineClient.setFakeStore(store)
@@ -63,14 +81,14 @@ func TestInstanceID(t *testing.T) {
 			vmList:       []string{"vm1"},
 			nodeName:     "vm1",
 			metadataName: "vm1",
-			expected:     "/subscriptions/script/resourceGroups/rg/providers/Microsoft.Compute/virtualMachines/vm1",
+			expected:     "/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Compute/virtualMachines/vm1",
 		},
 		{
 			name:         "InstanceID should get instanceID from Azure API if node is not local instance",
 			vmList:       []string{"vm2"},
 			nodeName:     "vm2",
 			metadataName: "vm1",
-			expected:     "/subscriptions/script/resourceGroups/rg/providers/Microsoft.Compute/virtualMachines/vm2",
+			expected:     "/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Compute/virtualMachines/vm2",
 		},
 		{
 			name:        "InstanceID should report error if VM doesn't exist",
@@ -96,7 +114,11 @@ func TestInstanceID(t *testing.T) {
 		defer listener.Close()
 
 		cloud.metadata.baseURL = "http://" + listener.Addr().String() + "/"
-		setTestVirtualMachines(cloud, test.vmList)
+		vmListWithPowerState := make(map[string]string)
+		for _, vm := range test.vmList {
+			vmListWithPowerState[vm] = ""
+		}
+		setTestVirtualMachines(cloud, vmListWithPowerState)
 		instanceID, err := cloud.InstanceID(context.Background(), types.NodeName(test.nodeName))
 		if test.expectError {
 			if err == nil {
@@ -113,3 +135,82 @@ func TestInstanceID(t *testing.T) {
 		}
 	}
 }
+
+func TestInstanceShutdownByProviderID(t *testing.T) {
+	testcases := []struct {
+		name        string
+		vmList      map[string]string
+		nodeName    string
+		expected    bool
+		expectError bool
+	}{
+		{
+			name:     "InstanceShutdownByProviderID should return false if the vm is in PowerState/Running status",
+			vmList:   map[string]string{"vm1": "PowerState/Running"},
+			nodeName: "vm1",
+			expected: false,
+		},
+		{
+			name:     "InstanceShutdownByProviderID should return true if the vm is in PowerState/Deallocated status",
+			vmList:   map[string]string{"vm2": "PowerState/Deallocated"},
+			nodeName: "vm2",
+			expected: true,
+		},
+		{
+			name:     "InstanceShutdownByProviderID should return false if the vm is in PowerState/Deallocating status",
+			vmList:   map[string]string{"vm3": "PowerState/Deallocating"},
+			nodeName: "vm3",
+			expected: false,
+		},
+		{
+			name:     "InstanceShutdownByProviderID should return false if the vm is in PowerState/Starting status",
+			vmList:   map[string]string{"vm4": "PowerState/Starting"},
+			nodeName: "vm4",
+			expected: false,
+		},
+		{
+			name:     "InstanceShutdownByProviderID should return true if the vm is in PowerState/Stopped status",
+			vmList:   map[string]string{"vm5": "PowerState/Stopped"},
+			nodeName: "vm5",
+			expected: true,
+		},
+		{
+			name:     "InstanceShutdownByProviderID should return false if the vm is in PowerState/Stopping status",
+			vmList:   map[string]string{"vm6": "PowerState/Stopping"},
+			nodeName: "vm6",
+			expected: false,
+		},
+		{
+			name:     "InstanceShutdownByProviderID should return false if the vm is in PowerState/Unknown status",
+			vmList:   map[string]string{"vm7": "PowerState/Unknown"},
+			nodeName: "vm7",
+			expected: false,
+		},
+		{
+			name:        "InstanceShutdownByProviderID should report error if VM doesn't exist",
+			vmList:      map[string]string{"vm1": "PowerState/running"},
+			nodeName:    "vm8",
+			expectError: true,
+		},
+	}
+
+	for _, test := range testcases {
+		cloud := getTestCloud()
+		setTestVirtualMachines(cloud, test.vmList)
+		providerID := "azure://" + cloud.getStandardMachineID("rg", test.nodeName)
+		hasShutdown, err := cloud.InstanceShutdownByProviderID(context.Background(), providerID)
+		if test.expectError {
+			if err == nil {
+				t.Errorf("Test [%s] unexpected nil err", test.name)
+			}
+		} else {
+			if err != nil {
+				t.Errorf("Test [%s] unexpected error: %v", test.name, err)
+			}
+		}
+
+		if hasShutdown != test.expected {
+			t.Errorf("Test [%s] unexpected hasShutdown: %v, expected %v", test.name, hasShutdown, test.expected)
+		}
+	}
+}

pkg/cloudprovider/providers/azure/azure_standard.go

@@ -346,13 +346,24 @@ func (as *availabilitySet) GetInstanceIDByNodeName(name string) (string, error)
 	return *machine.ID, nil
 }
 
-func (as *availabilitySet) GetProvisioningStateByNodeName(name string) (provisioningState string, err error) {
+// GetPowerStatusByNodeName returns the power state of the specified node.
+func (as *availabilitySet) GetPowerStatusByNodeName(name string) (powerState string, err error) {
 	vm, err := as.getVirtualMachine(types.NodeName(name))
 	if err != nil {
-		return provisioningState, err
+		return powerState, err
 	}
 
-	return *vm.ProvisioningState, nil
+	if vm.InstanceView != nil && vm.InstanceView.Statuses != nil {
+		statuses := *vm.InstanceView.Statuses
+		for _, status := range statuses {
+			state := to.String(status.Code)
+			if strings.HasPrefix(state, vmPowerStatePrefix) {
+				return strings.TrimPrefix(state, vmPowerStatePrefix), nil
+			}
+		}
+	}
+
+	return "", fmt.Errorf("failed to get power status for node %q", name)
 }
 
 // GetNodeNameByProviderID gets the node name by provider ID.

pkg/cloudprovider/providers/azure/azure_vmsets.go

@@ -65,6 +65,6 @@ type VMSet interface {
 	// GetDataDisks gets a list of data disks attached to the node.
 	GetDataDisks(nodeName types.NodeName) ([]compute.DataDisk, error)
 
-	// GetProvisioningStateByNodeName gets the provisioning state by node name.
+	// GetPowerStatusByNodeName returns the power state of the specified node.
-	GetProvisioningStateByNodeName(name string) (string, error)
+	GetPowerStatusByNodeName(name string) (string, error)
 }

pkg/cloudprovider/providers/azure/azure_vmss.go

@@ -128,13 +128,24 @@ func (ss *scaleSet) getVmssVM(nodeName string) (ssName, instanceID string, vm co
 	return ssName, instanceID, *(cachedVM.(*compute.VirtualMachineScaleSetVM)), nil
 }
 
-func (ss *scaleSet) GetProvisioningStateByNodeName(name string) (provisioningState string, err error) {
+// GetPowerStatusByNodeName returns the power state of the specified node.
+func (ss *scaleSet) GetPowerStatusByNodeName(name string) (powerState string, err error) {
 	_, _, vm, err := ss.getVmssVM(name)
 	if err != nil {
-		return provisioningState, err
+		return powerState, err
 	}
 
-	return *vm.ProvisioningState, nil
+	if vm.InstanceView != nil && vm.InstanceView.Statuses != nil {
+		statuses := *vm.InstanceView.Statuses
+		for _, status := range statuses {
+			state := to.String(status.Code)
+			if strings.HasPrefix(state, vmPowerStatePrefix) {
+				return strings.TrimPrefix(state, vmPowerStatePrefix), nil
+			}
+		}
+	}
+
+	return "", fmt.Errorf("failed to get power status for node %q", name)
 }
 
 // getCachedVirtualMachineByInstanceID gets scaleSetVMInfo from cache.

pkg/cloudprovider/providers/azure/azure_vmss_cache.go

@@ -199,6 +199,24 @@ func (ss *scaleSet) newVmssVMCache() (*timedCache, error) {
 			return nil, nil
 		}
 
+		// Get instanceView for vmssVM.
+		if result.InstanceView == nil {
+			viewCtx, viewCancel := getContextWithCancel()
+			defer viewCancel()
+			view, err := ss.VirtualMachineScaleSetVMsClient.GetInstanceView(viewCtx, resourceGroup, ssName, instanceID)
+			// It is possible that the vmssVM gets removed just before this call. So check whether the VM exist again.
+			exists, message, realErr = checkResourceExistsFromError(err)
+			if realErr != nil {
+				return nil, realErr
+			}
+			if !exists {
+				glog.V(2).Infof("Virtual machine scale set VM %q not found with message: %q", key, message)
+				return nil, nil
+			}
+
+			result.InstanceView = &view
+		}
+
 		return &result, nil
 	}