The following pull request was submitted through Github.
It can be accessed and reviewed at: https://github.com/lxc/lxd/pull/7326
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) ===
The `snapshotSubvolume` function used to have a `readonly` argument. However it was not applied when using the `recursion` option.
However fixing the function so that it did apply the `readonly` argument when `recursion` was set to true caused more problems because when snapshotting an instance volume, it is expected that only the root subvolume be marked as readonly and not any subvolumes (so that their write status is preserved in the snapshot in order to correctly backup or migrate the volumes with the same state).
However all subvolumes do need to be set readonly when migrating/exporting.
As such we need to do different things in different scenarios and a 'one size fits all' option here doesn't make sense. So I have removed it in place of more fine grained control where its needed.
From 1b1a4c9abdbe61428b63707a531356ffb7e7cb03 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Thu, 7 May 2020 14:47:22 +0100
Subject: [PATCH 1/3] lxd/storage/drivers/driver/btrfs/utils: Adds
setSubvolumeReadonlyProperty function
Properly detect running in user namespace and ignores setting readonly.
Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
lxd/storage/drivers/driver_btrfs_utils.go | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/lxd/storage/drivers/driver_btrfs_utils.go
b/lxd/storage/drivers/driver_btrfs_utils.go
index 5b30f043c4..88ba8fd6db 100644
--- a/lxd/storage/drivers/driver_btrfs_utils.go
+++ b/lxd/storage/drivers/driver_btrfs_utils.go
@@ -390,3 +390,15 @@ func (d *btrfs) volumeSize(vol Volume) string {
return size
}
+
+// setSubvolumeReadonlyProperty sets the readonly property on the subvolume to
true or false.
+func (d *btrfs) setSubvolumeReadonlyProperty(path string, readonly bool) error
{
+ // Silently ignore requests to set subvolume to readonly if running in
a user namespace as it would mean
+ // we cannot undo this action.
+ if readonly && d.state.OS.RunningInUserNS {
+ return nil
+ }
+
+ _, err := shared.RunCommand("btrfs", "property", "set", "-ts", path,
"ro", fmt.Sprintf("%t", readonly))
+ return err
+}
From 624cfb2690d4c1a7bfb82e457bdd18b5eed3025e Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Thu, 7 May 2020 14:47:07 +0100
Subject: [PATCH 2/3] lxd/storag/drivers/driver/btrfs/volumes: Removes readonly
argument from snapshotSubvolume
Readonly argument was only selectively applied which was confusing in some
situations.
The caller is now expected to set the subvolume(s) they want to readonly in the
specific situation they need.
Updates comments.
Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
lxd/storage/drivers/driver_btrfs_utils.go | 48 +++++++----------------
1 file changed, 15 insertions(+), 33 deletions(-)
diff --git a/lxd/storage/drivers/driver_btrfs_utils.go
b/lxd/storage/drivers/driver_btrfs_utils.go
index 88ba8fd6db..e6efa42902 100644
--- a/lxd/storage/drivers/driver_btrfs_utils.go
+++ b/lxd/storage/drivers/driver_btrfs_utils.go
@@ -86,18 +86,11 @@ func (d *btrfs) getSubvolumes(path string) ([]string,
error) {
return result, nil
}
-func (d *btrfs) snapshotSubvolume(path string, dest string, readonly bool,
recursion bool) error {
+// snapshotSubvolume creates a snapshot of the specified path at the dest
supplied. If recursion is true and
+// sub volumes are found below the path then they are created at the relative
location in dest.
+func (d *btrfs) snapshotSubvolume(path string, dest string, recursion bool)
error {
// Single subvolume deletion.
snapshot := func(path string, dest string) error {
- if readonly && !d.state.OS.RunningInUserNS {
- _, err := shared.RunCommand("btrfs", "subvolume",
"snapshot", "-r", path, dest)
- if err != nil {
- return err
- }
-
- return nil
- }
-
_, err := shared.RunCommand("btrfs", "subvolume", "snapshot",
path, dest)
if err != nil {
return err
@@ -106,43 +99,32 @@ func (d *btrfs) snapshotSubvolume(path string, dest
string, readonly bool, recur
return nil
}
+ // First snapshot the root.
+ err := snapshot(path, dest)
+ if err != nil {
+ return err
+ }
+
// Now snapshot all subvolumes of the root.
if recursion {
// Get the subvolumes list.
- subsubvols, err := d.getSubvolumes(path)
+ subSubVols, err := d.getSubvolumes(path)
if err != nil {
return err
}
- sort.Sort(sort.StringSlice(subsubvols))
+ sort.Sort(sort.StringSlice(subSubVols))
- if len(subsubvols) > 0 && readonly {
- // Creating subvolumes requires the parent to be
writable.
- readonly = false
- }
+ for _, subSubVol := range subSubVols {
+ subSubVolSnapPath := filepath.Join(dest, subSubVol)
- // First snapshot the root.
- err = snapshot(path, dest)
- if err != nil {
- return err
- }
-
- for _, subsubvol := range subsubvols {
// Clear the target for the subvol to use.
- os.Remove(filepath.Join(dest, subsubvol))
+ os.Remove(subSubVolSnapPath)
- err := snapshot(filepath.Join(path, subsubvol),
filepath.Join(dest, subsubvol))
+ err := snapshot(filepath.Join(path, subSubVol),
subSubVolSnapPath)
if err != nil {
return err
}
}
-
- return nil
- }
-
- // Handle standalone volume.
- err := snapshot(path, dest)
- if err != nil {
- return err
}
return nil
From 2f97c85c677d294f9b6d1a3b2b7dccb590102c10 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Thu, 7 May 2020 14:48:13 +0100
Subject: [PATCH 3/3] lxd/storage/drivers/driver/btrfs:
d.setSubvolumeReadonlyProperty and d.snapshotSubvolume usage
Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
lxd/storage/drivers/driver_btrfs_utils.go | 2 +-
lxd/storage/drivers/driver_btrfs_volumes.go | 51 +++++++++++++++++----
2 files changed, 42 insertions(+), 11 deletions(-)
diff --git a/lxd/storage/drivers/driver_btrfs_utils.go
b/lxd/storage/drivers/driver_btrfs_utils.go
index e6efa42902..bf48422b45 100644
--- a/lxd/storage/drivers/driver_btrfs_utils.go
+++ b/lxd/storage/drivers/driver_btrfs_utils.go
@@ -140,7 +140,7 @@ func (d *btrfs) deleteSubvolume(path string, recursion
bool) error {
}
// Attempt to make the subvolume writable.
- shared.RunCommand("btrfs", "property", "set", path, "ro",
"false")
+ d.setSubvolumeReadonlyProperty(path, false)
// Temporarily change ownership & mode to help with nesting.
os.Chmod(path, 0700)
diff --git a/lxd/storage/drivers/driver_btrfs_volumes.go
b/lxd/storage/drivers/driver_btrfs_volumes.go
index 275fc5d97d..f9d00d61e9 100644
--- a/lxd/storage/drivers/driver_btrfs_volumes.go
+++ b/lxd/storage/drivers/driver_btrfs_volumes.go
@@ -94,8 +94,8 @@ func (d *btrfs) CreateVolume(vol Volume, filler
*VolumeFiller, op *operations.Op
// Attempt to mark image read-only.
if vol.volType == VolumeTypeImage {
- _, err = shared.RunCommand("btrfs", "property", "set", volPath,
"ro", "true")
- if err != nil && !d.state.OS.RunningInUserNS {
+ err = d.setSubvolumeReadonlyProperty(volPath, true)
+ if err != nil {
return err
}
}
@@ -229,7 +229,7 @@ func (d *btrfs) CreateVolumeFromBackup(vol Volume,
snapshots []string, srcData i
defer d.deleteSubvolume(filepath.Join(unpackDir, ".backup"), true)
// Re-create the writable subvolume.
- err = d.snapshotSubvolume(filepath.Join(unpackDir, ".backup"),
vol.MountPath(), false, false)
+ err = d.snapshotSubvolume(filepath.Join(unpackDir, ".backup"),
vol.MountPath(), false)
if err != nil {
return nil, nil, err
}
@@ -244,7 +244,7 @@ func (d *btrfs) CreateVolumeFromCopy(vol Volume, srcVol
Volume, copySnapshots bo
defer revert.Fail()
// Recursively copy the main volume.
- err := d.snapshotSubvolume(srcVol.MountPath(), vol.MountPath(), false,
true)
+ err := d.snapshotSubvolume(srcVol.MountPath(), vol.MountPath(), true)
if err != nil {
return err
}
@@ -295,7 +295,12 @@ func (d *btrfs) CreateVolumeFromCopy(vol Volume, srcVol
Volume, copySnapshots bo
srcSnapshot := GetVolumeMountPath(d.name,
srcVol.volType, GetSnapshotVolumeName(srcVol.name, snapName))
dstSnapshot := GetVolumeMountPath(d.name, vol.volType,
GetSnapshotVolumeName(vol.name, snapName))
- err = d.snapshotSubvolume(srcSnapshot, dstSnapshot,
true, false)
+ err = d.snapshotSubvolume(srcSnapshot, dstSnapshot,
false)
+ if err != nil {
+ return err
+ }
+
+ err = d.setSubvolumeReadonlyProperty(dstSnapshot, true)
if err != nil {
return err
}
@@ -651,12 +656,17 @@ func (d *btrfs) MigrateVolume(vol Volume, conn
io.ReadWriteCloser, volSrcArgs *m
// Make read-only snapshot of the subvolume as writable subvolumes
cannot be sent.
migrationSendSnapshot := filepath.Join(tmpVolumesMountPoint,
".migration-send")
- err = d.snapshotSubvolume(vol.MountPath(), migrationSendSnapshot, true,
false)
+ err = d.snapshotSubvolume(vol.MountPath(), migrationSendSnapshot, false)
if err != nil {
return err
}
defer d.deleteSubvolume(migrationSendSnapshot, true)
+ err = d.setSubvolumeReadonlyProperty(migrationSendSnapshot, true)
+ if err != nil {
+ return err
+ }
+
// Setup progress tracking.
var wrapper *ioprogress.ProgressTracker
if volSrcArgs.TrackProgress {
@@ -706,10 +716,16 @@ func (d *btrfs) BackupVolume(vol Volume, tarWriter
*instancewriter.InstanceTarWr
// Create the read-only snapshot.
mountPath := vol.MountPath()
- err = d.snapshotSubvolume(sourcePath, mountPath, true,
true)
+ err = d.snapshotSubvolume(sourcePath, mountPath, true)
if err != nil {
return err
}
+
+ err = d.setSubvolumeReadonlyProperty(mountPath, true)
+ if err != nil {
+ return err
+ }
+
d.logger.Debug("Created read-only backup snapshot",
log.Ctx{"sourcePath": sourcePath, "path": mountPath})
defer d.deleteSubvolume(mountPath, true)
}
@@ -813,12 +829,17 @@ func (d *btrfs) BackupVolume(vol Volume, tarWriter
*instancewriter.InstanceTarWr
// Create the read-only snapshot.
targetVolume := fmt.Sprintf("%s/.backup", tmpInstanceMntPoint)
- err = d.snapshotSubvolume(sourceVolume, targetVolume, true, true)
+ err = d.snapshotSubvolume(sourceVolume, targetVolume, true)
if err != nil {
return err
}
defer d.deleteSubvolume(targetVolume, true)
+ err = d.setSubvolumeReadonlyProperty(targetVolume, true)
+ if err != nil {
+ return err
+ }
+
// Dump the container to a file.
fileName := "container.bin"
if vol.volType == VolumeTypeVM {
@@ -850,7 +871,17 @@ func (d *btrfs) CreateVolumeSnapshot(snapVol Volume, op
*operations.Operation) e
return err
}
- return d.snapshotSubvolume(srcPath, snapPath, true, true)
+ err = d.snapshotSubvolume(srcPath, snapPath, true)
+ if err != nil {
+ return err
+ }
+
+ err = d.setSubvolumeReadonlyProperty(snapPath, true)
+ if err != nil {
+ return err
+ }
+
+ return nil
}
// DeleteVolumeSnapshot removes a snapshot from the storage device. The
volName and snapshotName
@@ -920,7 +951,7 @@ func (d *btrfs) RestoreVolume(vol Volume, snapshotName
string, op *operations.Op
// Restore the snapshot.
source := GetVolumeMountPath(d.name, vol.volType,
GetSnapshotVolumeName(vol.name, snapshotName))
- err = d.snapshotSubvolume(source, vol.MountPath(), false, true)
+ err = d.snapshotSubvolume(source, vol.MountPath(), true)
if err != nil {
return err
}
_______________________________________________
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel
