hostpath: Add block volume support #6

darkowlzz · 2019-01-25T08:08:36Z

This change adds block volume support to hostpath driver.

When a block volume request is received, a block file is created at
provisionRoot with the requested capacity as size and a loop device is
created associated with the block file.

At node publish, a bind mount of the loop device is created at the
publish target path.

At node unpublish, the target path is unmounted and deleted.

At volume delete, loop device is disassociated and the block file is
deleted.

Add plugins-dir to hostpath plugin daemonset
The volume publish target path for block devices are usually under
/var/lib/kubelet/plugins directory. Hence, adding plugins directory to
the pod volumes with bidirectional mount propagation.

Run the plugin as privileged to use loop devices
In order to share loop devices with the host, the plugin container must
be run as a privileged container.

darkowlzz · 2019-01-30T18:41:58Z

I tried this with canary image of csi-provisioner and the ValidateVolumeCapabilities changes from #5, and it worked as expected. Created a block device and was able to access the device from a pod under /dev/.

root@my-csi-app:/# ls -l /dev/xfoo 
brw-rw---- 1 root disk 7, 0 Jan 30 17:55 /dev/xfoo

The same was also listed in the pod description:

...
    Mounts:
      /var/run/secrets/kubernetes.io/serviceaccount from default-token-f7df9 (ro)
    Devices:
      /dev/xfoo from my-csi-volume
...

The publish target path for block devices are at /var/lib/kubelet/plugins, hence we have to add that path to the pod with bidirectional mount propagation.

bswartz · 2019-01-30T19:17:10Z

pkg/hostpath/controllerserver.go

+		of := fmt.Sprintf("%s=%s", "of", path)
+		count := fmt.Sprintf("%s=%d", "count", capacity/mib)
+		// Create a block file.
+		out, err := executor.Command("dd", "if=/dev/zero", of, "bs=1M", count).CombinedOutput()


Why not use fallocate [1] instead of dd? It's way faster.

[1] http://man7.org/linux/man-pages/man1/fallocate.1.html

bswartz · 2019-01-30T19:21:11Z

pkg/hostpath/nodeserver.go

+
+		executor := utilexec.New()
+		// Get a free loop device.
+		out, err := executor.Command("losetup", "-f").CombinedOutput()


Two node publish calls can race here and choose the same loop device. It's possible to remove those invocation of losetup and just pass the -f flag to the invocation below, then also pass --show to find out which device it chose.

In k/k, makeLoopDevice is doing the way that @bswartz mentioned. So, it would be worth checking this code.

bswartz · 2019-01-30T19:22:46Z

pkg/hostpath/nodeserver.go

+		losetupArgs = append(losetupArgs, loopDevice, vol.VolPath)
+
+		// Associate block file with the loop device.
+		out, err = executor.Command("losetup", losetupArgs...).CombinedOutput()


This is not idempotent. If you get killed between the call to "losetup" and "ln", when you restart and come back into this method, you'll create another loop device, leaking the old one.

bswartz · 2019-01-30T19:26:00Z

pkg/hostpath/nodeserver.go

+		}
+
+		// Create symlink to the target path.
+		out, err = executor.Command("ln", "-s", loopDevice, targetPath).CombinedOutput()


You need a check higher up in this function to see if the symlink already exists and points to the right place, for idempotency. Otherwise you'll end up in a failure loop when the link already exists because "ln" without "-f" won't overwrite existing symlinks.

IIUC, for idempotency, CSI community recommends to use bind mount of device, instead of symlink. If you use bind mount, your can check if it is already done, by checking if file already exists in targetPath and if the loopDevice is already mounted to targetPath.

bswartz · 2019-01-30T21:52:37Z

pkg/hostpath/nodeserver.go

