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