Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Describe how to set qp resources #5617

Merged
merged 2 commits into from
Jul 13, 2023
Merged

Describe how to set qp resources #5617

merged 2 commits into from
Jul 13, 2023

Conversation

skonto
Copy link
Contributor

@skonto skonto commented Jun 29, 2023 •
edited
Loading

Proposed Changes

@skonto skonto requested a review from dprotaso June 29, 2023 12:01
@knative-prow knative-prow bot requested review from pmbanugo and snneji June 29, 2023 12:01
@knative-prow knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 29, 2023
@netlify
Copy link

netlify bot commented Jun 29, 2023 •
edited
Loading

✅ Deploy Preview for knative ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 925adc8
🔍 Latest deploy log https://app.netlify.com/sites/knative/deploys/64af9d248b48d50007ba7251
😎 Deploy Preview https://deploy-preview-5617--knative.netlify.app/docs/serving/services/configure-requests-limits-services
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@skonto skonto requested review from nak3 and ReToCode June 30, 2023 10:07
nak3
nak3 reviewed Jun 30, 2023
View reviewed changes
docs/serving/services/configure-requests-limits-services.md
| Cpu Request | 25m | 100m |
| Cpu Limit | 40m | 500m |
| Memory Request | 50Mi | 200Mi |
| Memory Limit | 200Mi | 500Mi |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to mention about ephemeral-storage's boundaries (I guess no boundaries?) here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no boundaries afaik.

docs/serving/services/configure-requests-limits-services.md Outdated
## Configure Queue Proxy resources

In order to set the Queue Proxy resource requests and limits you can either
set them globally in the [deployment config map](../configuration/deployment.md) or you can set them at the service level using the corresponding annotations targeting cpu, memory and ephemeral-storage resource types. The previous example becomes:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
set them globally in the [deployment config map](../configuration/deployment.md) or you can set them at the service level using the corresponding annotations targeting cpu, memory and ephemeral-storage resource types. The previous example becomes:
set them globally in the [deployment config map](../configuration/deployment.md) or you can set them at the service level using the corresponding annotations targeting cpu, memory and ephemeral-storage resource types. The latter example becomes:

Sorry my English might be wrong, but "the previous" means the global setting? If so, here should be "the latter"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I am not sure either.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe just say above

docs/serving/services/configure-requests-limits-services.md
!!! warning
The `queue.sidecar.serving.knative.dev/resource-percentage` annotation is now deprecated and will be removed in future versions.

### Additional resources
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
### Additional resources
## Additional resources

It is better to use same head?

currently:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if that belongs to that level. I moved it on down to align with the change.

@skonto
Copy link
Contributor Author

skonto commented Jul 3, 2023

@dprotaso gentle ping pls review.

@skonto
Copy link
Contributor Author

skonto commented Jul 7, 2023 •
edited
Loading

@pmbanugo @snneji @dprotaso gentle ping

@dprotaso
Copy link
Member

/lgtm
/approve
/hold if you want to address #5617 (comment) here

otherwise feel free to /unhold and do it in a separate PR

@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 11, 2023
@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2023
@knative-prow knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 13, 2023
@skonto
Copy link
Contributor Author

skonto commented Jul 13, 2023

@dprotaso thanks, comment addressed.

@skonto
Copy link
Contributor Author

skonto commented Jul 13, 2023

/unhold

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 13, 2023
ReToCode
ReToCode approved these changes Jul 13, 2023
View reviewed changes
Copy link
Member

@ReToCode ReToCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 13, 2023
@knative-prow
Copy link

knative-prow bot commented Jul 13, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, ReToCode, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow knative-prow bot merged commit a65dbda into knative:main Jul 13, 2023
17 checks passed
@skonto skonto mentioned this pull request Aug 28, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nak3 nak3 nak3 left review comments

@ReToCode ReToCode ReToCode approved these changes

@dprotaso dprotaso Awaiting requested review from dprotaso

@pmbanugo pmbanugo Awaiting requested review from pmbanugo

@snneji snneji Awaiting requested review from snneji

Assignees

@dprotaso dprotaso

@ReToCode ReToCode

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@skonto @dprotaso @nak3 @ReToCode