Fix, deprecate apiserver_flowcontrol_request_concurrency_limit #118959

MikeSpreitzer · 2023-06-29T05:32:05Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This PR deprecates the apiserver_flowcontrol_request_concurrency_limit metric, which rotted as we generalized request width and introduced borrowing. It is a duplicate of another metric with a better name and HELP string, apiserver_flowcontrol_nominal_limit_seats. This PR also copies that better HELP string into the apiserver_flowcontrol_request_concurrency_limit metric.

Which issue(s) this PR fixes:

Fixes #118957

Special notes for your reviewer:

Does this PR introduce a user-facing change?

The metric `apiserver_flowcontrol_request_concurrency_limit` has been deprecated and will be removed in a future release.  It is a duplicate of `apiserver_flowcontrol_nominal_limit_seats` (introduced in release 1.26) but has an outdated name and had an outdated HELP string.

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

MikeSpreitzer · 2023-06-29T05:32:44Z

@kubernetes/sig-api-machinery-bugs
/cc @JohnRusk
/cc @deads2k
/cc @wojtek-t
@tkashem

k8s-ci-robot · 2023-06-29T05:32:47Z

@MikeSpreitzer: GitHub didn't allow me to request PR reviews from the following users: JohnRusk.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

@kubernetes/sig-api-machinery-bugs
/cc @JohnRusk
/cc @deads2k
/cc @wojtek-t
@tkashem

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

MikeSpreitzer · 2023-06-29T05:33:08Z

/uncc @yue9944882

JohnRusk

Nice

wojtek-t · 2023-06-29T11:37:51Z

+1 for removing this metric, but I'm not sure if we don't officially deprecate metrics first.

/assign @dgrisonnet
Can you please double check that we're doing the right thing here?

jiahuif · 2023-06-29T16:34:50Z

/triage accepted

MikeSpreitzer · 2023-06-29T18:42:51Z

I do not see a formal stability level for deprecation.

andrewsykim

/hold

(on-going discussion in #118882)

MikeSpreitzer · 2023-07-06T04:19:04Z

@andrewsykim : is there a reason to hold this one? The equivalent metric apiserver_flowcontrol_nominal_limit_seats is already available.

MikeSpreitzer · 2023-07-10T20:19:48Z

The force-push to 5205cba18b4 is a rebase plus the changes suggested in review above.

MikeSpreitzer · 2023-07-11T00:40:22Z

/retest

andrewsykim

/lgtm

k8s-ci-robot · 2023-07-11T03:39:36Z

LGTM label has been added.

Git tree hash: 9180ff9cc8b081aa827422e24cea92510c2428d2

k8s-triage-robot · 2023-07-11T06:15:47Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

wojtek-t · 2023-07-11T07:16:02Z

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics/metrics.go

+			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",
+			// Remove this metric once all suppported releases have the equal current_executing_seats metric


Similarly here

The equal metric current_executing_seats is being introduced in release 1.28, and that will be the oldest supported release when 1.31 is the latest release, so I am setting the deprecated version to 1.31 here.

If we're following what we did for nominal_seats_limit, this should actually be 1.32 (4 releases from 1.28) since nominal_seats_limit was introduced in 1.26.

We can fix in a follow-up though since this is a deprecation in a future version. I would be in favor of doing the deprecation on the same versions too, so maybe move both of them up to 1.32 or backport current_executing_seats to older versions and keep them both at 1.30?

pacoxu · 2023-07-11T08:43:35Z

/hold
as there are some comments to be addressed. #118959 (comment)

Because it is redundant and has a bad name and its HELP string was outdated. Also note intended retention period for request_concurrency_in_use. Signed-off-by: Mike Spreitzer <mspreitz@us.ibm.com>

wojtek-t · 2023-07-13T07:28:45Z

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics/metrics.go

+			Name:      "request_concurrency_limit",
+			Help:      "Nominal number of execution seats configured for each priority level",
+			// Remove this metric once all suppported releases have the equal nominal_limit_seats metric
+			DeprecatedVersion: "1.30.0",


Why do you have 1.30 here (with 1.31 for the metric below)?

That is explained in my other comments in this PR.

dgrisonnet · 2023-07-17T17:26:06Z

Since both metrics have replacements that just graduated to Beta with #119110, I would actually be fine with just straight up removing them without requiring a deprecation period as per https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/3498-extending-stability#alpha-metrics.

I used to ask for a deprecation period on Alpha metrics because we didn't have a Beta stability class in place which could have broken the expectations of the users, but now that we have finer grained classes starting from 1.28, renaming an Alpha metric to be eligible for Beta criterias is normal and expected so it shouldn't require deprecation.

MikeSpreitzer · 2023-07-17T18:52:54Z

@dgrisonnet: the alpha metrics have established usage, despite being formally alpha. So we decided that for the sake of users of the old alpha metrics we will keep them available by default until the replacements are available in all supported releases.

dgrisonnet · 2023-07-17T19:04:44Z

That approach is fine by me, I just wanted to mention that if you wanted you could also skip the deprecation period.

/lgtm
/approve

k8s-ci-robot · 2023-07-17T19:04:54Z

LGTM label has been added.

Git tree hash: 69171ef756123295d5c4349bf6e2bd88370c2e7b

k8s-ci-robot · 2023-07-17T19:05:13Z

[APPROVALNOTIFIER] This PR is APPROVED

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

~~staging/src/k8s.io/apiserver/pkg/util/flowcontrol/OWNERS~~ [MikeSpreitzer]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

andrewsykim · 2023-07-17T19:13:23Z

/hold cancel

(#118959 (comment) now addressed)

k8s-ci-robot requested review from deads2k and yue9944882 June 29, 2023 05:32

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jun 29, 2023

k8s-ci-robot requested a review from wojtek-t June 29, 2023 05:32

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. area/apiserver approved Indicates a PR has been approved by an approver from all required OWNERS files. 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 removed the request for review from yue9944882 June 29, 2023 05:33

JohnRusk approved these changes Jun 29, 2023

View reviewed changes

k8s-ci-robot assigned dgrisonnet Jun 29, 2023

wojtek-t self-assigned this Jun 29, 2023

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

andrewsykim reviewed Jul 5, 2023

View reviewed changes

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

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 10, 2023

MikeSpreitzer force-pushed the fix-118957b branch from 9a834aa to 5205cba Compare July 10, 2023 20:19

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 10, 2023

andrewsykim reviewed Jul 11, 2023

View reviewed changes

k8s-ci-robot assigned andrewsykim Jul 11, 2023

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

wojtek-t reviewed Jul 11, 2023

View reviewed changes

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 11, 2023

Fix, deprecate apiserver_flowcontrol_request_concurrency_limit

75186b1

Because it is redundant and has a bad name and its HELP string was outdated. Also note intended retention period for request_concurrency_in_use. Signed-off-by: Mike Spreitzer <mspreitz@us.ibm.com>

MikeSpreitzer force-pushed the fix-118957b branch from 5205cba to 75186b1 Compare July 13, 2023 05:51

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 13, 2023

k8s-ci-robot requested review from andrewsykim, dgrisonnet and wojtek-t July 13, 2023 05:51

wojtek-t reviewed Jul 13, 2023

View reviewed changes

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

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 17, 2023

k8s-ci-robot merged commit af33d7a into kubernetes:master Jul 17, 2023
12 checks passed

k8s-ci-robot added this to the v1.28 milestone Jul 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix, deprecate apiserver_flowcontrol_request_concurrency_limit #118959

Fix, deprecate apiserver_flowcontrol_request_concurrency_limit #118959

MikeSpreitzer commented Jun 29, 2023 •

edited

MikeSpreitzer commented Jun 29, 2023

k8s-ci-robot commented Jun 29, 2023

MikeSpreitzer commented Jun 29, 2023

JohnRusk left a comment

wojtek-t commented Jun 29, 2023

jiahuif commented Jun 29, 2023

MikeSpreitzer commented Jun 29, 2023

andrewsykim left a comment

MikeSpreitzer commented Jul 6, 2023

MikeSpreitzer commented Jul 10, 2023

MikeSpreitzer commented Jul 11, 2023

andrewsykim left a comment

k8s-ci-robot commented Jul 11, 2023

k8s-triage-robot commented Jul 11, 2023

wojtek-t Jul 11, 2023

MikeSpreitzer Jul 13, 2023 •

edited

andrewsykim Jul 17, 2023

andrewsykim Jul 17, 2023 •

edited

pacoxu commented Jul 11, 2023 •

edited

wojtek-t Jul 13, 2023

MikeSpreitzer Jul 13, 2023

dgrisonnet commented Jul 17, 2023

MikeSpreitzer commented Jul 17, 2023

dgrisonnet commented Jul 17, 2023

k8s-ci-robot commented Jul 17, 2023

k8s-ci-robot commented Jul 17, 2023

andrewsykim commented Jul 17, 2023

Fix, deprecate apiserver_flowcontrol_request_concurrency_limit #118959

Fix, deprecate apiserver_flowcontrol_request_concurrency_limit #118959

Conversation

MikeSpreitzer commented Jun 29, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR 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.:

MikeSpreitzer commented Jun 29, 2023

k8s-ci-robot commented Jun 29, 2023

MikeSpreitzer commented Jun 29, 2023

JohnRusk left a comment

Choose a reason for hiding this comment

wojtek-t commented Jun 29, 2023

jiahuif commented Jun 29, 2023

MikeSpreitzer commented Jun 29, 2023

andrewsykim left a comment

Choose a reason for hiding this comment

MikeSpreitzer commented Jul 6, 2023

MikeSpreitzer commented Jul 10, 2023

MikeSpreitzer commented Jul 11, 2023

andrewsykim left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 11, 2023

k8s-triage-robot commented Jul 11, 2023

wojtek-t Jul 11, 2023

Choose a reason for hiding this comment

MikeSpreitzer Jul 13, 2023 • edited

Choose a reason for hiding this comment

andrewsykim Jul 17, 2023

Choose a reason for hiding this comment

andrewsykim Jul 17, 2023 • edited

Choose a reason for hiding this comment

pacoxu commented Jul 11, 2023 • edited

wojtek-t Jul 13, 2023

Choose a reason for hiding this comment

MikeSpreitzer Jul 13, 2023

Choose a reason for hiding this comment

dgrisonnet commented Jul 17, 2023

MikeSpreitzer commented Jul 17, 2023

dgrisonnet commented Jul 17, 2023

k8s-ci-robot commented Jul 17, 2023

k8s-ci-robot commented Jul 17, 2023

andrewsykim commented Jul 17, 2023

MikeSpreitzer commented Jun 29, 2023 •

edited

MikeSpreitzer Jul 13, 2023 •

edited

andrewsykim Jul 17, 2023 •

edited

pacoxu commented Jul 11, 2023 •

edited