[pve-devel] [PATCH pve-container 0/3] fix #3711: delete LXC container with missing storage

[pve-devel] [PATCH pve-container 0/3] fix #3711: delete LXC container with missing storage

[Stefan Hrdlicka]

The patch adds a new option 'force-remove-storage' that stops pct
destory from dying if the storage is not available. This also adds a
menu option for the delete dialog of containers.


Stefan Hrdlicka (2):
  fix 3711: enable delete of LXC container via force option
  fix #3711 cleanup: remove spaces from empty lines

# pve-container
 src/PVE/API2/LXC.pm |  8 ++++++++
 src/PVE/LXC.pm      | 30 ++++++++++++++++++++----------
 2 files changed, 28 insertions(+), 10 deletions(-)

# pve-manager
 www/manager6/lxc/Config.js              |  1 +
 www/manager6/window/SafeDestroyGuest.js | 34 +++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

-- 
2.30.2

[pve-devel] [PATCH pve-container 1/3] fix #3711: enable delete of LXC container via force option

[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 force-remove-storage=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.

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

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 589f96f..4d785c9 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,
 	    },
+	    'force-remove-storage' => {
+		type => 'boolean',
+		description => 'If set, this will ignore errors when trying to remove LXC'
+		    . ' container storage.',
+		default => 0,
+		optional => 1,
+	    }
 	},
     },
     returns => {
@@ -758,6 +765,7 @@ __PACKAGE__->register_method({
 		$conf,
 		{ lock => 'destroyed' },
 		$param->{'destroy-unreferenced-disks'},
+		$param->{'force-remove-storage'},
 	    );
 
 	    PVE::AccessControl::remove_vm_access($vmid);
diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index fe63087..74c8d17 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -848,13 +848,22 @@ sub get_primary_ips {
 }
 
 sub delete_mountpoint_volume {
-    my ($storage_cfg, $vmid, $volume) = @_;
+    my ($storage_cfg, $vmid, $volume, $force_remove_storage) = @_;
 
     return if PVE::LXC::Config->classify_mountpoint($volume) ne 'volume';
 
-    my ($vtype, $name, $owner) = PVE::Storage::parse_volname($storage_cfg, $volume);
+    my ($vtype, $name, $owner);
+    eval {
+	($vtype, $name, $owner) = PVE::Storage::parse_volname($storage_cfg, $volume);
+    };
 
-    if ($vmid == $owner) {
+    if (!$force_remove_storage && $@) {
+	die $@;
+    } elsif ($@) {
+	# $force_remove_storage == true is implied here
+	warn "storage not available, can't remove $volume disk image, continuing!\n"
+	    . "error: $@\n";
+    } elsif ($vmid == $owner) {
 	PVE::Storage::vdisk_free($storage_cfg, $volume);
     } else {
 	warn "ignore deletion of '$volume', CT $vmid isn't the owner!\n";
@@ -862,7 +871,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, $force_remove_storage) = @_;
 
     my $volids = {};
     my $remove_volume = sub {
@@ -873,7 +883,7 @@ sub destroy_lxc_container {
 	return if $volids->{$volume};
 	$volids->{$volume} = 1;
 
-	delete_mountpoint_volume($storage_cfg, $vmid, $volume);
+	delete_mountpoint_volume($storage_cfg, $vmid, $volume, $force_remove_storage);
     };
     PVE::LXC::Config->foreach_volume_full($conf, {include_unused => 1}, $remove_volume);
 
-- 
2.30.2

Re: [pve-devel] [PATCH pve-container 1/3] fix #3711: enable delete of LXC container via force option

[Wolfgang Bumiller]

On Wed, Jul 20, 2022 at 04:49:47PM +0200, Stefan Hrdlicka wrote:
> Make it possible to delete a container whoes underlying storage is no
> longer available. This will just write an warning instead of dying.

"no longer available" != "throws errors" (see below (*))

> 
> Without setting the option force-remove-storage=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.
> 
> Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
> ---
>  src/PVE/API2/LXC.pm |  8 ++++++++
>  src/PVE/LXC.pm      | 20 +++++++++++++++-----
>  2 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index 589f96f..4d785c9 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,
>  	    },
> +	    'force-remove-storage' => {

This name is a bit confusing as instead of enforcing removal you're
actually ignoring the case where removal *fails*, iow.: you allow *not*
removing data.

The `create_vm` call has an `ignore-unpack-errors` parameter, so maybe
`ignore-storage-errors` would work here. (And renaming all the
$variables accordingly.)

> +		type => 'boolean',
> +		description => 'If set, this will ignore errors when trying to remove LXC'

(*) when documenting it as ignoring errors, I would not expect it to
distinguish between unavailable storages and _other_ errors happening.

Side note: Almost all our API docs just refer to them as 'containers',
the 'LXC' portion can be dropped here.

> +		    . ' container storage.',
> +		default => 0,
> +		optional => 1,
> +	    }
>  	},
>      },
>      returns => {
> @@ -758,6 +765,7 @@ __PACKAGE__->register_method({
>  		$conf,
>  		{ lock => 'destroyed' },
>  		$param->{'destroy-unreferenced-disks'},
> +		$param->{'force-remove-storage'},
>  	    );
>  
>  	    PVE::AccessControl::remove_vm_access($vmid);
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index fe63087..74c8d17 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -848,13 +848,22 @@ sub get_primary_ips {
>  }
>  
>  sub delete_mountpoint_volume {
> -    my ($storage_cfg, $vmid, $volume) = @_;
> +    my ($storage_cfg, $vmid, $volume, $force_remove_storage) = @_;
>  
>      return if PVE::LXC::Config->classify_mountpoint($volume) ne 'volume';
>  
> -    my ($vtype, $name, $owner) = PVE::Storage::parse_volname($storage_cfg, $volume);
> +    my ($vtype, $name, $owner);
> +    eval {
> +	($vtype, $name, $owner) = PVE::Storage::parse_volname($storage_cfg, $volume);
> +    };

^ It is not clear to me why you'd cover this, but not the `vdisk_free`
below, unless you're trying to catch only specific errors (but this is
not specific enough...)

I think this sub can be left unchanged and instead the
`delete_mountpoint_volume()` call itself could be wrapped in an
`eval{}` instead.

>  
> -    if ($vmid == $owner) {
> +    if (!$force_remove_storage && $@) {
> +	die $@;
> +    } elsif ($@) {
> +	# $force_remove_storage == true is implied here
> +	warn "storage not available, can't remove $volume disk image, continuing!\n"
> +	    . "error: $@\n";
> +    } elsif ($vmid == $owner) {
>  	PVE::Storage::vdisk_free($storage_cfg, $volume);
>      } else {
>  	warn "ignore deletion of '$volume', CT $vmid isn't the owner!\n";
> @@ -862,7 +871,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, $force_remove_storage) = @_;
>  
>      my $volids = {};
>      my $remove_volume = sub {
> @@ -873,7 +883,7 @@ sub destroy_lxc_container {
>  	return if $volids->{$volume};
>  	$volids->{$volume} = 1;
>  
> -	delete_mountpoint_volume($storage_cfg, $vmid, $volume);
> +	delete_mountpoint_volume($storage_cfg, $vmid, $volume, $force_remove_storage);

So I think I'd just put an `eval{}` here, not pass the parameter
through, and just `die $@ if $@ && !$ignore_storage_errrors` afterwards.

>      };
>      PVE::LXC::Config->foreach_volume_full($conf, {include_unused => 1}, $remove_volume);
>  
> -- 
> 2.30.2

[pve-devel] [PATCH pve-container 2/3] fix #3711 cleanup: remove spaces from empty lines

[Stefan Hrdlicka]

remove spaces where they are not needed

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 74c8d17..42d94ac 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";
 
@@ -1695,14 +1695,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);
@@ -1711,7 +1711,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);
@@ -1820,7 +1820,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

Re: [pve-devel] [PATCH pve-container 2/3] fix #3711 cleanup: remove spaces from empty lines

[Wolfgang Bumiller]

nit: the subject shouldn't have the `fix #3711` included as this patch
is not directly affecting it ;-)

On Wed, Jul 20, 2022 at 04:49:48PM +0200, Stefan Hrdlicka wrote:
> remove spaces where they are not needed
> 
> 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 74c8d17..42d94ac 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";
>  
> @@ -1695,14 +1695,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);
> @@ -1711,7 +1711,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);
> @@ -1820,7 +1820,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 pve-manager 3/3] fix #3711: enable removing container with non existent storage

