[pve-devel] [PATCH v4 manager 0/4] vzdump: add 'notification-mode' parameter

[pve-devel] [PATCH v4 manager 0/4] 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

Changes v3 -> v4:
  - fix eslint warnings (thx @Philipp)
  - drop already applied pve-guest-common patch



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                     | 34 ++++++-
 .../form/NotificationPolicySelector.js        |  2 +-
 www/manager6/window/Backup.js                 | 25 +++++
 4 files changed, 116 insertions(+), 40 deletions(-)


Summary over all repositories:
  4 files changed, 116 insertions(+), 40 deletions(-)

-- 
murpp v0.4.0

[pve-devel] [PATCH v4 pve-manager 1/4] 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 v4 pve-manager 2/4] 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>
---

Notes:
    Changes v3 -> v4:
      - fix eslint warnings

 www/manager6/dc/Backup.js | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/www/manager6/dc/Backup.js b/www/manager6/dc/Backup.js
index 9aae4090..1258772b 100644
--- a/www/manager6/dc/Backup.js
+++ b/www/manager6/dc/Backup.js
@@ -206,12 +206,14 @@ 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) =>
+		['auto', 'legacy-sendmail', '__default__'].includes(get('notificationMode')),
 	},
     },
 
@@ -301,6 +303,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 +333,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

[pve-devel] [PATCH v4 pve-manager 3/4] 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>
---

Notes:
    Changes v3 -> v4:
      - Fix eslint warnings

 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..4418a9c7 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

[pve-devel] [PATCH v4 pve-manager 4/4] 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 1258772b..70903bdc 100644
--- a/www/manager6/dc/Backup.js
+++ b/www/manager6/dc/Backup.js
@@ -327,7 +327,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

Re: [pve-devel] [PATCH v4 manager 0/4] vzdump: add 'notification-mode' parameter

[Philipp Hufnagl]

On 11/21/23 13:52, Lukas Wagner wrote:
> 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
> 
> Changes v3 -> v4:
>   - fix eslint warnings (thx @Philipp)
>   - drop already applied pve-guest-common patch
> 
> 
> 
> 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                     | 34 ++++++-
>  .../form/NotificationPolicySelector.js        |  2 +-
>  www/manager6/window/Backup.js                 | 25 +++++
>  4 files changed, 116 insertions(+), 40 deletions(-)
> 
> 
> Summary over all repositories:
>   4 files changed, 116 insertions(+), 40 deletions(-)
> 

I do not know if this is expected but when I enter an invalid email
address for a user, and then press "Test" for SMTP, I get a message
that the email has been sent successfully, however the receiver does
not get any email and the sender account gets "Undelivered Mail
Returned to Sender".

[pve-devel] applied-series: [PATCH v4 manager 0/4] vzdump: add 'notification-mode' parameter

[Thomas Lamprecht]

Am 21/11/2023 um 13:52 schrieb 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
> 
> Changes v3 -> v4:
>   - fix eslint warnings (thx @Philipp)
>   - drop already applied pve-guest-common patch
> 
> 
> 
> 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                     | 34 ++++++-
>  .../form/NotificationPolicySelector.js        |  2 +-
>  www/manager6/window/Backup.js                 | 25 +++++
>  4 files changed, 116 insertions(+), 40 deletions(-)
> 
> 
> Summary over all repositories:
>   4 files changed, 116 insertions(+), 40 deletions(-)
> 


applied, thanks!