[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 at 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