[pve-devel] [PATCH container/qemu-server v6 0/7] fix #3711 and adapt drive detach/remove behavior

[pve-devel] [PATCH container/qemu-server v6 0/7] fix #3711 and adapt drive detach/remove behavior

[Michael Köppl]

This series aims to fix #3711 [0] and streamline the detach/remove
behavior around volumes that are either mounted into a container or
attached to a VM as a hard disk. It is a continuation of a series from
2022 [1], but makes the following changes:

Changes v5 -> v6:
- Fix links in cover letter
- Use Originally-by instead of Co-authored-by
- Add documentation for the second patch regarding the use of
  $mp->{volume} instead of $conf->{$opt}
- Note that no functional changes are intended for the the second patch

Changes v4 -> v5:
- Always ignore errors that originate from a removed storage and
  continue with destruction of a container or removal of a volume, 
  instead of adding an option to ignore these errors.
- Remove web UI checkbox
- Remove formatting patch
- Additionally allow removing a mount point with a removed storage
  from a running container (previously hotplug removal was not possible)
- Fix style nits from v4 review
- Print warnings for any errors that occur instead of ignoring them
- Add explicit check if storage still exists when destroying a container
  to differentiate between that case and other error cases (which should
  still fail)

The first change also means that this patch series should be held back
until PVE 9. In the original patch series the option to ignore these
errors was made to avoid breaking existing behavior. After some off-list
discussion it seems more reasonable to avoid an additional option and
instead allow users to remove containers with mount points even if the
underlying storage of the mount point was removed.

Where at least some of the implementation was taken from the previous
series, the patch was marked to be Originally-by by the original author.

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=3711
[1] https://lore.proxmox.com/pve-devel/20221125144008.2988072-1-s.hrdlicka@proxmox.com/t/#

pve-container:

Michael Köppl (4):
  fix #3711: lxc: print warning if storage for mounted volume does not
    exist anymore
  config: apply_pending: get unused volid through parse_volume()
  fix #3711: lxc: allow removing unused mp if storage no longer exists
  add linked clone check when destroying container

 src/PVE/LXC.pm        | 24 +++++++++++++++++++++++-
 src/PVE/LXC/Config.pm | 25 ++++++++++++++++++++-----
 2 files changed, 43 insertions(+), 6 deletions(-)


qemu-server:

Michael Köppl (3):
  adapt linked clone check to not die if an error occurs during check
  print warning for PVE::Storage::path errors instead of failing
  mark volumes pending detach as unused if storage was removed

 PVE/QemuServer.pm | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)


Summary over all repositories:
  3 files changed, 54 insertions(+), 11 deletions(-)

-- 
Generated by git-murpp 0.8.0


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

[pve-devel] [PATCH container v6 1/4] fix #3711: lxc: print warning if storage for mounted volume does not exist anymore

[Michael Köppl]

An explicit check for the existence of the storage is added to print a
warning and continue with the removal of the container without deleting
the mount point in case the storage does not exist anymore. For other
errors, the function should still die.

Originally-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/PVE/LXC.pm | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 2b9f0cf..6a1ce92 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -953,7 +953,17 @@ sub destroy_lxc_container {
 	return if $volids->{$volume};
 	$volids->{$volume} = 1;
 
-	delete_mountpoint_volume($storage_cfg, $vmid, $volume);
+	# explicitly check if storage still exists to avoid failing during
+	# deletion of the mountpoint volume. instead, only a warning is
+	# printed and destroying the container continues.
+	my ($storeid) = PVE::Storage::parse_volume_id($volume);
+	eval { PVE::Storage::storage_config($storage_cfg, $storeid) };
+	my $err = $@;
+	PVE::RESTEnvironment::log_warn("failed to delete $volume, $err") if $err;
+
+	if (!$err) {
+	    delete_mountpoint_volume($storage_cfg, $vmid, $volume);
+	}
     };
     PVE::LXC::Config->foreach_volume_full($conf, {include_unused => 1}, $remove_volume);
 
-- 
2.39.5



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

[pve-devel] [PATCH container v6 2/4] config: apply_pending: get unused volid through parse_volume()

[Michael Köppl]

The volume ID returned by parse_volume() is used here as opposed to
$conf->{$opt} used for regular mount points because $conf->{$opt} is not
necessarily a valid volume ID in this case. This distinction is
important because is_volume_in_use does internally not consider
'unused*' a valid volume key and does not check it. parse_volume knows
about 'unused*' and returns a valid volume ID.

No functional changes intended.

