Re: [PATCH proxmox{,-backup} v5 00/33] partially fix #6563: add s3 request and traffic counter statistics

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

This is a really well put together series, and with the things I've
mentioned this looks very good to me! I did do some testing, and
everything worked.

I am not super sure if having a "hard" limit makes sense, so no new
operations after that?


On 2026-03-11 14:08, 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 instantiation, being cached for further access.
> 
> 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 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 (13):
>   proxmox-sys: expose msync to flush mmapped contents to filesystem
>   shared-memory: add method without tmpfs check for mmap file location
>   shared-memory: expose msync to flush in-memory contents to filesystem
>   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
>   pbs-api-types: define api type for s3 request statistics
>   s3-client: api-types: define request counter thresholds
>   pbs-api-types: add notification thresholds to datastore config
>   s3-client: implement request counter threshold and exceeding callback
> 
>  pbs-api-types/src/datastore.rs                |  39 ++
>  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               |  96 +++-
>  proxmox-s3-client/src/lib.rs                  |   7 +-
>  proxmox-s3-client/src/response_reader.rs      |  75 ++-
>  .../src/shared_request_counters.rs            | 525 ++++++++++++++++++
>  proxmox-shared-memory/src/lib.rs              |  48 +-
>  proxmox-sys/src/mmap.rs                       |  12 +
>  11 files changed, 852 insertions(+), 19 deletions(-)
>  create mode 100644 proxmox-s3-client/src/shared_request_counters.rs
> 
> 
> proxmox-backup:
> 
> Christian Ebner (20):
>   metrics: split common module imports into individual use statements
>   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
>   datastore: add helper method to get datastore backend type
>   ui: improve variable name indirectly fixing typo
>   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 shared request counter loading into helper
>   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
>   notifications: template data: fix typos in docstrings
>   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
> 
>  debian/proxmox-backup-server.install          |   2 +
>  pbs-datastore/src/datastore.rs                | 158 ++++++--
>  pbs-datastore/src/lib.rs                      |   3 +-
>  pbs-datastore/src/snapshot_reader.rs          |   1 +
>  src/api2/admin/datastore.rs                   |  23 +-
>  src/api2/admin/s3.rs                          |  86 ++++-
>  src/api2/config/datastore.rs                  |  33 +-
>  src/api2/config/notifications/mod.rs          |   1 +
>  src/api2/config/s3.rs                         |   9 +-
>  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/tools/mod.rs                              |   9 +-
>  templates/Makefile                            |   2 +
>  .../default/thresholds-exceeded-body.txt.hbs  |   9 +
>  .../thresholds-exceeded-subject.txt.hbs       |   1 +
>  www/Makefile                                  |   1 +
>  www/Utils.js                                  |  10 +
>  www/datastore/OptionView.js                   |   9 +
>  www/datastore/Summary.js                      | 341 ++++++++++++------
>  www/window/NotificationThresholds.js          | 113 ++++++
>  25 files changed, 907 insertions(+), 182 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/NotificationThresholds.js
> 
> 
> Summary over all repositories:
>   36 files changed, 1759 insertions(+), 201 deletions(-)
>