conformance tests for percentage-based request mirroring #3508
Conversation
k8s-ci-robot
added
release-note-none
k8s-ci-robot
added
kind/gep
LiorLieberman
force-pushed
the
percent-based-mirror
branch
3 times, most recently
from
117f34b to
d63225c
Compare
LiorLieberman
changed the title
LiorLieberman
force-pushed
the
percent-based-mirror
branch
from
d63225c to
15db255
Compare
|
@LiorLieberman: GitHub didn't allow me to request PR reviews from the following users: jakebennert. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/cc @dprotaso |
LiorLieberman
force-pushed
the
percent-based-mirror
branch
from
8ed94ce to
aa266b3
Compare
LiorLieberman
force-pushed
the
percent-based-mirror
branch
from
aa266b3 to
63803d9
Compare
|
/cc @kflynn |
|
ping @kflynn @dprotaso |
| Features: []features.FeatureName{ | ||
| features.SupportGateway, | ||
| features.SupportHTTPRoute, | ||
| // features.SupportHTTPRouteRequestPercentageMirror, |
There was a problem hiding this comment.
Forgot to return it, I couldn't find the right command to invoke that will actually perform the test while this feature is added :)
| }, { | ||
| Request: http.Request{ | ||
| Path: "/percent-mirror-and-modify-headers", | ||
| Headers: map[string]string{ | ||
| "X-Header-Remove": "remove-val", | ||
| "X-Header-Add-Append": "append-val-1", | ||
| }, | ||
| }, | ||
| ExpectedRequest: &http.ExpectedRequest{ | ||
| Request: http.Request{ | ||
| Path: "/percent-mirror-and-modify-headers", | ||
| Headers: map[string]string{ | ||
| "X-Header-Add": "header-val-1", | ||
| "X-Header-Add-Append": "append-val-1,header-val-2", | ||
| "X-Header-Set": "set-overwrites-values", | ||
| }, | ||
| }, | ||
| AbsentHeaders: []string{"X-Header-Remove"}, | ||
| }, | ||
| Namespace: ns, | ||
| Backend: "infra-backend-v1", | ||
| MirroredTo: []http.MirroredBackend{ | ||
| { | ||
| BackendRef: http.BackendRef{ | ||
| Name: "infra-backend-v2", | ||
| Namespace: ns, | ||
| }, | ||
| Percent: ptrTo(int32(35)), | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Why are we testing it jointly with the requestHeaderModifier filter? To assess that both filters are properly applied?
There was a problem hiding this comment.
exactly, thats a practice I always used to include in tests I developed. To make sure the feature is not messing up when applied with other standard feature. Does that make sense?
| http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr, expected) | ||
|
|
||
| // Override to not have more requests than expected | ||
| suite.TimeoutConfig.RequiredConsecutiveSuccesses = 1 |
There was a problem hiding this comment.
This change will affect other tests as well. Why do we need to do so?
There was a problem hiding this comment.
oh man.. there is a whole story behind this change.
Essentially since I test mirroring, I started to see more requests that I had actually sent. so the mirror expectations were totally wrong. I learned to find this config the hard way..
Any other suggestion here if this is not desired?
| // used to limit number of parallel go routines | ||
| semaphore := make(chan struct{}, concurrentRequests) | ||
| var wg sync.WaitGroup |
There was a problem hiding this comment.
Why do we need to use concurrent clients here? Correct me If I'm wrong but what we want to check here is:
- send N requests
- check all the requests are received by the backend defined in the BackendRef
- check that n% of the requests are mirrored to the backend set in the mirror filter
Does it accurately summarize what we are trying to do here? If so, to me it's not clear why we do need to implement concurrent clients.
There was a problem hiding this comment.
since totalRequests are a high number of request, we are not sending them sequentially. Instead, we sending them in concurrently, BUT we limit the concurrency factor to be 10 (which is why the sempahore is used for).
Also we wrap the above process in loop - cause mirror distributions are not 100% accurate - so we have enough room to make sure it mirrors as expected (or nearly as expected).
LiorLieberman
force-pushed
the
percent-based-mirror
branch
from
3b693c1 to
d74a070
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: LiorLieberman The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@LiorLieberman: GitHub didn't allow me to request PR reviews from the following users: sayboras. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
What type of PR is this?
/kind test
/area conformance
What this PR does / why we need it:
Conformance tests for #3171 plus small updates to the GEP.
Tested with istio 1.24.1
Note: I also added a test case for fraction but it looks like it has an issue currently with the implementation I used (istio).
The specific test is failing, (which is good) so I think its in a good merge state. I also opened istio/istio#54357.
Mesh tests to follow
Which issue(s) this PR fixes:
Fixes #3171
Does this PR introduce a user-facing change?:
/cc @jakebennert @robscott