[pbs-devel] [PATCH proxmox-backup v2] fix #5622: backup client: properly handle rate/burst parameters

[pbs-devel] [PATCH proxmox-backup v2] fix #5622: backup client: properly handle rate/burst parameters

[Dominik Csapak]

The rate and burst parameters are integers, so the mapping from value
with `.as_str()` will always return `None` effectively never
applying any rate limit at all.

Fix it by turning them into a HumanByte instead of an integer.

To not crowd the parameter section so much, create a
ClientRateLimitConfig struct that gets flattened into the parameter list
of the backup client.

To adapt the description of the parameters, add new schemas that copy
the `HumanByte` schema but change the description.

With this, the rate limit actually works, and there is no lower limit
any more.

The old TRAFFIC_CONTROL_RATE/BURST_SCHEMAs can be deleted since the
client was the only user of them.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v1:
* instead of doubling down on the integer parameter, make them a proper
  HumanByte one, making it much more comfortable to use

 pbs-api-types/src/traffic_control.rs | 51 ++++++++++++++++++++------
 proxmox-backup-client/src/main.rs    | 53 ++++++++--------------------
 2 files changed, 54 insertions(+), 50 deletions(-)

diff --git a/pbs-api-types/src/traffic_control.rs b/pbs-api-types/src/traffic_control.rs
index fb264531..0da327f2 100644
--- a/pbs-api-types/src/traffic_control.rs
+++ b/pbs-api-types/src/traffic_control.rs
@@ -1,7 +1,7 @@
 use serde::{Deserialize, Serialize};
 
 use proxmox_human_byte::HumanByte;
-use proxmox_schema::{api, IntegerSchema, Schema, StringSchema, Updater};
+use proxmox_schema::{api, ApiType, Schema, StringSchema, Updater};
 
 use crate::{
     CIDR_SCHEMA, DAILY_DURATION_FORMAT, PROXMOX_SAFE_ID_FORMAT, SINGLE_LINE_COMMENT_SCHEMA,
@@ -18,16 +18,6 @@ pub const TRAFFIC_CONTROL_ID_SCHEMA: Schema = StringSchema::new("Rule ID.")
     .max_length(32)
     .schema();
 
-pub const TRAFFIC_CONTROL_RATE_SCHEMA: Schema =
-    IntegerSchema::new("Rate limit (for Token bucket filter) in bytes/second.")
-        .minimum(100_000)
-        .schema();
-
-pub const TRAFFIC_CONTROL_BURST_SCHEMA: Schema =
-    IntegerSchema::new("Size of the token bucket (for Token bucket filter) in bytes.")
-        .minimum(1000)
-        .schema();
-
 #[api(
     properties: {
         "rate-in": {
@@ -71,6 +61,45 @@ impl RateLimitConfig {
             burst_out: burst,
         }
     }
+
+    /// Create a [RateLimitConfig] from a [ClientRateLimitConfig]
+    pub fn from_client_config(limit: ClientRateLimitConfig) -> Self {
+        Self::with_same_inout(limit.rate, limit.burst)
+    }
+}
+
+const CLIENT_RATE_LIMIT_SCHEMA: Schema = StringSchema {
+    description: "Rate limit (for Token bucket filter) in bytes/s with optional unit (B, KB (base 10), MB, GB, ..., KiB (base 2), MiB, Gib, ...).",
+    ..*HumanByte::API_SCHEMA.unwrap_string_schema()
+}
+.schema();
+
+const CLIENT_BURST_SCHEMA: Schema = StringSchema {
+    description: "Size of the token bucket (for Token bucket filter) in bytes with optional unit (B, KB (base 10), MB, GB, ..., KiB (base 2), MiB, Gib, ...).",
+    ..*HumanByte::API_SCHEMA.unwrap_string_schema()
+}
+.schema();
+
+#[api(
+    properties: {
+        rate: {
+            schema: CLIENT_RATE_LIMIT_SCHEMA,
+            optional: true,
+        },
+        burst: {
+            schema: CLIENT_BURST_SCHEMA,
+            optional: true,
+        },
+    },
+)]
+#[derive(Serialize, Deserialize, Default, Clone)]
+#[serde(rename_all = "kebab-case")]
+/// Client Rate Limit Configuration
+pub struct ClientRateLimitConfig {
+    #[serde(skip_serializing_if = "Option::is_none")]
+    rate: Option<HumanByte>,
+    #[serde(skip_serializing_if = "Option::is_none")]
+    burst: Option<HumanByte>,
 }
 
 #[api(
diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
index 5edb2a82..c7bd5aa3 100644
--- a/proxmox-backup-client/src/main.rs
+++ b/proxmox-backup-client/src/main.rs
@@ -15,7 +15,6 @@ use xdg::BaseDirectories;
 
 use pathpatterns::{MatchEntry, MatchType, PatternFlag};
 use proxmox_async::blocking::TokioWriterAdapter;
-use proxmox_human_byte::HumanByte;
 use proxmox_io::StdChannelWriter;
 use proxmox_router::{cli::*, ApiMethod, RpcEnvironment};
 use proxmox_schema::api;
@@ -25,10 +24,10 @@ use pxar::accessor::aio::Accessor;
 use pxar::accessor::{MaybeReady, ReadAt, ReadAtOperation};
 
 use pbs_api_types::{
-    Authid, BackupDir, BackupGroup, BackupNamespace, BackupPart, BackupType, CryptMode,
-    Fingerprint, GroupListItem, PruneJobOptions, PruneListItem, RateLimitConfig, SnapshotListItem,
-    StorageStatus, BACKUP_ID_SCHEMA, BACKUP_NAMESPACE_SCHEMA, BACKUP_TIME_SCHEMA,
-    BACKUP_TYPE_SCHEMA, TRAFFIC_CONTROL_BURST_SCHEMA, TRAFFIC_CONTROL_RATE_SCHEMA,
+    Authid, BackupDir, BackupGroup, BackupNamespace, BackupPart, BackupType, ClientRateLimitConfig,
+    CryptMode, Fingerprint, GroupListItem, PruneJobOptions, PruneListItem, RateLimitConfig,
+    SnapshotListItem, StorageStatus, BACKUP_ID_SCHEMA, BACKUP_NAMESPACE_SCHEMA, BACKUP_TIME_SCHEMA,
+    BACKUP_TYPE_SCHEMA,
 };
 use pbs_client::catalog_shell::Shell;
 use pbs_client::pxar::{ErrorHandler as PxarErrorHandler, MetadataArchiveReader, PxarPrevRef};
@@ -699,13 +698,9 @@ fn spawn_catalog_upload(
                schema: CHUNK_SIZE_SCHEMA,
                optional: true,
            },
-           rate: {
-               schema: TRAFFIC_CONTROL_RATE_SCHEMA,
-               optional: true,
-           },
-           burst: {
-               schema: TRAFFIC_CONTROL_BURST_SCHEMA,
-               optional: true,
+           limit: {
+               type: ClientRateLimitConfig,
+               flatten: true,
            },
            "change-detection-mode": {
                type: BackupDetectionMode,
@@ -749,6 +744,7 @@ async fn create_backup(
     change_detection_mode: Option<BackupDetectionMode>,
     dry_run: bool,
     skip_e2big_xattr: bool,
+    limit: ClientRateLimitConfig,
     _info: &ApiMethod,
     _rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<Value, Error> {
@@ -764,16 +760,7 @@ async fn create_backup(
         verify_chunk_size(size)?;
     }
 
-    let rate = match param["rate"].as_str() {
-        Some(s) => Some(s.parse::<HumanByte>()?),
-        None => None,
-    };
-    let burst = match param["burst"].as_str() {
-        Some(s) => Some(s.parse::<HumanByte>()?),
-        None => None,
-    };
-
-    let rate_limit = RateLimitConfig::with_same_inout(rate, burst);
+    let rate_limit = RateLimitConfig::from_client_config(limit);
 
     let crypto = crypto_parameters(&param)?;
 
@@ -1402,13 +1389,9 @@ We do not extract '.pxar' archives when writing to standard output.
 
 "###
             },
-            rate: {
-                schema: TRAFFIC_CONTROL_RATE_SCHEMA,
-                optional: true,
-            },
-            burst: {
-                schema: TRAFFIC_CONTROL_BURST_SCHEMA,
-                optional: true,
+            limit: {
+                type: ClientRateLimitConfig,
+                flatten: true,
             },
             "allow-existing-dirs": {
                 type: Boolean,
@@ -1499,22 +1482,14 @@ async fn restore(
     overwrite_files: bool,
     overwrite_symlinks: bool,
     overwrite_hardlinks: bool,
+    limit: ClientRateLimitConfig,
     ignore_extract_device_errors: bool,
 ) -> Result<Value, Error> {
     let repo = extract_repository_from_value(&param)?;
 
     let archive_name = json::required_string_param(&param, "archive-name")?;
 
-    let rate = match param["rate"].as_str() {
-        Some(s) => Some(s.parse::<HumanByte>()?),
-        None => None,
-    };
-    let burst = match param["burst"].as_str() {
-        Some(s) => Some(s.parse::<HumanByte>()?),
-        None => None,
-    };
-
-    let rate_limit = RateLimitConfig::with_same_inout(rate, burst);
+    let rate_limit = RateLimitConfig::from_client_config(limit);
 
     let client = connect_rate_limited(&repo, rate_limit)?;
     record_repository(&repo);
-- 
2.39.2



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] applied: [PATCH proxmox-backup v2] fix #5622: backup client: properly handle rate/burst parameters

[Wolfgang Bumiller]

applied, thanks


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel