[pmg-devel] [PATCH pmg-api/gui/docs] small improvments for PBS integration

[pmg-devel] [PATCH pmg-api/gui/docs] small improvments for PBS integration

[Stoiko Ivanov]

This patchset addresses #3146 and #3154.
Additionally I added patches for small cosmetic cleanups, and a fix
for concurrent pbs-backups (they raced with each other during my tests)

patch 1/4 for pmg-gui depends on an updated pmg-docs-generator for building
(and on an updated pmg-docs for linking to the correct section)

pmg-api:
Stoiko Ivanov (5):
  backup: fix die invocation
  backup: fix #3154 add 'include-statistics' to pbs
  backup: fix #3146 add email notifications
  backup: add notify parameter to 'classic' backup
  backup: pbs: prevent race in concurrent backups

 src/Makefile                         |  3 +-
 src/PMG/API2/Backup.pm               | 21 +++++++++++--
 src/PMG/API2/PBS/Job.pm              | 47 +++++++++++++++++++++-------
 src/PMG/Backup.pm                    | 33 ++++++++++++++++++-
 src/PMG/PBSConfig.pm                 | 13 ++++++++
 src/templates/backup-notification.tt | 19 +++++++++++
 6 files changed, 120 insertions(+), 16 deletions(-)
 create mode 100644 src/templates/backup-notification.tt

pmg-gui:
Stoiko Ivanov (4):
  backup: pbs: add onlineHelp anchors
  backup: fix #3154: make statistic backup optional
  backup: pbs: fix #3154: add statistic setting to remote
  backup: pbs: fix #3146  add notify setting to remote

 js/BackupRestore.js   | 45 ++++++++++++++++++++++++++++---------------
 js/PBSConfig.js       |  1 +
 js/PBSRemoteEdit.js   | 20 +++++++++++++++++++
 js/PBSSnapshotView.js | 19 +++---------------
 4 files changed, 53 insertions(+), 32 deletions(-)

pmg-docs:
Stoiko Ivanov (3):
  backup: fix typos
  backup: add anchors to pbs sections
  backup: shortly document #3146 and #3154

 pmgbackup.adoc | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

-- 
2.20.1

[pmg-devel] [PATCH pmg-api 1/5] backup: fix die invocation

[Stoiko Ivanov]

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 src/PMG/Backup.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PMG/Backup.pm b/src/PMG/Backup.pm
index 275fa12..303bbb4 100644
--- a/src/PMG/Backup.pm
+++ b/src/PMG/Backup.pm
@@ -246,7 +246,7 @@ sub pmg_backup_pack {
 	pmg_backup($dirname, $include_statistics);
 
 	system("rm -f $filename; tar czf $filename --strip-components=1 -C $dirname .") == 0 ||
-	    die "unable to create backup archive: ERROR";
+	    die "unable to create backup archive: ERROR\n";
     };
     my $err = $@;
 
-- 
2.20.1

[pmg-devel] applied: [PATCH pmg-api 1/5] backup: fix die invocation

[Thomas Lamprecht]

On 24.02.21 19:30, Stoiko Ivanov wrote:
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  src/PMG/Backup.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
>

applied, thanks!

[pmg-devel] [PATCH pmg-api 2/5] backup: fix #3154 add 'include-statistics' to pbs

[Stoiko Ivanov]

This patch addresses part of #3154 - by adding the
'include-statistics' parameter for each remote in
/etc/pmg/pbs/pbs.conf.

The other part (actively asking the user whether to include it) is GUI
only, since the API already has the paramter for backup calls.

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 src/PMG/API2/PBS/Job.pm | 2 +-
 src/PMG/PBSConfig.pm    | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/PMG/API2/PBS/Job.pm b/src/PMG/API2/PBS/Job.pm
index b218779..2387422 100644
--- a/src/PMG/API2/PBS/Job.pm
+++ b/src/PMG/API2/PBS/Job.pm
@@ -291,7 +291,7 @@ __PACKAGE__->register_method ({
 	die "PBS remote '$remote' does not exist\n" if !$remote_config;
 	die "PBS remote '$remote' is disabled\n" if $remote_config->{disable};
 
-	$param->{statistic} //= 1;
+	$param->{statistic} //= $remote_config->{'include-statistics'} // 1;
 
 	my $pbs = PVE::PBSClient->new($remote_config, $remote, $conf->{secret_dir});
 	my $backup_dir = "/var/lib/pmg/backup/current";
diff --git a/src/PMG/PBSConfig.pm b/src/PMG/PBSConfig.pm
index 2d80e78..29cff22 100644
--- a/src/PMG/PBSConfig.pm
+++ b/src/PMG/PBSConfig.pm
@@ -90,6 +90,11 @@ sub properties {
 	    description => "Username or API token ID on the Proxmox Backup Server"
 	}),
 	fingerprint => get_standard_option('fingerprint-sha256'),
+	'include-statistics' => {
+	    description => "Include statistics in scheduled backups",
+	    type => 'boolean',
+	    optional => 1,
+	},
 	%prune_properties,
     };
 }
@@ -102,6 +107,7 @@ sub options {
 	username => { optional => 1 },
 	password => { optional => 1 },
 	fingerprint => { optional => 1 },
+	'include-statistics' => { optional => 1 },
 	'keep-last' => { optional => 1 },
 	'keep-hourly' =>  { optional => 1 },
 	'keep-daily' => { optional => 1 },
-- 
2.20.1

[pmg-devel] applied: [PATCH pmg-api 2/5] backup: fix #3154 add 'include-statistics' to pbs

[Thomas Lamprecht]

On 24.02.21 19:30, Stoiko Ivanov wrote:
> This patch addresses part of #3154 - by adding the
> 'include-statistics' parameter for each remote in
> /etc/pmg/pbs/pbs.conf.
> 
> The other part (actively asking the user whether to include it) is GUI
> only, since the API already has the paramter for backup calls.
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  src/PMG/API2/PBS/Job.pm | 2 +-
>  src/PMG/PBSConfig.pm    | 6 ++++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
>

applied, thanks!

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

[Stoiko Ivanov]

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
 
 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',
+	    },
 	},
     },
     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";
 
-	    -d $backup_dir || mkdir $backup_dir;
-	    PMG::Backup::pmg_backup($backup_dir, $param->{statistic});
+	    eval {
+		-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";
 
 	    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);
+
+    my $cfg = PMG::Config->new();
+    my $email = $cfg->get ('admin', 'email');
+    die "no admin email configured\n" if !$email;
+
+    my $nodename = PVE::INotify::nodename();
+    my $fqdn = PVE::Tools::get_fqdn($nodename);
+
+
+    my $vars = {
+	hostname => $nodename,
+	fqdn => $fqdn,
+	date => strftime("%F", localtime()),
+	target => $target,
+	log => $log,
+	err => $err,
+    };
+
+    my $tt = PMG::Config::get_template_toolkit();
+
+    my $mailfrom = "Proxmox Mail Gateway <postmaster>";
+    PMG::Utils::finalize_report($tt, 'backup-notification.tt', $vars, $mailfrom, $email);
+
+}
+
 1;
diff --git a/src/PMG/PBSConfig.pm b/src/PMG/PBSConfig.pm
index 29cff22..965d162 100644
--- a/src/PMG/PBSConfig.pm
+++ b/src/PMG/PBSConfig.pm
@@ -90,6 +90,12 @@ sub properties {
 	    description => "Username or API token ID on the Proxmox Backup Server"
 	}),
 	fingerprint => get_standard_option('fingerprint-sha256'),
+	notify => {
+	    description => "Specify when to notify via e-mail",
+	    type => 'string',
+	    enum => [ 'always', 'error', 'never' ],
+	    optional => 1,
+	},
 	'include-statistics' => {
 	    description => "Include statistics in scheduled backups",
 	    type => 'boolean',
@@ -107,6 +113,7 @@ sub options {
 	username => { optional => 1 },
 	password => { optional => 1 },
 	fingerprint => { optional => 1 },
+	notify => { optional => 1 },
 	'include-statistics' => { optional => 1 },
 	'keep-last' => { optional => 1 },
 	'keep-hourly' =>  { optional => 1 },
diff --git a/src/templates/backup-notification.tt b/src/templates/backup-notification.tt
new file mode 100644
index 0000000..3f956b0
--- /dev/null
+++ b/src/templates/backup-notification.tt
@@ -0,0 +1,19 @@
+[%- IF err -%]
+[%- SET titleprefix = "Backup failed" -%]
+[%- ELSE %]
+[%- SET titleprefix = "Backup successful" -%]
+[%- END -%]
+[%- IF target -%]
+[%- SET titlesuffix = "to ${target} - ($fqdn)" -%]
+[%- ELSE %]
+[%- SET titlesuffix = "($fqdn)" -%]
+[%- END -%]
+<html>
+  <head>
+    <title>[% titleprefix %] [% titlesuffix %]</title>
+  </head>
+  <body>
+    Detailed backup logs:<br /><br />
+    <pre>[% log %]</pre>
+  </body>
+</html>
-- 
2.20.1

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

[Thomas Lamprecht]

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.

[pmg-devel] [PATCH pmg-api 4/5] backup: add notify parameter to 'classic' backup

[Stoiko Ivanov]

for feature-parity, and since we recently had a user in our community
forum, who does regular backups via cron+rsync (small site w/o
dedicated backup host). Those setups could also benefit from this.

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 src/PMG/API2/Backup.pm | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/src/PMG/API2/Backup.pm b/src/PMG/API2/Backup.pm
index 4ea28d1..5887bb2 100644
--- a/src/PMG/API2/Backup.pm
+++ b/src/PMG/API2/Backup.pm
@@ -110,6 +110,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',
+	    },
 	},
     },
     returns => { type => "string" },
@@ -129,11 +136,19 @@ __PACKAGE__->register_method ({
 	my $worker = sub {
 	    my $upid = shift;
 
-	    print "starting backup to: $filename\n";
+	    my $log = "starting backup to: $filename\n";
+
+	    eval { PMG::Backup::pmg_backup_pack($filename, $param->{statistic}) };
+	    my $err = $@;
+	    $log .= $err if $err;
+
+	    $log .= "backup finished\n";
+
+	    PMG::Backup::send_backup_notification($param->{notify}, undef, $log, $err);
 
-	    PMG::Backup::pmg_backup_pack($filename, $param->{statistic});
+	    die "$err\n" if $err;
 
-	    print "backup finished\n";
+	    print $log;
 
 	    return;
 	};
-- 
2.20.1

Re: [pmg-devel] [PATCH pmg-api 4/5] backup: add notify parameter to 'classic' backup

[Thomas Lamprecht]

On 24.02.21 19:31, Stoiko Ivanov wrote:
> for feature-parity, and since we recently had a user in our community
> forum, who does regular backups via cron+rsync (small site w/o
> dedicated backup host). Those setups could also benefit from this.
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  src/PMG/API2/Backup.pm | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/src/PMG/API2/Backup.pm b/src/PMG/API2/Backup.pm
> index 4ea28d1..5887bb2 100644
> --- a/src/PMG/API2/Backup.pm
> +++ b/src/PMG/API2/Backup.pm
> @@ -110,6 +110,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',
> +	    },
>  	},
>      },
>      returns => { type => "string" },
> @@ -129,11 +136,19 @@ __PACKAGE__->register_method ({
>  	my $worker = sub {
>  	    my $upid = shift;
>  
> -	    print "starting backup to: $filename\n";
> +	    my $log = "starting backup to: $filename\n";

same regarding please avoiding $log at the end and no log in error case...
Just combine print and log string concat into a closure...

> +
> +	    eval { PMG::Backup::pmg_backup_pack($filename, $param->{statistic}) };
> +	    my $err = $@;
> +	    $log .= $err if $err;
> +
> +	    $log .= "backup finished\n";
> +
> +	    PMG::Backup::send_backup_notification($param->{notify}, undef, $log, $err);
>  
> -	    PMG::Backup::pmg_backup_pack($filename, $param->{statistic});
> +	    die "$err\n" if $err;
>  
> -	    print "backup finished\n";
> +	    print $log;
>  
>  	    return;
>  	};
> 

[pmg-devel] [PATCH pmg-api 5/5] backup: pbs: prevent race in concurrent backups

[Stoiko Ivanov]

If two pbs backup-creation calls happen simultaenously, it is possible
that the first removes the backup dir before the other is done
creating or sending it to the pbs remote.

non-PBS backups are not affected, since they create the files for
tar in a tempdir (indexed by PID and current time).

Noticed while having 2 schedules to different PBS instances with the
same interval and w/o random delay.

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 src/PMG/API2/PBS/Job.pm | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/PMG/API2/PBS/Job.pm b/src/PMG/API2/PBS/Job.pm
index 279afbc..e5dcb9c 100644
--- a/src/PMG/API2/PBS/Job.pm
+++ b/src/PMG/API2/PBS/Job.pm
@@ -303,13 +303,14 @@ __PACKAGE__->register_method ({
 
 	my $pbs = PVE::PBSClient->new($remote_config, $remote, $conf->{secret_dir});
 	my $backup_dir = "/var/lib/pmg/backup/current";
+	my $lockfile = "/var/lock/pmg-pbs-backup.lck";
 
 	my $worker = sub {
 	    my $upid = shift;
 
 	    my $log = "starting update of current backup state\n";
 
-	    eval {
+	    my $create_backup = sub {
 		-d $backup_dir || mkdir $backup_dir;
 		PMG::Backup::pmg_backup($backup_dir, $param->{statistic});
 
@@ -317,6 +318,10 @@ __PACKAGE__->register_method ({
 
 		rmtree $backup_dir;
 	    };
+
+	    eval {
+		PVE::Tools::lock_file($lockfile, undef, $create_backup);
+	    };
 	    my $err = $@;
 	    $log .= $err if $err;
 
-- 
2.20.1

Re: [pmg-devel] [PATCH pmg-api 5/5] backup: pbs: prevent race in concurrent backups

[Thomas Lamprecht]

On 24.02.21 19:31, Stoiko Ivanov wrote:
> If two pbs backup-creation calls happen simultaenously, it is possible

s/simultaenously/simultaneously/

> that the first removes the backup dir before the other is done
> creating or sending it to the pbs remote.
> 
> non-PBS backups are not affected, since they create the files for
> tar in a tempdir (indexed by PID and current time).

seems like that has a proven track record and avoids issues this one has,
see below.

> 
> Noticed while having 2 schedules to different PBS instances with the
> same interval and w/o random delay.
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  src/PMG/API2/PBS/Job.pm | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PMG/API2/PBS/Job.pm b/src/PMG/API2/PBS/Job.pm
> index 279afbc..e5dcb9c 100644
> --- a/src/PMG/API2/PBS/Job.pm
> +++ b/src/PMG/API2/PBS/Job.pm
> @@ -303,13 +303,14 @@ __PACKAGE__->register_method ({
>  
>  	my $pbs = PVE::PBSClient->new($remote_config, $remote, $conf->{secret_dir});
>  	my $backup_dir = "/var/lib/pmg/backup/current";
> +	my $lockfile = "/var/lock/pmg-pbs-backup.lck";
>  
>  	my $worker = sub {
>  	    my $upid = shift;
>  
>  	    my $log = "starting update of current backup state\n";
>  
> -	    eval {
> +	    my $create_backup = sub {
>  		-d $backup_dir || mkdir $backup_dir;
>  		PMG::Backup::pmg_backup($backup_dir, $param->{statistic});
>  
> @@ -317,6 +318,10 @@ __PACKAGE__->register_method ({
>  
>  		rmtree $backup_dir;
>  	    };
> +
> +	    eval {
> +		PVE::Tools::lock_file($lockfile, undef, $create_backup);

lock_file times out in 10s, as we have multiple people running into a 20s timeout
in PBS I guess this does not solves the problem at all, as the backup coming
second to the lock acquire can still always fail if backup always needs more than
10s (maybe unlikely in your fast local setup, not so unlikely if PBS is external
both are slow and/or high loaded).

Instead of bumping that timeout to dice-roll-times-100 I'd rather use different
target backups as mentioned yesterday in our lighthearted off-list lunch talk
about this.

Between same-backup job locking could be an idea, but not to sure how many people
plan to have jobs requiring minutes and setting up schedules minutely.

That could be something one could warn about in the backup task log at the end
if wanted, (there we now the duration and could check time between next run)

> +	    };
>  	    my $err = $@;
>  	    $log .= $err if $err;
>  
> 

[pmg-devel] [PATCH pmg-gui 1/4] backup: pbs: add onlineHelp anchors

[Stoiko Ivanov]

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 js/PBSConfig.js     | 1 +
 js/PBSRemoteEdit.js | 1 +
 2 files changed, 2 insertions(+)

diff --git a/js/PBSConfig.js b/js/PBSConfig.js
index 2a32d83..5e88574 100644
--- a/js/PBSConfig.js
+++ b/js/PBSConfig.js
@@ -1,6 +1,7 @@
 Ext.define('PMG.PBSScheduleEdit', {
     extend: 'Proxmox.window.Edit',
     xtype: 'pmgPBSScheduleEdit',
+    onlineHelp: 'pmgbackup_pbs_schedule',
 
     isAdd: true,
     isCreate: true,
diff --git a/js/PBSRemoteEdit.js b/js/PBSRemoteEdit.js
index e478dbd..0cb06d3 100644
--- a/js/PBSRemoteEdit.js
+++ b/js/PBSRemoteEdit.js
@@ -160,6 +160,7 @@ Ext.define('PMG.PBSInputPanel', {
 Ext.define('PMG.PBSEdit', {
     extend: 'Proxmox.window.Edit',
     xtype: 'pmgPBSEdit',
+    onlineHelp: 'pmgbackup_pbs_remotes',
 
     subject: 'Proxmox Backup Server',
     isAdd: true,
-- 
2.20.1

[pmg-devel] [PATCH pmg-gui 2/4] backup: fix #3154: make statistic backup optional

[Stoiko Ivanov]

by creating an edit-window when clicking on the respective Backup now
button.

This is the second part of the enhancement request

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 js/BackupRestore.js   | 45 ++++++++++++++++++++++++++++---------------
 js/PBSSnapshotView.js | 19 +++---------------
 2 files changed, 32 insertions(+), 32 deletions(-)

diff --git a/js/BackupRestore.js b/js/BackupRestore.js
index 3cfabd8..21a979f 100644
--- a/js/BackupRestore.js
+++ b/js/BackupRestore.js
@@ -69,6 +69,32 @@ Ext.define('PMG.RestoreWindow', {
     },
 });
 
+Ext.define('PMG.BackupWindow', {
+    extend: 'Proxmox.window.Edit',
+    xtype: 'pmgBackupWindow',
+    onlineHelp: 'chapter_pmgbackup',
+
+    showProgress: true,
+    title: gettext('Backup'),
+    isCreate: true,
+    method: 'POST',
+    submitText: gettext('Backup'),
+    fieldDefaults: {
+	labelWidth: 150,
+    },
+    showTaskViewer: true,
+    items: [
+	{
+	    xtype: 'proxmoxcheckbox',
+	    name: 'statistic',
+	    value: 1,
+	    uncheckedValue: 0,
+	    fieldLabel: gettext('Include Statistics'),
+	},
+    ],
+
+});
+
 Ext.define('PMG.BackupRestore', {
     extend: 'Ext.grid.GridPanel',
     xtype: 'pmgBackupRestore',
@@ -80,23 +106,10 @@ Ext.define('PMG.BackupRestore', {
 
 	createBackup: function() {
 	    let view = this.getView();
-	    Proxmox.Utils.API2Request({
+	    Ext.create('PMG.BackupWindow', {
 		url: "/nodes/" + Proxmox.NodeName + "/backup",
-		method: 'POST',
-		waitMsgTarget: view,
-		failure: function(response, opts) {
-		    Ext.Msg.alert(gettext('Error'), response.htmlStatus);
-		},
-		success: function(response, opts) {
-		    let upid = response.result.data;
-
-		    let win = Ext.create('Proxmox.window.TaskViewer', {
-			upid: upid,
-			autoShow: true,
-		    });
-		    view.mon(win, 'close', () => view.store.load());
-		},
-	    });
+		taskDone: () => view.store.load(),
+	    }).show();
 	},
 
 	onRestore: function() {
diff --git a/js/PBSSnapshotView.js b/js/PBSSnapshotView.js
index 841bff8..c182bfb 100644
--- a/js/PBSSnapshotView.js
+++ b/js/PBSSnapshotView.js
@@ -25,23 +25,10 @@ Ext.define('PMG.PBSConfig', {
 	    let me = this;
 	    let view = me.lookup('snapshotsGrid');
 	    let remote = me.getViewModel().get('remote');
-	    Proxmox.Utils.API2Request({
+	    Ext.create('PMG.BackupWindow', {
 		url: `/nodes/${Proxmox.NodeName}/pbs/${remote}/snapshot`,
-		method: 'POST',
-		waitMsgTarget: view,
-		failure: function(response, opts) {
-		    Ext.Msg.alert(gettext('Error'), response.htmlStatus);
-		},
-		success: function(response, opts) {
-		    let upid = response.result.data;
-
-		    let win = Ext.create('Proxmox.window.TaskViewer', {
-			upid: upid,
-		    });
-		    win.show();
-		    me.mon(win, 'close', function() { view.getStore().load(); });
-		},
-	    });
+		taskDone: () => view.getStore().load(),
+	    }).show();
 	},
 
 	reload: function(grid) {
-- 
2.20.1

[pmg-devel] [PATCH pmg-gui 3/4] backup: pbs: fix #3154: add statistic setting to remote

[Stoiko Ivanov]

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 js/PBSRemoteEdit.js | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/js/PBSRemoteEdit.js b/js/PBSRemoteEdit.js
index 0cb06d3..4fbe075 100644
--- a/js/PBSRemoteEdit.js
+++ b/js/PBSRemoteEdit.js
@@ -86,6 +86,13 @@ Ext.define('PMG.PBSInputPanel', {
 		    uncheckedValue: 0,
 		    fieldLabel: gettext('Enable'),
 		},
+		{
+		    xtype: 'proxmoxcheckbox',
+		    name: 'include-statistics',
+		    checked: true,
+		    uncheckedValue: 0,
+		    fieldLabel: gettext('Include Statistics'),
+		},
 	    ],
 	    columnB: [
 		{
-- 
2.20.1

[pmg-devel] [PATCH pmg-gui 4/4] backup: pbs: fix #3146 add notify setting to remote

[Stoiko Ivanov]

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 js/PBSRemoteEdit.js | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/js/PBSRemoteEdit.js b/js/PBSRemoteEdit.js
index 4fbe075..976b341 100644
--- a/js/PBSRemoteEdit.js
+++ b/js/PBSRemoteEdit.js
@@ -54,6 +54,18 @@ Ext.define('PMG.PBSInputPanel', {
 		    fieldLabel: 'Datastore',
 		    allowBlank: false,
 		},
+		{
+		    xtype: 'proxmoxKVComboBox',
+		    name: 'notify',
+		    fieldLabel: gettext('Notify'),
+		    comboItems: [
+			['always', 'always'],
+			['error', 'error'],
+			['never', 'never'],
+		    ],
+		    deleteEmpty: false,
+		    emptyText: gettext('never'),
+		},
 	    ],
 	    column2: [
 		{
-- 
2.20.1

[pmg-devel] [PATCH pmg-docs 1/3] backup: fix typos

[Stoiko Ivanov]

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 pmgbackup.adoc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/pmgbackup.adoc b/pmgbackup.adoc
index c91e0d1..8c27aa7 100644
--- a/pmgbackup.adoc
+++ b/pmgbackup.adoc
@@ -138,7 +138,7 @@ Server over a period of time.
 # pmgbackup remote set archive --keep-last 3 --keep-daily 14 --keep-weekly 8 --keep-monthly 12 --keep-yearly 7
 ----
 
-If prune settigns are configured, the backup-group of {pmg} is pruned
+If prune settings are configured, the backup-group of {pmg} is pruned
 automatically after each successful backup.
 
 The public settings are stored in `/etc/pmg/pbs/pbs.conf`, sensitive settings,
@@ -218,7 +218,7 @@ allows the Proxmox Backup Server to pick it up during garbage collection.
 # pmgbackup proxmox-backup forget archive pmg 2020-11-16T14:03:04Z
 ----
 
-TIP: You can configure and access all backup related functionallity on both,
+TIP: You can configure and access all backup related functionality on both,
 the web interface and the command line interface.
 
 Scheduled Backups
-- 
2.20.1

[pmg-devel] applied: [PATCH pmg-docs 1/3] backup: fix typos

[Thomas Lamprecht]

On 24.02.21 19:31, Stoiko Ivanov wrote:
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  pmgbackup.adoc | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
>

applied, thanks!

[pmg-devel] [PATCH pmg-docs 2/3] backup: add anchors to pbs sections

[Stoiko Ivanov]

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 pmgbackup.adoc | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/pmgbackup.adoc b/pmgbackup.adoc
index 8c27aa7..2a230a8 100644
--- a/pmgbackup.adoc
+++ b/pmgbackup.adoc
@@ -97,6 +97,7 @@ Analyzing/Upgrading existing Databases...done
 restore finished
 ----
 
+[[pmgbackup_pbs]]
 Proxmox Backup Server
 ---------------------
 
@@ -105,6 +106,7 @@ need configure the instance as backup 'remote'. You can then directly create
 and restore backups, as well as create a scheduled 'backup job' to run
 regular backups.
 
+[[pmgbackup_pbs_remotes]]
 Remotes
 ~~~~~~~
 
@@ -158,6 +160,7 @@ pbs: archive
         username pmgbackup@pbs!token
 ----
 
+[[pmgbackup_pbs_jobs]]
 Backup Jobs
 ~~~~~~~~~~~
 
@@ -221,6 +224,7 @@ allows the Proxmox Backup Server to pick it up during garbage collection.
 TIP: You can configure and access all backup related functionality on both,
 the web interface and the command line interface.
 
+[[pmgbackup_pbs_schedule]]
 Scheduled Backups
 ^^^^^^^^^^^^^^^^^
 
-- 
2.20.1

[pmg-devel] applied: [PATCH pmg-docs 2/3] backup: add anchors to pbs sections

[Thomas Lamprecht]

On 24.02.21 19:31, Stoiko Ivanov wrote:
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  pmgbackup.adoc | 4 ++++
>  1 file changed, 4 insertions(+)
> 
>

applied, thanks!

[pmg-devel] [PATCH pmg-docs 3/3] backup: shortly document #3146 and #3154

[Stoiko Ivanov]

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 pmgbackup.adoc | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/pmgbackup.adoc b/pmgbackup.adoc
index 2a230a8..be492ca 100644
--- a/pmgbackup.adoc
+++ b/pmgbackup.adoc
@@ -143,6 +143,10 @@ Server over a period of time.
 If prune settings are configured, the backup-group of {pmg} is pruned
 automatically after each successful backup.
 
+The `notify` and `include-statistics` setting of a remote define the defaults
+for notifications and whether to include the statistic database in backups.
+They are also used for xref:pmgbackup_pbs_schedule[scheduled backups].
+
 The public settings are stored in `/etc/pmg/pbs/pbs.conf`, sensitive settings,
 like passwords are stored in individual files named after the remote inside
 `/etc/pmg/pbs/`:
-- 
2.20.1