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

[Christian Ebner <c.ebner@proxmox.com>]

On 3/30/26 10:18 PM, Thomas Lamprecht wrote:
> 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?

That would be bad and was not the intention here.

The intention was for the notification callback to be executed only once 
when the threshold barrier is being passed, not for all the subsequent 
traffic exceeding. That is, until the counters are reset or the 
thresholds adapted. Will double check!

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

No, I'm also not aware of any such logic being in place

> 
> 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.

Yes, I agree that this should be implemented.
As discussed off-list, using the token bucket filter to achieve this 
might be a good option, will have a look on how to achieve this and send 
a new version, thanks!

> 
> 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.

I feel like it make sense to hold off until the next version, in case 
the threshold check logic needs further adaption.