Support cloudnative-pg (CNPG) clusters#473
Support cloudnative-pg (CNPG) clusters#473saifulmuhajir wants to merge 8 commits intorobusta-dev:mainfrom
Conversation
WalkthroughAdds CNPGCluster support to Kubernetes integration (discovery via CustomObjects API, CNPG-aware pod selection and allocation extraction), extends KindLiteral, updates CLI help text, switches formatters to safe dict lookups, and adds RBAC rules for CNPG clusters. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant ClusterLoader
participant K8sCore as "K8s Core APIs"
participant COAPI as "CustomObjects API"
User->>ClusterLoader: list_scannable_objects()
ClusterLoader->>K8sCore: List Deployments/StatefulSets/...
ClusterLoader->>COAPI: List CNPGCluster (cluster & namespaced)
Note over ClusterLoader: Build K8sObjectData for each item\n(CNPG: allocations from item.spec.resources,\ncontainer_name="postgres")
ClusterLoader-->>User: [K8sObjectData...]
sequenceDiagram
autonumber
participant Consumer
participant ClusterLoader
participant K8sCore as "K8s Core APIs"
Consumer->>ClusterLoader: list_pods(object)
alt object.kind == "CNPGCluster"
ClusterLoader->>K8sCore: List Pods selector: cnpg.io/cluster={name}
else
ClusterLoader->>K8sCore: List Pods via standard selectors
end
ClusterLoader-->>Consumer: [PodData...]
sequenceDiagram
autonumber
participant Builder as "__build_scannable_object"
participant Item as "K8s Item"
Item-->>Builder: item, container, kind
alt kind == "CNPGCluster"
Note right of Builder: allocations <- item.spec.resources\ncontainer_name = "postgres"
else
Note right of Builder: allocations <- container.resources\ncontainer_name = container.name
end
Builder-->>Item: K8sObjectData
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
robusta_krr/formatters/table.py (2)
20-26: Fix None-deref when recommendations are missing.recommended can now be None; accessing recommended.severity/recommended.value will raise. Short-circuit and return NONE_LITERAL.
Apply this diff:
def _format_request_str(item: ResourceScan, resource: ResourceType, selector: str) -> str: - allocated = getattr(item.object.allocations, selector).get(resource, None) + allocated = getattr(item.object.allocations, selector).get(resource, None) info = item.recommended.info.get(resource) - recommended = getattr(item.recommended, selector).get(resource, None) - severity = recommended.severity + recommended = getattr(item.recommended, selector).get(resource, None) + if recommended is None: + return f"{NONE_LITERAL}" + severity = recommended.severity
50-61: Guard format_diff call when recommendation is absent.Avoid passing None into format_diff.
Apply this diff:
def _format_total_diff(item: ResourceScan, resource: ResourceType, pods_current: int) -> str: selector = "requests" - allocated = getattr(item.object.allocations, selector).get(resource, None) - recommended = getattr(item.recommended, selector).get(resource, None) + allocated = getattr(item.object.allocations, selector).get(resource, None) + recommended = getattr(item.recommended, selector).get(resource, None) + if recommended is None: + return ""robusta_krr/formatters/csv.py (1)
29-41: Prevent AttributeError when recommendation is missing.recommended may be None now; recommended.value access will crash. Short-circuit.
Apply this diff:
def _format_request_str(item: ResourceScan, resource: ResourceType, selector: str) -> str: - allocated = getattr(item.object.allocations, selector).get(resource, None) - recommended = getattr(item.recommended, selector).get(resource, None) + allocated = getattr(item.object.allocations, selector).get(resource, None) + recommended = getattr(item.recommended, selector).get(resource, None) + if recommended is None: + return f"{NONE_LITERAL}" - if allocated is None and recommended.value is None: + if allocated is None and recommended.value is None: return f"{NONE_LITERAL}" diff = format_diff(allocated, recommended, selector) if diff != "": diff = f"({diff}) " return diff + format_recommendation_value(allocated) + " -> " + format_recommendation_value(recommended.value)
🧹 Nitpick comments (1)
robusta_krr/core/integrations/kubernetes/__init__.py (1)
443-466: Remove unused and incorrect CNPG pods helper.This method isn’t referenced, uses kind="Cluster" (not in KindLiteral), and an extract_containers shape that doesn’t apply to CNPG. Prefer deleting to avoid confusion.
Apply this diff to remove it:
- def _list_cnpgpods(self) -> list[K8sObjectData]: - # NOTE: Using custom objects API returns dicts, but all other APIs return objects - # We need to handle this difference using a small wrapper - return self._list_scannable_objects( - kind="Cluster", - all_namespaces_request=lambda **kwargs: ObjectLikeDict( - self.custom_objects.list_cluster_custom_object( - group="postgresql.cnpg.io", - version="v1", - plural="clusters", - **kwargs, - ) - ), - namespaced_request=lambda **kwargs: ObjectLikeDict( - self.custom_objects.list_namespaced_custom_object( - group="postgresql.cnpg.io", - version="v1", - plural="clusters", - **kwargs, - ) - ), - extract_containers=lambda item: item.spec.pods[0].spec.containers, - )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
robusta_krr/core/integrations/kubernetes/__init__.py(7 hunks)robusta_krr/core/models/objects.py(1 hunks)robusta_krr/formatters/csv.py(2 hunks)robusta_krr/formatters/csv_raw.py(1 hunks)robusta_krr/formatters/table.py(2 hunks)robusta_krr/main.py(1 hunks)tests/single_namespace_permissions.yaml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
robusta_krr/formatters/csv_raw.py (2)
robusta_krr/core/models/result.py (1)
ResourceScan(25-54)robusta_krr/core/models/allocations.py (1)
ResourceType(13-20)
robusta_krr/core/integrations/kubernetes/__init__.py (3)
robusta_krr/core/models/objects.py (2)
selector(76-83)K8sObjectData(38-107)robusta_krr/utils/object_like_dict.py (2)
get(25-26)ObjectLikeDict(1-29)robusta_krr/core/models/allocations.py (2)
ResourceAllocations(52-106)from_container(79-106)
🔇 Additional comments (9)
tests/single_namespace_permissions.yaml (1)
29-31: RBAC addition for CNPG clusters looks good.Correct API group and plural. Matches how CNPG clusters are discovered in code.
robusta_krr/main.py (1)
101-103: Help text update LGTM.Including CNPGCluster in --resource help keeps CLI aligned with new kind.
robusta_krr/formatters/csv_raw.py (2)
44-47: Safer access: good change.Using .get(...) avoids KeyError and aligns with other formatters.
51-55: Safer access: good change.Consistent with the project-wide guarded lookups.
robusta_krr/core/models/objects.py (1)
11-11: CNPGCluster kind addition LGTM.Extending KindLiteral to include CNPGCluster matches the integration changes.
robusta_krr/core/integrations/kubernetes/__init__.py (4)
150-152: Confirm CNPG pod selector label.Selector cnpg.io/cluster= is assumed. Please confirm this label is stable across your supported CNPG versions.
If needed, I can adjust to a configurable label.
331-335: Graceful handling for CNPG API absence looks good.CNPGCluster added to the “API not available” skip list.
341-363: Name/type correctness of CNPG listing helper.new helper is good, but ensure import of ResourceType (suggested above) to fix allocations mapping. No other issues with using CustomObjectsApi + ObjectLikeDict.
Would you like me to add a minimal e2e test that stubs a CNPG cluster object and validates parsed allocations?
23-25: ResourceAllocations import is valid — no change needed. result.py imports ResourceAllocations from core.models.allocations (from robusta_krr.core.models.allocations import RecommendationValue, ResourceAllocations, ResourceType), sofrom robusta_krr.core.models.result import ResourceAllocationswill succeed at runtime.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
robusta_krr/core/integrations/kubernetes/__init__.py (1)
193-199: CRD selector: matchExpressions branch iterates.items()(list) — will break.getattr(selector, "matchExpressions") is typically a list;
.items()will raise. Iterate directly.Apply this diff:
- if getattr(selector, "matchExpressions", None): - label_filters += [ - ClusterLoader._get_match_expression_filter(expression) for expression in getattr(selector, "matchExpressions").items() - ] + if getattr(selector, "matchExpressions", None): + label_filters += [ + ClusterLoader._get_match_expression_filter(expression) + for expression in getattr(selector, "matchExpressions") + ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
robusta_krr/core/integrations/kubernetes/__init__.py(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
robusta_krr/core/integrations/kubernetes/__init__.py (3)
robusta_krr/core/models/objects.py (2)
selector(76-83)K8sObjectData(38-107)robusta_krr/utils/object_like_dict.py (2)
get(25-26)ObjectLikeDict(1-29)robusta_krr/core/models/allocations.py (2)
ResourceAllocations(52-106)from_container(79-106)
🔇 Additional comments (5)
robusta_krr/core/integrations/kubernetes/__init__.py (5)
110-111: CNPG discovery wired into scan flow — LGTM.Including CNPG clusters in the gathered workloads aligns with the other resource paths.
331-335: Graceful handling when CNPG CRD is unavailable — LGTM.Adding CNPGCluster to the non-fatal API-not-available list is consistent with other CRDs.
341-363: CNPG listing via CustomObjectsApi — LGTM.The wrapper with ObjectLikeDict matches existing CRD patterns; passing the item through for CNPG-specific allocation handling is appropriate.
218-226: CNPG allocations likely incorrect: map string keys to ResourceType.spec.resources uses "cpu"/"memory" keys, but downstream expects ResourceType keys; otherwise allocations resolve to None in reports.
Apply this diff to map keys properly:
# Special handling for CNPGCluster if kind == "CNPGCluster": - resources = getattr(item.spec, "resources", None) or item.get("spec", {}).get("resources", {}) - allocations = ResourceAllocations( - requests=resources.get("requests", {}), - limits=resources.get("limits", {}), - ) + resources = getattr(item.spec, "resources", None) or item.get("spec", {}).get("resources", {}) or {} + req = resources.get("requests", {}) or {} + lim = resources.get("limits", {}) or {} + allocations = ResourceAllocations( + requests={ + ResourceType.CPU: req.get("cpu"), + ResourceType.Memory: req.get("memory"), + }, + limits={ + ResourceType.CPU: lim.get("cpu"), + ResourceType.Memory: lim.get("memory"), + }, + ) container_name = "postgres" else: allocations = ResourceAllocations.from_container(container) container_name = getattr(container, "name", None)Also add the missing import:
-from robusta_krr.core.models.result import ResourceAllocations +from robusta_krr.core.models.result import ResourceAllocations, ResourceType#!/bin/bash # Check call sites expect ResourceType keys (to validate why mapping is needed) rg -nP --type=py -C2 '\ballocations\.(requests|limits)\s*\[\s*ResourceType\.'Also applies to: 249-251
150-152: No change required — selectorcnpg.io/clusteris correctCloudNativePG pods use the label key
cnpg.io/clusterto associate pods with a cluster (confirmed in CloudNativePG docs/quickstart).
|
was hoping to check in on the status of this, and if we might see some movement on it. Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@robusta_krr/core/integrations/kubernetes/__init__.py`:
- Around line 176-177: The CNPGCluster selector currently only uses the cluster
label (selector = f"cnpg.io/cluster={object.name}") which matches both database
and pooler pods while the code assumes a postgres container; update the selector
in the CNPGCluster branch so it includes the component label to restrict to the
database pods (e.g. add ",app.kubernetes.io/component=database") or choose
component dynamically when you need pooler vs database; modify the code that
builds selector for object.kind == "CNPGCluster" to include the appropriate
app.kubernetes.io/component value so downstream references to the postgres
container are safe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ad6f0bde-0ef3-4b2f-b752-e7128e19c6f3
📒 Files selected for processing (7)
robusta_krr/core/integrations/kubernetes/__init__.pyrobusta_krr/core/models/objects.pyrobusta_krr/formatters/csv.pyrobusta_krr/formatters/csv_raw.pyrobusta_krr/formatters/table.pyrobusta_krr/main.pytests/single_namespace_permissions.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- robusta_krr/main.py
- robusta_krr/formatters/csv_raw.py
- robusta_krr/formatters/table.py
- robusta_krr/formatters/csv.py
- robusta_krr/core/models/objects.py
| elif object.kind == "CNPGCluster": | ||
| selector = f"cnpg.io/cluster={object.name}" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
CloudNativePG cnpg.io/cluster label pooler database app.kubernetes.io/component
💡 Result:
In CloudNativePG, the operator applies standard Kubernetes recommended labels to the resources it generates:
cnpg.io/cluster: <CLUSTER_NAME>→ identifies the owning Cluster name. [1]app.kubernetes.io/component: database|pooler|...→ identifies the resource “component”; for PostgreSQL instance resources it’s typicallydatabase, and for PgBouncer resources it’spooler. [1]- (Pooler-specific)
cnpg.io/poolerName: <POOLER_NAME>→ identifies the PgBouncer Pooler name. [1]
Example selectors:
- Database pods:
-l cnpg.io/cluster=<cluster>,app.kubernetes.io/component=database[1] - Pooler pods:
-l cnpg.io/cluster=<cluster>,app.kubernetes.io/component=pooler(optionally add,cnpg.io/poolerName=<pooler>). [1]
Sources: [1]
🏁 Script executed:
# First, let's examine the full context of the code around the flagged selector
cd /tmp && find . -type f -name "__init__.py" -path "*/kubernetes/*" 2>/dev/null | head -5Repository: robusta-dev/krr
Length of output: 41
🏁 Script executed:
# Look at the file structure to locate the file mentioned in the review
find . -type f -name "__init__.py" -path "*robusta_krr*kubernetes*" 2>/dev/nullRepository: robusta-dev/krr
Length of output: 113
🏁 Script executed:
# Examine the ClusterLoader class and surrounding context, especially the pod discovery and container lookup logic
cat -n robusta_krr/core/integrations/kubernetes/__init__.py | sed -n '1,200p'Repository: robusta-dev/krr
Length of output: 9205
🏁 Script executed:
# Search for postgres container name in the file
rg -n "postgres" robusta_krr/core/integrations/kubernetes/__init__.pyRepository: robusta-dev/krr
Length of output: 206
🏁 Script executed:
# Look for __build_scannable_object method and related container lookup logic
rg -n "__build_scannable_object|_get_container_from_pod" robusta_krr/core/integrations/kubernetes/__init__.py -A 3Repository: robusta-dev/krr
Length of output: 1039
🏁 Script executed:
# Get more context around the container discovery, especially for CNPGCluster
cat -n robusta_krr/core/integrations/kubernetes/__init__.py | sed -n '240,350p'Repository: robusta-dev/krr
Length of output: 5069
🏁 Script executed:
# Search for any verification of container existence or pooler-related filtering
rg -n "container|pooler|Pooler" robusta_krr/core/integrations/kubernetes/__init__.py | head -20Repository: robusta-dev/krr
Length of output: 1561
🏁 Script executed:
# Check if there's error handling when a container is not found in a pod
rg -n "get_container_from_pod|_get_container" robusta_krr/core/integrations/kubernetes/__init__.py -A 5Repository: robusta-dev/krr
Length of output: 41
🏁 Script executed:
# Search for any validation or filtering of pods after they're discovered
rg -n "list_pods|PodData" robusta_krr/core/integrations/kubernetes/__init__.py -B 2 -A 2 | head -40Repository: robusta-dev/krr
Length of output: 1298
Add component selector to CNPG pod discovery.
CloudNativePG applies cnpg.io/cluster to both database and pooler pods, but distinguishes them with app.kubernetes.io/component=database for database instances and app.kubernetes.io/component=pooler for PgBouncer poolers. The current selector uses only the cluster label and will match both pod types, but the code hard-codes the container name to postgres, which only exists on database pods. This causes an inconsistency that will fail when downstream code attempts to reference the container.
🔧 Suggested fix
elif object.kind == "CNPGCluster":
- selector = f"cnpg.io/cluster={object.name}"
+ selector = f"cnpg.io/cluster={object.name},app.kubernetes.io/component=database"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@robusta_krr/core/integrations/kubernetes/__init__.py` around lines 176 - 177,
The CNPGCluster selector currently only uses the cluster label (selector =
f"cnpg.io/cluster={object.name}") which matches both database and pooler pods
while the code assumes a postgres container; update the selector in the
CNPGCluster branch so it includes the component label to restrict to the
database pods (e.g. add ",app.kubernetes.io/component=database") or choose
component dynamically when you need pooler vs database; modify the code that
builds selector for object.kind == "CNPGCluster" to include the appropriate
app.kubernetes.io/component value so downstream references to the postgres
container are safe.
2 participants
Related to #139