Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remedy connection check timeouts in test_bridge #5442

Merged
merged 2 commits into from
Nov 17, 2023

Conversation

MarkusPettersson98
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 commented Nov 15, 2023

Delegate the task of picking entry & exit nodes to the relay selector in test_bridge because of latency concerns.

We have seen test_bridge fail due to high latency if slow + far-away servers were
selected, even with a generous timeout of 10 seconds.


This change is Reviewable

@MarkusPettersson98 MarkusPettersson98 force-pushed the fix/test_bridge branch 2 times, most recently from c431762 to da3b703 Compare November 16, 2023 10:59
Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 5 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98)


test/test-manager/src/tests/helpers.rs line 494 at r1 (raw file):

        .try_into()?;

    Ok(relay_list.relays().cloned().filter(criteria).collect())

Nit: Could the relays be cloned after filtering?

Copy link
Contributor Author

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 4 of 5 files reviewed, all discussions resolved (waiting on @dlon)


test/test-manager/src/tests/helpers.rs line 494 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Nit: Could the relays be cloned after filtering?

Done

Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Prefer types from `mullvad-types` over equivavelent types defined in
`mullvad-management-interface`.
We have seen `test_bridge` fail due to high latency if slow + far-away
servers were selected. Hopefully delegating the task of picking
appropriate entry & exit relays will help mitigate this.
@MarkusPettersson98 MarkusPettersson98 merged commit 0a82036 into main Nov 17, 2023
29 checks passed
@MarkusPettersson98 MarkusPettersson98 deleted the fix/test_bridge branch November 17, 2023 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants