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:

> On 11/10/26 13:53, Markus Armbruster wrote:
>
> Thanks for the review. Let me clarify before posting a v2.
>
>> Why block-core.json and not block-export.json?
>
> If I get this right, block-export.json covers the server part. I
> thought adding this next to BlockdevOptionsNbd was a better fit. I can
> change that in v2 if you think block-export.json is a better match.

My question was a question, not a change request :)

>>> +#
>>> +# @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?
>
> Yes. This is a client state that describes the connection as valid: the
> channel got negotiated and is ready to process requests.
>
>> 
>>> +#
>>> +# @quit: Client will not reconnect.
>>
>> What does this one mean exactly?
>
> The client stopped for good. Requests fail and it never reconnects.
> This state is reached on fatal error or teardown.

Work some of that into the description?

>>> +#
>>> +# Since: 11.2
>>
>> Making 11.2 seems unlikely.
>
> Sure. Would updating this to 12.0 make more sense ?

Yes.

>>> +#
>>> +# @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?
>
> On the nbd-stale-connection test it is not a problem but I can try to
> take the locks together in v2.

Worth a try, I guess.

>>> +#
>>> +# @state: The client state.
>>> +#
>>> +# @connector: The connector details.  Only present while a connection
>>> +#     attempt is running or its channel is parked.

When exactly is this member present?

Is it always present when @state has certain values, and never present
when it doesn't?  Or is it more complicated?

>>> +#
>>> +# 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?
>
> I did not find a proper documentation. I found the comments on the code
> to be quite explicit though. The connector only runs in a connecting-*
> state. In that state, when processing a request, the client tries to
> get the channel from the connector. When the connector has a valid
> channel but the client did not request it, the connector is in parked
> state. Do you think this command should document that behavior?

Perhaps the doc comments are good enough for a debugging command to be
used by people who know how the state machine works.

I don't, I've read the doc comments several times, and the state machine
is still a mystery to me.  Can we do better with reasonable effort?

>>> +
>>> +##
>>> +# @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?
>
> The node-name is listed by query-named-block-nodes or is the one passed
> to blockdev-add. I will add a reference to those commands in the
> docstring to clarify.

What exactly makes such a node name an "NBD client"?