Re: [pve-devel] [PATCH pve-storage 3/8] cephconfig: support sections in the format of [client.$NAME]

[Max Carrara <m.carrara@proxmox.com>]

On 1/31/24 14:18, Fabian Grünbichler wrote:
> On January 30, 2024 7:40 pm, Max Carrara wrote:
>> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
>> ---
>>  src/PVE/CephConfig.pm | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/src/PVE/CephConfig.pm b/src/PVE/CephConfig.pm
>> index 6b10d46..46b92ea 100644
>> --- a/src/PVE/CephConfig.pm
>> +++ b/src/PVE/CephConfig.pm
>> @@ -77,6 +77,7 @@ sub write_ceph_config {
>>  
>>      &$cond_write_sec('global');
>>      &$cond_write_sec('client');
>> +    &$cond_write_sec('client\..*');
>>  
>>      &$cond_write_sec('mds');
>>      &$cond_write_sec('mon');
> 
> this whole code is a bit weird (pre-existing, not your patch in
> particular)..
> 
> should we maybe switch it to
> - keep track of sections which were already written
> - write out all not-yet-written sections as a last step?

I agree that it's somewhat strange - I initially stumbled across it *after*
finishing patch 5 and I discovered that it wouldn't write the 'client.crash'
section at all. I figured we might only allow certain sections in that case.

`ceph_parse_config` parses the *whole* config anyway, so I don't see why
we couldn't just also support writing "arbitrary" sections.

> 
> else, a RMW cycle might lose config sections just because this code is
> not aware of them? 

That is indeed what happened, but as stated above, I had assumed we only
allow specific sections.

while we're at it, double-checking how the ceph
> parser handles sections with whitespace in their name and other funny
> business might be a good idea, just to prevent any discrepancy between
> our parser and theirs..

Will check, thanks!

Overall, I'll adapt this in v2 to support writing arbitrary sections while
also checking whether it's in line with the way Ceph handles things.

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