[pve-devel] [PATCH manager 1/3] Hold the guest migration lock when changing the replication config

[pve-devel] [PATCH manager 1/3] Hold the guest migration lock when changing the replication config

[Fabian Ebner]

The guest migration lock is already held when running replications,
but it also makes sense to hold it when updating the replication
config itself. Otherwise, it can happen that the migration does
not know the de-facto state of replication.

For example:
1. migration starts
2. replication job is deleted
3. migration reads the replication config
4. migration runs the replication which causes the
   replicated disks to be removed, because the job
   is marked for removal
5. migration will continue without replication

Note that the migration doesn't actually fail, but it's probably
not the desired behavior either.

Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/API2/ReplicationConfig.pm | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/PVE/API2/ReplicationConfig.pm b/PVE/API2/ReplicationConfig.pm
index 2b4ecd10..e5262068 100644
--- a/PVE/API2/ReplicationConfig.pm
+++ b/PVE/API2/ReplicationConfig.pm
@@ -9,6 +9,7 @@ use PVE::JSONSchema qw(get_standard_option);
 use PVE::RPCEnvironment;
 use PVE::ReplicationConfig;
 use PVE::Cluster;
+use PVE::GuestHelpers;
 
 use PVE::RESTHandler;
 
@@ -144,7 +145,9 @@ __PACKAGE__->register_method ({
 	    $cfg->write();
 	};
 
-	PVE::ReplicationConfig::lock($code);
+	PVE::GuestHelpers::guest_migration_lock($guest, 10, sub {
+	    PVE::ReplicationConfig::lock($code);
+	});
 
 	return undef;
     }});
@@ -167,6 +170,7 @@ __PACKAGE__->register_method ({
 	my $id = extract_param($param, 'id');
 	my $digest = extract_param($param, 'digest');
 	my $delete = extract_param($param, 'delete');
+	my ($guest_id) = PVE::ReplicationConfig::parse_replication_job_id($id);
 
 	my $code = sub {
 	    my $cfg = PVE::ReplicationConfig->new();
@@ -199,7 +203,9 @@ __PACKAGE__->register_method ({
 	    $cfg->write();
 	};
 
-	PVE::ReplicationConfig::lock($code);
+	PVE::GuestHelpers::guest_migration_lock($guest_id, 10, sub {
+	    PVE::ReplicationConfig::lock($code);
+	});
 
 	return undef;
     }});
@@ -237,10 +243,12 @@ __PACKAGE__->register_method ({
 
 	my $rpcenv = PVE::RPCEnvironment::get();
 
+	my $id = extract_param($param, 'id');
+	my ($guest_id) = PVE::ReplicationConfig::parse_replication_job_id($id);
+
 	my $code = sub {
 	    my $cfg = PVE::ReplicationConfig->new();
 
-	    my $id = $param->{id};
 	    if ($param->{force}) {
 		raise_param_exc({ 'keep' => "conflicts with parameter 'force'" }) if $param->{keep};
 		delete $cfg->{ids}->{$id};
@@ -262,7 +270,9 @@ __PACKAGE__->register_method ({
 	    $cfg->write();
 	};
 
-	PVE::ReplicationConfig::lock($code);
+	PVE::GuestHelpers::guest_migration_lock($guest_id, 10, sub {
+	    PVE::ReplicationConfig::lock($code);
+	});
 
 	return undef;
     }});
-- 
2.20.1

[pve-devel] [PATCH qemu-server 2/3] Repeat check for replication target in locked section

[Fabian Ebner]

No need to warn twice, so the warning from the outside check
was removed.

Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Qemu.pm   | 11 +++--------
 PVE/QemuMigrate.pm | 13 +++++++++++++
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 8da616a..bc67666 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -3539,14 +3539,9 @@ __PACKAGE__->register_method({
 	    my $repl_conf = PVE::ReplicationConfig->new();
 	    my $is_replicated = $repl_conf->check_for_existing_jobs($vmid, 1);
 	    my $is_replicated_to_target = defined($repl_conf->find_local_replication_job($vmid, $target));
-	    if ($is_replicated && !$is_replicated_to_target) {
-		if ($param->{force}) {
-		    warn "WARNING: Node '$target' is not a replication target. Existing replication " .
-		         "jobs will fail after migration!\n";
-		} else {
-		    die "Cannot live-migrate replicated VM to node '$target' - not a replication target." .
-		        " Use 'force' to override.\n";
-		}
+	    if (!$param->{force} && $is_replicated && !$is_replicated_to_target) {
+		die "Cannot live-migrate replicated VM to node '$target' - not a replication " .
+		    "target. Use 'force' to override.\n";
 	    }
 	} else {
 	    warn "VM isn't running. Doing offline migration instead.\n" if $param->{online};
diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index b699b67..a20e1c7 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -227,6 +227,19 @@ sub prepare {
 	die "can't migrate running VM without --online\n" if !$online;
 	$running = $pid;
 
+	my $repl_conf = PVE::ReplicationConfig->new();
+	my $is_replicated = $repl_conf->check_for_existing_jobs($vmid, 1);
+	my $is_replicated_to_target = defined($repl_conf->find_local_replication_job($vmid, $self->{node}));
+	if ($is_replicated && !$is_replicated_to_target) {
+	    if ($self->{opts}->{force}) {
+		$self->log('warn', "WARNING: Node '$self->{node}' is not a replication target. Existing " .
+			           "replication jobs will fail after migration!\n");
+	    } else {
+		die "Cannot live-migrate replicated VM to node '$self->{node}' - not a replication " .
+		    "target. Use 'force' to override.\n";
+	    }
+	}
+
 	$self->{forcemachine} = PVE::QemuServer::Machine::qemu_machine_pxe($vmid, $conf);
 
 	# To support custom CPU types, we keep QEMU's "-cpu" parameter intact.
-- 
2.20.1

[pve-devel] [PATCH/RFC qemu-server 3/3] Fix checks for transfering replication state/switching job target

[Fabian Ebner]

When there are offline disks, $self->{replicated_volumes} will be
auto-vivified to {} by the check:
next if $self->{replicated_volumes}->{$volid}
in sync_disks() and then {} would evaluate to true in a boolean context.

Now the replication job information is retrieved once in prepare,
and the job information rather than the information if volumes
were replicated is used to decide whether to make the calls or not.

For offline migration to a non-replication target, there are no
$self->{replicated_volumes}, but the state should be transfered nonetheless.

For online migration to a non-replication target, replication
is broken afterwards anyways, so it doesn't make much of a difference
if the state is transferred or not.

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

Hope I'm not misinterpreting when these calls should or shouldn't
be made.

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

diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index a20e1c7..6097ef2 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -220,6 +220,10 @@ sub prepare {
     # test if VM exists
     my $conf = $self->{vmconf} = PVE::QemuConfig->load_config($vmid);
 
+    my $repl_conf = PVE::ReplicationConfig->new();
+    $self->{replication_jobcfg} = $repl_conf->find_local_replication_job($vmid, $self->{node});
+    $self->{is_replicated} = $repl_conf->check_for_existing_jobs($vmid, 1);
+
     PVE::QemuConfig->check_lock($conf);
 
     my $running = 0;
@@ -227,10 +231,7 @@ sub prepare {
 	die "can't migrate running VM without --online\n" if !$online;
 	$running = $pid;
 
-	my $repl_conf = PVE::ReplicationConfig->new();
-	my $is_replicated = $repl_conf->check_for_existing_jobs($vmid, 1);
-	my $is_replicated_to_target = defined($repl_conf->find_local_replication_job($vmid, $self->{node}));
-	if ($is_replicated && !$is_replicated_to_target) {
+	if ($self->{is_replicated} && !$self->{replication_jobcfg}) {
 	    if ($self->{opts}->{force}) {
 		$self->log('warn', "WARNING: Node '$self->{node}' is not a replication target. Existing " .
 			           "replication jobs will fail after migration!\n");
@@ -362,9 +363,7 @@ sub sync_disks {
 	    });
 	}
 
-	my $rep_cfg = PVE::ReplicationConfig->new();
-	my $replication_jobcfg = $rep_cfg->find_local_replication_job($vmid, $self->{node});
-	my $replicatable_volumes = !$replication_jobcfg ? {}
+	my $replicatable_volumes = !$self->{replication_jobcfg} ? {}
 	    : PVE::QemuConfig->get_replicatable_volumes($storecfg, $vmid, $conf, 0, 1);
 
 	my $test_volid = sub {
@@ -489,7 +488,7 @@ sub sync_disks {
 	    }
 	}
 
-	if ($replication_jobcfg) {
+	if ($self->{replication_jobcfg}) {
 	    if ($self->{running}) {
 
 		my $version = PVE::QemuServer::kvm_user_version();
@@ -523,7 +522,7 @@ sub sync_disks {
 	    my $start_time = time();
 	    my $logfunc = sub { $self->log('info', shift) };
 	    $self->{replicated_volumes} = PVE::Replication::run_replication(
-	       'PVE::QemuConfig', $replication_jobcfg, $start_time, $start_time, $logfunc);
+	       'PVE::QemuConfig', $self->{replication_jobcfg}, $start_time, $start_time, $logfunc);
 	}
 
 	# sizes in config have to be accurate for remote node to correctly
@@ -1193,7 +1192,7 @@ sub phase3_cleanup {
     }
 
     # transfer replication state before move config
-    $self->transfer_replication_state() if $self->{replicated_volumes};
+    $self->transfer_replication_state() if $self->{is_replicated};
 
     # move config to remote node
     my $conffile = PVE::QemuConfig->config_file($vmid);
@@ -1202,7 +1201,7 @@ sub phase3_cleanup {
     die "Failed to move config to node '$self->{node}' - rename failed: $!\n"
         if !rename($conffile, $newconffile);
 
-    $self->switch_replication_job_target() if $self->{replicated_volumes};
+    $self->switch_replication_job_target() if $self->{replication_jobcfg};
 
     if ($self->{livemigration}) {
 	if ($self->{stopnbd}) {
-- 
2.20.1

Re: [pve-devel] [PATCH manager 1/3] Hold the guest migration lock when changing the replication config

[Fabian Ebner]

Am 30.07.20 um 13:29 schrieb Fabian Ebner:
> The guest migration lock is already held when running replications,
> but it also makes sense to hold it when updating the replication
> config itself. Otherwise, it can happen that the migration does
> not know the de-facto state of replication.
> 
> For example:
> 1. migration starts
> 2. replication job is deleted
> 3. migration reads the replication config
> 4. migration runs the replication which causes the
>     replicated disks to be removed, because the job
>     is marked for removal
> 5. migration will continue without replication
> 

This situation can still happen even with the locking from this patch:
1. replication job is deleted
2. migration starts before the replication was run, so the job is still 
marked for removal in the replication config
3.-5. same as above

So we probably want to check during migration whether the replication 
job that we want to use is marked for removal. If it is, we could either:
- leave the situation as is, i.e. the replication job will be removed 
during migration and migration will continue without replication
- fail the migration (principle of least surprise?)
- run replication without the removal mark during migration. Then the 
replication job would be removed the next time replication runs after 
migration and hence after the target was switched.

Also: If we only read the replication config once during a migration, 
the locking from this patch shouldn't even be necessary. 
switch_replication_job_target does read the config once more, but that 
would still be compatible with allowing other changes to the replication 
config during migration. But of course this locking might make things 
more future proof.

> Note that the migration doesn't actually fail, but it's probably
> not the desired behavior either.
> 
> Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>   PVE/API2/ReplicationConfig.pm | 18 ++++++++++++++----
>   1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/PVE/API2/ReplicationConfig.pm b/PVE/API2/ReplicationConfig.pm
> index 2b4ecd10..e5262068 100644
> --- a/PVE/API2/ReplicationConfig.pm
> +++ b/PVE/API2/ReplicationConfig.pm
> @@ -9,6 +9,7 @@ use PVE::JSONSchema qw(get_standard_option);
>   use PVE::RPCEnvironment;
>   use PVE::ReplicationConfig;
>   use PVE::Cluster;
> +use PVE::GuestHelpers;
>   
>   use PVE::RESTHandler;
>   
> @@ -144,7 +145,9 @@ __PACKAGE__->register_method ({
>   	    $cfg->write();
>   	};
>   
> -	PVE::ReplicationConfig::lock($code);
> +	PVE::GuestHelpers::guest_migration_lock($guest, 10, sub {
> +	    PVE::ReplicationConfig::lock($code);
> +	});
>   
>   	return undef;
>       }});
> @@ -167,6 +170,7 @@ __PACKAGE__->register_method ({
>   	my $id = extract_param($param, 'id');
>   	my $digest = extract_param($param, 'digest');
>   	my $delete = extract_param($param, 'delete');
> +	my ($guest_id) = PVE::ReplicationConfig::parse_replication_job_id($id);
>   
>   	my $code = sub {
>   	    my $cfg = PVE::ReplicationConfig->new();
> @@ -199,7 +203,9 @@ __PACKAGE__->register_method ({
>   	    $cfg->write();
>   	};
>   
> -	PVE::ReplicationConfig::lock($code);
> +	PVE::GuestHelpers::guest_migration_lock($guest_id, 10, sub {
> +	    PVE::ReplicationConfig::lock($code);
> +	});
>   
>   	return undef;
>       }});
> @@ -237,10 +243,12 @@ __PACKAGE__->register_method ({
>   
>   	my $rpcenv = PVE::RPCEnvironment::get();
>   
> +	my $id = extract_param($param, 'id');
> +	my ($guest_id) = PVE::ReplicationConfig::parse_replication_job_id($id);
> +
>   	my $code = sub {
>   	    my $cfg = PVE::ReplicationConfig->new();
>   
> -	    my $id = $param->{id};
>   	    if ($param->{force}) {
>   		raise_param_exc({ 'keep' => "conflicts with parameter 'force'" }) if $param->{keep};
>   		delete $cfg->{ids}->{$id};
> @@ -262,7 +270,9 @@ __PACKAGE__->register_method ({
>   	    $cfg->write();
>   	};
>   
> -	PVE::ReplicationConfig::lock($code);
> +	PVE::GuestHelpers::guest_migration_lock($guest_id, 10, sub {
> +	    PVE::ReplicationConfig::lock($code);
> +	});
>   
>   	return undef;
>       }});
> 

Re: [pve-devel] [PATCH manager 1/3] Hold the guest migration lock when changing the replication config

[Fabian Grünbichler]

On July 30, 2020 1:29 pm, Fabian Ebner wrote:
> The guest migration lock is already held when running replications,
> but it also makes sense to hold it when updating the replication
> config itself. Otherwise, it can happen that the migration does
> not know the de-facto state of replication.
> 
> For example:
> 1. migration starts
> 2. replication job is deleted
> 3. migration reads the replication config
> 4. migration runs the replication which causes the
>    replicated disks to be removed, because the job
>    is marked for removal
> 5. migration will continue without replication
> 
> Note that the migration doesn't actually fail, but it's probably
> not the desired behavior either.
> 
> Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/API2/ReplicationConfig.pm | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/PVE/API2/ReplicationConfig.pm b/PVE/API2/ReplicationConfig.pm
> index 2b4ecd10..e5262068 100644
> --- a/PVE/API2/ReplicationConfig.pm
> +++ b/PVE/API2/ReplicationConfig.pm
> @@ -9,6 +9,7 @@ use PVE::JSONSchema qw(get_standard_option);
>  use PVE::RPCEnvironment;
>  use PVE::ReplicationConfig;
>  use PVE::Cluster;
> +use PVE::GuestHelpers;
>  
>  use PVE::RESTHandler;
>  
> @@ -144,7 +145,9 @@ __PACKAGE__->register_method ({
>  	    $cfg->write();
>  	};
>  
> -	PVE::ReplicationConfig::lock($code);
> +	PVE::GuestHelpers::guest_migration_lock($guest, 10, sub {
> +	    PVE::ReplicationConfig::lock($code);
> +	});

it might make sense to have a single wrapper for this, or add the guest 
ID as parameter to ReplicationConfig::lock (to not miss it or get the 
order wrong).

what about the calls to lock within ReplicationConfig? they are all 
job/guest ID specific, and should also get this additional protection, 
right?

from a quick glance, there seems to be only a single call to 
ReplicationConfig::lock that spans more than one job (job_status in 
ReplicationState), but that immediately iterates over jobs, so we could 
either move the lock into the loop (expensive, since it involves a 
cfs_lock), or split the cfs and flock just for this instance?

(side note, that code and possibly other stuff in ReplicationConfig is 
buggy since it does not re-read the config after locking)

>  
>  	return undef;
>      }});
> @@ -167,6 +170,7 @@ __PACKAGE__->register_method ({
>  	my $id = extract_param($param, 'id');
>  	my $digest = extract_param($param, 'digest');
>  	my $delete = extract_param($param, 'delete');
> +	my ($guest_id) = PVE::ReplicationConfig::parse_replication_job_id($id);
>  
>  	my $code = sub {
>  	    my $cfg = PVE::ReplicationConfig->new();
> @@ -199,7 +203,9 @@ __PACKAGE__->register_method ({
>  	    $cfg->write();
>  	};
>  
> -	PVE::ReplicationConfig::lock($code);
> +	PVE::GuestHelpers::guest_migration_lock($guest_id, 10, sub {
> +	    PVE::ReplicationConfig::lock($code);
> +	});
>  
>  	return undef;
>      }});
> @@ -237,10 +243,12 @@ __PACKAGE__->register_method ({
>  
>  	my $rpcenv = PVE::RPCEnvironment::get();
>  
> +	my $id = extract_param($param, 'id');
> +	my ($guest_id) = PVE::ReplicationConfig::parse_replication_job_id($id);
> +
>  	my $code = sub {
>  	    my $cfg = PVE::ReplicationConfig->new();
>  
> -	    my $id = $param->{id};
>  	    if ($param->{force}) {
>  		raise_param_exc({ 'keep' => "conflicts with parameter 'force'" }) if $param->{keep};
>  		delete $cfg->{ids}->{$id};
> @@ -262,7 +270,9 @@ __PACKAGE__->register_method ({
>  	    $cfg->write();
>  	};
>  
> -	PVE::ReplicationConfig::lock($code);
> +	PVE::GuestHelpers::guest_migration_lock($guest_id, 10, sub {
> +	    PVE::ReplicationConfig::lock($code);
> +	});
>  
>  	return undef;
>      }});
> -- 
> 2.20.1
> 
>