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

test: Fix too many files open errors #503

Merged
merged 1 commit into from
Nov 3, 2022
Merged

Conversation

msbrogli
Copy link
Member

@msbrogli msbrogli commented Nov 3, 2022

Acceptance Criteria

  1. Be able to run tests without increasing the maximum number of open files.
  2. Properly close the file descriptor after it's been used.

@msbrogli msbrogli added the tests label Nov 3, 2022
@msbrogli msbrogli self-assigned this Nov 3, 2022
@msbrogli msbrogli requested a review from jansegre as a code owner November 3, 2022 08:17
@msbrogli msbrogli requested a review from luislhl November 3, 2022 08:17
@codecov
Copy link

codecov bot commented Nov 3, 2022

Codecov Report

Merging #503 (60b0f33) into dev (7cba651) will increase coverage by 0.11%.
The diff coverage is n/a.

❗ Current head 60b0f33 differs from pull request most recent head d80cb63. Consider uploading reports for the commit d80cb63 to get more accurate results

@@            Coverage Diff             @@
##              dev     #503      +/-   ##
==========================================
+ Coverage   83.45%   83.56%   +0.11%     
==========================================
  Files         188      187       -1     
  Lines       17083    16829     -254     
  Branches     2642     2590      -52     
==========================================
- Hits        14256    14063     -193     
+ Misses       2339     2301      -38     
+ Partials      488      465      -23     
Impacted Files Coverage Δ
hathor/prometheus.py 86.20% <0.00%> (-3.45%) ⬇️
hathor/debug_resources.py 56.12% <0.00%> (-3.07%) ⬇️
hathor/exception.py 100.00% <0.00%> (ø)
hathor/builder.py

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@msbrogli msbrogli force-pushed the test/fix-too-many-files-open branch from 2caf6de to 60b0f33 Compare November 3, 2022 13:59
@luislhl
Copy link
Collaborator

luislhl commented Nov 3, 2022

The error we are trying to fix happened only when running tests? Do we have more info about it?

@jansegre
Copy link
Member

jansegre commented Nov 3, 2022

The error we are trying to fix happened only when running tests? Do we have more info about it?

It definitely happens on my machine:

❯ uname -a
Darwin mewtwo.local 22.1.0 Darwin Kernel Version 22.1.0: Sun Oct  9 20:15:09 PDT 2022; root:xnu-8792.41.9~2/RELEASE_ARM64_T6000 arm64

Which by default has a low ulimit:

❯ ulimit -n
256

I can't set my ulimit to 1000 or higher:

❯ ulimit -n 10000
ulimit: Permission denied when changing resource of type 'Maximum number of open file descriptors'

But I can set it to 9999:

❯ ulimit -n 9999

I can't complete a large suit of tests (like make tests-lib or pytest tests/tx/) without it starting to fail nearly every test after a certain point, and eventually I get to see that all the errors are from "too many open files" when trying to open a rocksdb database.

Since this was only on my machine and not on any of the test environments I never bothered too much, but I guess this started happening for Marcelo too.

I'll test if this fix works for me, maybe it is possible to setup the CI to similar conditions (like lowering the ulimit), but I'm not sure if it's worth it. It might be good because it's a sign that we're cleaning up correctly, but most usually it is something test-environment related and dev-system specific.

@msbrogli msbrogli force-pushed the test/fix-too-many-files-open branch from 60b0f33 to dc47da9 Compare November 3, 2022 18:50
@msbrogli msbrogli force-pushed the test/fix-too-many-files-open branch from dc47da9 to 1c6b7a0 Compare November 3, 2022 19:24
@msbrogli msbrogli force-pushed the test/fix-too-many-files-open branch from 1c6b7a0 to d80cb63 Compare November 3, 2022 19:43
@msbrogli msbrogli merged commit 3ae3ed4 into dev Nov 3, 2022
@msbrogli msbrogli deleted the test/fix-too-many-files-open branch November 3, 2022 20:00
@jansegre jansegre mentioned this pull request Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants