From 2f434bb3c4beb82d5f7b82f1e206de70c66107a8 Mon Sep 17 00:00:00 2001 From: Gabriel Mougard Date: Mon, 8 Jan 2024 15:00:36 +0100 Subject: [PATCH 01/14] api: Add `disk_state_created` API extension Signed-off-by: Gabriel Mougard --- doc/api-extensions.md | 4 ++++ shared/version/api.go | 1 + 2 files changed, 5 insertions(+) diff --git a/doc/api-extensions.md b/doc/api-extensions.md index 694140531e15..dd230ed5682d 100644 --- a/doc/api-extensions.md +++ b/doc/api-extensions.md @@ -2557,3 +2557,7 @@ disk devices. Introduces per-project uplink IP limits for each available uplink network, adding `limits.networks.uplink_ips.ipv4.NETWORK_NAME` and `limits.networks.uplink_ips.ipv6.NETWORK_NAME` configuration keys for projects with `features.networks` enabled. These keys define the maximum value of IPs made available on a network named NETWORK_NAME to be assigned as uplink IPs for entities inside a certain project. These entities can be other networks, network forwards or load balancers. + +## `disk_state_created` + +This API extension provides the ability to check if a target directory was created within the instance file system at mount time. If a host directory is mounted to an existing container directory (e.g., `/opt`), the target directory won't be removed upon unmounting. However, if LXD creates a target directory during the mount, like `/new_dir`, it will be deleted when the device is unmounted. diff --git a/shared/version/api.go b/shared/version/api.go index db46490453ba..1517a933ebc3 100644 --- a/shared/version/api.go +++ b/shared/version/api.go @@ -432,6 +432,7 @@ var APIExtensions = []string{ "network_zones_all_projects", "instance_root_volume_attachment", "projects_limits_uplink_ips", + "disk_state_created", } // APIExtensionsCount returns the number of available API extensions. From 3d1e9c7f1b9292858f5779d4932ee4d426f76bfe Mon Sep 17 00:00:00 2001 From: Gabriel Mougard Date: Mon, 26 Feb 2024 16:32:16 +0100 Subject: [PATCH 02/14] api: Introduce a new `AgentDeviceRemove` type Some devices like disk devices with a target path need to be cleanly unmounted. In the case of a VM instance, we prefer to handle the unmount logic within the agent and handle the unmounting of the potential overmounts of the guest. In some cases, we also need to remove a path resource (or not) on the guest side. That is why we also pass some extra metadata contained in the `Volatile` field, to handle the resource removal process in the agent and not on the LXD side (this would result in spawning aan SFTP client, which is a wasteful approach) Signed-off-by: Gabriel Mougard --- shared/api/agent_device.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 shared/api/agent_device.go diff --git a/shared/api/agent_device.go b/shared/api/agent_device.go new file mode 100644 index 000000000000..1a33464b919d --- /dev/null +++ b/shared/api/agent_device.go @@ -0,0 +1,17 @@ +package api + +// AgentDeviceRemove represents the fields of an device removal request that needs to occur inside the VM agent. +// +// swagger:model +// +// API extension: disk_state_created. +type AgentDeviceRemove struct { + // Type of device ('disk', 'nic', etc.) + Type string `json:"type" yaml:"type"` + // Device configuration map + Config map[string]string `json:"config" yaml:"config"` + // Device name + Name string `json:"name" yaml:"name"` + // Optional device volatile configuration keys + Volatile map[string]string `json:"volatile" yaml:"volatile"` +} From 72e54c145c4b523448f024341b29ab988ccf5765 Mon Sep 17 00:00:00 2001 From: Gabriel Mougard Date: Fri, 26 Jan 2024 12:13:46 +0100 Subject: [PATCH 03/14] instance/instancetype/instance: Add a new volatile key `volatile..last_state.created` for the disk device Signed-off-by: Gabriel Mougard --- lxd/instance/instancetype/instance.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/lxd/instance/instancetype/instance.go b/lxd/instance/instancetype/instance.go index f183b69c7c9a..faf81835c853 100644 --- a/lxd/instance/instancetype/instance.go +++ b/lxd/instance/instancetype/instance.go @@ -1245,6 +1245,17 @@ func ConfigKeyChecker(key string, instanceType Type) (func(value string) error, return validate.IsAny, nil } + // lxdmeta:generate(entities=instance; group=volatile; key=volatile..last_state.created) + // If mounting a host directory through a `disk` device creates the target directory, this option contains the path of this newly created target directory. + // For example, if the target directory is `/opt` and `/opt` already exists in the instance, this key is not set. + // However, if the target directory is `/opt/foo` and `/opt/foo` doesn't exist in the instance, this key is set to `/opt/foo`. + // --- + // type: string + // shortdesc: Path of the directory created from mounting a `disk` device + if strings.HasSuffix(key, ".last_state.created") { + return validate.IsAny, nil + } + if strings.HasSuffix(key, ".driver") { return validate.IsAny, nil } From 0fb83150dedecb2945461f594ae321a7f86975e4 Mon Sep 17 00:00:00 2001 From: Gabriel Mougard Date: Fri, 5 Jan 2024 12:03:12 +0100 Subject: [PATCH 04/14] lxd/device/disk: Add the device name to the mount entry when stopping a device Signed-off-by: Gabriel Mougard --- lxd/device/disk.go | 1 + 1 file changed, 1 insertion(+) diff --git a/lxd/device/disk.go b/lxd/device/disk.go index fa26c2acb492..eaf7171ce401 100644 --- a/lxd/device/disk.go +++ b/lxd/device/disk.go @@ -2049,6 +2049,7 @@ func (d *disk) Stop() (*deviceConfig.RunConfig, error) { // Request an unmount of the device inside the instance. runConf.Mounts = append(runConf.Mounts, deviceConfig.MountEntryItem{ + DevName: d.Name(), TargetPath: relativeDestPath, }) From f13ac979c805bd9417cf78be0e83a2c97b9382a1 Mon Sep 17 00:00:00 2001 From: Gabriel Mougard Date: Wed, 15 Jan 2025 18:24:00 +0100 Subject: [PATCH 05/14] shared: Introduce the `EncodeRemoteAbsPathWithNonExistingDir` function When creating a directory within an instance filesystem, we should be able to track the directory structure that has been created in order to remove the chain of directories when removing a disk device associated to an instance. This function takes an sftp client that, given an absolute path (this path is the desired directory path that a user whishes to create in the instance), it creates a path encoding like the following: /./ ~~~~ SEP_MARK Using this encoding, when unmounting a device associated to a created directory within an instance, we can remove the chain of created directories, now that we know which have been created and which were there before. In the end, we remove at the path `/[0]` (for the VM case) For the container case, we remove directories using the sftp client and since this one doesn't have a recursive remove API for directories, we need to directories from the deepest level all the way up: 1) rm `/[:L]` 2) rm `/[:L-1]` 3) rm `/[:L-2]` ... Signed-off-by: Gabriel Mougard --- shared/util.go | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/shared/util.go b/shared/util.go index 5c98349d9c12..3c537ca03de9 100644 --- a/shared/util.go +++ b/shared/util.go @@ -26,6 +26,7 @@ import ( "time" "github.com/flosch/pongo2" + "github.com/pkg/sftp" "github.com/canonical/lxd/shared/api" "github.com/canonical/lxd/shared/cancel" @@ -1523,3 +1524,94 @@ func IsMicroOVNUsed() bool { return false } + +// EncodeRemoteAbsPathWithNonExistingDir splits a path into existing and non-existing parts and encodes it. +func EncodeRemoteAbsPathWithNonExistingDir(c *sftp.Client, absPath string) (string, error) { + if !filepath.IsAbs(absPath) { + return "", fmt.Errorf("Path is not absolute") + } + + // First, try to resolve the path (handles /./, /../, symlinks, etc.) + // A user might provide a path with /./ or /../ in it, so we need to resolve it. + // Note that this does not return an error if the path doesn't exist. We'll handle that later with Lstat. + resolvedExistingPath, err := c.RealPath(absPath) + if err != nil { + return "", fmt.Errorf("Failed to resolve path while encoding path with non existing directory %q: %w", absPath, err) + } + + // Now, check the path existence + _, err = c.Lstat(resolvedExistingPath) + if err == nil { + // The path exists. We can just return it as is. + return resolvedExistingPath, nil + } + + // If there is an error, it should be because the path doesn't exist. + if !os.IsNotExist(err) { + return "", err + } + + // The path doesn't exist. We need to split it into existing and non-existing parts, + // in order to be able to create the non-existing parts later. + currentPath := string(os.PathSeparator) + parts := strings.Split(resolvedExistingPath, string(os.PathSeparator))[1:] + + // Find the split point between existing and non-existing parts. + // We start iterating from the root of the provided path, and for each part, we check if it exists. + splitIndex := -1 + for i, part := range parts { + if part == "" { + continue + } + + if part == "." || part == ".." { + currentPath = filepath.Join(currentPath, part) + continue + } + + testPath := filepath.Join(currentPath, part) + p, err := c.Lstat(testPath) + if err != nil { + if os.IsNotExist(err) { + splitIndex = i + break + } + + return "", err + } + + if p.Mode()&os.ModeSymlink != 0 { + // This is a symlink, so we need to resolve it first. + resolvedPath, err := c.ReadLink(testPath) + if err != nil { + return "", err + } + + currentPath = resolvedPath + } else { + currentPath = testPath + } + } + + // Add a '/./' marker to separate the existing and non-existing parts + sep := string(os.PathSeparator) + "." + string(os.PathSeparator) + finalPath := string(os.PathSeparator) + filepath.Join(parts[:splitIndex]...) + sep + filepath.Join(parts[splitIndex:]...) + + // Check if the final path is 'clean' (e.g, `/a/b/./../c/d` is valid but `/../../..` is invalid since the resolved path goes out of the root). + cleanedPath := path.Clean(finalPath) + if strings.HasPrefix(cleanedPath, "../") || strings.HasPrefix(cleanedPath, "/..") || !strings.HasPrefix(cleanedPath, "/") { + return "", fmt.Errorf("Invalid cleaned final path, %q", cleanedPath) + } + + return finalPath, nil +} + +// DecodeRemoteAbsPathWithNonExistingDir splits an encoded path back into its parts. +func DecodeRemoteAbsPathWithNonExistingDir(encodedPath string) (existing, nonExisting string) { + parts := strings.Split(encodedPath, "/./") + if len(parts) == 1 { + return parts[0], "" + } + + return parts[0], parts[1] +} From a36e04f4852050c857e09d2f5bdbd0e5a07ec0a6 Mon Sep 17 00:00:00 2001 From: Gabriel Mougard Date: Thu, 4 Jan 2024 16:55:37 +0100 Subject: [PATCH 06/14] lxd/instance/drivers/lxc: detect if we need to remove a target directory when unmounting When a disk device is removed while relying on a host directory and mapped to a target within a container, we detect if the target directory has been created by LXD or not in order to not delete the content of a target directory during the unmount. Example: - Let say we mounted a custom host empty directory (`test`) on the existing `/opt` directory of the container, when unmounted (`lxc device remove ...`), the target `/opt` won't be removed, because we marked it as NOT being created by LXD at mount time. - If, however, we created a custom empty target directory at mount time: `lxc config device add u1 test disk source=/home/user/test path=/new_dir`, the directory `new_dir` will be created on the target instance and if we decide to unmount `test`, the target `/new_dir` will be removed because is has been created by LXD Signed-off-by: Gabriel Mougard --- lxd/instance/drivers/driver_lxc.go | 123 ++++++++++++++++++++++++----- 1 file changed, 105 insertions(+), 18 deletions(-) diff --git a/lxd/instance/drivers/driver_lxc.go b/lxd/instance/drivers/driver_lxc.go index 403addda2042..2c1559a5c71f 100644 --- a/lxd/instance/drivers/driver_lxc.go +++ b/lxd/instance/drivers/driver_lxc.go @@ -1704,6 +1704,15 @@ func (d *lxc) deviceDetachNIC(configCopy map[string]string, netIF []deviceConfig func (d *lxc) deviceHandleMounts(mounts []deviceConfig.MountEntryItem) error { reverter := revert.New() defer reverter.Fail() + + // Connect to files API. + files, err := d.FileSFTP() + if err != nil { + return err + } + + defer func() { _ = files.Close() }() + for _, mount := range mounts { if mount.DevPath != "" { flags := 0 @@ -1727,35 +1736,113 @@ func (d *lxc) deviceHandleMounts(mounts []deviceConfig.MountEntryItem) error { } } + _, err = files.Lstat(mount.TargetPath) + if err != nil { + absTargetPath := mount.TargetPath + if !strings.HasPrefix(mount.TargetPath, "/") { + absTargetPath = fmt.Sprintf("/%s", mount.TargetPath) + } + + encodedPath, err := shared.EncodeRemoteAbsPathWithNonExistingDir(files, absTargetPath) + if err != nil { + return fmt.Errorf("Failed to encode remote path: %w", err) + } + + err = d.deviceVolatileSetFunc(mount.DevName)(map[string]string{"last_state.created": encodedPath}) // We want to store the absolute path. + if err != nil { + return fmt.Errorf("Error updating volatile for the device: %w", err) + } + } + // Mount it into the container. - err := d.insertMount(mount.DevPath, mount.TargetPath, mount.FSType, flags, idmapType) + err = d.insertMount(mount.DevPath, mount.TargetPath, mount.FSType, flags, idmapType) if err != nil { return fmt.Errorf("Failed to add mount for device inside container: %s", err) } } else { - relativeTargetPath := strings.TrimPrefix(mount.TargetPath, "/") + _, err = files.Lstat(mount.TargetPath) + if err == nil { + err = d.removeMount(mount.TargetPath) + if err != nil { + return fmt.Errorf("Error unmounting the device path inside container: %w", err) + } - // Connect to files API. - files, err := d.FileSFTP() - if err != nil { - return err - } + removeTargetFiles := false - reverter.Add(func() { _ = files.Close() }) + // Check if the target path hasn't been created by LXD + mountConf := d.deviceVolatileGetFunc(mount.DevName)() + targetPath := mountConf["last_state.created"] + absMountTargetPath := mount.TargetPath + if !strings.HasPrefix(mount.TargetPath, "/") { + absMountTargetPath = fmt.Sprintf("/%s", mount.TargetPath) + } - _, err = files.Lstat(relativeTargetPath) - if err == nil { - err := d.removeMount(mount.TargetPath) + encodedTargetPath, err := shared.EncodeRemoteAbsPathWithNonExistingDir(files, absMountTargetPath) if err != nil { - return fmt.Errorf("Error unmounting the device path inside container: %s", err) + return fmt.Errorf("Failed to encode remote path: %w", err) } - err = files.Remove(relativeTargetPath) - if err != nil { - // Only warn here and don't fail as removing a directory - // mount may fail if there was already files inside - // directory before it was mouted over preventing delete. - d.logger.Warn("Could not remove the device path inside container", logger.Ctx{"err": err}) + if filepath.Clean(targetPath) == filepath.Clean(encodedTargetPath) { + removeTargetFiles = true + } + + if removeTargetFiles { + // Check if the path stored for the `last_state.created` volatile key + // contains the '/./' marker, which indicates that the left part of the path + // exists and the right part did not exist at the time of its creation during the mount. + // In this case, we should remove / instead of the full path. + if strings.Contains(targetPath, "/./") { + formerlyExistingPathPart, formerlyNonExistingPart := shared.DecodeRemoteAbsPathWithNonExistingDir(targetPath) + // Take the first component of the non-existing part. + parts := strings.Split(formerlyNonExistingPart, "/") + if len(parts) > 0 { + if formerlyExistingPathPart == "" { + formerlyExistingPathPart = "/" + } + + // SFTP doesn't support recursive directory removal (and it would be unsafe to use in this context anyway), so we need to remove the + // directory tree from the deepest level to the top. + for i := 0; i < len(parts); i++ { + pathToRemove := filepath.Clean(filepath.Join(formerlyExistingPathPart, strings.Join(parts[:len(parts)-i], "/"))) + fi, err := files.Lstat(pathToRemove) + if err != nil { + return fmt.Errorf("Failed to stat path to be removed %q: %w", pathToRemove, err) + } + + if fi.IsDir() { + entries, err := files.ReadDir(pathToRemove) + if err != nil { + return fmt.Errorf("Failed to read directory entries: %w", err) + } + + if len(entries) > 0 { + d.logger.Warn("Could not remove the device path inside container because a directory that needs to be removed is not empty", logger.Ctx{"entres": entries}) + break + } + } + + err = files.Remove(pathToRemove) + if err != nil { + d.logger.Warn("Could not remove the device path inside container", logger.Ctx{"err": err}) + break + } + } + } + } else { + err = files.Remove(targetPath) + if err != nil { + // Only warn here and don't fail as removing a directory + // mount may fail if there was already files inside + // directory before it was mouted over preventing delete. + d.logger.Warn("Could not remove the device path inside container", logger.Ctx{"err": err}) + } + } + } else { + // Remove option from the device. + err = d.deviceVolatileSetFunc(mount.DevName)(map[string]string{"last_state.created": ""}) + if err != nil { + return fmt.Errorf("Error updating volatile for the device: %w", err) + } } } From 8a77ebec8cd3dbf8fd8de7407d8b9269bd37cabb Mon Sep 17 00:00:00 2001 From: Gabriel Mougard Date: Wed, 21 Feb 2024 17:42:48 +0100 Subject: [PATCH 07/14] lxd/instance/drivers/qemu: fix error formatting Signed-off-by: Gabriel Mougard --- lxd/instance/drivers/driver_lxc.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/lxd/instance/drivers/driver_lxc.go b/lxd/instance/drivers/driver_lxc.go index 2c1559a5c71f..7bedc0abef8b 100644 --- a/lxd/instance/drivers/driver_lxc.go +++ b/lxd/instance/drivers/driver_lxc.go @@ -1497,7 +1497,7 @@ func (d *lxc) deviceStart(dev device.Device, instanceRunning bool) (*deviceConfi func (d *lxc) deviceStaticShiftMounts(mounts []deviceConfig.MountEntryItem) error { idmapSet, err := d.CurrentIdmap() if err != nil { - return fmt.Errorf("Failed to get idmap for device: %s", err) + return fmt.Errorf("Failed to get idmap for device: %w", err) } // If there is an idmap being applied and LXD not running in a user namespace then shift the @@ -1757,7 +1757,7 @@ func (d *lxc) deviceHandleMounts(mounts []deviceConfig.MountEntryItem) error { // Mount it into the container. err = d.insertMount(mount.DevPath, mount.TargetPath, mount.FSType, flags, idmapType) if err != nil { - return fmt.Errorf("Failed to add mount for device inside container: %s", err) + return fmt.Errorf("Failed to add mount for device inside container: %w", err) } } else { _, err = files.Lstat(mount.TargetPath) @@ -4253,7 +4253,7 @@ func (d *lxc) Update(args db.InstanceArgs, userRequested bool) error { if args.Architecture != 0 { _, err = osarch.ArchitectureName(args.Architecture) if err != nil { - return fmt.Errorf("Invalid architecture id: %s", err) + return fmt.Errorf("Invalid architecture id: %w", err) } } @@ -7711,12 +7711,12 @@ func (d *lxc) insertMountLXD(source, target, fstype string, flags int, mntnsPID if shared.IsDir(source) { tmpMount, err = os.MkdirTemp(d.ShmountsPath(), "lxdmount_") if err != nil { - return fmt.Errorf("Failed to create shmounts path: %s", err) + return fmt.Errorf("Failed to create shmounts path: %w", err) } } else { f, err := os.CreateTemp(d.ShmountsPath(), "lxdmount_") if err != nil { - return fmt.Errorf("Failed to create shmounts path: %s", err) + return fmt.Errorf("Failed to create shmounts path: %w", err) } tmpMount = f.Name() @@ -7728,7 +7728,7 @@ func (d *lxc) insertMountLXD(source, target, fstype string, flags int, mntnsPID // Mount the filesystem err = unix.Mount(source, tmpMount, fstype, uintptr(flags), "") if err != nil { - return fmt.Errorf("Failed to setup temporary mount: %s", err) + return fmt.Errorf("Failed to setup temporary mount: %w", err) } defer func() { _ = unix.Unmount(tmpMount, unix.MNT_DETACH) }() @@ -7970,7 +7970,7 @@ func (d *lxc) InsertSeccompUnixDevice(prefix string, m deviceConfig.Device, pid dev, err := device.UnixDeviceCreate(d.state, idmapSet, d.DevicesPath(), prefix, m, true) if err != nil { - return fmt.Errorf("Failed to setup device: %s", err) + return fmt.Errorf("Failed to setup device: %w", err) } devPath := dev.HostPath From 9602b8500b80431f9c4899eae40d13ebf6f184e3 Mon Sep 17 00:00:00 2001 From: Gabriel Mougard Date: Wed, 10 Jan 2024 16:43:05 +0100 Subject: [PATCH 08/14] lxd-agent: unmount logic inside a VM Signed-off-by: Gabriel Mougard --- lxd-agent/devices.go | 127 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 127 insertions(+) create mode 100644 lxd-agent/devices.go diff --git a/lxd-agent/devices.go b/lxd-agent/devices.go new file mode 100644 index 000000000000..b2e376b5e6f5 --- /dev/null +++ b/lxd-agent/devices.go @@ -0,0 +1,127 @@ +package main + +import ( + "bufio" + "encoding/json" + "fmt" + "net/http" + "os" + "path/filepath" + "strings" + + "golang.org/x/sys/unix" + + "github.com/canonical/lxd/lxd/response" + "github.com/canonical/lxd/shared" + "github.com/canonical/lxd/shared/api" +) + +var devicesCmd = APIEndpoint{ + Path: "devices", + + Delete: APIEndpointAction{Handler: deviceDelete}, +} + +// deviceDelete handles the removal of a device from the VM agent. +// e.g, if the device is a disk mount, this will cleanly unmount it and remove it if necessary. +func deviceDelete(d *Daemon, r *http.Request) response.Response { + var device api.AgentDeviceRemove + + err := json.NewDecoder(r.Body).Decode(&device) + if err != nil { + return response.InternalError(err) + } + + // We only support disk devices for now + if device.Type != "disk" { + return response.BadRequest(fmt.Errorf("Device type %q not supported for removal within VM agent", device.Type)) + } + + targetPath := device.Config["path"] + + if !filepath.IsAbs(targetPath) { + return response.SmartError(fmt.Errorf("The device path must be absolute: %q", device.Config["path"])) + } + + file, err := os.Open("/proc/self/mountinfo") + if err != nil { + return response.SmartError(fmt.Errorf("Error opening /proc/self/mountinfo: %v", err)) + } + + defer file.Close() + + var mountPoints []string + scanner := bufio.NewScanner(file) + for scanner.Scan() { + fields := strings.Fields(scanner.Text()) + if len(fields) >= 10 && strings.HasPrefix(fields[4], strings.TrimSuffix(targetPath, "/")) { + mountPoints = append(mountPoints, fields[4]) + } + } + + err = scanner.Err() + if err != nil { + return response.SmartError(fmt.Errorf("Error reading /proc/self/mountinfo: %v", err)) + } + + if len(mountPoints) == 0 { + return response.SmartError(fmt.Errorf("No mount points found for %s", targetPath)) + } + + // Reverse the slice to unmount in reverse order. + // This is needed to unmount potential over-mounts first. + for i, j := 0, len(mountPoints)-1; i < j; i, j = i+1, j-1 { + mountPoints[i], mountPoints[j] = mountPoints[j], mountPoints[i] + } + + for _, mountPoint := range mountPoints { + err = unix.Unmount(mountPoint, unix.MNT_DETACH) + if err != nil { + return response.SmartError(fmt.Errorf("Error unmounting %s: %v", mountPoint, err)) + } + } + + // Now that the unmount has occurred, + // check if we need to remove the target path. + if device.Volatile != nil { + path, ok := device.Volatile["last_state.created"] + if ok { + if filepath.Clean(targetPath) == filepath.Clean(path) { + // Check if the path stored for the `last_state.created` volatile key + // contains the '/./' marker, which indicates that the left part of the path + // exists and the right part did not exist at the time of its creation during the mount. + // In this case, we should remove / instead of the full path. + if strings.Contains(path, "/./") { + formerlyExistingPathPart, formerlyNonExistingPart := shared.DecodeRemoteAbsPathWithNonExistingDir(path) + // Take the first component of the non-existing part. + parts := strings.Split(formerlyNonExistingPart, "/") + if len(parts) > 0 { + if formerlyExistingPathPart == "" { + formerlyExistingPathPart = "/" + } + } + + if len(parts) > 0 { + if formerlyExistingPathPart == "" { + formerlyExistingPathPart = "/" + } + + // Remove the directory tree from the deepest level to the top. + // This will fail if the chain of directories contains files/directories other than the ones in the chain. + for i := 0; i < len(parts); i++ { + pathToRemove := filepath.Clean(filepath.Join(formerlyExistingPathPart, strings.Join(parts[:len(parts)-i], "/"))) + if strings.HasPrefix(pathToRemove, "/") { + err = os.Remove(pathToRemove) + if err != nil { + return response.SmartError(fmt.Errorf("Failed to remove directory during device deletion: %v", err)) + } + } + } + } + } + } + } + } + + return response.EmptySyncResponse +} From 95c98aaf3132f337aa308fe1a67ca57d53208fc2 Mon Sep 17 00:00:00 2001 From: Gabriel Mougard Date: Mon, 26 Feb 2024 16:43:17 +0100 Subject: [PATCH 09/14] lxd-agent: expose the `DELETE 1.0/devices` VM agent endpoint Signed-off-by: Gabriel Mougard --- lxd-agent/api_1.0.go | 1 + 1 file changed, 1 insertion(+) diff --git a/lxd-agent/api_1.0.go b/lxd-agent/api_1.0.go index 71558d9db832..8605d8cd9b36 100644 --- a/lxd-agent/api_1.0.go +++ b/lxd-agent/api_1.0.go @@ -29,6 +29,7 @@ var api10 = []APIEndpoint{ api10Cmd, execCmd, eventsCmd, + devicesCmd, metricsCmd, operationsCmd, operationCmd, From 56b3430166e332c78a18d851490330c85646da49 Mon Sep 17 00:00:00 2001 From: Gabriel Mougard Date: Mon, 26 Feb 2024 16:42:25 +0100 Subject: [PATCH 10/14] lxd/instance/drivers/qemu: Add the `devlxdDeviceRemove` function Signed-off-by: Gabriel Mougard --- lxd/instance/drivers/driver_qemu.go | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/lxd/instance/drivers/driver_qemu.go b/lxd/instance/drivers/driver_qemu.go index c704469d343f..0cc553ba9d44 100644 --- a/lxd/instance/drivers/driver_qemu.go +++ b/lxd/instance/drivers/driver_qemu.go @@ -8623,6 +8623,34 @@ func (d *qemu) devlxdEventSend(eventType string, eventMessage map[string]any) er return nil } +func (d *qemu) devlxdDeviceRemove(deviceType string, deviceName string, deviceConfig map[string]string, deviceVolatile map[string]string) error { + agentDevice := shared.Jmap{} + agentDevice["type"] = deviceType + agentDevice["config"] = deviceConfig + agentDevice["name"] = deviceName + agentDevice["volatile"] = deviceVolatile + + client, err := d.getAgentClient() + if err != nil { + return err + } + + agent, err := lxd.ConnectLXDHTTP(nil, client) + if err != nil { + d.logger.Error("Failed to connect to lxd-agent", logger.Ctx{"err": err}) + return fmt.Errorf("Failed to connect to lxd-agent") + } + + defer agent.Disconnect() + + _, _, err = agent.RawQuery("DELETE", "/1.0/devices", &agentDevice, "") + if err != nil { + return err + } + + return nil +} + // Info returns "qemu" and the currently loaded qemu version. func (d *qemu) Info() instance.Info { data := instance.Info{ From 0267a4618d4a4d73bd08dd19f92161de077e0e30 Mon Sep 17 00:00:00 2001 From: Gabriel Mougard Date: Tue, 9 Jan 2024 18:50:12 +0100 Subject: [PATCH 11/14] lxd/instance/driver_qemu: Ensure the underlying VM virtiofs mount has been unmounted Signed-off-by: Gabriel Mougard --- lxd/instance/drivers/driver_qemu.go | 62 +++++++++++++++++++++++++++-- 1 file changed, 58 insertions(+), 4 deletions(-) diff --git a/lxd/instance/drivers/driver_qemu.go b/lxd/instance/drivers/driver_qemu.go index 0cc553ba9d44..cd53ff6c9be8 100644 --- a/lxd/instance/drivers/driver_qemu.go +++ b/lxd/instance/drivers/driver_qemu.go @@ -2148,7 +2148,7 @@ func (d *qemu) deviceStart(dev device.Device, instanceRunning bool) (*deviceConf for i, mount := range runConf.Mounts { if mount.FSType == "9p" { - mountTag, err := d.deviceAttachPath(dev.Name()) + mountTag, err := d.deviceAttachPath(dev.Name(), mount) if err != nil { return nil, err } @@ -2182,7 +2182,7 @@ func (d *qemu) deviceStart(dev device.Device, instanceRunning bool) (*deviceConf return runConf, nil } -func (d *qemu) deviceAttachPath(deviceName string) (mountTag string, err error) { +func (d *qemu) deviceAttachPath(deviceName string, mount deviceConfig.MountEntryItem) (mountTag string, err error) { deviceID := qemuDeviceNameOrID(qemuDeviceIDPrefix, deviceName, "-virtio-fs", qemuDeviceIDMaxLength) mountTag = qemuDeviceNameOrID(qemuDeviceNamePrefix, deviceName, "", qemuDeviceNameMaxLength) @@ -2285,6 +2285,32 @@ func (d *qemu) deviceAttachPath(deviceName string) (mountTag string, err error) return "", fmt.Errorf("Failed to add the virtiofs device: %w", err) } + // Connect to files API. + files, err := d.FileSFTP() + if err != nil { + return "", err + } + + defer func() { _ = files.Close() }() + + _, err = files.Lstat(mount.TargetPath) + if err != nil { + absTargetPath := mount.TargetPath + if !strings.HasPrefix(mount.TargetPath, "/") { + absTargetPath = fmt.Sprintf("/%s", mount.TargetPath) + } + + encodedPath, err := shared.EncodeRemoteAbsPathWithNonExistingDir(files, absTargetPath) + if err != nil { + return "", fmt.Errorf("Failed to encode remote path: %w", err) + } + + err = d.deviceVolatileSetFunc(mount.DevName)(map[string]string{"last_state.created": encodedPath}) + if err != nil { + return "", fmt.Errorf("Error updating volatile for the device: %w", err) + } + } + reverter.Success() return mountTag, nil } @@ -2309,7 +2335,29 @@ func (d *qemu) deviceAttachBlockDevice(mount deviceConfig.MountEntryItem) error return nil } -func (d *qemu) deviceDetachPath(deviceName string) error { +func (d *qemu) unmountPath(deviceName string, rawConfig deviceConfig.Device) error { + // First, cleanly unmount the path inside the VM. + volatileMountConf := d.deviceVolatileGetFunc(deviceName)() + err := d.devlxdDeviceRemove("disk", deviceName, rawConfig, volatileMountConf) + if err != nil { + return err + } + + // Then, remove the volatile key if the path has changed. + targetPath := rawConfig["path"] + lastPath, ok := volatileMountConf["last_state.created"] + if !ok || targetPath != lastPath { + // Remove option from the mount. + err = d.deviceVolatileSetFunc(deviceName)(map[string]string{"last_state.created": ""}) + if err != nil { + return fmt.Errorf("Error updating volatile for the device: %w", err) + } + } + + return nil +} + +func (d *qemu) deviceDetachPath(deviceName string, rawConfig deviceConfig.Device) error { deviceID := qemuDeviceNameOrID(qemuDeviceIDPrefix, deviceName, "-virtio-fs", qemuDeviceIDMaxLength) // Check if the agent is running. @@ -2341,6 +2389,12 @@ func (d *qemu) deviceDetachPath(deviceName string) error { } } + // We still need to remove the virtiofs mount inside the VM. + err = d.unmountPath(deviceName, rawConfig) + if err != nil { + return err + } + return nil } @@ -2490,7 +2544,7 @@ func (d *qemu) deviceStop(dev device.Device, instanceRunning bool, _ string) err // Detach disk from running instance. if configCopy["type"] == "disk" { if configCopy["path"] != "" { - err = d.deviceDetachPath(dev.Name()) + err = d.deviceDetachPath(dev.Name(), configCopy) if err != nil { return err } From f1b37688d18283c46c408b86d96b392421dd5005 Mon Sep 17 00:00:00 2001 From: Gabriel Mougard Date: Fri, 5 Jan 2024 12:50:46 +0100 Subject: [PATCH 12/14] test: Check that we can still access the target dir if it hasn''t been created by lxd Signed-off-by: Gabriel Mougard --- test/suites/container_devices_disk.sh | 36 +++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/test/suites/container_devices_disk.sh b/test/suites/container_devices_disk.sh index 34479d796350..5d865a13b91e 100644 --- a/test/suites/container_devices_disk.sh +++ b/test/suites/container_devices_disk.sh @@ -85,6 +85,42 @@ _container_devices_disk_shift() { [ "$(lxc exec foo-isol1 -- stat /mnt/a -c '%u:%g')" = "123:456" ] [ "$(lxc exec foo-isol2 -- stat /mnt/a -c '%u:%g')" = "123:456" ] + mkdir -p "${TEST_DIR}/empty-dir" + lxc exec foo -- mkdir /opt + lxc config device add foo empty-dir disk source="${TEST_DIR}/empty-dir" path=/opt + # Check that no `volatile..last_state.created` has been set. + if [ -n "$(lxc config get foo volatile.empty-dir.last_state.created)" ]; then + echo "==> volatile..last_state.created should not be set" + false + fi + + lxc config device remove foo empty-dir + lxc exec foo -- stat /opt # /opt is not removed as it was not created by LXD. + + lxc exec foo -- mkdir -p /tmp/empty-dir # Create a new directory to check that it is not removed when unmounting the disk. + lxc config device add foo empty-dir disk source="${TEST_DIR}/empty-dir" path=/tmp/empty-dir + # Check that no `volatile..last_state.created` has been set. + if [ -n "$(lxc config get foo volatile.empty-dir.last_state.created)" ]; then + echo "==> volatile..last_state.created should not be set" + false + fi + + lxc config device remove foo empty-dir + lxc exec foo -- stat /tmp/empty-dir + + lxc config device add foo empty-dir disk source="${TEST_DIR}/empty-dir" path=/tmp/new-dir-parent/new-dir-child # /tmp/new-dir-parent/new-dir-child is created by LXD. + # Check that the volatile..last_state.created is set. + targetPath=$(lxc config get foo volatile.empty-dir.last_state.created) + if [ "${targetPath}" != "/tmp/./new-dir-parent/new-dir-child" ]; then + echo "==> volatile..last_state.created is not set to the correct value" + false + fi + + lxc config device remove foo empty-dir + ! lxc exec foo -- stat /tmp/new-dir-parent/new-dir-child || false # /tmp/new-dir-parent/new-dir-child is removed as it was created by LXD. + ! lxc exec foo -- stat /tmp/new-dir-parent || false # /tmp/new-dir-parent is removed as it was created by LXD. + rm -rf "${TEST_DIR}/empty-dir" + lxc delete -f foo-priv foo-isol1 foo-isol2 lxc config device remove foo shifted lxc storage volume delete "${POOL}" foo-shift From 864bc2b13eadeb8d1341e5bae9c4452a8a01ef9b Mon Sep 17 00:00:00 2001 From: Gabriel Mougard Date: Fri, 5 Jan 2024 12:11:45 +0100 Subject: [PATCH 13/14] lxd/metadata: update metadata configuration Signed-off-by: Gabriel Mougard --- doc/metadata.txt | 8 ++++++++ lxd/metadata/configuration.json | 7 +++++++ 2 files changed, 15 insertions(+) diff --git a/doc/metadata.txt b/doc/metadata.txt index 0516a3859197..b3386d993f77 100644 --- a/doc/metadata.txt +++ b/doc/metadata.txt @@ -2425,6 +2425,14 @@ Specify either a cron expression (` `), a comm +```{config:option} volatile..last_state.created instance-volatile +:shortdesc: "Path of the directory created from mounting a `disk` device" +:type: "string" +If mounting a host directory through a `disk` device creates the target directory, this option contains the path of this newly created target directory. +For example, if the target directory is `/opt` and `/opt` already exists in the instance, this key is not set. +However, if the target directory is `/opt/foo` and `/opt/foo` doesn't exist in the instance, this key is set to `/opt/foo`. +``` + ```{config:option} volatile..apply_quota instance-volatile :shortdesc: "Disk quota" :type: "string" diff --git a/lxd/metadata/configuration.json b/lxd/metadata/configuration.json index 5b099c44ffac..b315eb87afe0 100644 --- a/lxd/metadata/configuration.json +++ b/lxd/metadata/configuration.json @@ -2727,6 +2727,13 @@ }, "volatile": { "keys": [ + { + "volatile.\u003cdisk_dev_name\u003e.last_state.created": { + "longdesc": "If mounting a host directory through a `disk` device creates the target directory, this option contains the path of this newly created target directory.\nFor example, if the target directory is `/opt` and `/opt` already exists in the instance, this key is not set.\nHowever, if the target directory is `/opt/foo` and `/opt/foo` doesn't exist in the instance, this key is set to `/opt/foo`.", + "shortdesc": "Path of the directory created from mounting a `disk` device", + "type": "string" + } + }, { "volatile.\u003cname\u003e.apply_quota": { "longdesc": "The disk quota is applied the next time the instance starts.", From d290c7fcc68933f18183ab4285d37197e5b5f426 Mon Sep 17 00:00:00 2001 From: Gabriel Mougard Date: Mon, 26 Feb 2024 16:49:55 +0100 Subject: [PATCH 14/14] doc: update rest API Signed-off-by: Gabriel Mougard --- doc/rest-api.yaml | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/doc/rest-api.yaml b/doc/rest-api.yaml index a7e199e360d7..1840db710e74 100644 --- a/doc/rest-api.yaml +++ b/doc/rest-api.yaml @@ -1,4 +1,29 @@ definitions: + AgentDeviceRemove: + properties: + config: + additionalProperties: + type: string + description: Device configuration map + type: object + x-go-name: Config + name: + description: Device name + type: string + x-go-name: Name + type: + description: Type of device ('disk', 'nic', etc.) + type: string + x-go-name: Type + volatile: + additionalProperties: + type: string + description: Optional device volatile configuration keys + type: object + x-go-name: Volatile + title: AgentDeviceRemove represents the fields of an device removal request that needs to occur inside the VM agent. + type: object + x-go-package: github.com/canonical/lxd/shared/api AuthGroup: properties: description: