Add PVCRef to VolumeStats by vkamra · Pull Request #51448 · kubernetes/kubernetes

vkamra · 2017-08-28T06:50:54Z

What this PR does / why we need it:
For pod volumes that reference a PVC, add a PVCRef to the corresponding
volume stat. This allows metrics to be indexed/queried by PVC name
which is more user-friendly than Pod reference

Which issue this PR fixes : #363

Special notes for your reviewer:

Release note:

`VolumeStats` reported by the kubelet stats summary API 
(http://<node>:10255/stats/summary) now include a PVCRef
field describing the PVC referenced by the volume (if any).

k8s-ci-robot · 2017-08-28T06:51:02Z

Hi @vkamra. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Details

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-28T06:51:36Z

/cc @jingxu97

xiangpengzhao · 2017-08-28T07:39:14Z

/ok-to-test

wongma7 · 2017-08-28T15:07:05Z

/sig storage

wongma7 · 2017-08-28T18:25:59Z

The issue is wrong, should link to kubernetes/features. also needs hack/update-bazel.sh to pass verify 👍

jingxu97 · 2017-08-28T21:50:34Z

pkg/kubelet/server/stats/volume_stat_calculator_test.go

@@ -0,0 +1,141 @@
+/*
+Copyright 2016 The Kubernetes Authors.


jingxu97 · 2017-08-28T21:53:22Z

Do you have manual test for see whether the stats are working?

vkamra · 2017-08-28T22:10:10Z

@wongma7 - will do - thanks

vkamra · 2017-08-28T22:21:17Z

@jingxu97 - have tested manually (output below for a stateful set pod)

`     {
      "time": "2017-08-28T22:18:26Z",
      "availableBytes": 876806144,
      "capacityBytes": 1023303680,
      "usedBytes": 76034048,
      "inodesFree": 65507,
      "inodes": 65536,
      "inodesUsed": 29,
      "name": "datadir",
      "pvcRef": {
       "name": "datadir-cockroachdb-0",
       "namespace": "default"
      }`

jingxu97 · 2017-08-28T22:44:47Z

Could you also add a release note in the commit? Thanks!

jingxu97 · 2017-08-28T22:46:34Z

cc @vkamra @ddysher @xingzhou @krmayankk

vkamra · 2017-08-28T22:55:00Z

@jingxu97 - done

vkamra · 2017-08-28T23:57:14Z

/retest

jingxu97 · 2017-08-29T17:46:54Z

pkg/kubelet/server/stats/volume_stat_calculator.go

 	return stats.VolumeStats{
-		Name: podName,
+		Name:   podName,
+		PVCRef: pvcRef,


I am thinking to check pvcRef is nil or not here. If it is nil, return stats.VolumeStats without PVCRef field

the result will be the same since PVCRef will default to nil whether we explicitly set PVCRef: nil, or not at all

Is this needed - the behavior would be the same i.e VolumeStats.PVCRef will be nil in both cases?

ok, I am hoping if PVCRef is nil, the printed result of volumestat will not show this field, instead of showing PVCRef: nil?

👍 - it won't. It's tagged optional/omit-empty.

jingxu97 · 2017-08-29T17:47:41Z

cc @derekwaynecarr @Random-Liu could you please help approve this PR? Thanks!

vkamra · 2017-08-29T18:08:59Z

@jingxu97 @wongma7 - any idea what to do about the test failures above. They appear unrelated/flakes - would you mind taking a quick look to see if you agree?

vkamra · 2017-08-29T18:16:50Z

/retest

wongma7 · 2017-08-29T18:16:59Z

yeah it looks like flakes to me, gpu is currently flaky and "TestCRD" looks like it's not just us

vkamra · 2017-08-29T20:38:45Z

/retest

jingxu97 · 2017-08-29T21:13:58Z

@vkamra, you don't need to run /retest which run all the tests again. Just the tests that failed, like
"/test pull-kubernetes-e2e-gce-bazel"

Also for verify test, you can try to run locally with hack/verify-bazel.sh

jingxu97 · 2017-08-29T22:19:33Z

/lgtm

jingxu97 · 2017-08-29T22:31:32Z

cc @tallclair Could you help approve this PR?

vkamra · 2017-08-29T22:35:52Z

/test pull-kubernetes-e2e-gce-etcd3

For pod volumes that reference a PVC, add a PVCRef to the corresponding volume stat. This allows metrics to be indexed/queried by PVC name which is more user-friendly than Pod reference

xiangpengzhao · 2017-08-30T06:16:41Z

you don't need to run /retest which run all the tests again. Just the tests that failed, like
"/test pull-kubernetes-e2e-gce-bazel"

@jingxu97 fyi: /retest only runs the failed test(s) again, it doesn't re-run all the tests :)

wongma7 · 2017-08-30T14:20:48Z

/retest

jingxu97 · 2017-08-30T17:07:02Z

@xiangpengzhao thanks for the information! Good to know that.

derekwaynecarr · 2017-08-30T18:16:20Z

/approve

k8s-github-robot · 2017-08-30T18:17:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, jingxu97, vkamra

Associated issue: 363

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

Details

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

~~pkg/kubelet/OWNERS~~ [derekwaynecarr]
~~test/e2e_node/OWNERS~~ [derekwaynecarr]

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

k8s-github-robot · 2017-09-01T05:09:18Z

Automatic merge from submit-queue (batch tested with PRs 51513, 51515, 50570, 51482, 51448)

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 ```

For pod volumes that reference a PVC, add a PVCRef to the corresponding volume stat. This allows metrics to be indexed/queried by PVC name which is more user-friendly than Pod reference See kubernetes#51448.

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 28, 2017

k8s-github-robot assigned tallclair and feiskyer Aug 28, 2017

k8s-github-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Aug 28, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 28, 2017

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 Aug 28, 2017

k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Aug 28, 2017

jingxu97 self-requested a review August 28, 2017 21:46

jingxu97 reviewed Aug 28, 2017

View reviewed changes

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 29, 2017

jingxu97 reviewed Aug 29, 2017

View reviewed changes

wongma7 mentioned this pull request Aug 29, 2017

Expose PVC metrics via kubelet prometheus #51553

Merged

k8s-ci-robot assigned jingxu97 Aug 29, 2017

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

Add PVCRef to VolumeStats

1ac56d8

For pod volumes that reference a PVC, add a PVCRef to the corresponding volume stat. This allows metrics to be indexed/queried by PVC name which is more user-friendly than Pod reference

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

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

vkamra mentioned this pull request Aug 30, 2017

Proposal for adding PVC info to VolumeStats kubernetes/community#930

Merged

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 30, 2017

k8s-github-robot merged commit 17dffc1 into kubernetes:master Sep 1, 2017

jingxu97 mentioned this pull request Oct 13, 2017

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

Closed

morganchristiansson mentioned this pull request Apr 30, 2018

Add pvc name and namespace to kubernetes volume metricset elastic/beats#6977

Closed

julio-lopez deleted the pvc_ref_volstats branch October 9, 2020 18:39

Conversation

vkamra commented Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Aug 28, 2017

Uh oh!

vkamra commented Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xiangpengzhao commented Aug 28, 2017

Uh oh!

wongma7 commented Aug 28, 2017

Uh oh!

wongma7 commented Aug 28, 2017

Uh oh!

jingxu97 Aug 28, 2017

Choose a reason for hiding this comment

Uh oh!

jingxu97 commented Aug 28, 2017

Uh oh!

vkamra commented Aug 28, 2017

Uh oh!

vkamra commented Aug 28, 2017 • edited by jingxu97 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jingxu97 commented Aug 28, 2017

Uh oh!

jingxu97 commented Aug 28, 2017

Uh oh!

vkamra commented Aug 28, 2017

Uh oh!

vkamra commented Aug 28, 2017

Uh oh!

jingxu97 Aug 29, 2017

Choose a reason for hiding this comment

Uh oh!

wongma7 Aug 29, 2017

Choose a reason for hiding this comment

Uh oh!

vkamra Aug 29, 2017

Choose a reason for hiding this comment

Uh oh!

jingxu97 Aug 29, 2017

Choose a reason for hiding this comment

Uh oh!

vkamra Aug 29, 2017

Choose a reason for hiding this comment

Uh oh!

jingxu97 commented Aug 29, 2017

Uh oh!

vkamra commented Aug 29, 2017

Uh oh!

vkamra commented Aug 29, 2017

Uh oh!

wongma7 commented Aug 29, 2017

Uh oh!

vkamra commented Aug 29, 2017

Uh oh!

jingxu97 commented Aug 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jingxu97 commented Aug 29, 2017

Uh oh!

jingxu97 commented Aug 29, 2017

Uh oh!

vkamra commented Aug 29, 2017

Uh oh!

xiangpengzhao commented Aug 30, 2017

Uh oh!

wongma7 commented Aug 30, 2017

Uh oh!

jingxu97 commented Aug 30, 2017

Uh oh!

derekwaynecarr commented Aug 30, 2017

Uh oh!

k8s-github-robot commented Aug 30, 2017

Uh oh!

k8s-github-robot commented Sep 1, 2017

Uh oh!

Reviewers

vkamra commented Aug 28, 2017 •

edited

Loading

vkamra commented Aug 28, 2017 •

edited

Loading

vkamra commented Aug 28, 2017 •

edited by jingxu97

Loading

jingxu97 commented Aug 29, 2017 •

edited

Loading