* [pve-devel] [PATCH v4 pve-manager 1/4] vzdump: support 'notification-mode' parameter
2023-11-21 12:52 [pve-devel] [PATCH v4 manager 0/4] vzdump: add 'notification-mode' parameter Lukas Wagner
@ 2023-11-21 12:52 ` Lukas Wagner
2023-11-21 12:52 ` [pve-devel] [PATCH v4 pve-manager 2/4] ui: backup jobs: add 'notification-mode' selector for backup jobs Lukas Wagner
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Lukas Wagner @ 2023-11-21 12:52 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread* [pve-devel] [PATCH v4 pve-manager 2/4] ui: backup jobs: add 'notification-mode' selector for backup jobs
2023-11-21 12:52 [pve-devel] [PATCH v4 manager 0/4] vzdump: add 'notification-mode' parameter Lukas Wagner
2023-11-21 12:52 ` [pve-devel] [PATCH v4 pve-manager 1/4] vzdump: support " Lukas Wagner
@ 2023-11-21 12:52 ` Lukas Wagner
2023-11-21 12:52 ` [pve-devel] [PATCH v4 pve-manager 3/4] ui: backup: add 'notification-mode' param for one-shot " Lukas Wagner
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Lukas Wagner @ 2023-11-21 12:52 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread* [pve-devel] [PATCH v4 pve-manager 3/4] ui: backup: add 'notification-mode' param for one-shot backup jobs.
2023-11-21 12:52 [pve-devel] [PATCH v4 manager 0/4] vzdump: add 'notification-mode' parameter Lukas Wagner
2023-11-21 12:52 ` [pve-devel] [PATCH v4 pve-manager 1/4] vzdump: support " Lukas Wagner
2023-11-21 12:52 ` [pve-devel] [PATCH v4 pve-manager 2/4] ui: backup jobs: add 'notification-mode' selector for backup jobs Lukas Wagner
@ 2023-11-21 12:52 ` Lukas Wagner
2023-11-21 12:52 ` [pve-devel] [PATCH v4 pve-manager 4/4] ui: backup job: change field text for 'mailnotification' field Lukas Wagner
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Lukas Wagner @ 2023-11-21 12:52 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread* [pve-devel] [PATCH v4 pve-manager 4/4] ui: backup job: change field text for 'mailnotification' field
2023-11-21 12:52 [pve-devel] [PATCH v4 manager 0/4] vzdump: add 'notification-mode' parameter Lukas Wagner
` (2 preceding siblings ...)
2023-11-21 12:52 ` [pve-devel] [PATCH v4 pve-manager 3/4] ui: backup: add 'notification-mode' param for one-shot " Lukas Wagner
@ 2023-11-21 12:52 ` Lukas Wagner
2023-11-21 13:28 ` [pve-devel] [PATCH v4 manager 0/4] vzdump: add 'notification-mode' parameter Philipp Hufnagl
2023-11-21 16:33 ` [pve-devel] applied-series: " Thomas Lamprecht
5 siblings, 0 replies; 7+ messages in thread
From: Lukas Wagner @ 2023-11-21 12:52 UTC (permalink / raw)
To: pve-devel
... 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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [pve-devel] [PATCH v4 manager 0/4] vzdump: add 'notification-mode' parameter
2023-11-21 12:52 [pve-devel] [PATCH v4 manager 0/4] vzdump: add 'notification-mode' parameter Lukas Wagner
` (3 preceding siblings ...)
2023-11-21 12:52 ` [pve-devel] [PATCH v4 pve-manager 4/4] ui: backup job: change field text for 'mailnotification' field Lukas Wagner
@ 2023-11-21 13:28 ` Philipp Hufnagl
2023-11-21 16:33 ` [pve-devel] applied-series: " Thomas Lamprecht
5 siblings, 0 replies; 7+ messages in thread
From: Philipp Hufnagl @ 2023-11-21 13:28 UTC (permalink / raw)
To: pve-devel
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".
^ permalink raw reply [flat|nested] 7+ messages in thread* [pve-devel] applied-series: [PATCH v4 manager 0/4] vzdump: add 'notification-mode' parameter
2023-11-21 12:52 [pve-devel] [PATCH v4 manager 0/4] vzdump: add 'notification-mode' parameter Lukas Wagner
` (4 preceding siblings ...)
2023-11-21 13:28 ` [pve-devel] [PATCH v4 manager 0/4] vzdump: add 'notification-mode' parameter Philipp Hufnagl
@ 2023-11-21 16:33 ` Thomas Lamprecht
5 siblings, 0 replies; 7+ messages in thread
From: Thomas Lamprecht @ 2023-11-21 16:33 UTC (permalink / raw)
To: Proxmox VE development discussion, Lukas Wagner
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!
^ permalink raw reply [flat|nested] 7+ messages in thread