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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 6633D99B93 for ; Wed, 3 May 2023 11:08:09 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 49CD6861E for ; Wed, 3 May 2023 11:08:09 +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) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 3 May 2023 11:08:07 +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 5D4174712C for ; Wed, 3 May 2023 11:08:07 +0200 (CEST) Date: Wed, 03 May 2023 11:07:59 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20230502131732.1875692-1-a.lauterer@proxmox.com> <20230502131732.1875692-3-a.lauterer@proxmox.com> In-Reply-To: <20230502131732.1875692-3-a.lauterer@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1683103216.2rppe57tnl.astroid@yuna.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.075 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy 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 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox.com, migrate.pm] Subject: Re: [pve-devel] [PATCH container 2/2] migration: avoid migrating volume images multiple times 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: Wed, 03 May 2023 09:08:09 -0000 On May 2, 2023 3:17 pm, Aaron Lauterer wrote: > Scan the VM config and store the volid and full path for each storage. > Do the same when we scan each storage. Then we can have these > scenarios: > * multiple storage configurations might point to the same storage > The result is, that when scanning the storages, we find the volume > multiple times. > -> we ignore them >=20 > * a VM might have multiple volumes configured, pointing to the same > volume > -> We fail with a warning that two mpX configs point to the same > volume this one really doesn't make much sense for containers (if the volume contains the default ext4, MMP means the container can't be started?) - it could be handled though as well (just migrate the volume once, and replace the references with the target volid?) >=20 > Without these checks, it was possible to multiply the number of volumes > with each migration (with local disk) if at least another storage was > configured, pointing to the same place. >=20 > Signed-off-by: Aaron Lauterer > --- > src/PVE/LXC/Migrate.pm | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) >=20 > diff --git a/src/PVE/LXC/Migrate.pm b/src/PVE/LXC/Migrate.pm > index ccf5157..47cb94b 100644 > --- a/src/PVE/LXC/Migrate.pm > +++ b/src/PVE/LXC/Migrate.pm > @@ -256,6 +256,28 @@ sub phase1 { > &$log_error($@, $volid) if $@; > }; > =20 > + # store and map already referenced absolute paths and volids > + my $referencedpath =3D {}; # path -> volid wrong name - this contains both referenced and unreferenced paths (and also, long variable names would benefit from _ for separation of words) > + my $referenced =3D {}; # volid -> config key (e.g. mp0) also wrong - contains both referenced and unreferenced volids, and also doesn't always map to the config key ;) but before diving further into nit territory - we already have $volhash that is filled for all volumes, wouldn't it make more sense to re-use it? the order is inverse compared to your patch here, but that doesn't really matter (and it would avoid iterating over the config twice). compared to your patch, it also covers snapshots (everything in snapshots should also be in the current config either still used or as unusedX, but who knows). there are three things we'd need to do: - in $test_volid, if 'ref' is already config, error out (same volid referenced in current config multiple times) - add the path of each volid to the volhash while filling it - after $volhash is filled, iterate over it and check for aliased paths (and depending on where the volid comes from, warn and ignore or error out) or am I missing something? alternatively, instead of putting the path into volhash, it could be tracked separately (e.g. in a $path_to_volid hash) and checked directly while iterating instead of at the end. > + > + # reference volumes in config first > + PVE::LXC::Config->foreach_volume_full($conf, { include_unused =3D> 1= }, sub { > + my ($key, $volume) =3D @_; > + my $volid =3D $volume->{volume}; > + my $path =3D PVE::Storage::path($self->{storecfg}, $volid); > + if (defined $referencedpath->{$path}) { > + my $rkey =3D $referenced->{$referencedpath->{$path}}; > + &$log_error( > + "'$key' and '$rkey' reference the same volume. ". > + "(check guest and storage configuration?)\n", > + $volid > + ); > + return; > + } > + $referencedpath->{$path} =3D $volid; > + $referenced->{$volid} =3D $key; > + }); > + > # first unused / lost volumes owned by this container > my @sids =3D PVE::Storage::storage_ids($self->{storecfg}); > foreach my $storeid (@sids) { > @@ -280,6 +302,15 @@ sub phase1 { > PVE::Storage::foreach_volid($dl, sub { > my ($volid, $sid, $volname) =3D @_; > =20 > + # check if volume is already referenced > + my $path =3D PVE::Storage::path($self->{storecfg}, $volid); > + if (defined $referencedpath->{$path} && !$referenced->{$volid}) { > + $self->log('info', "ignoring '$volid' - already referenced by other st= orage '$referencedpath->{$path}'\n"); > + next; > + } > + $referencedpath->{$path} =3D $volid; > + $referenced->{$volid} =3D 1; > + > $volhash->{$volid}->{ref} =3D 'storage'; > $volhash->{$volid}->{targetsid} =3D $targetsid; > }); > --=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