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

Added ipv8 walk_interval scaling #5581

Merged
merged 3 commits into from
Sep 28, 2020
Merged

Conversation

qstokkink
Copy link
Contributor

Fixes #5384

This PR addresses scaling back IPv8's bandwidth and CPU use while the main thread is congested. Concretely, this PR makes the following changes:

  • Adds a configuration option for enabling IPv8 auto scaling (enabled by default).
  • Adds a configuration option for the IPv8 walk_interval.
  • Adds a configuration option for the upper limit the walk_interval may grow to, if scaling is enabled.
  • Adds the IPv8Monitor class (and tests), to measure the main thread choking and scale the walk_interval accordingly.
  • Adds the IPv8Monitor to the Session IPv8 initialization.
  • Updates the IPv8 initialization in Session to use the new ConfigBuilder.
  • Updates the IPv8 health debug screen to show the current target walk_interval.
  • Updates the IPv8 pointer (fixes a bug where inspecting the debug IPv8 health interferes with this code).

@qstokkink

This comment has been minimized.

@devos50
Copy link
Contributor

devos50 commented Sep 25, 2020

Unfortunately, these tests are still flaky :/

@devos50
Copy link
Contributor

devos50 commented Sep 25, 2020

retest this please

@qstokkink

This comment has been minimized.

@devos50
Copy link
Contributor

devos50 commented Sep 25, 2020

I suspect that last one is caused by having multiple test instances running at the same time; I didn't test that thoroughly yet.

@devos50
Copy link
Contributor

devos50 commented Sep 25, 2020

retest this please

@qstokkink
Copy link
Contributor Author

Alright, I'm done with this PR. @devos50 do you want to use this PR to do flaky test hunting?

@qstokkink qstokkink marked this pull request as ready for review September 25, 2020 12:33
@qstokkink qstokkink changed the title WIP: Added ipv8 walk_interval scaling READY: Added ipv8 walk_interval scaling Sep 25, 2020
@devos50
Copy link
Contributor

devos50 commented Sep 25, 2020

I'll do that in another PR.

@qstokkink
Copy link
Contributor Author

@devos50 https://jenkins-ci.tribler.org/job/GH_Tribler_PR_Tests/job/PR_win64_pytest/21/consoleFull

Is that due to [gw5] [ 99%] FAILED src/tribler-core/tribler_core/modules/tunnel/test_full_session/test_tunnel_community.py::test_anon_download ?

@devos50
Copy link
Contributor

devos50 commented Sep 25, 2020

Crap, there seems to be a critical bug in flaky: box/flaky#166 😢

@devos50
Copy link
Contributor

devos50 commented Sep 25, 2020

These tests are really driving me crazy. At this point, I'm totally fine with disabling them... @qstokkink your thoughts?

@qstokkink
Copy link
Contributor Author

qstokkink commented Sep 25, 2020

@devos50 🤷 "If you can't fix it, skip it." Though, of course, it would be better if these tests were fixed.

@devos50
Copy link
Contributor

devos50 commented Sep 25, 2020

Well, let me first revert the flaky stuff

@devos50
Copy link
Contributor

devos50 commented Sep 25, 2020

Ok, it seems that the pytest-rerunfailures works. I'll use that instead 👍

@qstokkink
Copy link
Contributor Author

@devos50 cool, tell me when to rebase 👍

@devos50
Copy link
Contributor

devos50 commented Sep 25, 2020

Noooooooo pytest-rerunfailures does not play nicely with pytest-timeout (see here) 😢

@qstokkink
Copy link
Contributor Author

So.. back to skipping?

@qstokkink
Copy link
Contributor Author

To be fair, if anything, it seems like test_anon_download is now consistently failing 😄

@devos50
Copy link
Contributor

devos50 commented Sep 25, 2020

I won't have time to look into it further today. It also seems that getting logging output from pytest-xdist is a hell. I didn't foresee these issues.

For now, I finished #5584 so that should be ready for merge. Please rebase then and try to get the tests passing I guess?

I look at it later.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.5% 0.5% Duplication

@qstokkink
Copy link
Contributor Author

The gods of testing have smiled upon me: none of the unrelated tests have failed during this testing run. 🙏

This PR is once again ready for review.

Copy link
Contributor

@devos50 devos50 left a comment

Choose a reason for hiding this comment

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

Looks good. Not sure how it would behave in the wild but there's only one way to find out 👍

@qstokkink
Copy link
Contributor Author

Looks good. Not sure how it would behave in the wild but there's only one way to find out 👍

I have spent quite some hours running this and tweaking it.. but, should worst come to worst, we can disable this by default in the config.

@qstokkink qstokkink changed the title READY: Added ipv8 walk_interval scaling Added ipv8 walk_interval scaling Sep 28, 2020
@qstokkink qstokkink merged commit 1900ab7 into Tribler:devel Sep 28, 2020
@qstokkink qstokkink deleted the add_ipv8_scaling branch September 28, 2020 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Scale IPv8 walk_interval
2 participants