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

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

[Christian Ebner]

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');
 			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);
-- 
2.39.5



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

[pbs-devel] [PATCH proxmox-backup 2/2] docs: mention how to set the push sync jobs rate limit

[Christian Ebner]

Explicitly mention how to set the rate limit for sync jobs in push
direction to avoid possible confusion.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 docs/managing-remotes.rst | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/docs/managing-remotes.rst b/docs/managing-remotes.rst
index f8012636e..a784fcc00 100644
--- a/docs/managing-remotes.rst
+++ b/docs/managing-remotes.rst
@@ -227,13 +227,16 @@ Bandwidth Limit
 
 Syncing a datastore to an archive can produce a lot of traffic and impact other
 users of the network. In order to avoid network or storage congestion, you can
-limit the bandwidth of the sync job by setting the ``rate-in`` option either in
-the web interface or using the ``proxmox-backup-manager`` command-line tool:
+limit the bandwidth of a sync job in pull direction by setting the ``rate-in``
+option either in the web interface or using the ``proxmox-backup-manager``
+command-line tool:
 
 .. code-block:: console
 
     # proxmox-backup-manager sync-job update ID --rate-in 20MiB
 
+For sync jobs in push direction use the ``rate-out`` option instead.
+
 Sync Direction Push
 ^^^^^^^^^^^^^^^^^^^
 
-- 
2.39.5



_______________________________________________
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 1/2] fix: ui: sync job: switch rate limit based on sync direction

[Dominik Csapak]

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?

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.

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)

>   			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

Re: [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: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

Re: [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@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?


_______________________________________________
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 1/2] fix: ui: sync job: switch rate limit based on sync direction

[Dominik Csapak]

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

> 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

Re: [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

Okay, that makes sense, but the issue I see there is that per-existing 
rate limits are not shown in the UI anymore, as the `rate` field is now 
used, while the config has the explicit `rate-in/out` set.

So this would need some merging first, or am I missing something?


_______________________________________________
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 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@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?


_______________________________________________
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 1/2] fix: ui: sync job: switch rate limit based on sync direction

[Dominik Csapak]

On 3/17/25 13:11, Christian Ebner wrote:
> 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
> 
> Okay, that makes sense, but the issue I see there is that per-existing rate limits are not shown in 
> the UI anymore, as the `rate` field is now used, while the config has the explicit `rate-in/out` set.
> 
> So this would need some merging first, or am I missing something?

no, you're right, but i think we have a few options:

* contrary to what i suggested, don't prioritize rate-in/out, but only use it when 'rate' is not set
* delete rate-in/out when setting the rate via the gui
* as you suggested in the other response, only keep rate-in and rate-out but expose both
   (although i can already see the confusion about what is 'in' and what is 'out' regarding sync 
direction)
* (other suggestions?)


_______________________________________________
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 1/2] fix: ui: sync job: switch rate limit based on sync direction

[Christian Ebner]

On 3/17/25 13:36, Dominik Csapak wrote:
> On 3/17/25 13:11, Christian Ebner wrote:
>> 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
>>
>> Okay, that makes sense, but the issue I see there is that per-existing 
>> rate limits are not shown in the UI anymore, as the `rate` field is 
>> now used, while the config has the explicit `rate-in/out` set.
>>
>> So this would need some merging first, or am I missing something?
> 
> no, you're right, but i think we have a few options:
> 
> * contrary to what i suggested, don't prioritize rate-in/out, but only 
> use it when 'rate' is not set
> * delete rate-in/out when setting the rate via the gui

Both of these however still require some additional logic on the config 
values, which would be nice if one could avoid. Having only 
`rate-in/out` at the config level makes more sense to me, avoiding any 
need to conditionally merge or set values.

> * as you suggested in the other response, only keep rate-in and rate-out 
> but expose both
>    (although i can already see the confusion about what is 'in' and what 
> is 'out' regarding sync direction)

This possible confusion could however be reduced by adding some more 
context to the field label (or even symbols as for the sync direction on 
the create sync job buttons).
E.g. `Rate limit in (pull contents)` and `Rate limit out (push contents)`.

Another option might be to make one or the other only visible in the UI 
for the corresponding sync direction, or render the "less" useful rate 
limit in the advanced options only, depending on the sync jobs type.

But still wide open for other suggestions!


_______________________________________________
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 1/2] fix: ui: sync job: switch rate limit based on sync direction

[Dominik Csapak]

On 3/17/25 13:54, Christian Ebner wrote:
> On 3/17/25 13:36, Dominik Csapak wrote:
>> On 3/17/25 13:11, Christian Ebner wrote:
>>> 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
>>>
>>> Okay, that makes sense, but the issue I see there is that per-existing rate limits are not shown 
>>> in the UI anymore, as the `rate` field is now used, while the config has the explicit `rate-in/ 
>>> out` set.
>>>
>>> So this would need some merging first, or am I missing something?
>>
>> no, you're right, but i think we have a few options:
>>
>> * contrary to what i suggested, don't prioritize rate-in/out, but only use it when 'rate' is not set
>> * delete rate-in/out when setting the rate via the gui
> 
> Both of these however still require some additional logic on the config values, which would be nice 
> if one could avoid. Having only `rate-in/out` at the config level makes more sense to me, avoiding 
> any need to conditionally merge or set values.
> 
>> * as you suggested in the other response, only keep rate-in and rate-out but expose both
>>    (although i can already see the confusion about what is 'in' and what is 'out' regarding sync 
>> direction)
> 
> This possible confusion could however be reduced by adding some more context to the field label (or 
> even symbols as for the sync direction on the create sync job buttons).
> E.g. `Rate limit in (pull contents)` and `Rate limit out (push contents)`.
> 
> Another option might be to make one or the other only visible in the UI for the corresponding sync 
> direction, or render the "less" useful rate limit in the advanced options only, depending on the 
> sync jobs type.

AFAIU this is basically what you sent (minus the deletion of the other option), right ?

> 
> But still wide open for other suggestions!

even another possibility is to simply set both from the ui?
as in we have a 'rate' field on the ui that sets both rate-in and rate-out?

the only question then is how to handle if the values are different on the backend...

The problem i have with the current way, is that we need to expose the user to a setting that
is unnecessary and confusing in most of the cases. I'd really like to make this not more
complicated as it already is. Thus my suggestion to reduce it to a single 'rate' parameter.


_______________________________________________
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 1/2] fix: ui: sync job: switch rate limit based on sync direction

[Christian Ebner]

On 3/17/25 14:01, Dominik Csapak wrote:
> On 3/17/25 13:54, Christian Ebner wrote:
>> On 3/17/25 13:36, Dominik Csapak wrote:
>>> On 3/17/25 13:11, Christian Ebner wrote:
>>>> 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
>>>>
>>>> Okay, that makes sense, but the issue I see there is that per- 
>>>> existing rate limits are not shown in the UI anymore, as the `rate` 
>>>> field is now used, while the config has the explicit `rate-in/ out` 
>>>> set.
>>>>
>>>> So this would need some merging first, or am I missing something?
>>>
>>> no, you're right, but i think we have a few options:
>>>
>>> * contrary to what i suggested, don't prioritize rate-in/out, but 
>>> only use it when 'rate' is not set
>>> * delete rate-in/out when setting the rate via the gui
>>
>> Both of these however still require some additional logic on the 
>> config values, which would be nice if one could avoid. Having only 
>> `rate-in/out` at the config level makes more sense to me, avoiding any 
>> need to conditionally merge or set values.
>>
>>> * as you suggested in the other response, only keep rate-in and rate- 
>>> out but expose both
>>>    (although i can already see the confusion about what is 'in' and 
>>> what is 'out' regarding sync direction)
>>
>> This possible confusion could however be reduced by adding some more 
>> context to the field label (or even symbols as for the sync direction 
>> on the create sync job buttons).
>> E.g. `Rate limit in (pull contents)` and `Rate limit out (push 
>> contents)`.
>>
>> Another option might be to make one or the other only visible in the 
>> UI for the corresponding sync direction, or render the "less" useful 
>> rate limit in the advanced options only, depending on the sync jobs type.
> 
> AFAIU this is basically what you sent (minus the deletion of the other 
> option), right ?

Yes, making this hidden would preserve the state of the other direction, 
in contrast to clearing it as is the case for the current patch. But it 
might make the code even cleaner, as the conditional switching of the 
values is not required anymore.

> 
>>
>> But still wide open for other suggestions!
> 
> even another possibility is to simply set both from the ui?
> as in we have a 'rate' field on the ui that sets both rate-in and rate-out?
> 
> the only question then is how to handle if the values are different on 
> the backend...

Yes, again requiring some additional logic to handle things differently.

> The problem i have with the current way, is that we need to expose the 
> user to a setting that
> is unnecessary and confusing in most of the cases. I'd really like to 
> make this not more
> complicated as it already is. Thus my suggestion to reduce it to a 
> single 'rate' parameter.

I do agree, but that is what I tried to achieve in the patch already 
(except for preserving the rate limit in the other direction)? Adding 
the `rate` parameter would only help to reduce the switching logic based 
on sync direction.

But given that, I would opt for keeping the current `rate-in/out` 
parameters in the config, having 2 input fields in the UI, one however 
hidden from the user to avoid unnecessary confusion and preserve the 
state of both, in-depended of sync direction.

Any objections?

In any case: Thanks a lot for the discussion!


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