[WIP] Add block supports to rbd driver #97

mkimuram · 2018-11-01T20:05:08Z

This PR is to add block supports to rbd driver.

This won't be ready to be merge until below discussions completed.
kubernetes/kubernetes#68635

mkimuram · 2018-11-01T20:15:46Z

pkg/rbd/nodeserver.go

-	if notMnt {
-		return nil, status.Error(codes.NotFound, "Volume not mounted")
-	}
-
 	devicePath, cnt, err := mount.GetDeviceNameFromMount(ns.mounter, targetPath)


@rootfs

As a result of discussion in CSI WG, csi driver needs bind mount device to target path for block volume case.

However, on unpublish, above code doesn't seem to return a proper devicePath for block volume.
Because mount command returns devtmpfs instead of devicePath, if the block volume is bind mounted.
On the other hand, findmnt will return an information on devicePath.

Are there any good ways to utilize findmnt command for this purpose inside csi driver, here?

# touch /tmp/dev # mount -o bind /dev/rbd0 /tmp/dev # mount | grep /tmp/dev devtmpfs on /tmp/dev type devtmpfs (rw,nosuid,seclabel,size=16460544k,nr_inodes=4115136,mode=755) # findmnt -n -o SOURCE --target /tmp/dev devtmpfs[/rbd0]

in this case, you can use the block's major and minor number to find the device path.
https://www.kernel.org/doc/Documentation/admin-guide/devices.txt

#mount -o bind /dev/sdc /tmp/dev # ls -l /tmp/dev brw-rw----. 1 root disk 8, 32 Nov 2 09:36 /tmp/dev # so the major number is 8, minor number is 32, you can also get them from stat # then find the same major and minor number in /dev # ls -l /dev/sd* brw-rw----. 1 root disk 8, 0 Oct 8 19:00 /dev/sda brw-rw----. 1 root disk 8, 1 Oct 8 19:00 /dev/sda1 brw-rw----. 1 root disk 8, 2 Oct 8 19:00 /dev/sda2 brw-rw----. 1 root disk 8, 16 Oct 8 19:00 /dev/sdb brw-rw----. 1 root disk 8, 17 Oct 8 19:00 /dev/sdb1 brw-rw----. 1 root disk 8, 18 Oct 8 19:00 /dev/sdb2 brw-rw----. 1 root disk 8, 32 Nov 2 09:36 /dev/sdc

mkimuram · 2018-11-01T22:35:10Z

I made a quick dirty hack to k8s's util side to make GetDeviceNameFromMount handle device bind mount properly. (In this hack, only NsenterMounter is handled.) Then, the issue could be worked around.

If there's no easy better way or this kind of change will also benefit to k8s, I will open an issue for k8s and make a PR for handling device bind mount. (Actually, I'm very new to these mount code, I really need someone's help.)

@rootfs

Any comments on this?

diff --git a/pkg/rbd/nodeserver.go b/pkg/rbd/nodeserver.go
index dbf2b069..adea3905 100644
--- a/pkg/rbd/nodeserver.go
+++ b/pkg/rbd/nodeserver.go
@@ -140,6 +140,8 @@ func (ns *nodeServer) NodeUnpublishVolume(ctx context.Context, req *csi.NodeUnpu
 		return nil, status.Error(codes.Internal, err.Error())
 	}
 
+	glog.V(4).Infof("NodeUnpublishVolume: targetPath: %s, devicePath: %s\n", targetPath, devicePath)
+
 	// Unmounting the image
 	err = ns.mounter.Unmount(targetPath)
 	if err != nil {
diff --git a/vendor/k8s.io/kubernetes/pkg/util/mount/exec_mount.go b/vendor/k8s.io/kubernetes/pkg/util/mount/exec_mount.go
index fe7dcbd7..f9f655bc 100644
--- a/vendor/k8s.io/kubernetes/pkg/util/mount/exec_mount.go
+++ b/vendor/k8s.io/kubernetes/pkg/util/mount/exec_mount.go
@@ -112,6 +112,12 @@ func (m *execMounter) GetDeviceNameFromMount(mountPath, pluginDir string) (strin
 	return m.wrappedMounter.GetDeviceNameFromMount(mountPath, pluginDir)
 }
 
+// ResolveBindMountedBlockDevice resolves a block device bind mounted on mountPath by calling findmnt
+// in the host's root mount namespace.
+func (m *execMounter) ResolveBindMountedBlockDevice(mountPath string) (string, error) {
+	return m.wrappedMounter.ResolveBindMountedBlockDevice(mountPath)
+}
+
 func (m *execMounter) IsMountPointMatch(mp MountPoint, dir string) bool {
 	return m.wrappedMounter.IsMountPointMatch(mp, dir)
 }
diff --git a/vendor/k8s.io/kubernetes/pkg/util/mount/fake.go b/vendor/k8s.io/kubernetes/pkg/util/mount/fake.go
index 10832fd3..99771139 100644
--- a/vendor/k8s.io/kubernetes/pkg/util/mount/fake.go
+++ b/vendor/k8s.io/kubernetes/pkg/util/mount/fake.go
@@ -185,6 +185,11 @@ func (f *FakeMounter) GetDeviceNameFromMount(mountPath, pluginDir string) (strin
 	return getDeviceNameFromMount(f, mountPath, pluginDir)
 }
 
+func (f *FakeMounter) ResolveBindMountedBlockDevice(mountPath string) (string, error) {
+	// TODO implement properly
+	return "", nil
+}
+
 func (f *FakeMounter) MakeRShared(path string) error {
 	return nil
 }
diff --git a/vendor/k8s.io/kubernetes/pkg/util/mount/mount.go b/vendor/k8s.io/kubernetes/pkg/util/mount/mount.go
index f5ce194c..16066ff6 100644
--- a/vendor/k8s.io/kubernetes/pkg/util/mount/mount.go
+++ b/vendor/k8s.io/kubernetes/pkg/util/mount/mount.go
@@ -72,6 +72,9 @@ type Interface interface {
 	// GetDeviceNameFromMount finds the device name by checking the mount path
 	// to get the global mount path which matches its plugin directory
 	GetDeviceNameFromMount(mountPath, pluginDir string) (string, error)
+	// ResolveBindMountedBlockDevice resolves the bind mounted block device path
+	// that can not be resolved by the output of List()
+	ResolveBindMountedBlockDevice(mountPath string) (string, error)
 	// MakeRShared checks that given path is on a mount with 'rshared' mount
 	// propagation. If not, it bind-mounts the path as rshared.
 	MakeRShared(path string) error
@@ -218,7 +221,7 @@ func GetDeviceNameFromMount(mounter Interface, mountPath string) (string, int, e
 	}
 
 	// Find the device name.
-	// FIXME if multiple devices mounted on the same mount path, only the first one is returned
+	// FIXME if multiple devices mounted on the same mount path, only the last one is returned
 	device := ""
 	// If mountPath is symlink, need get its target path.
 	slTarget, err := filepath.EvalSymlinks(mountPath)
@@ -226,9 +229,18 @@ func GetDeviceNameFromMount(mounter Interface, mountPath string) (string, int, e
 		slTarget = mountPath
 	}
 	for i := range mps {
+		if mps[i].Device == "devtmpfs" {
+			dev, err := mounter.ResolveBindMountedBlockDevice(mps[i].Path)
+			if err != nil {
+				// This mount point is not resolved properly skip checking
+				continue
+			}
+			// update to the resolved name
+			mps[i].Device = dev
+		}
 		if mps[i].Path == slTarget {
 			device = mps[i].Device
-			break
+			// Continue this loop to update the device with resolved name for refCount
 		}
 	}
 
diff --git a/vendor/k8s.io/kubernetes/pkg/util/mount/mount_linux.go b/vendor/k8s.io/kubernetes/pkg/util/mount/mount_linux.go
index e80dad59..3d355c3a 100644
--- a/vendor/k8s.io/kubernetes/pkg/util/mount/mount_linux.go
+++ b/vendor/k8s.io/kubernetes/pkg/util/mount/mount_linux.go
@@ -376,6 +376,11 @@ func getDeviceNameFromMount(mounter Interface, mountPath, pluginDir string) (str
 	return path.Base(mountPath), nil
 }
 
+func (mounter *Mounter) ResolveBindMountedBlockDevice(mountPath string) (string, error) {
+	// TODO implement properly
+	return "", nil
+}
+
 func listProcMounts(mountFilePath string) ([]MountPoint, error) {
 	content, err := utilio.ConsistentRead(mountFilePath, maxListTries)
 	if err != nil {
diff --git a/vendor/k8s.io/kubernetes/pkg/util/mount/nsenter_mount.go b/vendor/k8s.io/kubernetes/pkg/util/mount/nsenter_mount.go
index a122411c..9a19d125 100644
--- a/vendor/k8s.io/kubernetes/pkg/util/mount/nsenter_mount.go
+++ b/vendor/k8s.io/kubernetes/pkg/util/mount/nsenter_mount.go
@@ -22,6 +22,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"regexp"
 	"strings"
 	"syscall"
 
@@ -207,6 +208,42 @@ func parseFindMnt(out string) (string, error) {
 	return out[:i], nil
 }
 
+// ResolveBindMountedBlockDevice resolves a block device bind mounted on mountPath by calling findmnt
+// in the host's root mount namespace.
+func (n *NsenterMounter) ResolveBindMountedBlockDevice(mountPath string) (string, error) {
+	args := []string{"-n", "-o", "SOURCE", "--first-only", "--target", mountPath}
+	glog.V(5).Infof("nsenter findmnt args: %v", args)
+	out, err := n.ne.Exec("findmnt", args).CombinedOutput()
+	if err != nil {
+		glog.V(2).Infof("Failed findmnt command for path %s: %s %v", mountPath, out, err)
+		// Different operating systems behave differently for paths which are not mount points.
+		// On older versions (e.g. 2.20.1) we'd get error, on newer ones (e.g. 2.26.2) we'd get "/".
+		// It's safer to assume that it's not a mount point.
+		return "", err
+	}
+
+	return parseFindMntResolveSource(string(out))
+}
+
+// parse output of "findmnt -o SOURCE --first-only --target" and return just the SOURCE
+func parseFindMntResolveSource(out string) (string, error) {
+	// cut trailing newline
+	out = strings.TrimSuffix(out, "\n")
+	// Check if out is a mounted device
+	reMnt := regexp.MustCompile("^(/[^/]+(?:/[^/]*)*)$")
+	if match := reMnt.FindStringSubmatch(out); match != nil {
+		return match[1], nil
+	}
+
+	// Check if out is a block device
+	reBlk := regexp.MustCompile("^devtmpfs\\[(/[^/]+(?:/[^/]*)*)\\]$")
+	if match := reBlk.FindStringSubmatch(out); match != nil {
+		return fmt.Sprintf("/dev%s", match[1]), nil
+	}
+
+	return "", fmt.Errorf("parseFindMntResolveSource: %s doesn't match to any expected findMnt output", out)
+}
+
 // DeviceOpened checks if block device in use by calling Open with O_EXCL flag.
 // Returns true if open returns errno EBUSY, and false if errno is nil.
 // Returns an error if errno is any error other than EBUSY.

rootfs · 2018-11-02T12:17:23Z

deploy/rbd/kubernetes/csi-provisioner-rbac.yaml

@@ -24,6 +24,9 @@ rules:
  - apiGroups: [""]
    resources: ["events"]
    verbs: ["list", "watch", "create", "update", "patch"]
+  - apiGroups: [""]
+    resources: ["endpoints"]


why does provisioner need endpoints?

I retest with deleting this and confirmed that this isn't necessary, so I removed it.
I suspect that I enabled leader election in the previous tests.

rootfs · 2018-11-02T12:18:59Z

pkg/rbd/nodeserver.go

+		st, err := os.Stat(targetPath)
+		if err != nil {
+			if os.IsNotExist(err) {
+				return nil, status.Error(codes.NotFound, "targetPath not mounted")


nit: targetPath not exist

Fixed as suggested.

rootfs · 2018-11-02T12:20:24Z

pkg/rbd/nodeserver.go

 			return nil, status.Error(codes.Internal, err.Error())
 		}
-	}
+		if !st.Mode().IsRegular() {
+			return nil, status.Error(codes.Internal, "targetPath is not regular file")


could the target path be a sym link? if so, should the sym link be resolved first and check if it is a regular file?

It should not be a symlink and it should be a regular file that CO create just for bind mount the device. So, keeping it as is.

mkimuram · 2018-11-06T17:55:19Z

@rootfs

Thank you for your comments. Fixed as suggested.
Also, add a dirty hack on k8s side of code to the commit to help kubernetes/kubernetes#68635 discussions move forward.

If you find any other ways to resolve bind mounted device, please let me know it. (I guess that device number matching would also require getting information from node as it is done via findmount.)

mkimuram · 2018-11-07T02:15:15Z

As a result of investigation, it seems that devicePath could be resolved inside csi-rbdplugin, just by executing findmnt command there. So, 4aa6646 moves resolving bind mount logic from k8s to csi-rbdplugin.

rootfs · 2018-11-07T13:53:13Z

vendor/k8s.io/kubernetes/pkg/util/mount/exec_mount.go

@@ -112,12 +112,6 @@ func (m *execMounter) GetDeviceNameFromMount(mountPath, pluginDir string) (strin
 	return m.wrappedMounter.GetDeviceNameFromMount(mountPath, pluginDir)
 }

-// ResolveBindMountedBlockDevice resolves a block device bind mounted on mountPath by calling findmnt


how did you update vendor packages? wouldn't Gopkg.lock get updated too?

Actually, I directly updated them as a dirty hack, which should be fixed before merge.
As commented above, this dirty hack won't be necessary if 4aa6646 makes sense.

rootfs · 2018-11-09T17:22:44Z

thanks @mkimuram please ping me once 4aa6646 is merged upstream

mkimuram · 2018-11-20T21:00:41Z

@rootfs

In 1d770c6, I updated only rbd codes for csi v1.0.0 spec, to continue discussion on block volume related function in csi external provisioner (kubernetes-csi/external-provisioner#135). So, cephfs codes have issues in this commit. I would expect that csi rbd driver and cephfs driver codes for csi v1.0.0 will be merged in a separate PR, so that this PR can only focus on block volume feature for csi rbd driver.

rootfs · 2018-11-21T12:33:57Z

@mkimuram got it, thanks for the update and hard work!

rootfs · 2019-01-16T17:43:10Z

since csi v1 is already merged, I'll pick up the block volume support commits and create a new PR

rootfs · 2019-01-17T16:05:56Z

superseded by #132

sync downstream devel branch with upstream

mkimuram commented Nov 1, 2018

View reviewed changes

rootfs reviewed Nov 2, 2018

View reviewed changes

mkimuram mentioned this pull request Nov 6, 2018

CSI block volume refactor to fix target path kubernetes/kubernetes#68635

Merged

rootfs reviewed Nov 7, 2018

View reviewed changes

mkimuram added 7 commits November 20, 2018 16:38

Add block supports to rbd driver

591aefe

Fix issue for bind mount

d4e7a4c

Fix error message and provisioner rbac

fb82880

Move resolving bind mount logic from k8s

1447283

Change csi rbd to create/delete targetPath for publish/unpublish

2a11c04

Fix pv deletion issue caused by not dettaching due to wrong mount count

b1add26

Update csi rbd to support csi v1.0.0 spec

1d770c6

mkimuram force-pushed the blockv3 branch from e507d72 to 1d770c6 Compare November 20, 2018 20:36

gman0 mentioned this pull request Dec 18, 2018

rbd: refuse to create block volumes #110

Merged

rootfs mentioned this pull request Jan 16, 2019

support block volume #132

Merged

rootfs closed this Jan 17, 2019

nixpanic pushed a commit to nixpanic/ceph-csi that referenced this pull request Jun 21, 2022

Merge pull request ceph#97 from ceph/devel

f219445

sync downstream devel branch with upstream

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add block supports to rbd driver #97

[WIP] Add block supports to rbd driver #97

mkimuram commented Nov 1, 2018

mkimuram Nov 1, 2018

rootfs Nov 2, 2018

rootfs Nov 2, 2018

mkimuram commented Nov 1, 2018

rootfs Nov 2, 2018

mkimuram Nov 6, 2018

rootfs Nov 2, 2018

mkimuram Nov 6, 2018

rootfs Nov 2, 2018

mkimuram Nov 6, 2018

mkimuram commented Nov 6, 2018

mkimuram commented Nov 7, 2018

rootfs Nov 7, 2018

mkimuram Nov 7, 2018

rootfs commented Nov 9, 2018

mkimuram commented Nov 20, 2018

rootfs commented Nov 21, 2018

rootfs commented Jan 16, 2019

rootfs commented Jan 17, 2019

[WIP] Add block supports to rbd driver #97

[WIP] Add block supports to rbd driver #97

Conversation

mkimuram commented Nov 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkimuram commented Nov 1, 2018

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

mkimuram commented Nov 6, 2018

mkimuram commented Nov 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rootfs commented Nov 9, 2018

mkimuram commented Nov 20, 2018

rootfs commented Nov 21, 2018

rootfs commented Jan 16, 2019

rootfs commented Jan 17, 2019