[cinder-csi-plugin] NodeGetVolumeStats() implementation #941

zuzzas · 2020-02-19T08:49:14Z

The binaries affected:

What this PR does / why we need it:
Knowing Volume capabilities helps to sleep at night (also, capacity planning).

Which issue this PR fixes:
fixes #669

Special notes for reviewers:

The test is ugly right now. Should I mock a resizefs call?

Release note:

cinder-csi-plugin: Added support for GET_VOLUME_STATS node capability

This change is

k8s-ci-robot · 2020-02-19T08:49:22Z

Hi @zuzzas. 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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

theopenlab-ci · 2020-02-19T08:53:09Z

Build succeeded.

cloud-provider-openstack-format : SUCCESS in 3m 21s

theopenlab-ci · 2020-02-19T08:57:44Z

Build succeeded.

cloud-provider-openstack-unittest : SUCCESS in 7m 49s

jichenjc · 2020-02-19T09:01:46Z

/ok-to-test

theopenlab-ci · 2020-02-19T09:02:29Z

Build succeeded.

cloud-provider-openstack-acceptance-test-k8s-cinder : SUCCESS in 11m 14s

theopenlab-ci · 2020-02-19T09:03:18Z

Build succeeded.

cloud-provider-openstack-acceptance-test-csi-cinder : SUCCESS in 12m 26s

theopenlab-ci · 2020-02-19T09:14:44Z

Build succeeded.

cloud-provider-openstack-e2e-test-csi-cinder : SUCCESS in 22m 57s

theopenlab-ci · 2020-02-19T09:16:15Z

Build succeeded.

cloud-provider-openstack-acceptance-test-keystone-authentication-authorization : SUCCESS in 26m 17s

theopenlab-ci · 2020-02-19T09:43:40Z

Build succeeded.

cloud-provider-openstack-acceptance-test-flexvolume-cinder : SUCCESS in 53m 28s

theopenlab-ci · 2020-02-19T10:12:30Z

Build succeeded.

cloud-provider-openstack-format : SUCCESS in 3m 21s

theopenlab-ci · 2020-02-19T10:15:38Z

Build succeeded.

cloud-provider-openstack-unittest : SUCCESS in 6m 33s

theopenlab-ci · 2020-02-19T10:20:24Z

Build succeeded.

cloud-provider-openstack-acceptance-test-k8s-cinder : SUCCESS in 10m 31s

theopenlab-ci · 2020-02-19T10:21:07Z

Build succeeded.

cloud-provider-openstack-acceptance-test-csi-cinder : SUCCESS in 11m 11s

theopenlab-ci · 2020-02-19T10:36:05Z

Build succeeded.

cloud-provider-openstack-e2e-test-csi-cinder : SUCCESS in 26m 06s

theopenlab-ci · 2020-02-19T10:43:14Z

Build succeeded.

cloud-provider-openstack-acceptance-test-flexvolume-cinder : SUCCESS in 33m 43s

theopenlab-ci · 2020-02-19T10:49:32Z

Build succeeded.

cloud-provider-openstack-acceptance-test-keystone-authentication-authorization : SUCCESS in 40m 17s

lingxiankong · 2020-02-22T08:42:19Z

@ramineni please review this PR when you get time, thanks

ramineni

@zuzzas Thanks for the PR. Some initial comments inline

ramineni · 2020-02-25T04:55:46Z

pkg/csi/cinder/nodeserver.go

