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

[Ján Tomko]

On Wed, Apr 22, 2015 at 01:17:20PM -0400, John Ferlan wrote:
> 
> ...
> 
> 
> >>>> -    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.
> >>
> > 
> > The bug requests that this should be forbidden when defining the pool
> > (which is NOTABUG), because we then allow starting both of them, but
> > stopping one of them also stops the other one, while libvirt still
> > thinks it's up (this is the real bug).
> 
> Sure the part about causing an error at define time I see your point,
> but extrapolating a bit at startup time it's still an issue to have two
> pools pointing to the same source.
> 
> Just closing it NOTABUG because the wording isn't precise doesn't mean
> that part of the bug still is valid and needs some sort of resolution.
> 

I don't think we should guard everything and put pillows on every sharp
edge. Doing a STREQ on the provided source paths provides some
convenience to the user, with little effort. Resolving every hostname is
much more effort and not that foolproof.

Yes, if we leave the check out, you will be able to start two pools
pointing to the same host. Changes done to one pool won't be reflected in the
other. We also allow to two domains pointing to the same storage.

> Without letting two pools use the same network source host and device
> target path, we'd never be faced with the shutdown one conundrum. We
> disallow certain duplication at definition time, but network pools have
> an extra problem of source host resolution that's not resolved (yet).
> 
> I can take my 'localhost' netfs pool, change a couple of parameters
> (name to prefix with "dup-", targetpath to prefix with "dup-", and host
> name to be '127.0.0.1') and can start two pools up looking at the same
> source.
> 
> The shutdown path is just "assuming" that the startup path disallowed
> duplicate pools pointing at the same source paths, but I believe
> allowing a pool to be started that's looking at the same target is still
> a bug.
> 

For netfs, I don't see the problem - we create a separate mount, and
unmount it separately. (We don't actually check if it's the requested
host that's mounted there, but at least it doesn't block shutdown).

> > 
> > We definitely shouldn't resolve the hostnames when we parse the XML, the
> > XML parser/formatter should not depend on the host state.
> > 
> > Resolving the hostname on pool startup would at least remove the
> > roulette from XML parser, but still won't be foolproof -
> > multiple IP addresses can point to the same host.
> > More importantly, it won't solve the underlying issue:
> > 
> > We don't care what host we connect to.
> > 
> > As long as virStorageBackendISCSISession finds the <source><device path>
> > in the output of iscsiadm --mode session, the pool is marked as started.
> > 
> > While it might make sense to have the pool accessible via different
> > networks (or different address families), our iscsi backend is not ready
> > for that.
> > 
> > I don't know how to allow that, (without guessing what iscsiadm resolved
> > the hostname to), but marking the pool as started, if the session was
> > created by another pool is wrong.
> > 
> 
> 
> Right - the iscsi backend has made extra assumptions based on the theory
> that the source host checks ensured that the source host name wasn't
> duplicated.
> 

No, it made the assumptions purely on the source device path. Adding a
source host check won't solve the issue on the shutdown path.

We should either disallow duplicate device paths, or handle them
correctly. Doing so in the XML parser feels wrong, but we already
check for duplicate paths there (I think the only reason it is done
on parsing is because we automatically mark any pools that are already
mounted as started).

> I know the NETFS pool doesn't suffer from the same destroy issue, I'm
> not so sure about Gluster and Sheepdog as I don't have a configuration
> of them handy.
> 
> > 
> >>>
> >>> 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.

> If we did have two different hosts,
> then they could conceivably have the same target name/path. That could
> be a way to configure some sort of hot standby or primary/secondary
> where the device paths are the same, but the hosts change.  For iSCSI
> it'll create different iqn, portal results.

The iscsi backend doesn't care about iqn an portal results, if it sees a
session with a matching device path.

Jan
> 
> John
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150423/98251250/attachment-0001.sig>