feat: add command to enable mptcp #4014
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
evan-forbes
added
the
WS: Maintenance 🔧
evan-forbes
requested review from
rootulp and
rach-id
and removed request for
a team
evan-forbes
added
the
backport:v2.x
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce two new make targets in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Makefile
participant System
User->>Makefile: invoke enable-mptcp
Makefile->>System: Set sysctl parameters for MPTCP
Makefile->>System: Append settings to /etc/sysctl.conf
User->>Makefile: invoke disable-mptcp
Makefile->>System: Set MPTCP enabled parameter to 0
Makefile->>System: Remove settings from /etc/sysctl.conf
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: 2
🧹 Outside diff range and nitpick comments (1)
Makefile (1)
276-302: Document MPTCP implications and provide usage guidelines
The MPTCP configuration affects system-wide network settings, which could impact other applications. Consider adding documentation that:
- Explains when and why users should enable MPTCP
- Describes the performance implications
- Lists potential conflicts with other network configurations
- Provides troubleshooting steps
Would you like me to help draft this documentation as a new section in the README?
Makefile
Outdated
Comment on lines
290
to
301
| ## disable-mptcp: Disables mptcp over multiple ports. Only works on Linux Kernel 5.6 and above. | ||
| disable_mptcp: | ||
| @echo "Disabling MPTCP..." | ||
| @sudo sysctl -w net.mptcp.enabled=0 | ||
| @sudo sysctl -w net.mptcp.mptcp_path_manager=default | ||
| @echo "Removing MPTCP settings from /etc/sysctl.conf..." | ||
| @sudo sed -i '/net.mptcp.enabled=1/d' /etc/sysctl.conf | ||
| @sudo sed -i '/net.mptcp.mptcp_path_manager=ndiffports/d' /etc/sysctl.conf | ||
| @sudo sed -i '/net.mptcp.mptcp_ndiffports=16/d' /etc/sysctl.conf | ||
| @echo "MPTCP configuration reverted!" | ||
|
|
||
| .PHONY: disable-mptcp |
There was a problem hiding this comment.
Fix target naming and add safety measures for disable-mptcp target
The current implementation has several issues:
- Target name uses underscore (
disable_mptcp) whileenable-mptcpuses hyphen - Unsafe modification of system files without backup
- Missing error handling
Apply these fixes:
## disable-mptcp: Disables mptcp over multiple ports. Only works on Linux Kernel 5.6 and above.
-disable_mptcp:
+disable-mptcp:
+ @if [ "$$(uname)" != "Linux" ]; then \
+ echo "Error: MPTCP configuration is only supported on Linux systems"; \
+ exit 1; \
+ fi
+ @if ! command -v sudo >/dev/null 2>&1; then \
+ echo "Error: sudo is required to modify system settings"; \
+ exit 1; \
+ fi
@echo "Disabling MPTCP..."
+ @sudo cp /etc/sysctl.conf /etc/sysctl.conf.backup
@sudo sysctl -w net.mptcp.enabled=0
@sudo sysctl -w net.mptcp.mptcp_path_manager=default
@echo "Removing MPTCP settings from /etc/sysctl.conf..."
@sudo sed -i '/net.mptcp.enabled=1/d' /etc/sysctl.conf
@sudo sed -i '/net.mptcp.mptcp_path_manager=ndiffports/d' /etc/sysctl.conf
@sudo sed -i '/net.mptcp.mptcp_ndiffports=16/d' /etc/sysctl.conf
@echo "MPTCP configuration reverted!"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ## disable-mptcp: Disables mptcp over multiple ports. Only works on Linux Kernel 5.6 and above. | |
| disable_mptcp: | |
| @echo "Disabling MPTCP..." | |
| @sudo sysctl -w net.mptcp.enabled=0 | |
| @sudo sysctl -w net.mptcp.mptcp_path_manager=default | |
| @echo "Removing MPTCP settings from /etc/sysctl.conf..." | |
| @sudo sed -i '/net.mptcp.enabled=1/d' /etc/sysctl.conf | |
| @sudo sed -i '/net.mptcp.mptcp_path_manager=ndiffports/d' /etc/sysctl.conf | |
| @sudo sed -i '/net.mptcp.mptcp_ndiffports=16/d' /etc/sysctl.conf | |
| @echo "MPTCP configuration reverted!" | |
| .PHONY: disable-mptcp | |
| ## disable-mptcp: Disables mptcp over multiple ports. Only works on Linux Kernel 5.6 and above. | |
| disable-mptcp: | |
| @if [ "$$(uname)" != "Linux" ]; then \ | |
| echo "Error: MPTCP configuration is only supported on Linux systems"; \ | |
| exit 1; \ | |
| fi | |
| @if ! command -v sudo >/dev/null 2>&1; then \ | |
| echo "Error: sudo is required to modify system settings"; \ | |
| exit 1; \ | |
| fi | |
| @echo "Disabling MPTCP..." | |
| @sudo cp /etc/sysctl.conf /etc/sysctl.conf.backup | |
| @sudo sysctl -w net.mptcp.enabled=0 | |
| @sudo sysctl -w net.mptcp.mptcp_path_manager=default | |
| @echo "Removing MPTCP settings from /etc/sysctl.conf..." | |
| @sudo sed -i '/net.mptcp.enabled=1/d' /etc/sysctl.conf | |
| @sudo sed -i '/net.mptcp.mptcp_path_manager=ndiffports/d' /etc/sysctl.conf | |
| @sudo sed -i '/net.mptcp.mptcp_ndiffports=16/d' /etc/sysctl.conf | |
| @echo "MPTCP configuration reverted!" | |
| .PHONY: disable-mptcp |
Comment on lines
+276
to
+288
| ## enable-mptcp: Enable mptcp over multiple ports (not interfaces). Only works on Linux Kernel 5.6 and above. | ||
| enable-mptcp: | ||
| @echo "Configuring system to use mptcp..." | ||
| @sudo sysctl -w net.mptcp.enabled=1 | ||
| @sudo sysctl -w net.mptcp.mptcp_path_manager=ndiffports | ||
| @sudo sysctl -w net.mptcp.mptcp_ndiffports=16 | ||
| @echo "Making MPTCP settings persistent across reboots..." | ||
| @echo "net.mptcp.enabled=1" | sudo tee -a /etc/sysctl.conf | ||
| @echo "net.mptcp.mptcp_path_manager=ndiffports" | sudo tee -a /etc/sysctl.conf | ||
| @echo "net.mptcp.mptcp_ndiffports=16" | sudo tee -a /etc/sysctl.conf | ||
| @echo "MPTCP configuration complete and persistent!" | ||
|
|
||
| .PHONY: enable-mptcp |
There was a problem hiding this comment.
Add safety checks and improve error handling for enable-mptcp target
The current implementation has several potential issues:
- Missing kernel version check despite requiring Linux 5.6+
- No error handling for sudo operations
- No validation that MPTCP is supported
- Direct modification of system files without backup
Consider implementing these safety improvements:
## enable-mptcp: Enable mptcp over multiple ports (not interfaces). Only works on Linux Kernel 5.6 and above.
enable-mptcp:
+ @if [ "$$(uname)" != "Linux" ]; then \
+ echo "Error: MPTCP configuration is only supported on Linux systems"; \
+ exit 1; \
+ fi
+ @if [ "$$(uname -r | cut -d. -f1,2)" \< "5.6" ]; then \
+ echo "Error: MPTCP requires Linux kernel 5.6 or higher"; \
+ exit 1; \
+ fi
+ @if ! command -v sudo >/dev/null 2>&1; then \
+ echo "Error: sudo is required to modify system settings"; \
+ exit 1; \
+ fi
@echo "Configuring system to use mptcp..."
+ @sudo cp /etc/sysctl.conf /etc/sysctl.conf.backup
@sudo sysctl -w net.mptcp.enabled=1
@sudo sysctl -w net.mptcp.mptcp_path_manager=ndiffports
@sudo sysctl -w net.mptcp.mptcp_ndiffports=16
@echo "Making MPTCP settings persistent across reboots..."
@echo "net.mptcp.enabled=1" | sudo tee -a /etc/sysctl.conf
@echo "net.mptcp.mptcp_path_manager=ndiffports" | sudo tee -a /etc/sysctl.conf
@echo "net.mptcp.mptcp_ndiffports=16" | sudo tee -a /etc/sysctl.conf
@echo "MPTCP configuration complete and persistent!"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ## enable-mptcp: Enable mptcp over multiple ports (not interfaces). Only works on Linux Kernel 5.6 and above. | |
| enable-mptcp: | |
| @echo "Configuring system to use mptcp..." | |
| @sudo sysctl -w net.mptcp.enabled=1 | |
| @sudo sysctl -w net.mptcp.mptcp_path_manager=ndiffports | |
| @sudo sysctl -w net.mptcp.mptcp_ndiffports=16 | |
| @echo "Making MPTCP settings persistent across reboots..." | |
| @echo "net.mptcp.enabled=1" | sudo tee -a /etc/sysctl.conf | |
| @echo "net.mptcp.mptcp_path_manager=ndiffports" | sudo tee -a /etc/sysctl.conf | |
| @echo "net.mptcp.mptcp_ndiffports=16" | sudo tee -a /etc/sysctl.conf | |
| @echo "MPTCP configuration complete and persistent!" | |
| .PHONY: enable-mptcp | |
| ## enable-mptcp: Enable mptcp over multiple ports (not interfaces). Only works on Linux Kernel 5.6 and above. | |
| enable-mptcp: | |
| @if [ "$$(uname)" != "Linux" ]; then \ | |
| echo "Error: MPTCP configuration is only supported on Linux systems"; \ | |
| exit 1; \ | |
| fi | |
| @if [ "$$(uname -r | cut -d. -f1,2)" \< "5.6" ]; then \ | |
| echo "Error: MPTCP requires Linux kernel 5.6 or higher"; \ | |
| exit 1; \ | |
| fi | |
| @if ! command -v sudo >/dev/null 2>&1; then \ | |
| echo "Error: sudo is required to modify system settings"; \ | |
| exit 1; \ | |
| fi | |
| @echo "Configuring system to use mptcp..." | |
| @sudo cp /etc/sysctl.conf /etc/sysctl.conf.backup | |
| @sudo sysctl -w net.mptcp.enabled=1 | |
| @sudo sysctl -w net.mptcp.mptcp_path_manager=ndiffports | |
| @sudo sysctl -w net.mptcp.mptcp_ndiffports=16 | |
| @echo "Making MPTCP settings persistent across reboots..." | |
| @echo "net.mptcp.enabled=1" | sudo tee -a /etc/sysctl.conf | |
| @echo "net.mptcp.mptcp_path_manager=ndiffports" | sudo tee -a /etc/sysctl.conf | |
| @echo "net.mptcp.mptcp_ndiffports=16" | sudo tee -a /etc/sysctl.conf | |
| @echo "MPTCP configuration complete and persistent!" | |
| .PHONY: enable-mptcp |
There was a problem hiding this comment.
these seem to be interesting suggestions, but optional
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
Makefile (1)
276-301: Add documentation about operational impactConsider adding documentation in the target descriptions about:
- Impact on running services
- Whether a system restart is required
- How to verify MPTCP status after enabling/disabling
- Prerequisites (root access, kernel modules, etc.)
Update the target descriptions:
-## enable-mptcp: Enable mptcp over multiple ports (not interfaces). Only works on Linux Kernel 5.6 and above. +## enable-mptcp: Enable mptcp over multiple ports (not interfaces). Requires Linux Kernel 5.6+, root access, and may require system restart. +## Note: This will modify system-wide network settings and may affect running services. +## Verify status after enabling: sysctl net.mptcp.enabled -## disable-mptcp: Disables mptcp over multiple ports. Only works on Linux Kernel 5.6 and above. +## disable-mptcp: Disable mptcp over multiple ports. Requires Linux Kernel 5.6+ and root access. +## Note: This will modify system-wide network settings and may affect running services. +## Verify status after disabling: sysctl net.mptcp.enabled
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
Makefile(1 hunks)
🔇 Additional comments (2)
Makefile (2)
276-288:
Add safety checks and improve error handling
The implementation still needs the following safety measures:
- Verify Linux OS and kernel version 5.6+
- Check sudo availability
- Backup system files before modification
- Add error handling for sysctl operations
The previous review comment's suggested fixes are still valid and should be implemented:
enable-mptcp:
+ @if [ "$$(uname)" != "Linux" ]; then \
+ echo "Error: MPTCP configuration is only supported on Linux systems"; \
+ exit 1; \
+ fi
+ @if [ "$$(uname -r | cut -d. -f1,2)" \< "5.6" ]; then \
+ echo "Error: MPTCP requires Linux kernel 5.6 or higher"; \
+ exit 1; \
+ fi
+ @if ! command -v sudo >/dev/null 2>&1; then \
+ echo "Error: sudo is required to modify system settings"; \
+ exit 1; \
+ fi
@echo "Configuring system to use mptcp..."
+ @sudo cp /etc/sysctl.conf /etc/sysctl.conf.backup290-301:
Add safety measures and error handling
The implementation still needs the following safety measures:
- Verify Linux OS
- Check sudo availability
- Backup system files before modification
- Add error handling for system operations
The previous review comment's suggested fixes are still valid and should be implemented:
disable-mptcp:
+ @if [ "$$(uname)" != "Linux" ]; then \
+ echo "Error: MPTCP configuration is only supported on Linux systems"; \
+ exit 1; \
+ fi
+ @if ! command -v sudo >/dev/null 2>&1; then \
+ echo "Error: sudo is required to modify system settings"; \
+ exit 1; \
+ fi
@echo "Disabling MPTCP..."
+ @sudo cp /etc/sysctl.conf /etc/sysctl.conf.backup
rach-id
approved these changes
Oct 30, 2024
| ## debug-version: Print the git tag and version. | ||
| debug-version: | ||
| @echo "GIT_TAG: $(GIT_TAG)" | ||
| @echo "VERSION: $(VERSION)" | ||
| .PHONY: debug-version | ||
|
|
| .PHONY: enable-mptcp | ||
|
|
||
| ## disable-mptcp: Disables mptcp over multiple ports. Only works on Linux Kernel 5.6 and above. | ||
| disable-mptcp: |
There was a problem hiding this comment.
would be nice to have a disable-bbr too :D
Comment on lines
+276
to
+288
| ## enable-mptcp: Enable mptcp over multiple ports (not interfaces). Only works on Linux Kernel 5.6 and above. | ||
| enable-mptcp: | ||
| @echo "Configuring system to use mptcp..." | ||
| @sudo sysctl -w net.mptcp.enabled=1 | ||
| @sudo sysctl -w net.mptcp.mptcp_path_manager=ndiffports | ||
| @sudo sysctl -w net.mptcp.mptcp_ndiffports=16 | ||
| @echo "Making MPTCP settings persistent across reboots..." | ||
| @echo "net.mptcp.enabled=1" | sudo tee -a /etc/sysctl.conf | ||
| @echo "net.mptcp.mptcp_path_manager=ndiffports" | sudo tee -a /etc/sysctl.conf | ||
| @echo "net.mptcp.mptcp_ndiffports=16" | sudo tee -a /etc/sysctl.conf | ||
| @echo "MPTCP configuration complete and persistent!" | ||
|
|
||
| .PHONY: enable-mptcp |
There was a problem hiding this comment.
these seem to be interesting suggestions, but optional
mergify bot
pushed a commit
that referenced
this pull request
Nov 1, 2024
## Overview adds a makefile command to enable and disable mptcp over different ports (not interfaces) for linux kernels greater than 5.6 not required, but potentially useful (cherry picked from commit 808d0f2) # Conflicts: # Makefile
rach-id
pushed a commit
that referenced
this pull request
Nov 26, 2024
## Overview adds a makefile command to enable and disable mptcp over different ports (not interfaces) for linux kernels greater than 5.6 not required, but potentially useful (cherry picked from commit 808d0f2)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
adds a makefile command to enable and disable mptcp over different ports (not interfaces) for linux kernels greater than 5.6
not required, but potentially useful