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.
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
KEP-1287: InPlacePodVerticalScaling BETA update #4704
KEP-1287: InPlacePodVerticalScaling BETA update #4704
Changes from 1 commit
68952e4
453b7fe
60c5d79
ff92a6e
d774b5c
1a4fefb
59e0e46
079aa5f
a093b5b
be6445f
964260d
d7b996f
8a67b07
e53dcc5
ad43d22
9c2404d
5388d74
8e48db1
a505a5a
da2e849
7aee63a
ad13b95
04de3da
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Change to "No"?
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.
BTW, what does "disable on a running node" mean? Can this feature be turned on/off on a single node?
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.
TBH, I don't remember why I added this. I don't think it should be a problem to disable it, although there are some potential race conditions around 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.
This is a PRR ask. Featuregate allows you to enable/disable it at will, kubelet gets the feature flag from config/cli param. There was one bug around hash calculation that would restart containers even those not using the feature, but that has been fixed. Other races might exist and hopefully get discovered is it bakes alpha/beta pre-prod deployments.
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.
PRR reviewer here: I think I'm following the discussion but want to be sure... Can this be made to work without creating new nodes? If so I think we're all set from a PRR perspective. I don't think we can answer this with "no" and consider the feature production ready.
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.
OK, I removed the statement that the node needs to be recreated or drained. However, if there are pending or in-progress resizes, it's possible pods could be left in an unknown state, where depending on exactly when the restart happened they could be finished resizing, not resized, or partially resized, and the pod API will not accurately reflect the state.
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.
Sounds like there are enough gotchas here that we should retain some of this in the answer. I asked PRR leads about this situation and one piece of feedback was that we shouldn't assume clusters have disposable nodes, so proscribing that nodes should be drained here would be appropriate, but proscribing that new nodes be created would not 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.
Perhaps also addition of a RuntimeHandlerFeature for resize? as commented 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.
that's more for the underlying oci runtimes to report features they have available. We could add
features
to the runtime status object maybe, though I think the runtimes have been compatible for a while unless the design is changingThere 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.
there's now RuntimeFeature which we could use if we wanted to