[pve-devel] [PATCH V2 pve-storage/pve-container/qemu-server/pve-manager 0/8] fix #3711 & adapt drive detach/remove behavior

[pve-devel] [PATCH V2 pve-storage/pve-container/qemu-server/pve-manager 0/8] fix #3711 & adapt drive detach/remove behavior

[Stefan Hrdlicka]

V1 -> V2:
# overall
* matched detaching/removing drives behavior for VM & containers
  It currently works this way:
  - Detach drive
  - drive shows up as unused
  - remove drive
  - drive will be removed without removing data (obviously)

# pve-storage
* added storage_exists function for matching detach/remove behavoir

# pve-container
* review fixes:
    * variable naming
    * desciption string adapted
    * moved eval further up the call chain
    * removed ticket number form cleanup
* check if storage exists for unused disks

# qemu-server
* add same force option as for containers
* match detach/remove behavoir between VM/container
* shorten line

# pve-manager
* added same option for VMs as for containers


-- 
2.30.2

[pve-devel] [PATCH V2 pve-storage 1/8] add a storage_exists function

[Stefan Hrdlicka]

adds a function that can take a volume id and return the relevant
storage config

Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
---
 PVE/Storage.pm | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index b9c53a1..9e95e3d 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -158,6 +158,15 @@ my $convert_maxfiles_to_prune_backups = sub {
     }
 };
 
+sub storage_exists {
+    my ($cfg, $volid) = @_;
+
+    my ($storeid, $volname) = parse_volume_id($volid);
+    my $scfg = storage_config($cfg, $storeid, 1);
+
+    return $scfg;
+}
+
 sub storage_config {
     my ($cfg, $storeid, $noerr) = @_;
 
-- 
2.30.2

Re: [pve-devel] [PATCH V2 pve-storage 1/8] add a storage_exists function

[Thomas Lamprecht]

Am 12/09/2022 um 17:25 schrieb Stefan Hrdlicka:
> adds a function that can take a volume id and return the relevant
> storage config
> 
> Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
> ---
>  PVE/Storage.pm | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index b9c53a1..9e95e3d 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -158,6 +158,15 @@ my $convert_maxfiles_to_prune_backups = sub {
>      }
>  };
>  
> +sub storage_exists {

slightly odd interface name IMO, as it's a bit more of a "get storage config from
volid", so adapt the method name in that direction, e.g.:

# extract the storage ID from a volume ID and returns it config or undef if storage could not be
# found (i.e., got removed)
sub storage_config_from_volid {

> +    my ($cfg, $volid) = @_;
> +
> +    my ($storeid, $volname) = parse_volume_id($volid);
> +    my $scfg = storage_config($cfg, $storeid, 1);
> +
> +    return $scfg;

couldn't we avoid the useless intermediate variable and directly

return storage_config($cfg, $storeid, 1);

At which point the question arises if we really want a common helper "just" for this,
no hard feelings against, but is IMO a bit close to thinking that not much would be
lost in terms of code beauty or maintenance if we'd just program those two lines out
on the few call sites.

> +}
> +
>  sub storage_config {
>      my ($cfg, $storeid, $noerr) = @_;
>  

[pve-devel] [PATCH V2 pve-container 2/8] fix #3711: enable delete of LXC container

[Stefan Hrdlicka]

Make it possible to delete a container whoes underlying storage is no
longer available. This will just write an warning instead of dying.

Without setting the option ignore-storage-errors=1 a delete will still
fail, like it did before the changes. With this option set it will try to
delete the volume from the storage. If this fails it writes a warning.

review fixes
- rename parameter to ignore-storage-errors
- move eval further up the call chain

Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
---
 src/PVE/API2/LXC.pm | 8 ++++++++
 src/PVE/LXC.pm      | 8 ++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 589f96f..a7cd41d 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -697,6 +697,13 @@ __PACKAGE__->register_method({
 		    ." enabled storages which are not referenced in the config.",
 		optional => 1,
 	    },
+	    'ignore-storage-errors' => {
+		type => 'boolean',
+		description => 'If set, this will ignore errors when trying to remove'
+		    . ' container storage.',
+		default => 0,
+		optional => 1,
+	    }
 	},
     },
     returns => {
@@ -758,6 +765,7 @@ __PACKAGE__->register_method({
 		$conf,
 		{ lock => 'destroyed' },
 		$param->{'destroy-unreferenced-disks'},
+		$param->{'ignore-storage-errors'},
 	    );
 
 	    PVE::AccessControl::remove_vm_access($vmid);
diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index fe63087..e380b12 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -862,7 +862,8 @@ sub delete_mountpoint_volume {
 }
 
 sub destroy_lxc_container {
-    my ($storage_cfg, $vmid, $conf, $replacement_conf, $purge_unreferenced) = @_;
+    my ($storage_cfg, $vmid, $conf, $replacement_conf,
+	$purge_unreferenced, $ignore_storage_errors) = @_;
 
     my $volids = {};
     my $remove_volume = sub {
@@ -873,7 +874,10 @@ 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);
+	};
+	die $@ if !$ignore_storage_errors && $@;
     };
     PVE::LXC::Config->foreach_volume_full($conf, {include_unused => 1}, $remove_volume);
 
-- 
2.30.2

Re: [pve-devel] [PATCH V2 pve-container 2/8] fix #3711: enable delete of LXC container

[Thomas Lamprecht]

"enable delete of LXC container" makes it sound like we couldn't delete a CT
at all before and we know that it's about a LXC Container, we only understand
those, so rather something like:

"fix #3711: optionally allow CT deletion to complete on disk volume removal errors"


Am 12/09/2022 um 17:25 schrieb Stefan Hrdlicka:
> Make it possible to delete a container whoes underlying storage is no
> longer available. This will just write an warning instead of dying.
> 
> Without setting the option ignore-storage-errors=1 a delete will still
> fail, like it did before the changes. With this option set it will try to
> delete the volume from the storage. If this fails it writes a warning.
> 
> review fixes
> - rename parameter to ignore-storage-errors
> - move eval further up the call chain
> 
> Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
> ---
>  src/PVE/API2/LXC.pm | 8 ++++++++
>  src/PVE/LXC.pm      | 8 ++++++--
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index 589f96f..a7cd41d 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -697,6 +697,13 @@ __PACKAGE__->register_method({
>  		    ." enabled storages which are not referenced in the config.",
>  		optional => 1,
>  	    },
> +	    'ignore-storage-errors' => {
> +		type => 'boolean',
> +		description => 'If set, this will ignore errors when trying to remove'
> +		    . ' container storage.',
> +		default => 0,
> +		optional => 1,
> +	    }
>  	},
>      },
>      returns => {
> @@ -758,6 +765,7 @@ __PACKAGE__->register_method({
>  		$conf,
>  		{ lock => 'destroyed' },
>  		$param->{'destroy-unreferenced-disks'},
> +		$param->{'ignore-storage-errors'},
>  	    );
>  
>  	    PVE::AccessControl::remove_vm_access($vmid);
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index fe63087..e380b12 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -862,7 +862,8 @@ sub delete_mountpoint_volume {
>  }
>  
>  sub destroy_lxc_container {
> -    my ($storage_cfg, $vmid, $conf, $replacement_conf, $purge_unreferenced) = @_;
> +    my ($storage_cfg, $vmid, $conf, $replacement_conf,
> +	$purge_unreferenced, $ignore_storage_errors) = @_;
>  
>      my $volids = {};
>      my $remove_volume = sub {
> @@ -873,7 +874,10 @@ 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);
> +	};

nit: style wise we often go for a one-liner like:

eval { delete_mountpoint_volume($storage_cfg, $vmid, $volume) }

(no hard feelings though, mostly commented due to already replying for the
commit subject anyway)

> +	die $@ if !$ignore_storage_errors && $@;
>      };
>      PVE::LXC::Config->foreach_volume_full($conf, {include_unused => 1}, $remove_volume);
>  

[pve-devel] [PATCH V2 pve-container 3/8] adapt behavior for detaching/removing a mount point

[Stefan Hrdlicka]

detach of a mount point with a removed underlying storage causes it to
be labeled as a an 'unused disk'
remove of a 'unused disk' with a removed underlying storage causes it to
be removed from the configuration

Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
---
 src/PVE/LXC/Config.pm | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index b4b0261..7917863 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -1423,7 +1423,10 @@ sub vmconfig_apply_pending {
 		    $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_exists($storecfg, $conf->{$opt})
+	    ) {
 		PVE::LXC::delete_mountpoint_volume($storecfg, $vmid, $conf->{$opt})
 		    if !$class->is_volume_in_use($conf, $conf->{$opt}, 1, 1);
 	    }
-- 
2.30.2

[pve-devel] [PATCH V2 pve-container 4/8] cleanup: remove spaces from empty lines

[Stefan Hrdlicka]

Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
---
 src/PVE/LXC.pm | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index e380b12..4e29e9b 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -668,7 +668,7 @@ sub update_lxc_config {
 
     # some init scripts expect a linux terminal (turnkey).
     $raw .= "lxc.environment = TERM=linux\n";
-    
+
     my $utsname = $conf->{hostname} || "CT$vmid";
     $raw .= "lxc.uts.name = $utsname\n";
 
@@ -1689,14 +1689,14 @@ sub __mountpoint_mount {
     my $type = $mountpoint->{type};
     my $quota = !$snapname && !$mountpoint->{ro} && $mountpoint->{quota};
     my $mounted_dev;
-    
+
     return if !$volid || !$mount;
 
     $mount =~ s!/+!/!g;
 
     my $mount_path;
     my ($mpfd, $parentfd, $last_dir);
-    
+
     if (defined($rootdir)) {
 	($rootdir, $mount_path, $mpfd, $parentfd, $last_dir) =
 	    __mount_prepare_rootdir($rootdir, $mount, $rootuid, $rootgid);
@@ -1705,7 +1705,7 @@ sub __mountpoint_mount {
     if (defined($stage_mount)) {
 	$mount_path = $rootdir;
     }
-    
+
     my ($storage, $volname) = PVE::Storage::parse_volume_id($volid, 1);
 
     die "unknown snapshot path for '$volid'" if !$storage && defined($snapname);
@@ -1814,7 +1814,7 @@ sub __mountpoint_mount {
 	warn "cannot enable quota control for bind mounts\n" if $quota;
 	return wantarray ? ($volid, 0, undef) : $volid;
     }
-    
+
     die "unsupported storage";
 }
 
-- 
2.30.2

[pve-devel] [PATCH V2 qemu-server 5/8] add ignore-storage-errors attribute for removing VM with missing storage

[Stefan Hrdlicka]

Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
---
 PVE/API2/Qemu.pm  |  8 ++++++++
 PVE/QemuServer.pm | 27 ++++++++++++++++++++-------
 2 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index d9ef201..e51f777 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -1852,6 +1852,13 @@ __PACKAGE__->register_method({
 		optional => 1,
 		default => 0,
 	    },
+	    'ignore-storage-errors' => {
+		type => 'boolean',
+		description => 'If set, this will ignore errors when trying to remove VM'
+		    . ' storage.',
+		default => 0,
+		optional => 1,
+	    },
 	},
     },
     returns => {
@@ -1908,6 +1915,7 @@ __PACKAGE__->register_method({
 		    $vmid,
 		    $skiplock, { lock => 'destroyed' },
 		    $purge_unreferenced,
+		    $param->{'ignore-storage-errors'},
 		);
 
 		PVE::AccessControl::remove_vm_access($vmid);
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index c706653..4900e15 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2295,7 +2295,12 @@ sub check_type {
 }
 
 sub destroy_vm {
-    my ($storecfg, $vmid, $skiplock, $replacement_conf, $purge_unreferenced) = @_;
+    my ($storecfg,
+	$vmid,
+	$skiplock,
+	$replacement_conf,
+	$purge_unreferenced,
+	$ignore_storage_errors) = @_;
 
     my $conf = PVE::QemuConfig->load_config($vmid);
 
@@ -2309,10 +2314,12 @@ 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 {
+		    $result = PVE::Storage::volume_is_base_and_used($storecfg, $volid)
+		};
+		die "Couldn't remove one or more disks: $@\n" if $@ && !$ignore_storage_errors;
+		die "base volume '$volid' is still in use by linked cloned\n" if $result;
 	});
     }
 
@@ -2338,7 +2345,10 @@ sub destroy_vm {
 	include_unused => 1,
 	extra_keys => ['vmstate'],
     };
-    PVE::QemuConfig->foreach_volume_full($conf, $include_opts, $remove_owned_drive);
+    eval {
+	PVE::QemuConfig->foreach_volume_full($conf, $include_opts, $remove_owned_drive);
+    };
+    die "Couldn't remove one or more disks: $@\n" if $@ && !$ignore_storage_errors;
 
     for my $snap (values %{$conf->{snapshots}}) {
 	next if !defined($snap->{vmstate});
@@ -2347,7 +2357,10 @@ sub destroy_vm {
 	$remove_owned_drive->('vmstate', $drive);
     }
 
-    PVE::QemuConfig->foreach_volume_full($conf->{pending}, $include_opts, $remove_owned_drive);
+    eval {
+	PVE::QemuConfig->foreach_volume_full($conf->{pending}, $include_opts, $remove_owned_drive);
+    };
+    die "Couldn't remove one or more disks: $@\n" if $@ && !$ignore_storage_errors;
 
     if ($purge_unreferenced) { # also remove unreferenced disk
 	my $vmdisks = PVE::Storage::vdisk_list($storecfg, undef, $vmid, undef, 'images');
-- 
2.30.2

[pve-devel] [PATCH V2 qemu-server 6/8] adapt behavior for detaching drives to deatching container mount points

[Stefan Hrdlicka]

if a storage is not available a volume will be added to the container
config as unused. before it would just disappear from the config

Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
---
 PVE/QemuServer.pm | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 4900e15..85b005e 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2006,7 +2006,10 @@ sub vmconfig_register_unused_drive {
 	warn $@ if $@;
     } elsif (!drive_is_cdrom($drive)) {
 	my $volid = $drive->{file};
-	if (vm_is_volid_owner($storecfg, $vmid, $volid)) {
+	if (!PVE::Storage::storage_exists($storecfg, $volid)) {
+	    # if storage is not in config, skip owner check and add as unused
+	    PVE::QemuConfig->add_unused_volume($conf, $volid, $vmid);
+	} elsif (vm_is_volid_owner($storecfg, $vmid, $volid)) {
 	    PVE::QemuConfig->add_unused_volume($conf, $volid, $vmid);
 	}
     }
-- 
2.30.2

[pve-devel] [PATCH V2 qemu-server 7/8] cleanup: shorten line

[Stefan Hrdlicka]

Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
---
 PVE/QemuServer.pm | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 85b005e..558e8a9 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2361,7 +2361,11 @@ sub destroy_vm {
     }
 
     eval {
-	PVE::QemuConfig->foreach_volume_full($conf->{pending}, $include_opts, $remove_owned_drive);
+	PVE::QemuConfig->foreach_volume_full(
+	    $conf->{pending},
+	    $include_opts,
+	    $remove_owned_drive
+	);
     };
     die "Couldn't remove one or more disks: $@\n" if $@ && !$ignore_storage_errors;
 
-- 
2.30.2

[pve-devel] [PATCH V2 pve-manager 8/8] fix #3711: enable removing container with non existent storage

[Stefan Hrdlicka]

Add a checkbox to the remove dialog of LXC containers and VMs to force
deleting a container/VM if the storage it uses has been removed.

Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
---
 www/manager6/lxc/Config.js              |  1 +
 www/manager6/qemu/Config.js             |  1 +
 www/manager6/window/SafeDestroyGuest.js | 34 +++++++++++++++++++++++++
 3 files changed, 36 insertions(+)

diff --git a/www/manager6/lxc/Config.js b/www/manager6/lxc/Config.js
index 89b59c9b..3a0acc0a 100644
--- a/www/manager6/lxc/Config.js
+++ b/www/manager6/lxc/Config.js
@@ -154,6 +154,7 @@ Ext.define('PVE.lxc.Config', {
 			    url: base_url,
 			    item: { type: 'CT', id: vmid },
 			    taskName: 'vzdestroy',
+			    showForceRemoveMissingStorage: true,
 			}).show();
 		    },
 		    iconCls: 'fa fa-trash-o',
diff --git a/www/manager6/qemu/Config.js b/www/manager6/qemu/Config.js
index 9fe933df..150835ed 100644
--- a/www/manager6/qemu/Config.js
+++ b/www/manager6/qemu/Config.js
@@ -129,6 +129,7 @@ Ext.define('PVE.qemu.Config', {
 			    url: base_url,
 			    item: { type: 'VM', id: vmid },
 			    taskName: 'qmdestroy',
+			    showForceRemoveMissingStorage: true,
 			}).show();
 		    },
 		    iconCls: 'fa fa-trash-o',
diff --git a/www/manager6/window/SafeDestroyGuest.js b/www/manager6/window/SafeDestroyGuest.js
index 3328293a..6cffa2bb 100644
--- a/www/manager6/window/SafeDestroyGuest.js
+++ b/www/manager6/window/SafeDestroyGuest.js
@@ -28,8 +28,37 @@ Ext.define('PVE.window.SafeDestroyGuest', {
 		'data-qtip': gettext('Scan all enabled storages for unreferenced disks and delete them.'),
 	    },
 	},
+	{
+	    xtype: 'proxmoxcheckbox',
+	    name: 'forceStorageRemove',
+	    reference: 'forceRemoveMissingStorage',
+	    boxLabel: gettext('Ignore missing/unavailable storage.'),
+	    checked: false,
+	    submitValue: false,
+	    hidden: true,
+	    autoEl: {
+		tag: 'div',
+		'data-qtip': gettext('Force remove container when storage is unavailable. ' +
+		    'The disk is not cleaned if storage still exists.'),
+	    },
+	},
     ],
 
+    config: {
+        showForceRemoveMissingStorage: false,
+    },
+
+    initComponent: function() {
+	let me = this;
+
+	me.callParent();
+
+	if (me.showForceRemoveMissingStorage) {
+	    let frms = me.lookupReference('forceRemoveMissingStorage');
+	    frms.hidden = !me.showForceRemoveMissingStorage;
+	}
+    },
+
     note: gettext('Referenced disks will always be destroyed.'),
 
     getParams: function() {
@@ -41,6 +70,11 @@ Ext.define('PVE.window.SafeDestroyGuest', {
 	const destroyUnreferencedCheckbox = me.lookupReference('destroyUnreferencedCheckbox');
 	me.params["destroy-unreferenced-disks"] = destroyUnreferencedCheckbox.checked ? 1 : 0;
 
+	if (me.showForceRemoveMissingStorage) {
+	    const forceRemoveMissingStorage = me.lookupReference('forceRemoveMissingStorage');
+	    me.params['ignore-storage-errors'] = forceRemoveMissingStorage.checked ? 1 : 0;
+	}
+
 	return me.callParent();
     },
 });
-- 
2.30.2