Better-behaved HBONE pooling

[bleggett]

This works around several issues found in #864 relating to clients (loadgen mostly) that open many connections very rapidly.

1. hyper's pooling library has rather basic scheduling controls, to the point where we can rather easily overwhelm "send_request" such that it never resolves. Tweaking server/client streamlimits does not work around this, as the pooling scheduler does not respect them when selecting pooled streams, and will simply allow a single conn to get backed up.

2. hyper's pooling library doesn't really have the concept of multiple HTTP/2 connections to the same dest, which makes sense as the HTTP/2 spec doesn't recommend this (the spec says SHOULD NOT instead of MUST NOT, which in spec-speak is "we can't stop you, I guess")

3. Racing to create connections creates problems in scenarios where a lot of connection attempts are made at once due to lack of throttling - many connections can actually be established or "cross the finish line at the same time", when we only need one, and this noise can compromise the ability of the local or dest ztunnel to respond to other workloads.

Notes:

• By design, this pool does not manage Hyper client state, nor is it tightly coupled with it. All it does is hold references to Hyper clients that can be reused, and evict its own held references to those Hyper clients when they cannot be reused. The rest is managing async access.

• By design, this pool is designed to create backpressure when one client opens many connections to the same destination at the same time. It does this by locking connection-mutative operations per src/dest key.

• The only write lock should be on that inner src/dest key. The only operations inside the pool that must be ordered/force ordering are 1. Ensuring that we have a key writelock in the outer map. 2. Locking the inner key writelock. The rest should not demand any sort of ordering to be safe.

• This pool does not decrement stream counts - when a connection hits its max stream count, the pool pops the reference and removes it from the rotation, leaving the connection to die when whatever is using it drops all the refs, and will create a new connection on the next go round. This leads to simpler accounting, while still maintaining the protections against connection storms that we need.

[istio-testing]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[ilrudie]

I might be missing something but it seems like correctly resolving hash collisions is critical for the pool's functionality and not yet implemented or tested.

[bleggett]

Added a form of conn timeout to evict connections from the pool that haven't been used in a while.

Once evicted, they will be closed by hyper when clients close, due to streamcount || refcount being == 0

[bleggett]

Switched to a proper concurrent hashmap that actually has a decent pedigree, which should allow for nonblocking reads (at the cost of using a little more memory, TANSTAAFL)

[ilrudie]

I'm not fully through the PR but this seemed like it was worth commenting on early perhaps so here goes:

[ilrudie]

Thinking more about this it feels, really non-rusty. I suppose the issue pingora and there's not, at present, a very good way around this though so I don't have a suggestion.

[bleggett]

What does? The use of locks?

[ilrudie]

Just locks which don't wrap the thing they're locking...

[bleggett]

Ah, yep. It's not very cosmetic, agree.

But the kind of locking we have to do is not something the pools out there will do for us, and pingora_pool has some use, at least.

The important thing about a Mutex is what holds it, what's inside is sort of moot. The mutex here is almost a semaphore arguably, which is why it feels weird.

[ilrudie]

Its moot unless we want to fork or contribute to pingora (or I guess lock the entire pool, which we don't care to do) but the point of wrapping the mutex around the thing being accessed is more than cosmetic. It ties holding the lock and accessing the thing together in a way that we can't do with pingora. We'll need to be more careful since we can't do take advantage of that.

[bleggett]

If we wanted to make this simpler later, I would advocate we throw out pingora and implement our own inner data structure from scratch, what we want to do is non-standard and pingora is not likely to want or need what we would have to change to make it work like this - nor is any other "generic" HTTP2 pool.

Which is why for now I just use it and then do the other stuff outside.

[howardjohn]

Ill review more closely tomorrow, only had a chance for a quick look. As such, many of my comments are probably not accurate

[howardjohn]

If we have multiple waiters, can they all end up here at once and spam a bunch of new connections?

[bleggett]

No, because we only get a NONE here if we found a conn, checked it out, and it was at maxed streamcount.

(and we don't put it back in in that case, we leave it popped to die a natural death once streamcount + refcount == 0)

If we popped it, no one else can.

[howardjohn]

I am seeing unexpected behavior. Open up 99 connections at once, I got 9 connections. Only 1 of them closes after the idle tieout, the rest are zombies. Also if I then open up 99 more, I get the same - 8 more zombies, 16 total.

[bleggett]

I am seeing unexpected behavior. Open up 99 connections at once, I got 9 connections. Only 1 of them closes after the idle tieout, the rest are zombies. Also if I then open up 99 more, I get the same - 8 more zombies, 16 total.

I may have broken it with recent commits then, and the tests aren't sufficient/needs more tests - test_pool_100_clients_singleconn should/must/needs to catch this.

[bleggett]

/test test

[bleggett]

1. The hyper bug in http: fix connections that are never closed hyperium/hyper#3647 is worked around for now with server keepalives. This will do until we take a bumped hyper, and we need server keepalives anyway, as a safety valve to keep misbehaving clients (even if they are ours) from hanging connections.

2. I am not 100% happy with the test rig - it is crude, it is synthetic, but it does the job and makes this that much harder to break, and it's certainly covering more behavior than before.

[istio-testing]

In response to a cherrypick label: new pull request created: #983

[bleggett]

(ty vm to @howardjohn for tracking down that hyper bug, hyper code gives me the willies - that's HTTP/2's fault, not hyper's fault)