[pve-devel] [PATCH-SERIES v2 qemu-server] Cleanup migration code and improve migration disk cleanup

[pve-devel] [PATCH-SERIES v2 qemu-server] Cleanup migration code and improve migration disk cleanup

[Fabian Ebner]

This series intends to make the migration code more readable by
simplyfing/unifying how we keep track of local volumes and splitting up
sync_disks into multiple subroutines.

This is done by keeping more information within the hash of local_volumes we
obtain in the very beginning and re-using it later. Also a method to filter by
migration/replication type is introduced, making it possible to get rid of some
special-case handling when iterating over local volumes.

Still more should be possible, but the series is getting big, so I stopped here.
Namely, $self->{local_volumes}, $self->{volume_map} and $self->{target_drive}
still have some overlap and it might be possible to merge them somehow.

Also it might make sense to put abstract more things to the common code, one
example is to make target storage mapping available for containers too.


Changes from v1:
    * dropped already applied patches
    * rebased
    * prefix commit messages with 'migration' (and 'test')
    * increased test coverage a little, see patch #1 and patch #13
    * reordered patches #8+#9 to avoid temporary breakage, see
      patches #8 and patch #13 (added test-case) for the details
    * add patch to fix bandwidth limit calculation
    * add patch to split out run_replication from scan_local_volumes
    * add patch to improve error handling when block job finishing fails


Fabian Ebner (13):
  test: migration: add parse_volume_id calls
  migration: split sync_disks into two functions
  migration: avoid re-scanning all volumes
  migration: split out config_update_local_disksizes from
    scan_local_volumes
  migration: fix calculation of bandwith limit for non-disk migration
  migration: save targetstorage and bwlimit in local_volumes hash and
    re-use information
  migration: add nbd migrated volumes to volume_map earlier
  migration: simplify removal of local volumes and get rid of
    self->{volumes}
  migration: cleanup_remotedisks: simplify and include more disks
  migration: use storage_migration for checks instead of
    online_local_volumes
  migration: keep track of replicated volumes via local_volumes
  migration: split out replication from scan_local_volumes
  migration: move finishing block jobs to phase2 for better/uniform
    error handling

 PVE/QemuMigrate.pm                    | 401 ++++++++++++++------------
 PVE/QemuServer.pm                     |   2 +
 test/MigrationTest/QemuMigrateMock.pm |   9 +
 test/MigrationTest/QmMock.pm          |   2 +
 test/MigrationTest/Shared.pm          |   4 +
 test/run_qemu_migrate_tests.pl        |  40 ++-
 6 files changed, 267 insertions(+), 191 deletions(-)

-- 
2.20.1

[pve-devel] [PATCH v2 qemu-server 01/13] test: migration: add parse_volume_id calls

[Fabian Ebner]

so it fails when something bad comes in.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v2, added because I ran into a problem with an early version of patch #12
which wasn't detected by the tests. See patch #12 for the details.

 test/MigrationTest/QemuMigrateMock.pm | 3 +++
 test/MigrationTest/QmMock.pm          | 2 ++
 test/MigrationTest/Shared.pm          | 4 ++++
 3 files changed, 9 insertions(+)

diff --git a/test/MigrationTest/QemuMigrateMock.pm b/test/MigrationTest/QemuMigrateMock.pm
index efd6130..2d424e0 100644
--- a/test/MigrationTest/QemuMigrateMock.pm
+++ b/test/MigrationTest/QemuMigrateMock.pm
@@ -221,6 +221,8 @@ $MigrationTest::Shared::storage_module->mock(
     vdisk_free => sub {
 	my ($scfg, $volid) = @_;
 
+	PVE::Storage::parse_volume_id($volid);
+
 	die "vdisk_free '$volid' error\n" if defined($fail_config->{vdisk_free})
 					  && $fail_config->{vdisk_free} eq $volid;
 
@@ -292,6 +294,7 @@ $MigrationTest::Shared::tools_module->mock(
 		    $cmd = shift @{$cmd_tail};
 		    if ($cmd eq 'free') {
 			my $volid = shift @{$cmd_tail};
+			PVE::Storage::parse_volume_id($volid);
 			return 1 if $fail_config->{ssh_pvesm_free}
 				 && $fail_config->{ssh_pvesm_free} eq $volid;
 			MigrationTest::Shared::remove_target_volid($volid);
diff --git a/test/MigrationTest/QmMock.pm b/test/MigrationTest/QmMock.pm
index 2f1fffc..2d5d5c6 100644
--- a/test/MigrationTest/QmMock.pm
+++ b/test/MigrationTest/QmMock.pm
@@ -86,6 +86,8 @@ $MigrationTest::Shared::storage_module->mock(
 	    $volid = "${storeid}:${name_without_extension}";
 	}
 
+	PVE::Storage::parse_volume_id($volid);
+
 	die "vdisk_alloc '$volid' error\n" if $fail_config->{vdisk_alloc}
 					   && $fail_config->{vdisk_alloc} eq $volid;
 
diff --git a/test/MigrationTest/Shared.pm b/test/MigrationTest/Shared.pm
index d7aeb36..e48b82c 100644
--- a/test/MigrationTest/Shared.pm
+++ b/test/MigrationTest/Shared.pm
@@ -23,6 +23,8 @@ my $test_vmid = $migrate_params->{vmid};
 sub add_target_volid {
     my ($volid) = @_;
 
+    PVE::Storage::parse_volume_id($volid);
+
     lock_file_full("${RUN_DIR_PATH}/target_volids.lock", undef, 0, sub {
 	my $target_volids = decode_json(file_get_contents("${RUN_DIR_PATH}/target_volids"));
 	die "target volid already present " if defined($target_volids->{$volid});
@@ -35,6 +37,8 @@ sub add_target_volid {
 sub remove_target_volid {
     my ($volid) = @_;
 
+    PVE::Storage::parse_volume_id($volid);
+
     lock_file_full("${RUN_DIR_PATH}/target_volids.lock", undef, 0, sub {
 	my $target_volids = decode_json(file_get_contents("${RUN_DIR_PATH}/target_volids"));
 	die "target volid does not exist " if !defined($target_volids->{$volid});
-- 
2.20.1

[pve-devel] [PATCH v2 qemu-server 02/13] migration: split sync_disks into two functions

[Fabian Ebner]

by making local_volumes class-accessible. One functions is for scanning all local
volumes and one is for actually syncing offline volumes via storage_migrate. The
exception is replicated volumes, this still happens during the scan for now.

Also introduce a filter_local_volumes helper, to makes life easier.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Changes from v1:
    * rebase (include the deactivate_volumes block that didn't exist back then)

 PVE/QemuMigrate.pm | 108 +++++++++++++++++++++++++++++----------------
 1 file changed, 69 insertions(+), 39 deletions(-)

diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index 5c019fc..0cc13df 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -9,6 +9,7 @@ use POSIX qw( WNOHANG );
 use Time::HiRes qw( usleep );
 
 use PVE::Cluster;
+use PVE::GuestHelpers qw(safe_string_ne);
 use PVE::INotify;
 use PVE::RPCEnvironment;
 use PVE::Replication;
@@ -357,7 +358,7 @@ sub prepare {
     return $running;
 }
 
-sub sync_disks {
+sub scan_local_volumes {
     my ($self, $vmid) = @_;
 
     my $conf = $self->{vmconf};
@@ -366,12 +367,13 @@ sub sync_disks {
     # and their old_id => new_id pairs
     $self->{volumes} = [];
     $self->{volume_map} = {};
+    $self->{local_volumes} = {};
 
     my $storecfg = $self->{storecfg};
     eval {
 
 	# found local volumes and their origin
-	my $local_volumes = {};
+	my $local_volumes = $self->{local_volumes};
 	my $local_volumes_errors = {};
 	my $other_errors = [];
 	my $abort = 0;
@@ -608,11 +610,7 @@ sub sync_disks {
 	    PVE::QemuServer::update_efidisk_size($conf);
 	}
 
-	$self->log('info', "copying local disk images") if scalar(%$local_volumes);
-
 	foreach my $volid (sort keys %$local_volumes) {
-	    my ($sid, $volname) = PVE::Storage::parse_volume_id($volid);
-	    my $targetsid = PVE::QemuServer::map_storage($self->{opts}->{storagemap}, $sid);
 	    my $ref = $local_volumes->{$volid}->{ref};
 	    if ($self->{running} && $ref eq 'config') {
 		push @{$self->{online_local_volumes}}, $volid;
@@ -624,39 +622,70 @@ sub sync_disks {
 	    } else {
 		next if $self->{replicated_volumes}->{$volid};
 		push @{$self->{volumes}}, $volid;
-		my $opts = $self->{opts};
-		# use 'migrate' limit for transfer to other node
-		my $bwlimit = PVE::Storage::get_bandwidth_limit('migration', [$targetsid, $sid], $opts->{bwlimit});
-		# JSONSchema and get_bandwidth_limit use kbps - storage_migrate bps
-		$bwlimit = $bwlimit * 1024 if defined($bwlimit);
-
-		my $storage_migrate_opts = {
-		    'ratelimit_bps' => $bwlimit,
-		    'insecure' => $opts->{migration_type} eq 'insecure',
-		    'with_snapshots' => $local_volumes->{$volid}->{snapshots},
-		    'allow_rename' => !$local_volumes->{$volid}->{is_vmstate},
-		};
-
-		my $logfunc = sub { $self->log('info', $_[0]); };
-		my $new_volid = eval {
-		    PVE::Storage::storage_migrate($storecfg, $volid, $self->{ssh_info},
-						  $targetsid, $storage_migrate_opts, $logfunc);
-		};
-		if (my $err = $@) {
-		    die "storage migration for '$volid' to storage '$targetsid' failed - $err\n";
-		}
-
-		$self->{volume_map}->{$volid} = $new_volid;
-		$self->log('info', "volume '$volid' is '$new_volid' on the target\n");
-
-		eval { PVE::Storage::deactivate_volumes($storecfg, [$volid]); };
-		if (my $err = $@) {
-		    $self->log('warn', $err);
-		}
+		$local_volumes->{$volid}->{migration_mode} = 'offline';
 	    }
 	}
     };
-    die "Failed to sync data - $@" if $@;
+    die "Problem found while scanning volumes - $@" if $@;
+}
+
+sub filter_local_volumes {
+    my ($self, $migration_mode) = @_;
+
+    my $volumes = $self->{local_volumes};
+    my @filtered_volids;
+
+    foreach my $volid (sort keys %{$volumes}) {
+	next if defined($migration_mode) && safe_string_ne($volumes->{$volid}->{migration_mode}, $migration_mode);
+	push @filtered_volids, $volid;
+    }
+
+    return @filtered_volids;
+}
+
+sub sync_offline_local_volumes {
+    my ($self) = @_;
+
+    my $local_volumes = $self->{local_volumes};
+    my @volids = $self->filter_local_volumes('offline');
+
+    my $storecfg = $self->{storecfg};
+    my $opts = $self->{opts};
+
+    $self->log('info', "copying local disk images") if scalar(@volids);
+
+    foreach my $volid (@volids) {
+	my ($sid, $volname) = PVE::Storage::parse_volume_id($volid);
+	my $targetsid = PVE::QemuServer::map_storage($self->{opts}->{storagemap}, $sid);
+	# use 'migration' limit for transfer to other node
+	my $bwlimit = PVE::Storage::get_bandwidth_limit('migration', [$targetsid, $sid], $opts->{bwlimit});
+	# JSONSchema and get_bandwidth_limit use kbps - storage_migrate bps
+	$bwlimit = $bwlimit * 1024 if defined($bwlimit);
+
+	my $storage_migrate_opts = {
+	    'ratelimit_bps' => $bwlimit,
+	    'insecure' => $opts->{migration_type} eq 'insecure',
+	    'with_snapshots' => $local_volumes->{$volid}->{snapshots},
+	    'allow_rename' => !$local_volumes->{$volid}->{is_vmstate},
+	};
+
+	my $logfunc = sub { $self->log('info', $_[0]); };
+	my $new_volid = eval {
+	    PVE::Storage::storage_migrate($storecfg, $volid, $self->{ssh_info},
+					  $targetsid, $storage_migrate_opts, $logfunc);
+	};
+	if (my $err = $@) {
+	    die "storage migration for '$volid' to storage '$targetsid' failed - $err\n";
+	}
+
+	$self->{volume_map}->{$volid} = $new_volid;
+	$self->log('info', "volume '$volid' is '$new_volid' on the target\n");
+
+	eval { PVE::Storage::deactivate_volumes($storecfg, [$volid]); };
+	if (my $err = $@) {
+	    $self->log('warn', $err);
+	}
+    }
 }
 
 sub cleanup_remotedisks {
@@ -704,11 +733,12 @@ sub phase1 {
     $conf->{lock} = 'migrate';
     PVE::QemuConfig->write_config($vmid, $conf);
 
-    sync_disks($self, $vmid);
-
-    # sync_disks fixes disk sizes to match their actual size, write changes so
+    # scan_local_volumes fixes disk sizes to match their actual size, write changes so
     # target allocates correct volumes
+    $self->scan_local_volumes($vmid);
     PVE::QemuConfig->write_config($vmid, $conf);
+
+    $self->sync_offline_local_volumes();
 };
 
 sub phase1_cleanup {
-- 
2.20.1

[pve-devel] [PATCH v2 qemu-server 03/13] migration: avoid re-scanning all volumes

[Fabian Ebner]

by using the information obtained in the first scan. This
also makes sure we only scan local storages.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

No changes from v1

 PVE/QemuMigrate.pm | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index 0cc13df..d0295f8 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -415,6 +415,7 @@ sub scan_local_volumes {
 		my ($volid, $sid, $volinfo) = @_;
 
 		$local_volumes->{$volid}->{ref} = 'storage';
+		$local_volumes->{$volid}->{size} = $volinfo->{size};
 
 		# 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
@@ -587,17 +588,15 @@ sub scan_local_volumes {
 	}
 
 	# sizes in config have to be accurate for remote node to correctly
-	# allocate disks, rescan to be sure
-	my $volid_hash = PVE::QemuServer::scan_volids($storecfg, $vmid);
+	# allocate disks
 	PVE::QemuConfig->foreach_volume($conf, sub {
 	    my ($key, $drive) = @_;
 	    return if $key eq 'efidisk0'; # skip efidisk, will be handled later
 
 	    my $volid = $drive->{file};
 	    return if !defined($local_volumes->{$volid}); # only update sizes for local volumes
-	    return if !defined($volid_hash->{$volid});
 
-	    my ($updated, $msg) = PVE::QemuServer::Drive::update_disksize($drive, $volid_hash->{$volid}->{size});
+	    my ($updated, $msg) = PVE::QemuServer::Drive::update_disksize($drive, $local_volumes->{$volid}->{size});
 	    if (defined($updated)) {
 		$conf->{$key} = PVE::QemuServer::print_drive($updated);
 		$self->log('info', "drive '$key': $msg");
-- 
2.20.1

[pve-devel] [PATCH v2 qemu-server 04/13] migration: split out config_update_local_disksizes from scan_local_volumes

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

No changes from v1

 PVE/QemuMigrate.pm | 55 ++++++++++++++++++++++++++--------------------
 1 file changed, 31 insertions(+), 24 deletions(-)

diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index d0295f8..455581c 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -587,28 +587,6 @@ sub scan_local_volumes {
 	       'PVE::QemuConfig', $self->{replication_jobcfg}, $start_time, $start_time, $logfunc);
 	}
 
-	# sizes in config have to be accurate for remote node to correctly
-	# allocate disks
-	PVE::QemuConfig->foreach_volume($conf, sub {
-	    my ($key, $drive) = @_;
-	    return if $key eq 'efidisk0'; # skip efidisk, will be handled later
-
-	    my $volid = $drive->{file};
-	    return if !defined($local_volumes->{$volid}); # only update sizes for local volumes
-
-	    my ($updated, $msg) = PVE::QemuServer::Drive::update_disksize($drive, $local_volumes->{$volid}->{size});
-	    if (defined($updated)) {
-		$conf->{$key} = PVE::QemuServer::print_drive($updated);
-		$self->log('info', "drive '$key': $msg");
-	    }
-	});
-
-	# we want to set the efidisk size in the config to the size of the
-	# real OVMF_VARS.fd image, else we can create a too big image, which does not work
-	if (defined($conf->{efidisk0})) {
-	    PVE::QemuServer::update_efidisk_size($conf);
-	}
-
 	foreach my $volid (sort keys %$local_volumes) {
 	    my $ref = $local_volumes->{$volid}->{ref};
 	    if ($self->{running} && $ref eq 'config') {
@@ -628,6 +606,33 @@ sub scan_local_volumes {
     die "Problem found while scanning volumes - $@" if $@;
 }
 
+sub config_update_local_disksizes {
+    my ($self) = @_;
+
+    my $conf = $self->{vmconf};
+    my $local_volumes = $self->{local_volumes};
+
+    PVE::QemuConfig->foreach_volume($conf, sub {
+	my ($key, $drive) = @_;
+	return if $key eq 'efidisk0'; # skip efidisk, will be handled later
+
+	my $volid = $drive->{file};
+	return if !defined($local_volumes->{$volid}); # only update sizes for local volumes
+
+	my ($updated, $msg) = PVE::QemuServer::Drive::update_disksize($drive, $local_volumes->{$volid}->{size});
+	if (defined($updated)) {
+	    $conf->{$key} = PVE::QemuServer::print_drive($updated);
+	    $self->log('info', "drive '$key': $msg");
+	}
+    });
+
+    # we want to set the efidisk size in the config to the size of the
+    # real OVMF_VARS.fd image, else we can create a too big image, which does not work
+    if (defined($conf->{efidisk0})) {
+	PVE::QemuServer::update_efidisk_size($conf);
+    }
+}
+
 sub filter_local_volumes {
     my ($self, $migration_mode) = @_;
 
@@ -732,9 +737,11 @@ sub phase1 {
     $conf->{lock} = 'migrate';
     PVE::QemuConfig->write_config($vmid, $conf);
 
-    # scan_local_volumes fixes disk sizes to match their actual size, write changes so
-    # target allocates correct volumes
     $self->scan_local_volumes($vmid);
+
+    # fix disk sizes to match their actual size and write changes,
+    # so that the target allocates the correct volumes
+    $self->config_update_local_disksizes();
     PVE::QemuConfig->write_config($vmid, $conf);
 
     $self->sync_offline_local_volumes();
-- 
2.20.1

[pve-devel] [PATCH v2 qemu-server 05/13] migration: fix calculation of bandwith limit for non-disk migration

[Fabian Ebner]

The case with:
1. no generic 'migration' limit from the storage plugin
2. a migrate_speed limit in the VM config
was broken. It would assign 0 to migrate_speed when picking the minimum value
and then default to the default value. Fix it by checking if bwlimit is 0
before picking the minimum.

Also, make it a bit more readable by avoiding the trick of //-assigning bwlimit
before the units match up and relying on getting back the original bwlimit value
as the minimum. Instead, only ||-assign after the units match up and don't rely
on other things.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v2

 PVE/QemuMigrate.pm | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index 455581c..0522208 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -961,11 +961,15 @@ sub phase2 {
     # migrate speed can be set via bwlimit (datacenter.cfg and API) and via the
     # migrate_speed parameter in qm.conf - take the lower of the two.
     my $bwlimit = PVE::Storage::get_bandwidth_limit('migration', undef, $opt_bwlimit) // 0;
-    my $migrate_speed = $conf->{migrate_speed} // $bwlimit;
+    my $migrate_speed = $conf->{migrate_speed} // 0;
     # migrate_speed is in MB/s, bwlimit in KB/s
     $migrate_speed *= 1024;
 
-    $migrate_speed = ($bwlimit < $migrate_speed) ? $bwlimit : $migrate_speed;
+    if ($bwlimit && $migrate_speed) {
+	$migrate_speed = ($bwlimit < $migrate_speed) ? $bwlimit : $migrate_speed;
+    } else {
+	$migrate_speed ||= $bwlimit;
+    }
 
     # always set migrate speed (overwrite kvm default of 32m) we set a very high
     # default of 8192m which is basically unlimited
-- 
2.20.1

[pve-devel] [PATCH v2 qemu-server 06/13] migration: save targetstorage and bwlimit in local_volumes hash and re-use information

[Fabian Ebner]

It is enough to call get_bandwith_limit once for each source_storage.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Changes from v1:
    * avoid a long line

 PVE/QemuMigrate.pm | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index 0522208..db68371 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -411,11 +411,19 @@ sub scan_local_volumes {
 		    if !$target_scfg->{content}->{images};
 	    }
 
+	    my $bwlimit = PVE::Storage::get_bandwidth_limit(
+		'migration',
+		[$targetsid, $storeid],
+		$self->{opts}->{bwlimit},
+	    );
+
 	    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
@@ -659,12 +667,9 @@ sub sync_offline_local_volumes {
     $self->log('info', "copying local disk images") if scalar(@volids);
 
     foreach my $volid (@volids) {
-	my ($sid, $volname) = PVE::Storage::parse_volume_id($volid);
-	my $targetsid = PVE::QemuServer::map_storage($self->{opts}->{storagemap}, $sid);
-	# use 'migration' limit for transfer to other node
-	my $bwlimit = PVE::Storage::get_bandwidth_limit('migration', [$targetsid, $sid], $opts->{bwlimit});
-	# JSONSchema and get_bandwidth_limit use kbps - storage_migrate bps
-	$bwlimit = $bwlimit * 1024 if defined($bwlimit);
+	my $targetsid = $local_volumes->{$volid}->{targetsid};
+	my $bwlimit = $local_volumes->{$volid}->{bwlimit};
+	$bwlimit = $bwlimit * 1024 if defined($bwlimit); # storage_migrate uses bps
 
 	my $storage_migrate_opts = {
 	    'ratelimit_bps' => $bwlimit,
@@ -777,6 +782,7 @@ sub phase2 {
     my ($self, $vmid) = @_;
 
     my $conf = $self->{vmconf};
+    my $local_volumes = $self->{local_volumes};
 
     $self->log('info', "starting VM $vmid on remote node '$self->{node}'");
 
@@ -911,8 +917,6 @@ sub phase2 {
 
     my $start = time();
 
-    my $opt_bwlimit = $self->{opts}->{bwlimit};
-
     if (defined($self->{online_local_volumes})) {
 	$self->{storage_migration} = 1;
 	$self->{storage_migration_jobs} = {};
@@ -930,10 +934,7 @@ sub phase2 {
 	    my $source_volid = $source_drive->{file};
 	    my $target_volid = $target_drive->{file};
 
-	    my $source_sid = PVE::Storage::Plugin::parse_volume_id($source_volid);
-	    my $target_sid = PVE::Storage::Plugin::parse_volume_id($target_volid);
-
-	    my $bwlimit = PVE::Storage::get_bandwidth_limit('migration', [$source_sid, $target_sid], $opt_bwlimit);
+	    my $bwlimit = $local_volumes->{$source_volid}->{bwlimit};
 	    my $bitmap = $target->{bitmap};
 
 	    $self->log('info', "$drive: start migration to $nbd_uri");
@@ -960,7 +961,7 @@ sub phase2 {
 
     # migrate speed can be set via bwlimit (datacenter.cfg and API) and via the
     # migrate_speed parameter in qm.conf - take the lower of the two.
-    my $bwlimit = PVE::Storage::get_bandwidth_limit('migration', undef, $opt_bwlimit) // 0;
+    my $bwlimit = PVE::Storage::get_bandwidth_limit('migration', undef, $self->{opts}->{bwlimit}) // 0;
     my $migrate_speed = $conf->{migrate_speed} // 0;
     # migrate_speed is in MB/s, bwlimit in KB/s
     $migrate_speed *= 1024;
-- 
2.20.1

[pve-devel] [PATCH v2 qemu-server 07/13] migration: add nbd migrated volumes to volume_map earlier

[Fabian Ebner]

and avoid a little bit of duplication by creating a helper

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

No changes from v1

 PVE/QemuMigrate.pm | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index db68371..b10638a 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -846,6 +846,22 @@ sub phase2 {
 	}
     }
 
+    my $handle_storage_migration_listens = sub {
+	my ($drive_key, $drivestr, $nbd_uri) = @_;
+
+	$self->{stopnbd} = 1;
+	$self->{target_drive}->{$drive_key}->{drivestr} = $drivestr;
+	$self->{target_drive}->{$drive_key}->{nbd_uri} = $nbd_uri;
+
+	my $source_drive = PVE::QemuServer::parse_drive($drive_key, $conf->{$drive_key});
+	my $target_drive = PVE::QemuServer::parse_drive($drive_key, $drivestr);
+	my $source_volid = $source_drive->{file};
+	my $target_volid = $target_drive->{file};
+
+	$self->{volume_map}->{$source_volid} = $target_volid;
+	$self->log('info', "volume '$source_volid' is '$target_volid' on the target\n");
+    };
+
     my $target_replicated_volumes = {};
 
     # Note: We try to keep $spice_ticket secret (do not pass via command line parameter)
@@ -877,9 +893,7 @@ sub phase2 {
 	    my $targetdrive = $3;
 	    $targetdrive =~ s/drive-//g;
 
-	    $self->{stopnbd} = 1;
-	    $self->{target_drive}->{$targetdrive}->{drivestr} = $drivestr;
-	    $self->{target_drive}->{$targetdrive}->{nbd_uri} = $nbd_uri;
+	    $handle_storage_migration_listens->($targetdrive, $drivestr, $nbd_uri);
 	} elsif ($line =~ m!^storage migration listens on nbd:unix:(/run/qemu-server/(\d+)_nbd\.migrate):exportname=(\S+) volume:(\S+)$!) {
 	    my $drivestr = $4;
 	    die "Destination UNIX socket's VMID does not match source VMID" if $vmid ne $2;
@@ -888,9 +902,7 @@ sub phase2 {
 	    my $targetdrive = $3;
 	    $targetdrive =~ s/drive-//g;
 
-	    $self->{stopnbd} = 1;
-	    $self->{target_drive}->{$targetdrive}->{drivestr} = $drivestr;
-	    $self->{target_drive}->{$targetdrive}->{nbd_uri} = $nbd_uri;
+	    $handle_storage_migration_listens->($targetdrive, $drivestr, $nbd_uri);
 	    $unix_socket_info->{$nbd_unix_addr} = 1;
 	} elsif ($line =~ m/^re-using replicated volume: (\S+) - (.*)$/) {
 	    my $drive = $1;
@@ -929,19 +941,13 @@ sub phase2 {
 	    my $nbd_uri = $target->{nbd_uri};
 
 	    my $source_drive = PVE::QemuServer::parse_drive($drive, $conf->{$drive});
-	    my $target_drive = PVE::QemuServer::parse_drive($drive, $target->{drivestr});
-
 	    my $source_volid = $source_drive->{file};
-	    my $target_volid = $target_drive->{file};
 
 	    my $bwlimit = $local_volumes->{$source_volid}->{bwlimit};
 	    my $bitmap = $target->{bitmap};
 
 	    $self->log('info', "$drive: start migration to $nbd_uri");
 	    PVE::QemuServer::qemu_drive_mirror($vmid, $drive, $nbd_uri, $vmid, undef, $self->{storage_migration_jobs}, 'skip', undef, $bwlimit, $bitmap);
-
-	    $self->{volume_map}->{$source_volid} = $target_volid;
-	    $self->log('info', "volume '$source_volid' is '$target_volid' on the target\n");
 	}
     }
 
-- 
2.20.1

[pve-devel] [PATCH v2 qemu-server 08/13] migration: simplify removal of local volumes and get rid of self->{volumes}

[Fabian Ebner]

This also changes the behavior to remove the local copies of offline migrated
volumes only after the migration has finished successfully (this is relevant
for mixed settings, e.g. online migration with unused/vmstate disks).

local_volumes contains both, the volumes previously in $self->{volumes}
and the volumes in $self->{online_local_volumes}, and hence is the place
to look for which volumes we need to remove. Of course, replicated
volumes still need to be skipped.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Changes from v1:
    * changed the order of this and next patch to avoid temporary breakage in an
      edge case if only the old #8 would be applied: if we died in
      phase3_cleanup on the qemu_drive_mirror_monitor call unused disks could
      get lost, because they were already cleaned up in phase3. Now this patch
      (old #9) comes first, making sure the cleanup of local disks happens in
      phase3_cleanup after successful migration. See also the test in patch #13


 PVE/QemuMigrate.pm | 51 +++++++++++++++-------------------------------
 1 file changed, 16 insertions(+), 35 deletions(-)

diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index b10638a..5d92028 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -365,7 +365,6 @@ sub scan_local_volumes {
 
     # local volumes which have been copied
     # and their old_id => new_id pairs
-    $self->{volumes} = [];
     $self->{volume_map} = {};
     $self->{local_volumes} = {};
 
@@ -599,14 +598,10 @@ sub scan_local_volumes {
 	    my $ref = $local_volumes->{$volid}->{ref};
 	    if ($self->{running} && $ref eq 'config') {
 		push @{$self->{online_local_volumes}}, $volid;
-	    } elsif ($ref eq 'generated') {
-		die "can't live migrate VM with local cloudinit disk. use a shared storage instead\n" if $self->{running};
-		# skip all generated volumes but queue them for deletion in phase3_cleanup
-		push @{$self->{volumes}}, $volid;
-		next;
+	    } elsif ($self->{running} && $ref eq 'generated') {
+		die "can't live migrate VM with local cloudinit disk. use a shared storage instead\n";
 	    } else {
 		next if $self->{replicated_volumes}->{$volid};
-		push @{$self->{volumes}}, $volid;
 		$local_volumes->{$volid}->{migration_mode} = 'offline';
 	    }
 	}
@@ -764,8 +759,10 @@ sub phase1_cleanup {
 	$self->log('err', $err);
     }
 
-    if ($self->{volumes}) {
-	foreach my $volid (@{$self->{volumes}}) {
+    my @volids = $self->filter_local_volumes('offline');
+    if (scalar(@volids)) {
+	foreach my $volid (@volids) {
+	    next if defined($self->{replicated_volumes}->{$volid});
 	    $self->log('err', "found stale volume copy '$volid' on node '$self->{node}'");
 	    # fixme: try to remove ?
 	}
@@ -1198,18 +1195,7 @@ sub phase2_cleanup {
 sub phase3 {
     my ($self, $vmid) = @_;
 
-    my $volids = $self->{volumes};
-    return if $self->{phase2errors};
-
-    # destroy local copies
-    foreach my $volid (@$volids) {
-	eval { PVE::Storage::vdisk_free($self->{storecfg}, $volid); };
-	if (my $err = $@) {
-	    $self->log('err', "removing local copy of '$volid' failed - $err");
-	    $self->{errors} = 1;
-	    last if $err =~ /^interrupted by signal$/;
-	}
-    }
+    return;
 }
 
 sub phase3_cleanup {
@@ -1334,22 +1320,17 @@ sub phase3_cleanup {
 	$self->{errors} = 1;
     }
 
-    if($self->{storage_migration}) {
-	# destroy local copies
-	my $volids = $self->{online_local_volumes};
-
-	foreach my $volid (@$volids) {
-	    # keep replicated volumes!
-	    next if $self->{replicated_volumes}->{$volid};
+    # destroy local copies
+    foreach my $volid (keys %{$self->{local_volumes}}) {
+	# keep replicated volumes!
+	next if $self->{replicated_volumes}->{$volid};
 
-	    eval { PVE::Storage::vdisk_free($self->{storecfg}, $volid); };
-	    if (my $err = $@) {
-		$self->log('err', "removing local copy of '$volid' failed - $err");
-		$self->{errors} = 1;
-		last if $err =~ /^interrupted by signal$/;
-	    }
+	eval { PVE::Storage::vdisk_free($self->{storecfg}, $volid); };
+	if (my $err = $@) {
+	    $self->log('err', "removing local copy of '$volid' failed - $err");
+	    $self->{errors} = 1;
+	    last if $err =~ /^interrupted by signal$/;
 	}
-
     }
 
     # clear migrate lock
-- 
2.20.1

[pve-devel] [PATCH v2 qemu-server 09/13] migration: cleanup_remotedisks: simplify and include more disks

[Fabian Ebner]

Namely, those migrated with storage_migrate by using the information from
volume_map. Call cleanup_remotedisks in phase1_cleanup as well, because that's
where we end if sync_offline_local_volumes fails, and some disks might already
have been transfered successfully. Note that the local disks are still here, so
this is fine.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Changes from v1:
    * order changed, see previous patch
    * get rid of a fixme in the tests

 PVE/QemuMigrate.pm             | 22 ++++++----------------
 test/run_qemu_migrate_tests.pl |  5 +----
 2 files changed, 7 insertions(+), 20 deletions(-)

diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index 5d92028..94f3328 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -695,16 +695,11 @@ sub sync_offline_local_volumes {
 sub cleanup_remotedisks {
     my ($self) = @_;
 
-    foreach my $target_drive (keys %{$self->{target_drive}}) {
-	my $drivestr = $self->{target_drive}->{$target_drive}->{drivestr};
-	next if !defined($drivestr);
-
-	my $drive = PVE::QemuServer::parse_drive($target_drive, $drivestr);
-
+    foreach my $volid (values %{$self->{volume_map}}) {
 	# don't clean up replicated disks!
-	next if defined($self->{replicated_volumes}->{$drive->{file}});
+	next if defined($self->{replicated_volumes}->{$volid});
 
-	my ($storeid, $volname) = PVE::Storage::parse_volume_id($drive->{file});
+	my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid);
 
 	my $cmd = [@{$self->{rem_ssh}}, 'pvesm', 'free', "$storeid:$volname"];
 
@@ -759,20 +754,15 @@ sub phase1_cleanup {
 	$self->log('err', $err);
     }
 
-    my @volids = $self->filter_local_volumes('offline');
-    if (scalar(@volids)) {
-	foreach my $volid (@volids) {
-	    next if defined($self->{replicated_volumes}->{$volid});
-	    $self->log('err', "found stale volume copy '$volid' on node '$self->{node}'");
-	    # fixme: try to remove ?
-	}
+    eval { $self->cleanup_remotedisks() };
+    if (my $err = $@) {
+	$self->log('err', $err);
     }
 
     eval { $self->cleanup_bitmaps() };
     if (my $err =$@) {
 	$self->log('err', $err);
     }
-
 }
 
 sub phase2 {
diff --git a/test/run_qemu_migrate_tests.pl b/test/run_qemu_migrate_tests.pl
index 67b9d0e..4f7f021 100755
--- a/test/run_qemu_migrate_tests.pl
+++ b/test/run_qemu_migrate_tests.pl
@@ -1465,7 +1465,6 @@ my $tests = [
 	},
     },
     {
-	# FIXME also cleanup remote disks when failing this early
 	name => '149_storage_migrate_fail',
 	target => 'pve1',
 	vmid => 149,
@@ -1482,9 +1481,7 @@ my $tests = [
 	expect_die => "storage_migrate 'local-lvm:vm-149-disk-0' error",
 	expected => {
 	    source_volids => local_volids_for_vm(149),
-	    target_volids => {
-		'local-dir:149/vm-149-disk-0.qcow2' => 1,
-	    },
+	    target_volids => {},
 	    vm_config => $vm_configs->{149},
 	    vm_status => {
 		running => 0,
-- 
2.20.1

[pve-devel] [PATCH v2 qemu-server 10/13] migration: use storage_migration for checks instead of online_local_volumes

[Fabian Ebner]

Like this we don't need to worry about auto-vivifaction.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

No changes from v1

 PVE/QemuMigrate.pm | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index 94f3328..09289a5 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -597,7 +597,7 @@ sub scan_local_volumes {
 	foreach my $volid (sort keys %$local_volumes) {
 	    my $ref = $local_volumes->{$volid}->{ref};
 	    if ($self->{running} && $ref eq 'config') {
-		push @{$self->{online_local_volumes}}, $volid;
+		$local_volumes->{$volid}->{migration_mode} = 'online';
 	    } elsif ($self->{running} && $ref eq 'generated') {
 		die "can't live migrate VM with local cloudinit disk. use a shared storage instead\n";
 	    } else {
@@ -770,6 +770,9 @@ sub phase2 {
 
     my $conf = $self->{vmconf};
     my $local_volumes = $self->{local_volumes};
+    my @online_local_volumes = $self->filter_local_volumes('online');
+
+    $self->{storage_migration} = 1 if scalar(@online_local_volumes);
 
     $self->log('info', "starting VM $vmid on remote node '$self->{node}'");
 
@@ -810,7 +813,7 @@ sub phase2 {
 	push @$cmd, '--force-cpu', $self->{forcecpu};
     }
 
-    if ($self->{online_local_volumes}) {
+    if ($self->{storage_migration}) {
 	push @$cmd, '--targetstorage', ($self->{opts}->{targetstorage} // '1');
     }
 
@@ -823,14 +826,10 @@ sub phase2 {
     $input .= "nbd_protocol_version: $nbd_protocol_version\n";
 
     my $number_of_online_replicated_volumes = 0;
-
-    # prevent auto-vivification
-    if ($self->{online_local_volumes}) {
-	foreach my $volid (@{$self->{online_local_volumes}}) {
-	    next if !$self->{replicated_volumes}->{$volid};
-	    $number_of_online_replicated_volumes++;
-	    $input .= "replicated_volume: $volid\n";
-	}
+    foreach my $volid (@online_local_volumes) {
+	next if !$self->{replicated_volumes}->{$volid};
+	$number_of_online_replicated_volumes++;
+	$input .= "replicated_volume: $volid\n";
     }
 
     my $handle_storage_migration_listens = sub {
@@ -916,13 +915,12 @@ sub phase2 {
 
     my $start = time();
 
-    if (defined($self->{online_local_volumes})) {
-	$self->{storage_migration} = 1;
+    if ($self->{storage_migration}) {
 	$self->{storage_migration_jobs} = {};
 	$self->log('info', "starting storage migration");
 
 	die "The number of local disks does not match between the source and the destination.\n"
-	    if (scalar(keys %{$self->{target_drive}}) != scalar @{$self->{online_local_volumes}});
+	    if (scalar(keys %{$self->{target_drive}}) != scalar(@online_local_volumes));
 	foreach my $drive (keys %{$self->{target_drive}}){
 	    my $target = $self->{target_drive}->{$drive};
 	    my $nbd_uri = $target->{nbd_uri};
-- 
2.20.1

[pve-devel] [PATCH v2 qemu-server 11/13] migration: keep track of replicated volumes via local_volumes

[Fabian Ebner]

by extending filter_local_volumes.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Changes from v1:
    * rebase (new check for is_replicated was introduced in the meantime)
    * move setting of replicated flag to earlier (previously it happend after
      run_replication) so that the next patch works

 PVE/QemuMigrate.pm | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index 09289a5..64f3054 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -9,7 +9,7 @@ use POSIX qw( WNOHANG );
 use Time::HiRes qw( usleep );
 
 use PVE::Cluster;
-use PVE::GuestHelpers qw(safe_string_ne);
+use PVE::GuestHelpers qw(safe_boolean_ne safe_string_ne);
 use PVE::INotify;
 use PVE::RPCEnvironment;
 use PVE::Replication;
@@ -434,6 +434,9 @@ sub scan_local_volumes {
 
 	my $replicatable_volumes = !$self->{replication_jobcfg} ? {}
 	    : PVE::QemuConfig->get_replicatable_volumes($storecfg, $vmid, $conf, 0, 1);
+	foreach my $volid (keys %{$replicatable_volumes}) {
+	    $local_volumes->{$volid}->{replicated} = 1;
+	}
 
 	my $test_volid = sub {
 	    my ($volid, $attr) = @_;
@@ -590,7 +593,7 @@ sub scan_local_volumes {
 
 	    my $start_time = time();
 	    my $logfunc = sub { $self->log('info', shift) };
-	    $self->{replicated_volumes} = PVE::Replication::run_replication(
+	    my $replicated_volumes = PVE::Replication::run_replication(
 	       'PVE::QemuConfig', $self->{replication_jobcfg}, $start_time, $start_time, $logfunc);
 	}
 
@@ -601,7 +604,6 @@ sub scan_local_volumes {
 	    } elsif ($self->{running} && $ref eq 'generated') {
 		die "can't live migrate VM with local cloudinit disk. use a shared storage instead\n";
 	    } else {
-		next if $self->{replicated_volumes}->{$volid};
 		$local_volumes->{$volid}->{migration_mode} = 'offline';
 	    }
 	}
@@ -637,13 +639,14 @@ sub config_update_local_disksizes {
 }
 
 sub filter_local_volumes {
-    my ($self, $migration_mode) = @_;
+    my ($self, $migration_mode, $replicated) = @_;
 
     my $volumes = $self->{local_volumes};
     my @filtered_volids;
 
     foreach my $volid (sort keys %{$volumes}) {
 	next if defined($migration_mode) && safe_string_ne($volumes->{$volid}->{migration_mode}, $migration_mode);
+	next if defined($replicated) && safe_boolean_ne($volumes->{$volid}->{replicated}, $replicated);
 	push @filtered_volids, $volid;
     }
 
@@ -654,7 +657,7 @@ sub sync_offline_local_volumes {
     my ($self) = @_;
 
     my $local_volumes = $self->{local_volumes};
-    my @volids = $self->filter_local_volumes('offline');
+    my @volids = $self->filter_local_volumes('offline', 0);
 
     my $storecfg = $self->{storecfg};
     my $opts = $self->{opts};
@@ -695,9 +698,11 @@ sub sync_offline_local_volumes {
 sub cleanup_remotedisks {
     my ($self) = @_;
 
+    my $local_volumes = $self->{local_volumes};
+
     foreach my $volid (values %{$self->{volume_map}}) {
 	# don't clean up replicated disks!
-	next if defined($self->{replicated_volumes}->{$volid});
+	next if $local_volumes->{$volid}->{replicated};
 
 	my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid);
 
@@ -825,10 +830,8 @@ sub phase2 {
     my $input = $spice_ticket ? "$spice_ticket\n" : "\n";
     $input .= "nbd_protocol_version: $nbd_protocol_version\n";
 
-    my $number_of_online_replicated_volumes = 0;
-    foreach my $volid (@online_local_volumes) {
-	next if !$self->{replicated_volumes}->{$volid};
-	$number_of_online_replicated_volumes++;
+    my @online_replicated_volumes = $self->filter_local_volumes('online', 1);
+    foreach my $volid (@online_replicated_volumes) {
 	$input .= "replicated_volume: $volid\n";
     }
 
@@ -906,7 +909,7 @@ sub phase2 {
 
     die "unable to detect remote migration address\n" if !$raddr;
 
-    if (scalar(keys %$target_replicated_volumes) != $number_of_online_replicated_volumes) {
+    if (scalar(keys %$target_replicated_volumes) != scalar(@online_replicated_volumes)) {
 	die "number of replicated disks on source and target node do not match - target node too old?\n"
     }
 
@@ -1308,11 +1311,10 @@ sub phase3_cleanup {
 	$self->{errors} = 1;
     }
 
-    # destroy local copies
-    foreach my $volid (keys %{$self->{local_volumes}}) {
-	# keep replicated volumes!
-	next if $self->{replicated_volumes}->{$volid};
+    my @not_replicated_volumes = $self->filter_local_volumes(undef, 0);
 
+    # destroy local copies
+    foreach my $volid (@not_replicated_volumes) {
 	eval { PVE::Storage::vdisk_free($self->{storecfg}, $volid); };
 	if (my $err = $@) {
 	    $self->log('err', "removing local copy of '$volid' failed - $err");
-- 
2.20.1

[pve-devel] [PATCH v2 qemu-server 12/13] migration: split out replication from scan_local_volumes

[Fabian Ebner]

and avoid one loop over the config, by extending foreach_volid to include the
drivename.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v2

Having the drivename attribute might also be useful to make the refactor the
target_drive handling, but that's something for a follow-up series

On my first version of this (local, not on the list), I had an
auto-vivification problem introducing 'none' from the CD drive to the
local_volumes hash, which is the reason patch #1 exists

 PVE/QemuMigrate.pm | 86 ++++++++++++++++++++++++++--------------------
 PVE/QemuServer.pm  |  2 ++
 2 files changed, 51 insertions(+), 37 deletions(-)

diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index 64f3054..b503601 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -476,6 +476,9 @@ sub scan_local_volumes {
 
 	    $local_volumes->{$volid}->{is_vmstate} = $attr->{is_vmstate} ? 1 : 0;
 
+	    $local_volumes->{$volid}->{drivename} = $attr->{drivename}
+		if $attr->{drivename};
+
 	    if ($attr->{cdrom}) {
 		if ($volid =~ /vm-\d+-cloudinit/) {
 		    $local_volumes->{$volid}->{ref} = 'generated';
@@ -560,43 +563,6 @@ sub scan_local_volumes {
 	    }
 	}
 
-	if ($self->{replication_jobcfg}) {
-	    if ($self->{running}) {
-
-		my $version = PVE::QemuServer::kvm_user_version();
-		if (!min_version($version, 4, 2)) {
-		    die "can't live migrate VM with replicated volumes, pve-qemu to old (< 4.2)!\n"
-		}
-
-		my $live_replicatable_volumes = {};
-		PVE::QemuConfig->foreach_volume($conf, sub {
-		    my ($ds, $drive) = @_;
-
-		    my $volid = $drive->{file};
-		    $live_replicatable_volumes->{$ds} = $volid
-			if defined($replicatable_volumes->{$volid});
-		});
-		foreach my $drive (keys %$live_replicatable_volumes) {
-		    my $volid = $live_replicatable_volumes->{$drive};
-
-		    my $bitmap = "repl_$drive";
-
-		    # start tracking before replication to get full delta + a few duplicates
-		    $self->log('info', "$drive: start tracking writes using block-dirty-bitmap '$bitmap'");
-		    mon_cmd($vmid, 'block-dirty-bitmap-add', node => "drive-$drive", name => $bitmap);
-
-		    # other info comes from target node in phase 2
-		    $self->{target_drive}->{$drive}->{bitmap} = $bitmap;
-		}
-	    }
-	    $self->log('info', "replicating disk images");
-
-	    my $start_time = time();
-	    my $logfunc = sub { $self->log('info', shift) };
-	    my $replicated_volumes = PVE::Replication::run_replication(
-	       'PVE::QemuConfig', $self->{replication_jobcfg}, $start_time, $start_time, $logfunc);
-	}
-
 	foreach my $volid (sort keys %$local_volumes) {
 	    my $ref = $local_volumes->{$volid}->{ref};
 	    if ($self->{running} && $ref eq 'config') {
@@ -611,6 +577,50 @@ sub scan_local_volumes {
     die "Problem found while scanning volumes - $@" if $@;
 }
 
+sub handle_replication {
+    my ($self, $vmid) = @_;
+
+    my $conf = $self->{vmconf};
+    my $local_volumes = $self->{local_volumes};
+
+    return if !$self->{replication_jobcfg};
+    if ($self->{running}) {
+
+	my $version = PVE::QemuServer::kvm_user_version();
+	if (!min_version($version, 4, 2)) {
+	    die "can't live migrate VM with replicated volumes, pve-qemu to old (< 4.2)!\n"
+	}
+
+	my @live_replicatable_volumes = $self->filter_local_volumes('online', 1);
+	foreach my $volid (@live_replicatable_volumes) {
+	    my $drive = $local_volumes->{$volid}->{drivename};
+	    die "internal error - no drive for '$volid'\n" if !defined($drive);
+
+	    my $bitmap = "repl_$drive";
+
+	    # start tracking before replication to get full delta + a few duplicates
+	    $self->log('info', "$drive: start tracking writes using block-dirty-bitmap '$bitmap'");
+	    mon_cmd($vmid, 'block-dirty-bitmap-add', node => "drive-$drive", name => $bitmap);
+
+	    # other info comes from target node in phase 2
+	    $self->{target_drive}->{$drive}->{bitmap} = $bitmap;
+	}
+    }
+    $self->log('info', "replicating disk images");
+
+    my $start_time = time();
+    my $logfunc = sub { $self->log('info', shift) };
+    my $actual_replicated_volumes = PVE::Replication::run_replication(
+       'PVE::QemuConfig', $self->{replication_jobcfg}, $start_time, $start_time, $logfunc);
+
+    # extra safety check
+    my @replicated_volumes = $self->filter_local_volumes(undef, 1);
+    foreach my $volid (@replicated_volumes) {
+	die "expected volume '$volid' to get replicated, but it wasn't\n"
+	    if !$actual_replicated_volumes->{$volid};
+    }
+}
+
 sub config_update_local_disksizes {
     my ($self) = @_;
 
@@ -744,6 +754,8 @@ sub phase1 {
     $self->config_update_local_disksizes();
     PVE::QemuConfig->write_config($vmid, $conf);
 
+    $self->handle_replication($vmid);
+
     $self->sync_offline_local_volumes();
 };
 
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 9c65d76..c25743b 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -4323,6 +4323,8 @@ sub foreach_volid {
 
 	$volhash->{$volid}->{is_unused} //= 0;
 	$volhash->{$volid}->{is_unused} = 1 if $key =~ /^unused\d+$/;
+
+	$volhash->{$volid}->{drivename} = $key if is_valid_drivename($key);
     };
 
     my $include_opts = {
-- 
2.20.1

[pve-devel] [PATCH v2 qemu-server 13/13] migration: move finishing block jobs to phase2 for better/uniform error handling

[Fabian Ebner]

avoids the possibility to die during phase3_cleanup and instead of needing to
duplicate the cleanup ourselves, benefit from phase2_cleanup doing so.

The duplicate cleanup was also very incomplete: it didn't stop the remote kvm
process (leading to 'VM already running' when trying to migrate again
afterwards), but it removed its disks, and it didn't unlock the config, didn't
close the tunnel and didn't cancel the block-dirty bitmaps.

Since migrate_cancel should do nothing after the (non-storage) migrate process
has completed, even that cleanup step is fine here.

Since phase3 is empty at the moment, the order of operations is still the same.

Also add a test, that would complain about finish_tunnel not being called before
this patch. That test also checks that local disks are not already removed
before finishing the block jobs.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v2

The test would also expose the temporary breakage with the wrong #8/#9 patch
order

With and without this patch: When dying here, i.e. when finishing the
block jobs, the VM is in a blocked state afterwards (postmigrate), because the
(non-storage) migration was successful. Simply resuming it seems to work just
fine, would it be worth to add a (guarded) resume call in the cleanup too?

 PVE/QemuMigrate.pm                    | 23 ++++++++----------
 test/MigrationTest/QemuMigrateMock.pm |  6 +++++
 test/run_qemu_migrate_tests.pl        | 35 +++++++++++++++++++++++++++
 3 files changed, 51 insertions(+), 13 deletions(-)

diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index b503601..435c1f7 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -1134,6 +1134,16 @@ sub phase2 {
 	    die "unable to parse migration status '$stat->{status}' - aborting\n";
 	}
     }
+
+    if ($self->{storage_migration}) {
+	# finish block-job with block-job-cancel, to disconnect source VM from NBD
+	# to avoid it trying to re-establish it. We are in blockjob ready state,
+	# thus, this command changes to it to blockjob complete (see qapi docs)
+	eval { PVE::QemuServer::qemu_drive_mirror_monitor($vmid, undef, $self->{storage_migration_jobs}, 'cancel'); };
+	if (my $err = $@) {
+	    die "Failed to complete storage migration: $err\n";
+	}
+    }
 }
 
 sub phase2_cleanup {
@@ -1209,19 +1219,6 @@ sub phase3_cleanup {
 
     my $tunnel = $self->{tunnel};
 
-    if ($self->{storage_migration}) {
-	# finish block-job with block-job-cancel, to disconnect source VM from NBD
-	# to avoid it trying to re-establish it. We are in blockjob ready state,
-	# thus, this command changes to it to blockjob complete (see qapi docs)
-	eval { PVE::QemuServer::qemu_drive_mirror_monitor($vmid, undef, $self->{storage_migration_jobs}, 'cancel'); };
-
-	if (my $err = $@) {
-	    eval { PVE::QemuServer::qemu_blockjobs_cancel($vmid, $self->{storage_migration_jobs}) };
-	    eval { PVE::QemuMigrate::cleanup_remotedisks($self) };
-	    die "Failed to complete storage migration: $err\n";
-	}
-    }
-
     if ($self->{volume_map}) {
 	my $target_drives = $self->{target_drive};
 
diff --git a/test/MigrationTest/QemuMigrateMock.pm b/test/MigrationTest/QemuMigrateMock.pm
index 2d424e0..8e0b7d0 100644
--- a/test/MigrationTest/QemuMigrateMock.pm
+++ b/test/MigrationTest/QemuMigrateMock.pm
@@ -139,6 +139,12 @@ $MigrationTest::Shared::qemu_server_module->mock(
 	file_set_contents("${RUN_DIR_PATH}/nbd_info", to_json($nbd_info));
     },
     qemu_drive_mirror_monitor => sub {
+	my ($vmid, $vmiddst, $jobs, $completion, $qga) = @_;
+
+	if ($fail_config->{qemu_drive_mirror_monitor} &&
+	    $fail_config->{qemu_drive_mirror_monitor} eq $completion) {
+	    die "qemu_drive_mirror_monitor '$completion' error\n";
+	}
 	return;
     },
     set_migration_caps => sub {
diff --git a/test/run_qemu_migrate_tests.pl b/test/run_qemu_migrate_tests.pl
index 4f7f021..5edea7b 100755
--- a/test/run_qemu_migrate_tests.pl
+++ b/test/run_qemu_migrate_tests.pl
@@ -1444,6 +1444,41 @@ my $tests = [
 	    },
 	},
     },
+    {
+	name => '149_running_unused_block_job_cancel_fail',
+	target => 'pve1',
+	vmid => 149,
+	vm_status => {
+	    running => 1,
+	    runningmachine => 'pc-q35-5.0+pve0',
+	},
+	opts => {
+	    online => 1,
+	    'with-local-disks' => 1,
+	},
+	config_patch => {
+	    scsi1 => undef,
+	    unused0 => 'local-dir:149/vm-149-disk-0.qcow2',
+	},
+	expected_calls => {},
+	expect_die => "qemu_drive_mirror_monitor 'cancel' error",
+	# note that 'cancel' is also used to finish and that's what this test is about
+	fail_config => {
+	    'qemu_drive_mirror_monitor' => 'cancel',
+	},
+	expected => {
+	    source_volids => local_volids_for_vm(149),
+	    target_volids => {},
+	    vm_config => get_patched_config(149, {
+		scsi1 => undef,
+		unused0 => 'local-dir:149/vm-149-disk-0.qcow2',
+	    }),
+	    vm_status => {
+		running => 1,
+		runningmachine => 'pc-q35-5.0+pve0',
+	    },
+	},
+    },
     {
 	name => '149_offline',
 	target => 'pve1',
-- 
2.20.1

Re: [pve-devel] [PATCH-SERIES v2 qemu-server] Cleanup migration code and improve migration disk cleanup

[Fabian Ebner]

Ping for this series.

Patch #13 conflicts with Mira's proposed change for copying the 
conntrack information[0].

Patch #1 and Patch #5 are the only ones *not* conflicting with Fabian's 
remote migration RFC[1].

[0]: https://lists.proxmox.com/pipermail/pve-devel/2021-February/046873.html

[1]: https://lists.proxmox.com/pipermail/pve-devel/2021-April/047600.html

Am 29.01.21 um 16:11 schrieb Fabian Ebner:
> This series intends to make the migration code more readable by
> simplyfing/unifying how we keep track of local volumes and splitting up
> sync_disks into multiple subroutines.
> 
> This is done by keeping more information within the hash of local_volumes we
> obtain in the very beginning and re-using it later. Also a method to filter by
> migration/replication type is introduced, making it possible to get rid of some
> special-case handling when iterating over local volumes.
> 
> Still more should be possible, but the series is getting big, so I stopped here.
> Namely, $self->{local_volumes}, $self->{volume_map} and $self->{target_drive}
> still have some overlap and it might be possible to merge them somehow.
> 
> Also it might make sense to put abstract more things to the common code, one
> example is to make target storage mapping available for containers too.
> 
> 
> Changes from v1:
>      * dropped already applied patches
>      * rebased
>      * prefix commit messages with 'migration' (and 'test')
>      * increased test coverage a little, see patch #1 and patch #13
>      * reordered patches #8+#9 to avoid temporary breakage, see
>        patches #8 and patch #13 (added test-case) for the details
>      * add patch to fix bandwidth limit calculation
>      * add patch to split out run_replication from scan_local_volumes
>      * add patch to improve error handling when block job finishing fails
> 
> 
> Fabian Ebner (13):
>    test: migration: add parse_volume_id calls
>    migration: split sync_disks into two functions
>    migration: avoid re-scanning all volumes
>    migration: split out config_update_local_disksizes from
>      scan_local_volumes
>    migration: fix calculation of bandwith limit for non-disk migration
>    migration: save targetstorage and bwlimit in local_volumes hash and
>      re-use information
>    migration: add nbd migrated volumes to volume_map earlier
>    migration: simplify removal of local volumes and get rid of
>      self->{volumes}
>    migration: cleanup_remotedisks: simplify and include more disks
>    migration: use storage_migration for checks instead of
>      online_local_volumes
>    migration: keep track of replicated volumes via local_volumes
>    migration: split out replication from scan_local_volumes
>    migration: move finishing block jobs to phase2 for better/uniform
>      error handling
> 
>   PVE/QemuMigrate.pm                    | 401 ++++++++++++++------------
>   PVE/QemuServer.pm                     |   2 +
>   test/MigrationTest/QemuMigrateMock.pm |   9 +
>   test/MigrationTest/QmMock.pm          |   2 +
>   test/MigrationTest/Shared.pm          |   4 +
>   test/run_qemu_migrate_tests.pl        |  40 ++-
>   6 files changed, 267 insertions(+), 191 deletions(-)
> 

[pve-devel] applied-series: [PATCH-SERIES v2 qemu-server] Cleanup migration code and improve migration disk cleanup

[Thomas Lamprecht]

On 29.01.21 16:11, Fabian Ebner wrote:
> This series intends to make the migration code more readable by
> simplyfing/unifying how we keep track of local volumes and splitting up
> sync_disks into multiple subroutines.
> 
> This is done by keeping more information within the hash of local_volumes we
> obtain in the very beginning and re-using it later. Also a method to filter by
> migration/replication type is introduced, making it possible to get rid of some
> special-case handling when iterating over local volumes.
> 
> Still more should be possible, but the series is getting big, so I stopped here.
> Namely, $self->{local_volumes}, $self->{volume_map} and $self->{target_drive}
> still have some overlap and it might be possible to merge them somehow.
> 
> Also it might make sense to put abstract more things to the common code, one
> example is to make target storage mapping available for containers too.
> 
> 
> Changes from v1:
>     * dropped already applied patches
>     * rebased
>     * prefix commit messages with 'migration' (and 'test')
>     * increased test coverage a little, see patch #1 and patch #13
>     * reordered patches #8+#9 to avoid temporary breakage, see
>       patches #8 and patch #13 (added test-case) for the details
>     * add patch to fix bandwidth limit calculation
>     * add patch to split out run_replication from scan_local_volumes
>     * add patch to improve error handling when block job finishing fails
> 
> 
> Fabian Ebner (13):
>   test: migration: add parse_volume_id calls
>   migration: split sync_disks into two functions
>   migration: avoid re-scanning all volumes
>   migration: split out config_update_local_disksizes from
>     scan_local_volumes
>   migration: fix calculation of bandwith limit for non-disk migration
>   migration: save targetstorage and bwlimit in local_volumes hash and
>     re-use information
>   migration: add nbd migrated volumes to volume_map earlier
>   migration: simplify removal of local volumes and get rid of
>     self->{volumes}
>   migration: cleanup_remotedisks: simplify and include more disks
>   migration: use storage_migration for checks instead of
>     online_local_volumes
>   migration: keep track of replicated volumes via local_volumes
>   migration: split out replication from scan_local_volumes
>   migration: move finishing block jobs to phase2 for better/uniform
>     error handling
> 
>  PVE/QemuMigrate.pm                    | 401 ++++++++++++++------------
>  PVE/QemuServer.pm                     |   2 +
>  test/MigrationTest/QemuMigrateMock.pm |   9 +
>  test/MigrationTest/QmMock.pm          |   2 +
>  test/MigrationTest/Shared.pm          |   4 +
>  test/run_qemu_migrate_tests.pl        |  40 ++-
>  6 files changed, 267 insertions(+), 191 deletions(-)
> 



applied series, thanks!