[clickhouse] add clickhouse keeper #1298
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis update introduces ClickHouse Keeper support to the ClickHouse Helm chart and ecosystem. It adds new configuration options, CRDs, templates, and documentation for deploying and monitoring ClickHouse Keeper. Related operator and dashboard resources are updated for compatibility, configurability, and improved secret handling. Numerous CRD schemas and Helm values are extended. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Helm
participant Kubernetes API
participant ClickHouse Operator
participant ClickHouse Keeper Pods
participant ClickHouse Pods
User->>Helm: Install/upgrade ClickHouse chart (with clickhouseKeeper.enabled)
Helm->>Kubernetes API: Deploy ClickHouseKeeperInstallation (if enabled)
Helm->>Kubernetes API: Deploy ClickHouseInstallation (with ZooKeeper section if Keeper enabled)
Kubernetes API->>ClickHouse Operator: Notify of new/updated resources
ClickHouse Operator->>Kubernetes API: Create/manage Keeper and ClickHouse pods
ClickHouse Keeper Pods-->>ClickHouse Pods: Provide coordination (ZooKeeper API)
ClickHouse Keeper Pods->>Prometheus/VictoriaMetrics: Expose metrics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes 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. ✨ 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!
I've implemented support for ClickHouse Keeper within the ClickHouse Helm chart. This change introduces a new component that provides distributed coordination for ClickHouse, improving its resilience and scalability. The update includes new configuration options for the keeper and integrates it seamlessly with the existing ClickHouse deployment, allowing for more robust distributed setups.
Highlights
- ClickHouse Keeper Integration: I've introduced a new Helm template (
chkeeper.yaml) to deploy ClickHouse Keeper, a distributed coordination service that enhances the high availability and distributed capabilities of ClickHouse clusters. - New Configuration Options: I've added new configuration parameters (
clickhouseKeeper.enabled,clickhouseKeeper.size,clickhouseKeeper.resourcesPreset) tovalues.yamlandvalues.schema.json. These parameters allow users to easily enable, configure, and size the ClickHouse Keeper deployment. - Conditional ZooKeeper Configuration: The main ClickHouse Helm chart (
clickhouse.yaml) now includes conditional logic to configure ClickHouse to use the newly deployed ClickHouse Keeper instance as its ZooKeeper when theclickhouseKeeperfeature is enabled. - Chart Version Bump: To signify the addition of this significant new feature, I've incremented the chart version in
Chart.yamlto0.11.2.
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. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces support for ClickHouse Keeper, which is a great addition for enabling replication and distributed features. The implementation is mostly correct, but there are a few critical issues that would prevent the chart from deploying successfully, such as a placeholder in the Keeper service hostname and a misconfigured volume claim template. I've also identified some areas for improvement, like making the number of Keeper replicas and the container image configurable, which would increase the chart's flexibility. Additionally, there are some documentation inconsistencies that should be addressed for clarity.
| templates: | ||
| # Templates are specified as default for all clusters | ||
| podTemplate: default | ||
| dataVolumeClaimTemplate: default |
There was a problem hiding this comment.
There is a mismatch between the dataVolumeClaimTemplate name referenced in spec.defaults.templates and the one defined in spec.templates.volumeClaimTemplates. The reference is to default, but the template is named both-paths. This will cause the ClickHouse operator to fail when creating the Keeper installation.
dataVolumeClaimTemplate: both-paths
| {{- if .Values.clickhouseKeeper.enabled }} | ||
| zookeeper: | ||
| nodes: | ||
| - host: "clickhouse-asdf-keeper.{{ .Release.Namespace }}" |
There was a problem hiding this comment.
The host for the zookeeper node is using a placeholder asdf and an incorrect prefix clickhouse-. The service for ClickHouse Keeper will be named after the ClickHouseKeeperInstallation resource, which is {{ .Release.Name }}-keeper. This hardcoded and incorrect hostname will prevent ClickHouse from connecting to its Keeper.
- host: "{{ .Release.Name }}-keeper"
| - name: clickhouse-keeper | ||
| imagePullPolicy: IfNotPresent | ||
| image: clickhouse/clickhouse-keeper:24.9.2.42 | ||
| resources: {{- include "cozy-lib.resources.defaultingSanitize" (list .Values.clickhouseKeeper.resourcesPreset .Values.resources $) | nindent 20 }} |
There was a problem hiding this comment.
The VMPodScrape resource (defined from line 71) is configured to scrape metrics from a port named metrics. However, the clickhouse-keeper container spec does not define any named ports. This will cause scraping to fail. You should add a named port to the container spec.
- name: clickhouse-keeper
imagePullPolicy: IfNotPresent
image: clickhouse/clickhouse-keeper:24.9.2.42
ports:
- name: metrics
containerPort: 7000
resources: {{- include "cozy-lib.resources.defaultingSanitize" (list .Values.clickhouseKeeper.resourcesPreset .Values.resources $) | nindent 20 }}
packages/apps/clickhouse/README.md
Outdated
|
|
||
| | Name | Description | Value | | ||
| | ---------------------------------- | --------------------------------------------------------------------------------------------------------------------------- | ------- | | ||
| | `clickhouseKeeper.enabled` | Enable periodic backups | `true` | |
There was a problem hiding this comment.
The description for clickhouseKeeper.enabled is misleading. It states 'Enable periodic backups', which is incorrect. ClickHouse Keeper is a coordination system (similar to ZooKeeper) used for managing replication and distributed DDL queries, not for backups. This should be corrected to avoid confusion.
| clusters: | ||
| - name: "cluster1" | ||
| layout: | ||
| replicasCount: 3 |
There was a problem hiding this comment.
The replicasCount for ClickHouse Keeper is hardcoded to 3. While this is a good default for production, it prevents users from running a single-node Keeper for development/testing, or scaling to a larger quorum (e.g., 5 nodes). It would be better to make this configurable via values.yaml. You'll also need to add a replicas field to values.yaml and values.schema.json under the clickhouseKeeper section.
replicasCount: {{ .Values.clickhouseKeeper.replicas | default 3 }}
| "properties": { | ||
| "enabled": { | ||
| "default": true, | ||
| "description": "Enable periodic backups", |
There was a problem hiding this comment.
The description for enabled under clickhouseKeeper is incorrect. It seems to be copied from the backup configuration. ClickHouse Keeper is a coordination system (like ZooKeeper) and is not used for backups.
"description": "Enable ClickHouse Keeper for replication and distributed DDL queries",
packages/apps/clickhouse/values.yaml
Outdated
| @@ -56,3 +56,11 @@ backup: | |||
| s3SecretKey: ju3eum4dekeich9ahM1te8waeGai0oog | |||
| resticPassword: ChaXoveekoh6eigh4siesheeda2quai0 | |||
|
|
|||
| ## @section clickhouseKeeper parameters | |||
| ## @param clickhouseKeeper.enabled Enable periodic backups | |||
There was a problem hiding this comment.
The comment describing clickhouseKeeper.enabled is incorrect. It should explain that ClickHouse Keeper is used for coordination, not backups.
## @param clickhouseKeeper.enabled Enable ClickHouse Keeper for replication and distributed DDL queries
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/apps/clickhouse/Chart.yaml(1 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/values.schema.json(1 hunks)packages/apps/clickhouse/values.yaml(1 hunks)packages/apps/versions_map(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: in packages/apps/versions_map, using head for certain versions (like ferretdb 1.0.0) is intentional ...
Learnt from: kvaps
PR: cozystack/cozystack#1206
File: packages/apps/versions_map:30-31
Timestamp: 2025-07-16T13:17:07.580Z
Learning: In packages/apps/versions_map, using HEAD for certain versions (like ferretdb 1.0.0) is intentional and part of the core logic, not an oversight that needs to be fixed for reproducibility.
Applied to files:
packages/apps/versions_map
🪛 YAMLlint (1.37.1)
packages/apps/clickhouse/templates/chkeeper.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
⏰ 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 (4)
packages/apps/versions_map (1)
18-19: Confirm commit pin & HEAD usage are intentional.You froze 0.11.1 to
0e47e1e8and left 0.11.2 onHEAD, reversing the usual “latest → HEAD, previous → commit” pattern seen for some but not all charts.
Just sanity-check that0e47e1e8is the correct commit for 0.11.1 and that you really want 0.11.2 tracking HEAD.packages/apps/clickhouse/templates/chkeeper.yaml (2)
84-86: AlignVMPodScrapeportfield with container definition.After adding the named container port, you can keep
port: metrics.
If you decide not to add a named port, change this field to the numeric value7000.
Either way, ensure the two pieces stay in sync to avoid silent metrics loss.
1-3: YAML-lint false positive is safe to ignore.The reported
syntax error: expected the node content, but found '-'is produced because the linter does not understand Helm directives ({{- if … }}). No action needed.packages/apps/clickhouse/Chart.yaml (1)
19-19: Chart version bumped—verify accompanying artifacts are in syncThe patch-level bump to
0.11.2looks good and follows SemVer.
Please double-check that:
CHANGELOG.md(if present) reflects this version.packages/apps/versions_map/**and any dependent charts that pin the ClickHouse chart version were updated together with this change.- CI publishing pipelines (e.g., chart-museum index) pick up the new version tag.
| - name: clickhouse-keeper | ||
| imagePullPolicy: IfNotPresent | ||
| image: clickhouse/clickhouse-keeper:24.9.2.42 | ||
| resources: {{- include "cozy-lib.resources.defaultingSanitize" (list .Values.clickhouseKeeper.resourcesPreset .Values.resources $) | nindent 20 }} |
There was a problem hiding this comment.
Expose a named container port to match VMPodScrape expectation.
VMPodScrape (Line 84) targets port: metrics, but the container spec defines no port named metrics. Without it, the scrape will fail.
containers:
- name: clickhouse-keeper
imagePullPolicy: IfNotPresent
image: clickhouse/clickhouse-keeper:24.9.2.42
+ ports:
+ - name: metrics
+ containerPort: 7000
resources: {{- include "cozy-lib.resources.defaultingSanitize" (list .Values.clickhouseKeeper.resourcesPreset .Values.resources $) | nindent 20 }} 📝 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.
| - name: clickhouse-keeper | |
| imagePullPolicy: IfNotPresent | |
| image: clickhouse/clickhouse-keeper:24.9.2.42 | |
| resources: {{- include "cozy-lib.resources.defaultingSanitize" (list .Values.clickhouseKeeper.resourcesPreset .Values.resources $) | nindent 20 }} | |
| - name: clickhouse-keeper | |
| imagePullPolicy: IfNotPresent | |
| image: clickhouse/clickhouse-keeper:24.9.2.42 | |
| ports: | |
| - name: metrics | |
| containerPort: 7000 | |
| resources: {{- include "cozy-lib.resources.defaultingSanitize" (list .Values.clickhouseKeeper.resourcesPreset .Values.resources $) | nindent 20 }} |
🤖 Prompt for AI Agents
In packages/apps/clickhouse/templates/chkeeper.yaml around lines 55 to 58, the
container spec for clickhouse-keeper lacks a named port "metrics" which is
required by the VMPodScrape configuration at line 84. Add a ports section to the
container spec defining a containerPort with the name "metrics" and the
appropriate port number to ensure the scrape target can connect correctly.
| "resourcesPreset": { | ||
| "default": "micro", | ||
| "description": "Default sizing preset used when `resources` is omitted. Allowed values: nano, micro, small, medium, large, xlarge, 2xlarge.", | ||
| "type": "string" | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
resourcesPreset allows specific values but the schema does not enforce them.
You list the allowed presets in the description yet omit an enum, unlike the top-level resourcesPreset. Add the same enumeration here to catch typos at schema-validation time.
"resourcesPreset": {
"default": "micro",
"description": "Default sizing preset used when `resources` is omitted. Allowed values: nano, micro, small, medium, large, xlarge, 2xlarge.",
- "type": "string"
+ "type": "string",
+ "enum": ["nano","micro","small","medium","large","xlarge","2xlarge"]
},📝 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.
| "resourcesPreset": { | |
| "default": "micro", | |
| "description": "Default sizing preset used when `resources` is omitted. Allowed values: nano, micro, small, medium, large, xlarge, 2xlarge.", | |
| "type": "string" | |
| }, | |
| "resourcesPreset": { | |
| "default": "micro", | |
| "description": "Default sizing preset used when `resources` is omitted. Allowed values: nano, micro, small, medium, large, xlarge, 2xlarge.", | |
| "type": "string", | |
| "enum": ["nano","micro","small","medium","large","xlarge","2xlarge"] | |
| }, |
🤖 Prompt for AI Agents
In packages/apps/clickhouse/values.schema.json around lines 55 to 59, the
resourcesPreset property lists allowed values in the description but does not
enforce them with an enum. Add an enum field with the allowed values nano,
micro, small, medium, large, xlarge, 2xlarge to the resourcesPreset schema to
ensure validation catches invalid values and typos.
9a2fcbf to
bc1779d
Compare
ca93678 to
66ddb04
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseoperatorconfigurations.clickhouse.altinity.com.yaml (1)
226-245: Duplicatemetricsproperty – schema will be rejected by Kubernetes
spec.metricsis declared twice:
- Lines 226-244 – connection time-out settings
- Lines 431-445 – label-exclusion settings
YAML allows only one key with a given name in a mapping; the second occurrence silently overrides the first when parsed, so one of the sections will be lost and
kubectl applywill fail OpenAPI validation.Merge both definitions under a single
metricsobject, e.g.:spec: ... - metrics: - type: object - description: "parameters which use for connect to fetch metrics from clickhouse by clickhouse-operator" - properties: - timeouts: ... + metrics: + type: object + description: "Metric-exporter options" + properties: + timeouts: # existing block + ... + labels: # new block + type: object + description: "defines metric labels options" + properties: + exclude: + type: array + description: "When adding labels to a metric exclude labels from the list" + items: + type: stringRun
kubectl apply -fon the rendered CRD to ensure the schema now validates.Also applies to: 431-445
♻️ Duplicate comments (4)
packages/apps/clickhouse/values.yaml (1)
60-60: Fix the documentation comment and remove trailing spaces.The comment incorrectly mentions "Deploy ClickHouse Keeper for cluster coordination" but has trailing spaces that need to be removed.
Apply this diff to fix both issues:
-## @param clickhouseKeeper.enabled Deploy ClickHouse Keeper for cluster coordination +## @param clickhouseKeeper.enabled Deploy ClickHouse Keeper for cluster coordinationpackages/apps/clickhouse/templates/chkeeper.yaml (3)
85-86: Container port still not exposed – duplicate of previous review
VMPodScrapeexpectsport: metrics, but the container spec lacks it. See earlier comment; fix is still required.
14-16: Make replica count configurable via values.yaml
replicasCountis hard-coded to3, preventing users from running dev/test single-node clusters or larger quorums. Expose it through.Values.clickhouseKeeper.replicaswith a sensible default.- replicasCount: 3 + replicasCount: {{ .Values.clickhouseKeeper.replicas | default 3 }}
56-59: Hard-coded image and missingmetricsport
- Image repository/tag should be configurable from
values.yaml.VMPodScrapetargets a port namedmetrics, but the container exposes none, so scraping will fail.- imagePullPolicy: IfNotPresent - image: clickhouse/clickhouse-keeper:24.9.2.42 - resources: {{- include "cozy-lib.resources.defaultingSanitize" (list .Values.clickhouseKeeper.resourcesPreset .Values.resources $) | nindent 20 }} + imagePullPolicy: IfNotPresent + image: {{ .Values.clickhouseKeeper.image.repository | default "clickhouse/clickhouse-keeper" }}:{{ .Values.clickhouseKeeper.image.tag | default "24.9.2.42" }} + ports: + - name: metrics + containerPort: 7000 + resources: {{- include "cozy-lib.resources.defaultingSanitize" (list .Values.clickhouseKeeper.resourcesPreset .Values.resources $) | nindent 20 }}
🧹 Nitpick comments (9)
packages/apps/clickhouse/templates/workloadmonitor.yaml (1)
20-21: Consider making Keeper replica count configurable.The hardcoded replica count of 3 for Keeper might not be suitable for all deployments. Consider making this configurable through values.
Apply this diff to make the replica count configurable:
- replicas: 3 - minReplicas: 3 + replicas: {{ .Values.clickhouseKeeper.replicas | default 3 }} + minReplicas: {{ .Values.clickhouseKeeper.replicas | default 3 }}And add the corresponding value to
values.yaml:clickhouseKeeper: enabled: true replicas: 3 # Add this line size: 1Gi resourcesPreset: micropackages/system/clickhouse-operator/charts/altinity-clickhouse-operator/README.md (1)
70-75: Typo in documentation
Ypu→You.- ServiceMonitor ... operator-metrics # 9999. Ypu can specify interval + ServiceMonitor ... operator-metrics # 9999. You can specify intervalpackages/system/clickhouse-operator/charts/altinity-clickhouse-operator/values.yaml (1)
96-109: Correct typo in commentSame
Yputypo and truncated wordingestio.- # In serviceMonitor will be created two endpoints ... 9999. Ypu can specify interval, scrapeTimeout, relabelings, metricRelabelings for each endpoint below + # Two endpoints will be created (clickhouse-metrics on 8888, operator-metrics on 9999). You can specify interval, scrapeTimeout, relabelings, metricRelabelings for each endpoint below ... - # serviceMonitor.metricRelabelings for clickhouse-metrics endpoint -- Prometheus [MetricRelabelConfigs] to apply to samples before ingestio + # serviceMonitor.metricRelabelings for clickhouse-metrics endpoint -- Prometheus [MetricRelabelConfigs] to apply to samples before ingestionpackages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallationtemplates.clickhouse.altinity.com.yaml (3)
85-90: Consider exposingsuspendas a boolean rather than a string-bool surrogate
additionalPrinterColumnsdeclaressuspendastype: stringand the schema inherits theTypeStringBoolenumeration.
While this is consistent with existing flags (stop,troubleshoot), it diverges from native Kubernetes style where boolean CRD fields usetype: boolean. Migrating to a real boolean would simplifykubectlUX and reduce invalid input risk.
436-450: Validate extreme values for shard-reconcile concurrency
reconcileShardsThreadsNumber(1-65535) andreconcileShardsMaxConcurrencyPercent(0-100) are unconstrained relative to the actual shard count.
Without additional server-side validation the operator could be mis-configured with more concurrent threads than shards or a percentage >100, leading to wasted goroutines or panics in the scheduler.
Recommend adding admission-webhook logic or runtime clamps.
1256-1259: Typo: trailing quote in docstringThe help text for
topologyKeyends with an extra".
Minor, but this flows into generated docs and makeskubectl explainoutput look messy.- ..., more info: https://kubernetes.io/docs/concepts/scheduling-eviction/assign-pod-node/#inter-pod-affinity-and-anti-affinity" + ..., more info: https://kubernetes.io/docs/concepts/scheduling-eviction/assign-pod-node/#inter-pod-affinity-and-anti-affinitypackages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhousekeeperinstallations.clickhouse-keeper.altinity.com.yaml (2)
367-384: Anchor reuse is helpful but masks validation errors
The ObjectsCleanup anchors are merged into several properties. If the enum values or description are later updated in the anchor, every merge site must be reconverted to JSON before admission. Consider generating the CRD from code (e.g. controller-gen) instead of hand-maintaining YAML with anchors; this avoids subtle drift and produces cleaner CRDs.Also applies to: 393-404
284-316: Enum list for StringBool triples chart size & hampers readability
Enumerating every textual variant of boolean (["0","1","False","false",…]) inflates the CRD size and forces tight coupling between validation and parsing logic. Prefertype: stringwith a descriptive comment or accept only canonical"true"|"false"values and convert upstream.packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/files/ClickHouseKeeper_dashboard.json (1)
985-1003: Redundantds_prometheustemplate variableA datasource template variable named
ds_prometheusis created, but all queries already rely on theDS_PROMETHEUSinput variable. The extra variable is never referenced and may confuse users, especially when they see two datasource prompts on import.- { - "current": { - "selected": false, - "text": "", - "value": "" - }, - "hide": 0, - "includeAll": false, - "multi": false, - "name": "ds_prometheus", - ... - "type": "datasource" - },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (17)
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ClusterRole-clickhouse-operator-kube-system.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ClusterRoleBinding-clickhouse-operator-kube-system.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-clickhouse-operator-confd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-clickhouse-operator-configd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-clickhouse-operator-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-clickhouse-operator-templatesd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-clickhouse-operator-usersd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-keeper-operator-confd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-keeper-operator-configd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-keeper-operator-templatesd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-keeper-operator-usersd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/Deployment-clickhouse-operator.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/Role-clickhouse-operator.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/RoleBinding-clickhouse-operator.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/Secret-clickhouse-operator.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/Service-clickhouse-operator-metrics.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ServiceAccount-clickhouse-operator.yamlis excluded by!**/generated/**
📒 Files selected for processing (23)
packages/apps/clickhouse/Chart.yaml(1 hunks)packages/apps/clickhouse/Makefile(1 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/workloadmonitor.yaml(1 hunks)packages/apps/clickhouse/values.schema.json(1 hunks)packages/apps/clickhouse/values.yaml(1 hunks)packages/apps/versions_map(1 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/Chart.yaml(2 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/README.md(2 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/README.md.gotmpl(1 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallations.clickhouse.altinity.com.yaml(20 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallationtemplates.clickhouse.altinity.com.yaml(20 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhousekeeperinstallations.clickhouse-keeper.altinity.com.yaml(7 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseoperatorconfigurations.clickhouse.altinity.com.yaml(5 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/files/ClickHouseKeeper_dashboard.json(4 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/files/ClickHouse_Queries_dashboard.json(2 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/_helpers.tpl(3 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/dashboards-configmap.yaml(1 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/dashboards-secret.yaml(0 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/servicemonitor.yaml(1 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/values.yaml(14 hunks)
💤 Files with no reviewable changes (1)
- packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/dashboards-secret.yaml
✅ Files skipped from review due to trivial changes (6)
- packages/apps/clickhouse/templates/clickhouse.yaml
- packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/Chart.yaml
- packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/README.md.gotmpl
- packages/apps/clickhouse/Makefile
- packages/apps/clickhouse/README.md
- packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/files/ClickHouse_Queries_dashboard.json
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/apps/versions_map
- packages/apps/clickhouse/values.schema.json
- packages/apps/clickhouse/Chart.yaml
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: in helm templates, the `{{-` directive chomps all leading whitespace including newlines back to the ...
Learnt from: lllamnyp
PR: cozystack/cozystack#1160
File: packages/system/hetzner-robotlb/charts/robotlb/templates/role.yaml:6-8
Timestamp: 2025-07-11T06:28:13.696Z
Learning: In Helm templates, the `{{-` directive chomps all leading whitespace including newlines back to the previous content, so `{{- toYaml .Values.something | nindent 2 }}` will render correctly even with apparent indentation issues. However, for better style, it's cleaner to put the template directive on the same line as the parent key (e.g., `rules: {{- toYaml .Values.serviceAccount.permissions | nindent 2 }}`).
Applied to files:
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/_helpers.tpl
📚 Learning: in cozystack, the plural form for the kubernetes custom resource is `kuberneteses.apps.cozystack.io`...
Learnt from: lllamnyp
PR: cozystack/cozystack#1130
File: hack/e2e-apps/kubernetes.bats:101-101
Timestamp: 2025-07-11T06:11:25.438Z
Learning: In cozystack, the plural form for the Kubernetes custom resource is `kuberneteses.apps.cozystack.io`, not `kubernetes.apps.cozystack.io`. This is defined in the API schema even though it's not grammatically perfect.
Applied to files:
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallationtemplates.clickhouse.altinity.com.yamlpackages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallations.clickhouse.altinity.com.yaml
📚 Learning: the cozyvalues-gen tool has a known issue (https://github.com/cozystack/cozyvalues-gen/issues/10) wh...
Learnt from: NickVolynkin
PR: cozystack/cozystack#1216
File: packages/extra/monitoring/README.md:30-31
Timestamp: 2025-07-26T18:26:01.447Z
Learning: The cozyvalues-gen tool has a known issue (https://github.com/cozystack/cozyvalues-gen/issues/10) where it incorrectly places Grafana configuration entries under the wrong documentation sections, specifically placing grafana.resources.requests.* entries in the Metrics storage configuration table instead of the Grafana configuration table.
Applied to files:
packages/apps/clickhouse/templates/chkeeper.yamlpackages/apps/clickhouse/values.yaml
📚 Learning: in the cozystack repository, for the virtual-machine app's resources.sockets parameter, the value is...
Learnt from: NickVolynkin
PR: cozystack/cozystack#1216
File: packages/apps/virtual-machine/values.yaml:0-0
Timestamp: 2025-07-26T18:12:05.641Z
Learning: In the cozystack repository, for the virtual-machine app's resources.sockets parameter, the value is intentionally kept as a string in values.yaml despite being documented as {int} type, because the string-to-integer parsing happens elsewhere in the system pipeline rather than at the YAML validation level.
Applied to files:
packages/apps/clickhouse/templates/chkeeper.yaml
📚 Learning: the `./charts/robotlb/` directory in the hetzner-robotlb package contains vendored code, and the tea...
Learnt from: lllamnyp
PR: cozystack/cozystack#1233
File: packages/system/hetzner-robotlb/charts/robotlb/templates/deployment.yaml:33-35
Timestamp: 2025-07-23T09:15:09.658Z
Learning: The `./charts/robotlb/` directory in the hetzner-robotlb package contains vendored code, and the team generally avoids modifying vendored code to maintain clean separation from upstream dependencies.
Applied to files:
packages/apps/clickhouse/templates/chkeeper.yaml
🪛 YAMLlint (1.37.1)
packages/apps/clickhouse/templates/workloadmonitor.yaml
[error] 15-15: syntax error: could not find expected ':'
(syntax)
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/servicemonitor.yaml
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
packages/apps/clickhouse/templates/chkeeper.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/apps/clickhouse/values.yaml
[error] 60-60: trailing spaces
(trailing-spaces)
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/dashboards-configmap.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
[warning] 14-14: wrong indentation: expected 2 but found 4
(indentation)
[warning] 15-15: wrong indentation: expected 2 but found 4
(indentation)
[warning] 16-16: wrong indentation: expected 2 but found 4
(indentation)
🪛 LanguageTool
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/README.md
[grammar] ~72-~72: Ensure spelling is correct
Context: ... port 8888 and operator-metrics # 9999. Ypu can specify interval, scrapeTimeout, re...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.17.2)
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/README.md
36-36: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (18)
packages/apps/clickhouse/values.yaml (1)
59-66: LGTM! ClickHouse Keeper configuration is well-structured.The new clickhouseKeeper configuration section follows the established patterns in the file and provides appropriate defaults for Keeper deployment.
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/dashboards-configmap.yaml (1)
1-21: LGTM! ConfigMap template is correctly implemented.The dashboard ConfigMap template follows Helm best practices with proper conditional rendering, metadata templating, and file globbing. The static analysis warnings are false positives as they don't account for Helm template rendering.
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/servicemonitor.yaml (2)
6-6: LGTM! Proper use of new namespace template.The namespace field now uses the new
altinity-clickhouse-operator.namespacetemplate helper, enabling namespace override functionality.
16-44: LGTM! Enhanced ServiceMonitor configuration provides excellent flexibility.The addition of configurable monitoring parameters (interval, scrapeTimeout, relabelings, metricRelabelings) for both clickhouse-metrics and operator-metrics endpoints significantly improves observability customization capabilities.
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/_helpers.tpl (3)
2-11: LGTM! Namespace override template enhances deployment flexibility.The new
altinity-clickhouse-operator.namespacetemplate properly implements namespace override functionality, enabling multi-namespace deployments while maintaining backward compatibility.
54-55: LGTM! Label sourcing updated to use commonLabels.The change from
.Values.podLabelsto.Values.commonLabelsprovides better consistency across resources and aligns with the chart's enhanced labeling strategy.
68-77: LGTM! Centralized annotation management improves consistency.The new
altinity-clickhouse-operator.annotationstemplate provides a clean way to manage common annotations including Helm metadata and user-defined annotations across all resources.packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallationtemplates.clickhouse.altinity.com.yaml (3)
52-60: EnsurehostsCompletedis always populated by the operatorThe new printer column
hosts-completedpoints to.status.hostsCompleted.
Please double-check that the reconciler sets this field for all lifecycle paths, otherwise the column will remain empty and the UX benefit of the rename over the previoushosts-unchangedcolumn will be lost.
608-611:availabilityZonekey breaks backward compatibility with strict validationAdding
availabilityZoneto Zookeeper node objects is useful, but existing CRDs applied withkubectl apply --server-sidewill now be rejected unless all manifests are regenerated.
Call this out in release notes and bumpspec.preserveUnknownFieldsaccordingly to smooth upgrades.
450-492: Potential naming collision: pluralserviceTemplatesalongside legacyserviceTemplateIntroducing the array
serviceTemplateswhile still keeping the singular field may confuse users:
- Which one is honoured when both are set?
- Is the singular soon-to-be deprecated?
Consider deprecating the singular key explicitly (via
x-kubernetes-deprecated) or adding mutual-exclusion validation to avoid silent overrides.⛔ Skipped due to learnings
Learnt from: NickVolynkin PR: cozystack/cozystack#1216 File: packages/apps/virtual-machine/values.yaml:31-33 Timestamp: 2025-07-26T18:01:52.557Z Learning: In cozystack's schema generator annotation format, when documenting fields of array items, use the singular form of the item type rather than array notation. For example, for a parameter `gpus {[]gpu}`, use `@field gpu.name` rather than `@field gpus[].name` to refer to the name field of each GPU object in the array.Learnt from: lllamnyp PR: cozystack/cozystack#1130 File: hack/e2e-apps/kubernetes.bats:101-101 Timestamp: 2025-07-11T06:11:25.438Z Learning: In cozystack, the plural form for the Kubernetes custom resource is `kuberneteses.apps.cozystack.io`, not `kubernetes.apps.cozystack.io`. This is defined in the API schema even though it's not grammatically perfect.packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallations.clickhouse.altinity.com.yaml (6)
85-90: Usebooleanrather than the string-enum surrogate forspec.suspend
suspendis modelled via the*TypeStringBoolalias (a string with dozens of literal values).
Because the operator only needs an on/off switch, representing the field astype: boolean(with an optionaldefault: false) simplifies:
- client-side UX (kubectl/kustomize autocompletion, Helm value templating)
- CRD validation – you can drop the unwieldy 30-entry enum
- printer column ‑ the
typecould then beboolean, notstring- suspend: - !!merge <<: *TypeStringBool + suspend: + type: boolean + default: falseUnless you have a hard requirement to keep the historic string values, consider migrating now while the field is still new.
[ suggest_essential_refactor ]Also applies to: 318-325
501-506: Missingdefaultcontradicts documentation forreplicasUseFQDNThe description says
"no" by default, yet nodefault:keyword is provided.
Either add the default explicitly or re-phrase the description to avoid misleading chart users.replicasUseFQDN: !!merge <<: *TypeStringBool description: | define should replicas be specified by FQDN … - "no" by default + default: "no"[ request_verification ]
556-562: Ambiguous overlap betweenserviceTemplate(singular) and newserviceTemplates(array)Now that both keys exist, Helm/CI validation may not prevent users from setting both simultaneously, producing undefined precedence.
Recommend:
- Clarify precedence rules in the description.
- Add
x-kubernetes-mutually-exclusive(structural schema extension) or admission-webhook validation to reject dual usage.[ offer_architecture_advice ]
800-807:pdbMaxUnavailableupper bound should respect PDB semanticsThe current
maximum: 65535is formally correct but semantically meaningless: PDB calculates availability as absolute replica count when given an int.Consider constraining the value to the shard replica count at runtime or at least documenting that any value larger than replicas count is effectively “no restriction”.
[ suggest_optional_refactor ]
1255-1259: Minor typo & link formatting
more info:sentence is outside the block quote and starts with a lowercase “m”. Also, the closing quote (") at the end of the line looks superfluous.- more info: https://kubernetes.io/… +More info: https://kubernetes.io/…[ suggest_nitpick ]
1327-1333: Spelling: “clichouse-operator” → “clickhouse-operator”Line 1338 (
More info: https://git.k8s.io/...clichouse-operator) is missing the “k”.[ suggest_nitpick ]
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/files/ClickHouseKeeper_dashboard.json (2)
17-19: Confirm Grafana 11.3.0 requirement is intentionalThe dashboard declares a hard requirement on Grafana
11.3.0, a version that is not released at the time of writing (latest 10.x). This will prevent import on most clusters unless users override the check manually.If the dashboard truly needs unreleased features, please add a compatibility note to the chart README; otherwise lower
__requires[].versionto the minimum supported Grafana version (e.g.9.5.0) and droppluginVersionpinning inside each panel.
151-161: Verify latency metric unitsThe “Latency” panel displays
ClickHouseAsyncMetrics_KeeperAvgLatency/KeeperMaxLatencyas milliseconds (unit="ms"), but ClickHouse Keeper usually exposes these latencies in micro-seconds. If that is the case the graph will be off by ×1000.Please double-check the metric exposition and, if needed, switch the unit to
µsor divide the value by1000in the query (/ 1000).
| suspend: | ||
| !!merge <<: *TypeStringBool | ||
| description: | | ||
| Suspend reconciliation of resources managed by a ClickHouse Installation. | ||
| Works as the following: | ||
| - When `suspend` is `true` operator stops reconciling all resources. | ||
| - When `suspend` is `false` or not set, operator reconciles all resources. | ||
| troubleshoot: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify precedence of suspend vs. existing stop flag
Both spec.stop and the newly-added spec.suspend gate reconciliation but in subtly different ways.
Please document in the CRD (or operator docs) which flag wins when both are set, and whether suspend=true implies/overrides stop=1. Ambiguity here can lead to unexpected pod deletions or missed restarts in production.
🤖 Prompt for AI Agents
In
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallationtemplates.clickhouse.altinity.com.yaml
around lines 318 to 325, the documentation for the `suspend` field lacks clarity
on its precedence relative to the existing `stop` flag. Update the description
to explicitly state which flag takes precedence when both `spec.suspend` and
`spec.stop` are set, and clarify if `suspend=true` overrides or implies
`stop=1`. This will prevent ambiguity and ensure users understand the
reconciliation behavior clearly.
| type: integer | ||
| description: | | ||
| Pod eviction is allowed if at most "pdbMaxUnavailable" pods are unavailable after the eviction, | ||
| i.e. even in absence of the evicted pod. For example, one can prevent all voluntary evictions | ||
| by specifying 0. This is a mutually exclusive setting with "minAvailable". | ||
| minimum: 0 | ||
| maximum: 65535 | ||
| reconcile: |
There was a problem hiding this comment.
pdbMaxUnavailable vs. existing PDB knobs
The new pdbMaxUnavailable integer is mutually exclusive with minAvailable, but the schema does not enforce this.
A user can set both and kube-controller-manager will accept an invalid PDB object. Consider adding a CEL validation rule or webhook to guard against the illegal combination.
🤖 Prompt for AI Agents
In
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallationtemplates.clickhouse.altinity.com.yaml
around lines 800 to 807, the schema allows both pdbMaxUnavailable and
minAvailable to be set simultaneously, which is invalid. To fix this, add a CEL
validation rule or webhook that enforces mutual exclusivity between
pdbMaxUnavailable and minAvailable, preventing users from setting both at the
same time.
| additionalPrinterColumns: | ||
| - name: version | ||
| type: string | ||
| description: Operator version | ||
| priority: 1 # show in wide view | ||
| jsonPath: .status.chop-version | ||
| - name: clusters | ||
| type: integer | ||
| description: Clusters count | ||
| jsonPath: .status.clusters | ||
| - name: shards | ||
| type: integer | ||
| description: Shards count | ||
| priority: 1 # show in wide view | ||
| jsonPath: .status.shards | ||
| - name: hosts | ||
| type: integer | ||
| description: Hosts count | ||
| jsonPath: .status.hosts | ||
| - name: taskID | ||
| type: string | ||
| description: TaskID | ||
| priority: 1 # show in wide view | ||
| jsonPath: .status.taskID | ||
| - name: status | ||
| type: string | ||
| description: CHK status | ||
| description: Resource status | ||
| jsonPath: .status.status | ||
| - name: replicas | ||
| - name: hosts-unchanged | ||
| type: integer | ||
| description: Replica count | ||
| description: Unchanged hosts count | ||
| priority: 1 # show in wide view | ||
| jsonPath: .status.replicas | ||
| jsonPath: .status.hostsUnchanged | ||
| - name: hosts-updated | ||
| type: integer | ||
| description: Updated hosts count | ||
| priority: 1 # show in wide view | ||
| jsonPath: .status.hostsUpdated | ||
| - name: hosts-added | ||
| type: integer | ||
| description: Added hosts count | ||
| priority: 1 # show in wide view | ||
| jsonPath: .status.hostsAdded | ||
| - name: hosts-completed | ||
| type: integer | ||
| description: Completed hosts count | ||
| jsonPath: .status.hostsCompleted | ||
| - name: hosts-deleted | ||
| type: integer | ||
| description: Hosts deleted count | ||
| priority: 1 # show in wide view | ||
| jsonPath: .status.hostsDeleted | ||
| - name: hosts-delete | ||
| type: integer | ||
| description: Hosts to be deleted count | ||
| priority: 1 # show in wide view | ||
| jsonPath: .status.hostsDelete | ||
| - name: endpoint | ||
| type: string | ||
| description: Client access endpoint | ||
| priority: 1 # show in wide view | ||
| jsonPath: .status.endpoint | ||
| - name: age | ||
| type: date | ||
| description: Age of the resource | ||
| # Displayed in all priorities | ||
| jsonPath: .metadata.creationTimestamp |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Kubernetes only allows ≤ 10 additionalPrinterColumns
This CRD declares 20+ columns. kubectl apply against apiserver ≥1.26 rejects CRDs that exceed the limit (currently 10). Decide which columns are essential and trim the remainder or collapse related counts into a structured status field instead.
🤖 Prompt for AI Agents
In
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhousekeeperinstallations.clickhouse-keeper.altinity.com.yaml
between lines 24 and 90, the CRD defines more than 10 additionalPrinterColumns,
which exceeds Kubernetes' limit and causes rejection on apiserver versions 1.26
and above. To fix this, reduce the number of additionalPrinterColumns to 10 or
fewer by selecting only the most essential columns to display. Consider
consolidating related counts into a single structured status field or removing
less critical columns to comply with the limit.
| - name: version | ||
| type: string | ||
| description: Operator version | ||
| priority: 1 # show in wide view | ||
| jsonPath: .status.chop-version | ||
| - name: clusters | ||
| type: integer | ||
| description: Clusters count | ||
| jsonPath: .status.clusters | ||
| - name: shards |
There was a problem hiding this comment.
JSONPath needs bracket-notation for keys containing hyphens
Properties such as chop-version, chop-commit, chop-date, etc. are referenced with dot-notation (.status.chop-version).
In JSONPath a hyphen is interpreted as the minus operator, so the CRD will fail validation (jsonpath: expected identifier). Use bracket notation instead:
-jsonPath: .status.chop-version
+jsonPath: .status["chop-version"]Apply the same pattern for every additionalPrinterColumn that targets a *- key.
Also applies to: 124-135
🤖 Prompt for AI Agents
In
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhousekeeperinstallations.clickhouse-keeper.altinity.com.yaml
around lines 25 to 34 and also lines 124 to 135, the JSONPath expressions use
dot-notation for keys containing hyphens like .status.chop-version, which causes
validation errors. Replace these with bracket-notation by enclosing the key in
brackets and quotes, e.g., .status["chop-version"], for all
additionalPrinterColumn entries referencing keys with hyphens.
| !!merge <<: *TypeStringBool | ||
| description: | | ||
| Suspend reconciliation of resources managed by a ClickHouse Keeper. | ||
| Works as the following: | ||
| - When `suspend` is `true` operator stops reconciling all resources. | ||
| - When `suspend` is `false` or not set, operator reconciles all resources. | ||
| namespaceDomainPattern: |
There was a problem hiding this comment.
!!merge tag is non-standard YAML and breaks CRD parsing
Kubernetes’ YAML loader recognises the merge-key syntax <<: *anchor but not the explicit tag !!merge.
Replace:
suspend:
!!merge <<: *TypeStringBoolwith:
suspend:
<<: *TypeStringBool(All other occurrences—pvc, configMap, service, etc.—need the same treatment.)
🤖 Prompt for AI Agents
In
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhousekeeperinstallations.clickhouse-keeper.altinity.com.yaml
around lines 317 to 323, the use of the non-standard YAML tag `!!merge` before
the merge key `<<: *TypeStringBool` breaks CRD parsing. Remove the `!!merge` tag
and keep only the merge key syntax `<<: *TypeStringBool`. Apply this fix
consistently to all similar occurrences such as `pvc`, `configMap`, and
`service` in the file.
| type: object | ||
| description: "Whether the operator during reconcile procedure should wait for replicas to catch-up" | ||
| properties: | ||
| all: | ||
| !!merge <<: *TypeStringBool | ||
| description: "Whether the operator during reconcile procedure should wait for all replicas to catch-up" | ||
| new: | ||
| !!merge <<: *TypeStringBool | ||
| description: "Whether the operator during reconcile procedure should wait for new replicas to catch-up" | ||
| delay: | ||
| type: integer | ||
| description: "replication max absolute delay to consider replica is not delayed" | ||
| annotation: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Anchor copy uses !!merge – may break CRD generation
The replica-wait properties are added via
all:
!!merge <<: *TypeStringBoolThe explicit !!merge tag is non-standard and some YAML parsers (including controller-gen) fail on it.
The canonical syntax is just the merge key:
all:
<<: *TypeStringBoolSearch & replace the same pattern for new: and any other occurrences to preserve schema portability.
🤖 Prompt for AI Agents
In
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseoperatorconfigurations.clickhouse.altinity.com.yaml
around lines 369 to 381, replace all occurrences of the explicit !!merge tag
used with the merge key syntax. Specifically, change lines like "all: !!merge
<<: *TypeStringBool" to "all: <<: *TypeStringBool" and do the same for "new:"
and any other similar uses. This will ensure compatibility with YAML parsers
like controller-gen and maintain schema portability.
edd5e9e to
29a01d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (7)
packages/apps/clickhouse/templates/workloadmonitor.yaml (1)
14-15: Missing---separator still breaks multi-document YAMLThe second
WorkloadMonitoris emitted immediately after the first one without a document separator, so Helm renders invalid YAML (confirmed by YAML-lint error).
Add---before the conditional block.version: {{ $.Chart.Version }} +--- {{- if .Values.clickhouseKeeper.enabled }}packages/apps/clickhouse/templates/chkeeper.yaml (1)
59-63: Exposemetricsport soVMPodScrapecan reach it
VMPodScrape(Line 88) targetsport: metrics, but the container defines no such named port. Scraping will fail.- name: clickhouse-keeper imagePullPolicy: IfNotPresent image: clickhouse/clickhouse-keeper:24.9.2.42 + ports: + - name: metrics + containerPort: 7000 resources: {{- include "cozy-lib.resources.defaultingSanitize" (list .Values.clickhouseKeeper.resourcesPreset .Values.resources $) | nindent 20 }}packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallationtemplates.clickhouse.altinity.com.yaml (2)
318-324: Still no documented precedence betweenspec.suspendandspec.stop
Previous review (see past comments) already asked to clarify which flag wins when both are set. The newsuspenddescription is identical and does not answer the question.
800-807: Mutual-exclusivity betweenpdbMaxUnavailableandminAvailableremains unenforced
The schema still allows users to set both fields simultaneously, producing an invalid PDB. Please add a CEL validation such as:x-kubernetes-validations: - rule: "!has(self.minAvailable) || !has(self.pdbMaxUnavailable)" message: "Set either minAvailable or pdbMaxUnavailable, not both"packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhousekeeperinstallations.clickhouse-keeper.altinity.com.yaml (3)
24-95: Still exceeds 10additionalPrinterColumns‒ CRD will be rejected on K8s ≥ 1.26
This section still defines 15+ columns. The previous review already flagged the hard limit (≤ 10). Apply the recommended trimming or consolidation; otherwise,kubectl applywill fail.
29-30: Hyphenated keys must use bracket-notation in JSONPath
.status.chop-version,.status.chop-commit,.status.chop-date, etc. are still referenced with dot-notation. For keys containing-, JSONPath requires bracket syntax:jsonPath: .status["chop-version"]Update every affected path to avoid CRD validation errors.
Also applies to: 124-135
317-403:!!mergetag is non-standard YAML – breaks CRD parsing
The explicit!!mergetag before<<: *anchor(e.g., suspend, pvc, configMap, service, …) remains. Remove the tag and keep only the merge key:suspend: <<: *TypeStringBoolDo this consistently for every occurrence.
🧹 Nitpick comments (11)
packages/apps/clickhouse/templates/workloadmonitor.yaml (1)
20-21: Hard-coding replicas limits flexibility
replicas: 3&minReplicas: 3are fixed. Consider wiring these throughValues.clickhouseKeeper.replicasso users can run smaller dev clusters or larger quorums.packages/apps/clickhouse/values.yaml (1)
60-60: Strip trailing spaces to keepvalues.yamllint-cleanLine 60 ends with two spaces → fails
yamllint ‑-strict.-## @param clickhouseKeeper.enabled Deploy ClickHouse Keeper for cluster coordination·· +## @param clickhouseKeeper.enabled Deploy ClickHouse Keeper for cluster coordinationpackages/apps/clickhouse/templates/chkeeper.yaml (1)
18-18:replicasCountshould be configurableHard-coding to
3disallows single-node dev setups or larger quorums. AddclickhouseKeeper.replicasinvalues.yamland reference it here:- replicasCount: 3 + replicasCount: {{ .Values.clickhouseKeeper.replicas | default 3 }}packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/files/ClickHouseKeeper_dashboard.json (2)
355-377: Negative sign in IN-traffic query makes the metric hard to reason about
"-irate(ClickHouseAsyncMetrics_KeeperPacketsReceived…)"flips the sign for inbound packets.
While this yields a single bipolar series, it breaks Grafana’s auto-scale and alerting expectations (negative packet/sec values). Prefer two positive series and handle direction in legend/labelling, e.g.- "expr": "-irate(ClickHouseAsyncMetrics_KeeperPacketsReceived{…}[1m])", - "legendFormat": "IN {{namespace}}.{{pod_name}}", + "expr": "irate(ClickHouseAsyncMetrics_KeeperPacketsReceived{…}[1m])", + "legendFormat": "in {{namespace}}.{{pod_name}}",…or use separate axes/stacking if visual separation is required.
This keeps semantics intact and avoids confusing consumers of the metric.
150-158:irate()over 1 minute is noisy for low-QPS clustersMany Keeper installations emit packets sporadically;
irate()[1m]often renders spikes. Using
rate()[5m](or a variable window) smooths the graph while still alerting quickly.No change needed if you rely on latency, but consider widening the range for better signal/noise.
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/README.md (3)
34-37: Docs still talk about “secret with dashboards” after the move to ConfigMapsThe table rows for
dashboards.additionalLabels/dashboards.annotationsmention “a secret” but the implementation now provisions ConfigMaps.
Please s/secret/ConfigMap/ to avoid confusion for users upgrading from 0.23.x.
68-74: Minor typo – “Ypu” → “You”
Ypuat Line 72 breaks readability.- Ypu can specify interval, scrapeTimeout, ... + You can specify interval, scrapeTimeout, ...
60-62: Boolean description forrbac.namespaceScopedis ambiguousThe phrase “cluster level or namespace level” does not say which value of the boolean maps to which scope.
Recommend clarifying, e.g.
rbac.namespaceScoped: true # create Role/RoleBinding in the release namespace instead of ClusterRole/ClusterRoleBindingpackages/system/clickhouse-operator/charts/altinity-clickhouse-operator/values.yaml (1)
863-866: Comment still references Grafana sidecar link with bare URLMarkdown-lint flags the bare URL (MD034).
Wrap it in angle brackets to satisfy common linters:(can be synced by Grafana dashboards sidecar <https://github.com/grafana/helm-charts/blob/...>)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallationtemplates.clickhouse.altinity.com.yaml (1)
85-90: Printer column forspec.suspendis typed asstring– considerboolean
Since the field semantically represents a boolean, switching the column type tobooleanwould improvekubectl getreadability.packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallations.clickhouse.altinity.com.yaml (1)
85-90:suspendprinter column may breakkubectl getoutput if the field is boolean-encoded
spec.suspendis modelled via the*TypeStringBoolenum and therefore admits values such as"0","1","true","false"… – none of which are valid JSON/YAML booleans. With the column declared astype: string jsonPath: .spec.suspend
kubectl get chi -o widewill show the raw string, not a clean yes/no state, and unfamiliar values (e.g. an empty string) will be hard to read. Consider restricting the enum to just the canonical"true" | "false"pair or converting it to a real boolean so that the printer column renders consistently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (17)
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ClusterRole-clickhouse-operator-kube-system.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ClusterRoleBinding-clickhouse-operator-kube-system.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-clickhouse-operator-confd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-clickhouse-operator-configd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-clickhouse-operator-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-clickhouse-operator-templatesd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-clickhouse-operator-usersd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-keeper-operator-confd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-keeper-operator-configd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-keeper-operator-templatesd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-keeper-operator-usersd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/Deployment-clickhouse-operator.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/Role-clickhouse-operator.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/RoleBinding-clickhouse-operator.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/Secret-clickhouse-operator.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/Service-clickhouse-operator-metrics.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ServiceAccount-clickhouse-operator.yamlis excluded by!**/generated/**
📒 Files selected for processing (24)
packages/apps/clickhouse/Chart.yaml(1 hunks)packages/apps/clickhouse/Makefile(1 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/workloadmonitor.yaml(1 hunks)packages/apps/clickhouse/values.schema.json(1 hunks)packages/apps/clickhouse/values.yaml(1 hunks)packages/apps/versions_map(1 hunks)packages/core/installer/values.yaml(1 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/Chart.yaml(2 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/README.md(2 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/README.md.gotmpl(1 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallations.clickhouse.altinity.com.yaml(20 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallationtemplates.clickhouse.altinity.com.yaml(20 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhousekeeperinstallations.clickhouse-keeper.altinity.com.yaml(7 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseoperatorconfigurations.clickhouse.altinity.com.yaml(5 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/files/ClickHouseKeeper_dashboard.json(4 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/files/ClickHouse_Queries_dashboard.json(2 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/_helpers.tpl(3 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/dashboards-configmap.yaml(1 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/dashboards-secret.yaml(0 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/servicemonitor.yaml(1 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/values.yaml(14 hunks)
💤 Files with no reviewable changes (1)
- packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/dashboards-secret.yaml
✅ Files skipped from review due to trivial changes (2)
- packages/core/installer/values.yaml
- packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/README.md.gotmpl
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/apps/clickhouse/Chart.yaml
- packages/apps/clickhouse/Makefile
- packages/apps/versions_map
- packages/apps/clickhouse/templates/clickhouse.yaml
- packages/apps/clickhouse/README.md
- packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/_helpers.tpl
- packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/Chart.yaml
- packages/apps/clickhouse/values.schema.json
- packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/files/ClickHouse_Queries_dashboard.json
- packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseoperatorconfigurations.clickhouse.altinity.com.yaml
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: in cozystack, the plural form for the kubernetes custom resource is `kuberneteses.apps.cozystack.io`...
Learnt from: lllamnyp
PR: cozystack/cozystack#1130
File: hack/e2e-apps/kubernetes.bats:101-101
Timestamp: 2025-07-11T06:11:25.438Z
Learning: In cozystack, the plural form for the Kubernetes custom resource is `kuberneteses.apps.cozystack.io`, not `kubernetes.apps.cozystack.io`. This is defined in the API schema even though it's not grammatically perfect.
Applied to files:
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallationtemplates.clickhouse.altinity.com.yamlpackages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallations.clickhouse.altinity.com.yaml
📚 Learning: in helm templates, the `{{-` directive chomps all leading whitespace including newlines back to the ...
Learnt from: lllamnyp
PR: cozystack/cozystack#1160
File: packages/system/hetzner-robotlb/charts/robotlb/templates/role.yaml:6-8
Timestamp: 2025-07-11T06:28:13.696Z
Learning: In Helm templates, the `{{-` directive chomps all leading whitespace including newlines back to the previous content, so `{{- toYaml .Values.something | nindent 2 }}` will render correctly even with apparent indentation issues. However, for better style, it's cleaner to put the template directive on the same line as the parent key (e.g., `rules: {{- toYaml .Values.serviceAccount.permissions | nindent 2 }}`).
Applied to files:
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhousekeeperinstallations.clickhouse-keeper.altinity.com.yamlpackages/apps/clickhouse/templates/workloadmonitor.yaml
📚 Learning: the cozyvalues-gen tool has a known issue (https://github.com/cozystack/cozyvalues-gen/issues/10) wh...
Learnt from: NickVolynkin
PR: cozystack/cozystack#1216
File: packages/extra/monitoring/README.md:30-31
Timestamp: 2025-07-26T18:26:01.447Z
Learning: The cozyvalues-gen tool has a known issue (https://github.com/cozystack/cozyvalues-gen/issues/10) where it incorrectly places Grafana configuration entries under the wrong documentation sections, specifically placing grafana.resources.requests.* entries in the Metrics storage configuration table instead of the Grafana configuration table.
Applied to files:
packages/apps/clickhouse/templates/chkeeper.yamlpackages/apps/clickhouse/values.yaml
📚 Learning: in the cozystack repository, for the virtual-machine app's resources.sockets parameter, the value is...
Learnt from: NickVolynkin
PR: cozystack/cozystack#1216
File: packages/apps/virtual-machine/values.yaml:0-0
Timestamp: 2025-07-26T18:12:05.641Z
Learning: In the cozystack repository, for the virtual-machine app's resources.sockets parameter, the value is intentionally kept as a string in values.yaml despite being documented as {int} type, because the string-to-integer parsing happens elsewhere in the system pipeline rather than at the YAML validation level.
Applied to files:
packages/apps/clickhouse/templates/chkeeper.yaml
📚 Learning: the `./charts/robotlb/` directory in the hetzner-robotlb package contains vendored code, and the tea...
Learnt from: lllamnyp
PR: cozystack/cozystack#1233
File: packages/system/hetzner-robotlb/charts/robotlb/templates/deployment.yaml:33-35
Timestamp: 2025-07-23T09:15:09.658Z
Learning: The `./charts/robotlb/` directory in the hetzner-robotlb package contains vendored code, and the team generally avoids modifying vendored code to maintain clean separation from upstream dependencies.
Applied to files:
packages/apps/clickhouse/templates/chkeeper.yaml
🧬 Code Graph Analysis (1)
packages/apps/clickhouse/templates/workloadmonitor.yaml (1)
api/v1alpha1/workloadmonitor_types.go (1)
Selector(8-35)
🪛 YAMLlint (1.37.1)
packages/apps/clickhouse/templates/chkeeper.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/apps/clickhouse/templates/workloadmonitor.yaml
[error] 15-15: syntax error: could not find expected ':'
(syntax)
packages/apps/clickhouse/values.yaml
[error] 60-60: trailing spaces
(trailing-spaces)
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/dashboards-configmap.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
[warning] 14-14: wrong indentation: expected 2 but found 4
(indentation)
[warning] 15-15: wrong indentation: expected 2 but found 4
(indentation)
[warning] 16-16: wrong indentation: expected 2 but found 4
(indentation)
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/servicemonitor.yaml
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
🪛 LanguageTool
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/README.md
[grammar] ~72-~72: Ensure spelling is correct
Context: ... port 8888 and operator-metrics # 9999. Ypu can specify interval, scrapeTimeout, re...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.17.2)
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/README.md
36-36: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (6)
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/servicemonitor.yaml (1)
10-11: Indentation of additional labels section
toYamlis indented 4 spaces but lives two levels deep (should be 6). Helm usually copes, but Prometheus-operator’s strict CRD validation can reject badly indented metadata blocks. Consider:- {{- toYaml .Values.serviceMonitor.additionalLabels | nindent 4 }} + {{- toYaml .Values.serviceMonitor.additionalLabels | nindent 6 }}packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/README.md (1)
11-15: Pin CRD URLs to a released tag instead ofmasterFetching CRDs from the moving master branch can introduce breaking changes that the chart version is not aware of.
Point the URLs to the same tag (v0.25.2) shipped by the chart, e.g.-https://github.com/Altinity/clickhouse-operator/raw/master/deploy/helm/... +https://github.com/Altinity/clickhouse-operator/raw/v0.25.2/deploy/helm/...packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/values.yaml (1)
101-118: Quote30sliterals to prevent accidental numeric castingSome YAML parsers treat bare words ending with
sas integers (e.g.,30s→30).
Safer to quote time strings:-interval: 30s +interval: "30s"Do the same for
scrapeTimeout.packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallationtemplates.clickhouse.altinity.com.yaml (1)
7-14: Operator version & label bump looks good
No issues – the version constant and label are updated consistently.packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallations.clickhouse.altinity.com.yaml (2)
556-562:serviceTemplates(plural) overlaps with legacyserviceTemplate– clarify precedenceBoth a singular string and a plural array are now allowed. If a user sets both, which one wins? Ambiguity will lead to non-deterministic rendering of Services.
Doc suggestion: deprecate the singular key in 0.26, emit a warning when both are present, and honour
serviceTemplatesonly.
609-611:availabilityZoneunder ZooKeeper nodes is undocumented in operator codebaseThe operator currently ignores this field; adding it to the CRD without corresponding controller logic creates a false expectation for users.
Confirm the feature is implemented, otherwise drop the property until support lands.
| runtime: | ||
| type: object | ||
| description: "runtime parameters for clickhouse-operator process which are used during reconcile cycle" | ||
| properties: | ||
| reconcileShardsThreadsNumber: | ||
| type: integer | ||
| minimum: 1 | ||
| maximum: 65535 | ||
| description: "How many goroutines will be used to reconcile shards of a cluster in parallel, 1 by default" | ||
| reconcileShardsMaxConcurrencyPercent: | ||
| type: integer | ||
| minimum: 0 | ||
| maximum: 100 | ||
| description: "The maximum percentage of cluster shards that may be reconciled in parallel, 50 percent by default." |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicated reconcile-tuning knobs – risk of drift between operator flags and CRD
spec.reconciling.runtime.* introduces parallelism settings that already exist in the operator’s command-line flags / ConfigMap. Having two independent sources of truth (cluster-wide vs CHI-local) complicates support and upgrades.
If CHI-level overrides are truly required, surface them behind a single object (e.g. runtimeOverrides) and document precedence; otherwise remove the fields here and reference the operator-level setting.
🤖 Prompt for AI Agents
In
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallations.clickhouse.altinity.com.yaml
between lines 436 and 449, the reconcileShardsThreadsNumber and
reconcileShardsMaxConcurrencyPercent fields duplicate existing operator flags,
causing potential configuration drift. To fix this, either remove these fields
from the CRD to rely solely on operator-level settings or consolidate them under
a single runtimeOverrides object with clear documentation on precedence between
cluster-wide and CHI-local settings.
...-operator/crds/CustomResourceDefinition-clickhouseinstallations.clickhouse.altinity.com.yaml
Show resolved
Hide resolved
| pdbMaxUnavailable: | ||
| type: integer | ||
| description: | | ||
| Pod eviction is allowed if at most "pdbMaxUnavailable" pods are unavailable after the eviction, | ||
| i.e. even in absence of the evicted pod. For example, one can prevent all voluntary evictions | ||
| by specifying 0. This is a mutually exclusive setting with "minAvailable". | ||
| minimum: 0 | ||
| maximum: 65535 | ||
| reconcile: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
pdbMaxUnavailable should use IntOrString to match Kubernetes PDB semantics
Kubernetes PodDisruptionBudget accepts either an absolute number or a percentage. Modelling the field as plain integer removes the percentage option and makes CHI manifests less flexible.
- type: integer
+ x-kubernetes-int-or-string: truewill retain compatibility with both forms (1 or "25%").
📝 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.
| pdbMaxUnavailable: | |
| type: integer | |
| description: | | |
| Pod eviction is allowed if at most "pdbMaxUnavailable" pods are unavailable after the eviction, | |
| i.e. even in absence of the evicted pod. For example, one can prevent all voluntary evictions | |
| by specifying 0. This is a mutually exclusive setting with "minAvailable". | |
| minimum: 0 | |
| maximum: 65535 | |
| reconcile: | |
| pdbMaxUnavailable: | |
| - type: integer | |
| + x-kubernetes-int-or-string: true | |
| description: | | |
| Pod eviction is allowed if at most "pdbMaxUnavailable" pods are unavailable after the eviction, | |
| i.e. even in absence of the evicted pod. For example, one can prevent all voluntary evictions | |
| by specifying 0. This is a mutually exclusive setting with "minAvailable". | |
| minimum: 0 | |
| maximum: 65535 | |
| reconcile: |
🤖 Prompt for AI Agents
In
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallations.clickhouse.altinity.com.yaml
around lines 799 to 807, the pdbMaxUnavailable field is defined as an integer,
which prevents specifying a percentage value as allowed by Kubernetes
PodDisruptionBudget semantics. Change the type of pdbMaxUnavailable from integer
to IntOrString to support both absolute numbers and percentage strings, ensuring
compatibility and flexibility in CHI manifests.
| type: object | ||
| description: "runtime parameters for clickhouse-operator process which are used during reconcile cycle" | ||
| properties: | ||
| reconcileShardsThreadsNumber: | ||
| type: integer | ||
| minimum: 1 | ||
| maximum: 65535 | ||
| description: "How many goroutines will be used to reconcile shards of a cluster in parallel, 1 by default" | ||
| reconcileShardsMaxConcurrencyPercent: | ||
| type: integer | ||
| minimum: 0 | ||
| maximum: 100 | ||
| description: "The maximum percentage of cluster shards that may be reconciled in parallel, 50 percent by default." | ||
| macros: |
There was a problem hiding this comment.
reconcileShardsMaxConcurrencyPercent accepts 0, producing a no-op
A value of 0 would disable reconciliation entirely. Consider restricting the range to 1-100.
- minimum: 0
+ minimum: 1Replicate the same bound in the cluster-scoped reconcile.runtime block below.
📝 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.
| type: object | |
| description: "runtime parameters for clickhouse-operator process which are used during reconcile cycle" | |
| properties: | |
| reconcileShardsThreadsNumber: | |
| type: integer | |
| minimum: 1 | |
| maximum: 65535 | |
| description: "How many goroutines will be used to reconcile shards of a cluster in parallel, 1 by default" | |
| reconcileShardsMaxConcurrencyPercent: | |
| type: integer | |
| minimum: 0 | |
| maximum: 100 | |
| description: "The maximum percentage of cluster shards that may be reconciled in parallel, 50 percent by default." | |
| macros: | |
| type: object | |
| description: "runtime parameters for clickhouse-operator process which are used during reconcile cycle" | |
| properties: | |
| reconcileShardsThreadsNumber: | |
| type: integer | |
| minimum: 1 | |
| maximum: 65535 | |
| description: "How many goroutines will be used to reconcile shards of a cluster in parallel, 1 by default" | |
| reconcileShardsMaxConcurrencyPercent: | |
| type: integer | |
| minimum: 1 | |
| maximum: 100 | |
| description: "The maximum percentage of cluster shards that may be reconciled in parallel, 50 percent by default." | |
| macros: |
🤖 Prompt for AI Agents
In
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallationtemplates.clickhouse.altinity.com.yaml
around lines 437 to 450, the reconcileShardsMaxConcurrencyPercent property
currently allows a minimum value of 0, which disables reconciliation and causes
a no-op. Change the minimum value from 0 to 1 to ensure at least some
concurrency. Also, locate the cluster-scoped reconcile.runtime block in the same
file and apply the same minimum value restriction of 1 to 100 for
reconcileShardsMaxConcurrencyPercent there as well.
| description: "optional, template name from chi.spec.templates.serviceTemplates. used for customization of the `Service` resource, created by `clickhouse-operator` to cover all clusters in whole `chi` resource" | ||
| serviceTemplates: | ||
| type: array | ||
| description: "optional, template names from chi.spec.templates.serviceTemplates. used for customization of the `Service` resources, created by `clickhouse-operator` to cover all clusters in whole `chi` resource" | ||
| nullable: true | ||
| items: | ||
| type: string |
There was a problem hiding this comment.
serviceTemplate vs. new serviceTemplates array – no exclusivity rule
Having both a scalar and an array for the same logical knob is error-prone. Add a CEL rule to forbid setting both simultaneously, or mark the scalar as deprecated and document migration guidance.
🤖 Prompt for AI Agents
In
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallationtemplates.clickhouse.altinity.com.yaml
around lines 555 to 561, there are both a scalar `serviceTemplate` and an array
`serviceTemplates` for the same configuration purpose, which can cause
conflicts. To fix this, add a CEL validation rule that forbids setting both
`serviceTemplate` and `serviceTemplates` simultaneously, or alternatively mark
the scalar `serviceTemplate` as deprecated and provide documentation on
migrating to `serviceTemplates`.
| type: object | ||
| description: "allow tuning reconciling process" | ||
| properties: | ||
| runtime: | ||
| type: object | ||
| description: "runtime parameters for clickhouse-operator process which are used during reconcile cycle" | ||
| properties: | ||
| reconcileShardsThreadsNumber: | ||
| type: integer | ||
| minimum: 1 | ||
| maximum: 65535 | ||
| description: "How many goroutines will be used to reconcile shards of a cluster in parallel, 1 by default" | ||
| reconcileShardsMaxConcurrencyPercent: | ||
| type: integer | ||
| minimum: 0 | ||
| maximum: 100 | ||
| description: "The maximum percentage of cluster shards that may be reconciled in parallel, 50 percent by default." |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Cluster-level reconcile.runtime block repeats the same lower-bound issue
Please apply the same minimum: 1 guard to both reconcileShardsThreadsNumber and reconcileShardsMaxConcurrencyPercent here as well.
🤖 Prompt for AI Agents
In
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallationtemplates.clickhouse.altinity.com.yaml
around lines 808 to 824, the reconcile.runtime properties define minimum values
inconsistently. Update the minimum value for
reconcileShardsMaxConcurrencyPercent from 0 to 1 to match the minimum guard
applied to reconcileShardsThreadsNumber, ensuring both have a minimum of 1.
| how much shards for current ClickHouse cluster will run in Kubernetes, | ||
| each shard contains shared-nothing part of data and contains set of replicas, | ||
| cluster contains 1 shard by default" | ||
| replicasCount: | ||
| type: integer | ||
| description: "how much replicas in each shards for current ClickHouse cluster will run in Kubernetes, each replica is a separate `StatefulSet` which contains only one `Pod` with `clickhouse-server` instance, every shard contains 1 replica by default" | ||
| description: | | ||
| how much replicas in each shards for current cluster will run in Kubernetes, | ||
| each replica is a separate `StatefulSet` which contains only one `Pod` with `clickhouse-server` instance, | ||
| every shard contains 1 replica by default" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
shardsCount / replicasCount lack minimum: 1 validation
The description says defaults are 1, but the numeric schema omits a lower bound. Add minimum: 1 to prevent invalid zero-shard or zero-replica clusters.
shardsCount:
type: integer
+ minimum: 1Do the same for replicasCount.
🤖 Prompt for AI Agents
In
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallationtemplates.clickhouse.altinity.com.yaml
around lines 835 to 843, the numeric schema for shardsCount and replicasCount
lacks a minimum value constraint. Add minimum: 1 to both shardsCount and
replicasCount definitions to enforce that these values cannot be zero or less,
aligning with the description that defaults are 1 and preventing invalid cluster
configurations.
| {{- range $path, $_ := .Files.Glob "files/*.json" }} | ||
| {{ $path | trimPrefix "files/" }}: |- {{ $.Files.Get $path | nindent 4 -}} | ||
| {{ end }} |
There was a problem hiding this comment.
YAML literal block rendered with a leading space
filename.json: |- {{ … }} leaves one literal space between |- and the newline produced by nindent, which breaks the block scalar header (|- must be followed immediately by EOL).
Safe pattern:
- {{ $path | trimPrefix "files/" }}: |- {{ $.Files.Get $path | nindent 4 -}}
+ {{ $path | trimPrefix "files/" }}: |-
+{{ $.Files.Get $path | nindent 4 }}📝 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.
| {{- range $path, $_ := .Files.Glob "files/*.json" }} | |
| {{ $path | trimPrefix "files/" }}: |- {{ $.Files.Get $path | nindent 4 -}} | |
| {{ end }} | |
| {{- range $path, $_ := .Files.Glob "files/*.json" }} | |
| {{ $path | trimPrefix "files/" }}: |- | |
| {{ $.Files.Get $path | nindent 4 }} | |
| {{ end }} |
🤖 Prompt for AI Agents
In
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/dashboards-configmap.yaml
lines 18 to 20, the YAML literal block scalar header `|-` is followed by a space
before the newline, which is invalid. Remove the space between `|-` and the
newline by placing `|-` immediately before the newline and then applying
`nindent` to the content, ensuring the block scalar header is correctly
formatted without trailing spaces.
| queries: true | ||
| include: false | ||
| replicas: | ||
| all: no | ||
| new: yes | ||
| delay: 10 | ||
| ################################################ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use proper booleans for replicas.all / replicas.new
YAML treats the strings "yes"/"no" as literals, not booleans.
The Helm templates will need to cast them manually (eq .Values "yes"). Switching to real booleans avoids surprises:
- all: no
- new: yes
+ all: false
+ new: true📝 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.
| queries: true | |
| include: false | |
| replicas: | |
| all: no | |
| new: yes | |
| delay: 10 | |
| ################################################ | |
| queries: true | |
| include: false | |
| replicas: | |
| all: false | |
| new: true | |
| delay: 10 | |
| ################################################ |
🤖 Prompt for AI Agents
In
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/values.yaml
around lines 540 to 546, the values for replicas.all and replicas.new are set as
strings "no" and "yes" instead of proper booleans. Change these string values to
actual booleans false and true respectively to avoid YAML parsing issues and the
need for manual casting in Helm templates.
29a01d8 to
f8f99f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (14)
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/dashboards-configmap.yaml (1)
18-20: Block scalar header malformed – extra space after|-breaks YAML
|-must be immediately followed by a line-break. The trailing space renders an illegal header and causes the data chunk to be interpreted as plain scalars, defeating JSON loading in Grafana.- {{ $path | trimPrefix "files/" }}: |- {{ $.Files.Get $path | nindent 4 -}} + {{ $path | trimPrefix "files/" }}: |- +{{ $.Files.Get $path | nindent 4 -}}Putting
|-on its own line also avoids double-indent artefacts flagged by yamllint.packages/apps/clickhouse/templates/workloadmonitor.yaml (1)
14-27: Missing---document separator – YAML parses as one resource, causing syntax errorThe conditional WorkloadMonitor starts without a new document header; Kubernetes will treat lines 14-27 as extra fields on the previous object, tripping the API server (
could not find expected ':').version: {{ $.Chart.Version }} -{{- if .Values.clickhouseKeeper.enabled }} +--- +{{- if .Values.clickhouseKeeper.enabled }}Add the separator before the
ifand keep it outside the template so the file remains valid when Keeper is disabled.packages/apps/clickhouse/templates/chkeeper.yaml (2)
58-62: Container lacks namedmetricsport required byVMPodScrape
VMPodScrape(lines 88-90) targetsport: metrics; without the named port the scrape will fail.- name: clickhouse-keeper imagePullPolicy: IfNotPresent image: clickhouse/clickhouse-keeper:24.9.2.42 + ports: + - name: metrics + containerPort: 7000
18-19:replicasCounthard-coded – make this configurableDevelopment clusters may want 1 replica, prod may need 5. Surface
clickhouseKeeper.replicasinvalues.yamland consume it here.- replicasCount: 3 + replicasCount: {{ .Values.clickhouseKeeper.replicas | default 3 }}Remember to update
values.schema.json.packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseoperatorconfigurations.clickhouse.altinity.com.yaml (1)
372-377: Remove explicit!!mergetags – they break controller-gen & other YAML parsersThe newly-added
replicas.*,label.appendScope, andstatus.fields.*attributes repeat the same pattern previously flagged (!!merge <<: *TypeStringBool).
The YAML 1.1 tag!!mergeis non-standard; the canonical merge-key syntax is just the bare<<:anchor. Tooling such ascontroller-genfails to process CRDs that keep the explicit tag, which prevents code-generated clients and OpenAPI schemas from being produced.Please replace every occurrence with the plain merge key, e.g.
all: - !!merge <<: *TypeStringBool + <<: *TypeStringBool…and do the same for
new:,appendScope:,action:,errors:etc.Also applies to: 418-419, 454-464
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallationtemplates.clickhouse.altinity.com.yaml (6)
318-325: Still no precedence doc betweensuspendand existingstopflagThe description for
spec.suspendhas been added, but it never clarifies how it interacts with the olderspec.stop. Users remain unsure which flag “wins” when both are set.
441-449:reconcileShardsMaxConcurrencyPercentcan be set to 0 – disables reconciliationAllowing a minimum of
0reproduces the previously-reported no-op case. Constrain the field to1-100to guarantee forward progress.
815-824: Cluster-scoped runtime block repeats the same lower-bound issueThe nested
clusters[].reconcile.runtime.reconcileShardsMaxConcurrencyPercentalso keepsminimum: 0. Apply the sameminimum: 1guard here for consistency.
800-807:pdbMaxUnavailablestill lacks mutual-exclusion validation withminAvailableThe schema permits users to specify both fields simultaneously, yielding an invalid PodDisruptionBudget that Kubernetes will happily accept. Add a CEL rule (or webhook) to forbid the illegal combination.
556-561: ScalarserviceTemplatevs. arrayserviceTemplatesremains unconstrainedWithout an exclusivity rule, users can set both and create ambiguous behaviour. Either deprecate the scalar or enforce mutual exclusivity in the schema.
833-843:shardsCount/replicasCountstill missingminimum: 1guardZero-shard or zero-replica clusters are nonsensical yet remain permitted. Add
minimum: 1to both fields to align with the documented defaults.packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhousekeeperinstallations.clickhouse-keeper.altinity.com.yaml (3)
24-90: Too manyadditionalPrinterColumns– CRD will be rejected on K8s ≥ 1.26The list declares > 10 columns (currently ~20). From Kubernetes 1.26 the API server enforces an upper bound of 10, so
kubectl applywill fail and the whole CRD (and therefore the Helm release) will be refused.Trim the set to the truly essential columns or consolidate related counts into structured status fields.
25-30: JSONPath must use bracket-notation for keys containing hyphens
.status.chop-versionis parsed as “.status.chopminusversion”, causing validation errors.
Use bracket syntax:- jsonPath: .status.chop-version + jsonPath: .status["chop-version"]Apply the same fix to every
jsonPaththat targets a*-key.
317-384:!!mergeis not a supported YAML tag in CRDsThe explicit
!!mergetag breaks parsing by Kubernetes’ YAML loader. Replace every instance with the standard merge-key syntax:- suspend: - !!merge <<: *TypeStringBool + suspend: + <<: *TypeStringBoolThe same correction is required for
pvc,configMap,service, and other occurrences underunknownObjectsandreconcileFailedObjects.
🧹 Nitpick comments (5)
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/dashboards-configmap.yaml (1)
8-16: Indentation over-shoot – directive already indents the block
nindent 4already emits four spaces; prefixing it with four literal spaces pushes the rendered keys to 8-space depth, which violates your own linting rules. Prefer the cleaner one-liner pattern (see team learning).- labels: - {{- include "altinity-clickhouse-operator.labels" . | nindent 4 }} + labels: {{- include "altinity-clickhouse-operator.labels" . | nindent 2 }}Same applies to
annotations:.packages/apps/clickhouse/values.yaml (1)
60-63: Trailing whitespace & double-space before EOLLine 60 ends with two spaces. Trim to keep
values.yamllint-clean.-## @param clickhouseKeeper.enabled Deploy ClickHouse Keeper for cluster coordination·· +## @param clickhouseKeeper.enabled Deploy ClickHouse Keeper for cluster coordinationWhile here, consider adding a
replicasandimagesub-object as requested in earlier reviews so users can tune quorum size and image tag.packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/servicemonitor.yaml (1)
9-11: Over-indent onadditionalLabelsblockBecause the parent key is at 4 spaces,
nindent 4plus the literal 4 spaces results in 8-space indent, tripping yamllint. Align with parent key or switch to inline form:- {{- if .Values.serviceMonitor.additionalLabels }} - {{- toYaml .Values.serviceMonitor.additionalLabels | nindent 4 }} + {{- if .Values.serviceMonitor.additionalLabels }} +{{ toYaml .Values.serviceMonitor.additionalLabels | nindent 4 }}Minor, but keeps all chart YAML passing strict linters.
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/README.md (2)
36-37: Format bare URL as a Markdown linkThe raw URL inside the description violates
markdownlintrule MD034 and renders inconsistently. Wrap it as a proper link:- provision grafana dashboards as configMaps (can be synced by grafana dashboards sidecar https://github.com/grafana/helm-charts/blob/grafana-8.3.4/charts/grafana/values.yaml#L778 ) + provision Grafana dashboards as ConfigMaps (can be synced by the [Grafana dashboards sidecar](https://github.com/grafana/helm-charts/blob/grafana-8.3.4/charts/grafana/values.yaml#L778))
72-73: Fix typo in explanatory text
Ypu→You- In serviceMonitor will be created two endpoints clickhouse-metrics on port 8888 and operator-metrics # 9999. Ypu can specify interval, scrapeTimeout, relabelings, metricRelabelings for each endpoint below + The ServiceMonitor creates two endpoints: clickhouse-metrics on port 8888 and operator-metrics on port 9999. You can specify `interval`, `scrapeTimeout`, `relabelings`, and `metricRelabelings` for each endpoint below.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (17)
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ClusterRole-clickhouse-operator-kube-system.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ClusterRoleBinding-clickhouse-operator-kube-system.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-clickhouse-operator-confd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-clickhouse-operator-configd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-clickhouse-operator-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-clickhouse-operator-templatesd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-clickhouse-operator-usersd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-keeper-operator-confd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-keeper-operator-configd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-keeper-operator-templatesd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-keeper-operator-usersd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/Deployment-clickhouse-operator.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/Role-clickhouse-operator.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/RoleBinding-clickhouse-operator.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/Secret-clickhouse-operator.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/Service-clickhouse-operator-metrics.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ServiceAccount-clickhouse-operator.yamlis excluded by!**/generated/**
📒 Files selected for processing (25)
hack/e2e-apps/clickhouse.bats(1 hunks)packages/apps/clickhouse/Chart.yaml(1 hunks)packages/apps/clickhouse/Makefile(1 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/workloadmonitor.yaml(1 hunks)packages/apps/clickhouse/values.schema.json(1 hunks)packages/apps/clickhouse/values.yaml(1 hunks)packages/apps/versions_map(1 hunks)packages/core/installer/values.yaml(1 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/Chart.yaml(2 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/README.md(2 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/README.md.gotmpl(1 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallations.clickhouse.altinity.com.yaml(20 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallationtemplates.clickhouse.altinity.com.yaml(20 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhousekeeperinstallations.clickhouse-keeper.altinity.com.yaml(7 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseoperatorconfigurations.clickhouse.altinity.com.yaml(5 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/files/ClickHouseKeeper_dashboard.json(4 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/files/ClickHouse_Queries_dashboard.json(2 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/_helpers.tpl(3 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/dashboards-configmap.yaml(1 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/dashboards-secret.yaml(0 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/servicemonitor.yaml(1 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/values.yaml(14 hunks)
💤 Files with no reviewable changes (1)
- packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/dashboards-secret.yaml
✅ Files skipped from review due to trivial changes (3)
- packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/README.md.gotmpl
- packages/core/installer/values.yaml
- hack/e2e-apps/clickhouse.bats
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/apps/clickhouse/Chart.yaml
- packages/apps/clickhouse/templates/clickhouse.yaml
- packages/apps/clickhouse/README.md
- packages/apps/clickhouse/Makefile
- packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/Chart.yaml
- packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/files/ClickHouse_Queries_dashboard.json
- packages/apps/versions_map
- packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/files/ClickHouseKeeper_dashboard.json
- packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/_helpers.tpl
- packages/apps/clickhouse/values.schema.json
- packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/values.yaml
- packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallations.clickhouse.altinity.com.yaml
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: in helm templates, the `{{-` directive chomps all leading whitespace including newlines back to the ...
Learnt from: lllamnyp
PR: cozystack/cozystack#1160
File: packages/system/hetzner-robotlb/charts/robotlb/templates/role.yaml:6-8
Timestamp: 2025-07-11T06:28:13.696Z
Learning: In Helm templates, the `{{-` directive chomps all leading whitespace including newlines back to the previous content, so `{{- toYaml .Values.something | nindent 2 }}` will render correctly even with apparent indentation issues. However, for better style, it's cleaner to put the template directive on the same line as the parent key (e.g., `rules: {{- toYaml .Values.serviceAccount.permissions | nindent 2 }}`).
Applied to files:
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallationtemplates.clickhouse.altinity.com.yamlpackages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhousekeeperinstallations.clickhouse-keeper.altinity.com.yamlpackages/apps/clickhouse/templates/workloadmonitor.yamlpackages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/dashboards-configmap.yaml
📚 Learning: in cozystack's schema generator annotation format, when documenting fields of array items, use the s...
Learnt from: NickVolynkin
PR: cozystack/cozystack#1216
File: packages/apps/virtual-machine/values.yaml:31-33
Timestamp: 2025-07-26T18:01:52.557Z
Learning: In cozystack's schema generator annotation format, when documenting fields of array items, use the singular form of the item type rather than array notation. For example, for a parameter `gpus {[]gpu}`, use `@field gpu.name` rather than `@field gpus[].name` to refer to the name field of each GPU object in the array.
Applied to files:
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallationtemplates.clickhouse.altinity.com.yaml
📚 Learning: in cozystack, the plural form for the kubernetes custom resource is `kuberneteses.apps.cozystack.io`...
Learnt from: lllamnyp
PR: cozystack/cozystack#1130
File: hack/e2e-apps/kubernetes.bats:101-101
Timestamp: 2025-07-11T06:11:25.438Z
Learning: In cozystack, the plural form for the Kubernetes custom resource is `kuberneteses.apps.cozystack.io`, not `kubernetes.apps.cozystack.io`. This is defined in the API schema even though it's not grammatically perfect.
Applied to files:
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallationtemplates.clickhouse.altinity.com.yaml
📚 Learning: the cozyvalues-gen tool has a known issue (https://github.com/cozystack/cozyvalues-gen/issues/10) wh...
Learnt from: NickVolynkin
PR: cozystack/cozystack#1216
File: packages/extra/monitoring/README.md:30-31
Timestamp: 2025-07-26T18:26:01.447Z
Learning: The cozyvalues-gen tool has a known issue (https://github.com/cozystack/cozyvalues-gen/issues/10) where it incorrectly places Grafana configuration entries under the wrong documentation sections, specifically placing grafana.resources.requests.* entries in the Metrics storage configuration table instead of the Grafana configuration table.
Applied to files:
packages/apps/clickhouse/templates/chkeeper.yamlpackages/apps/clickhouse/values.yaml
📚 Learning: in the cozystack repository, for the virtual-machine app's resources.sockets parameter, the value is...
Learnt from: NickVolynkin
PR: cozystack/cozystack#1216
File: packages/apps/virtual-machine/values.yaml:0-0
Timestamp: 2025-07-26T18:12:05.641Z
Learning: In the cozystack repository, for the virtual-machine app's resources.sockets parameter, the value is intentionally kept as a string in values.yaml despite being documented as {int} type, because the string-to-integer parsing happens elsewhere in the system pipeline rather than at the YAML validation level.
Applied to files:
packages/apps/clickhouse/templates/chkeeper.yaml
📚 Learning: the `./charts/robotlb/` directory in the hetzner-robotlb package contains vendored code, and the tea...
Learnt from: lllamnyp
PR: cozystack/cozystack#1233
File: packages/system/hetzner-robotlb/charts/robotlb/templates/deployment.yaml:33-35
Timestamp: 2025-07-23T09:15:09.658Z
Learning: The `./charts/robotlb/` directory in the hetzner-robotlb package contains vendored code, and the team generally avoids modifying vendored code to maintain clean separation from upstream dependencies.
Applied to files:
packages/apps/clickhouse/templates/chkeeper.yaml
📚 Learning: in github actions yaml workflows, multi-line strings under `run:` without block scalars (| or >) are...
Learnt from: lllamnyp
PR: cozystack/cozystack#1092
File: .github/workflows/pull-requests.yaml:90-94
Timestamp: 2025-06-23T11:01:31.400Z
Learning: In GitHub Actions YAML workflows, multi-line strings under `run:` without block scalars (| or >) are automatically folded by YAML into single-line commands. This is correct behavior and doesn't require using block scalars with backslashes for line continuation.
Applied to files:
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/dashboards-configmap.yaml
🪛 YAMLlint (1.37.1)
packages/apps/clickhouse/templates/chkeeper.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/apps/clickhouse/templates/workloadmonitor.yaml
[error] 15-15: syntax error: could not find expected ':'
(syntax)
packages/apps/clickhouse/values.yaml
[error] 60-60: trailing spaces
(trailing-spaces)
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/dashboards-configmap.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
[warning] 14-14: wrong indentation: expected 2 but found 4
(indentation)
[warning] 15-15: wrong indentation: expected 2 but found 4
(indentation)
[warning] 16-16: wrong indentation: expected 2 but found 4
(indentation)
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/servicemonitor.yaml
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
🪛 markdownlint-cli2 (0.17.2)
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/README.md
36-36: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (1)
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseoperatorconfigurations.clickhouse.altinity.com.yaml (1)
140-140: Good addition – unknown fields are preserved
x-kubernetes-preserve-unknown-fields: trueon the innerrules.itemsschema correctly keeps the CRD tolerant to forward-compatible changes.
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 (1)
packages/core/installer/values.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: the cozystack project uses yq v4+ on their ci runner, so yq v4 syntax (-o json --indent 4) is compat...
Learnt from: NickVolynkin
PR: cozystack/cozystack#1117
File: packages/apps/mysql/Makefile:8-8
Timestamp: 2025-06-26T04:29:24.830Z
Learning: The cozystack project uses yq v4+ on their CI runner, so yq v4 syntax (-o json --indent 4) is compatible and version checks are not needed.
Applied to files:
packages/core/installer/values.yaml
📚 Learning: in the cozystack repository, the maintainer nickvolynkin prefers to keep realistic-looking example c...
Learnt from: NickVolynkin
PR: cozystack/cozystack#1120
File: packages/apps/ferretdb/README.md:35-37
Timestamp: 2025-07-02T09:58:11.406Z
Learning: In the cozystack repository, the maintainer NickVolynkin prefers to keep realistic-looking example credentials in README documentation rather than using generic placeholders like <ACCESS_KEY>, even though they are just examples and not real secrets.
Applied to files:
packages/core/installer/values.yaml
🪛 YAMLlint (1.37.1)
packages/core/installer/values.yaml
[error] 2-2: no new line character at the end of file
(new-line-at-end-of-file)
⏰ 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
packages/core/installer/values.yaml
Outdated
| @@ -1,2 +1,2 @@ | |||
| cozystack: | |||
| image: ghcr.io/cozystack/cozystack/installer:v0.35.0-alpha.3@sha256:b43615350697dbbc377d31152b290722c2bba7a053fd7c69403d666e6c7d53ee | |||
| image: ghcr.io/cozystack/cozystack/installer:v0.35.0-alpha.3@sha256:b43615350697dbbc377d31152b290722c2bba7a053fd7c69403d666e6c7d53ee | |||
There was a problem hiding this comment.
Add the missing trailing newline to satisfy YAML linting.
yamllint flags an error because the file lacks a newline at EOF. This can break CI where strict linting is enforced.
image: ghcr.io/cozystack/cozystack/installer:v0.35.0-alpha.3@sha256:b43615350697dbbc377d31152b290722c2bba7a053fd7c69403d666e6c7d53ee
+📝 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.
| image: ghcr.io/cozystack/cozystack/installer:v0.35.0-alpha.3@sha256:b43615350697dbbc377d31152b290722c2bba7a053fd7c69403d666e6c7d53ee | |
| image: ghcr.io/cozystack/cozystack/installer:v0.35.0-alpha.3@sha256:b43615350697dbbc377d31152b290722c2bba7a053fd7c69403d666e6c7d53ee | |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 2-2: no new line character at the end of file
(new-line-at-end-of-file)
🤖 Prompt for AI Agents
In packages/core/installer/values.yaml at line 2, the file is missing a trailing
newline at the end, which causes yamllint errors. Add a newline character after
the last line to ensure the file ends with a newline, satisfying YAML linting
requirements and preventing CI failures.
Signed-off-by: kklinch0 <kklinch0@gmail.com>
4cfd072 to
85063cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (16)
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/dashboards-configmap.yaml (1)
18-20: Fix invalid YAML block scalar – stray space after|-breaks renderingThe literal block header must be followed immediately by a newline.
{{ $path … }}: |- {{ … }}leaves a space between|-and the newline produced bynindent, producing invalid YAML and causingkubectl apply/helm templateto fail.- {{ $path | trimPrefix "files/" }}: |- {{ $.Files.Get $path | nindent 4 -}} + {{ $path | trimPrefix "files/" }}: |- +{{ $.Files.Get $path | nindent 4 }}This also aligns with the team style of putting the template directive on its own line (see learning from PR #1160).
packages/apps/clickhouse/templates/chkeeper.yaml (2)
18-18:replicasCountis hard-wired to 3 – followvalues.yaml.
layout.replicasCount: 3gives no way to run single-node Keeper for CI or to scale to 5.
Wire this to.Values.clickhouseKeeper.replicas | default 3.
58-62: Expose a named container port to satisfyVMPodScrape.
VMPodScrapeselectsport: metrics, yet the container declares no port namedmetrics.
Without it, the operator will fail to resolve the endpoint and scraping will silently break.- containers: - - name: clickhouse-keeper - imagePullPolicy: IfNotPresent - image: clickhouse/clickhouse-keeper:24.9.2.42 + containers: + - name: clickhouse-keeper + imagePullPolicy: IfNotPresent + image: {{ .Values.clickhouseKeeper.image.repository }}:{{ .Values.clickhouseKeeper.image.tag }} + ports: + - name: metrics + containerPort: 7000packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallations.clickhouse.altinity.com.yaml (2)
450-493:macrosblock is still nested underreconciling– operator will ignore it.
799-807:pdbMaxUnavailableshould beIntOrStringto allow percentages.Kubernetes PDB semantics accept either an absolute number or
"25%".
Changing the schema keeps backward-compatibility while unlocking percentage support:- type: integer + x-kubernetes-int-or-string: truepackages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhousekeeperinstallations.clickhouse-keeper.altinity.com.yaml (5)
24-85: Still declaring 15additionalPrinterColumns– Kubernetes hard-limit is 10The block introduces 15 columns (
version … suspend).kubectl applyon apiserver ≥ 1.26 will reject the CRD (spec.versions[n].additionalPrinterColumns: Too many: 15: must have at most 10).
Trim or consolidate to ≤ 10 essential columns.
29-29: JSONPath must use bracket-notation for keys with hyphens
.status.chop-versionis parsed as subtraction and fails CRD validation. Replace with:-jsonPath: .status.chop-version +jsonPath: .status["chop-version"]
317-323:!!mergetag is non-standard YAML – breaks CRD parsing
!!merge <<: *TypeStringBoolshould be plain merge-key syntax:-suspend: - !!merge <<: *TypeStringBool +suspend: + <<: *TypeStringBool
377-384: Same!!mergeissue inunknownObjectscleanup policiesReplace every occurrence like:
-pvc: - !!merge <<: *TypeObjectsCleanup +pvc: + <<: *TypeObjectsCleanup(and similarly for
configMap,service).
393-403:reconcileFailedObjectsrepeats the invalid!!mergepatternApply the same fix as above for
statefulSet,pvc,configMap,service.packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallationtemplates.clickhouse.altinity.com.yaml (6)
318-325: Still missing precedence clarification betweensuspendandstop
The description ofspec.suspenddoes not state how it interacts with the long-standingspec.stopflag when both are set. This remains the same concern raised in the previous review.
441-449:reconcileShardsMaxConcurrencyPercentlower-bound is still0Allowing
0disables reconciliation entirely and is almost certainly a misconfiguration. Previous feedback suggestedminimum: 1.- minimum: 0 + minimum: 1
556-561: ScalarserviceTemplateand newserviceTemplatesarray can both be setThe schema still allows users to specify both knobs simultaneously. Please enforce mutual exclusivity (CEL) or deprecate the scalar, as requested earlier.
800-807:pdbMaxUnavailablevs.minAvailable– exclusivity still unenforcedThe CRD continues to accept both fields together, which results in an invalid PDB object at runtime. Add a validation rule/webhook.
808-824: Cluster-level runtime block repeats theminimum: 0issueThe same zero-percent default slips through here. Please mirror the fix above.
833-843:shardsCount/replicasCountstill lack aminimum: 1guardSchema allows
0, contradicting the description.shardsCount: type: integer + minimum: 1 replicasCount: type: integer + minimum: 1
🧹 Nitpick comments (4)
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/dashboards-configmap.yaml (1)
8-16: Minor: cleaner inline helper usage reduces noisePlacing the helper output on the same line as the parent key trims two blank lines and satisfies YAML-lint indentation heuristics:
- labels: - {{- include "altinity-clickhouse-operator.labels" . | nindent 4 }} + labels: {{- include "altinity-clickhouse-operator.labels" . | nindent 2 }}Apply the same pattern to
additionalLabels,annotations, anddashboards.annotationsblocks for consistency.packages/apps/clickhouse/values.yaml (1)
60-60: Trailing whitespace.Line 60 contains trailing spaces – minor, but it breaks
helm lint --strict.packages/apps/clickhouse/templates/chkeeper.yaml (1)
1-2: Risky dependency on cluster-wide ConfigMap.Using
lookupon a cross-namespace ConfigMap (cozy-system/cozystack) at template-render time breakshelm templateoutside the cluster and renders ChartMuseum diffing unusable.
Cache the value in.Values.clusterDomaininstead of Helm-time discovery.packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallations.clickhouse.altinity.com.yaml (1)
436-449: Duplicated reconcile-tuning knobs – drift risk.
runtime.reconcileShards*duplicates operator-level flags. Keeping both sources of truth complicates support. Consider a single configuration path or document precedence explicitly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (17)
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ClusterRole-clickhouse-operator-kube-system.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ClusterRoleBinding-clickhouse-operator-kube-system.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-clickhouse-operator-confd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-clickhouse-operator-configd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-clickhouse-operator-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-clickhouse-operator-templatesd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-clickhouse-operator-usersd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-keeper-operator-confd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-keeper-operator-configd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-keeper-operator-templatesd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ConfigMap-etc-keeper-operator-usersd-files.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/Deployment-clickhouse-operator.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/Role-clickhouse-operator.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/RoleBinding-clickhouse-operator.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/Secret-clickhouse-operator.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/Service-clickhouse-operator-metrics.yamlis excluded by!**/generated/**packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/generated/ServiceAccount-clickhouse-operator.yamlis excluded by!**/generated/**
📒 Files selected for processing (24)
hack/e2e-apps/clickhouse.bats(1 hunks)packages/apps/clickhouse/Chart.yaml(1 hunks)packages/apps/clickhouse/Makefile(1 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/workloadmonitor.yaml(1 hunks)packages/apps/clickhouse/values.schema.json(1 hunks)packages/apps/clickhouse/values.yaml(1 hunks)packages/apps/versions_map(1 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/Chart.yaml(2 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/README.md(2 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/README.md.gotmpl(1 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallations.clickhouse.altinity.com.yaml(20 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallationtemplates.clickhouse.altinity.com.yaml(20 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhousekeeperinstallations.clickhouse-keeper.altinity.com.yaml(7 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseoperatorconfigurations.clickhouse.altinity.com.yaml(5 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/files/ClickHouseKeeper_dashboard.json(4 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/files/ClickHouse_Queries_dashboard.json(2 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/_helpers.tpl(3 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/dashboards-configmap.yaml(1 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/dashboards-secret.yaml(0 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/servicemonitor.yaml(1 hunks)packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/values.yaml(14 hunks)
💤 Files with no reviewable changes (1)
- packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/dashboards-secret.yaml
✅ Files skipped from review due to trivial changes (2)
- hack/e2e-apps/clickhouse.bats
- packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/README.md.gotmpl
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/Chart.yaml
- packages/apps/versions_map
- packages/apps/clickhouse/README.md
- packages/apps/clickhouse/values.schema.json
- packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseoperatorconfigurations.clickhouse.altinity.com.yaml
- packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/README.md
- packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/values.yaml
- packages/apps/clickhouse/Chart.yaml
- packages/apps/clickhouse/Makefile
- packages/apps/clickhouse/templates/clickhouse.yaml
- packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/files/ClickHouseKeeper_dashboard.json
- packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/files/ClickHouse_Queries_dashboard.json
- packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/_helpers.tpl
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: in helm templates, the `{{-` directive chomps all leading whitespace including newlines back to the ...
Learnt from: lllamnyp
PR: cozystack/cozystack#1160
File: packages/system/hetzner-robotlb/charts/robotlb/templates/role.yaml:6-8
Timestamp: 2025-07-11T06:28:13.696Z
Learning: In Helm templates, the `{{-` directive chomps all leading whitespace including newlines back to the previous content, so `{{- toYaml .Values.something | nindent 2 }}` will render correctly even with apparent indentation issues. However, for better style, it's cleaner to put the template directive on the same line as the parent key (e.g., `rules: {{- toYaml .Values.serviceAccount.permissions | nindent 2 }}`).
Applied to files:
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhousekeeperinstallations.clickhouse-keeper.altinity.com.yamlpackages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallationtemplates.clickhouse.altinity.com.yamlpackages/apps/clickhouse/templates/workloadmonitor.yamlpackages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/dashboards-configmap.yaml
📚 Learning: in cozystack's schema generator annotation format, when documenting fields of array items, use the s...
Learnt from: NickVolynkin
PR: cozystack/cozystack#1216
File: packages/apps/virtual-machine/values.yaml:31-33
Timestamp: 2025-07-26T18:01:52.557Z
Learning: In cozystack's schema generator annotation format, when documenting fields of array items, use the singular form of the item type rather than array notation. For example, for a parameter `gpus {[]gpu}`, use `@field gpu.name` rather than `@field gpus[].name` to refer to the name field of each GPU object in the array.
Applied to files:
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallationtemplates.clickhouse.altinity.com.yaml
📚 Learning: in cozystack, the plural form for the kubernetes custom resource is `kuberneteses.apps.cozystack.io`...
Learnt from: lllamnyp
PR: cozystack/cozystack#1130
File: hack/e2e-apps/kubernetes.bats:101-101
Timestamp: 2025-07-11T06:11:25.438Z
Learning: In cozystack, the plural form for the Kubernetes custom resource is `kuberneteses.apps.cozystack.io`, not `kubernetes.apps.cozystack.io`. This is defined in the API schema even though it's not grammatically perfect.
Applied to files:
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallationtemplates.clickhouse.altinity.com.yamlpackages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallations.clickhouse.altinity.com.yaml
📚 Learning: the cozyvalues-gen tool has a known issue (https://github.com/cozystack/cozyvalues-gen/issues/10) wh...
Learnt from: NickVolynkin
PR: cozystack/cozystack#1216
File: packages/extra/monitoring/README.md:30-31
Timestamp: 2025-07-26T18:26:01.447Z
Learning: The cozyvalues-gen tool has a known issue (https://github.com/cozystack/cozyvalues-gen/issues/10) where it incorrectly places Grafana configuration entries under the wrong documentation sections, specifically placing grafana.resources.requests.* entries in the Metrics storage configuration table instead of the Grafana configuration table.
Applied to files:
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallations.clickhouse.altinity.com.yamlpackages/apps/clickhouse/templates/chkeeper.yamlpackages/apps/clickhouse/values.yaml
📚 Learning: in the cozystack repository, for the virtual-machine app's resources.sockets parameter, the value is...
Learnt from: NickVolynkin
PR: cozystack/cozystack#1216
File: packages/apps/virtual-machine/values.yaml:0-0
Timestamp: 2025-07-26T18:12:05.641Z
Learning: In the cozystack repository, for the virtual-machine app's resources.sockets parameter, the value is intentionally kept as a string in values.yaml despite being documented as {int} type, because the string-to-integer parsing happens elsewhere in the system pipeline rather than at the YAML validation level.
Applied to files:
packages/apps/clickhouse/templates/chkeeper.yaml
📚 Learning: the `./charts/robotlb/` directory in the hetzner-robotlb package contains vendored code, and the tea...
Learnt from: lllamnyp
PR: cozystack/cozystack#1233
File: packages/system/hetzner-robotlb/charts/robotlb/templates/deployment.yaml:33-35
Timestamp: 2025-07-23T09:15:09.658Z
Learning: The `./charts/robotlb/` directory in the hetzner-robotlb package contains vendored code, and the team generally avoids modifying vendored code to maintain clean separation from upstream dependencies.
Applied to files:
packages/apps/clickhouse/templates/chkeeper.yaml
📚 Learning: in github actions yaml workflows, multi-line strings under `run:` without block scalars (| or >) are...
Learnt from: lllamnyp
PR: cozystack/cozystack#1092
File: .github/workflows/pull-requests.yaml:90-94
Timestamp: 2025-06-23T11:01:31.400Z
Learning: In GitHub Actions YAML workflows, multi-line strings under `run:` without block scalars (| or >) are automatically folded by YAML into single-line commands. This is correct behavior and doesn't require using block scalars with backslashes for line continuation.
Applied to files:
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/dashboards-configmap.yaml
🧬 Code Graph Analysis (1)
packages/apps/clickhouse/templates/workloadmonitor.yaml (1)
api/v1alpha1/workloadmonitor_types.go (1)
Selector(8-35)
🪛 YAMLlint (1.37.1)
packages/apps/clickhouse/templates/chkeeper.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/apps/clickhouse/templates/workloadmonitor.yaml
[error] 15-15: syntax error: could not find expected ':'
(syntax)
packages/apps/clickhouse/values.yaml
[error] 60-60: trailing spaces
(trailing-spaces)
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/dashboards-configmap.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
[warning] 14-14: wrong indentation: expected 2 but found 4
(indentation)
[warning] 15-15: wrong indentation: expected 2 but found 4
(indentation)
[warning] 16-16: wrong indentation: expected 2 but found 4
(indentation)
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/servicemonitor.yaml
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
⏰ 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 (6)
packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/templates/servicemonitor.yaml (1)
12-12: Ensure helper returns a mapping – otherwise YAML becomes invalid.
annotations: {{ include "altinity-clickhouse-operator.annotations" . | nindent 4 }}relies on the helper returning only YAML key-value lines preceded by a newline.
If the helper ever returns an empty string, you’ll end up withannotations:which is illegal. Guard with an
ifor default to{}to avoid malformed YAML.{{- $ann := include "altinity-clickhouse-operator.annotations" . }} {{- if $ann }} annotations: {{ $ann | nindent 4 }} {{- end }}packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/crds/CustomResourceDefinition-clickhouseinstallationtemplates.clickhouse.altinity.com.yaml (5)
7-15: Operator version bump looks goodThe metadata and label are consistently updated to 0.25.2. 👍
54-60: Printer-column rename is correctRenaming
hosts-unchangedtohosts-completedprovides better semantic clarity. No issues spotted.
85-89: Addedsuspendprinter column – LGTMThe new column surfaces the reconciliation-suspension flag and follows existing column conventions.
227-233: Newendpointsarray in statusHaving the full list of connection endpoints will help dashboards; the schema is sound.
251-256:hostsWithReplicaCaughtUpfield – nice observability gainNo schema inconsistencies detected.
| replicas: 3 | ||
| minReplicas: 3 | ||
| kind: clickhouse |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hard-coded replica counts – make them configurable.
replicas: 3 / minReplicas: 3 ignore the new clickhouseKeeper sizing that users may want to tweak (e.g., dev clusters).
Expose these through values.yaml instead of pinning to three.
- replicas: 3
- minReplicas: 3
+ replicas: {{ .Values.clickhouseKeeper.replicas | default 3 }}
+ minReplicas: {{ .Values.clickhouseKeeper.replicas | default 3 }}📝 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: 3 | |
| minReplicas: 3 | |
| kind: clickhouse | |
| replicas: {{ .Values.clickhouseKeeper.replicas | default 3 }} | |
| minReplicas: {{ .Values.clickhouseKeeper.replicas | default 3 }} | |
| kind: clickhouse |
🤖 Prompt for AI Agents
In packages/apps/clickhouse/templates/workloadmonitor.yaml around lines 21 to
23, the replica counts are hard-coded to 3, which prevents users from
customizing the sizing for different environments. Modify the file to use values
from values.yaml for replicas and minReplicas instead of fixed numbers, allowing
users to configure these parameters as needed.
| ## @section clickhouseKeeper parameters | ||
| ## @param clickhouseKeeper.enabled Deploy ClickHouse Keeper for cluster coordination | ||
| ## @param clickhouseKeeper.size Persistent Volume Claim size, available for application data | ||
| ## @param clickhouseKeeper.resourcesPreset Default sizing preset used when `resources` is omitted. Allowed values: nano, micro, small, medium, large, xlarge, 2xlarge. | ||
| clickhouseKeeper: | ||
| enabled: true | ||
| size: 1Gi | ||
| resourcesPreset: micro |
There was a problem hiding this comment.
Missing replicas & image settings for Keeper block.
The chart now deploys a 3-node Keeper by default, but the corresponding tunables are absent from values.yaml.
Please add at least:
clickhouseKeeper:
enabled: true
replicas: 3 # quorum size
image:
repository: clickhouse/clickhouse-keeper
tag: 24.9.2.42so that users can down-scale for dev or pin image versions without editing templates.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 60-60: trailing spaces
(trailing-spaces)
🤖 Prompt for AI Agents
In packages/apps/clickhouse/values.yaml around lines 59 to 66, the
clickhouseKeeper block is missing the replicas and image settings. Add a
replicas field set to 3 and an image block with repository set to
clickhouse/clickhouse-keeper and tag set to 24.9.2.42. This will allow users to
configure the number of Keeper nodes and specify the image version without
modifying the templates.
What this PR does
Release note
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Chores