🐛 fix: watch ipaddressclaim in openstackserver controller #2390

MisterKind · 2025-01-23T13:31:26Z

What this PR does / why we need it:
Add watch of IPAddressClaims in the OpenstackServer controller

Which issue(s) this PR fixes :
Fixes #2389

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

squashed commits

/hold

linux-foundation-easycla · 2025-01-23T13:31:30Z

The committers listed above are authorized under a signed CLA.

✅ login: MisterKind / name: Loïc Brun (4373e72)

k8s-ci-robot · 2025-01-23T13:31:35Z

Welcome @MisterKind!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-openstack 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-openstack has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2025-01-23T13:31:36Z

Hi @MisterKind. 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.

k8s-ci-robot · 2025-01-23T13:31:37Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign cecilerobertmichon 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

netlify · 2025-01-23T13:31:44Z

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

Name	Link
🔨 Latest commit	`4373e72`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/67924772c614f00008e867c5
😎 Deploy Preview	https://deploy-preview-2390--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 · 2025-01-23T19:24:23Z

/ok-to-test

EmilienM · 2025-01-23T19:25:10Z

I think you'll need to run make generate in order to generate new RBAC in the CRDs.

EmilienM · 2025-01-23T19:27:26Z

controllers/openstackserver_controller.go

@@ -214,6 +216,10 @@ func (r *OpenStackServerReconciler) SetupWithManager(ctx context.Context, mgr ct
 			}),
 			builder.WithPredicates(predicates.NewBecameAvailable(mgr.GetLogger(), &orcv1alpha1.Image{})),
 		).
+		Watches(
+			&ipamv1.IPAddressClaim{},
+			handler.EnqueueRequestForOwner(mgr.GetScheme(), mgr.GetRESTMapper(), &infrav1alpha1.OpenStackServer{}),


I know it's a copy paste from the openstackmachine controller but I wonder if we would want a predicate at some point, to ensure we trigger the create only when the FIP is created.

EmilienM · 2025-01-23T19:28:47Z

controllers/openstackserver_controller.go

@@ -77,6 +77,8 @@ type OpenStackServerReconciler struct {

 // +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=openstackservers,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=openstackservers/status,verbs=get;update;patch
+// +kubebuilder:rbac:groups=ipam.cluster.x-k8s.io,resources=ipaddressclaims;ipaddressclaims/status,verbs=get;watch;create;update;patch;delete
+// +kubebuilder:rbac:groups=ipam.cluster.x-k8s.io,resources=ipaddresses;ipaddresses/status,verbs=get;list;watch


@mdbooth @lentzi90 I'll use a second pair of eyes here, as I'm not 100% we want a copy/paste from the OpenStackMachine controller. I thought that ipaddressclaim would be enough but I don't know enough that resource. I can dig into it next week (at this point) if you don't know either.

We need both for the OpenstackServer controller, here links to code where we fetch IPAddressClaims and IPAddresses
https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/main/controllers/openstackserver_controller.go#L575
https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/main/controllers/openstackserver_controller.go#L624

However, maybe the watch and RBAC are not needed anymore in the OpenstackMachine controller.

Indeed I'm not sure either and we have no CI coverage for that feature 😕 ...

EmilienM

Thanks for your first contribution, it's very appreciated. It touches an area where I'm not 100% comfortable (the FIP resource) with so I pinged Matt and Lennart. If you don't hear from us by mid next week, please let me know. I think it's an important bug to fix.

Also we'll need it backported.

EmilienM · 2025-01-23T19:31:56Z

/cherry-pick release-0.12
/cherry-pick release-0.11

k8s-infra-cherrypick-robot · 2025-01-23T19:31:59Z

@EmilienM: once the present PR merges, I will cherry-pick it on top of release-0.11, release-0.12 in new PRs and assign them to you.

In response to this:

/cherry-pick release-0.12
/cherry-pick release-0.11

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.

MisterKind · 2025-01-24T16:26:27Z

I think you'll need to run make generate in order to generate new RBAC in the CRDs.

I already did it, but as the RBAC are the same as the OpenstackMachine controller, there no modifications.

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

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jan 23, 2025

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 23, 2025

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 23, 2025

k8s-ci-robot requested review from lentzi90 and mdbooth January 23, 2025 13:31

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jan 23, 2025

fix: watch ipaddressclaim in openstackserver controller

4373e72

MisterKind force-pushed the fix/openstackserver-floatingips branch from 5519450 to 4373e72 Compare January 23, 2025 13:43

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 23, 2025

EmilienM reviewed Jan 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 fix: watch ipaddressclaim in openstackserver controller #2390

🐛 fix: watch ipaddressclaim in openstackserver controller #2390

MisterKind commented Jan 23, 2025 •

edited

Loading

linux-foundation-easycla bot commented Jan 23, 2025 •

edited

Loading

k8s-ci-robot commented Jan 23, 2025

k8s-ci-robot commented Jan 23, 2025

k8s-ci-robot commented Jan 23, 2025

netlify bot commented Jan 23, 2025 •

edited

Loading

EmilienM commented Jan 23, 2025

EmilienM commented Jan 23, 2025

EmilienM Jan 23, 2025

EmilienM Jan 23, 2025

MisterKind Jan 24, 2025

EmilienM Jan 24, 2025

EmilienM left a comment •

edited

Loading

EmilienM commented Jan 23, 2025

k8s-infra-cherrypick-robot commented Jan 23, 2025

MisterKind commented Jan 24, 2025

🐛 fix: watch ipaddressclaim in openstackserver controller #2390

Are you sure you want to change the base?

🐛 fix: watch ipaddressclaim in openstackserver controller #2390

Conversation

MisterKind commented Jan 23, 2025 • edited Loading

linux-foundation-easycla bot commented Jan 23, 2025 • edited Loading

k8s-ci-robot commented Jan 23, 2025

k8s-ci-robot commented Jan 23, 2025

k8s-ci-robot commented Jan 23, 2025

netlify bot commented Jan 23, 2025 • edited Loading

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

EmilienM commented Jan 23, 2025

EmilienM commented Jan 23, 2025

EmilienM Jan 23, 2025

Choose a reason for hiding this comment

EmilienM Jan 23, 2025

Choose a reason for hiding this comment

MisterKind Jan 24, 2025

Choose a reason for hiding this comment

EmilienM Jan 24, 2025

Choose a reason for hiding this comment

EmilienM left a comment • edited Loading

Choose a reason for hiding this comment

EmilienM commented Jan 23, 2025

k8s-infra-cherrypick-robot commented Jan 23, 2025

MisterKind commented Jan 24, 2025

MisterKind commented Jan 23, 2025 •

edited

Loading

linux-foundation-easycla bot commented Jan 23, 2025 •

edited

Loading

netlify bot commented Jan 23, 2025 •

edited

Loading

EmilienM left a comment •

edited

Loading