[DO NOT MERGE] Test: Revert "Remove dead OVN_MULTI_NETWORK_ENABLE template variable"#3032
Open
weliang1 wants to merge 3 commits into
Open
[DO NOT MERGE] Test: Revert "Remove dead OVN_MULTI_NETWORK_ENABLE template variable"#3032weliang1 wants to merge 3 commits into
weliang1 wants to merge 3 commits into
Conversation
This reverts commit 411921c.
Walkthrough
ChangesOVN Multi-Network Default Enable with Per-Node Feature Gating
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: weliang1 The full list of commands accepted by this bot can be found here. DetailsNeeds 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bindata/network/ovn-kubernetes/common/008-script-lib.yaml`:
- Around line 607-616: The multi_network_enabled_flag variable is being
unconditionally overwritten on line 614 within the second condition block that
checks if OVN_NODE_MODE is not "dpu-host", which erases the conditional logic
from lines 609-611. Fix this by removing the incorrect assignment of
multi_network_enabled_flag on line 614 (it should only assign
network_segmentation_enabled_flag in that block). Additionally, the comment
indicates that neither the multi_network_enabled_flag nor
network_segmentation_enabled_flag variables are being included in the final
ovnkube command arguments, so add both flags to the exec args for the ovnkube
process to ensure the computed values are actually applied.
In `@bindata/network/ovn-kubernetes/managed/004-config.yaml`:
- Around line 43-45: The conditional logic for enable-multi-network is inverted,
causing the feature to be enabled when it should be disabled. In the template
section at lines 43-45 and the similar section at lines 133-138, remove the
`not` operator from the condition `{{- if not .OVN_MULTI_NETWORK_ENABLE }}` so
it becomes `{{- if .OVN_MULTI_NETWORK_ENABLE }}`. This ensures that
`enable-multi-network=true` is only rendered when the feature is actually
intended to be enabled, allowing DisableMultiNetwork to properly override the
setting.
In `@bindata/network/ovn-kubernetes/self-hosted/004-config.yaml`:
- Around line 49-51: The conditional logic is inverted in the template around
the enable-multi-network configuration. The current condition `{{- if not
.OVN_MULTI_NETWORK_ENABLE }}` causes enable-multi-network=true to be set only
when the feature flag is false, which breaks the intended behavior. Remove the
`not` operator from the if statement so that enable-multi-network=true is
applied when .OVN_MULTI_NETWORK_ENABLE is true.
In `@pkg/network/ovn_kubernetes_test.go`:
- Around line 714-753: The test case with desc "disabled multi-network" and
disableMultiNet set to true has a contradictory expected output that still
contains enable-multi-network=true in the ovnkubernetesfeature section. Change
the enable-multi-network value in the expected output from true to false for
this test case to align with the test's intent to validate the disabled
multi-network behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ac9a095e-2109-4eed-94ec-0325ebe1397d
📒 Files selected for processing (5)
bindata/network/ovn-kubernetes/common/008-script-lib.yamlbindata/network/ovn-kubernetes/managed/004-config.yamlbindata/network/ovn-kubernetes/self-hosted/004-config.yamlpkg/network/ovn_kubernetes.gopkg/network/ovn_kubernetes_test.go
Comment on lines
+43
to
+45
| {{- if not .OVN_MULTI_NETWORK_ENABLE }} | ||
| enable-multi-network=true | ||
| {{- end }} |
There was a problem hiding this comment.
enable-multi-network rendering currently defeats the disable override
These template branches render enable-multi-network=true when .OVN_MULTI_NETWORK_ENABLE is false (and in hostedcluster, effectively in both branches), so DisableMultiNetwork=true does not actually disable the feature.
Suggested fix
- {{- if not .OVN_MULTI_NETWORK_ENABLE }}
- enable-multi-network=true
- {{- end }}
+ enable-multi-network={{.OVN_MULTI_NETWORK_ENABLE}}
...
-{{- if .OVN_MULTI_NETWORK_ENABLE }}
- enable-multi-network=true
-{{- end }}
- {{- if not .OVN_MULTI_NETWORK_ENABLE }}
- enable-multi-network=true
- {{- end }}
+ enable-multi-network={{.OVN_MULTI_NETWORK_ENABLE}}Also applies to: 133-138
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bindata/network/ovn-kubernetes/managed/004-config.yaml` around lines 43 - 45,
The conditional logic for enable-multi-network is inverted, causing the feature
to be enabled when it should be disabled. In the template section at lines 43-45
and the similar section at lines 133-138, remove the `not` operator from the
condition `{{- if not .OVN_MULTI_NETWORK_ENABLE }}` so it becomes `{{- if
.OVN_MULTI_NETWORK_ENABLE }}`. This ensures that `enable-multi-network=true` is
only rendered when the feature is actually intended to be enabled, allowing
DisableMultiNetwork to properly override the setting.
Comment on lines
+49
to
+51
| {{- if not .OVN_MULTI_NETWORK_ENABLE }} | ||
| enable-multi-network=true | ||
| {{- end }} |
There was a problem hiding this comment.
Conditional is inverted for multi-network enablement
enable-multi-network=true is emitted only when .OVN_MULTI_NETWORK_ENABLE is false, which inverts the intended behavior and breaks DisableMultiNetwork.
Suggested fix
- {{- if not .OVN_MULTI_NETWORK_ENABLE }}
- enable-multi-network=true
- {{- end }}
+ enable-multi-network={{.OVN_MULTI_NETWORK_ENABLE}}📝 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
| {{- if not .OVN_MULTI_NETWORK_ENABLE }} | |
| enable-multi-network=true | |
| {{- end }} | |
| enable-multi-network={{.OVN_MULTI_NETWORK_ENABLE}} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bindata/network/ovn-kubernetes/self-hosted/004-config.yaml` around lines 49 -
51, The conditional logic is inverted in the template around the
enable-multi-network configuration. The current condition `{{- if not
.OVN_MULTI_NETWORK_ENABLE }}` causes enable-multi-network=true to be set only
when the feature flag is false, which breaks the intended behavior. Remove the
`not` operator from the if statement so that enable-multi-network=true is
applied when .OVN_MULTI_NETWORK_ENABLE is true.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bindata/network/ovn-kubernetes/common/008-script-lib.yaml`:
- Around line 548-550: The enable_multicast_flag variable is conditionally
cleared for dpu-host mode (lines 563-565), but this control is bypassed because
line 769 unconditionally passes the --enable-multicast argument directly instead
of using the enable_multicast_flag variable. Fix this by replacing the hardcoded
--enable-multicast argument at line 769 with a reference to the
enable_multicast_flag variable, ensuring that the mode-specific disable for
dpu-host actually takes effect.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 14ef1f8a-1392-43c7-8ae2-5216cf1c1cb6
📒 Files selected for processing (10)
README.mdbindata/network/ovn-kubernetes/common/008-script-lib.yamlbindata/network/ovn-kubernetes/managed/004-config.yamlbindata/network/ovn-kubernetes/managed/ovnkube-control-plane.yamlbindata/network/ovn-kubernetes/self-hosted/004-config.yamlbindata/network/ovn-kubernetes/self-hosted/ovnkube-control-plane.yamldocs/architecture.mddocs/operands.mddocs/ovn_node_mode.mdpkg/network/ovn_kubernetes_test.go
✅ Files skipped from review due to trivial changes (4)
- docs/operands.md
- docs/architecture.md
- README.md
- docs/ovn_node_mode.md
🚧 Files skipped from review as they are similar to previous changes (2)
- bindata/network/ovn-kubernetes/self-hosted/004-config.yaml
- bindata/network/ovn-kubernetes/managed/004-config.yaml
Comment on lines
+548
to
+550
| # enable multicast | ||
| enable_multicast_flag="--enable-multicast" | ||
|
|
There was a problem hiding this comment.
DPU-host multicast disable is bypassed by an unconditional CLI arg.
enable_multicast_flag is cleared for dpu-host (Lines 563-565), but Line 769 still always passes --enable-multicast, so the mode-specific disable never takes effect.
Suggested fix
exec /usr/bin/ovnkube \
${init_ovnkube_controller} \
--init-node "${K8S_NODE}" \
@@
${dns_name_resolver_enabled_flag} \
${network_observability_enabled_flag} \
${enable_multicast_flag} \
--zone ${K8S_NODE} \
${enable_interconnect_flag} \
- --enable-multicast \
--acl-logging-rate-limit "{{.OVNPolicyAuditRateLimit}}" \Also applies to: 563-565, 766-770
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bindata/network/ovn-kubernetes/common/008-script-lib.yaml` around lines 548 -
550, The enable_multicast_flag variable is conditionally cleared for dpu-host
mode (lines 563-565), but this control is bypassed because line 769
unconditionally passes the --enable-multicast argument directly instead of using
the enable_multicast_flag variable. Fix this by replacing the hardcoded
--enable-multicast argument at line 769 with a reference to the
enable_multicast_flag variable, ensuring that the mode-specific disable for
dpu-host actually takes effect.
Contributor
|
@weliang1: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
This reverts commit 411921c.
Summary by CodeRabbit
dpu-hostmode, node startup now automatically disables incompatible feature flags (including egress-related and policy/segmentation features).OVN_NODE_MODEand clarified which features are enabled/disabled per node mode.dpu-hostbehavior.