OCPBUGS-64771: Force Global IP forwarding for DPU host mode #2831
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-bug
|
@tsorya: This pull request references Jira Issue OCPBUGS-64771, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughNode startup script centralizes per-node IP forwarding handling, forcing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (2)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tsorya The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
![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
🧹 Nitpick comments (1)
bindata/network/ovn-kubernetes/common/008-script-lib.yaml (1)
583-596: Based on the web search results and code analysis, the original review comment is incorrect. While the bash syntax will function in this specific case, the code violates established bash best practices.Add quotes around variable expansion in the string comparison
The variable
${ip_forwarding_mode}should be quoted in the conditional:[ "${ip_forwarding_mode}" == "Global" ]instead of[ ${ip_forwarding_mode} == "Global" ].Always use double quotes around variable names to avoid any word splitting or globbing issues. While the code functions correctly in this instance because "Global" contains no spaces, quoting variables is a crucial best practice in Bash scripting and prevents unexpected errors from word splitting and glob expansion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
README.md(1 hunks)bindata/network/ovn-kubernetes/common/008-script-lib.yaml(2 hunks)docs/ovn_node_mode.md(3 hunks)pkg/network/ovn_kubernetes_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
docs/ovn_node_mode.mdpkg/network/ovn_kubernetes_test.goREADME.mdbindata/network/ovn-kubernetes/common/008-script-lib.yaml
🪛 LanguageTool
docs/ovn_node_mode.md
[grammar] ~27-~27: Did you mean “too Global to”?
Context: ...- ip_forwarding_mode="Global" (forced to Global to allow traffic forwarding across interfa...
(TOO_ADJECTIVE_TO)
🔇 Additional comments (5)
docs/ovn_node_mode.md (1)
91-108: LGTM: Clear documentation of IP forwarding behaviorThe new IP Forwarding Mode Behavior section clearly documents the differences between Full and DPU Host modes, with good rationale for why DPU hosts require Global mode. The documentation correctly explains that this is required for proper traffic flow between management and data plane interfaces.
bindata/network/ovn-kubernetes/common/008-script-lib.yaml (2)
551-552: Initial IP forwarding mode assignmentThe ip_forwarding_mode variable is correctly initialized from the template variable, which can then be overridden for specific node modes.
574-581: Correct implementation of forced Global mode for DPU hostsThe override logic properly forces IP forwarding to Global mode for DPU host nodes, regardless of cluster-wide configuration. The detailed comments clearly explain the rationale.
README.md (1)
170-172: LGTM: Concise documentation of DPU host IP forwardingThe README appropriately documents the forced Global IP forwarding for DPU host nodes and correctly references the detailed documentation in
docs/ovn_node_mode.md.pkg/network/ovn_kubernetes_test.go (1)
4458-4517: LGTM: Comprehensive test coverage for IP forwarding behaviorThe new test cases thoroughly validate the IP forwarding implementation across all relevant scenarios:
- DPU host mode with default/empty config - Correctly verifies that Global mode is forced and forwarding is enabled
- DPU host mode with Restricted template - Validates the override behavior, ensuring Global takes precedence
- Full mode with Global - Confirms that explicit Global configuration is respected
- Full mode with default/Restricted - Validates default Restricted behavior
The tests use appropriate assertions for both positive (mustContain) and negative (mustNotContain) cases, ensuring the rendered scripts contain the expected sysctl commands and flags.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
bindata/network/ovn-kubernetes/common/008-script-lib.yaml (1)
583-596: Consider quoting the variable for consistencyThe IP forwarding logic is correct and handles both Global and Restricted modes appropriately. However, on line 589,
${ip_forwarding_mode}should be quoted for consistency with line 558 where${OVN_NODE_MODE}is quoted.Apply this diff:
- if [ ${ip_forwarding_mode} == "Global" ]; then + if [ "${ip_forwarding_mode}" == "Global" ]; thenThis follows shell scripting best practices and maintains consistency with other conditionals in the script.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
README.md(1 hunks)bindata/network/ovn-kubernetes/common/008-script-lib.yaml(2 hunks)docs/ovn_node_mode.md(3 hunks)pkg/network/ovn_kubernetes_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
docs/ovn_node_mode.mdpkg/network/ovn_kubernetes_test.gobindata/network/ovn-kubernetes/common/008-script-lib.yaml
🪛 LanguageTool
docs/ovn_node_mode.md
[grammar] ~27-~27: Did you mean “too Global to”?
Context: ...- ip_forwarding_mode="Global" (forced to Global to allow traffic forwarding across interfa...
(TOO_ADJECTIVE_TO)
🔇 Additional comments (4)
bindata/network/ovn-kubernetes/common/008-script-lib.yaml (2)
551-552: LGTM: IP forwarding mode initializationThe template variable initialization is clean and allows cluster-wide configuration to be passed to the node startup script.
575-581: LGTM: DPU host mode override is well-documentedThe forced override to Global mode for DPU hosts is clearly justified in the comments. The rationale about traffic flow between management and data plane interfaces is helpful for future maintainers.
pkg/network/ovn_kubernetes_test.go (1)
4458-4478: Excellent test coverage for IP forwarding logicThe new test case comprehensively validates all aspects of the IP forwarding feature:
- Template variable initialization
- DPU-host mode override to Global
- Conditional sysctl commands for IPv4/IPv6
- Forwarding flag assignment
This ensures the generated script will correctly handle IP forwarding across different node modes.
docs/ovn_node_mode.md (1)
91-108: Comprehensive IP forwarding documentationThe new "IP Forwarding Mode Behavior" section clearly documents the differences between Full and DPU Host modes. The rationale explaining why DPU hosts require Global mode (traffic flow between management and data plane interfaces) is particularly helpful for operators troubleshooting connectivity issues.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
bindata/network/ovn-kubernetes/common/008-script-lib.yaml (1)
551-596: Consolidate and quote the ip_forwarding_mode variable for robustness.The new IP forwarding logic is sound: initializing from a templated value, overriding to "Global" for dpu-host, then applying sysctl settings and passing flags accordingly. However, the variable comparison at line 589 is unquoted, which is inconsistent with the pattern used elsewhere in the file (e.g., line 598:
"{{.OVN_GATEWAY_MODE}}") and could cause a bash syntax error if the template variable{{.IP_FORWARDING_MODE}}expands to an empty string.Apply this diff to quote the variable and improve safety:
- if [ ${ip_forwarding_mode} == "Global" ]; then + if [ "${ip_forwarding_mode}" == "Global" ]; then
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
README.md(1 hunks)bindata/network/ovn-kubernetes/common/008-script-lib.yaml(2 hunks)docs/ovn_node_mode.md(3 hunks)pkg/network/ovn_kubernetes_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/network/ovn_kubernetes_test.go
- README.md
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
docs/ovn_node_mode.mdbindata/network/ovn-kubernetes/common/008-script-lib.yaml
🔇 Additional comments (3)
bindata/network/ovn-kubernetes/common/008-script-lib.yaml (1)
575-581: Rationale and DPU-host override are clear and correct.The comment clearly documents why DPU hosts must force Global forwarding, and the override is placed appropriately in the mode-specific conditional. The logic correctly forces this setting regardless of cluster-wide configuration, which aligns with the PR objectives.
docs/ovn_node_mode.md (2)
91-108: IP Forwarding Mode Behavior section is well-documented and accurate.The new documentation clearly explains the difference between Full mode (respects cluster-wide config) and DPU Host mode (always forces Global), with explicit rationale. The section accurately describes the system-level sysctl changes and app-level flag behavior, and remains consistent with the script implementation.
27-27: Node mode table updated correctly.The addition of
ip_forwarding_mode="Global"to the dpu-host row maintains consistency with the detailed section below and matches the script logic.
Ensure DPU host nodes always use Global IP forwarding mode regardless
of cluster-wide configuration.
- Force ip_forwarding_mode="Global" for dpu-host mode
- Add comprehensive test cases for IP forwarding behavior
- Document DPU host IP forwarding requirements and rationale
DPU hosts require IP forwarding enabled to allow traffic flow between
management and data plane interfaces. This change ensures proper DPU
operation even when cluster-wide IPForwarding is set to Restricted
Signed-off-by: Igal Tsoiref <[email protected]>
|
/retest-required |
|
@tsorya: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
2 participants
Ensure DPU host nodes always use Global IP forwarding mode regardless of cluster-wide configuration.
DPU hosts require IP forwarding enabled to allow traffic flow between management and data plane interfaces. This change ensures proper DPU operation even when cluster-wide IPForwarding is set to Restricted.