The following pull request was submitted through Github.
It can be accessed and reviewed at: https://github.com/lxc/lxd/pull/6932
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) ===
Addresses issue https://github.com/lxc/lxd/issues/6929
There are 2 issues here:
1. We need to move the GPT alternative header to the end of the disk after unpacking a VM image to a disk file. This is what the GPT standard requires so it knows where to find the backup header. This change needs to be added to each VM capable driver:
- [ ] ZFS
- [ ] DIR
- [ ] BTRFS
- [ ] CEPH
- [ ] LVM
2. When cloning a ZFS volume from a snapshot, because snapshots don't have the `volmode` property, the property is instead inherited from parent (which LXD currently doesn't modify from default), and so when the new volume is created ZFS then probes the partitions on the disk and creates a device for each partition. This probing causes the host kernel to detect the the problem GPT header and logs warnings to the kernel log.
There is also potentially a bug in ZFS itself, because specifying `volmode=none` on the `zfs clone` command is not respected and instead it still uses the parent setting, even though `zfs get volmode` will show the correct setting on the cloned volume.
Although fixing the first issue will fix the second issue, it is an indication of a wider problem in that we do not want the partitions of the VM image to be presented on the host.
This PR will also add a patch to set `volmode=none` to the parent on `lxd/virtual-machines`.
- [ ] Patch for existing pools
- [ ] Fix for new pools
From 24afe0a39d070855f3833f2d1c85319abf4937a9 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Tue, 25 Feb 2020 12:39:58 +0000
Subject: [PATCH 1/6] lxd/storage/drivers/driver/zfs/volumes: Create block
volumes with volmode=none
Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
lxd/storage/drivers/driver_zfs_volumes.go | 33 +++++++++++++++--------
1 file changed, 22 insertions(+), 11 deletions(-)
diff --git a/lxd/storage/drivers/driver_zfs_volumes.go
b/lxd/storage/drivers/driver_zfs_volumes.go
index d8caa92c54..759ce07b7b 100644
--- a/lxd/storage/drivers/driver_zfs_volumes.go
+++ b/lxd/storage/drivers/driver_zfs_volumes.go
@@ -81,19 +81,19 @@ func (d *zfs) CreateVolume(vol Volume, filler
*VolumeFiller, op *operations.Oper
return err
}
+ // Use volmode=none so volume is invisible until mounted.
+ opts := []string{"volmode=none"}
+
loopPath := loopFilePath(d.name)
if d.config["source"] == loopPath {
// Create the volume dataset with sync disabled (to
avoid kernel lockups when using a disk based pool).
- err = d.createVolume(d.dataset(vol, false), sizeBytes,
"sync=disabled")
- if err != nil {
- return err
- }
- } else {
- // Create the volume dataset.
- err = d.createVolume(d.dataset(vol, false), sizeBytes)
- if err != nil {
- return err
- }
+ opts = append(opts, "sync=disabled")
+ }
+
+ // Create the volume dataset.
+ err = d.createVolume(d.dataset(vol, false), sizeBytes, opts...)
+ if err != nil {
+ return err
}
}
@@ -458,8 +458,19 @@ func (d *zfs) CreateVolumeFromCopy(vol Volume, srcVol
Volume, copySnapshots bool
}
}
} else {
+ args := []string{
+ "clone",
+ srcSnapshot,
+ d.dataset(vol, false),
+ }
+
+ if vol.contentType == ContentTypeBlock {
+ // Use volmode=none so volume is invisible until
mounted.
+ args = append(args, "-o", "volmode=none")
+ }
+
// Clone the snapshot.
- _, err := shared.RunCommand("zfs", "clone", srcSnapshot,
d.dataset(vol, false))
+ _, err := shared.RunCommand("zfs", args...)
if err != nil {
return err
}
From a5267929ac564d60959b36ee0be0c0f1840d8d2c Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Tue, 25 Feb 2020 15:54:19 +0000
Subject: [PATCH 2/6] lxd/storage/drivers/driver/zfs/volumes: Use MountTask
with CreateVolume
Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
lxd/storage/drivers/driver_zfs_volumes.go | 60 ++++++++++-------------
1 file changed, 26 insertions(+), 34 deletions(-)
diff --git a/lxd/storage/drivers/driver_zfs_volumes.go
b/lxd/storage/drivers/driver_zfs_volumes.go
index 759ce07b7b..597f827ae0 100644
--- a/lxd/storage/drivers/driver_zfs_volumes.go
+++ b/lxd/storage/drivers/driver_zfs_volumes.go
@@ -108,49 +108,41 @@ func (d *zfs) CreateVolume(vol Volume, filler
*VolumeFiller, op *operations.Oper
revert.Add(func() { d.DeleteVolume(fsVol, op) })
}
- // Mount the dataset.
- _, err := d.MountVolume(vol, op)
- if err != nil {
- return err
- }
+ err := vol.MountTask(func(mountPath string, op *operations.Operation)
error {
+ // Run the volume filler function if supplied.
+ if filler != nil && filler.Fill != nil {
+ if vol.contentType == ContentTypeFS {
+ // Run the filler.
+ err := filler.Fill(vol.MountPath(), "")
+ if err != nil {
+ return err
+ }
+ } else {
+ // Get the device path.
+ devPath, err := d.GetVolumeDiskPath(vol)
+ if err != nil {
+ return err
+ }
- if vol.contentType == ContentTypeFS {
- // Set the permissions.
- err = vol.EnsureMountPath()
- if err != nil {
- d.UnmountVolume(vol, op)
- return err
+ // Run the filler.
+ err = filler.Fill(vol.MountPath(), devPath)
+ if err != nil {
+ return err
+ }
+ }
}
- }
- // Run the volume filler function if supplied.
- if filler != nil && filler.Fill != nil {
if vol.contentType == ContentTypeFS {
- // Run the filler.
- err = filler.Fill(vol.MountPath(), "")
- if err != nil {
- d.UnmountVolume(vol, op)
- return err
- }
- } else {
- // Get the device path.
- devPath, err := d.GetVolumeDiskPath(vol)
+ // Run EnsureMountPath again after mounting and filling
to ensure the mount directory has
+ // the correct permissions set.
+ err := vol.EnsureMountPath()
if err != nil {
- d.UnmountVolume(vol, op)
- return err
- }
-
- // Run the filler.
- err = filler.Fill(vol.MountPath(), devPath)
- if err != nil {
- d.UnmountVolume(vol, op)
return err
}
}
- }
- // Unmount the volume.
- _, err = d.UnmountVolume(vol, op)
+ return nil
+ }, op)
if err != nil {
return err
}
From 8fbaf86ffd759825515827f2d0f4c15249cce91e Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Tue, 25 Feb 2020 15:55:35 +0000
Subject: [PATCH 3/6] lxd/storage/drivers/zfs/volumes: Makes MountVolume and
UnmountVolume more thorough in detecting mounts
Improves debug logging too.
Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
lxd/storage/drivers/driver_zfs_volumes.go | 77 ++++++++++++++---------
1 file changed, 46 insertions(+), 31 deletions(-)
diff --git a/lxd/storage/drivers/driver_zfs_volumes.go
b/lxd/storage/drivers/driver_zfs_volumes.go
index 597f827ae0..f5c25be0e4 100644
--- a/lxd/storage/drivers/driver_zfs_volumes.go
+++ b/lxd/storage/drivers/driver_zfs_volumes.go
@@ -20,6 +20,7 @@ import (
"github.com/lxc/lxd/lxd/revert"
"github.com/lxc/lxd/shared"
"github.com/lxc/lxd/shared/ioprogress"
+ log "github.com/lxc/lxd/shared/log15"
"github.com/lxc/lxd/shared/units"
)
@@ -841,55 +842,64 @@ func (d *zfs) GetVolumeDiskPath(vol Volume) (string,
error) {
// MountVolume simulates mounting a volume.
func (d *zfs) MountVolume(vol Volume, op *operations.Operation) (bool, error) {
- // For VMs, also mount the filesystem dataset.
- if vol.IsVMBlock() {
- fsVol := vol.NewVMBlockFilesystemVolume()
- _, err := d.MountVolume(fsVol, op)
+ var err error
+ mountPath := vol.MountPath()
+ dataset := d.dataset(vol, false)
+
+ // Check if already mounted.
+ if vol.contentType == ContentTypeFS && !shared.IsMountPoint(mountPath) {
+ // Mount the dataset.
+ _, err = shared.RunCommand("zfs", "mount", dataset)
if err != nil {
return false, err
}
+
+ d.logger.Debug("Mounted ZFS dataset", log.Ctx{"dev": dataset,
"path": mountPath})
+ return true, nil
}
- // For block devices, we make them appear.
+ var ourMountBlock, ourMountFs bool
+
if vol.contentType == ContentTypeBlock {
+ // Check if already active.
current, err := d.getDatasetProperty(d.dataset(vol, false),
"volmode")
- if err != nil {
- return false, err
- }
+ if current != "dev" {
+ // Activate.
+ err = d.setDatasetProperties(d.dataset(vol, false),
"volmode=dev")
+ if err != nil {
+ return false, err
+ }
- // Check if already active.
- if current == "dev" {
- return false, nil
+ // Wait half a second to give udev a chance to kick in.
+ time.Sleep(500 * time.Millisecond)
+
+ d.logger.Debug("Activated ZFS volume", log.Ctx{"dev":
dataset})
+ ourMountBlock = true
}
+ }
- // Activate.
- err = d.setDatasetProperties(d.dataset(vol, false),
"volmode=dev")
+ // For block devices, we make them appear.
+ if vol.IsVMBlock() {
+ // For VMs, also mount the filesystem dataset.
+ fsVol := vol.NewVMBlockFilesystemVolume()
+ ourMountFs, err = d.MountVolume(fsVol, op)
if err != nil {
return false, err
}
-
- // Wait half a second to give udev a chance to kick in.
- time.Sleep(500 * time.Millisecond)
-
- return true, nil
- }
-
- // Check if not already mounted.
- if shared.IsMountPoint(vol.MountPath()) {
- return false, nil
}
- // Mount the dataset.
- _, err := shared.RunCommand("zfs", "mount", d.dataset(vol, false))
- if err != nil {
- return false, err
+ // If we 'mounted' either block or filesystem volumes, this was our
mount.
+ if ourMountFs || ourMountBlock {
+ return true, nil
}
- return true, nil
+ return false, nil
}
// UnmountVolume simulates unmounting a volume.
func (d *zfs) UnmountVolume(vol Volume, op *operations.Operation) (bool,
error) {
+ dataset := d.dataset(vol, false)
+
// For VMs, also mount the filesystem dataset.
if vol.IsVMBlock() {
fsVol := vol.NewVMBlockFilesystemVolume()
@@ -901,25 +911,30 @@ func (d *zfs) UnmountVolume(vol Volume, op
*operations.Operation) (bool, error)
// For block devices, we make them disappear.
if vol.contentType == ContentTypeBlock {
- err := d.setDatasetProperties(d.dataset(vol, false),
"volmode=none")
+ err := d.setDatasetProperties(dataset, "volmode=none")
if err != nil {
return false, err
}
+ d.logger.Debug("Deactivated ZFS volume", log.Ctx{"dev":
dataset})
+
return false, nil
}
// Check if still mounted.
- if !shared.IsMountPoint(vol.MountPath()) {
+ mountPath := vol.MountPath()
+ if !shared.IsMountPoint(mountPath) {
return false, nil
}
// Mount the dataset.
- _, err := shared.RunCommand("zfs", "unmount", d.dataset(vol, false))
+ _, err := shared.RunCommand("zfs", "unmount", dataset)
if err != nil {
return false, err
}
+ d.logger.Debug("Unmounted ZFS dataset", log.Ctx{"dev": dataset, "path":
mountPath})
+
return true, nil
}
From 04f7290a66d9a8fe07eb7847019c3c807c81767f Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Tue, 25 Feb 2020 15:56:42 +0000
Subject: [PATCH 4/6] lxd/storage/drivers/driver/lvm/volumes: Always ensure
mount path after mount in CreateVolume
Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
lxd/storage/drivers/driver_lvm_volumes.go | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/lxd/storage/drivers/driver_lvm_volumes.go
b/lxd/storage/drivers/driver_lvm_volumes.go
index 016d163e73..4e6ff272fb 100644
--- a/lxd/storage/drivers/driver_lvm_volumes.go
+++ b/lxd/storage/drivers/driver_lvm_volumes.go
@@ -50,8 +50,8 @@ func (d *lvm) CreateVolume(vol Volume, filler *VolumeFiller,
op *operations.Oper
revert.Add(func() { d.DeleteVolume(fsVol, op) })
}
- if filler != nil && filler.Fill != nil {
- err = vol.MountTask(func(mountPath string, op
*operations.Operation) error {
+ err = vol.MountTask(func(mountPath string, op *operations.Operation)
error {
+ if filler != nil && filler.Fill != nil {
if vol.contentType == ContentTypeFS {
d.logger.Debug("Running filler function",
log.Ctx{"path": volPath})
err = filler.Fill(mountPath, "")
@@ -72,19 +72,21 @@ func (d *lvm) CreateVolume(vol Volume, filler
*VolumeFiller, op *operations.Oper
return err
}
}
+ }
- // Run EnsureMountPath again after mounting to ensure
the mount directory has the correct
- // permissions set.
+ if vol.contentType == ContentTypeFS {
+ // Run EnsureMountPath again after mounting and filling
to ensure the mount directory has
+ // the correct permissions set.
err = vol.EnsureMountPath()
if err != nil {
return err
}
-
- return nil
- }, op)
- if err != nil {
- return err
}
+
+ return nil
+ }, op)
+ if err != nil {
+ return err
}
revert.Success()
From 2309c22b3a27205f721e715fc04354eff8f62840 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Tue, 25 Feb 2020 14:56:18 +0000
Subject: [PATCH 5/6] lxd/storage/drivers/utils: Adds moveGPTAltHeader function
Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
lxd/storage/drivers/utils.go | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/lxd/storage/drivers/utils.go b/lxd/storage/drivers/utils.go
index 0a8a3aaa91..e783818541 100644
--- a/lxd/storage/drivers/utils.go
+++ b/lxd/storage/drivers/utils.go
@@ -5,8 +5,10 @@ import (
"io"
"io/ioutil"
"os"
+ "os/exec"
"path/filepath"
"strings"
+ "syscall"
"time"
"github.com/pkg/errors"
@@ -573,3 +575,26 @@ func copyDevice(inputPath, outputPath string) error {
func loopFilePath(poolName string) string {
return filepath.Join(shared.VarPath("disks"), fmt.Sprintf("%s.img",
poolName))
}
+
+var errNonGPTDisk = fmt.Errorf("Non-GPT disk")
+
+// moveGPTAltHeader moves the GPT alternative header to the end of the disk
device supplied.
+// If the device supplied is not detected as not being a GPT disk then no
action is taken and nil is returned.
+func moveGPTAltHeader(devPath string) error {
+ _, err := shared.RunCommand("sgdisk", "--move-second-header", devPath)
+ runErr, ok := err.(shared.RunError)
+ if ok {
+ exitError, ok := runErr.Err.(*exec.ExitError)
+ if ok {
+ waitStatus := exitError.Sys().(syscall.WaitStatus)
+
+ // sgdisk manpage says exit status 3 means:
+ // "Non-GPT disk detected and no -g option, but
operation requires a write action".
+ if waitStatus.ExitStatus() == 3 {
+ return errNonGPTDisk // Non-GPT disk specified.
+ }
+ }
+ }
+
+ return err
+}
From 395e07c7b949619badf05a4eba2412dd96287222 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Tue, 25 Feb 2020 16:26:14 +0000
Subject: [PATCH 6/6] lxd/storage/drivers/driver/zfs/volumes: Uses
moveGPTAltHeader during CreateVolume block filler
Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
lxd/storage/drivers/driver_zfs_volumes.go | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/lxd/storage/drivers/driver_zfs_volumes.go
b/lxd/storage/drivers/driver_zfs_volumes.go
index f5c25be0e4..f332bfe8df 100644
--- a/lxd/storage/drivers/driver_zfs_volumes.go
+++ b/lxd/storage/drivers/driver_zfs_volumes.go
@@ -130,6 +130,14 @@ func (d *zfs) CreateVolume(vol Volume, filler
*VolumeFiller, op *operations.Oper
if err != nil {
return err
}
+
+ // Move the GPT alt header to end of disk if
needed.
+ err = moveGPTAltHeader(devPath)
+ if err == nil {
+ d.logger.Debug("Moved GPT alternative
header to end of disk")
+ } else if err != nil && err != errNonGPTDisk {
+ return err
+ }
}
}
_______________________________________________
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel
