[pve-devel] [PATCH SERIES V3 pve-container/qemu-server/pve-manager 0/6] fix #3711 & adapt drive detach/remove behavior

[pve-devel] [PATCH SERIES V3 pve-container/qemu-server/pve-manager 0/6] 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


V2 -> V3:
# pve-storage
* change removed

# pve-container
* remove storage_exists
* reduce code lines used

# qemu-server
* reduce code lines used

# pve-manager
* no change

-- 
2.30.2

[pve-devel] [PATCH V3 pve-container 1/6] fix #3711: optionally allow CT deletion to complete on disk volume removal errors

[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      | 6 ++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 79aecaa..19806fa 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -700,6 +700,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 => {
@@ -761,6 +768,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 4bbd739..fe68f75 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -861,7 +861,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 {
@@ -872,7 +873,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); };
+	die $@ if !$ignore_storage_errors && $@;
     };
     PVE::LXC::Config->foreach_volume_full($conf, {include_unused => 1}, $remove_volume);
 
-- 
2.30.2

[pve-devel] [PATCH V3 pve-container 2/6] 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 | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index b1f779b..e8fbd86 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -1421,13 +1421,17 @@ sub vmconfig_apply_pending {
     foreach my $opt (sort keys %$pending_delete_hash) {
 	next if $selection && !$selection->{$opt};
 	eval {
+	    my ($storeid, $volname) = PVE::Storage::parse_volume_id($conf->{$opt});
 	    if ($opt =~ m/^mp(\d+)$/) {
 		my $mp = $class->parse_volume($opt, $conf->{$opt});
 		if ($mp->{type} eq 'volume') {
 		    $class->add_unused_volume($conf, $mp->{volume})
 			if !$class->is_volume_in_use($conf, $conf->{$opt}, 1, 1);
 		}
-	    } elsif ($opt =~ m/^unused(\d+)$/) {
+	    } elsif (
+		$opt =~ m/^unused(\d+)$/
+		&& PVE::Storage::storage_config($storecfg, $storeid, 1)
+	    ) {
 		PVE::LXC::delete_mountpoint_volume($storecfg, $vmid, $conf->{$opt})
 		    if !$class->is_volume_in_use($conf, $conf->{$opt}, 1, 1);
 	    }
-- 
2.30.2

[pve-devel] [PATCH V3 pve-container 3/6] 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 fe68f75..635cf44 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";
 
@@ -1690,14 +1690,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);
@@ -1706,7 +1706,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);
@@ -1815,7 +1815,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 V3 qemu-server 4/6] add ignore-storage-errors 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 | 23 ++++++++++++++++-------
 2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 30348e6..2a0806f 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -1982,6 +1982,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 => {
@@ -2038,6 +2045,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 51e9a51..e68d8b4 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2327,7 +2327,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);
 
@@ -2341,10 +2346,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 { $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;
 	});
     }
 
@@ -2370,7 +2375,8 @@ 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});
@@ -2379,7 +2385,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 V3 qemu-server 5/6] 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 | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index e68d8b4..886ac92 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2038,7 +2038,11 @@ sub vmconfig_register_unused_drive {
 	delete $conf->{cloudinit};
     } elsif (!drive_is_cdrom($drive)) {
 	my $volid = $drive->{file};
-	if (vm_is_volid_owner($storecfg, $vmid, $volid)) {
+	my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid);
+	if (!PVE::Storage::storage_config($storecfg, $storeid, 1)) {
+	    # 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 V3 pve-manager 6/6] fix #3711: optionally allow CT deletion to complete on disk volume removal errors

[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 93f385db..a00e1927 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

Re: [pve-devel] [PATCH V3 qemu-server 4/6] add ignore-storage-errors for removing VM with missing storage

[Fiona Ebner]

Am 15.11.22 um 11:55 schrieb Stefan Hrdlicka:
> @@ -2341,10 +2346,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 { $result = PVE::Storage::volume_is_base_and_used($storecfg, $volid) };
> +		die "Couldn't remove one or more disks: $@\n" if $@ && !$ignore_storage_errors;

This error message is wrong. The check failed, not the removal. The
check should be repeated in vdisk_free anyways and you should get the
appropriate error then below :)

AFAIU base volumes should still survive if they are still referenced by
linked clones, even when ignore-storage-errors is used (IMHO good). Is
that correct?

Nothing new and not directly related:
I noticed that for containers, we don't have this heads-up check. Maybe
worth adding there too? Arguably minor issue is that I can have a
container template with a disk on lvm-thin and a second disk on
non-lvm-thin. Even if there is a linked clone, removing the template
might remove the lvm-thin disk, and then fail, because the second disk
is referenced.

> +		die "base volume '$volid' is still in use by linked cloned\n" if $result;
>  	});
>      }
>  
> @@ -2370,7 +2375,8 @@ 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;

