Helm/simplify chart#1718
Conversation
|
CLA Assistant Lite bot: I have read the CLA Document and I hereby sign the CLA AdheipSingh seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. |
WalkthroughThe Helm chart is refactored from a monolithic/mode-specific configuration to a node-oriented schema (standalone, query, ingestor). Templates now derive service/persistence/env/credential settings from ChangesHelm Chart Node-Based Restructuring
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
helm/templates/standalone-deployment.yaml (1)
53-56: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
tplon env values will fail for non-string values.
tplrequires a string argument; if a user supplies a numeric or boolean env value undercommon.env/nodes.*.env(e.g.RETENTION_DAYS: 7orDEBUG: true), rendering fails withexpected string; got int/bool. Coerce withtoString. This same pattern recurs inquerier-statefulset.yaml(Lines 93-96) andingestor-statefulset.yaml(Lines 107-110).🔧 Proposed fix
{{- range $key, $value := $env }} - name: {{ $key }} - value: {{ tpl $value $ | quote }} + value: {{ tpl (toString $value) $ | quote }} {{- end }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@helm/templates/standalone-deployment.yaml` around lines 53 - 56, The env rendering uses tpl directly on $value, which breaks when common.env or nodes.*.env contains non-string values like ints or booleans. Update the env blocks in standalone-deployment.yaml, querier-statefulset.yaml, and ingestor-statefulset.yaml to coerce each $value with toString before passing it to tpl, so the templates always receive a string. Use the existing env range blocks as the target locations and keep the quoted output unchanged.helm/templates/querier-statefulset.yaml (1)
84-91: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueHot tier on query node has no guard against
local-store.The values comment notes hot tier "not supported with local-store", but nothing prevents
nodes.query.persistence.enabled: truewhilestore: local-store, which would setP_HOT_TIER_DIRin an unsupported configuration. Consider afailguard in the template or_helpers.tplvalidation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@helm/templates/querier-statefulset.yaml` around lines 84 - 91, The querier StatefulSet template allows P_HOT_TIER_DIR to be set whenever node.persistence.enabled is true, but it does not block the unsupported local-store combination. Add a guard in the querier template or shared validation helpers to fail rendering when nodes.query.persistence.enabled is enabled while store is local-store, and keep the P_HOT_TIER_DIR/P_MAX_DISK_USAGE_PERCENT block gated behind that safe configuration. Reference the querier statefulset logic and any helper validation used for store/persistence checks so the constraint is enforced consistently.helm/templates/standalone-service.yaml (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider making
targetPortconfigurable for consistency with ingestor service.The standalone service hardcodes
targetPort: 8000, while the ingestor service uses{{ $node.port }}from values. Ifnodes.standalone.portwere added to the values schema (matching the ingestor pattern), this would make both services configurable and avoid drift if the container port ever changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@helm/templates/standalone-service.yaml` at line 14, The standalone service currently hardcodes targetPort instead of following the configurable pattern used by the ingestor service. Update the standalone service template to read the port from values via a new nodes.standalone.port setting, and wire that value through the same style used by the existing service templates so targetPort stays configurable and consistent. Use the standalone service manifest and the ingestor service’s port lookup as the reference points when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@helm/README.md`:
- Around line 16-29: The cloud deployment section only says to “create the
secret” without showing the required keys, so update the README examples around
the distributed install instructions to include concrete kubectl create secret
commands for the cloud provider secrets. Use the existing standalone secret
example as the pattern, and make sure the distributed docs reference the same
secret names/keys used by the values files; for the GCP case, explicitly include
key.json in the secret because it is required by the credentialFile mount.
- Around line 33-35: The Helm upgrade example in the README is too generic and
can overwrite cloud-specific settings; update the upgrade guidance referenced by
the helm upgrade command to show the correct values file for each deployment
variant. Make the instructions explicitly distinguish the standalone, AWS, GCP,
and Azure cases so users reuse the same environment-specific values used at
install time instead of defaulting to values.yaml.
In `@helm/templates/ingestor-statefulset.yaml`:
- Around line 142-153: The ingestor’s stage-volume mount is only added when
$node.persistence.enabled, so with persistence disabled there is no dedicated
staging volume and writes fall back to the container layer. Update the ingestor
StatefulSet template’s volumeMounts/volumes logic to always provide a
stage-volume, using the persistent volume when enabled and an emptyDir fallback
when disabled, similar to the standalone/querier setup. Use the existing
ingestor staging mount block and stage-volume symbol to place the change
consistently.
In `@helm/templates/querier-statefulset.yaml`:
- Around line 46-50: The querier StatefulSet pod spec is missing the image pull
secret wiring, so private-registry images can fail to start in HA mode. Update
the pod spec in the querier StatefulSet template to render the same
`imagePullSecrets` value used by `standalone-deployment.yaml`, and apply the
same fix in `ingestor-statefulset.yaml` so both HA workloads use the
values-backed secret consistently.
In `@helm/values.yaml`:
- Around line 69-72: The pod Prometheus scrape annotation is advertising the
wrong port, so metrics discovery will target a closed port instead of the app’s
actual listener. Update the podAnnotations in values.yaml to use the same port
exposed by the container (the containerPort used by the workload), and keep the
scrape path unchanged so Prometheus targets the correct endpoint.
- Around line 91-95: The default standalone/local-store path currently writes to
/parseable/data but nodes.standalone.persistence.data.enabled is off, so
restarts lose ingested data. Update the Helm defaults in values.yaml to enable
the data PVC by default for the standalone install path, and make sure any
local-store-specific configuration still points fs.dir to the persistent mount.
If you choose not to change the default, then explicitly document the
persistence requirement in the install docs, but the preferred fix is to turn on
persistence in the standalone configuration.
---
Nitpick comments:
In `@helm/templates/querier-statefulset.yaml`:
- Around line 84-91: The querier StatefulSet template allows P_HOT_TIER_DIR to
be set whenever node.persistence.enabled is true, but it does not block the
unsupported local-store combination. Add a guard in the querier template or
shared validation helpers to fail rendering when nodes.query.persistence.enabled
is enabled while store is local-store, and keep the
P_HOT_TIER_DIR/P_MAX_DISK_USAGE_PERCENT block gated behind that safe
configuration. Reference the querier statefulset logic and any helper validation
used for store/persistence checks so the constraint is enforced consistently.
In `@helm/templates/standalone-deployment.yaml`:
- Around line 53-56: The env rendering uses tpl directly on $value, which breaks
when common.env or nodes.*.env contains non-string values like ints or booleans.
Update the env blocks in standalone-deployment.yaml, querier-statefulset.yaml,
and ingestor-statefulset.yaml to coerce each $value with toString before passing
it to tpl, so the templates always receive a string. Use the existing env range
blocks as the target locations and keep the quoted output unchanged.
In `@helm/templates/standalone-service.yaml`:
- Line 14: The standalone service currently hardcodes targetPort instead of
following the configurable pattern used by the ingestor service. Update the
standalone service template to read the port from values via a new
nodes.standalone.port setting, and wire that value through the same style used
by the existing service templates so targetPort stays configurable and
consistent. Use the standalone service manifest and the ingestor service’s port
lookup as the reference points when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ba9f10f6-d89f-44c6-a005-84fb206516fc
⛔ Files ignored due to path filters (1)
helm/Chart.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
helm-releases/pai-0.3.0.tgzhelm/Chart.yamlhelm/README.mdhelm/charts/fluent-bit-0.48.1.tgzhelm/charts/vector-0.20.1.tgzhelm/templates/_helpers_config_logstream.txthelm/templates/data-pvc.yamlhelm/templates/ingestor-service.yamlhelm/templates/ingestor-statefulset.yamlhelm/templates/logstream-configmap.yamlhelm/templates/logstream-job.yamlhelm/templates/querier-service.yamlhelm/templates/querier-statefulset.yamlhelm/templates/service-monitor.yamlhelm/templates/stage-pvc.yamlhelm/templates/standalone-deployment.yamlhelm/templates/standalone-service.yamlhelm/values-aws.yamlhelm/values-azure.yamlhelm/values-gcp.yamlhelm/values.yamlindex.yaml
💤 Files with no reviewable changes (4)
- helm/templates/service-monitor.yaml
- helm/templates/logstream-configmap.yaml
- helm/templates/logstream-job.yaml
- helm/templates/_helpers_config_logstream.txt
| ## Distributed on a cloud (querier + ingestors) | ||
|
|
||
| Create the secret with the keys for your cloud (see the matching values file), then: | ||
|
|
||
| ```sh | ||
| # AWS (S3) | ||
| helm install parseable ./ -n parseable --create-namespace -f values-aws.yaml | ||
|
|
||
| # GCP (GCS) | ||
| helm install parseable ./ -n parseable --create-namespace -f values-gcp.yaml | ||
|
|
||
| # Azure (Blob) | ||
| helm install parseable ./ -n parseable --create-namespace -f values-azure.yaml | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add concrete secret creation examples for each cloud provider.
The instruction "Create the secret with the keys for your cloud (see the matching values file)" requires users to cross-reference the values file to determine which keys to create. Including a concrete kubectl create secret example for at least one cloud (similar to the standalone section) would significantly improve usability. For GCP, note that the same secret must also contain the key.json service account key for the credentialFile mount.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@helm/README.md` around lines 16 - 29, The cloud deployment section only says
to “create the secret” without showing the required keys, so update the README
examples around the distributed install instructions to include concrete kubectl
create secret commands for the cloud provider secrets. Use the existing
standalone secret example as the pattern, and make sure the distributed docs
reference the same secret names/keys used by the values files; for the GCP case,
explicitly include key.json in the secret because it is required by the
credentialFile mount.
| ```sh | ||
| helm upgrade parseable ./ -n parseable -f values.yaml | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Upgrade command will break cloud-specific installs.
helm upgrade parseable ./ -n parseable -f values.yaml uses the chart's default values. Users who installed with -f values-aws.yaml (or GCP/Azure) would have their store, highAvailability, persistence, and secrets configuration overridden with defaults, potentially breaking their deployment. Helm does not automatically reuse values from the previous install.
🔧 Proposed fix
## Upgrade
```sh
-helm upgrade parseable ./ -n parseable -f values.yaml
+helm upgrade parseable ./ -n parseable -f values.yaml # standalone
+helm upgrade parseable ./ -n parseable -f values-aws.yaml # AWS
+helm upgrade parseable ./ -n parseable -f values-gcp.yaml # GCP
+helm upgrade parseable ./ -n parseable -f values-azure.yaml # Azure📝 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.
| ```sh | |
| helm upgrade parseable ./ -n parseable -f values.yaml | |
| ``` |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@helm/README.md` around lines 33 - 35, The Helm upgrade example in the README
is too generic and can overwrite cloud-specific settings; update the upgrade
guidance referenced by the helm upgrade command to show the correct values file
for each deployment variant. Make the instructions explicitly distinguish the
standalone, AWS, GCP, and Azure cases so users reuse the same
environment-specific values used at install time instead of defaulting to
values.yaml.
| {{- if or $node.persistence.enabled $cred.enabled }} | ||
| volumeMounts: | ||
| {{- if $node.persistence.enabled }} | ||
| - name: stage-volume | ||
| mountPath: "/parseable/staging" | ||
| {{- end }} | ||
| {{- if $cred.enabled }} | ||
| - name: {{ $cred.secretName }} | ||
| mountPath: {{ $cred.mountPath }} | ||
| readOnly: true | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Ingestor staging has no volume when persistence is disabled.
Unlike standalone/querier (which always attach a stage-volume emptyDir), the ingestor mounts stage-volume only when $node.persistence.enabled. With the default persistence.enabled: false, staging writes land on the container's writable layer with no dedicated volume — inconsistent with the other node types and surprising if a read-only root FS is later enabled. Consider an emptyDir fallback mount when persistence is disabled.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@helm/templates/ingestor-statefulset.yaml` around lines 142 - 153, The
ingestor’s stage-volume mount is only added when $node.persistence.enabled, so
with persistence disabled there is no dedicated staging volume and writes fall
back to the container layer. Update the ingestor StatefulSet template’s
volumeMounts/volumes logic to always provide a stage-volume, using the
persistent volume when enabled and an emptyDir fallback when disabled, similar
to the standalone/querier setup. Use the existing ingestor staging mount block
and stage-volume symbol to place the change consistently.
| spec: | ||
| terminationGracePeriodSeconds: 10 | ||
| serviceAccountName: {{ include "parseable.serviceAccountName" . }} | ||
| {{- with .Values.parseable.toleration }} | ||
| securityContext: | ||
| {{- toYaml $common.podSecurityContext | nindent 8 }} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Missing imagePullSecrets on HA querier pod spec.
imagePullSecrets was added to values and is rendered in standalone-deployment.yaml (Lines 32-35), but this StatefulSet pod spec omits it. In HA mode, querier pods pulling from a private registry will fail ImagePullBackOff. The same gap exists in ingestor-statefulset.yaml.
🔧 Proposed fix
terminationGracePeriodSeconds: 10
serviceAccountName: {{ include "parseable.serviceAccountName" . }}
+ {{- with .Values.parseable.imagePullSecrets }}
+ imagePullSecrets:
+ {{- toYaml . | nindent 8 }}
+ {{- end }}
securityContext:
{{- toYaml $common.podSecurityContext | nindent 8 }}📝 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.
| spec: | |
| terminationGracePeriodSeconds: 10 | |
| serviceAccountName: {{ include "parseable.serviceAccountName" . }} | |
| {{- with .Values.parseable.toleration }} | |
| securityContext: | |
| {{- toYaml $common.podSecurityContext | nindent 8 }} | |
| spec: | |
| terminationGracePeriodSeconds: 10 | |
| serviceAccountName: {{ include "parseable.serviceAccountName" . }} | |
| {{- with .Values.parseable.imagePullSecrets }} | |
| imagePullSecrets: | |
| {{- toYaml . | nindent 8 }} | |
| {{- end }} | |
| securityContext: | |
| {{- toYaml $common.podSecurityContext | nindent 8 }} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@helm/templates/querier-statefulset.yaml` around lines 46 - 50, The querier
StatefulSet pod spec is missing the image pull secret wiring, so
private-registry images can fail to start in HA mode. Update the pod spec in the
querier StatefulSet template to render the same `imagePullSecrets` value used by
`standalone-deployment.yaml`, and apply the same fix in
`ingestor-statefulset.yaml` so both HA workloads use the values-backed secret
consistently.
| podAnnotations: | ||
| prometheus.io/scrape: "true" | ||
| prometheus.io/port: "80" | ||
| prometheus.io/path: "/api/v1/metrics" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
prometheus.io/port mismatch with container port.
The container listens on containerPort: 8000, but the scrape annotation advertises port 80. Pod-level Prometheus scraping targets the pod IP on the annotated port, which isn't open, so metrics scraping will fail.
🔧 Proposed fix
podAnnotations:
prometheus.io/scrape: "true"
- prometheus.io/port: "80"
+ prometheus.io/port: "8000"
prometheus.io/path: "/api/v1/metrics"📝 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.
| podAnnotations: | |
| prometheus.io/scrape: "true" | |
| prometheus.io/port: "80" | |
| prometheus.io/path: "/api/v1/metrics" | |
| podAnnotations: | |
| prometheus.io/scrape: "true" | |
| prometheus.io/port: "8000" | |
| prometheus.io/path: "/api/v1/metrics" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@helm/values.yaml` around lines 69 - 72, The pod Prometheus scrape annotation
is advertising the wrong port, so metrics discovery will target a closed port
instead of the app’s actual listener. Update the podAnnotations in values.yaml
to use the same port exposed by the container (the containerPort used by the
workload), and keep the scrape path unchanged so Prometheus targets the correct
endpoint.
| data: | ||
| enabled: false | ||
| storageClass: "" | ||
| accessMode: ReadWriteOnce | ||
| size: 5Gi |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP 'fs\.dir|/parseable/data|P_FS_DIR' helm/Repository: parseablehq/parseable
Length of output: 459
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== helm/values.yaml relevant section =="
sed -n '1,140p' helm/values.yaml | nl -ba | sed -n '70,110p'
echo
echo "== helm/templates/standalone-deployment.yaml relevant section =="
sed -n '1,160p' helm/templates/standalone-deployment.yaml | nl -ba | sed -n '70,120p'
echo
echo "== helm/README.md relevant section =="
sed -n '1,80p' helm/README.md | nl -ba | sed -n '1,40p'
echo
echo "== search for data-volume / emptyDir / persistence wiring =="
rg -n 'emptyDir|data-volume|persistence\.data|store: local-store|store:' helm/Repository: parseablehq/parseable
Length of output: 239
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== helm/values.yaml (around persistence defaults) =="
sed -n '70,110p' helm/values.yaml
echo
echo "== helm/templates/standalone-deployment.yaml (around data volume mount) =="
sed -n '75,120p' helm/templates/standalone-deployment.yaml
echo
echo "== helm/README.md (fs.dir hint) =="
sed -n '1,40p' helm/README.md
echo
echo "== search for persistence and emptyDir wiring =="
rg -n 'emptyDir|data-volume|persistence\.data|store: local-store|fs\.dir|P_FS_DIR|/parseable/data' helm/Repository: parseablehq/parseable
Length of output: 4760
Default standalone installs lose local-store data on restart
store: local-store is the default, and /parseable/data is backed by emptyDir unless nodes.standalone.persistence.data.enabled is set. The chart’s local-store install path also points fs.dir at /parseable/data, so the default install drops ingested data on pod restarts. Default this PVC on, or make the local-store requirement explicit in the install docs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@helm/values.yaml` around lines 91 - 95, The default standalone/local-store
path currently writes to /parseable/data but
nodes.standalone.persistence.data.enabled is off, so restarts lose ingested
data. Update the Helm defaults in values.yaml to enable the data PVC by default
for the standalone install path, and make sure any local-store-specific
configuration still points fs.dir to the persistent mount. If you choose not to
change the default, then explicitly document the persistence requirement in the
install docs, but the preferred fix is to turn on persistence in the standalone
configuration.
1 participant
Fixes #XXXX.
Description
This PR has:
Summary by CodeRabbit
New Features
Documentation
Bug Fixes