Re: [pmg-devel] [PATCH pmg-api 3/5] backup: fix #3146 add email notifications

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 24.02.21 19:31, Stoiko Ivanov wrote:
> this patch addresses the missing email notification for scheduled
> backup related tasks, which we have in all our other products, for our
> mail product.
> 
> the parameter names are inspired by PBS' datastore config.
> 
> it uses the templateing system for the notification, because this
> results in less code and a bit of added flexibility for the users.
> 
> the recipient address is currently hardcoded to the admin address in
> pmg.conf as we also send the (admin) pmgreport there, and I did not
> want to overengineer this (even more).
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  src/Makefile                         |  3 ++-
>  src/PMG/API2/PBS/Job.pm              | 40 +++++++++++++++++++++-------
>  src/PMG/Backup.pm                    | 31 +++++++++++++++++++++
>  src/PMG/PBSConfig.pm                 |  7 +++++
>  src/templates/backup-notification.tt | 19 +++++++++++++
>  5 files changed, 89 insertions(+), 11 deletions(-)
>  create mode 100644 src/templates/backup-notification.tt
> 
> diff --git a/src/Makefile b/src/Makefile
> index 9d5c335..0ad805d 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -42,7 +42,8 @@ TEMPLATES =				\
>  	freshclam.conf.in		\
>  	clamd.conf.in 			\
>  	postgresql.conf.in		\
> -	pg_hba.conf.in
> +	pg_hba.conf.in			\
> +	backup-notification.tt

please add a trailing backslash, like one would do with colons in a array/hash,
make can handle that as long as there's a empty line below.

>  
>  TEMPLATES_FILES = $(addprefix templates/, ${TEMPLATES})
>  
> diff --git a/src/PMG/API2/PBS/Job.pm b/src/PMG/API2/PBS/Job.pm
> index 2387422..279afbc 100644
> --- a/src/PMG/API2/PBS/Job.pm
> +++ b/src/PMG/API2/PBS/Job.pm
> @@ -273,6 +273,13 @@ __PACKAGE__->register_method ({
>  		optional => 1,
>  		default => 1,
>  	    },
> +	    notify => {
> +		description => "Specify when to notify via e-mail",
> +		type => 'string',
> +		enum => [ 'always', 'error', 'never' ],
> +		optional => 1,
> +		default => 'error',

but you default to never below??

> +	    },
>  	},
>      },
>      returns => { type => "string" },
> @@ -292,6 +299,7 @@ __PACKAGE__->register_method ({
>  	die "PBS remote '$remote' is disabled\n" if $remote_config->{disable};
>  
>  	$param->{statistic} //= $remote_config->{'include-statistics'} // 1;
> +	my $notify = $param->{notify} // $remote_config->{notify} // 'never';
>  
>  	my $pbs = PVE::PBSClient->new($remote_config, $remote, $conf->{secret_dir});
>  	my $backup_dir = "/var/lib/pmg/backup/current";
> @@ -299,30 +307,42 @@ __PACKAGE__->register_method ({
>  	my $worker = sub {
>  	    my $upid = shift;
>  
> -	    print "starting update of current backup state\n";
> +	    my $log = "starting update of current backup state\n";

I do not like that immediate printing is avoided.

rather add a closure which handles both, something like

my $full_log = "";
my $log = sub {
    my $line = shift . "\n";
    print $line;
    $full_log .= $line;
};

Or shorter version:

my $full_log = "";
my $log = sub { print "$_[0]\n"; $full_log .= "$_[0]\n"; };

Its not really doing something overly clever so this
would work fine here too, IMO:

>  
> -	    -d $backup_dir || mkdir $backup_dir;
> -	    PMG::Backup::pmg_backup($backup_dir, $param->{statistic});
> +	    eval {

this holding off throwing an exception until the very end is also quite a change
in behavior, not really related to mail notifications?

Maybe pull such stuff out in a patch before this one, but actually I'm rather unsure
about it, see below.

> +		-d $backup_dir || mkdir $backup_dir;
> +		PMG::Backup::pmg_backup($backup_dir, $param->{statistic});
>  
> -	    $pbs->backup_fs_tree($backup_dir, $node, 'pmgbackup');
> +		$pbs->backup_fs_tree($backup_dir, $node, 'pmgbackup');
>  
> -	    rmtree $backup_dir;
> +		rmtree $backup_dir;
> +	    };> +	    my $err = $@;
> +	    $log .= $err if $err;
>  
> -	    print "backup finished\n";
> +	    $log .= "backup finished\n";
>  

we still prune if there's an error above now, I mean normally without manual
intervention this'd be a no-op, but if one made manual backups in-between it could
come to a surprise.

Such a change can be fine, but must definitively be its own patch, not "hidden"
in a mail-notification patch..

>  	    my $group = "host/$node";
>  	    if (defined(my $prune_opts = $conf->prune_options($remote))) {
> -		print "starting prune of $group\n";
> -		my $res = $pbs->prune_group(undef, $prune_opts, $group);
> +		$log .= "starting prune of $group\n";
> +		my $res = eval { $pbs->prune_group(undef, $prune_opts, $group) };
>  
> +		$log .= $@ if $@;
> +		$err //= $@ if $@;
>  		foreach my $pruned (@$res){
>  		    my $time = strftime("%FT%TZ", gmtime($pruned->{'backup-time'}));
>  		    my $snap = $pruned->{'backup-type'} . '/' . $pruned->{'backup-id'} . '/' .  $time;
> -		    print "pruned snapshot: $snap\n";
> +		    $log .= "pruned snapshot: $snap\n";
>  		}
> -		print "prune finished\n";
> +		$log .= "prune finished\n";
>  	    }
>  
> +	    PMG::Backup::send_backup_notification($notify, $remote, $log, $err);
> +
> +	    die "$err\n" if $err;
> +
> +	    print $log;
> +
>  	    return;
>  	};
>  
> diff --git a/src/PMG/Backup.pm b/src/PMG/Backup.pm
> index 303bbb4..1883d02 100644
> --- a/src/PMG/Backup.pm
> +++ b/src/PMG/Backup.pm
> @@ -5,6 +5,7 @@ use warnings;
>  use Data::Dumper;
>  use File::Basename;
>  use File::Path;
> +use POSIX qw(strftime);
>  
>  use PVE::JSONSchema qw(get_standard_option);
>  use PVE::Tools;
> @@ -376,4 +377,34 @@ sub pmg_restore {
>      die $err if $err;
>  }
>  
> +sub send_backup_notification {
> +    my ($when, $target, $log, $err) = @_;
> +
> +    return if $when eq 'never';
> +    return if ($when eq 'error' && !$err);

unnecessary parentheses, would be only required if both conditions
are checked in one statement like

return if $when eq 'never' || ($when eq 'error' && !$err);

(two returns are def. fine though)

nit: maybe rename $when to $notify or $notify_on - IMO "if when" is a bit
confusing to read.

> +
> +    my $cfg = PMG::Config->new();
> +    my $email = $cfg->get ('admin', 'email');
> +    die "no admin email configured\n" if !$email;

This is not in eval context when called so sending notification can mark the
backup job as failed? 

IMO this is not fatal, I'd rather warn that due to this sending notification is
skipped and return early.