[pve-devel] [PATCH qemu-server] migrate: fix replication false-positives

[Fabian Grünbichler]

hmm, and I just realized this would also need another fixup to unbreak 
new -> old (we'd need to report back that we understood 
'replicated_volume' input from the target to the source, and abort 
otherwise with proper cleanup). otherwise we blindly receive our delta 
on-top of a newly allocated disk, while leaving the actual base as 
unused, unreferenced disk. I'll send a v2 ;)

On March 26, 2020 10:10 am, Fabian Grünbichler wrote:
> by only checking for replicatable volumes when a replication job is
> defined, and passing only actually replicated volumes to the target node
> via STDIN.
> 
> otherwise this can pick up theoretically replicatable, but not actually
> replicated volumes and treat them wrong.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
> ---
> 
> Notes:
>     this effectively makes qemu-server 6.1-10 and 6.1-11 very broken qemu-server versions
> 
>  PVE/API2/Qemu.pm   |  5 ++++-
>  PVE/QemuMigrate.pm | 12 ++++++++----
>  PVE/QemuServer.pm  |  6 ++----
>  3 files changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index ef8a7c3..8a7e98c 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -2073,6 +2073,7 @@ __PACKAGE__->register_method({
>  	# read spice ticket from STDIN
>  	my $spice_ticket;
>  	my $nbd_protocol_version = 0;
> +	my $replicated_volumes = {};
>  	if ($stateuri && ($stateuri eq 'tcp' || $stateuri eq 'unix') && $migratedfrom && ($rpcenv->{type} eq 'cli')) {
>  	    while (defined(my $line = <STDIN>)) {
>  		chomp $line;
> @@ -2080,6 +2081,8 @@ __PACKAGE__->register_method({
>  		    $spice_ticket = $1;
>  		} elsif ($line =~ m/^nbd_protocol_version: (\d+)$/) {
>  		    $nbd_protocol_version = $1;
> +		} elsif ($line =~ m/^replicated_volume: (.*)$/) {
> +		    $replicated_volumes->{$1} = 1;
>  		} else {
>  		    # fallback for old source node
>  		    $spice_ticket = $line;
> @@ -2113,7 +2116,7 @@ __PACKAGE__->register_method({
>  
>  		PVE::QemuServer::vm_start($storecfg, $vmid, $stateuri, $skiplock, $migratedfrom, undef, $machine,
>  					  $spice_ticket, $migration_network, $migration_type, $targetstorage, $timeout,
> -					  $nbd_protocol_version);
> +					  $nbd_protocol_version, $replicated_volumes);
>  		return;
>  	    };
>  
> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
> index 9cff64d..ef5f6fd 100644
> --- a/PVE/QemuMigrate.pm
> +++ b/PVE/QemuMigrate.pm
> @@ -330,7 +330,9 @@ sub sync_disks {
>  	    });
>  	}
>  
> -	my $replicatable_volumes = PVE::QemuConfig->get_replicatable_volumes($self->{storecfg}, $self->{vmid}, $conf, 0, 1);
> +	my $rep_cfg = PVE::ReplicationConfig->new();
> +	my $replication_jobcfg = $rep_cfg->find_local_replication_job($vmid, $self->{node});
> +	my $replicatable_volumes = $replication_jobcfg ? PVE::QemuConfig->get_replicatable_volumes($self->{storecfg}, $self->{vmid}, $conf, 0, 1) : {};
>  
>  	my $test_volid = sub {
>  	    my ($volid, $attr) = @_;
> @@ -449,8 +451,7 @@ sub sync_disks {
>  	    }
>  	}
>  
> -	my $rep_cfg = PVE::ReplicationConfig->new();
> -	if (my $jobcfg = $rep_cfg->find_local_replication_job($vmid, $self->{node})) {
> +	if ($replication_jobcfg) {
>  	    if ($self->{running}) {
>  
>  		my $version = PVE::QemuServer::kvm_user_version();
> @@ -484,7 +485,7 @@ sub sync_disks {
>  	    my $start_time = time();
>  	    my $logfunc = sub { $self->log('info', shift) };
>  	    $self->{replicated_volumes} = PVE::Replication::run_replication(
> -	       'PVE::QemuConfig', $jobcfg, $start_time, $start_time, $logfunc);
> +	       'PVE::QemuConfig', $replication_jobcfg, $start_time, $start_time, $logfunc);
>  	}
>  
>  	# sizes in config have to be accurate for remote node to correctly
> @@ -657,6 +658,9 @@ sub phase2 {
>      # TODO change to 'spice_ticket: <ticket>\n' in 7.0
>      my $input = $spice_ticket ? "$spice_ticket\n" : "\n";
>      $input .= "nbd_protocol_version: $nbd_protocol_version\n";
> +    foreach my $volid (keys %{$self->{replicated_volumes}}) {
> +	$input .= "replicated_volume: $volid\n";
> +    }
>  
>      # Note: We try to keep $spice_ticket secret (do not pass via command line parameter)
>      # instead we pipe it through STDIN
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index a3e3269..f077915 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -4706,7 +4706,7 @@ sub vmconfig_update_disk {
>  sub vm_start {
>      my ($storecfg, $vmid, $statefile, $skiplock, $migratedfrom, $paused,
>  	$forcemachine, $spice_ticket, $migration_network, $migration_type,
> -	$targetstorage, $timeout, $nbd_protocol_version) = @_;
> +	$targetstorage, $timeout, $nbd_protocol_version, $replicated_volumes) = @_;
>  
>      PVE::QemuConfig->lock_config($vmid, sub {
>  	my $conf = PVE::QemuConfig->load_config($vmid, $migratedfrom);
> @@ -4755,14 +4755,12 @@ sub vm_start {
>  		$local_volumes->{$ds} = [$volid, $storeid, $volname];
>  	    });
>  
> -	    my $replicatable_volumes = PVE::QemuConfig->get_replicatable_volumes($storecfg, $vmid, $conf, 0, 1);
> -
>  	    my $format = undef;
>  
>  	    foreach my $opt (sort keys %$local_volumes) {
>  
>  		my ($volid, $storeid, $volname) = @{$local_volumes->{$opt}};
> -		if ($replicatable_volumes->{$volid}) {
> +		if ($replicated_volumes->{$volid}) {
>  		    # re-use existing, replicated volume with bitmap on source side
>  		    $local_volumes->{$opt} = $conf->{${opt}};
>  		    next;
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>