From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pbs-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id E0C121FF189
for <inbox@lore.proxmox.com>; Fri, 21 Mar 2025 14:28:30 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id A4C051E531;
Fri, 21 Mar 2025 14:28:29 +0100 (CET)
Date: Fri, 21 Mar 2025 14:28:24 +0100
From: Wolfgang Bumiller <w.bumiller@proxmox.com>
To: Lukas Wagner <l.wagner@proxmox.com>
Message-ID: <mypjw222ja54377hq5mewvloov4j4bfll34c73cl6h6ednejzv@pirmd3mjqtfp>
References: <20250321122521.198725-1-l.wagner@proxmox.com>
<20250321122521.198725-3-l.wagner@proxmox.com>
MIME-Version: 1.0
Content-Disposition: inline
In-Reply-To: <20250321122521.198725-3-l.wagner@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.079 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
KAM_SHORT 0.001 Use of a URL Shortener for very short URL
RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See
http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more
information. [docs.rs, mod.rs]
Subject: Re: [pbs-devel] [PATCH proxmox-backup 02/10] notifications: add
type for GC notification template data
X-BeenThere: pbs-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Backup Server development discussion
<pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox Backup Server development discussion
<pbs-devel@lists.proxmox.com>
Cc: pbs-devel@lists.proxmox.com
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pbs-devel-bounces@lists.proxmox.com
Sender: "pbs-devel" <pbs-devel-bounces@lists.proxmox.com>
On Fri, Mar 21, 2025 at 01:25:13PM +0100, Lukas Wagner wrote:
> This commit adds a separate type for the data passed to this type of
> notification template. Also we make sure that we do not expose any
> non-primitive types to the template renderer, any data
> needed in the template is mapped into the new dedicated
> template data type.
> This ensures that any changes in types defined in other places
> do not leak into the template rendering process by accident.
>
> This commit also tries to unify the style and naming of template
> variables.
>
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
> src/server/notifications/mod.rs | 50 ++++----
> src/server/notifications/template_data.rs | 132 ++++++++++++++++++++++
> templates/default/gc-err-body.txt.hbs | 2 +-
> templates/default/gc-err-subject.txt.hbs | 2 +-
> templates/default/gc-ok-body.txt.hbs | 22 ++--
> templates/default/gc-ok-subject.txt.hbs | 2 +-
> 6 files changed, 170 insertions(+), 40 deletions(-)
> create mode 100644 src/server/notifications/template_data.rs
>
> diff --git a/src/server/notifications/mod.rs b/src/server/notifications/mod.rs
> index eea55202..182af213 100644
> --- a/src/server/notifications/mod.rs
> +++ b/src/server/notifications/mod.rs
> @@ -21,6 +21,10 @@ use proxmox_notify::{Endpoint, Notification, Severity};
>
> const SPOOL_DIR: &str = concatcp!(pbs_buildcfg::PROXMOX_BACKUP_STATE_DIR, "/notifications");
>
> +mod template_data;
> +
> +use template_data::{GcErrTemplateData, GcOkTemplateData};
> +
> /// Initialize the notification system by setting context in proxmox_notify
> pub fn init() -> Result<(), Error> {
> proxmox_notify::context::set_context(&PBS_CONTEXT);
> @@ -146,38 +150,32 @@ pub fn send_gc_status(
> status: &GarbageCollectionStatus,
> result: &Result<(), Error>,
> ) -> Result<(), Error> {
> - let (fqdn, port) = get_server_url();
> - let mut data = json!({
> - "datastore": datastore,
> - "fqdn": fqdn,
> - "port": port,
> - });
> -
> - let (severity, template) = match result {
> - Ok(()) => {
> - let deduplication_factor = if status.disk_bytes > 0 {
> - (status.index_data_bytes as f64) / (status.disk_bytes as f64)
> - } else {
> - 1.0
> - };
> -
> - data["status"] = json!(status);
> - data["deduplication-factor"] = format!("{:.2}", deduplication_factor).into();
> -
> - (Severity::Info, "gc-ok")
> - }
> - Err(err) => {
> - data["error"] = err.to_string().into();
> - (Severity::Error, "gc-err")
> - }
> - };
> let metadata = HashMap::from([
> ("datastore".into(), datastore.into()),
> ("hostname".into(), proxmox_sys::nodename().into()),
> ("type".into(), "gc".into()),
> ]);
>
> - let notification = Notification::from_template(severity, template, data, metadata);
> + let notification = match result {
> + Ok(()) => {
> + let template_data = GcOkTemplateData::new(datastore.to_string(), status);
> + Notification::from_template(
> + Severity::Info,
> + "gc-ok",
> + serde_json::to_value(template_data)?,
> + metadata,
> + )
> + }
> + Err(err) => {
> + let template_data = GcErrTemplateData::new(datastore.to_string(), err.to_string());
^ While at it, we could consider switching this error to use
`format!{"{err:#}")` which will include the context as a short form.
This goes for all the templates/commits - but can just be a follow up.
Noticing this since Chris got me looking at anyhow's display
representations[1]
[1] https://docs.rs/anyhow/latest/anyhow/struct.Error.html#display-representations
> + Notification::from_template(
> + Severity::Error,
> + "gc-err",
> + serde_json::to_value(template_data)?,
> + metadata,
> + )
> + }
> + };
>
> let (email, notify, mode) = lookup_datastore_notify_settings(datastore);
> match mode {
> diff --git a/src/server/notifications/template_data.rs b/src/server/notifications/template_data.rs
> new file mode 100644
> index 00000000..2d87b435
> --- /dev/null
> +++ b/src/server/notifications/template_data.rs
> @@ -0,0 +1,132 @@
> +use pbs_api_types::GarbageCollectionStatus;
> +use serde::Serialize;
> +
> +// NOTE: For some of these types, the `XyzOkTemplateData` and `XyzErrTemplateData`
> +// types are almost identical except for the `error` member.
> +// While at first glance I might make sense
> +// to consolidate the two and make `error` an `Option`, I would argue
> +// that it is actually quite nice to have a single, distinct type for
> +// each template. This makes it 100% clear which params are accessible
> +// for every single template, at the cost of some boilerplate code.
> +
> +/// Template data which should be available in *all* notifications.
> +/// The fields of this struct will be flattened into the individual
> +/// *TemplateData structs.
> +#[derive(Serialize)]
> +#[serde(rename_all = "kebab-case")]
> +pub struct CommonData {
> + /// The hostname of the PBS host.
> + pub hostname: String,
> + /// The base URL for building links to the web interface.
> + pub base_url: String,
> +}
> +
> +impl CommonData {
> + pub fn new() -> CommonData {
> + let nodename = proxmox_sys::nodename();
> + let mut fqdn = nodename.to_owned();
> +
> + if let Ok(resolv_conf) = crate::api2::node::dns::read_etc_resolv_conf() {
> + if let Some(search) = resolv_conf["search"].as_str() {
> + fqdn.push('.');
> + fqdn.push_str(search);
> + }
> + }
> +
> + // TODO: Some users might want to be able to override this.
> + let base_url = format!("https://{fqdn}:8007");
> +
> + CommonData {
> + hostname: nodename.into(),
> + base_url,
> + }
> + }
> +}
> +
> +/// Template data for the gc-ok template.
> +#[derive(Serialize)]
> +#[serde(rename_all = "kebab-case")]
> +pub struct GcOkTemplateData {
> + /// Common properties.
> + #[serde(flatten)]
> + pub common: CommonData,
> + /// The datastore.
> + pub datastore: String,
> + /// The task's UPID.
> + pub upid: Option<String>,
> + /// Number of processed index files.
> + pub index_file_count: usize,
> + /// Sum of bytes referred by index files.
> + pub index_data_bytes: u64,
> + /// Bytes used on disk.
> + pub disk_bytes: u64,
> + /// Chunks used on disk.
> + pub disk_chunks: usize,
> + /// Sum of removed bytes.
> + pub removed_bytes: u64,
> + /// Number of removed chunks.
> + pub removed_chunks: usize,
> + /// Sum of pending bytes (pending removal - kept for safety).
> + pub pending_bytes: u64,
> + /// Number of pending chunks (pending removal - kept for safety).
> + pub pending_chunks: usize,
> + /// Number of chunks marked as .bad by verify that have been removed by GC.
> + pub removed_bad: usize,
> + /// Number of chunks still marked as .bad after garbage collection.
> + pub still_bad: usize,
> + /// Factor of deduplication.
> + pub deduplication_factor: String,
> +}
> +
> +impl GcOkTemplateData {
> + /// Create new a new instance.
> + pub fn new(datastore: String, status: &GarbageCollectionStatus) -> Self {
> + let deduplication_factor = if status.disk_bytes > 0 {
> + (status.index_data_bytes as f64) / (status.disk_bytes as f64)
> + } else {
> + 1.0
> + };
> + let deduplication_factor = format!("{:.2}", deduplication_factor);
> +
> + Self {
> + common: CommonData::new(),
> + datastore,
> + upid: status.upid.clone(),
> + index_file_count: status.index_file_count,
> + index_data_bytes: status.index_data_bytes,
> + disk_bytes: status.disk_bytes,
> + disk_chunks: status.disk_chunks,
> + removed_bytes: status.removed_bytes,
> + removed_chunks: status.removed_chunks,
> + pending_bytes: status.pending_bytes,
> + pending_chunks: status.pending_chunks,
> + removed_bad: status.removed_bad,
> + still_bad: status.still_bad,
> + deduplication_factor,
> + }
> + }
> +}
> +
> +/// Template data for the gc-err template.
> +#[derive(Serialize)]
> +#[serde(rename_all = "kebab-case")]
> +pub struct GcErrTemplateData {
> + /// Common properties.
> + #[serde(flatten)]
> + pub common: CommonData,
> + /// The datastore.
> + pub datastore: String,
> + /// The error that occured during the GC job.
> + pub error: String,
> +}
> +
> +impl GcErrTemplateData {
> + /// Create new a new instance.
> + pub fn new(datastore: String, error: String) -> Self {
> + Self {
> + common: CommonData::new(),
> + datastore,
> + error,
> + }
> + }
> +}
> diff --git a/templates/default/gc-err-body.txt.hbs b/templates/default/gc-err-body.txt.hbs
> index d6c2d0bc..107f9e2e 100644
> --- a/templates/default/gc-err-body.txt.hbs
> +++ b/templates/default/gc-err-body.txt.hbs
> @@ -5,4 +5,4 @@ Garbage collection failed: {{error}}
>
> Please visit the web interface for further details:
>
> -<https://{{fqdn}}:{{port}}/#pbsServerAdministration:tasks>
> +<{{base-url}}/#pbsServerAdministration:tasks>
> diff --git a/templates/default/gc-err-subject.txt.hbs b/templates/default/gc-err-subject.txt.hbs
> index ebf49f3b..f02873d1 100644
> --- a/templates/default/gc-err-subject.txt.hbs
> +++ b/templates/default/gc-err-subject.txt.hbs
> @@ -1 +1 @@
> -Garbage Collect Datastore '{{ datastore }}' failed
> +Garbage Collect Datastore '{{datastore}}' failed
> diff --git a/templates/default/gc-ok-body.txt.hbs b/templates/default/gc-ok-body.txt.hbs
> index d2f7cd81..b3aaedf4 100644
> --- a/templates/default/gc-ok-body.txt.hbs
> +++ b/templates/default/gc-ok-body.txt.hbs
> @@ -1,17 +1,17 @@
> Datastore: {{datastore}}
> -Task ID: {{status.upid}}
> -Index file count: {{status.index-file-count}}
> +Task ID: {{upid}}
> +Index file count: {{index-file-count}}
>
> -Removed garbage: {{human-bytes status.removed-bytes}}
> -Removed chunks: {{status.removed-chunks}}
> -Removed bad chunks: {{status.removed-bad}}
> +Removed garbage: {{human-bytes removed-bytes}}
> +Removed chunks: {{removed-chunks}}
> +Removed bad chunks: {{removed-bad}}
>
> -Leftover bad chunks: {{status.still-bad}}
> -Pending removals: {{human-bytes status.pending-bytes}} (in {{status.pending-chunks}} chunks)
> +Leftover bad chunks: {{still-bad}}
> +Pending removals: {{human-bytes pending-bytes}} (in {{pending-chunks}} chunks)
>
> -Original Data usage: {{human-bytes status.index-data-bytes}}
> -On-Disk usage: {{human-bytes status.disk-bytes}} ({{relative-percentage status.disk-bytes status.index-data-bytes}})
> -On-Disk chunks: {{status.disk-chunks}}
> +Original Data usage: {{human-bytes index-data-bytes}}
> +On-Disk usage: {{human-bytes disk-bytes}} ({{relative-percentage disk-bytes index-data-bytes}})
> +On-Disk chunks: {{disk-chunks}}
>
> Deduplication Factor: {{deduplication-factor}}
>
> @@ -20,4 +20,4 @@ Garbage collection successful.
>
> Please visit the web interface for further details:
>
> -<https://{{fqdn}}:{{port}}/#DataStore-{{datastore}}>
> +<{{base-url}}/#DataStore-{{datastore}}>
> diff --git a/templates/default/gc-ok-subject.txt.hbs b/templates/default/gc-ok-subject.txt.hbs
> index 538e3700..ee27ec50 100644
> --- a/templates/default/gc-ok-subject.txt.hbs
> +++ b/templates/default/gc-ok-subject.txt.hbs
> @@ -1 +1 @@
> -Garbage Collect Datastore '{{ datastore }}' successful
> +Garbage Collect Datastore '{{datastore}}' successful
> --
> 2.39.5
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel