[pbs-devel] [PATCH proxmox-backup 1/2] fix: ui: sync job: switch rate limit based on sync direction

[Christian Ebner]

On 3/17/25 11:32, Christian Ebner wrote:
> On 3/17/25 11:07, Dominik Csapak wrote:
>> High level comment:
>>
>> I know it's preexisting, bu does it even make sense to have a 'rate- 
>> in' and 'rate-out' for sync
>> jobs? would it not make more sense to have a single 'rate' parameter 
>> and apply it to both
>> directions?
> 
> You mean only as additional parameter for the api endpoint for sync job 
> config creation and update? Or as parameter for the sync job config itself?
> 
> The former might be the better option, and one can check if both rate 
> and rate-in/out were set and abort with error in that case or abort with 
> error if a rate-in was configured for a push or rate-out for a pull?
> 
>>
>> we could maybe introduce a new parameter now to replace both 'rate-in' 
>> and 'rate-out' and
>> use that? we could even maybe leave all three if some users really 
>> prefer to set the
>> limits seperately.
> 
> You mean rate as fallback if rate-in/out are not explicitly set?
> 
>>
>> one comment inline
>>
>> On 3/10/25 18:01, Christian Ebner wrote:
>>> Commit 9aa213b8 ("config: sync: use same config section type `sync`
>>> for push and pull") adapted the sync job edit so jobs in both, push
>>> and pull can be edited using the same window. This however did not
>>> include the switching of the direction to which the http client rate
>>> limit is applied to.
>>>
>>> Fix this by renaming the field to `rate-limit` and  conditionally
>>> settings the values to `rate-in` or `rate-out`.
>>>
>>> Reported in the community forum:
>>> https://forum.proxmox.com/threads/163414/
>>>
>>> Fixes: 9aa213b8 ("config: sync: use same config section type `sync` 
>>> for push and pull")
>>> Signed-off-by: Christian Ebner <c.ebner at proxmox.com>
>>> ---
>>>   www/window/SyncJobEdit.js | 20 +++++++++++++++++---
>>>   1 file changed, 17 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/www/window/SyncJobEdit.js b/www/window/SyncJobEdit.js
>>> index bcd2f2fb2..f980a2efd 100644
>>> --- a/www/window/SyncJobEdit.js
>>> +++ b/www/window/SyncJobEdit.js
>>> @@ -86,6 +86,13 @@ Ext.define('PBS.window.SyncJobEdit', {
>>>       } else {
>>>           values.location = 'remote';
>>>       }
>>> +    if (values['rate-out'] && me.syncDirection === 'push') {
>>> +        values['rate-limit'] = values['rate-out'];
>>> +        delete values['rate-out'];
>>> +    } else if (values['rate-in']) {
>>> +        values['rate-limit'] = values['rate-in'];
>>> +        delete values['rate-in'];
>>> +    }
>>>       me.callParent([values]);
>>>       },
>>> @@ -103,8 +110,15 @@ Ext.define('PBS.window.SyncJobEdit', {
>>>               if (!values.id && me.up('pbsSyncJobEdit').isCreate) {
>>>               values.id = 's-' + 
>>> Ext.data.identifier.Uuid.Global.generate().slice(0, 13);
>>>               }
>>> +            if (values['rate-limit'] && me.syncDirection === 'push') {
>>> +            values['rate-out'] = values['rate-limit'];
>>> +            } else {
>>> +            values['rate-in'] = values['rate-limit'];
>>> +            }
>>> +            delete values['rate-limit'];
>>>               if (!me.isCreate) {
>>>               PBS.Utils.delete_if_default(values, 'rate-in');
>>> +            PBS.Utils.delete_if_default(values, 'rate-out');
>>
>> this now deletes values set in the backend, or not?
>>
>> e.g. i had set
>> rate-in: 10
>> rate-out: 15
>>
>> then i update the config in the GUI and depending on the sync job 
>> direction,
>> the other gets deleted from the config.
>> (that's one reason where a single rate limit setting would make sense)
> 
> Yes, this clears the rate limit for the direction in which it does not 
> make sense at the moment. Issue with 3 parameters (rate, rate-in, rate- 
> out) is which one to show in the sync job edit window? All of them, the 
> more stringent one? I would rather keep the `rate-in/out` in the config, 
> show only the one making sense for that direction and maybe allow 
> setting it via the suggested rate `parameter`? Although I see increased 
> complexity for the additional `rate` parameter at not much gain... so a 
> bit torn.

Maybe adding the rate direction check based on the sync direction on 
config level might make most sense, disallowing to set the wrong rate 
direction for the wrong sync direction? By that one can keep the config 
as is while caching possible misconfiguration at a config level?