Devicemapper storage #997

amshinde · 2017-06-17T06:13:41Z

Check for devicemapper storage and use the underlying block device instead of overlay file system.

amshinde · 2017-06-18T18:03:13Z

Will be adding in more unit tests later.

jodh-intel · 2017-06-19T07:35:37Z

src/util.c

+// Legacy naming scheme for virtio block devices
+// Reference : https://github.com/torvalds/linux/blob/master/drivers/block/virtio_blk.c#L478
+static int
+virtblk_name_format(char *prefix, int index, char *buf, int buflen)


What if prefix or buf are NULL?

I have added checks for these

jodh-intel · 2017-06-19T08:37:11Z

src/util.c

+}
+
+// Legacy naming scheme for virtio block devices
+// Reference : https://github.com/torvalds/linux/blob/master/drivers/block/virtio_blk.c#L478


Ah - I see that you've copied this verbatim (with a very minor tweak to appease a compiler warning?) Don't we atleast need a copyright attribution here for the FSF?

@as per the discussion on this, I have updated the reference to include the git sha

grahamwhaley

Nice series @amshinde A few minor comments.

grahamwhaley · 2017-06-19T10:42:48Z

data/hypervisor.args.in

@@ -13,10 +13,6 @@ memory-backend-file,id=mem0,mem-path=@IMAGE@,size=@SIZE@
 @KERNEL@


In commit message s/diectory/directory/

grahamwhaley · 2017-06-19T10:50:48Z

src/mount.c

+	struct cc_oci_mount *m;
+
+	if (! config ) {
+		return false;


Returning 'false' in a func that returns a pointer feels odd - would a 'NULL' be more appropriate? @jodh-intel , do you have a preferred way in this codebase?

Ah, as I see we do a NULL at the error: exit - so, probably change this one to NULL?

yes, this needs to be NULL. Fixing this.

grahamwhaley · 2017-06-19T10:53:10Z

src/mount.c

+		goto error;
+	}
+
+	g_warning("Rootfs bind mounting done");


Silly question - should this be a warning? Seems we did what we wanted to - do we have the option of an info or similar?

This needs to be a debug message, will fix it

grahamwhaley · 2017-06-19T10:54:29Z

src/pod.c

@@ -32,6 +32,7 @@
 #include "process.h"
 #include "proxy.h"
 #include "state.h"
+#include "mount.h"


query: @jodh-intel do we tend to keep the includes alphabetical?

Changing the order here

grahamwhaley · 2017-06-19T11:02:41Z

src/mount.c

@@ -22,6 +22,7 @@
 #include <errno.h>


In the commit header:

missing space 'container.The'

missing space 'above.The'

missing space 'passthis'

fractured line '^name$'

Question: in the path '{runtime_dir}/cid/workload/cid/rootfs' - I wonder if there is a reason for the two cid parts - maybe it has something to do with pods, maybe I'll find a note in the code. Noting here before I forget.

grahamwhaley · 2017-06-19T12:04:40Z

src/mount.c

+	}
+
+	if (stat(path, &cur_stat) == -1) {
+		return NULL;


Should we have a g_warning for this one?

grahamwhaley · 2017-06-19T12:08:17Z

src/mount.c

+	 */
+
+	file = setmntent(PROC_MOUNTS_FILE, "r");
+	if ( !file) {


bracket whitespace inconsistency?

grahamwhaley · 2017-06-19T12:24:36Z

src/oci.c

@@ -229,6 +229,11 @@ cc_oci_get_config_and_state (gchar **config_file,
 			(*state)->procsock_path,


nit: ' , ' whitespace in commit message, and this one is 'tests' and the previous one is 'test' :-)

grahamwhaley · 2017-06-19T12:27:16Z

src/util.c

@@ -740,6 +745,55 @@ get_random_bytes(uint num) {
 	return buf;
 }

+// Legacy naming scheme for virtio block devices


I'll note, these are the only C++ style comments in this file ;-)

grahamwhaley · 2017-06-19T12:30:43Z

src/util.c

+	char *p;
+	int unit;
+
+	p = end - 1;


well, there is a mini mind melting algo - if we'd written it, I'd be asking for an explanatory comment - but, as it is a clone of the 'standard', that's fine.

chavafg · 2017-06-19T12:48:52Z

qa-failed

chavafg · 2017-06-19T13:29:15Z

Got this functional test error:

not ok 3 exec false
# (from function `verify_runtime_dirs' in file tests/functional/common.bash, line 161,
#  from function `testcontainer' in file tests/functional/common.bash, line 227,
#  in test file tests/functional/exec.bats, line 53)
#   `testcontainer "$container_id" "running"' failed
# Running: timeout 5 /home/cloud/go/src/github.com/01org/cc-oci-runtime/tests/functional/../../cc-oci-runtime  --debug --root /tmp/tmp.DexA6eLXsi-cor-test --global-log /run/cc-oci-runtime/cc-oci-runtime.log --hypervisor-log-dir /tmp/tmp.JwlAyuE8r2-cor-test --shim-path "/home/cloud/go/src/github.com/01org/cc-oci-runtime/cc-shim" --proxy-socket-path "/home/cloud/go/src/github.com/01org/cc-oci-runtime/tests/functional/proxy.sock" run -d --bundle /var/lib/oci/bundle tests_id
# this is a test
# README
# README.rst
# common.bash
# common.bash.in
# data
# exec.bats
# help.bats
# kill.bats
# pause.bats
# proxy.sock
# run-functional-tests.sh
# run-functional-tests.sh.in
# start.bats
# state.bats
# version.bats
# README
# README.rst
# common.bash
# common.bash.in
# data
# exec.bats
# help.bats
# kill.bats
# pause.bats
# proxy.sock
# run-functional-tests.sh
# run-functional-tests.sh.in
# start.bats
# state.bats
# version.bats
# exit code: 0
# expected exit code: 0
# Running: /home/cloud/go/src/github.com/01org/cc-oci-runtime/tests/functional/../../cc-oci-runtime  --debug --root /tmp/tmp.DexA6eLXsi-cor-test --global-log /run/cc-oci-runtime/cc-oci-runtime.log --hypervisor-log-dir /tmp/tmp.JwlAyuE8r2-cor-test --shim-path "/home/cloud/go/src/github.com/01org/cc-oci-runtime/cc-shim" --proxy-socket-path "/home/cloud/go/src/github.com/01org/cc-oci-runtime/tests/functional/proxy.sock" list --format table | grep tests_id | grep running
# exit code: 0
# expected exit code: 0
# SUCCESS
# Test logs: /tmp/tmp.DexA6eLXsi-cor-test
# Global log file: /run/cc-oci-runtime/cc-oci-runtime.log
# Hypervisor log dir: /tmp/tmp.JwlAyuE8r2-cor-test
# bats status: 0
# bats output (lines array contains 7 lines):
# lines[0]: 'console.sock'
# lines[1]: 'ga-ctl.sock'
# lines[2]: 'ga-tty.sock'
# lines[3]: 'hypervisor.sock'
# lines[4]: 'process.sock'
# lines[5]: 'state.json'
# lines[6]: 'workload'
# BUNDLE_DIR: /var/lib/oci/bundle
# ROOTFS_DIR: /var/lib/oci/bundle/rootfs

Seems like lines[6]: 'workload' is the issue:

[ "${lines[6]}" = "" ]

from https://github.com/01org/cc-oci-runtime/blob/master/tests/functional/common.bash.in#L161

dlespiau · 2017-06-19T13:55:46Z

This is quite a large change. Should we open master to 2.2.x and branch out a 2.1.x stable branch?

grahamwhaley · 2017-06-19T13:59:49Z

@dlespiau ack from me - good idea on the version bump

jcvenegas · 2017-06-19T15:57:21Z

Running checkcommits version 0.0.1 (commit 2f18bd36aedcb97213dc794be2d5fb1306f9015e)
Detected TravisCI Environment
Found 21 commits between commit db9196b and branch master
Checking commit 4e96669
Checking commit e9ddabc
Checking commit 867e277
Checking commit 93b13a3
Checking commit 56bc092
Checking commit 33c8aba
Checking commit 00189f1
Checking commit 2035ae6
Checking commit 9b66cf2
Checking commit 9097610
Checking commit f97694e
Commit f97694e: single-line "Signed-off-by" body not permitted

Instead of having static options for the 9pfs directory in hypervisor.args file, pass them dynamically. We want to have the ability to change the storage options. Signed-off-by: Archana Shinde <[email protected]>

chavafg · 2017-06-19T20:01:11Z

qa-failed

chavafg · 2017-06-19T21:52:43Z

qa-failed

sameo · 2017-06-20T09:34:11Z

src/mount.c

+		if (! volume) {
+			/* bind mount container rootfs. We do this for regular
+			 * container as well for a pod sandbox.
+			 */


Here you're also bind mounting the rootfs for pod containers. I don't think this is correct.

Will change that to include cc_pod_is_vm() check. Although this bind-mounting change with the added check doesnt seem to play well with crio. Still taking a look at this.

sameo · 2017-06-20T09:45:09Z

src/util.c

+ * \return \c Newly allocated buffer on success, NULL otherwise.
+ **/
+uint8_t *
+get_random_bytes(uint num) {


You really want to make that one a wrapper on top of getramdom.

@sameo Seems like this is new system call introduced in 3.17, would not be available in older kernels. The system call itself was added very recently in glibc: https://lwn.net/Articles/711013/.
I think reading from /dev/urandom is fairly standard way to get random data.

sameo · 2017-06-20T10:00:42Z

src/proxy.c

+	}
+
+	fsmap_arr = json_array_new();
+	for (l = config->oci.mounts; l && l->data; l = g_slist_next (l)) {


So this is only handling volumes, or both volumes and rootfs ?

Just the volumes. The oci.mounts should contain only volumes right? config->rootfs_mounts should contain just the rootfs mounts if I have understood correctly.

For pods, that's correct. I see you changed that logic with that PR, and that sounds reasonable to me.

sameo · 2017-06-20T10:11:09Z

src/mount.c

+               return false;
+       }
+
+       if (! cc_is_devicemapper(major, minor)) {


Could we make that one a little more generic, and have a cc_is_blockdevice() that could eventually also support other block device based graph drivers ?

sameo · 2017-06-20T10:12:59Z

src/mount.c

+ * \return \c true on success, else \c false.
+ */
+gboolean
+cc_oci_check_for_devicemapper(struct cc_oci_config *config)


We could make that one a little more generic and call it e.g. cc_oci_rootfs_is_block_device()

sameo · 2017-06-20T10:16:33Z

src/util.c

@@ -740,6 +745,65 @@ get_random_bytes(uint num) {
 	return buf;
 }

+// Legacy naming scheme for virtio block devices
+// Reference : https://github.com/torvalds/linux/blob/master/drivers/block/virtio_blk.c#L478


This specific file happens to not have any explicit copyright owners, so we should not add any attribution to our file.
For people to check who wrote this code, please replace that comment with:

// Reference: linux/blob/master/drivers/block/virtio_blk.c @ <SHA1>

From there a git log will tell who wrote the code before we copied it. Additionally, potential future copyright owners will not be able to claim ownership of this code.

sameo · 2017-06-20T10:18:40Z

@dlespiau The version bump makes sense, this is a significant change. As for the branch, opening a stable branch means this is something we'll have to maintain. I'd rather not be doing this.

amshinde · 2017-06-21T06:10:00Z

@sameo I have addressed your other review comments.

sameo · 2017-06-21T09:31:42Z

LGTM

chavafg · 2017-06-21T12:58:08Z

qa-failed

chavafg · 2017-06-21T13:37:23Z

qa-failed

chavafg · 2017-06-21T15:01:26Z

qa-passed

jcvenegas · 2017-06-21T15:13:59Z

@jodh-intel could you review this PR again ?

These will be needed to unmount the rootfs while stopping the container. Signed-off-by: Archana Shinde <[email protected]>

Now that we bind mount the container rootfs to a host directory, pass that to hyperstart. Signed-off-by: Archana Shinde <[email protected]>

This array tells hyperstart where to mount the mounts passed in the shared directory. Signed-off-by: Archana Shinde <[email protected]>

In case of a standalone container, unmount the rootfs from the shared hyperstart directory. Signed-off-by: Archana Shinde <[email protected]>

We perform the mounts on the hyperstart shared directory on the host namespace. So do not try to join the mount namespace while unmounting. Signed-off-by: Archana Shinde <[email protected]>

We need to update the rootfs mount path and the workload dir from the state file that were saved while creating the container. Signed-off-by: Archana Shinde <[email protected]>

This function returns the major and minor numbers of the underlying device on which the file resides. Signed-off-by: Archana Shinde <[email protected]>

The function checks whether a device identified by its major and minor numbers is a devicemapper block device. Signed-off-by: Archana Shinde <[email protected]>

Given a path, the function returns the mount point to which the path belongs. This is required for getting the device name to which a file system path belongs. Signed-off-by: Archana Shinde <[email protected]>

Given a mountpoint, the function returns the device name that has been mounted at the mount point and the file system type of the mount. Signed-off-by: Archana Shinde <[email protected]>

If a block device is found, we store the device name of the block device found. We also store the file system type and index. We are currently using an incrementing counter for the index. When we add support for CRIO with hot plugging drives, so that containers with block drives can be added after the VM is started, we should store the global index in a file and retrieve it from there. The index is needed to derive the drive name that we pass to qemu. We need to pass the drive name and file system type to hyperstart. Signed-off-by: Archana Shinde <[email protected]>

These will be needed while starting a container. Signed-off-by: Archana Shinde <[email protected]>

We check for block storage for standalone container. If block device is detected, we pass that to qemu. Skip bind-mounting the rootfs in that case. Signed-off-by: Archana Shinde <[email protected]>

For hyperstart to use the drive passed on qemu command line, we pass the drive name and the file system type of the block device. The drive name needs to be passed as image field in the newcontainer json. The drive name is deduced based on the index at which the drive is passed to qemu. Signed-off-by: Archana Shinde <[email protected]>

We no longer check for namespace and return true while unmounting. Change test according to this. Signed-off-by: Archana Shinde <[email protected]>

Now that workload_dir is required, fix the current state tests. Signed-off-by: Archana Shinde <[email protected]>

The runtime directory should now include "workloads" directory. Fix functional tests to address this. Signed-off-by: Archana Shinde <[email protected]>

jcvenegas

It is needed to update hyperstart (clearcontainers/hyperstart@2846708) in cc image before merge this change

docker cp is not expected to work while using block device instead of the overlay file system. Skip docker cp test in case of devicemapper storage driver. Signed-off-by: Archana Shinde <[email protected]>

chavafg · 2017-06-21T20:16:46Z

qa-passed

jcvenegas · 2017-06-21T21:42:51Z

just tested with hyperstart changes, tested that now dnf works with devicemapper, after the image is updated this.

chavafg · 2017-06-22T14:07:36Z

qa-passed

chavafg · 2017-06-22T15:26:28Z

I have tested this on Ubuntu, Fedora, Centos and ClearLinux with both overlay and devicemapper as the docker storage driver and no regressions found.

lgtm

amshinde force-pushed the devicemapper-storage branch 6 times, most recently from f984063 to 34ec45f Compare June 18, 2017 18:01

jodh-intel suggested changes Jun 19, 2017

View reviewed changes

grahamwhaley reviewed Jun 19, 2017

View reviewed changes

9pfs: Dynamically add 9pfs options to qemu.

5249cb4

Instead of having static options for the 9pfs directory in hypervisor.args file, pass them dynamically. We want to have the ability to change the storage options. Signed-off-by: Archana Shinde <[email protected]>

amshinde force-pushed the devicemapper-storage branch from 34ec45f to 82484ad Compare June 19, 2017 19:14

amshinde requested a review from sameo June 19, 2017 21:06

sameo reviewed Jun 20, 2017

View reviewed changes

amshinde force-pushed the devicemapper-storage branch 3 times, most recently from e460237 to 05fc7bb Compare June 21, 2017 02:17

amshinde added 17 commits June 21, 2017 09:26

state: Store the workload dir and rootfs mount to state.

0af8583

These will be needed to unmount the rootfs while stopping the container. Signed-off-by: Archana Shinde <[email protected]>

proxy: Pass shared directory to hyperstart.

27f25fc

Now that we bind mount the container rootfs to a host directory, pass that to hyperstart. Signed-off-by: Archana Shinde <[email protected]>

proxy: Pass hyperstart fsmap array with newcontainer command.

61b8b8b

This array tells hyperstart where to mount the mounts passed in the shared directory. Signed-off-by: Archana Shinde <[email protected]>

mounts: Unmount the container rootfs

9d8a083

In case of a standalone container, unmount the rootfs from the shared hyperstart directory. Signed-off-by: Archana Shinde <[email protected]>

namespace: Do not join mount namespace while unmounting.

f202ba4

We perform the mounts on the hyperstart shared directory on the host namespace. So do not try to join the mount namespace while unmounting. Signed-off-by: Archana Shinde <[email protected]>

state: Update the rootfs and workload dir config from state

69abef6

We need to update the rootfs mount path and the workload dir from the state file that were saved while creating the container. Signed-off-by: Archana Shinde <[email protected]>

mount: Add function to get underlying device for a path.

2c8374d

This function returns the major and minor numbers of the underlying device on which the file resides. Signed-off-by: Archana Shinde <[email protected]>

storage: Add function to check if device is a devicemapper device

f9c0d36

The function checks whether a device identified by its major and minor numbers is a devicemapper block device. Signed-off-by: Archana Shinde <[email protected]>

mount: Add function to get the mount point given a path

b1768e8

Given a path, the function returns the mount point to which the path belongs. This is required for getting the device name to which a file system path belongs. Signed-off-by: Archana Shinde <[email protected]>

mount: Add function to return the device name and file system type

e05e368

Given a mountpoint, the function returns the device name that has been mounted at the mount point and the file system type of the mount. Signed-off-by: Archana Shinde <[email protected]>

state: Store the block storage device file system type and index

b95748f

These will be needed while starting a container. Signed-off-by: Archana Shinde <[email protected]>

storage: Pass block device to qemu if found.

dd13e32

We check for block storage for standalone container. If block device is detected, we pass that to qemu. Skip bind-mounting the rootfs in that case. Signed-off-by: Archana Shinde <[email protected]>

tests: Fix mounts test for unmounting volume.

b8ffdb1

We no longer check for namespace and return true while unmounting. Change test according to this. Signed-off-by: Archana Shinde <[email protected]>

tests: Fix state tests and add check for workload_dir.

66cbd66

Now that workload_dir is required, fix the current state tests. Signed-off-by: Archana Shinde <[email protected]>

tests: Fix functional tests to include workload directory

9b91166

The runtime directory should now include "workloads" directory. Fix functional tests to address this. Signed-off-by: Archana Shinde <[email protected]>

amshinde force-pushed the devicemapper-storage branch from 05fc7bb to 9b91166 Compare June 21, 2017 16:27

jodh-intel approved these changes Jun 21, 2017

View reviewed changes

jcvenegas suggested changes Jun 21, 2017

View reviewed changes

tests: Skip docker cp integration test for devicemapper storage

271223a

docker cp is not expected to work while using block device instead of the overlay file system. Skip docker cp test in case of devicemapper storage driver. Signed-off-by: Archana Shinde <[email protected]>

amshinde mentioned this pull request Jun 21, 2017

version: Update container image to 16020 #1002

Merged

jcvenegas mentioned this pull request Jun 22, 2017

Release 2.2.0 #1003

Closed

25 tasks

jcvenegas approved these changes Jun 22, 2017

View reviewed changes

jcvenegas merged commit 50b6d6f into intel:master Jun 22, 2017

jodh-intel mentioned this pull request Jun 27, 2017

Create a set of data integrity tests (for devicemapper) #1017

Open

		@@ -13,10 +13,6 @@ memory-backend-file,id=mem0,mem-path=@IMAGE@,size=@SIZE@
		@KERNEL@

		@@ -229,6 +229,11 @@ cc_oci_get_config_and_state (gchar **config_file,
		(*state)->procsock_path,

Devicemapper storage #997

Devicemapper storage #997

Conversation

amshinde commented Jun 17, 2017

amshinde commented Jun 18, 2017

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

grahamwhaley 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

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

chavafg commented Jun 19, 2017 • edited by dlespiau Loading

chavafg commented Jun 19, 2017

dlespiau commented Jun 19, 2017 • edited Loading

grahamwhaley commented Jun 19, 2017

jcvenegas commented Jun 19, 2017

chavafg commented Jun 19, 2017

chavafg commented Jun 19, 2017

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

sameo commented Jun 20, 2017

amshinde commented Jun 21, 2017

sameo commented Jun 21, 2017 • edited by dlespiau Loading

chavafg commented Jun 21, 2017

chavafg commented Jun 21, 2017

chavafg commented Jun 21, 2017 • edited by dlespiau Loading

jcvenegas commented Jun 21, 2017

jcvenegas left a comment

Choose a reason for hiding this comment

chavafg commented Jun 21, 2017

jcvenegas commented Jun 21, 2017

chavafg commented Jun 22, 2017

chavafg commented Jun 22, 2017 • edited by dlespiau Loading

chavafg commented Jun 19, 2017 •

edited by dlespiau

Loading

dlespiau commented Jun 19, 2017 •

edited

Loading

sameo commented Jun 21, 2017 •

edited by dlespiau

Loading

chavafg commented Jun 21, 2017 •

edited by dlespiau

Loading

chavafg commented Jun 22, 2017 •

edited by dlespiau

Loading