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

[John Ferlan]

On 04/30/2015 05:59 AM, Ján Tomko wrote:
> On Wed, Apr 29, 2015 at 12:37:38PM -0400, John Ferlan wrote:
>>
>>
>> On 04/23/2015 07:50 AM, Ján Tomko wrote:
>>> On Wed, Apr 22, 2015 at 01:17:20PM -0400, John Ferlan wrote:
>> ...
>>
>> Having the dialog in your other series caused me to remember there was
>> still a question here.
>>
>>>>>
>>>>>>>
>>>>>>> Or just disallow starting two pools with the same targetname, since they
>>>>>>> are supposed to be unique?
>>>>>>>
>>>>>>
>>>>>> 'targetname' as in ?
>>>>>
>>>>> The <source><device path='xxxx'> attribute of the iscsi pool, which we
>>>>> feed to iscsiadm via --targetname.
>>>>>
>>>>
>>>> Oh - OK - that check is done, but the results overridden if the
>>>> <source...> duplicate check fails. 
>>>
>>> I don't see the check being done anywhere.
>>>
>>
>> virStoragePoolSourceFindDuplicate for VIR_STORAGE_POOL_ISCSI calls
>> virStoragePoolSourceFindDuplicateDevices which does the dual pool search
>> through source.devices[i].path (or the IQN for the device).
>>
>> If a duplicate is found, then it would check if the source hostname's
>> match. The formatstorage page describes "Will be used in combination
>> with a directory or device element." with regards to unique
>> identification.
> 
> That is the generic description shared with all the pools. Maybe the
> docs need some clarification?
> 

So you feel it's OK or better to document and restrict something that
could essentially work given some code?  IOW: For this one pool type it
is better to restrict based solely on the IQN - that's a preferable
solution? Because it's not worth doing that much work just to work
around misconfigured systems or a perception of misconfiguration?

>> Since port is optional, even that check is sketchy since
>> one could have the same name, but not provide the port number in the
>> incoming definition and thus have a duplicate.
>>
>> I suppose allowing two different 'iscsid' servers to advertise the same
>> "name" isn't necessarily an issue. It could allow for someone to set up
>> some sort of hot standby or backup (or who knows what) on separate
>> servers without needing to manage the IQN mapping between the two.
>>
> 
> Even though it is possible to configure two servers with the same IQN,
> I don't see why we should support starting pools with both of them at
> at the same time.
> 

Because we have already supported it and technically it can work if
you've done your configuration correctly and (more or less) know what
you're doing.

>> In the long run, the path in a vol-list is a combination of
>> /dev/disk-by-path (or <target... <path>...>), the host name/addr, and
>> the IQN such as:
>>
>> /dev/disk/by-path/ip-192.168.122.1:3260-iscsi-iqn.2013-12.com.example:iscsi-chap-netpool-lun-1
>>
>> and this links to the block device on the host (eg, ../../dev/sdb).
>>
>> If there was a second pool started using the same "host" as the first
>> pool, but just by a different name, it too would use the same block
>> device, so now there would be two pools using the same device. Since
>> using the same device isn't allowed for other pools, the iSCSI pool
>> should also block usage.
> 
> So we have bug A - starting two pools from the same host with the same IQN
> 
>> And yes, a completely separate host using the
>> same IQN would also use the same block device (but that's a different
>> bug IMO).
>>
> 
> and bug B - starting two pools from different hosts with the same IQN
> 
> Since we can't reliably tell it the hosts are different or the same,
> and I doubt the usefulness of two different hosts using the same IQN,
> why not just reduce this to one bug and reject duplicate IQNs regardless
> of the host?
> 


IMO, using address name resolution while not a "perfect solution" is
better than deciding to disallow something that someone may have a
reason to configure in such a manner. If the name resolution causes an
issue due to unreliable DNS or some other DNS factor beyond the scope of
libvirt, then the host configuration has far greater issues. I could
just as easily claim I doubt someone would have such an unreliable DNS
configuration, but I'm sure I'd be wrong too!  Still having an
unreliable DNS is allowed.

>> Perhaps the ultimate root cause is as you pointed out along the way that
>> virStorageBackendISCSISession just looks for (and assumes) the
>> 'targetname' is unique for the host (assumes that the hostname checks
>> already rooted out differences).  But that shows the second bug with
>> this code is that we could have the two hosts with the same IQN on each,
>> but our search will only ever find the "first" one (the regex in
>> virISCSIExtractSession doesn't compare hostnames).
> 
> The iscsiadm output only contains IP addresses for me.
> 

>From my quick read of the source code, the open-iscsi code uses
getaddrinfo for output. And the session matching code uses a two level
loop (usr/iscsi_util.c - iscsi_addr_match()) which compares a stored
address against the incoming/proposed session address (uses ai_addrlen
and ai_addr comparisons) after the getaddrinfo.

So essentially not much different than what I proposed. The open-iscsi
code though is claiming the two are the same, so it won't create a new
session which is why we get into the situation we get into.

>>
>> So while even my .last of this and my v2 series doesn't resolve the
>> second issue, I believe they do at least inhibit the same host by a
>> different name issue.
>>
>> If the v2 of the series was accepted, then the "next step" would be to
>> add/extract that hostname:port from the iscsiadm command and determine
>> whether it "matched" the expected one so that we could find that
>> theoretical second/different host using the same IQN as the first.
> 
> I don't think doing that much work just to work around misconfigured
> systems is worth it.
> 

There is a saying "Imitation is the sincerest form of flattery"... I
wonder how many people just cut-n-paste what's in the docs to get things
to work? In this case, it may be considered babysitting those that
aren't as proficient, but it may also help in the case where someone
doesn't realize their mistake. While we may find the practice of copying
a configuration that works less than desirable, it can happen. While we
don't think someone would configure one pool using the IP Address and
another pool using a name, it can happen.

I think restricting on the same IQN takes a big hammer approach, but if
that's what is desired because it's felt that it's either too much code
to add or because DNS is misconfigured, misbehaving, or not reliable
enough, then while I technically disagree, I suppose since we can make
up the rules to play by.

BTW: Beyond this bz (1171984), there is an iSCSI ipv4 vs. ipv6 host name
configuration issue described in bz1188463 and bz1207929 which describes
a usage with gluster volume lookups where a failure is declared
seemingly because the pool is defined with the IP address while the
vol-name lookup was done using a name. The changes in v2 could easily be
"reused" in order to ascertain that the name and number match. Beyond
that it'd be up the gluster code to do it's own resolution in order to
find the target.

John