Container Load Balancer - DiffTracker Integration #8464

georgeedward2000 · 2025-02-27T08:30:41Z

What type of PR is this?

/kind features

What this PR does / why we need it:

Which issue(s) this PR fixes:

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 · 2025-02-27T08:30:43Z

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

linux-foundation-easycla · 2025-02-27T08:30:44Z

❌ - login: @georgeedward2000 / name: George Edward Nechitoaia . The commit (9727188, ac1d5af, 248d01c, 47c6502) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.

k8s-ci-robot · 2025-02-27T08:30:46Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: georgeedward2000
Once this PR has been reviewed and has the lgtm label, please assign nilo19 for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

k8s-ci-robot · 2025-02-27T08:30:50Z

Hi @georgeedward2000. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

kartickmsft · 2025-03-03T06:31:57Z

pkg/provider/azure.go

+	az.diffTracker = diffTrackerState
+	// Print syncOps to surpress unused variable error
+	klog.V(2).Infof("Sync operations: %v", syncOperations)
+	// TODO (enechitoaia): call NRP APIs (including ServiceGateway) to update the state of the NRP


I think better to list a TODO for the k8 API calls as well here. The actual implementation could be done in a different function to keep it modular.

kartickmsft · 2025-03-03T06:33:41Z

pkg/provider/config/azure.go

@@ -183,6 +183,10 @@ func (az *Config) IsLBBackendPoolTypeNodeIP() bool {
 	return strings.EqualFold(az.LoadBalancerBackendPoolConfigurationType, consts.LoadBalancerBackendPoolConfigurationTypeNodeIP)
 }

+func (az *Config) IsLBBackendPoolTypePodIP() bool {
+	return strings.EqualFold(az.LoadBalancerBackendPoolConfigurationType, consts.LoadBalancerBackendPoolConfigurationTypeNodeIP) && az.UseStandardV2LoadBalancer()


BackendPoolConfigurationType should be PodIP.

kartickmsft · 2025-03-03T10:14:20Z

pkg/provider/difftracker/k8s_state_updates.go

+	return updateK8Resource(input, dt.K8s.Egresses, ResourceTypeEgress)
+}
+
+func updateK8Resource(input UpdateK8sResource, set *utilsets.IgnoreCaseSet, resourceType string) error {


I think utilsets.IgnoreCaseSet is not thread-safe. Better to make the update functions thread-safe as multiple service reconcile threads could update in parallel. Also, a read maybe happening in batch processor go routine when write via update function is taking place.

Yeah, I'd just lock/defer unlock at the entry points, might get complicated otherwise

kartickmsft · 2025-03-03T10:18:44Z

pkg/provider/difftracker/k8s_state_updates.go

+			continue
+		}
+
+		if location == "" {


This could be the first check at line 41. We need to continue.

kartickmsft · 2025-03-03T10:20:23Z

pkg/provider/difftracker/k8s_state_updates.go

+			continue
+		}
+
+		if location == "" {


Can be the first check. Continue.

kartickmsft · 2025-03-03T10:36:17Z

pkg/provider/difftracker/types.go

+	NATGatewayUpdates   SyncNRPServicesReturnType
+	LocationData        LocationData
+}
+


I think we need DTOs for Service (referring to LB/NATGW) also.

let's discuss

kartickmsft · 2025-03-03T11:09:52Z

pkg/provider/difftracker/types.go

+}
+
+type NRPState struct {
+	LoadBalancers *utilsets.IgnoreCaseSet


I'm wondering if NRPState should represent Service state in NRP. Service state in NRP will have references to LBBackendpool/NATGWs. To fit into the NRP API for Service/Location, we may need to store more information like LB name etc so that we can construct the URI.

let's discuss

kartickmsft · 2025-03-03T11:25:13Z

pkg/provider/difftracker/util.go

+	return true
+}
+
+// Map LocationData to LocationDataDTO to be used as payload in ServiceGateway API calls


Something similar is needed for Service API

let's discuss

david-kow

Some initial thoughts

david-kow · 2025-03-05T16:23:49Z

pkg/provider/config/azure.go

@@ -183,6 +183,10 @@ func (az *Config) IsLBBackendPoolTypeNodeIP() bool {
 	return strings.EqualFold(az.LoadBalancerBackendPoolConfigurationType, consts.LoadBalancerBackendPoolConfigurationTypeNodeIP)
 }

+func (az *Config) IsLBBackendPoolTypePodIP() bool {
+	return strings.EqualFold(az.LoadBalancerBackendPoolConfigurationType, consts.LoadBalancerBackendPoolConfigurationTypeNodeIP) && az.UseStandardV2LoadBalancer()


What happens if pool type is podip but lb sku is just standard (not v2)?

let's discuss

I think it's better to throw validation error in aks-rp before we pass on the settings to cloud-provider from aks-rp. There may be self-managed clusters wherein, aks-rp is not involved. So, maybe, throw validation error during cloud-provider init so that cloud-provider init fails and customer corrects the config.

david-kow · 2025-03-06T12:55:23Z

pkg/provider/azure.go

+
+	// Initialize the diff tracker state and get the necessary operations to sync the cluster with NRP
+	diffTrackerState, syncOperations := difftracker.InitializeDiffTrackerState(K8sState, NRPState)
+	az.diffTracker = diffTrackerState


naming: difftracker vs difftrackerstate might be confusing (the latter sounds like a part of difftracker itself) - should we align on the name? probably just diffTracker?

david-kow · 2025-03-06T12:58:30Z

pkg/provider/azure.go

+	diffTrackerState, syncOperations := difftracker.InitializeDiffTrackerState(K8sState, NRPState)
+	az.diffTracker = diffTrackerState
+	// Print syncOps to surpress unused variable error
+	klog.V(2).Infof("Sync operations: %v", syncOperations)


I think we can just do:

diffTrackerState, _ := difftracker.InitializeDiffTrackerState(K8sState, NRPState)

and avoid unnecessary print.

david-kow · 2025-03-06T12:59:34Z

pkg/provider/difftracker/difftracker.go

+		K8s: k8sState,
+		NRP: nrpState,
+	}
+	syncOperations := diffTrackerState.GetSyncDiffTrackerState()


Any specific reason to do that? While probably this will be a common pattern to call get that right after initialization, maybe we leave it to do to the user?

david-kow · 2025-03-06T13:00:04Z

pkg/provider/difftracker/difftracker.go

+		K8s: k8sState,
+		NRP: nrpState,
+	}
+	syncOperations := diffTrackerState.GetSyncDiffTrackerState()


Should it be called getSyncOperations?

david-kow · 2025-03-06T13:03:28Z

pkg/provider/difftracker/k8s_state_updates.go

