* [pve-devel] [PATCH manager 1/3] api: ceph: add endpoint to fetch config keys
2023-01-13 15:09 [pve-devel] [PATCH manager 0/3] fix 2515 use default sizes for new ceph Aaron Lauterer
@ 2023-01-13 15:09 ` Aaron Lauterer
2023-03-08 12:14 ` Dominik Csapak
2023-01-13 15:09 ` [pve-devel] [PATCH manager 2/3] fix #2515: ui: ceph pool create: use configured defaults for size and min_size Aaron Lauterer
2023-01-13 15:09 ` [pve-devel] [PATCH manager 3/3] ui: ceph pool edit: rework with controller and formulas Aaron Lauterer
2 siblings, 1 reply; 10+ messages in thread
From: Aaron Lauterer @ 2023-01-13 15:09 UTC (permalink / raw)
To: pve-devel
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>
---
I kept it as general as possible for any potential future use.
As mentioned in the cover letter, suggestions for a better name are
welcome.
Currently it returns the data as named hashes. My main reasoning for
this, instead of flatter arrays is the following:
The client is already requesting specific config keys. Being able to
use them directly means the client doesn't have to build its own dict or
object structure from the return values.
If the requested key is not set, a warning will be logged. The return
value will be 'null'.
PVE/API2/Ceph.pm | 84 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 84 insertions(+)
diff --git a/PVE/API2/Ceph.pm b/PVE/API2/Ceph.pm
index 55220324..3e21f0c8 100644
--- a/PVE/API2/Ceph.pm
+++ b/PVE/API2/Ceph.pm
@@ -90,6 +90,7 @@ __PACKAGE__->register_method ({
{ name => 'cmd-safety' },
{ name => 'config' },
{ name => 'configdb' },
+ { name => 'configkey' },
{ name => 'crush' },
{ name => 'fs' },
{ name => 'init' },
@@ -180,6 +181,89 @@ __PACKAGE__->register_method ({
}});
+my $CONFIGKEY_RE = qr/[0-9a-z\-_\.]*:[0-9a-zA-Z\-_]*/i;
+my $CONFIGKEYS_RE = qr/^(:?${CONFIGKEY_RE})(:?[;, ]${CONFIGKEY_RE})*$/;
+
+__PACKAGE__->register_method ({
+ name => 'configkey',
+ path => 'configkey',
+ method => 'GET',
+ proxyto => 'node',
+ protected => 1,
+ permissions => {
+ check => ['perm', '/', [ 'Sys.Audit' ]],
+ },
+ description => "Get configured keys from either the config file or config DB.",
+ parameters => {
+ additionalProperties => 0,
+ properties => {
+ node => get_standard_option('pve-node'),
+ config_keys => {
+ type => "string",
+ format => "string-list",
+ 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);
+ push(@{$requested_keys}, { section => $section, key => $key });
+ }
+
+ 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'};
+ $config->{$normalize->($section)}->{$normalize->($name)} = $value;
+ }
+
+ # read ceph.conf after config db as it has priority if settings are present in both
+ my $config_file = cfs_read_file('ceph.conf');
+ for my $section (keys %{$config_file}) {
+ for my $key (keys %{$config_file->{$section}}) {
+ $config->{$normalize->($section)}->{$normalize->($key)}
+ = $config_file->{$section}->{$key};
+ }
+ }
+
+ my $ret = {};
+
+ for my $pair (@{$requested_keys}) {
+ my ($section, $key) = $pair->@{'section', 'key'};
+ warn "section '${section}' does not exist in config" if !defined($config->{$section});
+ warn "key '${section}:${key}' does not exist in config"
+ if !defined($config->{$section}->{$key});
+
+ $ret->{$section}->{$key} = $config->{$section}->{$key};
+ }
+
+ return $ret;
+ }});
+
+
__PACKAGE__->register_method ({
name => 'init',
path => 'init',
--
2.30.2
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [pve-devel] [PATCH manager 1/3] api: ceph: add endpoint to fetch config keys
2023-01-13 15:09 ` [pve-devel] [PATCH manager 1/3] api: ceph: add endpoint to fetch config keys Aaron Lauterer
@ 2023-03-08 12:14 ` Dominik Csapak
2023-03-11 17:07 ` Thomas Lamprecht
0 siblings, 1 reply; 10+ messages in thread
From: Dominik Csapak @ 2023-03-08 12:14 UTC (permalink / raw)
To: Proxmox VE development discussion, Aaron Lauterer
high level:
as you mentioned the path 'configkey' is not really optimal
i recently mentioned off-list that we could clean this up on
the next breaking major release with a breaking api change:
have a 'config' dir and a
'file'
'db'
and 'key'( or 'value') api endpoint inside
that represents the different things
for now a possible change could be to do it in 'config'
but with a new parameter, though that's also not ideal
any further ideas/suggestions @Thomas?
some general comments inline:
On 1/13/23 16:09, Aaron Lauterer wrote:
> 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>
> ---
>
> I kept it as general as possible for any potential future use.
> As mentioned in the cover letter, suggestions for a better name are
> welcome.
>
> Currently it returns the data as named hashes. My main reasoning for
> this, instead of flatter arrays is the following:
> The client is already requesting specific config keys. Being able to
> use them directly means the client doesn't have to build its own dict or
> object structure from the return values.
>
> If the requested key is not set, a warning will be logged. The return
> value will be 'null'.
>
>
> PVE/API2/Ceph.pm | 84 ++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 84 insertions(+)
>
> diff --git a/PVE/API2/Ceph.pm b/PVE/API2/Ceph.pm
> index 55220324..3e21f0c8 100644
> --- a/PVE/API2/Ceph.pm
> +++ b/PVE/API2/Ceph.pm
> @@ -90,6 +90,7 @@ __PACKAGE__->register_method ({
> { name => 'cmd-safety' },
> { name => 'config' },
> { name => 'configdb' },
> + { name => 'configkey' },
> { name => 'crush' },
> { name => 'fs' },
> { name => 'init' },
> @@ -180,6 +181,89 @@ __PACKAGE__->register_method ({
> }});
>
>
> +my $CONFIGKEY_RE = qr/[0-9a-z\-_\.]*:[0-9a-zA-Z\-_]*/i;
> +my $CONFIGKEYS_RE = qr/^(:?${CONFIGKEY_RE})(:?[;, ]${CONFIGKEY_RE})*$/;
eh i get it, but imho those two names are too similar.
easy to confuse...
> +
> +__PACKAGE__->register_method ({
> + name => 'configkey',
> + path => 'configkey',
> + method => 'GET',
> + proxyto => 'node',
> + protected => 1,
> + permissions => {
> + check => ['perm', '/', [ 'Sys.Audit' ]],
> + },
> + description => "Get configured keys from either the config file or config DB.",
> + parameters => {
> + additionalProperties => 0,
> + properties => {
> + node => get_standard_option('pve-node'),
> + config_keys => {
> + type => "string",
> + format => "string-list",
> + typetext => "<section>:<config key>[;<section>:<config key>]",
> + pattern => $CONFIGKEYS_RE,
is it really necessary to have the pattern include the [;, ] and other
ocurrences if we use 'string-list' with a pattern?
if yes, we could also omit the 'format' if it's already contained in the
pattern
also, it seems that you'd allow the parameter: ':' (empty section/name)
is that intentional?
> + 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 _
we currently try to use only kebab-case for parameters, so i'd use that
too for return values if it really does not matter for ceph...
> + 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);
> + push(@{$requested_keys}, { section => $section, key => $key });
> + }
> +
> + 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'};
> + $config->{$normalize->($section)}->{$normalize->($name)} = $value;
> + }
> +
> + # read ceph.conf after config db as it has priority if settings are present in both
> + my $config_file = cfs_read_file('ceph.conf');
> + for my $section (keys %{$config_file}) {
> + for my $key (keys %{$config_file->{$section}}) {
> + $config->{$normalize->($section)}->{$normalize->($key)}
> + = $config_file->{$section}->{$key};
> + }
> + }
> +
> + my $ret = {};
> +
> + for my $pair (@{$requested_keys}) {
> + my ($section, $key) = $pair->@{'section', 'key'};
> + warn "section '${section}' does not exist in config" if !defined($config->{$section});
> + warn "key '${section}:${key}' does not exist in config"
> + if !defined($config->{$section}->{$key});
> +
> + $ret->{$section}->{$key} = $config->{$section}->{$key};
> + }
couldn't you directly filter the necessary section/names when iterating
over the config/db ? that would remove the extra step of filtering at
the end
> +
> + return $ret;
> + }});
> +
> +
> __PACKAGE__->register_method ({
> name => 'init',
> path => 'init',
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [pve-devel] [PATCH manager 1/3] api: ceph: add endpoint to fetch config keys
2023-03-08 12:14 ` Dominik Csapak
@ 2023-03-11 17:07 ` Thomas Lamprecht
2023-03-13 12:58 ` Aaron Lauterer
0 siblings, 1 reply; 10+ messages in thread
From: Thomas Lamprecht @ 2023-03-11 17:07 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak, Aaron Lauterer
Am 08/03/2023 um 13:14 schrieb Dominik Csapak:
> high level:
>
> as you mentioned the path 'configkey' is not really optimal
>
> i recently mentioned off-list that we could clean this up on
> the next breaking major release with a breaking api change:
>
> have a 'config' dir and a
> 'file'
> 'db'
> and 'key'( or 'value') api endpoint inside
> that represents the different things
>
> for now a possible change could be to do it in 'config'
> but with a new parameter, though that's also not ideal
>
> any further ideas/suggestions @Thomas?
We could add the full
cfg/
raw
db
value
now already, re-mount the 'cfg/raw' one on the current 'config' (or just keep
the code duplicated, not much gain if we remove it anyway) one and then drop that
old 'config' one in PVE 8.0; slightly hacky but IMO not that much.
Might want to check what other uses of config, cfg, conf, configuration there are
in API path's though, as ideally we keep the total unique count of them the same ;-)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [pve-devel] [PATCH manager 1/3] api: ceph: add endpoint to fetch config keys
2023-03-11 17:07 ` Thomas Lamprecht
@ 2023-03-13 12:58 ` Aaron Lauterer
2023-03-13 16:31 ` Thomas Lamprecht
0 siblings, 1 reply; 10+ messages in thread
From: Aaron Lauterer @ 2023-03-13 12:58 UTC (permalink / raw)
To: Thomas Lamprecht, Proxmox VE development discussion, Dominik Csapak
On 3/11/23 18:07, Thomas Lamprecht wrote:
> Am 08/03/2023 um 13:14 schrieb Dominik Csapak:
>> high level:
>>
>> as you mentioned the path 'configkey' is not really optimal
>>
>> i recently mentioned off-list that we could clean this up on
>> the next breaking major release with a breaking api change:
>>
>> have a 'config' dir and a
>> 'file'
>> 'db'
>> and 'key'( or 'value') api endpoint inside
>> that represents the different things
>>
>> for now a possible change could be to do it in 'config'
>> but with a new parameter, though that's also not ideal
>>
>> any further ideas/suggestions @Thomas?
>
>
> We could add the full
>
> cfg/
> raw
> db
> value
>
> now already, re-mount the 'cfg/raw' one on the current 'config' (or just keep
> the code duplicated, not much gain if we remove it anyway) one and then drop that
> old 'config' one in PVE 8.0; slightly hacky but IMO not that much.
>
> Might want to check what other uses of config, cfg, conf, configuration there are
> in API path's though, as ideally we keep the total unique count of them the same ;-)
AFAICT we basically only have "config" in the API paths according to the API
Viewer. So 'cfg' would be something new, not yet used.
I do like 'cfg' more than 'conf'. Once we dropped support for 'config', we could
wait a full major release and then move it back? Not sure but 'cfg' is also only
somewhat nice ;)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [pve-devel] [PATCH manager 1/3] api: ceph: add endpoint to fetch config keys
2023-03-13 12:58 ` Aaron Lauterer
@ 2023-03-13 16:31 ` Thomas Lamprecht
0 siblings, 0 replies; 10+ messages in thread
From: Thomas Lamprecht @ 2023-03-13 16:31 UTC (permalink / raw)
To: Aaron Lauterer, Proxmox VE development discussion, Dominik Csapak
Am 13/03/2023 um 13:58 schrieb Aaron Lauterer:
> On 3/11/23 18:07, Thomas Lamprecht wrote:
>> We could add the full
>>
>> cfg/
>> raw
>> db
>> value
>>
>> now already, re-mount the 'cfg/raw' one on the current 'config' (or just keep
>> the code duplicated, not much gain if we remove it anyway) one and then drop that
>> old 'config' one in PVE 8.0; slightly hacky but IMO not that much.
>>
>> Might want to check what other uses of config, cfg, conf, configuration there are
>> in API path's though, as ideally we keep the total unique count of them the same ;-)
>
> AFAICT we basically only have "config" in the API paths according to the API Viewer. So 'cfg' would be something new, not yet used.
> I do like 'cfg' more than 'conf'. Once we dropped support for 'config', we could wait a full major release and then move it back? Not sure but 'cfg' is also only somewhat nice ;)
Ok, so we need to use something new anyway, cfg works out fine for me, we just should
try to remember using that if a move like this needs to be done for some other API
subtree.
Moving then back again just for the sake of purity seems not the best argument, so
I'd rather live with config and cfg then than working through the churn of moving
back again.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [pve-devel] [PATCH manager 2/3] fix #2515: ui: ceph pool create: use configured defaults for size and min_size
2023-01-13 15:09 [pve-devel] [PATCH manager 0/3] fix 2515 use default sizes for new ceph Aaron Lauterer
2023-01-13 15:09 ` [pve-devel] [PATCH manager 1/3] api: ceph: add endpoint to fetch config keys Aaron Lauterer
@ 2023-01-13 15:09 ` Aaron Lauterer
2023-03-08 12:14 ` Dominik Csapak
2023-01-13 15:09 ` [pve-devel] [PATCH manager 3/3] ui: ceph pool edit: rework with controller and formulas Aaron Lauterer
2 siblings, 1 reply; 10+ messages in thread
From: Aaron Lauterer @ 2023-01-13 15:09 UTC (permalink / raw)
To: pve-devel
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>
---
www/manager6/ceph/Pool.js | 57 ++++++++++++++++++++++++++++++---------
1 file changed, 45 insertions(+), 12 deletions(-)
diff --git a/www/manager6/ceph/Pool.js b/www/manager6/ceph/Pool.js
index a1f008d1..86f83ffb 100644
--- a/www/manager6/ceph/Pool.js
+++ b/www/manager6/ceph/Pool.js
@@ -27,7 +27,9 @@ Ext.define('PVE.CephPoolInputPanel', {
name: 'size',
editConfig: {
xtype: 'proxmoxintegerfield',
- value: 3,
+ cbind: {
+ value: (get) => get('defaultSize') ?? 3,
+ },
minValue: 2,
maxValue: 7,
allowBlank: false,
@@ -40,7 +42,6 @@ Ext.define('PVE.CephPoolInputPanel', {
},
},
},
-
},
],
column2: [
@@ -78,9 +79,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') ?? 2,
+ minValue: (get) => {
+ if (Number(get('defaultMinSize')) === 1) {
+ return 1;
+ } else {
+ return get('isCreate') ? 2 : 1;
+ }
+ },
},
maxValue: 7,
allowBlank: false,
@@ -216,6 +223,8 @@ Ext.define('PVE.Ceph.PoolEdit', {
pool_name: '{pool_name}',
isErasure: '{isErasure}',
isCreate: '{isCreate}',
+ defaultSize: '{defaultSize}',
+ defaultMinSize: '{defaultMinSize}',
},
}],
});
@@ -372,6 +381,8 @@ Ext.define('PVE.node.Ceph.PoolList', {
Ext.create('PVE.Ceph.PoolEdit', {
title: gettext('Edit') + ': Ceph Pool',
nodename: nodename,
+ defaultSize: undefined,
+ defaultMinSize: undefined,
pool_name: rec.data.pool_name,
isErasure: rec.data.type === 'erasure',
autoShow: true,
@@ -388,14 +399,36 @@ 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/configkey',
+ method: 'GET',
+ params,
+ failure: response => Ext.Msg.alert(gettext('Error'), response.htmlStatus),
+ success: function({ result: { data } }) {
+ let global = data.global;
+ let defaultSize = global.osd_pool_default_size;
+ let defaultMinSize = global.osd_pool_default_min_size;
+
+ 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.30.2
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [pve-devel] [PATCH manager 2/3] fix #2515: ui: ceph pool create: use configured defaults for size and min_size
2023-01-13 15:09 ` [pve-devel] [PATCH manager 2/3] fix #2515: ui: ceph pool create: use configured defaults for size and min_size Aaron Lauterer
@ 2023-03-08 12:14 ` Dominik Csapak
0 siblings, 0 replies; 10+ messages in thread
From: Dominik Csapak @ 2023-03-08 12:14 UTC (permalink / raw)
To: Proxmox VE development discussion, Aaron Lauterer
some comments inline:
On 1/13/23 16:09, Aaron Lauterer wrote:
> 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>
> ---
> www/manager6/ceph/Pool.js | 57 ++++++++++++++++++++++++++++++---------
> 1 file changed, 45 insertions(+), 12 deletions(-)
>
> diff --git a/www/manager6/ceph/Pool.js b/www/manager6/ceph/Pool.js
> index a1f008d1..86f83ffb 100644
> --- a/www/manager6/ceph/Pool.js
> +++ b/www/manager6/ceph/Pool.js
> @@ -27,7 +27,9 @@ Ext.define('PVE.CephPoolInputPanel', {
> name: 'size',
> editConfig: {
> xtype: 'proxmoxintegerfield',
> - value: 3,
> + cbind: {
> + value: (get) => get('defaultSize') ?? 3,
> + },
> minValue: 2,
> maxValue: 7,
> allowBlank: false,
> @@ -40,7 +42,6 @@ Ext.define('PVE.CephPoolInputPanel', {
> },
> },
> },
> -
> },
> ],
> column2: [
> @@ -78,9 +79,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') ?? 2,
> + minValue: (get) => {
> + if (Number(get('defaultMinSize')) === 1) {
> + return 1;
> + } else {
> + return get('isCreate') ? 2 : 1;
> + }
> + },
> },
> maxValue: 7,
> allowBlank: false,
> @@ -216,6 +223,8 @@ Ext.define('PVE.Ceph.PoolEdit', {
> pool_name: '{pool_name}',
> isErasure: '{isErasure}',
> isCreate: '{isCreate}',
> + defaultSize: '{defaultSize}',
> + defaultMinSize: '{defaultMinSize}',
> },
> }],
> });
> @@ -372,6 +381,8 @@ Ext.define('PVE.node.Ceph.PoolList', {
> Ext.create('PVE.Ceph.PoolEdit', {
> title: gettext('Edit') + ': Ceph Pool',
> nodename: nodename,
> + defaultSize: undefined,
> + defaultMinSize: undefined,
i guess you did it for clarity, but it should not be
necessary to pass config that is undefined...
i'd rather document them in the class and set them there
to undefined
> pool_name: rec.data.pool_name,
> isErasure: rec.data.type === 'erasure',
> autoShow: true,
> @@ -388,14 +399,36 @@ 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/configkey',
> + method: 'GET',
> + params,
> + failure: response => Ext.Msg.alert(gettext('Error'), response.htmlStatus),
> + success: function({ result: { data } }) {
> + let global = data.global;
> + let defaultSize = global.osd_pool_default_size;
> + let defaultMinSize = global.osd_pool_default_min_size;
> +
> + Ext.create('PVE.Ceph.PoolEdit', {
> + title: gettext('Create') + ': Ceph Pool',
> + isCreate: true,
> + isErasure: false,
> + defaultSize,
> + defaultMinSize,
> + nodename: nodename,
> + autoShow: true,
> + listeners: {
> + destroy: () => rstore.load(),
> + },
> + });
one slight problem i see with that is that the user has no indication
that something is loading when clicking the button...
you can use the 'waitMsgTarget' of the API2Request to show a
'loading...' mask on the grid
> },
> });
> },
^ permalink raw reply [flat|nested] 10+ messages in thread
* [pve-devel] [PATCH manager 3/3] ui: ceph pool edit: rework with controller and formulas
2023-01-13 15:09 [pve-devel] [PATCH manager 0/3] fix 2515 use default sizes for new ceph Aaron Lauterer
2023-01-13 15:09 ` [pve-devel] [PATCH manager 1/3] api: ceph: add endpoint to fetch config keys Aaron Lauterer
2023-01-13 15:09 ` [pve-devel] [PATCH manager 2/3] fix #2515: ui: ceph pool create: use configured defaults for size and min_size Aaron Lauterer
@ 2023-01-13 15:09 ` Aaron Lauterer
2023-03-08 12:15 ` Dominik Csapak
2 siblings, 1 reply; 10+ messages in thread
From: Aaron Lauterer @ 2023-01-13 15:09 UTC (permalink / raw)
To: pve-devel
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>
---
www/manager6/ceph/Pool.js | 87 ++++++++++++++++++++++++++++-----------
1 file changed, 63 insertions(+), 24 deletions(-)
diff --git a/www/manager6/ceph/Pool.js b/www/manager6/ceph/Pool.js
index 86f83ffb..a38c5b3f 100644
--- a/www/manager6/ceph/Pool.js
+++ b/www/manager6/ceph/Pool.js
@@ -7,6 +7,49 @@ Ext.define('PVE.CephPoolInputPanel', {
onlineHelp: 'pve_ceph_pools',
subject: 'Ceph Pool',
+
+ 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;
+ },
+ },
+ },
+
+ controller: {
+ xclass: 'Ext.app.ViewController',
+
+ init: function(view) {
+ let vm = this.getViewModel();
+ vm.set('size', view.defaultSize ? Number(view.defaultSize) : 3);
+ vm.set('minSize', view.defaultMinSize ? Number(view.defaultMinSize) : 2);
+ },
+ 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
+ },
+ },
+
column1: [
{
xtype: 'pmxDisplayEditField',
@@ -34,12 +77,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',
},
},
},
@@ -77,10 +115,13 @@ Ext.define('PVE.CephPoolInputPanel', {
advancedColumn1: [
{
xtype: 'proxmoxintegerfield',
- fieldLabel: gettext('Min. Size'),
+ bind: {
+ fieldLabel: '{minSizeLabel}',
+ value: '{minSize}',
+ },
name: 'min_size',
+ reference: 'min_size',
cbind: {
- value: (get) => get('defaultMinSize') ?? 2,
minValue: (get) => {
if (Number(get('defaultMinSize')) === 1) {
return 1;
@@ -91,28 +132,26 @@ 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',
+ name: 'min_size_half-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',
+ name: 'min_size_one-warning',
+ 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.30.2
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [pve-devel] [PATCH manager 3/3] ui: ceph pool edit: rework with controller and formulas
2023-01-13 15:09 ` [pve-devel] [PATCH manager 3/3] ui: ceph pool edit: rework with controller and formulas Aaron Lauterer
@ 2023-03-08 12:15 ` Dominik Csapak
0 siblings, 0 replies; 10+ messages in thread
From: Dominik Csapak @ 2023-03-08 12:15 UTC (permalink / raw)
To: Proxmox VE development discussion, Aaron Lauterer
some (minor) comments inline
On 1/13/23 16:09, Aaron Lauterer wrote:
> 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>
> ---
> www/manager6/ceph/Pool.js | 87 ++++++++++++++++++++++++++++-----------
> 1 file changed, 63 insertions(+), 24 deletions(-)
>
> diff --git a/www/manager6/ceph/Pool.js b/www/manager6/ceph/Pool.js
> index 86f83ffb..a38c5b3f 100644
> --- a/www/manager6/ceph/Pool.js
> +++ b/www/manager6/ceph/Pool.js
> @@ -7,6 +7,49 @@ Ext.define('PVE.CephPoolInputPanel', {
> onlineHelp: 'pve_ceph_pools',
>
> subject: 'Ceph Pool',
> +
> + viewModel: {
typically we have the viewmodel between the controller
and the layout, but no big issue
(though we're probably not 100% consistent on that)
> + 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;
> + },
mhmm... i generally don't want to have negated variable names,
but since we bind that to the 'hidden' property (negated)
would it maybe make sense to reverse the logic in here and rename them
to 'hide*Warning' ?
not sure about that though
> + },
> + },
> +
> + controller: {
> + xclass: 'Ext.app.ViewController',
> +
> + init: function(view) {
> + let vm = this.getViewModel();
> + vm.set('size', view.defaultSize ? Number(view.defaultSize) : 3);
> + vm.set('minSize', view.defaultMinSize ? Number(view.defaultMinSize) : 2);
> + },
> + 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
> + },
> + },
> +
> column1: [
> {
> xtype: 'pmxDisplayEditField',
> @@ -34,12 +77,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',
> },
> },
> },
> @@ -77,10 +115,13 @@ Ext.define('PVE.CephPoolInputPanel', {
> advancedColumn1: [
> {
> xtype: 'proxmoxintegerfield',
> - fieldLabel: gettext('Min. Size'),
> + bind: {
> + fieldLabel: '{minSizeLabel}',
> + value: '{minSize}',
> + },
> name: 'min_size',
> + reference: 'min_size',
it seems you don't actually use the reference, maybe a leftover?
> cbind: {
> - value: (get) => get('defaultMinSize') ?? 2,
> minValue: (get) => {
> if (Number(get('defaultMinSize')) === 1) {
> return 1;
> @@ -91,28 +132,26 @@ 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',
> + name: 'min_size_half-warning',
a field that's not set/read does not really need a name,
so we can simply omit that
> + 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',
> + name: 'min_size_one-warning',
same here
> + 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',
^ permalink raw reply [flat|nested] 10+ messages in thread