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

[Christian Ebner <c.ebner@proxmox.com>]

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@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.

> 
>>               PBS.Utils.delete_if_default(values, 'remote');
>>               if (typeof values.delete === 'string') {
>>                   values.delete = values.delete.split(',');
>> @@ -185,7 +199,7 @@ Ext.define('PBS.window.SyncJobEdit', {
>>               },
>>               {
>>               xtype: 'pmxBandwidthField',
>> -            name: 'rate-in',
>> +            name: 'rate-limit',
>>               fieldLabel: gettext('Rate Limit'),
>>               emptyText: gettext('Unlimited'),
>>               submitAutoScaledSizeUnit: true,
>> @@ -221,7 +235,7 @@ Ext.define('PBS.window.SyncJobEdit', {
>>                   let me = this;
>>                   let form = me.up('pbsSyncJobEdit');
>>                   let nsField = form.down('field[name=remote-ns]');
>> -                let rateLimitField = form.down('field[name=rate-in]');
>> +                let rateLimitField = form.down('field[name=rate- 
>> limit]');
>>                   let remoteField = form.down('field[name=remote]');
>>                   let storeField = form.down('field[name=remote-store]');
>> @@ -263,7 +277,7 @@ Ext.define('PBS.window.SyncJobEdit', {
>>                   let me = this;
>>                   let remoteStoreField = 
>> me.up('pbsSyncJobEdit').down('field[name=remote-store]');
>>                   remoteStoreField.setRemote(value);
>> -                let rateLimitField = 
>> me.up('pbsSyncJobEdit').down('field[name=rate-in]');
>> +                let rateLimitField = 
>> me.up('pbsSyncJobEdit').down('field[name=rate-limit]');
>>                   rateLimitField.setDisabled(!value);
>>                   if (!value) {
>>                       rateLimitField.setValue(null);
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 



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