-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
📖 book: add ipam contract #10108
📖 book: add ipam contract #10108
Conversation
Skipping CI for Draft Pull Request. |
04ef806
to
3900311
Compare
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
@schrej Still interested in this one? |
Yes, just forgot about it... I think I put it on hold due to some uncertainty with regards to |
/remove-lifecycle stale |
@schrej is this still wip? lgtm as good starting point |
@schrej friendly reminder 😀 |
3900311
to
f1d5f69
Compare
I think this should be ready now. I've updated it to include the new Sorry that it took so long. |
f1d5f69
to
ddf8026
Compare
@lubronzhan Do you maybe have some time to take a first look? (in case you're familiar with what we did in CAPV at the time) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late reply. Overall LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx! Just a few small findings.
|
||
1. Create an IPAddressClaim | ||
1. The `spec.poolRef` must reference the pool you want to use | ||
2. It should have an owner reference to the infrastructure Machine it is created for (required to support `clusterctl move`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about controller / blockOwnerDeletion here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. We're currently investigating an issue when deleting metal3 based clusters where claims are deadlocked due to the cluster vanishing before they are cleaned up. The paused check prevents the claim from being released if the cluster can't be found. We'll either have to make sure that the cluster doesn't get deleted, or ignore the paused check when releasing addresses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose to merge this as is, and we'll update it if we change how this works in the in-cluster ipam implementation.
I've added a note to the ipam issue: kubernetes-sigs/cluster-api-ipam-provider-in-cluster#289
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a good idea to set both and I've added a bullet point that recommends it. I've also added a sentence to deletion indicating that the infra Machine deletion should block until the claim is gone, which is the case with the correct owner ref.
ddf8026
to
45ec5ce
Compare
Nice! |
45ec5ce
to
a3fcd3d
Compare
a3fcd3d
to
8f1147b
Compare
8f1147b
to
a06e6b1
Compare
a06e6b1
to
074ede5
Compare
Thanks for documenting this contract, good documentation is the foundation of CAPI extensibility! |
LGTM label has been added. Git tree hash: 0d317cd80dc02c2a55e5f6cc1ff25b80820ebd11
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini 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 |
What this PR does / why we need it:
Adds the IPAM contract to the book.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):/area ipam
/area documentation