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

Set max open rocksdb files to 8192 #2557

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

vicsn
Copy link
Contributor

@vicsn vicsn commented Oct 15, 2024

Motivation

rocksdb is creating and leaving open hundreds of files. While at the moment this is harmless, if it keeps growing in the future it could cause nodes to halt.
It is unknown what will happen once the max system-wide limit is hit.
To prevent that from happening, this PR sets a conservative limit.

Test Plan

Has been tested on a mainnet client. lsof outputs imply the node stayed below the limits anyway:

  • Initial values: 120k total snarkos file descriptors, 1047 unique
  • After a restart: 110k total snarkos file descriptors, 1049 unique
  • After a restart with this PR: 20k total snarkos file descriptors, 159 unique
  • After a resync with this PR: 80k total snarkos file descriptors, 781 unique

@onetrickwolf
Copy link

@vicsn should be able to test this today at some point.

@vicsn
Copy link
Contributor Author

vicsn commented Oct 16, 2024

@onetrickwolf amazing. Apologies one more extended question: at the end, with this new version, could you also remove the client's ledger, let it resync, and share the resulting lsof file after its finished syncing? Resyncing might trigger the creation of the file descriptors faster.

@onetrickwolf
Copy link

@vicsn yeah sure this will probably be a day or two until we are fully synced though.

@zosorock zosorock requested review from zkxuerb, zklimaleo and a team October 18, 2024 22:15
Copy link
Collaborator

@zkxuerb zkxuerb left a comment

Choose a reason for hiding this comment

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

LGTM

@vicsn
Copy link
Contributor Author

vicsn commented Oct 21, 2024

@vicsn yeah sure this will probably be a day or two until we are fully synced though.

Note to reviewers: test ran succesfully. It seems the node stayed far below this set limit anyway, so its not urgent, but useful defense in depth. I added data in the PR description above.

@zosorock zosorock merged commit dea322b into AleoNet:staging Oct 22, 2024
76 of 77 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.

6 participants