New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add attachable pvc in use metrics #64527

Merged

k8s-github-robot merged 1 commit into kubernetes:master from gnufied:add-pvc-in-use-metrics

Jun 28, 2018

Member

gnufied commented May 30, 2018 •

edited

Loading

This metric reports number of PVCs that are in-use in Kubernetes with plugin and node name as dimensions.

This allows us to figure out, how many PVCs each node is using. It is super helpful in figuring out attach/detach issues.

/sig storage

cc @jsafrane @tsmetana @msau42

Add metrics for PVC in-use

k8s-ci-robot added sig/storage size/L do-not-merge/release-note-label-needed cncf-cla: yes labels

k8s-ci-robot requested review from msau42 and verult

May 30, 2018 19:23

gnufied force-pushed the add-pvc-in-use-metrics branch from d2d5811 to abd3470 Compare

May 30, 2018 19:23

k8s-ci-robot added release-note and removed do-not-merge/release-note-label-needed labels

gnufied force-pushed the add-pvc-in-use-metrics branch from abd3470 to c365c99 Compare

May 30, 2018 19:34

verult reviewed

View reviewed changes

Contributor

verult left a comment

Just a quick initial pass

pkg/controller/volume/attachdetach/metrics/metrics.go Outdated

    
              }

              type nodePVCCount struct {

              	pvcCount map[types.NodeName]map[string]int

Contributor

verult Jun 1, 2018

To reduce a nested layer, what about this:

type nodePVCCount map[types.NodeName]map[string]int
...
func (pvcInUse nodePVCCount) add(...) {...}
...
nodePVCMap := make(nodePVCCount)

also nit: type PluginName string for better readability

pkg/controller/volume/attachdetach/metrics/metrics.go Outdated

    
              	}

              	if pvc.Status.Phase != v1.ClaimBound || pvc.Spec.VolumeName == "" {

              		return nil, fmt.Errorf(

              			"PVC %s has non-bound phase (%q) or empty pvc.Spec.VolumeName (%q)",

Contributor

verult Jun 1, 2018

nit: Error message could be confusing if only one of the conditions evaluate to true. Split up the error?

Member Author

gnufied Jun 5, 2018 •

edited

Loading

I am going to leave this as it is for now. I have not even considered logging these errors tbh, so whatever we return from here - simply gets ignored.

gnufied force-pushed the add-pvc-in-use-metrics branch from c365c99 to 21fbd9f Compare

June 5, 2018 18:51

msau42 reviewed

View reviewed changes

pkg/controller/volume/attachdetach/metrics/metrics.go Outdated

    
              	volumePluginMgr *volume.VolumePluginMgr

              }

              type nodePVCCount map[types.NodeName]map[string]int

Member

msau42 Jun 5, 2018

Can you add a comment here describe what each of the types represents

Member Author

gnufied Jun 7, 2018

fixed

pkg/controller/volume/attachdetach/metrics/metrics.go Outdated

    
              				glog.V(3).Infof("Error finding volume plugin for : %v", volumeSpec)

              				continue

              			}

              			nodePVCMap.add(nodeName, volumePlugin.GetPluginName())

Member

msau42 Jun 5, 2018

Is this to help with the attachable limit count? If so, do we want to use the attachable limit resource name instead of the plugin name? For CSI, I think this just returns "kubernetes.io/csi"

Member Author

gnufied Jun 5, 2018

This is not just for attachable limit count. It is useful for capacity planning in multi-tenant clusters and perhaps there are other usage too. In a multi-tenant cluster one typically has many more PV/PVCs than actually in-use PVCs. This metric obviously helps in determining if a cluster admin needs to spin up new nodes to accomodate in-use PVCs that takes into account both attachable limit or limits that apply in general to volume types that we don't even consider "attachable" right now.

I agree with unfortunate situation with CSI plugin name and GetPluginName is not a satisfactory solution. But all storage metrics are affected by this problem, because they all use GetPluginName as a label. I have filed #64590 to solve this. I think we are going to need additional function call or something for CSI plugins.

gnufied force-pushed the add-pvc-in-use-metrics branch from 21fbd9f to 2c47de3 Compare

June 6, 2018 14:58

msau42 reviewed

View reviewed changes

pkg/controller/volume/attachdetach/metrics/metrics.go

    
              		if nodeName == "" {

              			continue

              		}

              		for _, podVolume := range pod.Spec.Volumes {

Member

msau42 Jun 6, 2018

So this is actually just counting pods that have been scheduled to a node, but not necessarily attached. Is that what we want?

Member Author

gnufied Jun 6, 2018

Yeah for now I think - this should be fine. We may need separate metrics for actually attached volumes. But that requires querying cloudprovider or going through volume plugin at very minimum. I am still thinking, how to implement that interface so as it can be useful to most volume plugins.

jsafrane reviewed

View reviewed changes

pkg/controller/volume/attachdetach/metrics/metrics.go Outdated

    
              }

              func (collector *pvcInUseCollector) CreateVolumeSpec(podVolume v1.Volume, namespace string) (*volume.Spec, error) {

              	pvcSource := podVolume.VolumeSource.PersistentVolumeClaim

Member

jsafrane Jun 12, 2018

I think it would be useful to include also inline volumes in pods and not just PVCs. It would bring broader (and more correct) picture.

Member Author

gnufied Jun 15, 2018 •

edited

Loading

hrm, I overlooked something. turns out A/D controller only initializes attachable plugins (and similarly pv controller only initializes provisionable plugins) and hence it is pretty hard to emit a metric for all volumes in use without initiaizing all volume plugins in control plane.

The alternatives are:

Emit these metrics from kubelet. But the downside of that is, any unresponsive node could cause metrics to be incorrect.
Somehow find a way of initializing all plugins in control plane.

Still thinking how to workaround that....

Member Author

gnufied Jun 15, 2018

for now I have just renamed the metric and added support for inline attachable volumes too. But yet to think how to truly report ALL volumes... not just attachable types.

bertinatto reviewed

View reviewed changes

pkg/controller/volume/attachdetach/metrics/metrics_test.go Outdated

    
              	metricCollector := newPVCInUseCollector(pvcLister, fakePodInformer.Lister(), pvLister, fakeVolumePluginMgr)

              	nodeUseMap := metricCollector.getPVCUseByNode()

              	if len(nodeUseMap) < 1 {

              		t.Errorf("Expected one pvc in use got %d", len(nodeUseMap))

Member

bertinatto Jun 14, 2018

One or at least one?

gnufied force-pushed the add-pvc-in-use-metrics branch 2 times, most recently from ee2c689 to e11b8af Compare

June 15, 2018 19:12

Member

jsafrane commented Jun 28, 2018

/lgtm

k8s-ci-robot assigned jsafrane

k8s-ci-robot added lgtm approved labels

Member Author

gnufied commented Jun 28, 2018

/test pull-kubernetes-verify

gnufied changed the title ~~Add pvc in use metrics~~ Add attachable pvc in use metrics


          Add metrics for attachable volumes in use

8d46912

gnufied force-pushed the add-pvc-in-use-metrics branch from e11b8af to 8d46912 Compare

June 28, 2018 15:51

k8s-ci-robot removed the lgtm label

Member Author

gnufied commented Jun 28, 2018

@jsafrane can you lgtm this again? I had tor rebase this (with master) to resolve some bazel/verify failures.

Member

jsafrane commented Jun 28, 2018

/lgtm

k8s-ci-robot added the lgtm label

Contributor

k8s-ci-robot commented Jun 28, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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/controller/volume/attachdetach/OWNERS~~ [jsafrane]

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

Member Author

gnufied commented Jun 28, 2018

/test pull-kubernetes-e2e-kops-aws

k8s-github-robot commented Jun 28, 2018

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

Contributor

k8s-ci-robot commented Jun 28, 2018 •

edited

Loading

@gnufied: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-bazel-test	`8d46912`	link	`/test pull-kubernetes-bazel-test`
pull-kubernetes-e2e-gce	`8d46912`	link	`/test pull-kubernetes-e2e-gce`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

k8s-github-robot commented Jun 28, 2018

Automatic merge from submit-queue (batch tested with PRs 65361, 64527). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-github-robot merged commit a68a909 into kubernetes:master

wongma7 mentioned this pull request

Remove crappy fmt.Println #65911

Merged

k8s-github-robot pushed a commit that referenced this pull request


          Merge pull request #65911 from wongma7/crap

4bb3a2a

Automatic merge from submit-queue (batch tested with PRs 63194, 65911). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Remove crappy fmt.Println

Remove @gnufied's debug message #64527
```release-note
NONE
```

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved cncf-cla: yes lgtm release-note sig/storage size/L