[pve-devel] [PATCH v3 guest-common/manager 0/5] vzdump: add 'notification-mode' parameter

[pve-devel] [PATCH v3 guest-common/manager 0/5] vzdump: add 'notification-mode' parameter

[Lukas Wagner]

This patch series adds the 'notification-mode' setting for backup jobs.
It allows users to choose between the 'old-style' notifications 
(mail to configured address, directly via a call to sendmail) or 
the 'new-style' notification system.

notification-mode has three possible values:
  - legacy-sendmail: Use old system, considering mailto/mailtnotification parameters
  - notification-system: Use the new system (always sending a notification, irregardless 
    of success/failure. The user is supposed to configure filtering/matching in 
    notification settings)
  - auto: use old system if mailto is set, or new system if not

This should provide a fix/workaround for the users' reports of
  - double notifications (these happened in case mailto was set to the same address
    as root@pam)
  - notifications always being sent, even if 'mailnotification' is set to failure

Changes v2 -> v3:
  - change field text in the GUI for the 'mailnotification' param
    This should highlight that this setting only affects the 'legacy-sendmail' 
    notification mails



pve-guest-common:

Lukas Wagner (1):
  vzdump: config: add 'notification-mode' param for backup jobs

 src/PVE/VZDump/Common.pm | 14 ++++++++++++++
 1 file changed, 14 insertions(+)


pve-manager:

Lukas Wagner (4):
  vzdump: support 'notification-mode' parameter
  ui: backup jobs: add 'notification-mode' selector for backup jobs
  ui: backup: add 'notification-mode' param for one-shot backup jobs.
  ui: backup job: change field text for 'mailnotification' field

 PVE/VZDump.pm                                 | 95 +++++++++++--------
 www/manager6/dc/Backup.js                     | 35 ++++++-
 .../form/NotificationPolicySelector.js        |  2 +-
 www/manager6/window/Backup.js                 | 25 +++++
 4 files changed, 117 insertions(+), 40 deletions(-)


Summary over all repositories:
  5 files changed, 131 insertions(+), 40 deletions(-)

-- 
murpp v0.4.0

[pve-devel] [PATCH v3 pve-guest-common 1/5] vzdump: config: add 'notification-mode' param for backup jobs

[Lukas Wagner]

'legacy-sendmail': Use mailto/mailnotification parameters and send
emails directly.
'notification-system': Always notify via notification system
'auto': Notify via mail if mailto is set, otherwise use notification
system.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 src/PVE/VZDump/Common.pm | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/src/PVE/VZDump/Common.pm b/src/PVE/VZDump/Common.pm
index b93ad86..6ee8d3c 100644
--- a/src/PVE/VZDump/Common.pm
+++ b/src/PVE/VZDump/Common.pm
@@ -188,6 +188,20 @@ my $confdesc = {
 	enum => [ 'always', 'failure' ],
 	default => 'always',
     },
+    'notification-mode' => {
+	type => 'string',
+	description => "Determine which notification system to use." .
+	    " If set to 'legacy-sendmail', vzdump will consider the" .
+	    " mailto/mailnotification parameters and send emails to the" .
+	    " specified address(es) via the 'sendmail' command." .
+	    " If set to 'notification-system', a notification will be sent via PVE's" .
+	    " notification system and mailto/mailnotification will be ignored" .
+	    " If set to 'auto' (default setting), an email will be sent if " .
+	    " mailto is set, and the notification system will be used if not.",
+	optional => 1,
+	enum => [ 'auto', 'legacy-sendmail', 'notification-system'],
+	default => 'auto',
+    },
     'notification-policy' => {
 	type => 'string',
 	description => "Deprecated: Do not use",
-- 
2.39.2

[pve-devel] [PATCH v3 pve-manager 2/5] vzdump: support 'notification-mode' parameter

[Lukas Wagner]

This parameter lets us choose between the 'legacy' notification
system (sendmail to some email addresses) and the 'new' notification
system (pub-sub based system with targets and matchers).
'auto' (default) will use the 'legacy' system if a mail address is
provided and the 'new' system if not.
This is allows users to opt-in/opt-out from the new notification
system, which might be a bit chatty by default.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 PVE/VZDump.pm | 95 +++++++++++++++++++++++++++++++--------------------
 1 file changed, 58 insertions(+), 37 deletions(-)

diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index b0574d41..4185ed62 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -452,12 +452,8 @@ sub send_notification {
     my $opts = $self->{opts};
     my $mailto = $opts->{mailto};
     my $cmdline = $self->{cmdline};
-    # Old-style notification policy. This parameter will influce
-    # if an ad-hoc notification target/matcher will be created.
-    my $policy = $opts->{"notification-policy"} //
-	$opts->{mailnotification} //
-	'always';
-
+    my $policy = $opts->{mailnotification} // 'always';
+    my $mode = $opts->{"notification-mode"} // 'auto';
 
     sanitize_task_list($tasklist);
     my $error_count = count_failed_tasks($tasklist);
@@ -499,44 +495,69 @@ sub send_notification {
     };
 
     my $fields = {
+	# TODO: There is no straight-forward way yet to get the
+	# backup job id here... (I think pvescheduler would need
+	# to pass that to the vzdump call?)
 	type => "vzdump",
 	hostname => $hostname,
     };
 
-    my $notification_config = PVE::Notify::read_config();
-
-    my $legacy_sendmail = $policy eq "always" || ($policy eq "failure" && $failed);
-
-    if ($mailto && scalar(@$mailto) && $legacy_sendmail) {
-	# <, >, @ are not allowed in endpoint names, but that is only
-	# verified once the config is serialized. That means that
-	# we can rely on that fact that no other endpoint with this name exists.
-	my $endpoint_name = "<mail-to-" . join(",", @$mailto) . ">";
-	$notification_config->add_sendmail_endpoint(
-	    $endpoint_name,
-	    $mailto,
-	    undef,
-	    undef,
-	    "vzdump backup tool");
-
-	my $endpoints = [$endpoint_name];
+    my $severity = $failed ? "error" : "info";
+    my $email_configured = $mailto && scalar(@$mailto);
+
+    if (($mode eq 'auto' && $email_configured) || $mode eq 'legacy-sendmail') {
+	if ($email_configured && ($policy eq "always" || ($policy eq "failure" && $failed))) {
+	    # Start out with an empty config. Might still contain
+	    # built-ins, so we need to disable/remove them.
+	    my $notification_config = Proxmox::RS::Notify->parse_config('', '');
+
+	    # Remove built-in matchers, since we only want to send an
+	    # email to the specified recipients and nobody else.
+	    for my $matcher (@{$notification_config->get_matchers()}) {
+		$notification_config->delete_matcher($matcher->{name});
+	    }
 
-	$notification_config->add_matcher(
-	    "<matcher-$endpoint_name>",
-	    $endpoints,
+	    # <, >, @ are not allowed in endpoint names, but that is only
+	    # verified once the config is serialized. That means that
+	    # we can rely on that fact that no other endpoint with this name exists.
+	    my $endpoint_name = "<" . join(",", @$mailto) . ">";
+	    $notification_config->add_sendmail_endpoint(
+		$endpoint_name,
+		$mailto,
+		undef,
+		undef,
+		"vzdump backup tool"
+	    );
+
+	    my $endpoints = [$endpoint_name];
+
+	    # Add a matcher that matches all notifications, set our
+	    # newly created target as a target.
+	    $notification_config->add_matcher(
+		"<matcher-$endpoint_name>",
+		$endpoints,
+	    );
+
+	    PVE::Notify::notify(
+		$severity,
+		$subject_template,
+		$body_template,
+		$notification_props,
+		$fields,
+		$notification_config
+	    );
+	}
+    } else {
+	# We use the 'new' system, or we are set to 'auto' and
+	# no email addresses were configured.
+	PVE::Notify::notify(
+	    $severity,
+	    $subject_template,
+	    $body_template,
+	    $notification_props,
+	    $fields,
 	);
     }
-
-    my $severity = $failed ? "error" : "info";
-
-    PVE::Notify::notify(
-	$severity,
-	$subject_template,
-	$body_template,
-	$notification_props,
-	$fields,
-	$notification_config
-    );
 };
 
 sub new {
-- 
2.39.2

[pve-devel] [PATCH v3 pve-manager 3/5] ui: backup jobs: add 'notification-mode' selector for backup jobs

[Lukas Wagner]

This selector allows one to selected between the 'old' (send email
directly via sendmail) or the 'new' notification system.

The default is 'auto', which sends and email if one is configured,
and uses the notification system if no email address is set.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 www/manager6/dc/Backup.js | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/www/manager6/dc/Backup.js b/www/manager6/dc/Backup.js
index 9aae4090..8f7bab5a 100644
--- a/www/manager6/dc/Backup.js
+++ b/www/manager6/dc/Backup.js
@@ -206,12 +206,15 @@ Ext.define('PVE.dc.BackupEdit', {
     viewModel: {
 	data: {
 	    selMode: 'include',
+	    notificationMode: '__default__',
 	},
 
 	formulas: {
 	    poolMode: (get) => get('selMode') === 'pool',
 	    disableVMSelection: (get) => get('selMode') !== 'include' && get('selMode') !== 'exclude',
-	    mailNotificationSelected: (get) => get('notificationMode') === 'mailto',
+	    showMailtoFields: (get) => {
+		return ['auto', 'legacy-sendmail', '__default__'].includes(get('notificationMode'));
+	    },
 	},
     },
 
@@ -301,6 +304,28 @@ Ext.define('PVE.dc.BackupEdit', {
 				},
 			    ],
 			    column2: [
+				{
+				    xtype: 'proxmoxKVComboBox',
+				    comboItems: [
+					[
+					    '__default__',
+					    Ext.String.format(
+						gettext('{0} (Auto)'), Proxmox.Utils.defaultText
+					    )
+					],
+					['auto', gettext('Auto')],
+					['legacy-sendmail', gettext('Email (legacy)')],
+					['notification-system', gettext('Notification system')],
+				    ],
+				    fieldLabel: gettext('Notification mode'),
+				    name: 'notification-mode',
+				    cbind: {
+					deleteEmpty: '{!isCreate}',
+				    },
+				    bind: {
+					value: '{notificationMode}',
+				    },
+				},
 				{
 				    xtype: 'pveEmailNotificationSelector',
 				    fieldLabel: gettext('Notify'),
@@ -309,11 +334,17 @@ Ext.define('PVE.dc.BackupEdit', {
 					value: (get) => get('isCreate') ? 'always' : '',
 					deleteEmpty: '{!isCreate}',
 				    },
+				    bind: {
+					disabled: '{!showMailtoFields}',
+				    }
 				},
 				{
 				    xtype: 'textfield',
 				    fieldLabel: gettext('Send email to'),
 				    name: 'mailto',
+				    bind: {
+					disabled: '{!showMailtoFields}',
+				    }
 				},
 				{
 				    xtype: 'pveBackupCompressionSelector',
-- 
2.39.2

Re: [pve-devel] [PATCH v3 pve-manager 3/5] ui: backup jobs: add 'notification-mode' selector for backup jobs

[Philipp Hufnagl]

On 11/21/23 13:23, Lukas Wagner wrote:
> This selector allows one to selected between the 'old' (send email
> directly via sendmail) or the 'new' notification system.
> 
> The default is 'auto', which sends and email if one is configured,
> and uses the notification system if no email address is set.
> 
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
>  www/manager6/dc/Backup.js | 33 ++++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/www/manager6/dc/Backup.js b/www/manager6/dc/Backup.js
> index 9aae4090..8f7bab5a 100644
> --- a/www/manager6/dc/Backup.js
> +++ b/www/manager6/dc/Backup.js
> @@ -206,12 +206,15 @@ Ext.define('PVE.dc.BackupEdit', {
>      viewModel: {
>  	data: {
>  	    selMode: 'include',
> +	    notificationMode: '__default__',
>  	},
>  
>  	formulas: {
>  	    poolMode: (get) => get('selMode') === 'pool',
>  	    disableVMSelection: (get) => get('selMode') !== 'include' && get('selMode') !== 'exclude',
> -	    mailNotificationSelected: (get) => get('notificationMode') === 'mailto',
> +	    showMailtoFields: (get) => {
> +		return ['auto', 'legacy-sendmail', '__default__'].includes(get('notificationMode'));
> +	    },

WARN: line 215 col 33: arrow-body-style - Unexpected block statement
surrounding arrow body; move the returned value immediately after the
`=>`. (*)

I think it wants you to inline this like

showMailtoFields: (get) => ['auto', 'legacy-sendmail',
'__default__'].includes(get('notificationMode')),



>  	},
>      },
>  
> @@ -301,6 +304,28 @@ Ext.define('PVE.dc.BackupEdit', {
>  				},
>  			    ],
>  			    column2: [
> +				{
> +				    xtype: 'proxmoxKVComboBox',
> +				    comboItems: [
> +					[
> +					    '__default__',
> +					    Ext.String.format(
> +						gettext('{0} (Auto)'), Proxmox.Utils.defaultText
nit: comma
> +					    )
nit: comma
> +					],
> +					['auto', gettext('Auto')],
> +					['legacy-sendmail', gettext('Email (legacy)')],
> +					['notification-system', gettext('Notification system')],
> +				    ],
> +				    fieldLabel: gettext('Notification mode'),
> +				    name: 'notification-mode',
> +				    cbind: {
> +					deleteEmpty: '{!isCreate}',
> +				    },
> +				    bind: {
> +					value: '{notificationMode}',
> +				    },
> +				},
>  				{
>  				    xtype: 'pveEmailNotificationSelector',
>  				    fieldLabel: gettext('Notify'),
> @@ -309,11 +334,17 @@ Ext.define('PVE.dc.BackupEdit', {
>  					value: (get) => get('isCreate') ? 'always' : '',
>  					deleteEmpty: '{!isCreate}',
>  				    },
> +				    bind: {
> +					disabled: '{!showMailtoFields}',
> +				    }
nit: comma
>  				},
>  				{
>  				    xtype: 'textfield',
>  				    fieldLabel: gettext('Send email to'),
>  				    name: 'mailto',
> +				    bind: {
> +					disabled: '{!showMailtoFields}',
> +				    }
nit: comma
>  				},
>  				{
>  				    xtype: 'pveBackupCompressionSelector',

[pve-devel] [PATCH v3 pve-manager 4/5] ui: backup: add 'notification-mode' param for one-shot backup jobs.

[Lukas Wagner]

This selector allows one to selected between the 'old' (send email
directly via sendmail) or the 'new' notification system.

The default is 'auto', which sends and email if one is configured,
and uses the notification system if no email address is set.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 www/manager6/window/Backup.js | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/www/manager6/window/Backup.js b/www/manager6/window/Backup.js
index 8d8c9ff0..1733ae7c 100644
--- a/www/manager6/window/Backup.js
+++ b/www/manager6/window/Backup.js
@@ -36,6 +36,23 @@ Ext.define('PVE.window.Backup', {
 	    emptyText: Proxmox.Utils.noneText,
 	});
 
+	let notificationModeSelector = Ext.create({
+	    xtype: 'proxmoxKVComboBox',
+	    comboItems: [
+		['auto', gettext('Auto')],
+		['legacy-sendmail', gettext('Email (legacy)')],
+		['notification-system', gettext('Notification system')],
+	    ],
+	    fieldLabel: gettext('Notification mode'),
+	    name: 'notification-mode',
+	    value: 'auto',
+	    listeners: {
+		change: function(field, value) {
+		    mailtoField.setDisabled(value === 'notification-system');
+		}
+	    }
+	});
+
 	const keepNames = [
 	    ['keep-last', gettext('Keep Last')],
 	    ['keep-hourly', gettext('Keep Hourly')],
@@ -110,6 +127,9 @@ Ext.define('PVE.window.Backup', {
 			    if (!initialDefaults && data.mailto !== undefined) {
 				mailtoField.setValue(data.mailto);
 			    }
+			    if (!initialDefaults && data['notification-mode'] !== undefined) {
+				notificationModeSelector.setValue(data['notification-mode']);
+			    }
 			    if (!initialDefaults && data.mode !== undefined) {
 				modeSelector.setValue(data.mode);
 			    }
@@ -176,6 +196,7 @@ Ext.define('PVE.window.Backup', {
 	    ],
 	    column2: [
 		compressionSelector,
+		notificationModeSelector,
 		mailtoField,
 		removeCheckbox,
 	    ],
@@ -256,6 +277,10 @@ Ext.define('PVE.window.Backup', {
 		    params.mailto = values.mailto;
 		}
 
+		if (values['notification-mode']) {
+		    params['notification-mode'] = values['notification-mode'];
+		}
+
 		if (values.compress) {
 		    params.compress = values.compress;
 		}
-- 
2.39.2

Re: [pve-devel] [PATCH v3 pve-manager 4/5] ui: backup: add 'notification-mode' param for one-shot backup jobs.

[Lukas Wagner]

On 11/21/23 13:38, Philipp Hufnagl wrote:
>> +	let notificationModeSelector = Ext.create({
>> +	    xtype: 'proxmoxKVComboBox',
>> +	    comboItems: [
>> +		['auto', gettext('Auto')],
>> +		['legacy-sendmail', gettext('Email (legacy)')],
>> +		['notification-system', gettext('Notification system')],
>> +	    ],
>> +	    fieldLabel: gettext('Notification mode'),
>> +	    name: 'notification-mode',
>> +	    value: 'auto',
>> +	    listeners: {
>> +		change: function(field, value) {
>> +		    mailtoField.setDisabled(value === 'notification-system');
>> +		}
>> +	    }
>> +	});
>> +
> 
> nit: you missed the commas on 52 on 53

Argh, thx. Deployed/tested it via `make install` instead via
`make deb`, I think eslint is a bit more linient there.

-- 
- Lukas

[pve-devel] [PATCH v3 pve-manager 5/5] ui: backup job: change field text for 'mailnotification' field

[Lukas Wagner]

... to highlight that this setting only affects the 'legacy-sendmail'
mail notifications.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---

Notes:
    New in v3.

 www/manager6/dc/Backup.js                       | 2 +-
 www/manager6/form/NotificationPolicySelector.js | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/www/manager6/dc/Backup.js b/www/manager6/dc/Backup.js
index 8f7bab5a..87cdea35 100644
--- a/www/manager6/dc/Backup.js
+++ b/www/manager6/dc/Backup.js
@@ -328,7 +328,7 @@ Ext.define('PVE.dc.BackupEdit', {
 				},
 				{
 				    xtype: 'pveEmailNotificationSelector',
-				    fieldLabel: gettext('Notify'),
+				    fieldLabel: gettext('Send email'),
 				    name: 'mailnotification',
 				    cbind: {
 					value: (get) => get('isCreate') ? 'always' : '',
diff --git a/www/manager6/form/NotificationPolicySelector.js b/www/manager6/form/NotificationPolicySelector.js
index f318ea18..d2a51386 100644
--- a/www/manager6/form/NotificationPolicySelector.js
+++ b/www/manager6/form/NotificationPolicySelector.js
@@ -2,7 +2,7 @@ Ext.define('PVE.form.EmailNotificationSelector', {
     extend: 'Proxmox.form.KVComboBox',
     alias: ['widget.pveEmailNotificationSelector'],
     comboItems: [
-	['always', gettext('Notify always')],
+	['always', gettext('Always')],
 	['failure', gettext('On failure only')],
     ],
 });
-- 
2.39.2