[pve-devel] [PATCH storage] Exclude general MON section in get_monaddr_list

[Thomas Lamprecht]

On 1/9/19 2:27 PM, Alwin Antreich wrote:
> On Wed, Jan 09, 2019 at 02:04:17PM +0100, Thomas Lamprecht wrote:
>> On 1/9/19 11:59 AM, Alwin Antreich wrote:
>>> If a general MON section exists in the ceph.conf, the get_monaddr_list
>>> adds a undefined entry and a cephfs storage can't be mounted anymore.
>>>
>>> Signed-off-by: Alwin Antreich <a.antreich at proxmox.com>
>>> ---
>>>  PVE/CephConfig.pm | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/PVE/CephConfig.pm b/PVE/CephConfig.pm
>>> index b420fcc..aad2701 100644
>>> --- a/PVE/CephConfig.pm
>>> +++ b/PVE/CephConfig.pm
>>> @@ -107,7 +107,7 @@ sub get_monaddr_list {
>>>      }
>>>  
>>>      my $config = $parse_ceph_file->($configfile);
>>> -    @$server = sort map { $config->{$_}->{'mon addr'} } grep {/mon/} %{$config};
>>> +    @$server = sort map { $config->{$_}->{'mon addr'} } grep {/mon\./} %{$config};
>>
>> hmm, I had something in my tree for this, just forgot it -.-
>>
>> rebased it'd be:
>>
>> ----8<----
>> diff --git a/PVE/CephConfig.pm b/PVE/CephConfig.pm
>> index aad2701..75c4038 100644
>> --- a/PVE/CephConfig.pm
>> +++ b/PVE/CephConfig.pm
>> @@ -107,9 +107,17 @@ sub get_monaddr_list {
>>      }
>>
>>      my $config = $parse_ceph_file->($configfile);
>> -    @$server = sort map { $config->{$_}->{'mon addr'} } grep {/mon\./} %{$config};
>>
>> -    return join(',', @$server);
>> +    my $monlist;
>> +    for my $monid (grep { /mon/ } %{$config}) {
>> +       if (defined($config->{$monid}->{'mon addr'})) {
>> +           my $prefix = defined($monlist) ? ',' : '';
>> +
>> +           $monlist = "${prefix}$config->{$monid}->{'mon addr'}";
>> +       }
>> +    }
>> +
>> +    return $monlist;
>>  };
>>
>>  sub hostlist {
>> --
>>
>> this ensures that you really only pick those entries with a defined 'mon addr' up,
>> because theoretically you could have entries without one (even if it may not make
>> much sense). Yours works naturally too, at the moment at least and it keeps the
>> code a bit more concise. Maybe @Wolfgang can tie break this ;) 
> My take on this. As of now, we know our setup writes the config and only
> in a hyper-converged szenario we do the lookup, else the MON addresses
> need to be provided in the storage.cfg.

if this would help we wouldn't had issues at the first place, people edit their
ceph.conf, if hyperconverged or not, so as long it's easy I'd try to check for the
actual thing, not for a heuristic.

> 
> And the general MON section also can host a 'mon addr' with all MONs
> (csv) that should be contacted on service startup.
> 

hmm, ok, so as long as we ensure that the addresses are unique were good?
Or, as easy alternative do both, grep after /mon\./ and check 'mon addr'
definedness?