kvm: new volumes support #2328

jellonek · 2016-03-25T15:35:41Z

This PR provides new volumes support, based on bind mounting volumes in time of rootfs preparation, removing requirement to pass each volume separately through 9p. It also simplifies volumes support in kvm flavor.

Also - new approach closes #2263
Also - this PR adds possibility to choose as source symlink.

alban · 2016-03-29T09:39:08Z

Are the bind mounts done in the host mount namespace or another mount namespace?

jellonek · 2016-03-29T11:16:10Z

They are done in host namespace.

alban · 2016-03-29T13:33:11Z

stage1/init/common/kvm_mount.go

-		// TODO: verify if rkt should do this, or it should be some outer responsibility
-		// to ensure if such patch exists
-		if err := ensureDestinationExists(source, destination); err != nil {
+		// TODO: check if symlinked source tries to escape root


It could reuse code from https://github.com/coreos/rkt/pull/2290/files

evaluateAppMountPath is already invocated few lines above, but it's called for destination.
I have a limited understanding of https://github.com/coreos/rkt/blob/master/tests/rkt_mount_test.go#L68 test - how anything can escape in this situation root? (this test is failing on kvm even after this patch).

iaguis · 2016-03-29T15:02:15Z

stage1/usr_from_kvm/lkvm/patches/do_synchronous_writes.patch

+
+ 	if (kvm->cfg.using_rootfs) {
+-		strcat(real_cmdline, " rw rootflags=trans=virtio,version=9p2000.L,cache=loose rootfstype=9p");
+		strcat(real_cmdline, " rw rootflags=trans=virtio,version=9p2000.L,cache=none rootfstype=9p");


Why this change?

Argh, was sure that commented this. We had an issue connected with these caches. Without this change some data could be lost if they are saved by application just before pod stopping. For example if I started docker://busybox in interactive mode, added some volume, echoed some value into new file in this volume and then quickly closed pod - file was created, but without expected content inside.
Changing this option helped.
Change was done basing on information from https://www.kernel.org/doc/Documentation/filesystems/9p.txt

could you add such a comment to the code or commit log?

Yes, I'll do this. I'll extract this single change into separate commit.

krnowak · 2016-03-31T14:21:47Z

Needs rebase.

jellonek · 2016-03-31T14:25:25Z

ACK

jellonek · 2016-03-31T15:23:30Z

Done, also fix for mounting symlinks is updated and fully working.

Still kvm flavor will not pass functional tests, because of that we cannot pass exit status value from pod as lkvm exit value (i can workaround this spawning lkvm as child process, but this breaks idea of execing onto lkvm at the end of run process - so imo it's ugly).

alban · 2016-03-31T15:52:44Z

Sorry we didn't get to review this in time for v1.3.0. We will get it for the next milestone.

jellonek · 2016-04-01T08:56:17Z

ACK.

jellonek · 2016-04-05T11:58:56Z

Review request.

alban · 2016-04-05T12:57:26Z

stage1/init/common/pod.go

-		err := AppToSystemdMountUnits(common.Stage1RootfsPath(p.Root), appName, p.Manifest.Volumes, ra, UnitsDir)
-		if err != nil {
+		// bind mount all shared volumes (we don't use mechanism for bind-mounting given by nspawn)
+		if err := MountSharedVolumes(common.Stage1RootfsPath(p.Root), appName, p.Manifest.Volumes, ra); err != nil {


Since MountSharedVolumes is not related to systemd-nspawn parameters nor systemd units, I think the function call should not be in stage1/init/common/pod.go:appToSystemd but directly in stage1/init/init.go:stage1(). It would require to write another loop on p.Manifest.Apps though. What do you think?

Good idea. Give me some time to implement this, so this PR should be marked as needs rework for this purpose.

When using --volume=myvol,kind=host,source=/tmp/myvol-missing with a non-existent directory, the error message is:

stage1: failed to configure systemd: lstat /tmp/myvol-missing: no such file or directory

It should not say "failed to configure systemd" since it is not using systemd or systemd-nspawn anymore.

This will be fixed when I'll address previous comment.

alban · 2016-04-05T14:13:23Z

stage1/init/init.go:stage1():

    if err = stage1initcommon.PodToSystemd(p, interactive, flavor, privateUsers); err != nil {
        log.PrintE("failed to configure systemd", err)
        return 2
    }

    args, env, err := getArgsEnv(p, flavor, debug, n)
    if err != nil {
        log.Error(err)
        return 3
    }

    // create a separate mount namespace so the cgroup filesystems
    // are unmounted when exiting the pod
    if err := syscall.Unshare(syscall.CLONE_NEWNS); err != nil {
        log.FatalE("error unsharing", err)
    }

With the current PR, the bind mounts are done before the Unshare & the setup of the mnt namespace as slave. So the bind mounts are visible to the host. If the bind mounts were done just after (line 613), they would not be visible to the host. It looks cleaner to me.

jellonek · 2016-04-05T14:24:39Z

Ok, I'll look on this.

jellonek · 2016-04-07T15:33:14Z

@albanc: logic refactored, mounts are done after unshare and refactor of GenerateMounts leaved for #2380

alban · 2016-04-08T10:34:20Z

stage1/init/kvm.go

-// KvmPodToSystemd generates systemd unit files for a pod according to the manifest and network configuration
-func KvmPodToSystemd(p *stage1commontypes.Pod, n *networking.Networking) error {
+const (
+	sharedVolPerm = os.FileMode(0755)


Could it reuse the constant defined in the common pkg?

stage1/init/common/pod.go: sharedVolPerm = os.FileMode(0755)

It should. Give me a second.

Hmmm... This would require to export (uppercase) this constant in common/pod.go
I'm doing this now.

Without this change some data could be lost if they are saved by application just before pod stopping. For example if I started docker://busybox in interactive mode, added some volume, echoed some value into new file in this volume and then quickly closed pod - file was created, but without expected content inside. Changing this option helped. Change was done basing on information from https://www.kernel.org/doc/Documentation/filesystems/9p.txt

Co-authored-by: Michal Stachowski <michal.stachowski@outlook.com>

alban · 2016-04-08T13:37:47Z

stage1/init/kvm.go

+		}
+		if !strings.HasPrefix(absDestination, absAppRootfs) {
+			return fmt.Errorf("path escapes app's root: %v", absDestination)
+		}


Shouldn't this reuse pod.go:evaluateAppMountPath?

We were discussing about this earlier in #2328 (comment)

alban · 2016-04-11T10:00:53Z

LGTM

jellonek mentioned this pull request Mar 25, 2016

tests: missing cases in TestVolumes #2329

Open

jonboulle added needs/review technology/kvm component/stage1 labels Mar 25, 2016

jonboulle added this to the v1.3.0 milestone Mar 25, 2016

jellonek force-pushed the jell/volumes branch from 687bd67 to a7872ab Compare March 29, 2016 12:16

alban reviewed Mar 29, 2016
View reviewed changes

jellonek force-pushed the jell/volumes branch from a7872ab to 309bb71 Compare March 29, 2016 13:50

iaguis reviewed Mar 29, 2016
View reviewed changes

jonboulle added needs/some-changes and removed needs/review labels Mar 29, 2016

jellonek force-pushed the jell/volumes branch from 309bb71 to 9510971 Compare March 30, 2016 11:18

jonboulle added needs/review and removed needs/some-changes labels Mar 30, 2016

jellonek force-pushed the jell/volumes branch from 9510971 to c89b8d4 Compare March 31, 2016 15:20

alban modified the milestones: v1.4.0, v1.3.0 Mar 31, 2016

squall0gd mentioned this pull request Mar 31, 2016

kvm: Enable functional tests for kvm #2007

Merged

alban reviewed Apr 5, 2016
View reviewed changes

alban added needs/some-changes and removed needs/review labels Apr 5, 2016

alban mentioned this pull request Apr 5, 2016

stage1/kvm: Refactor generateMounts #2380

Open

jellonek force-pushed the jell/volumes branch 2 times, most recently from adf9225 to 3eed76c Compare April 7, 2016 15:30

alban reviewed Apr 8, 2016
View reviewed changes

jellonek force-pushed the jell/volumes branch 2 times, most recently from d86952a to 7f3631b Compare April 8, 2016 10:59

jellonek and others added 5 commits April 8, 2016 13:22

kvm: new volumes support

c5cb65a

kvm: fix for missing destination

67f1be7

kvm: fix for mounting symlinks

0ed8122

Co-authored-by: Michal Stachowski <michal.stachowski@outlook.com>

kvm: refactor volume mounting

df53308

jellonek force-pushed the jell/volumes branch from 7f3631b to df53308 Compare April 8, 2016 11:24

alban reviewed Apr 8, 2016
View reviewed changes

alban added reviewed/lgtm and removed needs/some-changes labels Apr 11, 2016

alban merged commit a2544f1 into rkt:master Apr 11, 2016

jellonek deleted the jell/volumes branch April 12, 2016 10:51

alban mentioned this pull request Apr 15, 2016

Release v1.4.0 #2431

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kvm: new volumes support #2328

kvm: new volumes support #2328

jellonek commented Mar 25, 2016

alban commented Mar 29, 2016

jellonek commented Mar 29, 2016

alban Mar 29, 2016

jellonek Mar 29, 2016

iaguis Mar 29, 2016

jellonek Mar 30, 2016

jonboulle Mar 30, 2016

jellonek Mar 30, 2016

jellonek Mar 30, 2016

krnowak commented Mar 31, 2016

jellonek commented Mar 31, 2016

jellonek commented Mar 31, 2016

alban commented Mar 31, 2016

jellonek commented Apr 1, 2016

jellonek commented Apr 5, 2016

alban Apr 5, 2016

jellonek Apr 5, 2016

alban Apr 5, 2016

jellonek Apr 5, 2016

alban commented Apr 5, 2016

jellonek commented Apr 5, 2016

jellonek commented Apr 7, 2016

alban Apr 8, 2016

jellonek Apr 8, 2016

jellonek Apr 8, 2016

alban Apr 8, 2016

jellonek Apr 8, 2016

alban commented Apr 11, 2016

kvm: new volumes support #2328

kvm: new volumes support #2328

Conversation

jellonek commented Mar 25, 2016

alban commented Mar 29, 2016

jellonek commented Mar 29, 2016

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

krnowak commented Mar 31, 2016

jellonek commented Mar 31, 2016

jellonek commented Mar 31, 2016

alban commented Mar 31, 2016

jellonek commented Apr 1, 2016

jellonek commented Apr 5, 2016

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

alban commented Apr 5, 2016

jellonek commented Apr 5, 2016

jellonek commented Apr 7, 2016

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

alban commented Apr 11, 2016