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 8795E9B4BA for ; Wed, 24 May 2023 17:00:11 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 66FA421910 for ; Wed, 24 May 2023 17:00:11 +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, 24 May 2023 17:00:10 +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 0A76B46DBF for ; Wed, 24 May 2023 17:00:10 +0200 (CEST) Message-ID: <3cac0868-71df-3197-81eb-344dbcd66262@proxmox.com> Date: Wed, 24 May 2023 17:00:08 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Content-Language: en-US To: Fiona Ebner , Proxmox VE development discussion References: <20230512124043.888785-1-a.lauterer@proxmox.com> <20230512124043.888785-2-a.lauterer@proxmox.com> <731e6d36-2943-9e69-1bcb-7ce23b712f11@proxmox.com> From: Aaron Lauterer In-Reply-To: <731e6d36-2943-9e69-1bcb-7ce23b712f11@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.051 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.089 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 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [qemumigrate.pm, qemumigratemock.pm] Subject: Re: [pve-devel] [PATCH qemu-server v2 1/6] migration: only migrate disks used by the guest 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, 24 May 2023 15:00:11 -0000 On 5/22/23 13:59, Fiona Ebner wrote: > Am 12.05.23 um 14:40 schrieb Aaron Lauterer: >> When scanning all configured storages for disk images belonging to the >> VM, the migration could easily fail if a storage is not available, but >> enabled. That storage might not even be used by the VM at all. >> >> By not doing that and only looking at the disk images referenced in the >> VM config, we can avoid that. >> Extra handling is needed for disk images currently in the 'pending' >> section of the VM config. These disk images used to be detected by >> scanning all storages before. >> It is also necessary to fetch some information (size, format) about the >> disk images explicitly that used to be provided by the initial scan of >> all storages. >> >> The big change regarding behavior is that disk images not referenced in >> the VM config file will be ignored. They are already orphans that used >> to be migrated as well, but are now left where they are. The tests have >> been adapted to that changed behavior. >> >> Signed-off-by: Aaron Lauterer >> --- >> PVE/QemuMigrate.pm | 71 +++++++++++---------------- >> test/MigrationTest/QemuMigrateMock.pm | 10 ++++ >> test/run_qemu_migrate_tests.pl | 12 ++--- >> 3 files changed, 44 insertions(+), 49 deletions(-) >> >> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm >> index 09cc1d8..1d21250 100644 >> --- a/PVE/QemuMigrate.pm >> +++ b/PVE/QemuMigrate.pm >> @@ -312,49 +312,6 @@ sub scan_local_volumes { >> $abort = 1; >> }; >> >> - my @sids = PVE::Storage::storage_ids($storecfg); >> - foreach my $storeid (@sids) { >> - my $scfg = PVE::Storage::storage_config($storecfg, $storeid); >> - next if $scfg->{shared} && !$self->{opts}->{remote}; >> - next if !PVE::Storage::storage_check_enabled($storecfg, $storeid, undef, 1); >> - >> - # get list from PVE::Storage (for unused volumes) >> - my $dl = PVE::Storage::vdisk_list($storecfg, $storeid, $vmid, undef, 'images'); >> - >> - next if @{$dl->{$storeid}} == 0; >> - >> - my $targetsid = PVE::JSONSchema::map_id($self->{opts}->{storagemap}, $storeid); >> - if (!$self->{opts}->{remote}) { >> - # check if storage is available on target node >> - my $target_scfg = PVE::Storage::storage_check_enabled( >> - $storecfg, >> - $targetsid, >> - $self->{node}, >> - ); >> - >> - die "content type 'images' is not available on storage '$targetsid'\n" >> - if !$target_scfg->{content}->{images}; > > This one might be worth re-adding after the storage enabled check > further below. The same check is already done in prepare() for the > result of get_vm_volumes(), but that does not (currently ;)) include > pending ones (a bit of foreshadowing here :P) > > Or actually, let's use the improved vtype-aware version from prepare(): > >> my ($vtype) = PVE::Storage::parse_volname($storecfg, $volid); >> >> die "$volid: content type '$vtype' is not available on storage '$targetsid'\n" >> if !$target_scfg->{content}->{$vtype}; > Might even be worth factoring out the whole block including the > if (!$self->{opts}->{remote}) { > ... > } > into a mini-helper? But it's only used twice, so do as you like :) > > (...) okay, will look into it. > >> @@ -405,8 +362,23 @@ sub scan_local_volumes { >> >> $local_volumes->{$volid}->{ref} = $attr->{referenced_in_config} ? 'config' : 'snapshot'; >> $local_volumes->{$volid}->{ref} = 'storage' if $attr->{is_unused}; >> + $local_volumes->{$volid}->{ref} = 'storage' if $attr->{is_pending}; >> $local_volumes->{$volid}->{ref} = 'generated' if $attr->{is_tpmstate}; >> >> + my $bwlimit = $self->get_bwlimit($sid, $targetsid); >> + $local_volumes->{$volid}->{targetsid} = $targetsid; >> + $local_volumes->{$volid}->{bwlimit} = $bwlimit; >> + >> + my $volume_list = PVE::Storage::volume_list($storecfg, $sid, $vmid, 'images'); >> + # TODO could probably be done better than just iterating > > volume_size_info() to the rescue :) Would avoid the loop and quite a bit > of overhead from calling volume_list() for each individual volume. ah thanks. looks like I can get both, size and format from it :) > >> + for my $volume (@$volume_list) { >> + if ($volume->{volid} eq $volid) { >> + $local_volumes->{$volid}->{size} = $volume->{size}; >> + $local_volumes->{$volid}->{format} = $volume->{format}; >> + last; >> + } >> + } >> + >> $local_volumes->{$volid}->{is_vmstate} = $attr->{is_vmstate} ? 1 : 0; >> >> $local_volumes->{$volid}->{drivename} = $attr->{drivename} >> @@ -450,6 +422,19 @@ sub scan_local_volumes { >> if PVE::Storage::volume_is_base_and_used($storecfg, $volid); >> }; >> >> + # add pending disks first >> + if (defined $conf->{pending} && %{$conf->{pending}}) { > > Style nit: please use parentheses for defined. And $conf->{pending}->%* > is slightly nicer, because it can be read from left to right, rather > than needing to look at the inner bit first and then remember the % on > the outside ;) > >> + PVE::QemuServer::foreach_volid($conf->{pending}, sub { > > Should we rather expand foreach_volid() instead? It handles snapshots > already, so handling pending too doesn't seem fundamentally wrong. > > Let's check the existing callers and whether they are fine with the change: > 1. this one right here: wants pending > 2. API2/Qemu.pm: check_vm_disks_local() for migration precondition: > related to migration, so more consistent with pending > 3. QemuConfig.pm: get_replicatable_volumes(): can be fine with pending, > but it's a change of course! > 4. QemuServer.pm: get_vm_volumes(): is used multiple times by: > 4a. vm_stop_cleanup() to deactivate/unmap: should also be fine with > including pending > 4b. QemuMigrate.pm: in prepare(): part of migration, so more consistent > with pending > 4c. QemuMigrate.pm: in phase3_cleanup() for deactivation: part of > migration, so more consistent with pending > > So the only issue is with replication, where we can decide between: > 1. Also include pending volumes for replication (would be fine by me). > 2. Keep behavior as-is. But then we need to adapt. Some possibilities: > 2a. Add a new attribute like 'pending-only' in the result of > foreach_volid(), so that get_replicatable_volumes() can filter them out. > 2b. Switch to a different function like foreach_volume() and get the two > attributes that are used there (cdrom and replicate) manually. > 2c. Add a switch to foreach_volid() whether it should include volumes > that are only in pending. I'll give 1 a try and see how it behaves with replication. If not, then I'd rather go for 2a or 2c, though I am not yet sure which one would be better in the long run. 2c would probably be "nicer" as the caller can decide what they want included, instead of having to check for and deal with a return attribute. > >> + my ($volid, $attr) = @_; >> + $attr->{is_pending} = 1; >> + eval { $test_volid->($volid, $attr); }; >> + if (my $err = $@) { >> + &$log_error($err, $volid); >> + } >> + }); >> + } >> + >> + # add non-pending referenced disks >> PVE::QemuServer::foreach_volid($conf, sub { >> my ($volid, $attr) = @_; >> eval { $test_volid->($volid, $attr); };