mount: support checking multiple kinds of block device driver #4743

yuchen0cc · 2022-07-26T07:05:20Z

Device mapper is the only supported block device driver so far, which seems limiting. Kata Containers can work well with other block devices. It is necessary to enhance supporting of multiple kinds of host block device.

Fixes #4714

Signed-off-by: yuchen.cc yuchen.cc@alibaba-inc.com

yuchen0cc · 2022-07-26T07:30:42Z

Please tell me the porting labels to be applied.

katacontainersbot · 2022-08-26T20:25:41Z

Can one of the admins verify this patch?

bergwolf

A more general question: the DM device check is a workaround for the device mapper snapshotter. Please explain a bit why we need to check for the extra block device types and how they are supposed to work with Kata Containers. Thanks!

bergwolf · 2023-01-04T09:54:21Z

src/runtime/virtcontainers/mount.go

@@ -208,7 +208,7 @@ func getDeviceForPath(path string) (device, error) {

 var blockFormatTemplate = "/sys/dev/block/%d:%d/dm"

-var checkStorageDriver = isDeviceMapper
+var checkStorageDriver = checkBlockDevice


isDeviceMapper is left over but it's a piece of dead code now? And the new code is not unit-tested as the code pieces.

+1 for adding unit tests.

+1 for removing the isDeviceMapper code if not used

shuaichang

Thanks for making the change, very useful to extend block device support outside of device mapper!

shuaichang · 2023-01-06T06:58:28Z

src/runtime/virtcontainers/mount.go

@@ -208,7 +208,7 @@ func getDeviceForPath(path string) (device, error) {

 var blockFormatTemplate = "/sys/dev/block/%d:%d/dm"

-var checkStorageDriver = isDeviceMapper
+var checkStorageDriver = checkBlockDevice


+1 for adding unit tests.

shuaichang · 2023-01-06T06:58:56Z

src/runtime/virtcontainers/mount.go

@@ -226,6 +226,40 @@ func isDeviceMapper(major, minor int) (bool, error) {
 	return false, err
 }

+var blockFormatTemplates = map[string]string{


nit: blockFormatTemplates -> blockFormatFmts?

blockFormatTemplates is consist with old code.

shuaichang · 2023-01-06T07:00:34Z

src/runtime/virtcontainers/mount.go

+}
+
+func checkBlockDevice(major, minor int) (bool, error) {
+	var lastErr error = nil


= nil seems to be unnecessary

shuaichang · 2023-01-06T07:03:25Z

src/runtime/virtcontainers/mount.go

+	for dev, template := range blockFormatTemplates {
+		isBD, err := isBlockDevice(major, minor, template)
+		if isBD {
+			mountLogger().Infof("Found storage driver for %s", dev)


This log seems to be a little off from the context, can we log "device % is identified as a block device"?

shuaichang · 2023-01-06T07:07:56Z

src/runtime/virtcontainers/mount.go

+	_, err := os.Stat(sysPath)
+	if err == nil {
+		return true, nil
+	} else if os.IsNotExist(err) {


I think we can remove the else if, or to add the last return into an else block for consistency

if err == nil { return true, nil } if os.IsNotExist(err) { return false, nil } return false, err

shuaichang · 2023-01-06T07:08:51Z

src/runtime/virtcontainers/mount.go

@@ -208,7 +208,7 @@ func getDeviceForPath(path string) (device, error) {

 var blockFormatTemplate = "/sys/dev/block/%d:%d/dm"

-var checkStorageDriver = isDeviceMapper
+var checkStorageDriver = checkBlockDevice


+1 for removing the isDeviceMapper code if not used

shuaichang · 2023-01-06T07:10:11Z

src/runtime/virtcontainers/mount.go

+			mountLogger().Infof("Found storage driver for %s", dev)
+			return isBD, err
+		} else if err != nil {
+			lastErr = err


Can we make this logic easier to understand like the following? I think if any error occurred while checking can just throw the error without continuing.

isBD, err := isBlockDevice(major, minor, template) if err != nil { return }

yuchen0cc · 2023-01-06T07:53:39Z

@bergwolf Thanks for commenting!
Kata Containers has already supported using block device as container's rootfs, and it is the only way for firecracker.
Device mapper is the only supported block device driver so far, which seems limiting. Actually, Kata Containers can work well with raw image on loop device or overlaybd on tcmu device. It is necessary to enhance supporting of multiple kinds of host block device driver.

I will add unit test and remove unused code.

yuchen0cc · 2023-01-06T11:00:27Z

What does "add-pr-size-label" failed mean ?

shuaichang · 2023-01-10T16:29:41Z

src/runtime/virtcontainers/mount_test.go


 	// fake the block device format
-	blockFormatTemplate = "/sys/dev/char/%d:%d"
-	isDM, err = isDeviceMapper(major, minor)
+	blockFormatTemplates = map[string]string{


Better to reset blockFormatTemplates so in case other case will not be impacted, I think the old code probably missed this part

blockFormatTemplatesOld := blockFormatTemplates defer func() { blockFormatTemplates = blockFormatTemplatesOld }() blockFormatTemplates = map[string]string{ "/dev/tty": "/sys/dev/char/%d:%d", }

It's a good way, I have done so.

shuaichang · 2023-01-10T16:30:30Z

src/runtime/virtcontainers/mount.go

+	for dev, template := range blockFormatTemplates {
+		isBD, err := isBlockDevice(major, minor, template)
+		if err != nil {
+			return false, err


Can we add UTs to cover all the branches for the newly added code?

The unchecked branch is err != nil which only happens when isBlockDevice return error. That happens when os.Stat fails and not because of os.IsNotExist.
I really don't know when it happens. Do you have any idea?

I think the easiest way to accomplish this here is to use a path with an invalid character (the null character \000 will work for both unix and window systems):

fmt.Println("Example: Invalid character") _, err := os.Stat("\000path") fmt.Println(err)

You can override the blockFormatTemplates like you did the tests below to get a path with \000 in it

(Permissions errors should also work, but might be a little hard to setup tests for)

Also, if you need help getting this PR to the finish line, please let me know

@sazzy4o I have supplemented the test as you suggested.
It's really grateful that you can help finishing this PR.

sazzy4o · 2023-11-21T05:31:05Z

@shuaichang Could you please have another look at reviewing this PR?

shuaichang

LGTM

sazzy4o · 2023-11-21T16:21:03Z

@yuchen0cc Could you please merge kata-containers:main into your branch, I think one of the PR checks is failing because the branch is not up to date?

yuchen0cc · 2023-11-22T02:44:14Z

@yuchen0cc Could you please merge kata-containers:main into your branch, I think one of the PR checks is failing because the branch is not up to date?

OK, I've rebase it by mainstream.

sazzy4o · 2023-11-22T03:29:08Z

@bergwolf Could you please review this PR?

sazzy4o · 2023-11-26T19:20:57Z

@yuchen0cc It looks like one of the checks is failing because a749dd1 does not have a commit message body

I think updating it to something like this should work:

mount_test: supplement test for checking block device

Adds test for handling os.Stat error

Signed-off-by: yuchen.cc <yuchen.cc@alibaba-inc.com>

See here for more info on expected commit message format

I hope this is the last change. If you want me to take over changes to this PR, please let me know (I just need permission to push to your fork)

yuchen0cc · 2023-11-27T02:35:23Z

@sazzy4o sorry to forget this checking, I've fixed it.

bergwolf

It looks useful to enable overlaybd for Kata. ~~Could you add a howto document in the docs/how-to/ directory?~~

After discussing with @sazzy4o, it is completely transparent, we do not need a specific overlaybd howto in kata.

bergwolf · 2023-11-29T05:49:48Z

src/runtime/virtcontainers/mount.go

-	//Check if /sys/dev/block/${major}-${minor}/dm exists
-	sysPath := fmt.Sprintf(blockFormatTemplate, major, minor)
+func isBlockDevice(major, minor int, formatTemplate string) (bool, error) {
+	sysPath := fmt.Sprintf(formatTemplate, major, minor)


It does not seem to work for nvme devices. Why not just check for /sys/dev/block/<major:minor>/ and forget about the specific block device types?

I have actually tested the above 3 block types with kata, and I'm not sure kata is compatible with all block types.
Just to be cautious, I make a minimal supportation, and allow for extensions.

All block devices are equal in terms of virtio-block backend. So it doesn't matter where it comes from.

OK, it seems reasonable.

yuchen0cc · 2023-11-30T14:45:50Z

As suggested by @bergwolf , I think there is no longer need to check multiple templates (using map and loop).
So I squash the previous 2 commit and make it a minor change.
@shuaichang @sazzy4o @bergwolf Could you please take a look at this.

bergwolf

lgtm!

yuchen0cc · 2023-12-01T03:43:27Z

TestIsDeviceMapper has been moved from mount_test.go to mount_linux_test
I need to move correspondingly.

bergwolf · 2023-12-01T03:43:31Z

Please test the code locally before sending it for review.

# github.com/kata-containers/kata-containers/src/runtime/virtcontainers [github.com/kata-containers/kata-containers/src/runtime/virtcontainers.test]
virtcontainers/mount_linux_test.go:315:15: undefined: isDeviceMapper
virtcontainers/mount_linux_test.go:[32](https://github.com/kata-containers/kata-containers/actions/runs/7047880519/job/19205798649?pr=4743#step:9:33)1:14: undefined: isDeviceMapper

Device mapper is the only supported block device driver so far, which seems limiting. Kata Containers can work well with other block devices. It is necessary to enhance supporting of multiple kinds of host block device. Fixes kata-containers#4714 Signed-off-by: yuchen.cc <yuchen.cc@alibaba-inc.com>

sazzy4o · 2023-12-01T06:43:55Z

I think that code looks good, but I'll try running locally (I think I should have some time to test locally tomorrow)

sazzy4o · 2023-12-03T06:19:59Z

Tested locally with overlaybd and it works as expected:

$ sudo nerdctl run --cgroup-manager=cgroupfs --runtime io.containerd.run.kata.v2 --snapshotter=overlaybd --rm -it registry.hub.docker.com/overlaybd/redis:6.2.1_obd /bin/sh
# df -h
Filesystem      Size  Used Avail Use% Mounted on
/dev/vda         63G  107M   60G   1% /
tmpfs            64M     0   64M   0% /dev
none            322G  179G  127G  59% /data
shm             996M     0  996M   0% /dev/shm
tmpfs           996M     0  996M   0% /sys/fs/cgroup
kataShared      322G  179G  127G  59% /etc/hosts
#

Notes:

Tested with cloud-hypervisor
overlaybd installed via instructions here
Important: sandbox_cgroup_only=true must be set in your configuration.toml

sazzy4o

Code looks good and works when testing locally

jepio · 2023-12-05T12:05:55Z

/test-arm
/test-s390x

jepio

looks reasonable

katacontainersbot added the size/small Small and simple task label Jul 26, 2022

liubin added the no-backport-needed label Jul 26, 2022

yuchen0cc force-pushed the main branch from 69dbcaa to aab3ee1 Compare July 27, 2022 02:17

bergwolf reviewed Jan 4, 2023

View reviewed changes

shuaichang suggested changes Jan 6, 2023

View reviewed changes

yuchen0cc force-pushed the main branch from aab3ee1 to f2ee577 Compare January 6, 2023 10:46

shuaichang reviewed Jan 10, 2023

View reviewed changes

yuchen0cc force-pushed the main branch from f2ee577 to 951183f Compare April 20, 2023 04:47

katacontainersbot added size/medium Average sized task and removed size/small Small and simple task labels Apr 20, 2023

shuaichang approved these changes Nov 21, 2023

View reviewed changes

yuchen0cc force-pushed the main branch from 05bb2ec to a749dd1 Compare November 22, 2023 02:35

Apokleos added the ok-to-test label Nov 24, 2023

yuchen0cc force-pushed the main branch from a749dd1 to de94ff6 Compare November 27, 2023 02:28

bergwolf reviewed Nov 29, 2023

View reviewed changes

yuchen0cc force-pushed the main branch from de94ff6 to e514b02 Compare November 30, 2023 14:27

katacontainersbot added size/small Small and simple task and removed size/medium Average sized task labels Nov 30, 2023

bergwolf approved these changes Dec 1, 2023

View reviewed changes

yuchen0cc force-pushed the main branch from e514b02 to 1cd1558 Compare December 1, 2023 04:00

sazzy4o approved these changes Dec 3, 2023

View reviewed changes

jepio approved these changes Dec 5, 2023

View reviewed changes

jepio merged commit e2c6b8a into kata-containers:main Dec 5, 2023
158 of 164 checks passed

mount: support checking multiple kinds of block device driver #4743

mount: support checking multiple kinds of block device driver #4743

Conversation

yuchen0cc commented Jul 26, 2022

yuchen0cc commented Jul 26, 2022

katacontainersbot commented Aug 26, 2022

bergwolf 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

shuaichang 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuchen0cc commented Jan 6, 2023

yuchen0cc commented Jan 6, 2023

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

sazzy4o Nov 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sazzy4o commented Nov 21, 2023

shuaichang left a comment

Choose a reason for hiding this comment

sazzy4o commented Nov 21, 2023

yuchen0cc commented Nov 22, 2023 • edited

sazzy4o commented Nov 22, 2023

sazzy4o commented Nov 26, 2023

yuchen0cc commented Nov 27, 2023

bergwolf 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

yuchen0cc commented Nov 30, 2023

bergwolf left a comment

Choose a reason for hiding this comment

yuchen0cc commented Dec 1, 2023 • edited

bergwolf commented Dec 1, 2023

sazzy4o commented Dec 1, 2023

sazzy4o commented Dec 3, 2023

sazzy4o left a comment

Choose a reason for hiding this comment

jepio commented Dec 5, 2023

jepio left a comment

Choose a reason for hiding this comment

sazzy4o Nov 19, 2023 •

edited

yuchen0cc commented Nov 22, 2023 •

edited

bergwolf left a comment •

edited

yuchen0cc commented Dec 1, 2023 •

edited