test: De-flake xds server_e2e_test #32004
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Do not return an error from xds server when the context is cancelled, as this is part of normal operation, and we test for this in server_e2e_test. This resolves a test flake: panic: Fail in goroutine after has completed Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
The main test goroutine might be completed before checks on the server goroutine are completed, hence cause the below panic issue. This commit is to defer the streamDone channel close to make sure the error check on the stream server is done before returning from the test. We keep the time check on the wait in the end of each test to not stall the tests in case the stream server fails to exit. Panic error ``` panic: Fail in goroutine after Test/ServerSuite/TestRequestStaleNonce has completed ``` Testing was done as per below: ``` $ go test -count 500 -run Test/ServerSuite/TestRequestStaleNonce ./pkg/envoy/xds/... ok github.com/cilium/cilium/pkg/envoy/xds 250.866s ``` Fixes: cilium#31855 Signed-off-by: Tam Mach <tam.mach@cilium.io> Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
Return io.EOF if test channel was closed, rather than returning a nil request. This mimics the behavior of generated gRPC code, which never returns nil request with a nil error. This resolves a test flake with this error logs: time="2024-04-16T08:46:23+02:00" level=error msg="received nil request from xDS stream; stopping xDS stream handling" subsys=xds xdsClientNode="node0~10.0.0.0~node0~bar" xdsStreamID=1 Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
Speed up tests by eliminating CacheUpdateDelay, as it is generally not needed. When needed, replace with IsCompletedInTimeChecker that waits for upto MaxCompletionDuration before returning, in contrast with IsCompletedChecker that only returns the current state without any wait. This change makes the server_e2e_test tests run >1000x faster. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
TestRequestStaleNonce test code was written with the assumption that no
response would be reveived for a request with a stale nonce, and a second
SendRequest was done right after with the correct nonce value. This
caused two responses to be returned, and the first one could have been
with the old version of the resources.
Remove this duplicate SendRequest.
This resolves test flakes like this:
--- FAIL: Test/ServerSuite/TestRequestStaleNonce (0.00s)
server_e2e_test.go:784:
... response *discoveryv3.DiscoveryResponse = &discoveryv3.DiscoveryResponse{state:impl.MessageState{NoUnkeyedLiterals:pragma.NoUnkeyedLiterals{}, DoNotCompare:pragma.DoNotCompare{}, DoNotCopy:pragma.DoNotCopy{}, atomicMessageInfo:(*impl.MessageInfo)(nil)}, sizeCache:0, unknownFields:[]uint8(nil), VersionInfo:"3", Resources:[]*anypb.Any{(*anypb.Any)(0x40003a63c0), (*anypb.Any)(0x40003a6410)}, Canary:false, TypeUrl:"type.googleapis.com/envoy.config.v3.DummyConfiguration", Nonce:"3", ControlPlane:(*corev3.ControlPlane)(nil)} ("version_info:\"3\" resources:{[type.googleapis.com/envoy.config.route.v3.RouteConfiguration]:{name:\"resource1\"}} resources:{[type.googleapis.com/envoy.config.route.v3.RouteConfiguration]:{name:\"resource0\"}} type_url:\"type.googleapis.com/envoy.config.v3.DummyConfiguration\" nonce:\"3\"")
... VersionInfo string = "4"
... Resources []protoreflect.ProtoMessage = []protoreflect.ProtoMessage{(*routev3.RouteConfiguration)(0xe45380)}
... Canary bool = false
... TypeUrl string = "type.googleapis.com/envoy.config.v3.DummyConfiguration"
Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
Stream timeout is a duration we use in tests to make sure the stream does not stall for too long. In production we do not have such a timeout at all, and in fact the requests are long-lived and responses are only sent when there is something (new) to send. Test stream timeout was 2 seconds, and it would occasionally cause a test flake, especially if debug logging is enabled. This seems to happen due to goroutine scheduling, and for this reason debug logging should not be on for these tests. Bump the test stream timeout to 4 seconds to further reduce the chance of a test flake due to it. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
jrajahalme
added
area/CI
maintainer-s-little-helper
bot
added
dont-merge/needs-release-note-label
jrajahalme
added
release-note/ci
sayboras
reviewed
Apr 16, 2024
sayboras
approved these changes
Apr 16, 2024
|
/test |
|
@sayboras How far you think we should backport this? The code touched here has barely changed, so it should be easy to backport, and recuding flakes in release builds may be valuable as well. |
Yes, xds code is not changed much for a long time, we can backport this till 1.13 imo. |
jrajahalme
added
needs-backport/1.13
maintainer-s-little-helper
bot
added
the
ready-to-merge
giorio94
added
backport-pending/1.13
9 tasks
jschwinger233
added
backport-pending/1.15
5 tasks
jschwinger233
added
backport-pending/1.14
github-actions
bot
added
backport-done/1.13
This was referenced Apr 29, 2024
This was referenced May 10, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
De-flake xds server_e2e_test by:
nilerror from xDS server after context cancelSee commit messages for rationale for each step.
Fixes: #31855