[pve-devel] [PATCH manager 0/2] fix #5570 ceph mds: allow custom MDS IDs from the UI

[pve-devel] [PATCH manager 0/2] fix #5570 ceph mds: allow custom MDS IDs from the UI

[Aaron Lauterer]

The ID for the MDS cannot start with a number [0]. The first patch adds
a check for this.

The second patch is the actual fix, by reworking the edit window when
adding new MDS'.

By allowing the users to set the name of the MDS directly, we can catch
the situation where the hostname starts with a number and let the user
decide how it should be named. Similar to what they can already do on
the CLI.


Another approach would be to leave the UI as is and catch the situation
in the backend. If an ID that starts with a number is detected, we could
prepend it with a default string.

[0] https://docs.ceph.com/en/latest/man/8/ceph-mds/

Aaron Lauterer (2):
  api: ceph mds: avoid creating MDS when ID starts with number
  fix#5570 ui: ceph: make MDS ID configurable

 PVE/API2/Ceph/MDS.pm             |  2 ++
 www/manager6/ceph/ServiceList.js | 21 +++++++++++----------
 2 files changed, 13 insertions(+), 10 deletions(-)

-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH manager 1/2] api: ceph mds: avoid creating MDS when ID starts with number

[Aaron Lauterer]

Ceph MDS IDs cannot start with a number [0].

[0] https://docs.ceph.com/en/latest/man/8/ceph-mds/

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
 PVE/API2/Ceph/MDS.pm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/PVE/API2/Ceph/MDS.pm b/PVE/API2/Ceph/MDS.pm
