From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id E6CE2A2A5 for ; Mon, 4 Apr 2022 17:45:14 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D8B8C10254 for ; Mon, 4 Apr 2022 17:45:14 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id CC2961023B for ; Mon, 4 Apr 2022 17:45:13 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 95BD341904 for ; Mon, 4 Apr 2022 17:45:13 +0200 (CEST) Date: Mon, 04 Apr 2022 17:44:57 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20220401152424.3811621-1-a.lauterer@proxmox.com> In-Reply-To: <20220401152424.3811621-1-a.lauterer@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1649086015.n05il0cmo5.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.177 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pve-devel] [RFC container] alloc disk: fix #3970 avoid ambiguous rbd image path X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 Apr 2022 15:45:15 -0000 On April 1, 2022 5:24 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. 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 format anything. >=20 > The ambiguous mapping is in > /dev/rbd/// where the namespace is optional. >=20 > Once we can tell the clusters apart in the mapping, we can remove these > checks again. >=20 > See bug #3969 for more information on the root cause. >=20 > Signed-off-by: Aaron Lauterer > --- > RFC because I would like someone else to take a look at the logic and I > wasn't sure how to format the grouping of the conditions in a way that > is easy to read >=20 > src/PVE/LXC.pm | 45 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 45 insertions(+) >=20 > diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm > index fe63087..b048ce0 100644 > --- a/src/PVE/LXC.pm > +++ b/src/PVE/LXC.pm > @@ -1970,6 +1970,51 @@ sub alloc_disk { > my $scfg =3D PVE::Storage::storage_config($storecfg, $storage); > # fixme: use better naming ct-$vmid-disk-X.raw? > =20 > + # 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///. Without any information to which clu= ster it > + # belongs, we cannot clearly determine which image we access and > + # potentially format an already used image. See > + # https://bugzilla.proxmox.com/show_bug.cgi?id=3D3969 and > + # https://bugzilla.proxmox.com/show_bug.cgi?id=3D3970 > + # TODO: remove these checks once #3969 is fixed and we can clearly t= ell to > + # which cluster an image belongs to > + if ($scfg->{type} eq 'rbd') { > + my $pool =3D $storecfg->{ids}->{$storage}->{pool}; not used anywhere? > + foreach my $stor (keys %{$storecfg->{ids}}) { > + next if $stor eq $storage; > + > + my $ccfg =3D PVE::Storage::storage_config($storecfg, $stor); that's actually not needed if you are iterating over the keys ;) just=20 use my $checked_scfg =3D $storecfg->{ids}->{$store}; > + next if $ccfg->{type} ne 'rbd'; > + > + if ($scfg->{pool} eq $ccfg->{pool}) { why not simply # different pools -> no clash possible next if $scfg->{pool} ne $checked_scfg->{pool}; > + if ( > + ( > + defined($scfg->{monhost}) > + && defined($ccfg->{monhost}) > + && $scfg->{monhost} eq $ccfg->{monhost} > + ) > + || ( > + !defined($scfg->{monhost}) > + && !defined($ccfg->{monhost}) > + ) > + ) { > + # both external ones same or both hyperconverged > + next; untested, but something like the following is probably more readable ;)=20 could also be adapted to check for monhost overlap instead if desired=20 (to catch storage A and B not having identical=20 strings/formatting/elements - if any single mon is listed for both, they=20 ARE the same cluster for all intents and purposes?) my $safe_compare =3D sub { my ($key) =3D shift; my $cmp =3D sub { $_[0] eq $_[1] }; return PVE::Tools::safe_compare($scfg->{$key}, $checked_scfg->{$key}, $cm= p); }; # internal and internal or external and external with identical monitors=20 # =3D> same cluster next if $safe_compare->("monhost"); # different namespaces =3D> no clash possible next if !$safe_compare->("namespace"); die .. > + } > + # different cluster here > + # we are ok if namespaces are not the same or only one storage has one > + if (defined($scfg->{namespace}) && defined($ccfg->{namespace})) { > + next if $scfg->{namespace} ne $ccfg->{namespace}; > + } elsif (defined($scfg->{namespace}) || defined($ccfg->{namespace})) { > + next; > + } > + die "Cannot determine which Ceph cluster the volume mapping belongs to= . Abort!\n"; > + } > + } > + } > + > eval { > my $do_format =3D 0; > if ($scfg->{content}->{rootdir} && $scfg->{path}) { > --=20 > 2.30.2 >=20 >=20 >=20 > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >=20 >=20 >=20