Collect metrics #505

ingvagabund · 2021-02-22T23:29:16Z

Fixes: #348

Have the descheduler collect metrics and serve them through /metrics endpoint on 10258 secure port.

How to test it? Have descheduler running with some descheduling internal (e.g. 1s) and have some pods evicted. Then just run curl https://localhost:10258/metrics -k.

Metrics bits mostly copy-pasted from kube-scheduler.

ingvagabund · 2021-02-22T23:32:42Z

#348 (comment)

@seanmalloy I merged pods_evicted_success and pods_evicted_failed into pods_evicted as a labeled CounterVec. Though, not against having two metrics either.

seanmalloy · 2021-02-22T23:38:44Z

/cc

damemi · 2021-02-23T20:22:23Z

pkg/descheduler/evictions/evictions.go

@@ -97,19 +98,21 @@ func (pe *PodEvictor) TotalEvicted() int {
 // EvictPod returns non-nil error only when evicting a pod on a node is not
 // possible (due to maxPodsToEvictPerNode constraint). Success is true when the pod
 // is evicted on the server side.
-func (pe *PodEvictor) EvictPod(ctx context.Context, pod *v1.Pod, node *v1.Node, reasons ...string) (bool, error) {
+func (pe *PodEvictor) EvictPod(ctx context.Context, pod *v1.Pod, node *v1.Node, strategy string, reasons ...string) (bool, error) {


Is this new strategy param used anywhere? It's kind of confusing to have to update all the strategies with lines like podEvictor.EvictPod(ctx, pod, node, "NodeAffinity", "NodeAffinity")

I don't like it either. We might drop reasons field since I don't see it used for anything but passing a strategy name.

I think reasons is fine, but specifically the strategy param you added here... I don't see it used anywhere, and it seems like you're just using reasons anyway, right?

Oh, I see. My bad. metrics.PodsEvicted.With(map[string]string{"result": "maximum number reached", "strategy": reason, "namespace": pod.Namespace}).Inc() is supposed to have "strategy": strategy pair in it. Right now, reasons can be basically anything meaningful. Yet, we set reasons param only to individual strategy names.

Ah yeah, I agree then we could either drop the ...reasons param all together, or add this strategy param, and just use that in each strategy (since reasons is optional, the strategy can just pass its name which will be used for metrics).

I can see a possible use case where one strategy may have different reasons for eviction, so I'm not sure it's worth getting rid of entirely yet. But it doesn't make much sense to keep if we're not using it. Wdyt?

I'm not sure it's worth getting rid of entirely yet

Yeah, not completely sure either. I'll keep it there for the moment. Better than drop it and re-introduce it later.

ingvagabund · 2021-03-01T11:43:52Z

$ curl -s https://localhost:10258/metrics -k | grep descheduler
# HELP descheduler_build_info [ALPHA] Build info about descheduler, including Go version, Descheduler version, Git SHA, Git branch
# TYPE descheduler_build_info gauge
descheduler_build_info{DeschedulerVersion="v20210301-v0.20.0-38-gf83615418",GitBranch="collect-metrics",GitSha1="f83615418e729595e6a39d8632304c05442bc67a",GoVersion="go1.15"} 0

ingvagabund · 2021-03-01T11:50:41Z

After some time running over vanilla OpenShift 4.8 with the example policy config:

$ curl -s https://localhost:10258/metrics -k | grep descheduler
# HELP descheduler_build_info [ALPHA] Build info about descheduler, including Go version, Descheduler version, Git SHA, Git branch
# TYPE descheduler_build_info gauge
descheduler_build_info{DeschedulerVersion="v20210301-v0.20.0-38-gf83615418",GitBranch="collect-metrics",GitSha1="f83615418e729595e6a39d8632304c05442bc67a",GoVersion="go1.15"} 0
# HELP descheduler_pods_evicted [ALPHA] Number of successfully evicted pods, by the result, by the strategy, by the namespace. 'failed' result means a pod could not be evicted
# TYPE descheduler_pods_evicted counter
descheduler_pods_evicted{namespace="openshift-marketplace",result="success",strategy="RemoveDuplicatePods"} 1

damemi · 2021-03-01T14:15:11Z

pkg/descheduler/evictions/evictions.go

@@ -97,19 +98,21 @@ func (pe *PodEvictor) TotalEvicted() int {
 // EvictPod returns non-nil error only when evicting a pod on a node is not
 // possible (due to maxPodsToEvictPerNode constraint). Success is true when the pod
 // is evicted on the server side.
-func (pe *PodEvictor) EvictPod(ctx context.Context, pod *v1.Pod, node *v1.Node, reasons ...string) (bool, error) {
+func (pe *PodEvictor) EvictPod(ctx context.Context, pod *v1.Pod, node *v1.Node, strategy string, reasons ...string) (bool, error) {
 	var reason string
 	if len(reasons) > 0 {
 		reason = " (" + strings.Join(reasons, ", ") + ")"


Continuing from what we're talking about above (https://github.com/kubernetes-sigs/descheduler/pull/505/files#r584629100), maybe we can change this line to:

reason = strategy + " (" + strings.Join(reasons, ", ") + ")"

then, all the calls to EvictPod can be shortened to just 1 parameter (so, no longer passing in the optional reasons list, but still keeping that available):

podEvictor.EvictPod(ctx, pod, nodeMap[nodeName], "RemoveDuplicatePods")

this way, we don't have to remove the reasons parameter but we clean up those weird-looking function calls. We can then start some work to actually use reasons for each strategy in a more informative way than how it is currently. What do you think?

Good idea!!! Updated.

pkg/descheduler/evictions/evictions.go

ingvagabund · 2021-03-02T10:46:45Z

/hold cancel

ingvagabund · 2021-03-02T10:49:08Z

Not sure if 10258 port is allocated by a different component. The PR also pulls a lot of new dependencies (mainly apiserver bits) to provide secure serving of the metrics. Right now, the metrics are served by default and there's no way to disable it. We might provide additional option/configuration to disable the metrics explicitly, wdyt?

ingvagabund · 2021-03-02T10:49:55Z

@damemi @seanmalloy @lixiang233 PTAL

lixiang233 · 2021-03-02T11:50:42Z

cmd/descheduler/app/options/options.go

 	"sigs.k8s.io/descheduler/pkg/apis/componentconfig"
 	"sigs.k8s.io/descheduler/pkg/apis/componentconfig/v1alpha1"
 	deschedulerscheme "sigs.k8s.io/descheduler/pkg/descheduler/scheme"
 )

+const (
+	DefaultDeschedulerPort = 10258


Can you mention in somewhere that port 10258 is used for metrics by default and users can use command line flag --secure-port to custom port number? It's not easy to get this info from the help command.

Added some comments into --disable-metrics flag and the readme.

metrics/metrics.go

lixiang233 · 2021-03-02T12:10:42Z

We might provide additional option/configuration to disable the metrics explicitly, wdyt?

+1 for providing a way to disable metrics.

ingvagabund · 2021-03-03T10:21:11Z

Let's merge #513 first

New metrics: - build_info: Build info about descheduler, including Go version, Descheduler version, Git SHA, Git branch - pods_evicted: Number of successfully evicted pods, by the result, by the strategy, by the namespace

damemi · 2021-03-05T15:32:54Z

README.md

@@ -521,6 +521,16 @@ Setting `--v=4` or greater on the Descheduler will log all reasons why any pod i
 Pods subject to a Pod Disruption Budget(PDB) are not evicted if descheduling violates its PDB. The pods
 are evicted by using the eviction subresource to handle PDB.

+## Metrics


Can you update the table of contents too? (we should add a verify script for that...)
Other than that, lgtm
/approve

k8s-ci-robot · 2021-03-05T15:33:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: damemi, ingvagabund

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:

~~OWNERS~~ [damemi]

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

seanmalloy · 2021-03-07T05:06:49Z

/kind feature

seanmalloy · 2021-03-07T05:33:45Z

/lgtm

Collect metrics

k8s-ci-robot requested review from aveshagarwal and damemi February 22, 2021 23:29

ingvagabund mentioned this pull request Feb 22, 2021

Add Support For Prometheus Metrics #348

Closed

k8s-ci-robot requested a review from seanmalloy February 22, 2021 23:38

damemi reviewed Feb 23, 2021

View reviewed changes

ingvagabund force-pushed the collect-metrics branch from 0551154 to a112beb Compare March 1, 2021 11:12

damemi reviewed Mar 1, 2021

View reviewed changes

ingvagabund mentioned this pull request Mar 1, 2021

Publish descheduler metrics openshift/cluster-kube-descheduler-operator#171

Merged

ingvagabund force-pushed the collect-metrics branch from a112beb to bba5fd2 Compare March 1, 2021 17:34

damemi reviewed Mar 1, 2021

View reviewed changes

pkg/descheduler/evictions/evictions.go Outdated Show resolved Hide resolved

ingvagabund force-pushed the collect-metrics branch from bba5fd2 to 74e2bed Compare March 1, 2021 17:52

ingvagabund changed the title ~~wip: Collect metrics~~ Collect metrics Mar 2, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 2, 2021

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

lixiang233 reviewed Mar 2, 2021

View reviewed changes

metrics/metrics.go Outdated Show resolved Hide resolved

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 2, 2021

ingvagabund added 2 commits March 3, 2021 15:56

Vendor metrics

ea6e9f2

Move build's versioning bits under pkg/version

3bd031b

ingvagabund force-pushed the collect-metrics branch from 74e2bed to 479620b Compare March 3, 2021 15:06

ingvagabund added 2 commits March 3, 2021 16:15

Register metrics

1c5b327

New metrics: - build_info: Build info about descheduler, including Go version, Descheduler version, Git SHA, Git branch - pods_evicted: Number of successfully evicted pods, by the result, by the strategy, by the namespace

Increase pods_evicted metric

24458fb

ingvagabund force-pushed the collect-metrics branch from 479620b to 572d100 Compare March 3, 2021 15:16

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 3, 2021

damemi reviewed Mar 5, 2021

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 5, 2021

Serve secure metrics at 10258

701f224

ingvagabund force-pushed the collect-metrics branch from 572d100 to 701f224 Compare March 5, 2021 15:35

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 7, 2021

k8s-ci-robot assigned seanmalloy Mar 7, 2021

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

k8s-ci-robot merged commit 24a0651 into kubernetes-sigs:master Mar 7, 2021

ingvagabund deleted the collect-metrics branch March 8, 2021 09:47

ingvagabund mentioned this pull request Mar 8, 2021

Sync upstream metrics openshift/descheduler#57

Merged

briend pushed a commit to briend/descheduler that referenced this pull request Feb 11, 2022

Merge pull request kubernetes-sigs#505 from ingvagabund/collect-metrics

33744ba

Collect metrics

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collect metrics #505

Collect metrics #505

ingvagabund commented Feb 22, 2021 •

edited

ingvagabund commented Feb 22, 2021

seanmalloy commented Feb 22, 2021

damemi Feb 23, 2021

ingvagabund Feb 24, 2021

damemi Feb 24, 2021

ingvagabund Feb 24, 2021 •

edited

damemi Feb 24, 2021

ingvagabund Mar 1, 2021

ingvagabund commented Mar 1, 2021

ingvagabund commented Mar 1, 2021

damemi Mar 1, 2021

ingvagabund Mar 1, 2021

ingvagabund commented Mar 2, 2021

ingvagabund commented Mar 2, 2021

ingvagabund commented Mar 2, 2021

lixiang233 Mar 2, 2021

ingvagabund Mar 3, 2021 •

edited

lixiang233 commented Mar 2, 2021

ingvagabund commented Mar 3, 2021

damemi Mar 5, 2021

ingvagabund Mar 5, 2021

k8s-ci-robot commented Mar 5, 2021

seanmalloy commented Mar 7, 2021

seanmalloy commented Mar 7, 2021

Collect metrics #505

Collect metrics #505

Conversation

ingvagabund commented Feb 22, 2021 • edited

ingvagabund commented Feb 22, 2021

seanmalloy commented Feb 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ingvagabund Feb 24, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ingvagabund commented Mar 1, 2021

ingvagabund commented Mar 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ingvagabund commented Mar 2, 2021

ingvagabund commented Mar 2, 2021

ingvagabund commented Mar 2, 2021

Choose a reason for hiding this comment

ingvagabund Mar 3, 2021 • edited

Choose a reason for hiding this comment

lixiang233 commented Mar 2, 2021

ingvagabund commented Mar 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 5, 2021

seanmalloy commented Mar 7, 2021

seanmalloy commented Mar 7, 2021

ingvagabund commented Feb 22, 2021 •

edited

ingvagabund Feb 24, 2021 •

edited

ingvagabund Mar 3, 2021 •

edited