[pve-devel] [PATCH manager v4 0/3] fix 2515 use size defaults

[pve-devel] [PATCH manager v4 0/3] fix 2515 use size defaults

[Aaron Lauterer]

The main goal of this series is to improve the handling of configured
default size & min_size values when creating a new Ceph Pool in the GUI.

A new Ceph API endpoint, 'cfg/value', is added. It allows us to fetch
values for config keys that are set either in the config DB of Ceph or
in the ceph.conf file.

changes since
v3: rebased

v2:
* API rework has been already applied
* cleaned up JS code to set default values right where we get them from
  the API instead of at multiple places in the CephPoolInputPanel
  itself.

Aaron Lauterer (3):
  api: ceph: add endpoint to fetch config keys
  fix #2515: ui: ceph pool create: use configured defaults for size and
    min_size
  ui: ceph pool edit: rework with controller and formulas

 PVE/API2/Ceph/Cfg.pm      |  82 ++++++++++++++++++++++
 www/manager6/ceph/Pool.js | 144 +++++++++++++++++++++++++++++---------
 2 files changed, 191 insertions(+), 35 deletions(-)

-- 
2.39.2

[pve-devel] [PATCH manager v4 1/3] api: ceph: add endpoint to fetch config keys

[Aaron Lauterer]

This new endpoint allows to get the values of config keys that are
either set in the config db or the ceph.conf file.

Values that are set in the ceph.conf file have priority over values set
in the conifg db via 'ceph config set'.

Expects the --config-keys parameter as a semicolon separated list of
"<section>:<config key>" where the section is a section in the ceph.conf
or config db. For example: global:osd_pool_default_size

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
changes since
v3:
* rebased

v2:
* fixed small typo

v1:
* use kebab-case parameter names
* use kebab-case for the ceph config parameters, which also are returned
  that way
* improve how we parse and merge the config db and ceph.conf file. This
  way though, we dont warn if we cannot find a config key.
* renamed regex to make the distinctions clearer
* dropped 'format => string-list' as it didn't work when leaving out
  [;, ] from the regex. But we don't need both.

 PVE/API2/Ceph/Cfg.pm | 82 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/PVE/API2/Ceph/Cfg.pm b/PVE/API2/Ceph/Cfg.pm
index 2225a1ac..f06c42f4 100644
--- a/PVE/API2/Ceph/Cfg.pm
+++ b/PVE/API2/Ceph/Cfg.pm
@@ -4,6 +4,7 @@ use strict;
 use warnings;
 
 use PVE::Ceph::Tools;
+use PVE::Cluster qw(cfs_read_file);
 use PVE::JSONSchema qw(get_standard_option);
 use PVE::RADOS;
 use PVE::Tools qw(file_get_contents);
@@ -36,6 +37,7 @@ __PACKAGE__->register_method ({
 	my $result = [
 	    { name => 'raw' },
 	    { name => 'db' },
+	    { name => 'value' },
 	];
 
 	return $result;
@@ -110,3 +112,83 @@ __PACKAGE__->register_method ({
 
 	return $res;
     }});
+
+
+my $SINGLE_CONFIGKEY_RE = qr/[0-9a-z\-_\.]+:[0-9a-zA-Z\-_]+/i;
+my $CONFIGKEYS_RE = qr/^(:?${SINGLE_CONFIGKEY_RE})(:?[;, ]${SINGLE_CONFIGKEY_RE})*$/;
+
+__PACKAGE__->register_method ({
+    name => 'value',
+    path => 'value',
+    method => 'GET',
+    proxyto => 'node',
+    protected => 1,
+    permissions => {
+	check => ['perm', '/', [ 'Sys.Audit' ]],
+    },
+    description => "Get configured values from either the config file or config DB.",
+    parameters => {
+	additionalProperties => 0,
+	properties => {
+	    node => get_standard_option('pve-node'),
+	    'config-keys' => {
+		type => "string",
+		typetext => "<section>:<config key>[;<section>:<config key>]",
+		pattern => $CONFIGKEYS_RE,
+		description => "List of <section>:<config key> items.",
+	    }
+	},
+    },
+    returns => {
+	type => 'object',
+	description => "Contains {section}->{key} children with the values",
+    },
+    code => sub {
+	my ($param) = @_;
+
+	PVE::Ceph::Tools::check_ceph_inited();
+
+	# Ceph treats '-' and '_' the same in parameter names, stick with '-'
+	my $normalize = sub {
+	    my $t = shift;
+	    $t =~ s/_/-/g;
+	    return $t;
+	};
+
+	my $requested_keys = {};
+	for my $pair (PVE::Tools::split_list($param->{'config-keys'})) {
+	    my ($section, $key) = split(":", $pair);
+	    $section = $normalize->($section);
+	    $key = $normalize->($key);
+
+	    $requested_keys->{$section}->{$key} = 1;
+	}
+
+	my $config = {};
+
+	my $rados = PVE::RADOS->new();
+	my $configdb = $rados->mon_command( { prefix => 'config dump', format => 'json' });
+	for my $s (@{$configdb}) {
+	    my ($section, $name, $value) = $s->@{'section', 'name', 'value'};
+	    my $n_section = $normalize->($section);
+	    my $n_name = $normalize->($name);
+
+	    $config->{$n_section}->{$n_name} = $value
+		if defined $requested_keys->{$n_section} && $n_name eq $n_name;
+	}
+
+	# read ceph.conf after config db as it has priority if settings are present in both
+	my $config_file = cfs_read_file('ceph.conf'); # cfs_read_file to get it parsed
+	for my $section (keys %{$config_file}) {
+	    my $n_section = $normalize->($section);
+	    next if !defined $requested_keys->{$n_section};
+
+	    for my $key (keys %{$config_file->{$section}}) {
+		my $n_key = $normalize->($key);
+		$config->{$n_section}->{$n_key} = $config_file->{$section}->{$key}
+		    if $requested_keys->{$n_section}->{$n_key};
+	    }
+	}
+
+	return $config;
+    }});
-- 
2.39.2

[pve-devel] [PATCH manager v4 2/3] fix #2515: ui: ceph pool create: use configured defaults for size and min_size

[Aaron Lauterer]

Instead of hard coded defaults for the size and min_size parameter,
check if we have defaults configured in the ceph.conf or config db and
use those.

There are clusters where different defaults are needed. For example if
the cluster spans two rooms and needs to survive the loss of one. A
size/min_size of 4/2 are common defaults in such a situation.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
changes since
v3:
* rebased

v2:
* default 3/2 values for size/min_size are now set right when we get the
  API response. The component expects to get a value set via cbind.
  This avoids setting default values in multiple places within the
  component.

v1:
* set defaultSize and defaultMinSize to undefined in CephPoolInputPanel
* set them in cbindData in PoolEdit (needed when editing existing pool)
* waitMsgTarget when opening pool create window
* guard returned values for config keys in case they are empty

 www/manager6/ceph/Pool.js | 62 +++++++++++++++++++++++++++++++--------
 1 file changed, 50 insertions(+), 12 deletions(-)

diff --git a/www/manager6/ceph/Pool.js b/www/manager6/ceph/Pool.js
index 8de23ecf..0ad59baf 100644
--- a/www/manager6/ceph/Pool.js
+++ b/www/manager6/ceph/Pool.js
@@ -7,6 +7,10 @@ Ext.define('PVE.CephPoolInputPanel', {
     onlineHelp: 'pve_ceph_pools',
 
     subject: 'Ceph Pool',
+
+    defaultSize: undefined,
+    defaultMinSize: undefined,
+
     column1: [
 	{
 	    xtype: 'pmxDisplayEditField',
@@ -27,7 +31,9 @@ Ext.define('PVE.CephPoolInputPanel', {
 	    name: 'size',
 	    editConfig: {
 		xtype: 'proxmoxintegerfield',
-		value: 3,
+		cbind: {
+		    value: (get) => get('defaultSize'),
+		},
 		minValue: 2,
 		maxValue: 7,
 		allowBlank: false,
@@ -40,7 +46,6 @@ Ext.define('PVE.CephPoolInputPanel', {
 		    },
 		},
 	    },
-
 	},
     ],
     column2: [
@@ -78,9 +83,15 @@ Ext.define('PVE.CephPoolInputPanel', {
 	    xtype: 'proxmoxintegerfield',
 	    fieldLabel: gettext('Min. Size'),
 	    name: 'min_size',
-	    value: 2,
 	    cbind: {
-		minValue: (get) => get('isCreate') ? 2 : 1,
+		value: (get) => get('defaultMinSize'),
+		minValue: (get) => {
+		    if (Number(get('defaultMinSize')) === 1) {
+			return 1;
+		    } else {
+			return get('isCreate') ? 2 : 1;
+		    }
+		},
 	    },
 	    maxValue: 7,
 	    allowBlank: false,
@@ -195,6 +206,8 @@ Ext.define('PVE.Ceph.PoolEdit', {
     cbindData: {
 	pool_name: '',
 	isCreate: (cfg) => !cfg.pool_name,
+	defaultSize: undefined,
+	defaultMinSize: undefined,
     },
 
     cbind: {
@@ -217,6 +230,8 @@ Ext.define('PVE.Ceph.PoolEdit', {
 	    pool_name: '{pool_name}',
 	    isErasure: '{isErasure}',
 	    isCreate: '{isCreate}',
+	    defaultSize: '{defaultSize}',
+	    defaultMinSize: '{defaultMinSize}',
 	},
     }],
 });
@@ -397,14 +412,37 @@ Ext.define('PVE.node.Ceph.PoolList', {
 		{
 		    text: gettext('Create'),
 		    handler: function() {
-			Ext.create('PVE.Ceph.PoolEdit', {
-			    title: gettext('Create') + ': Ceph Pool',
-			    isCreate: true,
-			    isErasure: false,
-			    nodename: nodename,
-			    autoShow: true,
-			    listeners: {
-				destroy: () => rstore.load(),
+			let keys = [
+			    'global:osd-pool-default-min-size',
+			    'global:osd-pool-default-size',
+			];
+			let params = {
+			    'config-keys': keys.join(';'),
+			};
+
+			Proxmox.Utils.API2Request({
+			    url: '/nodes/localhost/ceph/cfg/value',
+			    method: 'GET',
+			    params,
+			    waitMsgTarget: me.getView(),
+			    failure: response => Ext.Msg.alert(gettext('Error'), response.htmlStatus),
+			    success: function({ result: { data } }) {
+				let global = data.global;
+				let defaultSize = global?.['osd-pool-default-size'] ?? 3;
+				let defaultMinSize = global?.['osd-pool-default-min-size'] ?? 2;
+
+				Ext.create('PVE.Ceph.PoolEdit', {
+				    title: gettext('Create') + ': Ceph Pool',
+				    isCreate: true,
+				    isErasure: false,
+				    defaultSize,
+				    defaultMinSize,
+				    nodename: nodename,
+				    autoShow: true,
+				    listeners: {
+					destroy: () => rstore.load(),
+				    },
+				});
 			    },
 			});
 		    },
-- 
2.39.2

[pve-devel] [PATCH manager v4 3/3] ui: ceph pool edit: rework with controller and formulas

[Aaron Lauterer]

instead of relying purely on listeners that then manually change other
components, we can use binds, formulas and a basic controller.

This makes it quite a bit easier to let multiple components react to
changes.

A cbind is used for the size component to set the initial start value.
Other options, like using setValue in the controller init, will trigger
the change listener and therefore can affect the min size without any
user interaction.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
changes since
v3:
* rebased

v2:
* don't set default values in controller init() as we expect them to
  always be set via cbinds.

v1:
* moved view between controller and layout
* small code style cleanups

 www/manager6/ceph/Pool.js | 82 ++++++++++++++++++++++++++++-----------
 1 file changed, 59 insertions(+), 23 deletions(-)

diff --git a/www/manager6/ceph/Pool.js b/www/manager6/ceph/Pool.js
index 0ad59baf..c61d4f71 100644
--- a/www/manager6/ceph/Pool.js
+++ b/www/manager6/ceph/Pool.js
@@ -11,6 +11,48 @@ Ext.define('PVE.CephPoolInputPanel', {
     defaultSize: undefined,
     defaultMinSize: undefined,
 
+    controller: {
+	xclass: 'Ext.app.ViewController',
+
+	init: function(view) {
+	    let vm = this.getViewModel();
+	    vm.set('size', Number(view.defaultSize));
+	    vm.set('minSize', Number(view.defaultMinSize));
+	},
+	sizeChange: function(field, val) {
+	    let vm = this.getViewModel();
+	    let minSize = Math.round(val / 2);
+	    if (minSize > 1) {
+		vm.set('minSize', minSize);
+	    }
+	    vm.set('size', val); // bind does not work in a pmxDisplayEditField, update manually
+	},
+    },
+
+    viewModel: {
+	data: {
+	    minSize: null,
+	    size: null,
+	},
+	formulas: {
+	    minSizeLabel: (get) => {
+		if (get('showMinSizeOneWarning') || get('showMinSizeHalfWarning')) {
+		    return `${gettext('Min. Size')} <i class="fa fa-exclamation-triangle warning"></i>`;
+		}
+		return gettext('Min. Size');
+	    },
+	    showMinSizeOneWarning: (get) => get('minSize') === 1,
+	    showMinSizeHalfWarning: (get) => {
+		let minSize = get('minSize');
+		let size = get('size');
+		if (minSize === 1) {
+		    return false;
+		}
+		return minSize < (size / 2) && minSize !== size;
+	    },
+	},
+    },
+
     column1: [
 	{
 	    xtype: 'pmxDisplayEditField',
@@ -38,12 +80,7 @@ Ext.define('PVE.CephPoolInputPanel', {
 		maxValue: 7,
 		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);
-			}
-		    },
+		    change: 'sizeChange',
 		},
 	    },
 	},
@@ -81,7 +118,10 @@ Ext.define('PVE.CephPoolInputPanel', {
     advancedColumn1: [
 	{
 	    xtype: 'proxmoxintegerfield',
-	    fieldLabel: gettext('Min. Size'),
+	    bind: {
+		fieldLabel: '{minSizeLabel}',
+		value: '{minSize}',
+	    },
 	    name: 'min_size',
 	    cbind: {
 		value: (get) => get('defaultMinSize'),
@@ -95,28 +135,24 @@ Ext.define('PVE.CephPoolInputPanel', {
 	    },
 	    maxValue: 7,
 	    allowBlank: false,
-	    listeners: {
-		change: function(field, minSize) {
-		    let panel = field.up('inputpanel');
-		    let size = panel.down('field[name=size]').getValue();
-
-		    let showWarning = minSize < (size / 2) && minSize !== size;
-
-		    let fieldLabel = gettext('Min. Size');
-		    if (showWarning) {
-			fieldLabel = gettext('Min. Size') + ' <i class="fa fa-exclamation-triangle warning"></i>';
-		    }
-		    panel.down('field[name=min_size-warning]').setHidden(!showWarning);
-		    field.setFieldLabel(fieldLabel);
-		},
-	    },
 	},
 	{
 	    xtype: 'displayfield',
-	    name: 'min_size-warning',
+	    bind: {
+		hidden: '{!showMinSizeHalfWarning}',
+	    },
+	    hidden: true,
 	    userCls: 'pmx-hint',
 	    value: gettext('min_size < size/2 can lead to data loss, incomplete PGs or unfound objects.'),
+	},
+	{
+	    xtype: 'displayfield',
+	    bind: {
+		hidden: '{!showMinSizeOneWarning}',
+	    },
 	    hidden: true,
+	    userCls: 'pmx-hint',
+	    value: gettext('a min_size of 1 is not recommended and can lead to data loss'),
 	},
 	{
 	    xtype: 'pmxDisplayEditField',
-- 
2.39.2

Re: [pve-devel] [PATCH manager v4 0/3] fix 2515 use size defaults

[Maximiliano Sandoval]

Tested on a 3-node cluster.

Set `osd_pool_default_min_size` and `osd_pool_default_size` on
ceph.confg to min_size=1 size=2, created a pool using the web UI and it
has the correct size after creation.

One things I noticed is that if you set them to, for example min_size=3,
size=2, no errors will be displayed in the web UI until the creation
fails with:

    Could not set: min_size, size

I am not sure if this should be catched a bit earlier in the web UI, in
the same way we catch size=1 and prevent the creation of the pool. It is
an invalid configuration file after all.


Tested `ceph config set X value`. The values in ceph.conf indeed take
priority as per the commit message. When the values are not present in
ceph.conf, the values from the config DB take preference over the 3/2
default as expected.

Tested-by: Maximiliano Sandoval <m.sandoval@proxmox.com>

Aaron Lauterer <a.lauterer@proxmox.com> writes:

> The main goal of this series is to improve the handling of configured
> default size & min_size values when creating a new Ceph Pool in the GUI.
>
> A new Ceph API endpoint, 'cfg/value', is added. It allows us to fetch
> values for config keys that are set either in the config DB of Ceph or
> in the ceph.conf file.
>
> changes since
> v3: rebased
>
> v2:
> * API rework has been already applied
> * cleaned up JS code to set default values right where we get them from
>   the API instead of at multiple places in the CephPoolInputPanel
>   itself.
>
> Aaron Lauterer (3):
>   api: ceph: add endpoint to fetch config keys
>   fix #2515: ui: ceph pool create: use configured defaults for size and
>     min_size
>   ui: ceph pool edit: rework with controller and formulas
>
>  PVE/API2/Ceph/Cfg.pm      |  82 ++++++++++++++++++++++
>  www/manager6/ceph/Pool.js | 144 +++++++++++++++++++++++++++++---------
>  2 files changed, 191 insertions(+), 35 deletions(-)


--
Maximiliano

[pve-devel] applied: [PATCH manager v4 0/3] fix 2515 use size defaults

[Thomas Lamprecht]

Am 29/09/2023 um 15:02 schrieb Aaron Lauterer:
> The main goal of this series is to improve the handling of configured
> default size & min_size values when creating a new Ceph Pool in the GUI.
> 
> A new Ceph API endpoint, 'cfg/value', is added. It allows us to fetch
> values for config keys that are set either in the config DB of Ceph or
> in the ceph.conf file.
> 
> changes since
> v3: rebased
> 
> v2:
> * API rework has been already applied
> * cleaned up JS code to set default values right where we get them from
>   the API instead of at multiple places in the CephPoolInputPanel
>   itself.
> 
> Aaron Lauterer (3):
>   api: ceph: add endpoint to fetch config keys
>   fix #2515: ui: ceph pool create: use configured defaults for size and
>     min_size
>   ui: ceph pool edit: rework with controller and formulas
> 
>  PVE/API2/Ceph/Cfg.pm      |  82 ++++++++++++++++++++++
>  www/manager6/ceph/Pool.js | 144 +++++++++++++++++++++++++++++---------
>  2 files changed, 191 insertions(+), 35 deletions(-)
> 


applied series with Maximilano's T-b, thanks!