Originally-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/PVE/LXC/Config.pm | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index 0740e8c..a7c1023 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -1557,15 +1557,20 @@ sub vmconfig_apply_pending {
     foreach my $opt (sort keys %$pending_delete_hash) {
 	next if $selection && !$selection->{$opt};
 	eval {
+	    my $mp = $class->parse_volume($opt, $conf->{$opt});
+
 	    if ($opt =~ m/^mp(\d+)$/) {
-		my $mp = $class->parse_volume($opt, $conf->{$opt});
 		if ($mp->{type} eq 'volume') {
 		    $class->add_unused_volume($conf, $mp->{volume})
 			if !$class->is_volume_in_use($conf, $conf->{$opt}, 1, 1);
 		}
 	    } elsif ($opt =~ m/^unused(\d+)$/) {
+		# $mp->{volume} is used for is_volume_in_use() because parse_volume()
+		# knows about 'unused*' and will return a valid volume ID whereas
+		# $conf->{$opt} is not guaranteed to contain a valid volume ID in this
+		# case.
 		PVE::LXC::delete_mountpoint_volume($storecfg, $vmid, $conf->{$opt})
-		    if !$class->is_volume_in_use($conf, $conf->{$opt}, 1, 1);
+		    if !$class->is_volume_in_use($conf, $mp->{volume}, 1, 1);
 	    } elsif ($opt =~ m/^net(\d+)$/) {
 		if ($have_sdn) {
 		    my $net = $class->parse_lxc_network($conf->{$opt});
-- 
2.39.5



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

[pve-devel] [PATCH container v6 3/4] fix #3711: lxc: allow removing unused mp if storage no longer exists

[Michael Köppl]

Detaching a mount point with a removed underlying storage causes it to
be labeled as an unused disk. With this change, removing an unused disk
with a removed underlying storage causes it to be removed from the
configuration instead of failing. Deleting the mount point is skipped in
this case to avoid failing and to allow removing the unused volume from
the CT configuration and to achieve behavior consistent with destroying
a CT.

Originally-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/PVE/LXC/Config.pm | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index a7c1023..8c96691 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -1470,11 +1470,17 @@ sub vmconfig_hotplug_pending {
     foreach my $opt (sort keys %$pending_delete_hash) {
 	next if $selection && !$selection->{$opt};
 	eval {
+	    my $mp = $class->parse_volume($opt, $conf->{$opt});
+	    my ($storeid, undef) = PVE::Storage::parse_volume_id($mp->{volume});
+
 	    if ($LXC_FASTPLUG_OPTIONS->{$opt}) {
 		# pass
-	    } elsif ($opt =~ m/^unused(\d+)$/) {
+	    } elsif (
+		$opt =~ m/^unused(\d+)$/
+		&& PVE::Storage::storage_config($storecfg, $storeid, 1)
+	    ) {
 		PVE::LXC::delete_mountpoint_volume($storecfg, $vmid, $conf->{$opt})
-		    if !$class->is_volume_in_use($conf, $conf->{$opt}, 1, 1);
+		    if !$class->is_volume_in_use($conf, $mp->{volume}, 1, 1);
 	    } elsif ($opt eq 'swap') {
 		$hotplug_memory->(undef, 0);
 	    } elsif ($opt eq 'cpulimit') {
@@ -1558,13 +1564,17 @@ sub vmconfig_apply_pending {
 	next if $selection && !$selection->{$opt};
 	eval {
 	    my $mp = $class->parse_volume($opt, $conf->{$opt});
+	    my ($storeid, undef) = PVE::Storage::parse_volume_id($mp->{volume});
 
 	    if ($opt =~ m/^mp(\d+)$/) {
 		if ($mp->{type} eq 'volume') {
 		    $class->add_unused_volume($conf, $mp->{volume})
 			if !$class->is_volume_in_use($conf, $conf->{$opt}, 1, 1);
 		}
-	    } elsif ($opt =~ m/^unused(\d+)$/) {
+	    } elsif (
+		$opt =~ m/^unused(\d+)$/
+		&& PVE::Storage::storage_config($storecfg, $storeid, 1)
+	    ) {
 		# $mp->{volume} is used for is_volume_in_use() because parse_volume()
 		# knows about 'unused*' and will return a valid volume ID whereas
 		# $conf->{$opt} is not guaranteed to contain a valid volume ID in this
-- 
2.39.5



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

[pve-devel] [PATCH container v6 4/4] add linked clone check when destroying container

[Michael Köppl]

This behavior matches the behavior already implemented for VMs and
prevents partial storage deletion if a container template has a linked
clone. In such cases, the destruction of the container template will
now fail, informing the user that the base volume is still in use by
the linked clone.

Originally-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/PVE/LXC.pm | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 6a1ce92..7bc566a 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -945,6 +945,18 @@ sub destroy_lxc_container {
     my ($storage_cfg, $vmid, $conf, $replacement_conf, $purge_unreferenced) = @_;
 
     my $volids = {};
+
+    if ($conf->{template}) {
+	PVE::LXC::Config->foreach_volume_full($conf, {include_unused => 1}, sub {
+	   my ($ms, $mountpoint) = @_;
+	   my $volid = $mountpoint->{volume};
+	   return if !$volid || PVE::LXC::Config->classify_mountpoint($volid) ne 'volume';
+	   my $result = eval { PVE::Storage::volume_is_base_and_used($storage_cfg, $volid) };
+	   die "failed to delete volume '$volid': $@\n" if $@;
+	   die "base volume '$volid' is still in use by linked clone\n" if $result;
+	});
+    }
+
     my $remove_volume = sub {
 	my ($ms, $mountpoint) = @_;
 
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server v6 1/3] adapt linked clone check to not die if an error occurs during check

[Michael Köppl]

Align error handling behavior when checking for linked clones with the
rest of destroy_vm's error handling approach. Unlike the LXC destroy
implementation, this version continues execution when encountering
errors during the check, since:

1. The same validation occurs later in the process
2. This prevents potential crashes
3. The VM removal will still be blocked if the volume has linked clones

Align error handling behavior when checking for linked clones with the
rest of destroy_vm's error handling approach. Unlike the LXC implementation,
this version continues execution when encountering errors during the check,
since:

Co-authored-by: Stefan Hrdlicka
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 PVE/QemuServer.pm | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 577959a4..e02bf7d4 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2116,9 +2116,10 @@ sub destroy_vm {
 		my $volid = $drive->{file};
 		return if !$volid || $volid =~ m|^/|;
 
-		die "base volume '$volid' is still in use by linked cloned\n"
-		    if PVE::Storage::volume_is_base_and_used($storecfg, $volid);
-
+		# errors that occur during this check are not propagated since the
+		# same check is done again in vdisk_free
+		my $result = eval { PVE::Storage::volume_is_base_and_used($storecfg, $volid) };
+		die "base volume '$volid' is still in use by linked cloned\n" if $result;
 	});
     }
 
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server v6 2/3] print warning for PVE::Storage::path errors instead of failing

[Michael Köppl]

Co-authored-by: Stefan Hrdlicka
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 PVE/QemuServer.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index e02bf7d4..68bbf4ce 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2132,7 +2132,8 @@ sub destroy_vm {
 	return if !$volid || $volid =~ m|^/|;
 	return if $volids->{$volid};
 
-	my ($path, $owner) = PVE::Storage::path($storecfg, $volid);
+	my ($path, $owner) = eval { PVE::Storage::path($storecfg, $volid) };
+	log_warn("failed to get path and owner of volume '$volid': $@\n") if $@;
 	return if !$path || !$owner || ($owner != $vmid);
 
 	$volids->{$volid} = 1;
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server v6 3/3] mark volumes pending detach as unused if storage was removed

[Michael Köppl]

If the storage backing the volume was removed, the volume will be marked
as unused when pending actions are processed (e.g. volume was detached).
Previously, the volume would simply disappear from the config.

Co-authored-by: Stefan Hrdlicka
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 PVE/QemuServer.pm | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 68bbf4ce..d47fa51c 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -1838,7 +1838,11 @@ sub vmconfig_register_unused_drive {
 	delete $conf->{'special-sections'}->{cloudinit};
     } elsif (!drive_is_cdrom($drive)) {
 	my $volid = $drive->{file};
-	if (vm_is_volid_owner($storecfg, $vmid, $volid)) {
+	my ($storeid, undef) = PVE::Storage::parse_volume_id($volid);
+	if (
+	    !PVE::Storage::storage_config($storecfg, $storeid, 1)
+	    || vm_is_volid_owner($storecfg, $vmid, $volid)
+	) {
 	    PVE::QemuConfig->add_unused_volume($conf, $volid, $vmid);
 	}
     }
-- 
2.39.5



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

Re: [pve-devel] [PATCH container v6 1/4] fix #3711: lxc: print warning if storage for mounted volume does not exist anymore

[Fiona Ebner]

Am 20.05.25 um 11:08 schrieb Michael Köppl:
> An explicit check for the existence of the storage is added to print a
> warning and continue with the removal of the container without deleting
> the mount point in case the storage does not exist anymore. For other
> errors, the function should still die.
> 
> Originally-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>

Nit: Ideally, you also describe the changes to the original patch here.
For how this is usually done, see e.g.
https://git.proxmox.com/?p=pve-container.git;a=commit;h=ee81952f4fc8faf01ed4eda5b8962d1a82d5425d

> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
>  src/PVE/LXC.pm | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index 2b9f0cf..6a1ce92 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -953,7 +953,17 @@ sub destroy_lxc_container {
>  	return if $volids->{$volume};
>  	$volids->{$volume} = 1;
>  
> -	delete_mountpoint_volume($storage_cfg, $vmid, $volume);
> +	# explicitly check if storage still exists to avoid failing during
> +	# deletion of the mountpoint volume. instead, only a warning is
> +	# printed and destroying the container continues.
> +	my ($storeid) = PVE::Storage::parse_volume_id($volume);
> +	eval { PVE::Storage::storage_config($storage_cfg, $storeid) };
> +	my $err = $@;
> +	PVE::RESTEnvironment::log_warn("failed to delete $volume, $err") if $err;
> +
> +	if (!$err) {
> +	    delete_mountpoint_volume($storage_cfg, $vmid, $volume);
> +	}

Can we instead just surround the delete_mountpoint_volume() call itself
with an eval + printing warning? That also catches other situations
where deletion fails and is simpler.

>      };
>      PVE::LXC::Config->foreach_volume_full($conf, {include_unused => 1}, $remove_volume);
>  



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

Re: [pve-devel] [PATCH container v6 2/4] config: apply_pending: get unused volid through parse_volume()

[Fiona Ebner]

Am 20.05.25 um 11:08 schrieb Michael Köppl:
> The volume ID returned by parse_volume() is used here as opposed to
> $conf->{$opt} used for regular mount points because $conf->{$opt} is not
> necessarily a valid volume ID in this case. This distinction is
> important because is_volume_in_use does internally not consider
> 'unused*' a valid volume key and does not check it. parse_volume knows
> about 'unused*' and returns a valid volume ID.

The key is not passed to is_volume_in_use(), just the value. The
rationale is imprecise in that regard.

Hmm, what can the value for 'unusedN' be other than a valid volume ID?
Do you have an example? I'd rather avoid adding entries where it is not
a volume ID to 'unusedN' in the first place.

> No functional changes intended.

Yes there is, as you want to fix handling 'unusedN' entries. Why make
the change otherwise?

> 
> Originally-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
>  src/PVE/LXC/Config.pm | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
> index 0740e8c..a7c1023 100644
> --- a/src/PVE/LXC/Config.pm
> +++ b/src/PVE/LXC/Config.pm
> @@ -1557,15 +1557,20 @@ sub vmconfig_apply_pending {
>      foreach my $opt (sort keys %$pending_delete_hash) {
>  	next if $selection && !$selection->{$opt};
>  	eval {
> +	    my $mp = $class->parse_volume($opt, $conf->{$opt});

You don't know that $opt is for a volume at this stage, it can be
anything, e.g. 'net0'.

> +
>  	    if ($opt =~ m/^mp(\d+)$/) {
> -		my $mp = $class->parse_volume($opt, $conf->{$opt});
>  		if ($mp->{type} eq 'volume') {
>  		    $class->add_unused_volume($conf, $mp->{volume})
>  			if !$class->is_volume_in_use($conf, $conf->{$opt}, 1, 1);
>  		}
>  	    } elsif ($opt =~ m/^unused(\d+)$/) {
> +		# $mp->{volume} is used for is_volume_in_use() because parse_volume()
> +		# knows about 'unused*' and will return a valid volume ID whereas
> +		# $conf->{$opt} is not guaranteed to contain a valid volume ID in this
> +		# case.
>  		PVE::LXC::delete_mountpoint_volume($storecfg, $vmid, $conf->{$opt})
> -		    if !$class->is_volume_in_use($conf, $conf->{$opt}, 1, 1);
> +		    if !$class->is_volume_in_use($conf, $mp->{volume}, 1, 1);
>  	    } elsif ($opt =~ m/^net(\d+)$/) {
>  		if ($have_sdn) {
>  		    my $net = $class->parse_lxc_network($conf->{$opt});



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

Re: [pve-devel] [PATCH container v6 2/4] config: apply_pending: get unused volid through parse_volume()

[Fiona Ebner]

Am 20.05.25 um 15:45 schrieb Fiona Ebner:
> Am 20.05.25 um 11:08 schrieb Michael Köppl:
>> The volume ID returned by parse_volume() is used here as opposed to
>> $conf->{$opt} used for regular mount points because $conf->{$opt} is not
>> necessarily a valid volume ID in this case. This distinction is
>> important because is_volume_in_use does internally not consider
>> 'unused*' a valid volume key and does not check it. parse_volume knows
>> about 'unused*' and returns a valid volume ID.
> 
> The key is not passed to is_volume_in_use(), just the value. The
> rationale is imprecise in that regard.
> 
> Hmm, what can the value for 'unusedN' be other than a valid volume ID?
> Do you have an example? I'd rather avoid adding entries where it is not
> a volume ID to 'unusedN' in the first place.

Looking at the next patch, do you just mean it can be a volume ID whose
storage does not exist anymore?


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

Re: [pve-devel] [PATCH container v6 3/4] fix #3711: lxc: allow removing unused mp if storage no longer exists

[Fiona Ebner]

Am 20.05.25 um 11:08 schrieb Michael Köppl:
> Detaching a mount point with a removed underlying storage causes it to
> be labeled as an unused disk. With this change, removing an unused disk
> with a removed underlying storage causes it to be removed from the
> configuration instead of failing. Deleting the mount point is skipped in
> this case to avoid failing and to allow removing the unused volume from
> the CT configuration and to achieve behavior consistent with destroying
> a CT.
> 
> Originally-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
>  src/PVE/LXC/Config.pm | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
> index a7c1023..8c96691 100644
> --- a/src/PVE/LXC/Config.pm
> +++ b/src/PVE/LXC/Config.pm
> @@ -1470,11 +1470,17 @@ sub vmconfig_hotplug_pending {
>      foreach my $opt (sort keys %$pending_delete_hash) {
>  	next if $selection && !$selection->{$opt};
>  	eval {
> +	    my $mp = $class->parse_volume($opt, $conf->{$opt});

You don't know that $opt is for a volume at this stage, it can be
anything, e.g. 'net0'.

> +	    my ($storeid, undef) = PVE::Storage::parse_volume_id($mp->{volume});
> +
>  	    if ($LXC_FASTPLUG_OPTIONS->{$opt}) {
>  		# pass
> -	    } elsif ($opt =~ m/^unused(\d+)$/) {
> +	    } elsif (
> +		$opt =~ m/^unused(\d+)$/
> +		&& PVE::Storage::storage_config($storecfg, $storeid, 1)
> +	    ) {
>  		PVE::LXC::delete_mountpoint_volume($storecfg, $vmid, $conf->{$opt})
> -		    if !$class->is_volume_in_use($conf, $conf->{$opt}, 1, 1);
> +		    if !$class->is_volume_in_use($conf, $mp->{volume}, 1, 1);
>  	    } elsif ($opt eq 'swap') {
>  		$hotplug_memory->(undef, 0);
>  	    } elsif ($opt eq 'cpulimit') {
> @@ -1558,13 +1564,17 @@ sub vmconfig_apply_pending {
>  	next if $selection && !$selection->{$opt};
>  	eval {
>  	    my $mp = $class->parse_volume($opt, $conf->{$opt});
> +	    my ($storeid, undef) = PVE::Storage::parse_volume_id($mp->{volume});
>  
>  	    if ($opt =~ m/^mp(\d+)$/) {
>  		if ($mp->{type} eq 'volume') {
>  		    $class->add_unused_volume($conf, $mp->{volume})
>  			if !$class->is_volume_in_use($conf, $conf->{$opt}, 1, 1);
>  		}
> -	    } elsif ($opt =~ m/^unused(\d+)$/) {
> +	    } elsif (
> +		$opt =~ m/^unused(\d+)$/
> +		&& PVE::Storage::storage_config($storecfg, $storeid, 1)
> +	    ) {
>  		# $mp->{volume} is used for is_volume_in_use() because parse_volume()
>  		# knows about 'unused*' and will return a valid volume ID whereas
>  		# $conf->{$opt} is not guaranteed to contain a valid volume ID in this

Can we put the parsing/check in delete_mountpoint_volume() itself
instead? And maybe print an informational message if the storage didn't
exist anymore. That would also cover the caller in patch 1/4, although
we still might want to use the eval+print there for other kinds of errors.


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

Re: [pve-devel] [PATCH container v6 4/4] add linked clone check when destroying container

[Fiona Ebner]

Nit: destroy template: add early check for linked clones of volumes

Am 20.05.25 um 11:08 schrieb Michael Köppl:
> This behavior matches the behavior already implemented for VMs and
> prevents partial storage deletion if a container template has a linked
> clone. In such cases, the destruction of the container template will
> now fail, informing the user that the base volume is still in use by
> the linked clone.
> 
> Originally-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>

With the comments addressed, consider this:

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

> ---
>  src/PVE/LXC.pm | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index 6a1ce92..7bc566a 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -945,6 +945,18 @@ sub destroy_lxc_container {
>      my ($storage_cfg, $vmid, $conf, $replacement_conf, $purge_unreferenced) = @_;
>  
>      my $volids = {};
> +
> +    if ($conf->{template}) {
> +	PVE::LXC::Config->foreach_volume_full($conf, {include_unused => 1}, sub {

Style nit: usually, spaces are used after/before the curly braces for
such in-line hashes like: { include_unused => 1 }

> +	   my ($ms, $mountpoint) = @_;
> +	   my $volid = $mountpoint->{volume};
> +	   return if !$volid || PVE::LXC::Config->classify_mountpoint($volid) ne 'volume';
> +	   my $result = eval { PVE::Storage::volume_is_base_and_used($storage_cfg, $volid) };
> +	   die "failed to delete volume '$volid': $@\n" if $@;

s/failed to delete/failed to check for linked clones of/

No need for "\n" at the end, we expect $@ to already contain it.

> +	   die "base volume '$volid' is still in use by linked clone\n" if $result;
> +	});
> +    }
> +
>      my $remove_volume = sub {
>  	my ($ms, $mountpoint) = @_;
>  



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

Re: [pve-devel] [PATCH qemu-server v6 1/3] adapt linked clone check to not die if an error occurs during check

[Fiona Ebner]

Am 20.05.25 um 11:08 schrieb Michael Köppl:
> Align error handling behavior when checking for linked clones with the
> rest of destroy_vm's error handling approach. Unlike the LXC destroy
> implementation, this version continues execution when encountering
> errors during the check, since:
> 
> 1. The same validation occurs later in the process
> 2. This prevents potential crashes

What kind of crashes?

> 3. The VM removal will still be blocked if the volume has linked clones
> 
> Align error handling behavior when checking for linked clones with the
> rest of destroy_vm's error handling approach. Unlike the LXC implementation,
> this version continues execution when encountering errors during the check,
> since:

This paragraph is duplicated.

> 
> Co-authored-by: Stefan Hrdlicka

The qemu-server patches still need to have the trailer adapted.

> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
>  PVE/QemuServer.pm | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 577959a4..e02bf7d4 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -2116,9 +2116,10 @@ sub destroy_vm {
>  		my $volid = $drive->{file};
>  		return if !$volid || $volid =~ m|^/|;
>  
> -		die "base volume '$volid' is still in use by linked cloned\n"
> -		    if PVE::Storage::volume_is_base_and_used($storecfg, $volid);
> -
> +		# errors that occur during this check are not propagated since the
> +		# same check is done again in vdisk_free

For containers, you do propagate the error. Please adapt the code there
too to make it consistent. I'd also add a warning or print an
informational message about the failure, rather than quietly ignoring it.

> +		my $result = eval { PVE::Storage::volume_is_base_and_used($storecfg, $volid) };
> +		die "base volume '$volid' is still in use by linked cloned\n" if $result;
>  	});
>      }
>  



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

Re: [pve-devel] [PATCH qemu-server v6 2/3] print warning for PVE::Storage::path errors instead of failing

[Fiona Ebner]

I'd prefer a commit title like "fix #3711: make removal of VM possible
if storage doesn't exit anymore" and add a brief rationale to the commit
message. The current title does not convey any information about the "why".

Am 20.05.25 um 11:08 schrieb Michael Köppl:
> Co-authored-by: Stefan Hrdlicka

As already, mentioned, needs to be adapted.

> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>

With the comments addressed
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>

> ---
>  PVE/QemuServer.pm | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index e02bf7d4..68bbf4ce 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -2132,7 +2132,8 @@ sub destroy_vm {
>  	return if !$volid || $volid =~ m|^/|;
>  	return if $volids->{$volid};
>  
> -	my ($path, $owner) = PVE::Storage::path($storecfg, $volid);
> +	my ($path, $owner) = eval { PVE::Storage::path($storecfg, $volid) };
> +	log_warn("failed to get path and owner of volume '$volid': $@\n") if $@;

No need for the trailing "\n". In fact, it will be duplicate if $@
already contains it. Semi-related note: log_warn() will add a trailing
newline if the passed-in message doesn't contain it.

>  	return if !$path || !$owner || ($owner != $vmid);
>  
>  	$volids->{$volid} = 1;

There are two warnings further below in the function when vdisk_free()
fails, please adapt those to use log_warn() too for consistency and
better visibility for users. Should be its own patch.


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

Re: [pve-devel] [PATCH qemu-server v6 3/3] mark volumes pending detach as unused if storage was removed

[Fiona Ebner]

Am 20.05.25 um 11:08 schrieb Michael Köppl:
> If the storage backing the volume was removed, the volume will be marked
> as unused when pending actions are processed (e.g. volume was detached).
> Previously, the volume would simply disappear from the config.

Why? If the storage doesn't exist, the volume doesn't exist (from PVE's
perspective). What is the benefit of adding it as unused?

We could improve the current behavior by printing an informational
message for volumes that are not added as unused and a warning message
if the ownership couldn't be determined. I.e. warn if path() in
vm_is_volid_owner() fails, rather than quietly ignoring a failure there.

> Co-authored-by: Stefan Hrdlicka
> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
>  PVE/QemuServer.pm | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 68bbf4ce..d47fa51c 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -1838,7 +1838,11 @@ sub vmconfig_register_unused_drive {
>  	delete $conf->{'special-sections'}->{cloudinit};
>      } elsif (!drive_is_cdrom($drive)) {
>  	my $volid = $drive->{file};
> -	if (vm_is_volid_owner($storecfg, $vmid, $volid)) {
> +	my ($storeid, undef) = PVE::Storage::parse_volume_id($volid);
> +	if (
> +	    !PVE::Storage::storage_config($storecfg, $storeid, 1)
> +	    || vm_is_volid_owner($storecfg, $vmid, $volid)
> +	) {
>  	    PVE::QemuConfig->add_unused_volume($conf, $volid, $vmid);
>  	}
>      }



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

Re: [pve-devel] [PATCH container v6 1/4] fix #3711: lxc: print warning if storage for mounted volume does not exist anymore

[Thomas Lamprecht]

Am 20.05.25 um 15:33 schrieb Fiona Ebner:
> Am 20.05.25 um 11:08 schrieb Michael Köppl:
>> An explicit check for the existence of the storage is added to print a
>> warning and continue with the removal of the container without deleting
>> the mount point in case the storage does not exist anymore. For other
>> errors, the function should still die.
>>
>> Originally-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
> 
> Nit: Ideally, you also describe the changes to the original patch here.
> For how this is usually done, see e.g.
> https://git.proxmox.com/?p=pve-container.git;a=commit;h=ee81952f4fc8faf01ed4eda5b8962d1a82d5425d
> 
>> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
>> ---
>>  src/PVE/LXC.pm | 12 +++++++++++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
>> index 2b9f0cf..6a1ce92 100644
>> --- a/src/PVE/LXC.pm
>> +++ b/src/PVE/LXC.pm
>> @@ -953,7 +953,17 @@ sub destroy_lxc_container {
>>  	return if $volids->{$volume};
>>  	$volids->{$volume} = 1;
>>  
>> -	delete_mountpoint_volume($storage_cfg, $vmid, $volume);
>> +	# explicitly check if storage still exists to avoid failing during
>> +	# deletion of the mountpoint volume. instead, only a warning is
>> +	# printed and destroying the container continues.

nit: in general: use the full width for comments (at least 80cc, 100c is totally fine
too).

But most of the comment reads as description for what happens, which is relatively
obvious from reading the code here, e.g. a "log_warn" call isn't exactly complex, but
rather telling on its own already.

While comments can really help, they mostly do when they state the things that are
not already obvious from reading the code in the local context already, like, e.g.,
"distant" effects or assumptions, or if it really is complex and there is not a
good way to simplify the code.

If one want's a comment here it probably would be enough to write something like:

# storages can be removed while volumes still exist, check that for better UX.


Note that your single comment is not a problem on it's own, but having a lot of
these makes reading code harder and as especially long comments describing the
code itself, and not the reasons, why's and other such rationale, tend to get
outdated fast, making it even more confusing to read.

That doesn't mean no comments though, but if, then please lets favor succinct
comments focusing on background, one or maybe two lines should be enough for most
code that benefits from having one. Exceptions naturally exist, e.g., if you write
some crypto code (please don't, as that's even hard to get right for field experts
with dozens of years of good experience, but just as example) then having more
comment than code would even be expected.

>> +	my ($storeid) = PVE::Storage::parse_volume_id($volume);
>> +	eval { PVE::Storage::storage_config($storage_cfg, $storeid) };
>> +	my $err = $@;
>> +	PVE::RESTEnvironment::log_warn("failed to delete $volume, $err") if $err;
>> +
>> +	if (!$err) {
>> +	    delete_mountpoint_volume($storage_cfg, $vmid, $volume);
>> +	}
> 
> Can we instead just surround the delete_mountpoint_volume() call itself
> with an eval + printing warning? That also catches other situations
> where deletion fails and is simpler.

Yeah, that would be nicer. As in 

eval {
    foo();
    bar();
}
# ... error handling

The bar method won't be called if foo dies.

> 
>>      };
>>      PVE::LXC::Config->foreach_volume_full($conf, {include_unused => 1}, $remove_volume);
>>  


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

Re: [pve-devel] [PATCH container v6 1/4] fix #3711: lxc: print warning if storage for mounted volume does not exist anymore

[Thomas Lamprecht]

Am 20.05.25 um 11:08 schrieb Michael Köppl:
> An explicit check for the existence of the storage is added to print a
> warning and continue with the removal of the container without deleting
> the mount point in case the storage does not exist anymore. For other
> errors, the function should still die.

And something else, while this is IMO fine to do for now as stop-gap, it
might be good to also improve the removal of storage entries, like e.g.,
if the storage is still available then check if there are any volumes
on the storage that our code recognizes, and if, prompt the user if they
still want to remove the storage entry, as that would make those volumes
unusable. Ideally we would join that info with them being actually used,
like a disk in a VM.
The same holds for jobs being defined that use that storage, like backup or
replication (well that always works on guest disks, so that would be
covered already).

For clarity: I do not suggest adding some "auto prune everything", that's
a tad to dangerous in the storage config panel/api. Albeit some multi
select + delete (and other operations that make sense, like move to other
storage I guess) could be nice, that probably should go into the storage
content UI/API.

Again, nothing wrong with this here, one can still always remove storage
entries directly, and improving UX on such edge cases is always nice.


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

Re: [pve-devel] [PATCH qemu-server v6 3/3] mark volumes pending detach as unused if storage was removed

[Michael Köppl]

On 5/21/25 13:16, Fiona Ebner wrote:
> Am 20.05.25 um 11:08 schrieb Michael Köppl:
>> If the storage backing the volume was removed, the volume will be marked
>> as unused when pending actions are processed (e.g. volume was detached).
>> Previously, the volume would simply disappear from the config.
> 
> Why? If the storage doesn't exist, the volume doesn't exist (from PVE's
> perspective). What is the benefit of adding it as unused?
> 
> We could improve the current behavior by printing an informational
> message for volumes that are not added as unused and a warning message
> if the ownership couldn't be determined. I.e. warn if path() in
> vm_is_volid_owner() fails, rather than quietly ignoring a failure there.

Thanks for having a look at this and for your suggestion! The idea
behind this patch was to avoid the volume simply disappearing (from the
user's point of view). That way, the volume would still be there. In
theory, if the storage "re-appeared", it could even be mounted again as
it's not gone from the configuration. But it doesn't really make that
much sense, I agree.

I'll add the warnings you suggested for a v7.

> 
>> Co-authored-by: Stefan Hrdlicka
>> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
>> ---
>>  PVE/QemuServer.pm | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index 68bbf4ce..d47fa51c 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -1838,7 +1838,11 @@ sub vmconfig_register_unused_drive {
>>  	delete $conf->{'special-sections'}->{cloudinit};
>>      } elsif (!drive_is_cdrom($drive)) {
>>  	my $volid = $drive->{file};
>> -	if (vm_is_volid_owner($storecfg, $vmid, $volid)) {
>> +	my ($storeid, undef) = PVE::Storage::parse_volume_id($volid);
>> +	if (
>> +	    !PVE::Storage::storage_config($storecfg, $storeid, 1)
>> +	    || vm_is_volid_owner($storecfg, $vmid, $volid)
>> +	) {
>>  	    PVE::QemuConfig->add_unused_volume($conf, $volid, $vmid);
>>  	}
>>      }
> 



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

Re: [pve-devel] [PATCH container v6 1/4] fix #3711: lxc: print warning if storage for mounted volume does not exist anymore

[Michael Köppl via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 4837 bytes --]

From: "Michael Köppl" <m.koeppl@proxmox.com>
To: Thomas Lamprecht <t.lamprecht@proxmox.com>, Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH container v6 1/4] fix #3711: lxc: print warning if storage for mounted volume does not exist anymore
Date: Tue, 27 May 2025 09:29:47 +0200
Message-ID: <746f07c0-6038-44cd-9388-8445a8400069@proxmox.com>

On 5/22/25 08:17, Thomas Lamprecht wrote:
> Am 20.05.25 um 11:08 schrieb Michael Köppl:
>> An explicit check for the existence of the storage is added to print a
>> warning and continue with the removal of the container without deleting
>> the mount point in case the storage does not exist anymore. For other
>> errors, the function should still die.
> 
> And something else, while this is IMO fine to do for now as stop-gap, it
> might be good to also improve the removal of storage entries, like e.g.,
> if the storage is still available then check if there are any volumes
> on the storage that our code recognizes, and if, prompt the user if they
> still want to remove the storage entry, as that would make those volumes
> unusable. Ideally we would join that info with them being actually used,
> like a disk in a VM.
> The same holds for jobs being defined that use that storage, like backup or
> replication (well that always works on guest disks, so that would be
> covered already).
> 
> For clarity: I do not suggest adding some "auto prune everything", that's
> a tad to dangerous in the storage config panel/api. Albeit some multi
> select + delete (and other operations that make sense, like move to other
> storage I guess) could be nice, that probably should go into the storage
> content UI/API.
> 
> Again, nothing wrong with this here, one can still always remove storage
> entries directly, and improving UX on such edge cases is always nice.

Thanks for having a look and for the feedback. I'd like to implement
some sort of multi-select solution as an extension to this, but as a
separate series. I have it on my TODO list, though, as the solution
implemented by this patch at least gives some warning, but it's
certainly not optimal.



[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [pve-devel] [PATCH container v6 1/4] fix #3711: lxc: print warning if storage for mounted volume does not exist anymore

[Michael Köppl]

On 5/22/25 08:08, Thomas Lamprecht wrote:
> nit: in general: use the full width for comments (at least 80cc, 100c is totally fine
> too).
> 
> But most of the comment reads as description for what happens, which is relatively
> obvious from reading the code here, e.g. a "log_warn" call isn't exactly complex, but
> rather telling on its own already.
> 
> While comments can really help, they mostly do when they state the things that are
> not already obvious from reading the code in the local context already, like, e.g.,
> "distant" effects or assumptions, or if it really is complex and there is not a
> good way to simplify the code.
> 
> If one want's a comment here it probably would be enough to write something like:
> 
> # storages can be removed while volumes still exist, check that for better UX.
> 
> 
> Note that your single comment is not a problem on it's own, but having a lot of
> these makes reading code harder and as especially long comments describing the
> code itself, and not the reasons, why's and other such rationale, tend to get
> outdated fast, making it even more confusing to read.
> 
> That doesn't mean no comments though, but if, then please lets favor succinct
> comments focusing on background, one or maybe two lines should be enough for most
> code that benefits from having one. Exceptions naturally exist, e.g., if you write
> some crypto code (please don't, as that's even hard to get right for field experts
> with dozens of years of good experience, but just as example) then having more
> comment than code would even be expected.

I opted for wrapping the delete_mountpoint_volume in an eval in this
case, so the comment wasn't necessary anymore, but I'll keep that in
mind. I definitely understand the need for succinct comments. Thanks for
the feedback! Also, I won't be sending patches with crypto code anytime
soon, I promise ;)

> 
>>> +	my ($storeid) = PVE::Storage::parse_volume_id($volume);
>>> +	eval { PVE::Storage::storage_config($storage_cfg, $storeid) };
>>> +	my $err = $@;
>>> +	PVE::RESTEnvironment::log_warn("failed to delete $volume, $err") if $err;
>>> +
>>> +	if (!$err) {
>>> +	    delete_mountpoint_volume($storage_cfg, $vmid, $volume);
>>> +	}
>>
>> Can we instead just surround the delete_mountpoint_volume() call itself
>> with an eval + printing warning? That also catches other situations
>> where deletion fails and is simpler.
> 
> Yeah, that would be nicer. As in 
> 
> eval {
>     foo();
>     bar();
> }
> # ... error handling
> 
> The bar method won't be called if foo dies.
> 
>>
>>>      };
>>>      PVE::LXC::Config->foreach_volume_full($conf, {include_unused => 1}, $remove_volume);
>>>  



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

Re: [pve-devel] [PATCH container v6 3/4] fix #3711: lxc: allow removing unused mp if storage no longer exists

[Michael Köppl]

On 5/20/25 16:03, Fiona Ebner wrote:
>> @@ -1558,13 +1564,17 @@ sub vmconfig_apply_pending {
>>  	next if $selection && !$selection->{$opt};
>>  	eval {
>>  	    my $mp = $class->parse_volume($opt, $conf->{$opt});
>> +	    my ($storeid, undef) = PVE::Storage::parse_volume_id($mp->{volume});
>>  
>>  	    if ($opt =~ m/^mp(\d+)$/) {
>>  		if ($mp->{type} eq 'volume') {
>>  		    $class->add_unused_volume($conf, $mp->{volume})
>>  			if !$class->is_volume_in_use($conf, $conf->{$opt}, 1, 1);
>>  		}
>> -	    } elsif ($opt =~ m/^unused(\d+)$/) {
>> +	    } elsif (
>> +		$opt =~ m/^unused(\d+)$/
>> +		&& PVE::Storage::storage_config($storecfg, $storeid, 1)
>> +	    ) {
>>  		# $mp->{volume} is used for is_volume_in_use() because parse_volume()
>>  		# knows about 'unused*' and will return a valid volume ID whereas
>>  		# $conf->{$opt} is not guaranteed to contain a valid volume ID in this
> 
> Can we put the parsing/check in delete_mountpoint_volume() itself
> instead? And maybe print an informational message if the storage didn't
> exist anymore. That would also cover the caller in patch 1/4, although
> we still might want to use the eval+print there for other kinds of errors.

I think moving the check if the storage exists inside definitely makes
sense here. Thanks for the suggestion. I adapted the
delete_mountpoint_volume() for a v7, but opted to keep the parsing of
the volume ID in vmconfig_apply_pending() and
vmconfig_hotplug_pending(). destroy_lxc_container() uses
foreach_volume_full() to iterate over all its mountpoints, which uses
parse_volume() internally. So the $volume input used there is already
what we want, as opposed to the $conf and $opt arguments used by the
other 2 callers. Moving the parsing into delete_mountpoint_volume()
would require changing other parts of the implementation (to get the
required inputs for all 3 callers) for little benefit. One way to avoid
that would be a signature such as

	   my ($storage_cfg, $vmid, $volid, $conf, $opt) = @_;

and using $conf->{$opt} to call parse_volume() if $volid is undef. But I
think that's not very transparent to the caller.

Also, I don't know if you meant the is_volume_in_use() checks as well,
but I think keeping those where they are makes it very obvious from the
code that the delete only happens if the volume is not in use.

Would love your input on this, maybe I'm just missing something.


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

Re: [pve-devel] [PATCH container v6 3/4] fix #3711: lxc: allow removing unused mp if storage no longer exists

[Fiona Ebner]

Am 27.05.25 um 11:34 schrieb Michael Köppl:
> On 5/20/25 16:03, Fiona Ebner wrote:
>>> @@ -1558,13 +1564,17 @@ sub vmconfig_apply_pending {
>>>  	next if $selection && !$selection->{$opt};
>>>  	eval {
>>>  	    my $mp = $class->parse_volume($opt, $conf->{$opt});
>>> +	    my ($storeid, undef) = PVE::Storage::parse_volume_id($mp->{volume});
>>>  
>>>  	    if ($opt =~ m/^mp(\d+)$/) {
>>>  		if ($mp->{type} eq 'volume') {
>>>  		    $class->add_unused_volume($conf, $mp->{volume})
>>>  			if !$class->is_volume_in_use($conf, $conf->{$opt}, 1, 1);
>>>  		}
>>> -	    } elsif ($opt =~ m/^unused(\d+)$/) {
>>> +	    } elsif (
>>> +		$opt =~ m/^unused(\d+)$/
>>> +		&& PVE::Storage::storage_config($storecfg, $storeid, 1)
>>> +	    ) {
>>>  		# $mp->{volume} is used for is_volume_in_use() because parse_volume()
>>>  		# knows about 'unused*' and will return a valid volume ID whereas
>>>  		# $conf->{$opt} is not guaranteed to contain a valid volume ID in this
>>
>> Can we put the parsing/check in delete_mountpoint_volume() itself
>> instead? And maybe print an informational message if the storage didn't
>> exist anymore. That would also cover the caller in patch 1/4, although
>> we still might want to use the eval+print there for other kinds of errors.
> 
> I think moving the check if the storage exists inside definitely makes
> sense here. Thanks for the suggestion. I adapted the
> delete_mountpoint_volume() for a v7, but opted to keep the parsing of
> the volume ID in vmconfig_apply_pending() and
> vmconfig_hotplug_pending().

As we had discussed off-list, that check should even go in vdisk_free()
itself, not delete_mountpoint_volume(). And regarding the parsing, that
was an oversight on my part because the parameter is confusingly named
$volume, but it's already a volume ID. We already had discussed this too ;)

> destroy_lxc_container() uses
> foreach_volume_full() to iterate over all its mountpoints, which uses
> parse_volume() internally. So the $volume input used there is already

Yes, the $volume variable there is also just confusingly named, as it's
also already a volume ID.

> what we want, as opposed to the $conf and $opt arguments used by the
> other 2 callers. Moving the parsing into delete_mountpoint_volume()
> would require changing other parts of the implementation (to get the
> required inputs for all 3 callers) for little benefit. One way to avoid
> that would be a signature such as
> 
> 	   my ($storage_cfg, $vmid, $volid, $conf, $opt) = @_;
> 
> and using $conf->{$opt} to call parse_volume() if $volid is undef. But I
> think that's not very transparent to the caller.

We don't need to adapt the signature of delete_mountpoint_volume(),
except renaming the argument $volume => $volid would be worthwhile.

> Also, I don't know if you meant the is_volume_in_use() checks as well,
> but I think keeping those where they are makes it very obvious from the
> code that the delete only happens if the volume is not in use.

No, I didn't mean those.


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

[pve-devel] superseded: [PATCH container/qemu-server v6 0/7] fix #3711 and adapt drive detach/remove behavior

[Michael Köppl]

Supersed-by: https://lore.proxmox.com/pve-devel/20250527160140.230174-1 m.koeppl@proxmox.com/T

On 5/20/25 11:08, Michael Köppl wrote:
> This series aims to fix #3711 [0] and streamline the detach/remove
> behavior around volumes that are either mounted into a container or
> attached to a VM as a hard disk. It is a continuation of a series from
> 2022 [1], but makes the following changes:
> 
> Changes v5 -> v6:
> - Fix links in cover letter
> - Use Originally-by instead of Co-authored-by
> - Add documentation for the second patch regarding the use of
>   $mp->{volume} instead of $conf->{$opt}
> - Note that no functional changes are intended for the the second patch
> 
> Changes v4 -> v5:
> - Always ignore errors that originate from a removed storage and
>   continue with destruction of a container or removal of a volume, 
>   instead of adding an option to ignore these errors.
> - Remove web UI checkbox
> - Remove formatting patch
> - Additionally allow removing a mount point with a removed storage
>   from a running container (previously hotplug removal was not possible)
> - Fix style nits from v4 review
> - Print warnings for any errors that occur instead of ignoring them
> - Add explicit check if storage still exists when destroying a container
>   to differentiate between that case and other error cases (which should
>   still fail)
> 
> The first change also means that this patch series should be held back
> until PVE 9. In the original patch series the option to ignore these
> errors was made to avoid breaking existing behavior. After some off-list
> discussion it seems more reasonable to avoid an additional option and
> instead allow users to remove containers with mount points even if the
> underlying storage of the mount point was removed.
> 
> Where at least some of the implementation was taken from the previous
> series, the patch was marked to be Originally-by by the original author.
> 
> [0] https://bugzilla.proxmox.com/show_bug.cgi?id=3711
> [1] https://lore.proxmox.com/pve-devel/20221125144008.2988072-1-s.hrdlicka@proxmox.com/t/#
> 
> pve-container:
> 
> Michael Köppl (4):
>   fix #3711: lxc: print warning if storage for mounted volume does not
>     exist anymore
>   config: apply_pending: get unused volid through parse_volume()
>   fix #3711: lxc: allow removing unused mp if storage no longer exists
>   add linked clone check when destroying container
> 
>  src/PVE/LXC.pm        | 24 +++++++++++++++++++++++-
>  src/PVE/LXC/Config.pm | 25 ++++++++++++++++++++-----
>  2 files changed, 43 insertions(+), 6 deletions(-)
> 
> 
> qemu-server:
> 
> Michael Köppl (3):
>   adapt linked clone check to not die if an error occurs during check
>   print warning for PVE::Storage::path errors instead of failing
>   mark volumes pending detach as unused if storage was removed
> 
>  PVE/QemuServer.pm | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> 
> Summary over all repositories:
>   3 files changed, 54 insertions(+), 11 deletions(-)
> 



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