Re: [pve-devel] [PATCH v3 guest-common 1/1] guest helpers: add run_with_replication_guard

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

Am 21.02.22 um 12:58 schrieb Fabian Ebner:
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> New in v3.
> 
>  src/PVE/GuestHelpers.pm | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/GuestHelpers.pm b/src/PVE/GuestHelpers.pm
> index 970c460..1183819 100644
> --- a/src/PVE/GuestHelpers.pm
> +++ b/src/PVE/GuestHelpers.pm
> @@ -3,8 +3,9 @@ package PVE::GuestHelpers;
>  use strict;
>  use warnings;
>  
> -use PVE::Tools;
> +use PVE::ReplicationConfig;
>  use PVE::Storage;
> +use PVE::Tools;
>  
>  use POSIX qw(strftime);
>  use Scalar::Util qw(weaken);
> @@ -82,6 +83,18 @@ sub guest_migration_lock {
>      return $res;
>  }
>  
> +sub run_with_replication_guard {
> +    my ($vmid, $timeout, $log, $func, @param) = @_;
> +
> +    my $repl_conf = PVE::ReplicationConfig->new();
> +    if ($repl_conf->check_for_existing_jobs($vmid, 1)) {
> +	$log->("checking/waiting for active replication..") if $log;
> +	guest_migration_lock($vmid, $timeout, $func, @param);

I wonder if we should unconditionally take the lock? If not, we can race
with a newly created replication job:
1. snapshot deletion starts
2. replication job is created
3. replication job starts
4. snapshot deletion runs into 'dataset is busy' error, because snapshot
is used by replication

IIRC Thomas didn't want the log line to be printed when there is no
replication configured, and we can still do that if we go for the
"unconditionally acquire lock" approach (it should get the lock quickly
except in the above edge case), but it would mean checking the
replication config just for that. My suggestion would be to get rid of
this helper, not log anything in the cases with 10s timeout, and log if
replication is configured in the backup case with 600s timeout.

> +    } else {
> +	$func->(@param);
> +    }
> +}
> +
>  sub check_hookscript {
>      my ($volid, $storecfg) = @_;
>