Implement metrics for cephfs CSI driver. #310

humblec · 2019-04-04T13:11:53Z

Depends on

Signed-off-by: Humble Chirammal hchiramm@redhat.com

mykaul · 2019-04-14T15:55:51Z

@humblec - any updates?

ShyamsundarR · 2019-04-15T12:52:08Z

@humblec discussions around dropping credential passing to the plugins, and using what is passed down form the StorageClass make it so that, the NodeGetVolumeStats RPC will have to function without credentials. Noting it here as you may want to factor that into your implementation.

humblec · 2019-05-05T14:38:02Z

@humblec - any updates?

This PR is waiting for kubernetes support . I will update the PR once the support is added in upstream

anmolsachan · 2019-06-04T07:13:40Z

Dependency merged to kubernetes kubernetes/kubernetes#76188

humblec · 2019-06-04T09:31:26Z

Yeah, the dependency patch in kubernetes got merged on May 17th which was before 1.15 code freeze.

humblec · 2019-06-27T11:27:56Z

Couple of days back the kubernetes dependency of our CSI repo has been upgraded to 1.15 which support metrics code base. This PR also depends on new CephFS volume provisioning

humblec · 2019-07-17T04:50:33Z

The final dependency has also got merged last week and part of 1.1.0 release https://github.com/ceph/ceph-csi/releases . Revisiting this PR as all the dependencies are in.

Madhu-1 · 2019-07-17T14:28:13Z

is this for rbd also? if not please change the title and also add an E2E for this feature

humblec · 2019-07-17T15:05:06Z

is this for rbd also? if not please change the title and also add an E2E for this feature

Its for CephFS , updated the title.

ShyamsundarR · 2019-07-18T13:28:08Z

pkg/cephfs/cephfs_statfs.go

