diff mbox series

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

Message ID 20191014152002.4377-1-nusiddiq@redhat.com
State Accepted
Headers show
Series [ovs-dev,v3] ovsdb-server: Don't drop all connections on read/write status change. | expand

Commit Message

Numan Siddique Oct. 14, 2019, 3:20 p.m. UTC 
From: Numan Siddique <nusiddiq@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")

Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
---

v2 -> v3
-------
  * Addressed minor review comment.

v1 -> v2
-------
  * Addressed Dumitru's comment - Use LIST_FOR_EACH instead of
    LIST_FOR_EACH_SAFE

 ovsdb/jsonrpc-server.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

Comments

Ben Pfaff Oct. 14, 2019, 5:52 p.m. UTC | #1 
On Mon, Oct 14, 2019 at 08:50:02PM +0530, nusiddiq@redhat.com wrote:
> From: Numan Siddique <nusiddiq@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.

Thanks, applied to master.
Han Zhou Oct. 14, 2019, 6:11 p.m. UTC | #2 
On Mon, Oct 14, 2019 at 8:20 AM <nusiddiq@redhat.com> wrote:

> From: Numan Siddique <nusiddiq@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")
>
> Acked-by: Dumitru Ceara <dceara@redhat.com>
> Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
> ---
>
>
Thanks Numan. Is this the root cause of the ovn-controller transaction
failure you mentioned at last OVN meeting?
Numan Siddique Oct. 14, 2019, 6:15 p.m. UTC | #3 
On Mon, Oct 14, 2019, 11:42 PM Han Zhou <zhouhan@gmail.com> wrote:

>
>
> On Mon, Oct 14, 2019 at 8:20 AM <nusiddiq@redhat.com> wrote:
>
>> From: Numan Siddique <nusiddiq@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")
>>
>> Acked-by: Dumitru Ceara <dceara@redhat.com>
>> Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
>> ---
>>
>>
> Thanks Numan. Is this the root cause of the ovn-controller transaction
> failure you mentioned at last OVN meeting?
>

Hi Han,
Yes. This is the root cause. Earlier I was suspecting ovn-controller though.

Thanks
Numan
Numan Siddique Oct. 14, 2019, 6:15 p.m. UTC | #4 
On Mon, Oct 14, 2019 at 11:37 PM Ben Pfaff <blp@ovn.org> wrote:

> On Mon, Oct 14, 2019 at 08:50:02PM +0530, nusiddiq@redhat.com wrote:
> > From: Numan Siddique <nusiddiq@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.
>
> Thanks, applied to master.
>

Thanks for applying the patch.

Numan
Numan Siddique Oct. 14, 2019, 7:12 p.m. UTC | #5 
On Mon, Oct 14, 2019 at 11:45 PM Numan Siddique <nusiddiq@redhat.com> wrote:

>
>
> On Mon, Oct 14, 2019 at 11:37 PM Ben Pfaff <blp@ovn.org> wrote:
>
>> On Mon, Oct 14, 2019 at 08:50:02PM +0530, nusiddiq@redhat.com wrote:
>> > From: Numan Siddique <nusiddiq@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.
>>
>> Thanks, applied to master.
>>
>
> Thanks for applying the patch.
>

Can this patch be back ported to 2.12 please.

Thanks

Numan


>
> Numan
>
>
Ben Pfaff Oct. 14, 2019, 7:33 p.m. UTC | #6 
On Tue, Oct 15, 2019 at 12:42:04AM +0530, Numan Siddique wrote:
> On Mon, Oct 14, 2019 at 11:45 PM Numan Siddique <nusiddiq@redhat.com> wrote:
> 
> >
> >
> > On Mon, Oct 14, 2019 at 11:37 PM Ben Pfaff <blp@ovn.org> wrote:
> >
> >> On Mon, Oct 14, 2019 at 08:50:02PM +0530, nusiddiq@redhat.com wrote:
> >> > From: Numan Siddique <nusiddiq@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.
> >>
> >> Thanks, applied to master.
> >>
> >
> > Thanks for applying the patch.
> >
> 
> Can this patch be back ported to 2.12 please.

Done.
diff mbox series

Patch

diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
index ddbbc2e94..4e2dfc3d7 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;
+
+    LIST_FOR_EACH (s, node, &remote->sessions) {
+        s->read_only = read_only;
+    }
+}
+
 /* Sets the options for all of the JSON-RPC sessions managed by 'remote' to
  * 'options'.
  *