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
Fix list estimator for lists that are executed as gets #112557
Conversation
/assign @wojtek-t @MikeSpreitzer @tkashem |
b751499
to
09f1ce9
Compare
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.
/lgtm
Thanks!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: marseel, MikeSpreitzer 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 |
// requests in storage layer so their width should be 1. | ||
// Example of such list requests: | ||
// /apis/certificates.k8s.io/v1/certificatesigningrequests?fieldSelector=metadata.name%3Dcsr-xxs4m | ||
// /api/v1/namespaces/test/configmaps?fieldSelector=metadata.name%3Dbig-deployment-1&limit=500&resourceVersion=0 |
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.
question: is the cost same as a regular GET on etcd side, i see fieldSelector
in the request URI. I am not very familiar with the storage logic, just wondering whether it will be as expensive as a regular LIST on the etcd side.
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 will look into this.
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.
cf #85445
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.
Yes - it's the same.
This is the part that is crucial:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go#L362-L364
Thanks for fixing this @marseel !
/triage accepted |
@marseel - I think we should consider cherrypicking this. |
I think it makes sense to backport it back to 1.23 where we introduced width for list requests. |
…557-upstream-release-1.23 Automated cherry pick of #112557: Fix list estimator for lists that are executed as gets
…557-upstream-release-1.24 Automated cherry pick of #112557: Fix list estimator for lists that are executed as gets
…557-upstream-release-1.25 Automated cherry pick of #112557: Fix list estimator for lists that are executed as gets
What type of PR is this?
/kind bug
What this PR does / why we need it:
Currently, some of the LIST requests which are handled as GET requests are going through list work estimator and are assigned incorrect width. Examples:
This PR fixes it by assigning 1 seat for such requests.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: