robusta_krr/core/integrations/kubernetes/__init__.py (2)

27-31: Consider adding type hints to LightweightJobInfo.

The LightweightJobInfo class would benefit from type hints on the __init__ method parameters for better IDE support and type checking.

Apply this diff to add type hints:

 class LightweightJobInfo:
     """Lightweight job object containing only the fields needed for GroupedJob processing."""
-    def __init__(self, name: str, namespace: str):
+    def __init__(self, name: str, namespace: str) -> None:
         self.name = name
         self.namespace = namespace

---

552-599: Refactor exception handling for clarity.

Line 594 captures the exception in variable e but never uses it. Either include it in the log message or remove it. Also consider moving the return statement to an else block for clarity.

Apply this diff to improve exception handling:

     try:
         # ... batching logic ...
         
         logger.debug("Found %d regular jobs", len(all_jobs))
-        return all_jobs
         
-    except Exception as e:
+    except Exception:
         logger.error(
             "Failed to run jobs discovery",
             exc_info=True,
         )
-        return all_jobs
+    
+    return all_jobs

tests/test_grouped_jobs_metrics_logic.py (3)

1-4: Clean up unused imports.

Lines 1-4 import pytest, patch, and timedelta but never use them. Remove these to keep the imports clean.

Based on static analysis.

Apply this diff:

-import pytest
-from unittest.mock import MagicMock, patch
-from datetime import timedelta
-
+from unittest.mock import MagicMock

 from robusta_krr.core.integrations.kubernetes import LightweightJobInfo

---

44-83: Remove unused variable assignments in test.

Line 74 assigns pod_owner_kind but never uses it in the namespace-specific assertions. Consider removing this assignment or adding assertions for it.

Based on static analysis.

---

133-169: Remove unused variable assignment in test.

Line 151 assigns pod_owner_kind but never uses it afterward. Consider removing this assignment.

Based on static analysis.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

robusta_krr/core/integrations/kubernetes/__init__.py (1)

282-353: Critical: Continue token logic breaks pagination for multi-namespace scenarios.

When self.namespaces != "*", the method creates multiple parallel requests (one per namespace), but all requests receive the same continue_ref (Line 315), and only the first result's continue token is extracted (Line 331). This is incorrect because:

1. Continue tokens in the Kubernetes API are request-specific and cannot be shared across different namespace queries
2. Each namespace has its own independent result set requiring separate continue tokens
3. To properly paginate across multiple namespaces, you would need to track one continue token per namespace

The current implementation only works correctly when self.namespaces == "*" (single aggregated request). If pagination with continue tokens is attempted in multi-namespace mode, results will be incomplete or incorrect.

Recommended fix:

Either:

1. Option A (simpler): Only support pagination when namespaces == "*" and raise an error or warning if continue_ref is passed with multiple namespaces
2. Option B (complete): Track continue tokens per namespace using a dict structure

Apply this diff for Option A:

     async def _list_namespaced_or_global_objects_batched(
         self,
         kind: KindLiteral,
         all_namespaces_request: Callable,
         namespaced_request: Callable,
         limit: Optional[int] = None,
         continue_ref: Optional[str] = None,
     ) -> tuple[list[Any], Optional[str]]:
         logger.debug("Listing %s in %s with batching (limit=%d)", kind, self.cluster, limit)
         loop = asyncio.get_running_loop()
 
         try:
+            # Pagination with continue tokens only works for cluster-wide requests
+            if self.namespaces != "*" and continue_ref is not None:
+                raise ValueError(
+                    "Continue token pagination is only supported for cluster-wide requests (namespaces='*'). "
+                    "Per-namespace pagination would require tracking separate continue tokens."
+                )
+            
             if self.namespaces == "*":

robusta_krr/core/integrations/kubernetes/__init__.py (1)

27-32: Consider using dataclass for better type safety and immutability.

The LightweightJobInfo class could benefit from using Python's dataclass decorator for cleaner syntax, automatic type hints, and optional immutability.

Apply this diff:

+from dataclasses import dataclass
+
-class LightweightJobInfo:
-    """Lightweight job object containing only the fields needed for GroupedJob processing."""
-    def __init__(self, name: str, namespace: str):
-        self.name = name
-        self.namespace = namespace
+@dataclass(frozen=True)
+class LightweightJobInfo:
+    """Lightweight job object containing only the fields needed for GroupedJob processing."""
+    name: str
+    namespace: str

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

robusta_krr/core/integrations/kubernetes/__init__.py (2)

547-594: Clean up exception handling per previous review.

The exception handling has minor issues that were flagged in the previous review:

1. Line 589: Variable e is assigned but never used (the exc_info=True parameter already logs the exception)
2. Line 587: The success-path return should be in a try...else block for cleaner structure

Apply this diff:

             logger.debug("Found %d regular jobs", len(all_jobs))
-            return all_jobs
             
-        except Exception as e:
+        except Exception:
             logger.error(
                 "Failed to run jobs discovery",
                 exc_info=True,
             )
-            return all_jobs
+        
+        return all_jobs

This aligns with Python best practices for try/except/else structure.

---

662-667: Simplify exception re-raise per previous review.

Line 667 uses raise e which resets the traceback. Use bare raise to preserve the original traceback for better debugging.

Apply this diff:

         except Exception as e:
             logger.error(
                 "Failed to run grouped jobs discovery",
                 exc_info=True,
             )
-            raise e
+            raise

This preserves the full exception context when propagating the error.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

robusta_krr/core/integrations/kubernetes/__init__.py (3)

282-347: Critical: Multi-namespace pagination is broken.

This issue was previously identified but remains unresolved. The continue token handling doesn't work correctly when listing specific namespaces (when self.namespaces != "*"):

1. Lines 307-318 create one request per namespace, but all requests receive the same continue_ref parameter (lines 315-316)
2. Line 330 extracts the continue token only from the first namespace's result
3. On the next batch iteration, this single continue token is passed to all namespace requests

This is incorrect because each namespace has its own independent pagination state. A continue token from namespace A cannot be used to paginate namespace B's results. This will cause:

• Only the first namespace to paginate correctly
• Other namespaces to error, ignore the token, or return incorrect results
• Job discovery to be incomplete in multi-namespace deployments

Solution: Either restrict batched listing to cluster-wide mode (namespaces == "*") only, or track separate continue tokens per namespace (requires restructuring the batching logic).

---

546-594: Clean up exception handling.

Line 589 assigns the exception to variable e but never uses it (just logs with exc_info=True). Additionally, Line 587 should be moved to an else block for cleaner structure.

Apply this diff:

         
-            logger.debug("Found %d regular jobs", len(all_jobs))
-            return all_jobs
             
-        except Exception as e:
+        except Exception:
             logger.error(
                 "Failed to run jobs discovery",
                 exc_info=True,
             )
-            return all_jobs
+        else:
+            logger.debug("Found %d regular jobs", len(all_jobs))
+        
+        return all_jobs

---

666-671: Simplify exception re-raise.

Line 671 unnecessarily specifies the exception name when re-raising. Use bare raise to preserve the original traceback.

Apply this diff:

         except Exception as e:
             logger.error(
                 "Failed to run grouped jobs discovery",
                 exc_info=True,
             )
-            raise e
+            raise

robusta_krr/core/integrations/kubernetes/__init__.py (1)

27-32: Consider using dataclass for cleaner implementation.

The LightweightJobInfo class could benefit from the @dataclass decorator, which would automatically generate __init__, __repr__, __eq__, and other methods.

Apply this diff:

+from dataclasses import dataclass
+
+@dataclass
-class LightweightJobInfo:
-    """Lightweight job object containing only the fields needed for GroupedJob processing."""
-    def __init__(self, name: str, namespace: str):
-        self.name = name
-        self.namespace = namespace
+class LightweightJobInfo:
+    """Lightweight job object containing only the fields needed for GroupedJob processing."""
+    name: str
+    namespace: str

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

robusta_krr/core/integrations/kubernetes/__init__.py (3)

588-595: Clean up exception handling.

Line 590 assigns the exception to variable e but never uses it (just logs with exc_info=True). Additionally, line 588 could be moved to an else block for cleaner control flow structure.

Apply this diff:

             logger.debug("Found %d regular jobs", len(all_jobs))
-            return all_jobs
             
-        except Exception as e:
+        except Exception:
             logger.error(
                 "Failed to run jobs discovery",
                 exc_info=True,
             )
-            return all_jobs
+        
+        return all_jobs

---

620-626: Batch limit is cumulative across all namespaces, and duplicate initialization.

Two issues here:

1. Duplicate initialization: batch_count is initialized twice (lines 620 and 623), with the second one being the effective initialization.

2. Global batch limit: Similar to _list_all_jobs, the batch_count limit is cumulative across all namespaces. If the first namespace consumes all batches, subsequent namespaces won't be processed, resulting in incomplete grouped job discovery.

Solution: Remove the duplicate initialization and move batch_count = 0 inside the namespace loop:

         grouped_jobs = defaultdict(list)
         grouped_jobs_template = {}  # Store only ONE full job as template per group - needed for class K8sObjectData
         continue_ref: Optional[str] = None
-        batch_count = 0
         namespaces = self.namespaces if self.namespaces != "*" else ["*"]
         try:
-            batch_count = 0
             for namespace in namespaces:
+                batch_count = 0
                 continue_ref = None
                 while batch_count < settings.discovery_job_max_batches:

---

554-557: Batch limit is cumulative across all namespaces.

The batch_count variable is initialized once before the namespace loop (line 554) and checked against settings.discovery_job_max_batches (line 557). This means if namespace A consumes all 10 batches, subsequent namespaces B, C, etc. will not be processed at all, leading to incomplete job discovery in multi-namespace deployments.

Solution: Move batch_count = 0 inside the namespace loop to apply the limit per-namespace:

         namespaces = self.namespaces if self.namespaces != "*" else ["*"]
         all_jobs = []
         try:
-            batch_count = 0
             for namespace in namespaces:
+                batch_count = 0
                 continue_ref: Optional[str] = None
                 while batch_count < settings.discovery_job_max_batches:

robusta_krr/core/integrations/kubernetes/__init__.py (1)

337-337: Consider moving the JSON import to module level.

Importing json inside the exception handler works but is unconventional. For consistency with other imports and minor performance benefit, consider moving it to the top of the file.

Apply this diff:

 import asyncio
 import logging
+import json
 import re
 from collections import defaultdict

Then remove the import from line 337:

         except ApiException as e:
             if e.status == 410 and e.body:
                 # Continue token expired
-                import json
                 try:
                     error_body = json.loads(e.body)

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro