-
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
🌱 ipam: add Ready condition failure reasons #10660
🌱 ipam: add Ready condition failure reasons #10660
Conversation
// AllocationFailedReason is the reason used when allocating an IP address for a claim fails. More details should be | ||
// provided in the condition's message. | ||
// When the IP pool is full, [PoolExhaustedReason] should be used for better visibility instead. | ||
AllocationFailedReason = "AllocationFailed" |
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.
Question here is whether it makes sense to have more specific reasons depending on provider, and whether this one is therefore superfluous. In general it probably doesn't matter why the allocation failed exactly, and details can still be provided in the condition message.
The only exception is the pool being full, which can be remediated, which is why the second reason below exists.
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 fine to add this reason. It should be good enough for most cases and if someone wants to have a more specific infra-provider reason they can still use it
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.
The only exception is the pool being full, which can be remediated, which is why the second reason below exists.
This makes me think if AllocationFailed should be a condition on its own and PoolNotReady a reason. But you're more familiar than me so feel free to disregard.
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.
Yeah, I thought about that as well. Since there is only one thing that happens - you either get an address or you don't - I don't really think it makes sense to have multiple conditions.
Or is the intention to differentiate between no reconciliation yet and a failed reconciliation?
a502359
to
4fa60b1
Compare
|
||
// These reasons are intended to be used with failed Ready conditions on an [IPAddressClaim]. | ||
const ( | ||
// AllocationFailedReason is the reason used when allocating an IP address for a claim fails. More details should be |
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.
Is there any hint we can give in the comment about user action for this reason, e.g. how permanent / transient this failure might be?
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 that heavily depends on the provider. More details should be provided in the message. I can expand the comment regarding that though.
The only generic issues I can think of atm. are the ones we already have specific reasons for: a full pool or a not-ready pool (e.g. because the system backing the pool can't be reached).
/lgtm |
LGTM label has been added. Git tree hash: 2c7d10035f08648476f81b4b5444ff2fc313015b
|
4fa60b1
to
b339567
Compare
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.
docs nit, lgtm as of that
b339567
to
ee0a832
Compare
ee0a832
to
8d86eda
Compare
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.
/lgtm
LGTM label has been added. Git tree hash: ac8d8c3243391706e338cf9ae395f0f3ee7e2d82
|
/lgtm /assign @enxebre |
PR desc mentions 2 reasons but the code implement 3 now. Can you please update the PR desc to reflect latest changes? Feel free to cancel the hold afterwards. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enxebre 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 |
/hold cancel |
What this PR does / why we need it:
This adds three failure reasons for the Ready condition of IPAddressClaims that should be used by IPAM providers when reconciling claims. The goal is to offer some consistency between providers when observing conditions for e.g. alerting.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Related to #8424
/area ipam