Fix some scheduler metrics(pending_pods and schedule_attempts_total) are not recorded. #87692

everpeace · 2020-01-30T14:32:45Z

What type of PR is this?

/kind bug
/sig scheduling

What this PR does / why we need it:

fix two scheduler metrics (pending_pods and schedule_attempts_total) are not recorded.

Which issue(s) this PR fixes:

Special notes for your reviewer:

scheduler/metrics.{PodScheduleSuccesses, PodScheduleFailures, PodScheduleErrors} and scheduler/metrics.PendingPodsRecorders(ActivePods, UnschedulablePods, BackoffPods) seems to too early to be initialized.

All the k8s.io/component-base/metrics's metrics primitives are lazy metric. So, calling some_metric.With(labels) before registration returns noop metrics.

So I made mainly two modifications.

move scheduler/metrics.Registry() before crerating scheduler queue
delayed PodScheduleSuccesses, PodScheduleFailures, PodScheduleErrors metrics' initialization

Does this PR introduce a user-facing change?:

fixed two scheduler metrics (pending_pods and schedule_attempts_total) not being recorded

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

none.

pkg/scheduler/scheduler_test.go

pkg/scheduler/scheduler.go

everpeace · 2020-01-30T16:05:39Z

/retest

ahg-g

/cc @liu-cong

We should back port this to 1.17

ahg-g · 2020-01-30T16:46:13Z

pkg/scheduler/scheduler.go

@@ -291,6 +291,8 @@ func New(client clientset.Interface,
 		nodeInfoSnapshot:               snapshot,
 	}

+	metrics.Register()


why moving this to here makes a difference?

oh, because it has to be done before initializing the scheduling_queue

Yes, it is the reason. I wanted to find a more essential & safe way to Register metric instances, though.

ahg-g · 2020-01-30T16:49:00Z

pkg/scheduler/metrics/metrics.go

@@ -262,6 +265,9 @@ func Register() {
 			legacyregistry.MustRegister(metric)
 		}
 		volumeschedulingmetrics.RegisterVolumeSchedulingMetrics()
+		PodScheduleSuccesses = scheduleAttempts.With(metrics.Labels{"result": "scheduled"})


instead of doing this, perhaps we should follow the pendingPods metric approach, define functions that return the metric (see lines 275-292)

ok, I guess we access those handlers directly in the scheduler, ok I guess this is fine.

ahg-g · 2020-01-30T17:03:46Z

Looking at v1.16 and v1.15, registering the metrics has always been done after creating the queue, I wonder why this is an issue now.

liu-cong · 2020-01-30T17:03:52Z

We should back port this to 1.17

Isn't this a bug since the beginning so we need to backport to all applicable versions?

ahg-g · 2020-01-30T18:55:44Z

We should back port this to 1.17

Isn't this a bug since the beginning so we need to backport to all applicable versions?

Yes we do, I realized that this is a bug in 1.16 and 1.15 after I posted this.

ahg-g · 2020-01-30T19:39:49Z

I think commit 8da448d is the main culprit, it switched the scheduler from Prometheus to k8s wrapper in 1.16, but didn't fix the registration.

Ok, so this fix must be backported to 1.17 and 1.16 (just backport the call to Register to happen earlier since the attempts metric doesn't exist in 1.16)

ahg-g · 2020-01-30T20:20:20Z

/priority critical-urgent

k8s-ci-robot · 2020-01-30T20:20:22Z

@ahg-g: The label(s) priority/critical-urget cannot be applied, because the repository doesn't have them

In response to this:

/priority critical-urget

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.

ahg-g · 2020-01-30T20:21:46Z

/retest
/lgtm
/approve

ahg-g · 2020-01-30T21:19:58Z

/retest

everpeace · 2020-01-31T00:58:46Z

Thanks for the quick review!!

/retest

everpeace · 2020-01-31T01:51:46Z

hmm, although I'm not familiar with e2e test, I believe that my change would not affect e2e. I'm not sure what I could do other than /retest .

everpeace · 2020-01-31T02:25:02Z

/retest

pkg/scheduler/metrics/metrics.go

haosdent · 2020-01-31T03:41:01Z

/lgtm

haosdent · 2020-01-31T03:41:35Z

/retest

draveness

Thanks, please squash the commit.

/hold

because metric initializations are too early. This causes actual metric instance become no-op. modification made in thie commit to make sure actual metric instance won't be no-op metrics: - re-initialize scheduler/metrics.PodSchedule{Successes, Failure, Errors} after metric creation - scheduler/metrics.Register() should be called before initializing SchedulingQueue,

everpeace · 2020-01-31T08:07:24Z

squashed.

everpeace · 2020-01-31T08:09:00Z

/hold cancel

haosdent · 2020-01-31T08:10:01Z

/lgtm

fejta-bot · 2020-01-31T12:29:16Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

…-upstream-release-1.16 Automated cherry pick of #87692: Fix pending_pods, schedule_attempts_total was not recorded

…-upstream-release-1.17 Automated cherry pick of #87692: Fix pending_pods, schedule_attempts_total was not recorded

everpeace changed the title ~~Fix scheduler queue metrics(pending_pods and schedule_attempts_total) are not recorded.~~ Fix some scheduler metrics(pending_pods and schedule_attempts_total) are not recorded. Jan 30, 2020

k8s-ci-robot requested review from ahg-g and draveness January 30, 2020 14:33

Ye-Tian-Zero reviewed Jan 30, 2020

View reviewed changes

pkg/scheduler/scheduler_test.go Outdated Show resolved Hide resolved

Ye-Tian-Zero reviewed Jan 30, 2020

View reviewed changes

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

everpeace force-pushed the fix-scheduler-queue-metrics branch 2 times, most recently from 05ed996 to 26b7452 Compare January 30, 2020 15:43

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 30, 2020

ahg-g reviewed Jan 30, 2020

View reviewed changes

k8s-ci-robot requested a review from liu-cong January 30, 2020 16:46

ahg-g reviewed Jan 30, 2020

View reviewed changes

k8s-ci-robot added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jan 30, 2020

k8s-ci-robot assigned ahg-g Jan 30, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 30, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 30, 2020

haosdent reviewed Jan 31, 2020

View reviewed changes

pkg/scheduler/metrics/metrics.go Outdated Show resolved Hide resolved

everpeace force-pushed the fix-scheduler-queue-metrics branch from 26b7452 to 9d1c6d2 Compare January 31, 2020 02:48

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 31, 2020

k8s-ci-robot assigned haosdent Jan 31, 2020

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

draveness reviewed Jan 31, 2020

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 Jan 31, 2020

everpeace force-pushed the fix-scheduler-queue-metrics branch from 9d1c6d2 to c9c4be6 Compare January 31, 2020 08:07

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 31, 2020

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

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

k8s-ci-robot merged commit 7f0ea14 into kubernetes:master Jan 31, 2020

k8s-ci-robot added this to the v1.18 milestone Jan 31, 2020

ahg-g mentioned this pull request Feb 1, 2020

Backport scheduler metrics fix to 1.16 and 1.17 #87739

Closed

This was referenced Feb 1, 2020

Automated cherry pick of #87692: Fix pending_pods, schedule_attempts_total was not recorded #87742

Merged

Automated cherry pick of #87692: Fix pending_pods, schedule_attempts_total was not recorded #87745

Merged

k8s-ci-robot added a commit that referenced this pull request Feb 4, 2020

Merge pull request #87745 from notpad/automated-cherry-pick-of-#87692…

0cf6bad

…-upstream-release-1.16 Automated cherry pick of #87692: Fix pending_pods, schedule_attempts_total was not recorded

k8s-ci-robot added a commit that referenced this pull request Feb 4, 2020

Merge pull request #87742 from notpad/automated-cherry-pick-of-#87692…

a4d8045

…-upstream-release-1.17 Automated cherry pick of #87692: Fix pending_pods, schedule_attempts_total was not recorded

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some scheduler metrics(pending_pods and schedule_attempts_total) are not recorded. #87692

Fix some scheduler metrics(pending_pods and schedule_attempts_total) are not recorded. #87692

everpeace commented Jan 30, 2020

everpeace commented Jan 30, 2020

ahg-g left a comment

ahg-g Jan 30, 2020

ahg-g Jan 30, 2020

everpeace Jan 31, 2020

ahg-g Jan 30, 2020

ahg-g Jan 30, 2020

ahg-g commented Jan 30, 2020

liu-cong commented Jan 30, 2020

ahg-g commented Jan 30, 2020

ahg-g commented Jan 30, 2020

ahg-g commented Jan 30, 2020 •

edited

k8s-ci-robot commented Jan 30, 2020

ahg-g commented Jan 30, 2020

ahg-g commented Jan 30, 2020

everpeace commented Jan 31, 2020

everpeace commented Jan 31, 2020 •

edited

everpeace commented Jan 31, 2020

haosdent commented Jan 31, 2020

haosdent commented Jan 31, 2020

draveness left a comment

everpeace commented Jan 31, 2020 •

edited

everpeace commented Jan 31, 2020

haosdent commented Jan 31, 2020

fejta-bot commented Jan 31, 2020

Fix some scheduler metrics(pending_pods and schedule_attempts_total) are not recorded. #87692

Fix some scheduler metrics(pending_pods and schedule_attempts_total) are not recorded. #87692

Conversation

everpeace commented Jan 30, 2020

everpeace commented Jan 30, 2020

ahg-g left a comment

Choose a reason for hiding this comment

ahg-g Jan 30, 2020

Choose a reason for hiding this comment

ahg-g Jan 30, 2020

Choose a reason for hiding this comment

everpeace Jan 31, 2020

Choose a reason for hiding this comment

ahg-g Jan 30, 2020

Choose a reason for hiding this comment

ahg-g Jan 30, 2020

Choose a reason for hiding this comment

ahg-g commented Jan 30, 2020

liu-cong commented Jan 30, 2020

ahg-g commented Jan 30, 2020

ahg-g commented Jan 30, 2020

ahg-g commented Jan 30, 2020 • edited

k8s-ci-robot commented Jan 30, 2020

ahg-g commented Jan 30, 2020

ahg-g commented Jan 30, 2020

everpeace commented Jan 31, 2020

everpeace commented Jan 31, 2020 • edited

everpeace commented Jan 31, 2020

haosdent commented Jan 31, 2020

haosdent commented Jan 31, 2020

draveness left a comment

Choose a reason for hiding this comment

everpeace commented Jan 31, 2020 • edited

everpeace commented Jan 31, 2020

haosdent commented Jan 31, 2020

fejta-bot commented Jan 31, 2020

ahg-g commented Jan 30, 2020 •

edited

everpeace commented Jan 31, 2020 •

edited

everpeace commented Jan 31, 2020 •

edited