[pve-devel] [PATCH cluster/ha-manager/manager 0/6] preparation for #6143: notification template cleanup

[pve-devel] [PATCH cluster/ha-manager/manager 0/6] preparation for #6143: notification template cleanup

[Lukas Wagner]

With [#6143] on the horizon, notification templates, template variables
and template helpers become part of our public API and as such
we should provide some stability guarantees for them.
As a result, we use this opportunity to do a 'final' cleanup.

This series:
  - adds some common template vars for all notifications
    - hostname
    - fqdn
    - cluster-name
  - For HTML tables, we move the table generation to the handlebars template
    instead of using the {{ table }} helper. This gives users a
    better starting point for their changes (styling, structure).
    The plaintext version still uses the helper, mostly due to the
    fact that the helper automatically determines appropriate
    column widths
  - changes the names of some template variables for better clarity
  - For fencing notifications, we stop dumping status data as JSON
    to the notification and instead try to render it nicely, trying
    to include all useful info from the former JSON dump

Bumps:

The patches for pve-manager and pve-ha-manager must depend on a bumped
libpve-notify-perl due to the new 'PVE::Notify::common_template_data'
helper.

pve-cluster:

Lukas Wagner (1):
  notify: add common_template_data

 src/PVE/Notify.pm | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)


pve-manager:

Lukas Wagner (4):
  notification templates: vzdump: generate HTML table in template
  notifications: apt: clean up notification template
  notification: replication: add common properties to template data
  notifications: test: style fixup

 PVE/API2/APT.pm                               | 27 ++++++++-----------
 PVE/API2/Replication.pm                       | 16 +++++------
 PVE/VZDump.pm                                 | 21 +++++++--------
 .../default/package-updates-body.html.hbs     | 15 ++++++++++-
 .../default/package-updates-body.txt.hbs      |  2 +-
 .../default/package-updates-subject.txt.hbs   |  2 +-
 templates/default/replication-body.txt.hbs    |  2 +-
 templates/default/test-body.html.hbs          |  2 +-
 templates/default/vzdump-body.html.hbs        | 24 +++++++++++++++--
 templates/default/vzdump-body.txt.hbs         |  2 +-
 templates/default/vzdump-subject.txt.hbs      |  2 +-
 11 files changed, 69 insertions(+), 46 deletions(-)


pve-ha-manager:

Lukas Wagner (1):
  notifications: overhaul fence notification

 src/PVE/HA/NodeStatus.pm                      | 43 +++++++++++++++----
 src/PVE/HA/Sim/Env.pm                         |  8 ++--
 src/templates/default/fencing-body.html.hbs   | 43 ++++++++++++++++---
 src/templates/default/fencing-body.txt.hbs    | 40 +++++++++++++----
 src/templates/default/fencing-subject.txt.hbs |  6 ++-
 5 files changed, 111 insertions(+), 29 deletions(-)


Summary over all repositories:
  17 files changed, 200 insertions(+), 75 deletions(-)

-- 
Generated by git-murpp 0.8.0


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH pve-cluster 1/1] notify: add common_template_data

[Lukas Wagner]

This commit add the `common_template_data` sub to PVE::Notify,
providing a convenient way to get a hash with properties that
should be accessible from all templates, namely hostname, fqdn
and cluster-name.

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

diff --git a/src/PVE/Notify.pm b/src/PVE/Notify.pm
index c514111..9f7a74c 100644
--- a/src/PVE/Notify.pm
+++ b/src/PVE/Notify.pm
@@ -4,6 +4,7 @@ use strict;
 use warnings;
 
 use PVE::Cluster qw(cfs_register_file cfs_read_file cfs_lock_file cfs_write_file);
+use PVE::INotify;
 use Proxmox::RS::Notify;
 
 cfs_register_file(
@@ -130,4 +131,23 @@ sub check_may_use_target {
     }
 }
 
+sub common_template_data {
+    my $hostname = PVE::INotify::nodename();
+    my $fqdn = `hostname -f` || $hostname;
+    chomp $fqdn;
+
+    my $data = {
+        hostname => $hostname,
+        fqdn => $fqdn,
+    };
+
+    my $cluster_info = PVE::Cluster::get_clinfo();
+
+    if (my $d = $cluster_info->{cluster}) {
+        $data->{"cluster-name"} = $d->{name};
+    }
+
+    return $data;
+}
+
 1;
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH pve-manager 1/4] notification templates: vzdump: generate HTML table in template

[Lukas Wagner]

Instead of relying on the 'magic' table helper, we generate the guest
list in the vzdump HTML notification template using native Handlebars
syntax ({{#each}}). The template becomes a bit more ugly, mostly due to
HTML email's requirements to use inline CSS styling, but in the context
of providing user-customizable notification templates this is much
nicer. It gives the user a starting point for their modificiations
(custom styling, changing table formatting/order/etc.), which was not
possible before.

The plaintext template stays as is for now, mostly due to the fact that
the table helper automatically aligns table columns - something that
is not easily possible with native Handlebars syntax.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 PVE/VZDump.pm                            | 21 +++++++++------------
 templates/default/vzdump-body.html.hbs   | 24 ++++++++++++++++++++++--
 templates/default/vzdump-body.txt.hbs    |  2 +-
 templates/default/vzdump-subject.txt.hbs |  2 +-
 4 files changed, 33 insertions(+), 16 deletions(-)

diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index fd89945e..e0fbc555 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -528,16 +528,13 @@ sub send_notification {
 	    "See Task History for details!\n";
     };
 
-    my $notification_props = {
-	# Hostname, might include domain part
-	"hostname" => get_hostname(),
-	"error-message" => $err,
-	"guest-table" => build_guest_table($tasklist),
-	"logs" => $text_log_part,
-	"status-text" => $status_text,
-	"total-time" => $total_time,
-	"total-size" => $total_size,
-    };
+    my $template_data = PVE::Notify::common_template_data();
+    $template_data->{error} = $err;
+    $template_data->{"guest-table"} = build_guest_table($tasklist);
+    $template_data->{logs} = $text_log_part;
+    $template_data->{"status-text"} = $status_text;
+    $template_data->{"total-size"} = $total_size;
+    $template_data->{"total-time"} = $total_time;
 
     my $fields = {
 	type => "vzdump",
@@ -586,7 +583,7 @@ sub send_notification {
 	    PVE::Notify::notify(
 		$severity,
 		"vzdump",
-		$notification_props,
+		$template_data,
 		$fields,
 		$notification_config
 	    );
@@ -597,7 +594,7 @@ sub send_notification {
 	PVE::Notify::notify(
 	    $severity,
 	    "vzdump",
-	    $notification_props,
+	    $template_data,
 	    $fields,
 	);
     }
diff --git a/templates/default/vzdump-body.html.hbs b/templates/default/vzdump-body.html.hbs
index b6730cbb..7f2245d7 100644
--- a/templates/default/vzdump-body.html.hbs
+++ b/templates/default/vzdump-body.html.hbs
@@ -1,8 +1,28 @@
 <html>
     <body>
-        {{error-message}}
+        {{error}}
         <h1 style="font-size: 1.2em">Details</h1>
-        {{table guest-table}}
+        <table style="border: 1px solid;border-collapse=collapse;">
+            <tr>
+                <th style="border: 1px solid;border-collapse=collapse;">VMID</th>
+                <th style="border: 1px solid;border-collapse=collapse;">Name</th>
+                <th style="border: 1px solid;border-collapse=collapse;">Status</th>
+                <th style="border: 1px solid;border-collapse=collapse;">Time</th>
+                <th style="border: 1px solid;border-collapse=collapse;">Size</th>
+                <th style="border: 1px solid;border-collapse=collapse;">Filename</th>
+            </tr>
+        {{#each guest-table.data}}
+            <tr>
+                <td style="border: 1px solid;border-collapse=collapse;">{{this.vmid}}</th>
+                <td style="border: 1px solid;border-collapse=collapse;">{{this.name}}</th>
+                <td style="border: 1px solid;border-collapse=collapse;">{{this.status}}</th>
+                <td style="border: 1px solid;border-collapse=collapse;">{{duration this.time}}</th>
+                <td style="border: 1px solid;border-collapse=collapse;">{{human-bytes this.size}}</th>
+                <td style="border: 1px solid;border-collapse=collapse;">{{this.filename}}</th>
+            </tr>
+        {{/each}}
+        </table>
+        <br/>
         Total running time: {{duration total-time}}<br/>
         Total size: {{human-bytes total-size}}<br/>
         <h1 style="font-size: 1.2em">Logs</h1>
diff --git a/templates/default/vzdump-body.txt.hbs b/templates/default/vzdump-body.txt.hbs
index 90f5b0a4..4bd0fab8 100644
--- a/templates/default/vzdump-body.txt.hbs
+++ b/templates/default/vzdump-body.txt.hbs
@@ -1,4 +1,4 @@
-{{error-message}}
+{{error}}
 Details
 =======
 {{table guest-table}}
diff --git a/templates/default/vzdump-subject.txt.hbs b/templates/default/vzdump-subject.txt.hbs
index 98a3d9aa..107b5fc7 100644
--- a/templates/default/vzdump-subject.txt.hbs
+++ b/templates/default/vzdump-subject.txt.hbs
@@ -1 +1 @@
-vzdump backup status ({{hostname}}): {{status-text}}
+vzdump backup status ({{fqdn}}): {{status-text}}
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH pve-manager 2/4] notifications: apt: clean up notification template

[Lukas Wagner]

Clean up the notification templates to prepare for user-customizable
templates
 - Change some of the template variable names to improve clarity
 - Generate the table for available updates in the template itself,
   not via the 'table' helper. This makes it possible for users
   to change the style/structure of the table.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 PVE/API2/APT.pm                               | 27 ++++++++-----------
 .../default/package-updates-body.html.hbs     | 15 ++++++++++-
 .../default/package-updates-body.txt.hbs      |  2 +-
 .../default/package-updates-subject.txt.hbs   |  2 +-
 4 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/PVE/API2/APT.pm b/PVE/API2/APT.pm
index 47c50961..99c2e152 100644
--- a/PVE/API2/APT.pm
+++ b/PVE/API2/APT.pm
@@ -306,43 +306,38 @@ __PACKAGE__->register_method({
 		    schema => {
 			columns => [
 			    {
-				label => "Package",
-				id    => "package",
+				label => "Package Name",
+				id    => "package-name",
 			    },
 			    {
-				label => "Old Version",
-				id    => "old-version",
+				label => "Installed Version",
+				id    => "installed-version",
 			    },
 			    {
-				label => "New Version",
-				id    => "new-version",
+				label => "Available Version",
+				id    => "available-version",
 			    }
 			]
 		    },
 		    data => []
 		};
 
-		my $hostname = `hostname -f` || PVE::INotify::nodename();
-		chomp $hostname;
-
 		my $count = 0;
 		foreach my $p (sort {$a->{Package} cmp $b->{Package} } @$pkglist) {
 		    next if $p->{NotifyStatus} && $p->{NotifyStatus} eq $p->{Version};
 		    $count++;
 
 		    push @{$updates_table->{data}}, {
-			"package"     => $p->{Package},
-			"old-version" => $p->{OldVersion},
-			"new-version" => $p->{Version}
+			"package-name" => $p->{Package},
+			"installed-version" => $p->{OldVersion},
+			"available-version" => $p->{Version}
 		    };
 		}
 
 		return if !$count;
 
-		my $template_data = {
-		    updates  => $updates_table,
-		    hostname => $hostname,
-		};
+		my $template_data = PVE::Notify::common_template_data();
+		$template_data->{"available-updates"} = $updates_table;
 
 		# Additional metadata fields that can be used in notification
 		# matchers.
diff --git a/templates/default/package-updates-body.html.hbs b/templates/default/package-updates-body.html.hbs
index d058e114..ad29a941 100644
--- a/templates/default/package-updates-body.html.hbs
+++ b/templates/default/package-updates-body.html.hbs
@@ -1,6 +1,19 @@
 <html>
     <body>
         The following updates are available:
-        {{table updates}}
+        <table style="border: 1px solid;border-collapse=collapse;margin-top: 5px;">
+            <tr>
+                <th style="border: 1px solid;border-collapse=collapse;">Package Name</th>
+                <th style="border: 1px solid;border-collapse=collapse;">Installed Version</th>
+                <th style="border: 1px solid;border-collapse=collapse;">Available Version</th>
+            </tr>
+        {{#each available-updates.data}}
+            <tr>
+                <td style="border: 1px solid;border-collapse=collapse;">{{this.package-name}}</th>
+                <td style="border: 1px solid;border-collapse=collapse;">{{this.installed-version}}</th>
+                <td style="border: 1px solid;border-collapse=collapse;">{{this.available-version}}</th>
+            </tr>
+        {{/each}}
+        </table>
     </body>
 </html>
diff --git a/templates/default/package-updates-body.txt.hbs b/templates/default/package-updates-body.txt.hbs
index 14bdbf9e..74969433 100644
--- a/templates/default/package-updates-body.txt.hbs
+++ b/templates/default/package-updates-body.txt.hbs
@@ -1,3 +1,3 @@
 The following updates are available:
 
-{{table updates}}
+{{table available-updates}}
diff --git a/templates/default/package-updates-subject.txt.hbs b/templates/default/package-updates-subject.txt.hbs
index 556a67b8..b73fdefd 100644
--- a/templates/default/package-updates-subject.txt.hbs
+++ b/templates/default/package-updates-subject.txt.hbs
@@ -1 +1 @@
-New software packages available ({{hostname}})
+New software packages available ({{fqdn}})
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH pve-manager 3/4] notification: replication: add common properties to template data

[Lukas Wagner]

The new PVE::Notify::common_template_data helper gives us a hash of
properties which should be available in all notifications (hostname,
fqdn, cluster-name at this moment). This commit makes sure that
replication notifications have these available.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 PVE/API2/Replication.pm                    | 16 +++++++---------
 templates/default/replication-body.txt.hbs |  2 +-
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/PVE/API2/Replication.pm b/PVE/API2/Replication.pm
index e4a7180f..3a2be73a 100644
--- a/PVE/API2/Replication.pm
+++ b/PVE/API2/Replication.pm
@@ -112,15 +112,13 @@ my sub _handle_job_err {
     # The replication job is run every 15 mins if no schedule is set.
     my $schedule = $job->{schedule} // '*/15';
 
-    my $template_data = {
-	"failure-count" => $fail_count,
-	"last-sync"     => $jobstate->{last_sync},
-	"next-sync"     => $next_sync,
-	"job-id"        => $job->{id},
-	"job-target"    => $job->{target},
-	"job-schedule"  => $schedule,
-	"error"         => $err,
-    };
+    my $template_data = PVE::Notify::common_template_data();
+    $template_data->{"failure-count"} = $fail_count;
+    $template_data->{"last-sync"} = $jobstate->{last_sync};
+    $template_data->{"job-id"} = $job->{id};
+    $template_data->{"job-target"} = $job->{target};
+    $template_data->{"job-schedule"} = $schedule;
+    $template_data->{"error"} = $err;
 
     my $metadata_fields = {
 	type => "replication",
diff --git a/templates/default/replication-body.txt.hbs b/templates/default/replication-body.txt.hbs
index a9273fef..b1894ce9 100644
--- a/templates/default/replication-body.txt.hbs
+++ b/templates/default/replication-body.txt.hbs
@@ -9,4 +9,4 @@ Note: The system  will now reduce the frequency of error reports, as the job
 appears to be stuck.
 {{/if}}
 Error:
-{{ error }}
+{{error}}
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH pve-manager 4/4] notifications: test: style fixup

[Lukas Wagner]

Other commits introduced a consistent style for handlebars expressions,
this commit applies the same style to test notification templates.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 templates/default/test-body.html.hbs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/templates/default/test-body.html.hbs b/templates/default/test-body.html.hbs
index 26a43dde..24454433 100644
--- a/templates/default/test-body.html.hbs
+++ b/templates/default/test-body.html.hbs
@@ -1 +1 @@
-This is a test of the notification target '{{ target }}'.
+This is a test of the notification target '{{target}}'.
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH pve-ha-manager 1/1] notifications: overhaul fence notification

[Lukas Wagner]

- try to make template variable names more clear (in preparation
  for #6143)
- add common tempate variables (fqdn, hostname, cluster-name)
- Instead of dumping the status-data variable as a JSON blob we
  add template variables for the most useful information and
  render it in a structured manner

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 src/PVE/HA/NodeStatus.pm                      | 43 +++++++++++++++----
 src/PVE/HA/Sim/Env.pm                         |  8 ++--
 src/templates/default/fencing-body.html.hbs   | 43 ++++++++++++++++---
 src/templates/default/fencing-body.txt.hbs    | 40 +++++++++++++----
 src/templates/default/fencing-subject.txt.hbs |  6 ++-
 5 files changed, 111 insertions(+), 29 deletions(-)

diff --git a/src/PVE/HA/NodeStatus.pm b/src/PVE/HA/NodeStatus.pm
index 9e6d898..3a42746 100644
--- a/src/PVE/HA/NodeStatus.pm
+++ b/src/PVE/HA/NodeStatus.pm
@@ -3,6 +3,8 @@ package PVE::HA::NodeStatus;
 use strict;
 use warnings;
 
+use PVE::Notify;
+
 use JSON;
 
 my $fence_delay = 60;
@@ -195,15 +197,38 @@ my $send_fence_state_email = sub {
     my $haenv = $self->{haenv};
     my $status = $haenv->read_manager_status();
 
-    my $template_data = {
-	"status-data"    => {
-	    manager_status => $status,
-	    node_status    => $self->{status}
-	},
-	"node"           => $node,
-	"subject-prefix" => $subject_prefix,
-	"subject"        => $subject,
-    };
+    my $template_data = PVE::Notify::common_template_data();
+    # Those two are needed for the expected output for test cases,
+    # see src/PVE/HA/Sim/Env.pm
+    $template_data->{"fence-status"} = $subject;
+    $template_data->{"fence-prefix"} = $subject_prefix;
+
+    $template_data->{"is-success"} = 1 ? $subject_prefix eq "SUCCEED" : 0;
+
+    $template_data->{"failed-node"} = $node;
+    $template_data->{"master-node"} = $status->{master_node};
+    # There is a handlebars helper 'timestamp', we should not
+    # name a variable the same way.
+    $template_data->{"fence-timestamp"} = $status->{timestamp};
+
+    $template_data->{"nodes"} = [];
+    for my $key (sort keys $status->{node_status}->%*) {
+	push $template_data->{"nodes"}->@*, {
+	    node => $key,
+	    status => $status->{node_status}->{$key}
+	};
+    }
+
+    $template_data->{"resources"} = [];
+    for my $key (sort keys $status->{service_status}->%*) {
+	my $resource_status = $status->{service_status}->{$key};
+	push $template_data->{"resources"}->@*, {
+	    resource => $key,
+	    state => $resource_status->{state},
+	    node => $resource_status->{node},
+	    running => $resource_status->{running},
+	};
+    }
 
     my $metadata_fields = {
 	type => 'fencing',
diff --git a/src/PVE/HA/Sim/Env.pm b/src/PVE/HA/Sim/Env.pm
index b2ab231..f250d43 100644
--- a/src/PVE/HA/Sim/Env.pm
+++ b/src/PVE/HA/Sim/Env.pm
@@ -299,12 +299,12 @@ sub log {
 sub send_notification {
     my ($self, $template_name, $properties) = @_;
 
-    # The template for the subject is "{{subject-prefix}}: {{subject}}"
+    # The template for the subject is "{{fence-status}}: {{fence-message}}"
     # We have to perform poor-man's template rendering to pass the test cases.
 
-    my $subject = "{{subject-prefix}}: {{subject}}";
-    $subject = $subject =~ s/\{\{subject-prefix}}/$properties->{"subject-prefix"}/r;
-    $subject = $subject =~ s/\{\{subject}}/$properties->{"subject"}/r;
+    my $subject = "{{fence-prefix}}: {{fence-status}}";
+    $subject = $subject =~ s/\{\{fence-prefix}}/$properties->{"fence-prefix"}/r;
+    $subject = $subject =~ s/\{\{fence-status}}/$properties->{"fence-status"}/r;
 
     # only log subject, do not spam the logs
     $self->log('email', $subject);
diff --git a/src/templates/default/fencing-body.html.hbs b/src/templates/default/fencing-body.html.hbs
index 1420348..901a0e1 100644
--- a/src/templates/default/fencing-body.html.hbs
+++ b/src/templates/default/fencing-body.html.hbs
@@ -1,14 +1,43 @@
 <html>
     <body>
-    The node '{{node}}' failed and needs manual intervention.<br/><br/>
+        The node '{{failed-node}}' in cluster '{{cluster-name}}' failed and
+        needs manual intervention.<br/><br/>
 
-    The PVE HA manager tries to fence it and recover the configured HA resources to
-    a healthy node if possible.<br/><br/>
+        {{#if is-success~}}
+        The PVE HA manager successfully fenced '{{failed-node}}'.<br/><br/>
+        {{else}}
+        The PVE HA manager will now fence '{{failed-node}}'.<br/><br/>
+        {{/if}}
 
-    Current fence status: {{subject-prefix}}<br/>
-    {{subject}}<br/>
+        <b>Status:</b> {{fence-status}}<br/>
+        <b>Timestamp:</b> {{timestamp fence-timestamp}}<br/>
 
-    <h2 style="font-size: 1em">Overall Cluster status:</h2>
-    {{object status-data}}
+        <h2 style="font-size: 1em">Cluster Node Status:</h2>
+        <ul>
+        {{#each nodes}}
+            <li>
+                {{this.node}}: {{this.status}} {{#if (eq this.node ../master-node)}}[master]{{/if}}
+            </li>
+        {{/each}}
+        </ul>
+
+        <h2 style="font-size: 1em">HA Resources:</h2>
+        The following HA resources were running on the failed node and will be
+        recovered to a healthy node if possible:
+        <ul>
+        {{#each resources}}
+            {{#if (eq this.node ../failed-node)}}
+            <li>{{this.resource}} [{{this.node}}]: {{this.state}}</li>
+            {{/if}}
+        {{/each}}
+        </ul>
+        The other HA resources in this cluster are:
+        <ul>
+        {{#each resources}}
+            {{#if (ne this.node ../failed-node)}}
+            <li>{{this.resource}} [{{this.node}}]: {{this.state}}</li>
+            {{/if}}
+        {{/each}}
+        </ul>
     </body>
 </html>
diff --git a/src/templates/default/fencing-body.txt.hbs b/src/templates/default/fencing-body.txt.hbs
index e46a1fd..6f54122 100644
--- a/src/templates/default/fencing-body.txt.hbs
+++ b/src/templates/default/fencing-body.txt.hbs
@@ -1,11 +1,35 @@
-The node '{{node}}' failed and needs manual intervention.
+The node '{{failed-node}}' in cluster '{{cluster-name}}' failed
+and needs manual intervention.
 
-The PVE HA manager tries to fence it and recover the configured HA resources to
-a healthy node if possible.
+{{#if is-success~}}
+The PVE HA manager successfully fenced '{{failed-node}}'.
+{{else~}}
+The PVE HA manager will now fence '{{failed-node}}'.
+{{/if}}
+Status:    {{fence-status}}
+Timestamp: {{timestamp fence-timestamp}}
 
-Current fence status: {{subject-prefix}}
-{{subject}}
+Cluster Node Status:
+--------------------
+{{#each nodes~}}
+  - {{this.node}}: {{this.status}} {{#if (eq this.node ../master-node)}}[master]{{/if}}
+{{/each}}
+
+HA Resources:
+-------------
+The following HA resources were running on the failed node and will be
+recovered to a healthy node if possible:
+
+{{#each resources~}}
+{{#if (eq this.node ../failed-node)~}}
+  - {{this.resource}} [{{this.node}}]: {{this.state}}
+{{/if~}}
+{{/each}}
+The other HA resources in this cluster are:
+
+{{#each resources~}}
+{{#if (ne this.node ../failed-node)~}}
+  - {{this.resource}} [{{this.node}}]: {{this.state}}
+{{/if~}}
+{{/each~}}
 
-Overall Cluster status:
------------------------
-{{object status-data}}
diff --git a/src/templates/default/fencing-subject.txt.hbs b/src/templates/default/fencing-subject.txt.hbs
index 43651f9..1c140e3 100644
--- a/src/templates/default/fencing-subject.txt.hbs
+++ b/src/templates/default/fencing-subject.txt.hbs
@@ -1 +1,5 @@
-{{subject-prefix}}: {{subject}}
+{{#if is-success~}}
+Successfully fenced node '{{failed-node}}'
+{{else}}
+Trying to fence node '{{failed-node}}'
+{{/if}}
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH pve-cluster 1/1] notify: add common_template_data

[Thomas Lamprecht]

Am 27.03.25 um 15:23 schrieb Lukas Wagner:
> This commit add the `common_template_data` sub to PVE::Notify,
> providing a convenient way to get a hash with properties that
> should be accessible from all templates, namely hostname, fqdn
> and cluster-name.
> 
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
>  src/PVE/Notify.pm | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/src/PVE/Notify.pm b/src/PVE/Notify.pm
> index c514111..9f7a74c 100644
> --- a/src/PVE/Notify.pm
> +++ b/src/PVE/Notify.pm
> @@ -4,6 +4,7 @@ use strict;
>  use warnings;
>  
>  use PVE::Cluster qw(cfs_register_file cfs_read_file cfs_lock_file cfs_write_file);
> +use PVE::INotify;
>  use Proxmox::RS::Notify;
>  
>  cfs_register_file(
> @@ -130,4 +131,23 @@ sub check_may_use_target {
>      }
>  }
>  
> +sub common_template_data {
> +    my $hostname = PVE::INotify::nodename();
> +    my $fqdn = `hostname -f` || $hostname;
> +    chomp $fqdn;

A bit odd seeing the highly efficient hostname cached from inotify used
with getting the FQDN in a rather expensive (fork+exec) way.
Not so keen on the use of backticks, albeit it would not be a huge
problem.

I'd favor using PVE::Tools::get_fqdn which should work as well as calling
`hostname -f` but also be quite a bit cheaper due to avoiding fork+exec.
It'd be probably even fine to cache in a module-local variable, as this
is not something that changes often, but also probably not a must for now.

> +
> +    my $data = {
> +        hostname => $hostname,
> +        fqdn => $fqdn,
> +    };
> +
> +    my $cluster_info = PVE::Cluster::get_clinfo();
> +
> +    if (my $d = $cluster_info->{cluster}) {
> +        $data->{"cluster-name"} = $d->{name};
> +    }
> +
> +    return $data;
> +}
> +
>  1;



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH pve-cluster 1/1] notify: add common_template_data

[Lukas Wagner]

On  2025-03-27 16:31, Thomas Lamprecht wrote:
> Am 27.03.25 um 15:23 schrieb Lukas Wagner:
>> This commit add the `common_template_data` sub to PVE::Notify,
>> providing a convenient way to get a hash with properties that
>> should be accessible from all templates, namely hostname, fqdn
>> and cluster-name.
>>
>> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
>> ---
>>  src/PVE/Notify.pm | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/src/PVE/Notify.pm b/src/PVE/Notify.pm
>> index c514111..9f7a74c 100644
>> --- a/src/PVE/Notify.pm
>> +++ b/src/PVE/Notify.pm
>> @@ -4,6 +4,7 @@ use strict;
>>  use warnings;
>>  
>>  use PVE::Cluster qw(cfs_register_file cfs_read_file cfs_lock_file cfs_write_file);
>> +use PVE::INotify;
>>  use Proxmox::RS::Notify;
>>  
>>  cfs_register_file(
>> @@ -130,4 +131,23 @@ sub check_may_use_target {
>>      }
>>  }
>>  
>> +sub common_template_data {
>> +    my $hostname = PVE::INotify::nodename();
>> +    my $fqdn = `hostname -f` || $hostname;
>> +    chomp $fqdn;
> 
> A bit odd seeing the highly efficient hostname cached from inotify used
> with getting the FQDN in a rather expensive (fork+exec) way.
> Not so keen on the use of backticks, albeit it would not be a huge
> problem.
> 
> I'd favor using PVE::Tools::get_fqdn which should work as well as calling
> `hostname -f` but also be quite a bit cheaper due to avoiding fork+exec.
> It'd be probably even fine to cache in a module-local variable, as this
> is not something that changes often, but also probably not a must for now.
> 

Yeah, fair. The `hostname -f` was pre-existing in the place where we send APT update notifications,
I just moved it from there. I didn't know that there was a helper for it, good to know!
I'll check it out and send a v2.

We of course can cache the FQDN, but realistically speaking, this is only called once per
notification being sent, thus any real-world performance impact is absolutely tiny.

Thanks for the feedback!

-- 
- Lukas



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH pve-cluster 1/1] notify: add common_template_data

[Thomas Lamprecht]

Am 28.03.25 um 09:28 schrieb Lukas Wagner:
> We of course can cache the FQDN, but realistically speaking, this is only called once per
> notification being sent, thus any real-world performance impact is absolutely tiny.

Not so sure about that in general, e.g. sending out notifications could
correlate with an overloaded system, and for really overloaded systems
things that are normally cheap suddenly ain't – e.g., on low memory
situations even a doing a plain fork+exec of a tiny binary can hang for
a long time then, socket operations like our helper does are definitively
less problematic (I think, as I did not evaluate it [0] and that's something
one can less easily experience directly compared to the former, where even
starting a new basic dash shell on such overloaded system can need minutes).

And as the notification system now also handles things like HA events it's
definitively part of the more critical systems which _can_ justify some extra
scrutiny. That said, switching to the get_fqdn method makes this indeed quite
cheap to get [0], so I'm fine with not doing any caching here for now, but
let's not underestimate the impact of such things too much, especially for
anything in critical chains that can be important in critical (load) situation
(as general strategy for all, as I'm really not thinking about you here, and
it certainly is a balance).

[0]: FWIW, I just did a quick evaluation of querying the fqdn 100 000 times
with the socket variant and the hostname one, this was done on a very healthy
system though, I'd expect that the fork+exec one degrades a lot worse with
higher cpu/memory pressure. Test and result:

# perl -wE 'use PVE::Tools; use Time::HiRes qw(gettimeofday tv_interval); my $t0 = [gettimeofday]; for(my $i = 0; $i <= 100_000; $i++) { my $fqdn = PVE::Tools::get_fqdn("nina"); } my $elapsed = tv_interval ( $t0, [gettimeofday]); say "elapsed (s): ". $elapsed;' 
elapsed (s): 0.436712

Same with 1 million runs gets me 4.368217 s, so seems to scale quite linearly.

# perl -wE 'use Time::HiRes qw(gettimeofday tv_interval); my $t0 = [gettimeofday]; for(my $i = 0; $i <= 100_000; $i++) { my $fqdn = `hostname -f`; } my $elapsed = tv_interval ( $t0, [gettimeofday]); say "elapsed: ". $elapsed;'      
elapsed (s): 82.484177

Same with 1 million runs gets me 577.653117 s, so not fully linearly, but
in any way about 188x and 132x times slower, respectively.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH pve-cluster 1/1] notify: add common_template_data

[Lukas Wagner]

On  2025-03-28 10:38, Thomas Lamprecht wrote:
> Am 28.03.25 um 09:28 schrieb Lukas Wagner:
>> We of course can cache the FQDN, but realistically speaking, this is only called once per
>> notification being sent, thus any real-world performance impact is absolutely tiny.
> 
> Not so sure about that in general, e.g. sending out notifications could
> correlate with an overloaded system, and for really overloaded systems
> things that are normally cheap suddenly ain't – e.g., on low memory
> situations even a doing a plain fork+exec of a tiny binary can hang for
> a long time then, socket operations like our helper does are definitively
> less problematic (I think, as I did not evaluate it [0] and that's something
> one can less easily experience directly compared to the former, where even
> starting a new basic dash shell on such overloaded system can need minutes).

Yes, my 'impact is tiny' was mostly based on already using PVE::Tools::get_fqdn. For the
old fork+exec version you are absolutely right, this could take very long on a
system at its limits.

> 
> And as the notification system now also handles things like HA events it's
> definitively part of the more critical systems which _can_ justify some extra
> scrutiny. That said, switching to the get_fqdn method makes this indeed quite
> cheap to get [0], so I'm fine with not doing any caching here for now, but
> let's not underestimate the impact of such things too much, especially for
> anything in critical chains that can be important in critical (load) situation
> (as general strategy for all, as I'm really not thinking about you here, and
> it certainly is a balance).

That makes a lot of sense. Thanks for the detailed explanation, highly appreciated.
Actually I will add caching right away, it a pretty trivial change anyways.

> 
> [0]: FWIW, I just did a quick evaluation of querying the fqdn 100 000 times
> with the socket variant and the hostname one, this was done on a very healthy
> system though, I'd expect that the fork+exec one degrades a lot worse with
> higher cpu/memory pressure. Test and result:
> 
> # perl -wE 'use PVE::Tools; use Time::HiRes qw(gettimeofday tv_interval); my $t0 = [gettimeofday]; for(my $i = 0; $i <= 100_000; $i++) { my $fqdn = PVE::Tools::get_fqdn("nina"); } my $elapsed = tv_interval ( $t0, [gettimeofday]); say "elapsed (s): ". $elapsed;' 
> elapsed (s): 0.436712
> 
> Same with 1 million runs gets me 4.368217 s, so seems to scale quite linearly.
> 
> # perl -wE 'use Time::HiRes qw(gettimeofday tv_interval); my $t0 = [gettimeofday]; for(my $i = 0; $i <= 100_000; $i++) { my $fqdn = `hostname -f`; } my $elapsed = tv_interval ( $t0, [gettimeofday]); say "elapsed: ". $elapsed;'      
> elapsed (s): 82.484177
> 
> Same with 1 million runs gets me 577.653117 s, so not fully linearly, but
> in any way about 188x and 132x times slower, respectively.

Good to know, thanks for backing that up with some concrete data! :)

-- 
- Lukas



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] superseded: [PATCH cluster/ha-manager/manager 0/6] preparation for #6143: notification template cleanup

[Lukas Wagner]

v2 available!

Superseded-by: https://lore.proxmox.com/pve-devel/20250328101915.73951-1-l.wagner@proxmox.com/T/#t

-- 
- Lukas



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel