Re: [pve-devel] [PATCH pve-manager 2/3] fix #3967: enable ZFS dRAID creation in WebGUI

[Stefan Hrdlicka <s.hrdlicka@proxmox.com>]

On 6/3/22 14:24, Dominik Csapak wrote:
> comments inline
> 
> On 6/2/22 13:22, Stefan Hrdlicka wrote:
>> add fields for additional settings required by ZFS dRAID
>>
>> Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
>> ---
>> requires the changes in pve-storageto work
>>
>>   www/manager6/node/ZFS.js | 47 ++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 47 insertions(+)
>>
>> diff --git a/www/manager6/node/ZFS.js b/www/manager6/node/ZFS.js
>> index 5b3bdbda..5f3bbfef 100644
>> --- a/www/manager6/node/ZFS.js
>> +++ b/www/manager6/node/ZFS.js
>> @@ -42,6 +42,9 @@ Ext.define('PVE.node.CreateZFS', {
>>                   fieldLabel: gettext('RAID Level'),
>>                   name: 'raidlevel',
>>                   value: 'single',
>> +                listeners: {
>> +                change: 'onTargetChange',
>> +                },
>>                   comboItems: [
>>                   ['single', gettext('Single Disk')],
>>                   ['mirror', 'Mirror'],
>> @@ -49,8 +52,29 @@ Ext.define('PVE.node.CreateZFS', {
>>                   ['raidz', 'RAIDZ'],
>>                   ['raidz2', 'RAIDZ2'],
>>                   ['raidz3', 'RAIDZ3'],
>> +                ['draid', 'dRAID'],
>> +                ['draid2', 'dRAID2'],
>> +                ['draid3', 'dRAID3'],
>>                   ],
>>               },
>> +            {
>> +                xtype: 'proxmoxintegerfield',
>> +                fieldLabel: gettext('DRAID data devices'),
>> +                minValue: 1,
>> +                disabled: true,
>> +                hidden: true,
>> +                reference: 'draiddata',
>> +                name: 'draiddata',
>> +            },
>> +            {
>> +                xtype: 'proxmoxintegerfield',
>> +                fieldLabel: gettext('DRAID spares'),
>> +                minValue: 0,
>> +                disabled: true,
>> +                hidden: true,
>> +                reference: 'draidspares',
>> +                name: 'draidspares',
>> +            },
> 
> is that something someone always wants to configure?
> or more an advanced thing? in the latter case, i'd
> probably put them in the advanced options
> 

I would say you nearly always want to change at least the number of 
spares. The default values for creating a dRAID without setting anything 
are 0 spares and 8 devices per redundancy group (if disks >=8). The 
advantage of dRAID is fast rebuilding of a broken disk which you will 
only get (as far as I read) when you have spares configured.

The data devices per redundancy group would be possible to hide. But 
they have an impact on performance depending on the number chosen as 
well as storage usage. I'm not sure that it makes sense hiding one value 
of the two. I will think about it ;).


>>               {
>>                   xtype: 'proxmoxKVComboBox',
>>                   fieldLabel: gettext('Compression'),
>> @@ -101,6 +125,29 @@ Ext.define('PVE.node.CreateZFS', {
>>           me.callParent();
>>       },
>> +
>> +    controller: {
> 
> nit: normally we put the controller  on top of the class, not at the
> bottom
> 
>> +    xclass: 'Ext.app.ViewController',
>> +
>> +    onTargetChange: function(selection) {
>> +        var me = this;
>> +        var dataField = me.lookupReference('draiddata');
>> +        var sparesField = me.lookupReference('draidspares');
>> +        if (selection.value.startsWith("draid")) {
>> +        //show draid settings
>> +        dataField.setVisible(true);
>> +        dataField.setDisabled(false);
>> +        sparesField.setVisible(true);
>> +        sparesField.setDisabled(false);
>> +        } else {
>> +        //hide draid settings
>> +        dataField.setVisible(false);
>> +        dataField.setDisabled(true);
>> +        sparesField.setVisible(false);
>> +        sparesField.setDisabled(true);
>> +        }
> 
> this could be more elegantly solved in two other ways:
> 1. use a viewmodel with a formula that returns true/false
>     depending on the startsWith('draid') and then use a
>     bind on the hidden/disabled setting of the fields
> 
> 2. put the bool into a variable and use that, like this
> 
> let isDraid = ...startsWith('draid');
> dataField.setVisible(isDraid);
> dataField.setDisabled(!isDraid);
> ...
> 
> 
> 
>> +    },
>> +    },
>>   });
>>   Ext.define('PVE.node.ZFSList', {
> 
>