[clickhouse] add clickhouse keeper #1320
Conversation
Signed-off-by: kklinch0 <kklinch0@gmail.com>
WalkthroughThis change introduces a configurable replica count for ClickHouse Keeper in the Helm chart. The replica parameter is added to documentation, schema, and values files. Corresponding Helm templates are updated to use this parameter for resource definitions, replacing hardcoded replica values with dynamic configuration. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Helm Chart
participant Kubernetes API
User->>Helm Chart: Set clickhouseKeeper.replicas in values.yaml
Helm Chart->>Kubernetes API: Deploy resources using .Values.clickhouseKeeper.replicas
Kubernetes API->>Kubernetes API: Create Keeper, WorkloadMonitor, and Zookeeper nodes with specified replica count
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Suggested labels
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Summary of Changes
Hello @klinch0, 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 introduces support for ClickHouse Keeper within the ClickHouse Helm chart, allowing ClickHouse clusters to leverage ClickHouse Keeper for coordination. It also updates the underlying altinity-clickhouse-operator to a newer version, bringing in new features and CRD schema changes necessary for this integration.
Highlights
- ClickHouse Keeper Deployment: I've added a new Helm template (chkeeper.yaml) to deploy ClickHouse Keeper instances as a standalone component, which can then be used by ClickHouse clusters for distributed coordination.
- ClickHouse Integration: The main ClickHouse Helm chart (clickhouse.yaml) has been updated to allow ClickHouse clusters to connect to and utilize the deployed ClickHouse Keeper instances as their Zookeeper ensemble, enabling robust distributed operations.
- Operator and Chart Upgrades: I've upgraded the altinity-clickhouse-operator Helm chart to version 0.25.2 and the clickhouse chart to 0.12.0. This operator upgrade includes new Custom Resource Definitions (CRDs) for ClickHouse Keeper installations and various schema enhancements for existing ClickHouse resources.
- Configuration and Monitoring: New configuration parameters have been introduced in values.yaml and values.schema.json for managing ClickHouse Keeper deployments, including enabling/disabling it, setting resource limits, and defining replica counts. Additionally, the Grafana dashboard for ClickHouse Keeper has been updated to reflect new metric names and improve monitoring capabilities.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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 or fill out our survey to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
Signed-off-by: klinch0 <68821526+klinch0@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request introduces ClickHouse Keeper support and upgrades the Altinity ClickHouse Operator. The changes are extensive, covering CRD updates, Helm chart modifications, and new Kubernetes resources for the keeper. My review focuses on the new keeper implementation. I've found a few issues related to resource configuration, port exposure, and schema definitions for the new ClickHouse Keeper component that need to be addressed.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/apps/clickhouse/README.md (1)
56-64: Regenerate README to satisfy pre-commit hookCI failed (
readme-generator-for-helm) because the manual edit deviates from the auto-generated format.
Runmake generate(or the repo-specific generator) and commit the resulting README to unblock the pipeline.
🧹 Nitpick comments (3)
packages/apps/clickhouse/values.yaml (1)
63-69: Clarify replica parameters to avoid ambiguityWith both a top-level
.Values.replicas(ClickHouse) and.Values.clickhouseKeeper.replicasnow present, it’s easy for chart consumers to mix them up.
Consider adding an inline comment that explicitly distinguishes database replicas from keeper replicas, e.g.## @param clickhouseKeeper.replicas Number of ClickHouse **Keeper** replicas (quorum nodes), _not_ the main DB replicasThis tiny clarification will save future users a round-trip to the docs.
packages/apps/clickhouse/templates/chkeeper.yaml (1)
18-18: RenderreplicasCountexplicitly as an integer to avoid YAML type drift
.Values.clickhouseKeeper.replicasmay come through the values file as a quoted
string (e.g."3"). Helm will render that verbatim, which makes the CR field
a string instead of an int and can cause Altinity-keeper admission to reject
the object.- replicasCount: {{ .Values.clickhouseKeeper.replicas }} + replicasCount: {{ int .Values.clickhouseKeeper.replicas }}packages/apps/clickhouse/templates/clickhouse.yaml (1)
97-104: Reuse the already-derived$clusterDomainto keep FQDNs consistentEarlier in the template you compute
$clusterDomain := (index $cozyConfig.data "cluster-domain") | default "cozy.local".
Here you silently switch to.Values.clusterDomain, which can diverge from the
lookup and break keeper discovery inside ClickHouse.- {{- $clusterDomain := .Values.clusterDomain }} + {{- /* Re-use the global value computed at the top of the file */ -}} + {{- $clusterDomain := $clusterDomain }}Alternatively, drop the local assignment completely and reference the global
$clusterDomaindirectly in the host string.Please verify that only one source of truth is used for the cluster-domain
throughout the chart.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/apps/README.md(0 hunks)packages/apps/clickhouse/README.md(1 hunks)packages/apps/clickhouse/templates/chkeeper.yaml(1 hunks)packages/apps/clickhouse/templates/clickhouse.yaml(1 hunks)packages/apps/clickhouse/templates/dashboard-resourcemap.yaml(1 hunks)packages/apps/clickhouse/templates/workloadmonitor.yaml(1 hunks)packages/apps/clickhouse/values.schema.json(1 hunks)packages/apps/clickhouse/values.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- packages/apps/README.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: kubernetes rbac rules with resourcenames work correctly for list/watch verbs. when resourcenames is ...
Learnt from: lllamnyp
PR: cozystack/cozystack#1161
File: packages/apps/virtual-machine/templates/dashboard-resourcemap.yaml:6-12
Timestamp: 2025-07-10T12:14:27.197Z
Learning: Kubernetes RBAC rules with resourceNames work correctly for list/watch verbs. When resourceNames is specified in an RBAC rule, it properly restricts access to only those named resources, even for list and watch operations. Examples: `kubectl get resource resourcename -w` watches for changes on a single resource, and `kubectl get resource --field-selector .metadata.name=resourcename` lists a specific resource. The Kubernetes API server correctly distinguishes such requests from their less specific counterparts.
Applied to files:
packages/apps/clickhouse/templates/dashboard-resourcemap.yaml
🪛 GitHub Actions: Pre-Commit Checks
packages/apps/clickhouse/README.md
[error] 62-62: Make generate step failed due to modifications in README.md by readme-generator-for-helm hook. Files were modified by this hook, causing the pipeline to fail with exit code 123.
⏰ 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: Build
🔇 Additional comments (1)
packages/apps/clickhouse/templates/dashboard-resourcemap.yaml (1)
25-28: YAML indentation ok – but beware empty list when keeper disabledWhen
clickhouseKeeper.enabledisfalse, the conditional leaves an empty line that is still a valid YAML element, so the list remains well-formed (- {{ .Release.Name }}only). No action required – just signalling I checked the indentation & templating.
| replicas: {{ .Values.clickhouseKeeper.replicas }} | ||
| minReplicas: 1 | ||
| kind: clickhouse |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Lowering minReplicas to 1 may jeopardise keeper quorum
Keeper forms a Raft quorum (majority of replicas).
If HPA/WorkloadMonitor scales the StatefulSet below 3, the cluster can lose consensus and stall writes.
Confirm that WorkloadMonitor will never down-scale below a safe majority or set minReplicas: 3 instead.
- minReplicas: 1
+ minReplicas: 3 # keep quorum🏁 Script executed:
#!/bin/bash
set -e
echo "1) Locate the chart's values.yaml for clickhouseKeeper defaults"
fd values.yaml packages/apps/clickhouse
echo
echo "2) Extract clickhouseKeeper block from values.yaml"
rg -n "clickhouseKeeper" -n packages/apps/clickhouse/values.yaml -A 5 -B 2
echo
echo "3) Locate the WorkloadMonitor template"
fd workloadmonitor.yaml packages/apps/clickhouse/templates
echo
echo "4) Display the first 200 lines of workloadmonitor.yaml"
sed -n '1,200p' packages/apps/clickhouse/templates/workloadmonitor.yamlLength of output: 2102
Ensure Keeper HPA doesn’t break Raft quorum
Keeper requires a majority of its replicas to stay online. With clickhouseKeeper.replicas = 3, setting minReplicas: 1 lets the HPA scale below quorum and stall writes. Update the keeper monitor in
packages/apps/clickhouse/templates/workloadmonitor.yaml:
--- a/packages/apps/clickhouse/templates/workloadmonitor.yaml
@@ spec:
- minReplicas: 1
+ # guarantee a Raft majority
+ minReplicas: {{ .Values.clickhouseKeeper.replicas }}This will keep the keeper pods ≥ the configured replica count and preserve quorum.
📝 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.
| replicas: {{ .Values.clickhouseKeeper.replicas }} | |
| minReplicas: 1 | |
| kind: clickhouse | |
| replicas: {{ .Values.clickhouseKeeper.replicas }} | |
| # guarantee a Raft majority | |
| minReplicas: {{ .Values.clickhouseKeeper.replicas }} | |
| kind: clickhouse |
🤖 Prompt for AI Agents
In packages/apps/clickhouse/templates/workloadmonitor.yaml around lines 21 to
23, the minReplicas value is set to 1, which can cause the Keeper HPA to scale
below the quorum needed for Raft consensus. Update minReplicas to be equal to
the configured clickhouseKeeper.replicas value to ensure the Keeper pods never
scale below the required quorum and maintain write availability.
| "replicas": { | ||
| "default": 3, | ||
| "description": "Number of keeper replicas", | ||
| "type": "number" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use integer type and add validation bounds
replicas should be an integer ≥ 1. Using "number" allows non-integer & negative values.
- "replicas": {
- "default": 3,
- "description": "Number of keeper replicas",
- "type": "number"
- },
+ "replicas": {
+ "default": 3,
+ "description": "Number of keeper replicas",
+ "type": "integer",
+ "minimum": 1
+ },📝 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.
| "replicas": { | |
| "default": 3, | |
| "description": "Number of keeper replicas", | |
| "type": "number" | |
| "replicas": { | |
| "default": 3, | |
| "description": "Number of keeper replicas", | |
| "type": "integer", | |
| "minimum": 1 | |
| }, |
🤖 Prompt for AI Agents
In packages/apps/clickhouse/values.schema.json around lines 55 to 58, change the
type of the "replicas" property from "number" to "integer" and add validation
constraints to ensure the value is at least 1. This will prevent non-integer and
negative values from being accepted.
Signed-off-by: kklinch0 <kklinch0@gmail.com>
What this PR does
Release note
Summary by CodeRabbit
New Features
Documentation
clickhouseKeeper.replicasparameter and its usage.