[PATCH container/qemu-server/storage v9 00/10] fix #3711 and adapt drive detach/remove behavior

[PATCH container/qemu-server/storage v9 00/10] 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 also adds warnings in case a
volume's underlying storage does not exist anymore. It is a
continuation of a series from 2022 [1], but makes the following changes:

Changes v8 -> v9:
- Drop the parse_volname() warning patch in pve-storage. The removed
  storage case should now be handled by the new pending-change checks
  and vdisk_free
- Pass the noerr flag to storage_config() in vdisk_free() so the warning
  branch is actually reached
- Add patch to allow pending mount point changes for LXC if the storage
  is gone, matching VM behavior. Thanks for pointing that out in v8,
  @Lukas!

Changes v7 -> v8:
- Rebased on master

Changes v6 -> v7:
- Added descriptions of changes made to original patches where
  applicable
- Move check if storage exists to vdisk_free() and parse_volname() in
  pve-storage, print warnings
- Remove explicit storage exist check in destroy_lxc_container, wrap in
  eval instead
- Move parse_volume() calls to where input is known to be volume
- Add patch that renames $volume to $volid in destroy_lxc_container()
  and delete_mountpoint_volume()
- Rewrite commit message of pve-container 3/4 (2/4 in v6) such that it's clear this
  is about future-proofing
- Add FIXME note regarding is_volume_in_use() check
- Add warning to destroy_vm() if volume is still used by linked clone
- Adapt commit message of qemu-server 2/4
- Add patch to use log_warn in destroy_vm() instead of warn
- Rewrite patch in in qemu-server to print warnings of storage does not
  exist instead of marking volume without underlying storage as unused
  (vmconfig_register_unused_drive())

Thanks to Fiona for the comprehensive feedback on v6.

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)

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/


pve-storage:

Michael Köppl (1):
  fix #3711: vdisk_free: print warning if storage does not exist

 src/PVE/Storage.pm | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)


pve-container:

Michael Köppl (5):
  fix #3711: warn about storage errors during mountpoint delete
  destroy_lxc, delete_mp_volume: rename $volume to $volid
  config: ensure valid volid through parse_volume()
  allow pending mount point changes if storage is gone
  add linked clone check when destroying container template

 src/PVE/LXC.pm        | 38 ++++++++++++++++++++++++++++---------
 src/PVE/LXC/Config.pm | 44 +++++++++++++++++++++++++++++++++++--------
 2 files changed, 65 insertions(+), 17 deletions(-)


qemu-server:

Michael Köppl (4):
  adapt linked clone check to not die if an error occurs during check
  fix #3711: make removal of VM possible if store does not exist anymore
  destroy_vm: use log_warn for vdisk_free errors for consistency
  display warnings for storage errors or if storage no longer exists

 src/PVE/QemuServer.pm | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)


Summary over all repositories:
  4 files changed, 87 insertions(+), 25 deletions(-)

-- 
Generated by murpp 0.11.0

[PATCH storage v9 01/10] fix #3711: vdisk_free: print warning if storage does not exist

[Michael Köppl]

Removing a guest or one of its volumes should not fail when the volume's
backing storage does not exist in the configuration. Instead of dying,
warn and return so the removal can proceed. From PVE's point of view,
the volume is gone anyway. Pass the noerr flag to storage_config() so
this case is actually reached.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/PVE/Storage.pm | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index 64ea9dad..55462ff6 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -1187,7 +1187,12 @@ sub vdisk_free {
     my ($cfg, $volid) = @_;
 
     my ($storeid, $volname) = parse_volume_id($volid);
-    my $scfg = storage_config($cfg, $storeid);
+    my $scfg = storage_config($cfg, $storeid, 1);
+    if (!$scfg) {
+        log_warn("storage '$storeid' does not exist, volume '$volid' could not be freed");
+        return;
+    }
+
     my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
 
     activate_storage($cfg, $storeid);
-- 
2.47.3

[PATCH container v9 02/10] fix #3711: warn about storage errors during mountpoint delete

[Michael Köppl]

Errors during deletion of a mountpoint volume should not stop users from
destroying a container. Instead of failing, a warning is printed and the
destruction of the container continues.

Originally-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
 [ MK: remove ignore-storage-errors param ]
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/PVE/LXC.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index a9e9be9a..aa5ff315 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -1032,7 +1032,8 @@ sub destroy_lxc_container {
         return if $volids->{$volume};
         $volids->{$volume} = 1;
 
-        delete_mountpoint_volume($storage_cfg, $vmid, $volume);
+        eval { delete_mountpoint_volume($storage_cfg, $vmid, $volume); };
+        PVE::RESTEnvironment::log_warn("failed to delete mountpoint volume $volume: $@") if $@;
     };
     PVE::LXC::Config->foreach_volume_full($conf, { include_unused => 1 }, $remove_volume);
 
-- 
2.47.3

[PATCH container v9 03/10] destroy_lxc, delete_mp_volume: rename $volume to $volid

[Michael Köppl]

$volid states more clearly that it's a volume ID, avoiding confusion
about the values these variables hold.

No functional change intended.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/PVE/LXC.pm | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index aa5ff315..e76da29f 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -1007,16 +1007,16 @@ sub get_primary_ips {
 }
 
 sub delete_mountpoint_volume {
-    my ($storage_cfg, $vmid, $volume) = @_;
+    my ($storage_cfg, $vmid, $volid) = @_;
 
-    return if PVE::LXC::Config->classify_mountpoint($volume) ne 'volume';
+    return if PVE::LXC::Config->classify_mountpoint($volid) ne 'volume';
 
-    my ($vtype, $name, $owner) = PVE::Storage::parse_volname($storage_cfg, $volume);
+    my ($vtype, $name, $owner) = PVE::Storage::parse_volname($storage_cfg, $volid);
 
     if ($vmid == $owner) {
-        PVE::Storage::vdisk_free($storage_cfg, $volume);
+        PVE::Storage::vdisk_free($storage_cfg, $volid);
     } else {
-        warn "ignore deletion of '$volume', CT $vmid isn't the owner!\n";
+        warn "ignore deletion of '$volid', CT $vmid isn't the owner!\n";
     }
 }
 
@@ -1027,13 +1027,13 @@ sub destroy_lxc_container {
     my $remove_volume = sub {
         my ($ms, $mountpoint) = @_;
 
-        my $volume = $mountpoint->{volume};
+        my $volid = $mountpoint->{volume};
 
-        return if $volids->{$volume};
-        $volids->{$volume} = 1;
+        return if $volids->{$volid};
+        $volids->{$volid} = 1;
 
-        eval { delete_mountpoint_volume($storage_cfg, $vmid, $volume); };
-        PVE::RESTEnvironment::log_warn("failed to delete mountpoint volume $volume: $@") if $@;
+        eval { delete_mountpoint_volume($storage_cfg, $vmid, $volid); };
+        PVE::RESTEnvironment::log_warn("failed to delete mountpoint volume $volid: $@") if $@;
     };
     PVE::LXC::Config->foreach_volume_full($conf, { include_unused => 1 }, $remove_volume);
 
-- 
2.47.3

[PATCH container v9 04/10] config: ensure valid volid through parse_volume()

[Michael Köppl]

Even though the value in $conf->{$opt} contains a volume ID for unused
mount points at the moment, this is not guaranteed to be true in the
future. To ensure that a valid volume ID is used here, run call
parse_volume() first.

No functional change is intended here as the values of $conf->{$opt}
and $mp->{volume} are identical for unused mount points at the moment.

Also add FIXME for the is_volume_in_use() check for the branch that
handles mountpoint volumes that are in use. Currently, the check only
works by accident. The value in $conf->{$opt} isn’t just a volume ID -
it contains other things as well (such as mp=<path>). However, the
check only considers 'rootfs' and entries like 'mp0', 'mp1', etc. as
valid volume IDs. As a result, the volume is not detected to be in use.
Using a valid volume id with the current implementation, the check would
always report that the volume is in use and the mountpoint volume would
not be deleted. The correct behavior should use a valid volume id to
ensure that the volume can be safely removed.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/PVE/LXC/Config.pm | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index 80942d34..c8098538 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -1662,8 +1662,9 @@ sub vmconfig_hotplug_pending {
             if ($LXC_FASTPLUG_OPTIONS->{$opt}) {
                 # pass
             } elsif ($opt =~ m/^unused(\d+)$/) {
-                PVE::LXC::delete_mountpoint_volume($storecfg, $vmid, $conf->{$opt})
-                    if !$class->is_volume_in_use($conf, $conf->{$opt}, 1, 1);
+                my $volid = $class->parse_volume($opt, $conf->{$opt})->{volume};
+                PVE::LXC::delete_mountpoint_volume($storecfg, $vmid, $volid)
+                    if !$class->is_volume_in_use($conf, $volid, 1, 1);
             } elsif ($opt eq 'swap') {
                 $hotplug_memory->(undef, 0);
             } elsif ($opt eq 'cpulimit') {
@@ -1764,12 +1765,14 @@ sub vmconfig_apply_pending {
             if ($opt =~ m/^mp(\d+)$/) {
                 my $mp = $class->parse_volume($opt, $conf->{$opt});
                 if ($mp->{type} eq 'volume') {
+                    # FIXME: use $mp->{volume} for is_volume_in_use, fix conditions for check
                     $class->add_unused_volume($conf, $mp->{volume})
                         if !$class->is_volume_in_use($conf, $conf->{$opt}, 1, 1);
                 }
             } elsif ($opt =~ m/^unused(\d+)$/) {
-                PVE::LXC::delete_mountpoint_volume($storecfg, $vmid, $conf->{$opt})
-                    if !$class->is_volume_in_use($conf, $conf->{$opt}, 1, 1);
+                my $volid = $class->parse_volume($opt, $conf->{$opt})->{volume};
+                PVE::LXC::delete_mountpoint_volume($storecfg, $vmid, $volid)
+                    if !$class->is_volume_in_use($conf, $volid, 1, 1);
             } elsif ($opt =~ m/^net(\d+)$/) {
                 if ($have_sdn) {
                     my $net = $class->parse_lxc_network($conf->{$opt});
-- 
2.47.3

[PATCH container v9 05/10] allow pending mount point changes if storage is gone

[Michael Köppl]

Previously, applying a pending removal of an unused volume whose
backing storage was already removed would fail with "unable to apply
pending change unusedN", leaving a dangling reference that could never
be cleaned up.

Guard the volume deletion in vmconfig_apply_pending and
vmconfig_hotplug_pending with a check to ensure that the storage
exists. If the storage is gone, drop the entry with a warning instead of
trying to free a volume that no longer exists.

Apply the same check when a detached mount point is turned into an
'unusedN' entry, so a volume on a removed storage is not re-added as a
dangling unused entry, matching VM behavior.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/PVE/LXC/Config.pm | 43 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 34 insertions(+), 9 deletions(-)

diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index c8098538..a3dd3a1a 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -1663,8 +1663,14 @@ sub vmconfig_hotplug_pending {
                 # pass
             } elsif ($opt =~ m/^unused(\d+)$/) {
                 my $volid = $class->parse_volume($opt, $conf->{$opt})->{volume};
-                PVE::LXC::delete_mountpoint_volume($storecfg, $vmid, $volid)
-                    if !$class->is_volume_in_use($conf, $volid, 1, 1);
+                my ($storeid) = PVE::Storage::parse_volume_id($volid);
+                if (PVE::Storage::storage_config($storecfg, $storeid, 1)) {
+                    PVE::LXC::delete_mountpoint_volume($storecfg, $vmid, $volid)
+                        if !$class->is_volume_in_use($conf, $volid, 1, 1);
+                } else {
+                    PVE::RESTEnvironment::log_warn(
+                        "storage '$storeid' no longer exists, unused volume '$volid' will be removed");
+                }
             } elsif ($opt eq 'swap') {
                 $hotplug_memory->(undef, 0);
             } elsif ($opt eq 'cpulimit') {
@@ -1765,14 +1771,27 @@ sub vmconfig_apply_pending {
             if ($opt =~ m/^mp(\d+)$/) {
                 my $mp = $class->parse_volume($opt, $conf->{$opt});
                 if ($mp->{type} eq 'volume') {
-                    # FIXME: use $mp->{volume} for is_volume_in_use, fix conditions for check
-                    $class->add_unused_volume($conf, $mp->{volume})
-                        if !$class->is_volume_in_use($conf, $conf->{$opt}, 1, 1);
+                    my ($storeid, undef) = PVE::Storage::parse_volume_id($mp->{volume});
+                    if (PVE::Storage::storage_config($storecfg, $storeid, 1)) {
+                        # FIXME: use $mp->{volume} for is_volume_in_use, fix conditions for check
+                        $class->add_unused_volume($conf, $mp->{volume})
+                            if !$class->is_volume_in_use($conf, $conf->{$opt}, 1, 1);
+                    } else {
+                        PVE::RESTEnvironment::log_warn(
+                            "storage '$storeid' no longer exists, volume '$mp->{volume}' will be removed"
+                        );
+                    }
                 }
             } elsif ($opt =~ m/^unused(\d+)$/) {
                 my $volid = $class->parse_volume($opt, $conf->{$opt})->{volume};
-                PVE::LXC::delete_mountpoint_volume($storecfg, $vmid, $volid)
-                    if !$class->is_volume_in_use($conf, $volid, 1, 1);
+                my ($storeid) = PVE::Storage::parse_volume_id($volid);
+                if (PVE::Storage::storage_config($storecfg, $storeid, 1)) {
+                    PVE::LXC::delete_mountpoint_volume($storecfg, $vmid, $volid)
+                        if !$class->is_volume_in_use($conf, $volid, 1, 1);
+                } else {
+                    PVE::RESTEnvironment::log_warn(
+                        "storage '$storeid' no longer exists, unused volume '$volid' will be removed");
+                }
             } elsif ($opt =~ m/^net(\d+)$/) {
                 if ($have_sdn) {
                     my $net = $class->parse_lxc_network($conf->{$opt});
@@ -1891,8 +1910,14 @@ sub apply_pending_mountpoint {
     if (defined($old)) {
         my $mp = $class->parse_volume($opt, $old);
         if ($mp->{type} eq 'volume') {
-            $class->add_unused_volume($conf, $mp->{volume})
-                if !$class->is_volume_in_use($conf, $conf->{$opt}, 1, 1);
+            my ($storeid) = PVE::Storage::parse_volume_id($mp->{volume});
+            if (PVE::Storage::storage_config($storecfg, $storeid, 1)) {
+                $class->add_unused_volume($conf, $mp->{volume})
+                    if !$class->is_volume_in_use($conf, $conf->{$opt}, 1, 1);
+            } else {
+                PVE::RESTEnvironment::log_warn(
+                    "storage '$storeid' no longer exists, volume '$mp->{volume}' will be removed");
+            }
         }
     }
 }
-- 
2.47.3

[PATCH container v9 06/10] add linked clone check when destroying container template

[Michael Köppl]

This check 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. In case of a storage error (such as the underlying storage no
existing anymore), a warning will be printed and execution continues,
mimicking the handling of storage errors in later stages of
destroy_lxc_container().

Originally-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
 [ MK: use classify_mountpoint instead of pattern matching
       display warning for storage errors during linked clone check
       resolve style nit ]
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/PVE/LXC.pm | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index e76da29f..c9f3cba7 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -1024,6 +1024,25 @@ 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) };
+                PVE::RESTEnvironment::log_warn(
+                    "failed to check if volume '$volid' is used by linked clones: $@")
+                    if $@;
+                die "base volume '$volid' is still in use by linked clone\n" if $result;
+            },
+        );
+    }
+
     my $remove_volume = sub {
         my ($ms, $mountpoint) = @_;
 
-- 
2.47.3

[PATCH qemu-server v9 07/10] 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. In case an error occurred,
a warning is printed and the execution continues, since:

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

Originally-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
 [ MK: log_warn if check fails instead of ignoring
       resolve style nits ]
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/PVE/QemuServer.pm | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index cdf66e89..ed5bca0c 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -1838,8 +1838,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);
+                my $result = eval { PVE::Storage::volume_is_base_and_used($storecfg, $volid) };
+                log_warn("failed to check if volume '$volid' is used by linked clones: $@")
+                    if $@;
+                die "base volume '$volid' is still in use by linked cloned\n" if $result;
 
             },
         );
-- 
2.47.3

[PATCH qemu-server v9 08/10] fix #3711: make removal of VM possible if store does not exist anymore

[Michael Köppl]

Similar to the handling of storage errors in other parts of
destroy_vm(), an error during the call to PVE::Storage::path() should
not stop the VM from being destroyed. Instead, the user should be warned
and the function should continue.

Originally-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
 [ MK: log_warn if check fails instead of ignoring error ]
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/PVE/QemuServer.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index ed5bca0c..123a357d 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -1856,7 +1856,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': $@") if $@;
         return if !$path || !$owner || ($owner != $vmid);
 
         $volids->{$volid} = 1;
-- 
2.47.3

[PATCH qemu-server v9 09/10] destroy_vm: use log_warn for vdisk_free errors for consistency

[Michael Köppl]

To make warnings visually consistent with the handling of other storage
errors in destroy_vm(), replace the use of warn with log_warn.

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

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 123a357d..31680483 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -1862,7 +1862,7 @@ sub destroy_vm {
 
         $volids->{$volid} = 1;
         eval { PVE::Storage::vdisk_free($storecfg, $volid) };
-        warn "Could not remove disk '$volid', check manually: $@" if $@;
+        log_warn("Could not remove disk '$volid', check manually: $@") if $@;
     };
 
     # only remove disks owned by this VM (referenced in the config)
@@ -1888,7 +1888,7 @@ sub destroy_vm {
             sub {
                 my ($volid, $sid, $volname, $d) = @_;
                 eval { PVE::Storage::vdisk_free($storecfg, $volid) };
-                warn $@ if $@;
+                log_warn($@) if $@;
             },
         );
     }
-- 
2.47.3

[PATCH qemu-server v9 10/10] display warnings for storage errors or if storage no longer exists

[Michael Köppl]

Instead of continuing without informing the user, a warning will now be
displayed if the owner of a volume could not be determined due to a
storage error. In addition, an explicit check for the existence of the
underlying storage is added before the ownership check. If the storage
no longer exists, a warning will be displayed, consistent with the
handling of this scenario in other functions.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/PVE/QemuServer.pm | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 31680483..5cb31bdb 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -1557,6 +1557,7 @@ sub vm_is_volid_owner {
     if ($volid !~ m|^/|) {
         my ($path, $owner);
         eval { ($path, $owner) = PVE::Storage::path($storecfg, $volid); };
+        log_warn("ownership of volume '$volid' could not be determined: $@") if $@;
         if ($owner && ($owner == $vmid)) {
             return 1;
         }
@@ -1574,8 +1575,13 @@ 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)) {
-            PVE::QemuConfig->add_unused_volume($conf, $volid, $vmid);
+        my ($storeid, undef) = PVE::Storage::parse_volume_id($volid);
+        if (PVE::Storage::storage_config($storecfg, $storeid, 1)) {
+            if (vm_is_volid_owner($storecfg, $vmid, $volid)) {
+                PVE::QemuConfig->add_unused_volume($conf, $volid, $vmid);
+            }
+        } else {
+            log_warn("storage '$storeid' no longer exists, volume '$volid' will be removed");
         }
     }
 }
-- 
2.47.3

Re: [PATCH container v9 05/10] allow pending mount point changes if storage is gone

[Fiona Ebner]

Am 29.06.26 um 4:06 PM schrieb Michael Köppl:
> Previously, applying a pending removal of an unused volume whose
> backing storage was already removed would fail with "unable to apply
> pending change unusedN", leaving a dangling reference that could never
> be cleaned up.
> 
> Guard the volume deletion in vmconfig_apply_pending and
> vmconfig_hotplug_pending with a check to ensure that the storage
> exists. If the storage is gone, drop the entry with a warning instead of
> trying to free a volume that no longer exists.
> 
> Apply the same check when a detached mount point is turned into an
> 'unusedN' entry, so a volume on a removed storage is not re-added as a
> dangling unused entry, matching VM behavior.
> 
> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
>  src/PVE/LXC/Config.pm | 43 ++++++++++++++++++++++++++++++++++---------
>  1 file changed, 34 insertions(+), 9 deletions(-)
> 
> diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
> index c8098538..a3dd3a1a 100644
> --- a/src/PVE/LXC/Config.pm
> +++ b/src/PVE/LXC/Config.pm

Missing import for PVE::RESTEnvironment.

> @@ -1663,8 +1663,14 @@ sub vmconfig_hotplug_pending {
>                  # pass
>              } elsif ($opt =~ m/^unused(\d+)$/) {
>                  my $volid = $class->parse_volume($opt, $conf->{$opt})->{volume};
> -                PVE::LXC::delete_mountpoint_volume($storecfg, $vmid, $volid)
> -                    if !$class->is_volume_in_use($conf, $volid, 1, 1);
> +                my ($storeid) = PVE::Storage::parse_volume_id($volid);
> +                if (PVE::Storage::storage_config($storecfg, $storeid, 1)) {
> +                    PVE::LXC::delete_mountpoint_volume($storecfg, $vmid, $volid)
> +                        if !$class->is_volume_in_use($conf, $volid, 1, 1);
> +                } else {
> +                    PVE::RESTEnvironment::log_warn(
> +                        "storage '$storeid' no longer exists, unused volume '$volid' will be removed");
> +                }

Style nit: should we add a helper for readability/to reduce bloat and
repetition?

I'm thinking about something like

with_checked_volid($opt, $conf->{opt}, sub {
    my ($volid) = @_;
    PVE::LXC::delete_mountpoint_volume($storecfg, $vmid, $volid)
        if !$class->is_volume_in_use($conf, $volid, 1, 1);
});

where the helper does the parsing of the volume and volid, logs the
warning and returns if the storage does not exist and calls the
passed-in sub if it does exist.

Re: [PATCH container v9 06/10] add linked clone check when destroying container template

[Fiona Ebner]

Am 29.06.26 um 4:04 PM schrieb Michael Köppl:
> This check 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. In case of a storage error (such as the underlying storage no
> existing anymore), a warning will be printed and execution continues,
> mimicking the handling of storage errors in later stages of
> destroy_lxc_container().
> 
> Originally-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
>  [ MK: use classify_mountpoint instead of pattern matching
>        display warning for storage errors during linked clone check
>        resolve style nit ]
> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
>  src/PVE/LXC.pm | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index e76da29f..c9f3cba7 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -1024,6 +1024,25 @@ 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) };
> +                PVE::RESTEnvironment::log_warn(
> +                    "failed to check if volume '$volid' is used by linked clones: $@")
> +                    if $@;

Style nit: multi-line post-if. If you import the log_warn() directly, it
should be shorter ;)

Maybe note with a comment that this is an early check and removal of the
volume itself would still fail later, which is why it's fine to just log
a warning here.

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

Re: [PATCH qemu-server v9 07/10] adapt linked clone check to not die if an error occurs during check

[Fiona Ebner]

A prefix for the commit title like "destroy vm:" would be nice. Same
goes for patch 6/10 where I only noticed it now.

Am 29.06.26 um 4:05 PM schrieb Michael Köppl:
> Align error handling behavior when checking for linked clones with the
> rest of destroy_vm's error handling approach. In case an error occurred,
> a warning is printed and the execution continues, since:
> 
> 1. The same validation occurs later in the process
> 2. The VM removal will still be blocked if the volume has linked clones
> 
> Originally-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
>  [ MK: log_warn if check fails instead of ignoring
>        resolve style nits ]
> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
>  src/PVE/QemuServer.pm | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
> index cdf66e89..ed5bca0c 100644
> --- a/src/PVE/QemuServer.pm
> +++ b/src/PVE/QemuServer.pm
> @@ -1838,8 +1838,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);
> +                my $result = eval { PVE::Storage::volume_is_base_and_used($storecfg, $volid) };
> +                log_warn("failed to check if volume '$volid' is used by linked clones: $@")
> +                    if $@;

Same as for patch 6/10, a code comment would be nice here.

> +                die "base volume '$volid' is still in use by linked cloned\n" if $result;
>  
>              },
>          );

Re: [PATCH storage v9 01/10] fix #3711: vdisk_free: print warning if storage does not exist

[Fiona Ebner]

Am 29.06.26 um 4:05 PM schrieb Michael Köppl:
> Removing a guest or one of its volumes should not fail when the volume's
> backing storage does not exist in the configuration. Instead of dying,
> warn and return so the removal can proceed. From PVE's point of view,
> the volume is gone anyway. Pass the noerr flag to storage_config() so
> this case is actually reached.
> 
> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
>  src/PVE/Storage.pm | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index 64ea9dad..55462ff6 100755
> --- a/src/PVE/Storage.pm
> +++ b/src/PVE/Storage.pm
> @@ -1187,7 +1187,12 @@ sub vdisk_free {
>      my ($cfg, $volid) = @_;
>  
>      my ($storeid, $volname) = parse_volume_id($volid);
> -    my $scfg = storage_config($cfg, $storeid);
> +    my $scfg = storage_config($cfg, $storeid, 1);
> +    if (!$scfg) {
> +        log_warn("storage '$storeid' does not exist, volume '$volid' could not be freed");
> +        return;
> +    }
> +
>      my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>  
>      activate_storage($cfg, $storeid);

Not sure about this one. Note that vdisk_free() is also used for the
DELETE API, so the volume ID can also be user input and not something
generated by our stack. While the API call still returns an error code
for an invalid volume ID, that's more by chance, because of an earlier
call to path(). Failing does seem more natural and callers can decide
what they want to do. This change is not required for the rest of the
series to work or is it?

Re: [PATCH container/qemu-server/storage v9 00/10] fix #3711 and adapt drive detach/remove behavior

[Fiona Ebner]

Am 29.06.26 um 4:05 PM schrieb 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 also adds warnings in case a
> volume's underlying storage does not exist anymore. It is a
> continuation of a series from 2022 [1], but makes the following changes:

Seems like it's nearly ready, nice! Feel free to add my R-b to patches
(other than 01/10) that won't considerably change in v10 :)

Re: [PATCH storage v9 01/10] fix #3711: vdisk_free: print warning if storage does not exist

[Michael Köppl]

On Tue Jun 30, 2026 at 4:21 PM CEST, Fiona Ebner wrote:

[snip]

>  
>>      my ($storeid, $volname) = parse_volume_id($volid);
>> -    my $scfg = storage_config($cfg, $storeid);
>> +    my $scfg = storage_config($cfg, $storeid, 1);
>> +    if (!$scfg) {
>> +        log_warn("storage '$storeid' does not exist, volume '$volid' could not be freed");
>> +        return;
>> +    }
>> +
>>      my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>>  
>>      activate_storage($cfg, $storeid);
>
> Not sure about this one. Note that vdisk_free() is also used for the
> DELETE API, so the volume ID can also be user input and not something
> generated by our stack. While the API call still returns an error code
> for an invalid volume ID, that's more by chance, because of an earlier
> call to path(). Failing does seem more natural and callers can decide
> what they want to do. This change is not required for the rest of the
> series to work or is it?

Thanks for having a look at this series!

I wanted to double-check this before replying, but yes, you're right,
this was an oversight on my side. I'll drop the patch for the v10 of
this series. The call sites used for removal of guests/disks should all
be covered, so removal still works for all relevant cases even if the
underlying storage is gone.

Re: [PATCH container v9 05/10] allow pending mount point changes if storage is gone

[Michael Köppl]

On Tue Jun 30, 2026 at 3:41 PM CEST, Fiona Ebner wrote:

[snip]

>>  src/PVE/LXC/Config.pm | 43 ++++++++++++++++++++++++++++++++++---------
>>  1 file changed, 34 insertions(+), 9 deletions(-)
>> 
>> diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
>> index c8098538..a3dd3a1a 100644
>> --- a/src/PVE/LXC/Config.pm
>> +++ b/src/PVE/LXC/Config.pm
>
> Missing import for PVE::RESTEnvironment.

Ack, will fix in v10. Thanks!

>
>> @@ -1663,8 +1663,14 @@ sub vmconfig_hotplug_pending {
>>                  # pass
>>              } elsif ($opt =~ m/^unused(\d+)$/) {
>>                  my $volid = $class->parse_volume($opt, $conf->{$opt})->{volume};
>> -                PVE::LXC::delete_mountpoint_volume($storecfg, $vmid, $volid)
>> -                    if !$class->is_volume_in_use($conf, $volid, 1, 1);
>> +                my ($storeid) = PVE::Storage::parse_volume_id($volid);
>> +                if (PVE::Storage::storage_config($storecfg, $storeid, 1)) {
>> +                    PVE::LXC::delete_mountpoint_volume($storecfg, $vmid, $volid)
>> +                        if !$class->is_volume_in_use($conf, $volid, 1, 1);
>> +                } else {
>> +                    PVE::RESTEnvironment::log_warn(
>> +                        "storage '$storeid' no longer exists, unused volume '$volid' will be removed");
>> +                }
>
> Style nit: should we add a helper for readability/to reduce bloat and
> repetition?
>
> I'm thinking about something like
>
> with_checked_volid($opt, $conf->{opt}, sub {
>     my ($volid) = @_;
>     PVE::LXC::delete_mountpoint_volume($storecfg, $vmid, $volid)
>         if !$class->is_volume_in_use($conf, $volid, 1, 1);
> });
>
> where the helper does the parsing of the volume and volid, logs the
> warning and returns if the storage does not exist and calls the
> passed-in sub if it does exist.

Sounds good, I'll add a helper. Thanks for the suggestion. Though I'd go
with something like:

```
sub with_checked_volid {
    my ($storecfg, $volid, $callback) = @_;

    my ($storeid) = PVE::Storage::parse_volume_id($volid);
    if (PVE::Storage::storage_config($storecfg, $storeid, 1)) {
        callback->();
    } else {
        PVE::RESTEnvironment::log_warn(
            "storage '$storeid' no longer exists, volume '$volid' will be removed");
    }
}
```

I'd keep the parsing of the volume outside the helper. The call sites
don't use the result of parse_volume uniformly. It seems easier to me
using the same helper across all call sites by passing in an already
resolved volid as the common denominator. What do you think?

Re: [PATCH container v9 05/10] allow pending mount point changes if storage is gone

[Fiona Ebner]

Am 01.07.26 um 2:15 PM schrieb Michael Köppl:
> On Tue Jun 30, 2026 at 3:41 PM CEST, Fiona Ebner wrote:
>> I'm thinking about something like
>>
>> with_checked_volid($opt, $conf->{opt}, sub {
>>     my ($volid) = @_;
>>     PVE::LXC::delete_mountpoint_volume($storecfg, $vmid, $volid)
>>         if !$class->is_volume_in_use($conf, $volid, 1, 1);
>> });
>>
>> where the helper does the parsing of the volume and volid, logs the
>> warning and returns if the storage does not exist and calls the
>> passed-in sub if it does exist.
> 
> Sounds good, I'll add a helper. Thanks for the suggestion. Though I'd go
> with something like:
> 
> ```
> sub with_checked_volid {
>     my ($storecfg, $volid, $callback) = @_;
> 
>     my ($storeid) = PVE::Storage::parse_volume_id($volid);
>     if (PVE::Storage::storage_config($storecfg, $storeid, 1)) {
>         callback->();
>     } else {
>         PVE::RESTEnvironment::log_warn(
>             "storage '$storeid' no longer exists, volume '$volid' will be removed");
>     }
> }
> ```
> 
> I'd keep the parsing of the volume outside the helper. The call sites
> don't use the result of parse_volume uniformly. It seems easier to me
> using the same helper across all call sites by passing in an already
> resolved volid as the common denominator. What do you think?

Sound good :)

superseded: [PATCH container/qemu-server/storage v9 00/10] fix #3711 and adapt drive detach/remove behavior

[Michael Köppl]

Superseded-by:
https://lore.proxmox.com/pve-devel/20260701141339.181276-1-m.koeppl@proxmox.com/

On Mon Jun 29, 2026 at 4:04 PM CEST, 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

[snip]