| Message ID | 20230111000756.4054163-1-hzhou@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,1/2] ovsdb-idl: Provide API to disable set_db_change_aware request. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 1/11/23 01:07, 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> > --- Hi Han, Thanks for following up on this! > 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(-) > > diff --git a/lib/ovsdb-cs.c b/lib/ovsdb-cs.c > index 0fca03d7231e..df13c4764874 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 in the SERVER_MONITOR_REQUESTED Nit: I think it's actually in states DATA_MONITOR_COND_SINCE_REQUESTED, DATA_MONITOR_COND_REQUESTED, DATA_MONITOR_REQUESTED. > + * state (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..0e84b87699c5 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); Nit: indent (extra space). > void ovsdb_idl_reset_min_index(struct ovsdb_idl *); > void ovsdb_idl_destroy(struct ovsdb_idl *); > With these two minor issues fixed, please add my ack: Acked-by: Dumitru Ceara <dceara@redhat.com> Thanks, Dumitru
On Wed, Jan 11, 2023 at 1:42 AM Dumitru Ceara <dceara@redhat.com> wrote: > > On 1/11/23 01:07, 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> > > --- > > Hi Han, > > Thanks for following up on this! > > > 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(-) > > > > diff --git a/lib/ovsdb-cs.c b/lib/ovsdb-cs.c > > index 0fca03d7231e..df13c4764874 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 in the SERVER_MONITOR_REQUESTED > > Nit: I think it's actually in states DATA_MONITOR_COND_SINCE_REQUESTED, > DATA_MONITOR_COND_REQUESTED, DATA_MONITOR_REQUESTED. > You are right! Since the state isn't the major focus here, I'd rather not include all the possible state, but instead: /* 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) ... Does this look better? > > + * state (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..0e84b87699c5 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); > > Nit: indent (extra space). > Ack. > > void ovsdb_idl_reset_min_index(struct ovsdb_idl *); > > void ovsdb_idl_destroy(struct ovsdb_idl *); > > > > With these two minor issues fixed, please add my ack: > > Acked-by: Dumitru Ceara <dceara@redhat.com> > Thanks, Han > Thanks, > Dumitru > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 1/12/23 02:55, Han Zhou wrote: > On Wed, Jan 11, 2023 at 1:42 AM Dumitru Ceara <dceara@redhat.com> wrote: >> >> On 1/11/23 01:07, 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> >>> --- >> >> Hi Han, >> >> Thanks for following up on this! >> >>> 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(-) >>> >>> diff --git a/lib/ovsdb-cs.c b/lib/ovsdb-cs.c >>> index 0fca03d7231e..df13c4764874 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 in the > SERVER_MONITOR_REQUESTED >> >> Nit: I think it's actually in states DATA_MONITOR_COND_SINCE_REQUESTED, >> DATA_MONITOR_COND_REQUESTED, DATA_MONITOR_REQUESTED. >> > You are right! Since the state isn't the major focus here, I'd rather not > include all the possible state, but instead: > > /* 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) ... > > Does this look better? > Looks good to me, thanks!
diff --git a/lib/ovsdb-cs.c b/lib/ovsdb-cs.c index 0fca03d7231e..df13c4764874 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 in the SERVER_MONITOR_REQUESTED + * state (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..0e84b87699c5 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 *);
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> --- 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(-)