So, $removed_owned_drive already ignores all storage errors beside if
PVE::Storage::path() fails right? Can't we just add an eval around that
and be done? No need for a new ignore-storage-errors parameter. Most
storage errors are already ignored even without that parameter right
now! I don't think it's a big issue to start ignoring the few missing
ones as well?

>  
>      for my $snap (values %{$conf->{snapshots}}) {
>  	next if !defined($snap->{vmstate});

Re: [pve-devel] [PATCH V3 qemu-server 4/6] add ignore-storage-errors for removing VM with missing storage

[Stefan Hrdlicka]

On 11/15/22 13:17, Fiona Ebner wrote:
> Am 15.11.22 um 11:55 schrieb Stefan Hrdlicka:
>> @@ -2341,10 +2346,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 { $result = PVE::Storage::volume_is_base_and_used($storecfg, $volid) };
>> +		die "Couldn't remove one or more disks: $@\n" if $@ && !$ignore_storage_errors;
> 
> This error message is wrong. The check failed, not the removal. The
> check should be repeated in vdisk_free anyways and you should get the
> appropriate error then below :)

yes :).

> 
> AFAIU base volumes should still survive if they are still referenced by
> linked clones, even when ignore-storage-errors is used (IMHO good). Is
> that correct?

Yes this is correct, the volumes still exist.

> 
> Nothing new and not directly related:
> I noticed that for containers, we don't have this heads-up check. Maybe
> worth adding there too? Arguably minor issue is that I can have a
> container template with a disk on lvm-thin and a second disk on
> non-lvm-thin. Even if there is a linked clone, removing the template
> might remove the lvm-thin disk, and then fail, because the second disk
> is referenced.

maybe a good idea, I will look into that

> 
>> +		die "base volume '$volid' is still in use by linked cloned\n" if $result;
>>   	});
>>       }
>>   
>> @@ -2370,7 +2375,8 @@ 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;
> 
> So, $removed_owned_drive already ignores all storage errors beside if
> PVE::Storage::path() fails right? Can't we just add an eval around that
> and be done? No need for a new ignore-storage-errors parameter. Most
> storage errors are already ignored even without that parameter right
> now! I don't think it's a big issue to start ignoring the few missing
> ones as well?

Well I wasn't sure. Safe option was to tell the user that there is a 
problem and then the user decides if something should be forced deleted. 
But if you think this is fine without user input lets pretend this never 
existed :).

> 
>>   
>>       for my $snap (values %{$conf->{snapshots}}) {
>>   	next if !defined($snap->{vmstate});

Re: [pve-devel] [PATCH V3 qemu-server 4/6] add ignore-storage-errors for removing VM with missing storage

[Fiona Ebner]

Am 15.11.22 um 15:22 schrieb Stefan Hrdlicka:
> On 11/15/22 13:17, Fiona Ebner wrote:
>> Am 15.11.22 um 11:55 schrieb Stefan Hrdlicka:
>>>   @@ -2370,7 +2375,8 @@ 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;
>>
>> So, $removed_owned_drive already ignores all storage errors beside if
>> PVE::Storage::path() fails right? Can't we just add an eval around that
>> and be done? No need for a new ignore-storage-errors parameter. Most
>> storage errors are already ignored even without that parameter right
>> now! I don't think it's a big issue to start ignoring the few missing
>> ones as well?
> 
> Well I wasn't sure. Safe option was to tell the user that there is a
> problem and then the user decides if something should be forced deleted.
> But if you think this is fine without user input lets pretend this never
> existed :).
> 
When there are storage errors, the VM config is removed and the user is
warned about failed volume removal, *except* when the storage errors are
in path() (which can only happen if the storage is gone from the config
or in very few plugins like RBD), where it leads to the VM config
removal failing.

But I don't see why errors in path() should be fundamentally different
here and feel like we should just align the behavior for those too.
After all, that was the intention behind the eval block + warning around
vdisk_free() in commit 31b52247 ("destroy_vm: allow vdisk_free to fail")

To make the warnings more visible, we can switch to log_warn() instead
of plain warn.

For containers, all storage errors just propagate right now (or?), so
the new parameter can be fine there. We could also align the behavior
with what we do for VMs instead (if we do that, it should be done for a
point release), but we don't have to of course.