feat: add cluster_overview domain with cluster architecture overview rule - #95
feat: add cluster_overview domain with cluster architecture overview rule#95izackv wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR adds a new ChangesCluster Overview Feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Rule as ClusterArchitectureOverview
participant OC as oc CLI
participant API as oc_api
Rule->>OC: _get_resource(clusterversion, required=True)
OC-->>Rule: clusterversion JSON
Rule->>Rule: _collect_cluster_identity(infrastructure)
Rule->>Rule: _collect_topology(infrastructure)
Rule->>Rule: _collect_network()
Rule->>Rule: _collect_storage()
Rule->>Rule: _collect_identity_providers()
Rule->>API: _collect_operators()
API-->>Rule: subscriptions or UnExpectedSystemOutput
Rule->>Rule: _build_summary(overview)
Rule-->>Rule: RuleResult.info(summary, overview)
Related Issues: None specified Related PRs: None specified Suggested labels: enhancement, new-rule Suggested reviewers: None specified 🐰 A rabbit peeked inside the cluster's core, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #95 +/- ##
=======================================
Coverage ? 88.23%
=======================================
Files ? 58
Lines ? 6409
Branches ? 0
=======================================
Hits ? 5655
Misses ? 754
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)
tests/rules/cluster_overview/test_architecture_overview.py (1)
108-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding coverage for non-RBAC "missing resource" scenarios.
The scenarios cover a full healthy cluster and RBAC-denied access, but there's no scenario where
oc getsucceeds with an empty/absent result (e.g. nooauthresource configured, emptystorageclasslist, or a node lacking any recognized role label). These are distinct from RBAC denial and exercise different code paths in the collector helpers (e.g. empty lists/dicts vs.Nonefrom a failed command). As per path instructions, tests should "cover all meaningful scenarios including edge cases, partial state, empty output, and missing resources."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/rules/cluster_overview/test_architecture_overview.py` around lines 108 - 178, The test coverage in TestClusterArchitectureOverview only exercises healthy and RBAC-denied paths, but it misses missing-resource/empty-result cases. Add a new RuleScenarioParams in scenario_info (or scenario_unexpected_system_output if needed) that uses oc_cmd_output_dict entries for successful but empty responses from helpers like get_cluster_overview data sources, such as an empty oauth, storageclass, or nodes payload, and verify the info_msg reflects unknown/zero values rather than a denial. Reuse the existing FULL_CLUSTER_OC_OUTPUTS, PARTIAL_CLUSTER_OC_OUTPUTS, and SUBSCRIPTIONS_DENIED patterns to locate the setup, and ensure the new case distinguishes empty results from RBAC failures in ClusterArchitectureOverview.Source: Path instructions
🤖 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/cluster_overview/architecture_overview.py`:
- Around line 1-218: Black formatting is out of sync in
ClusterArchitectureOverview, so the file needs to be reformatted to match the
repo style. Run Black on the architecture_overview.py module and commit the
resulting formatting changes, keeping the logic in methods like run_rule,
_collect_cluster_identity, and _build_summary unchanged.
- Around line 34-36: The architecture_overview rule’s links list points to
public OpenShift docs instead of the required Confluence page. Update the links
field in the cluster_overview rule definition to use the appropriate Confluence
URL, and keep the documentation reference aligned with the domain/category used
by this rule.
- Around line 31-36: The Cluster architecture overview rule is intended to run
only in the general profile, but the rule definition does not declare that
scope. Update the rule class in architecture_overview.py by adding a
supported_profiles declaration alongside the existing objective_hosts,
unique_name, title, and links fields, using the profile name referenced by the
PR (“general”) so the rule is explicitly profile-specific.
- Around line 181-201: The _collect_operators helper is swallowing
UnExpectedSystemOutput from get_operator_subscriptions, which hides oc and JSON
parsing failures. Remove the broad try/except in _collect_operators and let
UnExpectedSystemOutput propagate so the architecture_overview rule can SKIP with
the original diagnostic context; keep the subscription-to-operator mapping and
sorting logic unchanged.
---
Nitpick comments:
In `@tests/rules/cluster_overview/test_architecture_overview.py`:
- Around line 108-178: The test coverage in TestClusterArchitectureOverview only
exercises healthy and RBAC-denied paths, but it misses
missing-resource/empty-result cases. Add a new RuleScenarioParams in
scenario_info (or scenario_unexpected_system_output if needed) that uses
oc_cmd_output_dict entries for successful but empty responses from helpers like
get_cluster_overview data sources, such as an empty oauth, storageclass, or
nodes payload, and verify the info_msg reflects unknown/zero values rather than
a denial. Reuse the existing FULL_CLUSTER_OC_OUTPUTS,
PARTIAL_CLUSTER_OC_OUTPUTS, and SUBSCRIPTIONS_DENIED patterns to locate the
setup, and ensure the new case distinguishes empty results from RBAC failures in
ClusterArchitectureOverview.
🪄 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: c763cb67-8d4c-4e0d-a9bb-4785347df6b4
📒 Files selected for processing (6)
src/in_cluster_checks/domains/cluster_overview_domain.pysrc/in_cluster_checks/rules/cluster_overview/__init__.pysrc/in_cluster_checks/rules/cluster_overview/architecture_overview.pytests/domains/test_cluster_overview_domain.pytests/rules/cluster_overview/__init__.pytests/rules/cluster_overview/test_architecture_overview.py
…rule Add an informational OrchestratorRule that collects a read-only snapshot of what the cluster is: identity (version, channel, platform, base domain, version history), topology (nodes by role, control-plane topology, kubelet/OS versions), network (CNI type, cluster/service CIDRs, MTU), storage classes, identity providers, and installed operator subscriptions. Results are returned as structured system_info on an INFO status. The ClusterVersion resource is required (rule is skipped if it cannot be read); all other sections degrade gracefully on RBAC-restricted or minimal clusters so a partial overview is still reported. Contributed from the ocp-analyzer project (https://github.com/izackv/ocp-analyzer), adapted from its offline architecture-overview report generator to run live via the cluster API. Assisted-by: Claude Code (Claude Fable 5) <noreply@anthropic.com>
e70ed1c to
e35c623
Compare
- Declare supported_profiles = {"general"} explicitly on the rule
- Collect operator subscriptions through the same _get_resource() helper
as other sections: RBAC denial degrades to an empty list, while JSON
parse failures still propagate as UnExpectedSystemOutput (SKIP with
diagnostics) instead of being silently swallowed
- Add test scenario for readable-but-empty resources (no IdPs, no
storage classes, no subscriptions, node without role labels),
distinct from RBAC denial
Assisted-by: Claude Code (Claude Fable 5) <noreply@anthropic.com>
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
sprizend-rh
left a comment
sprizend-rh
left a comment
There was a problem hiding this comment.
Nice contribution — the cluster overview concept and graceful degradation design are solid. Two suggestions to align with project patterns:
-
Use
select_resources()instead ofrun_oc_command+parse_json()— this is the standard resource-querying API used by all other rules. It's a bigger refactor than just swapping the helper (the_collect_*methods need.modelaccess too), but it drops theparse_jsondependency and gives you debug logging for free. -
Use
self._node_executorsfor role counting — the executors already have pre-computednode_labelsfrom startup. This removes the duplicated_get_node_roleslogic and stays consistent with howNodeResourcesCollectorhandles roles.get_all_nodes()would still be needed for kubelet/OS version data.
| Args: | ||
| resource_args: Arguments for `oc get` (e.g. ["network.config", "cluster"]) | ||
| required: If True, a failed command raises UnExpectedSystemOutput | ||
| (rule becomes SKIP); otherwise None is returned | ||
|
|
||
| Returns: | ||
| Parsed resource dict, or None if unavailable and not required | ||
| """ | ||
| args = [*resource_args, "-o", "json"] | ||
| return_code, output, _ = self.oc_api.run_oc_command("get", args, timeout=45, raise_on_error=required) | ||
| if return_code != 0: | ||
| return None | ||
| return parse_json(output, f"oc get {' '.join(args)}", self.get_host_ip()) |
There was a problem hiding this comment.
The project convention is to use self.oc_api.select_resources() instead of run_oc_command + parse_json(). It returns parsed Python objects (accessed via .model), handles timeouts, logs commands, and provides debug output — all the things this helper reimplements manually.
This is a larger refactor than just swapping _get_resource, since:
select_resources()returns openshift-client objects, so all_collect_*methods would change from.get("spec", {})to.model.spec- For list resources (
storageclass,subscriptions), it returns a flat list — not a{"items": [...]}dict — sostorage_classes.get("items", [])becomes direct iteration - Error handling differs:
select_resources()raisesOpenShiftPythonExceptionon RBAC denial instead of returning a non-zero return code. The_get_resourcewrapper would need atry/exceptfor non-required resources - Some resources already have dedicated helpers (e.g.
self.oc_api.get_all_nodes())
Example of what _get_resource could look like wrapping select_resources:
def _get_resource(self, resource_type, single=True, all_namespaces=False, required=False):
try:
return self.oc_api.select_resources(resource_type, single=single, all_namespaces=all_namespaces, timeout=45)
except Exception:
if required:
raise
return NoneAnd callers would use .model access:
# e.g. in _collect_network:
network_config = self._get_resource("network.config/cluster")
if not network_config:
return {}
return {
"network_type": network_config.model.status.get("networkType"),
...
}It's more work upfront but keeps the rule consistent with the rest of the codebase and drops the parse_json dependency.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rule_file="src/in_cluster_checks/rules/cluster_overview/architecture_overview.py"
ast-grep outline "$rule_file" --items all --view expanded
printf '\n--- select_resources definitions and usages ---\n'
rg -n -C 4 '\bselect_resources\s*\(' src tests
printf '\n--- OpenShiftPythonException references ---\n'
rg -n -C 3 '\bOpenShiftPythonException\b' src tests
printf '\n--- complete rule ---\n'
cat -n "$rule_file"Length of output: 37212
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- OcApiUtils.select_resources ---'
sed -n '23,145p' src/in_cluster_checks/utils/oc_api_utils.py
printf '%s\n' '--- framework exception handling ---'
rg -n -C 5 'UnExpectedSystemOutput|OpenShiftPythonException' src/in_cluster_checks/core src/in_cluster_checks
printf '%s\n' '--- existing narrow exception conversion patterns ---'
rg -n -C 8 'except oc\.OpenShiftPythonException|except OpenShiftPythonException' src/in_cluster_checks/rules src/in_cluster_checks/utils
printf '%s\n' '--- architecture overview tests ---'
sed -n '1,280p' tests/rules/cluster_overview/test_architecture_overview.pyLength of output: 124403
@sprizend-rh, verified. This refactor is valid.
select_resources() provides the command log, timeout, and debug behavior that this rule currently duplicates. The rule should remove parse_json and use resource objects or .as_dict() consistently.
One exception detail is important. select_resources() propagates OpenShiftPythonException, but the runner converts only UnExpectedSystemOutput to the required skip result. Therefore, _get_resource() must:
- Catch only
OpenShiftPythonException. - Return
Nonefor an optional resource. - Convert the exception to
UnExpectedSystemOutputfor requiredclusterversion/version. - Not catch broad
Exception.
The refactor must also update list handling for nodes, storage classes, and subscriptions. It should use get_all_nodes() for nodes. The tests must replace run_oc_command() output fixtures with select_resources() object/list fixtures and OpenShiftPythonException cases.
✏️ Learnings added
Learnt from: sprizend-rh
Repo: RedHatInsights/incluster-checks PR: 95
File: src/in_cluster_checks/rules/cluster_overview/architecture_overview.py:56-68
Timestamp: 2026-08-10T09:28:39.206Z
Learning: For Python in-cluster rules in `src/in_cluster_checks/rules`, use `self.oc_api.select_resources()` for Kubernetes and OpenShift resource queries instead of `run_oc_command()` followed by JSON parsing. `select_resources()` provides command logging, timeouts, and debug output; it returns resource objects for `single=True` and lists of resource objects otherwise. When an optional resource query must degrade gracefully, catch only `openshift_client.OpenShiftPythonException`. When a required query fails, convert that exception to `UnExpectedSystemOutput` so the framework reports the rule as SKIP.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
| @staticmethod | ||
| def _get_node_roles(node: dict) -> list: | ||
| """Extract role names from a node's node-role.kubernetes.io/* labels.""" | ||
| labels = node.get("metadata", {}).get("labels", {}) | ||
| roles = [label.split("/", 1)[1] for label in labels if label.startswith("node-role.kubernetes.io/")] | ||
| return sorted(roles) if roles else ["unknown"] |
There was a problem hiding this comment.
This duplicates role-extraction logic from NodeExecutorFactory._get_role_labels_string() (same label prefix parsing and sorting). Since OrchestratorRule has access to self._node_executors, the topology section can read roles from executor.node_labels — the same pattern NodeResourcesCollector._extract_roles() uses in resources_utilization.py.
You'd still need get_all_nodes() for kubeletVersion/osImage (not stored on executors), but the role counting can come from the executors, which removes this static method entirely.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- candidate files ---'
fd -t f -a 'resources_utilization.py|node_executor.*\.py|node_executor_factory.*\.py|architecture_overview.py' . | sort
echo '--- matching definitions and uses ---'
rg -n -C 5 \
'def _get_role_labels_string\b|def _extract_roles\b|_node_executors\b|node_labels\b|def get_all_nodes\b' \
src tests || true
echo '--- outlines ---'
for file in \
src/in_cluster_checks/rules/cluster_overview/architecture_overview.py \
src/in_cluster_checks/utils/resources_utilization.py
do
if [ -f "$file" ]; then
ast-grep outline "$file" --items all
fi
doneLength of output: 69138
@sprizend-rh, the executor labels use the same sorted role names. However, NodeExecutorFactory creates an executor only when a Node has an internal IP.
This rule obtains node_count from oc get nodes. If it obtains nodes_by_role from self._node_executors, a Node without an internal IP can be included in node_count but omitted from its role count.
I recommend retaining the API Node list as the source for both fields. Extract the common node-role.kubernetes.io/* parsing into a shared utility, then use that utility from NodeExecutorFactory and ClusterArchitectureOverview. This removes duplication without changing the overview snapshot semantics.
✏️ Learnings added
Learnt from: sprizend-rh
Repo: RedHatInsights/incluster-checks PR: 95
File: src/in_cluster_checks/rules/cluster_overview/architecture_overview.py:124-129
Timestamp: 2026-08-10T09:28:46.098Z
Learning: In `src/in_cluster_checks/core/executor_factory.py`, `NodeExecutorFactory` creates a `NodeExecutor` only when the Kubernetes Node has an internal IP. For cluster-wide inventory rules, `self._node_executors` can omit Nodes that `oc get nodes` returns, so the API Node list must remain the source for complete node counts and role aggregation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
sprizend-rh
left a comment
sprizend-rh
left a comment
There was a problem hiding this comment.
Two more items:
-
_collect_operatorsreimplementsget_operator_subscriptions()— there's an existing helper onoc_apithat does the same query, already used by two rules ink8s_validations.py. -
Grammar nit —
"1 nodes"in the summary when there's a single node.
|
|
||
| def _collect_operators(self) -> list: | ||
| """Collect installed operator subscriptions (name, namespace, channel, CSV).""" | ||
| subscriptions = self._get_resource(["subscriptions.operators.coreos.com", "--all-namespaces"]) |
There was a problem hiding this comment.
self.oc_api.get_operator_subscriptions() already does exactly this — same resource type, same --all-namespaces, same parse_json. It's used by two rules in k8s_validations.py. You could call it directly instead of going through _get_resource:
def _collect_operators(self) -> list:
subscriptions = self.oc_api.get_operator_subscriptions()
# ... rest of the mapping logicNote: get_operator_subscriptions() raises UnExpectedSystemOutput on failure (no graceful degradation), so you'd need a try/except if you want RBAC denial to degrade to an empty list.
| return ( | ||
| f"OpenShift {identity.get('version') or 'unknown'} " | ||
| f"on {identity.get('platform') or 'unknown platform'} | " | ||
| f"{topology.get('node_count', 0)} nodes ({roles_summary or 'roles unknown'}) | " |
There was a problem hiding this comment.
Nit: when node_count == 1, this produces "1 nodes". Consider:
node_count = topology.get('node_count', 0)
f"{node_count} {'node' if node_count == 1 else 'nodes'} ({roles_summary or 'roles unknown'})"
|
Merging is currently blocked because the branch protection rule requires verified (signed) commits. Your commits don't have verified signatures. To fix this, set up GPG or SSH commit signing: Option 1 — GPG signing: # Generate a GPG key (if you don't have one)
gpg --full-generate-key
# Get your key ID
gpg --list-secret-keys --keyid-format=long
# Configure git to use it
git config --global user.signingkey <YOUR_KEY_ID>
git config --global commit.gpgsign true
# Add the public key to GitHub: Settings → SSH and GPG keys → New GPG key
gpg --armor --export <YOUR_KEY_ID>Option 2 — SSH signing (simpler if you already have an SSH key on GitHub): git config --global gpg.format ssh
git config --global user.signingkey ~/.ssh/id_ed25519.pub
git config --global commit.gpgsign trueMake sure the same SSH key is added as a signing key in GitHub (Settings → SSH and GPG keys → New SSH key → Key type: Signing Key). Then re-sign your commits: # Rebase and re-sign the commits on this branch
git rebase --exec 'git commit --amend --no-edit -S' main
# Force-push the signed commits
git push --force-with-lease |
|
Tip For best results, initiate chat on the files or code changes.
I cannot configure a signing key, rewrite commits, or force-push this branch. A repository contributor must re-sign the commits and run After GitHub marks the replacement commits as Verified, the branch protection check should allow the merge. 🐇 ✏️ Learnings added
You are interacting with an AI system. |
3 participants
What
Adds a new
cluster_overviewdomain with one informational rule,cluster_architecture_overview— anOrchestratorRulethat collects a read-only snapshot of what the cluster is and returns it as structuredsystem_infoon an INFO result:oc_api.get_operator_subscriptions()The short human-readable summary lands in the result message, e.g.:
Why
The framework validates health in depth, but the JSON output contains no picture of what the cluster is — useful context when reading results from an unfamiliar cluster, comparing clusters, or attaching results to a support case. This follows the precedent of the
hw_fw_detailsdomain (inventory collection rather than pass/fail validation), at the cluster-API level.This is contributed from my ocp-analyzer project, which generates an equivalent architecture overview offline from collected
ocoutputs; this port queries the live cluster API viaoc_apiinstead and adds insights available live (e.g.controlPlaneTopologyfor SNO/compact detection).Design notes
oc get ... -o jsoncluster-API calls — no node access, no debug pods, no Secret/ConfigMap data. IdP reporting includes only name and type.UnExpectedSystemOutputpropagates and the rule reports SKIP.--light-run(a handful of API calls), profilegeneral.linkscurrently points to the OpenShift architecture docs; happy to update it to a Confluence page if a maintainer can create one under the In-Cluster Checks Rules space (I don't have access).Tests
tests/rules/cluster_overview/test_architecture_overview.py: full-cluster INFO scenario (message + completesystem_infostructure assertions), partial/RBAC-restricted scenario, and required-resource-failure scenario viaRuleTestBase.tests/domains/test_cluster_overview_domain.py: domain registration.pytest: 849 passed; coverage 89% (above the 80% gate).pre-commit run --all-files: all hooks pass.--list-domains/--list-rulesauto-discover the new domain.--debug-rule cluster_architecture_overviewoutput from a lab cluster if desired.Possible follow-ups
If this shape works for the project, I'd like to contribute further posture rules from ocp-analyzer (backup/DR detection, upgrade-readiness: zero-disruption PDBs /
failurePolicy=Failwebhooks, tenancy governance coverage).Summary by CodeRabbit
New Features
Bug Fixes