[pve-devel] [PATCH storage] lvm thin plugin: do not combine activation change and property change

[pve-devel] [PATCH storage] lvm thin plugin: do not combine activation change and property change

[Fiona Ebner]

As reported in the community forum [0], there currently is a warning
from LVM when converting to a base image on an LVM-thin storage:

> WARNING: Combining activation change with other commands is not advised.

From a comment in the LVM source code:

> Unfortunately, lvchange has previously allowed changing an LV
> property and changing LV activation in a single command.  This was
> not a good idea because the behavior/results are hard to predict and
> not possible to sensibly describe.  It's also unnecessary.  So, this
> is here for the sake of compatibility.
>
> This is extremely ugly; activation should always be done separately.
> This is not the full-featured lvchange capability, just the basic
> (the advanced activate options are not provided.)
>
> FIXME: wrap this in a config setting that we can disable by default
> to phase this out?

While it's not clear there's an actual issue in the specific use case
here, just follow what LVM recommends for future-proofing.

[0]: https://forum.proxmox.com/threads/165279/

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/Storage/LvmThinPlugin.pm | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/PVE/Storage/LvmThinPlugin.pm b/src/PVE/Storage/LvmThinPlugin.pm
index 49a4dcb..8f44f95 100644
--- a/src/PVE/Storage/LvmThinPlugin.pm
+++ b/src/PVE/Storage/LvmThinPlugin.pm
@@ -312,8 +312,13 @@ sub create_base {
     my $cmd = ['/sbin/lvrename', $vg, $volname, $newname];
     run_command($cmd, errmsg => "lvrename '$vg/$volname' => '$vg/$newname' error");
 
-    # set inactive, read-only and activationskip flags
-    $cmd = ['/sbin/lvchange', '-an', '-pr', '-ky', "$vg/$newname"];
+    # set read-only and activationskip flags
+    $cmd = ['/sbin/lvchange', '-pr', '-ky', "$vg/$newname"];
+    eval { run_command($cmd); };
+    warn $@ if $@;
+
+    # LVM warns when changing properties and activation at the same time, so inactivate separately
+    $cmd = ['/sbin/lvchange', '-an', "$vg/$newname"];
     eval { run_command($cmd); };
     warn $@ if $@;
 
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] applied: [PATCH storage] lvm thin plugin: do not combine activation change and property change

[Thomas Lamprecht]

On Tue, 22 Apr 2025 15:33:14 +0200, Fiona Ebner wrote:
> As reported in the community forum [0], there currently is a warning
> from LVM when converting to a base image on an LVM-thin storage:
> 
> > WARNING: Combining activation change with other commands is not advised.
> 
> >From a comment in the LVM source code:
> 
> [...]

Applied, thanks!

[1/1] lvm thin plugin: do not combine activation change and property change
      commit: 7b41368fc3c2ab1ce5b94a8ae62a38b295e6361e


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel