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

From: "Michael Köppl" <m.koeppl@proxmox.com>
To: Fiona Ebner <f.ebner@proxmox.com>,
	Proxmox VE development discussion <pve-devel@lists.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, 27 May 2025 11:34:17 +0200	[thread overview]
Message-ID: <8f515019-1d9d-4760-a027-4b7210d7f37d@proxmox.com> (raw)
In-Reply-To: <3cb1c0a2-19d2-4fdc-a8a7-b4a2835d423b@proxmox.com>

On 5/20/25 16:03, Fiona Ebner wrote:
>> @@ -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.

I think moving the check if the storage exists inside definitely makes
sense here. Thanks for the suggestion. I adapted the
delete_mountpoint_volume() for a v7, but opted to keep the parsing of
the volume ID in vmconfig_apply_pending() and
vmconfig_hotplug_pending(). destroy_lxc_container() uses
foreach_volume_full() to iterate over all its mountpoints, which uses
parse_volume() internally. So the $volume input used there is already
what we want, as opposed to the $conf and $opt arguments used by the
other 2 callers. Moving the parsing into delete_mountpoint_volume()
would require changing other parts of the implementation (to get the
required inputs for all 3 callers) for little benefit. One way to avoid
that would be a signature such as

	   my ($storage_cfg, $vmid, $volid, $conf, $opt) = @_;

and using $conf->{$opt} to call parse_volume() if $volid is undef. But I
think that's not very transparent to the caller.

Also, I don't know if you meant the is_volume_in_use() checks as well,
but I think keeping those where they are makes it very obvious from the
code that the delete only happens if the volume is not in use.

Would love your input on this, maybe I'm just missing something.

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