[pve-devel] [PATCH qemu-server] close #2792: allow online migration with replicated snapshots

[pve-devel] [PATCH qemu-server] close #2792: allow online migration with replicated snapshots

[Fiona Ebner]

Since commit 9b6efe43 ("migrate: add live-migration of replicated
disks") live-migration with replicated volumes is possible. When
handling the replication, it is checked that all local volumes
previously detected as replicatable are actually replicated. So the
check if migration with snapshots is possible can just allow volumes
that are detected as replicatable.

Note that VM state files are also replicated.

If there is an invalid configuration with a non-replicatable volume or
state file and replication is enabled, then replication will fail, and
thus migration will fail early.

Trying to live-migrate to a non-replication target (needs --force)
will still fail if there are snapshots, because they are (correctly)
detected as non-replicated.

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

This seems too easy. What did I miss ;)?

 PVE/QemuMigrate.pm             | 4 +++-
 test/run_qemu_migrate_tests.pl | 9 ++++++---
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index 5e466d95..0c12f77d 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -433,7 +433,9 @@ sub scan_local_volumes {
 		# we cannot migrate shapshots on local storage
 		# exceptions: 'zfspool' or 'qcow2' files (on directory storage)
 
-		die "online storage migration not possible if snapshot exists\n" if $self->{running};
+		die "online storage migration not possible if non-replicated snapshot exists\n"
+		    if $self->{running} && !$local_volumes->{$volid}->{replicated};
+
 		die "remote migration with snapshots not supported yet\n" if $self->{opts}->{remote};
 
 		if (!($scfg->{type} eq 'zfspool'
diff --git a/test/run_qemu_migrate_tests.pl b/test/run_qemu_migrate_tests.pl
index 0dffaa43..3a3049d7 100755
--- a/test/run_qemu_migrate_tests.pl
+++ b/test/run_qemu_migrate_tests.pl
@@ -678,7 +678,7 @@ my $tests = [
 	    'with-local-disks' => 1,
 	},
 	expected_calls => {},
-	expect_die => 'online storage migration not possible if snapshot exists',
+	expect_die => 'online storage migration not possible if non-replicated snapshot exists',
 	expected => {
 	    source_volids => local_volids_for_vm(4567),
 	    target_volids => {},
@@ -1237,8 +1237,11 @@ my $tests = [
 	    'with-local-disks' => 1,
 	},
 	target_volids => local_volids_for_vm(105),
-	expected_calls => {},
-	expect_die => "online storage migration not possible if snapshot exists",
+	expected_calls => {
+	    %{$replicated_expected_calls_online},
+	    'block-dirty-bitmap-add-drive-scsi0' => 1,
+	    'block-dirty-bitmap-add-drive-ide0' => 1,
+	},
 	expected => {
 	    source_volids => local_volids_for_vm(105),
 	    target_volids => local_volids_for_vm(105),
-- 
2.30.2

[pve-devel] applied: [PATCH qemu-server] close #2792: allow online migration with replicated snapshots

[Fabian Grünbichler]

On January 18, 2023 2:52 pm, Fiona Ebner wrote:
> Since commit 9b6efe43 ("migrate: add live-migration of replicated
> disks") live-migration with replicated volumes is possible. When
> handling the replication, it is checked that all local volumes
> previously detected as replicatable are actually replicated. So the
> check if migration with snapshots is possible can just allow volumes
> that are detected as replicatable.
> 
> Note that VM state files are also replicated.
> 
> If there is an invalid configuration with a non-replicatable volume or
> state file and replication is enabled, then replication will fail, and
> thus migration will fail early.
> 
> Trying to live-migrate to a non-replication target (needs --force)
> will still fail if there are snapshots, because they are (correctly)
> detected as non-replicated.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> 
> This seems too easy. What did I miss ;)?

it looks good to me - and I think all the work we did on restructuring the
migration code and especially the volume handling paid off :)

there is a thread waiting for this, so maybe we can ping them once it hits
pvetest:

https://forum.proxmox.com/threads/error-online-storage-migration-not-possible-if-snapshot-exists-on-zfspool-bug-in-qemumigrate-pm.107025

>  PVE/QemuMigrate.pm             | 4 +++-
>  test/run_qemu_migrate_tests.pl | 9 ++++++---
>  2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
> index 5e466d95..0c12f77d 100644
> --- a/PVE/QemuMigrate.pm
> +++ b/PVE/QemuMigrate.pm
> @@ -433,7 +433,9 @@ sub scan_local_volumes {
>  		# we cannot migrate shapshots on local storage
>  		# exceptions: 'zfspool' or 'qcow2' files (on directory storage)
>  
> -		die "online storage migration not possible if snapshot exists\n" if $self->{running};
> +		die "online storage migration not possible if non-replicated snapshot exists\n"
> +		    if $self->{running} && !$local_volumes->{$volid}->{replicated};
> +
>  		die "remote migration with snapshots not supported yet\n" if $self->{opts}->{remote};
>  
>  		if (!($scfg->{type} eq 'zfspool'
> diff --git a/test/run_qemu_migrate_tests.pl b/test/run_qemu_migrate_tests.pl
> index 0dffaa43..3a3049d7 100755
> --- a/test/run_qemu_migrate_tests.pl
> +++ b/test/run_qemu_migrate_tests.pl
> @@ -678,7 +678,7 @@ my $tests = [
>  	    'with-local-disks' => 1,
>  	},
>  	expected_calls => {},
> -	expect_die => 'online storage migration not possible if snapshot exists',
> +	expect_die => 'online storage migration not possible if non-replicated snapshot exists',
>  	expected => {
>  	    source_volids => local_volids_for_vm(4567),
>  	    target_volids => {},
> @@ -1237,8 +1237,11 @@ my $tests = [
>  	    'with-local-disks' => 1,
>  	},
>  	target_volids => local_volids_for_vm(105),
> -	expected_calls => {},
> -	expect_die => "online storage migration not possible if snapshot exists",
> +	expected_calls => {
> +	    %{$replicated_expected_calls_online},
> +	    'block-dirty-bitmap-add-drive-scsi0' => 1,
> +	    'block-dirty-bitmap-add-drive-ide0' => 1,
> +	},
>  	expected => {
>  	    source_volids => local_volids_for_vm(105),
>  	    target_volids => local_volids_for_vm(105),
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>