diff --git a/virtcontainers/agent.go b/virtcontainers/agent.go index 2b68d35027..663e38b839 100644 --- a/virtcontainers/agent.go +++ b/virtcontainers/agent.go @@ -224,9 +224,6 @@ type agent interface { // configureFromGrpc will update agent settings based on provided arguments which from Grpc configureFromGrpc(h hypervisor, id string, builtin bool, config interface{}) error - // getSharePath will return the agent 9pfs share mount path - getSharePath(id string) string - // reseedRNG will reseed the guest random number generator reseedRNG(data []byte) error diff --git a/virtcontainers/api_test.go b/virtcontainers/api_test.go index 710b5cc420..5fb8ff4d6d 100644 --- a/virtcontainers/api_test.go +++ b/virtcontainers/api_test.go @@ -340,7 +340,7 @@ func TestStartSandboxKataAgentSuccessful(t *testing.T) { // TODO: defaultSharedDir is a hyper var = /run/hyper/shared/sandboxes // do we need to unmount sandboxes and containers? - err = bindUnmountAllRootfs(ctx, testDir, pImpl) + err = bindUnmountAllRootfs(ctx, filepath.Join(testDir, p.ID()), pImpl) assert.NoError(err) } @@ -474,7 +474,7 @@ func TestRunSandboxKataAgentSuccessful(t *testing.T) { _, err = os.Stat(sandboxDir) assert.NoError(err) - err = bindUnmountAllRootfs(ctx, testDir, pImpl) + err = bindUnmountAllRootfs(ctx, filepath.Join(testDir, p.ID()), pImpl) assert.NoError(err) } diff --git a/virtcontainers/clh.go b/virtcontainers/clh.go index a0d7c7b722..56278e64ec 100644 --- a/virtcontainers/clh.go +++ b/virtcontainers/clh.go @@ -207,7 +207,7 @@ func (clh *cloudHypervisor) createSandbox(ctx context.Context, id string, networ clh.Logger().WithField("function", "createSandbox").Info("Sandbox already exist, loading from state") clh.virtiofsd = &virtiofsd{ PID: clh.state.VirtiofsdPID, - sourcePath: filepath.Join(kataHostSharedDir(), clh.id), + sourcePath: filepath.Join(getSharePath(clh.id)), debug: clh.config.Debug, socketPath: virtiofsdSocketPath, } @@ -307,7 +307,7 @@ func (clh *cloudHypervisor) createSandbox(ctx context.Context, id string, networ clh.virtiofsd = &virtiofsd{ path: clh.config.VirtioFSDaemon, - sourcePath: filepath.Join(kataHostSharedDir(), clh.id), + sourcePath: filepath.Join(getSharePath(clh.id)), socketPath: virtiofsdSocketPath, extraArgs: clh.config.VirtioFSExtraArgs, debug: clh.config.Debug, diff --git a/virtcontainers/container.go b/virtcontainers/container.go index 4ba7ee2e3a..9e2d1e94b4 100644 --- a/virtcontainers/container.go +++ b/virtcontainers/container.go @@ -480,7 +480,7 @@ func (c *Container) shareFiles(m Mount, idx int, hostSharedDir, guestSharedDir s } } else { // These mounts are created in the shared dir - mountDest := filepath.Join(hostSharedDir, c.sandbox.id, filename) + mountDest := filepath.Join(hostSharedDir, filename) if err := bindMount(c.ctx, m.Source, mountDest, false, "private"); err != nil { return "", false, err } @@ -850,7 +850,7 @@ func (c *Container) rollbackFailingContainerCreation() { if err := c.unmountHostMounts(); err != nil { c.Logger().WithError(err).Error("rollback failed unmountHostMounts()") } - if err := bindUnmountContainerRootfs(c.ctx, kataHostSharedDir(), c.sandbox.id, c.id); err != nil { + if err := bindUnmountContainerRootfs(c.ctx, getMountPath(c.sandbox.id), c.id); err != nil { c.Logger().WithError(err).Error("rollback failed bindUnmountContainerRootfs()") } } @@ -875,6 +875,7 @@ func (c *Container) create() (err error) { // of rolling back all the actions previously performed. defer func() { if err != nil { + c.Logger().WithError(err).Error("container create failed") c.rollbackFailingContainerCreation() } }() @@ -1119,7 +1120,7 @@ func (c *Container) stop(force bool) error { return err } - if err := bindUnmountContainerRootfs(c.ctx, kataHostSharedDir(), c.sandbox.id, c.id); err != nil && !force { + if err := bindUnmountContainerRootfs(c.ctx, getMountPath(c.sandbox.id), c.id); err != nil && !force { return err } diff --git a/virtcontainers/kata_agent.go b/virtcontainers/kata_agent.go index 160093b106..7ff0310025 100644 --- a/virtcontainers/kata_agent.go +++ b/virtcontainers/kata_agent.go @@ -140,6 +140,26 @@ var kataHostSharedDir = func() string { return defaultKataHostSharedDir } +// Shared path handling: +// 1. create two directories for each sandbox: +// -. /run/kata-containers/shared/sandboxes/$sbx_id/mounts/, a directory to hold all host/guest shared mounts +// -. /run/kata-containers/shared/sandboxes/$sbx_id/shared/, a host/guest shared directory (9pfs/virtiofs source dir) +// +// 2. /run/kata-containers/shared/sandboxes/$sbx_id/mounts/ is bind mounted readonly to /run/kata-containers/shared/sandboxes/$sbx_id/shared/, so guest cannot modify it +// +// 3. host-guest shared files/directories are mounted one-level under /run/kata-containers/shared/sandboxes/$sbx_id/mounts/ and thus present to guest at one level under /run/kata-containers/shared/sandboxes/$sbx_id/shared/ +func getSharePath(id string) string { + return filepath.Join(kataHostSharedDir(), id, "shared") +} + +func getMountPath(id string) string { + return filepath.Join(kataHostSharedDir(), id, "mounts") +} + +func getSandboxPath(id string) string { + return filepath.Join(kataHostSharedDir(), id) +} + // The function is declared this way for mocking in unit tests var kataGuestSharedDir = func() string { if rootless.IsRootless() { @@ -158,6 +178,10 @@ var kataGuestSandboxDir = func() string { return defaultKataGuestSandboxDir } +var kataGuestSandboxStorageDir = func() string { + return filepath.Join(defaultKataGuestSandboxDir, "storage") +} + func ephemeralPath() string { if rootless.IsRootless() { return filepath.Join(kataGuestSandboxDir(), kataEphemeralDevType) @@ -223,10 +247,6 @@ func (k *kataAgent) Logger() *logrus.Entry { return virtLog.WithField("subsystem", "kata_agent") } -func (k *kataAgent) getSharePath(id string) string { - return filepath.Join(kataHostSharedDir(), id) -} - func (k *kataAgent) longLiveConn() bool { return k.keepConn } @@ -356,7 +376,7 @@ func (k *kataAgent) capabilities() types.Capabilities { return caps } -func (k *kataAgent) internalConfigure(h hypervisor, id, sharePath string, builtin bool, config interface{}) error { +func (k *kataAgent) internalConfigure(h hypervisor, id string, builtin bool, config interface{}) error { var err error if config != nil { switch c := config.(type) { @@ -378,7 +398,7 @@ func (k *kataAgent) internalConfigure(h hypervisor, id, sharePath string, builti } func (k *kataAgent) configure(h hypervisor, id, sharePath string, builtin bool, config interface{}) error { - err := k.internalConfigure(h, id, sharePath, builtin, config) + err := k.internalConfigure(h, id, builtin, config) if err != nil { return err } @@ -424,14 +444,36 @@ func (k *kataAgent) configure(h hypervisor, id, sharePath string, builtin bool, } func (k *kataAgent) configureFromGrpc(h hypervisor, id string, builtin bool, config interface{}) error { - return k.internalConfigure(h, id, "", builtin, config) + return k.internalConfigure(h, id, builtin, config) +} + +func (k *kataAgent) setupSharedPath(sandbox *Sandbox) error { + // create shared path structure + sharePath := getSharePath(sandbox.id) + mountPath := getMountPath(sandbox.id) + if err := os.MkdirAll(sharePath, DirMode); err != nil { + return err + } + if err := os.MkdirAll(mountPath, DirMode); err != nil { + return err + } + + // slave mount so that future mountpoints under mountPath are shown in sharePath as well + if err := bindMount(context.Background(), mountPath, sharePath, true, "slave"); err != nil { + return err + } + + return nil } func (k *kataAgent) createSandbox(sandbox *Sandbox) error { span, _ := k.trace("createSandbox") defer span.Finish() - return k.configure(sandbox.hypervisor, sandbox.id, k.getSharePath(sandbox.id), k.proxyBuiltIn, sandbox.config.AgentConfig) + if err := k.setupSharedPath(sandbox); err != nil { + return err + } + return k.configure(sandbox.hypervisor, sandbox.id, getSharePath(sandbox.id), k.proxyBuiltIn, sandbox.config.AgentConfig) } func cmdToKataProcess(cmd types.Cmd) (process *grpc.Process, err error) { @@ -1030,7 +1072,7 @@ func (k *kataAgent) replaceOCIMountsForStorages(spec *specs.Spec, volumeStorages // Create a temporary location to mount the Storage. Mounting to the correct location // will be handled by the OCI mount structure. filename := fmt.Sprintf("%s-%s", uuid.Generate().String(), filepath.Base(m.Destination)) - path := filepath.Join(kataGuestSharedDir(), filename) + path := filepath.Join(kataGuestSandboxStorageDir(), filename) k.Logger().Debugf("Replacing OCI mount source (%s) with %s", m.Source, path) ociMounts[index].Source = path @@ -1245,7 +1287,7 @@ func (k *kataAgent) rollbackFailingContainerCreation(c *Container) { k.Logger().WithError(err2).Error("rollback failed unmountHostMounts()") } - if err2 := bindUnmountContainerRootfs(k.ctx, kataHostSharedDir(), c.sandbox.id, c.id); err2 != nil { + if err2 := bindUnmountContainerRootfs(k.ctx, getMountPath(c.sandbox.id), c.id); err2 != nil { k.Logger().WithError(err2).Error("rollback failed bindUnmountContainerRootfs()") } } @@ -1302,6 +1344,13 @@ func (k *kataAgent) buildContainerRootfs(sandbox *Sandbox, c *Container, rootPat rootfs.Options = []string{"nouuid"} } + // Ensure container mount destination exists + // TODO: remove dependency on shared fs path. shared fs is just one kind of storage sources. + // we should not always use shared fs path for all kinds of storage. Stead, all storage + // should be bind mounted to a tmpfs path for containers to use. + if err := os.MkdirAll(filepath.Join(getMountPath(c.sandbox.id), c.id, c.rootfsSuffix), DirMode); err != nil { + return nil, err + } return rootfs, nil } @@ -1313,7 +1362,7 @@ func (k *kataAgent) buildContainerRootfs(sandbox *Sandbox, c *Container, rootPat // (kataGuestSharedDir) is already mounted in the // guest. We only need to mount the rootfs from // the host and it will show up in the guest. - if err := bindMountContainerRootfs(k.ctx, kataHostSharedDir(), sandbox.id, c.id, c.rootFs.Target, false); err != nil { + if err := bindMountContainerRootfs(k.ctx, getMountPath(sandbox.id), c.id, c.rootFs.Target, false); err != nil { return nil, err } @@ -1346,6 +1395,7 @@ func (k *kataAgent) createContainer(sandbox *Sandbox, c *Container) (p *Process, // takes care of rolling back actions previously performed. defer func() { if err != nil { + k.Logger().WithError(err).Error("createContainer failed") k.rollbackFailingContainerCreation(c) } }() @@ -1366,7 +1416,7 @@ func (k *kataAgent) createContainer(sandbox *Sandbox, c *Container) (p *Process, } // Handle container mounts - newMounts, ignoredMounts, err := c.mountSharedDirMounts(kataHostSharedDir(), kataGuestSharedDir()) + newMounts, ignoredMounts, err := c.mountSharedDirMounts(getMountPath(sandbox.id), kataGuestSharedDir()) if err != nil { return nil, err } @@ -2363,13 +2413,20 @@ func (k *kataAgent) markDead() { } func (k *kataAgent) cleanup(s *Sandbox) { - path := k.getSharePath(s.id) + // Unmount shared path + path := getSharePath(s.id) k.Logger().WithField("path", path).Infof("cleanup agent") + if err := syscall.Unmount(path, syscall.MNT_DETACH|UmountNoFollow); err != nil { + k.Logger().WithError(err).Errorf("failed to unmount vm share path %s", path) + } + + // Unmount mount path + path = getMountPath(s.id) if err := bindUnmountAllRootfs(k.ctx, path, s); err != nil { - k.Logger().WithError(err).Errorf("failed to unmount container share path %s", path) + k.Logger().WithError(err).Errorf("failed to unmount vm mount path %s", path) } - if err := os.RemoveAll(path); err != nil { - k.Logger().WithError(err).Errorf("failed to cleanup vm share path %s", path) + if err := os.RemoveAll(getSandboxPath(s.id)); err != nil { + k.Logger().WithError(err).Errorf("failed to cleanup vm path %s", getSandboxPath(s.id)) } } diff --git a/virtcontainers/kata_agent_test.go b/virtcontainers/kata_agent_test.go index 52c1da7629..55444d46b6 100644 --- a/virtcontainers/kata_agent_test.go +++ b/virtcontainers/kata_agent_test.go @@ -785,19 +785,6 @@ func TestHandlePidNamespace(t *testing.T) { assert.False(testIsPidNamespacePresent(g)) } -func TestAgentPathAPI(t *testing.T) { - assert := assert.New(t) - - k1 := &kataAgent{} - k2 := &kataAgent{} - id := "foobar" - - // getSharePath - path1 := k1.getSharePath(id) - path2 := k2.getSharePath(id) - assert.Equal(path1, path2) -} - func TestAgentConfigure(t *testing.T) { assert := assert.New(t) diff --git a/virtcontainers/mount.go b/virtcontainers/mount.go index dd6177bef7..f7822a2968 100644 --- a/virtcontainers/mount.go +++ b/virtcontainers/mount.go @@ -275,11 +275,11 @@ func remount(ctx context.Context, mountflags uintptr, src string) error { // bindMountContainerRootfs bind mounts a container rootfs into a 9pfs shared // directory between the guest and the host. -func bindMountContainerRootfs(ctx context.Context, sharedDir, sandboxID, cID, cRootFs string, readonly bool) error { +func bindMountContainerRootfs(ctx context.Context, shareDir, cid, cRootFs string, readonly bool) error { span, _ := trace(ctx, "bindMountContainerRootfs") defer span.Finish() - rootfsDest := filepath.Join(sharedDir, sandboxID, cID, rootfsDir) + rootfsDest := filepath.Join(shareDir, cid, rootfsDir) return bindMount(ctx, cRootFs, rootfsDest, readonly, "private") } @@ -315,12 +315,12 @@ func isSymlink(path string) bool { return stat.Mode()&os.ModeSymlink != 0 } -func bindUnmountContainerRootfs(ctx context.Context, sharedDir, sandboxID, cID string) error { +func bindUnmountContainerRootfs(ctx context.Context, sharedDir, cID string) error { span, _ := trace(ctx, "bindUnmountContainerRootfs") defer span.Finish() - rootfsDest := filepath.Join(sharedDir, sandboxID, cID, rootfsDir) - if isSymlink(filepath.Join(sharedDir, sandboxID, cID)) || isSymlink(rootfsDest) { + rootfsDest := filepath.Join(sharedDir, cID, rootfsDir) + if isSymlink(filepath.Join(sharedDir, cID)) || isSymlink(rootfsDest) { logrus.Warnf("container dir %s is a symlink, malicious guest?", cID) return nil } @@ -343,7 +343,7 @@ func bindUnmountAllRootfs(ctx context.Context, sharedDir string, sandbox *Sandbo var errors *merr.Error for _, c := range sandbox.containers { - if isSymlink(filepath.Join(sharedDir, sandbox.id, c.id)) { + if isSymlink(filepath.Join(sharedDir, c.id)) { logrus.Warnf("container dir %s is a symlink, malicious guest?", c.id) continue } @@ -351,7 +351,7 @@ func bindUnmountAllRootfs(ctx context.Context, sharedDir string, sandbox *Sandbo if c.state.Fstype == "" { // even if error found, don't break out of loop until all mounts attempted // to be unmounted, and collect all errors - errors = merr.Append(errors, bindUnmountContainerRootfs(c.ctx, sharedDir, sandbox.id, c.id)) + errors = merr.Append(errors, bindUnmountContainerRootfs(c.ctx, sharedDir, c.id)) } } return errors.ErrorOrNil() diff --git a/virtcontainers/mount_test.go b/virtcontainers/mount_test.go index 5a39133be7..20b0157a40 100644 --- a/virtcontainers/mount_test.go +++ b/virtcontainers/mount_test.go @@ -386,7 +386,7 @@ func TestBindUnmountContainerRootfsENOENTNotError(t *testing.T) { assert.NoError(os.Remove(testPath)) } - err := bindUnmountContainerRootfs(context.Background(), testMnt, sID, cID) + err := bindUnmountContainerRootfs(context.Background(), filepath.Join(testMnt, sID), cID) assert.NoError(err) } @@ -407,7 +407,7 @@ func TestBindUnmountContainerRootfsRemoveRootfsDest(t *testing.T) { assert.NoError(err) defer os.RemoveAll(filepath.Join(testDir, sID)) - bindUnmountContainerRootfs(context.Background(), testDir, sID, cID) + bindUnmountContainerRootfs(context.Background(), filepath.Join(testDir, sID), cID) if _, err := os.Stat(testPath); err == nil { t.Fatal("empty rootfs dest should be removed") diff --git a/virtcontainers/noop_agent.go b/virtcontainers/noop_agent.go index 02c4cf0a17..73f6974823 100644 --- a/virtcontainers/noop_agent.go +++ b/virtcontainers/noop_agent.go @@ -185,11 +185,6 @@ func (n *noopAgent) configureFromGrpc(h hypervisor, id string, builtin bool, con return nil } -// getSharePath is the Noop agent share path getter. It does nothing. -func (n *noopAgent) getSharePath(id string) string { - return "" -} - // reseedRNG is the Noop agent RND reseeder. It does nothing. func (n *noopAgent) reseedRNG(data []byte) error { return nil diff --git a/virtcontainers/noop_agent_test.go b/virtcontainers/noop_agent_test.go index 8e60f44deb..f6172263d9 100644 --- a/virtcontainers/noop_agent_test.go +++ b/virtcontainers/noop_agent_test.go @@ -156,13 +156,6 @@ func TestNoopAgentConfigure(t *testing.T) { assert.NoError(err) } -func TestNoopAgentGetSharePath(t *testing.T) { - n := &noopAgent{} - path := n.getSharePath("") - assert := assert.New(t) - assert.Empty(path) -} - func TestNoopAgentStartProxy(t *testing.T) { assert := assert.New(t) n := &noopAgent{} diff --git a/virtcontainers/qemu.go b/virtcontainers/qemu.go index 51b68b0544..35478e3b0b 100644 --- a/virtcontainers/qemu.go +++ b/virtcontainers/qemu.go @@ -618,7 +618,7 @@ func (q *qemu) virtiofsdArgs(fd uintptr) []string { // The daemon will terminate when the vhost-user socket // connection with QEMU closes. Therefore we do not keep track // of this child process after returning from this function. - sourcePath := filepath.Join(kataHostSharedDir(), q.id) + sourcePath := filepath.Join(getSharePath(q.id)) args := []string{ fmt.Sprintf("--fd=%v", fd), "-o", "source=" + sourcePath, diff --git a/virtcontainers/qemu_test.go b/virtcontainers/qemu_test.go index f490d5d7de..6d8c01caa7 100644 --- a/virtcontainers/qemu_test.go +++ b/virtcontainers/qemu_test.go @@ -555,12 +555,12 @@ func TestQemuVirtiofsdArgs(t *testing.T) { kataHostSharedDir = savedKataHostSharedDir }() - result := "--fd=123 -o source=test-share-dir/foo -o cache=none --syslog -o no_posix_lock -d" + result := "--fd=123 -o source=test-share-dir/foo/shared -o cache=none --syslog -o no_posix_lock -d" args := q.virtiofsdArgs(123) assert.Equal(strings.Join(args, " "), result) q.config.Debug = false - result = "--fd=123 -o source=test-share-dir/foo -o cache=none --syslog -o no_posix_lock -f" + result = "--fd=123 -o source=test-share-dir/foo/shared -o cache=none --syslog -o no_posix_lock -f" args = q.virtiofsdArgs(123) assert.Equal(strings.Join(args, " "), result) } diff --git a/virtcontainers/virtcontainers_test.go b/virtcontainers/virtcontainers_test.go index 383310fbc7..3d10476210 100644 --- a/virtcontainers/virtcontainers_test.go +++ b/virtcontainers/virtcontainers_test.go @@ -13,6 +13,7 @@ import ( "os" "os/exec" "path/filepath" + "syscall" "testing" "github.com/kata-containers/runtime/virtcontainers/persist" @@ -59,6 +60,7 @@ func cleanUp() { globalSandboxList.removeSandbox(testSandboxID) os.RemoveAll(fs.MockRunStoragePath()) os.RemoveAll(fs.MockRunVMStoragePath()) + syscall.Unmount(getSharePath(testSandboxID), syscall.MNT_DETACH|UmountNoFollow) os.RemoveAll(testDir) os.MkdirAll(testDir, DirMode) diff --git a/virtcontainers/vm.go b/virtcontainers/vm.go index cfed806914..fcda1e977e 100644 --- a/virtcontainers/vm.go +++ b/virtcontainers/vm.go @@ -412,7 +412,7 @@ func (v *VM) assignSandbox(s *Sandbox) error { vmSharePath := buildVMSharePath(v.id, v.store.RunVMStoragePath()) vmSockDir := filepath.Join(v.store.RunVMStoragePath(), v.id) - sbSharePath := s.agent.getSharePath(s.id) + sbSharePath := getMountPath(s.id) sbSockDir := filepath.Join(v.store.RunVMStoragePath(), s.id) v.logger().WithFields(logrus.Fields{