[PATCH-SERIES storage v2 0/7] lvm plugin: activation/renaming/tag improvements

[PATCH-SERIES storage v2 0/7] lvm plugin: activation/renaming/tag improvements

[Fiona Ebner]

Changes in v2:
* Add more improvements, v1 was only the first two patches.

Fix multiple issues related to volume activation, volume renaming and
LV tags. In particular fix bug #7143 to avoid issues when there are
multiple storages with disks with the same name and #7280 to fix
disk reassign for qcow2. Also fix handling tags when doing disk
reassing for both, raw and qcow2.

storage:

Fiona Ebner (7):
  fix #7280: lvm plugin: rename: activate qcow2 volume to read snapshot
    info
  lvm plugin: rename: improve error message
  lvm plugin: rename: update tags when renaming
  lvm plugin: remove cryptic partial comment
  lvm plugin: activation: avoid extra variable
  fix #7143: ensure qcow2 volume (de)activation is limited to target
    volume chain
  lvm plugin: schema: align description of 'tagged_only' with actual
    behavior

 src/PVE/Storage/LVMPlugin.pm | 88 ++++++++++++++++++++++++++++--------
 1 file changed, 68 insertions(+), 20 deletions(-)


Summary over all repositories:
  1 files changed, 68 insertions(+), 20 deletions(-)

-- 
Generated by git-murpp 0.5.0

[PATCH storage v2 1/7] fix #7280: lvm plugin: rename: activate qcow2 volume to read snapshot info

[Fiona Ebner]

Other callers of volume_snapshot_info() already do this. For other
storages and formats, activation is not necessary to rename it, so the
activation is only added here, rather than in the qemu-server, and, for
consistency, container API endpoints.

There is no need for deactivation after the rename operation. In fact,
in case of reassign to a running target VM with SCSI or VirtIO block,
the renamed disk is even hotplugged, so deactivation would be wrong.

Note that the order of $scfg and $storeid really is switched when
comparing volume_snapshot_info() and activate_volume().

Technically, the check for snapshots would not be needed, as
reassigning a volume referenced in a snapshot is already prohibited by
the guest API endpoints. But additional checks on the storage layer
don't hurt and there might be other users of rename_volume() in the
future.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

No changes in v2.

 src/PVE/Storage/LVMPlugin.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index 32a8339..f9349de 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -1439,6 +1439,7 @@ sub rename_volume {
     ) = $class->parse_volname($source_volname);
 
     if ($format eq 'qcow2') {
+        $class->activate_volume($storeid, $scfg, $source_volname);
         my $snapshots = $class->volume_snapshot_info($scfg, $storeid, $source_volname);
         die "we can't rename volume if external snapshot exists" if $snapshots->{current}->{parent};
     }
-- 
2.47.3

[PATCH storage v2 2/7] lvm plugin: rename: improve error message

[Fiona Ebner]

Avoid the use of 'we', mention the volume in question and add a
newline.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

No changes in v2.

 src/PVE/Storage/LVMPlugin.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index f9349de..8392c89 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -1441,7 +1441,8 @@ sub rename_volume {
     if ($format eq 'qcow2') {
         $class->activate_volume($storeid, $scfg, $source_volname);
         my $snapshots = $class->volume_snapshot_info($scfg, $storeid, $source_volname);
-        die "we can't rename volume if external snapshot exists" if $snapshots->{current}->{parent};
+        die "can't rename volume '$source_volname' - external snapshot exists\n"
+            if $snapshots->{current}->{parent};
     }
 
     $target_volname = $class->find_free_diskname($storeid, $scfg, $target_vmid, $format)
-- 
2.47.3

[PATCH storage v2 3/7] lvm plugin: rename: update tags when renaming

[Fiona Ebner]

The rename_volume() function is used when reassigning a volume to a
different VM. When a volume is created, a tag 'pve-vm-ID' with the VM
ID is added to the volume, and for qcow2, additionally a tag with the
volume name. Replace the tag(s) with one(s) for the new VM ID (and new
volume name for qcow2) after renaming to keep them consistent.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

New in v2.

 src/PVE/Storage/LVMPlugin.pm | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index 8392c89..cd4d07a 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -1454,6 +1454,18 @@ sub rename_volume {
         if ($lvs->{$vg}->{$target_volname});
 
     lvrename($scfg, $source_volname, $target_volname);
+
+    eval {
+        my $tag_opts =
+            ['--addtag', "pve-vm-${target_vmid}", '--deltag', "pve-vm-${source_vmid}"];
+        if ($format eq 'qcow2') {
+            push $tag_opts->@*, '--addtag', "pve-$target_volname";
+            push $tag_opts->@*, '--deltag', "pve-$source_volname";
+        }
+        run_command(['lvchange', $tag_opts->@*, "${vg}/${target_volname}"]);
+    };
+    warn "unable to update tags for '$target_volname' - $@" if $@;
+
     return "${storeid}:${target_volname}";
 }
 
-- 
2.47.3

[PATCH storage v2 4/7] lvm plugin: remove cryptic partial comment

[Fiona Ebner]

This partial comment was added with commit c8943a8 ("remove non used
parameter exclusive") back in 2015, but there is no explanation what
it means or what the fix me refers to, not even on the mailing list
[0]. Just remove the comment now after 10 years, to avoid confusing
people. If there really is something to fix, then it should resurface
as an actual issue.

There also seems to be a typo, since 'lvmchange' was a pre-LVM2
command to "change attributes of the logical volume manager" [1]
rather than individual LVs and even at the time of commit c8943a8, the
'lvchange' command was used by the activate_volume() method and there
is no further reference to 'lvmchange' in the pve-storage git history.

[0]: https://lists.proxmox.com/pipermail/pve-devel/2015-September/017233.html
[1]: https://gitlab.com/lvmteam/lvm2/-/commit/8ef93c756eecc2036f0c945c80972902f27970ca

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

New in v2.

 src/PVE/Storage/LVMPlugin.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index cd4d07a..7ef148b 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -954,7 +954,7 @@ sub deactivate_storage {
 
 sub activate_volume {
     my ($class, $storeid, $scfg, $volname, $snapname, $cache) = @_;
-    #fix me lvmchange is not provided on
+
     my $path = $class->path($scfg, $volname, $storeid, $snapname);
 
     my $lvm_activate_mode = 'ey';
-- 
2.47.3

[PATCH storage v2 5/7] lvm plugin: activation: avoid extra variable

[Fiona Ebner]

The value for the activation mode has been hardcoded since commit
c8943a8 ("remove non used parameter exclusive"). Inline the value to
avoid the extra variable and slightly improve readability.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

New in v2.

 src/PVE/Storage/LVMPlugin.pm | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index 7ef148b..79a52c5 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -957,15 +957,13 @@ sub activate_volume {
 
     my $path = $class->path($scfg, $volname, $storeid, $snapname);
 
-    my $lvm_activate_mode = 'ey';
-
     #activate volume && all snapshots volumes by tag
     my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $format) =
         $class->parse_volname($volname);
 
     $path = "\@pve-$name" if $format eq 'qcow2';
 
-    my $cmd = ['/sbin/lvchange', "-a$lvm_activate_mode", $path];
+    my $cmd = ['/sbin/lvchange', '-aey', $path];
     run_command($cmd, errmsg => "can't activate LV '$path'");
     $cmd = ['/sbin/lvchange', '--refresh', $path];
     run_command($cmd, errmsg => "can't refresh LV '$path' for activation");
-- 
2.47.3

[PATCH storage v2 6/7] fix #7143: ensure qcow2 volume (de)activation is limited to target volume chain

[Fiona Ebner]

When using tag-based (de)activation for qcow2 volumes, volumes with
the same name (or snapshot volumes based on a volume with the same
name) on other storages are affected too. Including the storage ID in
the tag would be a possibility, but would mean updating the tags after
moving the volume and would break moving volumes or renaming outside
the Proxmox VE stack and would be rather involved to do for already
existing images.

Use a regex with '--select' instead of using tags to select the
volumes in the qcow2 volume chain.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

New in v2.

 src/PVE/Storage/LVMPlugin.pm | 66 ++++++++++++++++++++++++++++--------
 1 file changed, 51 insertions(+), 15 deletions(-)

diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index 79a52c5..b4cb0c5 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -713,7 +713,12 @@ my sub alloc_lvm_image {
     die "not enough free space ($free < $size)\n" if $free < $size;
 
     my $tags = ["pve-vm-$vmid"];
-    #tags all snapshots volumes with the main volume tag for easier activation of the whole group
+    # TODO PVE 10 - stop setting the volume name as a tag? It was initially used for (de)activation,
+    # but there are false positives if there are multiple storages, see bug #7143. Including the
+    # storage ID as a fix would've made moving disks more involved and break manual moving or
+    # renaming. Instead (de)activation switched to using '--select'. The rename_volume() method
+    # needs to be adapted as well.
+    # tags all snapshots volumes with the main volume tag
     push @$tags, "\@pve-$name" if $fmt eq 'qcow2';
     lvcreate($vg, $name, $lvmsize, $tags);
 
@@ -952,21 +957,53 @@ sub deactivate_storage {
     run_command($cmd, errmsg => "can't deactivate VG '$scfg->{vgname}'");
 }
 
+=head3 get_activate_volume_target_opts()
+
+    my ($target_opts, $target_str) =
+        get_activate_volume_target_opts($class, $scfg, $path, $volname);
+
+Returns C<$target_opts>, which is an array reference with parameters for C<lvchange> for selecting
+the volume given by C<($path,$volname)>, or volume chain for C<qcow2>. Also returns a description of
+the target C<$target_str> that can be used for log or error messages.
+
+=cut
+
+my sub get_activate_volume_target_opts {
+    my ($class, $scfg, $path, $volname) = @_;
+
+    my ($name, $format) = ($class->parse_volname($volname))[1,6];
+
+    my ($target_opts, $target_str);
+
+    if ($format eq 'qcow2') {
+        my $vg = $scfg->{vgname};
+        $name =~ s/\.qcow2$//;
+
+        my $filter = "vg_name = $vg";
+        $filter .= "&& lv_name =~ '^(${name}|snap_${name}_.+).qcow2\$'";
+
+        $target_opts = ['--select', $filter];
+        $target_str = "volume chain for LV '$path'";
+    } else {
+        $target_opts = [$path];
+        $target_str = "LV '$path'";
+    }
+
+    return ($target_opts, $target_str);
+}
+
 sub activate_volume {
     my ($class, $storeid, $scfg, $volname, $snapname, $cache) = @_;
 
     my $path = $class->path($scfg, $volname, $storeid, $snapname);
 
-    #activate volume && all snapshots volumes by tag
-    my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $format) =
-        $class->parse_volname($volname);
+    my ($target_opts, $target_str) =
+        get_activate_volume_target_opts($class, $scfg, $path, $volname);
 
-    $path = "\@pve-$name" if $format eq 'qcow2';
-
-    my $cmd = ['/sbin/lvchange', '-aey', $path];
-    run_command($cmd, errmsg => "can't activate LV '$path'");
-    $cmd = ['/sbin/lvchange', '--refresh', $path];
-    run_command($cmd, errmsg => "can't refresh LV '$path' for activation");
+    my $cmd = ['/sbin/lvchange', '-aey', $target_opts->@*];
+    run_command($cmd, errmsg => "can't activate $target_str");
+    $cmd = ['/sbin/lvchange', '--refresh', $target_opts->@*];
+    run_command($cmd, errmsg => "can't refresh $target_str for activation");
 }
 
 sub deactivate_volume {
@@ -975,12 +1012,11 @@ sub deactivate_volume {
     my $path = $class->path($scfg, $volname, $storeid, $snapname);
     return if !-b $path;
 
-    my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $format) =
-        $class->parse_volname($volname);
-    $path = "\@pve-$name" if $format eq 'qcow2';
+    my ($target_opts, $target_str) =
+        get_activate_volume_target_opts($class, $scfg, $path, $volname);
 
-    my $cmd = ['/sbin/lvchange', '-aln', $path];
-    run_command($cmd, errmsg => "can't deactivate LV '$path'");
+    my $cmd = ['/sbin/lvchange', '-aln', $target_opts->@*];
+    run_command($cmd, errmsg => "can't deactivate $target_str");
 }
 
 sub volume_resize {
-- 
2.47.3

[PATCH storage v2 7/7] lvm plugin: schema: align description of 'tagged_only' with actual behavior

[Fiona Ebner]

The only thing that the 'tagged_only' property affects is the
list_images() method and this has been the case since the option was
introduced 10 years ago with commit 7a9dd11 ("add tagged_only option
to LVM storage"). There are no other checks against using such
volumes if their name adheres to the vm-ID-XYZ schema. For example,
they can be accessed via 'pvesm free' and assigned to a VM with
'qm set'. If such checks are added, that should be done as part of a
major release to avoid breaking any existing setup. Document the
actual behavior for now.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

New in v2.

 src/PVE/Storage/LVMPlugin.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index b4cb0c5..ff8c5f4 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -440,7 +440,7 @@ sub properties {
             type => 'string',
         },
         tagged_only => {
-            description => "Only use logical volumes tagged with 'pve-vm-ID'.",
+            description => "Only list logical volumes tagged with 'pve-vm-ID'.",
             type => 'boolean',
         },
     };
-- 
2.47.3