Re: [pve-devel] [PATCH storage] rbd: alloc image: fix #3970 avoid ambiguous rbd path

[Aaron Lauterer <a.lauterer@proxmox.com>]

On 4/6/22 09:36, Fabian Grünbichler wrote:
> On April 5, 2022 2:40 pm, Aaron Lauterer wrote:
[...]
>>   
>> +    # check if another rbd storage with the same pool name but different
>> +    # cluster exists. If so, allocating a new volume can potentially be
>> +    # dangerous because the RBD mapping, exposes it in an ambiguous way under
>> +    # /dev/rbd/<pool>/<ns>/<image>. Without any information to which cluster it
>> +    # belongs, we cannot clearly determine which image we access and
>> +    # potentially use the wrong one. See
>> +    # https://bugzilla.proxmox.com/show_bug.cgi?id=3969 and
>> +    # https://bugzilla.proxmox.com/show_bug.cgi?id=3970
>> +    # TODO: remove these checks once #3969 is fixed and we can clearly tell to
>> +    # which cluster an image belongs to
>> +    my $storecfg = PVE::Storage::config();
>> +    foreach my $store  (keys %{$storecfg->{ids}}) {
> 
> I think this needs to go somewhere else - probably into a new private
> helper that gets called in alloc_image, clone_image and rename_image (at
> least those are the ones that currently call find_free_diskname).
> 
> basically all existing volids are as they are (they should be fine, else
> the user would probably already have noticed data loss/corruption), but
> anything that takes a new slot should be blocked before causing mayhem.

good point

> 
>> +	next if $store eq $storeid;
>> +
>> +	my $checked_scfg = $storecfg->{ids}->{$store};
>> +
>> +	next if $checked_scfg->{type} ne 'rbd';
>> +	next if $checked_scfg->{disable};
>> +	next if $scfg->{pool} ne $checked_scfg->{pool};
>> +
>> +	my $normalize_mons = sub { return join('/', sort( PVE::Tools::split_list(' ', shift))) };
> 
> this doesn't do what you think it does ;) split_list takes a single
> argument (the string to be split). I think joining with ';' might be
> more natural (it's basically a 'split->sort->join-as-string-list' then),
> and semicolons don't make any sense inside a monhost anyway.

thanks for catching it :)

> 
>> +	my $cmp_mons = sub { $normalize_mons->($_[0]) cmp $normalize_mons->($_[1]) };
>> +	my $cmp = sub { $_[0] cmp $_[1] };
> 
> that might be a nice addition to safe_compare (no $cmp -> use `cmp`),
> but alas.
> 
>> +	# internal and internal, or external and external with identical monitors
>> +	# => same cluster
>> +	next if PVE::Tools::safe_compare($scfg->{monhost}, $checked_scfg->{monhost}, $cmp_mons) == 0;
>> +
>> +	# different namespaces => no clash possible
>> +	next if !PVE::Tools::safe_compare($scfg->{namespace}, $checked_scfg->{namespace}, $cmp) == 0;
> 
> != 0 please!

yep :-/

> 
>> +
>> +	die "Other storage found which would lead to ambiguous mappings: '$store'\n";
> 
> it might make sense to include both storages here? e.g.:
> "Cannot create volume on '$storeid' - RBD blockdev paths shared with
> storage '$store'\n";
> 
> or even a reference to the bug that explains it all? could post a
> comment with workarounds as well then (although I do hope that not many
> people will run into this, and most of those are hopefully false
> positives of the check and not actually problematic setups).

hmm, a full on link to the bug in the error message? I tried to search via a few search engines for something like "proxmox bug #3969" and the results were not leading to bugzilla.proxmox.com. I don't think just adding the bug number will be that useful for most people.