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

[Fiona Ebner <f.ebner@proxmox.com>]

Am 27.05.25 um 11:34 schrieb Michael Köppl:
> 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().

As we had discussed off-list, that check should even go in vdisk_free()
itself, not delete_mountpoint_volume(). And regarding the parsing, that
was an oversight on my part because the parameter is confusingly named
$volume, but it's already a volume ID. We already had discussed this too ;)

> 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

Yes, the $volume variable there is also just confusingly named, as it's
also already a volume ID.

> 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.

We don't need to adapt the signature of delete_mountpoint_volume(),
except renaming the argument $volume => $volid would be worthwhile.

> 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.

No, I didn't mean those.


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