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

[pve-devel] [PATCH v2 guest-common/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



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 (3):
  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.

 PVE/VZDump.pm                 | 95 +++++++++++++++++++++--------------
 www/manager6/dc/Backup.js     | 33 +++++++++++-
 www/manager6/window/Backup.js | 25 +++++++++
 3 files changed, 115 insertions(+), 38 deletions(-)


Summary over all repositories:
  4 files changed, 129 insertions(+), 38 deletions(-)

-- 
murpp v0.4.0

[pve-devel] [PATCH v2 pve-guest-common 1/4] 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] applied: [PATCH v2 pve-guest-common 1/4] vzdump: config: add 'notification-mode' param for backup jobs

[Thomas Lamprecht]

Am 21/11/2023 um 11:22 schrieb 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(+)
> 
>

applied, thanks!

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

[pve-devel] [PATCH v2 pve-manager 4/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>
---
 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 v2 guest-common/manager 0/4] vzdump: add 'notification-mode' parameter

[Lukas Wagner]

This is actually v1, not v2. My bad.

-- 
- Lukas

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

[Fiona Ebner]

Am 21.11.23 um 11:22 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)

Can't we detect and avoid this more easily?

>   - notifications always being sent, even if 'mailnotification' is set to failure

Can't we just treat 'failure' mode as always defaulting to legacy
sendmail? And properly deprecate the setting, showing a warning/info
that new notification system is not used if set to 'failure' for both
CLI and UI. And maybe not even allow setting it for new jobs/manual backups?

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

[Lukas Wagner]

On 11/21/23 11:55, Fiona Ebner wrote:
> Am 21.11.23 um 11:22 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)
> 
> Can't we detect and avoid this more easily?

There would be other ways to solve this, yes. I could also deduplicate
email-addresses in the backend - which isn't is as trivial as it sounds, 
since the 'legacy' mails are sent via separate, temporary target of
type 'sendmail' - so essentially I'd need to have 'cross-target' context
or something alike.

> 
>>    - notifications always being sent, even if 'mailnotification' is set to failure
> 
> Can't we just treat 'failure' mode as always defaulting to legacy
> sendmail? And properly deprecate the setting, showing a warning/info
> that new notification system is not used if set to 'failure' for both
> CLI and UI. And maybe not even allow setting it for new jobs/manual backups?

I think an explicit switch here is much more obvious and predictable to 
the user.

Personally I think we should wait a bit before deprecating/disallowing 
'mailto' for new backup jobs. This gives us time to polish the UX of
creating matchers etc. without 'forcing' the user into the new system.

-- 
- Lukas

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

[Fiona Ebner]

Am 21.11.23 um 13:13 schrieb Lukas Wagner:
> On 11/21/23 11:55, Fiona Ebner wrote:
>> Am 21.11.23 um 11:22 schrieb Lukas Wagner:
>>> 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)
>>
>> Can't we detect and avoid this more easily?
> 
> There would be other ways to solve this, yes. I could also deduplicate
> email-addresses in the backend - which isn't is as trivial as it sounds,
> since the 'legacy' mails are sent via separate, temporary target of
> type 'sendmail' - so essentially I'd need to have 'cross-target' context
> or something alike.
> 

Okay, I hoped you could get the information about whether the 'mailto'
address is already notified via existing targets more easily.

>>
>>>    - notifications always being sent, even if 'mailnotification' is
>>> set to failure
>>
>> Can't we just treat 'failure' mode as always defaulting to legacy
>> sendmail? And properly deprecate the setting, showing a warning/info
>> that new notification system is not used if set to 'failure' for both
>> CLI and UI. And maybe not even allow setting it for new jobs/manual
>> backups?
> 
> I think an explicit switch here is much more obvious and predictable to
> the user.
> 

But also more complexity/mental load for both them and us. And having a
deprecation warning would also be obvious and predictable ;) Sure, we
(most likely) can get rid of the extra switch with PVE 9, was just
hoping for something simpler. But if that's not easily possible, then
let's go with the switch.

> Personally I think we should wait a bit before deprecating/disallowing
> 'mailto' for new backup jobs. This gives us time to polish the UX of
> creating matchers etc. without 'forcing' the user into the new system.
> 

I didn't mean deprecating 'mailto' like that, just 'mailnotification'
(being set to 'failure').

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

[Thomas Lamprecht]

Am 21/11/2023 um 13:13 schrieb Lukas Wagner:
> On 11/21/23 11:55, Fiona Ebner wrote:
>> Can't we just treat 'failure' mode as always defaulting to legacy
>> sendmail? And properly deprecate the setting, showing a warning/info
>> that new notification system is not used if set to 'failure' for both
>> CLI and UI. And maybe not even allow setting it for new jobs/manual backups?
> 
> I think an explicit switch here is much more obvious and predictable to 
> the user.
> 
> Personally I think we should wait a bit before deprecating/disallowing 
> 'mailto' for new backup jobs. This gives us time to polish the UX of
> creating matchers etc. without 'forcing' the user into the new system.
> 

I think so too, if it would be a new major release we might be a bit more
aggressive here, but for a point release we definitively should avoid
deprecation warnings, that something for next (or next next) major release,
possibly with an automated migration path for existing users (which would
have been a bit to much work to implement and test sanely in the remaining
time for next release)

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

[Philipp Hufnagl]

The build in mail-to-root target can be disabled but not be deleted.
Is this on purpose?

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

[Thomas Lamprecht]

Am 21/11/2023 um 13:29 schrieb Philipp Hufnagl:
> The build in mail-to-root target can be disabled but not be deleted.
> Is this on purpose?
> 

it's built-in after all (hardcoded in the code), so yes.