+	switch vol.VolAccessType {
+	case blockAccess:
+		// Remove the symlink.
+		os.RemoveAll(targetPath)


Error check?

mkimuram

@darkowlzz

Thank you for working on adding block volume support codes to csi hostpath driver.
It's a good idea to make loopback device visible to pod as a block volume for drivers that originally provide storage as filesystem, not block volume.

I'm adding some comments.

mkimuram · 2019-01-31T16:29:04Z

pkg/hostpath/nodeserver.go

+
+		executor := utilexec.New()
+		// Get a free loop device.
+		out, err := executor.Command("losetup", "-f").CombinedOutput()


In k/k, makeLoopDevice is doing the way that @bswartz mentioned. So, it would be worth checking this code.

mkimuram · 2019-01-31T16:36:45Z

pkg/hostpath/nodeserver.go

+		}
+
+		// Create symlink to the target path.
+		out, err = executor.Command("ln", "-s", loopDevice, targetPath).CombinedOutput()


IIUC, for idempotency, CSI community recommends to use bind mount of device, instead of symlink. If you use bind mount, your can check if it is already done, by checking if file already exists in targetPath and if the loopDevice is already mounted to targetPath.

bswartz · 2019-02-01T19:55:01Z

BTW I understand that the host path driver isn't meant for production and is just for testing, so my comments about idempotency are just suggestions. Given the low likelihood that non-idempotency will cause any actual issues during testing, I wouldn't spend a ton of time on that. Still there are a few sections of code that could be improved easily. See mkimuram's comments.

darkowlzz · 2019-02-17T15:16:17Z

Hi, I've added some more commits addressing the previous reviews. Keeping the old commits for now, in case I need to refer back. Will squash all the commits once the changes are final.

I moved the loop device creation from NodePublish to CreateVolume and also moved the corresponding loop device deletion from NodeUnpublish to DeleteVolume. Now NodePublish just checks the symlink targets of a given target path and creates symlink if needed. And NodeUnpublish deletes the symlinks. The volume ID is set to the loop device base name, avoiding the need to maintain any volume info in memory.

Regarding the usage of makeLoopDevice (#6 (comment)) like in k/k, I was unable to do that with the current version of losetup in alpine base container. Looks like the losetup packed with busybox is old and doesn't have --show option. Hence, doing losetup -f /file/path would associate the file to a loop device, but we can't get the name of the loop device used. Should we use some other base container image or maybe replace the old losetup with a new losetup binary?

Regarding using bind mount (#6 (comment)), I'm a little confused about that. Trying to bind mount a loop device associated with a block device fails unless the block device has a filesystem. AFAIK, we are trying to provide raw block here. Should we format it and then bind mount?
Maybe I'm missing something here. I would need some help in understanding that.

Also, I've added a security context to run the plugin as privileged in the plugin deployment manifest. Without this, loop device creation was failing with file not found error, because it wasn't shared with the host.

pohly · 2019-02-18T11:02:55Z

The version of losetup in the base alpine image is probably from busybox. You should be able to install the util-linux package as part of the Dockerfile to get the full-featured version.

darkowlzz · 2019-02-18T11:14:55Z

The version of losetup in the base alpine image is probably from busybox. You should be able to install the util-linux package as part of the Dockerfile to get the full-featured version.

Yeah, I just tried doing apk add util-linux in an alpine container and that updated losetup to a newer version with support for --show. Thanks, I'll add this in the Dockerfile.

bswartz · 2019-02-22T22:00:36Z

Regarding using bind mount (#6 (comment)), I'm a little confused about that. Trying to bind mount a loop device associated with a block device fails unless the block device has a filesystem. AFAIK, we are trying to provide raw block here. Should we format it and then bind mount?
Maybe I'm missing something here. I would need some help in understanding that.

When you bind mount a file, the destination must be an existing file. There's no need for a filesystem.

This change adds block volume support to hostpath driver. When a block volume request is received, a block file is created at provisionRoot with the requested capacity as size and a loop device is created associated with the block file. At node publish, a bind mount of the loop device is created at the publish target path. At node unpublish, the target path is unmounted and deleted. At volume delete, loop device is disassociated and the block file is deleted. Add plugins-dir to hostpath plugin daemonset The volume publish target path for block devices are usually under /var/lib/kubelet/plugins directory. Hence, adding plugins directory to the pod volumes with bidirectional mount propagation. Run the plugin as privileged to use loop devices In order to share loop devices with the host, the plugin container must be run as a privileged container.

darkowlzz · 2019-02-25T14:42:13Z

Block support works with bind mount now :)

I made some changes to how the statelessness was implemented. Instead of keeping the loop device name in volume ID, it is now queried through the updated version of losetup (-j option). Once a file is associated with a loop device, whenever there's a need to know the loop device name, it is queried through losetup with the block file as an argument.

Added new files in vendor because I've used the functions available in k8s.io/kubernetes/pkg/volume/util/volumepathhandler to attach device and get loop device.

bswartz · 2019-03-04T18:39:48Z

/lgtm

msau42 · 2019-03-04T22:43:35Z

/approve
Please open up followup issues for the remaining work

k8s-ci-robot · 2019-03-04T22:43:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: darkowlzz, msau42

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

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 childsb and saad-ali January 25, 2019 08:08

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 25, 2019

darkowlzz mentioned this pull request Jan 25, 2019

hostpath: Add block volume support kubernetes-retired/drivers#143

Closed

bswartz reviewed Jan 30, 2019

View reviewed changes

mkimuram reviewed Jan 31, 2019

View reviewed changes

darkowlzz force-pushed the hostpath-block-support branch from 1a9ef86 to 083b8fc Compare February 17, 2019 14:58

darkowlzz force-pushed the hostpath-block-support branch from 083b8fc to 267cbdc Compare February 25, 2019 14:27

darkowlzz force-pushed the hostpath-block-support branch from 267cbdc to ea76c5b Compare February 25, 2019 14:31

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 25, 2019

k8s-ci-robot assigned bswartz Mar 4, 2019

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 4, 2019

msau42 mentioned this pull request Mar 4, 2019

sync up master with "drivers" #22

Closed

k8s-ci-robot merged commit b862d4c into kubernetes-csi:master Mar 4, 2019

msau42 mentioned this pull request Mar 4, 2019

Promote CSI raw blocks to beta kubernetes/kubernetes#74909

Merged

pohly mentioned this pull request Apr 30, 2019

master not a complete copy of the original code #21

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hostpath: Add block volume support #6

hostpath: Add block volume support #6

darkowlzz commented Jan 25, 2019 •

edited

Loading

darkowlzz commented Jan 30, 2019

bswartz Jan 30, 2019

bswartz Jan 30, 2019

mkimuram Jan 31, 2019

bswartz Jan 30, 2019

bswartz Jan 30, 2019

mkimuram Jan 31, 2019

bswartz Jan 30, 2019

mkimuram left a comment

mkimuram Jan 31, 2019

mkimuram Jan 31, 2019

bswartz commented Feb 1, 2019

darkowlzz commented Feb 17, 2019

pohly commented Feb 18, 2019

darkowlzz commented Feb 18, 2019

bswartz commented Feb 22, 2019

darkowlzz commented Feb 25, 2019 •

edited

Loading

bswartz commented Mar 4, 2019

msau42 commented Mar 4, 2019

k8s-ci-robot commented Mar 4, 2019

hostpath: Add block volume support #6

hostpath: Add block volume support #6

Conversation

darkowlzz commented Jan 25, 2019 • edited Loading

darkowlzz commented Jan 30, 2019

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

mkimuram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bswartz commented Feb 1, 2019

darkowlzz commented Feb 17, 2019

pohly commented Feb 18, 2019

darkowlzz commented Feb 18, 2019

bswartz commented Feb 22, 2019

darkowlzz commented Feb 25, 2019 • edited Loading

bswartz commented Mar 4, 2019

msau42 commented Mar 4, 2019

k8s-ci-robot commented Mar 4, 2019

darkowlzz commented Jan 25, 2019 •

edited

Loading

darkowlzz commented Feb 25, 2019 •

edited

Loading