refactor: extract BondBase class and improve DPF validation robustness - #65
refactor: extract BondBase class and improve DPF validation robustness#65tkarbach wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR adds two new network validation rules ( ChangesDPF Network Validation Rules
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #65 +/- ##
=======================================
Coverage ? 87.64%
=======================================
Files ? 56
Lines ? 6029
Branches ? 0
=======================================
Hits ? 5284
Misses ? 745
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (1)
src/in_cluster_checks/rules/network/dpf_validations.py (1)
161-164: 💤 Low valueRemove redundant aggregator ID computation.
The
agg_idsset is already computed at line 144 and verified to have consistent values (len == 1 after handling missing_agg and mismatched cases). Re-computing it here on line 161 is unnecessary.♻️ Simplify by removing redundant computation
if not down_slaves and not missing_agg and not churned: - agg_ids = set(s["aggregator_id"] for s in info["slaves"]) - if len(agg_ids) == 1: - 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})") + 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})")Rationale: By the time we reach this block, we've already filtered out bonds with missing or inconsistent aggregator IDs in lines 138-149, so we know all slaves have the same aggregator_id.
🤖 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 161 - 164, Remove the redundant recomputation of agg_ids inside the block that builds slave_info: instead of re-evaluating agg_ids = set(s["aggregator_id"] for s in info["slaves"]), reuse the previously computed agg_ids variable (which has already been validated for consistency) and keep the existing conditional check and message construction using info, slaves, bond_name, and all_passed; delete the redundant set(...) line so the block simply checks len(agg_ids) == 1 and builds slave_info as before.
🤖 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`:
- Line 7: Remove the unused "stdout" import from sys at the top of the module to
avoid shadowing the local variable named stdout used later; locate the import
statement "from sys import stdout" and delete it, then run a quick search in the
file for references to stdout to confirm only the local variable (not the
removed import) is used and that no other code relies on the module-level
symbol.
- Around line 194-199: The try/except in _get_ovs_show currently catches all
Exception; change it to catch only UnExpectedSystemOutput (the specific
exception raised by get_output_from_run_cmd) and return None in that except
block so we avoid broad exception swallowing; locate the _get_ovs_show method
and replace "except Exception:" with "except UnExpectedSystemOutput:" and ensure
the UnExpectedSystemOutput symbol is imported or referenced correctly.
- Line 27: The DPF-focused rule class DpuBondLacpHealth currently sets
supported_profiles = {"gpu"}; update the supported_profiles attribute on the
DpuBondLacpHealth class to {"dpf"} so it matches the module intent (same pattern
as OvnGeneveTunnelLocalIp) and ensures the rule applies to the correct profile.
In `@src/profiles/profiles.yaml`:
- Around line 15-16: The dpf profile currently only includes "general" so DPF
validators gated by supported_profiles = {"gpu"} (e.g., DpuBondLacpHealth) stay
disabled; update the profiles YAML so the dpf profile includes "gpu" (either add
gpu to the include list for the dpf profile or ensure a transitive include that
brings in gpu) so that validators using supported_profiles = {"gpu"} are enabled
when active_profile=dpf.
---
Nitpick comments:
In `@src/in_cluster_checks/rules/network/dpf_validations.py`:
- Around line 161-164: Remove the redundant recomputation of agg_ids inside the
block that builds slave_info: instead of re-evaluating agg_ids =
set(s["aggregator_id"] for s in info["slaves"]), reuse the previously computed
agg_ids variable (which has already been validated for consistency) and keep the
existing conditional check and message construction using info, slaves,
bond_name, and all_passed; delete the redundant set(...) line so the block
simply checks len(agg_ids) == 1 and builds slave_info as before.
🪄 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: 3d51bcaf-3e06-41b0-8f7c-8c604321f743
📒 Files selected for processing (7)
src/in_cluster_checks/domains/network_domain.pysrc/in_cluster_checks/rules/network/bond_base.pysrc/in_cluster_checks/rules/network/dpf_validations.pysrc/in_cluster_checks/rules/network/node_connectivity_validations.pysrc/profiles/profiles.yamltests/domains/test_network_domain.pytests/rules/network/test_dpf_validations.py
| """ | ||
|
|
||
| import re | ||
| from sys import stdout |
There was a problem hiding this comment.
Remove unused import.
The stdout import from sys is unused and also causes a shadowing issue at line 227 where a local variable with the same name is defined.
🔧 Proposed fix
-from sys import stdout📝 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.
| from sys import stdout |
🧰 Tools
🪛 GitHub Actions: CI / 2_Pre-commit checks.txt
[error] 7-7: flake8 (hook id: flake8) reported unused import: F401 'sys.stdout' imported but unused
🪛 GitHub Actions: CI / 3_Linting.txt
[error] 7-7: flake8: F401 'sys.stdout' imported but unused
🪛 GitHub Actions: CI / Linting
[error] 7-7: flake8: F401 'sys.stdout' imported but unused
🪛 GitHub Actions: CI / Pre-commit checks
[error] 7-7: flake8 (F401): 'sys.stdout' imported but unused
🤖 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 7, Remove the
unused "stdout" import from sys at the top of the module to avoid shadowing the
local variable named stdout used later; locate the import statement "from sys
import stdout" and delete it, then run a quick search in the file for references
to stdout to confirm only the local variable (not the removed import) is used
and that no other code relies on the module-level symbol.
Source: Pipeline failures
| """ | ||
|
|
||
| objective_hosts = [Objectives.ALL_NODES] | ||
| supported_profiles = {"gpu"} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check profile definitions and DPU/DPF related configuration
echo "=== Checking profiles.yaml for dpf/gpu profiles ==="
cat src/profiles/profiles.yaml
echo -e "\n=== Checking other DPU/DPF references in codebase ==="
rg -n -C2 '\bdpf\b|\bdpu\b' --type=yaml
echo -e "\n=== Checking profile usage in both DPF rules ==="
rg -n 'supported_profiles.*=.*\{' src/in_cluster_checks/rules/network/dpf_validations.pyRepository: RedHatInsights/incluster-checks
Length of output: 1139
🏁 Script executed:
#!/bin/bash
# Inspect the rule around supported_profiles={"gpu"} near line 27 and list all supported_profiles in the file.
FILE="src/in_cluster_checks/rules/network/dpf_validations.py"
echo "=== Line 27 context ==="
nl -ba "$FILE" | sed -n '1,120p' | sed -n '20,60p'
echo -e "\n=== All supported_profiles in file ==="
rg -n 'supported_profiles\s*=\s*\{' "$FILE"Repository: RedHatInsights/incluster-checks
Length of output: 271
🏁 Script executed:
#!/bin/bash
FILE="src/in_cluster_checks/rules/network/dpf_validations.py"
echo "=== Context around line 27 ==="
awk 'NR>=15 && NR<=45 {printf "%d:%s\n", NR, $0}' "$FILE"
echo -e "\n=== Context around line 189 ==="
awk 'NR>=175 && NR<=205 {printf "%d:%s\n", NR, $0}' "$FILE"Repository: RedHatInsights/incluster-checks
Length of output: 2857
🏁 Script executed:
#!/bin/bash
rg -n 'supported_profiles\s*=\s*\{"gpu"\}' src/in_cluster_checks/rulesRepository: RedHatInsights/incluster-checks
Length of output: 166
🏁 Script executed:
#!/bin/bash
# Show nearby title/unique_name/links context for the GPU-profile rules
rg -n 'supported_profiles\s*=\s*\{"gpu"\}' -n src/in_cluster_checks/rules -C3Repository: RedHatInsights/incluster-checks
Length of output: 768
🏁 Script executed:
#!/bin/bash
# Check for other DPU-related supported_profiles assignments
rg -n '\bDpu\b|DPU' src/in_cluster_checks/rules/network/dpf_validations.pyRepository: RedHatInsights/incluster-checks
Length of output: 390
🏁 Script executed:
#!/bin/bash
rg -n 'supported_profiles\s*=\s*\{"gpu"\}' src/in_cluster_checks/rules -C2Repository: RedHatInsights/incluster-checks
Length of output: 536
🏁 Script executed:
#!/bin/bash
FILE="src/in_cluster_checks/rules/network/dpf_validations.py"
echo "=== File header (top ~60 lines) ==="
awk 'NR>=1 && NR<=80 {printf "%d:%s\n", NR, $0}' "$FILE"Repository: RedHatInsights/incluster-checks
Length of output: 3301
Fix supported_profiles for DPF rule (DpuBondLacpHealth)
In src/in_cluster_checks/rules/network/dpf_validations.py, the module is explicitly DPF-focused, but class DpuBondLacpHealth sets supported_profiles = {"gpu"} while class OvnGeneveTunnelLocalIp correctly uses supported_profiles = {"dpf"}. Change DpuBondLacpHealth.supported_profiles to {"dpf"}.
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 27-27: Mutable default value for class attribute
(RUF012)
🤖 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 27, The
DPF-focused rule class DpuBondLacpHealth currently sets supported_profiles =
{"gpu"}; update the supported_profiles attribute on the DpuBondLacpHealth class
to {"dpf"} so it matches the module intent (same pattern as
OvnGeneveTunnelLocalIp) and ensures the rule applies to the correct profile.
| def _get_ovs_show(self) -> Optional[str]: | ||
| """Run ovs-vsctl show and return stdout, or None on failure.""" | ||
| try: | ||
| return self.get_output_from_run_cmd(SafeCmdString("ovs-vsctl show")) | ||
| except Exception: | ||
| return None |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Narrow exception handling to UnExpectedSystemOutput.
Catching broad Exception violates coding guidelines and the linter warning (BLE001). Since get_output_from_run_cmd raises UnExpectedSystemOutput on command failure, catch that specific exception instead.
♻️ Catch specific exception type
def _get_ovs_show(self) -> Optional[str]:
"""Run ovs-vsctl show and return stdout, or None on failure."""
try:
return self.get_output_from_run_cmd(SafeCmdString("ovs-vsctl show"))
- except Exception:
+ except UnExpectedSystemOutput:
return NoneAs per coding guidelines: "Do NOT catch Exception and return empty/default values." The specific exception type provides better clarity about what's being handled.
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 198-198: Do not catch blind exception: Exception
(BLE001)
🤖 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 194 -
199, The try/except in _get_ovs_show currently catches all Exception; change it
to catch only UnExpectedSystemOutput (the specific exception raised by
get_output_from_run_cmd) and return None in that except block so we avoid broad
exception swallowing; locate the _get_ovs_show method and replace "except
Exception:" with "except UnExpectedSystemOutput:" and ensure the
UnExpectedSystemOutput symbol is imported or referenced correctly.
Sources: Coding guidelines, Linters/SAST tools
| dpf: | ||
| include: [general] # DPF (DPU Platform Framework) deployments with NVIDIA BlueField-3 DPUs |
There was a problem hiding this comment.
dpf profile does not currently enable DPF validators.
dpf only includes general, but DPF rules are gated with supported_profiles = {"gpu"} (for example, DpuBondLacpHealth), so these rules remain disabled when active_profile=dpf. Please include gpu (directly or transitively) in the dpf profile to satisfy the existing rule-gating contract.
Suggested fix
dpf:
- include: [general] # DPF (DPU Platform Framework) deployments with NVIDIA BlueField-3 DPUs
+ include: [gpu] # DPF (DPU Platform Framework) deployments with NVIDIA BlueField-3 DPUs📝 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.
| dpf: | |
| include: [general] # DPF (DPU Platform Framework) deployments with NVIDIA BlueField-3 DPUs | |
| dpf: | |
| include: [gpu] # DPF (DPU Platform Framework) deployments with NVIDIA BlueField-3 DPUs |
🤖 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/profiles/profiles.yaml` around lines 15 - 16, The dpf profile currently
only includes "general" so DPF validators gated by supported_profiles = {"gpu"}
(e.g., DpuBondLacpHealth) stay disabled; update the profiles YAML so the dpf
profile includes "gpu" (either add gpu to the include list for the dpf profile
or ensure a transitive include that brings in gpu) so that validators using
supported_profiles = {"gpu"} are enabled when active_profile=dpf.
Add two new operational health checks for OpenShift clusters with NVIDIA BlueField DPUs: DpuBondLacpHealth: - Validates 802.3ad (LACP) bond health on DPU ports - Detects slave interfaces down, slaves in different aggregators, LACP churn, and bonds with fewer than 2 active slaves - A degraded bond silently halves available bandwidth OvnGeneveTunnelLocalIp: - Verifies OVN Geneve tunnel local_ip matches the node InternalIP - Detects stale tunnel IPs after node IP changes (e.g. interface migration) which cause silent inter-node connectivity loss Both rules run on all nodes, skip when not applicable, and include unit tests covering pass, fail, and prerequisite scenarios.
4 participants
Summary
This PR builds on top of #39 with the following improvements:
BondBaseshared class for common bond prerequisite checkingDpuBondLacpHealthby splitting sections firstfile_utilsmethods instead of direct command executionNote: This PR has been updated. All changes from PR #65 have been incorporated into PR #39's commit using
--amend, preserving the original author (szigmon) and commit message.Changes
1. New
BondBaseclass (bond_base.py)/proc/net/bondingdirectory)VerifyBondedInterfacesUpandDpuBondLacpHealthnow inherit from itfile_utils.is_dir_exist()for directory checking2. Improved
DpuBondLacpHealthparsing (dpf_validations.py)file_utils.get_lines_in_file()instead ofcatcommandfile_utils.list_files()instead oflscommand3. Improved
OvnGeneveTunnelLocalIphelpers_get_ovs_show(): Usesget_output_from_run_cmd()with proper exception handling_get_node_ip(): Usesfile_utils.read_file()instead of directcatcommand4. Updated tests
file_utilsusageTesting
pytest tests/rules/network/ -v # 134 passed, 60 skippedAssisted-by: Claude Code (Claude Sonnet 4.5) noreply@anthropic.com