Re: [pve-devel] [PATCH container v6 1/4] fix #3711: lxc: print warning if storage for mounted volume does not exist anymore

From: Thomas Lamprecht <t.lamprecht@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 1/4] fix #3711: lxc: print warning if storage for mounted volume does not exist anymore
Date: Thu, 22 May 2025 08:08:14 +0200	[thread overview]
Message-ID: <92f78ba0-44bb-41e9-b43b-9e4b0b47390b@proxmox.com> (raw)
In-Reply-To: <eb43d943-9094-4942-8bb5-75367214a2cf@proxmox.com>

Am 20.05.25 um 15:33 schrieb Fiona Ebner:
> Am 20.05.25 um 11:08 schrieb Michael Köppl:
>> An explicit check for the existence of the storage is added to print a
>> warning and continue with the removal of the container without deleting
>> the mount point in case the storage does not exist anymore. For other
>> errors, the function should still die.
>>
>> Originally-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
> 
> Nit: Ideally, you also describe the changes to the original patch here.
> For how this is usually done, see e.g.
> https://git.proxmox.com/?p=pve-container.git;a=commit;h=ee81952f4fc8faf01ed4eda5b8962d1a82d5425d
> 
>> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
>> ---
>>  src/PVE/LXC.pm | 12 +++++++++++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
>> index 2b9f0cf..6a1ce92 100644
>> --- a/src/PVE/LXC.pm
>> +++ b/src/PVE/LXC.pm
>> @@ -953,7 +953,17 @@ sub destroy_lxc_container {
>>  	return if $volids->{$volume};
>>  	$volids->{$volume} = 1;
>>  
>> -	delete_mountpoint_volume($storage_cfg, $vmid, $volume);
>> +	# explicitly check if storage still exists to avoid failing during
>> +	# deletion of the mountpoint volume. instead, only a warning is
>> +	# printed and destroying the container continues.

nit: in general: use the full width for comments (at least 80cc, 100c is totally fine
too).

But most of the comment reads as description for what happens, which is relatively
obvious from reading the code here, e.g. a "log_warn" call isn't exactly complex, but
rather telling on its own already.

While comments can really help, they mostly do when they state the things that are
not already obvious from reading the code in the local context already, like, e.g.,
"distant" effects or assumptions, or if it really is complex and there is not a
good way to simplify the code.

If one want's a comment here it probably would be enough to write something like:

# storages can be removed while volumes still exist, check that for better UX.

Note that your single comment is not a problem on it's own, but having a lot of
these makes reading code harder and as especially long comments describing the
code itself, and not the reasons, why's and other such rationale, tend to get
outdated fast, making it even more confusing to read.

That doesn't mean no comments though, but if, then please lets favor succinct
comments focusing on background, one or maybe two lines should be enough for most
code that benefits from having one. Exceptions naturally exist, e.g., if you write
some crypto code (please don't, as that's even hard to get right for field experts
with dozens of years of good experience, but just as example) then having more
comment than code would even be expected.

>> +	my ($storeid) = PVE::Storage::parse_volume_id($volume);
>> +	eval { PVE::Storage::storage_config($storage_cfg, $storeid) };
>> +	my $err = $@;
>> +	PVE::RESTEnvironment::log_warn("failed to delete $volume, $err") if $err;
>> +
>> +	if (!$err) {
>> +	    delete_mountpoint_volume($storage_cfg, $vmid, $volume);
>> +	}
> 
> Can we instead just surround the delete_mountpoint_volume() call itself
> with an eval + printing warning? That also catches other situations
> where deletion fails and is simpler.

Yeah, that would be nicer. As in 

eval {
    foo();
    bar();
}
# ... error handling

The bar method won't be called if foo dies.

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

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