Re: [PATCH pve-manager 5/8] ui: Add CPU flag editor for custom models

[Maximiliano Sandoval <m.sandoval@proxmox.com>]

Arthur Bied-Charreton <a.bied-charreton@proxmox.com> writes:

> Add CPU flag editor to the CPUTypeEdit component, using the VMCPUFlagSelector
> also used in the VM creation flow. By default, only show the CPU flags that
> are currently meant to be shown in the VM creation window, see [0]. When in
> CPUTypeEdit, show all available flags.
>
> For each flag in VMCPUFlagSelector, also display which node(s) it is available
> on to limit misconfigurations.
>
> Original patch:
> https://lore.proxmox.com/pve-devel/20211028114150.3245864-10-s.reiter@proxmox.com
>
> [0] https://git.proxmox.com/?p=qemu-server.git;a=blob;f=src/PVE/QemuServer/CPUConfig.pm;h=32ec495422791422f20caa928d6b632b3de4fcd9;hb=refs/heads/master#l349
>
> Originally-by: Stefan Reiter <s.reiter@proxmox.com>
> Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> ---
>  www/manager6/dc/CPUTypeEdit.js         |  11 ++-
>  www/manager6/form/CPUModelSelector.js  |   1 +
>  www/manager6/form/VMCPUFlagSelector.js | 121 ++++++++++++++++++++++---
>  3 files changed, 117 insertions(+), 16 deletions(-)
>
> diff --git a/www/manager6/dc/CPUTypeEdit.js b/www/manager6/dc/CPUTypeEdit.js
> index 8cf508b4..d6e06601 100644
> --- a/www/manager6/dc/CPUTypeEdit.js
> +++ b/www/manager6/dc/CPUTypeEdit.js
> @@ -84,7 +84,16 @@ Ext.define('PVE.dc.CPUTypeEdit', {
>                      name: 'phys-bits',
>                  },
>              ],
> -
> +            columnB: [
> +                {
> +                    xtype: 'vmcpuflagselector',
> +                    fieldLabel: gettext('Extra CPU flags'),
> +                    name: 'flags',
> +                    restrictToVMFlags: false,
> +                    height: 380,
> +                    hideHeaders: false,
> +                },
> +            ],
>          },
>      ],
>  });
> diff --git a/www/manager6/form/CPUModelSelector.js b/www/manager6/form/CPUModelSelector.js
> index 2154ff46..683fa469 100644
> --- a/www/manager6/form/CPUModelSelector.js
> +++ b/www/manager6/form/CPUModelSelector.js
> @@ -17,6 +17,7 @@ Ext.define('PVE.form.CPUModelSelector', {
>      anyMatch: true,
>      forceSelection: true,
>      autoSelect: false,
> +    triggerAction: 'query',
>  
>      deleteEmpty: true,
>      config: {
> diff --git a/www/manager6/form/VMCPUFlagSelector.js b/www/manager6/form/VMCPUFlagSelector.js
> index 74b1a2c4..06c9d9f1 100644
> --- a/www/manager6/form/VMCPUFlagSelector.js
> +++ b/www/manager6/form/VMCPUFlagSelector.js
> @@ -1,3 +1,19 @@
> +const VM_CPU_FLAGS_SUBSET = {
> +    aes: true,
> +    'amd-no-ssb': true,
> +    'amd-ssbd': true,
> +    'hv-evmcs': true,
> +    'hv-tlbflush': true,
> +    ibpb: true,
> +    'md-clear': true,
> +    'nested-virt': true,
> +    pcid: true,
> +    pdpe1gb: true,
> +    'spec-ctrl': true,
> +    ssbd: true,
> +    'virt-ssbd': true,
> +};
> +
>  Ext.define('PVE.form.VMCPUFlagSelector', {
>      extend: 'Ext.grid.Panel',
>      alias: 'widget.vmcpuflagselector',
> @@ -6,6 +22,10 @@ Ext.define('PVE.form.VMCPUFlagSelector', {
>          field: 'Ext.form.field.Field',
>      },
>  
> +    config: {
> +        restrictToVMFlags: true,
> +    },
> +
>      disableSelection: true,
>      columnLines: false,
>      selectable: false,
> @@ -17,27 +37,18 @@ Ext.define('PVE.form.VMCPUFlagSelector', {
>      unknownFlags: [],
>  
>      store: {
> -        type: 'store',
> -        fields: ['name', { name: 'state', defaultValue: '=' }, 'description'],
> -        autoLoad: true,
> +        fields: ['name', { name: 'state', defaultValue: '=' }, 'description', 'supported-on'],
> +        autoLoad: false,
>          proxy: {
>              type: 'proxmox',
>              url: '/api2/json/nodes/localhost/capabilities/qemu/cpu-flags',
> +            extraParams: { accel: 'kvm' },
>          },
>          listeners: {
>              update: function () {
>                  this.commitChanges();
>              },
> -            refresh: function (store, eOpts) {
> -                let me = this;
> -                let view = me.view;
> -
> -                if (store.adjustedForValue !== view.value) {
> -                    view.adjustStoreForValue();
> -                }
> -            },
>          },
> -        adjustedForValue: undefined,
>      },
>  
>      getValue: function () {
> @@ -86,14 +97,18 @@ Ext.define('PVE.form.VMCPUFlagSelector', {
>              let rec = store.findRecord('name', flagName, 0, false, true, true);
>              if (rec !== null) {
>                  rec.set('state', sign);
> +                rec.commit();
>              } else {
>                  me.unknownFlags.push(flag);
>              }
>          });
>  
> -        store.adjustedForValue = value;
> +        me.getView().refresh();
> +    },
> +    isDirty: function () {
> +        let me = this;
> +        return me.originalValue !== me.getValue();
>      },
> -
>      setValue: function (value) {
>          let me = this;
>  
> @@ -109,6 +124,7 @@ Ext.define('PVE.form.VMCPUFlagSelector', {
>      },
>      columns: [
>          {
> +            text: gettext('State'),
>              dataIndex: 'state',
>              renderer: function (v) {
>                  switch (v) {
> @@ -125,6 +141,7 @@ Ext.define('PVE.form.VMCPUFlagSelector', {
>              width: 65,
>          },
>          {
> +            text: gettext('Set'),

This would benefit from a TRANSLATORS comment. Is this a noun (set as in
set theory)? is it a verb (to set)? From the point of view of
translators there is not enough context to decide.

>              xtype: 'widgetcolumn',
>              dataIndex: 'state',
>              width: 95,
> @@ -171,22 +188,96 @@ Ext.define('PVE.form.VMCPUFlagSelector', {
>              },
>          },
>          {
> +            text: gettext('Flag'),
>              dataIndex: 'name',
>              width: 100,
>          },
>          {
> +            text: gettext('Description'),
>              dataIndex: 'description',
> +            sortable: false,
> +            cellWrap: true,
> +            flex: 3,
> +        },
> +        {
> +            text: gettext('Supported On'),
> +            dataIndex: 'supported-on',
>              cellWrap: true,
>              flex: 1,
> +            renderer: (v) => (Array.isArray(v) ? v.join(', ') : ''),
>          },
>      ],
>  
>      initComponent: function () {
>          let me = this;
>  
> +        me.unknownFlags = [];
>          me.value = me.originalValue = '';
> -        me.store.view = me;
> +
> +        me.dockedItems = [
> +            {
> +                xtype: 'toolbar',
> +                dock: 'top',
> +                items: [
> +                    {
> +                        xtype: 'tbtext',
> +                        text: gettext('Acceleration:'),
> +                        autoEl: {
> +                            tag: 'span',
> +                            'data-qtip': gettext(
> +                                'A custom CPU model using acceleration-specific flags should only be assigned to VMs configured with the matching acceleration type, i.e., `kvm: off` for TCG, or `kvm: on` for KVM.',

I would recommend to use quotes `"`, single-quotes `'` in user-facing
strings. Backticks are a markup/markdown concept. See [1].

off-topic: In principle one should use “” rather than "", but I don't
think there is precedent for this the codebase.

[1] https://en.wikipedia.org/wiki/Quotation_mark#In_English

> +                            ),
> +                        },
> +                    },
> +                    {
> +                        xtype: 'radiogroup',
> +                        layout: 'hbox',
> +                        validateOnChange: false,
> +                        items: [
> +                            {
> +                                boxLabel: 'KVM',
> +                                inputValue: 'kvm',
> +                                name: 'accel',
> +                                checked: true,
> +                                isFormField: false,
> +                            },
> +                            {
> +                                boxLabel: 'TCG',
> +                                inputValue: 'tcg',
> +                                name: 'accel',
> +                                margin: '0 0 0 10',
> +                                isFormField: false,
> +                            },
> +                        ],
> +                        listeners: {
> +                            change: function (_, value) {
> +                                let view = this.up('grid');
> +                                let proxy = view.getStore().getProxy();
> +                                let accel = value.accel;
> +                                if (accel) {
> +                                    proxy.setExtraParam('accel', accel);
> +                                } else {
> +                                    delete proxy.extraParams.accel;
> +                                }
> +                                view.getStore().load();
> +                            },
> +                        },
> +                    },
> +                ],
> +            },
> +        ];
>  
>          me.callParent(arguments);
> +
> +        me.getStore().on('load', function (store, _, success) {
> +            if (success) {
> +                if (me.restrictToVMFlags) {
> +                    store.filterBy((rec) => VM_CPU_FLAGS_SUBSET[rec.get('name')] === true);
> +                }
> +                me.adjustStoreForValue();
> +                me.checkDirty();
> +            }
> +        });
> +        me.getStore().load();
>      },
>  });

-- 
Maximiliano