[Stefan Hrdlicka]

Add a checkbox to the remove dialog of LXC containers to force
deleting a container 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/window/SafeDestroyGuest.js | 34 +++++++++++++++++++++++++
 2 files changed, 35 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/window/SafeDestroyGuest.js b/www/manager6/window/SafeDestroyGuest.js
index 3328293a..2fc4ff26 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['force-remove-storage'] = forceRemoveMissingStorage.checked ? 1 : 0;
+	}
+
 	return me.callParent();
     },
 });
-- 
2.30.2

Re: [pve-devel] [PATCH pve-container 0/3] fix #3711: delete LXC container with missing storage

[Fiona Ebner]

Am 20.07.22 um 16:49 schrieb Stefan Hrdlicka:
> The patch adds a new option 'force-remove-storage' that stops pct
> destory from dying if the storage is not available. This also adds a
> menu option for the delete dialog of containers.
> 

VMs are also affected, so we probably want the new option there too.
Although for VMs, it is possible to work around the issue by detaching
all non-existing disks first.

So slightly related: when detaching a disk and the owner of the volume
is different (it also happens when the storage/disk does not exist
anymore), we drop the disk for VMs, but we register it as unused for
containers. Should we make that consistent?

Re: [pve-devel] [PATCH pve-container 0/3] fix #3711: delete LXC container with missing storage

[Wolfgang Bumiller]

On Mon, Jul 25, 2022 at 12:40:21PM +0200, Fiona Ebner wrote:
> Am 20.07.22 um 16:49 schrieb Stefan Hrdlicka:
> > The patch adds a new option 'force-remove-storage' that stops pct
> > destory from dying if the storage is not available. This also adds a
> > menu option for the delete dialog of containers.
> > 
> 
> VMs are also affected, so we probably want the new option there too.
> Although for VMs, it is possible to work around the issue by detaching
> all non-existing disks first.

Yeah, this is really mostly an issue with the `rootfs`, since you cannot
detach or remove it, so I think we could have this for VMs just for
consistency's sake.

> So slightly related: when detaching a disk and the owner of the volume
> is different (it also happens when the storage/disk does not exist
> anymore), we drop the disk for VMs, but we register it as unused for
> containers. Should we make that consistent?

Yeah I think consistency makes more sense there as well.

Re: [pve-devel] [PATCH pve-container 0/3] fix #3711: delete LXC container with missing storage

[Stefan Hrdlicka]

On 7/25/22 15:31, Wolfgang Bumiller wrote:
> On Mon, Jul 25, 2022 at 12:40:21PM +0200, Fiona Ebner wrote:
>> Am 20.07.22 um 16:49 schrieb Stefan Hrdlicka:
>>> The patch adds a new option 'force-remove-storage' that stops pct
>>> destory from dying if the storage is not available. This also adds a
>>> menu option for the delete dialog of containers.
>>>
>>
>> VMs are also affected, so we probably want the new option there too.
>> Although for VMs, it is possible to work around the issue by detaching
>> all non-existing disks first.
> 
> Yeah, this is really mostly an issue with the `rootfs`, since you cannot
> detach or remove it, so I think we could have this for VMs just for
> consistency's sake.
> 
>> So slightly related: when detaching a disk and the owner of the volume
>> is different (it also happens when the storage/disk does not exist
>> anymore), we drop the disk for VMs, but we register it as unused for
>> containers. Should we make that consistent?
> 
> Yeah I think consistency makes more sense there as well.

Consistent in that case would be that we also register the disk as 
unused for the VMs as well, instead of dropping it?