chore(helm): increase default clickhouse resources #2635
Conversation
|
WalkthroughThis pull request updates a Kubernetes Helm chart configuration across multiple files. The Chart.yaml version is bumped to 4.0.5 while the appVersion remains unchanged. Documentation and configuration examples for ClickHouse resource allocations are increased with explanatory comments about resource consumption and OOM risk. The values.yaml file undergoes structural refactoring, converting multi-line empty specifications to inline scalar forms and introducing a new Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Areas requiring attention:
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
hosting/k8s/helm/Chart.yaml(1 hunks)hosting/k8s/helm/README.md(1 hunks)hosting/k8s/helm/values-production-example.yaml(1 hunks)hosting/k8s/helm/values.yaml(11 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-25T13:18:44.103Z
Learnt from: nicktrn
PR: triggerdotdev/trigger.dev#2195
File: hosting/k8s/helm/Chart.yaml:1-18
Timestamp: 2025-06-25T13:18:44.103Z
Learning: For the Trigger.dev Helm chart, the chart name should be "trigger" (not "trigger-v4") since this is the first official chart release. Helper templates should use the actual chart name from .Chart.Name rather than hardcoded prefixes.
Applied to files:
hosting/k8s/helm/Chart.yaml
📚 Learning: 2025-06-25T13:20:17.174Z
Learnt from: nicktrn
PR: triggerdotdev/trigger.dev#2195
File: hosting/k8s/helm/values.yaml:22-51
Timestamp: 2025-06-25T13:20:17.174Z
Learning: In the Trigger.dev Helm chart values.yaml, the maintainer prefers to use explicit comprehensive warnings for security-sensitive default values rather than implementing secure-by-default behavior that would fail installation. The project uses deterministic default secrets with clear "TESTING ONLY" warnings and instructions for production deployment.
Applied to files:
hosting/k8s/helm/Chart.yaml
📚 Learning: 2025-06-25T13:18:04.827Z
Learnt from: nicktrn
PR: triggerdotdev/trigger.dev#2195
File: hosting/k8s/helm/templates/extra-manifests.yaml:1-4
Timestamp: 2025-06-25T13:18:04.827Z
Learning: In the Trigger.dev v4 Helm chart, the user prefers to rely on documentation and examples in values files rather than implementing defensive coding in templates, particularly for features like extraManifests where proper usage is documented.
Applied to files:
hosting/k8s/helm/Chart.yaml
⏰ 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). (23)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
hosting/k8s/helm/Chart.yaml (1)
5-5: Appropriate version bump.Patch version increment from 4.0.4 to 4.0.5 is correct for configuration and documentation updates.
hosting/k8s/helm/values.yaml (2)
113-113: Consistent inline formatting for empty structures.The shift to inline representations of empty arrays (
[]) and dicts ({}) is a stylistic improvement that doesn't alter YAML semantics. Applied consistently across webapp, supervisor, electric, registry, and root-level configuration.Also applies to: 127-127, 141-141, 158-158, 247-247, 320-320, 517-517, 738-738, 764-764, 786-786
550-555: Based on my verification of the Bitnami ClickHouse chart documentation, here is the rewritten review comment:
Confirm
"xlarge"resourcesPreset is appropriate and understand production implications.
"xlarge"is a valid preset value for the Bitnami ClickHouse chart, and real-world deployments use this preset with ClickHouse 9.3.x versions. However, be aware that in production workloads using resourcesPreset is discouraged as it may not fully adapt to your specific needs. Withresources: {}left empty, the preset will be applied. For production use, consider defining explicit resource requests and limits under theresourcesfield instead of relying on the preset.hosting/k8s/helm/README.md (1)
659-668: Clear and well-placed resource guidance.Documentation in the Production Readiness Checklist effectively communicates ClickHouse resource requirements with clear explanations of OOM risks. The example values (4000m/16Gi limits, 2000m/8Gi requests) provide concrete guidance and align with the production example configuration.
hosting/k8s/helm/values-production-example.yaml (1)
85-89: Memory allocation discrepancy found—verify against Bitnami chart repository.The web search indicates the "xlarge" preset allocates 4Gi memory, but the production example specifies 16Gi limits—a 4× difference. The CPU value (4000m) aligns correctly.
Before proceeding, verify the actual
xlargepreset values in the Bitnami ClickHouse chart v9.3.7 repository or consult your localvalues.yamlto confirm whether:
- The production example intentionally exceeds the preset
- The preset definition has changed
- The values require alignment
| # ClickHouse can be very resource intensive, so we recommend setting limits and requests accordingly | ||
| # Note: not doing this can cause OOM crashes which will cause issues across many different |
There was a problem hiding this comment.
Complete the truncated comment.
Line 82's comment appears to end mid-sentence: "...which will cause issues across many different" lacks a conclusion. Finish the thought to clarify the scope of OOM impact (e.g., "...across many different components" or "...across many different services").
🤖 Prompt for AI Agents
In hosting/k8s/helm/values-production-example.yaml around lines 81 to 82, the
comment is truncated and ends with "which will cause issues across many
different"; complete the sentence to clarify the impact of OOM crashes — for
example, change it to "which will cause OOM crashes which will cause issues
across many different components and services in the cluster." Update the
comment text in place to finish the thought and make the scope explicit.
Many self-hosters ran into issues with the defaults and prod resource examples. This should help, but at the same time it's important to keep an eye on resource usage of self-hosted deployments for reliable operation.