| Message ID | a8650e1e2d8ca7cc74ba0cd2db73165bd6005853.1786614464.git.tredaelli@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev] ovsdb: raft: Widen the pending command timeout. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_FreeBSD_Build_and_Test | success | github build: passed |
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 <tredaelli@redhat.com> > --- > 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.
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. */ 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)) { 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
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 <tredaelli@redhat.com> --- ovsdb/raft.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)