kubernetes-sigs / cluster-api-provider-openstack

Cluster API implementation for OpenStack
https://cluster-api-openstack.sigs.k8s.io/
Apache License 2.0
283 stars 253 forks source link

🌱 Reduce cyclomatic complexity of reconcileNetworkComponents #1905

Closed mdbooth closed 6 months ago

mdbooth commented 6 months ago

This change refactors reconcileNetworkComponents into several smaller logical functions which are easier to read and reason about.

It also makes the gocyclo linter happy when making new changes to this code.

/hold

k8s-ci-robot commented 6 months ago

[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](https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/main/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[bot] commented 6 months ago

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

Name Link
Latest commit 268645a3bee5e88344c78c6965db8f7b099773b9
Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/65df360be884290007dcb666
Deploy Preview https://deploy-preview-1905--kubernetes-sigs-cluster-api-openstack.netlify.app
Preview on mobile
Toggle QR Code...

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 commented 6 months ago

/test pull-cluster-api-provider-openstack-e2e-test while I'm reviewing it, so see if the e2e issue was transient.

EmilienM commented 6 months ago

@mdbooth I've added little tests to it, but I think we need to:

mdbooth commented 6 months ago

Lazily hoping that was a flake without checking:

/test pull-cluster-api-provider-openstack-e2e-test

lentzi90 commented 6 months ago

Testing if our jobs now work in the community prow cluster /test pull-cluster-api-provider-openstack-e2e-test

Update: Looks like it is working :tada: Sorry for the noice. (It does not matter if this test fails, I just wanted to make sure we could get the devstack up)

mdbooth commented 6 months ago

Looks like something related to floating IPs is borked because we run out. Perhaps they're not being deleted, or created in a loop. It's hard to tell, though, because of https://github.com/kubernetes-sigs/cluster-api-provider-openstack/issues/1840

mdbooth commented 6 months ago

I think I found the problem from code inspection, although this PR also currently includes #1913 for improved logging just in case. The issue was that we were re-executing floating IP creation on every cluster reconcile when not using an Octavia loadbalancer.

mdbooth commented 6 months ago

Yay! Looks like that fixed it.

mdbooth commented 6 months ago

/hold cancel

EmilienM commented 6 months ago

/lgtm