From 620c0902fd3f78cb9eec493b244cb22aedb95bb2 Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Tue, 2 Aug 2022 17:32:28 -0400 Subject: [PATCH 1/3] qemu: reduce monitor socket path file name The QEMU driver can take an optional `graceful_shutdown` configuration which will create a Unix socket to send ACPI shutdown signal to the VM. Unix sockets have a hard length limit and the driver implementation assumed that QEMU versions 2.10.1 were able to handle longer paths. This is not correct, the linked QEMU fix only changed the behaviour from silently truncating longer socket paths to throwing an error. By validating the socket path before starting the QEMU machine we can provide users a more actionable and meaningful error message, and by using a shorter socket file name we leave a bit more room for user-defined values in the path, such as the task name. The maximum length allowed is also platform-dependant, so validation needs to be different for each OS. --- drivers/qemu/driver.go | 87 +++++++++---------- drivers/qemu/driver_bsd.go | 12 +++ drivers/qemu/driver_fallback.go | 9 ++ drivers/qemu/driver_linux.go | 9 ++ drivers/qemu/driver_test.go | 117 +------------------------- website/content/docs/drivers/qemu.mdx | 26 +++--- 6 files changed, 79 insertions(+), 181 deletions(-) create mode 100644 drivers/qemu/driver_bsd.go create mode 100644 drivers/qemu/driver_fallback.go create mode 100644 drivers/qemu/driver_linux.go diff --git a/drivers/qemu/driver.go b/drivers/qemu/driver.go index b6fe6041b98..19336e58a49 100644 --- a/drivers/qemu/driver.go +++ b/drivers/qemu/driver.go @@ -13,7 +13,6 @@ import ( "strings" "time" - "github.com/coreos/go-semver/semver" hclog "github.com/hashicorp/go-hclog" "github.com/hashicorp/nomad/client/taskenv" "github.com/hashicorp/nomad/drivers/shared/eventer" @@ -39,14 +38,13 @@ const ( // Represents an ACPI shutdown request to the VM (emulates pressing a physical power button) // Reference: https://en.wikibooks.org/wiki/QEMU/Monitor + // Use a short file name since socket paths have a maximum length. qemuGracefulShutdownMsg = "system_powerdown\n" - qemuMonitorSocketName = "qemu-monitor.sock" + qemuMonitorSocketName = "qm.sock" // Socket file enabling communication with the Qemu Guest Agent (if enabled and running) - qemuGuestAgentSocketName = "qemu-guest-agent.sock" - - // Maximum socket path length prior to qemu 2.10.1 - qemuLegacyMaxMonitorPathLen = 108 + // Use a short file name since socket paths have a maximum length. + qemuGuestAgentSocketName = "qa.sock" // taskHandleVersion is the version of task handle which this driver sets // and understands how to decode driver state @@ -70,14 +68,6 @@ var ( versionRegex = regexp.MustCompile(`version (\d[\.\d+]+)`) - // Prior to qemu 2.10.1, monitor socket paths are truncated to 108 bytes. - // We should consider this if driver.qemu.version is < 2.10.1 and the - // generated monitor path is too long. - // - // Relevant fix is here: - // https://github.com/qemu/qemu/commit/ad9579aaa16d5b385922d49edac2c96c79bcfb6 - qemuVersionLongSocketPathFix = semver.New("2.10.1") - // pluginInfo is the response returned for the PluginInfo RPC pluginInfo = &base.PluginInfoResponse{ Type: base.PluginTypeDriver, @@ -307,11 +297,19 @@ func (d *Driver) RecoverTask(handle *drivers.TaskHandle) error { // Try to restore monitor socket path. taskDir := filepath.Join(handle.Config.AllocDir, handle.Config.Name) - monitorPath := filepath.Join(taskDir, qemuMonitorSocketName) - if _, err := os.Stat(monitorPath); err == nil { - d.logger.Debug("found existing monitor socket", "monitor", monitorPath) - } else { - monitorPath = "" + possiblePaths := []string{ + filepath.Join(taskDir, qemuMonitorSocketName), + // Support restoring tasks that used the old socket name. + filepath.Join(taskDir, "qemu-monitor.sock"), + } + + var monitorPath string + for _, path := range possiblePaths { + if _, err := os.Stat(path); err == nil { + monitorPath = path + d.logger.Debug("found existing monitor socket", "monitor", monitorPath) + break + } } h := &taskHandle{ @@ -468,6 +466,8 @@ func (d *Driver) StartTask(cfg *drivers.TaskConfig) (*drivers.TaskHandle, *drive } } + taskDir := filepath.Join(cfg.AllocDir, cfg.Name) + var monitorPath string if driverConfig.GracefulShutdown { if runtime.GOOS == "windows" { @@ -475,14 +475,8 @@ func (d *Driver) StartTask(cfg *drivers.TaskConfig) (*drivers.TaskHandle, *drive } // This socket will be used to manage the virtual machine (for example, // to perform graceful shutdowns) - taskDir := filepath.Join(cfg.AllocDir, cfg.Name) - fingerPrint := d.buildFingerprint() - if fingerPrint.Attributes == nil { - return nil, nil, fmt.Errorf("unable to get qemu driver version from fingerprinted attributes") - } - monitorPath, err = d.getMonitorPath(taskDir, fingerPrint) - if err != nil { - d.logger.Debug("could not get qemu monitor path", "error", err) + monitorPath = filepath.Join(taskDir, qemuMonitorSocketName) + if err := validateSocketPath(monitorPath); err != nil { return nil, nil, err } d.logger.Debug("got monitor path", "monitorPath", monitorPath) @@ -494,8 +488,12 @@ func (d *Driver) StartTask(cfg *drivers.TaskConfig) (*drivers.TaskHandle, *drive return nil, nil, errors.New("QEMU Guest Agent socket is unsupported on the Windows platform") } // This socket will be used to communicate with the Guest Agent (if it's running) - taskDir := filepath.Join(cfg.AllocDir, cfg.Name) - args = append(args, "-chardev", fmt.Sprintf("socket,path=%s/%s,server,nowait,id=qga0", taskDir, qemuGuestAgentSocketName)) + agentSocketPath := filepath.Join(taskDir, qemuGuestAgentSocketName) + if err := validateSocketPath(agentSocketPath); err != nil { + return nil, nil, err + } + + args = append(args, "-chardev", fmt.Sprintf("socket,path=%s,server,nowait,id=qga0", agentSocketPath)) args = append(args, "-device", "virtio-serial") args = append(args, "-device", "virtserialport,chardev=qga0,name=org.qemu.guest_agent.0") } @@ -647,6 +645,8 @@ func (d *Driver) StopTask(taskID string, timeout time.Duration, signal string) e if err := sendQemuShutdown(d.logger, handle.monitorPath, handle.pid); err != nil { d.logger.Debug("error sending graceful shutdown ", "pid", handle.pid, "error", err) } + } else { + d.logger.Debug("monitor socket is empty, forcing shutdown") } // TODO(preetha) we are calling shutdown on the executor here @@ -748,27 +748,16 @@ func (d *Driver) handleWait(ctx context.Context, handle *taskHandle, ch chan *dr } } -// getMonitorPath is used to determine whether a qemu monitor socket can be -// safely created and accessed in the task directory by the version of qemu -// present on the host. If it is safe to use, the socket's full path is -// returned along with a nil error. Otherwise, an empty string is returned -// along with a descriptive error. -func (d *Driver) getMonitorPath(dir string, fingerPrint *drivers.Fingerprint) (string, error) { - var longPathSupport bool - currentQemuVer := fingerPrint.Attributes[driverVersionAttr] - currentQemuSemver := semver.New(currentQemuVer.GoString()) - if currentQemuSemver.LessThan(*qemuVersionLongSocketPathFix) { - longPathSupport = false - d.logger.Debug("long socket paths are not available in this version of QEMU", "version", currentQemuVer) - } else { - longPathSupport = true - d.logger.Debug("long socket paths available in this version of QEMU", "version", currentQemuVer) - } - fullSocketPath := fmt.Sprintf("%s/%s", dir, qemuMonitorSocketName) - if len(fullSocketPath) > qemuLegacyMaxMonitorPathLen && !longPathSupport { - return "", fmt.Errorf("monitor path is too long for this version of qemu") +// validateSocketPath provides best effort validation of socket paths since +// some rules may be platform-dependant. +func validateSocketPath(path string) error { + if maxSocketPathLen > 0 && len(path) > maxSocketPathLen { + return fmt.Errorf( + "socket path %s is longer than the maximum length allowed (%d), try to reduce the task name or Nomad's data_dir if possible.", + path, maxSocketPathLen) } - return fullSocketPath, nil + + return nil } // sendQemuShutdown attempts to issue an ACPI power-off command via the qemu diff --git a/drivers/qemu/driver_bsd.go b/drivers/qemu/driver_bsd.go new file mode 100644 index 00000000000..21c31459890 --- /dev/null +++ b/drivers/qemu/driver_bsd.go @@ -0,0 +1,12 @@ +//go:build darwin || freebsd || netbsd || openbsd +// +build darwin freebsd netbsd openbsd + +package qemu + +const ( + // https://man.openbsd.org/unix.4#ADDRESSING + // https://www.freebsd.org/cgi/man.cgi?query=unix + // https://github.com/apple/darwin-xnu/blob/main/bsd/man/man4/unix.4#L72 + // https://man.netbsd.org/unix.4#ADDRESSING + maxSocketPathLen = 104 +) diff --git a/drivers/qemu/driver_fallback.go b/drivers/qemu/driver_fallback.go new file mode 100644 index 00000000000..cbc89500a7e --- /dev/null +++ b/drivers/qemu/driver_fallback.go @@ -0,0 +1,9 @@ +//go:build !linux && !darwin && !freebsd && !netbsd && !openbsd +// +build !linux,!darwin,!freebsd,!netbsd,!openbsd + +package qemu + +const ( + // Don't enforce any path limit. + maxSocketPathLen = 0 +) diff --git a/drivers/qemu/driver_linux.go b/drivers/qemu/driver_linux.go new file mode 100644 index 00000000000..76059aad809 --- /dev/null +++ b/drivers/qemu/driver_linux.go @@ -0,0 +1,9 @@ +//go:build linux +// +build linux + +package qemu + +const ( + // https://man7.org/linux/man-pages/man7/unix.7.html + maxSocketPathLen = 108 +) diff --git a/drivers/qemu/driver_test.go b/drivers/qemu/driver_test.go index 7bb27d1a788..a5676ce77f9 100644 --- a/drivers/qemu/driver_test.go +++ b/drivers/qemu/driver_test.go @@ -5,7 +5,6 @@ import ( "io" "os" "path/filepath" - "strings" "testing" "time" @@ -17,7 +16,6 @@ import ( "github.com/hashicorp/nomad/nomad/structs" "github.com/hashicorp/nomad/plugins/drivers" dtestutil "github.com/hashicorp/nomad/plugins/drivers/testutils" - pstructs "github.com/hashicorp/nomad/plugins/shared/structs" "github.com/hashicorp/nomad/testutil" "github.com/stretchr/testify/require" ) @@ -88,119 +86,6 @@ func TestQemuDriver_Start_Wait_Stop(t *testing.T) { } -// Verifies monitor socket path for old qemu -func TestQemuDriver_GetMonitorPathOldQemu(t *testing.T) { - ci.Parallel(t) - ctestutil.QemuCompatible(t) - - require := require.New(t) - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - - d := NewQemuDriver(ctx, testlog.HCLogger(t)) - harness := dtestutil.NewDriverHarness(t, d) - - task := &drivers.TaskConfig{ - ID: uuid.Generate(), - Name: "linux", - Resources: &drivers.Resources{ - NomadResources: &structs.AllocatedTaskResources{ - Memory: structs.AllocatedMemoryResources{ - MemoryMB: 512, - }, - Cpu: structs.AllocatedCpuResources{ - CpuShares: 100, - }, - Networks: []*structs.NetworkResource{ - { - ReservedPorts: []structs.Port{{Label: "main", Value: 22000}, {Label: "web", Value: 80}}, - }, - }, - }, - }, - } - - cleanup := harness.MkAllocDir(task, true) - defer cleanup() - - fingerPrint := &drivers.Fingerprint{ - Attributes: map[string]*pstructs.Attribute{ - driverVersionAttr: pstructs.NewStringAttribute("2.0.0"), - }, - } - shortPath := strings.Repeat("x", 10) - qemuDriver := d.(*Driver) - _, err := qemuDriver.getMonitorPath(shortPath, fingerPrint) - require.Nil(err) - - longPath := strings.Repeat("x", qemuLegacyMaxMonitorPathLen+100) - _, err = qemuDriver.getMonitorPath(longPath, fingerPrint) - require.NotNil(err) - - // Max length includes the '/' separator and socket name - maxLengthCount := qemuLegacyMaxMonitorPathLen - len(qemuMonitorSocketName) - 1 - maxLengthLegacyPath := strings.Repeat("x", maxLengthCount) - _, err = qemuDriver.getMonitorPath(maxLengthLegacyPath, fingerPrint) - require.Nil(err) -} - -// Verifies monitor socket path for new qemu version -func TestQemuDriver_GetMonitorPathNewQemu(t *testing.T) { - ci.Parallel(t) - ctestutil.QemuCompatible(t) - - require := require.New(t) - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - - d := NewQemuDriver(ctx, testlog.HCLogger(t)) - harness := dtestutil.NewDriverHarness(t, d) - - task := &drivers.TaskConfig{ - ID: uuid.Generate(), - Name: "linux", - Resources: &drivers.Resources{ - NomadResources: &structs.AllocatedTaskResources{ - Memory: structs.AllocatedMemoryResources{ - MemoryMB: 512, - }, - Cpu: structs.AllocatedCpuResources{ - CpuShares: 100, - }, - Networks: []*structs.NetworkResource{ - { - ReservedPorts: []structs.Port{{Label: "main", Value: 22000}, {Label: "web", Value: 80}}, - }, - }, - }, - }, - } - - cleanup := harness.MkAllocDir(task, true) - defer cleanup() - - fingerPrint := &drivers.Fingerprint{ - Attributes: map[string]*pstructs.Attribute{ - driverVersionAttr: pstructs.NewStringAttribute("2.99.99"), - }, - } - shortPath := strings.Repeat("x", 10) - qemuDriver := d.(*Driver) - _, err := qemuDriver.getMonitorPath(shortPath, fingerPrint) - require.Nil(err) - - // Should not return an error in this qemu version - longPath := strings.Repeat("x", qemuLegacyMaxMonitorPathLen+100) - _, err = qemuDriver.getMonitorPath(longPath, fingerPrint) - require.Nil(err) - - // Max length includes the '/' separator and socket name - maxLengthCount := qemuLegacyMaxMonitorPathLen - len(qemuMonitorSocketName) - 1 - maxLengthLegacyPath := strings.Repeat("x", maxLengthCount) - _, err = qemuDriver.getMonitorPath(maxLengthLegacyPath, fingerPrint) - require.Nil(err) -} - // copyFile moves an existing file to the destination func copyFile(src, dst string, t *testing.T) { in, err := os.Open(src) @@ -464,7 +349,7 @@ func TestIsAllowedImagePath(t *testing.T) { func TestArgsAllowList(t *testing.T) { ci.Parallel(t) - + pluginConfigAllowList := []string{"-drive", "-net", "-snapshot"} validArgs := [][]string{ diff --git a/website/content/docs/drivers/qemu.mdx b/website/content/docs/drivers/qemu.mdx index 72b5a910b99..f628f5d38a9 100644 --- a/website/content/docs/drivers/qemu.mdx +++ b/website/content/docs/drivers/qemu.mdx @@ -55,22 +55,16 @@ The `qemu` driver supports the following configuration in the job spec: signal to virtual machines rather than simply terminating them. This emulates a physical power button press, and gives instances a chance to shut down cleanly. If the VM is still running after `kill_timeout`, it will be - forcefully terminated. (Note that - [prior to qemu 2.10.1](https://github.com/qemu/qemu/commit/ad9579aaa16d5b385922d49edac2c96c79bcfb6), - the monitor socket path is limited to 108 characters. Graceful shutdown will - be disabled if QEMU is < 2.10.1 and the generated monitor path exceeds this - length. You may encounter this issue if you set long - [data_dir](/docs/configuration#data_dir) - or - [alloc_dir](/docs/configuration/client#alloc_dir) - paths.) This feature is currently not supported on Windows. - -- `guest_agent` `(bool: false)` - Enable support for the [QEMU Guest - Agent](https://wiki.qemu.org/Features/GuestAgent) for this virtual machine. This - will add the necessary virtual hardware and create a `qemu-guest-agent.sock` - file in the task's working directory for interacting with the agent. The QEMU Guest - Agent must be running in the guest VM. This feature is currently not supported - on Windows. + forcefully terminated. This feature uses a Unix socket that is placed within + the task directory and operating systems may impose a limit on how long these + paths can be. This feature is currently not supported on Windows. + +- `guest_agent` `(bool: false)` - Enable support for the [QEMU Guest + Agent](https://wiki.qemu.org/Features/GuestAgent) for this virtual machine. + This will add the necessary virtual hardware and create a `qa.sock` file in + the task's working directory for interacting with the agent. The QEMU Guest + Agent must be running in the guest VM. This feature is currently not + supported on Windows. - `port_map` - (Optional) A key-value map of port labels. From 06d9261615c7f2c5122bded0f1f2e4877db912bd Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Tue, 2 Aug 2022 18:00:51 -0400 Subject: [PATCH 2/3] changelog: add entry for #13971 --- .changelog/13971.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/13971.txt diff --git a/.changelog/13971.txt b/.changelog/13971.txt new file mode 100644 index 00000000000..3873e254e11 --- /dev/null +++ b/.changelog/13971.txt @@ -0,0 +1,3 @@ +```release-note:improvement +qemu: use shorter socket file names to reduce the chance of hitting the max path length +``` From 2478ad5ce817c53b5eadf7a886a27a3302f65cca Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Tue, 2 Aug 2022 18:16:58 -0400 Subject: [PATCH 3/3] go mod tidy --- go.mod | 1 - go.sum | 1 - 2 files changed, 2 deletions(-) diff --git a/go.mod b/go.mod index 5a777193a3c..6b45cf1c0dd 100644 --- a/go.mod +++ b/go.mod @@ -23,7 +23,6 @@ require ( github.com/containernetworking/cni v1.0.1 github.com/containernetworking/plugins v1.0.1 github.com/coreos/go-iptables v0.6.0 - github.com/coreos/go-semver v0.3.0 github.com/creack/pty v1.1.18 github.com/docker/cli v20.10.3-0.20220113150236-6e2838e18645+incompatible github.com/docker/distribution v2.8.1+incompatible diff --git a/go.sum b/go.sum index f739d8ca5d4..14f7f2c7ed5 100644 --- a/go.sum +++ b/go.sum @@ -352,7 +352,6 @@ github.com/coreos/go-iptables v0.6.0 h1:is9qnZMPYjLd8LYqmm/qlE+wwEgJIkTYdhV3rfZo github.com/coreos/go-iptables v0.6.0/go.mod h1:Qe8Bv2Xik5FyTXwgIbLAnv2sWSBmvWdFETJConOQ//Q= github.com/coreos/go-oidc v2.1.0+incompatible/go.mod h1:CgnwVTmzoESiwO9qyAFEMiHoZ1nMCKZlZ9V6mm3/LKc= github.com/coreos/go-semver v0.2.0/go.mod h1:nnelYz7RCh+5ahJtPPxZlU+153eP4D4r3EedlOD2RNk= -github.com/coreos/go-semver v0.3.0 h1:wkHLiw0WNATZnSG7epLsujiMCgPAc9xhjJ4tgnAxmfM= github.com/coreos/go-semver v0.3.0/go.mod h1:nnelYz7RCh+5ahJtPPxZlU+153eP4D4r3EedlOD2RNk= github.com/coreos/go-systemd v0.0.0-20161114122254-48702e0da86b/go.mod h1:F5haX7vjVVG0kc13fIWeqUViNPyEJxv/OmvnBo0Yme4= github.com/coreos/go-systemd v0.0.0-20180511133405-39ca1b05acc7/go.mod h1:F5haX7vjVVG0kc13fIWeqUViNPyEJxv/OmvnBo0Yme4=