raft: fix bug in unbounded log growth prevention mechanism #10199

tbg · 2018-10-19T18:07:48Z

The previous code was using the proto-generated Size() method to
track the size of an incoming proposal at the leader. This includes
the Index and Term, which were mutated after the call to Size()
when appending to the log. Additionally, it was not taking into
account that an ignored configuration change would ignore the
original proposal and append an empty entry instead.

As a result, a fully committed Raft group could end up with a non-
zero tracked uncommitted Raft log counter that would eventually hit
the ceiling and drop all future proposals indiscriminately. It would
also immediately imply that proposals exceeding the threshold alone
would get refused (as the "first uncommitted proposal" gets special
treatment and is always allowed in).

Track only the size of the payload actually appended to the Raft log
instead.

For context, see:
cockroachdb/cockroach#31618 (comment)

gyuho-bot · 2018-10-19T18:07:51Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approvers:

If they are not already assigned, you can assign the PR to them by writing /assign in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

gyuho · 2018-10-19T18:22:30Z

Please ignore above comments, while we are experimenting Prow integration.

/ok-to-test

gyuho · 2018-10-19T18:23:33Z

Looks good to me. Defer to @xiang90 and @bdarnell.

nvanbenschoten · 2018-10-19T18:38:25Z

Looks good to me as well, although it' kind of strange that the size limit imposed by raftLog.maxSize and Storage.Entries use the proto's size, while this size limit uses the entry.Data size.

An alternative you could explore is pushing this uncommitted size limit into appendEntry, after Term and Index are assigned, and use the proto size. This is what I did at first in #10167, but changed my approach because I didn't want the empty proposal from becomeLeader to be counted towards the limit in case the very first real proposal was above the uncommitted size limit. In hindsight, that would have avoided the issue we saw in cockroachdb/cockroach#31618.

codecov-io · 2018-10-19T19:18:34Z

Codecov Report

Merging #10199 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #10199      +/-   ##
==========================================
+ Coverage   71.85%   71.87%   +0.01%     
==========================================
  Files         390      390              
  Lines       36346    36348       +2     
==========================================
+ Hits        26116    26124       +8     
+ Misses       8424     8417       -7     
- Partials     1806     1807       +1

Impacted Files	Coverage Δ
raft/util.go	`56.52% <100%> (+1.29%)`	⬆️
raft/raft.go	`92.29% <100%> (ø)`	⬆️
proxy/grpcproxy/register.go	`80.55% <0%> (-13.89%)`	⬇️
pkg/adt/interval_tree.go	`86.48% <0%> (-6.91%)`	⬇️
pkg/tlsutil/tlsutil.go	`86.2% <0%> (-6.9%)`	⬇️
etcdctl/ctlv3/command/printer_simple.go	`72.48% <0%> (-1.35%)`	⬇️
etcdserver/api/rafthttp/peer.go	`78.77% <0%> (-1.12%)`	⬇️
etcdserver/cluster_util.go	`58.74% <0%> (-0.9%)`	⬇️
etcdserver/api/rafthttp/stream.go	`79.39% <0%> (-0.86%)`	⬇️
etcdserver/api/membership/cluster.go	`77.44% <0%> (-0.46%)`	⬇️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c80efb...ea5ff8a. Read the comment docs.

tbg · 2018-10-22T08:32:13Z

I pushed the computation down into appendEntries and had changed it to use .Size(), but I actually ended up reintroducing PayloadSize. It's pretty awkward to have to guess all the assigned terms and indexes during the tests, to the point where the tests become a mess (or you have to introduce additional tools).

The previous code was using the proto-generated `Size()` method to track the size of an incoming proposal at the leader. This includes the Index and Term, which were mutated after the call to `Size()` when appending to the log. Additionally, it was not taking into account that an ignored configuration change would ignore the original proposal and append an empty entry instead. As a result, a fully committed Raft group could end up with a non- zero tracked uncommitted Raft log counter that would eventually hit the ceiling and drop all future proposals indiscriminately. It would also immediately imply that proposals exceeding the threshold alone would get refused (as the "first uncommitted proposal" gets special treatment and is always allowed in). Track only the size of the payload actually appended to the Raft log instead. For context, see: cockroachdb/cockroach#31618 (comment)

tbg · 2018-10-22T13:34:07Z

Various tests failed, but I'm not sure that's really caused by this PR. @gyuho what do you think?

tbg · 2018-10-22T13:34:22Z

(I'm going to re-run just to get an idea of whether they're flakes or not)

tbg · 2018-10-22T18:23:50Z

@gyuho I re-triggered the build but it's still red. master looks pretty red as well. Could you help me push this over the finish line?

gyuho · 2018-10-22T18:28:39Z

@tschottdorf

https://status.github.com/messages

11:26 PDT
We have resumed webhook delivery and will continue to monitor as we process the backlog of events.

Seems like github is just back. Will retrigger manually.

tbg · 2018-10-22T18:47:41Z

@gyuho I re-triggered the build but it's still red

I actually meant that it failed again :-) I triggered directly on travis-ci. Let's see what the next run brings.

gyuho

LGTM. etcd is experiencing some flaky tests not related to this change. Should be good to merge. /cc @xiang90

tbg · 2018-10-22T19:29:55Z

Thanks @gyuho, appreciate the heads up.

Pick up etcd-io/etcd#10199.

