Limit HTTP server's concurrency using semaphore.

[juliusmh]

Signed-off-by: Julius Hinze [email protected]

Closes: #1346

[dougbtv]

Overall, I'm in favor of it, and I like that it's parameterized, but I'd like to see that pods do eventually come up in a situation where there's more than the concurrency limit, I believe those pods would crash loop?

I think maybe we should have some testing for it, best case scenario: e2e tests that both capture the error and the success.

[dougbtv]

Tomo also suggested taking a look at the limiters on https://pkg.go.dev/golang.org/x/net/netutil

[juliusmh]

Overall, I'm in favor of it, and I like that it's parameterized, but I'd like to see that pods do eventually come up in a situation where there's more than the concurrency limit, I believe those pods would crash loop?

I think maybe we should have some testing for it, best case scenario: e2e tests that both capture the error and the success.

These pods would wait as the semaphore blocks, until the shim timeouts the requests. In this case the pod will fail and kubelet try again in the future. Note that the overall CRI timeout is 4 minutes, we could adjust the shims timeout to match that.

So, we should decide if we want to

• fail the HTTP request if concurrency is too high, retry from shim?
• or block while concurrency limit is exceeded ?
• block using semaphore on listener level (LimitListener)?
• or block inside the handler (this PR)?

[juliusmh]

@dougbtv fixed unit test & rebased. Implementation using the LimitListener you suggested is here: #1356

[s1061123]

I suppose using LimitListener, we don't need to introduce semaphore because LimitListener does the same things. Can we close it and proceed #1356?

[coveralls]

coverage: 56.391% (-0.06%) from 56.454%
when pulling 685bb6b on juliusmh:jmh/limit_concurrency
into 781ecda on k8snetworkplumbingwg:master.

[juliusmh]

Closing in favor of #1356