Re: [PATCH 1/2] nbd: Add an unstable query for the client state

[Markus Armbruster <armbru@redhat.com>]

Guillaume Vincke <guillaume.vincke@ovhcloud.com> writes:

> At the moment, NBD client internals cannot be accessed. Therefore,
> tests have to infer the client state from I/O results and wait with
> sleeps, which is racy.
>
> Add the x-debug-query-nbd-client QMP command that reports the state of
> the client's connection and of its connector. Other fields will be
> added when needed.
>
> The next patch uses this command to make the nbd-stale-connection test
> deterministic.
>
> Signed-off-by: Guillaume Vincke <guillaume.vincke@ovhcloud.com>

[...]

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 23c5787d99..973be06dfd 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json

Why block-core.json and not block-export.json?

> @@ -2217,6 +2217,85 @@
>    'features': [ 'unstable' ],
>    'allow-preconfig': true }
>  
> +##
> +# @XDbgNbdClientState:
> +#     Enum of NBD client state.

"An enumeration of NBD client states" for consistency with enum
descriptions elsewhere.  Or maybe simply "NBD client state".

> +#
> +# @connecting-wait: No active connection.  A request waits for the
> +#     connection attempt to finish, while reconnect-delay has not
> +#     expired.
> +#
> +# @connecting-nowait: No active connection.  A request fails at once
> +#     if no channel is ready.
> +#
> +# @connected: Client has a valid connection.

Double-checking: when we're in this state, there is a client connected.
Correct?

> +#
> +# @quit: Client will not reconnect.

What does this one mean exactly?

> +#
> +# Since: 11.2

Making 11.2 seems unlikely.

> +##
> +{ 'enum': 'XDbgNbdClientState',
> +  'data': [ 'connecting-wait', 'connecting-nowait', 'connected', 'quit' ] }
> +
> +##
> +# @XDbgNbdClientConnectorState:
> +#     Enum of NBD client connector state.

Again, "Enumeration of ... states".

> +#
> +# @running: Connection attempt is in progress.
> +#
> +# @parked: Connection was successful and channel is kept for the next
> +#     request.
> +#
> +# Since: 11.2
> +##
> +{ 'enum': 'XDbgNbdClientConnectorState',
> +  'data': [ 'running', 'parked' ] }
> +
> +##
> +# @XDbgNbdClientConnector:
> +#     The NBD connector information.
> +#
> +# @state: State of the connector.
> +#
> +# Since: 11.2
> +##
> +{ 'struct': 'XDbgNbdClientConnector',
> +  'data': { 'state': 'XDbgNbdClientConnectorState' } }
> +
> +##
> +# @XDbgNbdClient:
> +#     The NBD connection state of the client and its connector.
> +#     The two members are read independently and may not reflect a
> +#     single point in time.

That's unfortunate.  It'll do for debugging.  Could it create robustness
issues in tests?

> +#
> +# @state: The client state.
> +#
> +# @connector: The connector details.  Only present while a connection
> +#     attempt is running or its channel is parked.
> +#
> +# Since: 11.2
> +##
> +{ 'struct': 'XDbgNbdClient',
> +  'data': { 'state': 'XDbgNbdClientState',
> +            '*connector': 'XDbgNbdClientConnector' } }

How the combined state machine works is not clear to me.  Is it
documented anywhere?

> +
> +##
> +# @x-debug-query-nbd-client:
> +#     Get the NBD client state.
> +#
> +# @node-name: node name of the NBD client to query.

How would I find the valid node names?  query-block-exports?

> +#
> +# Features:
> +#
> +# @unstable: This command is meant for debugging and testing.
> +#
> +# Since: 11.2
> +##
> +{ 'command': 'x-debug-query-nbd-client',
> +  'data': { 'node-name': 'str' },
> +  'returns': 'XDbgNbdClient',
> +  'features': [ 'unstable' ] }
> +
>  ##
>  # @drive-mirror:
>  #     Start mirroring a block device's writes to a new destination.