refactor: remove sysctl dependency from start command #4068
Conversation
mircea-c
requested review from
rach-id and
ninabarbakadze
and removed request for
a team
📝 WalkthroughWalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
cmd/celestia-appd/cmd/start.go (1)
598-606: Consider platform compatibility and error message clarityWhile the implementation is functionally correct, there are a few considerations:
The hardcoded path
/proc/sys/net/ipv4/tcp_congestion_controlmakes this Linux-specific. Consider:
- Adding a comment to document this limitation
- Adding platform-specific checks for better error messages on non-Linux systems
The error message could be more specific when the file read fails on non-Linux systems.
Consider this improvement:
func checkBBR(command *cobra.Command) error { + // This check is Linux-specific as it relies on /proc filesystem file, err := os.ReadFile("/proc/sys/net/ipv4/tcp_congestion_control") if err != nil { fmt.Print(warning) - return fmt.Errorf("failed to read file '/proc/sys/net/ipv4/tcp_congestion_control' %w", err) + if os.IsNotExist(err) { + return fmt.Errorf("BBR check is only supported on Linux systems: %w", err) + } + return fmt.Errorf("failed to read TCP congestion control settings: %w", err) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
cmd/celestia-appd/cmd/start.go(2 hunks)
🔇 Additional comments (1)
cmd/celestia-appd/cmd/start.go (1)
582-583: LGTM: Documentation update is clear and consistent
The addition of the alternative verification method using cat /proc/sys/net/ipv4/tcp_congestion_control provides users with a distribution-agnostic way to verify BBR status.
celestia-bot
requested review from
a team and
rootulp
and removed request for
a team
evan-forbes
added
backport:v3.x
|
Congrats, your important contribution to this open-source project has earned you a GitPOAP! GitPOAP: 2024 Celestia Contributor: Head to gitpoap.io & connect your GitHub account to mint! Learn more about GitPOAPs here. |
<!-- Please read and fill out this form before submitting your PR. Please make sure you have reviewed our contributors guide before submitting your first PR. --> ## Overview <!-- Please provide an explanation of the PR, including the appropriate context, background, goal, and rationale. If there is an issue with this information, please provide a tl;dr and link the issue. --> This change was prompted by an attempt to run the application in an unprivileged docker container based on a slim image. The absence of the `sysctl` command causes the application to fail the BBR check, even though that module is loaded and available. The goals of this PR are: - allow the application to run the BBR check in a restricted environment such as a docker container - make the application distribution agnostic by removing the `systemd` dependency - remove an instance of process forking with `exec` (cherry picked from commit e52234f)
#4072) ## Overview This change was prompted by an attempt to run the application in an unprivileged docker container based on a slim image. The absence of the `sysctl` command causes the application to fail the BBR check, even though that module is loaded and available. The goals of this PR are: - allow the application to run the BBR check in a restricted environment such as a docker container - make the application distribution agnostic by removing the `systemd` dependency - remove an instance of process forking with `exec` <hr>This is an automatic backport of pull request #4068 done by [Mergify](https://mergify.com). Co-authored-by: Mircea Colonescu <[email protected]>
Overview
This change was prompted by an attempt to run the application in an unprivileged docker container based on a slim image. The absence of the
sysctlcommand causes the application to fail the BBR check, even though that module is loaded and available.The goals of this PR are:
systemddependencyexec