From 6e374b7446638426bf047baa0d7135dcf1c3a51d Mon Sep 17 00:00:00 2001 From: Francis Caisse Date: Fri, 3 Jul 2026 12:00:14 -0400 Subject: [PATCH] Right-size native sidecar containers (+ bump kubernetes client) Native sidecars (init containers with restartPolicy: Always, GA in Kubernetes 1.29) run for the whole pod lifetime and reserve real resource requests, but KRR only discovered spec.template.spec.containers, so they were never right-sized. Add a _native_sidecars() helper and include them in every workload discovery path: Deployment, StatefulSet, DaemonSet, Job, CronJob, GroupedJob, Rollout, DeploymentConfig and StrimziPodSet. One-shot init containers are intentionally excluded: they run only briefly at startup, so their usage is not representative of steady state. Telling the two apart requires the container-level restartPolicy field, which the kubernetes client exposes only from 28.1.0, so bump the pin 26.1.0 -> 29.0.0 (pyproject.toml, requirements.txt, poetry.lock). The helper reads the field via getattr, so on an older client it simply finds no sidecars and the feature degrades to a no-op. Handles both typed clients (snake_case init_containers/restart_policy) and custom-resource workloads returned as camelCase dicts. Adds tests/test_native_sidecars.py. --- README.md | 4 + poetry.lock | 10 +- pyproject.toml | 2 +- requirements.txt | 2 +- .../core/integrations/kubernetes/__init__.py | 56 ++++- tests/test_native_sidecars.py | 193 ++++++++++++++++++ 6 files changed, 249 insertions(+), 18 deletions(-) create mode 100644 tests/test_native_sidecars.py diff --git a/README.md b/README.md index c5f8d7c1..f7b64cb7 100644 --- a/README.md +++ b/README.md @@ -483,6 +483,10 @@ By default, we use a _simple_ strategy to calculate resource recommendations. It - For memory, we take the maximum value over the past week and add a 15% buffer. +### Native sidecars + +KRR right-sizes [native sidecar containers](https://kubernetes.io/docs/concepts/workloads/pods/sidecar-containers/) — init containers with `restartPolicy: Always` (GA in Kubernetes 1.29) — alongside regular containers, since they run for the whole pod lifetime and reserve real requests. They appear as ordinary container rows in the report. One-shot init containers are intentionally **not** right-sized: they run only briefly at startup, so their usage is not representative of steady state. (Reading the container-level `restartPolicy` requires the `kubernetes` client `>= 28.1.0`.) + ### Prometheus connection Find about how KRR tries to find the default Prometheus to connect here. diff --git a/poetry.lock b/poetry.lock index 90b1d4e6..b3405c46 100644 --- a/poetry.lock +++ b/poetry.lock @@ -701,23 +701,23 @@ files = [ [[package]] name = "kubernetes" -version = "26.1.0" +version = "29.0.0" description = "Kubernetes python client" optional = false python-versions = ">=3.6" files = [ - {file = "kubernetes-26.1.0-py2.py3-none-any.whl", hash = "sha256:e3db6800abf7e36c38d2629b5cb6b74d10988ee0cba6fba45595a7cbe60c0042"}, - {file = "kubernetes-26.1.0.tar.gz", hash = "sha256:5854b0c508e8d217ca205591384ab58389abdae608576f9c9afc35a3c76a366c"}, + {file = "kubernetes-29.0.0-py2.py3-none-any.whl", hash = "sha256:ab8cb0e0576ccdfb71886366efb102c6a20f268d817be065ce7f9909c631e43e"}, + {file = "kubernetes-29.0.0.tar.gz", hash = "sha256:c4812e227ae74d07d53c88293e564e54b850452715a59a927e7e1bc6b9a60459"}, ] [package.dependencies] certifi = ">=14.05.14" google-auth = ">=1.0.1" +oauthlib = ">=3.2.2" python-dateutil = ">=2.5.3" pyyaml = ">=5.4.1" requests = "*" requests-oauthlib = "*" -setuptools = ">=21.0.0" six = ">=1.9.0" urllib3 = ">=1.24.2" websocket-client = ">=0.32.0,<0.40.0 || >0.40.0,<0.41.dev0 || >=0.43.dev0" @@ -1941,4 +1941,4 @@ type = ["pytest-mypy"] [metadata] lock-version = "2.0" python-versions = ">=3.10,<=3.12.9" -content-hash = "718445c7b4b883ce590028f1f05596a1245136cf03ebb9d0df4ca29ce42fe293" +content-hash = "f1b80e7c74f857b08c0d770c21f96c02d8c64ba6191f5498486cf3b7ce101f86" diff --git a/pyproject.toml b/pyproject.toml index 2b34dac1..66cb08c5 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -26,7 +26,7 @@ krr = "robusta_krr.main:run" python = ">=3.10,<=3.12.9" typer = { extras = ["all"], version = "^0.7.0" } pydantic = "^1.10.7" -kubernetes = "^26.1.0" +kubernetes = "^29.0.0" prometheus-api-client = "0.5.3" numpy = ">=1.26.4,<1.27.0" alive-progress = "^3.1.2" diff --git a/requirements.txt b/requirements.txt index 14862bf4..132286a1 100644 --- a/requirements.txt +++ b/requirements.txt @@ -18,7 +18,7 @@ httmock==1.4.0 ; python_version >= "3.10" and python_full_version < "3.13" idna==3.7 ; python_version >= "3.10" and python_full_version < "3.13" jmespath==1.0.1 ; python_version >= "3.10" and python_full_version < "3.13" kiwisolver==1.4.5 ; python_version >= "3.10" and python_full_version < "3.13" -kubernetes==26.1.0 ; python_version >= "3.10" and python_full_version < "3.13" +kubernetes==29.0.0 ; python_version >= "3.10" and python_full_version < "3.13" matplotlib==3.8.3 ; python_version >= "3.10" and python_full_version < "3.13" numpy==1.26.4 ; python_version >= "3.10" and python_full_version < "3.13" oauthlib==3.2.2 ; python_version >= "3.10" and python_full_version < "3.13" diff --git a/robusta_krr/core/integrations/kubernetes/__init__.py b/robusta_krr/core/integrations/kubernetes/__init__.py index 0ede135c..f4261721 100644 --- a/robusta_krr/core/integrations/kubernetes/__init__.py +++ b/robusta_krr/core/integrations/kubernetes/__init__.py @@ -40,6 +40,39 @@ def __init__(self, name: str, namespace: str): HPAKey = tuple[str, str, str] +def _native_sidecars(pod_spec: Any) -> list[Any]: + """Return init containers that are native sidecars (``restartPolicy: Always``). + + Native sidecars (GA in Kubernetes 1.29) run for the whole pod lifetime and + reserve real resource requests, so they are right-sized like normal containers. + One-shot init containers are intentionally excluded: their startup-only usage is + not representative of steady-state consumption. + + Handles both typed clients (snake_case ``init_containers`` / ``restart_policy``) + and custom-resource workloads wrapped in :class:`ObjectLikeDict` (camelCase + ``initContainers`` / ``restartPolicy``). ``getattr(..., None)`` means an older + kubernetes client that lacks the container-level ``restart_policy`` field simply + yields nothing, so the feature degrades to a no-op instead of raising. + """ + if pod_spec is None: + return [] + init_containers = getattr(pod_spec, "init_containers", None) or getattr(pod_spec, "initContainers", None) or [] + return [ + container + for container in init_containers + if (getattr(container, "restart_policy", None) or getattr(container, "restartPolicy", None)) == "Always" + ] + + +def _scannable_containers(pod_spec: Any) -> list[Any]: + """Regular containers plus native sidecars for a pod spec. + + Single point where the two container classes KRR right-sizes are combined, + so every workload discovery path stays consistent. + """ + return list(pod_spec.containers) + _native_sidecars(pod_spec) + + class ClusterLoader: def __init__(self, cluster: Optional[str] = None): self.cluster = cluster @@ -421,13 +454,13 @@ def _list_deployments(self) -> list[K8sObjectData]: kind="Deployment", all_namespaces_request=self.apps.list_deployment_for_all_namespaces, namespaced_request=self.apps.list_namespaced_deployment, - extract_containers=lambda item: item.spec.template.spec.containers, + extract_containers=lambda item: _scannable_containers(item.spec.template.spec), ) def _list_rollouts(self) -> list[K8sObjectData]: async def _extract_containers(item: Any) -> list[V1Container]: if item.spec.template is not None: - return item.spec.template.spec.containers + return _scannable_containers(item.spec.template.spec) loop = asyncio.get_running_loop() @@ -444,7 +477,7 @@ async def _extract_containers(item: Any) -> list[V1Container]: namespace=item.metadata.namespace, name=workloadRef.name ), ) - return ret.spec.template.spec.containers + return _scannable_containers(ret.spec.template.spec) return [] @@ -492,7 +525,7 @@ def _list_strimzipodsets(self) -> list[K8sObjectData]: **kwargs, ) ), - extract_containers=lambda item: item.spec.pods[0].spec.containers, + extract_containers=lambda item: _scannable_containers(item.spec.pods[0].spec), ) def _list_deploymentconfig(self) -> list[K8sObjectData]: @@ -516,7 +549,7 @@ def _list_deploymentconfig(self) -> list[K8sObjectData]: **kwargs, ) ), - extract_containers=lambda item: item.spec.template.spec.containers, + extract_containers=lambda item: _scannable_containers(item.spec.template.spec), ) def _list_all_statefulsets(self) -> list[K8sObjectData]: @@ -524,7 +557,7 @@ def _list_all_statefulsets(self) -> list[K8sObjectData]: kind="StatefulSet", all_namespaces_request=self.apps.list_stateful_set_for_all_namespaces, namespaced_request=self.apps.list_namespaced_stateful_set, - extract_containers=lambda item: item.spec.template.spec.containers, + extract_containers=lambda item: _scannable_containers(item.spec.template.spec), ) def _list_all_daemon_set(self) -> list[K8sObjectData]: @@ -532,7 +565,7 @@ def _list_all_daemon_set(self) -> list[K8sObjectData]: kind="DaemonSet", all_namespaces_request=self.apps.list_daemon_set_for_all_namespaces, namespaced_request=self.apps.list_namespaced_daemon_set, - extract_containers=lambda item: item.spec.template.spec.containers, + extract_containers=lambda item: _scannable_containers(item.spec.template.spec), ) async def _list_all_jobs(self) -> list[K8sObjectData]: @@ -571,7 +604,7 @@ async def _list_all_jobs(self) -> list[K8sObjectData]: continue if self._is_job_grouped(job): continue - for container in job.spec.template.spec.containers: + for container in _scannable_containers(job.spec.template.spec): all_jobs.append(self.__build_scannable_object(job, container, "Job")) if not continue_ref: break @@ -591,7 +624,7 @@ def _list_all_cronjobs(self) -> list[K8sObjectData]: kind="CronJob", all_namespaces_request=self.batch.list_cron_job_for_all_namespaces, namespaced_request=self.batch.list_namespaced_cron_job, - extract_containers=lambda item: item.spec.job_template.spec.template.spec.containers, + extract_containers=lambda item: _scannable_containers(item.spec.job_template.spec.template.spec), ) async def _list_all_groupedjobs(self) -> list[K8sObjectData]: @@ -678,13 +711,14 @@ async def _list_all_groupedjobs(self) -> list[K8sObjectData]: for namespace, namespace_jobs in jobs_by_namespace.items(): limited_jobs = namespace_jobs[: settings.job_grouping_limit] + template_containers = _scannable_containers(template_job.spec.template.spec) container_names = set() - for container in template_job.spec.template.spec.containers: + for container in template_containers: container_names.add(container.name) for container_name in container_names: template_container = None - for container in template_job.spec.template.spec.containers: + for container in template_containers: if container.name == container_name: template_container = container break diff --git a/tests/test_native_sidecars.py b/tests/test_native_sidecars.py new file mode 100644 index 00000000..354f1292 --- /dev/null +++ b/tests/test_native_sidecars.py @@ -0,0 +1,193 @@ +"""Tests for native-sidecar discovery. + +KRR right-sizes native sidecars (init containers with ``restartPolicy: Always``, +GA in Kubernetes 1.29) like normal containers, while intentionally ignoring +one-shot init containers. The core logic lives in the module-level helper +``_native_sidecars``; these tests cover it directly (it is pure) plus two +discovery paths that use it (the Deployment extractor lambda and the Job loop). + +Typed kubernetes-client models are simulated with ``SimpleNamespace`` rather than +``MagicMock`` on purpose: ``MagicMock`` auto-creates any attribute, so +``getattr(mock, "initContainers", None)`` would return a truthy mock and defeat +the snake_case/camelCase fallback under test. CRD workloads are simulated with a +real ``ObjectLikeDict`` (raw camelCase keys), exactly as the custom-objects API +returns them. +""" + +from types import SimpleNamespace +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from robusta_krr.core.integrations.kubernetes import ClusterLoader, _native_sidecars +from robusta_krr.core.models.config import Config +from robusta_krr.utils.object_like_dict import ObjectLikeDict + + +def typed_container(name: str, **kwargs) -> SimpleNamespace: + """A typed V1Container-like object. Pass restart_policy=... to set it; + omit it entirely to simulate an old client where the field does not exist.""" + return SimpleNamespace(name=name, **kwargs) + + +def typed_spec(containers, init_containers=None) -> SimpleNamespace: + """A typed V1PodSpec-like object (snake_case attributes).""" + return SimpleNamespace(containers=containers, init_containers=init_containers) + + +# --- pure helper tests ------------------------------------------------------- + + +def test_native_sidecar_discovered(): + spec = typed_spec( + containers=[typed_container("main")], + init_containers=[typed_container("proxy", restart_policy="Always")], + ) + sidecars = _native_sidecars(spec) + assert [c.name for c in sidecars] == ["proxy"] + + +def test_oneshot_init_container_excluded(): + # A one-shot init container has restart_policy unset (None) -> not a sidecar. + spec = typed_spec( + containers=[typed_container("main")], + init_containers=[typed_container("db-migrate", restart_policy=None)], + ) + assert _native_sidecars(spec) == [] + + +def test_no_init_containers(): + spec = typed_spec(containers=[typed_container("main")], init_containers=None) + assert _native_sidecars(spec) == [] + + +def test_none_pod_spec(): + # Guards the Rollout workloadRef branch and any spec-less edge. + assert _native_sidecars(None) == [] + + +def test_old_client_no_restart_policy_field(): + # On kubernetes < 28.1.0 the container model has no restart_policy attribute + # at all: the helper must degrade to a no-op, not crash. + spec = typed_spec( + containers=[typed_container("main")], + init_containers=[typed_container("maybe-sidecar")], # no restart_policy attr + ) + assert _native_sidecars(spec) == [] + + +def test_crd_camelcase_sidecar(): + # Custom-objects API (Rollout/StrimziPodSet/DeploymentConfig) returns raw + # camelCase keys wrapped in ObjectLikeDict. + spec = ObjectLikeDict( + { + "containers": [{"name": "main"}], + "initContainers": [ + {"name": "proxy", "restartPolicy": "Always"}, + {"name": "warmup", "restartPolicy": None}, + ], + } + ) + sidecars = _native_sidecars(spec) + assert [c.name for c in sidecars] == ["proxy"] + + +def test_mixed_init_containers(): + spec = typed_spec( + containers=[typed_container("main")], + init_containers=[ + typed_container("db-migrate", restart_policy=None), + typed_container("proxy", restart_policy="Always"), + typed_container("logshipper", restart_policy="Always"), + ], + ) + assert [c.name for c in _native_sidecars(spec)] == ["proxy", "logshipper"] + + +# --- integration tests through real discovery paths -------------------------- + + +@pytest.fixture +def mock_config(): + config = MagicMock(spec=Config) + config.resources = "*" + config.selector = None + config.namespaces = "*" + config.max_workers = 4 + config.get_kube_client = MagicMock() + # Job discovery settings + config.discovery_job_batch_size = 1000 + config.discovery_job_max_batches = 50 + return config + + +@pytest.fixture +def loader(mock_config): + with patch("robusta_krr.core.integrations.kubernetes.settings", mock_config): + loader = ClusterLoader() + loader.apps = MagicMock() + loader.batch = MagicMock() + loader.core = MagicMock() + + # __build_scannable_object is name-mangled; stub it to expose container name. + def build(item, container, kind): + obj = MagicMock() + obj.container = container.name + obj.kind = kind + return obj + + loader._ClusterLoader__build_scannable_object = build # type: ignore + return loader + + +@pytest.mark.asyncio +async def test_deployment_extractor_includes_sidecar(loader, mock_config): + """The real Deployment extractor lambda + _list_scannable_objects yields the + main container and the native sidecar, but not the one-shot init container.""" + deployment = SimpleNamespace( + spec=SimpleNamespace( + template=SimpleNamespace( + spec=typed_spec( + containers=[typed_container("web")], + init_containers=[ + typed_container("php-fpm", restart_policy="Always"), + typed_container("db-migrate", restart_policy=None), + ], + ) + ) + ) + ) + loader._list_namespaced_or_global_objects = AsyncMock(return_value=[deployment]) + + with patch("robusta_krr.core.integrations.kubernetes.settings", mock_config): + result = await loader._list_deployments() + + assert {obj.container for obj in result} == {"web", "php-fpm"} + + +@pytest.mark.asyncio +async def test_job_includes_native_sidecar(loader, mock_config): + """The Job loop (which bypasses _list_scannable_objects) also picks up the + native sidecar and skips the one-shot init container.""" + job = MagicMock() + job.metadata.owner_references = [] + job.metadata.labels = {} + job.spec.template.spec = typed_spec( + containers=[typed_container("batch")], + init_containers=[ + typed_container("metrics", restart_policy="Always"), + typed_container("seed", restart_policy=None), + ], + ) + + async def batched(*args, **kwargs): + return ([job], None) + + loader._list_namespaced_or_global_objects_batched = batched + loader._is_job_owned_by_cronjob = MagicMock(return_value=False) + loader._is_job_grouped = MagicMock(return_value=False) + + with patch("robusta_krr.core.integrations.kubernetes.settings", mock_config): + result = await loader._list_all_jobs() + + assert {obj.container for obj in result} == {"batch", "metrics"}