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

Define metrics for API Priority and Fairness borrowing #113483

Merged
merged 1 commit into from Nov 4, 2022
Merged

Define metrics for API Priority and Fairness borrowing #113483

merged 1 commit into from Nov 4, 2022

Conversation

MikeSpreitzer
Copy link
Member

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR defines the metrics introduced for borrowing between priority levels in API Priority and Fairness. The KEP changes that introduce these are kubernetes/enhancements#3391 and kubernetes/enhancements#3479 .

This PR also removes one metric that is poorly named (it speaks of "requests" but those have been different from "seats" for a while now) and meant two things that were equal before the borrowing feature but are now different: apiserver_flowcontrol_request_concurrency_limit can no longer be both the configured nominal concurrency limit and the dynamically derived current concurrency limit.

This PR also connects them to as much of the implementation as currently is merged. The rest waits on the API changes in #112830 .

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

[KEP] https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/1040-priority-and-fairness

@kubernetes/sig-api-machinery-misc
/cc @tkashem
/cc @deads2k
/cc @wojtek-t
@lavalamp
@cyang49

@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Oct 31, 2022
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. 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. 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 approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 31, 2022
@MikeSpreitzer MikeSpreitzer force-pushed the add-borrowing-metrics branch 4 times, most recently from 57d5643 to df10d43 Compare November 1, 2022 02:14
@MikeSpreitzer MikeSpreitzer mentioned this pull request Nov 1, 2022
Merged
@MikeSpreitzer MikeSpreitzer force-pushed the add-borrowing-metrics branch 2 times, most recently from e41c460 to ea8333e Compare November 1, 2022 03:31
wojtek-t
wojtek-t reviewed Nov 2, 2022
View reviewed changes
Copy link
Member

@wojtek-t wojtek-t left a comment

Choose a reason for hiding this comment

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

Couple smaller comments.

Also
/assign @dgrisonnet
For SIG-instrumentation approval for newly added metrics.

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/apf_controller.go Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics/metrics.go Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics/metrics.go Outdated
&compbasemetrics.GaugeOpts{
Namespace: namespace,
Subsystem: subsystem,
Name: "demand_envelope_seats",
Copy link
Member

Choose a reason for hiding this comment

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

Do I remember that envelope is simply adv+stddev?
If so - do we really need this metric?

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 KEP has this metric listed too, so I followed that.

Yes, currently it is redundant with avg + stddev.

Copy link
Member

Choose a reason for hiding this comment

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

In this case, I suggest removing this metric (we should update the KEP in the background too)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in the latest revision.

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics/metrics.go Show resolved Hide resolved
wojtek-t
wojtek-t reviewed Nov 3, 2022
View reviewed changes
Copy link
Member

@wojtek-t wojtek-t left a comment

Choose a reason for hiding this comment

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

This LGTM (modulo one comment), but we should get a look from SIG-instrumentation folks.
I will ping them on slack.

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/apf_controller.go Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics/metrics.go Show resolved Hide resolved
staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics/metrics.go Outdated
&compbasemetrics.GaugeOpts{
Namespace: namespace,
Subsystem: subsystem,
Name: "demand_envelope_seats",
Copy link
Member

Choose a reason for hiding this comment

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

In this case, I suggest removing this metric (we should update the KEP in the background too)

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics/metrics.go Show resolved Hide resolved
dgrisonnet
dgrisonnet reviewed Nov 3, 2022
View reviewed changes
staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics/metrics.go Outdated
&compbasemetrics.GaugeOpts{
Namespace: namespace,
Subsystem: subsystem,
Name: "demand_seats_hight_watermark",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Name: "demand_seats_hight_watermark",
Name: "demand_seats_high_watermark",

Copy link
Member

Choose a reason for hiding this comment

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

Isn't this just a typo of hight that should be high?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, thanks!

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics/metrics.go
// apiserverCurrentConcurrencyLimits.WithLabelValues(priorityLevel).Set(currentCL)
}

func SetFairFrac(fairFrac float64) {
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem to be called anywhere, are you also planning to wait for the API to be available to use it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly. You see it get used in #113485 .

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/metrics/metrics.go
Comment on lines +421 to +423
apiserverFairFracs = compbasemetrics.NewGauge(
&compbasemetrics.GaugeOpts{
Namespace: namespace,
Subsystem: subsystem,
Name: "seat_fair_frac",
Help: "Fair fraction of server's concurrency to allocate to each priority level that can use it",
StabilityLevel: compbasemetrics.ALPHA,
})
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure I understand this metric, shouldn't it be defined per priority level?

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 interesting thing about this value is that it is the same for every priority level. It is the share that each gets, except those that are clipped by bounds.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, I wasn't sure that was the case so I wanted to make sure

@fedebongio
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 Nov 3, 2022
@fedebongio
Copy link
Contributor

/cc @logicalhan

@MikeSpreitzer
Copy link
Member Author

/retest

logicalhan
logicalhan reviewed Nov 3, 2022
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.

unit test failure looks legit.

@MikeSpreitzer
Copy link
Member Author

@logicalhan : did you mean the integration test failure? I do not see how this change could cause that failure. This change only adds some instrumentation, should not cause any behavior change apart from adding some metrics and logging.

@MikeSpreitzer
Copy link
Member Author

/retest

@logicalhan
Copy link
Member

@logicalhan : did you mean the integration test failure? I do not see how this change could cause that failure. This change only adds some instrumentation, should not cause any behavior change apart from adding some metrics and logging.

I actually meant https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/113483/pull-kubernetes-unit/1588224012842962944 but it seems to have been fixed since.

@wojtek-t
Copy link
Member

wojtek-t commented Nov 4, 2022

/lgtm
/approve

Thanks!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2022
@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

@k8s-ci-robot k8s-ci-robot merged commit 7ae0396 into kubernetes:master Nov 4, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Nov 4, 2022
@MikeSpreitzer
Copy link
Member Author

Actually, regarding #113483 (comment) , that is a flake. See #113206 about it.

@MikeSpreitzer MikeSpreitzer deleted the add-borrowing-metrics branch November 7, 2022 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@logicalhan logicalhan logicalhan left review comments

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

@dgrisonnet dgrisonnet dgrisonnet left review comments

@deads2k deads2k Awaiting requested review from deads2k

@tkashem tkashem Awaiting requested review from tkashem

Assignees

@wojtek-t wojtek-t

@dgrisonnet dgrisonnet

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/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-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. 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.26
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@MikeSpreitzer @fedebongio @logicalhan @wojtek-t @k8s-ci-robot @dgrisonnet