Copy recordPluginMetrics in CycleState.Clone #108727

sanposhiho · 2022-03-16T08:52:30Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

We currently don't copy recordPluginMetrics field in CycleState.Clone() so recordPluginMetrics in the copied CycleCtate is always false.
But if we do so, for example, the following part will always be false. Is this the expected behavior..?

kubernetes/pkg/scheduler/framework/runtime/framework.go

Line 632 in 1007f49

if !state.ShouldRecordPluginMetrics() {

Ref: #108724 (comment)

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fix a bug where metrics are not recorded during Preemption(PostFilter).

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

k8s-ci-robot · 2022-03-16T08:52:38Z

@sanposhiho: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

denkensk

Can you also add an UT or add one case in exist UT for it?

sanposhiho · 2022-03-16T09:47:43Z

/retest

sanposhiho · 2022-03-16T13:27:52Z

/retest

sanposhiho · 2022-03-16T13:46:46Z

/retest

sanposhiho · 2022-03-20T08:02:10Z

@Huang-Wei your comment is for #108724, right?
Copied your comment in #108724 (comment)

alculquicondor · 2022-03-21T17:22:20Z

We only Clone the state when we are doing preemption. I think we don't care about metrics within each sub-step of preemption, as preemption itself has an overall latency metric.

Huang-Wei · 2022-03-21T19:09:33Z

@Huang-Wei your comment is for #108724, right?

ah, yes...

Huang-Wei · 2022-03-21T19:34:20Z

Hmm.. seems we had a bug that the following metrics were never recorded:

scheduler_plugin_execution_duration_seconds{extension_point=PreFilterExtensionAddPod}
scheduler_plugin_execution_duration_seconds{extension_point=PreFilterExtensionRemovePod}

as the cloned state always return false on ShouldRecordPluginMetrics(): runPreFilterExtensionAddPod and runPreFilterExtensionRemovePod.

I think we don't care about metrics within each sub-step of preemption, as preemption itself has an overall latency metric.

The impact is on RunPreFilterExtensionAddPod() and RunPreFilterExtensionRemovePod(). I think we should copy the value. See the explanation above.

alculquicondor · 2022-03-21T19:40:09Z

Right, but should we record the metrics for Filter/Score that run inside preemption?

Huang-Wei · 2022-03-21T20:53:37Z

Right, but should we record the metrics for Filter/Score that run inside preemption?

I think so. Actually plugin_execution_duration_seconds is not that useful to diagnose how a pod's overall scheduling looks like. I interpreted it simply to represent a plugin's running at a particular extension point, so it's more "stateless", and thus whether preemption is involved doesn't quite matter. Another note to read the plugin metrics as "stateless" is the samplePluginRate (10%).

pkg/scheduler/framework/cycle_state_test.go

Huang-Wei · 2022-03-23T19:23:19Z

pkg/scheduler/framework/cycle_state_test.go

@@ -41,6 +41,7 @@ func TestCycleStateClone(t *testing.T) {
 		data: data1,
 	}
 	state.Write(key, originalValue)
+	state.SetRecordPluginMetrics(true)


can we test both true and false?

👍
Changed the whole test to be table-driven test and added the test for that as a test case.

sanposhiho · 2022-03-31T11:18:07Z

/retest

alculquicondor · 2022-03-31T13:28:43Z

pkg/scheduler/framework/cycle_state_test.go

+
+			// we need to compare *sync.Map,
+			// because we cannot define cmp.Comparer for sync.Map or CycleState due to the issue from go vet.
+			if diff := cmp.Diff(&state.storage, &stateCopy.storage, cmpOpts...); diff != "" {


Now I think nothing is checking recordPluginMetrics?

is it a lot of changes if you make CycleState.storage a pointer?

Now I think nothing is checking recordPluginMetrics?

Ah,, right...

if you make CycleState.storage a pointer?

nice idea. No concerns from me.

sanposhiho · 2022-03-31T14:50:16Z

/retest

alculquicondor

/lgtm
/approve

This can wait for 1.25

k8s-ci-robot · 2022-03-31T16:00:07Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~pkg/scheduler/OWNERS~~ [Huang-Wei,alculquicondor]

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

Huang-Wei · 2022-03-31T21:56:07Z

since it's difficult to use cmp.Diff to compare sync.Map

I see. #108727 (comment) was based on the old implementation with regular map and mutex, now with sync.map, it seems we need extra work to compare the state.

sanposhiho · 2022-04-11T01:25:36Z

/retest

pkg/scheduler/framework/cycle_state_test.go

Huang-Wei · 2022-04-14T18:08:42Z

/lgtm

sanposhiho · 2022-05-06T04:44:08Z

@Huang-Wei @alculquicondor

Is it okay to /unhold?

alculquicondor

/hold cancel

sanposhiho · 2022-05-06T15:08:59Z

/retest

(#109863 #109864)

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Mar 16, 2022

sanposhiho mentioned this pull request Mar 16, 2022

use sync.Map in CycleState for better performance #108724

Merged

k8s-ci-robot requested review from denkensk and Huang-Wei March 16, 2022 08:53

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 Mar 16, 2022

denkensk reviewed Mar 16, 2022

View reviewed changes

sanposhiho force-pushed the fix-bug-clone branch from 764f7c1 to d65ccb0 Compare March 16, 2022 12:41

Huang-Wei reviewed Mar 23, 2022

View reviewed changes

pkg/scheduler/framework/cycle_state_test.go Outdated Show resolved Hide resolved

Huang-Wei reviewed Mar 23, 2022

View reviewed changes

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Mar 24, 2022

sanposhiho force-pushed the fix-bug-clone branch from d65ccb0 to 55c5d51 Compare March 24, 2022 09:51

k8s-ci-robot removed the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 24, 2022

sanposhiho force-pushed the fix-bug-clone branch from 2934df9 to dee8cb8 Compare March 31, 2022 08:27

This was referenced Mar 31, 2022

run govet in golangci-lint instead of ./hack/verify-govet.sh in pull-kubernetes-verify #109183

Closed

run govet in golangci-lint instead of ./hack/verify-govet.sh in pull-kubernetes-verify #109184

Closed

alculquicondor reviewed Mar 31, 2022

View reviewed changes

sanposhiho force-pushed the fix-bug-clone branch from dee8cb8 to 76a32ea Compare March 31, 2022 14:00

sanposhiho force-pushed the fix-bug-clone branch from 76a32ea to ed58683 Compare March 31, 2022 15:22

alculquicondor reviewed Mar 31, 2022

View reviewed changes

k8s-ci-robot assigned alculquicondor Mar 31, 2022

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

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 10, 2022

sanposhiho force-pushed the fix-bug-clone branch 4 times, most recently from c6f312b to cadfa7b Compare April 10, 2022 12:22

Huang-Wei reviewed Apr 13, 2022

View reviewed changes

pkg/scheduler/framework/cycle_state_test.go Outdated Show resolved Hide resolved

Copy recordPluginMetrics in CycleState.Clone

ccd6f7a

sanposhiho force-pushed the fix-bug-clone branch from cadfa7b to ccd6f7a Compare April 14, 2022 15:26

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 14, 2022

alculquicondor reviewed May 6, 2022

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 6, 2022

k8s-ci-robot merged commit 03a46ac into kubernetes:master May 6, 2022

k8s-ci-robot added this to the v1.25 milestone May 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Copy recordPluginMetrics in CycleState.Clone #108727

Copy recordPluginMetrics in CycleState.Clone #108727

sanposhiho commented Mar 16, 2022 •

edited

k8s-ci-robot commented Mar 16, 2022

denkensk left a comment

sanposhiho commented Mar 16, 2022

sanposhiho commented Mar 16, 2022

sanposhiho commented Mar 16, 2022

sanposhiho commented Mar 20, 2022

alculquicondor commented Mar 21, 2022

Huang-Wei commented Mar 21, 2022

Huang-Wei commented Mar 21, 2022 •

edited

alculquicondor commented Mar 21, 2022

Huang-Wei commented Mar 21, 2022 •

edited

Huang-Wei Mar 23, 2022

sanposhiho Mar 24, 2022

sanposhiho commented Mar 31, 2022

alculquicondor Mar 31, 2022

sanposhiho Mar 31, 2022

sanposhiho Mar 31, 2022

sanposhiho commented Mar 31, 2022

alculquicondor left a comment

k8s-ci-robot commented Mar 31, 2022

Huang-Wei commented Mar 31, 2022

sanposhiho commented Apr 11, 2022

Huang-Wei commented Apr 14, 2022

sanposhiho commented May 6, 2022

alculquicondor left a comment

sanposhiho commented May 6, 2022 •

edited

Copy recordPluginMetrics in CycleState.Clone #108727

Copy recordPluginMetrics in CycleState.Clone #108727

Conversation

sanposhiho commented Mar 16, 2022 • 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.:

k8s-ci-robot commented Mar 16, 2022

denkensk left a comment

Choose a reason for hiding this comment

sanposhiho commented Mar 16, 2022

sanposhiho commented Mar 16, 2022

sanposhiho commented Mar 16, 2022

sanposhiho commented Mar 20, 2022

alculquicondor commented Mar 21, 2022

Huang-Wei commented Mar 21, 2022

Huang-Wei commented Mar 21, 2022 • edited

alculquicondor commented Mar 21, 2022

Huang-Wei commented Mar 21, 2022 • edited

Huang-Wei Mar 23, 2022

Choose a reason for hiding this comment

sanposhiho Mar 24, 2022

Choose a reason for hiding this comment

sanposhiho commented Mar 31, 2022

alculquicondor Mar 31, 2022

Choose a reason for hiding this comment

sanposhiho Mar 31, 2022

Choose a reason for hiding this comment

sanposhiho Mar 31, 2022

Choose a reason for hiding this comment

sanposhiho commented Mar 31, 2022

alculquicondor left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 31, 2022

Huang-Wei commented Mar 31, 2022

sanposhiho commented Apr 11, 2022

Huang-Wei commented Apr 14, 2022

sanposhiho commented May 6, 2022

alculquicondor left a comment

Choose a reason for hiding this comment

sanposhiho commented May 6, 2022 • edited

sanposhiho commented Mar 16, 2022 •

edited

Huang-Wei commented Mar 21, 2022 •

edited

Huang-Wei commented Mar 21, 2022 •

edited

sanposhiho commented May 6, 2022 •

edited