[ovs-dev] [PATCH] ovsdb-server: Don't drop all connections on read/write status change.

[Dumitru Ceara]

On Mon, Oct 14, 2019 at 8:21 AM <nusiddiq at redhat.com> wrote:
>
> From: Numan Siddique <nusiddiq at redhat.com>
>
> The commit [1] force drops all connections when the db read/write status changes.
> Prior to the commit [1], when there was read/write status change, the existing
> jsonrpc sessions with 'db_change_aware' set to true, were not updated with the
> changed 'read_only' value. If the db status was changed to 'standby', the existing
> clients could still write to the db.
>
> In the case of pacemaker OVN HA, OVN OCF script 'start' action starts the
> ovsdb-servers in read-only state and later, it sets to read-write in the
> 'promote' action. We have observed that if some ovn-controllers connect to
> the SB ovsdb-server (in read-only state) just before the 'promote' action,
> the connection is not reset all the times and these ovn-controllers remain connected
> to the SB ovsdb-server in read-only state all the time. Even though
> the commit [1] calls 'ovsdb_jsonrpc_server_reconnect()' with 'forced' flag
> set to true when the db read/write status changes, somehow the FSM misses resetting
> the connections of these ovn-controllers.
>
> I think this needs to be addressed in the FSM. This patch doesn't address
> this FSM issue. Instead it changes the behavior of 'ovsdb_jsonrpc_server_set_read_only()'
> by setting the 'read_only' flag of all the jsonrpc sessions instead of forcefully
> resetting the connection.
>
> I think there is no need to reset the connection. In large scale production
> deployements with OVN, this results in unnecessary waste of CPU cycles as ovn-controllers
> will have to connect twice - once during 'start' action and again during 'promote'.
>
> [1] - 2a9679e3b2c6("ovsdb-server: drop all connections on read/write status change")
>
> Signed-off-by: Numan Siddique <nusiddiq at redhat.com>

Hi Numan,

The code looks good to me, just a minor comment below.

Thanks,
Dumitru

> ---
>  ovsdb/jsonrpc-server.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
> index ddbbc2e94..066826959 100644
> --- a/ovsdb/jsonrpc-server.c
> +++ b/ovsdb/jsonrpc-server.c
> @@ -80,6 +80,8 @@ static void ovsdb_jsonrpc_session_unlock_all(struct ovsdb_jsonrpc_session *);
>  static void ovsdb_jsonrpc_session_unlock__(struct ovsdb_lock_waiter *);
>  static void ovsdb_jsonrpc_session_send(struct ovsdb_jsonrpc_session *,
>                                         struct jsonrpc_msg *);
> +static void ovsdb_jsonrpc_session_set_readonly_all(
> +    struct ovsdb_jsonrpc_remote *remote, bool read_only);
>
>  /* Triggers. */
>  static void ovsdb_jsonrpc_trigger_create(struct ovsdb_jsonrpc_session *,
> @@ -365,10 +367,13 @@ ovsdb_jsonrpc_server_set_read_only(struct ovsdb_jsonrpc_server *svr,
>  {
>      if (svr->read_only != read_only) {
>          svr->read_only = read_only;
> -        ovsdb_jsonrpc_server_reconnect(svr, true,
> -                                       xstrdup(read_only
> -                                               ? "making server read-only"
> -                                               : "making server read/write"));
> +        struct shash_node *node;
> +
> +        SHASH_FOR_EACH (node, &svr->remotes) {
> +            struct ovsdb_jsonrpc_remote *remote = node->data;
> +
> +            ovsdb_jsonrpc_session_set_readonly_all(remote, read_only);
> +        }
>      }
>  }
>
> @@ -670,6 +675,17 @@ ovsdb_jsonrpc_session_reconnect_all(struct ovsdb_jsonrpc_remote *remote,
>      }
>  }
>
> +static void
> +ovsdb_jsonrpc_session_set_readonly_all(struct ovsdb_jsonrpc_remote *remote,
> +                                       bool read_only)
> +{
> +    struct ovsdb_jsonrpc_session *s, *next;
> +
> +    LIST_FOR_EACH_SAFE (s, next, node, &remote->sessions) {

LIST_FOR_EACH should be enough here.

> +        s->read_only = read_only;
> +    }
> +}
> +
>  /* Sets the options for all of the JSON-RPC sessions managed by 'remote' to
>   * 'options'.
>   *
> --
> 2.21.0
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev