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

[guillaume vincke <guillaume.vincke@ovhcloud.com>]

On 11/11/26 15:22, Markus Armbruster wrote:
> 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 :)
> 

I was a bit too eager there :) I will keep it where it stands for now.

>>>> +#
>>>> +# @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?
> 

Will do!
>>>> +#
>>>> +# @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.
> 

Will add that in v2.

>>>> +#
>>>> +# @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?
> 

It is never present when @state is connected. on connecting-* it is
absent until lazy reconnection is triggered (first request after a
failure). In quit, a running attempt or a parked channel stays until
the node is closed, so the connector can be absent, running or 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?
>>
>> 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?
> 

would something like this help:

1- the connector is absent while connected
2- a failure moves the client to connecting-wait. It gets to Nowait
    when the configured reconnect-delay is reached.
3- In connecting-* states, the next request starts the connector which
    gets into running.
4- On wait, the request blocks and takes the channel when it's ready.
    On nowait, the request fails at once if no parked channel is
    available.
5- on successful connection, the connector parks the channel.
6- when the channel gets picked up by the client, it gets to connected
    and the connector is cleared.
7- on fatal error or when teardown is requested, the state gets to
    quit.


>>>> +
>>>> +##
>>>> +# @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"?

It's a node whose driver is NBD. It gets added with blockdev-add with
"driver": "nbd". Any other node gets the error "is not an NBD client".
Would changing the doc to "node name of a block node using the nbd
driver" with a reference to the query-named-block-nodes be better ?