🌱 ipam: add spec.clusterName to IPAddressClaim #10182

schrej · 2024-02-20T16:46:45Z

What this PR does / why we need it:
Adds a spec.clusterName property to IPAddressClaims to allow properly referencing the Cluster to which the claim belongs. This is required for pausing claim reconciliation when the cluster is paused, which in turn is necessary for clusterctl move support.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Discussed in #9883

/area ipam

schrej · 2024-02-20T17:48:41Z

exp/ipam/api/v1alpha1/conversion.go

+		if _, ok := dst.ObjectMeta.Labels["cluster.x-k8s.io/cluster-name"]; !ok {
+			if dst.ObjectMeta.Annotations == nil {
+				dst.ObjectMeta.Annotations = map[string]string{}
+			}
+			dst.ObjectMeta.Annotations["conversion.cluster.x-k8s.io/cluster-name-label-set"] = "false"
+		}


This is done to make the fuzzy conversion tests happy. I preferred this over an annotation with a json representation of the old object, but can switch to that "standard" approach as well.

I would prefer if we use the standard approach here for consistency

Or is it a use case that we intentionally want to cover that controllers written against v1alpha1 might be able to get the cluster-name from the annotation? (which would only work if the spec.clusterName field is set)

Wondering if these controllers can simply upgrade to v1beta1 instead? Or does somebody already rely on the label?

I guess if we don't have to support some use cases around this I would prefer to just go with the standard conversion approach and avoid additional complexity.

Yeah, the idea was to support old providers. If we don't want that we can also just be forward-compatible instead. I'd be fine with just keeping the annotation when converting old to new, but fuzzy testing is unhappy.

Can we keep it this way? Since this is still considered experimental API we can probably remove v1alpha1 rather soon anyway.

Sorry I'll get back to it to take another look.

The way the apiserver is working today we'll never going to be able to actually drop v1alpha1 & the corresponding conversion (just set served: false)
(xref: #10051 (comment))

Ouch, ok. Well, then let me know if you're fine with this approach or whether I should switch to the regular one. I guess once we've set it to served: false we no longer need to update conversion for newer changes?

Maybe we can also just always set it and adapt the tests so they're fine with that.

Discussed this with Jakob here: https://kubernetes.slack.com/archives/C8TSNPY4T/p1714405639788599

Overall we will go the way with the additional annotation. The problem is that the annotation and the funcs we usually use doesn't cover metadata. So we can't use it to backup/restore the cluster-name. But even if we could use it to backup/restore the cluster-name label, we wouldn't want to do it. If someone modifies the cluster-name label on the v1alpha1 type we don't want to preserve the value that that label might have had before the conversion on the v1beta1 type.

mcbenjemaa

This is okay,
~~but I don't see the part where the claim pauses the reconciliation if the cluster is paused~~

my bad, I got confused sometimes.
after this is merged and released, we need to support this field in the ipam providers.

mcbenjemaa · 2024-02-21T09:49:43Z

/ok-to-test

exp/ipam/api/v1beta1/ipaddressclaim_types.go

exp/ipam/api/v1alpha1/conversion.go

exp/ipam/api/v1beta1/ipaddressclaim_types.go

Co-authored-by: Stefan Bueringer <[email protected]>

sbueringer · 2024-04-29T16:27:07Z

Thank you very much!

/lgtm

k8s-ci-robot · 2024-04-29T16:27:13Z

LGTM label has been added.

Git tree hash: cb301a63ac3fb264c7b93042a0eaac5b32d5e9dc

sbueringer · 2024-04-29T16:27:22Z

/assign @chrischdi

For a sanity check

chrischdi · 2024-04-30T09:09:13Z

/approve

Looks good, thanks

k8s-ci-robot · 2024-04-30T09:09:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrischdi

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

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

sbueringer · 2024-04-30T09:25:13Z

/lgtm

Accidentally dropped it when adding the squash label

k8s-ci-robot · 2024-04-30T09:25:18Z

LGTM label has been added.

Git tree hash: cb301a63ac3fb264c7b93042a0eaac5b32d5e9dc

ipam: add spec.clusterName to IPAddressClaim

9960dda

k8s-ci-robot added area/ipam Issues or PRs related to ipam size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 20, 2024

k8s-ci-robot requested review from fabriziopandini and JoelSpeed February 20, 2024 16:46

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 20, 2024

schrej mentioned this pull request Feb 20, 2024

✨ Make sure IPAddressClaim has a cluster name label. #9883

Closed

schrej commented Feb 20, 2024

View reviewed changes

mcbenjemaa reviewed Feb 21, 2024

View reviewed changes

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Feb 21, 2024

mcbenjemaa reviewed Feb 21, 2024

View reviewed changes

exp/ipam/api/v1beta1/ipaddressclaim_types.go Outdated Show resolved Hide resolved

sbueringer reviewed Mar 13, 2024

View reviewed changes

exp/ipam/api/v1alpha1/conversion.go Outdated Show resolved Hide resolved

schrej force-pushed the ipam-ipaddressclaim-cluster-name branch from 0cb1e43 to b54469d Compare March 25, 2024 11:49

schrej force-pushed the ipam-ipaddressclaim-cluster-name branch from b54469d to dbc96ca Compare April 29, 2024 16:05

sbueringer reviewed Apr 29, 2024

View reviewed changes

exp/ipam/api/v1beta1/ipaddressclaim_types.go Outdated Show resolved Hide resolved

schrej force-pushed the ipam-ipaddressclaim-cluster-name branch from dbc96ca to 0498a29 Compare April 29, 2024 16:17

fix fuzzy conversion tests and omit empty spec.ClusterName

bb75a9f

Co-authored-by: Stefan Bueringer <[email protected]>

schrej force-pushed the ipam-ipaddressclaim-cluster-name branch from 0498a29 to bb75a9f Compare April 29, 2024 16:24

k8s-ci-robot assigned sbueringer Apr 29, 2024

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

sbueringer added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Apr 29, 2024

k8s-ci-robot assigned chrischdi Apr 29, 2024

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

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

k8s-ci-robot merged commit c6e5706 into kubernetes-sigs:main Apr 30, 2024
22 checks passed

k8s-ci-robot added this to the v1.8 milestone Apr 30, 2024

schrej deleted the ipam-ipaddressclaim-cluster-name branch April 30, 2024 10:17

schrej mentioned this pull request May 21, 2024

Add support for IPAddressClaim spec.clusterName once CAPI 1.8 is released kubernetes-sigs/cluster-api-ipam-provider-in-cluster#258

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 ipam: add spec.clusterName to IPAddressClaim #10182

🌱 ipam: add spec.clusterName to IPAddressClaim #10182

schrej commented Feb 20, 2024

schrej Feb 20, 2024 •

edited

Loading

sbueringer Mar 13, 2024

sbueringer Mar 13, 2024

sbueringer Mar 13, 2024

schrej Mar 13, 2024

schrej Apr 17, 2024

sbueringer Apr 17, 2024

schrej Apr 17, 2024 •

edited

Loading

sbueringer Apr 29, 2024 •

edited

Loading

mcbenjemaa left a comment •

edited

Loading

mcbenjemaa commented Feb 21, 2024

sbueringer commented Apr 29, 2024

k8s-ci-robot commented Apr 29, 2024

sbueringer commented Apr 29, 2024

chrischdi commented Apr 30, 2024

k8s-ci-robot commented Apr 30, 2024

sbueringer commented Apr 30, 2024

k8s-ci-robot commented Apr 30, 2024

🌱 ipam: add spec.clusterName to IPAddressClaim #10182

🌱 ipam: add spec.clusterName to IPAddressClaim #10182

Conversation

schrej commented Feb 20, 2024

schrej Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

sbueringer Mar 13, 2024

Choose a reason for hiding this comment

sbueringer Mar 13, 2024

Choose a reason for hiding this comment

sbueringer Mar 13, 2024

Choose a reason for hiding this comment

schrej Mar 13, 2024

Choose a reason for hiding this comment

schrej Apr 17, 2024

Choose a reason for hiding this comment

sbueringer Apr 17, 2024

Choose a reason for hiding this comment

schrej Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

sbueringer Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

mcbenjemaa left a comment • edited Loading

Choose a reason for hiding this comment

mcbenjemaa commented Feb 21, 2024

sbueringer commented Apr 29, 2024

k8s-ci-robot commented Apr 29, 2024

sbueringer commented Apr 29, 2024

chrischdi commented Apr 30, 2024

k8s-ci-robot commented Apr 30, 2024

sbueringer commented Apr 30, 2024

k8s-ci-robot commented Apr 30, 2024

schrej Feb 20, 2024 •

edited

Loading

schrej Apr 17, 2024 •

edited

Loading

sbueringer Apr 29, 2024 •

edited

Loading

mcbenjemaa left a comment •

edited

Loading