OCPBUGS-78731: Revert "NVIDIA-554: DPU-host mode: use ConfigMap for OVN feature enablement instead of per-node script gating"#2938
Conversation
…lement instead of per-node script gating" This reverts commit dfa1262.
openshift-ci-robot
added
jira/valid-reference
|
@arkadeepsen: This pull request references Jira Issue OCPBUGS-78731, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn 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. |
WalkthroughThis PR implements per-node feature enforcement via OVN_NODE_MODE environment variables. Feature flags for egress, multicast, multi-network policies, and admin policies are moved from static ConfigMaps to dynamic startup scripts that conditionally enable/disable features based on node mode (full vs. dpu-host). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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.11.3)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: inconsistent vendoring in :\n\tgithub.com/Masterminds/semver@v1.5.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/Masterminds/sprig/v3@v3.2.3: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/containernetworking/cni@v0.8.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/ghodss/yaml@v1.0.1-0.20190212211648-25d852aebe32: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/go-bindata/go-bindata@v3.1.2+incompatible: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/onsi/gomega@v1.38.1: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/ope ... [truncated 17231 characters] ... ired in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/gengo/v2@v2.0.0-20250922181213-ec3ebc5fd46b: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/kms@v0.34.1: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/kube-aggregator@v0.34.1: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tsigs.k8s.io/randfill@v1.0.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tsigs.k8s.io/structured-merge-diff/v6@v6.3.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@arkadeepsen: This pull request references Jira Issue OCPBUGS-78731, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (weliang@redhat.com), skipping review request. DetailsIn 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. |
![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
🧹 Nitpick comments (1)
bindata/network/ovn-kubernetes/managed/004-config.yaml (1)
120-125: Collapse redundant template branches forenable-multi-network.Line 120-Line 125 renders
enable-multi-network=truein both branches, so the condition has no effect and obscures intent.♻️ Suggested simplification
-{{- if .OVN_MULTI_NETWORK_ENABLE }} - enable-multi-network=true -{{- end }} - {{- if not .OVN_MULTI_NETWORK_ENABLE }} - enable-multi-network=true - {{- end }} + enable-multi-network=trueAs per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindata/network/ovn-kubernetes/managed/004-config.yaml` around lines 120 - 125, The two conditional branches around .OVN_MULTI_NETWORK_ENABLE both render the same setting, so collapse them by removing the if/else and leaving a single declaration of enable-multi-network=true (or, if the setting should depend on the variable, change to a single conditional that renders enable-multi-network=true only when .OVN_MULTI_NETWORK_ENABLE is truthy); update the template that currently contains the .OVN_MULTI_NETWORK_ENABLE checks and the enable-multi-network entry to use one clear expression.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindata/network/ovn-kubernetes/common/008-script-lib.yaml`:
- Around line 613-623: The second conditional block is incorrectly overwriting
multi_network_enabled_flag for all non-DPU-host nodes; update the block that
checks OVN_NODE_MODE so it does not reassign multi_network_enabled_flag and only
sets network_segmentation_enabled_flag when appropriate (i.e., leave
multi_network_enabled_flag assignment solely to the conditional that checks
OVN_MULTI_NETWORK_ENABLE and OVN_NODE_MODE). Locate the
multi_network_enabled_flag and network_segmentation_enabled_flag assignments
(and the OVN_NODE_MODE/OVN_MULTI_NETWORK_ENABLE checks) and remove or change the
erroneous multi_network_enabled_flag="--enable-multi-network" in the later block
so operator-driven OVN_MULTI_NETWORK_ENABLE controls multi_network_enabled_flag
as intended.
In `@pkg/network/ovn_kubernetes_test.go`:
- Around line 3962-3977: renderControlPlaneWithOverrides currently ignores the
variant parameter and hardcodes HyperShiftConfig.Enabled: false, so it always
renders the self-hosted control plane; update renderControlPlaneWithOverrides to
check the variant string (e.g., if variant == "managed") and set
bs.OVN.OVNKubernetesConfig.HyperShiftConfig.Enabled = true for the managed path
(leave false for self-hosted), and then add at least one test case with variant:
"managed" in TestOVNKubernetesControlPlaneFlags to validate the managed template
rendering changes.
---
Nitpick comments:
In `@bindata/network/ovn-kubernetes/managed/004-config.yaml`:
- Around line 120-125: The two conditional branches around
.OVN_MULTI_NETWORK_ENABLE both render the same setting, so collapse them by
removing the if/else and leaving a single declaration of
enable-multi-network=true (or, if the setting should depend on the variable,
change to a single conditional that renders enable-multi-network=true only when
.OVN_MULTI_NETWORK_ENABLE is truthy); update the template that currently
contains the .OVN_MULTI_NETWORK_ENABLE checks and the enable-multi-network entry
to use one clear expression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 14775f1d-4d71-4ebc-819e-ca542fd4fe51
📒 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
| multi_network_enabled_flag= | ||
| if [[ "{{.OVN_MULTI_NETWORK_ENABLE}}" == "true" && "${OVN_NODE_MODE}" != "dpu-host" ]]; then | ||
| multi_network_enabled_flag="--enable-multi-network" | ||
| fi | ||
|
|
||
| network_segmentation_enabled_flag= | ||
| if [[ "${OVN_NODE_MODE}" != "dpu-host" ]]; then | ||
| multi_network_enabled_flag="--enable-multi-network" | ||
| network_segmentation_enabled_flag="--enable-network-segmentation" | ||
| fi | ||
|
|
There was a problem hiding this comment.
multi_network_enabled_flag is unintentionally forced on for all non-DPU-host nodes
Line [620] overrides the earlier config-gated assignment from Line [614]-Line [616], so --enable-multi-network is passed even when {{.OVN_MULTI_NETWORK_ENABLE}} is not true. This bypasses intended operator-driven enablement.
Proposed fix
- multi_network_enabled_flag=
- if [[ "{{.OVN_MULTI_NETWORK_ENABLE}}" == "true" && "${OVN_NODE_MODE}" != "dpu-host" ]]; then
- multi_network_enabled_flag="--enable-multi-network"
- fi
-
- network_segmentation_enabled_flag=
- if [[ "${OVN_NODE_MODE}" != "dpu-host" ]]; then
- multi_network_enabled_flag="--enable-multi-network"
- network_segmentation_enabled_flag="--enable-network-segmentation"
- fi
+ multi_network_enabled_flag=
+ network_segmentation_enabled_flag=
+ if [[ "{{.OVN_MULTI_NETWORK_ENABLE}}" == "true" && "${OVN_NODE_MODE}" != "dpu-host" ]]; then
+ multi_network_enabled_flag="--enable-multi-network"
+ network_segmentation_enabled_flag="--enable-network-segmentation"
+ fi📝 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.
| multi_network_enabled_flag= | |
| if [[ "{{.OVN_MULTI_NETWORK_ENABLE}}" == "true" && "${OVN_NODE_MODE}" != "dpu-host" ]]; then | |
| multi_network_enabled_flag="--enable-multi-network" | |
| fi | |
| network_segmentation_enabled_flag= | |
| if [[ "${OVN_NODE_MODE}" != "dpu-host" ]]; then | |
| multi_network_enabled_flag="--enable-multi-network" | |
| network_segmentation_enabled_flag="--enable-network-segmentation" | |
| fi | |
| multi_network_enabled_flag= | |
| network_segmentation_enabled_flag= | |
| if [[ "{{.OVN_MULTI_NETWORK_ENABLE}}" == "true" && "${OVN_NODE_MODE}" != "dpu-host" ]]; then | |
| multi_network_enabled_flag="--enable-multi-network" | |
| network_segmentation_enabled_flag="--enable-network-segmentation" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindata/network/ovn-kubernetes/common/008-script-lib.yaml` around lines 613 -
623, The second conditional block is incorrectly overwriting
multi_network_enabled_flag for all non-DPU-host nodes; update the block that
checks OVN_NODE_MODE so it does not reassign multi_network_enabled_flag and only
sets network_segmentation_enabled_flag when appropriate (i.e., leave
multi_network_enabled_flag assignment solely to the conditional that checks
OVN_MULTI_NETWORK_ENABLE and OVN_NODE_MODE). Locate the
multi_network_enabled_flag and network_segmentation_enabled_flag assignments
(and the OVN_NODE_MODE/OVN_MULTI_NETWORK_ENABLE checks) and remove or change the
erroneous multi_network_enabled_flag="--enable-multi-network" in the later block
so operator-driven OVN_MULTI_NETWORK_ENABLE controls multi_network_enabled_flag
as intended.
| func renderControlPlaneWithOverrides(t *testing.T, variant string, overrides map[string]interface{}) string { | ||
| g := NewGomegaWithT(t) | ||
| crd := OVNKubernetesConfig.DeepCopy() | ||
| config := &crd.Spec | ||
| fillDefaults(config, nil) | ||
|
|
||
| bs := fakeBootstrapResult() | ||
| bs.OVN = bootstrap.OVNBootstrapResult{ | ||
| ControlPlaneReplicaCount: 1, | ||
| OVNKubernetesConfig: &bootstrap.OVNConfigBoostrapResult{ | ||
| DpuHostModeLabel: OVN_NODE_SELECTOR_DEFAULT_DPU_HOST, | ||
| DpuModeLabel: OVN_NODE_SELECTOR_DEFAULT_DPU, | ||
| SmartNicModeLabel: OVN_NODE_SELECTOR_DEFAULT_SMART_NIC, | ||
| HyperShiftConfig: &bootstrap.OVNHyperShiftBootstrapResult{Enabled: false}, | ||
| ConfigOverrides: toStringMap(overrides), | ||
| }, |
There was a problem hiding this comment.
Managed control-plane path is not covered by the new flag tests.
renderControlPlaneWithOverrides ignores variant and hardcodes HyperShiftConfig.Enabled: false (Line 3975), so the helper always renders the self-hosted path. The new cases then only validate self-hosted behavior, leaving managed template changes untested.
✅ Minimal fix direction
func renderControlPlaneWithOverrides(t *testing.T, variant string, overrides map[string]interface{}) string {
@@
- HyperShiftConfig: &bootstrap.OVNHyperShiftBootstrapResult{Enabled: false},
+ HyperShiftConfig: &bootstrap.OVNHyperShiftBootstrapResult{Enabled: variant == "managed"},
@@
- if obj.GetKind() == "Deployment" && obj.GetName() == "ovnkube-control-plane" && obj.GetNamespace() == "openshift-ovn-kubernetes" {
+ if obj.GetKind() == "Deployment" && obj.GetName() == "ovnkube-control-plane" {
...
}
}
}Also add at least one variant: "managed" test case in TestOVNKubernetesControlPlaneFlags.
As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 4344-4447
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/network/ovn_kubernetes_test.go` around lines 3962 - 3977,
renderControlPlaneWithOverrides currently ignores the variant parameter and
hardcodes HyperShiftConfig.Enabled: false, so it always renders the self-hosted
control plane; update renderControlPlaneWithOverrides to check the variant
string (e.g., if variant == "managed") and set
bs.OVN.OVNKubernetesConfig.HyperShiftConfig.Enabled = true for the managed path
(leave false for self-hosted), and then add at least one test case with variant:
"managed" in TestOVNKubernetesControlPlaneFlags to validate the managed template
rendering changes.
|
/lgtm clean revert except for an easily-resolved conflict in |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arkadeepsen, danwinship The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/test e2e-metal-ipi-ovn-dualstack-bgp-local-gw |
|
[sig-network-edge] DNS should answer A and AAAA queries for a dual-stack service [apigroup:config.openshift.io] [Suite:openshift/conformance/parallel]
{ fail [github.com/openshift/origin/test/extended/dns/dns.go:251]: Failed: timed out waiting for the condition}Related bug: https://redhat.atlassian.net/browse/OCPBUGS-78053 |
|
/test e2e-aws-ovn-hypershift-conformance |
|
/test e2e-metal-ipi-ovn-dualstack-bgp-local-gw |
|
/test e2e-aws-ovn-hypershift-conformance |
|
Retesting again. Some of the tests are failing because one of the packet sniffer daemonset pods is not becoming ready. /test e2e-metal-ipi-ovn-dualstack-bgp-local-gw |
|
/verified by @weliang |
openshift-ci-robot
added
the
verified
|
@weliang1: This PR has been marked as verified by DetailsIn 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. |
|
/skip |
|
/test e2e-metal-ipi-ovn-dualstack-bgp-local-gw |
|
/test e2e-metal-ipi-ovn-dualstack-bgp-local-gw |
|
/override ci/prow/e2e-aws-ovn-hypershift-conformance https://redhat.atlassian.net/browse/OCPBUGS-78978 |
|
@jcaamano: Overrode contexts on behalf of jcaamano: ci/prow/e2e-aws-ovn-hypershift-conformance, ci/prow/e2e-metal-ipi-ovn-dualstack-bgp, ci/prow/e2e-metal-ipi-ovn-dualstack-bgp-local-gw DetailsIn 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 kubernetes-sigs/prow repository. |
|
/skip |
|
/tide refresh |
|
/skip |
|
/test e2e-metal-ipi-ovn-ipv6 |
|
/tide refresh |
1 similar comment
|
/tide refresh |
|
/skip |
|
/tide refresh |
|
/override pull-ci-openshift-ovn-kubernetes-master-e2e-metal-ipi-ovn-dualstack-bgp-local-gw-techpreview-frr-next https://redhat.atlassian.net/browse/OCPBUGS-78978 |
|
/override ci/prow/e2e-metal-ipi-ovn-dualstack-bgp-local-gw |
|
@jcaamano: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. DetailsIn 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 kubernetes-sigs/prow repository. |
|
@jcaamano: Overrode contexts on behalf of jcaamano: ci/prow/e2e-metal-ipi-ovn-dualstack-bgp-local-gw DetailsIn 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 kubernetes-sigs/prow repository. |
|
/refresh |
|
/override ci/prow/e2e-metal-ipi-ovn-ipv6 |
|
@jcaamano: Overrode contexts on behalf of jcaamano: ci/prow/e2e-metal-ipi-ovn-ipv6 DetailsIn 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 kubernetes-sigs/prow repository. |
|
@arkadeepsen: 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. |
|
/skip |
|
/override e2e-metal-ipi-ovn-ipv6-ipsec |
|
@jcaamano: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. DetailsIn 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 kubernetes-sigs/prow repository. |
|
/override ci/prow/e2e-metal-ipi-ovn-ipv6-ipsec |
|
@jcaamano: Overrode contexts on behalf of jcaamano: ci/prow/e2e-metal-ipi-ovn-ipv6-ipsec DetailsIn 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 kubernetes-sigs/prow repository. |
5 participants
Reverts #2907 (dfa1262)
#2907 caused multicast to be permanently disabled. This was caught by the
e2e-aws-ovn-fdp-qeCI jobs on the ovn-kubernetes repo: https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_ovn-kubernetes/3011/pull-ci-openshift-ovn-kubernetes-master-e2e-aws-ovn-fdp-qe/2032494493202124800Resolved merged conflicts in
ovn_kubernetes_test.gofile.