storage: avoid creating excessively large snapshots

[petermattis]

We sometimes see out of memory errors when creating excessively large snapshots. For example, from #6991:

I160701 14:45:05.143248 storage/replica_raftstorage.go:524  generated snapshot for range 403 at index
 3533112 in 31.747833551s. encoded size=1072891308, 6966 KV pairs, 1677671 log entries
fatal error: runtime: out of memory

Notice the huge raft log. Because the raft log is part of the replicated state we must send all of the raft log with the snapshot in order to avoid divergence of the new replica. (@bdarnell Perhaps the applied portion of the raft log should not be considered during consistency checks).

It should be possible to add a failsafe to Replica.snapshot() so that if we see a very large snapshot is being created we return raft.ErrSnapshotTemporarilyUnavailable and possibly add the replica to the raft-log-gc queue.

[tbg]

The Raft log actually isn't part of the replicated state. The part which
needs to be replicated is the truncated state (which is used to compute the
first index). It seems that we could get away with not sending the large
suffix when it is largely uncommitted. We probably can't artificially lower
the committed index (which would allow us to remove a tail of committed
commands, too) because on the receiving side, we might be overwriting a
state which has acknowledged entries we're now overwriting.

On Fri, Jul 1, 2016 at 11:06 AM Peter Mattis [email protected]
wrote:

We sometimes see out of memory errors when creating excessively large
snapshots. For example, from #6991
#6991:

I160701 14:45:05.143248 storage/replica_raftstorage.go:524 generated snapshot for range 403 at index
3533112 in 31.747833551s. encoded size=1072891308, 6966 KV pairs, 1677671 log entries
fatal error: runtime: out of memory

Notice the huge raft log. Because the raft log is part of the replicated
state we must send all of the raft log with the snapshot in order to avoid
divergence of the new replica. (@bdarnell https://github.com/bdarnell
Perhaps the applied portion of the raft log should not be considered during
consistency checks).

It should be possible to add a failsafe to Replica.snapshot() so that if
we see a very large snapshot is being created we return
raft.ErrSnapshotTemporarilyUnavailable and possibly add the replica to
the raft-log-gc queue.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#7581, or mute the thread
https://github.com/notifications/unsubscribe/AE135K5ruAFKlxenvdyzFro0JHiq7mZFks5qRSzpgaJpZM4JDOgY
.

-- Tobias

[petermattis]

Oh, I completely misread RaftLogKey. You're correct that the raft-log isn't part of the replicated state. Seems like it should be straightforward to not send the applied portion of the raft log.

[tbg]

Hmm, I just checked and we only send the applied log entries. Bummer, doesn't seem like there's a corner we can cut unless we make the truncated state unreplicated (I'd rather not).

[petermattis]

Eh, looks like snapshot() iterates over the raft log from truncState.Index + 1 to appliedIndex + 1. Doesn't that contain all of the applied entries?

[tbg]

yes, all of those that haven't been truncated (which in an ideal world is very few due to aggressive Raft log truncation). My point about truncatedState is that we could easily not send all of those entries if we could modify the truncatedState, but that is replicated. Maybe we want to change that, but I haven't thought it through. If there's no downside to it elsewhere, the benefit for snapshots would definitely make it worthwhile considering it.

[petermattis]

Got it. So the raft log entries are not part of the truncated state, but the index we've truncated to is.

[petermattis]

@dt Adding a failsafe to Replica.snapshot() as mentioned in the original message should be straightforward. As usual, probably the most difficult part will be adding a test.

[dt]

#7788