raft: send empty appends when replication is paused #14633

pav-kv · 2022-10-26T23:06:49Z

When Inflights to a particular node is full, i.e. MaxInflightMsgs for the append messages flow is saturated, it is still necessary to continue sending MsgApp to ensure progress. Currently this is achieved by "forgetting" the first in-flight message in the window, which frees up quota for one new MsgApp.

This new message is constructed in such a way that it potentially has multiple entries, or a large entry. The effect of this is that the in-flight limitations can be exceeded arbitrarily, for as long as the flow to this node continues being saturated. In particular, if a follower is stuck, the leader will keep sending entries to it.

This commit makes the MsgApp empty when Inflights is saturated, and prevents the described leakage of Entries to slow followers.

Signed-off-by: Pavel Kalinnikov pavel@cockroachlabs.com

pav-kv · 2022-10-26T23:10:04Z

Fix the tests in raft_flow_control_test.go
Make a regression test for the described unlimited leakage behaviour.

codecov-commenter · 2022-10-26T23:42:54Z

Codecov Report

Merging #14633 (b525d02) into main (5073af6) will decrease coverage by 0.28%.
The diff coverage is 90.19%.

@@            Coverage Diff             @@
##             main   #14633      +/-   ##
==========================================
- Coverage   75.81%   75.53%   -0.29%     
==========================================
  Files         457      457              
  Lines       37292    37298       +6     
==========================================
- Hits        28274    28174     -100     
- Misses       7271     7348      +77     
- Partials     1747     1776      +29

Flag	Coverage Δ
all	`75.53% <90.19%> (-0.29%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
raft/tracker/inflights.go	`95.83% <ø> (-0.09%)`	⬇️
raft/raft.go	`89.93% <89.47%> (-0.33%)`	⬇️
raft/tracker/progress.go	`95.91% <89.47%> (-1.76%)`	⬇️
raft/rafttest/interaction_env_handler_add_nodes.go	`71.64% <90.90%> (+2.28%)`	⬆️
raft/rafttest/interaction_env.go	`100.00% <100.00%> (ø)`
client/pkg/v3/testutil/leak.go	`62.83% <0.00%> (-7.08%)`	⬇️
client/v3/experimental/recipes/queue.go	`58.62% <0.00%> (-6.90%)`	⬇️
client/v3/leasing/util.go	`91.66% <0.00%> (-6.67%)`	⬇️
server/etcdserver/api/v3rpc/interceptor.go	`77.60% <0.00%> (-5.21%)`	⬇️
server/storage/mvcc/watchable_store.go	`89.13% <0.00%> (-5.08%)`	⬇️
... and 22 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

raft/raft_flow_control_test.go

pav-kv · 2022-10-28T18:33:13Z

@tbg This is now ready for review.

raft/tracker/progress.go

tbg

Looks good! I'd mainly try to rearrange code and add more comments so that the intricacies of this dance are bit easier to grasp for future readers. See inline comments.

I also think it is worthwhile to add a TestInteraction test for this.

raft/raft.go

raft/tracker/progress.go

pav-kv

@tbg Addressed most comments, notably added the data-driven test.

raft/raft.go

raft/tracker/progress.go

pav-kv · 2022-11-01T22:40:19Z

@ahrtr Please review or redirect to someone in etcd/raft folks. Commit-by-commit review is recommended. Only the 5-th commit raft: send empty appends when replication is paused changes the behaviour.

Overall commits structure:

First 2 commits only add and strengthen tests. In particular, the first commit captures the problematic behaviour that this PR solves.
Commits 3 and 4 contain drive-by small refactorings that don't change behaviour, but pave the way towards the fix.
Commit 5 is the fix. Notice how it modifies the behaviour, which is reflected in tests.
The last commit removes a no longer used method.

raft/testdata/replicate_pause.txt

raft/raft.go

raft/tracker/progress.go

tbg · 2022-11-03T13:35:48Z

Looks good to me.

@ahrtr mind taking a look? There is an E2E failure¹; I find these impossible to decode and they often seem to be flaky, so not sure if it's something that we should just retry.

https://github.com/etcd-io/etcd/actions/runs/3373310602/jobs/5598102673#step:5:1112 ↩

pav-kv · 2022-11-04T18:01:50Z

There is an E2E failure

Probably flaky. The last run is good.

raft/tracker/progress.go

raft/raft.go

pav-kv

Addressed comments form @ahrtr

raft/raft.go

raft/tracker/progress.go

ahrtr

Overall looks good to me.

Thank you @pavelkalinnikov

ahrtr · 2022-11-08T22:19:06Z

We recently added some mix-versions test, and also removed some dependencies, so please rebase this PR. Once all pipelines are green, then it's OK to merge this PR.

This commit adds a data-driven test which simulates conditions under which Raft messages flow to a particular node is throttled while in StateReplicate. The test demonstrates that MsgApp messages with non-empty Entries may "leak" to a paused stream every time there is successful heartbeat exchange. Signed-off-by: Pavel Kalinnikov <pavel@cockroachlabs.com>

Signed-off-by: Pavel Kalinnikov <pavel@cockroachlabs.com>

- avoid large indented blocks, leave the main block unindented - declare pb.Message inlined in the sending call Signed-off-by: Pavel Kalinnikov <pavel@cockroachlabs.com>

Previously, Progress update on MsgApp send was scattered across raft.go and tracker/progress.go. This commit better encapsulates this logic in the Progress type. Signed-off-by: Pavel Kalinnikov <pavel@cockroachlabs.com>

When Inflights to a particular node is full, i.e. MaxInflightMsgs for the append messages flow is saturated, it is still necessary to continue sending MsgApp to ensure progress. Currently this is achieved by "forgetting" the first in-flight message in the window, which frees up quota for one new MsgApp. This new message is constructed in such a way that it potentially has multiple entries, or a large entry. The effect of this is that the in-flight limitations can be exceeded arbitrarily, for as long as the flow to this node continues being saturated. In particular, if a follower is stuck, the leader will keep sending entries to it. This commit makes the MsgApp empty when Inflights is saturated, and prevents the described leakage of Entries to slow followers. Signed-off-by: Pavel Kalinnikov <pavel@cockroachlabs.com>

Signed-off-by: Pavel Kalinnikov <pavel@cockroachlabs.com>

Make the field name and comment clearer on the fact that it's used both in StateProbe and StateReplicate. The old name ProbeSent was slightly confusing, and also triggered thinking that it's used only in StateProbe. Signed-off-by: Pavel Kalinnikov <pavel@cockroachlabs.com>

pav-kv · 2022-11-08T22:23:00Z

We recently added some mix-versions test, and also removed some dependencies, so please rebase this PR. Once all pipelines are green, then it's OK to merge this PR.

@ahrtr Rebased. Could you trigger CI once again and/or merge?

ahrtr · 2022-11-08T22:24:54Z

We recently added some mix-versions test, and also removed some dependencies, so please rebase this PR. Once all pipelines are green, then it's OK to merge this PR.

@ahrtr Rebased. Could you trigger CI once again and/or merge?

Done.

pav-kv mentioned this pull request Oct 26, 2022

raft: add MaxInflightBytes limit to the outgoing messages flow #14624

Merged

pav-kv force-pushed the send_empty_append branch from 9386713 to 1233147 Compare October 26, 2022 23:38

pav-kv commented Oct 27, 2022

View reviewed changes

raft/raft_flow_control_test.go Outdated Show resolved Hide resolved

pav-kv force-pushed the send_empty_append branch 2 times, most recently from 8bf78f9 to e00dba8 Compare October 28, 2022 18:29

pav-kv commented Oct 28, 2022

View reviewed changes

raft/tracker/progress.go Outdated Show resolved Hide resolved

pav-kv mentioned this pull request Oct 28, 2022

kvserver: add MaxInflightMsgBytes to raft cockroachdb/cockroach#90314

Closed

pav-kv force-pushed the send_empty_append branch 6 times, most recently from 83559d3 to 330bcb2 Compare October 28, 2022 23:45

tbg approved these changes Oct 31, 2022

View reviewed changes

raft/raft.go Show resolved Hide resolved

raft/tracker/progress.go Outdated Show resolved Hide resolved

raft/tracker/progress.go Outdated Show resolved Hide resolved

pav-kv force-pushed the send_empty_append branch 3 times, most recently from 7680b2a to cae22cd Compare October 31, 2022 23:08

pav-kv commented Oct 31, 2022

View reviewed changes

raft/raft.go Show resolved Hide resolved

raft/tracker/progress.go Outdated Show resolved Hide resolved

pav-kv force-pushed the send_empty_append branch 6 times, most recently from 1928d4d to 9d81638 Compare November 1, 2022 22:19

tbg approved these changes Nov 3, 2022

View reviewed changes

raft/testdata/replicate_pause.txt Show resolved Hide resolved

raft/raft.go Show resolved Hide resolved

raft/raft.go Show resolved Hide resolved

raft/tracker/progress.go Outdated Show resolved Hide resolved

nvanbenschoten mentioned this pull request Nov 7, 2022

raft: support asynchronous storage writes #14627

Closed

ahrtr added the area/raft label Nov 7, 2022

ahrtr reviewed Nov 7, 2022

View reviewed changes

raft/tracker/progress.go Outdated Show resolved Hide resolved

ahrtr reviewed Nov 7, 2022

View reviewed changes

raft/tracker/progress.go Outdated Show resolved Hide resolved

ahrtr reviewed Nov 7, 2022

View reviewed changes

raft/tracker/progress.go Outdated Show resolved Hide resolved

ahrtr reviewed Nov 7, 2022

View reviewed changes

raft/raft.go Show resolved Hide resolved

pav-kv commented Nov 8, 2022

View reviewed changes

raft/raft.go Show resolved Hide resolved

raft/tracker/progress.go Outdated Show resolved Hide resolved

raft/tracker/progress.go Outdated Show resolved Hide resolved

pav-kv force-pushed the send_empty_append branch 3 times, most recently from 5817a3f to b525d02 Compare November 8, 2022 13:15

tbg approved these changes Nov 8, 2022

View reviewed changes

raft/tracker/progress.go Show resolved Hide resolved

ahrtr approved these changes Nov 8, 2022

View reviewed changes

pav-kv added 7 commits November 8, 2022 22:21

raft: elaborate checks in flow control tests

5619953

Signed-off-by: Pavel Kalinnikov <pavel@cockroachlabs.com>

raft: cleanup maybeSendAppend method

d5ac7b8

- avoid large indented blocks, leave the main block unindented - declare pb.Message inlined in the sending call Signed-off-by: Pavel Kalinnikov <pavel@cockroachlabs.com>

raft: extract Progress update on MsgApp to a method

3bc3d20

Previously, Progress update on MsgApp send was scattered across raft.go and tracker/progress.go. This commit better encapsulates this logic in the Progress type. Signed-off-by: Pavel Kalinnikov <pavel@cockroachlabs.com>

raft/tracker: remove unused Inflights.FreeFirstOne

467114e

Signed-off-by: Pavel Kalinnikov <pavel@cockroachlabs.com>

pav-kv force-pushed the send_empty_append branch from b525d02 to 1ea1349 Compare November 8, 2022 22:21

ahrtr merged commit 49ecea5 into etcd-io:main Nov 8, 2022

pav-kv deleted the send_empty_append branch November 8, 2022 22:56

pav-kv mentioned this pull request Mar 6, 2023

roachtest: sysbench/oltp_write_only/nodes=3/cpu=32/conc=256 failed [tocommit out of range] cockroachdb/cockroach#97926

Closed

andrewbaptist mentioned this pull request May 18, 2023

roachtest: suspicious KV logs in import/tpcc/warehouses=4000/geo failure cockroachdb/cockroach#102803

Closed

erikgrinaker mentioned this pull request Jan 15, 2024

Add changelog for 3.6 etcd-io/raft#125

Merged

pav-kv mentioned this pull request Feb 14, 2024

raft: consolidate all append message sending etcd-io/raft#134

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

raft: send empty appends when replication is paused #14633

raft: send empty appends when replication is paused #14633

pav-kv commented Oct 26, 2022 •

edited

Loading

pav-kv commented Oct 26, 2022 •

edited

Loading

codecov-commenter commented Oct 26, 2022 •

edited

Loading

pav-kv commented Oct 28, 2022

tbg left a comment

pav-kv left a comment

pav-kv commented Nov 1, 2022

tbg commented Nov 3, 2022

pav-kv commented Nov 4, 2022

pav-kv left a comment

ahrtr left a comment

ahrtr commented Nov 8, 2022

pav-kv commented Nov 8, 2022

ahrtr commented Nov 8, 2022

raft: send empty appends when replication is paused #14633

raft: send empty appends when replication is paused #14633

Conversation

pav-kv commented Oct 26, 2022 • edited Loading

pav-kv commented Oct 26, 2022 • edited Loading

codecov-commenter commented Oct 26, 2022 • edited Loading

Codecov Report

pav-kv commented Oct 28, 2022

tbg left a comment

Choose a reason for hiding this comment

pav-kv left a comment

Choose a reason for hiding this comment

pav-kv commented Nov 1, 2022

tbg commented Nov 3, 2022

Footnotes

pav-kv commented Nov 4, 2022

pav-kv left a comment

Choose a reason for hiding this comment

ahrtr left a comment

Choose a reason for hiding this comment

ahrtr commented Nov 8, 2022

pav-kv commented Nov 8, 2022

ahrtr commented Nov 8, 2022

pav-kv commented Oct 26, 2022 •

edited

Loading

pav-kv commented Oct 26, 2022 •

edited

Loading

codecov-commenter commented Oct 26, 2022 •

edited

Loading