Re: [PATCH manager 12/19] vzdump: allow jobs to run alongside each other

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

Am 02.11.26 um 12:01 AM schrieb Thomas Lamprecht:
> diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm
> index 73291189f..3e31505f3 100644
> --- a/PVE/API2/VZDump.pm
> +++ b/PVE/API2/VZDump.pm
> @@ -137,7 +137,7 @@ __PACKAGE__->register_method({
>              my $vzdump = PVE::VZDump->new($cmdline, $param, $skiplist);
>  
>              my $LOCK_FH = eval {
> -                my $lock_fh = $vzdump->getlock($upid); # only one process allowed
> +                my $lock_fh = $vzdump->getlock($upid);
>                  $vzdump->init_job();
>                  $lock_fh;
>              };
> diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
> index 33fe8793e..f597f7ed7 100644
> --- a/PVE/VZDump.pm
> +++ b/PVE/VZDump.pm
> @@ -46,6 +46,7 @@ PVE::JSONSchema::register_standard_option(
>  my @posix_filesystems = qw(ext3 ext4 nfs nfs4 reiserfs xfs);
>  
>  my $lockfile = '/var/run/vzdump.lock';
> +my $lockdir = '/run/vzdump';
>  my $logdir = '/var/log/vzdump';
>  
>  my @plugins = qw();
> @@ -682,6 +683,14 @@ sub new {
>  
>      $self->{'default-prune-backups'} = $defaults->{'prune-backups'};
>  
> +    # external providers are not known to cope with other backups running at the same time
> +    my $uses_provider = $opts->{storage} && eval {
> +        PVE::Storage::storage_has_feature(
> +            PVE::Storage::config(), $opts->{storage}, 'backup-provider',
> +        );
> +    };

Should log a message/warning if something goes wrong in the eval to help
debug issues.

> +    $self->{exclusive} = !$opts->{overlap} || $uses_provider;
> +
>      return $self;
>  }
>  
> @@ -781,14 +790,32 @@ sub getlock {
>      my $maxwait = $self->{opts}->{lockwait} || $self->{lockwait};

Pre-existing, but it seems like $self->{lockwait} is never set.

>  
>      die "missing UPID" if !$upid; # should not happen
> +    $self->{upid} = $upid;
> +
> +    mkdir $lockdir;

Might be nicer to add it to the package's tmpfiles config, like /run/pve
already is.

>  
>      my $SERVER_FLCK;
>      if (!open($SERVER_FLCK, '>>', "$lockfile")) {
>          debugmsg('err', "can't open lock on file '$lockfile' - $!", undef, 1);
>          die "can't open lock on file '$lockfile' - $!";
>      }
> +    open(my $gate, '>>', "$lockdir/gate.lock")
> +        or die "can't open lock on file '$lockdir/gate.lock' - $!\n";
>  
> -    if (!flock($SERVER_FLCK, LOCK_EX | LOCK_NB)) {
> +    # Jobs that may overlap share the global lock. A job waiting to get it exclusively holds the
> +    # gate meanwhile, so overlapping jobs starting later cannot keep it waiting forever.
> +    my $mode = $self->{exclusive} ? LOCK_EX : LOCK_SH;
> +    my $acquire = sub {
> +        my ($flags) = @_;
> +
> +        flock($gate, $mode | $flags) or return 0;
> +        my $locked = flock($SERVER_FLCK, $mode | $flags);
> +        flock($gate, LOCK_UN);
> +
> +        return $locked;
> +    };
> +
> +    if (!$acquire->(LOCK_NB)) {
>          if (!$maxwait) {
>              debugmsg('err', "can't acquire lock '$lockfile' (wait = 0)", undef, 1);
>              die "can't acquire lock '$lockfile' (wait = 0)";
> @@ -800,7 +827,7 @@ sub getlock {
>  
>              local $SIG{ALRM} = sub { alarm(0); die "got timeout\n"; };
>  
> -            if (!flock($SERVER_FLCK, LOCK_EX)) {
> +            if (!$acquire->(0)) {
>                  my $err = $!;

This might not be the error code from the failure of flock($SERVER_FLCK,
...), which we are interested in, since there is an flock($gate, ...)
call after it.

>                  close($SERVER_FLCK);
>                  alarm(0);
> @@ -1635,12 +1662,145 @@ my sub finish_guest_backup {
>      }
>  }
>  
> +my sub try_lock_file {
> +    my ($path) = @_;
> +
> +    open(my $fh, '>>', $path) or die "unable to open lock file '$path' - $!\n";
> +    return $fh if flock($fh, LOCK_EX | LOCK_NB);
> +
> +    close($fh);
> +    return undef;
> +}
> +
> +my sub try_guest_lock {
> +    my ($self, $vmid) = @_;
> +
> +    my $fh = try_lock_file("$lockdir/guest-$vmid.lock") or return undef;
> +    truncate($fh, 0);
> +    print $fh "$self->{upid}\n";
> +    $fh->flush();
> +
> +    return $fh;
> +}
> +
> +# Slots above a lowered limit stay in use until their backups end, no new one starts before.
> +my sub try_slot {
> +    my ($prefix, $limit) = @_;
> +
> +    for my $path (glob("$prefix-*.lock")) {
> +        my ($slot) = $path =~ m/^\Q$prefix\E-(\d+)\.lock$/ or next;
> +        next if $slot <= $limit;
> +        my $fh = try_lock_file("$prefix-$slot.lock") or return undef;

We could start re-using lower slots already, even if there is a
remaining slot in-use above the limit. We just need to make sure that
the count of in-use slots stays below the limit.

Otherwise, in a situation with a single long-running task with a slot
above the limit, we have to wait until that single high-slot task is
finished, which seems unfortunate.

> +        close($fh);
> +    }
> +    for my $slot (1 .. $limit) {
> +        my $fh = try_lock_file("$prefix-$slot.lock");
> +        return $fh if $fh;
> +    }
> +    return undef;
> +}
> +
> +my sub try_node_slot {
> +    return try_slot("$lockdir/slot", node_backup_concurrency());
> +}
> +
> +my sub log_wait {
> +    my ($self, $reason) = @_;
> +
> +    return if ($self->{'waiting-for'} // '') eq $reason;
> +    $self->{'waiting-for'} = $reason;
> +    debugmsg('info', "waiting for $reason");
> +}
> +
> +my sub guest_moved_to {
> +    my ($vmid) = @_;
> +
> +    PVE::Cluster::cfs_update();
> +    my $node = PVE::Cluster::get_vmlist()->{ids}->{$vmid}->{node};
> +
> +    return defined($node) && $node ne PVE::INotify::nodename() ? $node : undef;
> +}
> +
> +my sub claim_next_guest {
> +    my ($self, $queue, $deferred) = @_;
> +
> +    my $maxwait = ($self->{opts}->{lockwait} || 0) * 60;

The default according to the schema would be 3 hours. But getlock()
already relies on the value being initialized by new(), so we could do
the same here.

> +
> +    for (my $i = 0; $i < scalar($queue->@*); $i++) {
> +        my $task = $queue->[$i];
> +        my $vmid = $task->{vmid};
> +
> +        my $guest_lock = try_guest_lock($self, $vmid);
> +        if (!$guest_lock) {
> +            my $holder = PVE::Tools::file_read_firstline("$lockdir/guest-$vmid.lock") // '';

Nit: could use PVE::File

> +            if (!defined($deferred->{$vmid})) {
> +                $deferred->{$vmid} = time();
> +                debugmsg('info', "VM $vmid is being backed up by '$holder', deferring");
> +            } elsif (time() - $deferred->{$vmid} >= $maxwait) {
> +                splice($queue->@*, $i--, 1);
> +                skip_task($task, "still being backed up by '$holder'");
> +            }
> +            next;
> +        }
> +