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

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.

Reported-at:https://mail.openvswitch.org/pipermail/ovs-discuss/2020-August/050518.html
Signed-off-by: Zhen Wang <zhewang@nvidia.com>
---
 lib/jsonrpc.c   | 12 ++++++++++++
 lib/jsonrpc.h   |  2 ++
 lib/ovsdb-idl.c |  9 +++++++++
 lib/ovsdb-idl.h |  1 +
 4 files changed, 24 insertions(+)

On Thu, Aug 13, 2020 at 1:51 PM Zhen Wang <zhewang@nvidia.com> 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.
>
> Reported-at:
https://mail.openvswitch.org/pipermail/ovs-discuss/2020-August/050518.html
> Signed-off-by: Zhen Wang <zhewang@nvidia.com>
> ---
>  lib/jsonrpc.c   | 12 ++++++++++++
>  lib/jsonrpc.h   |  2 ++
>  lib/ovsdb-idl.c |  9 +++++++++
>  lib/ovsdb-idl.h |  1 +
>  4 files changed, 24 insertions(+)
>
> diff --git a/lib/jsonrpc.c b/lib/jsonrpc.c
> index ecbc939fe..08971ee94 100644
> --- a/lib/jsonrpc.c
> +++ b/lib/jsonrpc.c
> @@ -1250,3 +1250,15 @@ jsonrpc_session_set_dscp(struct jsonrpc_session
*s, uint8_t dscp)
>          jsonrpc_session_force_reconnect(s);
>      }
>  }
> +
> +/* Sets the next remote offset for next jsonrpc session reconnect. */
> +void
> +jsonrpc_session_set_next_remote(struct jsonrpc_session *s, const char
*name)
> +{
> +    for (size_t i = 0; i < s->remotes.n; i++) {
> +        if (!strcmp(s->remotes.names[i], name)) {
> +            s->next_remote = i;
> +            break;
> +        }
> +    }
> +}
> diff --git a/lib/jsonrpc.h b/lib/jsonrpc.h
> index a44114e8d..f81d62047 100644
> --- a/lib/jsonrpc.h
> +++ b/lib/jsonrpc.h
> @@ -140,6 +140,8 @@ void jsonrpc_session_set_probe_interval(struct
jsonrpc_session *,
>                                          int probe_interval);
>  void jsonrpc_session_set_dscp(struct jsonrpc_session *,
>                                uint8_t dscp);
> +void jsonrpc_session_set_next_remote(struct jsonrpc_session *,
> +                              const char *);
>  const char *jsonrpc_session_get_id(const struct jsonrpc_session *);
>
>  #endif /* jsonrpc.h */
> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> index d8f221ca6..82f09bbe7 100644
> --- a/lib/ovsdb-idl.c
> +++ b/lib/ovsdb-idl.c
> @@ -557,6 +557,15 @@ ovsdb_idl_set_remote(struct ovsdb_idl *idl, const
char *remote, bool retry)
>      }
>  }
>
> +/* Set next remote offset for ovsdb_idl jsonrpc session.*/
> +void
> +ovsdb_idl_set_next_remote(struct ovsdb_idl *idl, const char *remote)
> +{
> +    if (idl->session) {
> +        jsonrpc_session_set_next_remote(idl->session, remote);
> +    }
> +}
> +
>  /* Set whether the order of remotes should be shuffled, when there
>   * are more than one remotes.  The setting doesn't take effect
>   * until the next time when ovsdb_idl_set_remote() is called. */
> diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h
> index c56cd19b1..01bb9a409 100644
> --- a/lib/ovsdb-idl.h
> +++ b/lib/ovsdb-idl.h
> @@ -63,6 +63,7 @@ struct ovsdb_idl *ovsdb_idl_create(const char *remote,
>  struct ovsdb_idl *ovsdb_idl_create_unconnected(
>      const struct ovsdb_idl_class *, bool monitor_everything_by_default);
>  void ovsdb_idl_set_remote(struct ovsdb_idl *, const char *, bool);
> +void ovsdb_idl_set_next_remote(struct ovsdb_idl *, const char *);
>  void ovsdb_idl_set_shuffle_remotes(struct ovsdb_idl *, bool);
>  void ovsdb_idl_reset_min_index(struct ovsdb_idl *);
>  void ovsdb_idl_destroy(struct ovsdb_idl *);
> --
> 2.20.1
>

Acked-by: Han Zhou <hzhou@ovn.org>

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?)

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.

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.

BTW, do we need something like ovsdb_idl_current_remote() to check what is the
current remote we're connected to?

Best regards, Ilya Maximets.

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.