+	return updateK8Resource(input, dt.K8s.Egresses, ResourceTypeEgress)
+}
+
+func updateK8Resource(input UpdateK8sResource, set *utilsets.IgnoreCaseSet, resourceType string) error {


Yeah, I'd just lock/defer unlock at the entry points, might get complicated otherwise

david-kow · 2025-03-06T13:14:57Z

pkg/provider/difftracker/k8s_state_updates.go

+	ResourceTypeEgress  = "Egress"
+)
+
+func (dt *DiffTrackerState) UpdateK8service(input UpdateK8sResource) error {


Would we expect to have more details here about the service config or this is not necessary?

2. Added locking at entry points 3. Added DTOs and Mappers for LocationData and Service within ServiceGateway API 4. Clean code + updated initialization/sync cloud object 5. Added/Updated Tests

kartickmsft · 2025-03-28T12:30:19Z

pkg/provider/difftracker/handlers.go

+package difftracker
+
+func (dt *DiffTracker) handleService(input UpdateK8sResource) SyncServicesReturnType {
+	dt.mu.Lock()


I think better to move the locks within the UpdateK8service. Same comment applies for the other handlers.

kartickmsft · 2025-03-28T12:31:26Z

pkg/provider/difftracker/handlers.go

+	defer dt.mu.Unlock()
+
+	dt.UpdateK8service(input)
+	SyncDiffTrackerReturnType := dt.GetSyncLoadBalancerServices()


I think we may not need the Diff. It's just update K8 state. So, I think we don't need the Get functions and as a result, locks can be made more granular.

kartickmsft · 2025-03-28T12:36:17Z

pkg/provider/difftracker/sync_operations.go

+	return syncServices
+}
+
+func (dt *DiffTracker) GetSyncLoadBalancerServices() SyncServicesReturnType {


I think better to have lock for Get Diff functions as UpdateK8 state and GetDiff will be called independently from reconcile and batchprocessor threads respectively.

kartickmsft · 2025-03-28T13:25:13Z

pkg/provider/difftracker/nrp_state_updates.go

+	utilsets "sigs.k8s.io/cloud-provider-azure/pkg/util/sets"
+)
+
+func (dt *DiffTracker) UpdateNRPLoadBalancers() {


As discussed offline, we need to pass in the Diff State that needs to be updated, as current implementation will sync NRP to the latest k8 state. Not the k8 state that we updated in NRP. Same comment applies for all Update functions.

kartickmsft · 2025-03-28T13:28:07Z

pkg/provider/difftracker/nrp_state_updates.go

+		if dt.NRPResources.LoadBalancers.Has(service) {
+			continue
+		}
+		dt.NRPResources.LoadBalancers.Insert(service)


What happens if we insert a duplicate entry? I guess it will just replace with the latest entry, which won't impact any impact if the entry is already present. If my understanding is correct, then we can remove the find in line 15. Will avoid cpu cycles for the new entries.
Similar comment applies for other Update functions as well.

kartickmsft · 2025-03-28T13:30:01Z

pkg/provider/difftracker/nrp_state_updates.go

+			continue
+		}
+		dt.NRPResources.NATGateways.Insert(egress)
+		fmt.Printf("Added egress %s to NRP NATGateways\n", egress)


Why are we not using klog here?
Comment applies to line 48 also.

kartickmsft · 2025-03-28T14:02:19Z

pkg/provider/difftracker/sync_operations.go

+			serviceRef := createServiceRef(pod)
+			addressData := Address{ServiceRef: serviceRef}
+
+			if !exists || !serviceRef.Equals(nrpLocation.Addresses[address].Services) {


Shouldn't we check if nrpLocation.Addresses[address] is present before comparing services within the address?

david-kow · 2025-03-28T11:36:56Z

pkg/provider/azure.go

+	}
+
+	// Initialize the diff tracker state and get the necessary operations to sync the cluster with NRP
+	az.diffTracker = difftracker.InitializeDiffTracker(K8s, NRP)


nit: naming, should be k8s, nrp?

david-kow · 2025-03-28T11:40:48Z

pkg/provider/azure.go

+	// loadBalancerServicesDTO := difftracker.MapLoadBalancerUpdatesToServiceDataDTO(syncOperations.LoadBalancerUpdates)
+	// natGatewayServicesDTO := difftracker.MapNATGatewayUpdatesToServiceDataDTO(syncOperations.NATGatewayUpdates)
+	// servicesDTO := difftracker.ServiceDataDTO{
+	// 	Action:   difftracker.PartialUpdate,


Should this be a full update since we know full k8s state at this point?

david-kow · 2025-03-28T11:42:14Z

pkg/provider/azure.go

+	// }
+
+	// TODO (enechitoaia): Implement the logic for ServiceGatewayClient
+	// Call ServiceGate APIs (including ServiceGateway) to update the state of the NRP


Before that we'd need to create LBs, NATs, LB rules, etc.

david-kow · 2025-03-28T11:55:56Z

pkg/provider/difftracker/k8s_state_updates.go

+	case REMOVE:
+		set.Delete(input.ID)
+	default:
+		return fmt.Errorf("error Update%s, Operation=%s and ID=%s", resourceType, input.Operation, input.ID)


I'd change that as I think the intention was to indicate func name that was called. But in case of rename, or calling from a different func this will be misleading (eg. right now this will print error UpdateService, while the func name is Update_K8s_Service). I'd print out all params as they are, and let callers decide how to present it

david-kow · 2025-03-28T12:14:47Z

pkg/provider/difftracker/difftracker_test.go

+						},
+					},
+				},
+				NRPResources: NRP{


Might be more readable if you have a helper func to generate these (eg. pass string map like in expectedNRP). Will shorten this to:

Suggested change

NRPResources: NRP{

K8sResources: gen("node1": { "10.0.0.1": { "service1"}}),

david-kow

some thoughts

kartickmsft · 2025-03-28T14:19:02Z

pkg/provider/difftracker/util.go

+			Service: service,
+			LoadBalancerBackendPools: []LoadBalancerBackendPoolDTO{
+				{
+					Id: fmt.Sprintf("%s-backendpool", service),


I think we need to specify the full uri of the LB backendpool resource in NRP.

kartickmsft · 2025-03-28T14:19:46Z

pkg/provider/difftracker/util.go

+		serviceDTO := ServiceDTO{
+			Service: service,
+			PublicNatGateway: NatGatewayDTO{
+				Id: fmt.Sprintf("%s-natgateway", service),


I think we need to specify the full URI of NATGateway resource.

1. Drop event handlers and keep divided update k8s, get sync ops, update nrp flows 2. "Update nrp" now uses sync ops 3. Granular locking to address divded flows 4. Update DTO mappings to include URI formatting 5. Updated tests

kartickmsft · 2025-04-16T07:47:11Z

pkg/provider/difftracker/nrp_state_updates.go

+			existingAddress := nrpLocation.Addresses[addressKey]
+			if !serviceRefs.Equals(existingAddress.Services) {
+				existingAddress.Services = serviceRefs
+				nrpLocation.Addresses[addressKey] = existingAddress


I think we don't need to add the existingAddress back as it's already retrieved in line 78, which means it already exists in nrpLocation.Addresses.

kartickmsft · 2025-04-16T09:03:54Z

pkg/provider/difftracker/sync_operations.go

+)
+
+// SyncServices handles the synchronization of services between K8s and NRP
+func syncServices(k8sServices, Services *utilsets.IgnoreCaseSet) SyncServicesReturnType {


Should we rename it to GetServicesToSync to imply that it returns the services to sync?

kartickmsft · 2025-04-16T10:36:20Z

pkg/provider/difftracker/types.go

+	Pods map[string]Pod
+}
+
+type K8s struct {


Should we rename it to K8s_State and NRP_State for better readability?

kartickmsft · 2025-04-16T10:38:55Z

pkg/provider/difftracker/types.go

+}
+
+// LocationDataDTO represents the DTO for LocationData
+type LocationDataDTO struct {


LocationsDataDTO is better?

kartickmsft · 2025-04-16T10:39:20Z

pkg/provider/difftracker/types.go

+	isDelete                 bool
+}
+
+type ServiceDataDTO struct {


ServicesDataDTO is better?

kartickmsft · 2025-04-16T10:43:04Z

pkg/provider/difftracker/util.go

+func (dt *DiffTracker) DeepEqual() bool {
+	// Compare Services with LoadBalancers
+	if dt.K8sResources.Services.Len() != dt.NRPResources.LoadBalancers.Len() {
+		klog.Errorf("Services and LoadBalancers length mismatch")


Mismatch between k8 and NRP is not really an error. It could be just a transient state where we didn't sync NRP with k8 yet. So, suggesting to not log mismatches as an error in this function.

kartickmsft · 2025-04-16T10:55:56Z

pkg/provider/difftracker/util.go

+					klog.Errorf("Identity %s not found in Services for pod %s in node %s\n", identity, podKey, nodeKey)
+					return false
+				}
+			}


Shouldn't we check if services in nrp are different from k8?

It checks on line 72 (Inbound) and line 90 (Outbound) for services in NRP_State and not in K8s_State

kartickmsft · 2025-04-16T11:25:29Z

pkg/provider/difftracker/util.go

+						subscriptionID,
+						resourceGroup,
+						service,
+						fmt.Sprintf("%s-backendpool", service),


Please check the name of the backendpool that's used for CLB in createOrUpdateBackendPool. You could refer to getLocalServiceBackendPoolName()

Type property has been introduced for Service. We could pass Type as "Inbound" as we are mapping LB to Service.

This is what I found in a test for createOrUpdateBackendPool, which follows the same structure as the one I used:
ptr.To(fmt.Sprintf("/subscriptions/subscriptionID/resourceGroups/rg/providers/Microsoft.Network/loadBalancers/%s/backendAddressPools/%s", lbName, bpName)),

I thought line 350 should be fmt.Sprintf("%s",service). I mean not "%s-backendpool".

kartickmsft · 2025-04-16T11:32:14Z

pkg/provider/difftracker/util.go

+	for _, service := range natGatewayUpdates.Additions.UnsortedList() {
+		serviceDTO := ServiceDTO{
+			Service: service,
+			PublicNatGateway: NatGatewayDTO{


Type property has been introduced for Service. We could pass Type as "Outbound" as we are mapping NatGw to Service.

…efactor

difftracker initial integration (left TODOs)

9727188

k8s-ci-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Feb 27, 2025

k8s-ci-robot requested review from andyzhangx and feiskyer February 27, 2025 08:30

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. do-not-merge/needs-kind needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 27, 2025

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 27, 2025

georgeedward2000 mentioned this pull request Feb 27, 2025

Add support for backendpool of type PodIP #8395

Open

kartickmsft reviewed Mar 3, 2025

View reviewed changes

david-kow reviewed Mar 6, 2025

View reviewed changes

1. Updated namings

ac1d5af

2. Added locking at entry points 3. Added DTOs and Mappers for LocationData and Service within ServiceGateway API 4. Clean code + updated initialization/sync cloud object 5. Added/Updated Tests

kartickmsft reviewed Mar 28, 2025

View reviewed changes

david-kow reviewed Mar 28, 2025

View reviewed changes

kartickmsft reviewed Mar 28, 2025

View reviewed changes

Changes List:

47c6502

1. Drop event handlers and keep divided update k8s, get sync ops, update nrp flows 2. "Update nrp" now uses sync ops 3. Granular locking to address divded flows 4. Update DTO mappings to include URI formatting 5. Updated tests

kartickmsft reviewed Apr 16, 2025

View reviewed changes

Updating namings, add ServiceType to ServicesDataDTO, update tests, r…

248d01c

…efactor

github-actions bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Apr 17, 2025

	NRPResources: NRP{
	K8sResources: gen("node1": { "10.0.0.1": { "service1"}}),

Container Load Balancer - DiffTracker Integration #8464

Are you sure you want to change the base?

Container Load Balancer - DiffTracker Integration #8464

Uh oh!

Conversation

georgeedward2000 commented Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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.:

Uh oh!

k8s-ci-robot commented Feb 27, 2025

Uh oh!

linux-foundation-easycla bot commented Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Feb 27, 2025

Uh oh!

k8s-ci-robot commented Feb 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kartickmsft Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kartickmsft Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kartickmsft Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-kow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

georgeedward2000 commented Feb 27, 2025 •

edited

Loading

linux-foundation-easycla bot commented Feb 27, 2025 •

edited

Loading

kartickmsft Mar 3, 2025 •

edited

Loading

kartickmsft Mar 3, 2025 •

edited

Loading

kartickmsft Mar 3, 2025 •

edited

Loading

kartickmsft Mar 28, 2025 •

edited

Loading

kartickmsft Mar 28, 2025 •

edited

Loading

kartickmsft Apr 16, 2025 •

edited

Loading

kartickmsft Apr 16, 2025 •

edited

Loading