[pve-devel] [PATCH pve-manager] First beta of FreeNAS storage plugin

Dominik Csapak d.csapak at proxmox.com
Mon Jun 12 14:18:59 CEST 2017


i just gave this patch a quick glance, here a few remarks:

a bit nitpicky, but your indentation needs fixing, the FreeNASEdit.js 
seems mostly ok, but nearly every other file you touched has wrong 
indentation

other comments inline

On 06/12/2017 03:19 AM, Michael Rasmussen wrote:
> Signed-off-by: Michael Rasmussen <mir at datanom.net>
> ---
>  www/manager6/Makefile               |   1 +
>  www/manager6/Utils.js               |   4 +-
>  www/manager6/dc/StorageView.js      |  15 +++-
>  www/manager6/lxc/ResourceEdit.js    |   6 +-
>  www/manager6/qemu/Clone.js          |   3 +-
>  www/manager6/qemu/HDEdit.js         |   3 +-
>  www/manager6/qemu/HDEfi.js          |   2 +-
>  www/manager6/qemu/HDMove.js         |   3 +-
>  www/manager6/storage/FreeNASEdit.js | 152 ++++++++++++++++++++++++++++++++++++
>  9 files changed, 180 insertions(+), 9 deletions(-)
>  create mode 100644 www/manager6/storage/FreeNASEdit.js
>
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index 0c266ef..2a4489d 100644
> --- a/www/manager6/Makefile
> +++ b/www/manager6/Makefile
> @@ -186,6 +186,7 @@ JSSRC= 				                 	\
>  	storage/SheepdogEdit.js				\
>  	storage/ZFSEdit.js				\
>  	storage/ZFSPoolEdit.js				\
> +	storage/FreeNASEdit.js			\
>  	ha/StatusView.js				\
>  	ha/Status.js					\
>  	ha/GroupSelector.js				\
> diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
> index d1ada77..c6227d0 100644
> --- a/www/manager6/Utils.js
> +++ b/www/manager6/Utils.js
> @@ -799,7 +799,9 @@ Ext.define('PVE.Utils', { utilities: {
>  	    return 'iSCSIDirect';
>  	} else if (value === 'drbd') {
>  	    return 'DRBD';
> -	} else {
> +	} else if (value === 'freenas') {
> +        return 'FreeNAS';
> +    } else {
>  	    return PVE.Utils.unknownText;
>  	}
>      },
> diff --git a/www/manager6/dc/StorageView.js b/www/manager6/dc/StorageView.js
> index 2940a98..c16d464 100644
> --- a/www/manager6/dc/StorageView.js
> +++ b/www/manager6/dc/StorageView.js
> @@ -58,7 +58,9 @@ Ext.define('PVE.dc.StorageView', {
>  		editor = 'PVE.storage.ZFSEdit';
>  	    } else if (type === 'zfspool') {
>  		editor = 'PVE.storage.ZFSPoolEdit';
> -	    } else {
> +	    } else if (type === 'freenas') {
> +        editor = 'PVE.storage.FreeNASEdit';
> +        } else {
>  		return;
>  	    }
>  	    var win = Ext.create(editor, {
> @@ -191,7 +193,16 @@ Ext.define('PVE.dc.StorageView', {
>                                      win.on('destroy', reload);
>                                      win.show();
>                                  }
> -                            }
> +                            },
> +                {
> +                    text: PVE.Utils.format_storage_type('freenas'),
> +                    iconCls: 'fa fa-fw fa-building',
> +                    handler: function() {
> +                        var win = Ext.create('PVE.storage.FreeNASEdit', {});
> +                        win.on('destroy', reload);
> +                        win.show();
> +                    }
> +                }
>
>  /* the following type are conidered unstable
>   * so we do not enable that on the GUI for now
> diff --git a/www/manager6/lxc/ResourceEdit.js b/www/manager6/lxc/ResourceEdit.js
> index 61a5539..8bc3e76 100644
> --- a/www/manager6/lxc/ResourceEdit.js
> +++ b/www/manager6/lxc/ResourceEdit.js
> @@ -387,7 +387,8 @@ Ext.define('PVE.lxc.MountPointInputPanel', {
>  		    }
>  		    var rec = f.store.getById(value);
>  		    if (rec.data.type === 'zfs' ||
> -		        rec.data.type === 'zfspool') {
> +		        rec.data.type === 'zfspool' ||
> +                rec.data.type === 'freenas') {
>  			me.quota.setDisabled(true);
>  			me.quota.setValue(false);
>  		    } else {
> @@ -407,7 +408,8 @@ Ext.define('PVE.lxc.MountPointInputPanel', {
>  			       rec.data.type === 'rbd' ||
>  			       rec.data.type === 'sheepdog' ||
>  			       rec.data.type === 'zfs' ||
> -			       rec.data.type === 'zfspool') {
> +			       rec.data.type === 'zfspool' ||
> +                   rec.data.type === 'freenas') {
>  			me.hdfilesel.setDisabled(true);
>  			me.hdfilesel.setVisible(false);
>  			me.hdsizesel.setDisabled(false);
> diff --git a/www/manager6/qemu/Clone.js b/www/manager6/qemu/Clone.js
> index c5dad4d..9de9777 100644
> --- a/www/manager6/qemu/Clone.js
> +++ b/www/manager6/qemu/Clone.js
> @@ -116,7 +116,8 @@ Ext.define('PVE.window.Clone', {
>  		rec.data.type === 'iscsi' ||
>  		rec.data.type === 'sheepdog' ||
>  		rec.data.type === 'zfs' ||
> -		rec.data.type === 'zfspool'
> +		rec.data.type === 'zfspool' ||
> +        rec.data.type === 'freenas'
>  	) {
>  	    formatsel.setValue('raw');
>  	    formatsel.setDisabled(true);
> diff --git a/www/manager6/qemu/HDEdit.js b/www/manager6/qemu/HDEdit.js
> index 0060394..6920c0d 100644
> --- a/www/manager6/qemu/HDEdit.js
> +++ b/www/manager6/qemu/HDEdit.js
> @@ -46,7 +46,8 @@ Ext.define('PVE.qemu.HDInputPanel', {
>  			       rec.data.type === 'rbd' ||
>  			       rec.data.type === 'sheepdog' ||
>  			       rec.data.type === 'zfs' ||
> -			       rec.data.type === 'zfspool') {
> +			       rec.data.type === 'zfspool' ||
> +                   rec.data.type === 'freenas') {
>  			me.hdfilesel.setDisabled(true);
>  			me.hdfilesel.setVisible(false);
>  			me.formatsel.setValue('raw');
> diff --git a/www/manager6/qemu/HDEfi.js b/www/manager6/qemu/HDEfi.js
> index 45c9d70..7aa9608 100644
> --- a/www/manager6/qemu/HDEfi.js
> +++ b/www/manager6/qemu/HDEfi.js
> @@ -21,7 +21,7 @@ Ext.define('PVE.qemu.EFIDiskInputPanel', {
>  		    var me = this.getView();
>  		    var rec = f.store.getById(value);
>
> -		    var rawArray = ['iscsi', 'lvm', 'lvmthin', 'drbd', 'rbd', 'sheepdog', 'zfs', 'zfspool'];
> +		    var rawArray = ['iscsi', 'lvm', 'lvmthin', 'drbd', 'rbd', 'sheepdog', 'zfs', 'zfspool', 'freenas'];
>
>  		    me.hdfilesel.setDisabled(true);
>  		    me.hdfilesel.setVisible(false);
> diff --git a/www/manager6/qemu/HDMove.js b/www/manager6/qemu/HDMove.js
> index 745c791..4e69aa7 100644
> --- a/www/manager6/qemu/HDMove.js
> +++ b/www/manager6/qemu/HDMove.js
> @@ -80,7 +80,8 @@ Ext.define('PVE.window.HDMove', {
>  			    rec.data.type === 'rbd' ||
>  			    rec.data.type === 'sheepdog' ||
>  			    rec.data.type === 'zfs' ||
> -			    rec.data.type === 'zfspool'
> +			    rec.data.type === 'zfspool' ||
> +                rec.data.type === 'freenas'
>                          ) {
>                              me.formatsel.setValue('raw');
>                              me.formatsel.setDisabled(true);
> diff --git a/www/manager6/storage/FreeNASEdit.js b/www/manager6/storage/FreeNASEdit.js
> new file mode 100644
> index 0000000..5cc9f28
> --- /dev/null
> +++ b/www/manager6/storage/FreeNASEdit.js
> @@ -0,0 +1,152 @@
> +Ext.define('PVE.storage.FreeNASInputPanel', {
> +    extend: 'PVE.panel.InputPanel',
> +
> +    onGetValues: function(values) {
> +	var me = this;
> +
> +	if (me.isCreate) {
> +	    values.type = 'freenas';
> +	    values.content = 'images';

seems 'rootdir' is missing, this way you can never create containers there?

> +	} else {
> +	    delete values.storage;
> +	}
> +
> +	values.disable = values.enable ? 0 : 1;
> +	delete values.enable;
> +
> +	return values;
> +    },
> +
> +    initComponent : function() {
> +	var me = this;
> +
> +	me.column1 = [
> +	    {
> +		xtype: me.isCreate ? 'textfield' : 'displayfield',
> +		name: 'storage',
> +		value: me.storageId || '',
> +		fieldLabel: 'ID',
> +		vtype: 'StorageId',
> +		allowBlank: false
> +	    },
> +	    {
> +		xtype: me.isCreate ? 'textfield' : 'displayfield',
> +		name: 'portal',
> +		value: '',
> +		fieldLabel: gettext('Portal'),
> +		allowBlank: false
> +	    },
> +	    {
> +		xtype: me.isCreate ? 'textfield' : 'displayfield',
> +		name: 'pool',
> +		value: '',
> +		fieldLabel: gettext('Pool'),
> +		allowBlank: false
> +	    },
> +	    {
> +		xtype: me.isCreate ? 'textfield' : 'displayfield',
> +		name: 'portal_group',
> +		value: '',
> +		fieldLabel: gettext('Portal Group ID'),
> +		allowBlank: false
> +	    },
> +	    {
> +		xtype: me.isCreate ? 'textfield' : 'displayfield',
> +		name: 'initiator_group',
> +		value: '',
> +		fieldLabel: gettext('Initiator Group ID'),
> +		allowBlank: false
> +	    },

this trailing comma is an error in jslint

> +	];
> +
> +	me.column2 = [
> +	    {
> +		xtype: 'pvecheckbox',
> +		name: 'enable',
> +		checked: true,
> +		uncheckedValue: 0,
> +		fieldLabel: gettext('Enable')
> +	    },
> +/*        {
> +        xtype: 'textfield',
> +        name: 'blocksize',
> +        emptyText: '8k',
> +        fieldLabel: gettext('Block Size'),
> +        allowBlank: true
> +        },*/

is this part commented out for a reason? if yes please add it,
otherwise, please remove the code

> +	    {
> +		xtype: 'textfield',
> +		name: 'username',
> +		emptyText: 'root',
> +		fieldLabel: gettext('Username'),
> +		allowBlank: false
> +	    },
> +	    {
> +		xtype: 'textfield',
> +		name: 'password',
> +		emptyText: '',
> +        inputType: 'password',
> +		fieldLabel: gettext('Password'),
> +		allowBlank: false
> +	    },

not really a gui problem, but it is probably a bad idea to save/load 
passwords from/to a textfield, at least do not load and insert it in the 
field, but leave it empty

also, again a trailing comma

> +	];
> +
> +	if (me.isCreate || me.storageId !== 'local') {
> +	    me.column2.unshift({
> +		xtype: 'pveNodeSelector',
> +		name: 'nodes',
> +		fieldLabel: gettext('Nodes'),
> +		emptyText: gettext('All') + ' (' +
> +		    gettext('No restrictions') +')',
> +		multiSelect: true,
> +		autoSelect: false
> +	    });
> +	}
> +
> +	me.callParent();
> +    }
> +});
> +
> +Ext.define('PVE.storage.FreeNASEdit', {
> +    extend: 'PVE.window.Edit',
> +
> +    initComponent : function() {
> +	var me = this;
> +
> +	me.isCreate = !me.storageId;
> +
> +	if (me.isCreate) {
> +            me.url = '/api2/extjs/storage';
> +            me.method = 'POST';
> +        } else {
> +            me.url = '/api2/extjs/storage/' + me.storageId;
> +            me.method = 'PUT';
> +        }
> +
> +	var ipanel = Ext.create('PVE.storage.FreeNASInputPanel', {
> +	    isCreate: me.isCreate,
> +	    storageId: me.storageId
> +	});
> +
> +	Ext.apply(me, {
> +            subject: 'FreeNAS Storage',

you add the storage type to format_storage_type, but here you do not use 
it, is there a reason?

> +	    isAdd: true,
> +	    items: [ ipanel ]
> +	});
> +
> +	me.callParent();
> +
> +        if (!me.isCreate) {
> +            me.load({
> +                success:  function(response, options) {
> +                    var values = response.result.data;
> +                    if (values.nodes) {
> +                        values.nodes = values.nodes.split(',');
> +                    }
> +                    values.enable = values.disable ? 0 : 1;
> +                    ipanel.setValues(values);
> +                }
> +            });
> +        }
> +    }
> +});
>





More information about the pve-devel mailing list