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.

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

Introduce apiserver_flowcontrol_current_executing_seats metric #118960

Merged

Conversation

MikeSpreitzer
Copy link
Member

This is a duplicate of
apiserver_flowcontrol_request_concurrency_in_use but with a better name. Hopefully we can later remove the copy with the inferior name.

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This PR introduces the apiserver_flowcontrol_current_executing_seats metric, as a duplicate of the apiserver_flowcontrol_request_concurrency_in_use metric --- because the latter has a confusing name and I hope to delete it later.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

The metric `apiserver_flowcontrol_current_executing_seats` has been introduced as a duplicate of `apiserver_flowcontrol_request_concurrency_in_use` because the latter has a confusing name and will be removed in a later release.

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/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. 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. 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 Jun 29, 2023
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 29, 2023
@MikeSpreitzer
Copy link
Member Author

/cc @wojtek-t
@tkashem
/uncc @yue9944882

@k8s-ci-robot k8s-ci-robot requested review from wojtek-t and removed request for yue9944882 June 29, 2023 05:49
@jiahuif
Copy link
Member

jiahuif commented Jun 29, 2023

/assign @wojtek-t
/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 Jun 29, 2023
@MikeSpreitzer
Copy link
Member Author

/retest

1 similar comment
@MikeSpreitzer
Copy link
Member Author

/retest

@wojtek-t
Copy link
Member

wojtek-t commented Jul 3, 2023

/assign @dgrisonnet - for the review from SIG instrucmentation POV

@wojtek-t
Copy link
Member

wojtek-t commented Jul 4, 2023

This LGTM, but I would like @dgrisonnet to look to for newly introduced metric.

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.

IMO the safest path forward here, is to:

  1. Mark apiserver_flowcontrol_request_concurrency_in_use as deprecated in 1.28
  2. Continue to set apiserver_flowcontrol_request_concurrency_in_use in 1.28 so that we don't break users right away and leave them one release to update
  3. We remove apiserver_flowcontrol_request_concurrency_in_use in 1.29

@dgrisonnet
Copy link
Member

dgrisonnet commented Jul 4, 2023

So the only additional thing I would want in this PR is to set DeprecatedVersion to "1.28.0" on the metric.

This is a duplicate of
`apiserver_flowcontrol_request_concurrency_in_use` but with a better
name.  Hopefully we can later remove the copy with the inferior name.

Signed-off-by: Mike Spreitzer <mspreitz@us.ibm.com>
@MikeSpreitzer
Copy link
Member Author

The force-push to 65e818d adds that DeprecatedVersion: "1.28.0" suggested above by @dgrisonnet .

Since #118882 is also in flight, perhaps we should mark StabilityLevel: compbasemetrics.BETA on the apiserver_flowcontrol_current_executing_seats since this metric is equal to apiserver_flowcontrol_request_concurrency_in_use and that one was suggested for promotion to BETA but we plan to delete it in 1.29.

@wojtek-t
Copy link
Member

wojtek-t commented Jul 6, 2023

Since #118882 is also in flight, perhaps we should mark StabilityLevel: compbasemetrics.BETA on the apiserver_flowcontrol_current_executing_seats since this metric is equal to apiserver_flowcontrol_request_concurrency_in_use and that one was suggested for promotion to BETA but we plan to delete it in 1.29.

Let's do that in a separate PR.

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MikeSpreitzer, wojtek-t

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

@wojtek-t
Copy link
Member

wojtek-t commented Jul 6, 2023

Actually - wait..

/lgtm cancel

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 6, 2023
@wojtek-t
Copy link
Member

wojtek-t commented Jul 6, 2023

No.. it's fine.

/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 6, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: a3f1628ab24f8f9d3bce0b015f9abbfadb649186

@k8s-ci-robot k8s-ci-robot merged commit fbb2f89 into kubernetes:master Jul 6, 2023
12 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.28 milestone Jul 6, 2023
@MikeSpreitzer MikeSpreitzer deleted the add-seat-occupancy-metric branch July 6, 2023 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. 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/S Denotes a PR that changes 10-29 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants