Don't listen on :8448 for the Complement federation server #289
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
Instead, listen on a random OS-allocated high numbered port then do a switcheroo on the `ServerName` so it reads correctly e.g `host.docker.internal:56185`. This means the server name will be invalid if it is read before `Server.Listen()` is called so we now guard common access points in `Server` which rely on the server name and fail tests if the server is not yet listening when those functions are called. To further guard against misuse of server name whilst it isn't valid, turn it into a private field.
neilalexander
approved these changes
Jan 21, 2022
richvdh
reviewed
Jan 24, 2022
We don't need it anymore as CI does not run Complement inside Docker
kegsay
added a commit
to matrix-org/synapse
that referenced
this pull request
Jan 24, 2022
This requires matrix-org/complement#289 We now run Complement on the VM instead of inside a Docker container. This is to allow Complement to bind to any high-numbered port when it starts up its own federation servers. We want to do this to allow for more concurrency when running complement tests. Previously, Complement only ever bound to `:8448` when running its own federation server. This prevented multiple federation tests running at the same time as they would fight each other on the port. This did however allow Complement to run in Docker, as the host could just port forward `:8448` to allow homeserver containers to communicate to Complement. Now that we are using random ports however, we cannot use Docker to run Complement. This ends up being a good thing because: - Running Complement tests locally is closer to how they run in CI. - Allows the `CI` env var to be removed in Complement. - Slightly speeds up runs as we don't need to pull down the Complement image prior to running tests. This assumes GHA caches actions sensibly.
4 tasks
kegsay
added a commit
to matrix-org/synapse
that referenced
this pull request
Jan 25, 2022
* CI: run Complement on the VM, not inside Docker This requires matrix-org/complement#289 We now run Complement on the VM instead of inside a Docker container. This is to allow Complement to bind to any high-numbered port when it starts up its own federation servers. We want to do this to allow for more concurrency when running complement tests. Previously, Complement only ever bound to `:8448` when running its own federation server. This prevented multiple federation tests running at the same time as they would fight each other on the port. This did however allow Complement to run in Docker, as the host could just port forward `:8448` to allow homeserver containers to communicate to Complement. Now that we are using random ports however, we cannot use Docker to run Complement. This ends up being a good thing because: - Running Complement tests locally is closer to how they run in CI. - Allows the `CI` env var to be removed in Complement. - Slightly speeds up runs as we don't need to pull down the Complement image prior to running tests. This assumes GHA caches actions sensibly. * Changelog * Full stop * Update .github/workflows/tests.yml Co-authored-by: Richard van der Hoff <[email protected]> * Review comments * Update .github/workflows/tests.yml Co-authored-by: Richard van der Hoff <[email protected]> Co-authored-by: Richard van der Hoff <[email protected]>
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.
Instead, listen on a random OS-allocated high numbered port then
do a switcheroo on the
ServerNameso it reads correctly e.ghost.docker.internal:56185. This means the server name will beinvalid if it is read before
Server.Listen()is called so we nowguard common access points in
Serverwhich rely on the servername and fail tests if the server is not yet listening when those
functions are called. To further guard against misuse of server
name whilst it isn't valid, turn it into a private field.
As of this PR, it will now (in theory) be possible to scale out tests indefinitely as we no longer have any central point/resource which all tests rely on. This should make it possible to split
/testsinto more packages which make sense from a Matrix PoV.Indirectly fixes #60