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

[pve-devel] [PATCH v3 qemu-server, container, docs 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 patches in qemu-server and pve-container are split int two. The
first ones handle the change from scanning the storages to handle only
referenced images. The second one adds the alias check.

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


changes since 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 (4):
  migration: only migrate disks used by the guest
  tests: add migration test for pending disk
  migration: fail when aliased volume is detected
  tests: add migration alias check

 PVE/QemuMigrate.pm                    |  98 ++++++-----------
 PVE/QemuServer.pm                     |   9 +-
 test/MigrationTest/QemuMigrateMock.pm |   9 ++
 test/run_qemu_migrate_tests.pl        | 151 +++++++++++++++++++++++++-
 4 files changed, 198 insertions(+), 69 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 | 3 +++
 1 file changed, 3 insertions(+)

-- 
2.30.2

[pve-devel] [PATCH v3 qemu-server 1/7] 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 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.lauterer@proxmox.com>
---
changes sind v2:
- move handling of pending changes into QemuSerer::foreach_volid
    Seems to not have any bad side-effects
- style fixes
- use 'volume_size_info()' to get format and size of the image

 PVE/QemuMigrate.pm                    | 88 ++++++++-------------------
 PVE/QemuServer.pm                     |  9 ++-
 test/MigrationTest/QemuMigrateMock.pm |  9 +++
 test/run_qemu_migrate_tests.pl        | 12 ++--
 4 files changed, 50 insertions(+), 68 deletions(-)

diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index 09cc1d8..163a721 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -149,6 +149,22 @@ sub lock_vm {
     return PVE::QemuConfig->lock_config($vmid, $code, @param);
 }
 
+sub target_storage_check_available {
+    my ($self, $storecfg, $targetsid, $volid) = @_;
+
+    if (!$self->{opts}->{remote}) {
+	# check if storage is available on target node
+	my $target_scfg = PVE::Storage::storage_check_enabled(
+	    $storecfg,
+	    $targetsid,
+	    $self->{node},
+	);
+	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};
+    }
+}
+
 sub prepare {
     my ($self, $vmid) = @_;
 
@@ -236,18 +252,7 @@ sub prepare {
 
 	$storages->{$targetsid} = 1;
 
-	if (!$self->{opts}->{remote}) {
-	    # check if storage is available on target node
-	    my $target_scfg = PVE::Storage::storage_check_enabled(
-		$storecfg,
-		$targetsid,
-		$self->{node},
-	    );
-	    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};
-	}
+	$self->target_storage_check_available($storecfg, $targetsid, $volid);
 
 	if ($scfg->{shared}) {
 	    # PVE::Storage::activate_storage checks this for non-shared storages
@@ -312,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}) {
@@ -396,17 +358,21 @@ sub scan_local_volumes {
 		$targetsid = PVE::JSONSchema::map_id($self->{opts}->{storagemap}, $sid);
 	    }
 
-	    # check target storage on target node if intra-cluster migration
-	    if (!$self->{opts}->{remote}) {
-		PVE::Storage::storage_check_enabled($storecfg, $targetsid, $self->{node});
-
-		return if $scfg->{shared};
-	    }
+	    return if $scfg->{shared} && !$self->{opts}->{remote};
+	    $self->target_storage_check_available($storecfg, $targetsid, $volid);
 
 	    $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;
+
+	    ($local_volumes->{$volid}->{size}, $local_volumes->{$volid}->{format})
+		= PVE::Storage::volume_size_info($storecfg, $volid);
+
 	    $local_volumes->{$volid}->{is_vmstate} = $attr->{is_vmstate} ? 1 : 0;
 
 	    $local_volumes->{$volid}->{drivename} = $attr->{drivename}
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index ab33aa3..f88d695 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -4855,7 +4855,7 @@ sub foreach_volid {
     my $volhash = {};
 
     my $test_volid = sub {
-	my ($key, $drive, $snapname) = @_;
+	my ($key, $drive, $snapname, $pending) = @_;
 
 	my $volid = $drive->{file};
 	return if !$volid;
@@ -4888,6 +4888,8 @@ sub foreach_volid {
 	$volhash->{$volid}->{is_unused} //= 0;
 	$volhash->{$volid}->{is_unused} = 1 if $key =~ /^unused\d+$/;
 
+	$volhash->{$volid}->{is_pending} = 1 if $pending;
+
 	$volhash->{$volid}->{drivename} = $key if is_valid_drivename($key);
     };
 
@@ -4897,6 +4899,11 @@ sub foreach_volid {
     };
 
     PVE::QemuConfig->foreach_volume_full($conf, $include_opts, $test_volid);
+
+    if (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);
diff --git a/test/MigrationTest/QemuMigrateMock.pm b/test/MigrationTest/QemuMigrateMock.pm
index 94fe686..cec34b7 100644
--- a/test/MigrationTest/QemuMigrateMock.pm
+++ b/test/MigrationTest/QemuMigrateMock.pm
@@ -230,6 +230,15 @@ $MigrationTest::Shared::storage_module->mock(
 	}
 	return $res;
     },
+    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;
+	}
+    },
     vdisk_free => sub {
 	my ($scfg, $volid) = @_;
 
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.30.2

[pve-devel] [PATCH v3 qemu-server 2/7] tests: add migration test for pending disk

[Aaron Lauterer]

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
changes since v2:
removed autovivified 'snapshots => {}'

 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.30.2

[pve-devel] [PATCH v3 qemu-server 3/7] 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.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
We did discuss moving the check into its own function so that we can
call it in other places as well, for example during the VM start to be
able to show a warning.
I haven't gotten around to it yet, but wanted to get this version of the
sries out so we can review the other parts of the series. Therefore, we could
not apply this and the next patch (maching tests) for now.

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

diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index 163a721..6148cf5 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) = @_;
@@ -391,6 +391,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)) {
@@ -424,6 +426,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.30.2

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

[Aaron Lauterer]

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
changes since v2:
- changed mock storages and disk images, now there is 'alias-zfs' and
'alias-zfs-2' with the same disk image present to mimick an aliased
storage config.

 test/run_qemu_migrate_tests.pl | 81 ++++++++++++++++++++++++++++++++--
 1 file changed, 78 insertions(+), 3 deletions(-)

diff --git a/test/run_qemu_migrate_tests.pl b/test/run_qemu_migrate_tests.pl
index 3d5eb8d..ace557f 100755
--- a/test/run_qemu_migrate_tests.pl
+++ b/test/run_qemu_migrate_tests.pl
@@ -63,6 +63,24 @@ my $storage_config = {
 	    sparse => 1,
 	    type => "zfspool",
 	},
+	"alias-zfs" => {
+	    content => {
+		images => 1,
+		rootdir => 1,
+	    },
+	    pool => "aliaspool",
+	    sparse => 1,
+	    type => "zfspool",
+	},
+	"alias-zfs-2" => {
+	    content => {
+		images => 1,
+		rootdir => 1,
+	    },
+	    pool => "aliaspool",
+	    sparse => 1,
+	    type => "zfspool",
+	},
 	"rbd-store" => {
 	    monhost => "127.0.0.42,127.0.0.21,::1",
 	    fsid => 'fc4181a6-56eb-4f68-b452-8ba1f381ca2a',
@@ -149,6 +167,24 @@ my $vm_configs = {
 	'sockets' => 1,
 	'vmgenid' => '2c00c030-0b5b-4988-a371-6ab259893f22',
     },
+    123 => {
+	'bootdisk' => 'scsi0',
+	'cores' => 1,
+	'scsi0' => 'alias-zfs:vm-123-disk-0,size=4096M',
+	'scsi1' => 'alias-zfs-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',
@@ -351,9 +387,9 @@ my $source_vdisks = {
 	{
 	    'ctime' => '1589277334',
 	    'format' => 'raw',
-	    'size' => 108003328,
-	    'vmid' => '111',
-	    'volid' => 'local-zfs:vm-111-disk-0',
+	    'size' => 4294967296,
+	    'vmid' => '123',
+	    'volid' => 'local-zfs:vm-123-disk-0',
 	},
 	{
 	    'format' => 'raw',
@@ -364,6 +400,24 @@ my $source_vdisks = {
 	    'volid' => 'local-zfs:vm-4567-disk-0',
 	},
     ],
+    'alias-zfs' => [
+	{
+	    'ctime' => '1589277334',
+	    'format' => 'raw',
+	    'size' => 108003328,
+	    'vmid' => '111',
+	    'volid' => 'local-zfs:vm-111-disk-0',
+	},
+    ],
+    'alias-zfs-2' => [
+	{
+	    'ctime' => '1589277334',
+	    'format' => 'raw',
+	    'size' => 108003328,
+	    'vmid' => '111',
+	    'volid' => 'local-zfs:vm-111-disk-0',
+	},
+    ],
     'rbd-store' => [
 	{
 	    'ctime' => '1589277334',
@@ -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.30.2

[pve-devel] [PATCH v3 container 5/7] 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.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
changes since v2:
- use PVE::LCX::NEW_DISK_RE to check for a not yet created pending
volume
- style fixe

 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..c5bca7a 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.30.2

[pve-devel] [PATCH v3 container 6/7] 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.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
 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 c5bca7a..7405600 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.30.2

[pve-devel] [PATCH v3 docs 7/7] storage: add hint to avoid storage aliasing

[Aaron Lauterer]

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
I am happy for suggestions on how to improve the phrasing if it is not
clear enough.

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

diff --git a/pvesm.adoc b/pvesm.adoc
index 6ade1a4..7e91c50 100644
--- a/pvesm.adoc
+++ b/pvesm.adoc
@@ -174,6 +174,9 @@ zfspool: local-zfs
 	content images,rootdir
 ----
 
+CAUTION: It is not advisable to have multiple storage configurations pointing
+to the exact same underlying storage. Such an _aliased_ storage configuration
+can lead to unexpected behavior.
 
 Common Storage Properties
 ~~~~~~~~~~~~~~~~~~~~~~~~~
-- 
2.30.2

Re: [pve-devel] [PATCH v3 qemu-server 1/7] migration: only migrate disks used by the guest

[Fiona Ebner]

Am 01.06.23 um 15:53 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.lauterer@proxmox.com>
> ---
> changes sind v2:
> - move handling of pending changes into QemuSerer::foreach_volid
>     Seems to not have any bad side-effects

This should really be its own patch and talk about what the side effects
actually are and why they are okay in the commit message.

Maybe even keep old behavior for replication at first and then add a
second patch which explicitly enables replication for pending volumes by
removing the filtering initially added. Easiest probably is an option
$exclude_pending for foreach_volid() used only by replication and then
remove it in the following patch.

Like that, each patch has its own clear purpose and effect.

> - style fixes
> - use 'volume_size_info()' to get format and size of the image
> 
>  PVE/QemuMigrate.pm                    | 88 ++++++++-------------------
>  PVE/QemuServer.pm                     |  9 ++-
>  test/MigrationTest/QemuMigrateMock.pm |  9 +++
>  test/run_qemu_migrate_tests.pl        | 12 ++--
>  4 files changed, 50 insertions(+), 68 deletions(-)
> 
> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
> index 09cc1d8..163a721 100644
> --- a/PVE/QemuMigrate.pm
> +++ b/PVE/QemuMigrate.pm
> @@ -149,6 +149,22 @@ sub lock_vm {
>      return PVE::QemuConfig->lock_config($vmid, $code, @param);
>  }
>  
> +sub target_storage_check_available {
> +    my ($self, $storecfg, $targetsid, $volid) = @_;
> +
> +    if (!$self->{opts}->{remote}) {
> +	# check if storage is available on target node
> +	my $target_scfg = PVE::Storage::storage_check_enabled(
> +	    $storecfg,
> +	    $targetsid,
> +	    $self->{node},
> +	);
> +	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};
> +    }
> +}

This should also be its own patch, please.

> +
>  sub prepare {
>      my ($self, $vmid) = @_;
>  
> @@ -396,17 +358,21 @@ sub scan_local_volumes {
>  		$targetsid = PVE::JSONSchema::map_id($self->{opts}->{storagemap}, $sid);
>  	    }
>  
> -	    # check target storage on target node if intra-cluster migration
> -	    if (!$self->{opts}->{remote}) {
> -		PVE::Storage::storage_check_enabled($storecfg, $targetsid, $self->{node});
> -
> -		return if $scfg->{shared};
> -	    }
> +	    return if $scfg->{shared} && !$self->{opts}->{remote};
> +	    $self->target_storage_check_available($storecfg, $targetsid, $volid);

Shouldn't these two lines be switched?

Before, the enabled check would be done if !$self->{opts}->{remote} and
then early return.

But now, if $scfg->{shared} && !$self->{opts}->{remote};, you early
return without doing the check at all.

With switched lines, you call the helper, which does the check if
!$self->{opts}->{remote} and then you do the early return like before.

>  
>  	    $local_volumes->{$volid}->{ref} = $attr->{referenced_in_config} ? 'config' : 'snapshot';
>  	    $local_volumes->{$volid}->{ref} = 'storage' if $attr->{is_unused};

Not yours, but this is actually wrong, because if the same volume is
already referenced in a snapshot, the reference will be overwritten to
be 'storage' here :P

> +	    $local_volumes->{$volid}->{ref} = 'storage' if $attr->{is_pending};

But yours is also not correct ;) It should only be done for volumes that
are only referenced in pending, but not in the current config. You don't
touch the...

> @@ -4888,6 +4888,8 @@ sub foreach_volid {

$volhash->{$volid}->{referenced_in_config} = 1 if !defined($snapname);

...line in PVE/QemuServer.pm's foreach_volid(), so is_pending being 1
means referenced_in_config is also 1 and there's no way to distinguish
via attributes if a volume is only in pending or both in pending and the
current config.

Not sure if we should even bother to distinguish between more than
'config' and 'snapshot' anymore. Everything that's just in a snapshot
gets 'snapshot' and everything that's in the current config (including
pending) gets 'config'. No need for 'storage' at all.

>  	$volhash->{$volid}->{is_unused} //= 0;
>  	$volhash->{$volid}->{is_unused} = 1 if $key =~ /^unused\d+$/;
>  
> +	$volhash->{$volid}->{is_pending} = 1 if $pending;
> +
>  	$volhash->{$volid}->{drivename} = $key if is_valid_drivename($key);
>      };
>  

> diff --git a/test/MigrationTest/QemuMigrateMock.pm b/test/MigrationTest/QemuMigrateMock.pm
> index 94fe686..cec34b7 100644
> --- a/test/MigrationTest/QemuMigrateMock.pm
> +++ b/test/MigrationTest/QemuMigrateMock.pm
> @@ -230,6 +230,15 @@ $MigrationTest::Shared::storage_module->mock(
>  	}
>  	return $res;
>      },
> +    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;
> +	}

Please die if the expected volid was not among the ones in the list, so
that we get a clear error.

> +    },
>      vdisk_free => sub {
>  	my ($scfg, $volid) = @_;
>  

Re: [pve-devel] [PATCH v3 qemu-server 1/7] migration: only migrate disks used by the guest

[Fiona Ebner]

Am 02.06.23 um 11:45 schrieb Fiona Ebner:
> Am 01.06.23 um 15:53 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.lauterer@proxmox.com>
>> ---
>> changes sind v2:
>> - move handling of pending changes into QemuSerer::foreach_volid
>>     Seems to not have any bad side-effects
> 
> This should really be its own patch and talk about what the side effects
> actually are and why they are okay in the commit message.
> 
> Maybe even keep old behavior for replication at first and then add a
> second patch which explicitly enables replication for pending volumes by
> removing the filtering initially added. Easiest probably is an option
> $exclude_pending for foreach_volid() used only by replication and then
> remove it in the following patch.
> 
> Like that, each patch has its own clear purpose and effect.
> 
>> - style fixes
>> - use 'volume_size_info()' to get format and size of the image
>>
>>  PVE/QemuMigrate.pm                    | 88 ++++++++-------------------
>>  PVE/QemuServer.pm                     |  9 ++-
>>  test/MigrationTest/QemuMigrateMock.pm |  9 +++
>>  test/run_qemu_migrate_tests.pl        | 12 ++--
>>  4 files changed, 50 insertions(+), 68 deletions(-)
>>
>> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
>> index 09cc1d8..163a721 100644
>> --- a/PVE/QemuMigrate.pm
>> +++ b/PVE/QemuMigrate.pm
>> @@ -149,6 +149,22 @@ sub lock_vm {
>>      return PVE::QemuConfig->lock_config($vmid, $code, @param);
>>  }
>>  
>> +sub target_storage_check_available {
>> +    my ($self, $storecfg, $targetsid, $volid) = @_;
>> +
>> +    if (!$self->{opts}->{remote}) {
>> +	# check if storage is available on target node
>> +	my $target_scfg = PVE::Storage::storage_check_enabled(
>> +	    $storecfg,
>> +	    $targetsid,
>> +	    $self->{node},
>> +	);
>> +	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};
>> +    }
>> +}
> 
> This should also be its own patch, please.
> 
>> +
>>  sub prepare {
>>      my ($self, $vmid) = @_;
>>  
>> @@ -396,17 +358,21 @@ sub scan_local_volumes {
>>  		$targetsid = PVE::JSONSchema::map_id($self->{opts}->{storagemap}, $sid);
>>  	    }
>>  
>> -	    # check target storage on target node if intra-cluster migration
>> -	    if (!$self->{opts}->{remote}) {
>> -		PVE::Storage::storage_check_enabled($storecfg, $targetsid, $self->{node});
>> -
>> -		return if $scfg->{shared};
>> -	    }
>> +	    return if $scfg->{shared} && !$self->{opts}->{remote};
>> +	    $self->target_storage_check_available($storecfg, $targetsid, $volid);
> 
> Shouldn't these two lines be switched?
> 
> Before, the enabled check would be done if !$self->{opts}->{remote} and
> then early return.
> 
> But now, if $scfg->{shared} && !$self->{opts}->{remote};, you early
> return without doing the check at all.
> 
> With switched lines, you call the helper, which does the check if
> !$self->{opts}->{remote} and then you do the early return like before.
> 
>>  
>>  	    $local_volumes->{$volid}->{ref} = $attr->{referenced_in_config} ? 'config' : 'snapshot';
>>  	    $local_volumes->{$volid}->{ref} = 'storage' if $attr->{is_unused};
> 
> Not yours, but this is actually wrong, because if the same volume is
> already referenced in a snapshot, the reference will be overwritten to
> be 'storage' here :P
> 
>> +	    $local_volumes->{$volid}->{ref} = 'storage' if $attr->{is_pending};
> 
> But yours is also not correct ;) It should only be done for volumes that
> are only referenced in pending, but not in the current config. You don't
> touch the...
> 
>> @@ -4888,6 +4888,8 @@ sub foreach_volid {
> 
> $volhash->{$volid}->{referenced_in_config} = 1 if !defined($snapname);
> 
> ...line in PVE/QemuServer.pm's foreach_volid(), so is_pending being 1
> means referenced_in_config is also 1 and there's no way to distinguish
> via attributes if a volume is only in pending or both in pending and the
> current config.
> 
> Not sure if we should even bother to distinguish between more than
> 'config' and 'snapshot' anymore. Everything that's just in a snapshot
> gets 'snapshot' and everything that's in the current config (including
> pending) gets 'config'. No need for 'storage' at all.
> 

Argh, it's actually not just used informationally, but affects real
logic too :(

        foreach my $volid (sort keys %$local_volumes) {
            my $ref = $local_volumes->{$volid}->{ref};
            if ($self->{running} && $ref eq 'config') {
                $local_volumes->{$volid}->{migration_mode} = 'online';

So we do need a real way to distinguish 'only in pending' and 'both in
pending and current config' after all here.

Re: [pve-devel] [PATCH v3 qemu-server 4/7] tests: add migration alias check

[Fiona Ebner]

Am 01.06.23 um 15:53 schrieb Aaron Lauterer:
> @@ -351,9 +387,9 @@ my $source_vdisks = {
>  	{
>  	    'ctime' => '1589277334',
>  	    'format' => 'raw',
> -	    'size' => 108003328,
> -	    'vmid' => '111',
> -	    'volid' => 'local-zfs:vm-111-disk-0',
> +	    'size' => 4294967296,
> +	    'vmid' => '123',
> +	    'volid' => 'local-zfs:vm-123-disk-0',

Why is this changed?

>  	},
>  	{
>  	    'format' => 'raw',
> @@ -364,6 +400,24 @@ my $source_vdisks = {
>  	    'volid' => 'local-zfs:vm-4567-disk-0',
>  	},
>      ],
> +    'alias-zfs' => [
> +	{
> +	    'ctime' => '1589277334',
> +	    'format' => 'raw',
> +	    'size' => 108003328,
> +	    'vmid' => '111',
> +	    'volid' => 'local-zfs:vm-111-disk-0',
> +	},
> +    ],
> +    'alias-zfs-2' => [
> +	{
> +	    'ctime' => '1589277334',
> +	    'format' => 'raw',
> +	    'size' => 108003328,
> +	    'vmid' => '111',
> +	    'volid' => 'local-zfs:vm-111-disk-0',
> +	},
> +    ],

Those are added for 111 instead of 123 and the volids also are not
corresponding to the storage.

If you'd die in the mocked version of volume_size_info when not finding
the volid, you would have noticed ;)

Re: [pve-devel] [PATCH v3 container 5/7] migration: only migrate volumes used by the guest

[Fiona Ebner]

Am 01.06.23 um 15:53 schrieb Aaron Lauterer:
> @@ -256,42 +262,18 @@ sub phase1 {

(...)

>  
> +    # then all pending volumes
> +    if (defined $conf->{pending} && %{$conf->{pending}}) {

Same style nits I mentioned in the qemu-server patch last time ;)

> +	PVE::LXC::Config->foreach_volume($conf->{pending}, $test_mp);
> +    }
> +

Other than that, both container patches:

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

Re: [pve-devel] [PATCH v3 docs 7/7] storage: add hint to avoid storage aliasing

[Fiona Ebner]

Am 01.06.23 um 15:53 schrieb Aaron Lauterer:
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> I am happy for suggestions on how to improve the phrasing if it is not
> clear enough.
> 
>  pvesm.adoc | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/pvesm.adoc b/pvesm.adoc
> index 6ade1a4..7e91c50 100644
> --- a/pvesm.adoc
> +++ b/pvesm.adoc
> @@ -174,6 +174,9 @@ zfspool: local-zfs
>  	content images,rootdir
>  ----
>  
> +CAUTION: It is not advisable to have multiple storage configurations pointing
> +to the exact same underlying storage. Such an _aliased_ storage configuration
> +can lead to unexpected behavior.

s/not advisable/problematic/ would sound a bit stronger IMHO

Maybe give some rationale and mention that volume IDs are not unique
anymore with such a configuration and that PVE expects that in certain
places, maybe giving a quick example. And we could also mention that in
case the content types is different, it can be fine, but still not
recommended.

Re: [pve-devel] [PATCH v3 qemu-server 1/7] migration: only migrate disks used by the guest

[Aaron Lauterer]

On 6/2/23 11:50, Fiona Ebner wrote:
> Am 02.06.23 um 11:45 schrieb Fiona Ebner:
>> Am 01.06.23 um 15:53 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.lauterer@proxmox.com>
>>> ---
>>> changes sind v2:
>>> - move handling of pending changes into QemuSerer::foreach_volid
>>>      Seems to not have any bad side-effects
>>
>> This should really be its own patch and talk about what the side effects
>> actually are and why they are okay in the commit message.
>>
>> Maybe even keep old behavior for replication at first and then add a
>> second patch which explicitly enables replication for pending volumes by
>> removing the filtering initially added. Easiest probably is an option
>> $exclude_pending for foreach_volid() used only by replication and then
>> remove it in the following patch.
>>
>> Like that, each patch has its own clear purpose and effect.

Ah yes. I didn't think about the commit history. I will put them in their own 
patches.

>>
>>> - style fixes
>>> - use 'volume_size_info()' to get format and size of the image
>>>
>>>   PVE/QemuMigrate.pm                    | 88 ++++++++-------------------
>>>   PVE/QemuServer.pm                     |  9 ++-
>>>   test/MigrationTest/QemuMigrateMock.pm |  9 +++
>>>   test/run_qemu_migrate_tests.pl        | 12 ++--
>>>   4 files changed, 50 insertions(+), 68 deletions(-)
>>>
>>> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
>>> index 09cc1d8..163a721 100644
>>> --- a/PVE/QemuMigrate.pm
>>> +++ b/PVE/QemuMigrate.pm
>>> @@ -149,6 +149,22 @@ sub lock_vm {
>>>       return PVE::QemuConfig->lock_config($vmid, $code, @param);
>>>   }
>>>   
>>> +sub target_storage_check_available {
>>> +    my ($self, $storecfg, $targetsid, $volid) = @_;
>>> +
>>> +    if (!$self->{opts}->{remote}) {
>>> +	# check if storage is available on target node
>>> +	my $target_scfg = PVE::Storage::storage_check_enabled(
>>> +	    $storecfg,
>>> +	    $targetsid,
>>> +	    $self->{node},
>>> +	);
>>> +	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};
>>> +    }
>>> +}
>>
>> This should also be its own patch, please.
>>
>>> +
>>>   sub prepare {
>>>       my ($self, $vmid) = @_;
>>>   
>>> @@ -396,17 +358,21 @@ sub scan_local_volumes {
>>>   		$targetsid = PVE::JSONSchema::map_id($self->{opts}->{storagemap}, $sid);
>>>   	    }
>>>   
>>> -	    # check target storage on target node if intra-cluster migration
>>> -	    if (!$self->{opts}->{remote}) {
>>> -		PVE::Storage::storage_check_enabled($storecfg, $targetsid, $self->{node});
>>> -
>>> -		return if $scfg->{shared};
>>> -	    }
>>> +	    return if $scfg->{shared} && !$self->{opts}->{remote};
>>> +	    $self->target_storage_check_available($storecfg, $targetsid, $volid);
>>
>> Shouldn't these two lines be switched?
>>
>> Before, the enabled check would be done if !$self->{opts}->{remote} and
>> then early return.
>>
>> But now, if $scfg->{shared} && !$self->{opts}->{remote};, you early
>> return without doing the check at all.
>>
>> With switched lines, you call the helper, which does the check if
>> !$self->{opts}->{remote} and then you do the early return like before.
>>

Yep, will change it.

>>>   
>>>   	    $local_volumes->{$volid}->{ref} = $attr->{referenced_in_config} ? 'config' : 'snapshot';
>>>   	    $local_volumes->{$volid}->{ref} = 'storage' if $attr->{is_unused};
>>
>> Not yours, but this is actually wrong, because if the same volume is
>> already referenced in a snapshot, the reference will be overwritten to
>> be 'storage' here :P
>>
>>> +	    $local_volumes->{$volid}->{ref} = 'storage' if $attr->{is_pending};
>>
>> But yours is also not correct ;) It should only be done for volumes that
>> are only referenced in pending, but not in the current config. You don't
>> touch the...
>>
>>> @@ -4888,6 +4888,8 @@ sub foreach_volid {
>>
>> $volhash->{$volid}->{referenced_in_config} = 1 if !defined($snapname);
>>
>> ...line in PVE/QemuServer.pm's foreach_volid(), so is_pending being 1
>> means referenced_in_config is also 1 and there's no way to distinguish
>> via attributes if a volume is only in pending or both in pending and the
>> current config.
>>
>> Not sure if we should even bother to distinguish between more than
>> 'config' and 'snapshot' anymore. Everything that's just in a snapshot
>> gets 'snapshot' and everything that's in the current config (including
>> pending) gets 'config'. No need for 'storage' at all.
>>
> 
> Argh, it's actually not just used informationally, but affects real
> logic too :(
> 
>          foreach my $volid (sort keys %$local_volumes) {
>              my $ref = $local_volumes->{$volid}->{ref};
>              if ($self->{running} && $ref eq 'config') {
>                  $local_volumes->{$volid}->{migration_mode} = 'online';
> 
> So we do need a real way to distinguish 'only in pending' and 'both in
> pending and current config' after all here.

Hmm yeah. I'll look into it and what tests to add.