On 8/13/26 11:55 AM, Timothy Redaelli via dev wrote:
> A command forwarded to the leader by a follower, or appended locally by
> a leader that later loses leadership, is completed with an error after
> twice the election timer. The intent, as the comment says, is that the
> command survives a leader election and completes once the new leader
> commits the entry.
>
> Twice the election timer is not enough for that. A follower starts an
> election one election timer plus up to ELECTION_RANGE_MSEC (1000 ms) of
> random jitter after the last heartbeat it received. After that the new
> leader still has to complete the election and commit the entry before
> the command can finish. With the default 1000 ms election timer and
> unlucky jitter this leaves almost no time for the election itself, so
> the command can time out just before the new leader commits its entry.
> With election timers shorter than ELECTION_RANGE_MSEC the timeout can
> even expire before the election starts at all.
>
> The command then fails with a timeout, which ovsdb-server treats as a
> temporary error and retries the transaction internally, even though the
> original entry is about to be applied. For a non-idempotent
> transaction, such as a row insert, the retry duplicates the data.
>
> This was seen as a failure of the "OVSDB cluster - txn on follower-2,
> leader crash before sending execRep, follower-3 becomes leader" test,
> where the retried transaction inserted a second QoS row:
>
> ./ovsdb-cluster.at:819: ovs-vsctl --db="$db" --no-leader-only \
> --no-wait --columns=type --bare list QoS
> @@ -1,2 +1,4 @@
> x
>
> +x
> +
>
> Add the random part of the election timeout to the command timeout so
> that the command cannot expire before an election it is supposed to
> survive has had a chance to complete.
>
> Reported-at: https://issues.redhat.com/browse/FDP-4210
> Fixes: 5a9b53a51ec9 ("ovsdb raft: Fix duplicated transaction execution when
> leader failover.")
> Signed-off-by: Timothy Redaelli <[email protected]>
> ---
> ovsdb/raft.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
Thanks, Timothy. The change looks fine to me in general, but see some
comments below.
>
> diff --git a/ovsdb/raft.c b/ovsdb/raft.c
> index b1355d41c..92686a9f7 100644
> --- a/ovsdb/raft.c
> +++ b/ovsdb/raft.c
> @@ -2188,16 +2188,18 @@ raft_run(struct raft *raft)
> if (raft->role == RAFT_LEADER) {
> raft_send_heartbeats(raft);
> }
> - /* Check if any commands timeout. Timeout is set to twice the time of
> - * election base time so that commands can complete properly during
> - * leader election. E.g. a leader crashed and current node with
> pending
> - * commands becomes new leader: the pending commands can still
> complete
> + /* Check if any commands timeout. Timeout is set to twice the
> + * election base time plus the election random range so that
> + * commands can complete properly during leader election.
> + * E.g. a leader crashed and current node with pending commands
> + * becomes new leader: the pending commands can still complete
> * if the crashed leader has replicated the transactions to majority
> of
> * followers before it crashed. */
It looks weird that the last line is much longer than the previous ones.
Please, re-wrap the lines to be about the same length as before. You'll
need to touch more lines, but the comment will look much better in the
code.
> struct raft_command *cmd;
> HMAP_FOR_EACH_SAFE (cmd, hmap_node, &raft->commands) {
> if (cmd->timestamp
> - && now - cmd->timestamp > raft->election_timer * 2) {
> + && now - cmd->timestamp > (raft->election_timer * 2
> + + ELECTION_RANGE_MSEC)) {
This line is also getting a little hard to read. Please, create a
variable right under the comment, e.g. 'uint64_t timeout' and use it here
for the comparison. The whole condition should also fit into a single
line this way.
> if (cmd->index && raft->role != RAFT_LEADER) {
> /* This server lost leadership and command didn't
> complete
> * in time. Likely, it wasn't replicated to the majority
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
