From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 84ABE1FF0AF for ; Fri, 06 Nov 2026 15:49:33 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 3509A21668; Fri, 06 Nov 2026 15:49:14 +0100 (CET) Message-ID: <5bf543d6-b5c4-4d63-8187-023fd4da159c@proxmox.com> Date: Fri, 6 Nov 2026 15:49:02 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH manager 11/19] vzdump: back up guests of a job in parallel To: Thomas Lamprecht , pve-devel@lists.proxmox.com References: <20261101225849.884587-1-t.lamprecht@proxmox.com> <20261101225849.884587-12-t.lamprecht@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <20261101225849.884587-12-t.lamprecht@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1793976542060 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.290 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: TH2J7JAHKDPLAXFP3MTKPHTEATYQJEPN X-Message-ID-Hash: TH2J7JAHKDPLAXFP3MTKPHTEATYQJEPN X-MailFrom: f.ebner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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)"); > + } > +} > +