[ovs-dev] [PATCH v2] jsonrpc: Add a new API to allow clients to reset reconnect backoff.
Dumitru Ceara
dceara at redhat.com
Wed Jul 21 12:51:50 UTC 2021
On 7/21/21 2:07 PM, Ilya Maximets wrote:
> On 7/21/21 12:56 PM, Dumitru Ceara wrote:
>> Add jsonrpc_session_reset_backoff() which resets the number of
>> backoff-free tries to the number of remotes + 1. This is used by
>> upper layers (e.g., ovsdb-cs) before forcefully triggering a
>> reconnection. Like this the reconnection will happen as soon as
>> possible.
>>
>> The backoff mechanism should be used whenever the remote is overloaded
>> and causes connection drops. However, forceful reconnects triggered by
>> upper layers happen due to other reasons, e.g.: inconsistent data
>> received from the remote, the remote becomes a follower and the client
>> needs a "leader-only" connection.
>>
>> Since 3c2d6274bcee ("raft: Transfer leadership before creating
>> snapshots."), leadership changes inside the clustered database happen
>> more often and therefore "leader-only" clients need to reconnect more
>> often too. Not resetting the backoff every time a leadership change
>> happens will cause all reconnections to happen with the maximum backoff
>> (8 seconds) resulting in significant latency.
>>
>> Use jsonrpc_session_reset_backoff() in ovsdb-cs when forcing reconnects,
>> if the session is in state CS_S_MONITORING. This commit also updates
>> the Python IDL and jsonrpc implementations. The Python IDL wasn't
>> tracking the IDL_S_MONITORING state explicitly, we now do that too.
>>
>> This commit also adds tests, using test-ovsdb(.py), making sure the IDL
>> forced reconnects happen without backoff.
>>
>> Reported-at: https://bugzilla.redhat.com/1977264
>> Suggested-by: Ilya Maximets <i.maximets at ovn.org>
>> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
>> ---
>> The original v1 patch which was trying to add a new type of reconnect
>> operation was posted here:
>> https://patchwork.ozlabs.org/project/openvswitch/patch/20210629112035.17402-1-dceara@redhat.com/
>>
>> v2:
>> - Reworked the patch based on the discussion with Ilya:
>> - add a new jsonrpc_session_reset_backoff() API to be used by the
>> ovsdb-cs layer to avoid backoffs when reconnecting due to
>> inconsistent data/remote becoming follower.
>> ---
>
> Hi, Dumitru. Thanks for v2!
>
Hi Ilya,
> One comment would be that we need to rename this patch. The new API
> is not the reason for the change, but a necessity to implement a bug
> fix. So, the subject line should describe the primary goal of the patch,
> i.e. what issue you're trying to fix. The new internal API is just a
> helper and not really important. Could be mentioned in the commit
> message, but should not be the main subject.
>
> The commit message also needs to be more focused on the issue rather
> than the jsonrpc_session_reset_backoff() function.
Sure, will do.
>
> Some other comments inline.
>
>> lib/jsonrpc.c | 14 +++++++
>> lib/jsonrpc.h | 1 +
>> lib/ovsdb-cs.c | 10 +++++
>> python/ovs/db/idl.py | 11 +++++
>> python/ovs/jsonrpc.py | 13 ++++++
>> tests/ovsdb-idl.at | 93 +++++++++++++++++++++++++++++++++++++++++--
>> 6 files changed, 138 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/jsonrpc.c b/lib/jsonrpc.c
>> index 7e333ae25f..4dd0b4b2b0 100644
>> --- a/lib/jsonrpc.c
>> +++ b/lib/jsonrpc.c
>> @@ -1280,6 +1280,20 @@ jsonrpc_session_force_reconnect(struct jsonrpc_session *s)
>> reconnect_force_reconnect(s->reconnect, time_msec());
>> }
>>
>> +/* Resets the reconnect backoff for 's' by allowing as many free tries as the
>> + * number of configured remotes + 1. This is to be used by upper layers before
>> + * calling jsonrpc_session_force_reconnect() when one of the other, not
>> + * currently connected, remotes should be used instead (e.g., based on the
>> + * contents of the data received from the currently connected remote).
>
> This comment seems a bit cryptic. Maybe something simple like:
> "Resets the reconnect backoff for 's' by allowing as many free tries as the
> number of configured remotes. This is to be used by upper layers before
> calling jsonrpc_session_force_reconnect() if backoff is undesirable."
>
Sure, sounds better.
>> + *
>> + * The "+1" free try will be consumed when the current remote is disconnected.
>
> This part can be moved inside the function. And we can add +1 only in
> cases where jsonrpc_session_is_connected() == true. I think, this way
> it should be easier to understand.
>
OK.
>> + */
>> +void
>> +jsonrpc_session_reset_backoff(struct jsonrpc_session *s)
>> +{
>> + reconnect_set_backoff_free_tries(s->reconnect, s->remotes.n + 1);
>> +}
>> +
>> /* Sets 'max_backoff' as the maximum time, in milliseconds, to wait after a
>> * connection attempt fails before attempting to connect again. */
>> void
>> diff --git a/lib/jsonrpc.h b/lib/jsonrpc.h
>> index 034a30b716..2aa97d3fe6 100644
>> --- a/lib/jsonrpc.h
>> +++ b/lib/jsonrpc.h
>> @@ -137,6 +137,7 @@ void jsonrpc_session_get_reconnect_stats(const struct jsonrpc_session *,
>>
>> void jsonrpc_session_enable_reconnect(struct jsonrpc_session *);
>> void jsonrpc_session_force_reconnect(struct jsonrpc_session *);
>> +void jsonrpc_session_reset_backoff(struct jsonrpc_session *);
>>
>> void jsonrpc_session_set_max_backoff(struct jsonrpc_session *,
>> int max_backoff);
>> diff --git a/lib/ovsdb-cs.c b/lib/ovsdb-cs.c
>> index f13065c6c0..659d49dbf7 100644
>> --- a/lib/ovsdb-cs.c
>> +++ b/lib/ovsdb-cs.c
>> @@ -729,6 +729,16 @@ void
>> ovsdb_cs_force_reconnect(struct ovsdb_cs *cs)
>> {
>> if (cs->session) {
>> + if (cs->state == CS_S_MONITORING) {
>> + /* The ovsdb-cs was in MONITORING state, so we either had data
>> + * inconsistency on this server, or it stopped being the cluster
>> + * leader, or the user requested to re-connect. Avoiding backoff
>> + * in these cases, as we need to re-connect as soon as possible.
>> + * Connections that are not in MONITORING state should have their
>> + * backoff to avoid constant flood of re-connection attempts in
>> + * case there is no suitable database server. */
>> + jsonrpc_session_reset_backoff(cs->session);
>> + }
>> jsonrpc_session_force_reconnect(cs->session);
>> }
>> }
>> diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
>> index f24511720f..ecae5e1432 100644
>> --- a/python/ovs/db/idl.py
>> +++ b/python/ovs/db/idl.py
>> @@ -102,6 +102,7 @@ class Idl(object):
>> IDL_S_SERVER_MONITOR_REQUESTED = 2
>> IDL_S_DATA_MONITOR_REQUESTED = 3
>> IDL_S_DATA_MONITOR_COND_REQUESTED = 4
>> + IDL_S_MONITORING = 5
>>
>> def __init__(self, remote, schema_helper, probe_interval=None,
>> leader_only=True):
>> @@ -295,6 +296,7 @@ class Idl(object):
>> else:
>> assert self.state == self.IDL_S_DATA_MONITOR_REQUESTED
>> self.__parse_update(msg.result, OVSDB_UPDATE)
>> + self.state = self.IDL_S_MONITORING
>>
>> except error.Error as e:
>> vlog.err("%s: parse error in received schema: %s"
>> @@ -442,6 +444,15 @@ class Idl(object):
>> def force_reconnect(self):
>> """Forces the IDL to drop its connection to the database and reconnect.
>> In the meantime, the contents of the IDL will not change."""
>> + if self.state == self.IDL_S_MONITORING:
>> + # The IDL was in MONITORING state, so we either had data
>> + # inconsistency on this server, or it stopped being the cluster
>> + # leader, or the user requested to re-connect. Avoiding backoff
>> + # in these cases, as we need to re-connect as soon as possible.
>> + # Connections that are not in MONITORING state should have their
>> + # backoff to avoid constant flood of re-connection attempts in
>> + # case there is no suitable database server.
>> + self._session.reset_backoff()
>> self._session.force_reconnect()
>>
>> def session_name(self):
>> diff --git a/python/ovs/jsonrpc.py b/python/ovs/jsonrpc.py
>> index bf32f8c87c..5b8daa91ec 100644
>> --- a/python/ovs/jsonrpc.py
>> +++ b/python/ovs/jsonrpc.py
>> @@ -612,5 +612,18 @@ class Session(object):
>> def force_reconnect(self):
>> self.reconnect.force_reconnect(ovs.timeval.msec())
>>
>> + def reset_backoff(self):
>> + """ Resets the reconnect backoff by allowing as many free tries as the
>> + number of configured remotes + 1. This is to be used by upper
>> + layers before calling force_reconnect() when one of the other, not
>> + currently connected, remotes should be used instead (e.g., based on
>> + the contents of the data received from the currently connected
>> + remote).
>> +
>> + The "+1" free try will be consumed when the current remote is
>> + disconnected."""
>> +
>> + self.reconnect.set_backoff_free_tries(len(self.remotes) + 1)
>> +
>> def get_num_of_remotes(self):
>> return len(self.remotes)
>> diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
>> index e32f9ec895..a01f7bbd58 100644
>> --- a/tests/ovsdb-idl.at
>> +++ b/tests/ovsdb-idl.at
>> @@ -2227,11 +2227,29 @@ m4_define([OVSDB_CHECK_IDL_LEADER_ONLY_PY],
>> OVSDB_CHECK_IDL_LEADER_ONLY_PY([Check Python IDL connects to leader], 3, ['remote'])
>> OVSDB_CHECK_IDL_LEADER_ONLY_PY([Check Python IDL reconnects to leader], 3, ['remote' '+remotestop' 'remote'])
>>
>> -# same as OVSDB_CHECK_IDL but uses C IDL implementation with tcp
>> -# with multiple remotes.
>> +# OVSDB_CHECK_CLUSTER_IDL_C(TITLE, N_SERVERS, [PRE-IDL-TXN], TRANSACTIONS,
>> +# OUTPUT, [KEYWORDS], [FILTER], [LOG_FILTER])
>> +#
>> +# Creates a clustered database with a schema derived from idltest.ovsidl, runs
>> +# each PRE-IDL-TXN (if any), starts N_SERVERS ovsdb-server instances in RAFT,
>> +# on that database, and runs "test-ovsdb idl" passing each of the TRANSACTIONS
>> +# along.
>> +#
>> +# Checks that the overall output is OUTPUT. Before comparison, the
>> +# output is sorted (using "sort") and UUIDs in the output are replaced
>> +# by markers of the form <N> where N is a number. The first unique
>> +# UUID is replaced by <0>, the next by <1>, and so on. If a given
>> +# UUID appears more than once it is always replaced by the same
>> +# marker. If FILTER is supplied then the output is also filtered
>> +# through the specified program.
>> +#
>> +# TITLE is provided to AT_SETUP and KEYWORDS to AT_KEYWORDS.
>> +#
>> +# If LOG_FILTER is provided, checks that the contents of LOG_FILTER
>> +# are not matched by grep in the test-ovsdb logs.
>> m4_define([OVSDB_CHECK_CLUSTER_IDL_C],
>> [AT_SETUP([$1 - C - tcp])
>> - AT_KEYWORDS([ovsdb server idl positive tcp socket $5])
>> + AT_KEYWORDS([ovsdb server idl tcp $6])
>> m4_define([LPBK],[127.0.0.1])
>> OVSDB_CLUSTER_START_IDLTEST([$2], ["ptcp:0:"LPBK])
>> PARSE_LISTENING_PORT([s1.log], [TCP_PORT_1])
>> @@ -2242,11 +2260,55 @@ m4_define([OVSDB_CHECK_CLUSTER_IDL_C],
>> m4_if([$3], [], [],
>> [AT_CHECK([ovsdb-client transact $remotes $3], [0], [ignore], [ignore])])
>> AT_CHECK([test-ovsdb '-vPATTERN:console:test-ovsdb|%c|%m' -vjsonrpc -t10 idl tcp:LPBK:$TCP_PORT_1 $4],
>> - [0], [stdout], [ignore])
>> + [0], [stdout], [stderr])
>> + AT_CHECK([sort stdout | uuidfilt]m4_if([$7],,, [[| $7]]),
>> + [0], [$5])
>> + m4_ifval([$8], [AT_CHECK([grep '$8' stderr], [1])], [], [])
>> + AT_CLEANUP])
>> +
>> +# OVSDB_CHECK_CLUSTER_IDL_C(TITLE, N_SERVERS, [PRE-IDL-TXN], TRANSACTIONS,
>> +# OUTPUT, [KEYWORDS], [FILTER], [LOG_FILTER])
>
> Here should be OVSDB_CHECK_CLUSTER_IDL_PY.
> But we can just replace with:
> "same as OVSDB_CHECK_CLUSTER_IDL_C but uses the Python IDL implementation."
> Text is almost identical and very close in the file.
>
Ack.
I just posted v3:
https://patchwork.ozlabs.org/project/openvswitch/patch/20210721125100.12720-1-dceara@redhat.com/
Regards,
Dumitru
More information about the dev
mailing list