The following pull request was submitted through Github. It can be accessed and reviewed at: https://github.com/lxc/lxd/pull/6988
This e-mail was sent by the LXC bot, direct replies will not reach the author unless they happen to be subscribed to this list. === Description (from pull-request) ===
From 2d725c47f1359fcd86cd19c9147a0a0ca2aa8f65 Mon Sep 17 00:00:00 2001 From: Thomas Parrott <thomas.parr...@canonical.com> Date: Thu, 5 Mar 2020 11:36:40 +0000 Subject: [PATCH 1/6] lxd/device/disk: Adds mountPoolVolume function Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com> --- lxd/device/disk.go | 70 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/lxd/device/disk.go b/lxd/device/disk.go index 7b8dc60c19..fc01f1b748 100644 --- a/lxd/device/disk.go +++ b/lxd/device/disk.go @@ -634,6 +634,76 @@ func (d *disk) generateLimits(runConf *deviceConfig.RunConfig) error { return nil } +// mountPoolVolume mounts the pool volume specified in d.config["source"] from pool specified in d.config["pool"] +// and return the mount path. If the instance type is container volume will be shifted if needed. +func (d *disk) mountPoolVolume(reverter *revert.Reverter) (string, error) { + // Deal with mounting storage volumes created via the storage api. Extract the name of the storage volume + // that we are supposed to attach. We assume that the only syntactically valid ways of specifying a + // storage volume are: + // - <volume_name> + // - <type>/<volume_name> + // Currently, <type> must either be empty or "custom". + // We do not yet support instance mounts. + if filepath.IsAbs(d.config["source"]) { + return "", fmt.Errorf(`When the "pool" property is set "source" must specify the name of a volume, not a path`) + } + + volumeTypeName := "" + volumeName := filepath.Clean(d.config["source"]) + slash := strings.Index(volumeName, "/") + if (slash > 0) && (len(volumeName) > slash) { + // Extract volume name. + volumeName = d.config["source"][(slash + 1):] + // Extract volume type. + volumeTypeName = d.config["source"][:slash] + } + + var srcPath string + + switch volumeTypeName { + case db.StoragePoolVolumeTypeNameContainer: + return "", fmt.Errorf("Using instance storage volumes is not supported") + case "": + // We simply received the name of a storage volume. + volumeTypeName = db.StoragePoolVolumeTypeNameCustom + fallthrough + case db.StoragePoolVolumeTypeNameCustom: + srcPath = shared.VarPath("storage-pools", d.config["pool"], volumeTypeName, volumeName) + case db.StoragePoolVolumeTypeNameImage: + return "", fmt.Errorf("Using image storage volumes is not supported") + default: + return "", fmt.Errorf("Unknown storage type prefix %q found", volumeTypeName) + } + + volumeType, err := storagePools.VolumeTypeNameToType(volumeTypeName) + if err != nil { + return "", err + } + + pool, err := storagePools.GetPoolByName(d.state, d.config["pool"]) + if err != nil { + return "", err + } + + ourMount, err := pool.MountCustomVolume(volumeName, nil) + if err != nil { + return "", errors.Wrapf(err, "Failed mounting storage volume %q of type %q on storage pool %q", volumeName, volumeTypeName, pool.Name()) + } + + if ourMount { + reverter.Add(func() { pool.UnmountCustomVolume(volumeName, nil) }) + } + + if d.inst.Type() == instancetype.Container { + err = d.storagePoolVolumeAttachShift(pool.Name(), volumeName, volumeType) + if err != nil { + return "", errors.Wrapf(err, "Failed shifting storage volume %q of type %q on storage pool %q", volumeName, volumeTypeName, pool.Name()) + } + } + + return srcPath, nil +} + // createDevice creates a disk device mount on host. func (d *disk) createDevice() (string, error) { revert := revert.New() From 4dc23ca9ee09ad6a4d68e0d73de1f9741a9ec152 Mon Sep 17 00:00:00 2001 From: Thomas Parrott <thomas.parr...@canonical.com> Date: Thu, 5 Mar 2020 11:37:19 +0000 Subject: [PATCH 2/6] lxd/device/disk: Error message quoting Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com> --- lxd/device/disk.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/lxd/device/disk.go b/lxd/device/disk.go index fc01f1b748..2548adce94 100644 --- a/lxd/device/disk.go +++ b/lxd/device/disk.go @@ -778,7 +778,7 @@ func (d *disk) createDevice() (string, error) { // Map the RBD. rbdPath, err := diskCephRbdMap(clusterName, userName, poolName, volumeName) if err != nil { - msg := fmt.Sprintf("Could not mount map Ceph RBD: %s.", err) + msg := fmt.Sprintf("Could not mount map Ceph RBD: %v", err) if !isRequired { // Will fail the PathExists test below. logger.Warn(msg) @@ -877,7 +877,7 @@ func (d *disk) createDevice() (string, error) { if !isRequired { return "", nil } - return "", fmt.Errorf("Source path %s doesn't exist for device %s", srcPath, d.name) + return "", fmt.Errorf("Source path %q doesn't exist for device %q", srcPath, d.name) } // Create the devices directory if missing. @@ -1176,7 +1176,7 @@ func (d *disk) postStop() error { go func() { err := diskCephRbdUnmap(v["ceph_rbd"]) if err != nil { - logger.Errorf("Failed to unmap RBD volume '%s' for '%s': %v", v["ceph_rbd"], project.Instance(d.inst.Project(), d.inst.Name()), err) + logger.Errorf("Failed to unmap RBD volume %q for %q: %v", v["ceph_rbd"], project.Instance(d.inst.Project(), d.inst.Name()), err) } }() } @@ -1242,7 +1242,7 @@ func (d *disk) getDiskLimits() (map[string]diskBlockLimit, error) { if !shared.PathExists(source) { // Require that device is mounted before resolving block device if required. if d.isRequired(dev) { - return nil, fmt.Errorf("Block device path doesn't exist: %s", source) + return nil, fmt.Errorf("Block device path doesn't exist %q", source) } continue // Do not resolve block device if device isn't mounted. @@ -1276,7 +1276,7 @@ func (d *disk) getDiskLimits() (map[string]diskBlockLimit, error) { } if blockStr == "" { - return nil, fmt.Errorf("Block device doesn't support quotas: %s", block) + return nil, fmt.Errorf("Block device doesn't support quotas %q", block) } if blockLimits[blockStr] == nil { @@ -1436,7 +1436,7 @@ func (d *disk) getParentBlocks(path string) ([]string, error) { output, err := shared.RunCommand("zpool", "status", "-P", "-L", poolName) if err != nil { - return nil, fmt.Errorf("Failed to query zfs filesystem information for %s: %v", dev[1], err) + return nil, fmt.Errorf("Failed to query zfs filesystem information for %q: %v", dev[1], err) } header := true @@ -1484,13 +1484,13 @@ func (d *disk) getParentBlocks(path string) ([]string, error) { } if len(devices) == 0 { - return nil, fmt.Errorf("Unable to find backing block for zfs pool: %s", poolName) + return nil, fmt.Errorf("Unable to find backing block for zfs pool %q", poolName) } } else if fs == "btrfs" && shared.PathExists(dev[1]) { // Accessible btrfs filesystems output, err := shared.RunCommand("btrfs", "filesystem", "show", dev[1]) if err != nil { - return nil, fmt.Errorf("Failed to query btrfs filesystem information for %s: %v", dev[1], err) + return nil, fmt.Errorf("Failed to query btrfs filesystem information for %q: %v", dev[1], err) } for _, line := range strings.Split(output, "\n") { @@ -1515,7 +1515,7 @@ func (d *disk) getParentBlocks(path string) ([]string, error) { devices = append(devices, fmt.Sprintf("%d:%d", major, minor)) } else { - return nil, fmt.Errorf("Invalid block device: %s", dev[1]) + return nil, fmt.Errorf("Invalid block device %q", dev[1]) } return devices, nil From b6047c27480e410e23987b2d04b2d98c100fbb4b Mon Sep 17 00:00:00 2001 From: Thomas Parrott <thomas.parr...@canonical.com> Date: Thu, 5 Mar 2020 11:37:44 +0000 Subject: [PATCH 3/6] lxd/device/disk: Adds pool volume support for VMs Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com> --- lxd/device/disk.go | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/lxd/device/disk.go b/lxd/device/disk.go index 2548adce94..b61b6d58c5 100644 --- a/lxd/device/disk.go +++ b/lxd/device/disk.go @@ -361,6 +361,7 @@ func (d *disk) startContainer() (*deviceConfig.RunConfig, error) { // startVM starts the disk device for a virtual machine instance. func (d *disk) startVM() (*deviceConfig.RunConfig, error) { runConf := deviceConfig.RunConfig{} + isRequired := d.isRequired(d.config) if shared.IsRootDiskDevice(d.config) { runConf.Mounts = []deviceConfig.MountEntryItem{ @@ -392,11 +393,29 @@ func (d *disk) startVM() (*deviceConfig.RunConfig, error) { srcPath := shared.HostPath(d.config["source"]) - // This is a normal disk device, image or injected 9p directory share. + // Mount the pool volume and update srcPath to mount path. + if d.config["pool"] != "" { + var err error + srcPath, err = d.mountPoolVolume(revert) + if err != nil { + if !isRequired { + // Leave to the pathExists check below. + logger.Warn(err.Error()) + } else { + return nil, err + } + } + } + if !shared.PathExists(srcPath) { - return nil, fmt.Errorf("Cannot find disk source %q", srcPath) + if isRequired { + return nil, fmt.Errorf("Source path %q doesn't exist for device %q", srcPath, d.name) + } + + return &runConf, nil } + // Default to block device or image file passthrough first. mount := deviceConfig.MountEntryItem{ DevPath: srcPath, DevName: d.name, From abf0410f43a33f2173241a69aa38ee5f4a9f911b Mon Sep 17 00:00:00 2001 From: Thomas Parrott <thomas.parr...@canonical.com> Date: Thu, 5 Mar 2020 11:38:12 +0000 Subject: [PATCH 4/6] lxd/device/disk: Switches createDevice to use d.mountPoolVolume for containers Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com> --- lxd/device/disk.go | 70 +++------------------------------------------- 1 file changed, 4 insertions(+), 66 deletions(-) diff --git a/lxd/device/disk.go b/lxd/device/disk.go index b61b6d58c5..811dbedd38 100644 --- a/lxd/device/disk.go +++ b/lxd/device/disk.go @@ -816,74 +816,12 @@ func (d *disk) createDevice() (string, error) { isFile = false } } else { - // Deal with mounting storage volumes created via the storage api. Extract the name - // of the storage volume that we are supposed to attach. We assume that the only - // syntactically valid ways of specifying a storage volume are: - // - <volume_name> - // - <type>/<volume_name> - // Currently, <type> must either be empty or "custom". - // We do not yet support container mounts. - - if filepath.IsAbs(d.config["source"]) { - return "", fmt.Errorf("When the \"pool\" property is set \"source\" must specify the name of a volume, not a path") - } - - volumeTypeName := "" - volumeName := filepath.Clean(d.config["source"]) - slash := strings.Index(volumeName, "/") - if (slash > 0) && (len(volumeName) > slash) { - // Extract volume name. - volumeName = d.config["source"][(slash + 1):] - // Extract volume type. - volumeTypeName = d.config["source"][:slash] - } - - switch volumeTypeName { - case db.StoragePoolVolumeTypeNameContainer: - return "", fmt.Errorf("Using container storage volumes is not supported") - case "": - // We simply received the name of a storage volume. - volumeTypeName = db.StoragePoolVolumeTypeNameCustom - fallthrough - case db.StoragePoolVolumeTypeNameCustom: - srcPath = shared.VarPath("storage-pools", d.config["pool"], volumeTypeName, volumeName) - case db.StoragePoolVolumeTypeNameImage: - return "", fmt.Errorf("Using image storage volumes is not supported") - default: - return "", fmt.Errorf("Unknown storage type prefix %q found", volumeTypeName) - } - - volumeType, err := storagePools.VolumeTypeNameToType(volumeTypeName) - if err != nil { - return "", err - } - - pool, err := storagePools.GetPoolByName(d.state, d.config["pool"]) - if err != nil { - return "", err - } - - // Mount and prepare the volume to be attached. - err = func() error { - ourMount, err := pool.MountCustomVolume(volumeName, nil) - if err != nil { - return errors.Wrapf(err, "Could not mount storage volume %q of type %q on storage pool %q", volumeName, volumeTypeName, d.config["pool"]) - } - - if ourMount { - revert.Add(func() { pool.UnmountCustomVolume(volumeName, nil) }) - } - - err = d.storagePoolVolumeAttachPrepare(pool.Name(), volumeName, volumeType) - if err != nil { - return errors.Wrapf(err, "Could not attach storage volume %q of type %q on storage pool %q", volumeName, volumeTypeName, d.config["pool"]) - } - - return nil - }() + // Mount the pool volume. + var err error + srcPath, err = d.mountPoolVolume(revert) if err != nil { if !isRequired { - // Will fail the PathExists test below. + // Leave to the pathExists check below. logger.Warn(err.Error()) } else { return "", err From 3b507c5f705471ea3535b714a9b095cb404bc8e6 Mon Sep 17 00:00:00 2001 From: Thomas Parrott <thomas.parr...@canonical.com> Date: Thu, 5 Mar 2020 11:38:38 +0000 Subject: [PATCH 5/6] lxd/device/disk: Renames storagePoolVolumeAttachPrepare to storagePoolVolumeAttachShift Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com> --- lxd/device/disk.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lxd/device/disk.go b/lxd/device/disk.go index 811dbedd38..d9717498ab 100644 --- a/lxd/device/disk.go +++ b/lxd/device/disk.go @@ -878,7 +878,7 @@ func (d *disk) createDevice() (string, error) { return devPath, nil } -func (d *disk) storagePoolVolumeAttachPrepare(poolName string, volumeName string, volumeType int) error { +func (d *disk) storagePoolVolumeAttachShift(poolName string, volumeName string, volumeType int) error { // Load the DB records. poolID, pool, err := d.state.Cluster.StoragePoolGet(poolName) if err != nil { From 3a8fca5195cea739d1dba79d99888aecf6361b2b Mon Sep 17 00:00:00 2001 From: Thomas Parrott <thomas.parr...@canonical.com> Date: Thu, 5 Mar 2020 11:39:43 +0000 Subject: [PATCH 6/6] lxd/device/disk: Ensures custom pool volumes are unmounted on VM device stop Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com> --- lxd/device/disk.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lxd/device/disk.go b/lxd/device/disk.go index d9717498ab..bcbbeca2ec 100644 --- a/lxd/device/disk.go +++ b/lxd/device/disk.go @@ -1088,12 +1088,16 @@ func (d *disk) stopVM() (*deviceConfig.RunConfig, error) { os.Remove(filepath.Join(d.inst.DevicesPath(), fmt.Sprintf("%s.sock", d.name))) } - return &deviceConfig.RunConfig{}, nil + runConf := deviceConfig.RunConfig{ + PostHooks: []func() error{d.postStop}, + } + + return &runConf, nil } // postStop is run after the device is removed from the instance. func (d *disk) postStop() error { - // Check if pool-specific action should be taken. + // Check if pool-specific action should be taken to unmount custom volume. if d.config["pool"] != "" { pool, err := storagePools.GetPoolByName(d.state, d.config["pool"]) if err != nil {
_______________________________________________ lxc-devel mailing list lxc-devel@lists.linuxcontainers.org http://lists.linuxcontainers.org/listinfo/lxc-devel