Fix/improve guess cached pod name #1342
Conversation
# Conflicts: # src/robusta/core/reporting/base.py # src/robusta/integrations/kubernetes/autogenerated/events.py
WalkthroughThis update introduces the Changes
Sequence Diagram(s)sequenceDiagram
participant EventSource as Event Source
participant EventHandler as Event Handler
participant FindingSubject
participant TopServiceResolver
participant FindingOwner
EventSource->>EventHandler: Receives Kubernetes event
EventHandler->>FindingOwner: Extract ownerReferences from event metadata
EventHandler->>FindingSubject: Create with owner=FindingOwner(...)
EventHandler->>TopServiceResolver: guess_service_key(name, namespace, kind, owner)
TopServiceResolver->>FindingOwner: Use owner info to resolve top-level resource
TopServiceResolver-->>EventHandler: Returns service key
EventHandler-->>EventSource: Processed event with enriched ownership context
sequenceDiagram
participant Prometheus as Prometheus Alert
participant AlertHandler as Alert Handler
participant FindingSubject
participant FindingOwner
Prometheus->>AlertHandler: Receives alert with resource metadata
AlertHandler->>FindingOwner: Extract ownerReferences from resource metadata
AlertHandler->>FindingSubject: Create with owner=FindingOwner(...)
AlertHandler-->>Prometheus: Returns enriched FindingSubject
sequenceDiagram
participant UserCode as User Code
participant Finding as Finding
participant TopServiceResolver
participant FindingOwner
UserCode->>Finding: Create Finding with FindingSubject(owner=FindingOwner)
Finding->>TopServiceResolver: guess_cached_resource(name, namespace, kind, owner)
TopServiceResolver->>FindingOwner: Use ownerReferences to find top-level resource
TopServiceResolver-->>Finding: Returns resolved resource info
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
playbooks/robusta_playbooks/babysitter.py (1)
24-24: Remove unused import.The
FindingOwnerimport is not used anywhere in this file and should be removed to avoid dead code.-from robusta.core.reporting.findings import FindingOwnersrc/robusta/core/discovery/top_service_resolver.py (1)
76-114: Consider refactoring to reduce complexity.The static analysis correctly identifies that this method has too many return statements (7/6). Consider extracting helper methods to improve readability and maintainability.
The logic could be broken down into smaller, focused methods like:
_handle_owner_references()_handle_unknown_owner()_handle_no_owner()This would make the code more testable and easier to understand.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
playbooks/robusta_playbooks/babysitter.py(1 hunks)playbooks/robusta_playbooks/event_enrichments.py(2 hunks)scripts/generate_kubernetes_code.py(4 hunks)src/robusta/core/discovery/top_service_resolver.py(3 hunks)src/robusta/core/playbooks/internal/discovery_events.py(3 hunks)src/robusta/core/reporting/base.py(4 hunks)src/robusta/core/reporting/consts.py(2 hunks)src/robusta/core/reporting/finding_subjects.py(3 hunks)src/robusta/core/reporting/findings.py(1 hunks)src/robusta/core/triggers/error_event_trigger.py(2 hunks)src/robusta/integrations/kubernetes/autogenerated/events.py(39 hunks)src/robusta/integrations/prometheus/models.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
playbooks/robusta_playbooks/babysitter.py
24-24: robusta.core.reporting.findings.FindingOwner imported but unused
Remove unused import: robusta.core.reporting.findings.FindingOwner
(F401)
src/robusta/integrations/kubernetes/autogenerated/events.py
102-102: Undefined name FindingOwner
(F821)
165-165: Undefined name FindingOwner
(F821)
189-189: Undefined name FindingOwner
(F821)
223-223: Undefined name FindingOwner
(F821)
247-247: Undefined name FindingOwner
(F821)
281-281: Undefined name FindingOwner
(F821)
305-305: Undefined name FindingOwner
(F821)
339-339: Undefined name FindingOwner
(F821)
363-363: Undefined name FindingOwner
(F821)
397-397: Undefined name FindingOwner
(F821)
421-421: Undefined name FindingOwner
(F821)
455-455: Undefined name FindingOwner
(F821)
479-479: Undefined name FindingOwner
(F821)
513-513: Undefined name FindingOwner
(F821)
537-537: Undefined name FindingOwner
(F821)
571-571: Undefined name FindingOwner
(F821)
595-595: Undefined name FindingOwner
(F821)
629-629: Undefined name FindingOwner
(F821)
653-653: Undefined name FindingOwner
(F821)
687-687: Undefined name FindingOwner
(F821)
711-711: Undefined name FindingOwner
(F821)
745-745: Undefined name FindingOwner
(F821)
769-769: Undefined name FindingOwner
(F821)
803-803: Undefined name FindingOwner
(F821)
827-827: Undefined name FindingOwner
(F821)
861-861: Undefined name FindingOwner
(F821)
885-885: Undefined name FindingOwner
(F821)
919-919: Undefined name FindingOwner
(F821)
943-943: Undefined name FindingOwner
(F821)
977-977: Undefined name FindingOwner
(F821)
1001-1001: Undefined name FindingOwner
(F821)
1035-1035: Undefined name FindingOwner
(F821)
1059-1059: Undefined name FindingOwner
(F821)
1093-1093: Undefined name FindingOwner
(F821)
1117-1117: Undefined name FindingOwner
(F821)
1151-1151: Undefined name FindingOwner
(F821)
1175-1175: Undefined name FindingOwner
(F821)
1209-1209: Undefined name FindingOwner
(F821)
1233-1233: Undefined name FindingOwner
(F821)
🪛 Pylint (3.3.7)
src/robusta/core/reporting/findings.py
[refactor] 7-7: Too few public methods (0/2)
(R0903)
src/robusta/core/discovery/top_service_resolver.py
[refactor] 76-76: Too many return statements (7/6)
(R0911)
src/robusta/integrations/kubernetes/autogenerated/events.py
[error] 102-102: Undefined variable 'FindingOwner'
(E0602)
[error] 165-165: Undefined variable 'FindingOwner'
(E0602)
[error] 189-189: Undefined variable 'FindingOwner'
(E0602)
[error] 223-223: Undefined variable 'FindingOwner'
(E0602)
[error] 247-247: Undefined variable 'FindingOwner'
(E0602)
[error] 281-281: Undefined variable 'FindingOwner'
(E0602)
[error] 305-305: Undefined variable 'FindingOwner'
(E0602)
[error] 339-339: Undefined variable 'FindingOwner'
(E0602)
[error] 363-363: Undefined variable 'FindingOwner'
(E0602)
[error] 397-397: Undefined variable 'FindingOwner'
(E0602)
[error] 421-421: Undefined variable 'FindingOwner'
(E0602)
[error] 455-455: Undefined variable 'FindingOwner'
(E0602)
[error] 479-479: Undefined variable 'FindingOwner'
(E0602)
[error] 513-513: Undefined variable 'FindingOwner'
(E0602)
[error] 537-537: Undefined variable 'FindingOwner'
(E0602)
[error] 571-571: Undefined variable 'FindingOwner'
(E0602)
[error] 595-595: Undefined variable 'FindingOwner'
(E0602)
[error] 629-629: Undefined variable 'FindingOwner'
(E0602)
[error] 653-653: Undefined variable 'FindingOwner'
(E0602)
[error] 687-687: Undefined variable 'FindingOwner'
(E0602)
[error] 711-711: Undefined variable 'FindingOwner'
(E0602)
[error] 745-745: Undefined variable 'FindingOwner'
(E0602)
[error] 769-769: Undefined variable 'FindingOwner'
(E0602)
[error] 803-803: Undefined variable 'FindingOwner'
(E0602)
[error] 827-827: Undefined variable 'FindingOwner'
(E0602)
[error] 861-861: Undefined variable 'FindingOwner'
(E0602)
[error] 885-885: Undefined variable 'FindingOwner'
(E0602)
[error] 919-919: Undefined variable 'FindingOwner'
(E0602)
[error] 943-943: Undefined variable 'FindingOwner'
(E0602)
[error] 977-977: Undefined variable 'FindingOwner'
(E0602)
[error] 1001-1001: Undefined variable 'FindingOwner'
(E0602)
[error] 1035-1035: Undefined variable 'FindingOwner'
(E0602)
[error] 1059-1059: Undefined variable 'FindingOwner'
(E0602)
[error] 1093-1093: Undefined variable 'FindingOwner'
(E0602)
[error] 1117-1117: Undefined variable 'FindingOwner'
(E0602)
[error] 1151-1151: Undefined variable 'FindingOwner'
(E0602)
[error] 1175-1175: Undefined variable 'FindingOwner'
(E0602)
[error] 1209-1209: Undefined variable 'FindingOwner'
(E0602)
[error] 1233-1233: Undefined variable 'FindingOwner'
(E0602)
🔇 Additional comments (23)
playbooks/robusta_playbooks/event_enrichments.py (2)
43-43: Import looks good.The
FindingOwnerimport is properly used in the file.
76-76: Ownership enrichment implemented correctly.The
FindingOwneris properly constructed from the event's owner references and passed to theFindingSubject. This enriches the finding with ownership metadata as intended.src/robusta/core/reporting/findings.py (1)
1-10: Well-structured data model for ownership metadata.The
FindingOwnerclass properly encapsulates Kubernetes ownership information with appropriate fields and types. The Pylint warning about too few public methods is a false positive for data models.src/robusta/core/reporting/consts.py (1)
50-50: Consistent enum extension for replicaset support.The
TYPE_REPLICASETenum value and correspondingfrom_kindmethod case are properly added together, maintaining consistency in the enum implementation.Also applies to: 66-67
src/robusta/core/triggers/error_event_trigger.py (2)
5-5: Import properly used.The
FindingOwnerimport is correctly utilized in the service key resolution logic.
74-77: Enhanced service key resolution with ownership context.The changes correctly extract the
kindand create aFindingOwnerinstance with owner references to provide richer context for service key resolution. This improvement aligns with the broader ownership metadata integration.src/robusta/core/playbooks/internal/discovery_events.py (3)
19-19: LGTM: Import addition is consistent with the ownership enhancement.The import of
FindingOwneraligns with the broader changes to integrate ownership metadata.
85-87: LGTM: Enhanced service key resolution with ownership context.The addition of
kindandownerparameters toTopServiceResolver.guess_service_keyprovides better context for service resolution. The implementation correctly passes the owner from the finding's subject.
81-81: ```shell
#!/usr/bin/env bashLocate the file defining FindingOwner and inspect its constructor for None handling
1. Find class declaration
rg --no-heading "class FindingOwner" -n .
2. If found, display the class definition and init body
FILE=$(rg -l "class FindingOwner" . | head -n1)
if [[ -n "$FILE" ]]; then
echo "=== Definition in $FILE ==="
rg -n "class FindingOwner" -A 15 "$FILE"
echo
echo "=== init in $FILE ==="
rg -n "def init" -A 10 "$FILE"
else
echo "FindingOwner class not found in the repository."
fi</details> <details> <summary>src/robusta/core/reporting/base.py (3)</summary> `17-17`: **LGTM: Import addition supports ownership metadata integration.** The import of `FindingOwner` is correctly added to support the new ownership functionality. --- `205-205`: **LGTM: Well-designed optional owner parameter.** The `owner` parameter is properly implemented as optional with correct type annotation, maintaining backward compatibility while enabling ownership metadata. Also applies to: 214-214 --- `257-259`: **LGTM: Enhanced service resolution with ownership context.** The `TopServiceResolver.guess_cached_resource` call now includes ownership information, providing better context for service resolution. The parameters are correctly passed from the subject's attributes. </details> <details> <summary>src/robusta/core/reporting/finding_subjects.py (2)</summary> `4-4`: **LGTM: Import addition enables ownership metadata support.** The import of `FindingOwner` is correctly added to support the new ownership functionality in finding subjects. --- `27-27`: ```shell #!/bin/bash # Locate the FindingOwner class definition and inspect its __init__ for None handling rg -n "class FindingOwner" -C 5src/robusta/integrations/prometheus/models.py (3)
9-9: LGTM: Imports correctly added for ownership metadata support.The imports of
OwnerReferenceandFindingOwnerare properly added to support the enhanced ownership functionality.Also applies to: 13-13
143-197: LGTM: Comprehensive ownership metadata extraction.The implementation systematically extracts
ownerReferencesfrom all supported Kubernetes resource types (deployment, daemonset, statefulset, node, pod, job, hpa) and correctly sets theunknown_ownerflag when no specific resource type is matched. This provides comprehensive ownership context for Prometheus alerts.
202-203: LGTM: Enhanced FindingSubject with ownership information.The
FindingSubjectconstructor now includes theownerparameter with properly constructedFindingOwnerobject, maintaining consistency with the broader ownership enhancement across the codebase.scripts/generate_kubernetes_code.py (3)
80-80: LGTM: Import added to autogenerated code template.The import of
FindingOwneris correctly added to the code generation template, ensuring it will be available in all autogenerated Kubernetes event classes.
181-190: LGTM: Consistent ownership metadata integration in autogenerated events.The pattern of adding
owner=FindingOwner(owner_references=self.obj.metadata.ownerReferences)is consistently applied across all autogenerated event classes (base KubernetesResourceEvent, specific resource events, and change events). This ensures uniform ownership metadata support across the entire Kubernetes event system.Also applies to: 265-274, 289-298
189-189: ```shell
#!/bin/bashSearch for the FindingOwner class definition and inspect its constructor
rg "class FindingOwner" -A10 -n
</details> <details> <summary>src/robusta/core/discovery/top_service_resolver.py (3)</summary> `6-6`: **LGTM! Required imports properly added.** The imports are correctly added to support the new ownership-aware functionality. Also applies to: 10-11 --- `32-32`: **LGTM! Caching strategy for owner references.** Adding a cache for owner references is a good optimization to avoid repeated API calls. --- `58-60`: **LGTM! Method signature properly updated.** The method signature correctly includes the new ownership parameters and passes them through to the updated guess_cached_resource method. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| def get_pod_owner_reference(cls, name: str, namespace: str) -> Optional[OwnerReference]: | ||
| key = f"{namespace}/{name}" | ||
| if key in cls.__cached_owner_references: | ||
| return cls.__cached_owner_references[key] | ||
|
|
||
| robusta_pod = RobustaPod.find_pod(name, namespace) | ||
| if robusta_pod.metadata.ownerReferences: | ||
| cls.__cached_owner_references[key] = robusta_pod.metadata.ownerReferences[0] | ||
| return robusta_pod.metadata.ownerReferences[0] | ||
|
|
||
| return None |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for pod retrieval.
The method lacks error handling for cases where RobustaPod.find_pod might fail or return None.
@classmethod
def get_pod_owner_reference(cls, name: str, namespace: str) -> Optional[OwnerReference]:
key = f"{namespace}/{name}"
if key in cls.__cached_owner_references:
return cls.__cached_owner_references[key]
+ try:
robusta_pod = RobustaPod.find_pod(name, namespace)
- if robusta_pod.metadata.ownerReferences:
+ if robusta_pod and robusta_pod.metadata and robusta_pod.metadata.ownerReferences:
cls.__cached_owner_references[key] = robusta_pod.metadata.ownerReferences[0]
return robusta_pod.metadata.ownerReferences[0]
+ except Exception:
+ # Log error or handle gracefully
+ pass
return None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_pod_owner_reference(cls, name: str, namespace: str) -> Optional[OwnerReference]: | |
| key = f"{namespace}/{name}" | |
| if key in cls.__cached_owner_references: | |
| return cls.__cached_owner_references[key] | |
| robusta_pod = RobustaPod.find_pod(name, namespace) | |
| if robusta_pod.metadata.ownerReferences: | |
| cls.__cached_owner_references[key] = robusta_pod.metadata.ownerReferences[0] | |
| return robusta_pod.metadata.ownerReferences[0] | |
| return None | |
| def get_pod_owner_reference(cls, name: str, namespace: str) -> Optional[OwnerReference]: | |
| key = f"{namespace}/{name}" | |
| if key in cls.__cached_owner_references: | |
| return cls.__cached_owner_references[key] | |
| try: | |
| robusta_pod = RobustaPod.find_pod(name, namespace) | |
| if robusta_pod and robusta_pod.metadata and robusta_pod.metadata.ownerReferences: | |
| cls.__cached_owner_references[key] = robusta_pod.metadata.ownerReferences[0] | |
| return robusta_pod.metadata.ownerReferences[0] | |
| except Exception: | |
| # Log error or handle gracefully | |
| pass | |
| return None |
🤖 Prompt for AI Agents
In src/robusta/core/discovery/top_service_resolver.py around lines 63 to 73, the
method get_pod_owner_reference does not handle errors or None returns from
RobustaPod.find_pod. Add a check after calling find_pod to verify the pod was
found; if not, handle the error gracefully by returning None or logging an
appropriate message. This prevents attribute errors when accessing metadata on a
None object.
| if owner_kind == "replicaset": | ||
| new_owner_reference = cls.get_pod_owner_reference(name=owner_reference.name, namespace=namespace) | ||
| return TopLevelResource(name=new_owner_reference.name, resource_type=new_owner_reference.kind, | ||
| namespace=namespace) |
There was a problem hiding this comment.
Handle potential None return from get_pod_owner_reference.
The code doesn't handle the case where get_pod_owner_reference returns None, which could cause an AttributeError.
# replicset
if owner_kind == "replicaset":
new_owner_reference = cls.get_pod_owner_reference(name=owner_reference.name, namespace=namespace)
+ if not new_owner_reference:
+ return TopLevelResource(name=name, resource_type=kind, namespace=namespace)
return TopLevelResource(name=new_owner_reference.name, resource_type=new_owner_reference.kind,
namespace=namespace)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if owner_kind == "replicaset": | |
| new_owner_reference = cls.get_pod_owner_reference(name=owner_reference.name, namespace=namespace) | |
| return TopLevelResource(name=new_owner_reference.name, resource_type=new_owner_reference.kind, | |
| namespace=namespace) | |
| if owner_kind == "replicaset": | |
| new_owner_reference = cls.get_pod_owner_reference(name=owner_reference.name, namespace=namespace) | |
| if not new_owner_reference: | |
| return TopLevelResource(name=name, resource_type=kind, namespace=namespace) | |
| return TopLevelResource(name=new_owner_reference.name, resource_type=new_owner_reference.kind, | |
| namespace=namespace) |
🤖 Prompt for AI Agents
In src/robusta/core/discovery/top_service_resolver.py around lines 94 to 97, the
call to get_pod_owner_reference may return None, leading to an AttributeError
when accessing its attributes. Add a check after calling get_pod_owner_reference
to verify the result is not None before accessing its name and kind. If it is
None, handle this case appropriately, such as returning None or raising a clear
exception.
| node=KubeObjFindingSubject.get_node_name(self.obj), | ||
| labels=self.obj.metadata.labels, | ||
| annotations=self.obj.metadata.annotations, | ||
| owner=FindingOwner(owner_references=self.obj.metadata.ownerReferences) |
There was a problem hiding this comment.
Critical: Missing import for FindingOwner.
The file uses FindingOwner throughout but doesn't import it, which will cause runtime failures.
Add the missing import at the top of the file:
from ....core.reporting.base import FindingSubject
from ....core.reporting.consts import FindingSubjectType, FindingSource
+from ....core.reporting.findings import FindingOwner
from ....core.reporting.finding_subjects import KubeObjFindingSubjectThe pattern of adding ownership information to FindingSubject instances is consistent and correct across all event classes.
Also applies to: 165-165, 189-189, 223-223, 247-247, 281-281, 305-305, 339-339, 363-363, 397-397, 421-421, 455-455, 479-479, 513-513, 537-537, 571-571, 595-595, 629-629, 653-653, 687-687, 711-711, 745-745, 769-769, 803-803, 827-827, 861-861, 885-885, 919-919, 943-943, 977-977, 1001-1001, 1035-1035, 1059-1059, 1093-1093, 1117-1117, 1151-1151, 1175-1175, 1209-1209, 1233-1233
🧰 Tools
🪛 Ruff (0.11.9)
102-102: Undefined name FindingOwner
(F821)
🪛 Pylint (3.3.7)
[error] 102-102: Undefined variable 'FindingOwner'
(E0602)
🤖 Prompt for AI Agents
In src/robusta/integrations/kubernetes/autogenerated/events.py at lines 102,
165, 189, 223, 247, 281, 305, 339, 363, 397, 421, 455, 479, 513, 537, 571, 595,
629, 653, 687, 711, 745, 769, 803, 827, 861, 885, 919, 943, 977, 1001, 1035,
1059, 1093, 1117, 1151, 1175, 1209, and 1233, the code uses FindingOwner but
does not import it, causing runtime errors. To fix this, add the appropriate
import statement for FindingOwner at the top of the file, ensuring it is
imported from its correct module so all references to FindingOwner are resolved
properly.
No description provided.