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, deprecate apiserver_flowcontrol_request_concurrency_limit #118959

Merged
merged 1 commit into from Jul 17, 2023
Merged

Fix, deprecate apiserver_flowcontrol_request_concurrency_limit #118959

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 13 additions & 10 deletions staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics/metrics.go
View file
Open in desktop
Expand Up @@ -223,11 +223,13 @@ var (
)
apiserverRequestConcurrencyLimit = compbasemetrics.NewGaugeVec(
&compbasemetrics.GaugeOpts{
Namespace: namespace,
Subsystem: subsystem,
Name: "request_concurrency_limit",
Help: "Shared concurrency limit in the API Priority and Fairness subsystem",
StabilityLevel: compbasemetrics.ALPHA,
Namespace: namespace,
Subsystem: subsystem,
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",
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

That is explained in my other comments in this PR.

StabilityLevel: compbasemetrics.ALPHA,
},
[]string{priorityLevel},
)
Expand All @@ -253,11 +255,12 @@ var (
)
apiserverRequestConcurrencyInUse = compbasemetrics.NewGaugeVec(
&compbasemetrics.GaugeOpts{
Namespace: namespace,
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",
DeprecatedVersion: "1.28.0",
Namespace: namespace,
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
Copy link
Member

Choose a reason for hiding this comment

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

Similarly here

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

DeprecatedVersion: "1.31.0",
StabilityLevel: compbasemetrics.ALPHA,
},
[]string{priorityLevel, flowSchema},
Expand Down