From: Aaron Lauterer <a.lauterer@proxmox.com>
To: Wolfgang Bumiller <w.bumiller@proxmox.com>
Cc: pve-devel@lists.proxmox.com
Subject: Re: [pve-devel] [PATCH manager v2] fix #4631: ceph: osd: create: add osds-per-device
Date: Wed, 23 Aug 2023 11:38:47 +0200 [thread overview]
Message-ID: <6cc3a0c1-9cbd-484a-9737-c07552825beb@proxmox.com> (raw)
In-Reply-To: <orv7p6bohtn7qss26vni3xl4fpublbpr2ejhav3h7nyuxd3gym@242lc6srr6ca>
>>
>> die "unable to get fsid\n" if !$monstat->{monmap} || !$monstat->{monmap}->{fsid};
>
> Does it make sense to still keep this line above if we don't use fsid
> anymore? (And if not, $monstat then also becomes unused.)
>
thanks for catching this, will remove it
>> - my $fsid = $monstat->{monmap}->{fsid};
>> - $fsid = $1 if $fsid =~ m/^([0-9a-f\-]+)$/;
>>
>> my $ceph_bootstrap_osd_keyring = PVE::Ceph::Tools::get_config('ceph_bootstrap_osd_keyring');
>>
>> @@ -470,7 +484,10 @@ __PACKAGE__->register_method ({
>> $test_disk_requirements->($disklist);
>>
>> my $dev_class = $param->{'crush-device-class'};
>> - my $cmd = ['ceph-volume', 'lvm', 'create', '--cluster-fsid', $fsid ];
>> + # create allows for detailed configuration of DB and WAL devices
>> + # batch for easy creation of multiple OSDs (per device)
>> + my $create_mode = $param->{'osds-per-device'} ? 'batch' : 'create';
>> + my $cmd = ['ceph-volume', 'lvm', $create_mode ];
>> push @$cmd, '--crush-device-class', $dev_class if $dev_class;
>>
>> my $devname = $devs->{dev}->{name};
>> @@ -504,8 +521,11 @@ __PACKAGE__->register_method ({
>> push @$cmd, "--block.$type", $part_or_lv;
>> }
>>
>> - push @$cmd, '--data', $devpath;
>> + push @$cmd, '--data' if $create_mode eq 'create';
>> + push @$cmd, $devpath;
>
> ^ not the biggest fan of mixing positional arguments and options - does
> `ceph-volume` support `--` to separate positional arguments explicitly?
> If so, we should use it.
> ceph-volume lvm create --osds-per-device 3 -- /dev/path
> as opposed to
> ceph-volume lvm create /dev/path --osds-per-device 3
> that's just bad style and potentially dangerous
good point. I'll rework the building of the call
>
>> push @$cmd, '--dmcrypt' if $param->{encrypted};
>> + push @$cmd, '--osds-per-device', $param->{'osds-per-device'}, '--yes', '--no-auto'
>> + if $create_mode eq 'batch';
>>
>> PVE::Diskmanage::wipe_blockdev($devpath);
>>
>> --
>> 2.39.2
prev parent reply other threads:[~2023-08-23 9:38 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-21 11:45 Aaron Lauterer
2023-08-23 9:05 ` Wolfgang Bumiller
2023-08-23 9:38 ` Aaron Lauterer [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6cc3a0c1-9cbd-484a-9737-c07552825beb@proxmox.com \
--to=a.lauterer@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
--cc=w.bumiller@proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox