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

[Christian Ebner]

On 3/17/25 12:56, Dominik Csapak wrote:
> 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?
>>
> 
> i had actually imagined 3 options for the sync job config
> rate: limits both in/out
> rate-in/out: precedence over rate, limits the respective direction
> 
> and only expose the 'rate' option on the ui
> 
> 
> 
>>>
>>> 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?
> 
> no i mean as main parameter (see above) and rate-in/out only for users
> that want to explicitly handle the different directions
> 
> (AFAIK there is not much data flowing in the opposite direction of
> the sync job, but one might still want to control that)
> 
> the advantage is that it's backwards compatible for existing setups
> 
>>
>>>
>>> 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. 
> 
> i disagree that the reverse direction makes no sense (only a little ;) )
> i.e. pulling the snapshot list will count to the rate-in limit AFAIU
> and sometimes it may make sense to configure that...

Overlooked that part of your response, sorry!

Yes, that is true and if explicitly set should be honored: However, 
given this argument it might even be better to allow explicitly set both 
rates in the sync job edit window instead of having the combined `rate` 
field. This would allow to keep things backwards compatible without the 
need to merge much and give the user the ability to set both as desired 
without much hustle? And maybe a tooltip describing which to set best 
under which conditions?