Proposal for adding PVC info to VolumeStats #930

vkamra · 2017-08-17T06:59:56Z

Flushes out details for part 1 of the changes described in
#855

Feature: #363

Flushes out details for part 1 of the changes described in #855 Feature: kubernetes/enhancements#363

k8s-ci-robot · 2017-08-17T06:59:59Z

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://github.com/kubernetes/kubernetes/wiki/CLA-FAQ 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: [email protected]

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.

vkamra · 2017-08-17T07:15:56Z

@jingxu97 please review - thanks!

vkamra · 2017-08-17T07:16:27Z

/cc @jingxu97

jingxu97 · 2017-08-25T17:05:34Z

contributors/design-proposals/volume_stats_pvc_ref.md

+- The limitation of this approach is that we're limited to reporting only what is available in the pod spec (Pod namespace and PVC claimname)
+
+### Option 2
+- Modify the ```volumemanager::GetMountedVolumesForPod()``` (or add a new function) to return additional volume information from the ASOW/DSOW caches


Could you use actual and desired state instead of ASOW/DSOW?

@jingxu97 - addressed.

wongma7 · 2017-08-28T17:39:09Z

contributors/design-proposals/volume_stats_pvc_ref.md

+```
+
+## Implementation
+2 options are described below. Option 1 supports current requirements/requested use cases. Option 2 supports an additional use case that was being discussed and is called out for completeness/discussion/feedback.


Agree use case of getting metrics by PV name is iffy, it could be left as the responsibility of the higher-level tool (heapster?) to do that, since it should be trivial for them to find the PVC to which PV is bound.

If not option 2 sounds good to me, the PV name is right there in volume spec, it just needs to be plumbed through GetMountedVolumesForPod.

PV name is not available in volume spec, I think. We have to retrieve through API server to get the PVC object first to get PV name, I think?

from asw.attachedVolumes we can check attachedVolume.spec.PersistentVolume (attachedVolume.spec is volume.Spec https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/plugins.go#L277), so option #2 is possible w/o API call

so I think we have "option #1 + API get" or "option #2" if we want metrics indexed by PV name. I prefer option #2

Agreed - given that we're already populating these caches via API calls - option #2 would be preferred rather than adding API calls in the volume stats collector.

you are right, I got confused. @vkamra I think we could add a separate function to get a list of PV name indexed by outerVolumeSpecName similarly to ListVolumesForPod. Then we can add the PV name information too.

👍 - that's what I have in option 2. We should do that incrementally though if we need to return PV. For now, PVC seems good enough.

wongma7 · 2017-08-28T17:41:17Z

Do we care about exposing these metrics via prometheus? Is there any other case where we expose the same metrics via kubelet summary API and via prometheus directly, or is that unwanted duplication? I am not too familiar with the metrics stack.

wongma7 · 2017-08-28T17:48:31Z

contributors/design-proposals/volume_stats_pvc_ref.md

+- This allows us to get information not available in the Pod spec such as the PV name/UID which can be used to label metrics - enables exposing/querying volume metrics by PV name
+- It's unclear whether this is a use case we need to/should support:
+  * Volume metrics are only refreshed for mounted volumes which implies a bound/available PVC
+  * We expect most user-storage interactions to be via the PVC


One use-case to call out is admins monitoring PVs so that they know when their users are running out of space and they can immediately find the PV's backing storage to resize

vkamra · 2017-08-28T23:05:00Z

@wongma7 - we should expose these metrics via prometheus (/metrics API) as well. @jingxu97 details that in #855.

This proposal (and corresponding implementation PR) just gets PVC information into the volume stats.

wongma7 · 2017-08-28T23:08:16Z

I see, thanks. I would like to see that for 1.8, do you plan to implement it too? Else I can have a try after your PR merges, though I'm not sure where to register those prometheus metrics tbh :)

vkamra · 2017-08-28T23:19:43Z

I've looked at it but haven't had a chance to work on it for 1.8

Prometheus metrics are registered in https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/metrics/metrics.go. After registering them there, we could observe/update those in volume_stat_calculator::calcAndStoreStats()

childsb · 2017-08-30T16:08:19Z

This is for 1.8, correct?

@vkamra i believe the only outstanding change is @wongma7 use case mentioned then we are good to merge.

vkamra · 2017-08-30T17:35:58Z

@childsb - yes, for 1.8. Updated with the use case - should be good to go. The corresponding implementation of this is in PR 51448 also ready to be merged

childsb · 2017-08-30T17:39:54Z

/lgtm

k8s-github-robot · 2017-08-30T17:43:03Z

Automatic merge from submit-queue

Automatic merge from submit-queue Expose PVC metrics via kubelet prometheus This depends on #51448, opening early though. second commit is mine and mostly a copy/paste job. implements metrics listed in here kubernetes/community#855 following method here kubernetes/community#930 (comment) **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: kubernetes/enhancements#363 **Special notes for your reviewer**: **Release note**: ```release-note PersistentVolumeClaim metrics like "volume_stats_inodes" and "volume_stats_capacity_bytes" are now reported via kubelet prometheus ```

Automatic merge from submit-queue Proposal for adding PVC info to VolumeStats Flushes out details for part 1 of the changes described in [kubernetes#855](kubernetes#855) Feature: [kubernetes#363](kubernetes/enhancements#363)

Proposal for adding PVC info to VolumeStats

7196a25

Flushes out details for part 1 of the changes described in #855 Feature: kubernetes/enhancements#363

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Aug 17, 2017

k8s-github-robot assigned calebamiles and idvoretskyi Aug 17, 2017

k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 17, 2017

vkamra closed this Aug 17, 2017

vkamra reopened this Aug 17, 2017

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

k8s-ci-robot requested a review from jingxu97 August 17, 2017 07:16

jingxu97 reviewed Aug 25, 2017

View reviewed changes

Address review feedback

8f90478

wongma7 reviewed Aug 28, 2017

View reviewed changes

wongma7 mentioned this pull request Aug 29, 2017

Expose PVC metrics via kubelet prometheus kubernetes/kubernetes#51553

Merged

Add PV use case

94be197

k8s-ci-robot assigned childsb Aug 30, 2017

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

k8s-github-robot merged commit f6399e1 into kubernetes:master Aug 30, 2017

julio-lopez deleted the pvcname_in_volstats branch September 9, 2019 19:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal for adding PVC info to VolumeStats #930

Proposal for adding PVC info to VolumeStats #930

vkamra commented Aug 17, 2017

k8s-ci-robot commented Aug 17, 2017

vkamra commented Aug 17, 2017

vkamra commented Aug 17, 2017

jingxu97 Aug 25, 2017

vkamra Aug 28, 2017

wongma7 Aug 28, 2017

jingxu97 Aug 28, 2017

wongma7 Aug 28, 2017

vkamra Aug 28, 2017

jingxu97 Aug 28, 2017

vkamra Aug 28, 2017

wongma7 commented Aug 28, 2017

wongma7 Aug 28, 2017

vkamra commented Aug 28, 2017

wongma7 commented Aug 28, 2017

vkamra commented Aug 28, 2017 •

edited

Loading

childsb commented Aug 30, 2017

vkamra commented Aug 30, 2017

childsb commented Aug 30, 2017

k8s-github-robot commented Aug 30, 2017

Proposal for adding PVC info to VolumeStats #930

Proposal for adding PVC info to VolumeStats #930

Conversation

vkamra commented Aug 17, 2017

k8s-ci-robot commented Aug 17, 2017

vkamra commented Aug 17, 2017

vkamra commented Aug 17, 2017

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

wongma7 commented Aug 28, 2017

Choose a reason for hiding this comment

vkamra commented Aug 28, 2017

wongma7 commented Aug 28, 2017

vkamra commented Aug 28, 2017 • edited Loading

childsb commented Aug 30, 2017

vkamra commented Aug 30, 2017

childsb commented Aug 30, 2017

k8s-github-robot commented Aug 30, 2017

vkamra commented Aug 28, 2017 •

edited

Loading