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

Fix APF width estimate for creating service account's token #113206

Merged
merged 1 commit into from Nov 1, 2022
Merged

Fix APF width estimate for creating service account's token #113206

merged 1 commit into from Nov 1, 2022

Conversation

marseel
Copy link
Member

@marseel marseel commented Oct 20, 2022

What type of PR is this?

/kind bug

What this PR does / why we need it:

Creating token for service account does not modify serviceaccount and does not produce any event. Because of that it shouldn't be considered as such request, example:

"HTTP" verb="POST" URI="/api/v1/namespaces/kube-system/serviceaccounts/default/token" latency="266.784506ms" userAgent="kubelet/v1.25.1" apf_pl="system" apf_fs="system-nodes" apf_iseats=1 apf_fseats=10 apf_additionalLatency="258.5ms" fl_priorityandfairness="256.272604ms" apf_execution_time="10.256749ms" resp=201

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fix cost estimation of token creation request for service account in Priority and Fairness.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 20, 2022
@k8s-ci-robot k8s-ci-robot added area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 20, 2022
@marseel
Copy link
Member Author

marseel commented Oct 20, 2022

/assign @wojtek-t @tkashem @MikeSpreitzer

@marseel marseel force-pushed the fix/fix_estimator_for_serviceaccount_tokens branch from 99c4183 to 41a9d89 Compare October 20, 2022 13:27
@marseel
Copy link
Member Author

marseel commented Oct 20, 2022

/retest

@MikeSpreitzer
Copy link
Member

(1) Why are we talking about events here?

(2) We know that the current treatment of requests is approximate. It will always be approximate, no matter how finely we tune it. Do we have evidence that the current treatment of service account token creation requests is causing a problem?

@leilajal
Copy link
Contributor

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 20, 2022
@marseel
Copy link
Member Author

marseel commented Oct 21, 2022

(1) Why are we talking about events here?

I am talking about events, because we are estimating width and additional latency based on number of watches that are watching modified resource and assumption that each modification generates event. In case of tokens for serviceaccounts, creating token for service account does not modify serviceaccount and therefore does not produce event.

(2) We know that the current treatment of requests is approximate. It will always be approximate, no matter how finely we tune it. Do we have evidence that the current treatment of service account token creation requests is causing a problem?

Yes, that's why I created PR. From log that I attached you can see that request took 10 seats and ~260 ms of additional latency - apf_fseats=10 apf_additionalLatency="258.5ms" even though request took 10.256749ms and did not produce any watch events.
I was also considering non-persistent resources mentioned in

kubernetes/pkg/quota/v1/install/registry.go

Line 46 in 9853329

{Group: authentication.GroupName, Resource: "tokenreviews"}: {},

but they cannot be watched.

tkashem
tkashem reviewed Oct 21, 2022
View reviewed changes
staging/src/k8s.io/apiserver/pkg/util/flowcontrol/request/mutating_work_estimator.go
func isRequestExemptFromEvents(requestInfo *apirequest.RequestInfo) bool {
// Creating token for service account does not produce any event,
// but still serviceaccounts can have multiple watchers.
if requestInfo.Resource == "serviceaccounts" && requestInfo.Subresource == "token" {
Copy link
Contributor

Choose a reason for hiding this comment

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

so "/api/v1/namespaces/kube-system/serviceaccounts/default/token" does not change the underlying service account object, and clients watching service accounts in this namespace would receive no watch event, right?
so, are there other requests with sub resource that does not produce watch events?

Copy link
Member Author

@marseel marseel Oct 31, 2022 •
edited

Choose a reason for hiding this comment

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

so "/api/v1/namespaces/kube-system/serviceaccounts/default/token" does not change the underlying service account object, and clients watching service accounts in this namespace would receive no watch event, right?

correct

so, are there other requests with sub resource that does not produce watch events?

I wasn't able to find any other (there are still some long running subresources like proxy/logs etc, but they are exempt from P&F). Similarly there is also tokenreviews api, but you can only create tokens and not watch this resource.

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/request/mutating_work_estimator.go Outdated Show resolved Hide resolved
@MikeSpreitzer
Copy link
Member

MikeSpreitzer commented Oct 21, 2022 •
edited

.. and this is why I hate talking about "events". There are so many kinds to choose from! Remember that Kubernetes has other kinds of "events". You will note I have been talking about "watch notifications", but everyone else calls them "events". Maybe we can say "watch events"?

So I think @tkashem is trying to suggest that the proper generality here is exemption from watch events, right?

@marseel marseel force-pushed the fix/fix_estimator_for_serviceaccount_tokens branch from 41a9d89 to 029eb79 Compare October 31, 2022 10:00
@marseel marseel force-pushed the fix/fix_estimator_for_serviceaccount_tokens branch from 029eb79 to 2f7b4ca Compare October 31, 2022 11:06
MikeSpreitzer
MikeSpreitzer approved these changes Nov 1, 2022
View reviewed changes
Copy link
Member

@MikeSpreitzer MikeSpreitzer left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 1, 2022
@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 1, 2022
@k8s-ci-robot k8s-ci-robot merged commit b7f5de1 into kubernetes:master Nov 1, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Nov 1, 2022
@MikeSpreitzer MikeSpreitzer mentioned this pull request Nov 4, 2022
Merged
@wojtek-t
Copy link
Member

For posterity - LGTM

thanks for fixing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tkashem tkashem tkashem left review comments

@wojtek-t wojtek-t wojtek-t left review comments

@MikeSpreitzer MikeSpreitzer MikeSpreitzer approved these changes

@deads2k deads2k Awaiting requested review from deads2k

Assignees

@tkashem tkashem

@wojtek-t wojtek-t

@MikeSpreitzer MikeSpreitzer

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.26
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@marseel @MikeSpreitzer @leilajal @k8s-ci-robot @wojtek-t @tkashem