[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