Skip to content

Conversation

@rajpratik71
Copy link
Contributor

@rajpratik71 rajpratik71 commented Oct 22, 2024

fix #1601

Signed-off-by: Pratik Raj [email protected]

@moshemorad moshemorad added the Silence Ignored by schedule notification label Jun 22, 2025
@coderabbitai
Copy link

coderabbitai bot commented Jun 22, 2025

Walkthrough

The forwarder Helm template now unconditionally sets the kubewatch container’s cpu limit from values. Default values set cpu limits for kubewatch and grafanaRenderer to 50m instead of null. Memory limit logic remains unchanged.

Changes

Cohort / File(s) Summary
Forwarder Helm template
helm/robusta/templates/forwarder.yaml
Changed templating to always emit resources.limits.cpu for kubewatch using .Values.kubewatch.resources.limits.cpu; memory limit logic unchanged.
Default values
helm/robusta/values.yaml
Set kubewatch.resources.limits.cpu from ~ to 50m; set grafanaRenderer.resources.limits.cpu from ~ to 50m.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant U as User/CI
  participant H as Helm
  participant T as forwarder.yaml (template)
  participant K as Kubernetes API
  participant S as Scheduler/Kubelet

  U->>H: helm install/upgrade
  H->>T: Render with values.yaml
  note over T: Unconditionally set kubewatch<br/>resources.limits.cpu = values.kubewatch.resources.limits.cpu
  T-->>H: Manifest (Deployment/Pod spec)
  H->>K: Apply manifest
  K->>S: Create/Run Pod
  S-->>S: Enforce CPU limit at runtime
  note over S: kubewatch container capped per configured CPU limit
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Linked Issues Check ⚠️ Warning The changes add unconditional CPU limits for the kubewatch sidecar and update resource values for kubewatch and grafanaRenderer, but there is no evidence of setting a CPU limit for the primary robusta-forwarder container as required by issue #1601. Update the forwarder template and values.yaml to include and honor CPU limit settings for the core robusta-forwarder container, ensuring the main forwarder process is constrained as outlined in issue #1601.
Out of Scope Changes Check ⚠️ Warning The PR updates CPU limits for kubewatch and grafanaRenderer components, but only the kubewatch sidecar is part of the robusta-forwarder pods; the grafanaRenderer change is unrelated to limiting CPU for the forwarder service and falls outside the scope of issue #1601. Remove or isolate the grafanaRenderer CPU limit change into a separate update and ensure any CPU limit modifications directly target the robusta-forwarder container and its sidecars as defined by the issue scope.
✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title clearly summarizes the primary change of enabling a CPU limit for the robusta-forwarder service to address CPU hogging, directly reflecting the main objective of the pull request.
Description Check ✅ Passed The description references the relevant issue (#1601) and indicates the intent to fix CPU limit configuration, aligning with the changes made in the pull request.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

…ce as current config cause cpu hogging

Signed-off-by: Pratik Raj <[email protected]>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b066ce1 and 9576d07.

📒 Files selected for processing (2)
  • helm/robusta/templates/forwarder.yaml (1 hunks)
  • helm/robusta/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). (1)
  • GitHub Check: run_tests

Comment on lines +74 to 75
cpu: {{ .Values.kubewatch.resources.limits.cpu }}
memory: {{ if .Values.isSmallCluster }}"64Mi"{{ else if .Values.kubewatch.resources.limits.memory }}{{ .Values.kubewatch.resources.limits.memory | quote }}{{ else }}{{ .Values.kubewatch.resources.requests.memory | quote }}{{ end }}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Restore conditional rendering for cpu limit

Emitting cpu: {{ .Values.kubewatch.resources.limits.cpu }} unconditionally breaks the common use case where operators set that value to null to remove the limit—Helm renders it as <no value>, yielding invalid YAML and failing the deploy. Please wrap the stanza with an if/with, as it was before, so the key is omitted when the value is unset.

Apply this diff to bring back the guard:

-          limits:
-            cpu: {{ .Values.kubewatch.resources.limits.cpu }}
+          limits:
+          {{- with .Values.kubewatch.resources.limits.cpu }}
+            cpu: {{ . }}
+          {{- end }}
             memory: {{ if .Values.isSmallCluster }}"64Mi"{{ else if .Values.kubewatch.resources.limits.memory }}{{ .Values.kubewatch.resources.limits.memory | quote }}{{ else }}{{ .Values.kubewatch.resources.requests.memory | quote }}{{ end }}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
cpu: {{ .Values.kubewatch.resources.limits.cpu }}
memory: {{ if .Values.isSmallCluster }}"64Mi"{{ else if .Values.kubewatch.resources.limits.memory }}{{ .Values.kubewatch.resources.limits.memory | quote }}{{ else }}{{ .Values.kubewatch.resources.requests.memory | quote }}{{ end }}
limits:
{{- with .Values.kubewatch.resources.limits.cpu }}
cpu: {{ . }}
{{- end }}
memory: {{ if .Values.isSmallCluster }}"64Mi"{{ else if .Values.kubewatch.resources.limits.memory }}{{ .Values.kubewatch.resources.limits.memory | quote }}{{ else }}{{ .Values.kubewatch.resources.requests.memory | quote }}{{ end }}
🤖 Prompt for AI Agents
In helm/robusta/templates/forwarder.yaml around lines 74-75, the cpu limit key
is emitted unconditionally causing Helm to render "<no value>" when
.Values.kubewatch.resources.limits.cpu is null; wrap the cpu line in an if (or
with) guard that only renders the entire "cpu: ..." line when
.Values.kubewatch.resources.limits.cpu is set (preserve current indentation),
leaving the key out otherwise so the generated YAML remains valid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Silence Ignored by schedule notification

Projects

None yet

Development

Successfully merging this pull request may close these issues.

enable cpu limit for "robusta-forwarder" service as current config cause cpu hogging

2 participants