diff --git a/pkg/cloudprovider/providers/azure/azure_fakes.go b/pkg/cloudprovider/providers/azure/azure_fakes.go index 8bd3ac0eaf..bec303e932 100644 --- a/pkg/cloudprovider/providers/azure/azure_fakes.go +++ b/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") } diff --git a/pkg/cloudprovider/providers/azure/azure_instances.go b/pkg/cloudprovider/providers/azure/azure_instances.go index 4f609fa72f..916bbfd7ec 100644 --- a/pkg/cloudprovider/providers/azure/azure_instances.go +++ b/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. diff --git a/pkg/cloudprovider/providers/azure/azure_instances_test.go b/pkg/cloudprovider/providers/azure/azure_instances_test.go index 106cb0f276..20680b5150 100644 --- a/pkg/cloudprovider/providers/azure/azure_instances_test.go +++ b/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 { - nodeName := vmList[i] - instanceID := fmt.Sprintf("/subscriptions/script/resourceGroups/rg/providers/Microsoft.Compute/virtualMachines/%s", nodeName) - store["rg"][nodeName] = compute.VirtualMachine{ + for nodeName, powerState := range vmList { + instanceID := fmt.Sprintf("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Compute/virtualMachines/%s", nodeName) + vm := 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) + } + } +} diff --git a/pkg/cloudprovider/providers/azure/azure_standard.go b/pkg/cloudprovider/providers/azure/azure_standard.go index 263cc1e1a5..54b9e3cb11 100644 --- a/pkg/cloudprovider/providers/azure/azure_standard.go +++ b/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. diff --git a/pkg/cloudprovider/providers/azure/azure_vmsets.go b/pkg/cloudprovider/providers/azure/azure_vmsets.go index 5fe7817bef..c772813aa2 100644 --- a/pkg/cloudprovider/providers/azure/azure_vmsets.go +++ b/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. - GetProvisioningStateByNodeName(name string) (string, error) + // GetPowerStatusByNodeName returns the power state of the specified node. + GetPowerStatusByNodeName(name string) (string, error) } diff --git a/pkg/cloudprovider/providers/azure/azure_vmss.go b/pkg/cloudprovider/providers/azure/azure_vmss.go index 4a3cc712b7..539483d35b 100644 --- a/pkg/cloudprovider/providers/azure/azure_vmss.go +++ b/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. diff --git a/pkg/cloudprovider/providers/azure/azure_vmss_cache.go b/pkg/cloudprovider/providers/azure/azure_vmss_cache.go index c09f71f588..73bc59ea4e 100644 --- a/pkg/cloudprovider/providers/azure/azure_vmss_cache.go +++ b/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 }