add volume stats metrics - #677

AndyXiangLi · 2021-01-04T19:12:17Z

Is this a bug fix or adding new feature?
Fixes #223
Fixes #499
What is this PR about? / Why do we need it?
Implement Volume stats metrics

What testing is done?
Sanity test passed
Added unit test for the change
Validate volume status can be logged in the ebs-plugin in node pod

k8s-ci-robot · 2021-01-04T19:12:25Z

Hi @AndyXiangLi. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

gnufied · 2021-01-04T19:41:39Z

pkg/driver/node.go

+			},
+		},
+	}, nil
+


Can you post output of this passing csi sanity tests? I assume it did but just to be sure.

Yeah, sanity test is passed

Can you clarify in the PR description? Ideally with the output.

gnufied · 2021-01-04T19:44:10Z

/ok-to-test

coveralls · 2021-01-04T19:46:12Z

Pull Request Test Coverage Report for Build 1450

47 of 73 (64.38%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.5%) to 81.177%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/driver/node.go	47	73	64.38%

Totals
Change from base Build 1447:	-0.5%
Covered Lines:	1669
Relevant Lines:	2056

💛 - Coveralls

ayberk · 2021-01-05T00:46:15Z

pkg/driver/statter.go

+
+var _ Statter = realStatter{}
+
+type realStatter struct {


we should name this simply statter.

ayberk · 2021-01-05T00:51:10Z

pkg/driver/statter.go

+
+func (fakeStatter) StatFS(path string) (available, capacity, used, inodesFree, inodes, inodesUsed int64, err error) {
+	// Assume the file exists and give some dummy values back
+	return 1, 1, 1, 1, 1, 1, nil


Why don't you return back whatever is passed to this function? It'd be more useful for testing.

ayberk · 2021-01-05T00:52:06Z

pkg/driver/statter.go

+	"golang.org/x/sys/unix"
+)
+
+type Statter interface {


Can you add some documentation? It'd be really helpful going forward.

ayberk · 2021-01-05T00:52:36Z

pkg/driver/node.go

+	strOut := strings.TrimSpace(string(output))
+	gotSizeBytes, err := strconv.ParseInt(strOut, 10, 64)
+	if err != nil {
+		return -1, fmt.Errorf("failed to parse size %s into int a size", strOut)


nit: failed to parse size %s as int

ayberk · 2021-01-05T01:06:10Z

pkg/driver/statter.go

+		return false, err
+	}
+
+	return (st.Mode & unix.S_IFMT) == unix.S_IFBLK, nil


I'm going to assume this only works on unix?

Yeah, right now it is only working for unix

That's fine. I see you've added the checks for windows, which is good enough for now. I'm OK with only working on unix, but we shouldn't break windows.

ayberk · 2021-01-05T17:28:40Z

Supersedes #589.

wongma7 · 2021-01-06T04:15:46Z

fyi, test case failed

=== RUN TestNodeGetVolumeStats
node_test.go:1194: Expected no error, got rpc error: code = NotFound desc = path /test does not exist
panic.go:636: missing call(s) to *mocks.MockMounter.ExistsPath(is equal to /test) /home/prow/go/src/github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/driver/node_test.go:1180
panic.go:636: aborting test due to missing call(s)
--- FAIL: TestNodeGetVolumeStats (0.00s)

wongma7 · 2021-01-06T22:28:08Z

/approve

ayberk

A couple comments, good work!

Also, looks like you'll need to improve the coverage 😅

ayberk · 2021-01-06T22:48:07Z

pkg/driver/statter.go

+		return false, err
+	}
+
+	return (st.Mode & unix.S_IFMT) == unix.S_IFBLK, nil


That's fine. I see you've added the checks for windows, which is good enough for now. I'm OK with only working on unix, but we shouldn't break windows.

ayberk · 2021-01-06T22:49:45Z

pkg/driver/statter.go

+	}
+
+	// Available is blocks available * fragment size
+	available = int64(statfs.Bavail) * int64(statfs.Bsize)


Are all these fields guaranteed to be present if unix.Statfs succeeds?

Yes, they are all system level variables to my understanding, and we should be able to get all the following value in one call: Type Bsize Blocks Bfree Bavail Files Ffree Fsid NamelenFrsize Flags Spare

ayberk · 2021-01-06T22:50:52Z

pkg/driver/statter.go

+	// Capacity is total block count * fragment size
+	capacity = int64(statfs.Blocks) * int64(statfs.Bsize)
+	// Usage is block being used * fragment size (aka block size).
+	used = (int64(statfs.Blocks) - int64(statfs.Bfree)) * int64(statfs.Bsize)


We should probably do something like if used < 0 { used = 0 } and maybe log a warning for sanity.

ayberk · 2021-01-06T22:52:49Z

pkg/driver/statter.go

+	inodes = int64(statfs.Files)
+	inodesFree = int64(statfs.Ffree)
+	inodesUsed = inodes - inodesFree
+	return


I got confused for a moment here. God I hate the named return :)

Can you also add a comment to clarify the unit of these values? I'd assume they're bytes.

ayberk · 2021-01-06T22:55:50Z

pkg/driver/node.go

+	isBlock, err := d.statter.IsBlockDevice(req.VolumePath)
+
+	if err != nil {
+		return nil, status.Errorf(codes.NotFound, "failed to determine whether %s is block device: %v", req.VolumePath, err)


should this be codes.Internal?

ayberk · 2021-01-06T22:56:38Z

pkg/driver/node.go

+			},
+		},
+	}, nil
+


Can you clarify in the PR description? Ideally with the output.

ayberk · 2021-01-06T22:59:10Z

pkg/driver/sanity_test.go

@@ -73,6 +76,15 @@ func TestSanity(t *testing.T) {
 	sanity.Test(t, config)
 }

+func createDir(targetPath string) (string, error) {
+	if err := os.MkdirAll(targetPath, 0755); err != nil {


755 looks too permissive to me. 644 should be good for now.

Actually we need write and exec permit here, minimum 300 will work. But 755 is the default permit sanity_test used to mkdir, so I kept it..
https://github.com/kubernetes-csi/csi-test/blob/054980205e4fc45ab9e940e39bdfd71066d96d87/pkg/sanity/sanity.go#L365

I wasn't sure why we would need exec on a folder. TIL :) https://unix.stackexchange.com/a/150456

ayberk · 2021-01-06T23:03:24Z

pkg/driver/sanity_test.go

@@ -73,6 +76,15 @@ func TestSanity(t *testing.T) {
 	sanity.Test(t, config)
 }

+func createDir(targetPath string) (string, error) {
+	if err := os.MkdirAll(targetPath, 0755); err != nil {
+		if !os.IsExist(err) {


you can use IsNotExist here.

ayberk · 2021-01-06T23:05:32Z

pkg/driver/sanity_test.go

@@ -309,13 +321,48 @@ func (f *fakeMounter) GetDeviceName(mountPath string) (string, int, error) {
 }

 func (f *fakeMounter) MakeFile(pathname string) error {
+	file, err := os.OpenFile(pathname, os.O_CREATE, os.FileMode(0644))


I know we talked about this offline, but I want to note it publicly.

I am not quite sure why we have a different mounter for sanity tests. We should use the code from the driver itself, otherwise we're not really testing the "real" code.

/cc @wongma7 @gnufied

I think it's fine, I'm not too familiar with sanity but seems mocking the mount is common pattern. https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/blob/2014365ea7b3405e024be1d9552aa30d44da4a8b/pkg/mount-manager/fake-safe-mounter.go#L28

I think the sanity test is still testing everything in the driver code surrounding wheerever it actually tries to call "Mount/Statfs".

Should be fine as long as we don't modify the mount, but if we do that we risk silently failing sanity tests.

@AndyXiangLi can you add a note to the actual mount code? That it's mirrored here and any modification should be applied here too. Not ideal, but I'd feel safer.

@ayberk Sure will do that, and as we talked off line, I think it would be better if we can extract those three functions out of mounter in the future.

ayberk · 2021-01-09T01:08:02Z

pkg/driver/node_test.go

-		mounter:  mockMounter,
-		inFlight: internal.NewInFlight(),
-	}
+				err := os.MkdirAll(VolumePath, 0644)


looks like there are some indentation problems in this file.

ayberk · 2021-01-11T21:09:53Z

/lgtm

wongma7 · 2021-01-11T21:14:17Z

I'll manually merge, coverage decrease is because of getBlockSizeBytes which is not easy to exercise and anyway coveralls is not supposed to be required! ( longstanding TODO to fix this).

/approve

k8s-ci-robot · 2021-01-11T21:14:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AndyXiangLi, wongma7

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~~ [wongma7]

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

k8s-ci-robot requested review from d-nishi and jsafrane January 4, 2021 19:12

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 4, 2021

gnufied reviewed Jan 4, 2021

View reviewed changes

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 Jan 4, 2021

AndyXiangLi force-pushed the volume-stat-metrics branch from 938a448 to 29e2b72 Compare January 4, 2021 20:02

ayberk reviewed Jan 5, 2021

View reviewed changes

pkg/driver/statter.go Outdated

var _ Statter = realStatter{}

type realStatter struct {

Copy link

Contributor

ayberk Jan 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should name this simply statter.

ayberk reviewed Jan 5, 2021

View reviewed changes

AndyXiangLi closed this Jan 6, 2021

AndyXiangLi force-pushed the volume-stat-metrics branch from b69f5cd to ddaa68b Compare January 6, 2021 21:41

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 6, 2021

AndyXiangLi reopened this Jan 6, 2021

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 6, 2021

AndyXiangLi force-pushed the volume-stat-metrics branch from d254890 to 179a992 Compare January 6, 2021 21:55

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 6, 2021

AndyXiangLi requested review from gnufied and ayberk January 6, 2021 22:39

ayberk suggested changes Jan 6, 2021

View reviewed changes

k8s-ci-robot requested a review from wongma7 January 6, 2021 23:05

AndyXiangLi force-pushed the volume-stat-metrics branch 2 times, most recently from b88f7c1 to 13bf1a3 Compare January 7, 2021 23:34

Henni mentioned this pull request Jan 8, 2021

Volume metrics #524

Closed

AndyXiangLi force-pushed the volume-stat-metrics branch 2 times, most recently from 81b5c9d to e7e5d3e Compare January 9, 2021 01:12

ayberk reviewed Jan 9, 2021

View reviewed changes

Implement NodeGetVolumeStats to support node volume metrics

f7ec047

AndyXiangLi force-pushed the volume-stat-metrics branch from e7e5d3e to f7ec047 Compare January 9, 2021 01:26

k8s-ci-robot assigned ayberk Jan 11, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 11, 2021

wongma7 merged commit 7031827 into kubernetes-sigs:master Jan 11, 2021

juris mentioned this pull request Jan 26, 2021

node_exporter can not access persistent volume mounts bottlerocket-os/bottlerocket#1271

Closed

kovalyukm mentioned this pull request Feb 1, 2021

Report Metrics for the driver #223

Closed

wongma7 mentioned this pull request Feb 3, 2021

GCR build/push timing out #716

Closed

robcxyz mentioned this pull request Mar 30, 2022

Volume metrics vultr/vultr-csi#67

Closed

erdembas mentioned this pull request Jun 16, 2022

Volume metrics vmware/cloud-director-named-disk-csi-driver#62

Closed

georgew5656 mentioned this pull request Oct 21, 2022

EBS Usage Metrics are not being emitted for some pods #1432

Closed

add volume stats metrics - #677

add volume stats metrics - #677

Conversation

AndyXiangLi commented Jan 4, 2021 • edited

k8s-ci-robot commented Jan 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnufied commented Jan 4, 2021

coveralls commented Jan 4, 2021 • edited

Pull Request Test Coverage Report for Build 1450

💛 - Coveralls

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

ayberk commented Jan 5, 2021

wongma7 commented Jan 6, 2021

wongma7 commented Jan 6, 2021

ayberk left a comment • 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

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

ayberk commented Jan 11, 2021

wongma7 commented Jan 11, 2021

k8s-ci-robot commented Jan 11, 2021

AndyXiangLi commented Jan 4, 2021 •

edited

coveralls commented Jan 4, 2021 •

edited

ayberk left a comment •

edited