Feature/admission control multi layer support #2443
Conversation
szuecs
force-pushed
the
feature/admission-control-multi-layer-support
branch
from
8183949
to
7ee6f29
Compare
…y other admission controllers test: create test that check the leaf admission controllers will not interfere with inode admission controllers in the call tree test: fix flaky test (works on my machine) Signed-off-by: Sandor Szücs <[email protected]>
szuecs
force-pushed
the
feature/admission-control-multi-layer-support
branch
from
7ee6f29
to
d9cfbcc
Compare
|
Also got #2435 |
|
|
||
| argsLeaf := make([]interface{}, 0, 6) | ||
| argsLeaf = append(argsLeaf, "testmetric-leaf", "active", "5ms", 5, 5, 0.9, 0.95, 1.0) | ||
| fLeaf, err := spec.CreateFilter(argsLeaf) |
There was a problem hiding this comment.
I think filter creation in this tests obscures the main test logic. Do we need it here?
There was a problem hiding this comment.
Strictly we don't need it but creating invalid filters is easy to do with so many args, so I think it's better to bail out early.
| reqURL, err := url.Parse(proxy.URL) | ||
| if err != nil { | ||
| t.Fatalf("Failed to parse url %s: %v", proxy.URL, err) | ||
| } |
There was a problem hiding this comment.
Proxy url is always valid, no need to parse it
There was a problem hiding this comment.
True, but also does not hurt either.
I am used to always check the error.
| rt := net.NewTransport(net.Options{}) | ||
| defer rt.Close() |
There was a problem hiding this comment.
Why not proxy.Client() which is https://pkg.go.dev/net/http/httptest#Server.Client
There was a problem hiding this comment.
In this case I want to have a well configured connection pool. In cases which require a significant amount of requests to make sense we should rely on our configuration.
| windowsize: 5, | ||
| minRequests: 10, | ||
| successThreshold: 0.9, | ||
| maxrejectprobability: 0.95, | ||
| exponent: 1.0, | ||
| N: 20, | ||
| pBackendErr: 0.8, | ||
| pExpectedAdmissionShedding: 0.0, |
There was a problem hiding this comment.
I just trust these numbers make sense :)
There was a problem hiding this comment.
I basically copied the last test that has pExpectedAdmissionShedding: 0.91, and for "inactive" I expect pExpectedAdmissionShedding: 0. Same for case "logInactive".
| for i := 0; i < N; i++ { | ||
| r := lim.Reserve() | ||
| for !r.OK() { | ||
| time.Sleep(10 * time.Microsecond) |
There was a problem hiding this comment.
I need to have something that jitters the funcs, because otherwise you get errors from the runtime.
|
👍 |
1 similar comment
|
👍 |
feature: admission control multi layer support
fix: inode admission controllers should not count shedded responses by leaf admission controllers
test: create test that check the leaf admission controllers will not interfere with inode admission controllers in the call tree
test: fix flaky test (works on my machine)