[libvirt] [PATCH 7/7] storage: Check for duplicate host for incoming pool def

[John Ferlan]

On 04/20/2015 11:11 AM, Ján Tomko wrote:
> On Sun, Apr 19, 2015 at 08:49:12PM -0400, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1171984
>>
>> Use the new virIsSameHostnameInfo API to determine whether the proposed
>> storage pool definition matches the existing storage pool definition
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/conf/storage_conf.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
>> index 4852dfb..c1bc242 100644
>> --- a/src/conf/storage_conf.c
>> +++ b/src/conf/storage_conf.c
>> @@ -2415,7 +2415,16 @@ virStoragePoolSourceMatchSingleHost(virStoragePoolSourcePtr poolsrc,
>>      if (poolsrc->hosts[0].port != defsrc->hosts[0].port)
>>          return false;
>>  
> 
> This function is called when parsing the configuration, which should not
> depend on host state.
> 
> For example, if libvirt is started really early at boot time and the
> hostnames cannot be resolved by the DNS yet, they will pass the check
> but they will disappear on libvirtd restart.

Hmm... the downside of unreliable dependencies.

> 
> The hostname->ip pairings are not stable either, so if we do this, I
> think it should be done on pool startup, not config parsing.
> 

Right, but by the time we get to pool startup we'd be at the same point
as referenced above - if done early enough at boot time, then it's not
going to fail, but perhaps better than nothing.


I suppose at least moving to startup allows for better error paths since
currently errors can be overwritten or ignored.


>> -    return STREQ(poolsrc->hosts[0].name, defsrc->hosts[0].name);
>> +    if (STRNEQ(poolsrc->hosts[0].name, defsrc->hosts[0].name)) {
>> +        /* Matching just a name isn't reliable as someone could provide
>> +         * the name for one pool and the IP Address for another pool, so
> 
> Resolving them is IMHO just as unreliable.
> 
> Re: the original bug - is it possible to check that we have connected to
> a session with a different hostname than what we requested?

What does the connected session hostname have to do with the original
bug?  The bug requests checking that an iSCSI pool doesn't use a "<host
name='<some IP Address>'..." for one pool:

    <host name='10.66.6.12' port='3260'/>

and a resolved name for another pool on the same host:

    <host name='test1' port='3260'/>

Where :

# cat /etc/hosts
10.66.6.12 test1

The bug pointed out iSCSI in particular, but since other pools use the
<source... <host name='%s'.../>... /> XML formatting it seemed logical
to have them all use the same checks.

> 
> Or just disallow starting two pools with the same targetname, since they
> are supposed to be unique?
> 

'targetname' as in ?

A 'target.path' per pool would need to be unique, but using the same
target.path into two different networked pools is something that should
work. And the pool target path (/dev/disk/by-path) is used by multiple
iSCSI pools on the same host, so it cannot be used as something unique
per host.


John