feature(scheduler): implement plugin_evaluation_total metric by sanposhiho · Pull Request #115082 · kubernetes/kubernetes

sanposhiho · 2023-01-15T06:43:04Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

New ~~"schedule_attempts"~~ "plugin_evaluation_total" metric is added to the scheduler. This metric counts how many times the specific plugin affects the scheduling result while taking Skip status into consideration.

It's now only supporting the Filter status. After #114827, we can support "Score" as well.

Which issue(s) this PR fixes:

Part of #110643

Special notes for your reviewer:

Not sure if "schedule_attempts" is the most suitable name. Tell me if any other good idea on naming.

EDIT: renamed to plugin_evaluation_total as suggested in #115082 (comment).

Does this PR introduce a user-facing change?

New "plugin_evaluation_total" is added to the scheduler. 
This metric counts how many times the specific plugin affects the scheduling result. The metric doesn't get incremented when the plugin has nothing to do with an incoming Pod.

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

sanposhiho · 2023-01-26T14:09:30Z

/cc @alculquicondor

alculquicondor · 2023-01-26T15:47:25Z

counters should always end in _total https://prometheus.io/docs/practices/naming/#metric-names

plugin_evaluation_total? This is a low level metric, so I think it's fair to have the word plugin in it.

Renamed as suggested, thanks.

alculquicondor · 2023-01-26T15:48:31Z

@kerthcet or @chendave do you have cycles to look at this?

kerthcet · 2023-01-27T13:23:09Z

I'll be back tomorrow.
/assign

sanposhiho · 2023-02-04T07:22:46Z

@alculquicondor @kerthcet

Fixed based on the suggestion, please take a look. 🙏

kerthcet · 2023-02-06T13:43:01Z

This metric counts how many times the specific plugin affects the scheduling result while taking Skip status into consideration. In other words, the metric doesn't get incremented when the plugin has nothing to do with an incoming Pod.

By evaluating the plug being called times, what can we get from this? I mean in what scenario we need this metrics. Not opposing it, something I haven't figured out.

kerthcet · 2023-02-06T13:46:09Z

And sorry for let this sitting a long time. Busy with works. ☹️

alculquicondor · 2023-02-06T15:18:10Z

By evaluating the plug being called times, what can we get from this?

This is a common question in PRR for any new features: how can an admin know if a feature is being used? This PR moves the needle in that direction.

alculquicondor · 2023-02-06T17:50:19Z

 		if state.SkipFilterPlugins.Has(pl.Name()) {
 			continue
 		}
+		metrics.PluginEvaluationTotal.WithLabelValues(pl.Name(), Filter).Inc()


Let's add PreFilter too? Just in case some plugins only implement PreFilter.

Or we can add to all extension points for counting plugin called times. Starting with filter is also OK.

Let's start with preFilter/filter in this PR.
@kidddddddddddddddddddddd will work on the prescore/score.
#110643 (comment)

I'm not sure if it's worth implementing on other extension points for now because it's other extension points don't have Skip feature. I'd say we can support other extension points when we support Skip on them.

alculquicondor · 2023-02-06T17:50:51Z

/label sig-instrumentation

k8s-ci-robot · 2023-02-06T17:50:54Z

@alculquicondor: The label(s) /label sig-instrumentation cannot be applied. These labels are supported: api-review, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, team/katacoda, refactor, official-cve-feed. Is this label configured under labels -> additional_labels or labels -> restricted_labels in plugin.yaml?

Details

In response to this:

/label sig-instrumentation

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.

alculquicondor · 2023-02-06T17:51:02Z

/sig instrumentation

kerthcet · 2023-02-07T03:31:37Z

This is a common question in PRR for any new features: how can an admin know if a feature is being used? This PR moves the needle in that direction.

Got it, before without the support of Skip, all enabled filter plugins' being called times should be equal, now they're not very likely, this can be grabbed by metrics. Same to new plugins added.

kerthcet · 2023-02-07T03:57:34Z

 		if state.SkipFilterPlugins.Has(pl.Name()) {
 			continue
 		}
+		metrics.PluginEvaluationTotal.WithLabelValues(pl.Name(), Filter).Inc()


Or we can add to all extension points for counting plugin called times. Starting with filter is also OK.

kerthcet · 2023-02-07T04:01:46Z

+			Name:           "plugin_evaluation_total",
+			Help:           "Number of attempts to schedule pods by each plugin and the extension point (only Filter is supported now.).",
+			StabilityLevel: metrics.ALPHA,
+		}, []string{"plugin", "extension_point"})


Maybe profile here is also needed?

Good point 👍

logicalhan · 2023-02-09T17:42:08Z

/assign
/triage accepted

sanposhiho · 2023-02-22T08:38:37Z

Thanks @kerthcet @alculquicondor.
These days, I'm running out of my time with KEPs related implementation,
so will get back here after them. 🙏

sanposhiho · 2023-03-06T00:49:24Z

@kerthcet @alculquicondor Sorry for being late on it. Fixed based on the suggestions:

support PreFilter
support profile label

alculquicondor · 2023-03-06T18:42:11Z

This metric counts how many times the specific plugin affects the scheduling result. In other words, the metric doesn't get incremented when the scheduling plugin makes no decision about an incoming Pod. For example, the metric only increments for the NodeAffinity plugin if a Pod being scheduled defines node affinity.

