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 3BAB069320 for ; Mon, 22 Mar 2021 11:02:12 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 2899021C8F for ; Mon, 22 Mar 2021 11:01:42 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (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 id 35CF021C84 for ; Mon, 22 Mar 2021 11:01:41 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id F340B428AC for ; Mon, 22 Mar 2021 11:01:40 +0100 (CET) Date: Mon, 22 Mar 2021 11:01:33 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Fabian Ebner , pve-devel@lists.proxmox.com References: <20210319134908.10558-1-f.ebner@proxmox.com> <20210319134908.10558-2-f.ebner@proxmox.com> <1616162913.ti6lv8kl41.astroid@nora.none> In-Reply-To: MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1616406650.sq238n1ivv.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL -0.373 Adjusted score from AWL reputation of From: address KAM_ASCII_DIVIDERS 0.8 Spam that uses ascii formatting tricks KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH qemu-server 2/3] migrate: always check if content type images is available for target storage 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, 22 Mar 2021 10:02:12 -0000 On March 22, 2021 9:56 am, Fabian Ebner wrote: > Am 19.03.21 um 15:16 schrieb Fabian Gr=C3=BCnbichler: >> On March 19, 2021 2:49 pm, Fabian Ebner wrote: >>> it's cheap and saves code. >>=20 >> but also changes behaviour in a non-backwards-compatible fashion. >>=20 >> previously, if a disk was already on a storage that does not have images >> configured, and the migration leaves it on that storage, this config >> mismatch was ignored (hence the "grandfather in existing mismatches" >> comment). note that users might be able to migrate, but not able to >> change storage.cfg to fix this "misconfiguration". >>=20 >=20 > What about the recent change [0] in pve-storage then, i.e. not listing=20 > VM disks for storages without an appropriate content type? I'd have to think more about that (I mean, it obviously makes sense, the=20 question is whether it breaks something ;)) >=20 > I'd argue that unreferenced images that lie on storages without an=20 > 'image' content type should not be picked up in the first place. If we=20 > don't agree on this, then [0] needs to be reverted... but this is not just about unreferenced disks? I mean, this code runs=20 for ALL disks returned by pve-storage. if pve-storage now filters, then=20 this does not apply for the '"old =3D=3D new" storage which has wrong conte= nt=20 type' scenario, since this part of the code thinks there are no volumes=20 there, so what I originally thought is broken is not, > If we do agree on this, we should double down on [0] and actually be=20 > precise about the content type in vdisk_list by either: > A) adding a content/guest type parameter to vdisk_list. > B) adapting the call sites to filter storages. > Afterwards, this patch could be applied with the appropriate dependency=20 > bump ;) >=20 > Note that referenced unused images will still be picked up by the=20 > PVE::QemuServer::foreach_volid iteration, no matter what the content=20 > type of their storage is. but this would then mean that for referenced images, '"old !=3D new"=20 storage where both old and new storage have wrong content type' now=20 suddenly works where it previously didn't? not because of this patch,=20 but because of vdisk_list not returning those, thus skipping the check=20 that is changed in this patch? oh, and shared storages with wrong=20 content type also just work, but since we don't really touch volumes=20 there anyway with migration that's probably fine.. IFF we want to say "wrong content type" is wrong and should make the VM=20 unmigratable, we should probably move the check into test_volid so that=20 it catches ALL volumes always.. >=20 > [0]:=20 > https://git.proxmox.com/?p=3Dpve-storage.git;a=3Dcommit;h=3Da44c18925d223= a971296801a0985db34707ada4d >=20 >>> >>> Signed-off-by: Fabian Ebner >>> --- >>> PVE/QemuMigrate.pm | 7 ++----- >>> 1 file changed, 2 insertions(+), 5 deletions(-) >>> >>> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm >>> index 3597cc9..44cecce 100644 >>> --- a/PVE/QemuMigrate.pm >>> +++ b/PVE/QemuMigrate.pm >>> @@ -410,11 +410,8 @@ sub sync_disks { >>> $log_error->("storage '$targetsid' is not available on node '$se= lf->{node}'") >>> if !$target_scfg; >>> =20 >>> - # grandfather in existing mismatches >>> - if ($targetsid ne $storeid && $target_scfg) { >>> - $log_error->("content type 'images' is not available on storage '$ta= rgetsid'") >>> - if !$target_scfg->{content}->{images}; >>> - } >>> + $log_error->("content type 'images' is not available on storage '= $targetsid'") >>> + if $target_scfg && !$target_scfg->{content}->{images}; >>> =20 >>> PVE::Storage::foreach_volid($dl, sub { >>> my ($volid, $sid, $volinfo) =3D @_; >>> --=20 >>> 2.20.1 >>> >>> >>> >>> _______________________________________________ >>> pve-devel mailing list >>> pve-devel@lists.proxmox.com >>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >>> >>> >>> >>=20 >>=20 >> _______________________________________________ >> pve-devel mailing list >> pve-devel@lists.proxmox.com >> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >>=20 >>=20 >=20 =