From f51014673dfe3277cb2301916d0cdb6a211c51f9 Mon Sep 17 00:00:00 2001 From: qini Date: Thu, 7 Jan 2021 10:18:42 +0800 Subject: [PATCH 1/2] Use network.Interface.VirtualMachine.ID to get the binded VM --- .../azure/azure_loadbalancer.go | 4 ++ .../azure/azure_loadbalancer_test.go | 12 +++++- .../azure/azure_standard.go | 37 ++++++++++++++----- .../azure/azure_standard_test.go | 16 ++++++-- 4 files changed, 54 insertions(+), 15 deletions(-) diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go index 5a9662ef750..19b409d5679 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go @@ -1129,6 +1129,10 @@ func (az *Cloud) reconcileLoadBalancer(clusterName string, service *v1.Service, if err != nil { return nil, err } + if nodeName == "" { + // VM may under deletion + continue + } // If a node is not supposed to be included in the LB, it // would not be in the `nodes` slice. We need to check the nodes that // have been added to the LB's backendpool, find the unwanted ones and diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go index 9c9d5a327ac..c8857639f0d 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer_test.go @@ -3505,13 +3505,21 @@ func TestCleanBackendpoolForPrimarySLB(t *testing.T) { }) existingVMForAS1 := buildDefaultTestVirtualMachine("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Compute/availabilitySets/agentpool1-availabilitySet-00000000", []string{"/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/networkInterfaces/k8s-agentpool1-00000000-nic-1"}) existingVMForAS2 := buildDefaultTestVirtualMachine("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Compute/availabilitySets/agentpool2-availabilitySet-00000000", []string{"/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/networkInterfaces/k8s-agentpool2-00000000-nic-1"}) - existingNIC := buildDefaultTestInterface(true, []string{"/subscriptions/sub/resourceGroups/gh/providers/Microsoft.Network/loadBalancers/testCluster/backendAddressPools/testCluster"}) + existingNICForAS1 := buildDefaultTestInterface(true, []string{"/subscriptions/sub/resourceGroups/gh/providers/Microsoft.Network/loadBalancers/testCluster/backendAddressPools/testCluster"}) + existingNICForAS1.VirtualMachine = &network.SubResource{ + ID: to.StringPtr("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Compute/virtualMachines/k8s-agentpool1-00000000-1"), + } + existingNICForAS2 := buildDefaultTestInterface(true, []string{"/subscriptions/sub/resourceGroups/gh/providers/Microsoft.Network/loadBalancers/testCluster/backendAddressPools/testCluster"}) + existingNICForAS2.VirtualMachine = &network.SubResource{ + ID: to.StringPtr("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Compute/virtualMachines/k8s-agentpool2-00000000-1"), + } mockVMClient := mockvmclient.NewMockInterface(ctrl) mockVMClient.EXPECT().Get(gomock.Any(), cloud.ResourceGroup, "k8s-agentpool1-00000000-1", gomock.Any()).Return(existingVMForAS1, nil) mockVMClient.EXPECT().Get(gomock.Any(), cloud.ResourceGroup, "k8s-agentpool2-00000000-1", gomock.Any()).Return(existingVMForAS2, nil) cloud.VirtualMachinesClient = mockVMClient mockNICClient := mockinterfaceclient.NewMockInterface(ctrl) - mockNICClient.EXPECT().Get(gomock.Any(), "rg", "k8s-agentpool2-00000000-nic-1", gomock.Any()).Return(existingNIC, nil) + mockNICClient.EXPECT().Get(gomock.Any(), "rg", "k8s-agentpool1-00000000-nic-1", gomock.Any()).Return(existingNICForAS1, nil) + mockNICClient.EXPECT().Get(gomock.Any(), "rg", "k8s-agentpool2-00000000-nic-1", gomock.Any()).Return(existingNICForAS2, nil).Times(3) mockNICClient.EXPECT().CreateOrUpdate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) cloud.InterfacesClient = mockNICClient cleanedLB, err := cloud.cleanBackendpoolForPrimarySLB(&lb, &service, clusterName) diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_standard.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_standard.go index 007bfc41cda..d4854ef04a3 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_standard.go +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_standard.go @@ -76,7 +76,8 @@ var ( providerIDRE = regexp.MustCompile(`.*/subscriptions/(?:.*)/Microsoft.Compute/virtualMachines/(.+)$`) backendPoolIDRE = regexp.MustCompile(`^/subscriptions/(?:.*)/resourceGroups/(?:.*)/providers/Microsoft.Network/loadBalancers/(.+)/backendAddressPools/(?:.*)`) nicResourceGroupRE = regexp.MustCompile(`.*/subscriptions/(?:.*)/resourceGroups/(.+)/providers/Microsoft.Network/networkInterfaces/(?:.*)`) - nicIDRE = regexp.MustCompile(`/subscriptions/(?:.*)/resourceGroups/(?:.+)/providers/Microsoft.Network/networkInterfaces/(.+)-nic-(.+)/ipConfigurations/(?:.*)`) + nicIDRE = regexp.MustCompile(`(?i)/subscriptions/(?:.*)/resourceGroups/(.+)/providers/Microsoft.Network/networkInterfaces/(.+)/ipConfigurations/(?:.*)`) + vmIDRE = regexp.MustCompile(`(?i)/subscriptions/(?:.*)/resourceGroups/(?:.*)/providers/Microsoft.Compute/virtualMachines/(.+)`) vmasIDRE = regexp.MustCompile(`/subscriptions/(?:.*)/resourceGroups/(?:.*)/providers/Microsoft.Compute/availabilitySets/(.+)`) ) @@ -1055,24 +1056,40 @@ func (as *availabilitySet) GetNodeNameByIPConfigurationID(ipConfigurationID stri return "", "", fmt.Errorf("invalid ip config ID %s", ipConfigurationID) } - prefix := matches[1] - suffix := matches[2] - nodeName := fmt.Sprintf("%s-%s", prefix, suffix) + nicResourceGroup, nicName := matches[1], matches[2] + if nicResourceGroup == "" || nicName == "" { + return "", "", fmt.Errorf("invalid ip config ID %s", ipConfigurationID) + } + nic, rerr := as.InterfacesClient.Get(context.Background(), nicResourceGroup, nicName, "") + if rerr != nil { + return "", "", fmt.Errorf("GetNodeNameByIPConfigurationID(%s): failed to get interface of name %s: %s", ipConfigurationID, nicName, rerr.Error().Error()) + } + vmID := "" + if nic.InterfacePropertiesFormat != nil && nic.VirtualMachine != nil { + vmID = to.String(nic.VirtualMachine.ID) + } + if vmID == "" { + klog.V(2).Infof("GetNodeNameByIPConfigurationID(%s): empty vmID", ipConfigurationID) + return "", "", nil + } - vm, err := as.getVirtualMachine(types.NodeName(nodeName), azcache.CacheReadTypeDefault) + matches = vmIDRE.FindStringSubmatch(vmID) + if len(matches) != 2 { + return "", "", fmt.Errorf("invalid virtual machine ID %s", vmID) + } + vmName := matches[1] + + vm, err := as.getVirtualMachine(types.NodeName(vmName), azcache.CacheReadTypeDefault) if err != nil { - return "", "", fmt.Errorf("cannot get the virtual machine by node name %s", nodeName) + return "", "", fmt.Errorf("cannot get the virtual machine by node name %s", vmName) } asID := "" if vm.VirtualMachineProperties != nil && vm.AvailabilitySet != nil { asID = to.String(vm.AvailabilitySet.ID) } - if asID == "" { - return "", "", fmt.Errorf("cannot get the availability set ID from the virtual machine with node name %s", nodeName) - } asName, err := getAvailabilitySetNameByID(asID) if err != nil { return "", "", fmt.Errorf("cannot get the availability set name by the availability set ID %s", asID) } - return nodeName, strings.ToLower(asName), nil + return vmName, strings.ToLower(asName), nil } diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_standard_test.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_standard_test.go index 04916652cf0..3f7642c584d 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_standard_test.go +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_standard_test.go @@ -1673,7 +1673,10 @@ func TestStandardEnsureBackendPoolDeleted(t *testing.T) { mockVMClient.EXPECT().Get(gomock.Any(), cloud.ResourceGroup, "k8s-agentpool1-00000000-1", gomock.Any()).Return(test.existingVM, nil) cloud.VirtualMachinesClient = mockVMClient mockNICClient := mockinterfaceclient.NewMockInterface(ctrl) - mockNICClient.EXPECT().Get(gomock.Any(), "rg", "k8s-agentpool1-00000000-nic-1", gomock.Any()).Return(test.existingNIC, nil) + test.existingNIC.VirtualMachine = &network.SubResource{ + ID: to.StringPtr("/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Compute/virtualMachines/k8s-agentpool1-00000000-1"), + } + mockNICClient.EXPECT().Get(gomock.Any(), "rg", "k8s-agentpool1-00000000-nic-1", gomock.Any()).Return(test.existingNIC, nil).Times(2) mockNICClient.EXPECT().CreateOrUpdate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) cloud.InterfacesClient = mockNICClient @@ -1729,11 +1732,18 @@ func TestStandardGetNodeNameByIPConfigurationID(t *testing.T) { defer ctrl.Finish() cloud := GetTestCloud(ctrl) expectedVM := buildDefaultTestVirtualMachine("/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Compute/availabilitySets/AGENTPOOL1-AVAILABILITYSET-00000000", []string{}) + expectedVM.Name = to.StringPtr("name") mockVMClient := cloud.VirtualMachinesClient.(*mockvmclient.MockInterface) mockVMClient.EXPECT().Get(gomock.Any(), "rg", "k8s-agentpool1-00000000-0", gomock.Any()).Return(expectedVM, nil) + expectedNIC := buildDefaultTestInterface(true, []string{}) + expectedNIC.VirtualMachine = &network.SubResource{ + ID: to.StringPtr("/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Compute/virtualMachines/k8s-agentpool1-00000000-0"), + } + mockNICClient := cloud.InterfacesClient.(*mockinterfaceclient.MockInterface) + mockNICClient.EXPECT().Get(gomock.Any(), "rg", "k8s-agentpool1-00000000-nic-0", gomock.Any()).Return(expectedNIC, nil) ipConfigurationID := `/subscriptions/sub/resourceGroups/rg/providers/Microsoft.Network/networkInterfaces/k8s-agentpool1-00000000-nic-0/ipConfigurations/ipconfig1` nodeName, asName, err := cloud.VMSet.GetNodeNameByIPConfigurationID(ipConfigurationID) assert.NoError(t, err) - assert.Equal(t, nodeName, "k8s-agentpool1-00000000-0") - assert.Equal(t, asName, "agentpool1-availabilityset-00000000") + assert.Equal(t, "k8s-agentpool1-00000000-0", nodeName) + assert.Equal(t, "agentpool1-availabilityset-00000000", asName) } From a2350b5b2c958a230cfb40654ad93ededaa1df00 Mon Sep 17 00:00:00 2001 From: qini Date: Thu, 7 Jan 2021 10:20:14 +0800 Subject: [PATCH 2/2] Skip standalone VM when reconciling LoadBalancer --- .../k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go | 2 +- .../src/k8s.io/legacy-cloud-providers/azure/azure_standard.go | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go index 19b409d5679..629da8c2669 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_loadbalancer.go @@ -313,7 +313,7 @@ func (az *Cloud) cleanBackendpoolForPrimarySLB(primarySLB *network.LoadBalancer, return nil, err } primaryVMSetName := az.VMSet.GetPrimaryVMSetName() - if !strings.EqualFold(primaryVMSetName, vmSetName) { + if !strings.EqualFold(primaryVMSetName, vmSetName) && vmSetName != "" { klog.V(2).Infof("cleanBackendpoolForPrimarySLB: found unwanted vmSet %s, decouple it from the LB", vmSetName) // construct a backendPool that only contains the IP config of the node to be deleted interfaceIPConfigToBeDeleted := network.InterfaceIPConfiguration{ diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_standard.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_standard.go index d4854ef04a3..6f9fdb6dc93 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_standard.go +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_standard.go @@ -1087,6 +1087,10 @@ func (as *availabilitySet) GetNodeNameByIPConfigurationID(ipConfigurationID stri if vm.VirtualMachineProperties != nil && vm.AvailabilitySet != nil { asID = to.String(vm.AvailabilitySet.ID) } + if asID == "" { + return vmName, "", nil + } + asName, err := getAvailabilitySetNameByID(asID) if err != nil { return "", "", fmt.Errorf("cannot get the availability set name by the availability set ID %s", asID)