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 152759B718 for ; Thu, 25 May 2023 10:15:10 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id F251E27E35 for ; Thu, 25 May 2023 10:15: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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Thu, 25 May 2023 10:15:09 +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 3F6C747071 for ; Thu, 25 May 2023 10:15:09 +0200 (CEST) Message-ID: Date: Thu, 25 May 2023 10:15:08 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.10.0 Content-Language: en-US To: Proxmox VE development discussion , Aaron Lauterer References: <20230512124043.888785-1-a.lauterer@proxmox.com> <20230512124043.888785-4-a.lauterer@proxmox.com> From: Fiona Ebner In-Reply-To: <20230512124043.888785-4-a.lauterer@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.005 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 NICE_REPLY_A -0.107 Looks like a legit reply (A) 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] [PATCH qemu-server v2 3/6] migration: fail when aliased volume is detected 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: Thu, 25 May 2023 08:15:10 -0000 Am 12.05.23 um 14:40 schrieb Aaron Lauterer: > Aliased volumes (referencing the same disk image multiple times) can > lead to unexpected behavior in a migration. > > Therefore, stop the migration in such a case. > > The check works by comparing the path returned by the storage plugin. > This means that we should be able to catch the common situations where > it can happen: > > * by referencing the same volid multiple times This situation is not actually caught by the patch? If it's the same volid, it will resolve to the same path, so it will just execute $path_to_volid->{$path}->{$volid} = 1; multiple times and there won't be any detection of multi-reference, or what am I missing? Just incrementing the counter instead also doesn't work, because e.g. multi-reference is fine when done by snapshots. Maybe there should be a helper just for the currently attached disks? E.g. iterate over the currently attached disks with QemuConfig->foreach_volume() and for each volume, resolve the path and count. If any path is referenced multiple times, complain. To not lose the volids while counting, you could e.g. "count" with push $path_to_volid->{$path}->@*, $volid; The helper can then be called during migration additionally to your current patch here. And the helper can also be re-used during VM start (maybe only warn there to allow for exotic use-cases?) and I'd expect it to already catch most problematic situations there. > @@ -397,6 +397,8 @@ sub scan_local_volumes { > die "owned by other VM (owner = VM $owner)\n" > if !$owner || ($owner != $vmid); > > + $path_to_volid->{$path}->{$volid} = 1; > + > return if $attr->{is_vmstate}; > > if (defined($snaprefs)) { > @@ -443,6 +445,12 @@ sub scan_local_volumes { > } > }); > > + for my $path (keys %$path_to_volid) { > + my @volids = keys %{$path_to_volid->{$path}}; > + die "detected not supported aliased volumes: '" . join("', '", @volids) . "'" > + if (scalar @volids > 1); > + } > + > foreach my $vol (sort keys %$local_volumes) { > my $type = $replicatable_volumes->{$vol} ? 'local, replicated' : 'local'; > my $ref = $local_volumes->{$vol}->{ref};