Fix two additional flaky test sources in endtoend tests #11743
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
The logic here is racy since multiple endtoend tests can create the file at around the same time and allocate the same base port. This then leads to flaky failing endtoend tests since not all ports can be used. Even with the listening check later on, this is still racy because the listen check might run at a later point still trying to allocate the same port for the same service in different endtoend test. This copies the Go filelock implementation, but only for unix systems since that's the only supported platform for Vitess. Go has a proposal open to make filelock available, but that is still pending in golang/go#33974. Once that is resolved, we can remove our copy of the implementation. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
Next to the previous commit which fixes the race base port computation, we also need to avoid collisions between ephemeral outgoing ports to 127.0.0.1 and the listening ones. The problem is that we set 1024 as the first ephemeral port, but that means we can collide and trigger flaky tests with all the tests that allocate a server port as well. Given we start allocating server ports now starting at 6700 for endtoend tests and we allocate in blocks of 200, we have room for 75+ concurrent endtoend tests which should totally suffice. We still have a range of 40+ ephemeral ports which should still be sufficient as well. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
dbussink
requested review from
GuptaManan100,
frouioui,
rohit-nayak-ps,
deepthi and
mattlord
as code owners
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
If a new flag is being introduced:
If a workflow is added or modified:
Bug fixes
Non-trivial changes
New/Existing features
Backward compatibility
|
vmg
approved these changes
Nov 16, 2022
frouioui
approved these changes
Nov 16, 2022
mattlord
approved these changes
Nov 16, 2022
deepthi
approved these changes
Nov 16, 2022
notfelineit
pushed a commit
to planetscale/vitess
that referenced
this pull request
Nov 17, 2022
…) (vitessio#1331) Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
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.
Description
This change fixes two more sources of flaky tests in endtoend tests. The first once is that base port allocation for tests is racy and it that it can allocate the same base port for two parallel endtoend tests.
The second one is that we can also collide with ephemeral client ports since we allow those to overlap with the listening ports. This can cause flaky tests as well if a component can't actually be started in endtoend tests.
See also the commit messages for more details.
Related Issue(s)
This is a follow up to #11707 which helped some CI hangs and timeouts but it now shows real underlying issues which could be caused by the two issues this PR fixes.
Checklist