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 EDEBC692C0 for ; Mon, 22 Mar 2021 12:08:32 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D924422947 for ; Mon, 22 Mar 2021 12:08:02 +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 D0A3B2293C for ; Mon, 22 Mar 2021 12:08:01 +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 9973542BA4 for ; Mon, 22 Mar 2021 12:08:01 +0100 (CET) To: =?UTF-8?Q?Fabian_Gr=c3=bcnbichler?= , 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> <1616406650.sq238n1ivv.astroid@nora.none> From: Fabian Ebner Message-ID: Date: Mon, 22 Mar 2021 12:08:00 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.0 MIME-Version: 1.0 In-Reply-To: <1616406650.sq238n1ivv.astroid@nora.none> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.396 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 NICE_REPLY_A -0.001 Looks like a legit reply (A) 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 11:08:33 -0000 Am 22.03.21 um 11:01 schrieb Fabian Grünbichler: > On March 22, 2021 9:56 am, Fabian Ebner wrote: >> Am 19.03.21 um 15:16 schrieb Fabian Grünbichler: >>> On March 19, 2021 2:49 pm, Fabian Ebner wrote: >>>> it's cheap and saves code. >>> >>> but also changes behaviour in a non-backwards-compatible fashion. >>> >>> 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". >>> >> >> What about the recent change [0] in pve-storage then, i.e. not listing >> VM disks for storages without an appropriate content type? > > I'd have to think more about that (I mean, it obviously makes sense, the > question is whether it breaks something ;)) > For referenced volumes on storages without an 'image' or 'rootfs' content type, it does break updating the disk size before migration. >> >> I'd argue that unreferenced images that lie on storages without an >> 'image' content type should not be picked up in the first place. If we >> don't agree on this, then [0] needs to be reverted... > > but this is not just about unreferenced disks? I mean, this code runs > for ALL disks returned by pve-storage. if pve-storage now filters, then > this does not apply for the '"old == new" storage which has wrong content > type' scenario, since this part of the code thinks there are no volumes > there, so what I originally thought is broken is not, It does apply when the content type of the storage is only 'rootfs' for example (and we don't currently have a dependency bump on the new pve-storage behavior ;) > >> If we do agree on this, we should double down on [0] and actually be >> 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 >> bump ;) >> >> Note that referenced unused images will still be picked up by the >> PVE::QemuServer::foreach_volid iteration, no matter what the content >> type of their storage is. > > but this would then mean that for referenced images, '"old != new" > storage where both old and new storage have wrong content type' now > suddenly works where it previously didn't? not because of this patch, > but because of vdisk_list not returning those, thus skipping the check > that is changed in this patch? oh, and shared storages with wrong > content type also just work, but since we don't really touch volumes > there anyway with migration that's probably fine.. > For shared ones, we skip the scan, and if there is a --targetstorge map, there already is a check as part of the API call itself. > IFF we want to say "wrong content type" is wrong and should make the VM > unmigratable, we should probably move the check into test_volid so that > it catches ALL volumes always.. > But there might be more subtle things like the disk size issue, so I'd be in favor of reverting [0] (for now) and instead adapting the image removal on VM destruction to not scan storages without 'images' content. >> >> [0]: >> https://git.proxmox.com/?p=pve-storage.git;a=commit;h=a44c18925d223a971296801a0985db34707ada4d >> >>>> >>>> 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 '$self->{node}'") >>>> if !$target_scfg; >>>> >>>> - # grandfather in existing mismatches >>>> - if ($targetsid ne $storeid && $target_scfg) { >>>> - $log_error->("content type 'images' is not available on storage '$targetsid'") >>>> - if !$target_scfg->{content}->{images}; >>>> - } >>>> + $log_error->("content type 'images' is not available on storage '$targetsid'") >>>> + if $target_scfg && !$target_scfg->{content}->{images}; >>>> >>>> PVE::Storage::foreach_volid($dl, sub { >>>> my ($volid, $sid, $volinfo) = @_; >>>> -- >>>> 2.20.1 >>>> >>>> >>>> >>>> _______________________________________________ >>>> 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 >>> >>> >>