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

[pbs-devel] [PATCH proxmox-backup] 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.

To fix it, just map from u64 to HumanByte.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---

Alternatively, we could introduce a new string schema to parse into
HumanByte, if that's preferred. (Did not do it that way, because this
fix was way faster for me and is also OK in my opinion).

 proxmox-backup-client/src/main.rs | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
index 5edb2a82..63ad47fc 100644
--- a/proxmox-backup-client/src/main.rs
+++ b/proxmox-backup-client/src/main.rs
@@ -764,14 +764,8 @@ 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 = param["rate"].as_u64().map(HumanByte::from);
+    let burst = param["burst"].as_u64().map(HumanByte::from);
 
     let rate_limit = RateLimitConfig::with_same_inout(rate, burst);
 
@@ -1505,14 +1499,8 @@ async fn restore(
 
     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 = param["rate"].as_u64().map(HumanByte::from);
+    let burst = param["burst"].as_u64().map(HumanByte::from);
 
     let rate_limit = RateLimitConfig::with_same_inout(rate, burst);
 
-- 
2.39.2



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

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

[Christian Ebner]

On 7/23/24 12:04, Dominik Csapak wrote:
> 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.
> 
> To fix it, just map from u64 to HumanByte.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> 
> Alternatively, we could introduce a new string schema to parse into
> HumanByte, if that's preferred. (Did not do it that way, because this
> fix was way faster for me and is also OK in my opinion).

There is already a string schema definition for rate limits [0], might 
be worth to reuse that and parse into `HumanByte` as you suggested?

[0] 
https://git.proxmox.com/?p=proxmox-backup.git;a=blob;f=pbs-api-types/src/traffic_control.rs;h=fb264531e291329fbc0a0a7f54da158ba50e8bb1;hb=HEAD#l21




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

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

[Christian Ebner]

> On 23.07.2024 14:48 CEST Christian Ebner <c.ebner@proxmox.com> wrote:
> 
>  
> On 7/23/24 12:04, Dominik Csapak wrote:
> > 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.
> > 
> > To fix it, just map from u64 to HumanByte.
> > 
> > Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> > ---
> > 
> > Alternatively, we could introduce a new string schema to parse into
> > HumanByte, if that's preferred. (Did not do it that way, because this
> > fix was way faster for me and is also OK in my opinion).
> 
> There is already a string schema definition for rate limits [0], might 
> be worth to reuse that and parse into `HumanByte` as you suggested?
> 
> [0] 
> https://git.proxmox.com/?p=proxmox-backup.git;a=blob;f=pbs-api-types/src/traffic_control.rs;h=fb264531e291329fbc0a0a7f54da158ba50e8bb1;hb=HEAD#l21

Ah no, this is the already used IntegerSchema, never mind.


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

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

[Christian Ebner]

On 7/23/24 12:04, Dominik Csapak wrote:
> 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.
> 
> To fix it, just map from u64 to HumanByte.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---

Did some tests on this and while the rate limit gets applied, the burst 
value does not seem to be honored for my case. Am I misinterpreting the 
burst parameter or is there still an underlying issue?

Here is what was tested:
- Monitor network throughput to localhost by running `tcpsubnet-bpfcc 
127.0.0.1/32` as well as `iftop` (got only low additional traffic noise 
without invoking the proxmox-backup-client).
- Before each proxmox-backup-client invocation, the chunks and snapshots 
were deleted from the datastore to have similar conditions.
- Backup using proxmox-backup-client to local datastore via localhost by 
running `proxmox-backup-client backup usr.pxar:/usr --rate=100000`, rate 
limit is now honored as expected!
- `proxmox-backup-client backup usr.pxar:/usr --rate=100000 
--burst=1000000000000`, expected to reach much higher initial burst 
throughput because of the huge bucket size, but rate is limited to the 
same rate as above already from the beginning.
- Without rate limit, much higher rates can be reached, so that is not 
the limiting factor.


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

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

[Christian Ebner]

On 8/1/24 14:44, Christian Ebner wrote:
> On 7/23/24 12:04, Dominik Csapak wrote:
>> 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.
>>
>> To fix it, just map from u64 to HumanByte.
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
> 
> Did some tests on this and while the rate limit gets applied, the burst 
> value does not seem to be honored for my case. Am I misinterpreting the 
> burst parameter or is there still an underlying issue?
> 
> Here is what was tested:
> - Monitor network throughput to localhost by running `tcpsubnet-bpfcc 
> 127.0.0.1/32` as well as `iftop` (got only low additional traffic noise 
> without invoking the proxmox-backup-client).
> - Before each proxmox-backup-client invocation, the chunks and snapshots 
> were deleted from the datastore to have similar conditions.
> - Backup using proxmox-backup-client to local datastore via localhost by 
> running `proxmox-backup-client backup usr.pxar:/usr --rate=100000`, rate 
> limit is now honored as expected!
> - `proxmox-backup-client backup usr.pxar:/usr --rate=100000 
> --burst=1000000000000`, expected to reach much higher initial burst 
> throughput because of the huge bucket size, but rate is limited to the 
> same rate as above already from the beginning.
> - Without rate limit, much higher rates can be reached, so that is not 
> the limiting factor.

Ah, I see the bucket starts empty, so this is expected then.



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

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

[Thomas Lamprecht]

On 23/07/2024 12:04, Dominik Csapak wrote:
> 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.
> 
> To fix it, just map from u64 to HumanByte.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> 
> Alternatively, we could introduce a new string schema to parse into
> HumanByte, if that's preferred. (Did not do it that way, because this
> fix was way faster for me and is also OK in my opinion).

I mean, tbh. it seems like this was the original intention, i.e. that one can
also pass HumanByte here, which would be pretty convenient. 

FWIW, this was u64 back when added and there's a commit that changes this in a
(buggy) way to the as_str, when HumanByte got introduced:

2d5287fb ("use RateLimitConfig for HttpClient and pull")

Sadly the comment message of this and the previous ones are basically
non-existent, but I faintly remember that Dietmar and I talked about this back
then, and I'm pretty sure that my stance back then is as now: I find it odd
that the API and config can have this written in HumanByte form but not on the
CLI, where it'd be actually the most useful place; as the API is either
accessed through web UI, where one can transform this from a human readable
form to bytes or through a automation system, which normally have ways to
allow X*1024 or the like calculations.

So I'd rather go towards a HuamnByte based schema, albeit as the
TRAFFIC_CONTROL_RATE_SCHEMA and TRAFFIC_CONTROL_BURST_SCHEMA are only used
in the client CLI code anyway I'd actually drop them from pbs-api-types as
they are obviously confusing (not used in the actual API) and either declare a
ClientRateLimitConfig struct there (that is then flattened in the client backup
schema definition) or just have something directly in the client code.



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

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

[Dominik Csapak]

On 8/7/24 21:20, Thomas Lamprecht wrote:
> On 23/07/2024 12:04, Dominik Csapak wrote:
>> 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.
>>
>> To fix it, just map from u64 to HumanByte.
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>
>> Alternatively, we could introduce a new string schema to parse into
>> HumanByte, if that's preferred. (Did not do it that way, because this
>> fix was way faster for me and is also OK in my opinion).
> 
> I mean, tbh. it seems like this was the original intention, i.e. that one can
> also pass HumanByte here, which would be pretty convenient.
> 
> FWIW, this was u64 back when added and there's a commit that changes this in a
> (buggy) way to the as_str, when HumanByte got introduced:
> 
> 2d5287fb ("use RateLimitConfig for HttpClient and pull")
> 
> Sadly the comment message of this and the previous ones are basically
> non-existent, but I faintly remember that Dietmar and I talked about this back
> then, and I'm pretty sure that my stance back then is as now: I find it odd
> that the API and config can have this written in HumanByte form but not on the
> CLI, where it'd be actually the most useful place; as the API is either
> accessed through web UI, where one can transform this from a human readable
> form to bytes or through a automation system, which normally have ways to
> allow X*1024 or the like calculations.
> 
> So I'd rather go towards a HuamnByte based schema, albeit as the
> TRAFFIC_CONTROL_RATE_SCHEMA and TRAFFIC_CONTROL_BURST_SCHEMA are only used
> in the client CLI code anyway I'd actually drop them from pbs-api-types as
> they are obviously confusing (not used in the actual API) and either declare a
> ClientRateLimitConfig struct there (that is then flattened in the client backup
> schema definition) or just have something directly in the client code.
> 

Sure, make sense, I'll see that I take the time to do that, thanks for the input!


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

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

[Dominik Csapak]

sent a v2:

https://lists.proxmox.com/pipermail/pbs-devel/2024-August/010486.html


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