* [pve-devel] [PATCH manager] Allow setting device class on osd create
@ 2020-07-23 13:25 Alwin Antreich
2020-07-24 9:34 ` [pve-devel] applied: " Thomas Lamprecht
0 siblings, 1 reply; 7+ messages in thread
From: Alwin Antreich @ 2020-07-23 13:25 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread* [pve-devel] applied: [PATCH manager] Allow setting device class on osd create
2020-07-23 13:25 [pve-devel] [PATCH manager] Allow setting device class on osd create Alwin Antreich
@ 2020-07-24 9:34 ` Thomas Lamprecht
2020-07-24 9:46 ` Alwin Antreich
0 siblings, 1 reply; 7+ messages in thread
From: Thomas Lamprecht @ 2020-07-24 9:34 UTC (permalink / raw)
To: Proxmox VE development discussion, Alwin Antreich
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";
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [pve-devel] applied: [PATCH manager] Allow setting device class on osd create
2020-07-24 9:34 ` [pve-devel] applied: " Thomas Lamprecht
@ 2020-07-24 9:46 ` Alwin Antreich
2020-07-24 9:54 ` Thomas Lamprecht
0 siblings, 1 reply; 7+ messages in thread
From: Alwin Antreich @ 2020-07-24 9:46 UTC (permalink / raw)
To: Thomas Lamprecht; +Cc: Proxmox VE development discussion
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";
> >
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [pve-devel] applied: [PATCH manager] Allow setting device class on osd create
2020-07-24 9:46 ` Alwin Antreich
@ 2020-07-24 9:54 ` Thomas Lamprecht
2020-07-24 12:24 ` Alwin Antreich
0 siblings, 1 reply; 7+ messages in thread
From: Thomas Lamprecht @ 2020-07-24 9:54 UTC (permalink / raw)
To: Alwin Antreich, Proxmox VE development discussion
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.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [pve-devel] applied: [PATCH manager] Allow setting device class on osd create
2020-07-24 9:54 ` Thomas Lamprecht
@ 2020-07-24 12:24 ` Alwin Antreich
2020-07-24 12:38 ` Thomas Lamprecht
0 siblings, 1 reply; 7+ messages in thread
From: Alwin Antreich @ 2020-07-24 12:24 UTC (permalink / raw)
To: Thomas Lamprecht; +Cc: Proxmox VE development discussion
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. :)
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [pve-devel] applied: [PATCH manager] Allow setting device class on osd create
2020-07-24 12:24 ` Alwin Antreich
@ 2020-07-24 12:38 ` Thomas Lamprecht
2020-07-24 12:47 ` Alwin Antreich
0 siblings, 1 reply; 7+ messages in thread
From: Thomas Lamprecht @ 2020-07-24 12:38 UTC (permalink / raw)
To: Alwin Antreich, Proxmox VE development discussion
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.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [pve-devel] applied: [PATCH manager] Allow setting device class on osd create
2020-07-24 12:38 ` Thomas Lamprecht
@ 2020-07-24 12:47 ` Alwin Antreich
0 siblings, 0 replies; 7+ messages in thread
From: Alwin Antreich @ 2020-07-24 12:47 UTC (permalink / raw)
To: Thomas Lamprecht; +Cc: Proxmox VE development discussion
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-07-24 12:47 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-23 13:25 [pve-devel] [PATCH manager] Allow setting device class on osd create Alwin Antreich
2020-07-24 9:34 ` [pve-devel] applied: " Thomas Lamprecht
2020-07-24 9:46 ` Alwin Antreich
2020-07-24 9:54 ` Thomas Lamprecht
2020-07-24 12:24 ` Alwin Antreich
2020-07-24 12:38 ` Thomas Lamprecht
2020-07-24 12:47 ` Alwin Antreich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox