diff mbox series

[ovs-dev,v2,1/2] ovsdb-idl: Provide API to disable set_db_change_aware request.

Message ID 20230112170854.1081336-1-hzhou@ovn.org
State Accepted
Commit 8833e7c8edb0c19d4f8b5ba8f15ba0405ebb0cb1
Headers show
Series [ovs-dev,v2,1/2] ovsdb-idl: Provide API to disable set_db_change_aware request. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Han Zhou Jan. 12, 2023, 5:08 p.m. UTC
For ovsdb clients that are short-lived, e.g. when using
ovn-nbctl/ovn-sbctl to read some metrics from the OVN NB/SB server, they
don't really need to be aware of db changes, because they exit
immediately after getting the initial response for the requested data.
In such use cases, however, the clients still send 'set_db_change_aware'
request, which results in server side error logs when the server tries
to send out the response for the 'set_db_change_aware' request, because
at the moment the client that is supposed to receive the request has
already closed the connection and exited. E.g.:

2023-01-10T18:23:29.431Z|00007|jsonrpc|WARN|unix#3: receive error: Connection reset by peer
2023-01-10T18:23:29.431Z|00008|reconnect|WARN|unix#3: connection dropped (Connection reset by peer)

To avoid such problems, this patch provides an API to allow a client to
choose to not send the 'set_db_change_aware' request.

There was an earlier attempt to fix this [0], but it was not accepted
back then as discussed in the email [1]. It was also discussed in the
emails that an alternative approach is to use notification instead of
request, but that would require protocol changes and taking backward
compatibility into consideration. So this patch takes a different
approach and tries to keep the change small.

[0] http://patchwork.ozlabs.org/project/openvswitch/patch/1594380801-32134-1-git-send-email-dceara@redhat.com/

[1] https://mail.openvswitch.org/pipermail/ovs-discuss/2021-February/050919.html

Reported-by: Girish Moodalbail <gmoodalbail@nvidia.com>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2020-July/050343.html
Reported-by: Tobias Hofmann <tohofman@cisco.com>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2021-February/050914.html
Signed-off-by: Han Zhou <hzhou@ovn.org>
Acked-by: Dumitru Ceara <dceara@redhat.com>
---
v2: Addressed Dumitru's comments.
 lib/ovsdb-cs.c  | 22 +++++++++++++++++++++-
 lib/ovsdb-cs.h  |  3 +++
 lib/ovsdb-idl.c |  8 ++++++++
 lib/ovsdb-idl.h |  2 ++
 4 files changed, 34 insertions(+), 1 deletion(-)

Comments

Ilya Maximets Jan. 16, 2023, 8:55 p.m. UTC | #1
On 1/12/23 18:08, Han Zhou wrote:
> For ovsdb clients that are short-lived, e.g. when using
> ovn-nbctl/ovn-sbctl to read some metrics from the OVN NB/SB server, they
> don't really need to be aware of db changes, because they exit
> immediately after getting the initial response for the requested data.
> In such use cases, however, the clients still send 'set_db_change_aware'
> request, which results in server side error logs when the server tries
> to send out the response for the 'set_db_change_aware' request, because
> at the moment the client that is supposed to receive the request has
> already closed the connection and exited. E.g.:
> 
> 2023-01-10T18:23:29.431Z|00007|jsonrpc|WARN|unix#3: receive error: Connection reset by peer
> 2023-01-10T18:23:29.431Z|00008|reconnect|WARN|unix#3: connection dropped (Connection reset by peer)
> 
> To avoid such problems, this patch provides an API to allow a client to
> choose to not send the 'set_db_change_aware' request.
> 
> There was an earlier attempt to fix this [0], but it was not accepted
> back then as discussed in the email [1]. It was also discussed in the
> emails that an alternative approach is to use notification instead of
> request, but that would require protocol changes and taking backward
> compatibility into consideration. So this patch takes a different
> approach and tries to keep the change small.
> 
> [0] http://patchwork.ozlabs.org/project/openvswitch/patch/1594380801-32134-1-git-send-email-dceara@redhat.com/
> 
> [1] https://mail.openvswitch.org/pipermail/ovs-discuss/2021-February/050919.html
> 
> Reported-by: Girish Moodalbail <gmoodalbail@nvidia.com>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2020-July/050343.html
> Reported-by: Tobias Hofmann <tohofman@cisco.com>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2021-February/050914.html
> Signed-off-by: Han Zhou <hzhou@ovn.org>
> Acked-by: Dumitru Ceara <dceara@redhat.com>
> ---
> v2: Addressed Dumitru's comments.