+
+// getFsInfo writes metrics.Capacity, metrics.Used and metrics.Available from the filesystem info
+func (md *cephfsStatFS) getFsInfo(metrics *volume.Metrics) error {
+	available, capacity, usage, inodes, inodesFree, inodesUsed, err := fs.FsInfo(md.path)


@humblec (tagging @ajarr as well) The call to fs.FsInfo uses the syscall statfs. I checked that statfs on a CephFS subvolume mount, returns used space as per the set attrs on the subvolume. I wanted to know if the same applies to inodes, IOW, statfs fields,

fsfilcnt_t f_files; /* total file nodes in file system */ fsfilcnt_t f_ffree; /* free file nodes in fs */

and hence can be used to generate metrics as below.

@ShyamsundarR not sure I got the comment, I would like to mention that, whatever statfs returns is good enough for us. If subvol is not responding properly to statfs its totally a different bug which Ceph has to take care. I dont think there is a bug though.

@humblec I tested and determined that cephfs subvolume mounts as used in ceph-csi returns the correct total space as needed by us (i.e it sticks to the quota set on the subvolume). I am however not able to determine if it returns inode count information as required (or used space, which is a short test away if needed). Hence I need confirmation that it does, in case you know it or can test it, else from CephFS developers.

We need to understand statfs limitations if any for CephFS subvolumes, before interpreting it's output the way we want OR assuming any errors is a bug in CephFS subvolume implementation.

The provided image shows used space, which was never my concern as stated in the comment.

Hope this provides the required clarity.

We need to understand statfs limitations if any for CephFS subvolumes, before interpreting it's output the way we want OR assuming any errors is a bug in CephFS subvolume implementation.

@ShyamsundarR If you mount the subvolume and call stat against, you should be able to verify it , Isnt it ? Agree that, its good to verify with cephfs team too.

humblec · 2019-07-18T15:54:56Z

Btw @anmolsachan was testing this PR and we got the expected result in his testing.

anmolsachan · 2019-07-18T15:57:02Z

Btw @anmolsachan was testing this PR and we got the expected result in his testing.

yes, thanks Humble for the PR. Attaching screenshot

pkg/cephfs/cephfs_statfs.go

Madhu-1 · 2019-07-19T05:30:46Z

pkg/cephfs/cephfs_statfs.go

+	"k8s.io/kubernetes/pkg/volume/util/fs"
+)
+
+const (


@humblec I think this entire code is already there in https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/metrics_statfs.go can we just make use of it?

@Madhu-1 Its a mix or more than that with errors, It can also have ceph specific errors and can be enhanced for more functions. Thats why it has been modified and kept it separately. copyright carries for both for the same reason.

Any way, this has been shrinked now.

pkg/cephfs/nodeserver.go

Madhu-1 · 2019-07-19T05:40:10Z

pkg/cephfs/nodeserver.go

+
+	if err != nil {
+		klog.Errorf("stat failed on path: %v, error: %v", targetPath, err)
+		return nil, status.Error(codes.Internal, err.Error())


return invalid agrument if targetPath does not exist

still this need to be handled

Madhu-1 · 2019-07-19T08:08:52Z

@humblec please resolve the merge conflict

humblec · 2019-07-19T08:13:35Z

@Madhu-1 Done. Thanks!

ShyamsundarR · 2019-07-19T10:39:23Z

is this for rbd also? if not please change the title and also add an E2E for this feature

@humblec are we adding an E2E test for this? Or is this not feasible?

humblec · 2019-07-19T10:48:07Z

is this for rbd also? if not please change the title and also add an E2E for this feature

@humblec are we adding an E2E test for this? Or is this not feasible?

No @ShyamsundarR. Its not feasible.

ShyamsundarR · 2019-07-19T10:50:34Z

is this for rbd also? if not please change the title and also add an E2E for this feature

@humblec are we adding an E2E test for this? Or is this not feasible?

No @ShyamsundarR. Its not feasible.

What are the alternatives for the future? csi-test is one that I can see. Also feasibility is an issue because there are no callers to this?

humblec · 2019-07-19T10:51:33Z

@ShyamsundarR we can add csi-test later for the RPC call.

ShyamsundarR · 2019-07-19T10:53:18Z

@ShyamsundarR we can add csi-test later for the RPC call.

Yes I am also looking at the "future" and not at in this PR, but wanted clarity on the feasibility issue, so that we know how to tackle it later.

humblec · 2019-07-19T10:54:09Z

@ShyamsundarR we can add csi-test later for the RPC call.

Yes I am also looking at the "future" and not at in this PR, but wanted clarity on the feasibility issue, so that we know how to tackle it later.

@ShyamsundarR Yeah, we could start with csi-test . Meanwhile I will think about the E2E possibilities, if any.

Madhu-1 · 2019-07-19T11:09:48Z

pkg/cephfs/nodeserver.go

+
+	isMnt, err := isMountPoint(targetPath)
+
+	if err != nil {


suggestion

if err != nil{ klog.Errorf("stat failed on path: %v, error: %v", targetPath, err) if os.IsNotExist(err){ return nil, status.Error(codes.InvalidArgument, err.Error()) } else{ return nil, status.Error(codes.Internal, err.Error()) } }

isMountPoint is always returning status.Error(codes.Internal, err.Error()) on errors, but the current error message does need some massaging around "stat failing etc."

humble can you just return an error here?

Madhu-1 · 2019-07-19T12:40:30Z

question:
@humblec if the implementation remains same for rbd also can we move this to csi-common?

ShyamsundarR · 2019-07-19T12:59:16Z

question:
@humblec if the implementation remains same for rbd also can we move this to csi-common?

Implementation is common for RBD and CephFS as long as we are dealing with RWX/RWO volume types. VolumeID validation would differ for each, but that can be layered I guess. So yes there is commonality at the moment. Also, for block volumes we need to return only total size, and possibly should do so (when we implement the same).

For RBD we should not need secrets as we can get the image name from the VolumeID decomposing the image ID and pool ID and using the same to validate pool_id and 'name' contents at /sys/devices/rbd/X (where X is the device number) like in the call at getRbdDevFromImageAndPool. There is a slim chance that we have the same 'imagename' and 'poolID' across 2 clusters, that are mapped to the same node, but that seems slim. Tagging @dillaman to rectify any misconceptions.

For CephFS need to think a little more and see if we can map the mount options to check the subvolume name maps to the image name, but needs more thought to understand if secrets are not needed or are mandatory.

Madhu-1

for rbd, if it's required to move it to the csi-common folder please do it as it will not affect the functionality

humblec · 2019-07-19T14:42:51Z

Thanks @Madhu-1 , RBD , I am still testing and will do the changes accordingly. @ShyamsundarR Can you review this in priority? Other teams are waiting for availability of this PR , so the urgency.

ShyamsundarR

Mostly error handling comments, and some message nits.

@humblec thoughts?

ShyamsundarR · 2019-07-19T15:24:24Z

pkg/cephfs/nodeserver.go

+
+	isMnt, err := isMountPoint(targetPath)
+
+	if err != nil {


isMountPoint is always returning status.Error(codes.Internal, err.Error()) on errors, but the current error message does need some massaging around "stat failing etc."

pkg/cephfs/nodeserver.go

ShyamsundarR · 2019-07-19T15:32:19Z

pkg/cephfs/nodeserver.go

+
+	if isMnt {
+		klog.Infof("cephfs: volume %s is already mounted to %s", volID, targetPath)
+		cephfsProvider := volume.NewMetricsStatFS(targetPath)


Error check to ensure cephfsProvider is non-nil?

this is not required. never going to happen

yep, this is not gonna happen.

Is this because the signature does not have a return like (xxx, err) that we can assume this will not fail? Attempting to understand why this is not going to happen? Not a blocker for merge, but looking for information.

pkg/cephfs/nodeserver.go

Madhu-1 · 2019-07-19T15:53:04Z

pkg/cephfs/nodeserver.go

+					Available: available,
+					Total:     capacity,
+					Used:      used,
+					Unit:      1,


nit:
use VolumeUsage_BYTES instead of 1 and use VolumeUsage_INODES instead of 2

humblec · 2019-07-20T09:29:24Z

@Madhu-1 @ShyamsundarR comments are addressed. Please go through it, if something else is missing, we can followup.

humblec · 2019-07-22T10:40:30Z

The travis is failing due to an issue at ceph container image #486

Signed-off-by: Humble Chirammal <hchiramm@redhat.com>

humblec changed the title ~~Implement metrics for ceph-csi driver.~~ [WIP] [DNM]Implement metrics for ceph-csi driver. Apr 4, 2019

Madhu-1 added the DNM DO NOT MERGE label Apr 4, 2019

humblec force-pushed the metrics branch 3 times, most recently from 26316b6 to 5a12f32 Compare April 4, 2019 13:36

humblec mentioned this pull request Jun 24, 2019

Update base dependency of kube to 1.15.0 #439

Merged

humblec force-pushed the metrics branch 2 times, most recently from 8ffa64e to 2ab5c28 Compare July 17, 2019 04:55

humblec changed the title ~~[WIP] [DNM]Implement metrics for ceph-csi driver.~~ Implement metrics for ceph-csi driver. Jul 17, 2019

humblec force-pushed the metrics branch 6 times, most recently from d7a39f5 to e4e7600 Compare July 17, 2019 07:19

humblec added release-1.2.0 Priority-0 highest priority issue and removed DNM DO NOT MERGE labels Jul 17, 2019

humblec changed the title ~~Implement metrics for ceph-csi driver.~~ Implement metrics for cephfs CSI driver. Jul 17, 2019

ShyamsundarR reviewed Jul 18, 2019

View reviewed changes

Madhu-1 requested changes Jul 19, 2019

View reviewed changes

humblec force-pushed the metrics branch from cd59bf4 to c6e4193 Compare July 19, 2019 07:41

humblec force-pushed the metrics branch from c6e4193 to 7b7fa5c Compare July 19, 2019 08:11

humblec force-pushed the metrics branch from 7b7fa5c to 22cc3ab Compare July 19, 2019 10:14

Madhu-1 reviewed Jul 19, 2019

View reviewed changes

Madhu-1 approved these changes Jul 19, 2019

View reviewed changes

ShyamsundarR reviewed Jul 19, 2019

View reviewed changes

Madhu-1 reviewed Jul 19, 2019

View reviewed changes

humblec force-pushed the metrics branch from 22cc3ab to 0f3d99e Compare July 20, 2019 09:26

humblec force-pushed the metrics branch from 0f3d99e to c571cd1 Compare July 22, 2019 09:47

ShyamsundarR approved these changes Jul 22, 2019

View reviewed changes

humblec force-pushed the metrics branch from c571cd1 to a863f38 Compare July 24, 2019 11:15

Implement metrics for CephFS CSI driver

e14422b

Signed-off-by: Humble Chirammal <hchiramm@redhat.com>

humblec force-pushed the metrics branch from a863f38 to e14422b Compare July 25, 2019 05:17

mergify bot merged commit 561cc26 into ceph:master Jul 25, 2019

Implement metrics for cephfs CSI driver. #310

Implement metrics for cephfs CSI driver. #310

Conversation

humblec commented Apr 4, 2019 • edited Loading

mykaul commented Apr 14, 2019

ShyamsundarR commented Apr 15, 2019

humblec commented May 5, 2019

anmolsachan commented Jun 4, 2019

humblec commented Jun 4, 2019

humblec commented Jun 27, 2019 • edited Loading

humblec commented Jul 17, 2019 • edited Loading

Madhu-1 commented Jul 17, 2019

humblec commented Jul 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humblec Jul 19, 2019 • edited Loading

Choose a reason for hiding this comment

humblec commented Jul 18, 2019

anmolsachan commented Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

humblec Jul 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Madhu-1 Jul 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Madhu-1 commented Jul 19, 2019

humblec commented Jul 19, 2019

ShyamsundarR commented Jul 19, 2019

humblec commented Jul 19, 2019

ShyamsundarR commented Jul 19, 2019

humblec commented Jul 19, 2019

ShyamsundarR commented Jul 19, 2019

humblec commented Jul 19, 2019 • edited Loading

Madhu-1 Jul 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Madhu-1 commented Jul 19, 2019

ShyamsundarR commented Jul 19, 2019

Madhu-1 left a comment

Choose a reason for hiding this comment

humblec commented Jul 19, 2019

ShyamsundarR 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

humblec commented Jul 20, 2019

humblec commented Jul 22, 2019

humblec commented Apr 4, 2019 •

edited

Loading

humblec commented Jun 27, 2019 •

edited

Loading

humblec commented Jul 17, 2019 •

edited

Loading

humblec Jul 19, 2019 •

edited

Loading

anmolsachan commented Jul 18, 2019 •

edited

Loading

humblec Jul 19, 2019 •

edited

Loading

Madhu-1 Jul 19, 2019 •

edited

Loading

humblec commented Jul 19, 2019 •

edited

Loading

Madhu-1 Jul 19, 2019 •

edited

Loading