Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🌱 Reduce cyclomatic complexity of reconcileNetworkComponents #1905

Merged

Conversation

mdbooth
Copy link
Contributor

@mdbooth mdbooth commented Feb 26, 2024

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 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 26, 2024
@k8s-ci-robot
Copy link
Contributor

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

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 k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 26, 2024
Copy link

netlify bot commented Feb 26, 2024

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

Name Link
🔨 Latest commit 268645a
🔍 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
Copy link
Contributor

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

@EmilienM
Copy link
Contributor

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

@mdbooth
Copy link
Contributor Author

mdbooth commented Feb 27, 2024

Lazily hoping that was a flake without checking:

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

@lentzi90
Copy link
Contributor

lentzi90 commented Feb 27, 2024

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 🎉 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
Copy link
Contributor Author

mdbooth commented Feb 27, 2024

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 #1840

@k8s-ci-robot 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 28, 2024
@mdbooth mdbooth force-pushed the reconcileNetworkComponents branch 2 times, most recently from f9bb7e8 to 196022d Compare February 28, 2024 12:08
@mdbooth
Copy link
Contributor Author

mdbooth commented Feb 28, 2024

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
Copy link
Contributor Author

mdbooth commented Feb 28, 2024

Yay! Looks like that fixed it.

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.

Co-Authored-By: Emilien Macchi <emacchi@redhat.com>
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 28, 2024
@mdbooth
Copy link
Contributor Author

mdbooth commented Feb 28, 2024

/hold cancel

@k8s-ci-robot 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 28, 2024
@EmilienM
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2024
@k8s-ci-robot k8s-ci-robot merged commit a405eeb into kubernetes-sigs:main Feb 28, 2024
9 checks passed
@mdbooth mdbooth deleted the reconcileNetworkComponents branch February 28, 2024 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants