[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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




On 05/04/2015 09:44 AM, Ján Tomko wrote:
> On Thu, Apr 30, 2015 at 10:16:55AM -0400, John Ferlan wrote:
>> On 04/30/2015 05:59 AM, Ján Tomko wrote:
>>> On Wed, Apr 29, 2015 at 12:37:38PM -0400, John Ferlan wrote:
>>>> 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?
>>
> 
> d/perception of / ;)
> 
> Yes.
> 

Wasn't too much work for iscsid which is able to "use" an existing
session if it determines the IP Addresses are the same. Technically it's
not a misconfiguration, it seems just an artificial limitation due to an
inability to trust DNS name resolutions.

>>>> 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.
>>
> 
> But we haven't supported it - starting the second pool will
> short-circuit because we already see a session with that IQN there.
> 

Whether "we" saw it or not, iscsid will "match" if it determines that
the host:port portion is a duplicate.

>>>> 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.
>>
> 
> The difference would be in the amount of code added to libvirt. :)
> 

Not sure that should be a reason to not accept something. The first set
of patches associated with virIsValidHostname were an "afterthought"
after adding the name resolution checking code.  There were added
because I thought it might be a good thing to check and/or notify that
the name to be used wasn't valid.  Allowing netfs, gluster, iscsi, and
sheepdog to go through their startups only to fail is still an option. I
guess it all depends on what you consider too much

>>
>> 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.
> 
> The lookup code in v2 can be used by gluster regardless of its usage by
> iSCSI pool (where checking the IQN should be enough) or NFS (where I'm
> not sure we want to check the hostname at all - historically we've
> allowed starting a NFS pool as long as there was something mounted to
> the target path).
> 

So it seems at least patch 6 is useful and that's the bulk of the code.
Patch 7 just uses that. Whether patches 1-5 are dropped due to code
bloat is no big deal since the server startup will error anyway (and in
the case of gluster leak memory according to Peter's description).

John


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]