From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <t.lamprecht@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256)
 (No client certificate requested)
 by lists.proxmox.com (Postfix) with ESMTPS id 1541669DEE
 for <pmg-devel@lists.proxmox.com>; Thu, 25 Feb 2021 11:04:38 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
 by firstgate.proxmox.com (Proxmox) with ESMTP id 08A9932AED
 for <pmg-devel@lists.proxmox.com>; Thu, 25 Feb 2021 11:04:38 +0100 (CET)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
 [212.186.127.180])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256)
 (No client certificate requested)
 by firstgate.proxmox.com (Proxmox) with ESMTPS id D0A4632ADE
 for <pmg-devel@lists.proxmox.com>; Thu, 25 Feb 2021 11:04:36 +0100 (CET)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
 by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 9FD3A46312
 for <pmg-devel@lists.proxmox.com>; Thu, 25 Feb 2021 11:04:36 +0100 (CET)
Message-ID: <e42b4d5c-f6b5-d4c3-9726-cc72e700ea5c@proxmox.com>
Date: Thu, 25 Feb 2021 11:04:35 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:86.0) Gecko/20100101
 Thunderbird/86.0
Content-Language: en-US
To: Stoiko Ivanov <s.ivanov@proxmox.com>, pmg-devel@lists.proxmox.com
References: <20210224183109.29014-1-s.ivanov@proxmox.com>
 <20210224183109.29014-4-s.ivanov@proxmox.com>
From: Thomas Lamprecht <t.lamprecht@proxmox.com>
In-Reply-To: <20210224183109.29014-4-s.ivanov@proxmox.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
X-SPAM-LEVEL: Spam detection results:  0
 AWL -0.427 Adjusted score from AWL reputation of From: address
 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
 NICE_REPLY_A           -0.001 Looks like a legit reply (A)
 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
 URIBL_SBL 0.644 Contains an URL's NS IP listed in the Spamhaus SBL blocklist
 [backup.pm]
 URIBL_SBL_A 0.1 Contains URL's A record listed in the Spamhaus SBL blocklist
 [backup.pm]
Subject: Re: [pmg-devel] [PATCH pmg-api 3/5] backup: fix #3146 add email
 notifications
X-BeenThere: pmg-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Mail Gateway development discussion
 <pmg-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pmg-devel>, 
 <mailto:pmg-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pmg-devel/>
List-Post: <mailto:pmg-devel@lists.proxmox.com>
List-Help: <mailto:pmg-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel>, 
 <mailto:pmg-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Thu, 25 Feb 2021 10:04:38 -0000

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.