VolumeManager mount/unmount volume metrics #57766

dvonthenen · 2018-01-02T21:00:31Z

What this PR does / why we need it:
Starts to implement VolumeManager storage metrics as defined in the Metrics Spec (https://docs.google.com/document/d/1Fh0T60T_y888LsRwC51CQHO75b2IZ3A34ZQS71s_F0g/edit#heading=h.ys6pjpbasqdu)

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Partial fix kubernetes/enhancements#496

Special notes for your reviewer:
Implements mount/unmount volume metrics only. Will have subsequent PRs for additional metrics.

Release note:

Intended for post-1.9

gnufied · 2018-01-02T21:29:11Z

/ok-to-test

gnufied · 2018-01-02T21:47:02Z

/assign

k8s-ci-robot · 2018-01-02T22:11:00Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dvonthenen
We suggest the following additional approvers: gnufied, vishh

Assign the PR to them by writing /assign @gnufied @vishh in a comment when ready.

Associated issue: #496

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

mlmhl · 2018-01-03T06:36:08Z

pkg/kubelet/volumemanager/cache/desired_state_of_world.go

@@ -258,10 +272,16 @@ func (dsw *desiredStateOfWorld) MarkVolumesReportedInUse(
 }

 func (dsw *desiredStateOfWorld) DeletePodFromVolume(
-	podName types.UniquePodName, volumeName v1.UniqueVolumeName) {
+	podName types.UniquePodName, volumeName v1.UniqueVolumeName, volumeSpec *volume.Spec) {


Is this volumeSpec parameter necessary here? Maybe we can get it from the podToMount struct, see the code here.

Will look into it, but this is for an unmount operation.

The calling function is grabbing the volumeSpec from podToMount. The issue is that the desired_state_of_world_populator doesnt have access to the volumePluginMgr which is why the plugin lookup happens in DeletePodFromVolume.
https://github.com/kubernetes/kubernetes/pull/57766/files#diff-94e527d26427ceb4f990f05f76952656R246

I'm not sure if I understand what you mean. I mean we can find the podToMount object in here, then we can use podToMount.spec instead of the volumeSpec parameter of DeletePodFromVolume .
The benefit of this approach is we needn't to change the signature of DesiredStateOfWorld.DeletePodFromVolume, which will reduce some work.

I agree with @mlmhl. The volumeSpec is already available in the cache, it doesn't need to be passed in.

Maybe I am misunderstanding something. Will take another look.

My bad... that's what happens when you drop something before a 2 week Christmas break and come back to it afterward. Will have a fix for this.

mlmhl · 2018-01-03T06:43:40Z

pkg/kubelet/volumemanager/cache/desired_state_of_world.go

@@ -278,6 +298,9 @@ func (dsw *desiredStateOfWorld) DeletePodFromVolume(
 		// Delete volume if no child pods left
 		delete(dsw.volumesToMount, volumeName)
 	}
+
+	// create metric for volume unmount
+	dsw.volumesToUnmount[volumeName] = metrics.VolumeManagerOperationCompleteHook(volumePluginName, "volume_unmount")


Do we need to add pod name as a dimension? As this umount operation is just umount volume from pod, not umount the volume device from node.

We generally never want to add pod names as a dimension because in a large enough cluster, it will cause blowing up of cardinality of metrics

we need to make sure that - we aren't leaking memory from volumesToUnmount map. Are we ever removing elements from this map? I don't see it.

Yup, will update and perform the delete.

You can find the delete here:
https://github.com/kubernetes/kubernetes/pull/57766/files#diff-d905eaaa258ac0c788ec26e31079ce60R371

Delete from the map just prior to invoking the callback

mlmhl · 2018-01-03T06:56:59Z

Hi @gnufied , I have doubts about the definition of mount/unmount time, as we have two mount/umount operations: mount/umount the volume device to/from the node, and mount/umount the volume to/from a specific pod. Do we need to add metrics for each operation, or just use a same metric?

gnufied · 2018-01-05T17:22:30Z

pkg/kubelet/volumemanager/cache/desired_state_of_world.go

@@ -278,6 +298,9 @@ func (dsw *desiredStateOfWorld) DeletePodFromVolume(
 		// Delete volume if no child pods left
 		delete(dsw.volumesToMount, volumeName)
 	}
+
+	// create metric for volume unmount
+	dsw.volumesToUnmount[volumeName] = metrics.VolumeManagerOperationCompleteHook(volumePluginName, "volume_unmount")


We generally never want to add pod names as a dimension because in a large enough cluster, it will cause blowing up of cardinality of metrics

gnufied · 2018-01-05T17:24:10Z

pkg/kubelet/volumemanager/cache/desired_state_of_world.go

 	dsw.Lock()
 	defer dsw.Unlock()

+	volumePluginName := "plugin_unknown"


If a value is unknown the convention is to use <n/a>

Will update to change the default.

gnufied · 2018-01-05T17:36:14Z

pkg/kubelet/volumemanager/cache/desired_state_of_world.go

 		}
 	}
 	return volumesToMount
 }

