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

[Fabian Ebner <f.ebner@proxmox.com>]

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;
>       }});
>