* [pbs-devel] [PATCH proxmox-backup 1/2] fix: ui: sync job: switch rate limit based on sync direction @ 2025-03-10 17:01 Christian Ebner 2025-03-10 17:01 ` [pbs-devel] [PATCH proxmox-backup 2/2] docs: mention how to set the push sync jobs rate limit Christian Ebner ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Christian Ebner @ 2025-03-10 17:01 UTC (permalink / raw) To: pbs-devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 2/2] docs: mention how to set the push sync jobs rate limit 2025-03-10 17:01 [pbs-devel] [PATCH proxmox-backup 1/2] fix: ui: sync job: switch rate limit based on sync direction Christian Ebner @ 2025-03-10 17:01 ` Christian Ebner 2025-03-17 10:07 ` [pbs-devel] [PATCH proxmox-backup 1/2] fix: ui: sync job: switch rate limit based on sync direction Dominik Csapak 2025-03-18 9:49 ` Christian Ebner 2 siblings, 0 replies; 14+ messages in thread From: Christian Ebner @ 2025-03-10 17:01 UTC (permalink / raw) To: pbs-devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 1/2] fix: ui: sync job: switch rate limit based on sync direction 2025-03-10 17:01 [pbs-devel] [PATCH proxmox-backup 1/2] fix: ui: sync job: switch rate limit based on sync direction Christian Ebner 2025-03-10 17:01 ` [pbs-devel] [PATCH proxmox-backup 2/2] docs: mention how to set the push sync jobs rate limit Christian Ebner @ 2025-03-17 10:07 ` Dominik Csapak 2025-03-17 10:32 ` Christian Ebner 2025-03-18 9:49 ` Christian Ebner 2 siblings, 1 reply; 14+ messages in thread From: Dominik Csapak @ 2025-03-17 10:07 UTC (permalink / raw) To: pbs-devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 1/2] fix: ui: sync job: switch rate limit based on sync direction 2025-03-17 10:07 ` [pbs-devel] [PATCH proxmox-backup 1/2] fix: ui: sync job: switch rate limit based on sync direction Dominik Csapak @ 2025-03-17 10:32 ` Christian Ebner 2025-03-17 10:42 ` Christian Ebner 2025-03-17 11:56 ` Dominik Csapak 0 siblings, 2 replies; 14+ messages in thread From: Christian Ebner @ 2025-03-17 10:32 UTC (permalink / raw) To: Proxmox Backup Server development discussion, Dominik Csapak 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 1/2] fix: ui: sync job: switch rate limit based on sync direction 2025-03-17 10:32 ` Christian Ebner @ 2025-03-17 10:42 ` Christian Ebner 2025-03-17 11:56 ` Dominik Csapak 1 sibling, 0 replies; 14+ messages in thread From: Christian Ebner @ 2025-03-17 10:42 UTC (permalink / raw) To: Proxmox Backup Server development discussion, 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? > >> >> 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 1/2] fix: ui: sync job: switch rate limit based on sync direction 2025-03-17 10:32 ` Christian Ebner 2025-03-17 10:42 ` Christian Ebner @ 2025-03-17 11:56 ` Dominik Csapak 2025-03-17 12:11 ` Christian Ebner 2025-03-17 12:34 ` Christian Ebner 1 sibling, 2 replies; 14+ messages in thread From: Dominik Csapak @ 2025-03-17 11:56 UTC (permalink / raw) To: Christian Ebner, Proxmox Backup Server development discussion 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 1/2] fix: ui: sync job: switch rate limit based on sync direction 2025-03-17 11:56 ` Dominik Csapak @ 2025-03-17 12:11 ` Christian Ebner 2025-03-17 12:36 ` Dominik Csapak 2025-03-17 12:34 ` Christian Ebner 1 sibling, 1 reply; 14+ messages in thread From: Christian Ebner @ 2025-03-17 12:11 UTC (permalink / raw) To: Dominik Csapak, Proxmox Backup Server development discussion 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 1/2] fix: ui: sync job: switch rate limit based on sync direction 2025-03-17 12:11 ` Christian Ebner @ 2025-03-17 12:36 ` Dominik Csapak 2025-03-17 12:54 ` Christian Ebner 0 siblings, 1 reply; 14+ messages in thread From: Dominik Csapak @ 2025-03-17 12:36 UTC (permalink / raw) To: Christian Ebner, Proxmox Backup Server development discussion 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 1/2] fix: ui: sync job: switch rate limit based on sync direction 2025-03-17 12:36 ` Dominik Csapak @ 2025-03-17 12:54 ` Christian Ebner 2025-03-17 13:01 ` Dominik Csapak 0 siblings, 1 reply; 14+ messages in thread From: Christian Ebner @ 2025-03-17 12:54 UTC (permalink / raw) To: Dominik Csapak, Proxmox Backup Server development discussion 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 1/2] fix: ui: sync job: switch rate limit based on sync direction 2025-03-17 12:54 ` Christian Ebner @ 2025-03-17 13:01 ` Dominik Csapak 2025-03-17 13:17 ` Christian Ebner 0 siblings, 1 reply; 14+ messages in thread From: Dominik Csapak @ 2025-03-17 13:01 UTC (permalink / raw) To: Christian Ebner, Proxmox Backup Server development discussion 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 1/2] fix: ui: sync job: switch rate limit based on sync direction 2025-03-17 13:01 ` Dominik Csapak @ 2025-03-17 13:17 ` Christian Ebner 2025-03-18 8:18 ` Dominik Csapak 0 siblings, 1 reply; 14+ messages in thread From: Christian Ebner @ 2025-03-17 13:17 UTC (permalink / raw) To: Dominik Csapak, Proxmox Backup Server development discussion 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 1/2] fix: ui: sync job: switch rate limit based on sync direction 2025-03-17 13:17 ` Christian Ebner @ 2025-03-18 8:18 ` Dominik Csapak 0 siblings, 0 replies; 14+ messages in thread From: Dominik Csapak @ 2025-03-18 8:18 UTC (permalink / raw) To: Christian Ebner, Proxmox Backup Server development discussion On 3/17/25 14:17, Christian Ebner wrote: > 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! No, I think having one field exposed to the user is an OK solution for now. thanks! _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 1/2] fix: ui: sync job: switch rate limit based on sync direction 2025-03-17 11:56 ` Dominik Csapak 2025-03-17 12:11 ` Christian Ebner @ 2025-03-17 12:34 ` Christian Ebner 1 sibling, 0 replies; 14+ messages in thread From: Christian Ebner @ 2025-03-17 12:34 UTC (permalink / raw) To: Dominik Csapak, Proxmox Backup Server development discussion 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 1/2] fix: ui: sync job: switch rate limit based on sync direction 2025-03-10 17:01 [pbs-devel] [PATCH proxmox-backup 1/2] fix: ui: sync job: switch rate limit based on sync direction Christian Ebner 2025-03-10 17:01 ` [pbs-devel] [PATCH proxmox-backup 2/2] docs: mention how to set the push sync jobs rate limit Christian Ebner 2025-03-17 10:07 ` [pbs-devel] [PATCH proxmox-backup 1/2] fix: ui: sync job: switch rate limit based on sync direction Dominik Csapak @ 2025-03-18 9:49 ` Christian Ebner 2 siblings, 0 replies; 14+ messages in thread From: Christian Ebner @ 2025-03-18 9:49 UTC (permalink / raw) To: pbs-devel superseded-by version 2: https://lore.proxmox.com/pbs-devel/20250318094756.204368-1-c.ebner@proxmox.com/T/ _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2025-03-18 9:49 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2025-03-10 17:01 [pbs-devel] [PATCH proxmox-backup 1/2] fix: ui: sync job: switch rate limit based on sync direction Christian Ebner 2025-03-10 17:01 ` [pbs-devel] [PATCH proxmox-backup 2/2] docs: mention how to set the push sync jobs rate limit Christian Ebner 2025-03-17 10:07 ` [pbs-devel] [PATCH proxmox-backup 1/2] fix: ui: sync job: switch rate limit based on sync direction Dominik Csapak 2025-03-17 10:32 ` Christian Ebner 2025-03-17 10:42 ` Christian Ebner 2025-03-17 11:56 ` Dominik Csapak 2025-03-17 12:11 ` Christian Ebner 2025-03-17 12:36 ` Dominik Csapak 2025-03-17 12:54 ` Christian Ebner 2025-03-17 13:01 ` Dominik Csapak 2025-03-17 13:17 ` Christian Ebner 2025-03-18 8:18 ` Dominik Csapak 2025-03-17 12:34 ` Christian Ebner 2025-03-18 9:49 ` Christian Ebner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inboxService provided by Proxmox Server Solutions GmbH | Privacy | Legal