Re: [PATCH proxmox-backup v6 18/22] datastore: add thresholds notification callback on datastore lookup

Thomas Lamprecht
2026-03-30T20:18:59Z

Am 19.03.26 um 10:41 schrieb Christian Ebner:
> Pass in the callback to be executed when a datastore threshold value
> exceeds its set limit. This callback is then callable by the s3
> client request counters implementation, currently the only component
> defining thresholds.
>
> When a threshold is exceeded, the notification is generated by the
> callback and queued for being send.
>
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> pbs-datastore/src/datastore.rs | 36 ++++++++++++++++++++++++----
> pbs-datastore/src/snapshot_reader.rs | 1 +
> src/tools/mod.rs | 9 ++++++-
> 3 files changed, 41 insertions(+), 5 deletions(-)
>
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
[...]
> @@ -710,6 +728,16 @@ impl DataStore {
> let mut request_counters = Self::request_counters(&config, &backend_config)?;
>
> Self::update_notification_thresholds(&mut request_counters, &config)?;
> + request_counters.set_thresholds_exceeded_callback(
> + config.name,
> + Box::new(move |label, counter_id, threshold, value| {
> + thresholds_exceeded_callback.map(|cb| {
> + if let Err(err) = cb(label, counter_id, threshold, value) {
> + log::error!("failed to send notification: {err:?}");
> + }
> + });
> + }),
> + );
>
> (Some(cache), Some(Arc::new(request_counters)))
> } else {

> diff --git a/src/tools/mod.rs b/src/tools/mod.rs
[...]
> @@ -192,5 +193,11 @@ pub(crate) fn backup_info_to_snapshot_list_item(
> /// Lookup the datastore by name with given operation.
> #[inline(always)]
> pub fn lookup_with<'a>(name: &'a str, operation: Operation) -> DataStoreLookup<'a> {
> - DataStoreLookup::with(name, operation)
> + DataStoreLookup::with(
> + name,
> + operation,
> + Arc::new(Some(
> + crate::server::notifications::send_datastore_threshold_exceeded,

But in the s3-client's add_download_traffic we _always_ trigger this threshold,
and this callback _always_ sends a notification. This seems like allowing
for a notification storm?

I do not recall any generic debouncing or rate limit logic in the notification
stack, or am I mistaken?

Because if not, then we really should get some basic rate limiting done here,
like only trigger a notification if the previous one was at least X minutes
ago.

As on a big (e.g. initial) sync with a underestimated low limit chosen this
could cause millions of notifications - having dealt with some software going
"nuts" w.r.t. mails only a few days ago, I really would like to avoid this,
as it can be more than annoying (e.g., mail server might buckle up on that
amount of mails in the queue).

But this - if it really does apply and I did not just overlook something -
seems to be the only bigger issue to me. And FWIW, we can also follow up on
this, e.g. I could apply the proxmox parts already.

> + )),
> + )
> }