From 032ad5a7ebf9c78e5353afa1adfa068e8797b985 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 07:32:19 +0000 Subject: [PATCH 1/2] Stop logging an ERROR for pods whose status is not populated yet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit is_pod_ready() defaulted `conditions = []` and then overwrote it with `pod.status.conditions` unconditionally. That field is None on a pod the API server has accepted but kubelet has not reported on yet, so the iteration raised TypeError: 'NoneType' object is not iterable. extract_ready_pods() caught it and logged an ERROR, returning 0 via the exception path instead of the correct 0. In 48h of runner logs that is 12 occurrences, every one a GKE cluster-autoscaler gke-system-balloon-pod-* discovered in the same second it was created. The outcome was harmless but it is a swallowed crash, not an expected condition, so fix the read: `pod.status.conditions or []`, matching the idiom already used in core/model/jobs.py. Also stop interpolating the resource itself into the extract_* failure logs. Each of those 12 ERRORs dumped the entire pretty-printed V1Pod — 182 to 377 lines apiece, about 2,900 lines of a 3,193-line log — which buries the traceback that actually matters and would do the same for a genuine failure. Log `Kind namespace/name` via a new resource_ref() helper and keep the full object at debug level. Applied to all six extract_* handlers (containers, ready pods, total pods, volumes) since they shared the pattern. Signed-off-by: Claude --- src/robusta/core/discovery/discovery.py | 49 ++++++++++++++--- tests/discovery/test_discovery.py | 73 ++++++++++++++++++++++++- 2 files changed, 113 insertions(+), 9 deletions(-) diff --git a/src/robusta/core/discovery/discovery.py b/src/robusta/core/discovery/discovery.py index d3277a864..ea044dddf 100644 --- a/src/robusta/core/discovery/discovery.py +++ b/src/robusta/core/discovery/discovery.py @@ -854,6 +854,28 @@ def discover_stats() -> ClusterStats: ) +def resource_ref(resource) -> str: + """Short `Kind namespace/name` reference for logging. + + The extract_* helpers below log on failure. Interpolating the resource + itself dumps its entire pretty-printed spec/status — a few hundred lines per + pod, which drowns the log and the traceback that matters. Identify the + resource instead; the full object is available at debug level. + """ + try: + kind = getattr(resource, "kind", None) or type(resource).__name__ + metadata = getattr(resource, "metadata", None) + name = getattr(metadata, "name", None) + if not name: + # Nothing to identify it by — the kind alone still beats a full dump, + # and the traceback carries the rest. + return str(kind) + namespace = getattr(metadata, "namespace", None) + return f"{kind} {namespace}/{name}" if namespace else f"{kind} {name}" + except Exception: + return type(resource).__name__ + + # This section below contains utility related to k8s python api objects (rather than hikaru) def extract_containers(resource) -> List[V1Container]: """Extract containers from k8s python api object (not hikaru)""" @@ -871,7 +893,8 @@ def extract_containers(resource) -> List[V1Container]: return containers except Exception: # may fail if one of the attributes is None - logging.error(f"Failed to extract containers from {resource}", exc_info=True) + logging.error(f"Failed to extract containers from {resource_ref(resource)}", exc_info=True) + logging.debug(f"Resource that failed containers extraction: {resource}") return [] @@ -892,17 +915,23 @@ def extract_containers_k8(resource) -> List[Container]: return containers except Exception: # may fail if one of the attributes is None - logging.error(f"Failed to extract containers from {resource}", exc_info=True) + logging.error(f"Failed to extract containers from {resource_ref(resource)}", exc_info=True) + logging.debug(f"Resource that failed containers extraction: {resource}") return [] def is_pod_ready(pod) -> bool: + # `status.conditions` is None on a pod the API server has accepted but + # kubelet has not reported on yet (routinely seen on autoscaler-created + # pods discovered in the same second they were created). Without the + # `or []` the iteration below raised TypeError, which extract_ready_pods + # caught and logged as an ERROR with the whole pod object dumped into it. conditions = [] if isinstance(pod, V1Pod): - conditions = pod.status.conditions + conditions = pod.status.conditions or [] if isinstance(pod, Pod): - conditions = pod.status.conditions + conditions = pod.status.conditions or [] for condition in conditions: if condition.type == "Ready": @@ -957,7 +986,8 @@ def extract_ready_pods(resource) -> int: return 0 except Exception: # fields may not exist if all the pods are not ready - example: deployment crashpod - logging.error(f"Failed to extract ready pods from {resource}", exc_info=True) + logging.error(f"Failed to extract ready pods from {resource_ref(resource)}", exc_info=True) + logging.debug(f"Resource that failed ready pods extraction: {resource}") return 0 @@ -980,7 +1010,8 @@ def extract_total_pods(resource) -> int: return 1 return 0 except Exception: - logging.error(f"Failed to extract total pods from {resource}", exc_info=True) + logging.error(f"Failed to extract total pods from {resource_ref(resource)}", exc_info=True) + logging.debug(f"Resource that failed total pods extraction: {resource}") return 1 @@ -1023,7 +1054,8 @@ def extract_volumes(resource) -> List[V1Volume]: volumes = resource.spec.volumes return volumes except Exception: # may fail if one of the attributes is None - logging.error(f"Failed to extract volumes from {resource}", exc_info=True) + logging.error(f"Failed to extract volumes from {resource_ref(resource)}", exc_info=True) + logging.debug(f"Resource that failed volumes extraction: {resource}") return [] @@ -1042,5 +1074,6 @@ def extract_volumes_k8(resource) -> List[Volume]: volumes = resource.spec.volumes return volumes except Exception: # may fail if one of the attributes is None - logging.error(f"Failed to extract volumes from {resource}", exc_info=True) + logging.error(f"Failed to extract volumes from {resource_ref(resource)}", exc_info=True) + logging.debug(f"Resource that failed volumes extraction: {resource}") return [] diff --git a/tests/discovery/test_discovery.py b/tests/discovery/test_discovery.py index b50317eb5..d98b705e8 100644 --- a/tests/discovery/test_discovery.py +++ b/tests/discovery/test_discovery.py @@ -1,3 +1,4 @@ +import logging import signal from concurrent.futures import ProcessPoolExecutor from contextlib import contextmanager @@ -7,9 +8,15 @@ import kubernetes import pytest +from kubernetes.client import V1ObjectMeta, V1Pod, V1PodCondition, V1PodStatus from kubernetes.client.exceptions import ApiException -from robusta.core.discovery.discovery import Discovery +from robusta.core.discovery.discovery import ( + Discovery, + extract_ready_pods, + is_pod_ready, + resource_ref, +) # pytest-timeout requires pytest>=7, https://github.com/pytest-dev/pytest-timeout/blob/main/setup.cfg @@ -42,3 +49,67 @@ def test_discovery_recovery_on_failure(): assert patched_pool._shutdown_thread assert not Discovery.executor._shutdown_thread + + +# ---- is_pod_ready / extract_ready_pods with an unpopulated status ---- + + +def _v1_pod(conditions: Any, name: str = "balloon-pod-pcrkw") -> V1Pod: + return V1Pod( + metadata=V1ObjectMeta(name=name, namespace="kube-system"), + status=V1PodStatus(conditions=conditions), + ) + + +def test_is_pod_ready_handles_unset_conditions(): + """A pod the API server accepted but kubelet has not reported on yet has + `status.conditions is None`. This used to raise TypeError, which + extract_ready_pods swallowed into an ERROR log per discovery cycle.""" + assert is_pod_ready(_v1_pod(None)) is False + + +def test_is_pod_ready_handles_empty_conditions(): + assert is_pod_ready(_v1_pod([])) is False + + +@pytest.mark.parametrize( + "status,expected", [("True", True), ("False", False), ("Unknown", False)] +) +def test_is_pod_ready_reads_ready_condition(status: str, expected: bool): + pod = _v1_pod([V1PodCondition(type="Ready", status=status)]) + assert is_pod_ready(pod) is expected + + +def test_is_pod_ready_ignores_other_conditions(): + pod = _v1_pod( + [ + V1PodCondition(type="PodScheduled", status="True"), + V1PodCondition(type="Initialized", status="True"), + ] + ) + assert is_pod_ready(pod) is False + + +def test_extract_ready_pods_does_not_log_for_unset_conditions(caplog): + """The regression this guards: 12 ERROR blocks in 48h of runner logs, each + dumping the whole pod object.""" + with caplog.at_level(logging.ERROR): + assert extract_ready_pods(_v1_pod(None)) == 0 + assert not [r for r in caplog.records if "Failed to extract ready pods" in r.getMessage()] + + +def test_extract_ready_pods_counts_ready_pod(): + pod = _v1_pod([V1PodCondition(type="Ready", status="True")]) + assert extract_ready_pods(pod) == 1 + + +def test_resource_ref_identifies_resource_without_dumping_it(): + """On a genuine failure the ERROR must name the resource, not paste its + entire spec/status into the log line.""" + ref = resource_ref(_v1_pod(None)) + assert "kube-system/balloon-pod-pcrkw" in ref + assert "managed_fields" not in ref + + +def test_resource_ref_survives_a_resource_without_metadata(): + assert resource_ref(object()) == "object" From 54d76a8a017724f29b1ffec66bb6b67df0de039f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 07:56:32 +0000 Subject: [PATCH 2/2] Cut to the minimal fix and cover status=None MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drop the resource_ref helper and its six call sites — out of scope for the TypeError this PR is about, which the conditions fix makes moot anyway since the ERROR stops firing. Use getattr so a pod with status=None is covered too, per review: that path raised AttributeError rather than TypeError but reached the same log. Signed-off-by: Claude --- src/robusta/core/discovery/discovery.py | 50 ++++-------------- tests/discovery/test_discovery.py | 70 ++++++------------------- 2 files changed, 25 insertions(+), 95 deletions(-) diff --git a/src/robusta/core/discovery/discovery.py b/src/robusta/core/discovery/discovery.py index ea044dddf..ec11a25d2 100644 --- a/src/robusta/core/discovery/discovery.py +++ b/src/robusta/core/discovery/discovery.py @@ -854,28 +854,6 @@ def discover_stats() -> ClusterStats: ) -def resource_ref(resource) -> str: - """Short `Kind namespace/name` reference for logging. - - The extract_* helpers below log on failure. Interpolating the resource - itself dumps its entire pretty-printed spec/status — a few hundred lines per - pod, which drowns the log and the traceback that matters. Identify the - resource instead; the full object is available at debug level. - """ - try: - kind = getattr(resource, "kind", None) or type(resource).__name__ - metadata = getattr(resource, "metadata", None) - name = getattr(metadata, "name", None) - if not name: - # Nothing to identify it by — the kind alone still beats a full dump, - # and the traceback carries the rest. - return str(kind) - namespace = getattr(metadata, "namespace", None) - return f"{kind} {namespace}/{name}" if namespace else f"{kind} {name}" - except Exception: - return type(resource).__name__ - - # This section below contains utility related to k8s python api objects (rather than hikaru) def extract_containers(resource) -> List[V1Container]: """Extract containers from k8s python api object (not hikaru)""" @@ -893,8 +871,7 @@ def extract_containers(resource) -> List[V1Container]: return containers except Exception: # may fail if one of the attributes is None - logging.error(f"Failed to extract containers from {resource_ref(resource)}", exc_info=True) - logging.debug(f"Resource that failed containers extraction: {resource}") + logging.error(f"Failed to extract containers from {resource}", exc_info=True) return [] @@ -915,23 +892,18 @@ def extract_containers_k8(resource) -> List[Container]: return containers except Exception: # may fail if one of the attributes is None - logging.error(f"Failed to extract containers from {resource_ref(resource)}", exc_info=True) - logging.debug(f"Resource that failed containers extraction: {resource}") + logging.error(f"Failed to extract containers from {resource}", exc_info=True) return [] def is_pod_ready(pod) -> bool: - # `status.conditions` is None on a pod the API server has accepted but - # kubelet has not reported on yet (routinely seen on autoscaler-created - # pods discovered in the same second they were created). Without the - # `or []` the iteration below raised TypeError, which extract_ready_pods - # caught and logged as an ERROR with the whole pod object dumped into it. + # Unset until kubelet reports on the pod. conditions = [] if isinstance(pod, V1Pod): - conditions = pod.status.conditions or [] + conditions = getattr(pod.status, "conditions", None) or [] if isinstance(pod, Pod): - conditions = pod.status.conditions or [] + conditions = getattr(pod.status, "conditions", None) or [] for condition in conditions: if condition.type == "Ready": @@ -986,8 +958,7 @@ def extract_ready_pods(resource) -> int: return 0 except Exception: # fields may not exist if all the pods are not ready - example: deployment crashpod - logging.error(f"Failed to extract ready pods from {resource_ref(resource)}", exc_info=True) - logging.debug(f"Resource that failed ready pods extraction: {resource}") + logging.error(f"Failed to extract ready pods from {resource}", exc_info=True) return 0 @@ -1010,8 +981,7 @@ def extract_total_pods(resource) -> int: return 1 return 0 except Exception: - logging.error(f"Failed to extract total pods from {resource_ref(resource)}", exc_info=True) - logging.debug(f"Resource that failed total pods extraction: {resource}") + logging.error(f"Failed to extract total pods from {resource}", exc_info=True) return 1 @@ -1054,8 +1024,7 @@ def extract_volumes(resource) -> List[V1Volume]: volumes = resource.spec.volumes return volumes except Exception: # may fail if one of the attributes is None - logging.error(f"Failed to extract volumes from {resource_ref(resource)}", exc_info=True) - logging.debug(f"Resource that failed volumes extraction: {resource}") + logging.error(f"Failed to extract volumes from {resource}", exc_info=True) return [] @@ -1074,6 +1043,5 @@ def extract_volumes_k8(resource) -> List[Volume]: volumes = resource.spec.volumes return volumes except Exception: # may fail if one of the attributes is None - logging.error(f"Failed to extract volumes from {resource_ref(resource)}", exc_info=True) - logging.debug(f"Resource that failed volumes extraction: {resource}") + logging.error(f"Failed to extract volumes from {resource}", exc_info=True) return [] diff --git a/tests/discovery/test_discovery.py b/tests/discovery/test_discovery.py index d98b705e8..88796eb48 100644 --- a/tests/discovery/test_discovery.py +++ b/tests/discovery/test_discovery.py @@ -11,12 +11,7 @@ from kubernetes.client import V1ObjectMeta, V1Pod, V1PodCondition, V1PodStatus from kubernetes.client.exceptions import ApiException -from robusta.core.discovery.discovery import ( - Discovery, - extract_ready_pods, - is_pod_ready, - resource_ref, -) +from robusta.core.discovery.discovery import Discovery, extract_ready_pods, is_pod_ready # pytest-timeout requires pytest>=7, https://github.com/pytest-dev/pytest-timeout/blob/main/setup.cfg @@ -51,65 +46,32 @@ def test_discovery_recovery_on_failure(): assert not Discovery.executor._shutdown_thread -# ---- is_pod_ready / extract_ready_pods with an unpopulated status ---- +# ---- is_pod_ready with an unpopulated status ---- -def _v1_pod(conditions: Any, name: str = "balloon-pod-pcrkw") -> V1Pod: - return V1Pod( - metadata=V1ObjectMeta(name=name, namespace="kube-system"), - status=V1PodStatus(conditions=conditions), - ) +def _pod(status: Any) -> V1Pod: + return V1Pod(metadata=V1ObjectMeta(name="p", namespace="kube-system"), status=status) -def test_is_pod_ready_handles_unset_conditions(): - """A pod the API server accepted but kubelet has not reported on yet has - `status.conditions is None`. This used to raise TypeError, which - extract_ready_pods swallowed into an ERROR log per discovery cycle.""" - assert is_pod_ready(_v1_pod(None)) is False +@pytest.mark.parametrize("status", [None, V1PodStatus(conditions=None), V1PodStatus(conditions=[])]) +def test_is_pod_ready_handles_unpopulated_status(status: Any): + """Regression: a pod kubelet has not reported on yet raised TypeError, which + extract_ready_pods logged as an ERROR with the whole pod dumped into it.""" + assert is_pod_ready(_pod(status)) is False -def test_is_pod_ready_handles_empty_conditions(): - assert is_pod_ready(_v1_pod([])) is False - - -@pytest.mark.parametrize( - "status,expected", [("True", True), ("False", False), ("Unknown", False)] -) -def test_is_pod_ready_reads_ready_condition(status: str, expected: bool): - pod = _v1_pod([V1PodCondition(type="Ready", status=status)]) +@pytest.mark.parametrize("condition_status,expected", [("True", True), ("False", False)]) +def test_is_pod_ready_reads_ready_condition(condition_status: str, expected: bool): + pod = _pod(V1PodStatus(conditions=[V1PodCondition(type="Ready", status=condition_status)])) assert is_pod_ready(pod) is expected def test_is_pod_ready_ignores_other_conditions(): - pod = _v1_pod( - [ - V1PodCondition(type="PodScheduled", status="True"), - V1PodCondition(type="Initialized", status="True"), - ] - ) + pod = _pod(V1PodStatus(conditions=[V1PodCondition(type="PodScheduled", status="True")])) assert is_pod_ready(pod) is False -def test_extract_ready_pods_does_not_log_for_unset_conditions(caplog): - """The regression this guards: 12 ERROR blocks in 48h of runner logs, each - dumping the whole pod object.""" +def test_extract_ready_pods_does_not_log_for_unpopulated_status(caplog): with caplog.at_level(logging.ERROR): - assert extract_ready_pods(_v1_pod(None)) == 0 - assert not [r for r in caplog.records if "Failed to extract ready pods" in r.getMessage()] - - -def test_extract_ready_pods_counts_ready_pod(): - pod = _v1_pod([V1PodCondition(type="Ready", status="True")]) - assert extract_ready_pods(pod) == 1 - - -def test_resource_ref_identifies_resource_without_dumping_it(): - """On a genuine failure the ERROR must name the resource, not paste its - entire spec/status into the log line.""" - ref = resource_ref(_v1_pod(None)) - assert "kube-system/balloon-pod-pcrkw" in ref - assert "managed_fields" not in ref - - -def test_resource_ref_survives_a_resource_without_metadata(): - assert resource_ref(object()) == "object" + assert extract_ready_pods(_pod(None)) == 0 + assert not caplog.records