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

Ensure IPAddress has a ClusterName label as CAPI resources #220

Merged
merged 2 commits into from
Feb 8, 2024

Conversation

zhanggbj
Copy link
Contributor

What this PR does / why we need it:
CAPI has a contract to label all created resources with a ClusterName label. CAPV will label IPAddressClaim. This PR is to ensure IPAddress also got one.

Please find more discussion in kubernetes-sigs/cluster-api-provider-vsphere#2218

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

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 10, 2024
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 10, 2024
Copy link
Member

@schrej schrej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

I would really like to merge #203 before this, and then add this change there though. I'm still looking for some feedback on it, but rikatz hasn't gotten around to it yet.
Let me know if you'd like to have this right away. If someone else is available to take a look at #203 I'd of course appreciate that as well!

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 10, 2024
@schrej
Copy link
Member

schrej commented Jan 10, 2024

/remove-approve
/lgtm

Somewhat unintuitive 😄

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 10, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: schrej, zhanggbj

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

@schrej schrej removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 10, 2024
@zhanggbj
Copy link
Contributor Author

zhanggbj commented Jan 11, 2024

@schrej thanks for your review! No rush, but it would be great to merge this small change If everything works fine.
BTW, looks like the test is failing, and is there any way to retrigger it without push commit again?

@rikatz
Copy link
Contributor

rikatz commented Jan 24, 2024

@zhanggbj IIRC you can trigger the tests directly back on pipelines...

Or...let's see if prow is smart:

/retest

@schrej
Copy link
Member

schrej commented Jan 29, 2024

We've merged #203 now. Could you adapt your PR? I think it makes sense to move this into the generic reconciler, probably around here: https://github.com/kubernetes-sigs/cluster-api-ipam-provider-in-cluster/blob/main/pkg/ipamutil/reconciler.go#L210

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 29, 2024
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 30, 2024
Signed-off-by: Gong Zhang <gongz@vmware.com>
@schrej
Copy link
Member

schrej commented Jan 31, 2024

I finally figured out why the test is failing:

reconciler.go:133] "IPAddressClaim linked to a cluster that is not found, unable to determine cluster's paused state, skipping reconciliation" controller="ipaddressclaim" controllerGroup="ipam.cluster.x-k8s.io" controllerKind="IPAddressClaim" IPAddressClaim="test-ns-t25bg/test" namespace="test-ns-t25bg" name="test" reconcileID="764c6bb4-3a5e-4e90-a5e7-0f0fdf3bed9c"

Can you create the cluster you're referencing during that test to avoid reconciliation to be skipped?

@schrej schrej added this to the v0.1.0 milestone Feb 8, 2024
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 8, 2024
@schrej
Copy link
Member

schrej commented Feb 8, 2024

/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 8, 2024
@k8s-ci-robot k8s-ci-robot merged commit 11e9866 into kubernetes-sigs:main Feb 8, 2024
5 checks passed
@zhanggbj
Copy link
Contributor Author

@schrej Thank you for fixing the test! I just came back from a vacation.

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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants