From 6ce6a262a842e8b771eef939f1b72e6194e89314 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Fri, 16 Aug 2019 10:47:12 +0100 Subject: [PATCH 1/4] kata_agent: use virtio-fs 0.3+ mount options virtio-fs changed the mount command-line. Previously "mount none -o tag=kataShared ..." was used. Now "mount kataShared ..." is used instead. Since the "kataShared" tag is used for both 9P and virtio-fs, rename the variable so that it is not 9P-specific. Signed-off-by: Stefan Hajnoczi Fixes: #1993 --- virtcontainers/kata_agent.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/virtcontainers/kata_agent.go b/virtcontainers/kata_agent.go index 064011bbf3..6981f44c79 100644 --- a/virtcontainers/kata_agent.go +++ b/virtcontainers/kata_agent.go @@ -59,7 +59,7 @@ var ( errorMissingOCISpec = errors.New("Missing OCI specification") defaultKataHostSharedDir = "/run/kata-containers/shared/sandboxes/" defaultKataGuestSharedDir = "/run/kata-containers/shared/containers/" - mountGuest9pTag = "kataShared" + mountGuestTag = "kataShared" defaultKataGuestSandboxDir = "/run/kata-containers/sandbox/" type9pFs = "9p" typeVirtioFS = "virtio_fs" @@ -72,7 +72,7 @@ var ( kataNvdimmDevType = "nvdimm" kataVirtioFSDevType = "virtio-fs" sharedDir9pOptions = []string{"trans=virtio,version=9p2000.L,cache=mmap", "nodev"} - sharedDirVirtioFSOptions = []string{"default_permissions,allow_other,rootmode=040000,user_id=0,group_id=0,dax,tag=" + mountGuest9pTag, "nodev"} + sharedDirVirtioFSOptions = []string{"default_permissions,allow_other,rootmode=040000,user_id=0,group_id=0", "nodev"} sharedDirVirtioFSDaxOptions = "dax" shmDir = "shm" kataEphemeralDevType = "ephemeral" @@ -401,7 +401,7 @@ func (k *kataAgent) configure(h hypervisor, id, sharePath string, builtin bool, // Create shared directory and add the shared volume if filesystem sharing is supported. // This volume contains all bind mounted container bundles. sharedVolume := types.Volume{ - MountTag: mountGuest9pTag, + MountTag: mountGuestTag, HostPath: sharePath, } @@ -872,7 +872,7 @@ func setupStorages(sandbox *Sandbox) []*grpc.Storage { } sharedVolume := &grpc.Storage{ Driver: kataVirtioFSDevType, - Source: "none", + Source: mountGuestTag, MountPoint: kataGuestSharedDir(), Fstype: typeVirtioFS, Options: sharedDirVirtioFSOptions, @@ -884,7 +884,7 @@ func setupStorages(sandbox *Sandbox) []*grpc.Storage { sharedVolume := &grpc.Storage{ Driver: kata9pDevType, - Source: mountGuest9pTag, + Source: mountGuestTag, MountPoint: kataGuestSharedDir(), Fstype: type9pFs, Options: sharedDir9pOptions, From d5a3d0a61c703b9d82802e3a1e586b21c2286381 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Wed, 21 Aug 2019 10:49:43 +0100 Subject: [PATCH 2/4] virtiofs: use virtiofsd --fd=FDNUM The new --fd=FDNUM file descriptor passing option eliminates the need to wait for virtiofsd to create the vhost-user UNIX domain socket. This is a nice simplification because we can remove the timeouts and stderr parsing. There is no longer a race between launching virtiofsd and launching QEMU, so we don't need to wait anymore. Signed-off-by: Stefan Hajnoczi --- virtcontainers/qemu.go | 98 +++++++++++-------------------------- virtcontainers/qemu_test.go | 29 ++--------- 2 files changed, 33 insertions(+), 94 deletions(-) diff --git a/virtcontainers/qemu.go b/virtcontainers/qemu.go index c297c14c43..47dd22e6cf 100644 --- a/virtcontainers/qemu.go +++ b/virtcontainers/qemu.go @@ -13,6 +13,7 @@ import ( "fmt" "io/ioutil" "math" + "net" "os" "os/exec" "path/filepath" @@ -602,13 +603,13 @@ func (q *qemu) vhostFSSocketPath(id string) (string, error) { return utils.BuildSocketPath(store.RunVMStoragePath(), id, vhostFSSocket) } -func (q *qemu) virtiofsdArgs(sockPath string) []string { +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) args := []string{ - "-o", "vhost_user_socket=" + sockPath, + fmt.Sprintf("--fd=%v", fd), "-o", "source=" + sourcePath, "-o", "cache=" + q.config.VirtioFSCache} if q.config.Debug { @@ -623,82 +624,41 @@ func (q *qemu) virtiofsdArgs(sockPath string) []string { return args } -func (q *qemu) setupVirtiofsd(timeout int) (remain int, err error) { +func (q *qemu) setupVirtiofsd() (err error) { + var listener *net.UnixListener + var fd *os.File + sockPath, err := q.vhostFSSocketPath(q.id) if err != nil { - return 0, err + return err } - cmd := exec.Command(q.config.VirtioFSDaemon, q.virtiofsdArgs(sockPath)...) - stderr, err := cmd.StderrPipe() + listener, err = net.ListenUnix("unix", &net.UnixAddr{ + Name: sockPath, + Net: "unix", + }) if err != nil { - return 0, err + return err } + listener.SetUnlinkOnClose(false) - if err = cmd.Start(); err != nil { - return 0, err - } - defer func() { - if err != nil { - cmd.Process.Kill() - } else { - q.state.VirtiofsdPid = cmd.Process.Pid - } - }() - - // Wait for socket to become available - sockReady := make(chan error, 1) - timeStart := time.Now() - go func() { - scanner := bufio.NewScanner(stderr) - var sent bool - for scanner.Scan() { - if q.config.Debug { - q.Logger().WithField("source", "virtiofsd").Debug(scanner.Text()) - } - if !sent && strings.Contains(scanner.Text(), "Waiting for vhost-user socket connection...") { - sockReady <- nil - sent = true - } - } - if !sent { - if err := scanner.Err(); err != nil { - sockReady <- err - } else { - sockReady <- fmt.Errorf("virtiofsd did not announce socket connection") - } - } - q.Logger().Info("virtiofsd quits") - // Wait to release resources of virtiofsd process - cmd.Process.Wait() - q.stopSandbox() - }() - - return q.waitVirtiofsd(timeStart, timeout, sockReady, - fmt.Sprintf("virtiofsd (pid=%d) socket %s", cmd.Process.Pid, sockPath)) -} - -func (q *qemu) waitVirtiofsd(start time.Time, timeout int, ready chan error, errMsg string) (int, error) { - var err error - - timeoutDuration := time.Duration(timeout) * time.Second - select { - case err = <-ready: - case <-time.After(timeoutDuration): - err = fmt.Errorf("timed out waiting for %s", errMsg) - } + fd, err = listener.File() + listener.Close() // no longer needed since fd is a dup + listener = nil if err != nil { - return 0, err + return err } - // Now reduce timeout by the elapsed time - elapsed := time.Since(start) - if elapsed < timeoutDuration { - timeout = timeout - int(elapsed.Seconds()) - } else { - timeout = 0 + const sockFd = 3 // Cmd.ExtraFiles[] fds are numbered starting from 3 + cmd := exec.Command(q.config.VirtioFSDaemon, q.virtiofsdArgs(sockFd)...) + cmd.ExtraFiles = append(cmd.ExtraFiles, fd) + + err = cmd.Start() + if err == nil { + q.state.VirtiofsdPid = cmd.Process.Pid } - return timeout, nil + fd.Close() + return err } // startSandbox will start the Sandbox's VM. @@ -746,7 +706,7 @@ func (q *qemu) startSandbox(timeout int) error { }() if q.config.SharedFS == config.VirtioFS { - timeout, err = q.setupVirtiofsd(timeout) + err = q.setupVirtiofsd() if err != nil { return err } @@ -769,7 +729,7 @@ func (q *qemu) startSandbox(timeout int) error { return fmt.Errorf("failed to launch qemu: %s, error messages from qemu log: %s", err, strErr) } - err = q.waitSandbox(timeout) // the virtiofsd deferred checks err's value + err = q.waitSandbox(timeout) if err != nil { return err } diff --git a/virtcontainers/qemu_test.go b/virtcontainers/qemu_test.go index 759cabe209..72dcace956 100644 --- a/virtcontainers/qemu_test.go +++ b/virtcontainers/qemu_test.go @@ -13,7 +13,6 @@ import ( "path/filepath" "strings" "testing" - "time" govmmQemu "github.com/intel/govmm/qemu" "github.com/kata-containers/runtime/virtcontainers/device/config" @@ -503,36 +502,16 @@ func TestQemuVirtiofsdArgs(t *testing.T) { kataHostSharedDir = savedKataHostSharedDir }() - result := "-o vhost_user_socket=bar1 -o source=test-share-dir/foo -o cache=none -d" - args := q.virtiofsdArgs("bar1") + result := "--fd=123 -o source=test-share-dir/foo -o cache=none -d" + args := q.virtiofsdArgs(123) assert.Equal(strings.Join(args, " "), result) q.config.Debug = false - result = "-o vhost_user_socket=bar2 -o source=test-share-dir/foo -o cache=none -f" - args = q.virtiofsdArgs("bar2") + result = "--fd=123 -o source=test-share-dir/foo -o cache=none -f" + args = q.virtiofsdArgs(123) assert.Equal(strings.Join(args, " "), result) } -func TestQemuWaitVirtiofsd(t *testing.T) { - assert := assert.New(t) - - q := &qemu{} - - ready := make(chan error, 1) - timeout := 5 - - ready <- nil - remain, err := q.waitVirtiofsd(time.Now(), timeout, ready, "") - assert.Nil(err) - assert.True(remain <= timeout) - assert.True(remain >= 0) - - timeout = 0 - remain, err = q.waitVirtiofsd(time.Now(), timeout, ready, "") - assert.NotNil(err) - assert.True(remain == 0) -} - func TestQemuGetpids(t *testing.T) { assert := assert.New(t) From 23a5dc7ff809d7ce3538ded8cca040055174c961 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Wed, 21 Aug 2019 10:52:19 +0100 Subject: [PATCH 3/4] virtiofsd: use virtiofsd --syslog Log to syslog instead of stderr. This way all Kata and virtiofsd logs are captured in syslog (or the systemd journal). This makes debugging much easier. Signed-off-by: Stefan Hajnoczi --- virtcontainers/qemu.go | 3 ++- virtcontainers/qemu_test.go | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/virtcontainers/qemu.go b/virtcontainers/qemu.go index 47dd22e6cf..7500c0ae6e 100644 --- a/virtcontainers/qemu.go +++ b/virtcontainers/qemu.go @@ -611,7 +611,8 @@ func (q *qemu) virtiofsdArgs(fd uintptr) []string { args := []string{ fmt.Sprintf("--fd=%v", fd), "-o", "source=" + sourcePath, - "-o", "cache=" + q.config.VirtioFSCache} + "-o", "cache=" + q.config.VirtioFSCache, + "--syslog"} if q.config.Debug { args = append(args, "-d") } else { diff --git a/virtcontainers/qemu_test.go b/virtcontainers/qemu_test.go index 72dcace956..7a4a7b6f53 100644 --- a/virtcontainers/qemu_test.go +++ b/virtcontainers/qemu_test.go @@ -502,12 +502,12 @@ func TestQemuVirtiofsdArgs(t *testing.T) { kataHostSharedDir = savedKataHostSharedDir }() - result := "--fd=123 -o source=test-share-dir/foo -o cache=none -d" + result := "--fd=123 -o source=test-share-dir/foo -o cache=none --syslog -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 -f" + result = "--fd=123 -o source=test-share-dir/foo -o cache=none --syslog -f" args = q.virtiofsdArgs(123) assert.Equal(strings.Join(args, " "), result) } From ad1563196e29a24ccccaf6ea7a4e02ac7994ecea Mon Sep 17 00:00:00 2001 From: Salvador Fuentes Date: Mon, 7 Oct 2019 16:00:13 -0500 Subject: [PATCH 4/4] virtiofsd: Do not use posix lock. We have some issues trying to run `apt upgrade` on a container that uses virtiofsd with `-o posix_lock`. Add virtiofsd `-o no_posix_lock` argument to not use the posix lock. Signed-off-by: Salvador Fuentes --- virtcontainers/qemu.go | 2 +- virtcontainers/qemu_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/virtcontainers/qemu.go b/virtcontainers/qemu.go index 7500c0ae6e..948feba80b 100644 --- a/virtcontainers/qemu.go +++ b/virtcontainers/qemu.go @@ -612,7 +612,7 @@ func (q *qemu) virtiofsdArgs(fd uintptr) []string { fmt.Sprintf("--fd=%v", fd), "-o", "source=" + sourcePath, "-o", "cache=" + q.config.VirtioFSCache, - "--syslog"} + "--syslog", "-o", "no_posix_lock"} if q.config.Debug { args = append(args, "-d") } else { diff --git a/virtcontainers/qemu_test.go b/virtcontainers/qemu_test.go index 7a4a7b6f53..1f0d988b71 100644 --- a/virtcontainers/qemu_test.go +++ b/virtcontainers/qemu_test.go @@ -502,12 +502,12 @@ func TestQemuVirtiofsdArgs(t *testing.T) { kataHostSharedDir = savedKataHostSharedDir }() - result := "--fd=123 -o source=test-share-dir/foo -o cache=none --syslog -d" + result := "--fd=123 -o source=test-share-dir/foo -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 -f" + result = "--fd=123 -o source=test-share-dir/foo -o cache=none --syslog -o no_posix_lock -f" args = q.virtiofsdArgs(123) assert.Equal(strings.Join(args, " "), result) }