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

[PATCH container/qemu-server v10 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 v9 -> v10 (thanks @Fiona for the feedback!):
- Remove pve-storage patch since it was not required for the rest of the
  series to work
- Add patch for pve-container that adds a helper function for
  skip-and-warn if a volume's underlying storage doesn't exist. The
  helper is used in the following patch to avoid repeating the same
  if-else in 4 call sites
- Adapt mountpoint-related patch for pve-container to use the
  with_checked_volid helper
- Add missing PVE::RESTEnvironment import
- Fix multi-line post-if style nit (although without directly importing
  log_warn() to keep it consistent with the remaining codebase)
- Add comments explaining why printing a warning and continuing is fine
  for the linked clone checks in pve-container and qemu-server

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-container:

Michael Köppl (6):
  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()
  lxc config: add helper for parsing volid and checking if storage
    exists
  allow pending mount point changes if storage is gone
  destroy template: add early check for linked clones of volumes

 src/PVE/LXC.pm        | 41 +++++++++++++++++++++++-------
 src/PVE/LXC/Config.pm | 58 +++++++++++++++++++++++++++++++++++++------
 2 files changed, 82 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 | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)


Summary over all repositories:
  3 files changed, 99 insertions(+), 24 deletions(-)

-- 
Generated by murpp 0.11.0

[PATCH container v10 01/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.

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
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 v10 02/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.

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
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 v10 03/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.

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
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 v10 04/10] lxc config: add helper for parsing volid and checking if storage exists

[Michael Köppl]

Add a helper function to avoid repetition across multiple call sites
where a volume id is parsed and functions check for the existence of the
given storage, log a warning and return if it does not exist, and
perform some action otherwise (e.g. adding a disk as unused).

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

diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index c8098538..57c93e8a 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -11,6 +11,7 @@ use PVE::DataCenterConfig;
 use PVE::GuestHelpers;
 use PVE::INotify;
 use PVE::JSONSchema qw(get_standard_option);
+use PVE::RESTEnvironment;
 use PVE::Tools;
 
 use PVE::LXC;
@@ -1609,6 +1610,20 @@ sub calculate_memory_constraints {
     return ($memory_max, $memory_high);
 }
 
+# Skip and warn if a volume's backing storage was already removed from the storage config, so
+# guest/volume removal can still proceed.
+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");
+    }
+}
+
 my $LXC_FASTPLUG_OPTIONS = {
     'description' => 1,
     'onboot' => 1,
-- 
2.47.3

[PATCH container v10 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 | 42 +++++++++++++++++++++++++++++++++---------
 1 file changed, 33 insertions(+), 9 deletions(-)

diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index 57c93e8a..8c99c931 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -1678,8 +1678,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);
+                with_checked_volid(
+                    $storecfg,
+                    $volid,
+                    sub {
+                        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') {
@@ -1780,14 +1786,26 @@ 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);
+                    with_checked_volid(
+                        $storecfg,
+                        $mp->{volume},
+                        sub {
+                            # 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+)$/) {
                 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);
+                with_checked_volid(
+                    $storecfg,
+                    $volid,
+                    sub {
+                        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});
@@ -1906,8 +1924,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);
+            with_checked_volid(
+                $storecfg,
+                $mp->{volume},
+                sub {
+                    $class->add_unused_volume($conf, $mp->{volume})
+                        if !$class->is_volume_in_use($conf, $conf->{$opt}, 1, 1);
+                },
+            );
         }
     }
 }
-- 
2.47.3

[PATCH container v10 06/10] destroy template: add early check for linked clones of volumes

[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 not
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 | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index e76da29f..3d217f24 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -1024,6 +1024,28 @@ 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) };
+                if ($@) {
+                    # early check, removal of volume will fail later anyway, so warning here is fine
+                    PVE::RESTEnvironment::log_warn(
+                        "failed to check if volume '$volid' is used by linked clones: $@");
+                }
+
+                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 v10 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 | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index cdf66e89..50fd66a5 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -1838,8 +1838,11 @@ 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) };
+                # early check, removal of volume will fail later anyway, so warning here is fine
+                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 v10 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.

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
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 50fd66a5..b69af937 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -1857,7 +1857,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 v10 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.

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
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 b69af937..3fb499a9 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -1863,7 +1863,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)
@@ -1889,7 +1889,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 v10 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.

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
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 3fb499a9..1ca4e911 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