NVIDIA-554: DPU-host mode: use ConfigMap for OVN feature enablement instead of per-node script gating#2944
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@tsorya: This pull request references NVIDIA-554 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPer-node OVN_NODE_MODE-based feature gating was removed; feature enablement is now configured cluster-wide via the ovnkube ConfigMap. Startup scripts and control-plane manifests stop constructing/passing per-node feature flags; documentation and tests were updated to match the centralized configuration model. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bindata/network/ovn-kubernetes/common/008-script-lib.yaml (1)
701-724:⚠️ Potential issue | 🟠 MajorMulticast should be moved to
004-config.yamlper documented design.The documentation states that "Feature enablement (egress IP, multicast, multi-network, network segmentation, admin network policy, etc.) is managed through the cluster-wide ConfigMap (
004-config.yaml)", but--enable-multicastis still hardcoded as a CLI flag at line 723. The004-config.yamltemplates contain dozens ofenable-*settings (e.g.,enable-egress-ip,enable-multi-network,enable-admin-network-policy) butenable-multicastis absent. Either move multicast configuration into the config file to align with the documented design and other features, or update the documentation to clarify why multicast remains CLI-driven.🤖 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 701 - 724, The script currently hardcodes the CLI flag --enable-multicast in the ovnkube exec line; move multicast feature toggling into the cluster-wide ConfigMap like other features in 004-config.yaml. Add a ConfigMap/template key (e.g., enable-multicast) to 004-config.yaml, expose it as a rendered variable (e.g., multicast_enable_flag) alongside the existing flags (similar to network_observability_enabled_flag), then replace the literal --enable-multicast in the exec invocation with the variable ${multicast_enable_flag} (or remove it when false). Alternatively, if you intend multicast to stay CLI-driven, update the docs to explicitly state why --enable-multicast remains a hardcoded flag instead of being managed via 004-config.yaml.
🧹 Nitpick comments (1)
pkg/network/ovn_kubernetes_test.go (1)
296-306: Add one DPU-host regression case.These updated goldens prove the new
[ovnkubernetesfeature]contents, but they still don't lock in the behavior this PR is changing:dpu-hostshould stop rewriting feature flags while still changing gateway/interface selection,--ovnkube-node-mode, andinit_ovnkube_controller. A small render test aroundovnkube-lib.shwould make that contract much harder to regress.Also applies to: 342-352, 401-412, 463-474, 525-535, 586-596, 636-646, 689-699, 735-745, 782-793, 831-841, 877-887, 925-935, 972-982
🤖 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 296 - 306, Add a single regression test case in pkg/network/ovn_kubernetes_test.go that renders ovnkube-lib.sh for the dpu-host node mode: feed the test input containing the full [ovnkubernetesfeature] block shown in the diff, set node mode to "dpu-host", and assert that the output preserves the entire [ovnkubernetesfeature] block exactly (no feature flags rewritten) while still updating only the gateway/interface selection, the --ovnkube-node-mode value to "dpu-host", and the init_ovnkube_controller behavior; place the case alongside the existing render tests in the file that exercise ovnkube-lib.sh so it will fail if dpu-host rewrites feature flags.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@bindata/network/ovn-kubernetes/common/008-script-lib.yaml`:
- Around line 701-724: The script currently hardcodes the CLI flag
--enable-multicast in the ovnkube exec line; move multicast feature toggling
into the cluster-wide ConfigMap like other features in 004-config.yaml. Add a
ConfigMap/template key (e.g., enable-multicast) to 004-config.yaml, expose it as
a rendered variable (e.g., multicast_enable_flag) alongside the existing flags
(similar to network_observability_enabled_flag), then replace the literal
--enable-multicast in the exec invocation with the variable
${multicast_enable_flag} (or remove it when false). Alternatively, if you intend
multicast to stay CLI-driven, update the docs to explicitly state why
--enable-multicast remains a hardcoded flag instead of being managed via
004-config.yaml.
---
Nitpick comments:
In `@pkg/network/ovn_kubernetes_test.go`:
- Around line 296-306: Add a single regression test case in
pkg/network/ovn_kubernetes_test.go that renders ovnkube-lib.sh for the dpu-host
node mode: feed the test input containing the full [ovnkubernetesfeature] block
shown in the diff, set node mode to "dpu-host", and assert that the output
preserves the entire [ovnkubernetesfeature] block exactly (no feature flags
rewritten) while still updating only the gateway/interface selection, the
--ovnkube-node-mode value to "dpu-host", and the init_ovnkube_controller
behavior; place the case alongside the existing render tests in the file that
exercise ovnkube-lib.sh so it will fail if dpu-host rewrites feature flags.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5c1994eb-63ed-421c-ab98-8900f240f6ff
📒 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 with no reviewable changes (3)
- README.md
- docs/architecture.md
- docs/operands.md
|
/retest-required |
|
/test ? |
|
oc exec -it -n openshift-ovn-kubernetes ovnkube-node-bpkjk -- bash [root@dhcp-8-231-235 ~]# ovn-nbctl find ACL | grep -i mcast match : "(ip4.mcast || mldv1 || mldv2 || (ip6.dst[120..127] == 0xff && ip6.dst[116] == 1))" match : "(ip4.mcast || mldv1 || mldv2 || (ip6.dst[120..127] == 0xff && ip6.dst[116] == 1))" match : "outport == @a15696136357465712812 && (igmp || (ip4.src == $a14513626322132345176 && ip4.mcast))" match : "outport == @a4743249366342378346 && (ip4.mcast || mldv1 || mldv2 || (ip6.dst[120..127] == 0xff && ip6.dst[116] == 1))" match : "inport == @a4743249366342378346 && (ip4.mcast || mldv1 || mldv2 || (ip6.dst[120..127] == 0xff && ip6.dst[116] == 1))" match : "inport == @a15696136357465712812 && ip4.mcast" 10.130.0.3 : unicast, xmt/rcv/%loss = 10/10/0%, min/avg/max/std-dev = 0.290/0.404/1.026/0.222 10.130.0.3 : multicast, xmt/rcv/%loss = 10/9/9% (seq>=2 0%), min/avg/max/std-dev = 0.307/0.522/1.115/0.319 10.128.0.30 : unicast, xmt/rcv/%loss = 10/10/0%, min/avg/max/std-dev = 0.314/0.449/0.717/0.140 10.128.0.30 : multicast, xmt/rcv/%loss = 10/9/9% (seq>=2 0%), min/avg/max/std-dev = 0.285/0.445/0.823/0.155 10.128.0.30 : unicast, xmt/rcv/%loss = 10/10/0%, min/avg/max/std-dev = 0.307/0.488/0.991/0.218 10.128.0.30 : multicast, xmt/rcv/%loss = 10/9/9% (seq>=2 0%), min/avg/max/std-dev = 0.313/0.499/1.212/0.273 10.129.0.7 : unicast, xmt/rcv/%loss = 10/10/0%, min/avg/max/std-dev = 0.307/0.462/1.057/0.252 10.129.0.7 : multicast, xmt/rcv/%loss = 10/9/9% (seq>=2 0%), min/avg/max/std-dev = 0.320/0.501/1.513/0.382 |
|
/retest |
openshift-ci
Bot
added
the
needs-rebase
tsorya
force-pushed
the
igal/dpu-host-mode
branch
from
f58272e to
7f928af
Compare
openshift-ci
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/network/ovn_kubernetes_test.go`:
- Around line 298-308: Update the two failing tests to match the ConfigMap-based
migration: in TestOVNKubernetesControlPlaneFlags and
TestOVNKubernetesScriptLibCombined remove assertions that expect the removed CLI
flags and script-lib variables (the old enable-* CLI flags and corresponding
script-lib vars) and instead assert the presence/contents of the new
ovnkube.conf configmap golden (the block shown at lines 298–308). Also either
implement or remove references to the missing helpers
renderControlPlaneWithOverrides and renderScriptLibWithOverrides used by those
tests—if keeping them, implement helpers that render using the new ConfigMap
migration semantics and return the rendered ovnkube.conf/script-lib output;
otherwise delete calls to them and inline the appropriate rendering/assertion
using existing render helpers. Ensure all references are to the test names
TestOVNKubernetesControlPlaneFlags, TestOVNKubernetesScriptLibCombined and the
helper names renderControlPlaneWithOverrides/renderScriptLibWithOverrides so
reviewers can locate the edits.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d16edd6a-c511-46cc-ada2-5d68285bf3f3
📒 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 with no reviewable changes (3)
- docs/architecture.md
- docs/operands.md
- README.md
✅ Files skipped from review due to trivial changes (3)
- bindata/network/ovn-kubernetes/managed/004-config.yaml
- docs/ovn_node_mode.md
- bindata/network/ovn-kubernetes/common/008-script-lib.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- bindata/network/ovn-kubernetes/self-hosted/004-config.yaml
openshift-ci
Bot
added
the
needs-rebase
tsorya
force-pushed
the
igal/dpu-host-mode
branch
from
7f928af to
f93b3fc
Compare
openshift-ci
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/network/ovn_kubernetes_test.go (1)
1020-1024:⚠️ Potential issue | 🟠 MajorTest expectation appears inconsistent with ConfigMap template changes.
The "enable network connect (DevPreview)" test case expected output (lines 1020-1024) is missing the egress feature flags (
enable-egress-ip=true,enable-egress-firewall=true,enable-egress-qos=true,enable-egress-service=true) and other flags (enable-multi-network=true,enable-admin-network-policy=true,enable-multi-external-gateway=true) that are now unconditionally emitted in the ConfigMap template per this PR.All other test cases in this file were updated to include these flags, but this one appears to have been missed. This will likely cause the test to fail.
🐛 Proposed fix to add missing flags
[ovnkubernetesfeature] +enable-egress-ip=true +enable-egress-firewall=true +enable-egress-qos=true +enable-egress-service=true egressip-node-healthcheck-port=9107 +enable-multi-network=true enable-network-segmentation=true enable-preconfigured-udn-addresses=true +enable-admin-network-policy=true +enable-multi-external-gateway=true enable-network-connect=true🤖 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 1020 - 1024, The test case "enable network connect (DevPreview)" in ovn_kubernetes_test.go has an outdated expected ConfigMap snippet; update its expected output to include the unconditionally emitted flags now present in the template: add enable-egress-ip=true, enable-egress-firewall=true, enable-egress-qos=true, enable-egress-service=true, enable-multi-network=true, enable-admin-network-policy=true, and enable-multi-external-gateway=true to the expected config string for that test case so it matches the other updated cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/network/ovn_kubernetes_test.go`:
- Around line 1020-1024: The test case "enable network connect (DevPreview)" in
ovn_kubernetes_test.go has an outdated expected ConfigMap snippet; update its
expected output to include the unconditionally emitted flags now present in the
template: add enable-egress-ip=true, enable-egress-firewall=true,
enable-egress-qos=true, enable-egress-service=true, enable-multi-network=true,
enable-admin-network-policy=true, and enable-multi-external-gateway=true to the
expected config string for that test case so it matches the other updated cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 37e2203b-5661-4153-8807-475f50c6af4f
📒 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 with no reviewable changes (3)
- README.md
- docs/architecture.md
- docs/operands.md
🚧 Files skipped from review as they are similar to previous changes (4)
- bindata/network/ovn-kubernetes/managed/ovnkube-control-plane.yaml
- bindata/network/ovn-kubernetes/self-hosted/004-config.yaml
- bindata/network/ovn-kubernetes/common/008-script-lib.yaml
- docs/ovn_node_mode.md
|
/verified by pre-merge testing. |
openshift-ci-robot
added
the
verified
|
@yingwang-0320: 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. |
openshift-ci-robot
removed
the
verified
tsorya
force-pushed
the
igal/dpu-host-mode
branch
from
ed84508 to
526a5aa
Compare
|
/retest-required |
|
/retest-requires |
|
@tsorya hi, was there any QE testing done with this PR applied and automation/CI developed for the DPU-HOST mode? If so, would you mind pasting the testing result and CI links. I'd like to make sure we have a record for the testing on this PR and there is continuous CI job running to gate this functionality since we (core networking) currently don't have the necessary env nor CI to verify the piece of code for DPU functinoaliies. @yingwang-0320 Sicne this change touches the enablement/disablement of several OVN-K features, could you also run testing for each of the mentioned features and see if they are functional as expected? meanwhile, like @tssurya suggested, we would also want to try enable the DPU Host mode (without testing DPU specific functionalities) and scheck if the cluster is stable. |
|
@danwinship Would you mind taking a look at this PR once the testing is done? |
|
@tsorya we don't have any DPU-HOST mode related testing yet. Not sure what environment or card needed to enable DPU-host mode, let me confirm and update here. |
|
Confirmed with @tsorya , it's OK to verify CNO on regular mode. |
|
@tsorya: trigger 5 job(s) of type blocking for the ci release of OCP 4.22
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/c732cbd0-4347-11f1-8158-5f42053a853a-0 trigger 13 job(s) of type blocking for the nightly release of OCP 4.22
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/c732cbd0-4347-11f1-8158-5f42053a853a-1 |
|
/payload 4.22 ci blocking |
|
@tsorya: trigger 5 job(s) of type blocking for the ci release of OCP 4.22
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/7958f360-4385-11f1-8387-ec92c7662d69-0 trigger 13 job(s) of type blocking for the nightly release of OCP 4.22
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/7958f360-4385-11f1-8387-ec92c7662d69-1 |
|
/test periodic-ci-openshift-release-main-ci-4.22-e2e-azure-ovn-upgrade |
|
/test 4.22-upgrade-from-stable-4.21-e2e-azure-ovn-upgrade |
|
4.22-upgrade-from-stable-4.21-e2e-azure-ovn-upgrade
|
|
/payload 4.22 ci blocking |
|
@tsorya: trigger 5 job(s) of type blocking for the ci release of OCP 4.22
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/891a0760-44e5-11f1-9797-812882e74eb1-0 trigger 13 job(s) of type blocking for the nightly release of OCP 4.22
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/891a0760-44e5-11f1-9797-812882e74eb1-1 |
|
• periodic-ci-openshift-release-main-ci-4.22-e2e-aws-ovn-techpreview-serial-1of3: 401fc048-0800-4690-abe1-e83e251e8e7b �periodic-ci-openshift-release-main-ci-4.22-e2e-aws-ovn-techpreview-serial-2of3 Seems to be broken, thread around it |
|
Last run `* 0001-01-01T00:00:00Z 0x : 0/118 nodes are available: 1 node(s) had untolerated taint {node.kubernetes.io/disk-pressure: }, 1 node(s) had untolerated taint {node.kubernetes.io/not-ready: }, 11 node(s) had untolerated taint {node-role.kubernetes.io/ci-prowjobs-worker: ci-prowjobs-worker}, 12 node(s) didn't match Pod's node affinity/selector, 2 node(s) had untolerated taint {node-role.kubernetes.io/ci-builds-worker: ci-builds-worker}, 24 node(s) had untolerated taint {node-role.kubernetes.io/ci-longtests-worker: ci-longtests-worker}, 3 node(s) had untolerated taint {node-role.kubernetes.io/master: }, 58 Insufficient cpu, 6 node(s) had untolerated taint {node-role.kubernetes.io/infra: }. preemption: 0/118 nodes are available: 58 No preemption victims found for incoming pod, 60 Preemption is not helpful for scheduling.
|
|
/payload 4.22 ci blocking |
|
@tsorya: trigger 5 job(s) of type blocking for the ci release of OCP 4.22
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/5f245b10-457e-11f1-88b3-bbe09eda6f67-0 trigger 13 job(s) of type blocking for the nightly release of OCP 4.22
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/5f245b10-457e-11f1-88b3-bbe09eda6f67-1 |
|
/retest |
|
latest run was better periodic-ci-openshift-release-main-ci-4.22-upgrade-from-stable-4.21-e2e-gcp-ovn-rt-upgrade: e13d2307-1204-4495-81f9-9d0f27458b4b periodic-ci-openshift-release-main-ci-4.22-e2e-aws-ovn-techpreview-serial-1of3: dbd9ca29-0561-463d-811c-3e34e78a5978 event happened 22 times, something is wrong: namespace/openshift-config-operator node/ip-10-0-111-205.ec2.internal pod/openshift-config-operator-7cdbb85bd7-5n8md hmsg/440100f6ee - reason/ProbeError Liveness probe error: Get "https://10.129.0.30:8443/healthz": dial tcp 10.129.0.30:8443: connect: connection refused result=reject https://prow.ci.openshift.org/view/gs/test-platform-results/logs/openshift-cluster-network-operator-2944-ci-4.22-upgrade-from-stable-4.21-e2e-aws-ovn-upgrade/2050257487575126016 Don't see anything that can be somehow influenced by this PR |
|
/payload 4.22 ci blocking |
|
@tsorya: trigger 5 job(s) of type blocking for the ci release of OCP 4.22
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/2c0fff90-4650-11f1-98f7-ca3c66133201-0 trigger 13 job(s) of type blocking for the nightly release of OCP 4.22
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/2c0fff90-4650-11f1-98f7-ca3c66133201-1 |
|
periodic-ci-openshift-release-main-ci-4.22-e2e-aws-upgrade-ovn-single-node:
periodic-ci-openshift-release-main-ci-4.22-upgrade-from-stable-4.21-e2e-gcp-ovn-rt-upgrade
periodics-e2e-aws-ovn-conformance
|
|
/verified by @tsorya |
openshift-ci-robot
added
the
verified
|
@tsorya: 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. |
|
/verified by @tsorya |
|
@tsorya: 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. |
|
@tsorya: 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. |
|
/retest-required |
1 similar comment
|
/retest-required |
openshift-merge-bot
Bot
merged commit cb1b119
into
openshift:master
|
/cherrypick release-4.22 |
|
@tsorya: new pull request created: #2997 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. |
7 participants
Uh oh!
There was an error while loading. Please reload this page.