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

[pve-devel] [PATCH container/qemu-server/storage v7 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 contains 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 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)

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

Michael Köppl (2):
  vdisk_free: print warning if underlying storage does not exist
  parse_volname: print warning if storage does not exists

 src/PVE/Storage.pm | 9 +++++++++
 1 file changed, 9 insertions(+)


pve-container.git:

Michael Köppl (4):
  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()
  add linked clone check when destroying container template

 src/PVE/LXC.pm        | 31 ++++++++++++++++++++++---------
 src/PVE/LXC/Config.pm | 11 +++++++----
 2 files changed, 29 insertions(+), 13 deletions(-)


qemu-server.git:

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

 PVE/QemuServer.pm | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)


Summary over all repositories:
  4 files changed, 53 insertions(+), 21 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 storage v7 1/2] vdisk_free: print warning if underlying storage does not exist

[Michael Köppl]

Instead of failing later in the function, users are warned if the
underlying storage no longer exists.

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

diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index d0a696a..f1fd46f 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -1046,6 +1046,11 @@ sub vdisk_free {
 
     my ($storeid, $volname) = parse_volume_id($volid);
     my $scfg = storage_config($cfg, $storeid);
+    if (!$scfg) {
+	log_warn("storage '$storeid' does not exist, mountpoint volume '$volid' could not be deleted");
+	return;
+    }
+
     my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
 
     activate_storage($cfg, $storeid);
-- 
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 storage v7 2/2] parse_volname: print warning if storage does not exists

[Michael Köppl]

To keep behavior around non-existent storages consistent with
vdisk_free(), also print warning if storage does not exist.

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

diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index f1fd46f..e7cb2f3 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -523,6 +523,10 @@ sub parse_volname {
     my ($storeid, $volname) = parse_volume_id($volid);
 
     my $scfg = storage_config($cfg, $storeid);
+    if (!$scfg) {
+	log_warn("storage '$storeid' does not exist, mountpoint volume '$volid' could not be deleted");
+	return;
+    }
 
     my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
 
-- 
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 v7 1/4] 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 2b9f0cf..20ff54e 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -953,7 +953,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.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 v7 2/4] 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 20ff54e..4403701 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -928,16 +928,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";
     }
 }
 
@@ -948,13 +948,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.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 v7 3/4] 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 0740e8c..052cc27 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -1473,8 +1473,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') {
@@ -1560,12 +1561,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.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 v7 4/4] 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 | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 4403701..fa762f5 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) };
+	   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.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 v7 1/4] 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>
---
 PVE/QemuServer.pm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 577959a4..c55617bd 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2116,9 +2116,9 @@ 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.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 v7 2/4] 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>
---
 PVE/QemuServer.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index c55617bd..c0170a38 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2131,7 +2131,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.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 v7 3/4] 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>
---
 PVE/QemuServer.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index c0170a38..5eb439a8 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2137,7 +2137,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)
@@ -2161,7 +2161,7 @@ sub destroy_vm {
 	PVE::Storage::foreach_volid($vmdisks, sub {
 	    my ($volid, $sid, $volname, $d) = @_;
 	    eval { PVE::Storage::vdisk_free($storecfg, $volid) };
-	    warn $@ if $@;
+	    log_warn($@) if $@;
 	});
     }
 
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server v7 4/4] 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>
---
 PVE/QemuServer.pm | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 5eb439a8..8a7bd30e 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -1821,6 +1821,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;
 	}
@@ -1838,8 +1839,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.39.5



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