[pve-devel] [PATCH v5 qemu-server 0/7] migration: don't scan all storages, fail on aliases

[pve-devel] [PATCH v5 qemu-server 0/7] migration: don't scan all storages, fail on aliases

[Aaron Lauterer]

This patch series changes the behavior during guest migrations:

Don't scan all storages for potential images belonging to the guest.
Only migrate images referenced in the config.
This made it necessary to handle pending changes explicitly which had
been covered by the storage scan.

We also added checks for any aliased volids and fail the migration if
detected.

The qemu-server part consists of quite a lot more commits since we had
to change a few things and for a better history, they are mostly their
own patches.

There is also a small patch for the documentation to add a hint that
aliased storages should be avoided.


changes since
v3:
- drop patch 4/12 rest regular config last
- refactor test_volid and ref handling -> is_attached
- reorder patches, first volid_changes reg. pending disks, then the
change in migration storage scanning
- rebase as 5/12 is already applied

v3:
- split it into many smaller commits
- changed / improved the handling of the 'ref's in
QemuMigrate::scan_local_volumes()
- fixed tests and added error handling in new mock functions
- rephrased the documentation hint

v2:
- style fixes
- qemu-server: mainly change the handling of pending changes
- container: use NEW_DISK_RE to check if the volume is a not yet created
pending volume

More in each patch.

qemu-server: Aaron Lauterer (7):
  qemuserver: foreach_volid: include pending volumes
  qemuserver: foreach_volid: always include pending disks
  migration: only migrate disks used by the guest
  qemuserver: migration: test_volid: change attr name and ref handling
  tests: add migration test for pending disk
  migration: fail when aliased volume is detected
  tests: add migration alias check

 PVE/QemuMigrate.pm                    |  82 +++++---------
 PVE/QemuServer.pm                     |  19 +++-
 test/MigrationTest/QemuMigrateMock.pm |  10 ++
 test/run_qemu_migrate_tests.pl        | 151 +++++++++++++++++++++++++-
 4 files changed, 198 insertions(+), 64 deletions(-)


container: Aaron Lauterer (2):
  migration: only migrate volumes used by the guest
  migration: fail when aliased volume is detected

 src/PVE/LXC/Migrate.pm | 51 +++++++++++++++++-------------------------
 1 file changed, 21 insertions(+), 30 deletions(-)


docs: Aaron Lauterer (1):
  storage: add hint to avoid storage aliasing

 pvesm.adoc | 6 ++++++
 1 file changed, 6 insertions(+)

-- 
2.39.2

[pve-devel] [PATCH v5 qemu-server 1/10] qemuserver: foreach_volid: include pending volumes

[Aaron Lauterer]

Make it possible to optionally iterate over disks in the pending section
of VMs, similar as to how snapshots are handled already.

This is for example useful in the migration if we don't want to rely on
the scanning of all storages.

All calling sites are adapted and enable it, except for
QemuConfig::get_replicatable_volumes as that would cause a change for
the replication if pending disks would be included.

The following lists the calling sites and if they should be fine with
the change (source [0]):

1. QemuMigrate: scan_local_volumes(): needed to include pending disk
   images
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(): would change the behavior
   of the replication, will not use it for now.
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

[0] https://lists.proxmox.com/pipermail/pve-devel/2023-May/056868.html

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
changes since v4:
- reodered
- only change QemuServer::foreach_volid and not Storage::foreach_volid

I kept the if condition and following function call as is, since this is
just an intermediate patch and readability is a bit nicer this way.

 PVE/API2/Qemu.pm   |  2 +-
 PVE/QemuConfig.pm  |  2 +-
 PVE/QemuMigrate.pm |  2 +-
 PVE/QemuServer.pm  | 15 +++++++++++----
 4 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index c92734a..37f78fe 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -4164,7 +4164,7 @@ my $check_vm_disks_local = sub {
     my $local_disks = {};
 
     # add some more information to the disks e.g. cdrom
-    PVE::QemuServer::foreach_volid($vmconf, sub {
+    PVE::QemuServer::foreach_volid($vmconf, 1, sub {
 	my ($volid, $attr) = @_;
 
 	my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid, 1);
diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
index 10e6929..5e46db2 100644
--- a/PVE/QemuConfig.pm
+++ b/PVE/QemuConfig.pm
@@ -161,7 +161,7 @@ sub get_replicatable_volumes {
 	$volhash->{$volid} = 1;
     };
 
-    PVE::QemuServer::foreach_volid($conf, $test_volid);
+    PVE::QemuServer::foreach_volid($conf, undef, $test_volid);
 
     return $volhash;
 }
diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index b10a515..3b3ef2a 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -451,7 +451,7 @@ sub scan_local_volumes {
 		if PVE::Storage::volume_is_base_and_used($storecfg, $volid);
 	};
 
-	PVE::QemuServer::foreach_volid($conf, sub {
+	PVE::QemuServer::foreach_volid($conf, 1, sub {
 	    my ($volid, $attr) = @_;
 	    eval { $test_volid->($volid, $attr); };
 	    if (my $err = $@) {
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 6cbaf87..8e6658e 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -4855,12 +4855,12 @@ sub set_migration_caps {
 }
 
 sub foreach_volid {
-    my ($conf, $func, @param) = @_;
+    my ($conf, $include_pending, $func, @param) = @_;
 
     my $volhash = {};
 
     my $test_volid = sub {
-	my ($key, $drive, $snapname) = @_;
+	my ($key, $drive, $snapname, $pending) = @_;
 
 	my $volid = $drive->{file};
 	return if !$volid;
@@ -4876,11 +4876,13 @@ sub foreach_volid {
 	$volhash->{$volid}->{shared} = 1 if $drive->{shared};
 
 	$volhash->{$volid}->{referenced_in_config} //= 0;
-	$volhash->{$volid}->{referenced_in_config} = 1 if !defined($snapname);
+	$volhash->{$volid}->{referenced_in_config} = 1 if !defined($snapname) && !defined($pending);
 
 	$volhash->{$volid}->{referenced_in_snapshot}->{$snapname} = 1
 	    if defined($snapname);
 
+	$volhash->{$volid}->{referenced_in_pending} = 1 if defined($pending);
+
 	my $size = $drive->{size};
 	$volhash->{$volid}->{size} //= $size if $size;
 
@@ -4902,6 +4904,11 @@ sub foreach_volid {
     };
 
     PVE::QemuConfig->foreach_volume_full($conf, $include_opts, $test_volid);
+
+    if ($include_pending && defined($conf->{pending}) && $conf->{pending}->%*) {
+	PVE::QemuConfig->foreach_volume_full($conf->{pending}, $include_opts, $test_volid, undef, 1);
+    }
+
     foreach my $snapname (keys %{$conf->{snapshots}}) {
 	my $snap = $conf->{snapshots}->{$snapname};
 	PVE::QemuConfig->foreach_volume_full($snap, $include_opts, $test_volid, $snapname);
@@ -6149,7 +6156,7 @@ sub get_vm_volumes {
     my ($conf) = @_;
 
     my $vollist = [];
-    foreach_volid($conf, sub {
+    foreach_volid($conf, 1, sub {
 	my ($volid, $attr) = @_;
 
 	return if $volid =~ m|^/|;
-- 
2.39.2

[pve-devel] [PATCH v5 qemu-server 2/10] qemuserver: foreach_volid: always include pending disks

[Aaron Lauterer]

All calling sites except for QemuConfig.pm::get_replicatable_volumes()
already enabled it. Making it the non-configurable default results in a
change in the VM replication.  Now a disk image only referenced in the
pending section will also be replicated.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
changes since v4:
- changed call in foreach_volid to a post-if as we only check for 2
conditions now. It should still be quite readable this way

 PVE/API2/Qemu.pm   | 2 +-
 PVE/QemuConfig.pm  | 2 +-
 PVE/QemuMigrate.pm | 2 +-
 PVE/QemuServer.pm  | 9 ++++-----
 4 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 37f78fe..c92734a 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -4164,7 +4164,7 @@ my $check_vm_disks_local = sub {
     my $local_disks = {};
 
     # add some more information to the disks e.g. cdrom
-    PVE::QemuServer::foreach_volid($vmconf, 1, sub {
+    PVE::QemuServer::foreach_volid($vmconf, sub {
 	my ($volid, $attr) = @_;
 
 	my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid, 1);
diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
index 5e46db2..10e6929 100644
--- a/PVE/QemuConfig.pm
+++ b/PVE/QemuConfig.pm
@@ -161,7 +161,7 @@ sub get_replicatable_volumes {
 	$volhash->{$volid} = 1;
     };
 
-    PVE::QemuServer::foreach_volid($conf, undef, $test_volid);
+    PVE::QemuServer::foreach_volid($conf, $test_volid);
 
     return $volhash;
 }
diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index 3b3ef2a..b10a515 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -451,7 +451,7 @@ sub scan_local_volumes {
 		if PVE::Storage::volume_is_base_and_used($storecfg, $volid);
 	};
 
-	PVE::QemuServer::foreach_volid($conf, 1, sub {
+	PVE::QemuServer::foreach_volid($conf, sub {
 	    my ($volid, $attr) = @_;
 	    eval { $test_volid->($volid, $attr); };
 	    if (my $err = $@) {
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 8e6658e..7666dcb 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -4855,7 +4855,7 @@ sub set_migration_caps {
 }
 
 sub foreach_volid {
-    my ($conf, $include_pending, $func, @param) = @_;
+    my ($conf, $func, @param) = @_;
 
     my $volhash = {};
 
@@ -4905,9 +4905,8 @@ sub foreach_volid {
 
     PVE::QemuConfig->foreach_volume_full($conf, $include_opts, $test_volid);
 
-    if ($include_pending && defined($conf->{pending}) && $conf->{pending}->%*) {
-	PVE::QemuConfig->foreach_volume_full($conf->{pending}, $include_opts, $test_volid, undef, 1);
-    }
+    PVE::QemuConfig->foreach_volume_full($conf->{pending}, $include_opts, $test_volid, undef, 1)
+	if defined($conf->{pending}) && $conf->{pending}->%*;
 
     foreach my $snapname (keys %{$conf->{snapshots}}) {
 	my $snap = $conf->{snapshots}->{$snapname};
@@ -6156,7 +6155,7 @@ sub get_vm_volumes {
     my ($conf) = @_;
 
     my $vollist = [];
-    foreach_volid($conf, 1, sub {
+    foreach_volid($conf, sub {
 	my ($volid, $attr) = @_;
 
 	return if $volid =~ m|^/|;
-- 
2.39.2

[pve-devel] [PATCH v5 qemu-server 3/10] migration: only migrate disks used by the guest

[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 scanning all storages and only looking at the disk images
referenced in the VM config, we can avoid unnecessary failures.
Some information that used to be provided by the storage scanning needs
to be fetched explicilty (size, format).

Behaviorally the biggest change is that unreferenced disk images will
not be migrated anymore. Only images referenced in the config will be
migrated.

The tests have been adapted accordingly.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
changes since v4:
- reordered
- kept check if snaphots on qcow2 exist
- fixed return values in mock volume_size_info()
- removed missed fixme in tests that doesn't apply anymore

 PVE/QemuMigrate.pm                    | 52 +++++----------------------
 test/MigrationTest/QemuMigrateMock.pm | 10 ++++++
 test/run_qemu_migrate_tests.pl        | 12 +++----
 3 files changed, 25 insertions(+), 49 deletions(-)

diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index b10a515..4f6ab64 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -317,49 +317,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};
-
-	    }
-
-	    my $bwlimit = $self->get_bwlimit($storeid, $targetsid);
-
-	    PVE::Storage::foreach_volid($dl, sub {
-		my ($volid, $sid, $volinfo) = @_;
-
-		$local_volumes->{$volid}->{ref} = 'storage';
-		$local_volumes->{$volid}->{size} = $volinfo->{size};
-		$local_volumes->{$volid}->{targetsid} = $targetsid;
-		$local_volumes->{$volid}->{bwlimit} = $bwlimit;
-
-		# If with_snapshots is not set for storage migrate, it tries to use
-		# a raw+size stream, but on-the-fly conversion from qcow2 to raw+size
-		# back to qcow2 is currently not possible.
-		$local_volumes->{$volid}->{snapshots} = ($volinfo->{format} =~ /^(?:qcow2|vmdk)$/);
-		$local_volumes->{$volid}->{format} = $volinfo->{format};
-	    });
-	}
-
 	my $replicatable_volumes = !$self->{replication_jobcfg} ? {}
 	    : PVE::QemuConfig->get_replicatable_volumes($storecfg, $vmid, $conf, 0, 1);
 	foreach my $volid (keys %{$replicatable_volumes}) {
@@ -408,6 +365,11 @@ sub scan_local_volumes {
 	    $local_volumes->{$volid}->{ref} = 'storage' if $attr->{is_unused};
 	    $local_volumes->{$volid}->{ref} = 'generated' if $attr->{is_tpmstate};
 
+	    $local_volumes->{$volid}->{bwlimit} = $self->get_bwlimit($sid, $targetsid);
+	    $local_volumes->{$volid}->{targetsid} = $targetsid;
+
+	    $local_volumes->{$volid}->@{qw(size format)} = PVE::Storage::volume_size_info($storecfg, $volid);
+
 	    $local_volumes->{$volid}->{is_vmstate} = $attr->{is_vmstate} ? 1 : 0;
 
 	    $local_volumes->{$volid}->{drivename} = $attr->{drivename}
@@ -420,6 +382,10 @@ sub scan_local_volumes {
 		}
 		die "local cdrom image\n";
 	    }
+	    # If with_snapshots is not set for storage migrate, it tries to use
+	    # a raw+size stream, but on-the-fly conversion from qcow2 to raw+size
+	    # back to qcow2 is currently not possible.
+	    $local_volumes->{$volid}->{snapshots} = ($local_volumes->{$volid}->{format} =~ /^(?:qcow2|vmdk)$/);
 
 	    my ($path, $owner) = PVE::Storage::path($storecfg, $volid);
 
diff --git a/test/MigrationTest/QemuMigrateMock.pm b/test/MigrationTest/QemuMigrateMock.pm
index 94fe686..1efabe2 100644
--- a/test/MigrationTest/QemuMigrateMock.pm
+++ b/test/MigrationTest/QemuMigrateMock.pm
@@ -240,6 +240,16 @@ $MigrationTest::Shared::storage_module->mock(
 
 	delete $source_volids->{$volid};
     },
+    volume_size_info => sub {
+	my ($scfg, $volid) = @_;
+	my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid);
+
+	for my $v ($source_vdisks->{$storeid}->@*) {
+	    return wantarray ? ($v->{size}, $v->{format}, $v->{used}, $v->{parent}) : $v->{size}
+		if $v->{volid} eq $volid;
+	}
+	die "could not find '$volid' in mock 'source_vdisks'\n";
+    },
 );
 
 $MigrationTest::Shared::tools_module->mock(
diff --git a/test/run_qemu_migrate_tests.pl b/test/run_qemu_migrate_tests.pl
index 090449f..fedbc32 100755
--- a/test/run_qemu_migrate_tests.pl
+++ b/test/run_qemu_migrate_tests.pl
@@ -708,7 +708,6 @@ my $tests = [
 	},
     },
     {
-	# FIXME: Maybe add orphaned drives as unused?
 	name => '149_running_orphaned_disk_targetstorage_zfs',
 	target => 'pve1',
 	vmid => 149,
@@ -729,10 +728,11 @@ my $tests = [
 	},
 	expected_calls => $default_expected_calls_online,
 	expected => {
-	    source_volids => {},
+	    source_volids => {
+		'local-dir:149/vm-149-disk-0.qcow2' => 1,
+	    },
 	    target_volids => {
 		'local-zfs:vm-149-disk-10' => 1,
-		'local-zfs:vm-149-disk-0' => 1,
 	    },
 	    vm_config => get_patched_config(149, {
 		scsi0 => 'local-zfs:vm-149-disk-10,format=raw,size=4G',
@@ -745,7 +745,6 @@ my $tests = [
 	},
     },
     {
-	# FIXME: Maybe add orphaned drives as unused?
 	name => '149_running_orphaned_disk',
 	target => 'pve1',
 	vmid => 149,
@@ -765,10 +764,11 @@ my $tests = [
 	},
 	expected_calls => $default_expected_calls_online,
 	expected => {
-	    source_volids => {},
+	    source_volids => {
+		'local-dir:149/vm-149-disk-0.qcow2' => 1,
+	    },
 	    target_volids => {
 		'local-lvm:vm-149-disk-10' => 1,
-		'local-dir:149/vm-149-disk-0.qcow2' => 1,
 	    },
 	    vm_config => get_patched_config(149, {
 		scsi0 => 'local-lvm:vm-149-disk-10,format=raw,size=4G',
-- 
2.39.2

[pve-devel] [PATCH v5 qemu-server 4/10] qemuserver: migration: test_volid: change attr name and ref handling

[Aaron Lauterer]

Since we don't scan all storages for matching disk images anymore for a
migration we don't have any images found via storage alone. They will be
referenced in the config somewhere.

Therefore, there is no need for the 'storage' ref.
The 'referenced_in_config' is not really needed and can apply to both,
attached and unused disk images.

Therefore the QemuServer::foreach_volid() will change the
'referenced_in_config' attribute to an 'is_attached' one that only
applies to disk images that are in the _main_ config part and are not
unused.

In QemuMigrate::scan_local_volumes() we can then quite easily map the
refs to each state, attached, unused, referenced_in_{pending,snapshot}.

The refs are mostly used for informational use to print out in the logs
why a disk image is part of the migration. Except for the 'attached' case.

In the future the extra step of the refs in QemuMigrate could probably
be streamlined even more.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
changes since v4:
- drop 'referenced_in_config' in favor of 'is_attached'
needed a reordering in test_volid() to have the 'is_unused' attribute
available
- simplified the setting of the 'ref' property, can be streamlined in
the future as we can probably just set it, or a substitute property in
test_volid() directly.

 PVE/QemuMigrate.pm | 20 ++++++++++++--------
 PVE/QemuServer.pm  | 11 ++++++-----
 2 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index 4f6ab64..f51904d 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -337,7 +337,7 @@ sub scan_local_volumes {
 	    if ($attr->{cdrom}) {
 		if ($volid eq 'cdrom') {
 		    my $msg = "can't migrate local cdrom drive";
-		    if (defined($snaprefs) && !$attr->{referenced_in_config}) {
+		    if (defined($snaprefs) && !$attr->{is_attached}) {
 			my $snapnames = join(', ', sort keys %$snaprefs);
 			$msg .= " (referenced in snapshot - $snapnames)";
 		    }
@@ -361,8 +361,10 @@ sub scan_local_volumes {
 	    $self->target_storage_check_available($storecfg, $targetsid, $volid);
 	    return if $scfg->{shared} && !$self->{opts}->{remote};
 
-	    $local_volumes->{$volid}->{ref} = $attr->{referenced_in_config} ? 'config' : 'snapshot';
-	    $local_volumes->{$volid}->{ref} = 'storage' if $attr->{is_unused};
+	    $local_volumes->{$volid}->{ref} = 'pending' if $attr->{referenced_in_pending};
+	    $local_volumes->{$volid}->{ref} = 'snapshot' if $attr->{referenced_in_snapshot};
+	    $local_volumes->{$volid}->{ref} = 'unused' if $attr->{is_unused};
+	    $local_volumes->{$volid}->{ref} = 'attached' if $attr->{is_attached};
 	    $local_volumes->{$volid}->{ref} = 'generated' if $attr->{is_tpmstate};
 
 	    $local_volumes->{$volid}->{bwlimit} = $self->get_bwlimit($sid, $targetsid);
@@ -428,14 +430,16 @@ sub scan_local_volumes {
 	foreach my $vol (sort keys %$local_volumes) {
 	    my $type = $replicatable_volumes->{$vol} ? 'local, replicated' : 'local';
 	    my $ref = $local_volumes->{$vol}->{ref};
-	    if ($ref eq 'storage') {
-		$self->log('info', "found $type disk '$vol' (via storage)\n");
-	    } elsif ($ref eq 'config') {
+	    if ($ref eq 'attached') {
 		&$log_error("can't live migrate attached local disks without with-local-disks option\n", $vol)
 		    if $self->{running} && !$self->{opts}->{"with-local-disks"};
-		$self->log('info', "found $type disk '$vol' (in current VM config)\n");
+		$self->log('info', "found $type disk '$vol' (attached)\n");
+	    } elsif ($ref eq 'unused') {
+		$self->log('info', "found $type disk '$vol' (unused)\n");
 	    } elsif ($ref eq 'snapshot') {
 		$self->log('info', "found $type disk '$vol' (referenced by snapshot(s))\n");
+	    } elsif ($ref eq 'pending') {
+		$self->log('info', "found $type disk '$vol' (pending change)\n");
 	    } elsif ($ref eq 'generated') {
 		$self->log('info', "found generated disk '$vol' (in current VM config)\n");
 	    } else {
@@ -475,7 +479,7 @@ sub scan_local_volumes {
 
 	foreach my $volid (sort keys %$local_volumes) {
 	    my $ref = $local_volumes->{$volid}->{ref};
-	    if ($self->{running} && $ref eq 'config') {
+	    if ($self->{running} && $ref eq 'attached') {
 		$local_volumes->{$volid}->{migration_mode} = 'online';
 	    } elsif ($self->{running} && $ref eq 'generated') {
 		# offline migrate the cloud-init ISO and don't regenerate on VM start
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 7666dcb..a49aeea 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -4875,8 +4875,12 @@ sub foreach_volid {
 	$volhash->{$volid}->{shared} //= 0;
 	$volhash->{$volid}->{shared} = 1 if $drive->{shared};
 
-	$volhash->{$volid}->{referenced_in_config} //= 0;
-	$volhash->{$volid}->{referenced_in_config} = 1 if !defined($snapname) && !defined($pending);
+	$volhash->{$volid}->{is_unused} //= 0;
+	$volhash->{$volid}->{is_unused} = 1 if $key =~ /^unused\d+$/;
+
+	$volhash->{$volid}->{is_attached} //= 0;
+	$volhash->{$volid}->{is_attached} = 1
+	    if !$volhash->{$volid}->{is_unused} && !defined($snapname) && !defined($pending);
 
 	$volhash->{$volid}->{referenced_in_snapshot}->{$snapname} = 1
 	    if defined($snapname);
@@ -4892,9 +4896,6 @@ sub foreach_volid {
 	$volhash->{$volid}->{is_tpmstate} //= 0;
 	$volhash->{$volid}->{is_tpmstate} = 1 if $key eq 'tpmstate0';
 
-	$volhash->{$volid}->{is_unused} //= 0;
-	$volhash->{$volid}->{is_unused} = 1 if $key =~ /^unused\d+$/;
-
 	$volhash->{$volid}->{drivename} = $key if is_valid_drivename($key);
     };
 
-- 
2.39.2

[pve-devel] [PATCH v5 qemu-server 5/10] tests: add migration test for pending disk

[Aaron Lauterer]

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
changes since v4: none
 test/run_qemu_migrate_tests.pl | 64 ++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/test/run_qemu_migrate_tests.pl b/test/run_qemu_migrate_tests.pl
index fedbc32..3d5eb8d 100755
--- a/test/run_qemu_migrate_tests.pl
+++ b/test/run_qemu_migrate_tests.pl
@@ -130,6 +130,25 @@ my $vm_configs = {
 	'startup' => 'order=2',
 	'vmgenid' => '4eb1d535-9381-4ddc-a8aa-af50c4d9177b',
     },
+    111 => {
+	'bootdisk' => 'scsi0',
+	'cores' => 1,
+	'ide0' => 'local-lvm:vm-111-disk-0,size=4096M',
+	'ide2' => 'none,media=cdrom',
+	'memory' => 512,
+	'name' => 'pending-test',
+	'net0' => 'virtio=4A:A3:E4:4C:CF:F0,bridge=vmbr0,firewall=1',
+	'numa' => 0,
+	'ostype' => 'l26',
+	'pending' => {
+		'scsi0' => 'local-zfs:vm-111-disk-0,size=103M',
+	},
+	'scsihw' => 'virtio-scsi-pci',
+	'snapshots' => {},
+	'smbios1' => 'uuid=5ad71d4d-8f73-4377-853e-2d22c10c96a5',
+	'sockets' => 1,
+	'vmgenid' => '2c00c030-0b5b-4988-a371-6ab259893f22',
+    },
     149 => {
 	'agent' => '0',
 	'bootdisk' => 'scsi0',
@@ -306,6 +325,13 @@ my $source_vdisks = {
 	    'vmid' => '341',
 	    'volid' => 'local-lvm:vm-341-disk-0',
 	},
+	{
+	    'ctime' => '1589277334',
+	    'format' => 'raw',
+	    'size' =>  4294967296,
+	    'vmid' => '111',
+	    'volid' => 'local-lvm:vm-111-disk-0',
+	},
     ],
     'local-zfs' => [
 	{
@@ -322,6 +348,13 @@ my $source_vdisks = {
 	    'vmid' => '105',
 	    'volid' => 'local-zfs:vm-105-disk-1',
 	},
+	{
+	    'ctime' => '1589277334',
+	    'format' => 'raw',
+	    'size' => 108003328,
+	    'vmid' => '111',
+	    'volid' => 'local-zfs:vm-111-disk-0',
+	},
 	{
 	    'format' => 'raw',
 	    'name' => 'vm-4567-disk-0',
@@ -1528,6 +1561,37 @@ my $tests = [
 	    },
 	},
     },
+    {
+	name => '111_running_pending',
+	target => 'pve1',
+	vmid => 111,
+	vm_status => {
+	    running => 1,
+	    runningmachine => 'pc-q35-5.0+pve0',
+	},
+	opts => {
+	    online => 1,
+	    'with-local-disks' => 1,
+	},
+	expected_calls => $default_expected_calls_online,
+	expected => {
+	    source_volids => {},
+	    target_volids => {
+		'local-zfs:vm-111-disk-0' => 1,
+		'local-lvm:vm-111-disk-10' => 1,
+	    },
+	    vm_config => get_patched_config(111, {
+		ide0 => 'local-lvm:vm-111-disk-10,format=raw,size=4G',
+		pending => {
+		    scsi0 => 'local-zfs:vm-111-disk-0,size=103M',
+		},
+	    }),
+	    vm_status => {
+		running => 1,
+		runningmachine => 'pc-q35-5.0+pve0',
+	    },
+	},
+    },
 ];
 
 my $single_test_name = shift;
-- 
2.39.2

[pve-devel] [PATCH v5 qemu-server 6/10] migration: fail when aliased volume is detected

[Aaron Lauterer]

Aliased volids can lead to unexpected behavior in a migration.

An aliased volid can happen if we have two storage configurations,
pointing to the same place. The resulting 'path' for a disk image
will be the same.
Therefore, stop the migration in such a case.

The check works by comparing the path returned by the storage plugin.

We decided against checking the storages themselves being aliased. It is
not possible to infer that reliably from just the storage configuration
options alone.

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
changes since v4: none

 PVE/QemuMigrate.pm | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index f51904d..42a3060 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -299,12 +299,12 @@ sub scan_local_volumes {
 
     my $storecfg = $self->{storecfg};
     eval {
-
 	# found local volumes and their origin
 	my $local_volumes = $self->{local_volumes};
 	my $local_volumes_errors = {};
 	my $other_errors = [];
 	my $abort = 0;
+	my $path_to_volid = {};
 
 	my $log_error = sub {
 	    my ($msg, $volid) = @_;
@@ -394,6 +394,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)) {
@@ -427,6 +429,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};
-- 
2.39.2

[pve-devel] [PATCH v5 qemu-server 7/10] tests: add migration alias check

[Aaron Lauterer]

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
changes since v4: none

 test/run_qemu_migrate_tests.pl | 75 ++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/test/run_qemu_migrate_tests.pl b/test/run_qemu_migrate_tests.pl
index 3d5eb8d..4373a38 100755
--- a/test/run_qemu_migrate_tests.pl
+++ b/test/run_qemu_migrate_tests.pl
@@ -88,6 +88,24 @@ my $storage_config = {
 	    path => "/some/other/dir/",
 	    type => "dir",
 	},
+	"zfs-alias-1" => {
+	    content => {
+		images => 1,
+		rootdir => 1,
+	    },
+	    pool => "aliaspool",
+	    sparse => 1,
+	    type => "zfspool",
+	},
+	"zfs-alias-2" => {
+	    content => {
+		images => 1,
+		rootdir => 1,
+	    },
+	    pool => "aliaspool",
+	    sparse => 1,
+	    type => "zfspool",
+	},
     },
 };
 
@@ -149,6 +167,24 @@ my $vm_configs = {
 	'sockets' => 1,
 	'vmgenid' => '2c00c030-0b5b-4988-a371-6ab259893f22',
     },
+    123 => {
+	'bootdisk' => 'scsi0',
+	'cores' => 1,
+	'scsi0' => 'zfs-alias-1:vm-123-disk-0,size=4096M',
+	'scsi1' => 'zfs-alias-2:vm-123-disk-0,size=4096M',
+	'ide2' => 'none,media=cdrom',
+	'memory' => 512,
+	'name' => 'alias-test',
+	'net0' => 'virtio=4A:A3:E4:4C:CF:F0,bridge=vmbr0,firewall=1',
+	'numa' => 0,
+	'ostype' => 'l26',
+	'pending' => {},
+	'scsihw' => 'virtio-scsi-pci',
+	'snapshots' => {},
+	'smbios1' => 'uuid=5ad71d4d-8f73-4377-853e-2d22c10c96a5',
+	'sockets' => 1,
+	'vmgenid' => '2c00c030-0b5b-4988-a371-6ab259893f22',
+    },
     149 => {
 	'agent' => '0',
 	'bootdisk' => 'scsi0',
@@ -380,6 +416,24 @@ my $source_vdisks = {
 	    'volid' => 'rbd-store:vm-1033-cloudinit',
 	},
     ],
+    'zfs-alias-1' => [
+	{
+	    'ctime' => '1589277334',
+	    'format' => 'raw',
+	    'size' => 4294967296,
+	    'vmid' => '123',
+	    'volid' => 'zfs-alias-1:vm-123-disk-0',
+	},
+    ],
+    'zfs-alias-2' => [
+	{
+	    'ctime' => '1589277334',
+	    'format' => 'raw',
+	    'size' => 4294967296,
+	    'vmid' => '123',
+	    'volid' => 'zfs-alias-2:vm-123-disk-0',
+	},
+    ],
 };
 
 my $default_expected_calls_online = {
@@ -1592,6 +1646,27 @@ my $tests = [
 	    },
 	},
     },
+    {
+	name => '123_alias_fail',
+	target => 'pve1',
+	vmid => 123,
+	vm_status => {
+	    running => 0,
+	},
+	opts => {
+	    'with-local-disks' => 1,
+	},
+	expected_calls => {},
+	expect_die => "detected not supported aliased volumes",
+	expected => {
+	    source_volids => local_volids_for_vm(123),
+	    target_volids => {},
+	    vm_config => $vm_configs->{123},
+	    vm_status => {
+		running => 0,
+	    },
+	},
+    },
 ];
 
 my $single_test_name = shift;
-- 
2.39.2

[pve-devel] [PATCH v5 container 8/10] migration: only migrate volumes used by the guest

[Aaron Lauterer]

When scanning all configured storages for volumes belonging to the
container, the migration could easily fail if a storage is not
available, but enabled. That storage might not even be used by the
container at all.

By not doing that and only looking at the disk images referenced in the
config, we can avoid that.
We need to add additional steps for pending volumes with checks if they
actually exist. Changing an existing mountpoint to a new volume
will only create the volume on the next start of the container.

The big change regarding behavior is that volumes not referenced in the
container config will be ignored.  They are already orphans that used to
be migrated as well, but are now left where they are.

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
changes since v4: fixed last defined() style nit

 src/PVE/LXC/Migrate.pm | 42 ++++++++++++------------------------------
 1 file changed, 12 insertions(+), 30 deletions(-)

diff --git a/src/PVE/LXC/Migrate.pm b/src/PVE/LXC/Migrate.pm
index ccf5157..3947227 100644
--- a/src/PVE/LXC/Migrate.pm
+++ b/src/PVE/LXC/Migrate.pm
@@ -195,6 +195,12 @@ sub phase1 {
 
 	return if !$volid;
 
+	# check if volume exists, might be a pending new one
+	if ($volid =~ $PVE::LXC::NEW_DISK_RE) {
+	    $self->log('info', "volume '$volid' does not exist (pending change?)");
+	    return;
+	}
+
 	my ($sid, $volname) = PVE::Storage::parse_volume_id($volid);
 
 	# check if storage is available on source node
@@ -256,42 +262,18 @@ sub phase1 {
 	&$log_error($@, $volid) if $@;
     };
 
-    # first unused / lost volumes owned by this container
-    my @sids = PVE::Storage::storage_ids($self->{storecfg});
-    foreach my $storeid (@sids) {
-	my $scfg = PVE::Storage::storage_config($self->{storecfg}, $storeid);
-	next if $scfg->{shared} && !$remote;
-	next if !PVE::Storage::storage_check_enabled($self->{storecfg}, $storeid, undef, 1);
-
-	# get list from PVE::Storage (for unreferenced volumes)
-	my $dl = PVE::Storage::vdisk_list($self->{storecfg}, $storeid, $vmid, undef, 'rootdir');
-
-	next if @{$dl->{$storeid}} == 0;
-
-	# check if storage is available on target node
-	my $targetsid = PVE::JSONSchema::map_id($self->{opts}->{storagemap}, $storeid);
-	if (!$remote) {
-	    my $target_scfg = PVE::Storage::storage_check_enabled($self->{storecfg}, $targetsid, $self->{node});
-
-	    die "content type 'rootdir' is not available on storage '$targetsid'\n"
-		if !$target_scfg->{content}->{rootdir};
-	}
-
-	PVE::Storage::foreach_volid($dl, sub {
-	    my ($volid, $sid, $volname) = @_;
-
-	    $volhash->{$volid}->{ref} = 'storage';
-	    $volhash->{$volid}->{targetsid} = $targetsid;
-	});
-    }
-
-    # then all volumes referenced in snapshots
+    # first all volumes referenced in snapshots
     foreach my $snapname (keys %{$conf->{snapshots}}) {
 	&$test_volid($conf->{snapshots}->{$snapname}->{'vmstate'}, 0, undef)
 	    if defined($conf->{snapshots}->{$snapname}->{'vmstate'});
 	PVE::LXC::Config->foreach_volume($conf->{snapshots}->{$snapname}, $test_mp, $snapname);
     }
 
+    # then all pending volumes
+    if (defined($conf->{pending}) && $conf->{pending}->%*) {
+	PVE::LXC::Config->foreach_volume($conf->{pending}, $test_mp);
+    }
+
     # finally all current volumes
     PVE::LXC::Config->foreach_volume_full($conf, { include_unused => 1 }, $test_mp);
 
-- 
2.39.2

[pve-devel] [PATCH v5 container 9/10] migration: fail when aliased volume is detected

[Aaron Lauterer]

Aliased volumes (referencing the same volume 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
* having a different volid due to an aliased storage: different storage
name but pointing to the same location.

We decided against checking the storages themselves being aliased. It is
not possible to infer that reliably from just the storage configuration
options alone.

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
changes since v4: none

 src/PVE/LXC/Migrate.pm | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/PVE/LXC/Migrate.pm b/src/PVE/LXC/Migrate.pm
index 3947227..abef737 100644
--- a/src/PVE/LXC/Migrate.pm
+++ b/src/PVE/LXC/Migrate.pm
@@ -182,6 +182,7 @@ sub phase1 {
     my $volhash = {}; # 'config', 'snapshot' or 'storage' for local volumes
     my $volhash_errors = {};
     my $abort = 0;
+    my $path_to_volid = {};
 
     my $log_error = sub {
 	my ($msg, $volid) = @_;
@@ -231,6 +232,8 @@ sub phase1 {
 	die "owned by other guest (owner = $owner)\n"
 	    if !$owner || ($owner != $self->{vmid});
 
+	$path_to_volid->{$path}->{$volid} = 1;
+
 	if (defined($snapname)) {
 	    # we cannot migrate shapshots on local storage
 	    # exceptions: 'zfspool', 'btrfs'
@@ -277,6 +280,12 @@ sub phase1 {
     # finally all current volumes
     PVE::LXC::Config->foreach_volume_full($conf, { include_unused => 1 }, $test_mp);
 
+    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);
+    }
+
     # additional checks for local storage
     foreach my $volid (keys %$volhash) {
 	eval {
-- 
2.39.2

[pve-devel] [PATCH v5 docs 10/10] storage: add hint to avoid storage aliasing

[Aaron Lauterer]

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
changes since v4: none

 pvesm.adoc | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/pvesm.adoc b/pvesm.adoc
index d250e0b..49eb972 100644
--- a/pvesm.adoc
+++ b/pvesm.adoc
@@ -177,6 +177,12 @@ zfspool: local-zfs
 	content images,rootdir
 ----
 
+CAUTION: It is problematic to have multiple storage configurations pointing to
+the exact same underlying storage. Such an _aliased_ storage configuration can
+lead to two different volume IDs ('volid') pointing to the exact same disk
+image. {pve} expects that the images' volume IDs point to, are unique. Choosing
+different content types for _aliased_ storage configurations can be fine, but
+is not recommended.
 
 Common Storage Properties
 ~~~~~~~~~~~~~~~~~~~~~~~~~
-- 
2.39.2

Re: [pve-devel] [PATCH v5 qemu-server 1/10] qemuserver: foreach_volid: include pending volumes

[Fiona Ebner]

Am 19.06.23 um 11:29 schrieb Aaron Lauterer:
>  
> @@ -4876,11 +4876,13 @@ sub foreach_volid {
>  	$volhash->{$volid}->{shared} = 1 if $drive->{shared};
>  
>  	$volhash->{$volid}->{referenced_in_config} //= 0;
> -	$volhash->{$volid}->{referenced_in_config} = 1 if !defined($snapname);
> +	$volhash->{$volid}->{referenced_in_config} = 1 if !defined($snapname) && !defined($pending);

Nit: I would've made $pending behave like a boolean, i.e. check for
$pending rather than defined($pending). $snapname is a string, so there
one wouldn't accidentally pass in an explicit 0.

>  
>  	$volhash->{$volid}->{referenced_in_snapshot}->{$snapname} = 1
>  	    if defined($snapname);
>  
> +	$volhash->{$volid}->{referenced_in_pending} = 1 if defined($pending);
> +

Same.

Re: [pve-devel] [PATCH v5 container 9/10] migration: fail when aliased volume is detected

[Fiona Ebner]

Am 19.06.23 um 11:29 schrieb Aaron Lauterer:
> @@ -277,6 +280,12 @@ sub phase1 {
>      # finally all current volumes
>      PVE::LXC::Config->foreach_volume_full($conf, { include_unused => 1 }, $test_mp);
>  
> +    for my $path (keys %$path_to_volid) {
> +	my @volids = keys $path_to_volid->{$path}->%*;
> +	die "detected not supported aliased volumes: '" . join("', '", @volids) . "'"

Nit: missing newline after error

> +	    if (scalar @volids > 1);
> +    }
> +
>      # additional checks for local storage
>      foreach my $volid (keys %$volhash) {
>  	eval {

Re: [pve-devel] [PATCH v5 qemu-server 6/10] migration: fail when aliased volume is detected

[Fiona Ebner]

Am 19.06.23 um 11:29 schrieb Aaron Lauterer:
> @@ -427,6 +429,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) . "'"

Nit: missing newline after error

> +		if (scalar(@volids) > 1);
> +	}

Re: [pve-devel] [PATCH v5 qemu-server 0/7] migration: don't scan all storages, fail on aliases

[Fiona Ebner]

Am 19.06.23 um 11:29 schrieb Aaron Lauterer:
> This patch series changes the behavior during guest migrations:
> 
> Don't scan all storages for potential images belonging to the guest.
> Only migrate images referenced in the config.
> This made it necessary to handle pending changes explicitly which had
> been covered by the storage scan.
> 
> We also added checks for any aliased volids and fail the migration if
> detected.
> 
> The qemu-server part consists of quite a lot more commits since we had
> to change a few things and for a better history, they are mostly their
> own patches.
> 
> There is also a small patch for the documentation to add a hint that
> aliased storages should be avoided.
> 
> 

Looks good to me so far. Just three minor nits remaining (see replies to
individual patches), but those don't warrant a new version by themselves
and can be fixed upon/after applying.

For the patches that don't have it yet:

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>

[pve-devel] applied-series: [PATCH v5 qemu-server 0/7] migration: don't scan all storages, fail on aliases

[Fiona Ebner]

Am 19.06.23 um 11:29 schrieb Aaron Lauterer:
> This patch series changes the behavior during guest migrations:
> 
> Don't scan all storages for potential images belonging to the guest.
> Only migrate images referenced in the config.
> This made it necessary to handle pending changes explicitly which had
> been covered by the storage scan.
> 
> We also added checks for any aliased volids and fail the migration if
> detected.
> 
> The qemu-server part consists of quite a lot more commits since we had
> to change a few things and for a better history, they are mostly their
> own patches.
> 
> There is also a small patch for the documentation to add a hint that
> aliased storages should be avoided.
> 
> 
applied the series, thanks! Added follow-ups for my nits and found one
issue with qcow2 cloudinit disk and pushed a fix (the check for
with_snapshots was only done after the cdrom check which would return
early).