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 <a.laute...@proxmox.com>
---
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); };
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel