From 475f0c454c9b1afc8f2731e4ad2a9afa9c330568 Mon Sep 17 00:00:00 2001 From: Hodaya Berger Date: Wed, 22 Jul 2026 09:25:17 +0300 Subject: [PATCH] feat(PDRIVE-687): add mypy-based pre-commit hook for type checking Add a pre-commit hook that runs mypy to catch type errors at commit time. Reports all mypy errors, with [assignment] errors filtered to SafeCmdString violations only. - Add mypy.ini configuration for type checking - Add check_mypy.py wrapper with SafeCmdString-specific filtering - Add safecmdstring-mypy-check hook to .pre-commit-config.yaml - Add mypy to dev dependencies Assisted-by: Claude Code (Claude Opus 4.6) --- .claude/rules/code-style.md | 4 + .claude/skills/new-rule/SKILL.md | 2 +- .pre-commit-config.yaml | 8 + CONTRIBUTING.md | 2 +- mypy.ini | 19 ++ pyproject.toml | 1 + .../core/data_collector_runner.py | 14 +- src/in_cluster_checks/core/domain.py | 18 +- src/in_cluster_checks/core/exceptions.py | 3 +- .../core/executor_factory.py | 2 +- src/in_cluster_checks/core/operations.py | 26 ++- src/in_cluster_checks/core/printer.py | 32 ++-- src/in_cluster_checks/core/rule.py | 2 +- .../rules/hw_fw_details/hw_fw_base.py | 9 +- .../rules/k8s/k8s_validations.py | 9 +- .../k8s/subscription_operator_validations.py | 6 +- .../rules/network/dns_validations.py | 17 +- .../rules/network/nmstate_validations.py | 26 ++- .../network/node_connectivity_validations.py | 2 +- .../rules/network/ovnk8s_validations.py | 7 +- .../rules/network/ovs_base.py | 6 +- .../resources_utilization.py | 14 +- .../rules/storage/storage_validations.py | 6 +- src/in_cluster_checks/runner.py | 5 +- src/in_cluster_checks/utils/oc_api_utils.py | 172 ++++++++++++------ src/in_cluster_checks/utils/parsing_utils.py | 2 +- tests/linters/check_mypy.py | 59 ++++++ tests/rules/k8s/test_k8s_validations.py | 10 +- .../rules/network/test_ovnk8s_validations.py | 2 +- tests/rules/network/test_ovs_validations.py | 6 +- .../rules/storage/test_storage_validations.py | 100 +++++----- tests/unit/utils/test_oc_api_utils.py | 4 +- 32 files changed, 392 insertions(+), 203 deletions(-) create mode 100644 mypy.ini create mode 100644 tests/linters/check_mypy.py diff --git a/.claude/rules/code-style.md b/.claude/rules/code-style.md index e900443..c910bda 100644 --- a/.claude/rules/code-style.md +++ b/.claude/rules/code-style.md @@ -195,6 +195,10 @@ def has_resource(self) -> bool: **Check `src/in_cluster_checks/utils` for suitable functions before executing shell commands or implementing new logic.** +## Mypy Type Errors + +**Fix mypy errors by correcting type annotations — never suppress them with `# noqa`, `# type: ignore`, or by modifying the filter script (`check_mypy.py`).** + ## Debug Logging See [@.claude/rules/debug-rule.md](debug-rule.md) for debug logging guidelines. diff --git a/.claude/skills/new-rule/SKILL.md b/.claude/skills/new-rule/SKILL.md index 544d8e6..29f3f77 100644 --- a/.claude/skills/new-rule/SKILL.md +++ b/.claude/skills/new-rule/SKILL.md @@ -107,7 +107,7 @@ class MyOrchestratorRule(OrchestratorRule): ```python # Use existing oc_api methods when available pods = self.oc_api.get_pods(namespace="openshift-etcd") -network = self.oc_api.select_resources("network.operator/cluster", single=True) +network = self.oc_api.select_single_resource("network.operator/cluster") # Run commands inside pods cmd = SafeCmdString("etcdctl version") diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 81ab172..582f252 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -58,6 +58,14 @@ repos: types: [python] files: ^src/in_cluster_checks/ + - id: mypy-type-check + name: check types with mypy + entry: python tests/linters/check_mypy.py + language: system + types: [python] + files: ^src/in_cluster_checks/ + pass_filenames: false + - id: pytest-coverage name: pytest with coverage entry: python -m pytest diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ba531fa..cdb159c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -298,7 +298,7 @@ This validates variables to prevent shell injection. ```python # Use existing oc_api methods when available pods = self.oc_api.get_pods(namespace="openshift-etcd") -network = self.oc_api.select_resources("network.operator/cluster", single=True) +network = self.oc_api.select_single_resource("network.operator/cluster") # Use run_oc_command for other oc commands rc, out, err = self.oc_api.run_oc_command("get", ["nodes", "-o", "json"]) diff --git a/mypy.ini b/mypy.ini new file mode 100644 index 0000000..30754a9 --- /dev/null +++ b/mypy.ini @@ -0,0 +1,19 @@ +[mypy] +python_version = 3.12 +check_untyped_defs = True +show_error_codes = True + +# Only report SafeCmdString type errors +disable_error_code = var-annotated,no-untyped-def,import-untyped,no-any-return,override + +# Ignore third-party modules without stubs +ignore_missing_imports = True + +# Files to check +files = src/in_cluster_checks/ + +[mypy-openshift_client.*] +ignore_missing_imports = True + +[mypy-dateutil.*] +ignore_missing_imports = True diff --git a/pyproject.toml b/pyproject.toml index 52e9455..f554e13 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -34,6 +34,7 @@ dev = [ "black>=24.1.1", "flake8>=7.0.0", "isort>=5.13.2", + "mypy>=1.7.0", ] [project.scripts] diff --git a/src/in_cluster_checks/core/data_collector_runner.py b/src/in_cluster_checks/core/data_collector_runner.py index b5ac58f..022c7b6 100644 --- a/src/in_cluster_checks/core/data_collector_runner.py +++ b/src/in_cluster_checks/core/data_collector_runner.py @@ -13,6 +13,7 @@ from in_cluster_checks.core.exceptions import UnExpectedSystemOutput from in_cluster_checks.core.executor import OrchestratorExecutor +from in_cluster_checks.core.operations import DataCollector from in_cluster_checks.core.parallel_runner import ParallelRunner from in_cluster_checks.utils.dict_utils import convert_dict_to_sorted_json_str from in_cluster_checks.utils.enums import ORCHESTRATOR_HOST_NAME, Objectives @@ -37,7 +38,7 @@ class DataCollectorRunner: @classmethod def execute_data_collector( - cls, rule_instance, collector_class: type, use_parallel: bool = True, **kwargs + cls, rule_instance, collector_class: type[DataCollector], use_parallel: bool = True, **kwargs ) -> Dict[str, Any]: """ Execute a DataCollector on all hosts and return aggregated results. @@ -193,7 +194,12 @@ def clear_data_collector_cache(cls): @classmethod def run_collectors( - cls, collector_instances: list, use_parallel: bool, collector_class: type, rule_instance, **kwargs + cls, + collector_instances: list, + use_parallel: bool, + collector_class: type[DataCollector], + rule_instance, + **kwargs, ) -> Dict[str, Dict]: """ Run collectors with caching for many-to-one relationships. @@ -391,7 +397,7 @@ def aggregate_collector_results(rule_instance, results_dict: Dict[str, Dict]) -> @staticmethod def handle_collector_failures( - rule_instance, collector_class: type, host_exceptions: Dict[str, str], hosts_dict: Dict[str, Any] + rule_instance, collector_class: type[DataCollector], host_exceptions: Dict[str, str], hosts_dict: Dict[str, Any] ): """ Handle collector failures and track success. @@ -428,7 +434,7 @@ def is_collector_failed_on_all_hosts(host_exceptions_dict: Dict[str, str], hosts Returns: True if all hosts failed, False otherwise """ - return host_exceptions_dict and set(host_exceptions_dict.keys()) == set(hosts_dict.keys()) + return bool(host_exceptions_dict) and set(host_exceptions_dict.keys()) == set(hosts_dict.keys()) @staticmethod def format_collector_exceptions(collector_name: str, host_exceptions_dict: Dict[str, str]) -> list: diff --git a/src/in_cluster_checks/core/domain.py b/src/in_cluster_checks/core/domain.py index f2ddb93..f1c1af9 100644 --- a/src/in_cluster_checks/core/domain.py +++ b/src/in_cluster_checks/core/domain.py @@ -43,7 +43,7 @@ def domain_name(self) -> str: raise NotImplementedError(f"domain_name() must be implemented in {self.__class__.__name__}") @abc.abstractmethod - def get_rule_classes(self) -> List[type]: + def get_rule_classes(self) -> List[type[Rule]]: """ Get list of rule classes to run in this domain. @@ -59,7 +59,7 @@ def get_rule_classes(self) -> List[type]: """ raise NotImplementedError(f"get_rule_classes() must be implemented in {self.__class__.__name__}") - def _filter_rules_for_light_run(self, rule_classes: List[type]) -> List[type]: + def _filter_rules_for_light_run(self, rule_classes: List[type[Rule]]) -> List[type[Rule]]: """ Filter rules based on light_run mode. @@ -111,7 +111,9 @@ def clean_domain(self): """Clean up after domain execution — e.g. free cached command outputs.""" pass - def _create_rule_groups(self, rule_classes: List[type], host_executors_dict: Dict[str, Any]) -> List[List[Rule]]: + def _create_rule_groups( + self, rule_classes: List[type[Rule]], host_executors_dict: Dict[str, Any] + ) -> List[List[Rule]]: """ Create grouped rule instances following HC's pattern. @@ -139,7 +141,7 @@ def _create_rule_groups(self, rule_classes: List[type], host_executors_dict: Dic return rule_groups - def _create_instances_for_rule(self, rule_class: type, host_executors_dict: Dict[str, Any]) -> List[Rule]: + def _create_instances_for_rule(self, rule_class: type[Rule], host_executors_dict: Dict[str, Any]) -> List[Rule]: """ Create rule instances for a single rule class. @@ -163,7 +165,7 @@ def _create_instances_for_rule(self, rule_class: type, host_executors_dict: Dict # Create instances for matching nodes (handles both ONE_* and multi-type) return self._create_per_node_instances(rule_class, host_executors_dict) - def _create_orchestrator_instance(self, rule_class: type, host_executors_dict: Dict[str, Any]) -> List[Rule]: + def _create_orchestrator_instance(self, rule_class: type[Rule], host_executors_dict: Dict[str, Any]) -> List[Rule]: """ Create single orchestrator instance. @@ -186,7 +188,7 @@ def _create_orchestrator_instance(self, rule_class: type, host_executors_dict: D self.logger.error(f"Failed to instantiate {rule_class.__name__} as orchestrator: {e}") return [] - def _create_per_node_instances(self, rule_class: type, host_executors_dict: Dict[str, Any]) -> List[Rule]: + def _create_per_node_instances(self, rule_class: type[Rule], host_executors_dict: Dict[str, Any]) -> List[Rule]: """ Create rule instances for each matching node. @@ -210,7 +212,7 @@ def _create_per_node_instances(self, rule_class: type, host_executors_dict: Dict return instances - def _should_create_for_executor(self, rule_class: type, executor: Any) -> bool: + def _should_create_for_executor(self, rule_class: type[Rule], executor: Any) -> bool: """ Check if rule should be created for the given executor. @@ -231,7 +233,7 @@ def _should_create_for_executor(self, rule_class: type, executor: Any) -> bool: return True - def _matches_debug_filter(self, rule_class: type) -> bool: + def _matches_debug_filter(self, rule_class: type[Rule]) -> bool: """ Check if rule matches debug filter (by unique_name or title). diff --git a/src/in_cluster_checks/core/exceptions.py b/src/in_cluster_checks/core/exceptions.py index 1827709..37042fe 100644 --- a/src/in_cluster_checks/core/exceptions.py +++ b/src/in_cluster_checks/core/exceptions.py @@ -44,7 +44,8 @@ def __init__( # Sanitize command before including in exception message safe_cmd = SecretFilter.sanitize(self.cmd) - super().__init__(f"{message} on {ip}: {safe_cmd}\nOutput: {output[:500]}") # Limit output length + safe_output = SecretFilter.sanitize(output[:500]) + super().__init__(f"{message} on {ip}: {safe_cmd}\nOutput: {safe_output}") def __str__(self): """Return formatted exception string (HC-style).""" diff --git a/src/in_cluster_checks/core/executor_factory.py b/src/in_cluster_checks/core/executor_factory.py index b45befc..782a560 100644 --- a/src/in_cluster_checks/core/executor_factory.py +++ b/src/in_cluster_checks/core/executor_factory.py @@ -91,7 +91,7 @@ def build_host_executors(self) -> Dict[str, NodeExecutor]: return self._host_executors_dict - def _get_internal_ip(self, node_dict: dict) -> str: + def _get_internal_ip(self, node_dict: dict) -> str | None: """ Extract internal IP from node data. diff --git a/src/in_cluster_checks/core/operations.py b/src/in_cluster_checks/core/operations.py index de8487c..48a6126 100644 --- a/src/in_cluster_checks/core/operations.py +++ b/src/in_cluster_checks/core/operations.py @@ -35,6 +35,8 @@ class Operator: # e.g., [Objectives.ALL_NODES], [Objectives.ICE_CONTAINER], etc. objective_hosts = [] + unique_name: str | None = None + # Thread-safe debug output lock (prevents interleaved output in parallel execution) _debug_lock = threading.RLock() @@ -76,7 +78,11 @@ def _debug_log(self, message: str): print(f"\n[DEBUG] [{self.get_host_name()}] {message}", flush=True) def run_cmd( - self, cmd: SafeCmdString, timeout: int = 120, hosts_cached_pool: dict = None, add_bash_timeout: bool = False + self, + cmd: SafeCmdString, + timeout: int = 120, + hosts_cached_pool: dict | None = None, + add_bash_timeout: bool = False, ) -> tuple: """ Run command on host/container and log it. @@ -141,7 +147,7 @@ def _run_cmd_use_cached( return res def get_output_from_run_cmd( - self, cmd: SafeCmdString, timeout: int = 30, message: str = None, hosts_cached_pool: dict = None + self, cmd: SafeCmdString, timeout: int = 30, message: str | None = None, hosts_cached_pool: dict | None = None ) -> str: """ Run command, log it, and return stdout if successful. @@ -315,14 +321,10 @@ def _enforce_have_document(self): f"Add as class variable: unique_name = 'your_unique_name'" ) - def get_unique_name(self) -> str: + def get_unique_name(self) -> str | None: """Get unique operation name (accessible as class or instance attribute).""" return self.unique_name - def get_severity(self) -> str: - """Get severity level (HC-style interface).""" - return self._severity if self._severity else "NA" - def get_implication_tags(self) -> list: """Get implication tags (HC-style interface).""" return getattr(self, "_implication_tags", []) @@ -505,7 +507,9 @@ class MyCollector(OrchestratorDataCollector): objective_hosts = [Objectives.ORCHESTRATOR] def collect_data(self): - network_obj = self.oc_api.select_resources("network.operator/cluster", single=True) + network_obj = self.oc_api.select_single_resource("network.operator/cluster") + if not network_obj: + return None return network_obj.model.spec.defaultNetwork.type """ @@ -531,7 +535,11 @@ def _validate_objective_hosts(self): pass def run_cmd( - self, cmd: SafeCmdString, timeout: int = 120, hosts_cached_pool: dict = None, add_bash_timeout: bool = False + self, + cmd: SafeCmdString, + timeout: int = 120, + hosts_cached_pool: dict | None = None, + add_bash_timeout: bool = False, ) -> tuple: """ Not available for OrchestratorDataCollector - use oc_api methods instead. diff --git a/src/in_cluster_checks/core/printer.py b/src/in_cluster_checks/core/printer.py index 862a8af..e3ca67b 100644 --- a/src/in_cluster_checks/core/printer.py +++ b/src/in_cluster_checks/core/printer.py @@ -311,12 +311,12 @@ def print_summary(self, domain_name: str) -> None: logger.info("") # Empty line after host validations @staticmethod - def print_to_json(results: Dict[str, Any], output_file: str) -> None: + def print_to_json(results: List[Dict[str, Any]], output_file: str) -> None: """ Write rule results to JSON file with restricted permissions (owner-only). Args: - results: Dictionary with rule results in Insights format + results: List of report dicts from format_results() output_file: Path to output JSON file """ file_descriptor = os.open(output_file, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) @@ -359,7 +359,7 @@ def print_to_junit(results: List[Dict[str, Any]], output_file: str) -> None: if run_time is not None: tc_attrs["time"] = str(run_time) - testcase = ET.SubElement(testsuite, "testcase", **tc_attrs) + testcase = ET.SubElement(testsuite, "testcase", tc_attrs) status = host_result.get("status") message = _strip_xml_illegal_chars(host_result.get("message", "")) @@ -380,7 +380,7 @@ def print_to_junit(results: List[Dict[str, Any]], output_file: str) -> None: tree.write(output_file, encoding="unicode", xml_declaration=True) @staticmethod - def format_results(flow_results: list, rule_component_map: Dict[str, str]) -> Dict[str, Any]: + def format_results(flow_results: list, rule_component_map: Dict[str, str]) -> List[Dict[str, Any]]: """ Format multiple flow results into Insights-compatible structure. @@ -393,21 +393,17 @@ def format_results(flow_results: list, rule_component_map: Dict[str, str]) -> Di rule_component_map: Map of {rule_name: full_component_path} Returns: - Formatted results dictionary: + List of report dicts, each containing: { - "in_cluster_rules": [ - { - "rule_id": "domain|rule", - "component": "...", - "key": "rule", - "status": "aggregated_status", # Worst status across all hosts - "description": "...", - "domain": "...", - "details": [ # Array of host results - {"node_ip": "...", "node_name": "...", "status": "...", ...}, - {"node_ip": "...", "node_name": "...", "status": "...", ...} - ] - } + "rule_id": "domain|rule", + "component": "...", + "key": "rule", + "status": "aggregated_status", # Worst status across all hosts + "description": "...", + "domain": "...", + "details": [ # Array of host results + {"node_ip": "...", "node_name": "...", "status": "...", ...}, + {"node_ip": "...", "node_name": "...", "status": "...", ...} ] } """ diff --git a/src/in_cluster_checks/core/rule.py b/src/in_cluster_checks/core/rule.py index b0be5c2..662a2bb 100644 --- a/src/in_cluster_checks/core/rule.py +++ b/src/in_cluster_checks/core/rule.py @@ -78,7 +78,7 @@ def get_roles_for_current_deployment(self) -> list: return self.objective_hosts @classmethod - def get_unique_name_classmethod(cls) -> str: + def get_unique_name_classmethod(cls) -> str | None: """ Get unique operation name without instantiation. diff --git a/src/in_cluster_checks/rules/hw_fw_details/hw_fw_base.py b/src/in_cluster_checks/rules/hw_fw_details/hw_fw_base.py index 2409358..dd2fbde 100644 --- a/src/in_cluster_checks/rules/hw_fw_details/hw_fw_base.py +++ b/src/in_cluster_checks/rules/hw_fw_details/hw_fw_base.py @@ -182,7 +182,7 @@ def compare_within_groups(self, collected_data: Dict, node_groups: Dict) -> Rule # Process each node group for group_label, executors in node_groups.items(): - group_result = OrderedDict() + group_result: OrderedDict[str, Any] = OrderedDict() group_result["node_count"] = len(executors) group_result["nodes"] = [e.node_name for e in executors] group_result[category_key] = OrderedDict() @@ -223,9 +223,10 @@ def compare_within_groups(self, collected_data: Dict, node_groups: Dict) -> Rule collector_result["value"] = self._get_list_of_id_host_name_data(group_data) # Create nested structure: topic -> name -> result (HC Blueprint format) - if topic not in group_result[category_key]: - group_result[category_key][topic] = OrderedDict() - group_result[category_key][topic][name] = collector_result + category_data: OrderedDict = group_result[category_key] + if topic not in category_data: + category_data[topic] = OrderedDict() + category_data[topic][name] = collector_result result_data[group_label] = group_result diff --git a/src/in_cluster_checks/rules/k8s/k8s_validations.py b/src/in_cluster_checks/rules/k8s/k8s_validations.py index 8a15f56..43fc9f9 100644 --- a/src/in_cluster_checks/rules/k8s/k8s_validations.py +++ b/src/in_cluster_checks/rules/k8s/k8s_validations.py @@ -275,7 +275,12 @@ def _is_old_pod(pod_data: dict, threshold_seconds: int) -> bool: age_seconds = (datetime.now(timezone.utc) - parsed_time).total_seconds() return age_seconds > threshold_seconds except (ValueError, AttributeError) as err: - raise UnExpectedSystemOutput(f"Failed to parse pod timestamp: {timestamp_str}") from err + raise UnExpectedSystemOutput( + ip="cluster-api", + cmd="oc get pods -A -o json", + output=str(timestamp_str), + message=f"Failed to parse pod timestamp: {timestamp_str}", + ) from err class NodesAreReady(OrchestratorRule): @@ -1318,7 +1323,7 @@ def is_prerequisite_fulfilled(self): def run_rule(self): """Check FAR controller manager has correct number of replicas.""" # Step 1: Check if this is a Single Node OpenShift cluster - infrastructure = self.oc_api.select_resources("infrastructure/cluster", single=True, timeout=30) + infrastructure = self.oc_api.select_single_resource("infrastructure/cluster", timeout=30) if infrastructure: infra_dict = infrastructure.as_dict() topology = infra_dict.get("status", {}).get("controlPlaneTopology", "") diff --git a/src/in_cluster_checks/rules/k8s/subscription_operator_validations.py b/src/in_cluster_checks/rules/k8s/subscription_operator_validations.py index 3473be1..282df1e 100644 --- a/src/in_cluster_checks/rules/k8s/subscription_operator_validations.py +++ b/src/in_cluster_checks/rules/k8s/subscription_operator_validations.py @@ -4,6 +4,8 @@ Validates health and status of subscription operators. """ +from typing import Any + from in_cluster_checks.core.rule import OrchestratorRule from in_cluster_checks.core.rule_result import PrerequisiteResult, RuleResult from in_cluster_checks.utils.enums import Objectives @@ -39,7 +41,7 @@ def is_prerequisite_fulfilled(self) -> PrerequisiteResult: f"{self.operator_display_name} operator is not installed on this cluster" ) - def _check_pod_security_context(self, pod_name: str, security_context: dict[str, object] | None) -> list[str]: + def _check_pod_security_context(self, pod_name: str, security_context: dict[str, Any] | None) -> list[str]: """Validate pod-level security context has runAsNonRoot set to true. Args: @@ -61,7 +63,7 @@ def _check_pod_security_context(self, pod_name: str, security_context: dict[str, ) return errors - def _check_containers_non_root(self, pod_name: str, all_containers: list[dict[str, object]]) -> list[str]: + def _check_containers_non_root(self, pod_name: str, all_containers: list[dict[str, Any]]) -> list[str]: """Validate no container runs as root (runAsUser != 0, runAsNonRoot not false). Args: diff --git a/src/in_cluster_checks/rules/network/dns_validations.py b/src/in_cluster_checks/rules/network/dns_validations.py index d39fd27..8a934c1 100644 --- a/src/in_cluster_checks/rules/network/dns_validations.py +++ b/src/in_cluster_checks/rules/network/dns_validations.py @@ -1,5 +1,3 @@ -from typing import ClassVar - from in_cluster_checks.core.exceptions import UnExpectedSystemOutput from in_cluster_checks.core.operations import OrchestratorDataCollector from in_cluster_checks.core.rule import Rule, RuleResult @@ -17,7 +15,7 @@ class DnsOperatorConfigCollector(OrchestratorDataCollector): upstream resolver addresses. """ - objective_hosts: ClassVar[list] = [Objectives.ORCHESTRATOR] + objective_hosts = [Objectives.ORCHESTRATOR] def collect_data(self, **kwargs) -> list[str]: """ @@ -45,7 +43,12 @@ def collect_data(self, **kwargs) -> list[str]: if "NotFound" in combined_output or "not found" in combined_output.lower(): return [] # Other errors should propagate - raise UnExpectedSystemOutput(f"Failed to query DNS operator config: {stderr}") + raise UnExpectedSystemOutput( + ip=self.get_host_ip(), + cmd="oc get dns.operator.openshift.io/cluster -o json", + output=f"{dns_config_output}\n{stderr}".strip(), + message="Failed to query DNS operator config", + ) # Parse DNS config dns_config = parse_json( @@ -80,11 +83,11 @@ class VerifyDnsReachability(Rule): 4. Reports which DNS servers are reachable/unreachable from this node """ - objective_hosts: ClassVar[list] = [Objectives.ALL_NODES] - supported_profiles: ClassVar[set] = {"general"} + objective_hosts = [Objectives.ALL_NODES] + supported_profiles = {"general"} unique_name = "verify_dns_reachability" title = "Verify DNS server reachability" - links: ClassVar[list] = [ + links = [ "https://redhat.atlassian.net/wiki/spaces/PDRIVE/pages/418450933/Verify+DNS+reachability", ] RESOLV_CONF_PATH = "/etc/resolv.conf" diff --git a/src/in_cluster_checks/rules/network/nmstate_validations.py b/src/in_cluster_checks/rules/network/nmstate_validations.py index 50c3d7b..bec0035 100644 --- a/src/in_cluster_checks/rules/network/nmstate_validations.py +++ b/src/in_cluster_checks/rules/network/nmstate_validations.py @@ -76,10 +76,18 @@ def _check_nncp_conditions(self, nncps: List) -> List[str]: for nncp in nncps: # Validate required metadata field if not hasattr(nncp, "model") or not hasattr(nncp.model, "metadata"): - raise UnExpectedSystemOutput(f"NNCP object missing required 'model.metadata' structure: {nncp}") + raise UnExpectedSystemOutput( + ip=self.get_host_ip(), + cmd="oc get nodenetworkconfigurationpolicies -A", + output=str(nncp), + message=f"NNCP object missing required 'model.metadata' structure: {nncp}", + ) if not hasattr(nncp.model.metadata, "name"): raise UnExpectedSystemOutput( - f"NNCP object missing required 'model.metadata.name' field: {nncp.model.metadata}" + ip=self.get_host_ip(), + cmd="oc get nodenetworkconfigurationpolicies -A", + output=str(nncp.model.metadata), + message=f"NNCP object missing required 'model.metadata.name' field: {nncp.model.metadata}", ) nncp_name = nncp.model.metadata.name @@ -107,9 +115,19 @@ def _validate_conditions(self, nncp_name: str, conditions: List[Dict]) -> List[s condition_map = {} for cond in conditions: if not hasattr(cond, "type"): - raise UnExpectedSystemOutput(f"NNCP {nncp_name} condition missing required 'type' field: {cond}") + raise UnExpectedSystemOutput( + ip=self.get_host_ip(), + cmd="oc get nodenetworkconfigurationpolicies -A", + output=str(cond), + message=f"NNCP {nncp_name} condition missing required 'type' field: {cond}", + ) if not hasattr(cond, "status"): - raise UnExpectedSystemOutput(f"NNCP {nncp_name} condition missing required 'status' field: {cond}") + raise UnExpectedSystemOutput( + ip=self.get_host_ip(), + cmd="oc get nodenetworkconfigurationpolicies -A", + output=str(cond), + message=f"NNCP {nncp_name} condition missing required 'status' field: {cond}", + ) condition_map[cond.type] = cond # Check Available condition diff --git a/src/in_cluster_checks/rules/network/node_connectivity_validations.py b/src/in_cluster_checks/rules/network/node_connectivity_validations.py index 327a46b..3d505da 100644 --- a/src/in_cluster_checks/rules/network/node_connectivity_validations.py +++ b/src/in_cluster_checks/rules/network/node_connectivity_validations.py @@ -163,7 +163,7 @@ def _collect_dns_for_bonds(self, bond_devices: list[str]) -> dict: return all_bonds_dns - def collect_data(self, **kwargs) -> dict: + def collect_data(self, **kwargs) -> dict | None: """ Collect DNS server data from all bond interfaces. diff --git a/src/in_cluster_checks/rules/network/ovnk8s_validations.py b/src/in_cluster_checks/rules/network/ovnk8s_validations.py index 76abcc8..a682b03 100644 --- a/src/in_cluster_checks/rules/network/ovnk8s_validations.py +++ b/src/in_cluster_checks/rules/network/ovnk8s_validations.py @@ -32,7 +32,7 @@ def is_prerequisite_fulfilled(self) -> PrerequisiteResult: PrerequisiteResult indicating if OVN-Kubernetes is the network type """ try: - network_obj = self.oc_api.select_resources(resource_type="network.operator/cluster", single=True) + network_obj = self.oc_api.select_single_resource(resource_type="network.operator/cluster") if not network_obj: return PrerequisiteResult.not_met("Cannot determine network type: network.operator/cluster not found") @@ -225,10 +225,7 @@ def run_rule(self) -> RuleResult: return RuleResult.passed() def _get_expected_mtu(self) -> Optional[int]: - network_obj = self.oc_api.select_resources( - resource_type="network.operator/cluster", - single=True, - ) + network_obj = self.oc_api.select_single_resource(resource_type="network.operator/cluster") if not network_obj: return None diff --git a/src/in_cluster_checks/rules/network/ovs_base.py b/src/in_cluster_checks/rules/network/ovs_base.py index d9a9575..00be9b3 100644 --- a/src/in_cluster_checks/rules/network/ovs_base.py +++ b/src/in_cluster_checks/rules/network/ovs_base.py @@ -282,7 +282,7 @@ def collect_data(self, **kwargs) -> bool: True if OVN-Kubernetes, False otherwise """ try: - network_obj = self.oc_api.select_resources(resource_type="network.operator/cluster", single=True) + network_obj = self.oc_api.select_single_resource(resource_type="network.operator/cluster") if not network_obj: return False @@ -340,7 +340,7 @@ def collect_data(self) -> set[str]: return set() nncps = parse_json( - output=out, cmd="oc get nodenetworkconfigurationpolicies.nmstate.io -A -o json", ip=self.get_host_ip + output=out, cmd="oc get nodenetworkconfigurationpolicies.nmstate.io -A -o json", ip=self.get_host_ip() ).get("items", []) ovs_bond_vlans = set() @@ -397,7 +397,7 @@ def collect_data(self) -> set[str]: return set() nncps = parse_json( - output=out, cmd="oc get nodenetworkconfigurationpolicies.nmstate.io -A -o json", ip=self.get_host_ip + output=out, cmd="oc get nodenetworkconfigurationpolicies.nmstate.io -A -o json", ip=self.get_host_ip() ).get("items", []) secondary_bridges = set() for nncp in nncps: diff --git a/src/in_cluster_checks/rules/resources_utilization/resources_utilization.py b/src/in_cluster_checks/rules/resources_utilization/resources_utilization.py index 5f95949..dd7addb 100644 --- a/src/in_cluster_checks/rules/resources_utilization/resources_utilization.py +++ b/src/in_cluster_checks/rules/resources_utilization/resources_utilization.py @@ -1,7 +1,7 @@ """Orchestrator rule for cluster resources utilization reporting.""" import re -from typing import Any, Dict, List +from typing import Any, Dict, List, TypedDict from in_cluster_checks.core.exceptions import UnExpectedSystemOutput from in_cluster_checks.core.operations import OrchestratorDataCollector @@ -10,6 +10,14 @@ from in_cluster_checks.utils.parsing_utils import format_cpu, format_memory +class ParsedResourceLine(TypedDict): + resource_name: str + requests_value: str + requests_pct: str | None + limits_value: str + limits_pct: str | None + + class NodeResourcesCollector(OrchestratorDataCollector): """Collect node resources, allocatable capacity, roles, schedulability, and allocated resources.""" @@ -99,7 +107,7 @@ def _extract_roles(self, node_name: str, node_executors: Dict[str, Any]) -> List return sorted(node_labels.split(",")) - def _parse_allocated_resources(self, describe_output: str) -> Dict[str, Dict[str, str]]: + def _parse_allocated_resources(self, describe_output: str) -> Dict[str, Dict[str, str | None]]: """Parse 'Allocated resources' section from oc describe node output. Args: @@ -149,7 +157,7 @@ def _parse_allocated_resources(self, describe_output: str) -> Dict[str, Dict[str return allocated - def _parse_resource_line(self, line: str) -> Dict[str, str] | None: + def _parse_resource_line(self, line: str) -> ParsedResourceLine | None: """Parse a single resource line from the allocated resources table. Args: diff --git a/src/in_cluster_checks/rules/storage/storage_validations.py b/src/in_cluster_checks/rules/storage/storage_validations.py index 2da0acf..d14a0f4 100644 --- a/src/in_cluster_checks/rules/storage/storage_validations.py +++ b/src/in_cluster_checks/rules/storage/storage_validations.py @@ -95,9 +95,7 @@ def is_prerequisite_fulfilled(self) -> PrerequisiteResult: try: # Check if openshift-storage namespace exists (Rook-Ceph namespace) - namespace_obj = self.oc_api.select_resources( - resource_type="namespace/openshift-storage", timeout=10, single=True - ) + namespace_obj = self.oc_api.select_single_resource(resource_type="namespace/openshift-storage", timeout=10) if not namespace_obj: return PrerequisiteResult.not_met( "OpenShift Storage namespace not found. Ceph is not deployed in this cluster." @@ -618,7 +616,7 @@ def run_rule(self) -> RuleResult: def _get_osd_pods(self) -> list: """Get all OSD pods from the cluster.""" - return self._get_pods(namespace=self.NAMESPACE, labels={"app": "rook-ceph-osd"}) + return self.oc_api.get_pods(namespace=self.NAMESPACE, labels={"app": "rook-ceph-osd"}) def _check_pod_health(self, pod) -> dict | None: """ diff --git a/src/in_cluster_checks/runner.py b/src/in_cluster_checks/runner.py index ec0f149..d04de31 100644 --- a/src/in_cluster_checks/runner.py +++ b/src/in_cluster_checks/runner.py @@ -14,6 +14,7 @@ from in_cluster_checks import global_config from in_cluster_checks.core.data_collector_runner import DataCollectorRunner from in_cluster_checks.core.domain import RuleDomain +from in_cluster_checks.core.executor import NodeExecutor from in_cluster_checks.core.executor_factory import NodeExecutorFactory from in_cluster_checks.core.printer import StructedPrinter from in_cluster_checks.utils.enums import Status @@ -61,8 +62,8 @@ def __init__( self.logger = logging.getLogger(__name__) self.domain_package = domain_package - self.factory = None - self.node_executors = None + self.factory: NodeExecutorFactory | None = None + self.node_executors: Dict[str, NodeExecutor] | None = None # Set global config so other components can access it global_config.set_config( diff --git a/src/in_cluster_checks/utils/oc_api_utils.py b/src/in_cluster_checks/utils/oc_api_utils.py index 8ba9325..2ccbdfa 100644 --- a/src/in_cluster_checks/utils/oc_api_utils.py +++ b/src/in_cluster_checks/utils/oc_api_utils.py @@ -16,7 +16,7 @@ def run_rule(self): class MyCollector(OrchestratorDataCollector): def collect_data(self): - network_obj = self.oc_api.select_resources("network.operator/cluster", single=True) + network_obj = self.oc_api.select_single_resource("network.operator/cluster") ... """ @@ -347,48 +347,22 @@ def _calculate_age(self, creation_timestamp: str) -> str: # If parsing fails, return the original timestamp return creation_timestamp - def select_resources( + def _log_and_build_selector_kwargs( self, resource_type: str, namespace: str | None = None, labels: Dict[str, str] | None = None, - field_selector: dict | None = None, + field_selector: Dict[str, str] | None = None, all_namespaces: bool = False, - timeout: int = 30, - single: bool = False, - ) -> list | Any | None: - """Execute oc.selector with consistent error handling and timeout management. - - This is a generic wrapper around oc.selector() that provides: - - Consistent timeout management - - Standardized error handling with contextual logging - - Support for both .objects() (list) and .object() (single) patterns - - Validation of mutually exclusive parameters - - Args: - resource_type: Resource type to select (e.g., "node", "pod", "network.operator/cluster") - namespace: Specific namespace to search in (mutually exclusive with all_namespaces) - labels: Dictionary of label selectors (e.g., {"app": "myapp"}) - field_selector: Dictionary of field selectors for server-side filtering. - Prefix key with '!' for != logic. - (e.g., {"!status.phase": "Succeeded"} for status.phase!=Succeeded) - all_namespaces: Search across all namespaces (mutually exclusive with namespace) - timeout: Timeout in seconds (default: 30) - single: If True, return single object via .object() instead of list via .objects() - - Returns: - - If single=True: Single resource object or None if not found - - If single=False: List of resource objects (empty list if none found) + ) -> Dict[str, Any]: + """Validate params, log the command, and build selector kwargs. Raises: ValueError: If both namespace and all_namespaces are specified - OpenShiftPythonException: If command fails (e.g., invalid resource type, timeout) """ - # Validate mutually exclusive parameters if namespace and all_namespaces: raise ValueError("Cannot specify both 'namespace' and 'all_namespaces' parameters") - # Build command string for logging cmd_parts = ["oc", "get", resource_type] if namespace: cmd_parts.extend(["-n", namespace]) @@ -405,42 +379,109 @@ def select_resources( cmd_str = " ".join(cmd_parts) self.operator._add_cmd_to_log(cmd_str) - # In debug mode, print command BEFORE execution self._debug_log(f"Executing command via oc.selector: {cmd_str}") + selector_kwargs: Dict[str, Any] = {} + if labels: + selector_kwargs["labels"] = labels + if field_selector: + selector_kwargs["field_selectors"] = field_selector + if all_namespaces: + selector_kwargs["all_namespaces"] = True + + return selector_kwargs + + def select_resources( + self, + resource_type: str, + namespace: str | None = None, + labels: Dict[str, str] | None = None, + field_selector: Dict[str, str] | None = None, + all_namespaces: bool = False, + timeout: int = 30, + ) -> list[oc.APIObject]: + """Select multiple resources of a given type. + + Args: + resource_type: Resource type to select (e.g., "node", "pod") + namespace: Specific namespace to search in (mutually exclusive with all_namespaces) + labels: Dictionary of label selectors (e.g., {"app": "myapp"}) + field_selector: Dictionary of field selectors for server-side filtering. + Prefix key with '!' for != logic. + (e.g., {"!status.phase": "Succeeded"} for status.phase!=Succeeded) + all_namespaces: Search across all namespaces (mutually exclusive with namespace) + timeout: Timeout in seconds (default: 30) + + Returns: + List of resource objects (empty list if none found) + + Raises: + ValueError: If both namespace and all_namespaces are specified + OpenShiftPythonException: If command fails (e.g., invalid resource type, timeout) + """ + selector_kwargs = self._log_and_build_selector_kwargs( + resource_type, namespace, labels, field_selector, all_namespaces + ) + with oc.timeout(timeout): - # Build selector kwargs - selector_kwargs = {} - if labels: - selector_kwargs["labels"] = labels - if field_selector: - selector_kwargs["field_selectors"] = field_selector - if all_namespaces: - selector_kwargs["all_namespaces"] = True - - # Create selector with appropriate context if namespace: with oc.project(namespace): - selector = oc.selector(resource_type, **selector_kwargs) - result = selector.object(ignore_not_found=True) if single else selector.objects() + result = oc.selector(resource_type, **selector_kwargs).objects() else: - selector = oc.selector(resource_type, **selector_kwargs) - result = selector.object(ignore_not_found=True) if single else selector.objects() + result = oc.selector(resource_type, **selector_kwargs).objects() - # In debug mode, print results after execution with limited fields - # Extract base resource type (e.g., "pod" from "pod" or "deployment" from "deployment.apps") base_type = resource_type.split("/")[-1].split(".")[0] + msg = f"Found {len(result)} {base_type}(s) (showing limited fields equivalent to -o wide)" + self._debug_log(msg, obj=result, resource_type=base_type) - if single: - result_name = result.name() if result else "None" - self._debug_log( - f"Command result: {result_name} (showing limited fields equivalent to -o wide)", - obj=result, - resource_type=base_type, - ) + return result + + def select_single_resource( + self, + resource_type: str, + namespace: str | None = None, + labels: Dict[str, str] | None = None, + field_selector: Dict[str, str] | None = None, + all_namespaces: bool = False, + timeout: int = 30, + ) -> Any | None: + """Select a single resource of a given type. + + Args: + resource_type: Resource type to select (e.g., "network.operator/cluster") + namespace: Specific namespace to search in (mutually exclusive with all_namespaces) + labels: Dictionary of label selectors (e.g., {"app": "myapp"}) + field_selector: Dictionary of field selectors for server-side filtering. + Prefix key with '!' for != logic. + (e.g., {"!status.phase": "Succeeded"} for status.phase!=Succeeded) + all_namespaces: Search across all namespaces (mutually exclusive with namespace) + timeout: Timeout in seconds (default: 30) + + Returns: + Single resource object or None if not found + + Raises: + ValueError: If both namespace and all_namespaces are specified + OpenShiftPythonException: If command fails (e.g., invalid resource type, timeout) + """ + selector_kwargs = self._log_and_build_selector_kwargs( + resource_type, namespace, labels, field_selector, all_namespaces + ) + + with oc.timeout(timeout): + if namespace: + with oc.project(namespace): + result = oc.selector(resource_type, **selector_kwargs).object(ignore_not_found=True) else: - msg = f"Found {len(result)} {base_type}(s) (showing limited fields equivalent to -o wide)" - self._debug_log(msg, obj=result, resource_type=base_type) + result = oc.selector(resource_type, **selector_kwargs).object(ignore_not_found=True) + + base_type = resource_type.split("/")[-1].split(".")[0] + result_name = result.name() if result else "None" + self._debug_log( + f"Command result: {result_name} (showing limited fields equivalent to -o wide)", + obj=result, + resource_type=base_type, + ) return result @@ -448,7 +489,7 @@ def get_pods( self, namespace: str = None, labels: dict = None, - field_selector: dict = None, + field_selector: Dict[str, str] | None = None, timeout: int = 30, ) -> list: """Get pods from namespace with optional label and field selector filtering. @@ -505,7 +546,7 @@ def get_pod_name(self, namespace: str, labels: dict, log_errors: bool = True, ti self.logger.info(error_msg) return None - def run_rsh_cmd(self, namespace: str, pod: str, command: SafeCmdString, timeout: int = 120) -> tuple: + def run_rsh_cmd(self, namespace: str | None, pod: str | None, command: SafeCmdString, timeout: int = 120) -> tuple: """ Run command in a pod using oc rsh. @@ -519,8 +560,21 @@ def run_rsh_cmd(self, namespace: str, pod: str, command: SafeCmdString, timeout: Tuple of (return_code, stdout, stderr) Raises: + UnExpectedSystemOutput: If namespace or pod is None TypeError: If command is not a SafeCmdString instance """ + if not namespace: + raise UnExpectedSystemOutput( + ip=self.operator.get_host_ip(), cmd=str(command), output="", message="Namespace not provided" + ) + if not pod: + raise UnExpectedSystemOutput( + ip=self.operator.get_host_ip(), + cmd=str(command), + output="", + message=f"Pod not found in namespace {namespace}", + ) + # Enforce SafeCmdString usage to prevent shell injection if not isinstance(command, SafeCmdString): raise TypeError( diff --git a/src/in_cluster_checks/utils/parsing_utils.py b/src/in_cluster_checks/utils/parsing_utils.py index ed851b7..268a89e 100644 --- a/src/in_cluster_checks/utils/parsing_utils.py +++ b/src/in_cluster_checks/utils/parsing_utils.py @@ -104,7 +104,7 @@ def get_dict_from_string(text: str, delimiter: str = None) -> Dict[str, Union[st Returns: Dictionary mapping keys to values (auto-converts values to int when possible) """ - result = {} + result: Dict[str, Union[str, int]] = {} delimiter = delimiter or " " for line in text.splitlines(): diff --git a/tests/linters/check_mypy.py b/tests/linters/check_mypy.py new file mode 100644 index 0000000..4637537 --- /dev/null +++ b/tests/linters/check_mypy.py @@ -0,0 +1,59 @@ +#!/usr/bin/env python3 +""" +Mypy pre-commit hook that reports all errors except non-SafeCmdString [assignment] errors. + +Used as a pre-commit hook entry point (see .pre-commit-config.yaml). +Mypy configuration is read from mypy.ini. +""" + +import subprocess +import sys + + +def main() -> None: + """Run mypy and report all errors, filtering [assignment] to SafeCmdString only.""" + files = sys.argv[1:] if len(sys.argv) > 1 else ["src/in_cluster_checks/"] + + cmd = ["mypy", *files] + + result = subprocess.run(cmd, capture_output=True, text=True) + + if result.returncode not in (0, 1): + print(result.stderr or result.stdout) + sys.exit(result.returncode) + + errors = [] + has_safecmd_errors = False + for line in result.stdout.splitlines(): + if "error:" not in line: + continue + if "[assignment]" in line and "SafeCmdString" not in line: + continue + errors.append(line) + if "SafeCmdString" in line: + has_safecmd_errors = True + + if not errors: + sys.exit(0) + + for error in errors: + print(error) + + if has_safecmd_errors: + print() + print("Methods requiring SafeCmdString:") + print(" - run_cmd(cmd, ...)") + print(" - get_output_from_run_cmd(cmd, ...)") + print(" - execute_cmd(cmd, ...)") + print(" - run_cmd_return_is_successful(cmd, ...)") + print(" - run_and_get_the_nth_field(cmd, ...)") + print(" - run_rsh_cmd(namespace, pod, command, ...)") + print() + print("Use: SafeCmdString('cmd {var}').format(var=value)") + + print() + sys.exit(1) + + +if __name__ == "__main__": + main() diff --git a/tests/rules/k8s/test_k8s_validations.py b/tests/rules/k8s/test_k8s_validations.py index 9976f65..c5393f8 100644 --- a/tests/rules/k8s/test_k8s_validations.py +++ b/tests/rules/k8s/test_k8s_validations.py @@ -2369,7 +2369,7 @@ class TestVerifyFARControllerReplicas(RuleTestBase): RuleScenarioParams( "FAR deployment has 2 replicas and all are ready", tested_object_mock_dict={ - "oc_api.select_resources": Mock(return_value=create_mock_infrastructure_for_far("HighlyAvailable")), + "oc_api.select_single_resource": Mock(return_value=create_mock_infrastructure_for_far("HighlyAvailable")), "oc_api.get_all_deployments": Mock( return_value=[ create_mock_deployment( @@ -2389,7 +2389,7 @@ class TestVerifyFARControllerReplicas(RuleTestBase): RuleScenarioParams( "FAR deployment disappears before rule execution", tested_object_mock_dict={ - "oc_api.select_resources": Mock( + "oc_api.select_single_resource": Mock( return_value=create_mock_infrastructure_for_far("HighlyAvailable") ), "oc_api.get_all_deployments": Mock(return_value=[]), @@ -2402,7 +2402,7 @@ class TestVerifyFARControllerReplicas(RuleTestBase): RuleScenarioParams( "FAR deployment has wrong spec replicas", tested_object_mock_dict={ - "oc_api.select_resources": Mock(return_value=create_mock_infrastructure_for_far("HighlyAvailable")), + "oc_api.select_single_resource": Mock(return_value=create_mock_infrastructure_for_far("HighlyAvailable")), "oc_api.get_all_deployments": Mock( return_value=[ create_mock_deployment( @@ -2419,7 +2419,7 @@ class TestVerifyFARControllerReplicas(RuleTestBase): RuleScenarioParams( "FAR deployment has correct spec but not all replicas ready", tested_object_mock_dict={ - "oc_api.select_resources": Mock(return_value=create_mock_infrastructure_for_far("HighlyAvailable")), + "oc_api.select_single_resource": Mock(return_value=create_mock_infrastructure_for_far("HighlyAvailable")), "oc_api.get_all_deployments": Mock( return_value=[ create_mock_deployment( @@ -2457,7 +2457,7 @@ def test_scenario_failed(self, scenario_params, tested_object): def test_sno_cluster_skipped(self, tested_object): """Test that rule is skipped on SNO (Single Node OpenShift) cluster.""" - tested_object.oc_api.select_resources = Mock(return_value=create_mock_infrastructure_for_far("SingleReplica")) + tested_object.oc_api.select_single_resource = Mock(return_value=create_mock_infrastructure_for_far("SingleReplica")) tested_object.oc_api.get_all_deployments = Mock( return_value=[ create_mock_deployment( diff --git a/tests/rules/network/test_ovnk8s_validations.py b/tests/rules/network/test_ovnk8s_validations.py index 56a3919..de49a41 100644 --- a/tests/rules/network/test_ovnk8s_validations.py +++ b/tests/rules/network/test_ovnk8s_validations.py @@ -46,7 +46,7 @@ def _create_mock_network_ovnkube(): RuleScenarioParams( "prerequisite_fulfilled", tested_object_mock_dict={ - "oc_api.select_resources": Mock(return_value=_create_mock_network_ovnkube()) + "oc_api.select_single_resource": Mock(return_value=_create_mock_network_ovnkube()) }, ) ] diff --git a/tests/rules/network/test_ovs_validations.py b/tests/rules/network/test_ovs_validations.py index d3050e5..04d8683 100644 --- a/tests/rules/network/test_ovs_validations.py +++ b/tests/rules/network/test_ovs_validations.py @@ -168,18 +168,16 @@ class TestIsOVNKubernetesCollector(DataCollectorTestBase): "OVN-Kubernetes cluster", {}, scenario_res=True, + tested_object_mock_dict={"oc_api.select_single_resource": Mock(return_value=network_mock_ovn)}, ), DataCollectorScenarioParams( "non-OVN cluster", {}, scenario_res=False, + tested_object_mock_dict={"oc_api.select_single_resource": Mock(return_value=network_mock_other)}, ), ] - # Set tested_object_mock_dict for each scenario - scenarios[0].tested_object_mock_dict = {"oc_api.select_resources": Mock(return_value=network_mock_ovn)} - scenarios[1].tested_object_mock_dict = {"oc_api.select_resources": Mock(return_value=network_mock_other)} - @pytest.mark.parametrize("scenario_params", scenarios) def test_collect_data(self, scenario_params, tested_object): DataCollectorTestBase.test_collect_data(self, scenario_params, tested_object) diff --git a/tests/rules/storage/test_storage_validations.py b/tests/rules/storage/test_storage_validations.py index 6feb86c..f524cce 100644 --- a/tests/rules/storage/test_storage_validations.py +++ b/tests/rules/storage/test_storage_validations.py @@ -76,7 +76,7 @@ class TestCephOsdTreeWorks(RuleTestBase): tested_object_mock_dict={ # Both tools and operator pods return None "oc_api.get_pod_name": Mock(return_value=None), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, ) ] @@ -86,7 +86,7 @@ class TestCephOsdTreeWorks(RuleTestBase): "operator pod available", tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-operator-abc123"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, ) ] @@ -102,7 +102,7 @@ class TestCephOsdTreeWorks(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-12345"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, ), RuleScenarioParams( @@ -116,7 +116,7 @@ class TestCephOsdTreeWorks(RuleTestBase): tested_object_mock_dict={ # First call returns None (no tools pod), second call returns operator pod "oc_api.get_pod_name": Mock(side_effect=[None, "rook-ceph-operator-abc123"]), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, ) ] @@ -131,7 +131,7 @@ class TestCephOsdTreeWorks(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-12345"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, failed_msg="ceph osd tree is not working.\nError: connection refused", ), @@ -144,7 +144,7 @@ class TestCephOsdTreeWorks(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(side_effect=[None, "rook-ceph-operator-abc123"]), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, failed_msg="ceph osd tree is not working.\nError: failed to connect", ) @@ -216,7 +216,7 @@ class TestIsCephHealthOk(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-12345"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, ), RuleScenarioParams( @@ -230,7 +230,7 @@ class TestIsCephHealthOk(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-12345"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, ), RuleScenarioParams( @@ -244,7 +244,7 @@ class TestIsCephHealthOk(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(side_effect=[None, "rook-ceph-operator-abc123"]), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, ), ] @@ -261,7 +261,7 @@ class TestIsCephHealthOk(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-12345"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, failed_msg=( "Ceph health is not ok.\n" @@ -288,7 +288,7 @@ class TestIsCephHealthOk(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-12345"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, failed_msg=( "Ceph health is not ok.\n" @@ -313,7 +313,7 @@ class TestIsCephHealthOk(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-12345"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, failed_msg="Failed to get ceph health status.\nError: connection timeout", ), @@ -328,7 +328,7 @@ class TestIsCephHealthOk(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(side_effect=[None, "rook-ceph-operator-abc123"]), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, failed_msg="Failed to get ceph health status.\nError: cluster unreachable", ), @@ -382,7 +382,7 @@ class TestIsCephOSDsNearFull(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-12345"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, ) ] @@ -397,7 +397,7 @@ class TestIsCephOSDsNearFull(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-12345"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, failed_msg=( "There are OSDs disk usage near or already over the limit.\n" @@ -421,7 +421,7 @@ class TestIsCephOSDsNearFull(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-12345"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, failed_msg=( "There are OSDs disk usage near or already over the limit.\n" @@ -442,7 +442,7 @@ class TestIsCephOSDsNearFull(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-12345"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, failed_msg="Failed to get ceph osd df status.\nError: command not found", ), @@ -493,7 +493,7 @@ class TestIsOSDsUp(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-12345"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, ) ] @@ -508,7 +508,7 @@ class TestIsOSDsUp(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-12345"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, failed_msg="The following OSDs are in down state: [osd.1, osd.2]", ), @@ -521,7 +521,7 @@ class TestIsOSDsUp(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-12345"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, failed_msg="Failed to get ceph osd tree status.\nError: ceph cluster not available", ), @@ -572,7 +572,7 @@ class TestIsOSDsWeightOK(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-12345"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, ) ] @@ -587,7 +587,7 @@ class TestIsOSDsWeightOK(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-12345"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, failed_msg=( "The following OSDs weight not in acceptable range:\n\n" @@ -603,7 +603,7 @@ class TestIsOSDsWeightOK(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-12345"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, failed_msg=( "The following OSDs weight not in acceptable range:\n\n" @@ -622,7 +622,7 @@ class TestIsOSDsWeightOK(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-12345"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, failed_msg="Failed to get ceph osd df status.\nError: timeout", ), @@ -656,7 +656,7 @@ class TestOrphanCsiVolumes(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-xyz"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, ), RuleScenarioParams( @@ -673,7 +673,7 @@ class TestOrphanCsiVolumes(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-xyz"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, ), RuleScenarioParams( @@ -685,7 +685,7 @@ class TestOrphanCsiVolumes(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-xyz"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, ), RuleScenarioParams( @@ -702,7 +702,7 @@ class TestOrphanCsiVolumes(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-xyz"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, ), ] @@ -722,7 +722,7 @@ class TestOrphanCsiVolumes(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-xyz"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, ), ] @@ -752,7 +752,7 @@ class TestOrphanCsiVolumes(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-xyz"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, ), RuleScenarioParams( @@ -777,7 +777,7 @@ class TestOrphanCsiVolumes(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-xyz"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, ), RuleScenarioParams( @@ -804,7 +804,7 @@ class TestOrphanCsiVolumes(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-xyz"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, ), ] @@ -834,7 +834,7 @@ class TestOrphanCsiVolumes(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-xyz"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, ), RuleScenarioParams( @@ -861,7 +861,7 @@ class TestOrphanCsiVolumes(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-xyz"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, ), ] @@ -891,7 +891,7 @@ class TestOrphanCsiVolumes(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-xyz"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, ), RuleScenarioParams( @@ -920,7 +920,7 @@ class TestOrphanCsiVolumes(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-xyz"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, ), RuleScenarioParams( @@ -945,7 +945,7 @@ class TestOrphanCsiVolumes(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-xyz"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, ), RuleScenarioParams( @@ -972,7 +972,7 @@ class TestOrphanCsiVolumes(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-xyz"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, failed_msg="Failed to list CSI subvolumes from Ceph.\nError: Error: unable to connect to ceph cluster", ), @@ -1008,7 +1008,7 @@ class TestOsdJournalError(RuleTestBase): "all OSD pods healthy", tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-12345"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), "_get_osd_pods": Mock( return_value=[ create_pod_mock("rook-ceph-osd-0", phase="Running", ready=True, restarts=0), @@ -1024,7 +1024,7 @@ class TestOsdJournalError(RuleTestBase): "OSD pod not running", tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-12345"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), "_get_osd_pods": Mock( return_value=[ create_pod_mock("rook-ceph-osd-0", phase="Pending", ready=False, restarts=0), @@ -1044,7 +1044,7 @@ class TestOsdJournalError(RuleTestBase): "OSD pod with recent restarts", tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-12345"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), "_get_osd_pods": Mock( return_value=[ create_pod_mock( @@ -1070,7 +1070,7 @@ class TestOsdJournalError(RuleTestBase): "OSD pod in CrashLoopBackOff", tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-12345"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), "_get_osd_pods": Mock( return_value=[ create_pod_mock("rook-ceph-osd-0", phase="Running", ready=False, waiting_reason="CrashLoopBackOff"), @@ -1114,7 +1114,7 @@ class TestCheckPoolSize(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-12345"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, ) ] @@ -1130,7 +1130,7 @@ class TestCheckPoolSize(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-12345"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, failed_msg="ceph replication factor is less than 2 in following pools:\nname2", ), @@ -1144,7 +1144,7 @@ class TestCheckPoolSize(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-12345"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, failed_msg="ceph replication factor is less than 2 in following pools:\npool1\npool2", ), @@ -1160,7 +1160,7 @@ class TestCheckPoolSize(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-12345"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, failed_msg="Failed to get ceph pool details.\nError: connection refused", ), @@ -1194,7 +1194,7 @@ class TestCephSlowOps(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-12345"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, ), ] @@ -1213,7 +1213,7 @@ class TestCephSlowOps(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-12345"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, failed_msg=( "There are slow ops observed on this cluster. " @@ -1231,7 +1231,7 @@ class TestCephSlowOps(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-12345"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, failed_msg=( "There are slow ops observed on this cluster. " @@ -1251,7 +1251,7 @@ class TestCephSlowOps(RuleTestBase): }, tested_object_mock_dict={ "oc_api.get_pod_name": Mock(return_value="rook-ceph-tools-12345"), - "oc_api.select_resources": Mock(return_value=Mock()), + "oc_api.select_single_resource": Mock(return_value=Mock()), }, failed_msg="Failed to get ceph health detail.\nError: connection refused", ), diff --git a/tests/unit/utils/test_oc_api_utils.py b/tests/unit/utils/test_oc_api_utils.py index 21dc336..be48cf5 100644 --- a/tests/unit/utils/test_oc_api_utils.py +++ b/tests/unit/utils/test_oc_api_utils.py @@ -67,7 +67,7 @@ def test_single_returns_object(self, mock_oc, oc_api): mock_oc.timeout.return_value.__exit__ = Mock(return_value=False) mock_oc.selector.return_value = mock_selector - result = oc_api.select_resources("network.operator/cluster", single=True) + result = oc_api.select_single_resource("network.operator/cluster") assert result == mock_obj mock_selector.object.assert_called_once_with(ignore_not_found=True) @@ -81,7 +81,7 @@ def test_single_not_found_returns_none(self, mock_oc, oc_api): mock_oc.timeout.return_value.__exit__ = Mock(return_value=False) mock_oc.selector.return_value = mock_selector - result = oc_api.select_resources("namespace/nonexistent", single=True) + result = oc_api.select_single_resource("namespace/nonexistent") assert result is None mock_selector.object.assert_called_once_with(ignore_not_found=True)