Re: [Qemu-devel] [PATCH v19 4/5] block/gluster: using new qapi schema

[Eric Blake]

On 07/19/2016 05:12 AM, Markus Armbruster wrote:
> Cc'ing Eric, because I'd like his advice on a couple of points.

I've been following the thread, but with this specific invitation, I'll
definitely chime in.


>> int
>> pub_glfs_set_volfile_server (struct glfs *fs, const char *transport,
>>                              const char *host, int port)
>>
>> So, I hope you stand with me, in making port as int;
> 
> If we consider just interfacing with (the current version of)
> glusterfs-devel, int is the natural type of port.
> 
> However, we already have a related abstraction: InetSocketAddress.  If
> it fits, we should probably reuse it.  If we decide not to reuse it now,
> we may still want to minimize differences to keep the door open for
> future reuse.

Furthermore, just because gluster code doesn't accept a string does not
prevent qemu from accepting a string, looking it up in the database of
named ports, and passing the corresponding number on to gluster.  I am
very strongly in favor of supporting named ports in the qemu interface,
even if it means more work under the hood for qemu to pass on to
gluster.  Whether we support that by an alternate between int and
string, or keep symmetry with existing interfaces being string only, is
where it becomes a judgment call (and going string-only for now can
always be improved later for convenience).

> 
> InetSocketAddress's port is str.  If we make GlusterInetSocketAddress's
> port an integer, then a future unification with InetSocketAddress will
> have to make port an alternate.  Not impossible, but why tie our hands
> that way now?  But let's not get bogged down in this detail now.  We
> first have to decide whether to reuse InetSocketAddress now (more on
> that below).  If yes, the question is moot.
> 

>>>>>   Changes in GlusterInetSocketAddress over InetSocketAddress:
>>>>>
>>>>>   - @port is optional
>>>>>
>>>>>     Convenience feature.  We can discuss making it optional in
>>>>>     InetSocketAddress, too.
>>>>
>>>> sure.
> 
> Of course, it's too late in the development cycle for making it optional
> *now*.
> 
> If we reuse InetSocketAddress now, as is, then the QMP client has to
> specify the Gluster port, even though it's usually 24007.  For command
> line and HMP, we can still make it optional.  I think that's tolerable.
> We can investigate making it optional later.

Agree - we're too late to make it optional in 2.7, but there is nothing
preventing us from making it optional in the future, and it's neither
too much burden on libvirt to supply a sane default if it is mandatory,
nor too hard for the command line and HMP to make it optional on top of
a mandatory QMP, for the sake of human interaction.


> 
> Port ranges make sense for some users of InetSocketAddress, but not for
> others.  Gluster is of the latter kind.
> 
> So far, the latter kind simply uses InetSocketAddress, and either
> rejects @to or ignores it.  The latter would be a bug.
> 
> We could do the same for Gluster: if has_to, error out.
> 
> Alternatively, we could split InetSocketAddressRange (with @to) off
> InetSocketAddress (delete @to).  Makes range support visible in
> introspection.

Ultimately, I think we should split InetSocketAddress. But we're too
late to do it in 2.7.

> 
> If we use InetSocketAddress as is now, and have gluster.c reject @to, we
> can still split off InetSocketAddressRange later.  The external
> interface doesn't care whether @to is rejected as unknown parameter by
> QAPI or as unsupported parameter by gluster.c.
> 

Agree - at this point, manually rejecting has_to will let us reuse the
type, and would then be part of the cleanup work when 2.8 splits
InetSocketAddress.


> Now let me summarize.  We could do without GlusterInetSocketAddress,
> because:
> 
> * SocketAddress's non-optional @port is a minor inconvenience which
>   we can address in the future without breaking compatibility.
> 
> * SocketAddress's string @port is a minor inconvenience for the C code
>   using it.  Keeping the external interface consistent (same type for
>   TCP ports everywhere) is worth some inconvenience in the C code.
> 
> * SocketAddress's @to complicates the external interface, but the
>   complication exists elsewhere already.  gluster.c can reject @to.  We
>   can clean up the external interface in the future without breaking
>   compatibility.

In fact, introspection will be nicer when we split InetSocketAddress to
no longer include 'to', as you will then be able to precisely determine
which uses support ranges.  For now, management apps just have to be
aware that ranges may be rejected.

> 
> * SocketAddress's @ipv4, @ipv6 cannot be fully implemented by gluster.c,
>   yet.  gluster.c can simply reject the settings it doesn't implement,
>   until they get implemented.
> 
> Reasons for having a separate GlusterInetSocketAddress:
> 
> * Slightly less code in gluster.c.  I reject that reason.  Keeping the
>   interface lean is worth some extra code.
> 
>   Note that extra schema definitions to avoid code complications may be
>   fine as long as they don't affect external interfaces.
> 
> * Makes non-support of port ranges and IPv6 visible in introspection.
>   That's a valid argument, but it's an argument for having
>   Inet4SockAddress, not for GlusterInetSocketAddress.
> 
>   Eric, do you think there's a need for introspecting IPv6 support?

I'm borderline on that one.  If the error message given when attempting
an IPv6 address is nice enough, then libvirt can just blindly try an
IPv6 address and inform the user that qemu/gluster is too old to support
it.  If the error message is lousy, then being able to introspect
whether IPv6 support is present would let libvirt provide a saner error
message.  But at the moment, I'm not convinced that introspection alone
is reason to create a struct without IPv6 now, where IPv6 is added later
once gluster supports it.

Furthermore, a system administrator that NEEDS to use IPv6 is very
likely to upgrade their system to support things before trying to use
IPv6.  So in this case, I'm leaning towards just reusing the type and
rejecting IPv6 in the gluster code until it works.


> 
> Similar design question as for @ipv4, @ipv6: reuse of SocketAddress is
> possible, but the C code has to reject options it doesn't implement,
> i.e. "type": "fd".  Non-support of fd isn't visible in introspection
> then.
> 
> Eric, do you think there's a need for introspecting fd support?

There's no easy way to declare one QAPI union that is a superset of
another (all the same branches as before, plus these additional
branches); it requires duplication of all the common branches.
Arguably, we could extend QAPI if we find ourselves doing that a lot, to
make it supported with less copy and paste; but manual duplication right
now doesn't hurt.

Here, I think introspection is probably more useful, particularly since
there are security aspects in fd passing (where libvirt can pre-open the
connection) that are nicer to know up front if they will work, and where
it is less obvious to a system administrator that they need/want to use
fd passing under the hood.  IPv6 is very easy to say: "will my
deployment need IPv6 addresses - if so, upgrade gluster to a version
that supports IPv6".  But fd passing is not so straightforward.  So I
think being able to introspect this one is useful - but I also think
that having a union that shares the same underlying types for the common
branches is still slightly nicer than having different types for the
same-named branches (that is, since both the reduced union without 'fd'
support and the full union will have 'unix' and 'inet' branches, both
the 'unix' and 'inet' branch should use the same underlying type rather
than being subtly different).

> 
> Additional design question: do we want to move away from SocketAddress's
> pointless nesting on the wire for new interfaces?  I.e. move from
> something like
> 
>     { "type": "tcp", "data": { "host": "gluster.example.com", ... }
> 
> to
> 
>     { "type": "tcp", "host": "gluster.example.com", ... }
> 
> This isn't a Gluster question, it's a general question.  Eric, what do
> you think?
> 

Nested vs. flat is somewhat cosmetic - machine-generated code (like
libvirt's interactions) can cope with either.  On the other hand, adding
a flat type now may make OTHER blockdev-add drivers easier to QAPIfy,
such as nbd and sheepdog, and I kind of like the simplicity afforded by
a flat type.

>> { 'union': 'SocketAddress',
>>   'data': {
>>     'inet': 'InetSocketAddress',
>>     'unix': 'UnixSocketAddress',
>>     'fd': 'String' } }

I also think that if we add a flat type, it would be nicer to use
'fd':'str' instead of 'fd':'String' (which is itself another pointless
nesting due to backwards compatibility), at least for the version of the
flat type where 'fd' is supported.

>>
>> after removing 'fd' part which is not supported now, this look like
>>
>> { 'union': 'GlusterSocketAddress',
>>   'data': {
>>     'inet': 'GlusterInetSocketAddress',
>>     'unix': 'UnixSocketAddress' } }
>>
>> What do you think ?
> 
> This is fine if
> 
> * we decide we want a new GlusterInetSocketAddress instead of reusing
>   InetSocketAddress, and
> 
> * we decide we don't want to avoid nesting on the wire.
> 
> But we haven't settled these questions.  I'd like to settle them today,
> taking Eric's advice into account.

I think sharing InetSocketAddress is reasonable, but having separate
GlusterSocketAddress (or more generically, FlatSocketAddress that can be
shared by gluster, sheepdog, and nbd), seems okay.  I'm also not fussed
about naming - naming it 'GlusterSocketAddress' now and renaming it
'FlatSocketAddress' later when it gets more use does not impact
introspection.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature