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

Reduce number of concurrent DNS lookups in Machine Tracker #2917

Merged
merged 5 commits into from
May 21, 2024

Conversation

stveit
Copy link
Contributor

@stveit stveit commented May 15, 2024

Fixes #2669

Limits concurrent parallel lookups to 100 at a time.

Tested manually that machine tracker no longer crashes from hitting the file descriptor limit with ulimit set as low as 200 on a search with 4096 results. This would previously crash instantly

The amount of parallel lookups should perhaps be configurable but that can be separate pr if we see the need

A practical change I made is that saving the results from a lookup to the dict self.results will now happen during the parallel lookups, instead of at the end. I made this change simply because using Cooperator changes how the DefferedList we use look like. Instead of all the separate lookup tasks being in the list, the list now contains 100 iterators.
I dont know how to add a callback to the list that will actually be able to go through all the results produced instead of just going over the iterators themselves, so I moved the saving step to be part of the smaller work units.

Errors that were previously raised by themselves are now eaten up by the Cooperator. Tried to fix this by saving all errors then raising the first one that occurred. I expect this keeps the interfacing with the asyncdns module intact, but side effect is that it will only raise the exceptions after its done running, so its slower to fail

@stveit stveit self-assigned this May 15, 2024
@stveit stveit force-pushed the fix/reduce-dns-lookup-parallel-searches branch from 92e6afa to e85b842 Compare May 16, 2024 12:27
stveit added 3 commits May 16, 2024 14:28
It doesnt really do much parsing, the saving part
is the most significant function of it
@stveit stveit force-pushed the fix/reduce-dns-lookup-parallel-searches branch from e85b842 to cf29469 Compare May 16, 2024 12:29
Copy link

codecov bot commented May 16, 2024

Codecov Report

Attention: Patch coverage is 95.83333% with 1 line in your changes missing coverage. Please review.

Project coverage is 59.52%. Comparing base (a2be786) to head (93fca87).
Report is 87 commits behind head on 5.10.x.

Files Patch % Lines
python/nav/asyncdns.py 95.83% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           5.10.x    #2917      +/-   ##
==========================================
- Coverage   60.21%   59.52%   -0.69%     
==========================================
  Files         601      602       +1     
  Lines       43981    43308     -673     
==========================================
- Hits        26481    25780     -701     
- Misses      17500    17528      +28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

github-actions bot commented May 16, 2024

Test results

     11 files       11 suites   11m 8s ⏱️
3 325 tests 3 325 ✔️ 0 💤 0
9 443 runs  9 443 ✔️ 0 💤 0

Results for commit 93fca87.

♻️ This comment has been updated with latest results.

@stveit stveit force-pushed the fix/reduce-dns-lookup-parallel-searches branch 2 times, most recently from c13be7e to 9e1383d Compare May 21, 2024 07:46
@stveit stveit marked this pull request as ready for review May 21, 2024 08:04
@stveit stveit force-pushed the fix/reduce-dns-lookup-parallel-searches branch from 9e1383d to 3d0afbe Compare May 21, 2024 08:07
The reactor will by default log exceptions that occur
in the coiterator, resulting in nothing being raised.
Code using the asyncdns lib except certain errors
to occur when something is wrong so this should keep
the current asyncdns interfacing intact, at the cost of
taking longer to fail
@stveit stveit force-pushed the fix/reduce-dns-lookup-parallel-searches branch from 3d0afbe to 93fca87 Compare May 21, 2024 08:08
@lunkwill42 lunkwill42 changed the title Reduce concurrent DNS lookups Reduce number of concurrent DNS lookups in Machine Tracker May 21, 2024
lunkwill42
lunkwill42 previously approved these changes May 21, 2024
Copy link
Member

@lunkwill42 lunkwill42 left a comment

Choose a reason for hiding this comment

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

This feels like putting lipstick on a pig. It's still ugly, but it works - great work 😆

I have a feeling this will be revisited again at some point in the future (hopefully when we replace Twisted with asyncio)

@lunkwill42 lunkwill42 changed the base branch from master to 5.10.x May 21, 2024 12:57
@lunkwill42 lunkwill42 dismissed their stale review May 21, 2024 12:57

The base branch was changed.

Copy link
Member

@lunkwill42 lunkwill42 left a comment

Choose a reason for hiding this comment

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

This feels like putting lipstick on a pig. It's still ugly, but it works - great work 😆

I have a feeling this will be revisited again at some point in the future (hopefully when we replace Twisted with asyncio)

I changed the base to the new stable branch, which caused my old review to be dismissed - so posting the same again.

@stveit stveit merged commit 5ec7e3f into Uninett:5.10.x May 21, 2024
9 of 12 checks passed
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.

[BUG] Machine Tracker DNS searches eat all available file descriptors and crashes
2 participants