Re: [pve-devel] [PATCH manager v2 5/8] ceph: gui: rework pool input panel

[Dominik Csapak <d.csapak@proxmox.com>]

seems not to work to set the target_size(_bytes) via gui (cli works)

comments inline

On 11/24/20 11:58 AM, Alwin Antreich wrote:
> * add the ability to edit an existing pool
> * allow adjustment of autoscale settings
> * warn if user specifies min_size 1
> * disallow min_size 1 on pool create
> * calculate min_size replica by size
> 
> Signed-off-by: Alwin Antreich <a.antreich@proxmox.com>
> ---
>   www/manager6/ceph/Pool.js | 276 ++++++++++++++++++++++++++++++--------
>   1 file changed, 221 insertions(+), 55 deletions(-)
> 
> diff --git a/www/manager6/ceph/Pool.js b/www/manager6/ceph/Pool.js
> index 6febe1fc..35ecbf09 100644
> --- a/www/manager6/ceph/Pool.js
> +++ b/www/manager6/ceph/Pool.js
> @@ -1,94 +1,237 @@
> -Ext.define('PVE.CephCreatePool', {
> -    extend: 'Proxmox.window.Edit',
> -    alias: 'widget.pveCephCreatePool',
> +Ext.define('PVE.CephPoolInputPanel', {
> +    extend: 'Proxmox.panel.InputPanel',
> +    xtype: 'pveCephPoolInputPanel',
> +    mixins: ['Proxmox.Mixin.CBind'],
>   
>       showProgress: true,
>       onlineHelp: 'pve_ceph_pools',
>   
>       subject: 'Ceph Pool',
> -    isCreate: true,
> -    method: 'POST',
> -    items: [
> +    column1: [
>   	{
> -	    xtype: 'textfield',
> +	    xtype: 'pmxDisplayEditField',
>   	    fieldLabel: gettext('Name'),
> +	    cbind: {
> +		editable: '{isCreate}',
> +		value: '{pool_name}',
> +		disabled: '{!isCreate}'
> +	    },
>   	    name: 'name',
> +	    labelWidth: 80,
>   	    allowBlank: false
>   	},
>   	{
>   	    xtype: 'proxmoxintegerfield',
>   	    fieldLabel: gettext('Size'),
>   	    name: 'size',
> +	    labelWidth: 80,
>   	    value: 3,
> -	    minValue: 1,
> +	    minValue: 2,
>   	    maxValue: 7,
> -	    allowBlank: false
> +	    allowBlank: false,
> +	    listeners: {
> +		change: function(field, val) {
> +		    let size = Math.round(val / 2);
> +		    if (size > 1) {
> +			field.up('inputpanel').down('field[name=min_size]').setValue(size);
> +		    }
> +		},
> +	    },
> +	},
> +    ],
> +    column2: [
> +	{
> +	    xtype: 'proxmoxKVComboBox',
> +	    fieldLabel: 'PG Autoscale ' + gettext('Mode'),

like the previous patch, please combine the gettext (and optimally use 
the same text as in the columns)

> +	    name: 'pg_autoscale_mode',
> +	    comboItems: [
> +		['warn', 'warn'],
> +		['on', 'on'],
> +		['off', 'off'],
> +	    ],
> +	    value: 'warn',
> +	    allowBlank: false,
> +	    autoSelect: false,
> +	    labelWidth: 140,
> +	},
> +	{
> +	    xtype: 'proxmoxcheckbox',
> +	    fieldLabel: gettext('Add as Storage'),
> +	    cbind: {
> +		value: '{isCreate}',
> +		hidden: '{!isCreate}',

i guess you'd need to disable it also on !isCreate so that the value 
does not get submitted.. (for safety)

> +	    },
> +	    name: 'add_storages',
> +	    labelWidth: 140,
> +	    autoEl: {
> +		tag: 'div',
> +		'data-qtip': gettext('Add the new pool to the cluster storage configuration.'),
> +	    },
>   	},
> +    ],
> +    advancedColumn1: [
>   	{
>   	    xtype: 'proxmoxintegerfield',
>   	    fieldLabel: gettext('Min. Size'),
>   	    name: 'min_size',
> +	    labelWidth: 80,
>   	    value: 2,
> -	    minValue: 1,
> +	    cbind: {
> +		minValue: (get) => get('isCreate') ? 2 : 1,
> +	    },
>   	    maxValue: 7,
> -	    allowBlank: false
> +	    allowBlank: false,
> +	    listeners: {
> +		change: function(field, val) {
> +		    let warn = true;
> +		    let warn_text = gettext('Min. Size');
> +
> +		    if (val < 2) {
> +			warn = false;
> +			warn_text = gettext('Min. Size') + ' <i class="fa fa-exclamation-triangle warning"></i>';
> +		    }
> +
> +		    field.up().down('field[name=min_size-warning]').setHidden(warn);

please reverse the logic of 'warn' and use setHidden(!warn) or
use setVisible(warn), it's confusing otherwise

> +		    field.setFieldLabel(warn_text);
> +		}
> +	    },
> +	},
> +	{
> +	    xtype: 'displayfield',
> +	    name: 'min_size-warning',
> +	    padding: 5,
> +	    userCls: 'pmx-hint',
> +	    value: 'A pool with min_size=1 could lead to data loss, incomplete PGs or unfound objects.',
> +	    hidden: true,
>   	},
>   	{
>   	    xtype: 'pveCephRuleSelector',
>   	    fieldLabel: 'Crush Rule', // do not localize
> +	    cbind: { nodename: '{nodename}' },
> +	    labelWidth: 80,
>   	    name: 'crush_rule',
>   	    allowBlank: false
>   	},
> -	{
> -	    xtype: 'proxmoxKVComboBox',
> -	    fieldLabel: 'PG Autoscale Mode', // do not localize
> -	    name: 'pg_autoscale_mode',
> -	    comboItems: [
> -		['warn', 'warn'],
> -		['on', 'on'],
> -		['off', 'off'],
> -	    ],
> -	    value: 'warn',
> -	    allowBlank: false,
> -	    autoSelect: false,
> -	},
> +    ],
> +    advancedColumn2: [
>   	{
>   	    xtype: 'proxmoxintegerfield',
> -	    fieldLabel: 'pg_num',
> +	    fieldLabel: '# of PGs',
>   	    name: 'pg_num',
> +	    labelWidth: 120,
>   	    value: 128,
> -	    minValue: 8,
> +	    minValue: 1,
>   	    maxValue: 32768,
> +	    allowBlank: false,
> +	    emptyText: 128,
> +	},
> +	{
> +	    xtype: 'numberfield',
> +	    fieldLabel: gettext('Target Size') + ' (GiB)',
> +	    name: 'target_size',
> +	    labelWidth: 120,
> +	    value: 0,
> +	    minValue: 0,
>   	    allowBlank: true,
> -	    emptyText: gettext('Autoscale'),
> +	    emptyText: '0.0',
> +	    disabled: false,
> +	    listeners: {
> +		change: function(field, val) {
> +		    let ratio = field.up().down('field[name=target_size_ratio]').getValue() > +		    if ( ratio === null) {

spacing

> +			field.up().down('field[name=target_size_ratio]').setDisabled(val);
> +		    }
> +		}
> +	    },
>   	},
>   	{
> -	    xtype: 'proxmoxcheckbox',
> -	    fieldLabel: gettext('Add as Storage'),
> -	    value: true,
> -	    name: 'add_storages',
> +	    xtype: 'numberfield',
> +	    fieldLabel: gettext('Target Size Ratio'),
> +	    name: 'target_size_ratio',
> +	    labelWidth: 120,
> +	    value: 0,
> +	    minValue: 0,
> +	    allowBlank: true,
> +	    emptyText: '0.0',
> +	    disabled: false,
> +	    listeners: {
> +		change: function(field, val) {
> +		    field.up().down('field[name=target_size]').setDisabled(val);
> +		}

i find that disabling the other field on change very weird ux
i'd rather use a selector or radio button for the mode...

also if you already have the emptytext you do not need to set
the default value, that makes the input even weirder

> +	    },
>   	    autoEl: {
>   		tag: 'div',
> -		 'data-qtip': gettext('Add the new pool to the cluster storage configuration.'),
> +		'data-qtip': gettext('If Target Size Ratio is set it takes precedence over Target Size.'),
>   	    },
> -	}
> +	},
>       ],
> -    initComponent : function() {
> -        var me = this;
>   
> -	if (!me.nodename) {
> -	    throw "no node name specified";
> +    onGetValues: function(values) {
> +	Object.keys(values || {}).forEach(function(name) {
> +	    if (values[name] === '') {
> +		delete values[name];
> +	    }
> +	});
> +
> +	if (values['target_size'] && values['target_size'] !== 0) {
> +	    values['target_size'] = values['target_size']*1024*1024*1024;
> +	} else {
> +	    // needs to be 0 to be cleared
> +	    values['target_size'] = 0;
>   	}
>   
> -        Ext.apply(me, {
> -	    url: "/nodes/" + me.nodename + "/ceph/pools",
> -	    defaults: {
> -		nodename: me.nodename
> -	    }
> -        });
> +	// needs to be 0 to be cleared
> +	if (!values['target_size_ratio']) {
> +	    values['target_size_ratio'] = 0;
> +	}

i think this is the reason it does not work (set to 0 instead of delete)

>   
> -        me.callParent();
> -    }
> +	return values;
> +    },
> +
> +    setValues: function(values) {
> +	if (values['target_size'] && values['target_size'] !== 0) {
> +	    values['target_size'] = values['target_size']/1024/1024/1024;
> +	}
> +
> +	this.callParent([values]);
> +    },
> +
> +    initComponent: function() {
> +	let me = this;
> +	me.callParent();
> +    },

you can drop the fucntion if it only calls callParent

> +
> +});
> +
> +Ext.define('PVE.CephPoolEdit', {
> +    extend: 'Proxmox.window.Edit',
> +    alias: 'widget.pveCephPoolEdit',
> +    xtype: 'pveCephPoolEdit',
> +    mixins: ['Proxmox.Mixin.CBind'],
> +
> +    cbindData: {
> +	pool_name: '',
> +	isCreate: (cfg) => !cfg.pool_name,
> +    },
> +
> +    cbind: {
> +	autoLoad: get => !get('isCreate'),
> +	url: get => get('isCreate') ?
> +	`/nodes/${get('nodename')}/ceph/pools` :
> +	`/nodes/${get('nodename')}/ceph/pools/${get('pool_name')}`,
> +	method: get => get('isCreate') ? 'POST' : 'PUT',
> +    },
> +
> +    subject: gettext('Ceph Pool'),
> +
> +    items: [{
> +	xtype: 'pveCephPoolInputPanel',
> +	cbind: {
> +	    nodename: '{nodename}',
> +	    pool_name: '{pool_name}',
> +	    isCreate: '{isCreate}',
> +	},
> +    }],
>   });
>   
>   Ext.define('PVE.node.CephPoolList', {
> @@ -214,6 +357,9 @@ Ext.define('PVE.node.CephPoolList', {
>   	});
>   
>   	var store = Ext.create('Proxmox.data.DiffStore', { rstore: rstore });
> +	var reload = function() {
> +	    rstore.load();
> +	};
>   
>   	var regex = new RegExp("not (installed|initialized)", "i");
>   	PVE.Utils.handleStoreErrorOrMask(me, rstore, regex, function(me, error){
> @@ -230,16 +376,37 @@ Ext.define('PVE.node.CephPoolList', {
>   	var create_btn = new Ext.Button({
>   	    text: gettext('Create'),
>   	    handler: function() {
> -		var win = Ext.create('PVE.CephCreatePool', {
> -                    nodename: nodename
> +		var win = Ext.create('PVE.CephPoolEdit', {
> +		    title: gettext('Create') + ': Ceph Pool',
> +		    nodename: nodename,
>   		});
>   		win.show();
> -		win.on('destroy', function() {
> -		    rstore.load();
> -		});
> +		win.on('destroy', reload);
>   	    }
>   	});
>   
> +	var run_editor = function() {
> +	    var rec = sm.getSelection()[0];
> +	    if (!rec) {
> +		return;
> +	    }
> +
> +	    var win = Ext.create('PVE.CephPoolEdit', {
> +		title: gettext('Edit') + ': Ceph Pool',
> +		nodename: nodename,
> +		pool_name: rec.data.pool_name,
> +	    });
> +            win.on('destroy', reload);
> +            win.show();
> +	};
> +
> +	var edit_btn = new Proxmox.button.Button({
> +	    text: gettext('Edit'),
> +	    disabled: true,
> +	    selModel: sm,
> +	    handler: run_editor,
> +	});
> +
>   	var destroy_btn = Ext.create('Proxmox.button.Button', {
>   	    text: gettext('Destroy'),
>   	    selModel: sm,
> @@ -261,19 +428,18 @@ Ext.define('PVE.node.CephPoolList', {
>   		    },
>   		    item: { type: 'CephPool', id: rec.data.pool_name }
>   		}).show();
> -		win.on('destroy', function() {
> -		    rstore.load();
> -		});
> +		win.on('destroy', reload);
>   	    }
>   	});
>   
>   	Ext.apply(me, {
>   	    store: store,
>   	    selModel: sm,
> -	    tbar: [ create_btn, destroy_btn ],
> +	    tbar: [ create_btn, edit_btn, destroy_btn ],
>   	    listeners: {
>   		activate: () => rstore.startUpdate(),
>   		destroy: () => rstore.stopUpdate(),
> +		itemdblclick: run_editor,
>   	    }
>   	});
>   
> @@ -295,7 +461,7 @@ Ext.define('PVE.node.CephPoolList', {
>   		  { name: 'pg_autoscale_mode', type: 'string'},
>   		  { name: 'pg_num_final', type: 'integer'},
>   		  { name: 'target_size_ratio', type: 'number'},
> -		  { name: 'target_size_bytes', type: 'integer'},
> +		  { name: 'target_size', type: 'integer'},
>   		],
>   	idProperty: 'pool_name'
>       });
>