🌱 Reduce cyclomatic complexity of ReconcileLoadBalancer #1904

mdbooth · 2024-02-22T16:06:12Z

This function had become genuinely too complex over time, to the point that even the linter was starting to complain about it when making almost any change.

This change refactors ReconcileLoadBalancer into several smaller logical functions which are much easier to read and reason about. It also revealed some trivial optimisations:

Only fetch Octavia providers if we need them to create a new loadbalancer
Only calculate allowed CIDRs once
Don't re-fetch a loadbalancer to check it's active if it's already active

Apart from just the code cleanup, this has the benefit of not requiring these changes to be mixed into other PRs when the complexity exceeds the linter's threshold.

/hold

k8s-ci-robot · 2024-02-22T16:06:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mdbooth

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~~ [mdbooth]

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

netlify · 2024-02-22T16:06:27Z

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name	Link
🔨 Latest commit	`e8ccd21`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/65de485fd2f4b0000862af65
😎 Deploy Preview	https://deploy-preview-1904--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

EmilienM · 2024-02-22T17:10:22Z

pkg/cloud/services/loadbalancer/loadbalancer.go

+
+// getAPIServerVIPAddress gets the VIP address for the API server from wherever it is specified.
+// Returns an empty string if the VIP address is not specified and it should be allocated automatically.
+func getAPIServerVIPAddress(openStackCluster *infrav1.OpenStackCluster) (string, error) {


we could unit test it but I'm not going to nitpick on that. Just make sure the existing tests cover the output of that function and I'm fine.

I wrote it.

EmilienM · 2024-02-22T17:10:34Z

pkg/cloud/services/loadbalancer/loadbalancer.go

+
+// getAPIServerFloatingIP gets the floating IP from wherever it is specified.
+// Returns an empty string if the floating IP is not specified and it should be allocated automatically.
+func getAPIServerFloatingIP(openStackCluster *infrav1.OpenStackCluster) (string, error) {


I wrote it.

EmilienM · 2024-02-22T17:10:44Z

pkg/cloud/services/loadbalancer/loadbalancer.go

+// getCanonicalAllowedCIDRs gets a filtered list of CIDRs which should be allowed to access the API server loadbalancer.
+// Invalid CIDRs are filtered from the list and emil a warning event.
+// It returns a canonical representation that can be directly compared with other canonicalized lists.
+func getCanonicalAllowedCIDRs(openStackCluster *infrav1.OpenStackCluster) []string {


I wrote it.

EmilienM · 2024-02-22T17:10:53Z

pkg/cloud/services/loadbalancer/loadbalancer.go

 }

-func (s *Service) getOrCreateLoadBalancer(openStackCluster *infrav1.OpenStackCluster, loadBalancerName, subnetID, clusterName, vipAddress, provider string) (*loadbalancers.LoadBalancer, error) {
+// getOrCreateAPILoadBalancer returns an existing API loadbalancer if it already exists, or creates a new one if it does not.
+func (s *Service) getOrCreateAPILoadBalancer(openStackCluster *infrav1.OpenStackCluster, clusterName string) (*loadbalancers.LoadBalancer, error) {


I wrote it.

EmilienM · 2024-02-22T17:11:18Z

pkg/cloud/services/loadbalancer/loadbalancer.go

@@ -230,7 +307,41 @@ func (s *Service) getOrCreateLoadBalancer(openStackCluster *infrav1.OpenStackClu
 	return lb, nil
 }

-func (s *Service) getOrCreateListener(openStackCluster *infrav1.OpenStackCluster, listenerName, lbID string, port int) (*listeners.Listener, error) {
+// reconcileAPILoadBalancerListener ensures that the listener on the given port exists and is configured correctly.
+func (s *Service) reconcileAPILoadBalancerListener(lb *loadbalancers.LoadBalancer, openStackCluster *infrav1.OpenStackCluster, clusterName string, port int) error {


do we have unit test coverage for this one?

no we don't but it's not an easy one so I've filled #1906 so we can get back to it at some point.

EmilienM

Awesome change, I just have questions on unit tests coverage.

mdbooth · 2024-02-26T13:32:32Z

/cc @MaysaMacedo as this touches some of your code.

mdbooth · 2024-02-26T13:38:03Z

I 100% agree about unit test coverage. However, this is basically just code motion so I can separate these changes from the other changes I'm making to the same code without triggering the gocyclo linter. IOW this is just yak shaving for me right now.

If you fancied taking the yak shears and writing some good unit tests while I get back to the API stuff I'd add to the beers I owe you.

EmilienM · 2024-02-27T13:22:19Z

/lgtm
I'll let another reviewer take a look.

mdbooth · 2024-02-27T14:46:54Z

/hold cancel

MaysaMacedo · 2024-02-27T14:44:41Z

pkg/cloud/services/loadbalancer/loadbalancer.go

-		return false, fmt.Errorf("load balancer %q with id %s is not active after timeout: %v", loadBalancerName, lb.ID, err)
+	if lb.ProvisioningStatus != loadBalancerProvisioningStatusActive {
+		var err error
+		lb, err = s.waitForLoadBalancerActive(lb.ID)


why do we need to return the lb again if it was already retrieved at line 79?
Looks like all the places that need the LB after this line 91 look for the provider, the ID or VipPortID of the LB, so I believe returning it again is unnecessarily.

Because it's a different object, returned from the most recent query. It's probably not necessary, but I figured that as we already had the latest state we might as well use it.

e.g. If we don't do this and for whatever reason we happened to look at ProvisioningStatus again, we would see that it's still not active, because we're still referencing the original version of the object before it became active.

MaysaMacedo · 2024-02-27T14:55:43Z

pkg/cloud/services/loadbalancer/loadbalancer.go

+		return fmt.Errorf("APIServerLoadBalancer is not yet available in OpenStackCluster.Status")
+	}
+
+	allowedCIDRs := openStackCluster.Status.APIServerLoadBalancer.AllowedCIDRs


has the allowedCIDRs been updated in the openStackCluster previously?

Yes. It was set directly in ReconcileLoadBalancer before reconciling any listeners.

MaysaMacedo · 2024-02-27T14:56:44Z

pkg/cloud/services/loadbalancer/loadbalancer.go

+}
+
+// getCanonicalAllowedCIDRs gets a filtered list of CIDRs which should be allowed to access the API server loadbalancer.
+// Invalid CIDRs are filtered from the list and emil a warning event.


MaysaMacedo · 2024-02-27T15:02:25Z

pkg/cloud/services/loadbalancer/loadbalancer.go


-	return false, nil
+	// Filter invalid CIDRs and convert any IPs into CIDRs.


couldn't we keep using validateIPs function?

It's a toss-up, but I thought it was too small to be a separate function with only one caller. This function is small enough now, I think it's clearer to inline it here.

Also it doesn't just validate, it also filters, which you wouldn't know just from its name so you'd have to go read the function intentionally to notice that.

MaysaMacedo · 2024-02-27T15:07:46Z

/hold

@mdbooth holding just to give a chance for you to check my comments, feel free to unhold once you checked them

mdbooth

Thanks, @MaysaMacedo. I fixed the typo and responded to the other comments. Let me know what you think.

mdbooth · 2024-02-27T18:02:07Z

pkg/cloud/services/loadbalancer/loadbalancer.go

-		return false, fmt.Errorf("load balancer %q with id %s is not active after timeout: %v", loadBalancerName, lb.ID, err)
+	if lb.ProvisioningStatus != loadBalancerProvisioningStatusActive {
+		var err error
+		lb, err = s.waitForLoadBalancerActive(lb.ID)


Because it's a different object, returned from the most recent query. It's probably not necessary, but I figured that as we already had the latest state we might as well use it.

e.g. If we don't do this and for whatever reason we happened to look at ProvisioningStatus again, we would see that it's still not active, because we're still referencing the original version of the object before it became active.

mdbooth · 2024-02-27T18:06:57Z

pkg/cloud/services/loadbalancer/loadbalancer.go

+		return fmt.Errorf("APIServerLoadBalancer is not yet available in OpenStackCluster.Status")
+	}
+
+	allowedCIDRs := openStackCluster.Status.APIServerLoadBalancer.AllowedCIDRs


Yes. It was set directly in ReconcileLoadBalancer before reconciling any listeners.

mdbooth · 2024-02-27T18:31:29Z

pkg/cloud/services/loadbalancer/loadbalancer.go


-	return false, nil
+	// Filter invalid CIDRs and convert any IPs into CIDRs.


It's a toss-up, but I thought it was too small to be a separate function with only one caller. This function is small enough now, I think it's clearer to inline it here.

Also it doesn't just validate, it also filters, which you wouldn't know just from its name so you'd have to go read the function intentionally to notice that.

MaysaMacedo · 2024-02-27T18:42:14Z

/lgtm

This function had become genuinely too complex over time, to the point that even the linter was starting to complain about it when making almost any change. This change refactors ReconcileLoadBalancer into several smaller logical functions which are much easier to read and reason about. It also revealed some trivial optimisations: * Only fetch Octavia providers if we need them to create a new loadbalancer * Only calculate allowed CIDRs once * Don't re-fetch a loadbalancer to check it's active if it's already active Co-Authored-By: Emilien Macchi <emacchi@redhat.com>

EmilienM · 2024-02-28T01:48:08Z

Restoring Maysa's LGTM, fixing unit tests and removing /hold.
/lgtm
/hold cancel

dalees · 2024-03-11T02:26:17Z

pkg/cloud/services/loadbalancer/loadbalancer.go

-				allowedCIDRs = listener.AllowedCIDRs
+			}
+
+			if openStackCluster.Status.Router != nil && len(openStackCluster.Status.Router.IPs) > 0 {


This fixes a bug we just hit when specifying both internalNetwork.networkFilter and apiServerLoadBalancer.allowedCIDRs. Thanks for adding this nil guard!

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 22, 2024

k8s-ci-robot requested review from dulek and lentzi90 February 22, 2024 16:06

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 22, 2024

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 22, 2024

EmilienM reviewed Feb 22, 2024

View reviewed changes

EmilienM force-pushed the ReconcileLoadBalancer branch from 726d253 to 94b97cc Compare February 26, 2024 16:23

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 26, 2024

mdbooth force-pushed the ReconcileLoadBalancer branch from 94b97cc to 4a78a3f Compare February 26, 2024 16:44

EmilienM force-pushed the ReconcileLoadBalancer branch from 4a78a3f to fb6e0ee Compare February 26, 2024 17:09

k8s-ci-robot assigned EmilienM Feb 27, 2024

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

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 27, 2024

MaysaMacedo reviewed Feb 27, 2024

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 27, 2024

mdbooth commented Feb 27, 2024

View reviewed changes

mdbooth force-pushed the ReconcileLoadBalancer branch from fb6e0ee to 9ae35a3 Compare February 27, 2024 18:32

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2024

k8s-ci-robot assigned MaysaMacedo Feb 27, 2024

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

EmilienM force-pushed the ReconcileLoadBalancer branch from 9ae35a3 to e8ccd21 Compare February 27, 2024 20:38

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2024

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Feb 28, 2024

k8s-ci-robot merged commit 05d4e9b into kubernetes-sigs:main Feb 28, 2024
9 checks passed

dalees reviewed Mar 11, 2024

View reviewed changes

EmilienM deleted the ReconcileLoadBalancer branch May 27, 2024 12:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Reduce cyclomatic complexity of ReconcileLoadBalancer #1904

🌱 Reduce cyclomatic complexity of ReconcileLoadBalancer #1904

mdbooth commented Feb 22, 2024 •

edited

k8s-ci-robot commented Feb 22, 2024

netlify bot commented Feb 22, 2024 •

edited

EmilienM Feb 22, 2024

EmilienM Feb 26, 2024

EmilienM Feb 22, 2024

EmilienM Feb 26, 2024

EmilienM Feb 22, 2024

EmilienM Feb 26, 2024

EmilienM Feb 22, 2024

EmilienM Feb 26, 2024

EmilienM Feb 22, 2024

EmilienM Feb 26, 2024

EmilienM left a comment

mdbooth commented Feb 26, 2024

mdbooth commented Feb 26, 2024

EmilienM commented Feb 27, 2024

mdbooth commented Feb 27, 2024

MaysaMacedo Feb 27, 2024

mdbooth Feb 27, 2024

MaysaMacedo Feb 27, 2024

mdbooth Feb 27, 2024

MaysaMacedo Feb 27, 2024

MaysaMacedo Feb 27, 2024

mdbooth Feb 27, 2024

MaysaMacedo commented Feb 27, 2024

mdbooth left a comment

mdbooth Feb 27, 2024

mdbooth Feb 27, 2024

mdbooth Feb 27, 2024

MaysaMacedo commented Feb 27, 2024

EmilienM commented Feb 28, 2024

dalees Mar 11, 2024


		return false, nil
		// Filter invalid CIDRs and convert any IPs into CIDRs.

🌱 Reduce cyclomatic complexity of ReconcileLoadBalancer #1904

🌱 Reduce cyclomatic complexity of ReconcileLoadBalancer #1904

Conversation

mdbooth commented Feb 22, 2024 • edited

k8s-ci-robot commented Feb 22, 2024

netlify bot commented Feb 22, 2024 • edited

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EmilienM left a comment

Choose a reason for hiding this comment

mdbooth commented Feb 26, 2024

mdbooth commented Feb 26, 2024

EmilienM commented Feb 27, 2024

mdbooth commented Feb 27, 2024

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

MaysaMacedo commented Feb 27, 2024

mdbooth left a 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

MaysaMacedo commented Feb 27, 2024

EmilienM commented Feb 28, 2024

Choose a reason for hiding this comment

mdbooth commented Feb 22, 2024 •

edited

netlify bot commented Feb 22, 2024 •

edited