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

Promote kube-apiserver flowcontrol metrics to Beta #119110

Merged
merged 2 commits into from
Jul 17, 2023
Merged

Promote kube-apiserver flowcontrol metrics to Beta #119110

merged 2 commits into from
Jul 17, 2023

Conversation

andrewsykim
Copy link
Member

@andrewsykim andrewsykim commented Jul 5, 2023 •
edited

What type of PR is this?

/kind feature

What this PR does / why we need it:

Promote the following apiserver flowcontrol metrics to Beta:

apiserver_flowcontrol_request_wait_duration_seconds
apiserver_flowcontrol_current_executing_seats
apiserver_flowcontrol_nominal_limit_seats
apiserver_flowcontrol_rejected_requests_total
apiserver_flowcontrol_dispatched_requests_total
apiserver_flowcontrol_current_inqueue_requests
apiserver_flowcontrol_current_executing_requests

These metrics have been around for a while and are very valuable in monitoring flowcontrol state in apiserver.

Which issue(s) this PR fixes:

Fixes #118882

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Promote the following apiserver flowcontrol metrics to Beta:

apiserver_flowcontrol_request_wait_duration_seconds 
apiserver_flowcontrol_current_executing_seats
apiserver_flowcontrol_nominal_limit_seats
apiserver_flowcontrol_rejected_requests_total
apiserver_flowcontrol_dispatched_requests_total
apiserver_flowcontrol_current_inqueue_requests
apiserver_flowcontrol_current_executing_requests

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/S Denotes a PR that changes 10-29 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 5, 2023
@k8s-ci-robot k8s-ci-robot added area/apiserver do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. 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. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 5, 2023
@andrewsykim
Copy link
Member Author

/hold

On-going discussion in #118882

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 5, 2023
@andrewsykim andrewsykim changed the title promote the some APF metrics to beta: promote some APF metrics to beta: Jul 6, 2023
@andrewsykim andrewsykim changed the title promote some APF metrics to beta: Promote kube-apiserver flowcontrol metrics to Beta Jul 6, 2023
@jiahuif
Copy link
Member

jiahuif commented Jul 6, 2023

/triage accepted
(for being under discussion

@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 Jul 6, 2023
@MikeSpreitzer
Copy link
Member

@MikeSpreitzer

@MikeSpreitzer
Copy link
Member

#118882 (comment)

@MikeSpreitzer
Copy link
Member

I have updated #118959 to deprecate rather than remove apiserver_flowcontrol_request_concurrency_limit, and fix its HELP string.

MikeSpreitzer
MikeSpreitzer reviewed Jul 6, 2023
View reviewed changes
staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics/metrics.go Outdated
@@ -227,7 +227,7 @@ var (
Subsystem: subsystem,
Name: "request_concurrency_limit",
Help: "Shared concurrency limit in the API Priority and Fairness subsystem",
StabilityLevel: compbasemetrics.ALPHA,
StabilityLevel: compbasemetrics.BETA,
Copy link
Member

Choose a reason for hiding this comment

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

Since we are deprecating this metric and plan to delete it, I think that we should promote the equal metric apiserver_flowcontrol_nominal_limit_seats to BETA instead of this one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Kept this as alpha and updated nominal_limit_seats to BETA

MikeSpreitzer
MikeSpreitzer reviewed Jul 6, 2023
View reviewed changes
staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics/metrics.go
@@ -247,7 +247,7 @@ var (
Subsystem: subsystem,
Name: "request_concurrency_in_use",
Help: "Concurrency (number of seats) occupied by the currently executing (initial stage for a WATCH, any stage otherwise) requests in the API Priority and Fairness subsystem",
StabilityLevel: compbasemetrics.ALPHA,
StabilityLevel: compbasemetrics.BETA,
Copy link
Member

Choose a reason for hiding this comment

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

Since we plan to deprecate and remove this metric, and the equal metric apiserver_flowcontrol_current_executing_seats is available (since #118960), I think that the latter should become BETA rather than the former.

Copy link
Member

Choose a reason for hiding this comment

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

This has been resolved (and GitHub moved the comment to the resolution!)

@MikeSpreitzer MikeSpreitzer mentioned this pull request Jul 6, 2023
Closed
andrewsykim
andrewsykim commented Jul 13, 2023
View reviewed changes
Copy link
Member Author

@andrewsykim andrewsykim left a comment

Choose a reason for hiding this comment

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

@wojtek-t @MikeSpreitzer I updated the PR to leave request_concurrency_in_use as ALPHA and promote nominal_limit_seats to BETA. PTAL

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics/metrics.go Outdated
@@ -227,7 +227,7 @@ var (
Subsystem: subsystem,
Name: "request_concurrency_limit",
Help: "Shared concurrency limit in the API Priority and Fairness subsystem",
StabilityLevel: compbasemetrics.ALPHA,
StabilityLevel: compbasemetrics.BETA,
Copy link
Member Author

Choose a reason for hiding this comment

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

Kept this as alpha and updated nominal_limit_seats to BETA

@andrewsykim
promote the following APF metrics to beta:
0bb419b 
apiserver_flowcontrol_request_wait_duration_seconds
apiserver_flowcontrol_request_concurrency_in_use
apiserver_flowcontrol_request_concurrency_limit
apiserver_flowcontrol_rejected_requests_total
apiserver_flowcontrol_dispatched_requests_total
apiserver_flowcontrol_current_inqueue_requests
apiserver_flowcontrol_current_executing_requests

Signed-off-by: Andrew Sy Kim <[email protected]>
@andrewsykim
Copy link
Member Author

/assign @wojtek-t @MikeSpreitzer

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/stable-metrics Issues or PRs involving stable metrics sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 13, 2023
@k8s-triage-robot
Copy link

This PR may require stable metrics review.

Stable metrics are guaranteed to not change. Please review the documentation for the requirements and lifecycle of stable metrics and ensure that your metrics meet these guidelines.

@andrewsykim
Copy link
Member Author

/assign @dgrisonnet

/retest

MikeSpreitzer
MikeSpreitzer approved these changes Jul 14, 2023
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 Jul 14, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 63d5fa0aa328eb6080a5557ce4a65db3b9a8c783

@MikeSpreitzer
Copy link
Member

@andrewsykim : I think you can remove the hold here, we have agreed on what to do.

andrewsykim
andrewsykim commented Jul 16, 2023
View reviewed changes
Copy link
Member Author

@andrewsykim andrewsykim left a comment

Choose a reason for hiding this comment

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

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 16, 2023
@andrewsykim
Copy link
Member Author

/assign @logicalhan

dgrisonnet
dgrisonnet approved these changes Jul 17, 2023
View reviewed changes
Copy link
Member

@dgrisonnet dgrisonnet 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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 17, 2023
logicalhan
logicalhan approved these changes Jul 17, 2023
View reviewed changes
Copy link
Member

@logicalhan logicalhan 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

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, dgrisonnet, logicalhan, 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

@MikeSpreitzer
Copy link
Member

/retest

@k8s-ci-robot k8s-ci-robot merged commit 4f60a8d into kubernetes:master Jul 17, 2023
13 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.28 milestone Jul 17, 2023
@dgrisonnet dgrisonnet mentioned this pull request Jul 17, 2023
Merged
@wojtek-t
Copy link
Member

LGTM

@jaskaransarkaria jaskaransarkaria mentioned this pull request Jun 5, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@logicalhan logicalhan logicalhan approved these changes

@MikeSpreitzer MikeSpreitzer MikeSpreitzer approved these changes

@dgrisonnet dgrisonnet dgrisonnet approved these changes

@deads2k deads2k Awaiting requested review from deads2k

Assignees

@logicalhan logicalhan

@wojtek-t wojtek-t

@MikeSpreitzer MikeSpreitzer

@dgrisonnet dgrisonnet

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/stable-metrics Issues or PRs involving stable metrics cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. 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. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.28
Development

Successfully merging this pull request may close these issues.

Promoting some flowcontrol metrics to Beta
8 participants
@andrewsykim @jiahuif @MikeSpreitzer @k8s-triage-robot @k8s-ci-robot @wojtek-t @logicalhan @dgrisonnet