From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 ; 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 ; 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 ; 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 ; Thu, 25 Feb 2021 11:04:36 +0100 (CET) Message-ID: 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 , pmg-devel@lists.proxmox.com References: <20210224183109.29014-1-s.ivanov@proxmox.com> <20210224183109.29014-4-s.ivanov@proxmox.com> From: Thomas Lamprecht 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-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 > --- > 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.