Replace liveness and readiness with initialization node controller #1186
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
✔️ Deploy Preview for karpenter-docs-prod canceled. 🔨 Explore the source changes: 382440e 🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/61e8a48a02c93b00075484ef |
mattsre
reviewed
Jan 19, 2022
ellistarn
reviewed
Jan 19, 2022
ellistarn
reviewed
Jan 19, 2022
ellistarn
reviewed
Jan 19, 2022
ellistarn
reviewed
Jan 19, 2022
suket22
changed the title
bwagner5
reviewed
Jan 24, 2022
| } | ||
|
|
||
| if !node.IsReady(n) { | ||
| if age := injectabletime.Now().Sub(n.GetCreationTimestamp().Time); age < InitializationTimeout { |
There was a problem hiding this comment.
you could shorten with:
if age := injectabletime.Since(n.GetCreationTimestamp().Time); age < InitializationTimeout {
| if age := injectabletime.Now().Sub(n.GetCreationTimestamp().Time); age < InitializationTimeout { | ||
| return reconcile.Result{RequeueAfter: InitializationTimeout - age}, nil | ||
| } | ||
| logging.FromContext(ctx).Infof("Triggering termination for node that failed to become ready") |
There was a problem hiding this comment.
I think it would be helpful to log the node name here, wdyt?
There was a problem hiding this comment.
This is included in our logger. All reconcilers include resource name in their logger at the top level. It helps avoid redundant statements.
ellistarn
approved these changes
Jan 24, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
1. Issue, if available:
#1135
2. Description of changes:
Earlier we would only delete worker nodes iff the kubelet would have a status of
NodeStatusNeverUpdatedfor more than 15 minutes. If this were the case, it meant the Kubelet never connected to the API Server at all.We now delete worker nodes iff the kubelet is in NotReady status for more than 15 minutes after startup. The mechanism we use to discover that a node has only been started is by looking at a taint we apply during node object creation. Once the taint has been removed, we declare the startup as complete and no longer re-evaluate the node for deletion. We will re-introduce something like a liveness controller in the future if necessary but we want to be very careful with that to maintain static stability of the cluster.
3. How was this change tested?
Case 1 - I removed the CNI policy from the Node Role. Nodes were stuck in NotReady and I can see them being terminated after 15 minutes.
Case 2 - I'm still looking to replicate the case, where a node does come up as healthy, but then goes into NotReady. I'm probably going to try a network partition to replicate this scenario but this is WIP.
4. Does this change impact docs?
I don't think this impact our docs but I'm happy to add a callout somewhere if needed.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.