Re: [pve-devel] [PATCH container v6 3/4] fix #3711: lxc: allow removing unused mp if storage no longer exists

From: Fiona Ebner <f.ebner@proxmox.com>
To: "Proxmox VE development discussion" <pve-devel@lists.proxmox.com>,
	"Michael Köppl" <m.koeppl@proxmox.com>
Subject: Re: [pve-devel] [PATCH container v6 3/4] fix #3711: lxc: allow removing unused mp if storage no longer exists
Date: Tue, 20 May 2025 16:03:35 +0200	[thread overview]
Message-ID: <3cb1c0a2-19d2-4fdc-a8a7-b4a2835d423b@proxmox.com> (raw)
In-Reply-To: <20250520090818.44881-4-m.koeppl@proxmox.com>

Am 20.05.25 um 11:08 schrieb Michael Köppl:
> Detaching a mount point with a removed underlying storage causes it to
> be labeled as an unused disk. With this change, removing an unused disk
> with a removed underlying storage causes it to be removed from the
> configuration instead of failing. Deleting the mount point is skipped in
> this case to avoid failing and to allow removing the unused volume from
> the CT configuration and to achieve behavior consistent with destroying
> a CT.
> 
> Originally-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
>  src/PVE/LXC/Config.pm | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
> index a7c1023..8c96691 100644
> --- a/src/PVE/LXC/Config.pm
> +++ b/src/PVE/LXC/Config.pm
> @@ -1470,11 +1470,17 @@ sub vmconfig_hotplug_pending {
>      foreach my $opt (sort keys %$pending_delete_hash) {
>  	next if $selection && !$selection->{$opt};
>  	eval {
> +	    my $mp = $class->parse_volume($opt, $conf->{$opt});

You don't know that $opt is for a volume at this stage, it can be
anything, e.g. 'net0'.

> +	    my ($storeid, undef) = PVE::Storage::parse_volume_id($mp->{volume});
> +
>  	    if ($LXC_FASTPLUG_OPTIONS->{$opt}) {
>  		# pass
> -	    } 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);
> +		    if !$class->is_volume_in_use($conf, $mp->{volume}, 1, 1);
>  	    } elsif ($opt eq 'swap') {
>  		$hotplug_memory->(undef, 0);
>  	    } elsif ($opt eq 'cpulimit') {
> @@ -1558,13 +1564,17 @@ sub vmconfig_apply_pending {
>  	next if $selection && !$selection->{$opt};
>  	eval {
>  	    my $mp = $class->parse_volume($opt, $conf->{$opt});
> +	    my ($storeid, undef) = PVE::Storage::parse_volume_id($mp->{volume});
>  
>  	    if ($opt =~ m/^mp(\d+)$/) {
>  		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)
> +	    ) {
>  		# $mp->{volume} is used for is_volume_in_use() because parse_volume()
>  		# knows about 'unused*' and will return a valid volume ID whereas
>  		# $conf->{$opt} is not guaranteed to contain a valid volume ID in this

Can we put the parsing/check in delete_mountpoint_volume() itself
instead? And maybe print an informational message if the storage didn't
exist anymore. That would also cover the caller in patch 1/4, although
we still might want to use the eval+print there for other kinds of errors.

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