Merge pull request #2984 from nilo19/fix/empty-provider-id

fix: get vmss name and resource group from vm ID if the provider ID o…
kubernetes-sigs · Dec 22, 2022 · eaa9da7 · eaa9da7
2 parents fc34b8c + 40d467c
commit eaa9da7
Show file tree

Hide file tree

Showing 2 changed files with 84 additions and 21 deletions.
diff --git a/pkg/provider/azure_vmss.go b/pkg/provider/azure_vmss.go
@@ -47,11 +47,13 @@ var (
 	ErrorNotVmssInstance = errors.New("not a vmss instance")
 	ErrScaleSetNotFound  = errors.New("scale set not found")
 
-	scaleSetNameRE         = regexp.MustCompile(`.*/subscriptions/(?:.*)/Microsoft.Compute/virtualMachineScaleSets/(.+)/virtualMachines(?:.*)`)
-	resourceGroupRE        = regexp.MustCompile(`.*/subscriptions/(?:.*)/resourceGroups/(.+)/providers/Microsoft.Compute/virtualMachineScaleSets/(?:.*)/virtualMachines(?:.*)`)
-	vmssIPConfigurationRE  = regexp.MustCompile(`.*/subscriptions/(?:.*)/resourceGroups/(.+)/providers/Microsoft.Compute/virtualMachineScaleSets/(.+)/virtualMachines/(.+)/networkInterfaces(?:.*)`)
-	vmssPIPConfigurationRE = regexp.MustCompile(`.*/subscriptions/(?:.*)/resourceGroups/(.+)/providers/Microsoft.Compute/virtualMachineScaleSets/(.+)/virtualMachines/(.+)/networkInterfaces/(.+)/ipConfigurations/(.+)/publicIPAddresses/(.+)`)
-	vmssVMProviderIDRE     = regexp.MustCompile(`azure:///subscriptions/(?:.*)/resourceGroups/(.+)/providers/Microsoft.Compute/virtualMachineScaleSets/(.+)/virtualMachines/(?:\d+)`)
+	scaleSetNameRE           = regexp.MustCompile(`.*/subscriptions/(?:.*)/Microsoft.Compute/virtualMachineScaleSets/(.+)/virtualMachines(?:.*)`)
+	resourceGroupRE          = regexp.MustCompile(`.*/subscriptions/(?:.*)/resourceGroups/(.+)/providers/Microsoft.Compute/virtualMachineScaleSets/(?:.*)/virtualMachines(?:.*)`)
+	vmssIPConfigurationRE    = regexp.MustCompile(`.*/subscriptions/(?:.*)/resourceGroups/(.+)/providers/Microsoft.Compute/virtualMachineScaleSets/(.+)/virtualMachines/(.+)/networkInterfaces(?:.*)`)
+	vmssPIPConfigurationRE   = regexp.MustCompile(`.*/subscriptions/(?:.*)/resourceGroups/(.+)/providers/Microsoft.Compute/virtualMachineScaleSets/(.+)/virtualMachines/(.+)/networkInterfaces/(.+)/ipConfigurations/(.+)/publicIPAddresses/(.+)`)
+	vmssVMResourceIDTemplate = `/subscriptions/(?:.*)/resourceGroups/(.+)/providers/Microsoft.Compute/virtualMachineScaleSets/(.+)/virtualMachines/(?:\d+)`
+	vmssVMResourceIDRE       = regexp.MustCompile(vmssVMResourceIDTemplate)
+	vmssVMProviderIDRE       = regexp.MustCompile(fmt.Sprintf("%s%s", "azure://", vmssVMResourceIDTemplate))
 )
 
 // vmssMetaInfo contains the metadata for a VMSS.
@@ -1231,6 +1233,14 @@ func getVmssAndResourceGroupNameByVMProviderID(providerID string) (string, strin
 	return matches[1], matches[2], nil
 }
 
