[pve-devel] [PATCH pve-zsync] Allow detecting a syncing instance of a job

[Thomas Lamprecht]

On 9/30/19 12:55 PM, Fabian Ebner wrote:
> Before, the check whether a syncing instance of the same job is already present
> was inside the locked section. This caused cron to continuously spawn new
> instances of pve-zsync on syncs (or rather groups of syncs) taking longer
> than 15 minutes, see [0] in the forum. This patch introduces a new locked
> section for checking the current status and a new 'waiting' status.
> The 'waiting' status is needed to mark jobs which are currently waiting
> for the lock for syncing. So if job A is syncing and job B is waiting for
> the lock then all new instances of job B will see that one instance is
> already scheduled to sync.
> 
> [0]: https://forum.proxmox.com/threads/pve-zsync-bug-spawns-endless-cron-processes.58087/
> 
> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
> ---
>  pve-zsync | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 

Looks OK, a small style nit and proposal for eventual refactoring in-line

> diff --git a/pve-zsync b/pve-zsync
> index 425ffa2..90c1bb3 100755
> --- a/pve-zsync
> +++ b/pve-zsync
> @@ -19,6 +19,7 @@ my $PVE_DIR = "/etc/pve/local";
>  my $QEMU_CONF = "${PVE_DIR}/qemu-server";
>  my $LXC_CONF = "${PVE_DIR}/lxc";
>  my $LOCKFILE = "$CONFIG_PATH/${PROGNAME}.lock";
> +my $LOCKFILE_STATUS_CHECK = "$CONFIG_PATH/${PROGNAME}_status_check.lock";
>  my $PROG_PATH = "$PATH/${PROGNAME}";
>  my $INTERVAL = 15;
>  my $DEBUG;
> @@ -578,20 +579,34 @@ sub destroy_job {
>  sub sync {
>      my ($param) = @_;
>  
> +    my $lock_status_check_fh = IO::File->new("> $LOCKFILE_STATUS_CHECK");
> +    die "Can't open Lock File: $LOCKFILE_STATUS_CHECK $!\n" if !$lock_status_check_fh;
> +    lock($lock_status_check_fh);

Above pattern is repeated quite a few times in this file, maybe we could replace it
by something like:

sub locked {
    my ($lock_fn, $code) = @_;

    my $lock_fh = File...
 
    flock($lock_fh, LOCK_EX) || die ...;
    my $res = eval { $code->() };
    my $err = $@;

    flock($fh, LOCK_UN) || warn ...; # always unlock
    die "$err" if $err;

    close($lock_fh); # or maybe cache this one and keep around? not sure though
    return $res;
}


Then we'd have explicit locked segments where we know for sure that unlocking
is always done. But as said, that's some refactoring that could be great nothing
important.

> +
> +    my $job;
> +    eval {
> +	$job = get_job($param);
> +    };

Above could be also written as one-liner without losing expressiveness:
my $job = eval { get_job($param) };

This uses the fact that eval (and most blocks in general) in perl implicitly
return the result of the last statement, if no explicit return is (in all
branches). I know you just used a pattern already existent in this file,
but such a "cleanup" could be OK to even to then, IMO.

Not sure, depending if you want to refactor a bit here you could send a v2
else I could apply this and just fixup those "eval assignment" places here,
whatever you prefer.

> +
> +    if ($job && defined($job->{state}) && ($job->{state} eq "syncing" || $job->{state} eq "waiting")) {
> +	unlock($lock_status_check_fh);
> +	die "Job --source $param->{source} --name $param->{name} is already scheduled to sync\n";
> +    }
> +
> +    $job->{state} = "waiting";
> +    update_state($job);
> +    unlock($lock_status_check_fh);
> +
>      my $lock_fh = IO::File->new("> $LOCKFILE");
>      die "Can't open Lock File: $LOCKFILE $!\n" if !$lock_fh;
>      lock($lock_fh);
>  
> +    #job might've changed while we waited for the lock, but we can be sure it's not syncing
>      my $date = get_date();
> -    my $job;
>      eval {
>  	$job = get_job($param);
>      };
>  
> -    if ($job && defined($job->{state}) && $job->{state} eq "syncing") {
> -	die "Job --source $param->{source} --name $param->{name} is syncing at the moment";
> -    }
> -
>      my $dest = parse_target($param->{dest});
>      my $source = parse_target($param->{source});
>  
>