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 73F399ABB9 for ; Mon, 22 May 2023 13:59:30 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 5B4E230C5E for ; Mon, 22 May 2023 13:59:30 +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 ; Mon, 22 May 2023 13:59:29 +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 F083F46657 for ; Mon, 22 May 2023 13:59:28 +0200 (CEST) Message-ID: <731e6d36-2943-9e69-1bcb-7ce23b712f11@proxmox.com> Date: Mon, 22 May 2023 13:59:20 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.10.0 To: Proxmox VE development discussion , Aaron Lauterer References: <20230512124043.888785-1-a.lauterer@proxmox.com> <20230512124043.888785-2-a.lauterer@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <20230512124043.888785-2-a.lauterer@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.004 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.091 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 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: Mon, 22 May 2023 11:59:30 -0000 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 :) (...) > @@ -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. > + 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. > + 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); };