+func getVmssAndResourceGroupNameByVMID(id string) (string, string, error) {
+	matches := vmssVMResourceIDRE.FindStringSubmatch(id)
+	if len(matches) != 3 {
+		return "", "", ErrorNotVmssInstance
+	}
+	return matches[1], matches[2], nil
+}
+
 func (ss *ScaleSet) ensureVMSSInPool(service *v1.Service, nodes []*v1.Node, backendPoolID string, vmSetNameOfLB string) error {
 	klog.V(2).Infof("ensureVMSSInPool: ensuring VMSS with backendPoolID %s", backendPoolID)
 	vmssNamesMap := make(map[string]bool)
@@ -1254,10 +1264,25 @@ func (ss *ScaleSet) ensureVMSSInPool(service *v1.Service, nodes []*v1.Node, back
 			}
 
 			// in this scenario the vmSetName is an empty string and the name of vmss should be obtained from the provider IDs of nodes
-			resourceGroupName, vmssName, err := getVmssAndResourceGroupNameByVMProviderID(node.Spec.ProviderID)
-			if err != nil {
-				klog.V(4).Infof("ensureVMSSInPool: found VMSS node %s, will skip checking and continue", node.Name)
-				continue
+			var resourceGroupName, vmssName string
+			if node.Spec.ProviderID != "" {
+				resourceGroupName, vmssName, err = getVmssAndResourceGroupNameByVMProviderID(node.Spec.ProviderID)
+				if err != nil {
+					klog.V(4).Infof("ensureVMSSInPool: the provider ID %s of node %s is not the format of VMSS VM, will skip checking and continue", node.Spec.ProviderID, node.Name)
+					continue
+				}
+			} else {
+				klog.V(4).Infof("ensureVMSSInPool: the provider ID of node %s is empty, will check the VM ID", node.Name)
+				instanceID, err := ss.InstanceID(context.TODO(), types.NodeName(node.Name))
+				if err != nil {
+					klog.Errorf("ensureVMSSInPool: Failed to get instance ID for node %q: %v", node.Name, err)
+					return err
+				}
+				resourceGroupName, vmssName, err = getVmssAndResourceGroupNameByVMID(instanceID)
+				if err != nil {
+					klog.V(4).Infof("ensureVMSSInPool: the instance ID %s of node %s is not the format of VMSS VM, will skip checking and continue", node.Spec.ProviderID, node.Name)
+					continue
+				}
 			}
 			// only vmsses in the resource group same as it's in azure config are included
 			if strings.EqualFold(resourceGroupName, ss.ResourceGroup) {

diff --git a/pkg/provider/azure_vmss_test.go b/pkg/provider/azure_vmss_test.go
@@ -2031,18 +2031,20 @@ func TestEnsureVMSSInPool(t *testing.T) {
 	defer ctrl.Finish()
 
 	testCases := []struct {
-		description        string
-		backendPoolID      string
-		vmSetName          string
-		clusterIP          string
-		nodes              []*v1.Node
-		isBasicLB          bool
-		isVMSSDeallocating bool
-		isVMSSNilNICConfig bool
-		expectedPutVMSS    bool
-		setIPv6Config      bool
-		useMultipleSLBs    bool
-		expectedErr        error
+		description           string
+		backendPoolID         string
+		vmSetName             string
+		clusterIP             string
+		nodes                 []*v1.Node
+		isBasicLB             bool
+		isVMSSDeallocating    bool
+		isVMSSNilNICConfig    bool
+		expectedGetInstanceID string
+		expectedPutVMSS       bool
+		setIPv6Config         bool
+		useMultipleSLBs       bool
+		getInstanceIDErr      error
+		expectedErr           error
 	}{
 		{
 			description: "ensureVMSSInPool should skip the node if it isn't managed by VMSS",
@@ -2179,6 +2181,36 @@ func TestEnsureVMSSInPool(t *testing.T) {
 			useMultipleSLBs: true,
 			expectedPutVMSS: true,
 		},
+		{
+			description: "ensureVMSSInPool should get the vmss name from vm instance ID if the provider ID of the node is empty",
+			nodes: []*v1.Node{
+				{},
+			},
+			vmSetName:             testVMSSName,
+			backendPoolID:         testLBBackendpoolID1,
+			expectedGetInstanceID: "/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Compute/virtualMachineScaleSets/vmss/virtualMachines/0",
+			expectedPutVMSS:       true,
+		},
+		{
+			description: "ensureVMSSInPool should skip updating vmss if the node is not a vmss vm",
+			nodes: []*v1.Node{
+				{},
+			},
+			vmSetName:             testVMSSName,
+			backendPoolID:         testLBBackendpoolID1,
+			expectedGetInstanceID: "invalid",
+		},
+		{
+			description: "ensureVMSSInPool should report an error if failed to get instsance ID",
+			nodes: []*v1.Node{
+				{},
+			},
+			vmSetName:             testVMSSName,
+			backendPoolID:         testLBBackendpoolID1,
+			expectedGetInstanceID: "invalid",
+			getInstanceIDErr:      errors.New("error"),
+			expectedErr:           errors.New("error"),
+		},
 	}
 
 	for _, test := range testCases {
@@ -2209,6 +2241,12 @@ func TestEnsureVMSSInPool(t *testing.T) {
 		mockVMSSVMClient := ss.cloud.VirtualMachineScaleSetVMsClient.(*mockvmssvmclient.MockInterface)
 		mockVMSSVMClient.EXPECT().List(gomock.Any(), ss.ResourceGroup, testVMSSName, gomock.Any()).Return(expectedVMSSVMs, nil).AnyTimes()
 
+		if test.expectedGetInstanceID != "" {
+			mockVMSet := NewMockVMSet(ctrl)
+			mockVMSet.EXPECT().GetInstanceIDByNodeName(gomock.Any()).Return(test.expectedGetInstanceID, test.getInstanceIDErr)
+			ss.VMSet = mockVMSet
+		}
+
 		err = ss.ensureVMSSInPool(&v1.Service{Spec: v1.ServiceSpec{ClusterIP: test.clusterIP}}, test.nodes, test.backendPoolID, test.vmSetName)
 		assert.Equal(t, test.expectedErr, err, test.description+", but an error occurs")
 	}