Re: [pve-devel] [PATCH manager v2] fix #4631: ceph: osd: create: add osds-per-device

[Aaron Lauterer <a.lauterer@proxmox.com>]

>>   
>>   	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