Re: [PATCH manager 06/19] vzdump: notification: capture guest logs right after each backup

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

Am 01.11.26 um 11:59 PM schrieb Thomas Lamprecht:
> The log of a guest backup is written to a path derived from the guest
> alone and was only read back when sending the notification at the end
> of the job. This relies on no other backup of the guest starting in the
> meantime, which no longer holds once backup jobs can run alongside each
> other.
> 
> Read the log as the last step of each guest backup instead. Logs
> longer than the notification size limit are cut, as they would not be
> included anyway.
> 
> Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
> ---
>  PVE/VZDump.pm | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
> index defe9e0a5..d07ced61b 100644
> --- a/PVE/VZDump.pm
> +++ b/PVE/VZDump.pm
> @@ -368,24 +368,35 @@ sub read_vzdump_defaults {
>      return $res;
>  }
>  
> -my sub read_backup_task_logs {
> +use constant MAX_LOG_SIZE => 1024 * 1024;
> +
> +my sub read_guest_log {
> +    my ($path) = @_;
> +
> +    open(my $fh, '<', $path) or return undef;

Nit: should we log/capture the error code?

> +    my $log = '';
> +    read($fh, $log, MAX_LOG_SIZE + 1); # anything longer is not sent anyway
> +    close($fh);
> +
> +    return $log;
> +}
> +
> +my sub format_backup_task_logs {
>      my ($task_list) = @_;
>  
>      my $task_logs = "";
>  
>      for my $task (@$task_list) {
>          my $vmid = $task->{vmid};
> -        my $log_file = $task->{tmplog};
>          if (!$task->{tmplog}) {
>              $task_logs .= "$vmid: no log available\n\n";
>              next;
>          }
> -        if (open(my $TMP, '<', "$log_file")) {
> -            while (my $line = <$TMP>) {
> +        if (defined(my $log = $task->{log})) {
> +            for my $line (split(/^/m, $log)) {
>                  next if $line =~ /^status: \d+/; # not useful in mails
>                  $task_logs .= encode8bit("$vmid: $line");
>              }
> -            close($TMP);
>          } else {
>              $task_logs .= "$vmid: Could not open log file\n\n";
>          }
> @@ -484,8 +495,6 @@ my sub get_hostname {
>      return $hostname;
>  }
>  
> -use constant MAX_LOG_SIZE => 1024 * 1024;
> -
>  sub send_notification {
>      my ($self, $tasklist, $total_time, $err, $detail_pre, $detail_post) = @_;
>  
> @@ -514,7 +523,7 @@ sub send_notification {
>  
>      my $text_log_part = "$cmdline\n\n";
>      $text_log_part .= $detail_pre . "\n" if defined($detail_pre);
> -    $text_log_part .= read_backup_task_logs($tasklist);
> +    $text_log_part .= format_backup_task_logs($tasklist);
>      $text_log_part .= $detail_post if defined($detail_post);
>  
>      if (length($text_log_part) > MAX_LOG_SIZE) {
> @@ -1385,6 +1394,8 @@ sub exec_backup_task {
>      eval { $self->run_hook_script('log-end', $task); };
>      debugmsg('warn', $@) if $@; # message already contains command with phase name
>  
> +    $task->{log} = read_guest_log($task->{tmplog}) if $task->{tmplog};

Nit: we could unlink $task->{tmplog} here after reading, and maybe even
drop it from the hash since we are not supposed to access it afterwards
anymore. Of course then the check for $task->{tmplog} in
format_backup_task_logs() would need to be dropped too. We could put
"$vmid: no log available\n\n" in $task->{log} at the beginning of the
task, so we end up with that message if it never is replaced.

> +
>      die $err if $err && $err =~ m/^interrupted by signal$/;
>  }
>