On April 6, 2022 1:46 pm, Aaron Lauterer wrote:
> If two RBD storages use the same pool, but connect to different
> clusters, we cannot say to which cluster the mapped RBD image belongs to
> if krbd is used. To avoid potential data loss, we need to verify that no
> other storage is configured that could have a volume mapped under the
> same path before we create the image.
>
> The ambiguous mapping is in
> /dev/rbd/<pool>/<ns>/<image> where the namespace <ns> is optional.
>
> Once we can tell the clusters apart in the mapping, we can remove these
> checks again.
>
> See bug #3969 for more information on the root cause.
>
> Signed-off-by: Aaron Lauterer <a.laute...@proxmox.com>
Acked-by: Fabian Grünbichler <f.gruenbich...@proxmox.com>
Reviewed-by: Fabian Grünbichler <f.gruenbich...@proxmox.com>
(small nit below, and given the rather heavy-handed approach a 2nd ack
might not hurt.. IMHO, a few easily fixable false-positives beat more
users actually running into this with move disk/volume and losing
data..)
> ---
> changes since
> v1:
> * fixed code style issues
> * moved check to a helper function and call it from
> - alloc_image
> - clone_image
> - rename_image
> * rephrased error message with a link to the bugzilla issue
>
> RFC:
> * moved check to pve-storage since containers and VMs both have issues
> not just on a move or clone of the image, but also when creating a new
> volume
> * reworked the checks, instead of large if conditions, we use
> PVE::Tools::safe_compare with comparison functions
> * normalize monhost list to match correctly if the list is in different
> order
> * add storage name to error message that triggered the checks
> * ignore disabled storages
>
> PVE/Storage/RBDPlugin.pm | 45 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
>
> diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
> index e287e28..2a4e1a8 100644
> --- a/PVE/Storage/RBDPlugin.pm
> +++ b/PVE/Storage/RBDPlugin.pm
> @@ -127,6 +127,45 @@ my $krbd_feature_update = sub {
> }
> };
>
> +# 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 $check_blockdev_collision = sub {
> + my ($storeid, $scfg) = @_;
parameter order is reversed compared to our pve-storage convention,
might be worthy of a fixup on application to match the rest:
my ($scfg, $storeid) = @_;
> +
> + my $storecfg = PVE::Storage::config();
> + foreach my $store (keys %{$storecfg->{ids}}) {
> + 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))) };
> + my $cmp_mons = sub { $normalize_mons->($_[0]) cmp
> $normalize_mons->($_[1]) };
> + my $cmp = sub { $_[0] cmp $_[1] };
> +
> + # 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;
> +
> + die "Cannot create volume on '$storeid' - RBD blockdev paths shared
> with storage '$store'. ".
> + "See https://bugzilla.proxmox.com/show_bug.cgi?id=3969 for more
> details.\n";
> + }
> +};
> +
> sub run_rbd_command {
> my ($cmd, %args) = @_;
>
> @@ -475,6 +514,8 @@ sub clone_image {
> my $snap = '__base__';
> $snap = $snapname if length $snapname;
>
> + $check_blockdev_collision->($storeid, $scfg);
> +
> my ($vtype, $basename, $basevmid, undef, undef, $isBase) =
> $class->parse_volname($volname);
>
> @@ -516,6 +557,8 @@ sub alloc_image {
> die "illegal name '$name' - should be 'vm-$vmid-*'\n"
> if $name && $name !~ m/^vm-$vmid-/;
>
> + $check_blockdev_collision->($storeid, $scfg);
> +
> $name = $class->find_free_diskname($storeid, $scfg, $vmid) if !$name;
>
> my @options = (
> @@ -769,6 +812,8 @@ sub volume_has_feature {
> sub rename_volume {
> my ($class, $scfg, $storeid, $source_volname, $target_vmid,
> $target_volname) = @_;
>
> + $check_blockdev_collision->($storeid, $scfg);
> +
> my (
> undef,
> $source_image,
> --
> 2.30.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
