Multi-slb related bug fixes #7432

nilo19 · 2024-10-29T04:18:44Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

All endpointslices of a local service should be included in local backend pool updater, instead of only the first endpointslice.
In some rare cases, migration from NIC to IP-based LB can be in a middle state where the NIC references are removed, but those IPConfigs in the backend pool are not. In this case, we should manually exclude those IPConfigs from the request body.
localServiceOwnsBackendPool should compare the full backend pool name, not just prefix, because two service names can share the same prefix.
There is a corner case when the cluster is being updated to multi-slb from classic NIC-based single lb, not from an IP-based cluster. In this case, if the service being reconciled is local, the cloud provider will try to update a NIC pool to IP-based pool direct, which is not allowed. We should skip adding IPs to NIC-based pool in multi-slb mode.
There is a bug in ReconcileBackendPools, where we by mistake parse the LB name to use as the backend pool name.

Which issue(s) this PR fixes:

Fixes #7113
Fixes #7200
Fixes #6980

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fix: several bugs related to multiple standard load balancers mode.
1. All endpointslices of a local service should be included in local backend pool updater, instead of only the first endpointslice.
2. In some rare cases, migration from NIC to IP-based LB can be in a middle state where the NIC references are removed, but those IPConfigs in the backend pool are not. In this case, we should manually exclude those IPConfigs from the request body.
3. localServiceOwnsBackendPool should compare the full backend pool name, not just prefix, because two service names can share the same prefix.
4. There is a corner case when the cluster is being updated to multi-slb from classic NIC-based single lb, not from an IP-based cluster. In this case, if the service being reconciled is local, the cloud provider will try to update a NIC pool to IP-based pool direct, which is not allowed. We should skip adding IPs to NIC-based pool in multi-slb mode.
5. There is a bug in ReconcileBackendPools, where we by mistake parse the LB name to use as the backend pool name.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2024-10-29T04:18:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nilo19

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [nilo19]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

nilo19 · 2024-10-29T04:19:57Z

pkg/provider/azure_loadbalancer_backendpool.go

+			activeNodes = bi.getLocalServiceEndpointsNodeNames(service)
+		}
+
+		if isNICPool(backendPool) {


nilo19 · 2024-10-29T04:20:41Z

pkg/provider/azure_loadbalancer_backendpool.go

@@ -886,7 +889,13 @@ func removeNodeIPAddressesFromBackendPool(
 		if addresses[i].LoadBalancerBackendAddressPropertiesFormat != nil {
 			ipAddress := ptr.Deref((*backendPool.LoadBalancerBackendAddresses)[i].IPAddress, "")
 			if ipAddress == "" {
-				klog.V(4).Infof("removeNodeIPAddressFromBackendPool: LoadBalancerBackendAddress %s is not IP-based, skipping", ptr.Deref(addresses[i].Name, ""))
+				if isNodeIP {


nilo19 · 2024-10-29T04:24:50Z

pkg/provider/azure_loadbalancer_backendpool.go

@@ -633,7 +634,7 @@ func (bi *backendPoolTypeNodeIP) ReconcileBackendPools(ctx context.Context, clus
 		if isMigration && bi.EnableMigrateToIPBasedBackendPoolAPI {
 			var backendPoolNames []string
 			for _, id := range lbBackendPoolIDsSlice {
-				name, err := getLBNameFromBackendPoolID(id)
+				name, err := getBackendPoolNameFromBackendPoolID(id)


nilo19 · 2024-10-29T04:27:52Z

pkg/provider/azure_local_services.go

@@ -446,8 +445,10 @@ func (az *Cloud) getLocalServiceBackendPoolID(serviceName string, lbName string,

 // localServiceOwnsBackendPool checks if a backend pool is owned by a local service.
 func localServiceOwnsBackendPool(serviceName, bpName string) bool {
-	prefix := strings.Replace(serviceName, "/", "-", -1)
-	return strings.HasPrefix(strings.ToLower(bpName), strings.ToLower(prefix))
+	if strings.HasSuffix(strings.ToLower(bpName), consts.IPVersionIPv6StringLower) {


nilo19 · 2024-10-29T04:28:36Z

pkg/provider/azure_local_services.go

-			ep = endpointSlice
-			foundInCache = true
-			return false
+			eps = append(eps, endpointSlice)


feiskyer · 2024-11-11T00:32:23Z

pkg/provider/azure_local_services_test.go

+			client := fake.NewSimpleClientset(&svc)
+			// if tc.existingEPS != nil {
+			// 	client = fake.NewSimpleClientset(&svc, tc.existingEPS)
+			// } else {


nit: delete unused codes

feiskyer · 2024-11-11T00:34:21Z

pkg/provider/azure_loadbalancer_repo.go

+		bp.LoadBalancerBackendAddresses != nil {
+		for _, addr := range *bp.LoadBalancerBackendAddresses {
+			if ptr.Deref(addr.IPAddress, "") == "" {
+				logger.Info("The load balancer backend address has empty ip address, assuming it is a NIC pool",


change to v(4)

feiskyer · 2024-11-11T07:22:41Z

/retest

nilo19 · 2024-11-11T22:00:02Z

/retest

nilo19 · 2024-11-12T01:49:14Z

/test pull-cloud-provider-azure-e2e-ccm-capz

nilo19 · 2024-11-12T02:01:39Z

/test pull-cloud-provider-azure-e2e-ccm-vmss-capz

feiskyer · 2024-11-12T02:27:38Z

Thanks for the fixes
/lgtm

nilo19 · 2024-11-12T07:30:49Z

/test pull-cloud-provider-azure-e2e-ccm-capz

nilo19 · 2024-11-13T03:02:28Z

/retest

nilo19 · 2024-11-13T10:48:19Z

/retest

MartinForReal · 2024-11-14T07:45:09Z

/retest

k8s-ci-robot · 2024-11-14T10:00:23Z

@nilo19: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cloud-provider-azure-e2e-ccm-vmss-ip-lb-capz	`1731376`	link	true	`/test pull-cloud-provider-azure-e2e-ccm-vmss-ip-lb-capz`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

MartinForReal · 2024-11-15T06:30:23Z

/retest

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 29, 2024

k8s-ci-robot requested review from andyzhangx and feiskyer October 29, 2024 04:18

nilo19 commented Oct 29, 2024

View reviewed changes

nilo19 force-pushed the fix/multi-slb/endpointslice branch from 2b510ac to 5084147 Compare October 29, 2024 04:30

feiskyer reviewed Nov 11, 2024

View reviewed changes

nilo19 added 2 commits November 11, 2024 11:43

fix: Include all endpointslices for local services when using multi-slb

342f7c1

fix: Reconcile correct backend pool when using multi-slb

1731376

nilo19 force-pushed the fix/multi-slb/endpointslice branch from 5084147 to 1731376 Compare November 11, 2024 00:44

k8s-ci-robot assigned feiskyer Nov 12, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi-slb related bug fixes #7432

Multi-slb related bug fixes #7432

nilo19 commented Oct 29, 2024 •

edited

Loading

k8s-ci-robot commented Oct 29, 2024

nilo19 Oct 29, 2024

nilo19 Oct 29, 2024

nilo19 Oct 29, 2024 •

edited

Loading

nilo19 Oct 29, 2024

nilo19 Oct 29, 2024

feiskyer Nov 11, 2024

nilo19 Nov 11, 2024

feiskyer Nov 11, 2024

nilo19 Nov 11, 2024

feiskyer commented Nov 11, 2024

nilo19 commented Nov 11, 2024

nilo19 commented Nov 12, 2024

nilo19 commented Nov 12, 2024

feiskyer commented Nov 12, 2024

nilo19 commented Nov 12, 2024

nilo19 commented Nov 13, 2024

nilo19 commented Nov 13, 2024

MartinForReal commented Nov 14, 2024

k8s-ci-robot commented Nov 14, 2024

MartinForReal commented Nov 15, 2024

Multi-slb related bug fixes #7432

Are you sure you want to change the base?

Multi-slb related bug fixes #7432

Conversation

nilo19 commented Oct 29, 2024 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Oct 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilo19 Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feiskyer commented Nov 11, 2024

nilo19 commented Nov 11, 2024

nilo19 commented Nov 12, 2024

nilo19 commented Nov 12, 2024

feiskyer commented Nov 12, 2024

nilo19 commented Nov 12, 2024

nilo19 commented Nov 13, 2024

nilo19 commented Nov 13, 2024

MartinForReal commented Nov 14, 2024

k8s-ci-robot commented Nov 14, 2024

MartinForReal commented Nov 15, 2024

nilo19 commented Oct 29, 2024 •

edited

Loading

nilo19 Oct 29, 2024 •

edited

Loading