From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 52F9A1FF15E for ; Fri, 9 Aug 2024 10:20:55 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 3DEBF39EB5; Fri, 9 Aug 2024 10:21:06 +0200 (CEST) From: Dominik Csapak To: pbs-devel@lists.proxmox.com Date: Fri, 9 Aug 2024 10:20:32 +0200 Message-Id: <20240809082032.831946-1-d.csapak@proxmox.com> X-Mailer: git-send-email 2.39.2 MIME-Version: 1.0 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.016 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: [pbs-devel] [PATCH proxmox-backup v2] fix #5622: backup client: properly handle rate/burst parameters X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" 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 --- 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, + #[serde(skip_serializing_if = "Option::is_none")] + burst: Option, } #[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, dry_run: bool, skip_e2big_xattr: bool, + limit: ClientRateLimitConfig, _info: &ApiMethod, _rpcenv: &mut dyn RpcEnvironment, ) -> Result { @@ -764,16 +760,7 @@ async fn create_backup( verify_chunk_size(size)?; } - let rate = match param["rate"].as_str() { - Some(s) => Some(s.parse::()?), - None => None, - }; - let burst = match param["burst"].as_str() { - Some(s) => Some(s.parse::()?), - 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 { 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::()?), - None => None, - }; - let burst = match param["burst"].as_str() { - Some(s) => Some(s.parse::()?), - 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