Set QP resource defaults

[skonto]

Part of the work for #13861.

Proposed Changes

• Sets the deployment cm resource values by default.
• Defaults are useful to avoid quota issues and guide users of what is the recommended values to use

Release Note

A new flag is introduced `queueproxy.resource-defaults` that sets resource requests, limits for Queue Proxy when enabled (applies only to cpu and memory).

[codecov]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (32ec382) 86.27% compared to head (82f917f) 86.27%.

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     

Impacted Files	Coverage Δ
pkg/deployment/config.go	100.00% <ø> (ø)
pkg/apis/config/features.go	95.65% <100.00%> (+0.12%)	⬆️
pkg/reconciler/revision/resources/queue.go	98.42% <100.00%> (+0.05%)	⬆️

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[skonto]

/assign @dprotaso

[xtreme-vikram-yadav]

lgtm

[dprotaso]

Is there a setting that lets you 'unset' this value? ie. empty string? If so then we should add that to the comments

[skonto]

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.

[dprotaso]

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.

[dprotaso]

I don't know if we need this much storage - in theory we use nothing no?

[skonto]

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).

[skonto]

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.

[dprotaso]

From: #14039 (comment)

Maybe the default can just remain unset

[dprotaso]

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

[dprotaso]

Should we deprecate the % knob?

[ReToCode]

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.

[evankanderson]

@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?

Dave's point was specifically about defining limits where they don't exist today. The k8s default value for limit is "unlimited" unless a value is set, so we're effectively reducing from ♾️ -> some finite value. This is probably good, particularly for a noncompressible resource like memory, but it could break someone who has a lot of really big requests buffered in-flight.

[ReToCode]

Dave's point was specifically about defining limits where they don't exist today.

Vanilla K8S does not define any defaults, so we don't get any requests & limits today

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?

[dprotaso]

FWIW - it looks like downstream we're setting our defaults to

  queue-sidecar-cpu-request: "25m"
  queue-sidecar-cpu-limit: "1000m"
  queue-sidecar-memory-request: "50Mi"
  queue-sidecar-memory-limit: "200Mi"

Can we do that when announcing it as breaking change in the release notes?

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.

[skonto]

FWIW - it looks like downstream we're setting our defaults to...

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.

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 will add a flag to make migration smoother as with secure-pod-defaults. I will add qp-resource-defaults if there is no objection. At some point we can just set this to "enabled" by default.

[dprotaso]

I will add a flag to make migration smoother as with secure-pod-defaults. I will add qp-resource-defaults if there is no objection. At some point we can just set this to "enabled" by default.

Sounds good

[skonto]

@dprotaso gentle ping. Status:

• Added the flag for smooth transition, users can choose to disable the flag for a new release that turns it on by default.
• Added some test scenarios
• Updated the release note

[dprotaso]

/lgtm
/approve

@skonto can you update the release note to say it only applies to cpu and memory?

[knative-prow]

[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:

• OWNERS [dprotaso,skonto]
• pkg/apis/OWNERS [dprotaso]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment