-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Include "computed QoS" instead of "status QoS" in validation error when in-place resource updates result in recalculated QoS #124713
base: master
Are you sure you want to change the base?
Conversation
Pod QoS is immutable, but it is a computed field, so in-place resource changes can result in updates being rejected because they would change the QoS class -- e.g. removing all resources from a Burstable pod would re-compute its QoS class as BestEffort, so it's not allowed. When we return an error for a re-computed QoS, we erroneously log the existing/status QoS class rather than the new computed one, which makes it very confusing, e.g. a Burstable pod getting re-calculated as BestEffort would log that Qos could not be changed to Burstable because it was immutable. This commit just changes the output such that it properly logs the computed QoS that couldn't be applied, which should make it easier for the user to understand why their resource changes are impossible. Signed-off-by: John Kyros <jkyros@redhat.com>
Welcome @jkyros! |
Hi @jkyros. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jkyros 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 |
/sig node |
/triage accepted |
/retest |
computedNewQOS := qos.ComputePodQOS(newPod) | ||
if qos.GetPodQOS(oldPod) != computedNewQOS { | ||
allErrs = append(allErrs, field.Invalid(fldPath, computedNewQOS, "Pod QoS is immutable")) |
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.
nit
computedNewQOS := qos.ComputePodQOS(newPod) | |
if qos.GetPodQOS(oldPod) != computedNewQOS { | |
allErrs = append(allErrs, field.Invalid(fldPath, computedNewQOS, "Pod QoS is immutable")) | |
if computedNewQOS := qos.ComputePodQOS(newPod); qos.GetPodQOS(oldPod) != computedNewQOS { | |
allErrs = append(allErrs, field.Invalid(fldPath, computedNewQOS, "Pod QoS is immutable")) |
computedNewQOS := qos.ComputePodQOS(newPod) | ||
if qos.GetPodQOS(oldPod) != computedNewQOS { | ||
allErrs = append(allErrs, field.Invalid(fldPath, computedNewQOS, "Pod QoS is immutable")) |
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 it possible/easy to understand if there's an in-place scaling taking place? If it is, the problem can be made much clearer if we mention this was caused due to a resize IMO. WDYT?
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.
Yes, I think the only time this message can come out is when there is an in-place scaling taking place because:
- the function we're in (
ValidatePodUpdate
) seems to only validate the spec, while the QoS is in the status - we're comparing "old" QoS to "computed new" QoS, so "new" QoS is ignored
So I think at least here it's just kind of quietly ignoring the case where you directly change the QoS.
So hmm, keeping with the tone of the other validation errors, maybe something like pod resource changes may not change pod QoS, pod QoS is immutable
?
What type of PR is this?
/kind bug
What this PR does / why we need it:
Currently:
This PR:
Example:
Take this pod:
Remove the resources to make it recalculate to
BestEffort
:Observe the validation output (the pod is already
Burstable
, but the error saysBurstable
is the invalid value):Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: