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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list