Add LACP bond health check for DPU deployments - #39
Open
szigmon wants to merge 1 commit into
Open
Conversation
szigmon
requested review from
hoberger-rh,
sarad-rh,
sprizend-rh and
tkarbach
as code owners
May 18, 2026 18:02
Contributor
|
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:
📝 WalkthroughWalkthroughAdds two DPF-focused network validators: DpuBondLacpHealth (checks LACP bond health from /proc/net/bonding) and OvnGeneveTunnelLocalIp (validates OVN Geneve local_ip vs node InternalIP). Integrates validators into NetworkValidationDomain, adds a ChangesDPF Network Validators
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/rules/network/test_dpf_validations.py (1)
121-136: ⚡ Quick winAdd a scenario for non-LACP bond mode being ignored.
BOND_ACTIVE_BACKUPis defined but never exercised. Please add a scenario asserting non-802.3ad bonds are ignored (and result is SKIP when only non-LACP bonds are present), matching the rule contract.As per coding guidelines,
Prefer comprehensive test coverage... including edge casesand verify that the rule validates the intended functionality.Also applies to: 198-218
🤖 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 `@tests/rules/network/test_dpf_validations.py` around lines 121 - 136, Add a new test scenario using the BOND_ACTIVE_BACKUP fixture in tests/rules/network/test_dpf_validations.py that exercises the rule when only non-802.3ad bonds exist and assert the rule returns SKIP; specifically, create a test (e.g., test_non_lacp_bonds_are_ignored) that feeds the BOND_ACTIVE_BACKUP string into the same test harness/validator used by the other cases and asserts the outcome is "SKIP" and no failures are reported, mirroring how existing scenarios are structured so the BOND_ACTIVE_BACKUP constant is actually exercised; apply the same pattern for the equivalent cases noted around the other section mentioned.
🤖 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 `@src/in_cluster_checks/rules/network/dpf_validations.py`:
- Around line 87-89: In run_rule, check the return code from
self.run_cmd(SafeCmdString("ls /proc/net/bonding/")) instead of ignoring rc; if
rc is non-zero, raise UnExpectedSystemOutput including the rc and the command
output (stdout/stderr) so failures are surfaced instead of falling through to
"No LACP (802.3ad) bonds found". Locate the call to run_cmd/SafeCmdString in
run_rule and add the conditional branch that raises UnExpectedSystemOutput with
a clear message and the captured output.
- Line 28: The rule currently defines an empty links list (links = []) and must
include its GitHub wiki URL; update the links variable in
src/in_cluster_checks/rules/network/dpf_validations.py by adding the rule's wiki
page string (e.g. "https://github.com/<org>/<repo>/wiki/<Rule-Name>") to the
list so links contains the wiki entry; ensure the string matches the project's
wiki URL pattern used by other rules.
- Around line 116-133: The code currently builds agg_ids = set(...) filtering
out falsy aggregator_id so missing IDs are ignored; update the logic in the
block handling agg_ids (around the variable agg_ids, info["slaves"], bond_name
and down_slaves) to first detect any slaves with missing or empty aggregator_id
and append an issue (e.g. "{bond_name}: missing aggregator_id on slaves ...") to
all_issues before performing the set-consistency check; this ensures a slave
lacking aggregator_id is treated as an error rather than being silently ignored
when others appear consistent.
---
Nitpick comments:
In `@tests/rules/network/test_dpf_validations.py`:
- Around line 121-136: Add a new test scenario using the BOND_ACTIVE_BACKUP
fixture in tests/rules/network/test_dpf_validations.py that exercises the rule
when only non-802.3ad bonds exist and assert the rule returns SKIP;
specifically, create a test (e.g., test_non_lacp_bonds_are_ignored) that feeds
the BOND_ACTIVE_BACKUP string into the same test harness/validator used by the
other cases and asserts the outcome is "SKIP" and no failures are reported,
mirroring how existing scenarios are structured so the BOND_ACTIVE_BACKUP
constant is actually exercised; apply the same pattern for the equivalent cases
noted around the other section mentioned.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 214b2be1-7bd3-455a-9536-3c6a9e33be85
📒 Files selected for processing (3)
src/in_cluster_checks/domains/network_domain.pysrc/in_cluster_checks/rules/network/dpf_validations.pytests/rules/network/test_dpf_validations.py
szigmon
force-pushed
the
dpf-lacp-bond-health
branch
from
May 18, 2026 19:03
e1bbf86 to
309360d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/rules/network/test_dpf_validations.py (1)
121-135: ⚡ Quick winAdd an explicit scenario asserting non-LACP bonds are skipped.
BOND_ACTIVE_BACKUPis defined, but there is no test that verifies the rule returns skip when only non-802.3ad bonds exist.As per coding guidelines,
Prefer comprehensive test coverage. Check that tests cover all meaningful scenarios including edge cases, partial state, empty output, and missing resources..Also applies to: 161-219
🤖 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 `@tests/rules/network/test_dpf_validations.py` around lines 121 - 135, Add a new unit test in tests/rules/network/test_dpf_validations.py that feeds the BOND_ACTIVE_BACKUP fixture into the same validation helper used by the other tests in this file and asserts the rule returns a skip/ignored result (i.e., non-802.3ad bonds should produce a skip). Locate where other scenarios call the rule/validator in this file and replicate that pattern (use the same test helper and assert the skip status) so the case "only non-LACP bonds exist" is explicitly covered.
🤖 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 `@src/in_cluster_checks/rules/network/dpf_validations.py`:
- Around line 179-187: The current loop incorrectly excludes lines containing
"geneve" and therefore can pick up local_ip values from non-Geneve interfaces;
update the condition so you only extract local_ip for Geneve interfaces by
requiring "geneve" in the line (e.g., replace the if "local_ip=" in line and
"geneve" not in line check with one that requires "local_ip=" and "geneve" in
line), then keep the existing per-part parse that extracts ip and appends to
local_ips (variables: stdout, local_ips, ip, and the "local_ip" token) so the
rule only validates Geneve interface local_ip values.
---
Nitpick comments:
In `@tests/rules/network/test_dpf_validations.py`:
- Around line 121-135: Add a new unit test in
tests/rules/network/test_dpf_validations.py that feeds the BOND_ACTIVE_BACKUP
fixture into the same validation helper used by the other tests in this file and
asserts the rule returns a skip/ignored result (i.e., non-802.3ad bonds should
produce a skip). Locate where other scenarios call the rule/validator in this
file and replicate that pattern (use the same test helper and assert the skip
status) so the case "only non-LACP bonds exist" is explicitly covered.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d02b31fc-f35d-4aa7-b2d7-c1ae8e50ca81
📒 Files selected for processing (4)
src/in_cluster_checks/domains/network_domain.pysrc/in_cluster_checks/rules/network/dpf_validations.pytests/domains/test_network_domain.pytests/rules/network/test_dpf_validations.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/in_cluster_checks/domains/network_domain.py
tkarbach
reviewed
May 19, 2026
Comment on lines
+5
to
+6
| Validates LACP bond health on DPU network interfaces to detect | ||
| degraded bond configurations that reduce available bandwidth. |
szigmon
force-pushed
the
dpf-lacp-bond-health
branch
from
May 19, 2026 09:58
309360d to
0f78ea6
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (5)
src/in_cluster_checks/rules/network/dpf_validations.py (5)
158-158:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the required wiki link to
links.This new rule has an empty
linkslist. As per coding guidelines, every new rule MUST have a corresponding wiki page in the GitHub wiki and the wiki URL must be added to the rule'slinksfield pointing to:https://github.com/RedHatInsights/incluster-checks/wiki/Network---Verify-OVN-Geneve-tunnel-local_ip-matches-node-InternalIP📝 Proposed fix
- links = [] + links = ["https://github.com/RedHatInsights/incluster-checks/wiki/Network---Verify-OVN-Geneve-tunnel-local_ip-matches-node-InternalIP"]🤖 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 `@src/in_cluster_checks/rules/network/dpf_validations.py` at line 158, The rule's links list is empty; update the links variable by adding the required GitHub wiki URL for this rule so the rule points to its documentation. Locate the links list (variable name links) in src/in_cluster_checks/rules/network/dpf_validations.py and append the URL https://github.com/RedHatInsights/incluster-checks/wiki/Network---Verify-OVN-Geneve-tunnel-local_ip-matches-node-InternalIP to that list (ensure it's a string entry in the existing list format used by this rule).
82-86:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
ls /proc/net/bonding/failure inrun_rule.The return code
rcis unpacked but never checked. If thelscommand fails (rc != 0), the code proceeds to parsestdoutwhich could contain an error message, leading to misreporting as "No LACP (802.3ad) bonds found" instead of surfacing the actual failure. As per coding guidelines, prefer raisingUnExpectedSystemOutputexception when a command produces unexpected output or fails.🛡️ Proposed fix
+from in_cluster_checks.utils.exceptions import UnExpectedSystemOutput + def run_rule(self) -> RuleResult: """Check LACP bond health on all bond interfaces.""" rc, stdout, _ = self.run_cmd(SafeCmdString("ls /proc/net/bonding/")) + if rc != 0: + raise UnExpectedSystemOutput(f"Failed to list bond interfaces: {stdout}") bond_names = stdout.strip().split()🤖 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 `@src/in_cluster_checks/rules/network/dpf_validations.py` around lines 82 - 86, In run_rule, check the return code from run_cmd(SafeCmdString("ls /proc/net/bonding/")) before parsing stdout: if rc != 0, raise UnExpectedSystemOutput including the command context and the stdout/stderr content so failures are surfaced instead of treating an error string as bond names; only proceed to split stdout into bond_names when rc == 0. Ensure you reference the run_rule method and the run_cmd/SafeCmdString call when adding the guard and raising the exception.
26-26:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the required wiki link to
links.This new rule has an empty
linkslist. As per coding guidelines, every new rule MUST have a corresponding wiki page in the GitHub wiki and the wiki URL must be added to the rule'slinksfield pointing to:https://github.com/RedHatInsights/incluster-checks/wiki/Network---Verify-LACP-bond-health-on-DPU-ports📝 Proposed fix
- links = [] + links = ["https://github.com/RedHatInsights/incluster-checks/wiki/Network---Verify-LACP-bond-health-on-DPU-ports"]🤖 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 `@src/in_cluster_checks/rules/network/dpf_validations.py` at line 26, The rule's links list is empty in dpf_validations.py; update the links variable (links = []) to include the required wiki URL for this rule by adding "https://github.com/RedHatInsights/incluster-checks/wiki/Network---Verify-LACP-bond-health-on-DPU-ports" so the links list contains that single string entry (ensure the variable name links in the module remains unchanged).
183-211:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftScope
local_ipextraction strictly to Geneve interfaces.The regex
_OVS_LOCAL_IP_RE.findall(ovs_output)at line 189 matches anylocal_ip="..."pattern anywhere in the OVS output. If other interface types (e.g., vxlan, gre) also havelocal_ipoptions, those would be incorrectly included in the validation, producing false failures or false passes. The prerequisite only checks that "geneve" appears somewhere in the output, not that extractedlocal_ipvalues belong to geneve interfaces.🔍 Proposed fix to parse geneve interfaces correctly
def run_rule(self) -> RuleResult: """Check that Geneve tunnel local_ip matches node InternalIP.""" ovs_output = self._get_ovs_show() if not ovs_output: return RuleResult.skip("Cannot read OVS state") - local_ips = list(set(self._OVS_LOCAL_IP_RE.findall(ovs_output))) + local_ips: List[str] = [] + in_geneve_interface = False + for line in ovs_output.splitlines(): + line = line.strip() + if line.startswith("type:"): + in_geneve_interface = line.split(":", 1)[1].strip() == "geneve" + elif in_geneve_interface and line.startswith("options:"): + matches = self._OVS_LOCAL_IP_RE.findall(line) + for ip in matches: + if ip and ip not in local_ips: + local_ips.append(ip) + in_geneve_interface = False + if not local_ips: return RuleResult.skip("No Geneve local_ip found in OVS")🤖 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 `@src/in_cluster_checks/rules/network/dpf_validations.py` around lines 183 - 211, The current run_rule uses _OVS_LOCAL_IP_RE.findall(ovs_output) which picks up any local_ip="..." anywhere in the OVS dump; restrict extraction to only Geneve interfaces by first locating the Geneve interface blocks/lines in the output (e.g., lines/stanzas that contain "type=geneve" or an interface name/type that identifies geneve) from _get_ovs_show(), then apply the local_ip regex only within those geneve-specific blocks to build local_ips; update the logic around _OVS_LOCAL_IP_RE, local_ips and geneve_local_ip accordingly so only Geneve local_ip values are considered for the comparison with _get_node_ip().
113-131:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle missing aggregator IDs as a bond issue.
The aggregator ID check at line 113 filters out empty
aggregator_idvalues usingif s["aggregator_id"], so slaves with missing aggregator IDs are silently ignored. Then at line 129, the healthy check useslen(agg_ids) <= 1, which passes whenagg_idsis empty (all IDs missing) or contains a single ID. This incorrectly marks a bond as healthy when aggregator IDs are missing. Missing aggregator ID should be treated as a bond issue before the consistency check.🛡️ Proposed fix
down_slaves = [s for s in info["slaves"] if s["mii_status"] != "up"] if down_slaves: names = ", ".join(s["name"] for s in down_slaves) all_issues.append(f"{bond_name}: slave(s) down: {names}") + missing_agg = [s["name"] for s in info["slaves"] if not s["aggregator_id"]] + if missing_agg: + all_issues.append( + f"{bond_name}: missing aggregator ID for slave(s): {', '.join(missing_agg)}" + ) + agg_ids = set(s["aggregator_id"] for s in info["slaves"] if s["aggregator_id"]) if len(agg_ids) > 1: details = ", ".join(f"{s['name']}=agg{s['aggregator_id']}" for s in info["slaves"]) all_issues.append( f"{bond_name}: slaves in different aggregators ({details}), LACP not fully negotiated" ) churned: List[str] = [] for s in info["slaves"]: if s["actor_churn"] and s["actor_churn"] != "none": churned.append(f"{s['name']} actor={s['actor_churn']}") if s["partner_churn"] and s["partner_churn"] != "none": churned.append(f"{s['name']} partner={s['partner_churn']}") if churned: all_issues.append(f"{bond_name}: LACP churn detected: {', '.join(churned)}") - if not down_slaves and len(agg_ids) <= 1 and not churned: + if not down_slaves and not missing_agg and len(agg_ids) == 1 and not churned: slave_info = ", ".join(f"{s['name']} ({s['speed']})" for s in info["slaves"]) all_passed.append(f"{bond_name}: LACP healthy, {len(info['slaves'])} slaves UP ({slave_info})")🤖 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 `@src/in_cluster_checks/rules/network/dpf_validations.py` around lines 113 - 131, The code currently builds agg_ids with set(s["aggregator_id"] for s in info["slaves"] if s["aggregator_id"]) which ignores missing/empty aggregator IDs and can make agg_ids empty (wrongly marking the bond healthy); update validation to first detect any slave where s["aggregator_id"] is missing/empty and append an all_issues entry referencing bond_name and those slave names (e.g. "missing aggregator_id on ..."), then build agg_ids from all non-empty IDs and change the healthy condition (the final if that appends to all_passed) to require that at least one aggregator ID exists and is consistent (e.g. agg_ids and len(agg_ids) <= 1) so bonds with all-missing IDs are not considered healthy; refer to symbols agg_ids, info["slaves"], churned, down_slaves, and bond_name when making these checks and messages.
🧹 Nitpick comments (2)
tests/rules/network/test_dpf_validations.py (2)
138-219: ⚡ Quick winConsider adding test coverage for non-LACP bond skip scenario.
The
BOND_ACTIVE_BACKUPfixture is defined but not used in any test scenario. Consider adding a test case toscenario_passedor a new skip scenario list to verify that the rule correctly skips when only non-LACP bonds (e.g., active-backup) are present. This would exercise the skip path at lines 97-98 and 134 of the rule implementation.💡 Suggested additional test scenario
scenario_passed = [ RuleScenarioParams( scenario_title="lacp_bond_healthy_two_slaves", cmd_input_output_dict={ "ls /proc/net/bonding/": CmdOutput("bond0"), "cat /proc/net/bonding/bond0": CmdOutput(BOND_LACP_HEALTHY), }, ), ] + scenario_skip = [ + RuleScenarioParams( + scenario_title="only_non_lacp_bonds", + cmd_input_output_dict={ + "ls /proc/net/bonding/": CmdOutput("bond0"), + "cat /proc/net/bonding/bond0": CmdOutput(BOND_ACTIVE_BACKUP), + }, + ), + ] + scenario_failed = [Then add the corresponding test method:
`@pytest.mark.parametrize`("scenario_params", scenario_skip) def test_scenario_skip(self, scenario_params, tested_object): RuleTestBase.test_scenario_skip(self, scenario_params, tested_object)🤖 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 `@tests/rules/network/test_dpf_validations.py` around lines 138 - 219, Add a test that exercises the non‑LACP (active-backup) skip path for DpuBondLacpHealth by creating a new scenario list (e.g., scenario_skip) that uses the BOND_ACTIVE_BACKUP fixture in cmd_input_output_dict and expected to be skipped, then add a pytest param test method def test_scenario_skip(self, scenario_params, tested_object): that calls RuleTestBase.test_scenario_skip(self, scenario_params, tested_object). Ensure the new scenario references BOND_ACTIVE_BACKUP and that the test name (test_scenario_skip) and scenario list (scenario_skip) match exactly so the rule’s skip branch is covered.
246-314: ⚡ Quick winAdd test coverage for multiple local_ip and missing node_ip scenarios.
The test suite is missing coverage for two scenarios handled by the rule:
Multiple different local_ip values (lines 197-201 in rule): When geneve tunnels have inconsistent
local_ipvalues, the rule returns a failed result with a specific error message. This scenario is not tested.Cannot determine node primary IP (lines 194-195 in rule): When
/run/nodeip-configuration/primary-ipcannot be read, the rule returns skip. This scenario is not tested.💡 Suggested additional test scenarios
Add a fixture for multiple local_ip values:
OVS_SHOW_MULTIPLE_LOCAL_IPS = """Bridge br-int Port ovn-abc123-0 Interface ovn-abc123-0 type: geneve options: {csum="true", key=flow, local_ip="10.6.135.202", remote_ip="10.6.135.236"} Port ovn-def456-0 Interface ovn-def456-0 type: geneve options: {csum="true", key=flow, local_ip="10.6.135.1", remote_ip="10.6.135.225"} """Add to
scenario_failed:RuleScenarioParams( scenario_title="multiple_different_local_ips", cmd_input_output_dict={ "ovs-vsctl show": CmdOutput(OVS_SHOW_MULTIPLE_LOCAL_IPS), "cat /run/nodeip-configuration/primary-ip": CmdOutput("10.6.135.202"), }, failed_msg=( "Multiple different local_ip values in Geneve tunnels: ['10.6.135.1', '10.6.135.202']. " "OVS configuration may be inconsistent." ), ),Add a skip scenario list:
scenario_skip = [ RuleScenarioParams( scenario_title="cannot_read_node_ip", cmd_input_output_dict={ "ovs-vsctl show": CmdOutput(OVS_SHOW_WITH_GENEVE), "cat /run/nodeip-configuration/primary-ip": CmdOutput("", return_code=1), }, ), ]Then add the corresponding test method:
`@pytest.mark.parametrize`("scenario_params", scenario_skip) def test_scenario_skip(self, scenario_params, tested_object): RuleTestBase.test_scenario_skip(self, scenario_params, tested_object)🤖 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 `@tests/rules/network/test_dpf_validations.py` around lines 246 - 314, Tests for TestOvnGeneveTunnelLocalIp are missing two scenarios: inconsistent Geneve local_ip values and unreadable node primary IP; add a new fixture constant (e.g., OVS_SHOW_MULTIPLE_LOCAL_IPS) representing two different local_ip values and add a RuleScenarioParams to scenario_failed (title "multiple_different_local_ips") using that fixture plus a primary-ip that matches one of them and the expected failed_msg matching the rule's message; also add a scenario_skip list with a RuleScenarioParams (title "cannot_read_node_ip") where "ovs-vsctl show" uses OVS_SHOW_WITH_GENEVE and the "cat /run/nodeip-configuration/primary-ip" CmdOutput has return_code=1, then add a test method test_scenario_skip decorated with pytest.mark.parametrize over scenario_skip that calls RuleTestBase.test_scenario_skip(self, scenario_params, tested_object); reference TestOvnGeneveTunnelLocalIp, scenario_failed, scenario_skip, OVS_SHOW_WITH_GENEVE, OVS_SHOW_MULTIPLE_LOCAL_IPS, RuleScenarioParams, CmdOutput, and test_scenario_skip to locate where to insert these.
🤖 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.
Duplicate comments:
In `@src/in_cluster_checks/rules/network/dpf_validations.py`:
- Line 158: The rule's links list is empty; update the links variable by adding
the required GitHub wiki URL for this rule so the rule points to its
documentation. Locate the links list (variable name links) in
src/in_cluster_checks/rules/network/dpf_validations.py and append the URL
https://github.com/RedHatInsights/incluster-checks/wiki/Network---Verify-OVN-Geneve-tunnel-local_ip-matches-node-InternalIP
to that list (ensure it's a string entry in the existing list format used by
this rule).
- Around line 82-86: In run_rule, check the return code from
run_cmd(SafeCmdString("ls /proc/net/bonding/")) before parsing stdout: if rc !=
0, raise UnExpectedSystemOutput including the command context and the
stdout/stderr content so failures are surfaced instead of treating an error
string as bond names; only proceed to split stdout into bond_names when rc == 0.
Ensure you reference the run_rule method and the run_cmd/SafeCmdString call when
adding the guard and raising the exception.
- Line 26: The rule's links list is empty in dpf_validations.py; update the
links variable (links = []) to include the required wiki URL for this rule by
adding
"https://github.com/RedHatInsights/incluster-checks/wiki/Network---Verify-LACP-bond-health-on-DPU-ports"
so the links list contains that single string entry (ensure the variable name
links in the module remains unchanged).
- Around line 183-211: The current run_rule uses
_OVS_LOCAL_IP_RE.findall(ovs_output) which picks up any local_ip="..." anywhere
in the OVS dump; restrict extraction to only Geneve interfaces by first locating
the Geneve interface blocks/lines in the output (e.g., lines/stanzas that
contain "type=geneve" or an interface name/type that identifies geneve) from
_get_ovs_show(), then apply the local_ip regex only within those geneve-specific
blocks to build local_ips; update the logic around _OVS_LOCAL_IP_RE, local_ips
and geneve_local_ip accordingly so only Geneve local_ip values are considered
for the comparison with _get_node_ip().
- Around line 113-131: The code currently builds agg_ids with
set(s["aggregator_id"] for s in info["slaves"] if s["aggregator_id"]) which
ignores missing/empty aggregator IDs and can make agg_ids empty (wrongly marking
the bond healthy); update validation to first detect any slave where
s["aggregator_id"] is missing/empty and append an all_issues entry referencing
bond_name and those slave names (e.g. "missing aggregator_id on ..."), then
build agg_ids from all non-empty IDs and change the healthy condition (the final
if that appends to all_passed) to require that at least one aggregator ID exists
and is consistent (e.g. agg_ids and len(agg_ids) <= 1) so bonds with all-missing
IDs are not considered healthy; refer to symbols agg_ids, info["slaves"],
churned, down_slaves, and bond_name when making these checks and messages.
---
Nitpick comments:
In `@tests/rules/network/test_dpf_validations.py`:
- Around line 138-219: Add a test that exercises the non‑LACP (active-backup)
skip path for DpuBondLacpHealth by creating a new scenario list (e.g.,
scenario_skip) that uses the BOND_ACTIVE_BACKUP fixture in cmd_input_output_dict
and expected to be skipped, then add a pytest param test method def
test_scenario_skip(self, scenario_params, tested_object): that calls
RuleTestBase.test_scenario_skip(self, scenario_params, tested_object). Ensure
the new scenario references BOND_ACTIVE_BACKUP and that the test name
(test_scenario_skip) and scenario list (scenario_skip) match exactly so the
rule’s skip branch is covered.
- Around line 246-314: Tests for TestOvnGeneveTunnelLocalIp are missing two
scenarios: inconsistent Geneve local_ip values and unreadable node primary IP;
add a new fixture constant (e.g., OVS_SHOW_MULTIPLE_LOCAL_IPS) representing two
different local_ip values and add a RuleScenarioParams to scenario_failed (title
"multiple_different_local_ips") using that fixture plus a primary-ip that
matches one of them and the expected failed_msg matching the rule's message;
also add a scenario_skip list with a RuleScenarioParams (title
"cannot_read_node_ip") where "ovs-vsctl show" uses OVS_SHOW_WITH_GENEVE and the
"cat /run/nodeip-configuration/primary-ip" CmdOutput has return_code=1, then add
a test method test_scenario_skip decorated with pytest.mark.parametrize over
scenario_skip that calls RuleTestBase.test_scenario_skip(self, scenario_params,
tested_object); reference TestOvnGeneveTunnelLocalIp, scenario_failed,
scenario_skip, OVS_SHOW_WITH_GENEVE, OVS_SHOW_MULTIPLE_LOCAL_IPS,
RuleScenarioParams, CmdOutput, and test_scenario_skip to locate where to insert
these.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6174766b-3a4c-4acb-ac15-126d520afd59
📒 Files selected for processing (4)
src/in_cluster_checks/domains/network_domain.pysrc/in_cluster_checks/rules/network/dpf_validations.pytests/domains/test_network_domain.pytests/rules/network/test_dpf_validations.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/domains/test_network_domain.py
- src/in_cluster_checks/domains/network_domain.py
szigmon
force-pushed
the
dpf-lacp-bond-health
branch
from
May 19, 2026 10:04
0f78ea6 to
b27c66f
Compare
tkarbach
reviewed
May 19, 2026
| if ( | ||
| not down_slaves | ||
| and not missing_agg | ||
| and len(set(s["aggregator_id"] for s in info["slaves"])) <= 1 |
Collaborator
There was a problem hiding this comment.
isn't that problematic if it's < 1 (meaning it's 0)?
it will be the same as missing_agg has a value
tkarbach
reviewed
May 19, 2026
| objective_hosts = [Objectives.ALL_NODES] | ||
| unique_name = "dpu_bond_lacp_health" | ||
| title = "Verify LACP bond health on DPU ports" | ||
| links = ["https://github.com/RedHatInsights/incluster-checks/wiki/dpu_bond_lacp_health"] |
Collaborator
There was a problem hiding this comment.
need to update this wiki with validation info
sprizend-rh
reviewed
May 19, 2026
tkarbach
reviewed
May 19, 2026
| objective_hosts = [Objectives.ALL_NODES] | ||
| unique_name = "ovn_geneve_tunnel_local_ip" | ||
| title = "Verify OVN Geneve tunnel local_ip matches node InternalIP" | ||
| links = ["https://github.com/RedHatInsights/incluster-checks/wiki/ovn_geneve_tunnel_local_ip"] |
Collaborator
There was a problem hiding this comment.
need to update this wiki with validation info
tkarbach
reviewed
May 19, 2026
Comment on lines
+204
to
+205
| if not ovs_output: | ||
| return RuleResult.skip("Cannot read OVS state") |
Collaborator
There was a problem hiding this comment.
can be deleted as validation won't run as it covered on is_prerequisite_fulfilled
tkarbach
reviewed
May 19, 2026
Comment on lines
+207
to
+209
| local_ips = list(set(self._OVS_LOCAL_IP_RE.findall(ovs_output))) | ||
| if not local_ips: | ||
| return RuleResult.skip("No Geneve local_ip found in OVS") |
Collaborator
There was a problem hiding this comment.
isn't that covered also as part of is_prerequisite_fulfilled on line 190?
szigmon
force-pushed
the
dpf-lacp-bond-health
branch
from
May 19, 2026 12:11
b27c66f to
a2f6e26
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/in_cluster_checks/rules/network/dpf_validations.py (2)
178-178:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope
local_ipextraction to Geneve interfaces only.Line 207 currently extracts every
local_ipinovs-vsctl show, including non-Geneve interfaces. This can report false failures/passes for this rule’s intent.Suggested diff
- _OVS_LOCAL_IP_RE = re.compile(r'local_ip="([^"]+)"') + _OVS_LOCAL_IP_RE = re.compile(r'local_ip="([^"]+)"') @@ - local_ips = list(set(self._OVS_LOCAL_IP_RE.findall(ovs_output))) + local_ips: List[str] = [] + in_geneve_interface = False + for raw_line in ovs_output.splitlines(): + line = raw_line.strip() + if line.startswith("type:"): + in_geneve_interface = line.split(":", 1)[1].strip() == "geneve" + continue + if in_geneve_interface and line.startswith("options:"): + match = self._OVS_LOCAL_IP_RE.search(line) + if match: + local_ips.append(match.group(1)) + + local_ips = sorted(set(local_ips))As per coding guidelines,
Verify the rule actually validates functionality, not just connectivity or presence.Also applies to: 207-209
🤖 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 `@src/in_cluster_checks/rules/network/dpf_validations.py` at line 178, The current _OVS_LOCAL_IP_RE captures any local_ip in ovs-vsctl output; restrict it to Geneve interfaces only by changing the pattern used in _OVS_LOCAL_IP_RE (and the code that uses it at the local_ip extraction site around lines 207-209) so it only matches a local_ip that belongs to a Geneve interface (e.g., require the interface/type context such as an interface name starting with "geneve" or a preceding/type field indicating "geneve" in the same stanza). Update the regex and extraction logic (referencing _OVS_LOCAL_IP_RE) so non-Geneve interfaces are ignored.
24-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
supported_profilesfor DPF-scoped rules.These validators are introduced as DPF-focused checks, but both classes currently execute for all profiles. Please declare
supported_profiles = {"gpu"}to avoid non-target profile noise/failures.Suggested diff
class DpuBondLacpHealth(Rule): @@ objective_hosts = [Objectives.ALL_NODES] + supported_profiles = {"gpu"} unique_name = "dpu_bond_lacp_health" @@ class OvnGeneveTunnelLocalIp(Rule): @@ objective_hosts = [Objectives.ALL_NODES] + supported_profiles = {"gpu"} unique_name = "ovn_geneve_tunnel_local_ip"As per coding guidelines,
If a rule is profile-specific, it must declare supported_profiles = {"profile-name"}.Also applies to: 173-176
🤖 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 `@src/in_cluster_checks/rules/network/dpf_validations.py` around lines 24 - 27, Add the class attribute supported_profiles = {"gpu"} to the rule class that defines objective_hosts, unique_name = "dpu_bond_lacp_health", title = "Verify LACP bond health on DPU ports" so it only runs for the GPU/DPF profile; also add the same supported_profiles = {"gpu"} to the other DPF-scoped rule class referenced around lines 173-176 (the other rule with similar objective/unique_name) so both validators are limited to the "gpu" profile.
🤖 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 `@tests/rules/network/test_dpf_validations.py`:
- Around line 318-325: The skip scenario `_scenario_skip_node_ip` is defined but
not executed; update the test harness that collects/iterates scenarios (the list
or param for the test that calls `run_rule()`, e.g., RULE_SCENARIOS or the
parametrized test function) to include `_scenario_skip_node_ip` so the
missing-node-IP SKIP path is exercised; likewise add the analogous scenario
defined around lines 353-367 into the same scenarios list/param set so both
edge-case SKIP paths are covered by the test that calls `run_rule()`.
---
Duplicate comments:
In `@src/in_cluster_checks/rules/network/dpf_validations.py`:
- Line 178: The current _OVS_LOCAL_IP_RE captures any local_ip in ovs-vsctl
output; restrict it to Geneve interfaces only by changing the pattern used in
_OVS_LOCAL_IP_RE (and the code that uses it at the local_ip extraction site
around lines 207-209) so it only matches a local_ip that belongs to a Geneve
interface (e.g., require the interface/type context such as an interface name
starting with "geneve" or a preceding/type field indicating "geneve" in the same
stanza). Update the regex and extraction logic (referencing _OVS_LOCAL_IP_RE) so
non-Geneve interfaces are ignored.
- Around line 24-27: Add the class attribute supported_profiles = {"gpu"} to the
rule class that defines objective_hosts, unique_name = "dpu_bond_lacp_health",
title = "Verify LACP bond health on DPU ports" so it only runs for the GPU/DPF
profile; also add the same supported_profiles = {"gpu"} to the other DPF-scoped
rule class referenced around lines 173-176 (the other rule with similar
objective/unique_name) so both validators are limited to the "gpu" profile.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 94e26658-9dc3-4fcc-9e67-d96bc4d11886
📒 Files selected for processing (4)
src/in_cluster_checks/domains/network_domain.pysrc/in_cluster_checks/rules/network/dpf_validations.pytests/domains/test_network_domain.pytests/rules/network/test_dpf_validations.py
✅ Files skipped from review due to trivial changes (1)
- tests/domains/test_network_domain.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/in_cluster_checks/domains/network_domain.py
szigmon
force-pushed
the
dpf-lacp-bond-health
branch
5 times, most recently
from
May 19, 2026 17:28
08f078b to
d7ad2e3
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/rules/network/test_dpf_validations.py (1)
213-213: ⚡ Quick winMove
Statusimport to module scope.Please avoid imports inside test methods; import
Statusonce at the top-level.Proposed fix
import pytest from in_cluster_checks.rules.network.dpf_validations import DpuBondLacpHealth, OvnGeneveTunnelLocalIp +from in_cluster_checks.utils.enums import Status from tests.pytest_tools.test_operator_base import CmdOutput from tests.pytest_tools.test_rule_base import RuleScenarioParams, RuleTestBase @@ def test_scenario_skip(self, scenario_params, tested_object): """Test that non-LACP bonds result in SKIP.""" - from in_cluster_checks.utils.enums import Status - self._init_validation_object(tested_object, scenario_params) @@ def test_scenario_skip_geneve(self, scenario_params, tested_object): """Test that missing node IP results in SKIP.""" - from in_cluster_checks.utils.enums import Status - self._init_validation_object(tested_object, scenario_params)As per coding guidelines,
Always place all imports at the top of the file; never add imports in the middle of functions or methods.Also applies to: 382-382
🤖 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 `@tests/rules/network/test_dpf_validations.py` at line 213, The inline imports of Status (from in_cluster_checks.utils.enums import Status) inside test functions should be moved to module scope: add a single top-level import "from in_cluster_checks.utils.enums import Status" alongside the other imports at the top of tests/rules/network/test_dpf_validations.py, then remove the duplicate inline imports inside the test methods (the ones currently importing Status). Ensure all test functions that reference Status use the module-level name.
🤖 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 `@src/in_cluster_checks/rules/network/dpf_validations.py`:
- Around line 224-237: The code in run_rule calls self._get_ovs_show() and then
indexes local_ips[0] without guarding for ovs_output being None or local_ips
being empty; update the logic in run_rule (around calls to _get_ovs_show and
_extract_geneve_local_ips) to first check if ovs_output is falsy and if so raise
UnExpectedSystemOutput with a clear message, then compute local_ips and if
local_ips is empty return RuleResult.failed (or raise UnExpectedSystemOutput per
guideline) instead of indexing; ensure you reference the _get_ovs_show,
_extract_geneve_local_ips, and geneve_local_ip usage and use
UnExpectedSystemOutput from core/exceptions to represent command failure cases.
---
Nitpick comments:
In `@tests/rules/network/test_dpf_validations.py`:
- Line 213: The inline imports of Status (from in_cluster_checks.utils.enums
import Status) inside test functions should be moved to module scope: add a
single top-level import "from in_cluster_checks.utils.enums import Status"
alongside the other imports at the top of
tests/rules/network/test_dpf_validations.py, then remove the duplicate inline
imports inside the test methods (the ones currently importing Status). Ensure
all test functions that reference Status use the module-level name.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: edc957bf-6d47-41d4-a7f8-d2fc50fc71af
📒 Files selected for processing (5)
src/in_cluster_checks/domains/network_domain.pysrc/in_cluster_checks/rules/network/dpf_validations.pysrc/profiles/profiles.yamltests/domains/test_network_domain.pytests/rules/network/test_dpf_validations.py
✅ Files skipped from review due to trivial changes (1)
- src/profiles/profiles.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/domains/test_network_domain.py
- src/in_cluster_checks/domains/network_domain.py
szigmon
force-pushed
the
dpf-lacp-bond-health
branch
4 times, most recently
from
May 20, 2026 08:35
8276327 to
240b955
Compare
hoberger-rh
reviewed
May 20, 2026
| if not local_ips: | ||
| return RuleResult.skip("No Geneve local_ip found in OVS output") | ||
|
|
||
| node_ip = self._get_node_ip() |
Collaborator
There was a problem hiding this comment.
Why does _get_node_ip() read /run/nodeip-configuration/primary-ip instead of using self.get_host_ip() (Kubernetes InternalIP)?
hoberger-rh
reviewed
May 20, 2026
| def run_rule(self) -> RuleResult: | ||
| """Check that Geneve tunnel local_ip matches node InternalIP.""" | ||
| ovs_output = self._get_ovs_show() | ||
| if ovs_output is None: |
Collaborator
There was a problem hiding this comment.
Since the prerequisite already verified that _get_ovs_show() returns non-None output, this check is redundant.
Suggested refactor:
def _get_ovs_show(self) -> str:
"""Run ovs-vsctl show and return stdout."""
return self.get_output_from_run_cmd(SafeCmdString("ovs-vsctl show"))
def is_prerequisite_fulfilled(self) -> PrerequisiteResult:
try:
stdout = self._get_ovs_show()
except UnExpectedSystemOutput:
return PrerequisiteResult.not_met("Cannot access OVS")
if "geneve" not in stdout:
return PrerequisiteResult.not_met("No Geneve tunnels configured")
return PrerequisiteResult.met()
def run_rule(self) -> RuleResult:
ovs_output = self._get_ovs_show() # Raises UnExpectedSystemOutput if fails
tunnel_info = self._extract_geneve_tunnel_info(ovs_output)
# ...
sprizend-rh
reviewed
May 25, 2026
| supported_profiles = {"dpf"} | ||
| unique_name = "dpu_bond_lacp_health" | ||
| title = "Verify LACP bond health on DPU ports" | ||
| links = ["https://github.com/RedHatInsights/incluster-checks/wiki/DPF---LACP-bond-health-on-DPU-ports"] |
| supported_profiles = {"dpf"} | ||
| unique_name = "ovn_geneve_tunnel_local_ip" | ||
| title = "Verify OVN Geneve tunnel local_ip matches node InternalIP" | ||
| links = ["https://github.com/RedHatInsights/incluster-checks/wiki/DPF---OVN-Geneve-tunnel-local_ip"] |
Collaborator
There was a problem hiding this comment.
this isnt a valid wiki url
Collaborator
|
Hi @szigmon! 👋 I've made some improvements to your PR that would enhance code quality and robustness. The changes include:
All 134 network tests pass ✅ To incorporate these changes, you can pull from my fork: git remote add tkarbach https://github.com/tkarbach/incluster-checks.git
git fetch tkarbach pr-39
git reset --hard tkarbach/pr-39
git push origin dpf-lacp-bond-health --force-with-leaseAlternatively, you can review the changes at #65 and cherry-pick what you'd like. Let me know if you'd prefer a different approach! |
szigmon
force-pushed
the
dpf-lacp-bond-health
branch
from
June 9, 2026 11:13
240b955 to
89e429c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #39 +/- ##
=======================================
Coverage ? 86.79%
=======================================
Files ? 59
Lines ? 6756
Branches ? 0
=======================================
Hits ? 5864
Misses ? 892
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
szigmon
force-pushed
the
dpf-lacp-bond-health
branch
from
June 11, 2026 09:55
71dc2c4 to
ac39695
Compare
Author
|
Tested on OCP 4.22 cluster (3 VM nodes, no DPU hardware):
Not tested: No bare metal nodes with BlueField DPUs available — |
szigmon
force-pushed
the
dpf-lacp-bond-health
branch
from
August 9, 2026 13:39
9b6de8f to
9bf4049
Compare
Add two new rules for OpenShift clusters with NVIDIA BlueField DPUs: - DpuBondLacpHealth: verifies LACP bond health on DPU ports - OvnGeneveTunnelLocalIp: verifies Geneve tunnel local_ip matches node IP Includes BondBase extraction, dpf profile, and full test coverage. Assisted-by: Claude Code (Claude Opus 4.6) <noreply@anthropic.com>
szigmon
force-pushed
the
dpf-lacp-bond-health
branch
from
August 9, 2026 15:55
9bf4049 to
c634fa6
Compare
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
5 participants
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.
Summary
DpuBondLacpHealthrule to validate 802.3ad (LACP) bond health on nodes with NVIDIA BlueField DPUsMotivation
On OpenShift clusters with NVIDIA BlueField DPUs, worker nodes use an LACP bond (802.3ad) across the two DPU ports as the primary network uplink. A degraded bond — where one port goes down, LACP negotiation fails, or ports land in different aggregators — causes traffic to flow through a single port, silently halving available bandwidth. The node remains
Readyand no existing check detects this condition.What the check validates
upThe rule runs on all nodes, skips nodes without bond interfaces, and skips non-LACP bond modes (e.g. active-backup).
Test plan
Summary by CodeRabbit
New Features
Tests
Profiles