[RFC proxmox{,-backup} 0/4] fix #6841: implement request rate limits for all methods

[RFC proxmox{,-backup} 0/4] fix #6841: implement request rate limits for all methods

[Christian Ebner]

Currently request performed by the s3 client to the backend can only
be limited by bandwidth limits or on a per-instance based PUT rate
limit. Providers might however enforce limits on the number of requests
per second, non of which can currently be enforced.

Therefore this patch series implements request limits, using a memory
mapped shared request limiter, similar to what is used to enforce
bandwidth limits. Since providers might have different limits as e.g.
the soft limits by AWS S3 mentioned in [0], allow to set different limits
between `active` and `passive` requests methods.

Sending as RFC as a bit unsure if we should make this configurable
as currently the case or rather trying to dynamically adapt the request
rate in case of requests failing with the respective HTTP status codes.
The latter has the downside that different providers may not clearly
signal when a rate limit is being enforced and/or have different status
codes to do so. Therefore the current approach allows to hard code the
limits.

Currently the rate limit is not yet updated on config changes via API,
only on s3 client instantiation.

[1] https://docs.aws.amazon.com/AmazonS3/latest/userguide/optimizing-performance.html

Reference to the issue in bugzilla: https://bugzilla.proxmox.com/show_bug.cgi?id=6841


proxmox:

Christian Ebner (2):
  fix #6841: allow to set active/passive request rate limits
  s3-client: add 429 HTTP status code as retryable

 proxmox-s3-client/src/api_types.rs |  8 +++-
 proxmox-s3-client/src/client.rs    | 66 ++++++++++++++++++++++++++----
 2 files changed, 66 insertions(+), 8 deletions(-)


proxmox-backup:

Christian Ebner (2):
  s3: config: update or delete active/passive request limit options
  ui: expose request rate limit config options for S3 endpoints

 src/api2/config/s3.rs      | 16 ++++++++++++++++
 www/window/S3ClientEdit.js | 14 ++++++++++++++
 2 files changed, 30 insertions(+)


Summary over all repositories:
  4 files changed, 96 insertions(+), 8 deletions(-)

-- 
Generated by murpp 0.11.0

[PATCH proxmox 1/4] fix #6841: allow to set active/passive request rate limits

[Christian Ebner]

Currently the s3 client implementation only allows to limit request
bandwidth or to limit PUT requests. Latter is further limited to the
instance only.

Extend the current implementation by shared request rate limiters for
for PUT/POST/DELETE (referred to as `active`) and GET/HEAD (referred
to as `passive`) requests [0].

Pre-existing, backwards compatible PUT request limits are used only
when no `active` limit is set, otherwise use the shared limiter.

[0] https://docs.aws.amazon.com/AmazonS3/latest/userguide/optimizing-performance.html

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 proxmox-s3-client/src/api_types.rs |  8 +++-
 proxmox-s3-client/src/client.rs    | 65 ++++++++++++++++++++++++++----
 2 files changed, 65 insertions(+), 8 deletions(-)

diff --git a/proxmox-s3-client/src/api_types.rs b/proxmox-s3-client/src/api_types.rs
index 36bd2510..05b0e322 100644
--- a/proxmox-s3-client/src/api_types.rs
+++ b/proxmox-s3-client/src/api_types.rs
@@ -167,7 +167,7 @@ pub struct S3ClientConfig {
     /// Use path style bucket addressing over vhost style.
     #[serde(skip_serializing_if = "Option::is_none")]
     pub path_style: Option<bool>,
-    /// Rate limit for put requests given as #request/s.
+    /// Rate limit for put requests given as #request/s (deprecated: use active-rate-limit instead).
     #[serde(skip_serializing_if = "Option::is_none")]
     pub put_rate_limit: Option<u64>,
     /// List of provider specific feature implementation quirks.
@@ -185,6 +185,12 @@ pub struct S3ClientConfig {
     /// Upload burst
     #[serde(skip_serializing_if = "Option::is_none")]
     pub burst_out: Option<HumanByte>,
+    /// Combined rate limit for PUT, POST and DELETE requests given as #request/s.
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub limit_active_requests: Option<u64>,
+    /// Combined rate limit for GET and HEAD requests given as #request/s.
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub limit_passive_requests: Option<u64>,
 }
 
 impl S3ClientConfig {
diff --git a/proxmox-s3-client/src/client.rs b/proxmox-s3-client/src/client.rs
index 064793f4..5c8fe43f 100644
--- a/proxmox-s3-client/src/client.rs
+++ b/proxmox-s3-client/src/client.rs
@@ -22,7 +22,7 @@ use tracing::error;
 
 use proxmox_http::client::HttpsConnector;
 use proxmox_http::{Body, ProxyConfig};
-use proxmox_rate_limiter::{RateLimit, RateLimiter, SharedRateLimiter};
+use proxmox_rate_limiter::{RateLimit, RateLimiter, ShareableRateLimit, SharedRateLimiter};
 use proxmox_schema::api_types::CERT_FINGERPRINT_SHA256_SCHEMA;
 
 use crate::api_types::{ProviderQuirks, S3ClientConfig};
@@ -76,10 +76,14 @@ pub struct S3RateLimiterOptions {
 /// Configuration  for the https connector's rate limiter
 pub struct S3RateLimiterConfig {
     options: S3RateLimiterOptions,
+    // bandwidth limits
     rate_in: Option<u64>,
     burst_in: Option<u64>,
     rate_out: Option<u64>,
     burst_out: Option<u64>,
+    // request rate limits
+    active: Option<u64>,
+    passive: Option<u64>,
 }
 
 /// Configuration for the s3 client's shared request counters
@@ -143,6 +147,8 @@ impl S3ClientOptions {
             burst_in: config.burst_in.map(|human_bytes| human_bytes.as_u64()),
             rate_out: config.rate_out.map(|human_bytes| human_bytes.as_u64()),
             burst_out: config.burst_out.map(|human_bytes| human_bytes.as_u64()),
+            active: config.limit_active_requests,
+            passive: config.limit_passive_requests,
         });
         Self {
             endpoint: config.endpoint,
@@ -169,6 +175,9 @@ pub struct S3Client {
     client: Client<HttpsConnector, Body>,
     options: S3ClientOptions,
     authority: Authority,
+    active_request_rate_limiter: Option<Arc<SharedRateLimiter>>,
+    passive_request_rate_limiter: Option<Arc<SharedRateLimiter>>,
+    // TODO: Drop for PBS 5.
     put_rate_limiter: Option<Arc<Mutex<RateLimiter>>>,
     request_counters: Option<Arc<SharedRequestCounters>>,
 }
@@ -234,6 +243,7 @@ impl S3Client {
             S3_TCP_KEEPIDLE_TIME,
         );
 
+        let (mut passive_request_rate_limiter, mut active_request_rate_limiter) = (None, None);
         if let Some(limiter_config) = &options.rate_limiter_config {
             if let Some(limit) = limiter_config.rate_in {
                 let limiter = SharedRateLimiter::mmap_shmem(
@@ -256,6 +266,28 @@ impl S3Client {
                 )?;
                 https_connector.set_write_limiter(Some(Arc::new(limiter)));
             }
+
+            if let Some(limit) = limiter_config.active {
+                let limiter = SharedRateLimiter::mmap_shmem(
+                    &format!("{}.active-requests", limiter_config.options.id),
+                    limit,
+                    limit,
+                    limiter_config.options.user.clone(),
+                    limiter_config.options.base_path.clone(),
+                )?;
+                active_request_rate_limiter = Some(Arc::new(limiter));
+            }
+
+            if let Some(limit) = limiter_config.passive {
+                let limiter = SharedRateLimiter::mmap_shmem(
+                    &format!("{}.active-requests", limiter_config.options.id),
+                    limit,
+                    limit,
+                    limiter_config.options.user.clone(),
+                    limiter_config.options.base_path.clone(),
+                )?;
+                passive_request_rate_limiter = Some(Arc::new(limiter));
+            }
         }
 
         if let Some(proxy_config) = &options.proxy_config {
@@ -300,15 +332,21 @@ impl S3Client {
 
         let authority = Authority::try_from(authority)?;
 
-        let put_rate_limiter = options.put_rate_limit.map(|limit| {
-            let limiter = RateLimiter::new(limit, limit);
-            Arc::new(Mutex::new(limiter))
-        });
+        let put_rate_limiter = if active_request_rate_limiter.is_none() {
+            options.put_rate_limit.map(|limit| {
+                let limiter = RateLimiter::new(limit, limit);
+                Arc::new(Mutex::new(limiter))
+            })
+        } else {
+            None
+        };
 
         Ok(Self {
             client,
             options,
             authority,
+            active_request_rate_limiter,
+            passive_request_rate_limiter,
             put_rate_limiter,
             request_counters,
         })
@@ -440,8 +478,14 @@ impl S3Client {
 
         for retry in 0..MAX_S3_HTTP_REQUEST_RETRY {
             let request = Request::from_parts(parts.clone(), Body::from(body_bytes.clone()));
-            if parts.method == Method::PUT {
-                if let Some(limiter) = &self.put_rate_limiter {
+            if let Some(limiter) = &self.active_request_rate_limiter {
+                if matches!(parts.method, Method::PUT | Method::POST | Method::DELETE) {
+                    let sleep = limiter.register_traffic(Instant::now(), 1);
+                    tokio::time::sleep(sleep).await;
+                }
+            } else if let Some(limiter) = &self.put_rate_limiter {
+                //TODO: Drop with PBS 5
+                if parts.method == Method::PUT {
                     let sleep = {
                         let mut limiter = limiter.lock().unwrap();
                         limiter.register_traffic(Instant::now(), 1)
@@ -450,6 +494,13 @@ impl S3Client {
                 }
             }
 
+            if let Some(limiter) = &self.passive_request_rate_limiter {
+                if matches!(parts.method, Method::GET | Method::HEAD) {
+                    let sleep = limiter.register_traffic(Instant::now(), 1);
+                    tokio::time::sleep(sleep).await;
+                }
+            }
+
             let response = if let Some(deadline) = deadline {
                 tokio::time::timeout_at(deadline, self.client.request(request))
                     .await
-- 
2.47.3

[PATCH proxmox 2/4] s3-client: add 429 HTTP status code as retryable

[Christian Ebner]

When enforcing request rate limits, providers might return HTTP
status code 429 signaling too may requests. Since on retry there
will be a backoff time and the request rate limiter will be checked
again, retry requests which returned this status codes as well.

Fixes: https://forum.proxmox.com/threads/184371/
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 proxmox-s3-client/src/client.rs | 1 +
 1 file changed, 1 insertion(+)

diff --git a/proxmox-s3-client/src/client.rs b/proxmox-s3-client/src/client.rs
index 5c8fe43f..fea98f27 100644
--- a/proxmox-s3-client/src/client.rs
+++ b/proxmox-s3-client/src/client.rs
@@ -515,6 +515,7 @@ impl S3Client {
                     StatusCode::INTERNAL_SERVER_ERROR
                         | StatusCode::SERVICE_UNAVAILABLE
                         | StatusCode::GATEWAY_TIMEOUT
+                        | StatusCode::TOO_MANY_REQUESTS
                 ),
                 Err(_) => true,
             };
-- 
2.47.3

[PATCH proxmox-backup 3/4] s3: config: update or delete active/passive request limit options

[Christian Ebner]

Correctly handle the update/deletion of these limits via the api.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/api2/config/s3.rs | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/src/api2/config/s3.rs b/src/api2/config/s3.rs
index fc0d39e7d..85b5e2557 100644
--- a/src/api2/config/s3.rs
+++ b/src/api2/config/s3.rs
@@ -147,6 +147,10 @@ pub enum DeletableProperty {
     RateOut,
     /// Delete the burst-out property.
     BurstOut,
+    /// Delete the limit-active-requests property.
+    LimitActiveRequests,
+    /// Delete the limit-passive-requests property.
+    LimitPassiveRequests,
     /// Delete the provider quirks property.
     ProviderQuirks,
 }
@@ -230,6 +234,12 @@ pub fn update_s3_client_config(
                 DeletableProperty::BurstOut => {
                     data.config.burst_out = None;
                 }
+                DeletableProperty::LimitActiveRequests => {
+                    data.config.limit_active_requests = None;
+                }
+                DeletableProperty::LimitPassiveRequests => {
+                    data.config.limit_passive_requests = None;
+                }
                 DeletableProperty::ProviderQuirks => {
                     data.config.provider_quirks = None;
                 }
@@ -267,6 +277,12 @@ pub fn update_s3_client_config(
     if let Some(burst_out) = update.burst_out {
         data.config.burst_out = Some(burst_out);
     }
+    if let Some(limit_active_requests) = update.limit_active_requests {
+        data.config.limit_active_requests = Some(limit_active_requests);
+    }
+    if let Some(limit_passive_requests) = update.limit_passive_requests {
+        data.config.limit_passive_requests = Some(limit_passive_requests);
+    }
     if let Some(provider_quirks) = update.provider_quirks {
         data.config.provider_quirks = Some(provider_quirks);
     }
-- 
2.47.3

[PATCH proxmox-backup 4/4] ui: expose request rate limit config options for S3 endpoints

[Christian Ebner]

Allows to specify the s3 client request rate limits in the respective
endpoint configuration.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 www/window/S3ClientEdit.js | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/www/window/S3ClientEdit.js b/www/window/S3ClientEdit.js
index c7e8ade1f..f44d3b43e 100644
--- a/www/window/S3ClientEdit.js
+++ b/www/window/S3ClientEdit.js
@@ -135,6 +135,13 @@ Ext.define('PBS.window.S3ClientEdit', {
                 emptyText: gettext('Unlimited'),
                 submitAutoScaledSizeUnit: true,
             },
+            {
+                xtype: 'proxmoxintegerfield',
+                name: 'limit-active-requests',
+                fieldLabel: gettext('PUT/POST/DELETE request limit (#/s)'),
+                emptyText: gettext('Unlimited'),
+                minValue: 1,
+            },
         ],
         advancedColumn2: [
             {
@@ -151,6 +158,13 @@ Ext.define('PBS.window.S3ClientEdit', {
                 emptyText: gettext('Same as Rate'),
                 submitAutoScaledSizeUnit: true,
             },
+            {
+                xtype: 'proxmoxintegerfield',
+                name: 'limit-passive-requests',
+                fieldLabel: gettext('GET/HEAD request limit (#/s)'),
+                emptyText: gettext('Unlimited'),
+                minValue: 1,
+            },
         ],
         advancedColumnB: [
             {
-- 
2.47.3