31554: exec: initial commit of execgen tool r=solongordon a=solongordon Execgen will be our tool for generating templated code necessary for columnarized execution. So far it only generates the EncDatumRowsToColVec function, which is used by the columnarizer to convert a RowSource into a columnarized Operator. Release note: None 31610: sql: fix pg_catalog.pg_constraint's confkey column r=BramGruneir a=BramGruneir Prior to this patch, all columns in the index were included instead of only the ones being used in the foreign key reference. Fixes #31545. Release note (bug fix): Fix pg_catalog.pg_constraint's confkey column from including columns that were not involved in the foreign key reference. 31689: storage: pick up fix for Raft uncommitted entry size tracking r=benesch a=tschottdorf Waiting for the upstream PR etcd-io/etcd#10199 to merge, but this is going to be what the result will look like. ---- The tracking of the uncommitted portion of the log had a bug where it wasn't releasing everything as it should've. As a result, over time, all proposals would be dropped. We're hitting this way earlier in our import tests, which propose large proposals. As an intentional implementation detail, a proposal that itself exceeds the max uncommitted log size is allowed only if the uncommitted log is empty. Due to the leak, we weren't ever hitting this case and so AddSSTable commands were often dropped indefinitely. Fixes #31184. Fixes #28693. Fixes #31642. Optimistically: Fixes #31675. Fixes #31654. Fixes #31446. Release note: None Co-authored-by: Solon Gordon <solon@cockroachlabs.com> Co-authored-by: Bram Gruneir <bram@cockroachlabs.com> Co-authored-by: Tobias Schottdorf <tobias.schottdorf@gmail.com>

Picks up etcd-io/etcd#10199.

Picks up the bug fix etcd-io/etcd#10199. It also picks up these further small fixes (which is intentional, I authored most of them): 5c209d6 raft: ensure leader is in ProgressStateReplicate 1569f48 raft: print RejectHint of zero on MsgAppResp 9668536 raft: add a test case in TestStorageAppend e4af2be raft: separate MaxCommittedSizePerReady config from MaxSizePerMsg aa4313a *: fix github links 4aa72ca raft: Explain ReportSnapshot and Propose behavior b7ed416 raft: fix godoc in tests 10255cf raft: Fix comment on TestLeaderBcastBeat c561f83 OWNERS: experiment ad49c8f raft: fix bug in unbounded log growth prevention mechanism de47099 raft: fix description in UT 73c20cc raft: Fix comment on sendHeartbeat 7be7ac5 raft: Fix spelling in doc.go Release note: None

gyuho-bot added size/M labels Oct 19, 2018

tbg force-pushed the fix-max-uncommitted-size branch from 490ed06 to ea5ff8a Compare October 19, 2018 18:09

nvanbenschoten mentioned this pull request Oct 19, 2018

storage: sideloaded Raft entries don't play well with Raft memory limits cockroachdb/cockroach#31618

Closed

gyuho-bot removed the needs-ok-to-test label Oct 19, 2018

tbg force-pushed the fix-max-uncommitted-size branch from ea5ff8a to 3c480ce Compare October 22, 2018 08:28

tbg requested a review from bdarnell October 22, 2018 08:32

tbg force-pushed the fix-max-uncommitted-size branch from 3c480ce to ad49c8f Compare October 22, 2018 09:28

tbg mentioned this pull request Oct 22, 2018

storage: pick up fix for Raft uncommitted entry size tracking cockroachdb/cockroach#31689

Merged

bdarnell approved these changes Oct 22, 2018

View reviewed changes

gyuho approved these changes Oct 22, 2018

View reviewed changes

tbg merged commit b42b394 into etcd-io:master Oct 22, 2018

tbg deleted the fix-max-uncommitted-size branch October 22, 2018 19:30

tbg added a commit to cockroachdb/vendored that referenced this pull request Oct 22, 2018

Bump etcd

50f622b

Pick up etcd-io/etcd#10199.

tbg mentioned this pull request Dec 18, 2018

roachtest: import/tpch/nodes=8 failed [release-2.1] cockroachdb/cockroach#32898

Closed

tbg added a commit to cockroachdb/vendored that referenced this pull request Dec 18, 2018

Bump etcd

7384d66

Picks up etcd-io/etcd#10199.

tbg mentioned this pull request Dec 18, 2018

backport-2.1: Bump etcd/raft to pick up uncommitted log size fix cockroachdb/cockroach#33228

Merged

absolute8511 mentioned this pull request Mar 13, 2019

Consider merge some optimize from etcd-raft youzan/ZanRedisDB#23

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

raft: fix bug in unbounded log growth prevention mechanism #10199

raft: fix bug in unbounded log growth prevention mechanism #10199

tbg commented Oct 19, 2018

gyuho-bot commented Oct 19, 2018

gyuho commented Oct 19, 2018

gyuho commented Oct 19, 2018

nvanbenschoten commented Oct 19, 2018

codecov-io commented Oct 19, 2018 •

edited

Loading

tbg commented Oct 22, 2018

tbg commented Oct 22, 2018

tbg commented Oct 22, 2018

tbg commented Oct 22, 2018

gyuho commented Oct 22, 2018

tbg commented Oct 22, 2018

gyuho left a comment

tbg commented Oct 22, 2018

raft: fix bug in unbounded log growth prevention mechanism #10199

raft: fix bug in unbounded log growth prevention mechanism #10199

Conversation

tbg commented Oct 19, 2018

gyuho-bot commented Oct 19, 2018

gyuho commented Oct 19, 2018

gyuho commented Oct 19, 2018

nvanbenschoten commented Oct 19, 2018

codecov-io commented Oct 19, 2018 • edited Loading

Codecov Report

tbg commented Oct 22, 2018

tbg commented Oct 22, 2018

tbg commented Oct 22, 2018

tbg commented Oct 22, 2018

gyuho commented Oct 22, 2018

tbg commented Oct 22, 2018

gyuho left a comment

Choose a reason for hiding this comment

tbg commented Oct 22, 2018

codecov-io commented Oct 19, 2018 •

edited

Loading