-
Notifications
You must be signed in to change notification settings - Fork 292
Fix/improve guess cached pod name #1342
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3,9 +3,12 @@ | |||||||||||||||||||||
| from collections import defaultdict | ||||||||||||||||||||||
| from typing import Dict, List, Optional | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| from hikaru.model.rel_1_26 import OwnerReference | ||||||||||||||||||||||
| from pydantic.main import BaseModel | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| from robusta.core.model.env_vars import RESOURCE_UPDATES_CACHE_TTL_SEC | ||||||||||||||||||||||
| from robusta.core.reporting.findings import FindingOwner | ||||||||||||||||||||||
| from robusta.integrations.kubernetes.custom_models import RobustaPod | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| class TopLevelResource(BaseModel): | ||||||||||||||||||||||
|
|
@@ -26,6 +29,7 @@ class TopServiceResolver: | |||||||||||||||||||||
| __recent_resource_updates: Dict[str, CachedResourceInfo] = {} | ||||||||||||||||||||||
| __namespace_to_resource: Dict[str, List[TopLevelResource]] = defaultdict(list) | ||||||||||||||||||||||
| __cached_updates_lock = threading.Lock() | ||||||||||||||||||||||
| __cached_owner_references: Dict[str, OwnerReference] = {} | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @classmethod | ||||||||||||||||||||||
| def store_cached_resources(cls, resources: List[TopLevelResource]): | ||||||||||||||||||||||
|
|
@@ -51,20 +55,62 @@ def store_cached_resources(cls, resources: List[TopLevelResource]): | |||||||||||||||||||||
| # TODO remove this guess function | ||||||||||||||||||||||
| # temporary try to guess who the owner service is. | ||||||||||||||||||||||
| @classmethod | ||||||||||||||||||||||
| def guess_service_key(cls, name: str, namespace: str) -> str: | ||||||||||||||||||||||
| resource = cls.guess_cached_resource(name, namespace) | ||||||||||||||||||||||
| def guess_service_key(cls, name: str, namespace: str, kind: str, owner: Optional[FindingOwner]) -> str: | ||||||||||||||||||||||
| resource = cls.guess_cached_resource(name, namespace, kind=kind, owner=owner) | ||||||||||||||||||||||
| return resource.get_resource_key() if resource else "" | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| # TODO remove this guess function | ||||||||||||||||||||||
| # temporary try to guess who the owner service is. | ||||||||||||||||||||||
| @classmethod | ||||||||||||||||||||||
| def guess_cached_resource(cls, name: str, namespace: str) -> Optional[TopLevelResource]: | ||||||||||||||||||||||
| 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 | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @classmethod | ||||||||||||||||||||||
| def guess_cached_resource(cls, name: str, namespace: str, kind: str, owner: Optional[FindingOwner]) \ | ||||||||||||||||||||||
| -> Optional[TopLevelResource]: | ||||||||||||||||||||||
| if name is None or namespace is None: | ||||||||||||||||||||||
| return None | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| for cached_resource in cls.__namespace_to_resource[namespace]: | ||||||||||||||||||||||
| if name.startswith(cached_resource.name): | ||||||||||||||||||||||
| return cached_resource | ||||||||||||||||||||||
| kind = kind.lower() | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| # owner references available | ||||||||||||||||||||||
| if owner and owner.owner_references: | ||||||||||||||||||||||
| owner_kind = owner.owner_references[0].kind.lower() | ||||||||||||||||||||||
| owner_reference = owner.owner_references[0] | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if owner_kind in ["deployment", "statefulset", "daemonset", "job", "deploymentconfig", | ||||||||||||||||||||||
| "argorollout"]: | ||||||||||||||||||||||
| return TopLevelResource(name=owner_reference.name, resource_type=owner_reference.kind, | ||||||||||||||||||||||
| namespace=namespace) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| # replicset | ||||||||||||||||||||||
| 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) | ||||||||||||||||||||||
|
Comment on lines
+94
to
+97
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handle potential None return from get_pod_owner_reference. The code doesn't handle the case where # 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| # crd | ||||||||||||||||||||||
| if owner_kind not in ["deployment", "statefulset", "daemonset", "job", "deploymentconfig", | ||||||||||||||||||||||
| "argorollout", "pod"]: | ||||||||||||||||||||||
| return TopLevelResource(name=name, resource_type=kind, namespace=namespace) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| # owner references NOT available | ||||||||||||||||||||||
| if owner is None or not owner.owner_references: | ||||||||||||||||||||||
| return TopLevelResource(name=name, resource_type=kind, namespace=namespace) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| # unknown owner | ||||||||||||||||||||||
| if owner.unknown_owner: | ||||||||||||||||||||||
| for cached_resource in cls.__namespace_to_resource[namespace]: | ||||||||||||||||||||||
| if name.startswith(cached_resource.name): | ||||||||||||||||||||||
| return cached_resource | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| return None | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @classmethod | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| from typing import Optional, List | ||
|
|
||
| from hikaru.model.rel_1_26 import OwnerReference | ||
| from pydantic import BaseModel | ||
|
|
||
|
|
||
| class FindingOwner(BaseModel): | ||
| owner_references: Optional[List[OwnerReference]] = None | ||
| unknown_owner: bool = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add error handling for pod retrieval.
The method lacks error handling for cases where
RobustaPod.find_podmight 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
🤖 Prompt for AI Agents