-
Notifications
You must be signed in to change notification settings - Fork 15
refactor: extract BondBase class and improve DPF validation robustness #65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| """ | ||
| Base class for network bond validation rules. | ||
|
|
||
| Provides common functionality for rules that validate bonded network interfaces. | ||
| """ | ||
|
|
||
| from in_cluster_checks.core.rule import PrerequisiteResult, Rule | ||
|
|
||
|
|
||
| class BondBase(Rule): | ||
| """ | ||
| Base class for bond-related validation rules. | ||
|
|
||
| Provides common prerequisite checking for rules that require | ||
| bond interfaces to be configured on the node. | ||
| """ | ||
|
|
||
| BONDING_PATH = "/proc/net/bonding" | ||
|
|
||
| def is_prerequisite_fulfilled(self) -> PrerequisiteResult: | ||
| """ | ||
| Check if bond interfaces exist on this node. | ||
|
|
||
| Returns: | ||
| PrerequisiteResult indicating if bonding is configured | ||
| """ | ||
| if self.file_utils.is_dir_exist(self.BONDING_PATH): | ||
| return PrerequisiteResult.met() | ||
| return PrerequisiteResult.not_met("No bond interfaces configured") |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,274 @@ | ||
| """ | ||
| DPF (DPU Platform Framework) validation checks for OpenShift clusters | ||
| with NVIDIA BlueField DPUs. | ||
| """ | ||
|
|
||
| import re | ||
| from sys import stdout | ||
| from typing import Dict, List, Optional | ||
|
|
||
| from in_cluster_checks.core.exceptions import UnExpectedSystemOutput | ||
| from in_cluster_checks.core.rule import PrerequisiteResult, Rule, RuleResult | ||
| from in_cluster_checks.rules.network.bond_base import BondBase | ||
| from in_cluster_checks.utils.enums import Objectives | ||
| from in_cluster_checks.utils.safe_cmd_string import SafeCmdString | ||
|
|
||
|
|
||
| class DpuBondLacpHealth(BondBase): | ||
| """Verify LACP bond health on DPU ports. | ||
|
|
||
| Checks that bond interfaces using 802.3ad (LACP) mode have all slave | ||
| interfaces UP, are in the same aggregator, and are not in churned state. | ||
| A degraded bond means traffic flows through a single port, reducing | ||
| available bandwidth without any Kubernetes-visible failure. | ||
| """ | ||
|
|
||
| objective_hosts = [Objectives.ALL_NODES] | ||
| supported_profiles = {"gpu"} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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 🧰 Tools🪛 Ruff (0.15.15)[warning] 27-27: Mutable default value for class attribute (RUF012) 🤖 Prompt for AI Agents |
||
| 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"] | ||
|
|
||
| def _parse_bond_info(self, bond_name: str) -> Dict: | ||
| """Parse /proc/net/bonding/<bond> for LACP-specific fields. | ||
|
|
||
| Args: | ||
| bond_name: Name of the bond interface. | ||
|
|
||
| Returns: | ||
| Dictionary with bond mode, MII status, and per-slave details. | ||
| """ | ||
| bond_file = f"/proc/net/bonding/{bond_name}" | ||
| lines = self.file_utils.get_lines_in_file(bond_file) | ||
| if lines is None: | ||
| return {"error": f"Cannot read bond {bond_name}"} | ||
|
|
||
| content = "\n".join(lines) | ||
| sections = [s.strip() for s in content.split("\n\n") if s.strip()] | ||
|
|
||
| info: Dict = {"mode": "", "mii_status": "", "slaves": []} | ||
|
|
||
| for section in sections: | ||
| section_lines = [line.strip() for line in section.split("\n") if line.strip()] | ||
|
|
||
| if any(line.startswith("Slave Interface:") for line in section_lines): | ||
| slave = self._parse_slave_section(section_lines) | ||
| if slave: | ||
| info["slaves"].append(slave) | ||
| else: | ||
| for line in section_lines: | ||
| if line.startswith("Bonding Mode:"): | ||
| info["mode"] = line.split(":", 1)[1].strip() | ||
| elif line.startswith("MII Status:"): | ||
| info["mii_status"] = line.split(":", 1)[1].strip() | ||
|
|
||
| return info | ||
|
|
||
| def _parse_slave_section(self, section_lines: List[str]) -> Optional[Dict]: | ||
| """Parse a slave interface section. | ||
|
|
||
| Args: | ||
| section_lines: Lines from a slave section. | ||
|
|
||
| Returns: | ||
| Dictionary with slave details, or None if parsing fails. | ||
| """ | ||
| slave: Dict = { | ||
| "name": "", | ||
| "mii_status": "", | ||
| "speed": "", | ||
| "aggregator_id": "", | ||
| "actor_churn": "", | ||
| "partner_churn": "", | ||
| } | ||
|
|
||
| for line in section_lines: | ||
| if line.startswith("Slave Interface:"): | ||
| slave["name"] = line.split(":", 1)[1].strip() | ||
| elif line.startswith("MII Status:"): | ||
| slave["mii_status"] = line.split(":", 1)[1].strip() | ||
| elif line.startswith("Speed:"): | ||
| slave["speed"] = line.split(":", 1)[1].strip() | ||
| elif line.startswith("Aggregator ID:"): | ||
| slave["aggregator_id"] = line.split(":", 1)[1].strip() | ||
| elif line.startswith("Actor Churn State:"): | ||
| slave["actor_churn"] = line.split(":", 1)[1].strip() | ||
| elif line.startswith("Partner Churn State:"): | ||
| slave["partner_churn"] = line.split(":", 1)[1].strip() | ||
|
|
||
| return slave if slave["name"] else None | ||
|
|
||
| def run_rule(self) -> RuleResult: | ||
| """Check LACP bond health on all bond interfaces.""" | ||
| bond_names_list = self.file_utils.list_files(self.BONDING_PATH) | ||
| if bond_names_list is None: | ||
| raise UnExpectedSystemOutput( | ||
| ip=self.get_host_ip(), | ||
| cmd=f"ls {self.BONDING_PATH}", | ||
| output="Failed to list bond interfaces", | ||
| ) | ||
|
|
||
| bond_names = bond_names_list | ||
| all_issues: List[str] = [] | ||
| all_passed: List[str] = [] | ||
|
|
||
| for bond_name in bond_names: | ||
| info = self._parse_bond_info(bond_name) | ||
|
|
||
| if "error" in info: | ||
| all_issues.append(f"{bond_name}: {info['error']}") | ||
| continue | ||
|
|
||
| if "802.3ad" not in info["mode"]: | ||
| continue | ||
|
|
||
| if info["mii_status"] != "up": | ||
| all_issues.append(f"{bond_name}: bond MII status is {info['mii_status']}") | ||
| continue | ||
|
|
||
| if len(info["slaves"]) < 2: | ||
| all_issues.append(f"{bond_name}: only {len(info['slaves'])} slave(s), expected 2+ for LACP") | ||
| continue | ||
|
|
||
| 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 on slave(s): {', '.join(missing_agg)}, LACP not negotiated" | ||
| ) | ||
| else: | ||
| agg_ids = set(s["aggregator_id"] for s in info["slaves"]) | ||
| 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 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})") | ||
|
|
||
| if not all_issues and not all_passed: | ||
| return RuleResult.skip("No LACP (802.3ad) bonds found") | ||
|
|
||
| if all_issues: | ||
| msg = "LACP bond issues detected:\n" + "\n".join(f" - {i}" for i in all_issues) | ||
| if all_passed: | ||
| msg += "\nHealthy bonds:\n" + "\n".join(f" - {p}" for p in all_passed) | ||
| return RuleResult.failed(msg) | ||
|
|
||
| return RuleResult.passed("All LACP bonds healthy:\n" + "\n".join(f" - {p}" for p in all_passed)) | ||
|
|
||
|
|
||
| class OvnGeneveTunnelLocalIp(Rule): | ||
| """Verify OVN Geneve tunnel local_ip matches the node's Kubernetes InternalIP. | ||
|
|
||
| OVN programs Geneve tunnels between nodes using each node's IP as the | ||
| local_ip in the tunnel options. If the node's IP changes (e.g. during | ||
| interface migration) without a corresponding OVN restart, the tunnels | ||
| retain the old local_ip, causing silent inter-node connectivity loss | ||
| while the node remains Ready. | ||
| """ | ||
|
|
||
| objective_hosts = [Objectives.ALL_NODES] | ||
| 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"] | ||
|
|
||
| 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 | ||
|
Comment on lines
+194
to
+199
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win Narrow exception handling to UnExpectedSystemOutput. Catching broad ♻️ 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: (BLE001) 🤖 Prompt for AI AgentsSources: Coding guidelines, Linters/SAST tools |
||
|
|
||
| def _extract_geneve_local_ips(self, ovs_output: str) -> List[str]: | ||
| """Extract local_ip values only from Geneve tunnel interface blocks. | ||
|
|
||
| Args: | ||
| ovs_output: Output from ovs-vsctl show. | ||
|
|
||
| Returns: | ||
| Unique list of local_ip values found in Geneve interface blocks. | ||
| """ | ||
| local_ips = [] | ||
| in_geneve_block = False | ||
| for line in ovs_output.splitlines(): | ||
| stripped = line.strip() | ||
| if stripped == "type: geneve": | ||
| in_geneve_block = True | ||
| elif stripped.startswith("type:"): | ||
| in_geneve_block = False | ||
| elif in_geneve_block and "local_ip=" in stripped: | ||
| match = re.search(r'local_ip="([^"]+)"', stripped) | ||
| if match and match.group(1) not in local_ips: | ||
| local_ips.append(match.group(1)) | ||
| in_geneve_block = False | ||
| return local_ips | ||
|
|
||
| def is_prerequisite_fulfilled(self) -> PrerequisiteResult: | ||
| """Check if OVS has any Geneve tunnel interfaces.""" | ||
| stdout = self._get_ovs_show() | ||
| if stdout is None: | ||
| return PrerequisiteResult.not_met("Cannot access OVS") | ||
| if "geneve" not in stdout: | ||
| return PrerequisiteResult.not_met("No Geneve tunnels configured") | ||
| return PrerequisiteResult.met() | ||
|
|
||
| def _get_node_ip(self) -> Optional[str]: | ||
| """Get this node's primary IP from the nodeip-configuration file.""" | ||
| try: | ||
| content = self.file_utils.read_file("/run/nodeip-configuration/primary-ip") | ||
| return content.strip() if content.strip() else None | ||
| except UnExpectedSystemOutput: | ||
| return None | ||
|
|
||
| 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: | ||
| raise UnExpectedSystemOutput( | ||
| ip=self.get_host_ip(), | ||
| cmd="ovs-vsctl show", | ||
| output="Failed to run ovs-vsctl show", | ||
| ) | ||
|
|
||
| local_ips = list(set(self._extract_geneve_local_ips(ovs_output))) | ||
| if not local_ips: | ||
| return RuleResult.skip("No Geneve local_ip found in OVS output") | ||
|
|
||
| node_ip = self._get_node_ip() | ||
| if not node_ip: | ||
| return RuleResult.skip("Cannot determine node primary IP") | ||
|
|
||
| if len(local_ips) > 1: | ||
| return RuleResult.failed( | ||
| f"Multiple different local_ip values in Geneve tunnels: {sorted(local_ips)}. " | ||
| f"OVS configuration may be inconsistent." | ||
| ) | ||
|
|
||
| geneve_local_ip = local_ips[0] | ||
| if geneve_local_ip != node_ip: | ||
| return RuleResult.failed( | ||
| f"Geneve tunnel local_ip ({geneve_local_ip}) does not match " | ||
| f"node primary IP ({node_ip}). " | ||
| f"Inter-node pod connectivity may be broken." | ||
| ) | ||
|
|
||
| return RuleResult.passed(f"Geneve tunnel local_ip ({geneve_local_ip}) matches node primary IP") | ||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -12,6 +12,8 @@ profiles: | |||||||||
| # --- Layer 3: Platforms & Hardware --- | ||||||||||
| gpu: | ||||||||||
| include: [ai-base] # GPU implies AI-base rules | ||||||||||
| dpf: | ||||||||||
| include: [general] # DPF (DPU Platform Framework) deployments with NVIDIA BlueField-3 DPUs | ||||||||||
|
Comment on lines
+15
to
+16
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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
Suggested change
🤖 Prompt for AI Agents |
||||||||||
| rh-nokia: | ||||||||||
| include: [telco-base] | ||||||||||
|
|
||||||||||
|
|
||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unused import.
The
stdoutimport fromsysis 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
🧰 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
Source: Pipeline failures