[pve-devel] [PATCH storage 0/2] fix #4997: lvm: avoid autoactivating (new) LVs after boot

[pve-devel] [PATCH storage 0/2] fix #4997: lvm: avoid autoactivating (new) LVs after boot

[Friedrich Weber]

By default, LVM autoactivates LVs after boot. In a cluster with VM disks on a
shared LVM VG (e.g. on top of iSCSI), this can indirectly cause guest creation
or VM live-migration to fail. See bug #4997 [1] and patch #2 for details.

The goal of this series is to avoid autoactivating LVs after boot. Fabian
suggested to use the "activation skip" flag for LVs. LVs with that flag can
only be activated if the `-K` flag is passed during activation (`-K` is not
passed for autoactivation after boot).

With patch #1, the LVM plugin passes the `-K` flag to activation commands. If
the LV does not have the "activation skip" flag set, this should not have any
effect, so it should be safe to apply this patch in the near future. It does
not yet fix #4997, though.

With patch #2, the LVM plugin sets the "activation skip" flag for newly created
LVs. As this can be considered a breaking change, it may make sense to only
apply it close to the PVE 9 release. If patch #1 has been available in the last
PVE 8 minor release, this should ensure a smooth upgrade even if a cluster is
temporarily mixed between PVE 8.x and 9 (PVE 8.x will be able to activate LVs
created by PVE 9 with "activation skip"). This will fix #4997 for newly created
LVs.

Some points to discuss:

* Fabian and I discussed whether it may be better to pass `-K` and set the
  "activation skip" flag only for LVs on a *shared* LVM storage. But this may
  cause issues for users that incorrectly mark an LVM storage as shared, create a
  bunch of LVs (with "activation skip" flag), then unset the "shared" flag, and
  won't be able to activate LVs afterwards (`lvchange -ay` without `-K` on an LV
  with "activation skip" is a noop). What do you think?

* Even with patch #1 and #2 applied, users can still run into #4997 for LVs
  that were created before #2, so without the "activation skip" flag. So it might
  be good to include a note in the 8->9 release notes and/or a warning in the
  pve8to9 helper and/or ship a script that automatically sets the flag for all
  existing (PVE-owned) LVs.

[1] https://bugzilla.proxmox.com/show_bug.cgi?id=4997

storage:

Friedrich Weber (2):
  lvm: ignore "activation skip" LV flag during LV activation
  fix #4997: lvm: set "activation skip" flag for newly created LVs

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


Summary over all repositories:
  1 files changed, 3 insertions(+), 0 deletions(-)

-- 
Generated by git-murpp 0.5.0

[pve-devel] [PATCH storage 1/2] lvm: ignore "activation skip" LV flag during LV activation

[Friedrich Weber]

LVs with the "activation skip" flag (can be set via `lvchange -ky`)
are only activated if `-K` is passed to the activation command. In the
next major release, we intend to set the "activation skip" flag for
newly created LVs. The goal is to prevent LVs from being
auto-activated after boot, and thus fix bug #4997.

In preparation for this future change, adjust activation commands
issued by the LVM plugin to include the `-K` flag. For LVs without the
"activation skip" flag, passing the `-K` flag during activation has no
effect, so it should be harmless to always provide it.

[1] https://bugzilla.proxmox.com/show_bug.cgi?id=4997

Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
---
 src/PVE/Storage/LVMPlugin.pm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index 4b951e7..66b42de 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -342,7 +342,7 @@ sub lvcreate {
 	$size .= "k"; # default to kilobytes
     }
 
-    my $cmd = ['/sbin/lvcreate', '-aly', '-Wy', '--yes', '--size', $size, '--name', $name];
+    my $cmd = ['/sbin/lvcreate', '-aly', '-K', '-Wy', '--yes', '--size', $size, '--name', $name];
     for my $tag (@$tags) {
 	push @$cmd, '--addtag', $tag;
     }
@@ -422,7 +422,7 @@ sub free_image {
 	print "successfully removed volume $volname ($vg/del-$volname)\n";
     };
 
-    my $cmd = ['/sbin/lvchange', '-aly', "$vg/$volname"];
+    my $cmd = ['/sbin/lvchange', '-aly', '-K', "$vg/$volname"];
     run_command($cmd, errmsg => "can't activate LV '$vg/$volname' to zero-out its data");
     $cmd = ['/sbin/lvchange', '--refresh', "$vg/$volname"];
     run_command($cmd, errmsg => "can't refresh LV '$vg/$volname' to zero-out its data");
@@ -536,7 +536,7 @@ sub activate_volume {
 
     my $lvm_activate_mode = 'ey';
 
-    my $cmd = ['/sbin/lvchange', "-a$lvm_activate_mode", $path];
+    my $cmd = ['/sbin/lvchange', "-a$lvm_activate_mode", '-K', $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.39.2

[pve-devel] [PATCH storage 2/2] fix #4997: lvm: set "activation skip" flag for newly created LVs

[Friedrich Weber]

Activating an LV creates a device-mapper device. In a cluster with a
shared LVM VG (e.g. on top of iSCSI) where an LV is active on nodes 1
and 2, deleting the LV on node 1 will not clean up the device-mapper
device on node 2. If an LV with the same name is recreated later, the
leftover device-mapper device will cause activation of that LV on node
2 to fail with:

> device-mapper: create ioctl on [...] failed: Device or resource busy

By default, LVM autoactivates all discovered LVs after boot, thus
creating device-mapper device for all discovered LVs. As a result,
certain combinations of guest removal (and thus LV removals) and node
reboots can cause guest creation or VM live migration (which both
entail LV activation) to fail with the above error message, see [1].

To avoid this issue in the future, adjust the LVM plugin to create new
LVs with the "activation skip" flag. LVs with that flag are not
activated unless `-K` is passed to the activation command. Consequently,
new LVs will not be autoactivated after boot anymore, and removing LVs
will not leave behind device-mapper devices on other nodes anymore.

The LVM plugin is still able to activate LVs, as it already passes
`-K` to all activation commands.

Note that the flag is only set for newly created LVs, so LVs created
before this patch can still trigger #4997. To avoid this, users can
manually set the "activation skip" flag on existing LVs.

[1] https://bugzilla.proxmox.com/show_bug.cgi?id=4997

Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
---

Notes:
    Should only be applied close to the next major release, see cover
    letter.

 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 66b42de..191e8d3 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -342,7 +342,7 @@ sub lvcreate {
 	$size .= "k"; # default to kilobytes
     }
 
-    my $cmd = ['/sbin/lvcreate', '-aly', '-K', '-Wy', '--yes', '--size', $size, '--name', $name];
+    my $cmd = ['/sbin/lvcreate', '-aly', '-K', '-ky', '-Wy', '--yes', '--size', $size, '--name', $name];
     for my $tag (@$tags) {
 	push @$cmd, '--addtag', $tag;
     }
-- 
2.39.2

Re: [pve-devel] [PATCH storage 0/2] fix #4997: lvm: avoid autoactivating (new) LVs after boot

[Fiona Ebner]

Am 11.01.24 um 16:03 schrieb Friedrich Weber:
> By default, LVM autoactivates LVs after boot. In a cluster with VM disks on a
> shared LVM VG (e.g. on top of iSCSI), this can indirectly cause guest creation
> or VM live-migration to fail. See bug #4997 [1] and patch #2 for details.
> 
> The goal of this series is to avoid autoactivating LVs after boot. Fabian
> suggested to use the "activation skip" flag for LVs. LVs with that flag can
> only be activated if the `-K` flag is passed during activation (`-K` is not
> passed for autoactivation after boot).
> 
> With patch #1, the LVM plugin passes the `-K` flag to activation commands. If
> the LV does not have the "activation skip" flag set, this should not have any
> effect, so it should be safe to apply this patch in the near future. It does
> not yet fix #4997, though.
> 
> With patch #2, the LVM plugin sets the "activation skip" flag for newly created
> LVs. As this can be considered a breaking change, it may make sense to only
> apply it close to the PVE 9 release. If patch #1 has been available in the last
> PVE 8 minor release, this should ensure a smooth upgrade even if a cluster is
> temporarily mixed between PVE 8.x and 9 (PVE 8.x will be able to activate LVs
> created by PVE 9 with "activation skip"). This will fix #4997 for newly created
> LVs.
> 

Yes, patch #2 can cause issues for clusters with mixed minor versions,
so I'd consider this a breaking change.

> Some points to discuss:
> 
> * Fabian and I discussed whether it may be better to pass `-K` and set the
>   "activation skip" flag only for LVs on a *shared* LVM storage. But this may
>   cause issues for users that incorrectly mark an LVM storage as shared, create a
>   bunch of LVs (with "activation skip" flag), then unset the "shared" flag, and
>   won't be able to activate LVs afterwards (`lvchange -ay` without `-K` on an LV
>   with "activation skip" is a noop). What do you think?
> 

Is there a way to prevent auto-activation on boot for LVs on a shared
(PVE-managed) LVM storage? Also a breaking change, because users might
have other LVs on the same storage, but would avoid the need for the
flag. Not against the current approach, just wondering.

Guardrails against issues caused by misconfiguration always warrant a
cost-benefits analysis. What is the cost for also setting the flag for
LVs on non-shared LVM storages? Or logic needs to be correct either way ;)

> * Even with patch #1 and #2 applied, users can still run into #4997 for LVs
>   that were created before #2, so without the "activation skip" flag. So it might
>   be good to include a note in the 8->9 release notes and/or a warning in the
>   pve8to9 helper and/or ship a script that automatically sets the flag for all
>   existing (PVE-owned) LVs.
> 

Sure, mentioning the changed behavior and suggestion/script for making
it consistent make sense.

Re: [pve-devel] [PATCH storage 1/2] lvm: ignore "activation skip" LV flag during LV activation

[Fiona Ebner]

Am 11.01.24 um 16:03 schrieb Friedrich Weber:
> LVs with the "activation skip" flag (can be set via `lvchange -ky`)
> are only activated if `-K` is passed to the activation command. In the
> next major release, we intend to set the "activation skip" flag for
> newly created LVs. The goal is to prevent LVs from being
> auto-activated after boot, and thus fix bug #4997.
> 
> In preparation for this future change, adjust activation commands
> issued by the LVM plugin to include the `-K` flag. For LVs without the
> "activation skip" flag, passing the `-K` flag during activation has no
> effect, so it should be harmless to always provide it.
> 
> [1] https://bugzilla.proxmox.com/show_bug.cgi?id=4997
> 
> Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Signed-off-by: Friedrich Weber <f.weber@proxmox.com>

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

> ---
>  src/PVE/Storage/LVMPlugin.pm | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
> index 4b951e7..66b42de 100644
> --- a/src/PVE/Storage/LVMPlugin.pm
> +++ b/src/PVE/Storage/LVMPlugin.pm
> @@ -342,7 +342,7 @@ sub lvcreate {
>  	$size .= "k"; # default to kilobytes
>      }
>  
> -    my $cmd = ['/sbin/lvcreate', '-aly', '-Wy', '--yes', '--size', $size, '--name', $name];
> +    my $cmd = ['/sbin/lvcreate', '-aly', '-K', '-Wy', '--yes', '--size', $size, '--name', $name];

Style nit: line too long

>      for my $tag (@$tags) {
>  	push @$cmd, '--addtag', $tag;
>      }

Re: [pve-devel] [PATCH storage 2/2] fix #4997: lvm: set "activation skip" flag for newly created LVs

[Fiona Ebner]

Am 11.01.24 um 16:03 schrieb Friedrich Weber:
> Activating an LV creates a device-mapper device. In a cluster with a
> shared LVM VG (e.g. on top of iSCSI) where an LV is active on nodes 1
> and 2, deleting the LV on node 1 will not clean up the device-mapper
> device on node 2. If an LV with the same name is recreated later, the
> leftover device-mapper device will cause activation of that LV on node
> 2 to fail with:
> 
>> device-mapper: create ioctl on [...] failed: Device or resource busy
> 
> By default, LVM autoactivates all discovered LVs after boot, thus
> creating device-mapper device for all discovered LVs. As a result,
> certain combinations of guest removal (and thus LV removals) and node
> reboots can cause guest creation or VM live migration (which both
> entail LV activation) to fail with the above error message, see [1].
> 
> To avoid this issue in the future, adjust the LVM plugin to create new
> LVs with the "activation skip" flag. LVs with that flag are not
> activated unless `-K` is passed to the activation command. Consequently,
> new LVs will not be autoactivated after boot anymore, and removing LVs
> will not leave behind device-mapper devices on other nodes anymore.
> 
> The LVM plugin is still able to activate LVs, as it already passes
> `-K` to all activation commands.
> 
> Note that the flag is only set for newly created LVs, so LVs created
> before this patch can still trigger #4997. To avoid this, users can
> manually set the "activation skip" flag on existing LVs.
> 
> [1] https://bugzilla.proxmox.com/show_bug.cgi?id=4997
> 
> Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Signed-off-by: Friedrich Weber <f.weber@proxmox.com>

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

> ---
> 
> Notes:
>     Should only be applied close to the next major release, see cover
>     letter.
> 
>  src/PVE/Storage/LVMPlugin.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Re: [pve-devel] [PATCH storage 0/2] fix #4997: lvm: avoid autoactivating (new) LVs after boot

[Friedrich Weber]

Thanks for the review!

On 26/01/2024 12:14, Fiona Ebner wrote:
>> Some points to discuss:
>>
>> * Fabian and I discussed whether it may be better to pass `-K` and set the
>>   "activation skip" flag only for LVs on a *shared* LVM storage. But this may
>>   cause issues for users that incorrectly mark an LVM storage as shared, create a
>>   bunch of LVs (with "activation skip" flag), then unset the "shared" flag, and
>>   won't be able to activate LVs afterwards (`lvchange -ay` without `-K` on an LV
>>   with "activation skip" is a noop). What do you think?
>>
> 
> Is there a way to prevent auto-activation on boot for LVs on a shared
> (PVE-managed) LVM storage? Also a breaking change, because users might
> have other LVs on the same storage, but would avoid the need for the
> flag. Not against the current approach, just wondering.

One can also disable autoactivation for a whole VG (i.e., all LVs of
that VG):

	vgchange --setautoactivation n VG

At least in my tests, after setting this no LV in that VG is active
after boot, so this might also solve the problem. I suppose setting this
automatically for existing VGs would be too dangerous (as users might
have other LVs in that VGs). But our LVM storage plugin *could* set this
when creating a new shared VG [1]?

Not sure which option is better, though.

> Guardrails against issues caused by misconfiguration always warrant a
> cost-benefits analysis. What is the cost for also setting the flag for
> LVs on non-shared LVM storages? Or logic needs to be correct either way ;)

AFAICT, setting this LV flag on non-shared LVM storages doesn't have
negative side-effects. I don't think we rely on autoactivation anywhere.
We'd need to take care of passing `-K` for all our `lvchange -ay` calls,
but AFAICT, `lvchange` calls are only done in the LVM/LvmThin plugins in
pve-storage.

[1]
https://git.proxmox.com/?p=pve-storage.git;a=blob;f=src/PVE/Storage/LVMPlugin.pm;h=4b951e7a;hb=8289057e#l94

Re: [pve-devel] [PATCH storage 0/2] fix #4997: lvm: avoid autoactivating (new) LVs after boot

[Fiona Ebner]

Am 31.01.24 um 16:07 schrieb Friedrich Weber:
> Thanks for the review!
> 
> On 26/01/2024 12:14, Fiona Ebner wrote:
>>> Some points to discuss:
>>>
>>> * Fabian and I discussed whether it may be better to pass `-K` and set the
>>>   "activation skip" flag only for LVs on a *shared* LVM storage. But this may
>>>   cause issues for users that incorrectly mark an LVM storage as shared, create a
>>>   bunch of LVs (with "activation skip" flag), then unset the "shared" flag, and
>>>   won't be able to activate LVs afterwards (`lvchange -ay` without `-K` on an LV
>>>   with "activation skip" is a noop). What do you think?
>>>
>>
>> Is there a way to prevent auto-activation on boot for LVs on a shared
>> (PVE-managed) LVM storage? Also a breaking change, because users might
>> have other LVs on the same storage, but would avoid the need for the
>> flag. Not against the current approach, just wondering.
> 
> One can also disable autoactivation for a whole VG (i.e., all LVs of
> that VG):
> 
> 	vgchange --setautoactivation n VG
> 
> At least in my tests, after setting this no LV in that VG is active
> after boot, so this might also solve the problem. I suppose setting this
> automatically for existing VGs would be too dangerous (as users might
> have other LVs in that VGs). But our LVM storage plugin *could* set this
> when creating a new shared VG [1]?
> 
> Not sure which option is better, though.
> 

Do you still need the -K flag to activate volumes in such a VG? If yes,
nothing is gained compared to the more fine-grained "setting it on
individual LVs". If no, we could save that. OTOH, users might want to
use existing shared VGs and then they would need to apply this setting
themselves.

>> Guardrails against issues caused by misconfiguration always warrant a
>> cost-benefits analysis. What is the cost for also setting the flag for
>> LVs on non-shared LVM storages? Or logic needs to be correct either way ;)
> 
> AFAICT, setting this LV flag on non-shared LVM storages doesn't have
> negative side-effects. I don't think we rely on autoactivation anywhere.
> We'd need to take care of passing `-K` for all our `lvchange -ay` calls,
> but AFAICT, `lvchange` calls are only done in the LVM/LvmThin plugins in
> pve-storage.
> 

We need to have -K for activations, no matter if we only set the
activationskip flag for shared or all. That's not an additional cost.