Re: [PATCH manager 11/19] vzdump: back up guests of a job in parallel

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

Am 02.11.26 um 12:02 AM schrieb Thomas Lamprecht:
> +my sub fork_guest_backup {
> +    my ($self, $task, $running, $interrupted) = @_;
> +
> +    pipe(my $output_reader, my $output_writer) or die "unable to create pipe - $!\n";
> +    pipe(my $result_reader, my $result_writer) or die "unable to create pipe - $!\n";
> +    pipe(my $start_reader, my $start_writer) or die "unable to create pipe - $!\n";
> +
> +    # Until the child set up its own handlers, a signal would only reach the inherited one of the
> +    # job process and get lost.
> +    my $signals = POSIX::SigSet->new(
> +        POSIX::SIGINT(),
> +        POSIX::SIGTERM(),
> +        POSIX::SIGQUIT(),
> +        POSIX::SIGHUP(),
> +        POSIX::SIGPIPE(),
> +    );
> +    my $old_mask = POSIX::SigSet->new();
> +    POSIX::sigprocmask(POSIX::SIG_BLOCK(), $signals, $old_mask)
> +        or die "unable to block signals - $!\n";
> +
> +    my $pid = fork();
> +    if (!defined($pid)) {
> +        my $err = $!;
> +        POSIX::sigprocmask(POSIX::SIG_SETMASK(), $old_mask);
> +        die "unable to fork guest backup - $err\n";
> +    }
> +
> +    if (!$pid) {
> +        my $interrupted = 0;
> +        $SIG{INT} = $SIG{TERM} = $SIG{QUIT} = $SIG{HUP} = $SIG{PIPE} = sub {
> +            # a second signal must not abort the cleanup the first one started
> +            die "interrupted by signal\n" if !$interrupted++;
> +        };
> +
> +        my $exitcode = 1;
> +        eval {
> +            POSIX::sigprocmask(POSIX::SIG_SETMASK(), $old_mask);
> +            close($output_reader);
> +            close($result_reader);
> +            close($start_writer);
> +            # only the job process reads the output of the other guest backups
> +            for my $sibling (values $running->%*) {
> +                close($_) for grep { defined } $sibling->@{qw(output result)};
> +            }
> +            open(STDOUT, '>&', $output_writer) or die "unable to redirect STDOUT - $!\n";
> +            open(STDERR, '>&', $output_writer) or die "unable to redirect STDERR - $!\n";
> +            close($output_writer);
> +
> +            sysread($start_reader, my $start, 1) or die "interrupted by signal\n";

Could maybe warrant a short comment why this error message is used here.
See also my next comment below.

> +            close($start_reader);
> +
> +            PVE::INotify::inotify_close();
> +
> +            eval { $self->exec_backup_task($task) };
> +            my $err = $@;
> +
> +            my $result = { map { $_ => $task->{$_} } $guest_result_keys->@* };
> +            print $result_writer Storable::nfreeze($result);
> +            close($result_writer);
> +
> +            die $err if $err;
> +            $exitcode = 0;
> +        };
> +        warn $@ if $@ && $@ !~ m/^interrupted by signal$/;
> +        STDOUT->flush();
> +        POSIX::_exit($exitcode);
> +    }
> +
> +    POSIX::sigprocmask(POSIX::SIG_SETMASK(), $old_mask);
> +
> +    close($output_writer);
> +    close($result_writer);
> +    close($start_reader);
> +
> +    # A stop request that came in while forking reached only the job process, so the backup
> +    # process must not start then.
> +    my $cancelled = $$interrupted;
> +    if (!$cancelled) {
> +        local $SIG{PIPE} = 'IGNORE';
> +        syswrite($start_writer, '1');
> +    }

Should we send a different message if cancelled? Then we could
distinguish other failure from that above.

> +    close($start_writer);
> +
> +    return {
> +        pid => $pid,
> +        cancelled => $cancelled,
> +        task => $task,
> +        output => $output_reader,
> +        result => $result_reader,
> +        'output-buffer' => '',
> +        'result-data' => '',
> +    };
> +}
> +
> +my sub forward_guest_output {
> +    my ($child, $data, $final) = @_;
> +
> +    my $vmid = $child->{task}->{vmid};
> +
> +    $child->{'output-buffer'} .= $data;
> +    $child->{'output-buffer'} .= "\n" if $final && length($child->{'output-buffer'});
> +
> +    while ($child->{'output-buffer'} =~ s/^(.*)\n//) {
> +        my $line = $1;
> +        # warnings of a guest backup process count for the task, but its counter ends with it
> +        PVE::RPCEnvironment::get()->{warning_count}++ if $line =~ m/^WARN: /;
> +        print STDERR "$vmid: $line\n";
> +    }
> +}
> +
> +my sub read_guest_backup_pipe {
> +    my ($child, $kind) = @_;
> +
> +    my $count = sysread($child->{$kind}, my $data, 64 * 1024);
> +    return if !defined($count) && ($!{EINTR} || $!{EAGAIN});
> +
> +    if (!$count) {
> +        close(delete $child->{$kind});
> +        forward_guest_output($child, '', 1) if $kind eq 'output';
> +    } elsif ($kind eq 'output') {
> +        forward_guest_output($child, $data);
> +    } else {
> +        $child->{'result-data'} .= $data;
> +    }
> +}
> +
> +my sub finish_guest_backup {
> +    my ($child, $status) = @_;
> +
> +    # the backup process is gone, but helpers it started might still hold the output pipe
> +    for my $kind (qw(result output)) {
> +        while ($child->{$kind} && IO::Select->new($child->{$kind})->can_read(0)) {
> +            read_guest_backup_pipe($child, $kind);
> +        }
> +        close(delete $child->{$kind}) if $child->{$kind};
> +    }
> +    forward_guest_output($child, '', 1);

This might be a second call to forward_guest_output() with $final=1 if
it was called by read_guest_backup_pipe() already. Should be guarded by
checking $child->{output}, or?

> +    return if $child->{cancelled};
> +
> +    my $task = $child->{task};
> +    my $result = eval { Storable::thaw($child->{'result-data'}) };
> +    if (ref($result) eq 'HASH') {
> +        $task->{$_} = $result->{$_} for keys $result->%*;
> +    } else {

Might be better as a separate "if ($status != 0)" instead of an else?

> +        my $reason = $status & 127 ? "signal " . ($status & 127) : "exit code " . ($status >> 8);
> +        fail_task($task, "backup process ended unexpectedly ($reason)");
> +    }
> +}
> +