Skip is an implementation detail that users are not aware of.

alculquicondor

/lgtm
/approve

alculquicondor · 2023-03-06T18:43:54Z

 		if state.SkipFilterPlugins.Has(pl.Name()) {
 			continue
 		}
+		metrics.PluginEvaluationTotal.WithLabelValues(pl.Name(), Filter, f.profileName).Inc()


🤔 why is the Filter value exported, but not preFilter? But this is outside of the scope of this PR.

Seems Filter value is used in another package. (schedule_one.go)
https://github.com/sanposhiho/kubernetes/blob/799444a689da80470ad1aa87153f4feb5134b3db/pkg/scheduler/schedule_one.go#L533

It may be better to move those variables to metrics package? since they are used in the metric value's label.

yes, that sounds like a better location.

PTAL #116312

k8s-ci-robot · 2023-03-06T18:44:31Z

LGTM label has been added.

Details

Git tree hash: 95cdc134b79f33807c3ee8847b54a32b64167d1d

logicalhan · 2023-03-06T18:45:52Z

+			Name:           "plugin_evaluation_total",
+			Help:           "Number of attempts to schedule pods by each plugin and the extension point (available only in PreFilter and Filter.).",
+			StabilityLevel: metrics.ALPHA,
+		}, []string{"plugin", "extension_point", "profile"})


What's the cardinality of these labels?

<20, <10, <5

k8s-ci-robot · 2023-03-06T18:48:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, sanposhiho

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/scheduler/OWNERS~~ [alculquicondor]

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

sanposhiho · 2023-03-06T21:02:23Z

#115082 (comment)

modified the release not a bit

k8s-ci-robot requested review from chendave and kerthcet January 15, 2023 06:43

k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 15, 2023

k8s-ci-robot requested a review from alculquicondor January 26, 2023 14:09

alculquicondor reviewed Jan 26, 2023

View reviewed changes

k8s-ci-robot assigned kerthcet Jan 27, 2023

feature(scheduler): implement plugin_evaluation_total metric

4f77178

sanposhiho force-pushed the filter-metrics branch from 13a515b to 4f77178 Compare February 4, 2023 07:21

sanposhiho changed the title ~~feature(scheduler): implement schedule_attempts metric~~ feature(scheduler): implement plugin_evaluation_total metric Feb 4, 2023

alculquicondor reviewed Feb 6, 2023

View reviewed changes

k8s-ci-robot added the sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. label Feb 6, 2023

kerthcet reviewed Feb 7, 2023

View reviewed changes

k8s-ci-robot assigned logicalhan Feb 9, 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 Feb 9, 2023

alculquicondor mentioned this pull request Feb 13, 2023

feature(scheduler): won't run Score if PreScore returned a Skip status #115652

Merged

AxeZhan mentioned this pull request Feb 14, 2023

metrics to see which plugins affect to scheduler's decisions in Filter/Score phase #110643

Closed

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 6, 2023

support PreFilter as well

608f480

sanposhiho force-pushed the filter-metrics branch from 6fb7b49 to 608f480 Compare March 6, 2023 00:48

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 6, 2023

alculquicondor reviewed Mar 6, 2023

View reviewed changes

k8s-ci-robot assigned alculquicondor Mar 6, 2023

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

logicalhan reviewed Mar 6, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 6, 2023

k8s-ci-robot merged commit 283c26f into kubernetes:master Mar 6, 2023

k8s-ci-robot added this to the v1.27 milestone Mar 6, 2023

sanposhiho mentioned this pull request Mar 6, 2023

cleanup(scheduler): move metric labels to metrics package #116312

Merged

tosi3k mentioned this pull request Apr 25, 2023

Performance degradation in scheduler after adding metrics for counting plugin evaluation #117592

Closed

mochizuki875 mentioned this pull request Jun 11, 2025

KEP-3243: Update milestone v1.33 to v1.34 and add the new feature gate to control the design change of TopologySpreadConstraint's matchLabelKeys kubernetes/enhancements#5205

Merged

Conversation

sanposhiho commented Jan 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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.:

Uh oh!

sanposhiho commented Jan 26, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alculquicondor commented Jan 26, 2023

Uh oh!

kerthcet commented Jan 27, 2023

Uh oh!

sanposhiho commented Feb 4, 2023

Uh oh!

kerthcet commented Feb 6, 2023

Uh oh!

kerthcet commented Feb 6, 2023

Uh oh!

alculquicondor commented Feb 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanposhiho Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alculquicondor commented Feb 6, 2023

Uh oh!

k8s-ci-robot commented Feb 6, 2023

Uh oh!

alculquicondor commented Feb 6, 2023

Uh oh!

kerthcet commented Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

logicalhan commented Feb 9, 2023

Uh oh!

sanposhiho commented Feb 22, 2023

Uh oh!

sanposhiho commented Mar 6, 2023

Uh oh!

alculquicondor commented Mar 6, 2023

Uh oh!

alculquicondor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Mar 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alculquicondor Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Mar 6, 2023

Uh oh!

sanposhiho commented Jan 15, 2023 •

edited

Loading

sanposhiho Mar 6, 2023 •

edited

Loading

kerthcet commented Feb 7, 2023 •

edited

Loading

alculquicondor Mar 6, 2023 •

edited

Loading