distsql: flow scheduler back up could result in failed streams

[asubiotto]

When scheduling a flow, the flowScheduler checks how many running flows there are

cockroach/pkg/sql/distsqlrun/flow_scheduler.go

Line 90 in a9410c5

return fs.mu.numRunning < fs.mu.maxRunningFlows

and enqueues a flow to be run later if there are too many running . This is a potential issue because if there are two nodes with uneven load, node A with a low load and node B with a high load, both could receive a request to set up a flow, node A could try to connect to node B but node B would have this flow backed up in its queue. If it takes too long to dequeue the flow on node B, the whole flow will fail.

A solution might be to set up these connections early regardless of the load (i.e. registering a flow through RegisterFlow as soon as we receive a request to set up a flow prior to a possible enqueue)

[ajwerner]

This behavior has become a regular nuisance. Having a fixed limit in the first place is probably wrong. The queuing can be okay if the expected wait time is low (probably not more than some multiple of the expected life of the flow itself). As the queue builds up the failure mode it triggers is to lead flow setup to timeout from the perspective of the upstream node. This ultimately results in a cryptic error to the user (no inboud stream connection) after the connection setup times out (10s!). There is essentially no back pressure mechanism whatsoever at this level. This is all made worse by the fact that when this flow setup fails there’s no mechanism to go and clean up all the other associated flow state.

We need a mechanism to determine when there are too many flows and that the cluster is overloaded and gracefully report that to the user.

[ajwerner]

In the very short term should we just question the default value of 500? Is it justified? My expectation is that the resource we're preserving here is RAM? What is a simplistic RAM model for each flow?

[petermattis]

We're limiting network and CPU use as well, though RAM is probably the most important (network and CPU will get slower, RAM will hit a wall and cause the node to die).

The flow limit is a form of admission control, but it is interesting that this is happening at an internal layer and not when the query first arrives. My intuition is that once we decide to process a query, we should process it.

Bumping the value of sql.distsql.max_running_flows significantly in the short term sounds reasonable. Medium term, we should figure out a better story for admission control / queuing / back pressure.

Cc @RaduBerinde and @andreimatei for their insight on the current design.
Cc @andy-kimball who has had thoughts about admission control.

[ajwerner]

My understanding is that @andreimatei and @andy-kimball have been discussing this throughout the day. I'm looking forward to seeing what falls out of the various discussions.

[awoods187]

@jordanlewis any update on addressing this item?

[ajwerner]

I intend to explore putting an admission controller here. The basic approach that’s being applied to kv should drop in nicely. There are some differences, especially that there’s not clear expectation that running flows will finish in a relatively timely manner the way that there is for kv reads but even the naive approach should be better than we have today.

[asubiotto]

Superseded by #34229