feat: increase LimitNOFILE for both celestia-app and celestia-node #1407
Conversation
jcstein
requested review from
a team,
Wondertan,
cmwaters,
distractedm1nd,
evan-forbes and
smuu
and removed request for
a team
|
WalkthroughThe update to the systemd configuration file involves increasing the Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- nodes/systemd.md (4 hunks)
Additional comments: 4
nodes/systemd.md (4)
- 33-33: The
LimitNOFILEvalue forcelestia-appdhas been increased to 16384. This change aligns with the PR objectives but consider verifying if this value suffices for all operational scenarios ofcelestia-appd. It might be lower than required for high-load environments.- 91-91: The
LimitNOFILEvalue for the Celestia full storage node has been set to 1400000. This substantial increase directly addresses the requirements of badgerdb for production environments. Ensure this value does not exceed system limits or require additional configuration to be supported on all target deployment systems.- 129-129: The
LimitNOFILEvalue for the Celestia bridge node is also set to 1400000. Similar to the full storage node, confirm that this value is within acceptable system limits and that users are guided on any necessary system configurations to support such a high limit.- 185-185: For the Celestia light node, the
LimitNOFILEvalue is again set to 1400000. As with previous comments, ensure this setting is appropriate for light nodes, considering their expected workload and resource usage might differ from full nodes or bridge nodes.
|
Celestia light node shouldn't need that high of a limit though |
|
Does it use badgerdb? Badegrdb suggests "a soft limit of 1048576" |
|
I think we should remove this configuration from LN. The default hard limit in systemd is high enough(524288) for LN and since go1.20, the runtime gets up to the hard limit automatically. |
|
@jcstein, LN uses badgerdb, but note that the shared link about limits is from Dgraph database which uses badgerdb underneath, so it's not for the badger itself. Badger needs less then that by default. In case of FN/BN where we have multiple instances of badger and CAR files IO we definitely need more than badger suggests and the proposed value makes total sense. |
i'm not a db expert, but i'm nearly positive this is badgerDBs official documentation? and the google results, for example, only show other things which don't mention "db" |
I'll suggest this on the pr |
| Restart=on-failure | ||
| RestartSec=3 | ||
| LimitNOFILE=4096 | ||
| LimitNOFILE=65535 |
There was a problem hiding this comment.
👍 LGTM b/c this is what POPS uses.
@jcstein, the link in the description is about dgraph, not about Badger. From the link:
The screenshots are from Badger, and they are on the same website as Dgraph, but there are no guidelines on file limits for Badger in particular. |
Overview
Resolves #1390. An issue was been reported cites the minimum required by badger is 1million+ and their bridge node requires 1330736 files open in badgerDB, but we currently have documented 4096, 65536, 65536 for light/full/bridge and 4096 documented for celestia-app.
This PR updates the values to ones suggested in the issue or in slack.
This needs review from someone on these teams, as it impacts suggested parameter changes relevant to all of the teams:
Reference
From badgerdb's documentation:
Checklist
Summary by CodeRabbit
Summary by CodeRabbit