[ovs-dev] [PATCH] ovsdb-idl: Add function to set next_remote in jsonrpc session.

[winson wang]

Hi Ilya,

Thanks for your comments.

Please see my reply inline.

On 9/3/20 2:01 PM, Ilya Maximets wrote:
> External email: Use caution opening links or attachments
>
>
> On 8/13/20 10:52 PM, Zhen Wang wrote:
>> OVN SouthBound DB in clustered mode, when one raft node down and
>> online. All the ovn-controller clients will not migirate back which
>> cause RAFT DB clients unbalanced state.
>>
>> This function provides a way to set the IDL jsonrpc session next_remote.
>> Which can let caller to set the next reconnect remote which can address
>> the unbalance issue with reconnect operation.>
>> Notice that this function is not actually used anywhere in this patch.
>> This will be used by OVN, though, since OVN is the primary user of
>> clustered OVSDB.
>>
>
> Hi.  Thanks for working on this!
>
> A couple of thoughts about this patch:
> 1. It's pretty easy to misspell the remote address or port or pass this
>     in an unexpected format, so this API, I think, should return a meaningful
>     error code in case desired remote not found on a list. (ENOENT?)

Sure I will add it.

As Han suggested in the dependent patch in ovn-controller.c,  I will 
also take care of

the error checking in ovn-controller unix command callback function.

Since this function could be used for other purpose in future,  I agree 
this API

need return value.

>
> 2. We already have some feature gap between C and python idl implementations
>     and I'd like to not increase it.  So, please, implement the equivalent
>     python API.
I will study the similar commit and add the support.
>
> 3. As far as this functionality has not user in OVS itself, i.e. not tested
>     anyhow, we really need a unit test for that.  See tests/ovsdb-idl.at
>     for examples.  It should not be hard to add 'set_remote' command to
>     both tests/test-ovsdb.c and tests/test-ovsdb.py and write a basic unit test.
>     These tools already has 'reconnect' command that could be used as a reference.
Will work on it.
>
> BTW, do we need something like ovsdb_idl_current_remote() to check what is the
> current remote we're connected to?

Agree.

For now, I use netstat tool to check the current remote for ovn-controller.

It would be better to show the current_remote via new unix command such 
as "current_remote"?


Regards,

Zhen


>
> Best regards, Ilya Maximets.