Allow to set QP resources per service #14038

skonto · 2023-05-31T08:46:46Z

Part of the work for #13861.

Proposed Changes

Adds an annotation per resource type. Here is a sample ksvc:

apiVersion: serving.knative.dev/v1
kind: Service
spec:
  template:
    metadata:
      labels:
        app: helloworld-go
      annotations:
        queue.sidecar.serving.knative.dev/cpu-resource-request: "1"
        queue.sidecar.serving.knative.dev/cpu-resource-limit: "2"
        queue.sidecar.serving.knative.dev/memory-resource-request: "1Gi"
        queue.sidecar.serving.knative.dev/memory-resource-limit: "2Gi"
        queue.sidecar.serving.knative.dev/ephemeral-storage-resource-request: "400Mi"
        queue.sidecar.serving.knative.dev/ephemeral-storage-resource-limit: "450Mi"
    spec:
 ...

The values passed by the user have higher priority compared to the percentage annotation.
Tests added.

Release Note

Queue proxy resources can be configured via annotations at the service level.  The resource percentage annotation is now deprecated.

codecov · 2023-05-31T08:54:37Z

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.03 ⚠️

Comparison is base (c12c917) 86.22% compared to head (278a5e4) 86.19%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #14038      +/-   ##
==========================================
- Coverage   86.22%   86.19%   -0.03%     
==========================================
  Files         199      199              
  Lines       14767    14795      +28     
==========================================
+ Hits        12733    12753      +20     
- Misses       1732     1737       +5     
- Partials      302      305       +3

Impacted Files	Coverage Δ
pkg/apis/serving/v1/revision_validation.go	`91.56% <100.00%> (+0.77%)`	⬆️
pkg/reconciler/revision/resources/queue.go	`98.36% <100.00%> (+0.12%)`	⬆️

... 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 · 2023-06-06T08:38:43Z

/assign @dprotaso

dprotaso

Should we deprecate the % knob?

dprotaso · 2023-06-07T20:13:28Z

pkg/apis/serving/register.go

@@ -162,6 +180,30 @@ var (
 		QueueSidecarResourcePercentageAnnotationKey,
 		"queue.sidecar." + GroupName + "/resourcePercentage",
 	}
+	QueueSidecarCPUResourceRequestAnnotation = kmap.KeyPriority{
+		QueueSidecarCPUResourceRequestAnnotationKey,
+		"queue.sidecar." + GroupName + "/CPUResourceRequest",


don't need the alternate casing for new annotations

@dprotaso You mean s/CPUResourceRequest /cpuesourcerequest? Wouldnt that be a bit misaligned compared to the resourcePercentage?

resourcePercentage is present here because it was the old casing that we supported. We didn't want to break old deployments

For new keys we should just use kebab-casing only

Ok then I will fix.

pkg/apis/serving/register.go

dprotaso · 2023-06-07T20:15:58Z

I was mixed on whether this should be exposed to users but ultimately if they can set their own request/limits on the user container then it's no different from setting it on the sidecar (in terms of abusing it)

ReToCode · 2023-06-08T05:33:59Z

LGTM.

skonto · 2023-06-12T09:50:37Z

Should we deprecate the % knob?

Yeah I think it makes sense.

skonto · 2023-06-13T18:56:46Z

Hi @dprotaso gentle ping. I deprecated the resource percentage (should be done at docs side too), now it prints:

Warning: Queue proxy resource percentage annotation is deprecated. Please use the available annotations to explicitly set resource values per service: 
service.serving.knative.dev/helloworld-go configured

Also replied to the comments above.

skonto · 2023-06-20T10:51:40Z

@dprotaso gentle ping, I removed the redundant key.

dprotaso · 2023-06-20T13:34:53Z

pkg/apis/serving/v1/revision_validation.go

@@ -188,14 +188,15 @@ func validateQueueSidecarAnnotation(m map[string]string) *apis.FieldError {
 	if !ok {
 		return nil
 	}
+	errs := apis.ErrGeneric("Queue proxy resource percentage annotation is deprecated. Please use the available annotations to explicitly set resource values per service").ViaKey(k).At(apis.WarningLevel)


We should validate the new annotations here by attempting to parse them. Then it will fail when we create/update/apply vs. silently dropping it later on when creating the deployment

Ok will add it.

skonto · 2023-06-21T17:56:25Z

@dprotaso thanks for reviewing, I added the validation part:

Error from server (BadRequest): error when creating "qp_resource_anno_service.yaml": admission webhook "validation.webhook.serving.knative.dev" denied the request: validation failed: invalid value: 450Mxx: spec.template.metadata.annotations.[queue.sidecar.serving.knative.dev/ephemeral-storage-resource-limit]

dprotaso · 2023-06-23T13:25:49Z

pkg/apis/serving/v1/revision_validation.go

+	validate := func(k, v string) *apis.FieldError {
+		if _, err := resource.ParseQuantity(v); err != nil {
+			return errs.Also(apis.ErrInvalidValue(v, apis.CurrentField).ViaKey(k))
+		}
+		return nil
 	}


Can we just move this into the loop below? so we're not creating a closure on every validate invocation?

dprotaso · 2023-06-23T13:27:12Z

pkg/apis/serving/v1/revision_validation.go

+		if ok {
+			errs = errs.Also(validate(k, v))
+		}


Suggested change

if ok {

errs = errs.Also(validate(k, v))

}

if !ok {

continue

}

... // add code above here

skonto · 2023-06-26T07:31:50Z

@dprotaso gentle ping.

skonto · 2023-06-27T07:29:17Z

@dprotaso hi should we merge this? I will add the docs.

dprotaso · 2023-06-27T14:22:51Z

/lgtm
/approve

knative-prow · 2023-06-27T14:22:57Z

[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

skonto · 2023-06-28T07:02:00Z

/retest

* allow to set qp resources per service * add ephemeral storage * deprecate resource percentage * remove the old casing * add proper validation * fix loop

allow to set qp resources per service

c4cc59c

skonto requested a review from dprotaso May 31, 2023 08:46

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/API API objects and controllers labels May 31, 2023

knative-prow bot requested review from jsanin-vmw and ReToCode May 31, 2023 08:46

skonto changed the title ~~Allow to set QP resources per service~~ [wip] Allow to set QP resources per service May 31, 2023

knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 31, 2023

add ephemeral storage

1d1c3ef

skonto changed the title ~~[wip] Allow to set QP resources per service~~ Allow to set QP resources per service Jun 1, 2023

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 1, 2023

knative-prow bot assigned dprotaso Jun 6, 2023

dprotaso reviewed Jun 7, 2023

View reviewed changes

deprecate resource percentage

cdd5bbd

remove the old casing

192ca46

dprotaso reviewed Jun 20, 2023

View reviewed changes

add proper validation

e722d50

dprotaso reviewed Jun 23, 2023

View reviewed changes

fix loop

278a5e4

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jun 27, 2023

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 27, 2023

knative-prow bot merged commit 63fa389 into knative:main Jun 28, 2023
62 checks passed

skonto mentioned this pull request Jun 29, 2023

Describe how to set qp resources knative/docs#5617

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to set QP resources per service #14038

Allow to set QP resources per service #14038

skonto commented May 31, 2023 •

edited

codecov bot commented May 31, 2023 •

edited

skonto commented Jun 6, 2023

dprotaso left a comment

dprotaso Jun 7, 2023

skonto Jun 12, 2023 •

edited

dprotaso Jun 16, 2023 •

edited

skonto Jun 19, 2023

dprotaso commented Jun 7, 2023 •

edited

ReToCode commented Jun 8, 2023

skonto commented Jun 12, 2023

skonto commented Jun 13, 2023 •

edited

skonto commented Jun 20, 2023

dprotaso Jun 20, 2023 •

edited

skonto Jun 20, 2023

skonto commented Jun 21, 2023 •

edited

dprotaso Jun 23, 2023

dprotaso Jun 23, 2023

skonto commented Jun 26, 2023

skonto commented Jun 27, 2023 •

edited

dprotaso commented Jun 27, 2023

knative-prow bot commented Jun 27, 2023

skonto commented Jun 28, 2023

Allow to set QP resources per service #14038

Allow to set QP resources per service #14038

Conversation

skonto commented May 31, 2023 • edited

Proposed Changes

codecov bot commented May 31, 2023 • edited

Codecov Report

skonto commented Jun 6, 2023

dprotaso left a comment

Choose a reason for hiding this comment

dprotaso Jun 7, 2023

Choose a reason for hiding this comment

skonto Jun 12, 2023 • edited

Choose a reason for hiding this comment

dprotaso Jun 16, 2023 • edited

Choose a reason for hiding this comment

skonto Jun 19, 2023

Choose a reason for hiding this comment

dprotaso commented Jun 7, 2023 • edited

ReToCode commented Jun 8, 2023

skonto commented Jun 12, 2023

skonto commented Jun 13, 2023 • edited

skonto commented Jun 20, 2023

dprotaso Jun 20, 2023 • edited

Choose a reason for hiding this comment

skonto Jun 20, 2023

Choose a reason for hiding this comment

skonto commented Jun 21, 2023 • edited

dprotaso Jun 23, 2023

Choose a reason for hiding this comment

dprotaso Jun 23, 2023

Choose a reason for hiding this comment

skonto commented Jun 26, 2023

skonto commented Jun 27, 2023 • edited

dprotaso commented Jun 27, 2023

knative-prow bot commented Jun 27, 2023

skonto commented Jun 28, 2023

skonto commented May 31, 2023 •

edited

codecov bot commented May 31, 2023 •

edited

skonto Jun 12, 2023 •

edited

dprotaso Jun 16, 2023 •

edited

dprotaso commented Jun 7, 2023 •

edited

skonto commented Jun 13, 2023 •

edited

dprotaso Jun 20, 2023 •

edited

skonto commented Jun 21, 2023 •

edited

skonto commented Jun 27, 2023 •

edited