index 6fc0ae45..d99eebe6 100644
--- a/PVE/API2/Ceph/MDS.pm
+++ b/PVE/API2/Ceph/MDS.pm
@@ -133,6 +133,8 @@ __PACKAGE__->register_method ({
 
 	my $mds_id = $param->{name} // $nodename;
 
+	die "ID of the MDS cannot start with a number!\n" if ($mds_id =~ /^[0-9]/);
+
 	my $worker = sub {
 	    my $timeout = PVE::Ceph::Tools::get_config('long_rados_timeout');
 	    my $rados = PVE::RADOS->new(timeout => $timeout);
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH manager 2/2] fix#5570 ui: ceph: make MDS ID configurable

[Aaron Lauterer]

Since the ID of an MDS cannot start with a number [0], we cannot just
use the hostname in all situations, as they are allowed to start with a
number.

By having an extra field for the MDS ID, we can check for that via a
regex. This field is filled with the hostname when the host on which it
should be installed is selected.

This means, we can remove the extra ID field, as additional MDS, and
their unique ID can be set with the new ID field.

[0] https://docs.ceph.com/en/latest/man/8/ceph-mds/

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
 www/manager6/ceph/ServiceList.js | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/www/manager6/ceph/ServiceList.js b/www/manager6/ceph/ServiceList.js
index 76710146..9075f197 100644
--- a/www/manager6/ceph/ServiceList.js
+++ b/www/manager6/ceph/ServiceList.js
@@ -13,18 +13,17 @@ Ext.define('PVE.CephCreateService', {
 	me.nodename = node;
 	me.updateUrl();
     },
-    setExtraID: function(extraID) {
+    setServiceID: function(value) {
 	let me = this;
-	me.extraID = me.type === 'mds' ? `-${extraID}` : '';
+	me.serviceID = value;
 	me.updateUrl();
     },
     updateUrl: function() {
 	let me = this;
-
-	let extraID = me.extraID ?? '';
 	let node = me.nodename;
+	let service_id = me.serviceID ?? me.nodename;
 
-	me.url = `/nodes/${node}/ceph/${me.type}/${node}${extraID}`;
+	me.url = `/nodes/${node}/ceph/${me.type}/${service_id}`;
     },
 
     defaults: {
@@ -40,15 +39,17 @@ Ext.define('PVE.CephCreateService', {
 	    listeners: {
 		change: function(f, value) {
 		    let view = this.up('pveCephCreateService');
+		    view.lookup('mds-id').setValue(value);
 		    view.setNode(value);
 		},
 	    },
 	},
 	{
 	    xtype: 'textfield',
-	    fieldLabel: gettext('Extra ID'),
-	    regex: /[a-zA-Z0-9]+/,
-	    regexText: gettext('ID may only consist of alphanumeric characters'),
+	    reference: 'mds-id',
+	    fieldLabel: gettext('MDS ID'),
+	    regex: /^([a-zA-Z]([-a-zA-Z0-9]*[a-zA-Z0-9])?)$/,
+	    regexText: gettext('ID may consist of alphanumeric characters and hyphen. It cannot start with a number or end in a hyphen.'),
 	    submitValue: false,
 	    emptyText: Proxmox.Utils.NoneText,
 	    cbind: {
@@ -58,7 +59,7 @@ Ext.define('PVE.CephCreateService', {
 	    listeners: {
 		change: function(f, value) {
 		    let view = this.up('pveCephCreateService');
-		    view.setExtraID(value);
+		    view.setServiceID(value);
 		},
 	    },
 	},
@@ -73,7 +74,7 @@ Ext.define('PVE.CephCreateService', {
 	    cbind: {
 		hidden: get => get('type') !== 'mds',
 	    },
-	    html: gettext('The Extra ID allows creating multiple MDS per node, which increases redundancy with more than one CephFS.'),
+	    html: gettext('By using different IDs, you can have multiple MDS per node, which increases redundancy with more than one CephFS.'),
 	},
     ],
 
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH manager 0/2] fix #5570 ceph mds: allow custom MDS IDs from the UI

[Christoph Heiss]

Tested this on a small 3-node cluster, works as advertised. Code looks
good too, just one small comment on the second patch.

But otherwise please consider the series at least:

Tested-by: Christoph Heiss <c.heiss@proxmox.com>

On Wed, Jul 10, 2024 at 02:42:58PM GMT, Aaron Lauterer wrote:
> The ID for the MDS cannot start with a number [0]. The first patch adds
> a check for this.
>
> The second patch is the actual fix, by reworking the edit window when
> adding new MDS'.
>
> By allowing the users to set the name of the MDS directly, we can catch
> the situation where the hostname starts with a number and let the user
> decide how it should be named. Similar to what they can already do on
> the CLI.
>
>
> Another approach would be to leave the UI as is and catch the situation
> in the backend. If an ID that starts with a number is detected, we could
> prepend it with a default string.
>
> [0] https://docs.ceph.com/en/latest/man/8/ceph-mds/
>
> Aaron Lauterer (2):
>   api: ceph mds: avoid creating MDS when ID starts with number
>   fix#5570 ui: ceph: make MDS ID configurable
>
>  PVE/API2/Ceph/MDS.pm             |  2 ++
>  www/manager6/ceph/ServiceList.js | 21 +++++++++++----------
>  2 files changed, 13 insertions(+), 10 deletions(-)
>
> --
> 2.39.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH manager 2/2] fix#5570 ui: ceph: make MDS ID configurable

[Christoph Heiss]

On Wed, Jul 10, 2024 at 02:43:00PM GMT, Aaron Lauterer wrote:
> [..]
>  www/manager6/ceph/ServiceList.js | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/www/manager6/ceph/ServiceList.js b/www/manager6/ceph/ServiceList.js
> index 76710146..9075f197 100644
> --- a/www/manager6/ceph/ServiceList.js
> +++ b/www/manager6/ceph/ServiceList.js
> [..]
> @@ -40,15 +39,17 @@ Ext.define('PVE.CephCreateService', {
>  	    listeners: {
>  		change: function(f, value) {
>  		    let view = this.up('pveCephCreateService');
> +		    view.lookup('mds-id').setValue(value);
>  		    view.setNode(value);
>  		},
>  	    },
>  	},
>  	{
>  	    xtype: 'textfield',
> -	    fieldLabel: gettext('Extra ID'),
> -	    regex: /[a-zA-Z0-9]+/,
> -	    regexText: gettext('ID may only consist of alphanumeric characters'),
> +	    reference: 'mds-id',
> +	    fieldLabel: gettext('MDS ID'),
> +	    regex: /^([a-zA-Z]([-a-zA-Z0-9]*[a-zA-Z0-9])?)$/,
> +	    regexText: gettext('ID may consist of alphanumeric characters and hyphen. It cannot start with a number or end in a hyphen.'),

Is there a check in the backend whether the name ends with a hyphen? If
not, should there be one?

Also, while at it, maybe also set `maxLength` to a sensible value?
E.g. while the API schema allows up to 200 characters, it then fails to
create a task log file with "File name too long" :^)
~ 180 characters seem to be fine, but probably something like 128
characters is already a pretty sensible limit.

>  	    submitValue: false,
>  	    emptyText: Proxmox.Utils.NoneText,
>  	    cbind: {
> @@ -58,7 +59,7 @@ Ext.define('PVE.CephCreateService', {
>  	    listeners: {
>  		change: function(f, value) {
>  		    let view = this.up('pveCephCreateService');
> -		    view.setExtraID(value);
> +		    view.setServiceID(value);
>  		},
>  	    },
>  	},
> @@ -73,7 +74,7 @@ Ext.define('PVE.CephCreateService', {
>  	    cbind: {
>  		hidden: get => get('type') !== 'mds',
>  	    },
> -	    html: gettext('The Extra ID allows creating multiple MDS per node, which increases redundancy with more than one CephFS.'),
> +	    html: gettext('By using different IDs, you can have multiple MDS per node, which increases redundancy with more than one CephFS.'),
>  	},
>      ],
>
> --
> 2.39.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH manager 2/2] fix#5570 ui: ceph: make MDS ID configurable

[Aaron Lauterer]

On  2024-07-10  15:59, Christoph Heiss wrote:
> On Wed, Jul 10, 2024 at 02:43:00PM GMT, Aaron Lauterer wrote:
>> [..]
>>   www/manager6/ceph/ServiceList.js | 21 +++++++++++----------
>>   1 file changed, 11 insertions(+), 10 deletions(-)
>>
>> diff --git a/www/manager6/ceph/ServiceList.js b/www/manager6/ceph/ServiceList.js
>> index 76710146..9075f197 100644
>> --- a/www/manager6/ceph/ServiceList.js
>> +++ b/www/manager6/ceph/ServiceList.js
>> [..]
>> @@ -40,15 +39,17 @@ Ext.define('PVE.CephCreateService', {
>>   	    listeners: {
>>   		change: function(f, value) {
>>   		    let view = this.up('pveCephCreateService');
>> +		    view.lookup('mds-id').setValue(value);
>>   		    view.setNode(value);
>>   		},
>>   	    },
>>   	},
>>   	{
>>   	    xtype: 'textfield',
>> -	    fieldLabel: gettext('Extra ID'),
>> -	    regex: /[a-zA-Z0-9]+/,
>> -	    regexText: gettext('ID may only consist of alphanumeric characters'),
>> +	    reference: 'mds-id',
>> +	    fieldLabel: gettext('MDS ID'),
>> +	    regex: /^([a-zA-Z]([-a-zA-Z0-9]*[a-zA-Z0-9])?)$/,
>> +	    regexText: gettext('ID may consist of alphanumeric characters and hyphen. It cannot start with a number or end in a hyphen.'),
> 
> Is there a check in the backend whether the name ends with a hyphen? If
> not, should there be one?

Yes, the parameter checks [0] against the Ceph Service Regex [1] which 
is basically a hostname regex.

> 
> Also, while at it, maybe also set `maxLength` to a sensible value?
> E.g. while the API schema allows up to 200 characters, it then fails to
> create a task log file with "File name too long" :^)
> ~ 180 characters seem to be fine, but probably something like 128
> characters is already a pretty sensible limit.

nice catch. we might want to lower the limit in the API then, and could 
also set the limit in the GUI.
There might be other places in the API with a similar issue... glancing 
at all the other Ceph Services which can get customized names with the 
API/CLI.


[0] 
https://git.proxmox.com/?p=pve-manager.git;a=blob;f=PVE/API2/Ceph/MDS.pm;h=6fc0ae450b7f65b5b5b70f72fb23f2b8dbef5a98;hb=HEAD#l107
[1] 
https://git.proxmox.com/?p=pve-manager.git;a=blob;f=PVE/Ceph/Services.pm;h=e0f31e8eb6bc9b3777b3d0d548497276efaa5c41;hb=HEAD#l14


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH manager 2/2] fix#5570 ui: ceph: make MDS ID configurable

[Christoph Heiss]

On Wed, Jul 10, 2024 at 04:18:25PM GMT, Aaron Lauterer wrote:
>
>
> On  2024-07-10  15:59, Christoph Heiss wrote:
> > On Wed, Jul 10, 2024 at 02:43:00PM GMT, Aaron Lauterer wrote:
> > > [..]
> > >   www/manager6/ceph/ServiceList.js | 21 +++++++++++----------
> > >   1 file changed, 11 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/www/manager6/ceph/ServiceList.js b/www/manager6/ceph/ServiceList.js
> > > index 76710146..9075f197 100644
> > > --- a/www/manager6/ceph/ServiceList.js
> > > +++ b/www/manager6/ceph/ServiceList.js
> > > [..]
> > > @@ -40,15 +39,17 @@ Ext.define('PVE.CephCreateService', {
> > >   	    listeners: {
> > >   		change: function(f, value) {
> > >   		    let view = this.up('pveCephCreateService');
> > > +		    view.lookup('mds-id').setValue(value);
> > >   		    view.setNode(value);
> > >   		},
> > >   	    },
> > >   	},
> > >   	{
> > >   	    xtype: 'textfield',
> > > -	    fieldLabel: gettext('Extra ID'),
> > > -	    regex: /[a-zA-Z0-9]+/,
> > > -	    regexText: gettext('ID may only consist of alphanumeric characters'),
> > > +	    reference: 'mds-id',
> > > +	    fieldLabel: gettext('MDS ID'),
> > > +	    regex: /^([a-zA-Z]([-a-zA-Z0-9]*[a-zA-Z0-9])?)$/,
> > > +	    regexText: gettext('ID may consist of alphanumeric characters and hyphen. It cannot start with a number or end in a hyphen.'),
> >
> > Is there a check in the backend whether the name ends with a hyphen? If
> > not, should there be one?
>
> Yes, the parameter checks [0] against the Ceph Service Regex [1] which is
> basically a hostname regex.

Perfect, that's fine then.

With that resolved, you can also consider the entire series:

Reviewed-by: Christoph Heiss <c.heiss@proxmox.com>

>
> >
> > Also, while at it, maybe also set `maxLength` to a sensible value?
> > E.g. while the API schema allows up to 200 characters, it then fails to
> > create a task log file with "File name too long" :^)
> > ~ 180 characters seem to be fine, but probably something like 128
> > characters is already a pretty sensible limit.
>
> nice catch. we might want to lower the limit in the API then, and could also
> set the limit in the GUI.
> There might be other places in the API with a similar issue... glancing at
> all the other Ceph Services which can get customized names with the API/CLI.
>

I really was just mashing the keyboard in the usual "lets see what
breaks" fashion here :^)

Can be done in a separate patch(-series) though, as I don't think its
*that* important - esp. considering such long names are (hopefully) very
much an edge case.

>
> [0] https://git.proxmox.com/?p=pve-manager.git;a=blob;f=PVE/API2/Ceph/MDS.pm;h=6fc0ae450b7f65b5b5b70f72fb23f2b8dbef5a98;hb=HEAD#l107
> [1] https://git.proxmox.com/?p=pve-manager.git;a=blob;f=PVE/Ceph/Services.pm;h=e0f31e8eb6bc9b3777b3d0d548497276efaa5c41;hb=HEAD#l14


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] applied-series: [PATCH manager 0/2] fix #5570 ceph mds: allow custom MDS IDs from the UI

[Thomas Lamprecht]

Am 10/07/2024 um 14:42 schrieb Aaron Lauterer:
> The ID for the MDS cannot start with a number [0]. The first patch adds
> a check for this.
> 
> The second patch is the actual fix, by reworking the edit window when
> adding new MDS'.
> 
> By allowing the users to set the name of the MDS directly, we can catch
> the situation where the hostname starts with a number and let the user
> decide how it should be named. Similar to what they can already do on
> the CLI.
> 
> 
> Another approach would be to leave the UI as is and catch the situation
> in the backend. If an ID that starts with a number is detected, we could
> prepend it with a default string.
> 
> [0] https://docs.ceph.com/en/latest/man/8/ceph-mds/
> 
> Aaron Lauterer (2):
>   api: ceph mds: avoid creating MDS when ID starts with number
>   fix#5570 ui: ceph: make MDS ID configurable
> 
>  PVE/API2/Ceph/MDS.pm             |  2 ++
>  www/manager6/ceph/ServiceList.js | 21 +++++++++++----------
>  2 files changed, 13 insertions(+), 10 deletions(-)
> 


applied with a small s/service_id/serviceID/ style fix squashed into the
UI patch plus added the missing space in the subject between "fix" and the
issue ID, thanks!

Oh, I also made a follow-up [0] that makes the field required now to avoid
an API error if the user clears the field before submitting.<

[0]: https://git.proxmox.com/?p=pve-manager.git;a=commitdiff;h=d69d4ed8cb276b492a5fe7883f94db777e06d2b2


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] applied-series: [PATCH manager 0/2] fix #5570 ceph mds: allow custom MDS IDs from the UI

[Aaron Lauterer]

On  2024-07-22  18:57, Thomas Lamprecht wrote:
> Am 10/07/2024 um 14:42 schrieb Aaron Lauterer:
>> The ID for the MDS cannot start with a number [0]. The first patch adds
>> a check for this.
>>
>> The second patch is the actual fix, by reworking the edit window when
>> adding new MDS'.
>>
>> By allowing the users to set the name of the MDS directly, we can catch
>> the situation where the hostname starts with a number and let the user
>> decide how it should be named. Similar to what they can already do on
>> the CLI.
>>
>>
>> Another approach would be to leave the UI as is and catch the situation
>> in the backend. If an ID that starts with a number is detected, we could
>> prepend it with a default string.
>>
>> [0] https://docs.ceph.com/en/latest/man/8/ceph-mds/
>>
>> Aaron Lauterer (2):
>>    api: ceph mds: avoid creating MDS when ID starts with number
>>    fix#5570 ui: ceph: make MDS ID configurable
>>
>>   PVE/API2/Ceph/MDS.pm             |  2 ++
>>   www/manager6/ceph/ServiceList.js | 21 +++++++++++----------
>>   2 files changed, 13 insertions(+), 10 deletions(-)
>>
> 
> 
> applied with a small s/service_id/serviceID/ style fix squashed into the
> UI patch plus added the missing space in the subject between "fix" and the
> issue ID, thanks!
> 
> Oh, I also made a follow-up [0] that makes the field required now to avoid
> an API error if the user clears the field before submitting.<

thx for the fixes!
> 
> [0]: https://git.proxmox.com/?p=pve-manager.git;a=commitdiff;h=d69d4ed8cb276b492a5fe7883f94db777e06d2b2


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel