Fix kubelet PVC stale metrics #59170

cofyc · 2018-02-01T05:56:32Z

What this PR does / why we need it:

Volumes on each node changes, we should not only add PVC metrics into
gauge vector. It's better use a collector to collector metrics from internal
stats.

Currently, if a PV (bound to a PVC testpv) is attached and used by node A, then migrated to node B or just deleted from node A later. testpvc metrics will not disappear from kubelet on node A. After a long running time, kubelet process will keep a lot of stale volume metrics in memory.

For these dynamic metrics, it's better to use a collector to collect metrics from a data source (StatsProvider here), like kube-state-metrics scraping metrics from kube-apiserver.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #57686

Special notes for your reviewer:

Release note:

Fix kubelet PVC stale metrics

cofyc · 2018-02-01T05:58:43Z

/cc @gnufied @wongma7

zhangxiaoyu-zidif · 2018-02-01T07:11:19Z

/ok-to-test

cofyc · 2018-02-01T07:15:48Z

@zhangxiaoyu-zidif Thanks!

zhangxiaoyu-zidif · 2018-02-01T07:55:28Z

pkg/kubelet/metrics/collectors/helper_test.go

+
+		return fmt.Errorf(`metric output does not match expectation; want:
+%s
+


run 'gofmt' =)

@zhangxiaoyu-zidif fixed

cofyc · 2018-02-01T10:51:33Z

/sig storage
/assign @gnufied @jingxu97

jingxu97 · 2018-02-01T23:43:47Z

@cofyc Thanks for your PR. I am ok with this PR, but will wait for @gnufied to review in more detail since I am very familiar with area.
One question is that besides PVC metrics, do you notice any other metrics have the same problem?

cofyc · 2018-02-02T02:24:36Z

@jingxu97 Just checked other metrics, they don't have the same problem.

Volumes on each node changes, we should not only add PVC metrics into gauge vector. It's better use a collector to collector metrics from stats. Sync from kubernetes#59170 with some modifications; - Use SummaryProvider instead of StatsProvider - Remove test files. - Generate build files.

jingxu97 · 2018-02-02T05:44:02Z

@cofyc thank you for checking. What metrics did you check and why pvc metric has the problem and others don't? Thanks!

cofyc · 2018-02-02T06:58:58Z

hi, @jingxu97

I've checked all other metrics:

ContainersPerPodCount, PodStartLatency, PodWorkerStartLatency, PLEGRelistLatency, PLEGRelistInterval are Summary metrics. They are used to sample observations. They don't have labels, just call Observe on each observed value is ok (e.g. https://github.com/kubernetes/kubernetes/blob/v1.10.0-alpha.3/pkg/kubelet/kubelet.go#L1930).
PodWorkerLatency, CgroupManagerLatency, RuntimeOperationsLatency, EvictionStatsAge, DevicePluginAllocationLatency are Summary vector, and RuntimeOperations, RuntimeOperationsErrors, DevicePluginRegistrationCount are Counter vector. They can call WithLabelValues to create new Summary or Counter dynamically, but they use fixed set of label values (e.g. syncType, resourceName), so they create fixed set of metrics. (e.g. https://github.com/kubernetes/kubernetes/blob/v1.10.0-alpha.3/pkg/kubelet/kubelet.go#L1925)

PVC metrics have problem, because they use namespace and pvc names as label values. WithLabelValues(labelValues) will create new metric for each new label values and PVC may be migrated to another node or deleted from cluster, but it's metrics will not to be cleared. So, after a long running time, kubelet will keep a lot of stale volume metrics in memory.

Theoretically there are two ways to fix:

Clear its metrics when PVC is removed.
Use a collector to generate all PVC metrics from stats when collecting.

The second is good and easier way to go for PVC metrics. It also decouples statistics and metrics generating.

gnufied · 2018-02-02T14:25:34Z

pkg/kubelet/metrics/collectors/volume_stats.go

+	}
+	addGauge := func(desc *prometheus.Desc, pvcRef *stats.PVCReference, v float64, lv ...string) {
+		lv = append([]string{pvcRef.Namespace, pvcRef.Name}, lv...)
+		ch <- prometheus.MustNewConstMetric(desc, prometheus.GaugeValue, v, lv...)


I would prefer that we did not use version that causes panics.

Updated. Log a warning message if NewConstMetric emits an error.

gnufied · 2018-02-02T14:28:52Z

pkg/kubelet/metrics/collectors/volume_stats_test.go

+		podStats = []statsapi.PodStats{
+			{
+				PodRef:    statsapi.PodReference{Name: "test-pod", Namespace: "test-namespace", UID: "UID_test-pod"},
+				StartTime: metav1.NewTime(time.Now()),


I think there is a metav1.Now()

cofyc · 2018-02-09T03:14:57Z

@jsafrane

That's weird.

curl -s http://localhost:10255/metrics | grep '^kubelet_volume'

Could you check this output on each node instead of prometheus?

If all PVC volumes on node were correctly unmounted and detached, kubelet /metrics endpoint should have no kubelet_volume_xxx metrics.

jsafrane · 2018-02-09T14:10:51Z

@cofyc, weird, I can't reproduce the issue today. I recompiled and reinstalled everything and it's gone. Sorry for the noise. Tested and LGTM from my side :-)

gnufied · 2018-02-09T17:25:18Z

/lgtm

rootfs · 2018-02-09T18:17:44Z

/assign @dchen1107 @derekwaynecarr for approval

cbandy · 2018-02-11T14:41:04Z

pkg/kubelet/metrics/collectors/volume_stats.go

+				continue
+			}
+			pvcUniqStr := pvcRef.Namespace + pvcRef.Name
+			if allPVCs.Has(pvcUniqStr) {


It looks as though pvcRef can be used as a map key.

I would prefer to gather everything into a map[stats.PVCReference]stats.VolumeStats then iterate over that.

@cbandy sets.String is implemented by a map. It has no performance issue here and we can generate metrics in one iteration.

cbandy · 2018-02-11T15:06:52Z

pkg/kubelet/metrics/collectors/volume_stats.go

+				// ignore if no PVC reference
+				continue
+			}
+			pvcUniqStr := pvcRef.Namespace + pvcRef.Name


This isn't really unique... a/bc and ab/c would collide.

@cbandy Thanks! I should use / to separate namespace and name. It's the way to generate unique key for objects in client-go cache store. Fixed now: fecff55#diff-ae1cd608f0ca9bcb432eeeca0cf3cde0R106
@gnufied Sorry, please take a look at again.

Volumes on each node changes, we should not only add PVC metrics into gauge vector. It's better use a collector to collector metrics from stats.

jsafrane · 2018-02-12T09:19:57Z

/lgtm

jsafrane · 2018-02-12T09:20:56Z

@kubernetes/sig-node-pr-reviews, please approve

derekwaynecarr

/lgtm
/approve

Kubelet changes look good

k8s-ci-robot · 2018-02-16T00:05:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cofyc, derekwaynecarr, gnufied, jsafrane

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/kubelet/OWNERS~~ [derekwaynecarr]

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

k8s-github-robot · 2018-02-16T02:04:08Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-02-16T02:44:07Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

…upstream-release-1.8 Automatic merge from submit-queue. Automated cherry pick of #59170: Fix kubelet PVC metrics using a volume stats collector. Cherry pick of #59170 on release-1.8. #59170: Fix kubelet PVC metrics using a volume stats collector.

…upstream-release-1.9 Automatic merge from submit-queue. Automated cherry pick of #59170: Fix kubelet PVC metrics using a volume stats collector. Cherry pick of #59170 on release-1.9. #59170: Fix kubelet PVC metrics using a volume stats collector.

k8s-ci-robot requested review from vishh and yujuhong February 1, 2018 05:57

k8s-ci-robot requested review from gnufied and wongma7 February 1, 2018 05:58

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 1, 2018

cofyc changed the title ~~Fix kubelet PVC metrics using a volume stats collector.~~ Fix kubelet PVC stale metrics Feb 1, 2018

cofyc mentioned this pull request Feb 1, 2018

Prometheus metrics for PVC showing stale volume data #57686

Closed

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 1, 2018

cofyc force-pushed the fix_kubelet_volume_metrics branch from 05aab55 to ff8f139 Compare February 1, 2018 07:44

zhangxiaoyu-zidif reviewed Feb 1, 2018

View reviewed changes

k8s-ci-robot assigned dchen1107 Feb 1, 2018

k8s-ci-robot assigned gnufied and jingxu97 Feb 1, 2018

k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Feb 1, 2018

gnufied reviewed Feb 2, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 9, 2018

cbandy reviewed Feb 11, 2018

View reviewed changes

Fix kubelet PVC metrics using a volume stats collector.

fecff55

Volumes on each node changes, we should not only add PVC metrics into gauge vector. It's better use a collector to collector metrics from stats.

cofyc force-pushed the fix_kubelet_volume_metrics branch from 9502c28 to fecff55 Compare February 11, 2018 15:48

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 11, 2018

k8s-ci-robot assigned jsafrane Feb 12, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 12, 2018

k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Feb 12, 2018

k8s-ci-robot assigned derekwaynecarr Feb 16, 2018

derekwaynecarr approved these changes Feb 16, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 16, 2018

k8s-github-robot merged commit bfdd94c into kubernetes:master Feb 16, 2018

This was referenced Feb 17, 2018

Automated cherry pick of #59170: Fix kubelet PVC metrics using a volume stats collector. #60013

Merged

Automated cherry pick of #59170: Fix kubelet PVC metrics using a volume stats collector. #60014

Merged

cofyc deleted the fix_kubelet_volume_metrics branch March 14, 2018 03:42

jingxu97 mentioned this pull request May 14, 2018

Volume metrics exposed in /stats/summary not available in /metrics #34137

Closed

wgliang mentioned this pull request Jun 19, 2018

volume stats no metrics #65208

Closed

cofyc mentioned this pull request Jun 29, 2018

Vendor github.com/prometheus/client_golang/prometheus/testutil package #65632

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix kubelet PVC stale metrics #59170

Fix kubelet PVC stale metrics #59170

cofyc commented Feb 1, 2018 •

edited

Loading

cofyc commented Feb 1, 2018

zhangxiaoyu-zidif commented Feb 1, 2018

cofyc commented Feb 1, 2018

zhangxiaoyu-zidif Feb 1, 2018

cofyc Feb 1, 2018

cofyc commented Feb 1, 2018

jingxu97 commented Feb 1, 2018

cofyc commented Feb 2, 2018

jingxu97 commented Feb 2, 2018

cofyc commented Feb 2, 2018 •

edited

Loading

gnufied Feb 2, 2018

cofyc Feb 2, 2018 •

edited

Loading

gnufied Feb 2, 2018

cofyc Feb 2, 2018

cofyc commented Feb 9, 2018

jsafrane commented Feb 9, 2018

gnufied commented Feb 9, 2018

rootfs commented Feb 9, 2018

cbandy Feb 11, 2018

cofyc Feb 11, 2018

cbandy Feb 11, 2018

cofyc Feb 11, 2018 •

edited

Loading

jsafrane commented Feb 12, 2018

jsafrane commented Feb 12, 2018

derekwaynecarr left a comment

k8s-ci-robot commented Feb 16, 2018

k8s-github-robot commented Feb 16, 2018

k8s-github-robot commented Feb 16, 2018


		return fmt.Errorf(`metric output does not match expectation; want:
		%s

Fix kubelet PVC stale metrics #59170

Fix kubelet PVC stale metrics #59170

Conversation

cofyc commented Feb 1, 2018 • edited Loading

cofyc commented Feb 1, 2018

zhangxiaoyu-zidif commented Feb 1, 2018

cofyc commented Feb 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cofyc commented Feb 1, 2018

jingxu97 commented Feb 1, 2018

cofyc commented Feb 2, 2018

jingxu97 commented Feb 2, 2018

cofyc commented Feb 2, 2018 • edited Loading

Choose a reason for hiding this comment

cofyc Feb 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cofyc commented Feb 9, 2018

jsafrane commented Feb 9, 2018

gnufied commented Feb 9, 2018

rootfs commented Feb 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cofyc Feb 11, 2018 • edited Loading

Choose a reason for hiding this comment

jsafrane commented Feb 12, 2018

jsafrane commented Feb 12, 2018

derekwaynecarr left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 16, 2018

k8s-github-robot commented Feb 16, 2018

k8s-github-robot commented Feb 16, 2018

cofyc commented Feb 1, 2018 •

edited

Loading

cofyc commented Feb 2, 2018 •

edited

Loading

cofyc Feb 2, 2018 •

edited

Loading

cofyc Feb 11, 2018 •

edited

Loading