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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

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

Let's do just string now.  It's consistent with what we use elsewhere,
and doesn't tie our hands.

To avoid further delay, arbitrarily restrict the string to port number
for now.

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

Agreed then.

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

Makes sense.

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

Okay, let's reuse InetSocketAddress, and reject the following in C code:

* has_to

* has_ipv4 || has_ipv6

  This is overkill.  We only have to reject the combinations that
  normally result in v6, but don't with Gluster.
  inet_ai_family_from_address()'s function comment documents how we pick
  the address family.  I'm proposing overkill since this is at v19, and
  we need to make progress.

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

Yes.

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

Agreed.

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

Makes sense.

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

The longer I do QAPI, the more I dislike "simple" unions.

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

Oh yes.  String's documentation justifies its existence with "to be
embedded in lists."  Well, strList exists.

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

Renaming won't be a big deal because it should affect mostly just
gluster.c.  So let's start with

{ 'enum': 'GlusterTransport',
  'data': [ 'unix', 'tcp' ] }

{ 'union': 'GlusterServer',
  'base': { 'type': 'GlusterTransport' },
  'discriminator': 'type',
  'data': { 'unix': 'UnixSocketAddress',
            'tcp': 'InetSocketAddress' } }

I.e. the current patch with GlusterUnixSocketAddress and
GlusterSocketAddress dropped.  I'll discuss the next steps with Prasanna
on IRC right away.

This GlusterServer type can be compatibly evolved into a perfectly
generic replacement for the current SocketAddress type to be used in new
code.  The current SocketAddress is only used by commands
nbd-server-start and chardev-add.