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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

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.

> +        )),
> +    )
>  }