[pbs-devel] [PATCH proxmox-backup v2] fix #5622: backup client: properly handle rate/burst parameters
Dominik Csapak
d.csapak at proxmox.com
Fri Aug 9 10:20:32 CEST 2024
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 at 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(¶m)?;
@@ -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(¶m)?;
let archive_name = json::required_string_param(¶m, "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
More information about the pbs-devel
mailing list