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

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.

>>> 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.

>>> 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. :)

> > 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).