Re: [PATCH proxmox{,-backup} v6 00/34] partially fix #6563: add s3 counter for statistics and notifications

[Hannes Laimer <h.laimer@proxmox.com>]

Aside from the few small things I've mentioned and that can easily be
addressed in a follow-up, this looks very good to me! The things form v5
are fixed, and I also did some more extensive testing and everything
worked as expected.

Consider this

Reviewed-by: Hannes Laimer <h.laimer@proxmox.com>
Tested-by: Hannes Laimer <h.laimer@proxmox.com>


(side-note: proxmox-backup patches don't seem to apply with the proxy
series underneath, proxmox is fine)

On 2026-03-19 10:41, Christian Ebner wrote:
> This patch series implements request and traffic counters for the s3
> client, being shared as atomic counters via shared memory and mmapping
> across all s3-client instances.
> 
> The shared counters are instantiated on demand for individual
> datastores with s3 backend on s3 client instantiation and stored as
> part of the datastore implementation by loading the mmapped file on
> datastore creation, being cached for further access. Counter threshold
> values can be defined in the datastore options, allowing to send
> notifications via the notification system if counters exceeded the set
> threshold values. Further, a per-datastore schedule can be configured
> to reset the counters with given interval.
> 
> Further, counters are being loaded during rrd metrics collection and
> exposed as charts in the datastore summary, including the total
> request counts per method, the total upload and download traffic as
> well as the averaged upload and download rate.
> 
> Usage statistics, are not included in this series an will be tackled
> as followup series.
> 
> Link to the bugtracker issue:
> https://bugzilla.proxmox.com/show_bug.cgi?id=6563
> 
> Changes since version 5 (thanks a lot @Hannes for review and testing):
> - Implemented the scheduled reset logic on top of the previous series
> - Also upload/download traffic counters are now reset, not just request counters
> - Fixed the possible swallowing of errors in `Content`s `Body` implementation
> - request_flush() is no longer called by spawning a blocking tokio task
> - Fixed the notification callback to not capture the datastore name, but provide
>   a `label` as callback parameter instead
> - Fix the api definition for the reset_counters endpoint
> - Fixed incorrect graph labels and typos discovered during review
> - Reordered patches in a more logical way rather than development flow
> 
> Changes since version 4:
> - Rebase on top of patch series https://lore.proxmox.com/pbs-devel/20260311095906.202410-1-c.ebner@proxmox.com/T/
>   as requested by Fabian in order to facilitate review flow.
> 
> Changes since version 3:
> - Extend previous implementation by threshold values for counters and
>   mechanism to send notifications once thresholds are exceeded
> 
> Changes since version 2:
> - Introduce msync flushing mechanism to periodically persist counters
>   state to mmap backing file.
> 
> Changes since version 1 (thanks @Robert for feedback):
> - Keep tmpfs check in shmem mapping for pre-existing code, add
>   dedicated methods to create mmapped files in persistent locations.
> - Reorder and align atomic counters to 32-byte (half default cache
>   line size) to reduce false sharing.
> - Rework request counter init logic, avoid unsafe code and undefined
>   behaviour.
> - Rework page size calculation based on new counter alignment.
> - Avoid the need to open and mmap counter file for each rrd data
>   collection, keep the per-datastores filehandle cached instead.
> 
> 
> proxmox:
> 
> Christian Ebner (12):
>   tree-wide: fix formatting issue via cargo fmt
>   s3-client: add persistent shared request counters for client
>   s3-client: add counters for upload/download traffic
>   s3-client: account for upload traffic on successful request sending
>   s3-client: account for downloaded bytes in incoming response body
>   s3-client: request counters: periodically persist counters to file
>   s3-client: sync flush request counters on client instance drop
>   s3-client: api-types: define request counter thresholds
>   s3-client: implement request counter threshold and exceeding callback
>   pbs-api-types: define api type for s3 request statistics
>   pbs-api-types: add notification thresholds to datastore config
>   pbs-api-types: add reset schedule for notification threshold counters
> 
>  pbs-api-types/src/datastore.rs                |  56 ++
>  pbs-api-types/src/remote.rs                   |   9 +-
>  proxmox-s3-client/Cargo.toml                  |   6 +
>  proxmox-s3-client/debian/control              |   3 +
>  proxmox-s3-client/examples/s3_client.rs       |   1 +
>  proxmox-s3-client/src/api_types.rs            |  59 ++
>  proxmox-s3-client/src/client.rs               | 101 +++-
>  proxmox-s3-client/src/lib.rs                  |   7 +-
>  proxmox-s3-client/src/response_reader.rs      |  67 ++-
>  .../src/shared_request_counters.rs            | 537 ++++++++++++++++++
>  10 files changed, 824 insertions(+), 22 deletions(-)
>  create mode 100644 proxmox-s3-client/src/shared_request_counters.rs
> 
> 
> proxmox-backup:
> 
> Christian Ebner (22):
>   metrics: split common module imports into individual use statements
>   ui: improve variable name indirectly fixing typo
>   notifications: template data: fix typos in docstrings
>   datastore: collect request statistics for s3 backed datastores
>   datastore: expose request counters for s3 backed datastores
>   api: s3: add endpoint to reset s3 request counters
>   bin: s3: expose request counter reset method as cli command
>   ui: datastore summary: move store to be part of summary panel
>   ui: expose s3 request counter statistics in the datastore summary
>   metrics: collect s3 datastore statistics as rrd metrics
>   api: admin: expose s3 statistics in datastore rrd data
>   partially fix #6563: ui: expose s3 rrd charts in datastore summary
>   datastore: refactor datastore lookup parameters into dedicated type
>   api: config: update notification thresholds for config and counters
>   ui: utils: add helper to render notification threshold property string
>   ui: add notification thresholds edit window
>   notification: define templates and template data for thresholds
>   datastore: add thresholds notification callback on datastore lookup
>   api/ui: notifications: add 'thresholds' as notification type value
>   api: config: allow counter reset schedule editing
>   ui: expose counter reset schedule edit window
>   bin: proxy: periodically schedule counter reset task
> 
>  debian/proxmox-backup-server.install          |   2 +
>  pbs-datastore/src/datastore.rs                | 194 ++++++++--
>  pbs-datastore/src/lib.rs                      |   3 +-
>  pbs-datastore/src/snapshot_reader.rs          |   7 +-
>  src/api2/admin/datastore.rs                   |  48 ++-
>  src/api2/admin/namespace.rs                   |   9 +-
>  src/api2/admin/s3.rs                          |  86 ++++-
>  src/api2/backup/mod.rs                        |   3 +-
>  src/api2/config/datastore.rs                  |  42 ++-
>  src/api2/config/notifications/mod.rs          |   1 +
>  src/api2/config/s3.rs                         |   9 +-
>  src/api2/reader/mod.rs                        |   3 +-
>  src/api2/status/mod.rs                        |   6 +-
>  src/api2/tape/backup.rs                       |   6 +-
>  src/api2/tape/restore.rs                      |   6 +-
>  src/bin/proxmox-backup-proxy.rs               |  77 +++-
>  src/bin/proxmox_backup_manager/s3.rs          |  33 ++
>  src/server/metric_collection/metric_server.rs |   8 +-
>  src/server/metric_collection/mod.rs           | 108 +++++-
>  src/server/metric_collection/pull_metrics.rs  |  18 +-
>  src/server/metric_collection/rrd.rs           |  34 +-
>  src/server/notifications/mod.rs               |  39 +-
>  src/server/notifications/template_data.rs     |  38 +-
>  src/server/prune_job.rs                       |   3 +-
>  src/server/pull.rs                            |   7 +-
>  src/server/push.rs                            |   3 +-
>  src/server/verify_job.rs                      |   3 +-
>  src/tools/mod.rs                              |  17 +-
>  templates/Makefile                            |   2 +
>  .../default/thresholds-exceeded-body.txt.hbs  |   9 +
>  .../thresholds-exceeded-subject.txt.hbs       |   1 +
>  www/Makefile                                  |   2 +
>  www/Utils.js                                  |  10 +
>  www/datastore/OptionView.js                   |  16 +
>  www/datastore/Summary.js                      | 345 ++++++++++++------
>  www/window/CounterResetScheduleEdit.js        |  27 ++
>  www/window/NotificationThresholds.js          | 113 ++++++
>  37 files changed, 1106 insertions(+), 232 deletions(-)
>  create mode 100644 templates/default/thresholds-exceeded-body.txt.hbs
>  create mode 100644 templates/default/thresholds-exceeded-subject.txt.hbs
>  create mode 100644 www/window/CounterResetScheduleEdit.js
>  create mode 100644 www/window/NotificationThresholds.js
> 
> 
> Summary over all repositories:
>   47 files changed, 1930 insertions(+), 254 deletions(-)
>