Right-size native sidecar containers (+ bump kubernetes client to 29)#535
Open
JReko wants to merge 1 commit into
Open
Right-size native sidecar containers (+ bump kubernetes client to 29)#535JReko wants to merge 1 commit into
JReko wants to merge 1 commit into
Conversation
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.
|
|
WalkthroughThis PR adds detection and inclusion of Kubernetes native sidecar init containers (restartPolicy: Always) across all workload extraction paths (Deployments, Rollouts, StrimziPodSet, StatefulSets, DaemonSets, DeploymentConfig, Jobs, CronJobs, GroupedJob), bumps the kubernetes client dependency to ^29.0.0, updates README documentation, and adds corresponding tests. ChangesNative sidecar detection
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ClusterLoader
participant extract_containers
participant _native_sidecars
ClusterLoader->>extract_containers: request containers for workload
extract_containers->>_native_sidecars: pod_spec
_native_sidecars-->>extract_containers: init containers with restartPolicy Always
extract_containers-->>ClusterLoader: regular containers + native sidecars
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
robusta_krr/core/integrations/kubernetes/__init__.py (1)
448-456: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated
list(x.containers) + _native_sidecars(x)pattern across 9 call sites.Consider extracting a small helper (e.g.
_scannable_containers(spec)) that returnslist(spec.containers) + _native_sidecars(spec), to reduce duplication and make future changes (e.g., dedup logic, additional container classes) a single-point edit.♻️ Example helper
def _scannable_containers(pod_spec: Any) -> list[Any]: """Regular containers plus native sidecars, for a pod spec.""" return list(pod_spec.containers) + _native_sidecars(pod_spec)Then callers become e.g.:
- extract_containers=lambda item: list(item.spec.template.spec.containers) - + _native_sidecars(item.spec.template.spec), + extract_containers=lambda item: _scannable_containers(item.spec.template.spec),Also applies to: 473-473, 521-522, 546-565, 604-607, 627-628, 715-724
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@robusta_krr/core/integrations/kubernetes/__init__.py` around lines 448 - 456, The repeated list(spec.containers) + _native_sidecars(spec) pattern in the Kubernetes integration should be centralized to avoid duplication. Add a small helper such as _scannable_containers(pod_spec) near the existing container-extraction logic in __init__.py, have it return the combined containers list, and update the call sites in _list_rollouts and the other affected extract_containers lambdas to use that helper instead of inlining the merge.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@robusta_krr/core/integrations/kubernetes/__init__.py`:
- Around line 448-456: The repeated list(spec.containers) +
_native_sidecars(spec) pattern in the Kubernetes integration should be
centralized to avoid duplication. Add a small helper such as
_scannable_containers(pod_spec) near the existing container-extraction logic in
__init__.py, have it return the combined containers list, and update the call
sites in _list_rollouts and the other affected extract_containers lambdas to use
that helper instead of inlining the merge.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 50d15904-0639-4255-a0ac-79963f68f547
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
README.mdpyproject.tomlrequirements.txtrobusta_krr/core/integrations/kubernetes/__init__.pytests/test_native_sidecars.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Right-size native sidecar containers — init containers with
restartPolicy: Always(GA in Kubernetes 1.29) — alongside regular containers.Why
KRR discovers containers only from
spec.template.spec.containers, so native sidecars are never discovered, never queried for metrics, and never appear in reports. But they run for the whole pod lifetime and reserve real CPU/memory requests, so they should be right-sized like any other container. In one namespace we manage, native sidecars accounted for ~160 cores / ~250 GiB of reserved capacity that was completely invisible to KRR — and hid real under-provisioning.One-shot init containers are intentionally excluded: they run only briefly at startup, so their usage is not representative of steady state and a "recommendation" for them would be misleading.
How
_native_sidecars(pod_spec)inrobusta_krr/core/integrations/kubernetes/__init__.pythat returns init containers withrestartPolicy: Always._list_scannable_objects, so they're handled at their own loops.)init_containers/restart_policy) and custom-resource workloads returned as camelCase dicts (initContainers/restartPolicy).getattr(..., None), so on an older client that lacks it the helper simply finds no sidecars and the feature degrades to a no-op rather than raising.Kubernetes client bump
Distinguishing a native sidecar from a one-shot init container requires the container-level
restartPolicyfield, which thekubernetesPython client only exposes on the container model from 28.1.0. KRR currently pins^26.1.0, so this PR bumps it:pyproject.toml:^26.1.0→^29.0.0requirements.txt:26.1.0→29.0.0poetry.lock: regenerated (poetry lock --no-update)KRR's kubernetes API surface (
AppsV1Api/BatchV1Api/CoreV1Api/CustomObjectsApi/Autoscaling*Apilist calls,config.list_kube_config_contexts) is unchanged across 26.x → 29.x, so there's no breaking risk.28.1.0is the acceptable strict minimum if you'd prefer a smaller bump — happy to change it.Tests
New
tests/test_native_sidecars.py(9 cases): native sidecar discovered; one-shot init container excluded; mixed; no init containers;Nonespec; old client without the field → no-op; CRD camelCase; plus integration tests through the real Deployment extractor and the Job loop. Full existing suite still passes (pytest→ 64 passed).Relation to #491
#491 also addresses init containers but right-sizes all of them across the workload types. This PR is deliberately narrower and safer: it right-sizes only native sidecars (
restartPolicy: Always) and leaves one-shot init containers untouched, which avoids emitting misleading recommendations for containers that only run at startup. That correctness distinction is what motivates the client bump. Happy to reconcile the two approaches if maintainers prefer.