On 7/12/21 10:36 PM, Ilya Maximets wrote:
> On 6/29/21 1:20 PM, Dumitru Ceara wrote:
>> Until now clients that needed to reconnect immediately could only use
>> reconnect_force_reconnect(). However, reconnect_force_reconnect()
>> doesn't reset the backoff for connections that were alive long enough
>> (more than backoff seconds).
>>
>> Moreover, the reconnect library cannot determine the exact reason why a
>> client wishes to initiate a reconnection. In most cases reconnection
>> happens because of a fatal error when communicating with the remote,
>> e.g., in the ovsdb-cs layer, when invalid messages are received from
>> ovsdb-server. In such cases it makes sense to not reset the backoff
>> because the remote seems to be unhealthy.
>>
>> There are however cases when reconnection is needed for other reasons.
>> One such example is when ovsdb-clients require "leader-only" connections
>> to clustered ovsdb-server databases. Whenever the client determines
>> that the remote is not a leader anymore, it decides to reconnect to a
>> new remote from its list, searching for the new leader. Using
>> jsonrpc_force_reconnect() (which calls reconnect_force_reconnect()) will
>> not reset backoff even though the former leader is still likely in good
>> shape.
>>
>> 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.
>>
>> This commit also updates the Python reconnect and IDL implementations
>> and adds tests for force-reconnect and graceful-reconnect.
>>
>> Reported-at: https://bugzilla.redhat.com/1977264
>> Signed-off-by: Dumitru Ceara <[email protected]>
>> ---
>
> Hi, Dumitru.
>
Hi Ilya,
> Thanks for working on this issue. I've seen it in practice while running
> OVN tests, but I still don't quiet understand why it happens. Could you,
> please, describe how state transitioning work here for the ovsdb-idl case?
>
Without the patch, assuming a current backoff of X seconds, the sequence
of events (even for a connection that has seen activity after backoff)
is:
- reconnect_force_reconnect() -> move to S_RECONNECT
- ovsdb_cs_run()
-> jsonrpc_session_run()
-> reconnect_run()
-> reconnect_disconnected()
-> because state is not S_ACTIVE | S_IDLE backoff is not
changed, stays X.
>> +# Forcefully reconnect.
>> +force-reconnect
>> + in RECONNECT for 0 ms (2000 ms backoff)
>> + 1 successful connections out of 3 attempts, seqno 2
>> + disconnected
>> +run
>> + should disconnect
>> +connecting
>> + in CONNECTING for 0 ms (2000 ms backoff)
>
> Especially this part seems wrong to me. Because after 'should disconnect'
> there should be 'disconnect' of 'connect-fail', but not 'connecting'. We
> literally should disconnect here, otherwise it's a violation of the reconnect
> API. And my concern is that ovsdb-cs or jsonrpc violates the API somewhere
> by not calling reconnect_disconnectd() when it is required, or there is some
> other bug that makes 'reconnect' module to jump over few states in a fsm.
>
> The logical workflow for the force-reconnect, from what I see in the code
> should be:
>
> 1. force-reconnect --> transition to S_RECONNECT
> 2. run -> in S_RECONNECT, so returning RECONNECT_DISCONNECT
> 3. disconnect -> check the state, update backoff and transition to S_BACKOFF
> 4. run -> in S_BACKOFF, so returning RECONNECT_CONNECT
> 5. connected ....
This is just a bug in the test case I added, I need to issue
"disconnect" in the test case to trigger reconnect_disconnected()
to be called (like ovsdb-cs does).
>
> Something is fishy here, because ovsdb-cs somehow jumps over step #3 and
> maybe also #4.
As mentioned above, it's not the case for ovsdb-cs.
However, while checking the test case I realized that the patch will
cause all "graceful" reconnects to happen with an initial backoff of 2
seconds for long lived sessions (instead of 1s).
That's because:
reconnect_graceful_reconnect()
-> reconnect_reset_backoff__()
-> session was active after the initial 'backoff' seconds, so
reset backoff to minimum.
-> reconnect_transition__(..., S_RECONNECT);
ovsdb_cs_run()
-> jsonrpc_session_run()
-> reconnect_run()
-> reconnect_disconnected():
-> if (!reconnect_reset_backoff__(..)) then double backoff.
I see a couple of potential ways to fix that:
1. reconnect_graceful_reconnect() could bump fsm->backoff_free_tries to
allow a single backoff free reconnect.
2. reconnect_graceful_reconnect() could reset fsm->backoff to 0 if it
was active recently (instead of calling reconnect_reset_backoff__()).
However, this is all under the assumption that we want to add support
for two types of reconnect "semantics":
a. forced, e.g., when the client detects inconsistencies in the data
received from the server (the server is in "bad shape") in which we
should never reset the backoff.
b. graceful, e.g., when the client reconnects because it needs a
leader-only connection and a leadership transfer happened on the
server side (the server is likely in "good shape" just not a leader
anymore).
An alternative to all this is to change reconnect_disconnected() to
reset backoff for connections in states S_ACTIVE | S_IDLE | S_RECONNECT
(used to be just active and idle).
I guess we could treat this as a bug fix and continue discussion for a
separate follow up patch to add the "graceful vs force" semantics if
they turn out to make sense for the future.
In essence (excluding potential python and test changes) the patch would
become:
diff --git a/lib/reconnect.c b/lib/reconnect.c
index a929ddfd2d..3a6d93f9d1 100644
--- a/lib/reconnect.c
+++ b/lib/reconnect.c
@@ -385,7 +385,7 @@ reconnect_disconnected(struct reconnect *fsm, long long int
now, int error)
if (fsm->backoff_free_tries > 1) {
fsm->backoff_free_tries--;
fsm->backoff = 0;
- } else if (fsm->state & (S_ACTIVE | S_IDLE)
+ } else if (fsm->state & (S_ACTIVE | S_IDLE | S_RECONNECT)
&& (fsm->last_activity - fsm->last_connected >= fsm->backoff
|| fsm->passive)) {
fsm->backoff = fsm->passive ? 0 : fsm->min_backoff;
---
What do you think?
>
> Best regards, Ilya Maximets.
>
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
