fix(base-cluster/monitoring): increase limit of grafana sidecar#1878
fix(base-cluster/monitoring): increase limit of grafana sidecar#1878marvinWolff merged 1 commit intomainfrom
Conversation
Summary of ChangesHello @marvinWolff, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a potential resource constraint for the Grafana sidecar by explicitly setting a memory limit within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
WalkthroughA memory limit of 256Mi was added to the monitoring sidecar resource configuration in the base cluster values file. This is a configuration-only update that establishes resource constraints for the monitoring sidecar container without modifying any other settings. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
There was a problem hiding this comment.
Code Review
This pull request increases the memory limit for the Grafana sidecar to 256Mi, likely to address out-of-memory issues. The change is straightforward. I have added one suggestion to also explicitly define a memory request for improved clarity and consistency with the project's configuration standards.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
charts/base-cluster/values.yaml (1)
245-249: Consider adding CPU limits alongside memory for consistency.The change adds a memory limit to the grafana sidecar, which addresses the stated objective. However, reviewing similar configurations in this file (e.g.,
oauthProxyat lines 214–220,nodeExporterat lines 293–300), components with explicit resource limits typically define both memory and CPU constraints. The sidecar is now memory-constrained but lacks CPU constraints, which could be inconsistent with the resource management patterns established elsewhere.Verify whether:
- The 256Mi memory limit is adequate for the grafana sidecar workload
- CPU limits or requests should be added for resource consistency and to prevent unbounded CPU consumption
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
charts/base-cluster/values.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint helm chart (base-cluster)
- GitHub Check: check licenses
🤖 I have created a release *beep* *boop* --- ## [11.0.0](base-cluster-v10.1.2...base-cluster-v11.0.0) (2025-12-10) ### ⚠ BREAKING CHANGES * **base-cluster/monitoring:** grafana-tempo-distributed would need s3 ([#1875](#1875)) ### Features * **base-cluster/flux:** use centralised helmRepositories template ([#1845](#1845)) ([60658ed](60658ed)) ### Bug Fixes * **base-cluster/monitoring:** grafana-tempo-distributed would need s3 ([#1875](#1875)) ([df5c372](df5c372)) * **base-cluster/monitoring:** increase limit of grafana sidecar ([#1878](#1878)) ([fb1ebc6](fb1ebc6)) * **base-cluster:** correctly set image repository ([#1848](#1848)) ([0e108dc](0e108dc)) ### Miscellaneous Chores * **base-cluster/dependencies:** update common docker tag to v1.7.0 ([#1872](#1872)) ([b50ea98](b50ea98)) * **base-cluster/dependencies:** update helm release reflector to v9.1.44 ([#1861](#1861)) ([2cfe4e5](2cfe4e5)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Breaking Changes** * grafana-tempo-distributed now requires S3 configuration * **New Features** * Centralized helmRepositories template management * **Bug Fixes** * Increased Grafana sidecar limit * Corrected image repository reference * **Chores** * Updated dependencies and component versions <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.