Re: [PATCH 0/5] Introduce 'yank' oob qmp command to recover from hanging qemu

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 13.05.2020 um 12:53 hat Dr. David Alan Gilbert geschrieben:
>> * Kevin Wolf (kwolf@redhat.com) wrote:
>> > Am 12.05.2020 um 11:43 hat Daniel P. Berrangé geschrieben:
>> > > On Tue, May 12, 2020 at 11:32:06AM +0200, Lukas Straub wrote:
>> > > > On Mon, 11 May 2020 16:46:45 +0100
>> > > > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
>> > > > 
>> > > > > * Daniel P. Berrangé (berrange@redhat.com) wrote: 
>> > > > > > ...
>> > > > > > That way if QEMU does get stuck, you can start by tearing down the
>> > > > > > least distruptive channel. eg try tearing down the migration 
>> > > > > > connection
>> > > > > > first (which shouldn't negatively impact the guest), and only if 
>> > > > > > that
>> > > > > > doesn't work then, move on to tear down the NBD connection (which 
>> > > > > > risks
>> > > > > > data loss)  
>> > > > > 
>> > > > > I wonder if a different way would be to make all network connections
>> > > > > register with yank, but then make yank take a list of connections to
>> > > > > shutdown(2).
>> > > > 
>> > > > Good Idea. We could name the connections (/yank callbacks) in the
>> > > > form "nbd:<node-name>", "chardev:<chardev-name>" and "migration"
>> > > > (and add "netdev:...", etc. in the future). Then make yank take a
>> > > > list of connection names as you suggest and silently ignore connections
>> > > > that don't exist. And maybe even add a 'query-yank' oob command 
>> > > > returning
>> > > > a list of registered connections so the management application can do
>> > > > pattern matching if it wants.
[...]
>> > > Yes, that would make the yank command much more flexible in how it can
>> > > be used.
>> > > 
>> > > As an alternative to using formatted strings like this, it could be
>> > > modelled more explicitly in QAPI
>> > > 
>> > >   { 'struct':  'YankChannels',
>> > >     'data': { 'chardev': [ 'string' ],
>> > >               'nbd': ['string'],
>> > >        'migration': bool } }
>> > > 
>> > > In this example, 'chardev' would accept a list of chardev IDs which
>> > > have it enabled, 'nbd' would accept a list of block node IDs which
>> > > have it enabled, and migration is a singleton on/off.
>> > 
>> > Of course, it also means that the yank code needs to know about every
>> > single object that supports the operation, whereas if you only have
>> > strings, the objects could keep registering their connection with a
>> > generic function like yank_register_function() in this version.
>> > 
>> > I'm not sure if the additional complexity is worth the benefits.
>> 
>> I tend to agree; although we do have to ensure we either use an existing
>> naming scheme (e.g. QOM object names?) or make sure we've got a well
>> defined list of prefixes.
>
> Not everything that has a network connection is a QOM object (in fact,
> neither migration nor chardev nor nbd are QOM objects).
>
> I guess it would be nice to have a single namespace for everything in
> QEMU, but the reality is that we have a few separate ones. As long as we
> consistently add a prefix that identifies the namespace in question, I
> think that would work.
>
> This means that if we're using node-name to identify the NBD connection,
> the namespace should be 'block' rather than 'nbd'.
>
> One more thing to consider is, what if a single object has multiple
> connections? In the case of node-names, we have a limited set of allowed
> characters, so we can use one of the remaining characters as a separator
> and then suffix a counter. In other places, the identifier isn't
> restricted, so suffixing doesn't work. Maybe prefixing does, but it
> would have to be there from the beginning then.

If I understand it correctly, the argument for encoding the structured
"what to yank" information into a string is ease of implementation.  The
yank core sees only strings, and doesn't care about their contents.
Code registering "yankables" can build strings however it likes, as long
as they're all distinct.  QMP clients need to understand how the strings
are built to be able to yank specific "yankables" (as opposed to blindly
yanking stuff until QEMU gets unstuck").

I disagree with this argument for a number of reasons.

1. Use of strings doesn't reduce complexity, it moves it.

   String:

   * QMP clients may need to parse the strings they get back from
     command query-yank intro structured data.

   * QMP clients may need to format structured data into a string for
     command yank.

   * How structured data is be parsed from / formatted to a string is
     part of the external interface, and needs to be documented, in
     addition to the data structure.

   * The strings need to be kept backward compatible.  We could
     inadvertently create problems like the one you described above,
     where a format like UNIQUE-PREFIX:ID with an unrestricted ID is not
     extensible.

   * Code providing "yankables" needs to somehow ensure their strings
     are distinct.

   Complex type:

   * The result of query-yank is already structured data, no need for
     QMP clients to parse.

   * The argument of yank is structured data, no need to for QMP clients
     to format it into a string first.

   * Documenting just the data structure suffices.

   * Backward compatibility of complex QMP types is a well-understood
     problem.

   * Defining the structure in the QAPI schema as union trivially
     ensures the union branches are distinct.

2. Even with structured yank arguments, the yank core doesn't *need* to
   understand the structure.

   The yank core manages a set of instances.  Each instance associates a
   key with a list of YankFuncAndParam.  This is a basically dictionary.
   All the yank core needs to do with the dictionary keys is looking
   them up.

   The proposed implementation uses a list of instances (which is just
   fine).  All the lookup needs from the keys is an "is equal" relation.

   Checking two QAPI objects for structural equality is admittedly not
   as simple as comparing strings.  It does not, however, require
   understanding their structure.  Two stupid solutions come to mind:

   * Hand-written compare that keeps specifics out of the yank core

     Whatever uses a variant of YankInstance gets to provide a
     comparison function of the variant members.

     Compare the common members (initially just @type, which is the
     discriminator).  Return false if any of them differs.

     Else both instances use the same variant.  Return the value of the
     variant comparison function.

   * Visitors exist

     Convert both YankInstances to QObject, compare with
     qobject_is_equal(), done.

3. In QMP, encoding structured data in strings is anathema :)

[...]