-func (ns *nodeServer) NodeGetVolumeStats(ctx context.Context, req *csi.NodeGetVolumeStatsRequest) (*csi.NodeGetVolumeStatsResponse, error) {
-	return nil, status.Error(codes.Unimplemented, fmt.Sprintf("NodeGetVolumeStats is not yet implemented"))
+func (ns *nodeServer) NodeGetVolumeStats(_ context.Context, req *csi.NodeGetVolumeStatsRequest) (*csi.NodeGetVolumeStatsResponse, error) {
+	klog.V(4).Infof("NodeExpandVolume: called with args %+v", *req)


s/NodeExpandVolume/NodeGetVolumeStats

ramineni · 2020-02-25T05:02:13Z

pkg/csi/cinder/nodeserver.go

+func (ns *nodeServer) NodeGetVolumeStats(_ context.Context, req *csi.NodeGetVolumeStatsRequest) (*csi.NodeGetVolumeStatsResponse, error) {
+	klog.V(4).Infof("NodeExpandVolume: called with args %+v", *req)
+
+	volumePath := req.GetVolumePath()


This function should comply with csi spec on required fields etc.
please run make test-cinder-csi-sanity and fix the issues respective to this function.

All sanity test errors related to NodeGetVolumeStats are fixed. Some tests fail on other methods, but it seems to be out of the scope of this PR.

+1, Awesome!! Thanks.

ramineni · 2020-02-25T05:02:33Z

pkg/csi/cinder/nodeserver.go

+	}
+	return &csi.NodeGetVolumeStatsResponse{
+		Usage: []*csi.VolumeUsage{
+			{Total: capacity, Available: available, Used: usage, Unit: 1},


what does Unit:1 mean?

Magic numbers are bad. Fixed!

theopenlab-ci · 2020-02-25T08:43:52Z

Build failed.

cloud-provider-openstack-unittest : FAILURE in 2m 55s

theopenlab-ci · 2020-02-25T08:48:40Z

Build succeeded.

cloud-provider-openstack-format : SUCCESS in 2m 31s

theopenlab-ci · 2020-03-27T05:13:50Z

Build failed.

cloud-provider-openstack-unittest : FAILURE in 2m 25s

theopenlab-ci · 2020-03-27T05:15:58Z

Build succeeded.

cloud-provider-openstack-format : SUCCESS in 2m 59s

theopenlab-ci · 2020-03-27T05:19:24Z

Build failed.

cloud-provider-openstack-e2e-test-csi-cinder : POST_FAILURE in 7m 12s

theopenlab-ci · 2020-03-27T05:20:13Z

Build failed.

cloud-provider-openstack-acceptance-test-csi-cinder : POST_FAILURE in 8m 04s

theopenlab-ci · 2020-03-27T05:24:45Z

Build succeeded.

cloud-provider-openstack-acceptance-test-lb-octavia : SUCCESS in 12m 46s

zuzzas · 2020-03-27T05:28:46Z

@lingxiankong
Now the unit tests fail. The log output is extremely hard to understand, is there an issue on my side?

lingxiankong · 2020-03-27T07:58:58Z

020-03-27 05:12:38.225611 | ubuntu-xenial-citynetwork | 	github.com/codegangsta/cli: github.com/codegangsta/[email protected]: parsing go.mod:
2020-03-27 05:12:38.225717 | ubuntu-xenial-citynetwork | 	module declares its path as: github.com/urfave/cli
2020-03-27 05:12:38.225813 | ubuntu-xenial-citynetwork | 	        but was required as: github.com/codegangsta/cli

zuzzas · 2020-03-27T09:17:39Z

@lingxiankong
It fails with the same error in your PR as well. Any tips on fixing this?
#991 (comment)

kayrus · 2020-03-27T09:37:19Z

Issue is related to theopenlab/openlab-zuul-jobs#837

ramineni · 2020-03-27T13:39:20Z

pkg/util/mount/mount.go

+	UsedInodes      int64
+}
+
+func (m *Mount) PathExists(volumePath string) (bool, error) {


:nit: i think this func exists in k8s.io/util/

It has an additional check for a "corrupted mount". I'm not sure that this is required and this may introduce a potential false-positive.

https://github.com/kubernetes/utils/blob/d1ab8797c55812f4fefe2c7b00a0d04a4740a93c/mount/mount_helper_common.go#L99-L100

ramineni · 2020-03-27T13:41:10Z

/test cloud-provider-openstack-e2e-test-csi-cinder

ramineni · 2020-03-27T13:41:35Z

Thanks for the work on this
/lgtm

ramineni · 2020-03-27T13:42:40Z

/test cloud-provider-openstack-e2e-test-csi-cinder

theopenlab-ci · 2020-03-27T14:15:52Z

Build succeeded.

cloud-provider-openstack-e2e-test-csi-cinder : SUCCESS in 30m 42s

lingxiankong · 2020-03-28T03:23:29Z

Please wait for theopenlab/openlab-zuul-jobs#841 merged.

zuzzas · 2020-03-28T05:24:56Z

/test cloud-provider-openstack-unittest

theopenlab-ci · 2020-03-28T05:29:48Z

Build succeeded.

cloud-provider-openstack-unittest : SUCCESS in 4m 23s

lingxiankong · 2020-03-28T06:09:02Z

/test cloud-provider-openstack-acceptance-test-csi-cinder

theopenlab-ci · 2020-03-28T06:19:46Z

Build succeeded.

cloud-provider-openstack-acceptance-test-csi-cinder : SUCCESS in 10m 11s

zuzzas · 2020-03-28T11:35:04Z

Love these green checkmarks.

lingxiankong · 2020-04-01T21:19:55Z

/lgtm
/approve

Given this PR got a +1 previously, merge now.

k8s-ci-robot · 2020-04-01T21:20:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lingxiankong

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:

~~OWNERS~~ [lingxiankong]

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

* NodeGetVolumeStats() implementation Signed-off-by: Andrey Klimentyev <[email protected]> * Sanity checks and removal of magic numbers Signed-off-by: Andrey Klimentyev <[email protected]> * Rewrite Signed-off-by: Andrey Klimentyev <[email protected]> * + * + * Latest fixes Signed-off-by: Andrey Klimentyev <[email protected]>

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 19, 2020

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 19, 2020

k8s-ci-robot requested review from FengyunPan2 and lingxiankong February 19, 2020 08:49

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 19, 2020

zuzzas force-pushed the csi-cinder-get-volume-stats branch from bab8a36 to c99011b Compare February 19, 2020 10:08

ramineni reviewed Feb 25, 2020

View reviewed changes

zuzzas force-pushed the csi-cinder-get-volume-stats branch from 0645db0 to 8e7fbde Compare February 25, 2020 08:45

ramineni reviewed Mar 27, 2020

View reviewed changes

k8s-ci-robot assigned ramineni Mar 27, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 27, 2020

k8s-ci-robot assigned lingxiankong Apr 1, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 1, 2020

k8s-ci-robot merged commit 90488ac into kubernetes:master Apr 1, 2020

zuzzas deleted the csi-cinder-get-volume-stats branch April 2, 2020 05:14

ramineni mentioned this pull request May 12, 2020

[cinder-csi-plugin] Volumes are not exposed to kubelet_volume_stats_* metrics. #1064

Closed

[cinder-csi-plugin] NodeGetVolumeStats() implementation #941

[cinder-csi-plugin] NodeGetVolumeStats() implementation #941

Conversation

zuzzas commented Feb 19, 2020 • edited by lingxiankong Loading

k8s-ci-robot commented Feb 19, 2020

theopenlab-ci bot commented Feb 19, 2020

theopenlab-ci bot commented Feb 19, 2020

jichenjc commented Feb 19, 2020

theopenlab-ci bot commented Feb 19, 2020

theopenlab-ci bot commented Feb 19, 2020

theopenlab-ci bot commented Feb 19, 2020

theopenlab-ci bot commented Feb 19, 2020

theopenlab-ci bot commented Feb 19, 2020

theopenlab-ci bot commented Feb 19, 2020

theopenlab-ci bot commented Feb 19, 2020

theopenlab-ci bot commented Feb 19, 2020

theopenlab-ci bot commented Feb 19, 2020

theopenlab-ci bot commented Feb 19, 2020

theopenlab-ci bot commented Feb 19, 2020

theopenlab-ci bot commented Feb 19, 2020

lingxiankong commented Feb 22, 2020

ramineni left a 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

theopenlab-ci bot commented Feb 25, 2020

theopenlab-ci bot commented Feb 25, 2020

theopenlab-ci bot commented Mar 27, 2020

theopenlab-ci bot commented Mar 27, 2020

theopenlab-ci bot commented Mar 27, 2020

theopenlab-ci bot commented Mar 27, 2020

theopenlab-ci bot commented Mar 27, 2020

zuzzas commented Mar 27, 2020

lingxiankong commented Mar 27, 2020

zuzzas commented Mar 27, 2020

kayrus commented Mar 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramineni commented Mar 27, 2020

ramineni commented Mar 27, 2020

ramineni commented Mar 27, 2020

theopenlab-ci bot commented Mar 27, 2020

lingxiankong commented Mar 28, 2020

zuzzas commented Mar 28, 2020

theopenlab-ci bot commented Mar 28, 2020

lingxiankong commented Mar 28, 2020

theopenlab-ci bot commented Mar 28, 2020

zuzzas commented Mar 28, 2020

lingxiankong commented Apr 1, 2020

k8s-ci-robot commented Apr 1, 2020

zuzzas commented Feb 19, 2020 •

edited by lingxiankong

Loading