Device: Don't remove an existing target directory when unmounting a disk device if the original dir hasn't been created by LXD

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.

doc/metadata.txt

@@ -2425,6 +2425,14 @@ Specify either a cron expression (`<minute> <hour> <dom> <month> <dow>`), a comm
 
 <!-- config group instance-snapshots end -->
 <!-- config group instance-volatile start -->
+```{config:option} volatile.<disk_dev_name>.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.<name>.apply_quota instance-volatile
 :shortdesc: "Disk quota"
 :type: "string"

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:

lxd-agent/api_1.0.go

@@ -29,6 +29,7 @@ var api10 = []APIEndpoint{
 	api10Cmd,
 	execCmd,
 	eventsCmd,
+	devicesCmd,
 	metricsCmd,
 	operationsCmd,
 	operationCmd,

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 <left_part>/<first_component_of_right_part> 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
+}

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,
 	})
 

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
@@ -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 {
 				}
 			}
 
-			// Mount it into the container.
-			err := d.insertMount(mount.DevPath, mount.TargetPath, mount.FSType, flags, idmapType)
+			_, err = files.Lstat(mount.TargetPath)
 			if err != nil {
-				return fmt.Errorf("Failed to add mount for device inside container: %s", err)
+				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)
+				}
 			}
-		} else {
-			relativeTargetPath := strings.TrimPrefix(mount.TargetPath, "/")
 
-			// Connect to files API.
-			files, err := d.FileSFTP()
+			// Mount it into the container.
+			err = d.insertMount(mount.DevPath, mount.TargetPath, mount.FSType, flags, idmapType)
 			if err != nil {
-				return err
+				return fmt.Errorf("Failed to add mount for device inside container: %w", err)
 			}
-
-			reverter.Add(func() { _ = files.Close() })
-
-			_, err = files.Lstat(relativeTargetPath)
+		} else {
+			_, err = files.Lstat(mount.TargetPath)
 			if err == nil {
-				err := d.removeMount(mount.TargetPath)
+				err = d.removeMount(mount.TargetPath)
 				if err != nil {
-					return fmt.Errorf("Error unmounting the device path inside container: %s", err)
+					return fmt.Errorf("Error unmounting the device path inside container: %w", err)
 				}
 
-				err = files.Remove(relativeTargetPath)
+				removeTargetFiles := false
+
+				// 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)
+				}
+
+				encodedTargetPath, err := shared.EncodeRemoteAbsPathWithNonExistingDir(files, absMountTargetPath)
 				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})
+					return fmt.Errorf("Failed to encode remote path: %w", 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 <left_part>/<first_component_of_right_part> 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)
+					}
 				}
 			}
 
@@ -4166,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)
 		}
 	}
 
@@ -7624,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()
@@ -7641,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) }()
@@ -7883,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