Thanks, Han and Dumitru!  Applied (before branching).

Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/lib/ovsdb-cs.c b/lib/ovsdb-cs.c
index 0fca03d7231e..e26403648920 100644
--- a/lib/ovsdb-cs.c
+++ b/lib/ovsdb-cs.c
@@ -219,6 +219,9 @@  struct ovsdb_cs {
     struct uuid cid;
     struct hmap server_rows;
 
+    /* Whether to send 'set_db_change_aware'. */
+    bool set_db_change_aware;
+
     /* Clustered servers. */
     uint64_t min_index;      /* Minimum allowed index, to avoid regression. */
     bool leader_only;        /* If true, do not connect to Raft followers. */
@@ -331,6 +334,7 @@  ovsdb_cs_create(const char *db_name, int max_version,
     cs->request_id = NULL;
     cs->leader_only = true;
     cs->shuffle_remotes = true;
+    cs->set_db_change_aware = true;
     hmap_init(&cs->server_rows);
 
     return cs;
@@ -461,7 +465,7 @@  ovsdb_cs_process_response(struct ovsdb_cs *cs, struct jsonrpc_msg *msg)
             cs->server.monitor_version = cs->server.max_version;
             ovsdb_cs_db_parse_monitor_reply(&cs->server, msg->result,
                                             cs->server.monitor_version);
-            if (ovsdb_cs_check_server_db(cs)) {
+            if (ovsdb_cs_check_server_db(cs) && cs->set_db_change_aware) {
                 ovsdb_cs_send_db_change_aware(cs);
             }
         } else {
@@ -1150,6 +1154,22 @@  ovsdb_cs_send_cond_change(struct ovsdb_cs *cs)
     }
 }
 
+/* Database change awareness. */
+
+/* By default, or if 'db_change_aware' is true, 'cs' will send
+ * 'set_db_change_aware' request to the server after receiving the _SERVER data
+ * (when the server supports it), which is useful for clients that intends to
+ * keep long connections to the server. Otherwise, 'cs' will not send the
+ * 'set_db_change_aware' request, which is more reasonable for short-lived
+ * connections to avoid unnecessary processing at the server side and possible
+ * error handling due to connections being closed by the clients before the
+ * responses are sent by the server. */
+void
+ovsdb_cs_set_db_change_aware(struct ovsdb_cs *cs, bool set_db_change_aware)
+{
+    cs->set_db_change_aware = set_db_change_aware;
+}
+
 /* Clustered servers. */
 
 /* By default, or if 'leader_only' is true, when 'cs' connects to a clustered
diff --git a/lib/ovsdb-cs.h b/lib/ovsdb-cs.h
index 5d5b58f0a0a6..4cf9ca2b99c1 100644
--- a/lib/ovsdb-cs.h
+++ b/lib/ovsdb-cs.h
@@ -142,6 +142,9 @@  unsigned int ovsdb_cs_set_condition(struct ovsdb_cs *, const char *table,
                                     const struct json *condition);
 unsigned int ovsdb_cs_get_condition_seqno(const struct ovsdb_cs *);
 
+/* Database change awareness. */
+void ovsdb_cs_set_db_change_aware(struct ovsdb_cs *, bool set_db_change_aware);
+
 /* Clustered servers. */
 void ovsdb_cs_set_leader_only(struct ovsdb_cs *, bool leader_only);
 void ovsdb_cs_set_shuffle_remotes(struct ovsdb_cs *, bool shuffle);
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index dbdfe45d87ea..634fbb56df24 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -321,6 +321,14 @@  ovsdb_idl_set_shuffle_remotes(struct ovsdb_idl *idl, bool shuffle)
     ovsdb_cs_set_shuffle_remotes(idl->cs, shuffle);
 }
 
+/* Passes 'set_db_change_aware' to ovsdb_cs_set_db_change_aware().  See that
+ * function for documentation. */
+void
+ovsdb_idl_set_db_change_aware(struct ovsdb_idl *idl, bool set_db_change_aware)
+{
+    ovsdb_cs_set_db_change_aware(idl->cs, set_db_change_aware);
+}
+
 /* Reset min_index to 0. This prevents a situation where the client
  * thinks all databases have stale data, when they actually have all
  * been destroyed and rebuilt from scratch.
diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h
index 9a3e19f20553..86fd2bd36f27 100644
--- a/lib/ovsdb-idl.h
+++ b/lib/ovsdb-idl.h
@@ -66,6 +66,8 @@  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 *remote, bool retry);
 void ovsdb_idl_set_shuffle_remotes(struct ovsdb_idl *, bool shuffle);
+void ovsdb_idl_set_db_change_aware(struct ovsdb_idl *,
+                                   bool set_db_change_aware);
 void ovsdb_idl_reset_min_index(struct ovsdb_idl *);
 void ovsdb_idl_destroy(struct ovsdb_idl *);