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.eb...@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});
>
>
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