+func (dsw *desiredStateOfWorld) GetVolumeUnmountMetric(volumeName v1.UniqueVolumeName) func(error) {


I think we should rename this function because it does not exactly return the metric itself, but it returns a callback which can be called to emit the metric.

gnufied · 2018-01-05T17:41:56Z

@mlmhl yeah I think this PR only captures mount volume and umount volume... which is fine. mount volume and mount device operation is in particular tricky because - both operations happen inside one function.

We can probably refactor this PR to report umount_volume and umount_device metrics though, but we can do that in a follow up PR too.

gnufied · 2018-01-05T17:53:27Z

W0105 17:48:42.079] 2018/01/05 17:48:41 error running compiler: exit status 1
I0105 17:48:42.180] pkg/kubelet/volumemanager/cache/desired_state_of_world_test.go:157:25: not enough arguments in call to dsw.DeletePodFromVolume
I0105 17:48:42.181] 	have ("k8s.io/kubernetes/pkg/volume/util/types".UniquePodName, "k8s.io/api/core/v1".UniqueVolumeName)
I0105 17:48:42.182] 	want ("k8s.io/kubernetes/pkg/volume/util/types".UniquePodName, "k8s.io/api/core/v1".UniqueVolumeName, *volume.Spec)
I0105 17:48:42.224] //cmd/cloud-controller-manager/app/options:go_default_test            NO STATUS
I0105 17:48:42.225] //cmd/genkubedocs:go_default_test

@dvonthenen you probably need to fix that.

verult · 2018-01-05T19:51:12Z

pkg/kubelet/metrics/metrics.go

@@ -47,6 +47,9 @@ const (
 	// Metrics keys of device plugin operations
 	DevicePluginRegistrationCountKey = "device_plugin_registration_count"
 	DevicePluginAllocationLatencyKey = "device_plugin_alloc_latency_microseconds"
+	// Metrics keys of volume operations
+	VolumeManagerOpeationsKey      = "volumemanager_operation_duration_seconds"


verult · 2018-01-05T20:55:33Z

pkg/kubelet/metrics/metrics.go

@@ -283,3 +304,18 @@ func (pc *podAndContainerCollector) Collect(ch chan<- prometheus.Metric) {
 		prometheus.GaugeValue,
 		float64(runningContainers))
 }
+
+// VolumeManagerOperationCompleteHook returns a hook to call when an operation is completed
+func VolumeManagerOperationCompleteHook(plugin, operationName string) func(error) {


Could the OperationCompleteHook in pkg/volume/util/metrics.go be reused?

The callback in pkg/volume/util/metrics.go represent the volume operation only. These metrics are at the volume manager view/perspective. We don't want to put both sets of data into the same bucket.

verult · 2018-01-05T20:57:57Z

pkg/kubelet/volumemanager/cache/desired_state_of_world.go

@@ -120,6 +126,9 @@ type desiredStateOfWorld struct {
 	// the map is the name of the volume and the value is a volume object
 	// containing more information about the volume.
 	volumesToMount map[v1.UniqueVolumeName]volumeToMount
+	// volumesToUnmount is a map containing callback functions to record metrics
+	// for a given Volume's unmount operation
+	volumesToUnmount map[v1.UniqueVolumeName]func(error)


Use a different name? The map values aren't exactly volumes. Or the function can be wrapped in a volumeToUnmount struct

Yeah I agree. may be call this - unmountCompletionCallbackMap ?

verult · 2018-01-05T20:58:38Z

pkg/kubelet/volumemanager/cache/desired_state_of_world.go

@@ -94,6 +95,10 @@ type DesiredStateOfWorld interface {
 	// current desired state of the world.
 	GetVolumesToMount() []VolumeToMount

+	// GetVolumeUnmountMetricCallback retreives the associated metric callback for a given


gnufied · 2018-01-05T21:38:51Z

pkg/kubelet/volumemanager/cache/desired_state_of_world.go

+func (dsw *desiredStateOfWorld) GetVolumeUnmountMetricCallback(volumeName v1.UniqueVolumeName) func(error) {
+	operationComplete := dsw.volumesToUnmount[volumeName]
+	delete(dsw.volumesToUnmount, volumeName)
+	return operationComplete


Don't we need to protect this with mutex?

Yup. Will add.

gnufied · 2018-01-09T16:46:22Z

Looks like failure is from:

W0109 16:21:37.006] 2018/01/09 16:21:36 error running compiler: exit status 1
I0109 16:21:37.107] pkg/kubelet/volumemanager/reconciler/reconciler_test.go:334:25: not enough arguments in call to dsw.DeletePodFromVolume
I0109 16:21:37.108] 	have ("k8s.io/kubernetes/pkg/volume/util/types".UniquePodName, "k8s.io/api/core/v1".UniqueVolumeName)
I0109 16:21:37.108] 	want ("k8s.io/kubernetes/pkg/volume/util/types".UniquePodName, "k8s.io/api/core/v1".UniqueVolumeName, *volume.Spec)
I0109 16:21:37.109] pkg/kubelet/volumemanager/reconciler/reconciler_test.go:426:25: not enough arguments in call to dsw.DeletePodFromVolume
I0109 16:21:37.110] 	have ("k8s.io/kubernetes/pkg/volume/util/types".UniquePodName, "k8s.io/api/core/v1".UniqueVolumeName)
I0109 16:21:37.110] 	want ("k8s.io/kubernetes/pkg/volume/util/types".UniquePodName, "k8s.io/api/core/v1".UniqueVolumeName, *volume.Spec)
I0109 16:21:37.111] pkg/kubelet/volumemanager/reconciler/reconciler_test.go:704:25: not enough arguments in call to dsw.DeletePodFromVolume
I0109 16:21:37.111] 	have ("k8s.io/kubernetes/pkg/volume/util/types".UniquePodName, "k8s.io/api/core/v1".UniqueVolumeName)
I0109 16:21:37.112] 	want ("k8s.io/kubernetes/pkg/volume/util/types".UniquePodName, "k8s.io/api/core/v1".UniqueVolumeName, *volume.Spec)
I0109 16:21:37.112] pkg/kubelet/volumemanager/reconciler/reconciler_test.go:808:25: not enough arguments in call to dsw.DeletePodFromVolume
I0109 16:21:37.113] 	have ("k8s.io/kubernetes/pkg/volume/util/types".UniquePodName, "k8s.io/api/core/v1".UniqueVolumeName)
I0109 16:21:37.113] 	want ("k8s.io/kubernetes/pkg/volume/util/types".UniquePodName, "k8s.io/api/core/v1".UniqueVolumeName, *volume.Spec)
I0109 16:21:37.129] //cmd/cloud-controller-manager/app/options:go_default_test            NO STATUS
I0109 16:21:37.131] //cmd/genkubedocs:go_default_test                                     NO STATUS
I0109 16:21:37.131] //cmd/gke-certificates-controller/app:go_default_test                 NO STATUS
I0109 16:21:37.131] //cmd/hyperkube:go_default_test                                       NO STATUS
I0109 16:21:37.132] //cmd/kube-apiserver/app/options:go_default_test                      NO STATUS

Can you check the test logs? https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/57766/pull-kubernetes-bazel-test/24789/ ?

saad-ali · 2018-01-10T01:24:32Z

pkg/kubelet/volumemanager/cache/desired_state_of_world.go

@@ -258,10 +272,16 @@ func (dsw *desiredStateOfWorld) MarkVolumesReportedInUse(
 }

 func (dsw *desiredStateOfWorld) DeletePodFromVolume(
-	podName types.UniquePodName, volumeName v1.UniqueVolumeName) {
+	podName types.UniquePodName, volumeName v1.UniqueVolumeName, volumeSpec *volume.Spec) {


I agree with @mlmhl. The volumeSpec is already available in the cache, it doesn't need to be passed in.

saad-ali · 2018-01-10T01:30:23Z

pkg/kubelet/volumemanager/cache/desired_state_of_world.go

@@ -278,6 +298,9 @@ func (dsw *desiredStateOfWorld) DeletePodFromVolume(
 		// Delete volume if no child pods left
 		delete(dsw.volumesToMount, volumeName)
 	}
+
+	// create metric for volume unmount
+	dsw.unmountCompletionCallbackMap[volumeName] = metrics.VolumeManagerOperationCompleteHook(volumePluginName, "volume_unmount")


This is not the right place to add metrics. This code is responsible for updating internal state. Which is one step of the operation. We should be adding metrics at the operation executor layer.

@davidz627 recently added common event logic and did a good job of factoring. His code may be worth a look. Will add him as a reviewer on this PR.

Thanks for the feedback. I am not familiar with the event logic functionality. Will dig into it.

You can take a look at #56872, it touches most of the places where events and metrics are being generated in the operation executor.

Specifically : https://github.com/kubernetes/kubernetes/pull/56872/files#diff-450e811a4953f760ff1594ede8b2037eR344 if you take a look at the "CompleteFunc's" they handle some sort of metric generation already, maybe you can augment those.

This is bit tricky. @davidz627 the operation hooks you linked are used for tracking operation level metrics once operation has already started.

This PR is for metrics that try to cover entirety of mount or umount operation (please refer to linked Google Doc for more details). Having a operation hook on umount on its own is not useful, we need a way to store that hook at the time when volume is removed from desired_state_of_world and we need to call the hook when umount actually finishes. The goal of these metrics is to measure - total time spent inside these controllers.

The only other thing that I could/might do is wrap the callback in a volumesToUnmount struct in the event there is more information/state we need to carry forward for other operations then rename unmountCompletionCallbackMap map[v1.UniqueVolumeName]func(error) to something like volumesToUnmount map[v1.UniqueVolumeName]volumeToUnmount. I just started with the basic/simple case (just the callback itself) until we actually need to carry more state forward.

@gnufied @davidz627 @saad-ali Please advise on how you want to move forward.

Thanks for the explanation @gnufied. Now that I understand the use cases better, I understand why the code is structured this way. So next question: is it worth the added complexity in the code (and potential bugs; another data structure, unmountCompletionCallbackMap, that may get out of sync) to add this metric?

k8s-github-robot · 2018-02-13T08:30:42Z

@dvonthenen PR needs rebase

fejta-bot · 2018-05-14T08:33:51Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

k8s-ci-robot · 2018-05-14T08:34:00Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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. I understand the commands that are listed here.

fejta-bot · 2018-06-13T09:20:26Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten
/remove-lifecycle stale

fejta-bot · 2018-07-13T10:06:11Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

k8s-github-robot assigned verult and rootfs Jan 2, 2018

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 Jan 2, 2018

k8s-ci-robot assigned gnufied Jan 2, 2018

dvonthenen force-pushed the feature/mountmetrics branch from 803d67b to 84632d7 Compare January 2, 2018 22:10

mlmhl reviewed Jan 3, 2018

View reviewed changes

gnufied reviewed Jan 5, 2018

View reviewed changes

verult reviewed Jan 5, 2018

View reviewed changes

gnufied reviewed Jan 5, 2018

View reviewed changes

dvonthenen mentioned this pull request Jan 9, 2018

Implement Two Volume Metrics in Kubernetes thecodeteam/roadmap#217

Open

saad-ali added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jan 10, 2018

saad-ali suggested changes Jan 10, 2018

View reviewed changes

saad-ali assigned davidz627 Jan 10, 2018

dvonthenen closed this Jan 11, 2018

dvonthenen added 2 commits January 11, 2018 08:11

Initial check-in for mount/unmount volume metrics

abf8d3c

Add kubelet/metrics path to BUILD file

baf8260

dvonthenen added 10 commits January 11, 2018 08:11

Update with correct dep package

95972bd

Add to go test BUILD

37b0846

Update after running ./hack/update-bazel.sh

9a6a4d5

Updates based on feedback

6e66bd5

Updates based on additional feedback

6e88922

Fix call function name

bc7a8ec

Fix by reference

995a44d

Revert change

9612cb0

Update the go test for reconciler

8592faa

Remove volumeSpec from the DeletePodFromVolume function signature

232c156

dvonthenen reopened this Jan 11, 2018

dvonthenen force-pushed the feature/mountmetrics branch from 8452301 to 232c156 Compare January 11, 2018 16:38

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 13, 2018

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 14, 2018

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 14, 2018

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jun 13, 2018

k8s-ci-robot closed this Jul 13, 2018

msau42 mentioned this pull request Feb 12, 2019

Add storage scale testing kep kubernetes/enhancements#812

Merged

VolumeManager mount/unmount volume metrics #57766

VolumeManager mount/unmount volume metrics #57766

Conversation

dvonthenen commented Jan 2, 2018 • edited

gnufied commented Jan 2, 2018

gnufied commented Jan 2, 2018

k8s-ci-robot commented Jan 2, 2018

Choose a reason for hiding this comment

dvonthenen Jan 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvonthenen Jan 5, 2018 • edited

Choose a reason for hiding this comment

mlmhl commented Jan 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnufied commented Jan 5, 2018

gnufied commented Jan 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnufied Jan 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnufied commented Jan 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-github-robot commented Feb 13, 2018

fejta-bot commented May 14, 2018

k8s-ci-robot commented May 14, 2018

fejta-bot commented Jun 13, 2018

fejta-bot commented Jul 13, 2018

dvonthenen commented Jan 2, 2018 •

edited

dvonthenen Jan 5, 2018 •

edited

dvonthenen Jan 5, 2018 •

edited

gnufied Jan 6, 2018 •

edited