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.

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

feature(scheduler): add "goroutines" metric and deprecate the "scheduler_goroutines" metric #112003

Merged
merged 1 commit into from Sep 12, 2022

Conversation

sanposhiho
Copy link
Member

@sanposhiho sanposhiho commented Aug 24, 2022

What type of PR is this?

/kind feature

What this PR does / why we need it:

See https://github.com/kubernetes/kubernetes/pull/111775/files#r945093057

We often create goroutines with parallelizer. But, we don't inc/dec the SchedulerGoroutines metric in it.

The metric is called scheduler_goroutines, but it's already under the scheduler system. So the final metric has the name:
scheduler_scheduler_goroutines. Maybe we should just call it goroutines.
Note that we can't just remove a metric. We need to duplicate and mark the old one as deprecated for a release or two.

Also see #112034.

This PR adds the new metric goroutines that replaces scheduler_goroutines metric and increment the number of goroutines in more place than scheduler_goroutines.

Which issue(s) this PR fixes:

Fixes #112034

Special notes for your reviewer:

What labeling should be added is open to debate.
Currently:

  • In plugin -> plugin name.
  • In framework -> related extension point name. (Like "Filter", "Score")

Does this PR introduce a user-facing change?

The `goroutines` metric is newly added in the scheduler. 
It replaces `scheduler_goroutines` metric and it counts the number of goroutine in more places than `scheduler_goroutine` does.

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


@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. 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. labels Aug 24, 2022
@k8s-ci-robot 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 Aug 24, 2022
@k8s-ci-robot 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 Aug 24, 2022
@sanposhiho sanposhiho force-pushed the metrics-goroutine branch 2 times, most recently from 890d261 to 6e2de16 Compare August 24, 2022 10:21
@sanposhiho
Copy link
Member Author

/retest

@@ -183,7 +183,7 @@ func (pl *PodTopologySpread) PreScore(
atomic.AddInt64(tpCount, int64(count))
}
}
pl.parallelizer.Until(ctx, len(allNodes), processAllNode)
pl.parallelizer.Until(ctx, len(allNodes), processAllNode,pl.Name())
Copy link
Contributor

Choose a reason for hiding this comment

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

please run ./hack/update-gofmt.sh to execute gofmt

Copy link
Member Author

Choose a reason for hiding this comment

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

🙏
Updated.

@@ -1408,7 +1408,7 @@ func BenchmarkTestDefaultEvenPodsSpreadPriority(b *testing.B) {
score, _ := p.Score(ctx, state, pod, n.Name)
gotList[i] = framework.NodeScore{Name: n.Name, Score: score}
}
p.parallelizer.Until(ctx, len(filteredNodes), scoreNode)
p.parallelizer.Until(ctx, len(filteredNodes), scoreNode,"")
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@alculquicondor
Copy link
Member

Add "fixes #112034" to the description.

I'll review once there is an lgtm, thanks :)

@sanposhiho
Copy link
Member Author

/retitle feature(scheduler): add "goroutines" metric and deprecate the "scheduler_goroutines" metric

@k8s-ci-robot k8s-ci-robot added the sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. label Aug 25, 2022
@k8s-ci-robot k8s-ci-robot changed the title add(scheduler): increment/decrement SchedulerGoroutines metrics in parallelizer feature(scheduler): add "goroutines" metric and deprecate the "scheduler_goroutines" metric Aug 25, 2022
@sanposhiho
Copy link
Member Author

sanposhiho commented Aug 25, 2022

added the change suggested in #112034. Also, updated the PR description and the release note.

The metric is called scheduler_goroutines, but it's already under the scheduler system. So the final metric has the name:
scheduler_scheduler_goroutines. Maybe we should just call it goroutines.
Note that we can't just remove a metric. We need to duplicate and mark the old one as deprecated for a release or two.

@dashpole
Copy link
Contributor

/triage accepted
/assign @logicalhan

@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 Aug 25, 2022
@sanposhiho
Copy link
Member Author

/retest

pkg/scheduler/framework/parallelize/parallelism.go Outdated Show resolved Hide resolved
pkg/scheduler/framework/parallelize/parallelism.go Outdated Show resolved Hide resolved
pkg/scheduler/metrics/metrics.go Show resolved Hide resolved
pkg/scheduler/schedule_one.go Show resolved Hide resolved
DeprecatedVersion: "1.26.0",
Name: "scheduler_goroutines",
Help: "Number of running goroutines split by the work they do such as binding. This metric is replaced by the \"goroutines\" metric.",
StabilityLevel: metrics.ALPHA,
Copy link
Member

Choose a reason for hiding this comment

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

Just a question, this metric is still alpha, can we just remove it instead?

// ALPHA metrics have no stability guarantees, as such, labels may
// be arbitrarily added/removed and the metric may be deleted at any time.
ALPHA StabilityLevel = "ALPHA"

Copy link
Member

Choose a reason for hiding this comment

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

In theory, we can. But in practice, we would probably be breaking some users too suddenly. One release is not too bad for us to take the burden of maintenance.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, scheduler_goroutines was introduced in v1.17 and it's expected that many users are already using it.
#83535
It's kind to announce it's deprecated and wait some time so that users can migrate to the new metric.

pkg/scheduler/schedule_one.go Show resolved Hide resolved
pkg/scheduler/metrics/metrics.go Show resolved Hide resolved
doWorkPiesWithMetrics := func(piece int) {
metrics.Goroutines.WithLabelValues(labelValue).Inc()
defer metrics.Goroutines.WithLabelValues(labelValue).Dec()
doWorkPiece(piece)
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 actually cannot get the point of this metric, how we can get from this metric? it's inc and then dec, what we expect from this metric finally.

I am not sure if the metric can record the intermediate state, this needs to be confirmed.

Copy link
Member

Choose a reason for hiding this comment

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

The metric helps monitor the health of the controller, to make sure that the are no goroutine leaks.

pkg/scheduler/framework/parallelize/parallelism.go Outdated Show resolved Hide resolved
@sanposhiho
Copy link
Member Author

@alculquicondor @chendave Thanks for reviewing, fixed some issues.

workqueue.ParallelizeUntil(ctx, p.parallelism, pieces, doWorkPiece, workqueue.WithChunkSize(chunkSizeFor(pieces, p.parallelism)))
func (p Parallelizer) Until(ctx context.Context, pieces int, doWorkPiece workqueue.DoWorkPieceFunc, operations ...string) {
withMetrics := func(piece int) {
metrics.Goroutines.WithLabelValues(operations...).Inc()
Copy link
Member

Choose a reason for hiding this comment

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

this looks dangerous.
What happens if you pass a number of operations different than 1? Does that cause a panic?

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 check the length of the parameters and log some warning but this seems like violating the rule of optional parameters, or revert this logic back to your early version. I am fine with either of them.

btw, do you want to add some comments on the operations for what is it?

Copy link
Member

Choose a reason for hiding this comment

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

+1 to reverting and comment

Copy link
Member

Choose a reason for hiding this comment

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

another option is to have a second function Parallelizer.UntilWithOperation

Copy link
Member

Choose a reason for hiding this comment

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

Or pull the operation into the context? 🤷

Copy link
Member Author

Choose a reason for hiding this comment

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

👌 Reverted and add the comment about operation.

@@ -96,14 +96,23 @@ var (
Help: "Number of pending pods, by the queue type. 'active' means number of pods in activeQ; 'backoff' means number of pods in backoffQ; 'unschedulable' means number of pods in unschedulablePods.",
StabilityLevel: metrics.STABLE,
}, []string{"queue"})
// SchedulerGoroutines isn't called in some parts where starts goroutines.
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
// SchedulerGoroutines isn't called in some parts where starts goroutines.
// SchedulerGoroutines isn't called in some parts where goroutines start.

@@ -96,14 +96,23 @@ var (
Help: "Number of pending pods, by the queue type. 'active' means number of pods in activeQ; 'backoff' means number of pods in backoffQ; 'unschedulable' means number of pods in unschedulablePods.",
StabilityLevel: metrics.STABLE,
}, []string{"queue"})
// SchedulerGoroutines isn't called in some parts where starts goroutines.
// Goroutines metric will replace SchedulerGoroutines metric and Goroutine metric contains all goroutines.
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
// Goroutines metric will replace SchedulerGoroutines metric and Goroutine metric contains all goroutines.
// Goroutines metric replaces SchedulerGoroutines metric. Goroutine metric tracks all goroutines.

@sanposhiho
Copy link
Member Author

@alculquicondor @chendave Addressed the given reviews and squashed commits. Please retake a look 🙏

&metrics.GaugeOpts{
Subsystem: SchedulerSubsystem,
Name: "scheduler_goroutines",
Name: "goroutines",
Help: "Number of running goroutines split by the work they do such as binding.",
StabilityLevel: metrics.ALPHA,
}, []string{"work"})
Copy link
Member

Choose a reason for hiding this comment

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

operation

Copy link
Member Author

Choose a reason for hiding this comment

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

👌 renamed.

Copy link
Member

@alculquicondor alculquicondor left a comment

Choose a reason for hiding this comment

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

/approve
/hold for lgtm from sig-instrumentation

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 1, 2022
@k8s-ci-robot
Copy link
Contributor

[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

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 1, 2022
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.

/lgtm
(from instrumentation)

Thanks for deprecating the metric with the scheduler stutter at the beginning of the metric name.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 12, 2022
@alculquicondor
Copy link
Member

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 12, 2022
@k8s-ci-robot k8s-ci-robot merged commit 3ac752e into kubernetes:master Sep 12, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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 Denotes a PR that will be considered when it comes time to generate release notes. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some scheduler goroutines are not tracked in the metric
7 participants