Skip to content

increase file descriptors limits for tests #2340

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

grusev
Copy link
Collaborator

@grusev grusev commented Apr 24, 2025

Reference Issues/PRs

What does this implement or fix?

This is needed for real S3 and GCP tests as file descriptors required are more than initial limit of 100k
Link to S3 run (manual): https://github.com/man-group/ArcticDB/actions/runs/14643182856

A run with 1M file descriptors on Linux: https://github.com/man-group/ArcticDB/actions/runs/15393174400/job/43307772735
No errors because of filedescriptors. Compat testst fail due to fact that GCP symbol names cannot have certain characters. Similar issues exists and this will be added to it and test will be xfailed

A run with 1M filedescriptors + compat test xfail: https://github.com/man-group/ArcticDB/actions/runs/15409225380

After making changes only for GCP: https://github.com/man-group/ArcticDB/actions/runs/15435539023

Any other comments?

Checklist

Checklist for code changes...
  • Have you updated the relevant docstrings, documentation and copyright notice?
  • Is this contribution tested against all ArcticDB's features?
  • Do all exceptions introduced raise appropriate error messages?
  • Are API changes highlighted in the PR description?
  • Is the PR labelled as enhancement or bug so it appears in autogenerated release notes?

@grusev grusev added the patch Small change, should increase patch version label Apr 24, 2025
@grusev grusev changed the title increate file descriptors limits for tests increase file descriptors limits for tests Apr 25, 2025
@grusev grusev marked this pull request as ready for review June 3, 2025 05:24
@poodlewars
Copy link
Collaborator

I really don't think we should do this. Why are we using so many descriptors in the first place?

@grusev
Copy link
Collaborator Author

grusev commented Jun 4, 2025

I really don't think we should do this. Why are we using so many descriptors in the first place?

We have at least 2 sets which cannot run alone on a machine with one pytests executor.
Without this fix run of GCP tests is impossible. So this is a must do at this moment.

I can make the run step specific for GCP only not for others. And if we address at some point the issue with the GCP we can roll it back.

But as of this point not having a fix is same as not being able to run GCP tests in GitHub wich is much worse option

@poodlewars
Copy link
Collaborator

I'd rather someone digs in to the root cause. Concerned about the ulimit complexity hiding there and the added complexity in our CI. GCP is not heavily used - there's no need for this kind of hotfix IMO. Could also hide real problems if we start opening loads of file descriptors but our CI uses unusual settings that can handle them.

Copy link
Collaborator

@poodlewars poodlewars left a comment

Choose a reason for hiding this comment

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

@grusev
Copy link
Collaborator Author

grusev commented Jun 6, 2025

I'd rather someone digs in to the root cause. Concerned about the ulimit complexity hiding there and the added complexity in our CI. GCP is not heavily used - there's no need for this kind of hotfix IMO. Could also hide real problems if we start opening loads of file descriptors but our CI uses unusual settings that can handle them.

We have 2 problems:

Problem 1, to have tests executed in gitbub is done with this PR.

Problem 2 fixing the GCP leak requires first investigation, then depending on findings prioritization. And eventual fix which might come quite late. Or never. In last case agree we do not need to merge even that PR. So I am fine with parking this PR as opened until a fix is implemented, but until then there is no other way to run all tests on GCP in GitHub, even on the laptop this is impossible without patching ulimit of Linux.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Small change, should increase patch version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants