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

[Ján Tomko]

On Mon, Apr 20, 2015 at 12:23:25PM -0400, John Ferlan wrote:
> 
> 
> On 04/20/2015 11:11 AM, Ján Tomko wrote:
> > On Sun, Apr 19, 2015 at 08:49:12PM -0400, John Ferlan wrote:
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1171984
> >>
> >> Use the new virIsSameHostnameInfo API to determine whether the proposed
> >> storage pool definition matches the existing storage pool definition
> >>
> >> Signed-off-by: John Ferlan <jferlan at redhat.com>
> >> ---
> >>  src/conf/storage_conf.c | 11 ++++++++++-
> >>  1 file changed, 10 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> >> index 4852dfb..c1bc242 100644
> >> --- a/src/conf/storage_conf.c
> >> +++ b/src/conf/storage_conf.c
> >> @@ -2415,7 +2415,16 @@ virStoragePoolSourceMatchSingleHost(virStoragePoolSourcePtr poolsrc,
> >>      if (poolsrc->hosts[0].port != defsrc->hosts[0].port)
> >>          return false;
> >>  
> > 
> > This function is called when parsing the configuration, which should not
> > depend on host state.
> > 
> > For example, if libvirt is started really early at boot time and the
> > hostnames cannot be resolved by the DNS yet, they will pass the check
> > but they will disappear on libvirtd restart.
> 
> Hmm... the downside of unreliable dependencies.
> 
> > 
> > The hostname->ip pairings are not stable either, so if we do this, I
> > think it should be done on pool startup, not config parsing.
> > 
> 
> Right, but by the time we get to pool startup we'd be at the same point
> as referenced above - if done early enough at boot time, then it's not
> going to fail, but perhaps better than nothing.
> 

Randomly failing to parse a config is worse than nothing.

> 
> I suppose at least moving to startup allows for better error paths since
> currently errors can be overwritten or ignored.
> 
> 
> >> -    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).

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.


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

Jan
-------------- 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/20150421/6d306a3d/attachment-0001.sig>