-
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
base: main
Are you sure you want to change the base?
📖 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
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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
ddf8026
to
45ec5ce
Compare
Nice! |
# Conflicts: # docs/book/src/developer/providers/contracts.md
45ec5ce
to
a3fcd3d
Compare
PR needs rebase. 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. |
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