From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
Fiona Ebner <f.ebner@proxmox.com>
Subject: Re: [pve-devel] [PATCH manager] close #4513: ui: backup job: add performance tab
Date: Tue, 6 Jun 2023 17:23:29 +0200 [thread overview]
Message-ID: <47e917a8-6b93-3590-1313-715dd3b0a7fe@proxmox.com> (raw)
In-Reply-To: <20230315132425.119055-1-f.ebner@proxmox.com>
stumbled upon this again when checking out old(er) patches without comments,
some higher level comments inline.
Am 15/03/2023 um 14:24 schrieb Fiona Ebner:
> pigz is not exposed, because it only works after manually installing
> the pigz package.
>
> ionice is not exposed, because it only works in combination with the
> BFQ scheduler and even then not in all cases (only affects the
> compressor when doing snapshot/suspend mode backup of a VM).
>
> These can still be added with appropriate notes if there is enough
> user demand.
>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> www/manager6/Makefile | 1 +
> www/manager6/dc/Backup.js | 12 +++
> www/manager6/panel/BackupPerformance.js | 110 ++++++++++++++++++++++++
> 3 files changed, 123 insertions(+)
> create mode 100644 www/manager6/panel/BackupPerformance.js
>
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index b73b729a..0c92b984 100644
> --- a/www/manager6/Makefile
> +++ b/www/manager6/Makefile
> @@ -88,6 +88,7 @@ JSSRC= \
> grid/ResourceGrid.js \
> panel/ConfigPanel.js \
> panel/BackupJobPrune.js \
> + panel/BackupPerformance.js \
> panel/HealthWidget.js \
> panel/IPSet.js \
> panel/RunningChart.js \
> diff --git a/www/manager6/dc/Backup.js b/www/manager6/dc/Backup.js
> index d0046177..8da6e7d7 100644
> --- a/www/manager6/dc/Backup.js
> +++ b/www/manager6/dc/Backup.js
> @@ -172,6 +172,11 @@ Ext.define('PVE.dc.BackupEdit', {
> PVE.Utils.unEscapeNotesTemplate(data['notes-template']);
> }
>
> + if (data.performance) {
> + Object.assign(data, data.performance);
> + delete data.performance;
> + }
> +
> view.setValues(data);
> },
> });
> @@ -411,6 +416,13 @@ Ext.define('PVE.dc.BackupEdit', {
> },
> ],
> },
> + {
> + xtype: 'pveBackupPerformancePanel',
> + title: gettext('Performance'),
> + cbind: {
> + isCreate: '{isCreate}',
> + },
> + },
> ],
> },
> ],
> diff --git a/www/manager6/panel/BackupPerformance.js b/www/manager6/panel/BackupPerformance.js
> new file mode 100644
> index 00000000..17e0f34c
> --- /dev/null
> +++ b/www/manager6/panel/BackupPerformance.js
> @@ -0,0 +1,110 @@
> +/*
> + * Input panel for backup performance settings intended to be used as part of an edit/create window.
> + */
> +Ext.define('PVE.panel.BackupPerformance', {
> + extend: 'Proxmox.panel.InputPanel',
> + xtype: 'pveBackupPerformancePanel',
> + mixins: ['Proxmox.Mixin.CBind'],
> +
> + cbindData: function() {
> + let me = this;
> + me.isCreate = !!me.isCreate;
> + return {};
> + },
> +
> + onGetValues: function(formValues) {
> + if (this.needMask) { // isMasked() may not yet be true if not rendered once
> + return {};
> + }
> +
> + let options = { 'delete': [] };
> +
> + let performance = {};
> + let performanceOptions = ['max-workers'];
> +
> + for (const [key, value] of Object.entries(formValues)) {
> + if (performanceOptions.includes(key)) {
> + performance[key] = value;
> + // deleteEmpty is not currently implemented for pveBandwidthField
> + } else if (key === 'bwlimit' && value === '') {
> + options.delete.push('bwlimit');
> + } else if (key === 'delete') {
> + if (Array.isArray(value)) {
> + value.filter(opt => !performanceOptions.includes(opt)).forEach(
> + opt => options.delete.push(opt),
> + );
> + } else if (!performanceOptions.includes(formValues.delete)) {
> + options.delete.push(value);
> + }
> + } else {
> + options[key] = value;
> + }
> + }
> +
> + if (Object.keys(performance).length > 0) {
> + options.performance = PVE.Parser.printPropertyString(performance);
> + } else {
> + options.delete.push('performance');
> + }
> +
> + if (this.isCreate) {
> + delete options.delete;
> + }
> +
> + return options;
> + },
> +
hmm, maybe we should go for a layout with one field per row, but besides that a
short description about what it controls, like:
Label [ Field ] Description
Where the descriptions are sorta in the style of the CPU flag ones for VM CPU
edit, i.e., provide some context and hint of what this does.
Because max-workers "VM Worker Count" is not easily understandable IMO, i.e.,
one could think that's parallel backups done by separate workers (like e.g.,
max worker is used for migrate-all or HA LRM parallel workers)
> + column1: [
> + {
> + xtype: 'proxmoxintegerfield',
> + name: 'zstd',
> + fieldLabel: Ext.String.format('{0} ({1})', gettext('Thread count'), 'zstd'),
> + emptyText: gettext('use fallback'),
disabling that when ZSTD is not selected might slightly improve UX too.
> + minValue: 0,
> + cbind: {
> + deleteEmpty: '{!isCreate}',
> + },
> + autoEl: {
> + tag: 'div',
> + 'data-qtip': gettext('With 0, half of the available cores are used'),
> + },
> + },
> + {
> + xtype: 'proxmoxintegerfield',
> + name: 'max-workers',
> + minValue: 1,
> + maxValue: 256,
> + fieldLabel: Ext.String.format('{0} ({1})', gettext('Worker count'), 'VM'),
> + emptyText: gettext('use fallback'),
> + cbind: {
> + deleteEmpty: '{!isCreate}',
> + },
> + },
> + ],
> +
> + column2: [
> + {
> + xtype: 'pveBandwidthField',
> + name: 'bwlimit',
> + fieldLabel: gettext('Bandwidth Limit'),
> + emptyText: gettext('use fallback'),
> + backendUnit: 'KiB',
> + allowZero: true,
> + emptyValue: '',
> + autoEl: {
> + tag: 'div',
> + 'data-qtip': Ext.String.format(gettext('Use {0} for unlimited'), 0),
> + },
> + },
> + ],
> +
> + columnB: [
> + {
> + xtype: 'component',
> + userCls: 'pmx-hint',
> + name: 'pbs-hint',
> + padding: '5 1',
> + html: gettext("Note that vzdump.conf is used as a fallback"),
> + },
> + ],
> +});
prev parent reply other threads:[~2023-06-06 15:23 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-15 13:24 Fiona Ebner
2023-03-15 13:24 ` [pve-devel] [PATCH docs] backup: update information about performance settings Fiona Ebner
2023-03-15 13:24 ` [pve-devel] [PATCH guest-common] vzdump: config: improve description of ionice setting Fiona Ebner
2023-06-06 15:23 ` Thomas Lamprecht [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=47e917a8-6b93-3590-1313-715dd3b0a7fe@proxmox.com \
--to=t.lamprecht@proxmox.com \
--cc=f.ebner@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox