storage: raft snapshot queue inefficiencies

[tbg]

When Raft wants a snapshot (MsgSnap), we (on the leader) .Add the range to the raft snapshot queue. However, there are various code paths that can lead to the range not receiving a snapshot "right away" (purgatory, max size limitation). If that happens, the Raft group is stuck in an awkward state until the scanner picks up the range again and hopefully does give it a snapshot.

This setup seems risky and we may want to address the above points so that the Raft-initiated snapshots have no chance of being lost.

[tbg]

Another issue that I've noticed in #31732 (comment) is that Raft snapshots block on a semaphore on the receiver, which adds a component that can add latency in non-obvious ways.

The raft log queue allows a concurrency of four, so you'd have to send to four backed-up nodes at the same time to get a complete pipeline stall, but it did happen in my (admittedly pretty busted) experiments.

Not sure there's really something we can easily improve there. I guess we could change the queue semantics to have at most four "sending" snapshots at any given point (i.e. allow more goroutines to get queued into semaphores on various nodes).

The reasonable place to invest the energy is probably to make sure that we're not getting into situations in which there are hundreds of outstanding Raft snapshots.

[petermattis]

However, there are various code paths that can lead to the range not receiving a snapshot "right away" (purgatory, max size limitation).

Does the Raft snapshot queue have a purgatory? (I know that all queues have purgatory, but actually placing replicas in purgatory requires an error that implements purgatoryErrorMarker).

If that happens, the Raft group is stuck in an awkward state until the scanner picks up the range again and hopefully does give it a snapshot.

s/awkward/fragile/g.

Note that we call Replica.reportSnapshotStatus on both success and failure. That calls into Raft which will likely request another snapshot placing the replica back in the queue immediately.

[tbg]

Does the Raft snapshot queue have a purgatory?

I doubt it, but it shouldn't have room for this kind of error.

Note that we call Replica.reportSnapshotStatus on both success and failure.

No, we would never call this if the queue were full (limit is 10k). We only report a status if we actually tried to process a snapshot.

I'm not aware that there's anything fishy going on in the real world. I'm just uneasy that the queue code isn't really made for a kind of queue in which offering a replica requires an action to be taken.

[petermattis]

We only report a status if we actually tried to process a snapshot.

Oh, that's a good point.

I'm not aware that there's anything fishy going on in the real world. I'm just uneasy that the queue code isn't really made for a kind of queue in which offering a replica requires an action to be taken.

Now you've made me uneasy. Perhaps we should bump the max size for this queue. On the other hand, can we realistically process 10k Raft snapshots (10k is the max size for the Raft snapshot queue) in the scanner interval?

[tbg]

Another issue (that I think I've observed) is that the raft snapshot queue uses the same priority for everything. It doesn't use this code

cockroach/pkg/storage/queue.go

Lines 148 to 157 in 3cc9d57

	if diff := now.GoTime().Sub(last.GoTime()); diff >= minInterval {
	priority := float64(1)
	// If there's a non-zero last processed timestamp, adjust the
	// priority by a multiple of how long it's been since the last
	// time this replica was processed.
	if last != (hlc.Timestamp{}) {
	priority = float64(diff.Nanoseconds()) / float64(minInterval.Nanoseconds())
	}
	return true, priority
	}

As a result, I think that the priority queue will mostly be LIFO, which means that a snapshot that's queued up can be delayed arbitrarily if there's a frequent enough stream of other snapshot requests being pushed into the queue.

[tbg]

Have to think about whether it's the right thing to do, but we could just use a priority of (morally) math.MaxInt64 - time.Now() to make that FIFO.

[petermattis]

I noticed that same weakness of the queue priorities in some other context. Seems worth fixing as the starvation it can cause is unexpected. My thought was to add a sequence number of some sort so that priorities are never exactly equal.