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.
_______________________________________________
pve-devel mailing list
[email protected]
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
