-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Set QP resource defaults #14039
Set QP resource defaults #14039
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #14039 +/- ##
=======================================
Coverage 86.27% 86.27%
=======================================
Files 199 199
Lines 14795 14808 +13
=======================================
+ Hits 12764 12776 +12
- Misses 1730 1732 +2
+ Partials 301 300 -1
☔ View full report in Codecov by Sentry. |
/assign @dprotaso |
lgtm |
queue-sidecar-ephemeral-storage-request: "512Mi" | ||
|
||
# Sets the queue proxy's ephemeral storage limit. | ||
# If omitted, no value is specified and the system default is used. | ||
# If omitted, a default value (currently "1024Mi"), is used. |
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 a setting that lets you 'unset' this value? ie. empty string? If so then we should add that to the comments
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.
Config map validation does not allow an empty string eg. queue-sidecar-memory-limit: ""
, as the quantity is parsed and checked:
error: configmaps "config-deployment" could not be patched: admission webhook "config.webhook.serving.knative.dev" denied the request: validation failed: failed to parse "queue-sidecar-memory-limit": quantities must match the regular expression '^([+-]?[0-9.]+)([eEinumkKMGTP][-+]?[0-9])$'
The only way is not to set a value. However with this PR if the user enables the feature for QP defaults (planning to introduce a feature flag) he could only change them via the configmap but not unset them, unless I have a feature flag per resource type.
The latter is redundant as if they want partially to define defaults they can always set them explicitly in the corresponding configmap and disable this feature.
The idea is to enable this feature by default at some point as a good practice.
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.
Maybe an explicit unset
constant could be used to allow the default to be unset.
This could also be the way operators can retain the prior behaviour allowing us to safely change the default over two releases.
queue-sidecar-memory-limit: "800Mi" | ||
|
||
# Sets the queue proxy's ephemeral storage request. | ||
# If omitted, no value is specified and the system default is used. | ||
# If omitted, a default value (currently "512Mi"), is used. |
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 don't know if we need this much storage - in theory we use nothing 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.
I was not sure about that either, there is an interesting history behind the validation of the quota resources: kubernetes/kubernetes#110956. Also ephemeral storage is not validated as reported in the ticket, explanation here. So I think its safe to remove (I will double check).
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.
After some experimentation it seems to me that ephemeral storage should be skipped as there is no guarantee that someone will use it if set and can cause issues in a namespace where there is a hard limit on it via a resourceQuota.
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.
From: #14039 (comment)
Maybe the default can just remain unset
queue-sidecar-memory-request: "400Mi" | ||
|
||
# Sets the queue proxy's memory limit. | ||
# If omitted, no value is specified and the system default is used. | ||
# If omitted, a default value (currently "800Mi"), is used. |
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.
Do these defaults still make sense? Also could setting these break existing users - ie. there's some queue-proxy using more than 800Mi - and now upgrading is causing crashes
Should we deprecate the % knob? |
I agree with Daves comments, the current requests seem very high, as these are blocking resources that we probably do not use. I still have a todo open to get the real world scenario numbers for various components. This might help to find better defaults in a follow-up PR. @dprotaso I do not see a way how this is not breaking. Vanilla K8S does not define any defaults, so we don't get any requests & limits today. We cannot not break that if we define something. Can we add it as a breaking change to the release notes? Other than that, LGTM. |
Dave's point was specifically about defining limits where they don't exist today. The k8s default value for |
Exactly. This is why I think it will always be a breaking change. Can we do that when announcing it as breaking change in the release notes? |
FWIW - it looks like downstream we're setting our defaults to
Yeah. I also think we need a way to 'unset' this so we can apply this default to our KinD installations and give operators a way to 'preserve' past behaviour. |
I was just using the numbers downstream suggested by @norman465 which work at a bigger scale, but lower values could be more attractive for average scenarios, so I will use that.
I will add a flag to make migration smoother as with |
Sounds good |
84898f9
to
82f917f
Compare
@dprotaso gentle ping. Status:
|
/lgtm @skonto can you update the release note to say it only applies to cpu and memory? |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dprotaso, skonto 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 |
* enforce qp resource defaults * add flag and several fixes
Part of the work for #13861.
Proposed Changes
Release Note