[pve-devel] [PATCH manager] Allow setting device class on osd create

[pve-devel] [PATCH manager] Allow setting device class on osd create

[Alwin Antreich]

In some situations Ceph's auto-detection doesn't recognize the device
class correctly. The option allows to set it directly on osd create,
instead of altering it afterwards. This way the cluster doesn't need to
shift data back and forth unnecessarily.

Signed-off-by: Alwin Antreich <a.antreich@proxmox.com>
---
 PVE/API2/Ceph/OSD.pm | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/PVE/API2/Ceph/OSD.pm b/PVE/API2/Ceph/OSD.pm
index ceaed129..f1f39bf9 100644
--- a/PVE/API2/Ceph/OSD.pm
+++ b/PVE/API2/Ceph/OSD.pm
@@ -260,6 +260,11 @@ __PACKAGE__->register_method ({
 		default => 0,
 		description => "Enables encryption of the OSD."
 	    },
+	    'crush-device-class' => {
+		optional => 1,
+		type => 'string',
+		description => "Set the device class of the OSD in crush."
+	    },
 	},
     },
     returns => { type => 'string' },
@@ -429,7 +434,9 @@ __PACKAGE__->register_method ({
 		# update disklist
 		$disklist = PVE::Diskmanage::get_disks($devlist, 1);
 
+		my $dev_class = $param->{'crush-device-class'};
 		my $cmd = ['ceph-volume', 'lvm', 'create', '--cluster-fsid', $fsid ];
+		push @$cmd, '--crush-device-class', $dev_class if $dev_class;
 
 		my $devpath = $disklist->{$devname}->{devpath};
 		print "create OSD on $devpath (bluestore)\n";
-- 
2.26.2

[pve-devel] applied: [PATCH manager] Allow setting device class on osd create

[Thomas Lamprecht]

Am 7/23/20 um 3:25 PM schrieb Alwin Antreich:
> In some situations Ceph's auto-detection doesn't recognize the device
> class correctly. The option allows to set it directly on osd create,
> instead of altering it afterwards. This way the cluster doesn't need to
> shift data back and forth unnecessarily.
> 
> Signed-off-by: Alwin Antreich <a.antreich@proxmox.com>
> ---
>  PVE/API2/Ceph/OSD.pm | 7 +++++++
>  1 file changed, 7 insertions(+)
> 

applied, thanks - comments still inline

> diff --git a/PVE/API2/Ceph/OSD.pm b/PVE/API2/Ceph/OSD.pm
> index ceaed129..f1f39bf9 100644
> --- a/PVE/API2/Ceph/OSD.pm
> +++ b/PVE/API2/Ceph/OSD.pm
> @@ -260,6 +260,11 @@ __PACKAGE__->register_method ({
>  		default => 0,
>  		description => "Enables encryption of the OSD."
>  	    },
> +	    'crush-device-class' => {
> +		optional => 1,
> +		type => 'string',
> +		description => "Set the device class of the OSD in crush."
> +	    },

why not having an enum with 'nvme', 'ssd', and 'hdd' here?

>  	},
>      },
>      returns => { type => 'string' },
> @@ -429,7 +434,9 @@ __PACKAGE__->register_method ({
>  		# update disklist
>  		$disklist = PVE::Diskmanage::get_disks($devlist, 1);
>  
> +		my $dev_class = $param->{'crush-device-class'};
>  		my $cmd = ['ceph-volume', 'lvm', 'create', '--cluster-fsid', $fsid ];

nit: rather would have the declaration here, or even more explicit:

if (my $dev_class = $param->{'crush-device-class'}) {
    push @$cmd, '--crush-device-class', $dev_class;
}

but as said, a very nit, just that the split addition of lines got my attention
somehow ^^

> +		push @$cmd, '--crush-device-class', $dev_class if $dev_class;
>  
>  		my $devpath = $disklist->{$devname}->{devpath};
>  		print "create OSD on $devpath (bluestore)\n";
> 

Re: [pve-devel] applied: [PATCH manager] Allow setting device class on osd create

[Alwin Antreich]

On Fri, Jul 24, 2020 at 11:34:33AM +0200, Thomas Lamprecht wrote:
> Am 7/23/20 um 3:25 PM schrieb Alwin Antreich:
> > In some situations Ceph's auto-detection doesn't recognize the device
> > class correctly. The option allows to set it directly on osd create,
> > instead of altering it afterwards. This way the cluster doesn't need to
> > shift data back and forth unnecessarily.
> > 
> > Signed-off-by: Alwin Antreich <a.antreich@proxmox.com>
> > ---
> >  PVE/API2/Ceph/OSD.pm | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> 
> applied, thanks - comments still inline
> 
> > diff --git a/PVE/API2/Ceph/OSD.pm b/PVE/API2/Ceph/OSD.pm
> > index ceaed129..f1f39bf9 100644
> > --- a/PVE/API2/Ceph/OSD.pm
> > +++ b/PVE/API2/Ceph/OSD.pm
> > @@ -260,6 +260,11 @@ __PACKAGE__->register_method ({
> >  		default => 0,
> >  		description => "Enables encryption of the OSD."
> >  	    },
> > +	    'crush-device-class' => {
> > +		optional => 1,
> > +		type => 'string',
> > +		description => "Set the device class of the OSD in crush."
> > +	    },
> 
> why not having an enum with 'nvme', 'ssd', and 'hdd' here?
Ceph allows the class to be an arbitrary string, eg. my-very-fast-disk.

> 
> >  	},
> >      },
> >      returns => { type => 'string' },
> > @@ -429,7 +434,9 @@ __PACKAGE__->register_method ({
> >  		# update disklist
> >  		$disklist = PVE::Diskmanage::get_disks($devlist, 1);
> >  
> > +		my $dev_class = $param->{'crush-device-class'};
> >  		my $cmd = ['ceph-volume', 'lvm', 'create', '--cluster-fsid', $fsid ];
> 
> nit: rather would have the declaration here, or even more explicit:
> 
> if (my $dev_class = $param->{'crush-device-class'}) {
>     push @$cmd, '--crush-device-class', $dev_class;
> }
> 
> but as said, a very nit, just that the split addition of lines got my attention
> somehow ^^
Thanks I will keep that in mind for the next time. ;)

> 
> > +		push @$cmd, '--crush-device-class', $dev_class if $dev_class;
> >  
> >  		my $devpath = $disklist->{$devname}->{devpath};
> >  		print "create OSD on $devpath (bluestore)\n";
> > 
> 

Re: [pve-devel] applied: [PATCH manager] Allow setting device class on osd create

[Thomas Lamprecht]

Am 7/24/20 um 11:46 AM schrieb Alwin Antreich:
> On Fri, Jul 24, 2020 at 11:34:33AM +0200, Thomas Lamprecht wrote:
>> Am 7/23/20 um 3:25 PM schrieb Alwin Antreich:
>>> In some situations Ceph's auto-detection doesn't recognize the device
>>> class correctly. The option allows to set it directly on osd create,
>>> instead of altering it afterwards. This way the cluster doesn't need to
>>> shift data back and forth unnecessarily.
>>>
>>> Signed-off-by: Alwin Antreich <a.antreich@proxmox.com>
>>> ---
>>>  PVE/API2/Ceph/OSD.pm | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>
>> applied, thanks - comments still inline
>>
>>> diff --git a/PVE/API2/Ceph/OSD.pm b/PVE/API2/Ceph/OSD.pm
>>> index ceaed129..f1f39bf9 100644
>>> --- a/PVE/API2/Ceph/OSD.pm
>>> +++ b/PVE/API2/Ceph/OSD.pm
>>> @@ -260,6 +260,11 @@ __PACKAGE__->register_method ({
>>>  		default => 0,
>>>  		description => "Enables encryption of the OSD."
>>>  	    },
>>> +	    'crush-device-class' => {
>>> +		optional => 1,
>>> +		type => 'string',
>>> +		description => "Set the device class of the OSD in crush."
>>> +	    },
>>
>> why not having an enum with 'nvme', 'ssd', and 'hdd' here?
> Ceph allows the class to be an arbitrary string, eg. my-very-fast-disk.
> 

Is it then "auto-generated" or has ceph an index of known ones floating
around?

We could also add this to the UI, to advanced as editable KVCombobox which is
emptyText "auto", and has "hdd", "nvme" and "ssd" as convenience selectors.
If we can get all currently available ones also (relatively) cheaply we could
add them too.

Re: [pve-devel] applied: [PATCH manager] Allow setting device class on osd create

[Alwin Antreich]

On Fri, Jul 24, 2020 at 11:54:10AM +0200, Thomas Lamprecht wrote:
> Am 7/24/20 um 11:46 AM schrieb Alwin Antreich:
> > On Fri, Jul 24, 2020 at 11:34:33AM +0200, Thomas Lamprecht wrote:
> >> Am 7/23/20 um 3:25 PM schrieb Alwin Antreich:
> >>> In some situations Ceph's auto-detection doesn't recognize the device
> >>> class correctly. The option allows to set it directly on osd create,
> >>> instead of altering it afterwards. This way the cluster doesn't need to
> >>> shift data back and forth unnecessarily.
> >>>
> >>> Signed-off-by: Alwin Antreich <a.antreich@proxmox.com>
> >>> ---
> >>>  PVE/API2/Ceph/OSD.pm | 7 +++++++
> >>>  1 file changed, 7 insertions(+)
> >>>
> >>
> >> applied, thanks - comments still inline
> >>
> >>> diff --git a/PVE/API2/Ceph/OSD.pm b/PVE/API2/Ceph/OSD.pm
> >>> index ceaed129..f1f39bf9 100644
> >>> --- a/PVE/API2/Ceph/OSD.pm
> >>> +++ b/PVE/API2/Ceph/OSD.pm
> >>> @@ -260,6 +260,11 @@ __PACKAGE__->register_method ({
> >>>  		default => 0,
> >>>  		description => "Enables encryption of the OSD."
> >>>  	    },
> >>> +	    'crush-device-class' => {
> >>> +		optional => 1,
> >>> +		type => 'string',
> >>> +		description => "Set the device class of the OSD in crush."
> >>> +	    },
> >>
> >> why not having an enum with 'nvme', 'ssd', and 'hdd' here?
> > Ceph allows the class to be an arbitrary string, eg. my-very-fast-disk.
> > 
> 
> Is it then "auto-generated" or has ceph an index of known ones floating
> around?
AFAICT, Ceph check /sys/block and gathers the information from there and
then decides if it is hdd/ssd/nvme.

> 
> We could also add this to the UI, to advanced as editable KVCombobox which is
> emptyText "auto", and has "hdd", "nvme" and "ssd" as convenience selectors.
> If we can get all currently available ones also (relatively) cheaply we could
> add them too.
I think only the active ones. But I guess it wouldn't hurt if we just
hard-code the usual suspects + auto. :)

Re: [pve-devel] applied: [PATCH manager] Allow setting device class on osd create

[Thomas Lamprecht]

Am 7/24/20 um 2:24 PM schrieb Alwin Antreich:
> On Fri, Jul 24, 2020 at 11:54:10AM +0200, Thomas Lamprecht wrote:
>> Am 7/24/20 um 11:46 AM schrieb Alwin Antreich:
>>> On Fri, Jul 24, 2020 at 11:34:33AM +0200, Thomas Lamprecht wrote:
>>>> Am 7/23/20 um 3:25 PM schrieb Alwin Antreich:
>>>>> In some situations Ceph's auto-detection doesn't recognize the device
>>>>> class correctly. The option allows to set it directly on osd create,
>>>>> instead of altering it afterwards. This way the cluster doesn't need to
>>>>> shift data back and forth unnecessarily.
>>>>>
>>>>> Signed-off-by: Alwin Antreich <a.antreich@proxmox.com>
>>>>> ---
>>>>>  PVE/API2/Ceph/OSD.pm | 7 +++++++
>>>>>  1 file changed, 7 insertions(+)
>>>>>
>>>>
>>>> applied, thanks - comments still inline
>>>>
>>>>> diff --git a/PVE/API2/Ceph/OSD.pm b/PVE/API2/Ceph/OSD.pm
>>>>> index ceaed129..f1f39bf9 100644
>>>>> --- a/PVE/API2/Ceph/OSD.pm
>>>>> +++ b/PVE/API2/Ceph/OSD.pm
>>>>> @@ -260,6 +260,11 @@ __PACKAGE__->register_method ({
>>>>>  		default => 0,
>>>>>  		description => "Enables encryption of the OSD."
>>>>>  	    },
>>>>> +	    'crush-device-class' => {
>>>>> +		optional => 1,
>>>>> +		type => 'string',
>>>>> +		description => "Set the device class of the OSD in crush."
>>>>> +	    },
>>>>
>>>> why not having an enum with 'nvme', 'ssd', and 'hdd' here?
>>> Ceph allows the class to be an arbitrary string, eg. my-very-fast-disk.
>>>
>>
>> Is it then "auto-generated" or has ceph an index of known ones floating
>> around?
> AFAICT, Ceph check /sys/block and gathers the information from there and
> then decides if it is hdd/ssd/nvme.

that's not what I meant, talking about existing classes ;-)

> 
>>
>> We could also add this to the UI, to advanced as editable KVCombobox which is
>> emptyText "auto", and has "hdd", "nvme" and "ssd" as convenience selectors.
>> If we can get all currently available ones also (relatively) cheaply we could
>> add them too.

> I think only the active ones. But I guess it wouldn't hurt if we just
> hard-code the usual suspects + auto. :)
 
As said, if it's editable one can actually enter all information, the provided
ones are just for convenience, check the replication schedule field for what I
mean.

Re: [pve-devel] applied: [PATCH manager] Allow setting device class on osd create

[Alwin Antreich]

On Fri, Jul 24, 2020 at 02:38:50PM +0200, Thomas Lamprecht wrote:
> Am 7/24/20 um 2:24 PM schrieb Alwin Antreich:
> > On Fri, Jul 24, 2020 at 11:54:10AM +0200, Thomas Lamprecht wrote:
> >> Am 7/24/20 um 11:46 AM schrieb Alwin Antreich:
> >>> On Fri, Jul 24, 2020 at 11:34:33AM +0200, Thomas Lamprecht wrote:
> >>>> Am 7/23/20 um 3:25 PM schrieb Alwin Antreich:
> >>>>> In some situations Ceph's auto-detection doesn't recognize the device
> >>>>> class correctly. The option allows to set it directly on osd create,
> >>>>> instead of altering it afterwards. This way the cluster doesn't need to
> >>>>> shift data back and forth unnecessarily.
> >>>>>
> >>>>> Signed-off-by: Alwin Antreich <a.antreich@proxmox.com>
> >>>>> ---
> >>>>>  PVE/API2/Ceph/OSD.pm | 7 +++++++
> >>>>>  1 file changed, 7 insertions(+)
> >>>>>
> >>>>
> >>>> applied, thanks - comments still inline
> >>>>
> >>>>> diff --git a/PVE/API2/Ceph/OSD.pm b/PVE/API2/Ceph/OSD.pm
> >>>>> index ceaed129..f1f39bf9 100644
> >>>>> --- a/PVE/API2/Ceph/OSD.pm
> >>>>> +++ b/PVE/API2/Ceph/OSD.pm
> >>>>> @@ -260,6 +260,11 @@ __PACKAGE__->register_method ({
> >>>>>  		default => 0,
> >>>>>  		description => "Enables encryption of the OSD."
> >>>>>  	    },
> >>>>> +	    'crush-device-class' => {
> >>>>> +		optional => 1,
> >>>>> +		type => 'string',
> >>>>> +		description => "Set the device class of the OSD in crush."
> >>>>> +	    },
> >>>>
> >>>> why not having an enum with 'nvme', 'ssd', and 'hdd' here?
> >>> Ceph allows the class to be an arbitrary string, eg. my-very-fast-disk.
> >>>
> >>
> >> Is it then "auto-generated" or has ceph an index of known ones floating
> >> around?
> > AFAICT, Ceph check /sys/block and gathers the information from there and
> > then decides if it is hdd/ssd/nvme.
> 
> that's not what I meant, talking about existing classes ;-)
I couldn't find an index as such.

> 
> > 
> >>
> >> We could also add this to the UI, to advanced as editable KVCombobox which is
> >> emptyText "auto", and has "hdd", "nvme" and "ssd" as convenience selectors.
> >> If we can get all currently available ones also (relatively) cheaply we could
> >> add them too.
> 
> > I think only the active ones. But I guess it wouldn't hurt if we just
> > hard-code the usual suspects + auto. :)
>  
> As said, if it's editable one can actually enter all information, the provided
> ones are just for convenience, check the replication schedule field for what I
> mean.
+1, I am on it.