RFC: Degraded NodePool Status Condition #1885
Conversation
k8s-ci-robot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rschalo 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 |
k8s-ci-robot
added
cncf-cla: yes
Pull Request Test Coverage Report for Build 12366878113Details
💛 - Coveralls |
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
There's another option. Conditions can have three values, so we can have a condition Stable (or a different name).
If Karpenter self-detects flapping, or the logic for the proposed Degraded condition were to fire, Stable can become false. Crucially, if Karpenter is not sure, the Stable condition can become Unknown and we can provide a reason string for that.
For example, I'd set a brand-new NodePool to Stable Unknown, and eagerly transition (from Stable false unknown to Stable unknown false) as soon as just one NodeClaim fails to provision.
|
So in the case of utilizing Flapping in and out of |
It's weak evidence. For example, let's say 4 out of 5 nodes fail to provision (not at random; let's say exactly one in 5 succeeds). If that continues long term I would prefer to see the status become - or remain as - If a new NodePool provisions one node and that one node is fine, sure, turn the |
| ConditionTypeDegraded = "Degraded" | ||
| ``` | ||
|
|
||
| This option would set `Degraded: true` on a NodePool whenever Karpenter suspects something is wrong with the launch path but isn't sure. In this case, if 3 or more NodeClaims fail to launch with a NodePool then the NodePool will be marked as degraded. The retries are included to account for transient errors. The number of failed launches is stored as a status on the NodePool and then reset to zero following an edit to the NodePool or a sufficient amount time has passed. |
There was a problem hiding this comment.
Have you thought of something we could do to make it scale to the size of the cluster? And which errors are included in "failing to launch"?
There was a problem hiding this comment.
The number of failed launches is stored as a status on the NodePool and then reset to zero following an edit to the NodePool or a sufficient amount time has passed.
I worry about false positives for "waiting to edit" the nodepool. I'm guessing this would be bubbled up from the nodeclass, but I think having the "amount of time" as a backstop makes sense. What amount of time are you thinking?
There was a problem hiding this comment.
I was thinking that the edit to a NodePool would be a signal that a user updated the NodePool and is expecting the issue to be resolved.
What amount of time are you thinking?
To keep it simple, 3x the NodeClaim registration TTL.
There was a problem hiding this comment.
Have you thought of something we could do to make it scale to the size of the cluster? And which errors are included in "failing to launch"?
Not really, the number of NodePools/NodeClasses wouldn't affect the number of failures since we're looking at the state of a single NodePool. I think the cluster size would just affect how quickly a NodePool is considered degraded.
And which errors are included in "failing to launch"?
This currently includes node registration failures.
| Resources v1.ResourceList `json:"resources,omitempty"` | ||
| // FailedLaunches tracks the number of times a nodepool failed before being marked degraded | ||
| // +optional | ||
| FailedLaunches int `json:"failedLaunches,omitempty"` |
There was a problem hiding this comment.
How can we indicate that this is "failed launches in a row"? Does this mechanically reset if we get a successful launch?
| FailedLaunches int `json:"failedLaunches,omitempty"` | |
| FailedSuccessiveLaunches int `json:"failedLaunches,omitempty"` |
There was a problem hiding this comment.
This is probably actually FailedRegistrations
There was a problem hiding this comment.
So the number of times we hit registration TTL?
The azure provider will use the native controller retries for successive launches of a nodeclaim.
Attempt 1: Try with Cheapest sku -> FAIL Quota error(need a better quota api), cache sku as lacking quota for
Attempt 2: Try again but with the next cheapest sku -> Succeed.
So sometimes with our provider we will have an expected launch failure, that we know will succeed on the retry.
I like FailedRegistrations better, but also for some of these LRO operations, being able to report back from the cloudprovider side when a launch fails would be useful.
There was a problem hiding this comment.
Maybe this should be nodeClaimRecentLaunchSuccessRatio. We really care about recent launch attempts and failures, not the entire lifecycle of the NodePool. But I also don't think that one successful launch should discard the info here.
Imagine if the NodePool spans multiple zones but is misconfigured, except for in a zone that the workload hardly uses. (eg: workload uses zones 1 and 2; cluster supports zones 1, 2 and 3). Launching in zone 3 might make things look better than they are.
| } | ||
| ``` | ||
|
|
||
| Once a NodePool is `Degraded`, it recovers with `Degraded: false` after an update to the NodePool or when the NodeClaim registration expiration TTL (currently 15 minutes) passes since the `lastTransitionTime` for the status condition on the NodePool, whichever comes first. A `Degraded` NodePool is not passed over when provisioning and may continue to be chosen during scheduling. A successful provisioning could also remove the status condition but this may cause more apiserver and metric churn than is necessary. |
There was a problem hiding this comment.
Once a NodePool is
Degraded, it recovers withDegraded: falseafter an update to the NodePool or when the NodeClaim registration expiration TTL (currently 15 minutes) passes since thelastTransitionTimefor the status condition on the NodePool, whichever comes first.
Doesn't this defeat the purpose? if it's 15m then I imagine if I'm having bootstrapping errors that I'll almost never hit this feature if I'm not scaling up 3 nodes at a time.
There was a problem hiding this comment.
A
DegradedNodePool is not passed over when provisioning and may continue to be chosen during scheduling.
Maybe eventually configurable if we get it right. i could imagine users wanting to orchestrate a signal (or maybe leverage this one) to signal to Karpenter when they think something's wrong.
There was a problem hiding this comment.
A successful provisioning could also remove the status condition but this may cause more apiserver and metric churn than is necessary.
This seems like a natural time to un mark the status condition. This only causes churn if the issue is ephemeral or racey, which in the ephemeral case, I'd imagine the NodePool isn't degraded. I'd like to understand what some of the racey cases would be though.
There was a problem hiding this comment.
Doesn't this defeat the purpose? if it's 15m then I imagine if I'm having bootstrapping errors that I'll almost never hit this feature if I'm not scaling up 3 nodes at a time.
Yea, I was initially thinking it was the same as NodeClaim registration but it's probably 3x it to address this issue.
Maybe eventually configurable if we get it right. i could imagine users wanting to orchestrate a signal (or maybe leverage this one) to signal to Karpenter when they think something's wrong.
Yea, I think for introducing this we don't want to make functional changes yet but it could be done in the future. One thing I want to sort out is how to appropriately add/filter metrics for NodeClaims being provisioned for NodePools that are degraded.
|
|
||
| Once a NodePool is `Degraded`, it recovers with `Degraded: false` after an update to the NodePool or when the NodeClaim registration expiration TTL (currently 15 minutes) passes since the `lastTransitionTime` for the status condition on the NodePool, whichever comes first. A `Degraded` NodePool is not passed over when provisioning and may continue to be chosen during scheduling. A successful provisioning could also remove the status condition but this may cause more apiserver and metric churn than is necessary. | ||
|
|
||
| As additional misconfigurations are handled, they can be added to the `Degraded` status condition and the `Degraded` controller expanded to handle automated recovery efforts. This is probably most simpoly achieved by changing the Status Condition metrics to use comma-delimiting for `Reason`s with the most recent change present in the `Message`. |
There was a problem hiding this comment.
Is there prior art for doing this? I'm wondering if we're actually going to be able to bubble up these failures correctly anyway, or if this is work we're carving out that wouldn't even be usable.
There was a problem hiding this comment.
reason should be a single machine readable value and we don'tdelimit it
If we wanted multiple reasons, the API would support a list here.
There was a problem hiding this comment.
We can fire Events, though, and I think we can even store annotations onto those Events if we want to.
There was a problem hiding this comment.
Agree that the csv style reasons is a bad idea.
We can fire Events, though, and I think we can even store annotations onto those Events if we want to.
I think it's possible we fire an event for the NodeClaim if the backing compute fails to join the cluster. The only time these events would spam is if the NodePool/NodeClass is so misconfigured that every initialization attempt fails. Looks like we currently only event for initial NodeClaim launch failures and not for failures in Registration and Initialization.
|
|
||
| This introduces challenges when determining when to evaluate contributors to the status condition but since the `Degraded` status condition only has a single contributor this decision can be punted. When the time comes to implement the multiple contributors to this status condition, this probably looks like a `Degraded` controller which acts as a "heartbeat" and evaluates each of the contributors. | ||
|
|
||
| Finally, this status condition would not be a precondition for NodePool `Readiness` because the NodePool should still be considered for scheduling purposes. |
There was a problem hiding this comment.
But this would need to be rolled up somehow from the NodeClass too, right? So we'd need a new nodepool "Degraded" condition which relies on the nodeclass "Degraded"
There was a problem hiding this comment.
Can we actually assume that the NodeClass has a .status field? (I don't think we should assume that).
There was a problem hiding this comment.
We should probably just track on the NodePool but we can do this in a way where which NodeClass was used is stored as a hash on the NodePool (see approach 4).
designs/degraded-nodepools.md
Outdated
|
|
||
| #### Considerations | ||
|
|
||
| 1. 👎 Three retries can still be a long time to wait on compute that would never succeed |
There was a problem hiding this comment.
Maybe I misunderstood, but resetting the timer on the registration timeout means we never hit this, right?
|
|
||
| #### Considerations | ||
|
|
||
| 1. 👎👎 Validation implies the original state of the NodePool was correct and is something Karpenter can vet with certainty. A NodePool could have been correctly validated but then degraded. |
There was a problem hiding this comment.
False positives. I think the severity of this con depends on what signals we're choosing to include.
designs/degraded-nodepools.md
Outdated
|
|
||
| 1. 👎👎 Validation implies the original state of the NodePool was correct and is something Karpenter can vet with certainty. A NodePool could have been correctly validated but then degraded. | ||
| 2. 👎👎 Changes the meaning of `Validated` in terms of `Readiness` | ||
| 3. 👎 Relies on statuses that were not part of the original validation of a NodePool. |
There was a problem hiding this comment.
I'm not sure I get what this con is
There was a problem hiding this comment.
I was thinking of Validation as a "valid state of the NodePool when it was first created" and that it should be true when we know something is definitely true or false. The failures Degraded seeks to capture are for grey areas where something might be wrong and we aren't sure.
There was a problem hiding this comment.
I think a NodePool can start valid and become invalid (eg due to someone writing a new spec, or because they deleted the node class object, or potentially even by the cloud provider changing its offering).
- Example of a NodePool becoming invalid through a spec update: set a requirement for
karpenter.sh/capacity-type: ["nonexistent"] - Maybe if someone sets
expireAfterto zero, that should also be invalid?
k8s-ci-robot
added
size/L
| // LaunchedNode indicates if the current NodePool configuration has launched a node. | ||
| // +optional | ||
| Verified bool `json:"verified,omitempty"` |
There was a problem hiding this comment.
| // LaunchedNode indicates if the current NodePool configuration has launched a node. | |
| // +optional | |
| Verified bool `json:"verified,omitempty"` | |
| // Verified indicates if the current NodePool configuration has launched a node. | |
| // +optional | |
| Verified bool `json:"verified,omitempty"` |
Using language like launch is interesting, the node has launched but to a user is it clear the differences between Launched, Registered, Initialized etc? Are we going to use the Launched condition, or will we check that its gone through the full nodeclaim lifecycle before verifying a nodepool?
Maybe they aren't configuring labels for daemonsets on their nodes correctly, so a daemonset dropping configuration for CNI never initializes and pods can't schedule, the nodes startup taints for that CNI are never are removed, but from the controllers perspective we have moved past a launch so its considered verified.
Should we only consider a nodepool verified if a nodeclaim has gone through all the create lifecycle operations? (Launch + Initialization + Registration)
There was a problem hiding this comment.
the node has launched but to a user is it clear the differences between Launched, Registered, Initialized etc?
we have moved past a launch so its considered verified.
Are we going to use the Launched condition, will we check that its gone through the full nodeclaim lifecycle before verifying a nodepool?
My mistake in wording - I think we consider a NodePool Verified after all NodeClaim lifecycle operations succeed for a given NodePool/NodeClass.
As far as Karpenter could tell, everything should have succeeded. Karpenter didn't see a cloudprovider create error but some other misconfiguration prevented a node from joining the cluster. If the NodePool/NodeClass has been verified then we can fire an event on the NodePool about the most recent failure with a probable cause. That could be the startup taints issue you mention or hitting registration TTLs because of no network path.
There was a problem hiding this comment.
Also, if a nodepool is updated then these verifications should be cleared.
Description
See Design File for full RFC but this change seeks to add the
Degradedstatus condition to NodePools in order to better communicate to users when a NodePool may be misconfigured.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.