Allow setting Queue Proxy resources in ConfigMap #8195

julz · 2020-06-04T11:19:25Z

Proposed Changes

defaults request.cpu to 25m for backwards compatibility
all other requests and limits also maintain current defaults
(i.e. nil/unspecified/inherit kubernetes)
annotation continues to work to override based on user container resources

Release Note

Operators can now set Queue Proxy resource requests/limits in the config-deployment.yaml config map

knative-prow-robot

@julz: 0 warnings.

In response to this:

Fixes #8012.

Proposed Changes

defaults request.cpu to 25m for backwards compatibility

all other requests and limits also maintain current defaults
(i.e. nil/unspecified/inherit kubernetes)

annotation continues to work to override based on user container resources

Release Note
Operators can now set Queue Proxy resource requests/limits in the config-deployment.yaml config map

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/test-infra repository.

pkg/deployment/config.go

taragu · 2020-06-04T15:36:03Z

pkg/reconciler/revision/resources/queue_test.go

+		rev: revision("bar", "foo",
+			withContainers(containers),
+			withContainerConcurrency(1)),
+		cc: deployment.Config{


Should we rename cc to dc?

yep, good catch

vagababov · 2020-06-04T19:43:36Z

pkg/deployment/config_test.go

+		got.QueueSidecarCPULimit = nil
+		got.QueueSidecarMemoryRequest, got.QueueSidecarMemoryLimit = nil, nil
+		got.QueueSidecarEphemeralStorageRequest, got.QueueSidecarEphemeralStorageLimit = nil, nil


Shouldn't we actually set those as defaults anyway?

Maybe eventually, but for this PR the feature proposal specifically says we won’t change current defaults (which are empty ie inherit k8s/no limit), we’ll just let operators override that if they want.

vagababov · 2020-06-04T19:44:11Z

config/core/configmaps/deployment.yaml

+    # queueSidecarCPULimit is the limits.cpu to set for the queue proxy sidecar container.
+    # If omitted, no value is specified and the system default is used.
+    queueSidecarCPULimit: "25m"


Should this be larger?

yeah, seems reasonable that it should be bigger than request, bumped it to 75m

.. actually, bumped it some more :D. Might as well make it 1000m = 1 full cpu and have it match default container resources. It's only an example anyway but limit is a hard limit so better to have the example be on the high side imo.

vagababov · 2020-06-04T19:44:36Z

pkg/deployment/config.go

 )

 func defaultConfig() *Config {
 	return &Config{
 		ProgressDeadline:               ProgressDeadlineDefault,
 		RegistriesSkippingTagResolving: sets.NewString("ko.local", "dev.local"),
+		QueueSidecarCPURequest:         &QueueSidecarCPURequestDefault,


I think we need to set all of them. Since that's what the CM above implies.

The CM example doc explicitly says that all fields except this one (to avoid changing current defaults) default to unset (ie no limit). This is same behaviour - again other than cpu to avoid changing current default - as defaults.yaml has for user container resource defaults: if you don’t explicitly set, we default to no request/limit, but we still have examples of what valid values look like.

But I thought for QP we do set them, vs what we do for UserContainer.

yah I think eventually it'd make sense to, but tbh I don't know what the correct values to default to would be for all workloads yet so I limited the initial feature proposal to letting operators set, but maintaining existing behaviour (i.e unset/inherit/infinite) if they don't. Hopefully we can experiment a bit and determine good defaults once this lands.

shashwathi

/lgtm
question: is there need for validation of resource limit/requests set in cm?

julz · 2020-06-05T04:25:24Z

@savitaashture we don’t currently validate similar fields for setting default user container resources so I think it’s ok if we don’t validate these here.

vagababov

/lgtm
/assign @dprotaso @mattmoor

- defaults request.cpu to 25m for backwards compatibility - all other requests and limits maintain current default (i.e. nil/unspecified/inherit kubernetes default) - annotation continues to work to override

julz · 2020-06-08T18:02:26Z

rebased

dprotaso · 2020-06-09T14:01:41Z

/lgtm
/approve

knative-prow-robot · 2020-06-09T14:01:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, julz

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:

~~config/OWNERS~~ [dprotaso]
~~pkg/deployment/OWNERS~~ [dprotaso]
~~pkg/reconciler/OWNERS~~ [dprotaso]

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

knative-test-reporter-robot · 2020-06-09T14:10:58Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-serving-unit-tests		0/3

Failed non-flaky tests preventing automatic retry of pull-knative-serving-unit-tests:

pkg/reconciler/revision.TestNewRevisionCallsSyncHandler

julz · 2020-06-09T14:13:05Z

looks like a flake: #7852

/retest

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 4, 2020

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/API API objects and controllers labels Jun 4, 2020

knative-prow-robot reviewed Jun 4, 2020

View reviewed changes

knative-prow-robot requested review from dprotaso, markusthoemmes and vagababov June 4, 2020 11:19

taragu reviewed Jun 4, 2020

View reviewed changes

vagababov reviewed Jun 4, 2020

View reviewed changes

shashwathi reviewed Jun 4, 2020

View reviewed changes

knative-prow-robot assigned shashwathi Jun 4, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 4, 2020

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 5, 2020

vagababov reviewed Jun 5, 2020

View reviewed changes

knative-prow-robot assigned dprotaso, mattmoor and vagababov Jun 5, 2020

knative-prow-robot added lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 5, 2020

Allow setting default Queue Proxy resources in ConfigMap

0a88144

- defaults request.cpu to 25m for backwards compatibility - all other requests and limits maintain current default (i.e. nil/unspecified/inherit kubernetes default) - annotation continues to work to override

julz force-pushed the queueyoumind branch from 51d9f64 to 0a88144 Compare June 8, 2020 17:59

knative-prow-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 8, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 9, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 9, 2020

knative-prow-robot merged commit ec97b20 into knative:master Jun 9, 2020

yuzisun mentioned this pull request Jun 22, 2020

Remove default queue proxy resource limit and Add KFServing benchmarking kserve/kserve#894

Merged

julz mentioned this pull request Aug 18, 2020

Allow configurable boundary for queue proxy resource #7349

Closed

julz mentioned this pull request Aug 27, 2020

Can we set a default CPU/Memory Limit/Request for queue_proxy container #5829

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow setting Queue Proxy resources in ConfigMap #8195

Allow setting Queue Proxy resources in ConfigMap #8195

julz commented Jun 4, 2020

knative-prow-robot left a comment

taragu Jun 4, 2020

julz Jun 4, 2020 •

edited

Loading

vagababov Jun 4, 2020

julz Jun 5, 2020 •

edited

Loading

vagababov Jun 4, 2020

julz Jun 5, 2020

julz Jun 5, 2020

vagababov Jun 4, 2020

julz Jun 5, 2020 •

edited

Loading

vagababov Jun 5, 2020

julz Jun 5, 2020

vagababov Jun 5, 2020

shashwathi left a comment •

edited

Loading

julz commented Jun 5, 2020

vagababov left a comment

julz commented Jun 8, 2020

dprotaso commented Jun 9, 2020

knative-prow-robot commented Jun 9, 2020

knative-test-reporter-robot commented Jun 9, 2020

julz commented Jun 9, 2020

Allow setting Queue Proxy resources in ConfigMap #8195

Allow setting Queue Proxy resources in ConfigMap #8195

Conversation

julz commented Jun 4, 2020

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Choose a reason for hiding this comment

julz Jun 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julz Jun 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julz Jun 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shashwathi left a comment • edited Loading

Choose a reason for hiding this comment

julz commented Jun 5, 2020

vagababov left a comment

Choose a reason for hiding this comment

julz commented Jun 8, 2020

dprotaso commented Jun 9, 2020

knative-prow-robot commented Jun 9, 2020

knative-test-reporter-robot commented Jun 9, 2020

julz commented Jun 9, 2020

julz Jun 4, 2020 •

edited

Loading

julz Jun 5, 2020 •

edited

Loading

julz Jun 5, 2020 •

edited

Loading

shashwathi left a comment •

edited

Loading