feat(qdrant): add Qdrant vector database chart - #310
Conversation
Add a datasource Helm chart for a single-node Qdrant (v1.19.0): - StatefulSet on a persistent volume at /qdrant/storage, non-root uid 1000 - REST (6333) + gRPC (6334); ClusterIP + headless services - Generated API key stored in a Secret and preserved across upgrades - Built-in Prometheus /metrics scraped by an authenticated ServiceMonitor, plus a PrometheusRule with a QdrantDown alert - Optional customConfig mounted at /qdrant/config/local.yaml with reloader - values.schema.json with UI categories for the zop.dev datasource form Packaged as qdrant-v0.0.1.tgz and indexed in docs/index.yaml.
arunesh-j
left a comment
arunesh-j
left a comment
There was a problem hiding this comment.
Review: feat(qdrant): add Qdrant vector database chart
Reviewed at 3ce6741 in an isolated worktree, and installed on a local minikube.
The chart is well built — the non-root securityContext, the snapshot-path
redirect, the lookup-preserved API key and the customConfig mount all work
exactly as documented, which I confirmed on a cluster rather than by reading.
Findings below are ordered by consequence. Two are blocking, and one of them I
was able to reproduce as a hard helm install failure.
Blocking
1. alerts.yaml:4 — the PrometheusRule name collides with every other datastore chart
metadata:
name: {{ .Release.Name }}Qdrant is a datasource chart, so its normal use is as a dependency of an app
chart — and app charts here almost always also pull in postgres. postgres,
redis, mysql, clickhouse, solr and kafka all name their PrometheusRule
{{ .Release.Name }} too, so the release contains two objects of the same kind
and name and Helm refuses the install outright.
Reproduced with an umbrella chart depending on qdrant + postgres:
Error: INSTALLATION FAILED: 1 error occurred:
* prometheusrules.monitoring.coreos.com "myapp" already exists
Fix — name it after the chart, as localai and holmesgpt do. The helper is
already defined in this chart and currently never used (see note 8):
metadata:
name: {{ include "qdrant.fullname" . }}2. Chart.yaml:6 — the icon hotlinks qdrant.tech, and the URL is dead
icon: "https://qdrant.tech/images/logo_with_text.svg"curl -sI on it returns HTTP 404, so the chart renders logo-less on
helm.zop.dev today. Separately, this is the only chart of the 32 in the repo
pointing at a third-party host — every other icon is served from
storage.googleapis.com/zopdev-* or zop.dev/logo.png. An image the repo does
not control can 404, be replaced, or start serving something else, on a zop.dev
product surface.
This does not fail CI (icon is recommended is INFO), but it is a reject-level
rule for this repo. Please upload the logo to
storage.googleapis.com/zopdev-blog-resources/ — as clickhouse, litellm,
localai, holmesgpt and n8n do — and point icon: there. A reviewer cannot
upload to the bucket, so this one needs you.
3. README.md:5-9 — the Prometheus Operator CRDs are an undeclared prerequisite
alerts.yaml and serviceMonitor.yaml are ungated, so every install emits a
ServiceMonitor and a PrometheusRule. On a cluster without
kube-prometheus-stack, helm install fails with no matches for kind "ServiceMonitor" in version "monitoring.coreos.com/v1". Prerequisites currently
promise only Kubernetes 1.19+ and Helm 3+.
Ungating matches the datastore family (postgres, mysql, clickhouse, kafka
are all unconditional), so I am not asking you to add a metrics.enabled flag —
just to state the CRD requirement in Prerequisites so nobody walks into a failed
install.
Not verified on a CRD-less cluster — the minikube I tested on already had the
CRDs installed. This one is reasoned from the templates.
Should fix
4. /metrics is NOT open — README.md:59 and statefulset.yaml:46-48 say it is
Measured against the running container, through the Service:
| Request | Status |
|---|---|
/metrics no key |
401 |
/metrics with api-key header |
200 |
/livez, /readyz, /healthz no key |
200 |
So this comment on the container env is wrong:
# The health and /metrics endpoints stay open, so probes and the
# ServiceMonitor keep working without the key.and so is README.md:59 ("The health endpoints ... and /metrics remain open"),
which also contradicts README.md:108, which gets it right.
The ServiceMonitor itself is correct — it sends the key as a Bearer token,
and that is the only reason scraping works. The risk is purely that the comment
invites a future reader to delete the authorization: block as redundant, which
would silently break metrics collection. Please correct both spots.
5. serviceMonitor.yaml:9-11 — the selector matches both Services, so everything is scraped twice
service.yaml:7 and service-headless.yaml:7 both carry app: <release>-qdrant,
and the ServiceMonitor selects on exactly that label. Verified — both back the
same pod:
r310-qdrant [('10.244.0.76', True)]
r310-qdrant-headless [('10.244.0.76', True)]
Two consequences:
- Every Qdrant metric is ingested twice, doubling scrape load and series count.
service-headless.yaml:12setspublishNotReadyAddresses: true, so during
startup and every rolling upgrade Prometheus scrapes a not-ready pod and gets
up == 0.QdrantDown(alerts.yaml:17) isfor: 0m, so a critical alert
fires on every single upgrade.
Fix: narrow the ServiceMonitor to the ClusterIP Service — e.g. give the headless
one a distinguishing label (app: <release>-qdrant-headless, keeping its pod
selector unchanged), or add a discriminating label to the scrape target. Raising
for: to 2m on QdrantDown is worth doing regardless.
6. alerts.yaml:27-28 — QdrantHighRestResponseErrors can never fire
The expression uses rest_responses_total, which Qdrant does not export. I
dumped every metric name from a live v1.19.0 /metrics, including at
?detail_level=10, and there is no rest_ or grpc_ metric at all (0 matches).
Qdrant tracks REST responses only under /telemetry (requests.rest.responses),
which Prometheus never sees.
The alert is inert — it reads as protection and provides none. Either drop it, or
build one on a metric that exists. From the live output, app_status_recovery_mode
is the useful one:
- alert: QdrantInRecoveryMode
expr: app_status_recovery_mode{namespace="...", service="..."} == 1Other real metrics available: collection_dead_replicas, collection_points,
memory_resident_bytes, process_open_fds, snapshot_creation_running.
7. values.schema.json — no description on any leaf
version, diskSize, customConfig and the four resources.* leaves all lack
description. The schema drives the zop.dev configuration form, so these render
as unlabelled inputs. charts/litellm/values.schema.json is the reference shape:
"version": {
"category": "runtime",
"type": "string",
"description": "Qdrant image tag (qdrant/qdrant:<version>)",
"default": "v1.19.0",
"mutable": true
}The good text already exists as comments in values.yaml — it just needs to be
in the schema too.
Notes
_helpers.tpl:4definesqdrant.fullnameand nothing uses it — every
template hardcodes{{ .Release.Name }}-qdrant. Adopting it fixes finding 1.
The helper set is also missing.name,.chartand.labels, which the repo
convention includes.Chart.yaml:2—appVersion: "1.0"while the shipped image isv1.19.0.
Mixed precedent (clickhouseandlocalaialso say"1.0"), butlitellm
(1.83.14) andholmesgpt(0.39.0) track the real version."1.19.0"here.- Branch is
feat/qdrant; CONTRIBUTING §2 asks forfeature/,fix/or
docs/. Cosmetic at this point, noting for the next PR. _helpers.tpl:19mixesapp.kubernetes.io/part-ofinto an otherwise flat
label set. The repo convention is the flatapp/chart/releaseset.
Note the StatefulSet selector is immutable, so this is easier to settle now
than later.- No
NOTES.txt. Mixed precedent (localaiandholmesgptship one, the
datastores do not), but this chart generates a credential the operator has to
go fetch — the retrieval command and the retained-PVC warning would sit well
there. - helm.zop.dev renders from a hardcoded list in
docs/src/js/config.js, so
qdrant will not appear on the site after merge. Repo-level drift tracked in
#308, not a condition of this PR.
Verified clean
Reproduced CI and the packaging checks — no problems:
helm dependency update+helm lint→ 0 failed, no warnings.docs/index.yaml:added: 1, removed: 0, changed: 0— no other chart's entry
touched. 181 tarball digests verified.docs/qdrant-v0.0.1.tgzunpacks to
exactly the chart source,vprefix correct,annotations.type: datasource
preserved in the packagedChart.yaml. No committed subchart tarballs, no hard
tabs.- Schema: no
additionalProperties: falseat root, all fourcategoryvalues
legal,diskSizecarrying bothmutableandeditDisabledmatches all 30
instances of that pair in the repo.
Cluster test — minikube, helm install into a dedicated namespace:
- Pod reached Ready with 0 restarts.
- Non-root
securityContextgenuinely works: container runsuid=1000 gid=2000
with supplementary group3000, and writes to the freshly provisioned PVC
succeed. Created a collection, inserted a point, and took a snapshot — the
QDRANT__STORAGE__SNAPSHOTS_PATHredirect to/qdrant/storage/snapshotsis
what makes that work, and the directory comes out1000:2000. helm upgradepreserved the API key byte-for-byte and the collection
survived — thelookupbranch does what it claims.customConfigmounts at/qdrant/config/local.yamland takes effect (set
log_level: DEBUG, DEBUG lines appeared in the logs).
Nice chart overall — findings 1 and 2 are the ones that need you before merge.
arunesh-j
left a comment
arunesh-j
left a comment
There was a problem hiding this comment.
Posting the findings inline against the lines they refer to, so they are easier to work through than the summary review. Same content, nothing new — the blocking ones are the PrometheusRule name in alerts.yaml, the 404ing icon in Chart.yaml, and the undeclared CRD prerequisite in the README.
| apiVersion: monitoring.coreos.com/v1 | ||
| kind: PrometheusRule | ||
| metadata: | ||
| name: {{ .Release.Name }} |
There was a problem hiding this comment.
Blocking — this name collides and makes helm install fail.
qdrant is a datasource chart, so its normal use is as a dependency of an app chart — and app charts here almost always also pull in postgres. postgres, redis, mysql, clickhouse, solr and kafka all name their PrometheusRule {{ .Release.Name }} too, so the release ends up with two objects of the same kind and name.
I reproduced it with an umbrella chart depending on qdrant + postgres:
Error: INSTALLATION FAILED: 1 error occurred:
* prometheusrules.monitoring.coreos.com "myapp" already exists
localai and holmesgpt avoid this by naming it after the chart. The helper is already defined in this chart (_helpers.tpl:4) and currently unused:
| name: {{ .Release.Name }} | |
| name: {{ include "qdrant.fullname" . }} |
| - alert: QdrantHighRestResponseErrors | ||
| expr: increase(rest_responses_total{namespace="{{ .Release.Namespace }}", service="{{ .Release.Name }}-qdrant", status=~"5.."}[5m]) > 0 |
There was a problem hiding this comment.
Should fix — this alert can never fire.
rest_responses_total is not a metric Qdrant exposes. I dumped every metric name from a live v1.19.0 /metrics, including at ?detail_level=10, and there is no rest_ or grpc_ metric at all (0 matches). Qdrant tracks REST responses only under /telemetry (requests.rest.responses), which Prometheus never sees.
An inert alert is worse than no alert, because it reads as coverage. Either drop it, or rebuild it on a metric that exists. From the live output, the genuinely useful one is:
- alert: QdrantInRecoveryMode
expr: app_status_recovery_mode{namespace="{{ .Release.Namespace }}", service="{{ .Release.Name }}-qdrant"} == 1Other real metrics available: collection_dead_replicas, collection_points, memory_resident_bytes, process_open_fds, snapshot_creation_running.
| description: Helm chart for deploying qdrant vector database datastore | ||
| name: qdrant | ||
| version: 0.0.1 | ||
| icon: "https://qdrant.tech/images/logo_with_text.svg" |
There was a problem hiding this comment.
Blocking — off-domain icon, and the URL is dead.
curl -sI on this returns HTTP 404, so the chart renders logo-less on helm.zop.dev today.
Separately, this is the only chart of the 32 in the repo pointing at a third-party host — every other icon is served from storage.googleapis.com/zopdev-* or zop.dev/logo.png. An image the repo does not control can 404, be replaced, or start serving something else, on a zop.dev product surface.
This does not fail CI (icon is recommended is INFO), but it is a reject-level rule for this repo. Please upload the logo to storage.googleapis.com/zopdev-blog-resources/ — as clickhouse, litellm, localai, holmesgpt and n8n do — and point icon: there. A reviewer cannot upload to that bucket, so this one needs you.
| @@ -0,0 +1,11 @@ | |||
| apiVersion: v2 | |||
| appVersion: "1.0" | |||
There was a problem hiding this comment.
Note — appVersion should track the app actually shipped; values.yaml pins qdrant/qdrant:v1.19.0.
Mixed precedent (clickhouse and localai also say "1.0"), but litellm (1.83.14) and holmesgpt (0.39.0) track the real version.
| appVersion: "1.0" | |
| appVersion: "1.19.0" |
| # Enabling the API key makes Qdrant reject unauthenticated requests. | ||
| # The health and /metrics endpoints stay open, so probes and the | ||
| # ServiceMonitor keep working without the key. |
There was a problem hiding this comment.
Should fix — this comment is factually wrong, and the error is load-bearing.
Measured against the running container, through the Service:
| Request | Status |
|---|---|
/metrics no key |
401 |
/metrics with api-key header |
200 |
/livez, /readyz, /healthz no key |
200 |
/metrics does not stay open. The ServiceMonitor works only because it sends the key as a Bearer token. The risk is that this comment invites a future reader to delete that authorization: block as redundant, silently breaking metrics collection.
(The probes are fine — the three health endpoints really are unauthenticated.)
| {{/* | ||
| Fully qualified app name: <release>-qdrant, capped at 63 chars for DNS. | ||
| */}} | ||
| {{- define "qdrant.fullname" -}} |
There was a problem hiding this comment.
Note — this helper is defined and then never used; every template hardcodes {{ .Release.Name }}-qdrant instead.
Worth wiring up, because using it in alerts.yaml:4 is exactly the fix for the install-blocking PrometheusRule collision.
The helper set is also missing .name, .chart and .labels, which the repo convention includes.
| {{- define "qdrant.selectorLabels" -}} | ||
| app.kubernetes.io/part-of: qdrant |
There was a problem hiding this comment.
Note — this mixes an app.kubernetes.io/* label into an otherwise flat label set. The repo convention is the flat app / chart / release set rather than the upstream Helm one.
Worth settling now rather than later: this feeds spec.selector.matchLabels on the StatefulSet, which is immutable — changing it after release means users have to delete the StatefulSet to upgrade.
| - Kubernetes 1.19+ | ||
| - Helm 3+ | ||
| - [Stakater Reloader](https://github.com/stakater/Reloader) (optional, recommended) — required only for the pod to restart automatically when `customConfig` changes. |
There was a problem hiding this comment.
Blocking — the Prometheus Operator CRDs are a hard prerequisite and are not listed.
alerts.yaml and serviceMonitor.yaml are ungated, so every install emits a ServiceMonitor and a PrometheusRule. On a cluster without kube-prometheus-stack, helm install fails with:
no matches for kind "ServiceMonitor" in version "monitoring.coreos.com/v1"
Ungating matches the datastore family (postgres, mysql, clickhouse, kafka are all unconditional), so I am not asking for a metrics.enabled flag — just state the requirement so nobody walks into a failed install:
| - Kubernetes 1.19+ | |
| - Helm 3+ | |
| - [Stakater Reloader](https://github.com/stakater/Reloader) (optional, recommended) — required only for the pod to restart automatically when `customConfig` changes. | |
| - Kubernetes 1.19+ | |
| - Helm 3+ | |
| - **Prometheus Operator CRDs** (`ServiceMonitor`, `PrometheusRule`) — required, not optional: the chart renders both unconditionally, so `helm install` fails on a cluster without them. | |
| - [Stakater Reloader](https://github.com/stakater/Reloader) (optional, recommended) — required only for the pod to restart automatically when `customConfig` changes. |
Caveat: reasoned from the templates, not verified — the minikube I tested on already had the CRDs installed.
|
|
||
| ## Authentication | ||
|
|
||
| The chart generates a strong API key on install and stores it in a Kubernetes Secret named `<release-name>-qdrant-apikey-secret` under the key `api-key`. Qdrant is started with `QDRANT__SERVICE__API_KEY` set to this value, so all data requests must be authenticated. The health endpoints (`/livez`, `/readyz`, `/healthz`) and `/metrics` remain open, so probes and monitoring work without the key. |
There was a problem hiding this comment.
Should fix — /metrics is not open, and this contradicts line 108.
Verified against the running container: /metrics returns 401 without the key and 200 with it. Only /livez, /readyz and /healthz are unauthenticated.
Line 108 of this same README gets it right ("When the API key is enabled, /metrics requires authentication"). Please drop /metrics from this sentence so the two agree — the shipped ServiceMonitor depends on the line-108 behaviour being the true one.
| helm uninstall [RELEASE_NAME] | ||
| ``` | ||
|
|
||
| The PersistentVolumeClaim created by the StatefulSet is retained by default; delete it manually if you want to reclaim the storage. |
There was a problem hiding this comment.
Note — the retained-PVC warning is good. Two things would round the docs out:
- The chart ships no
templates/NOTES.txt. Mixed precedent (localaiandholmesgptship one, the datastores do not), but this chart generates a credential the operator has to go and fetch — the retrieval command and this PVC warning would sit well there, printed at install time. - Worth stating explicitly that
helm uninstalldeletes the API-key Secret while keeping the PVC, so a reinstall against the retained volume comes up with a new key.
…abels, docs - alerts: name PrometheusRule after the chart (qdrant.fullname) to avoid a name collision with sibling datastore charts in an umbrella release; raise QdrantDown to for: 2m; replace the inert QdrantHighRestResponseErrors (metric Qdrant does not export) with QdrantInRecoveryMode on app_status_recovery_mode - serviceMonitor/headless: give the headless Service a distinct app label so only the ClusterIP Service is scraped (no double scrape, no upgrade-time QdrantDown flap); flatten selector labels to the immutable-safe app-only set - statefulset/README: correct the claim that /metrics is unauthenticated — it requires the key; only the health endpoints are open - README: document the Prometheus Operator CRDs as a hard prerequisite; clarify uninstall keeps the PVC but drops the API-key Secret - schema: add descriptions to every leaf for the datasource form - add NOTES.txt with API-key retrieval and connection details - Chart.yaml: appVersion 1.19.0; icon to on-domain zop.dev/logo.png (was a 404ing third-party URL) - repackage qdrant-v0.0.1.tgz and refresh its index digest
|
Thanks for the thorough, cluster-verified review — this was exactly the kind of feedback that catches things Blocking
Should fix
Notes
Repackaged |
arunesh-j
left a comment
arunesh-j
left a comment
There was a problem hiding this comment.
Re-review of 6525d29
Re-ran the whole review against the new commit in a fresh worktree and reinstalled on minikube. All three blocking findings are fixed, and I confirmed the two that were testable rather than taking the diff's word for it.
One thing came back in a new file — details inline.
Blocking — all resolved
| # | Finding | Status |
|---|---|---|
| 1 | PrometheusRule collision | Fixed & verified |
| 2 | Off-domain 404 icon | Fixed |
| 3 | Undeclared CRD prerequisite | Fixed |
On (1), the umbrella chart that previously failed now installs cleanly, and both rules coexist:
STATUS: deployed
$ kubectl get prometheusrule
myapp <- postgres
myapp-qdrant <- qdrant
On (3), this was the one claim I could not verify last time. The README now states it, which is what I asked for — still reasoned rather than tested, since the minikube still has the CRDs.
Should fix — resolved
/metricsauth —statefulset.yamlandREADME.md:59both corrected. Re-measured:/metrics→ 401 without the key, 200 with.- Double scrape — the headless Service's new
app: <release>-qdrant-headlesslabel works.kubectl get svc -l app=r310b-qdrantnow returns exactly one Service, and the pod selector is untouched so DNS identity still works. - Dead alert —
QdrantInRecoveryModereplaces it. I confirmedapp_status_recovery_modeis really exported (app_status_recovery_mode 0) and the CRD accepted the rule.QdrantDownatfor: 2mis the right call. - Schema descriptions —
verify-values.pynow reports0 blocking, 0 should-fix(was 1 should-fix).
Notes — resolved
appVersion: "1.19.0", qdrant.fullname now used, app.kubernetes.io/part-of dropped from the immutable selector, NOTES.txt added.
Nice touch on the PVC docs — I checked the command you documented actually works, which was not a given: the StatefulSet controller copies the selector label onto the volumeClaimTemplate PVC, so kubectl delete pvc -l app=<release>-qdrant really does match.
One remaining should-fix
templates/NOTES.txt:11 reintroduces the exact /metrics claim that was just fixed in the other two places. Commented inline.
Verified on this commit
helm lint→ 0 failed. Icon returns 200, and the repo-wide off-domain audit is clean.docs/index.yaml:added: 1, removed: 0, changed: 0— the repackaged tarball unpacks to exactly the current source (NOTES.txtincluded), digest matches,vprefix intact. Repackagingv0.0.1is safe here since it has never been published tomain.- Cluster: Ready with 0 restarts;
helm upgradepreserved the API key byte-for-byte and the test collection survived.
Nothing blocking from me now. I'm leaving my earlier CHANGES_REQUESTED in place rather than clearing it myself — that call is @zopdev's, and the NOTES.txt line above is a one-liner if you want it gone first.
| The health endpoints and /metrics stay reachable, but all data | ||
| operations are rejected without it. |
There was a problem hiding this comment.
Should fix — this is the /metrics claim from the last round, back in a new file.
README.md:59 and the statefulset.yaml comment were both corrected in this commit, but the new NOTES.txt repeats the original error. Re-measured against this build:
/metrics no key -> 401
/metrics with key -> 200
/metrics does not stay reachable — only /livez, /readyz and /healthz do. This is the text an operator sees at install time, so it is the most likely of the three to be believed.
| The health endpoints and /metrics stay reachable, but all data | |
| operations are rejected without it. | |
| Pass it in the "api-key" header (or "Authorization: Bearer <key>"). | |
| Only the health endpoints (/livez, /readyz, /healthz) are open — every | |
| other request, including /metrics, is rejected without the key. |
arunesh-j
left a comment
arunesh-j
left a comment
There was a problem hiding this comment.
Correction to my re-review — the icon is not properly resolved
In the re-review above I marked the icon finding Fixed. That was wrong, and I want to correct it before it ships.
https://zop.dev/logo.png is on the allowed list and returns 200, which is what I checked. What I did not check is whether that generic logo is appropriate for a chart of this type — and in this repo it never is. Details inline on Chart.yaml.
The original problem is genuinely gone: the icon is no longer off-domain and no longer 404s, so the supply-chain and broken-image concerns are resolved. What replaced it is a placeholder, and qdrant is a datasource chart that will render in the zop.dev Datasources catalog.
Downgrading this from blocking to should fix — it installs fine and carries no risk — but it does need the real asset, which only someone with bucket access can upload.
| description: Helm chart for deploying qdrant vector database datastore | ||
| name: qdrant | ||
| version: 0.0.1 | ||
| icon: "https://zop.dev/logo.png" |
There was a problem hiding this comment.
Should fix — this is the generic zop.dev mark, not the Qdrant logo, and this chart is type: datasource.
Correcting myself here: I accepted this in my re-review because it is on the allowed list and returns 200. But I checked the wrong thing. Auditing all 33 charts against their annotations.type:
| Chart type | Own product logo | Generic zop.dev/logo.png |
|---|---|---|
datasource (15) |
15 | 0 |
application (8) |
8 | 0 |
| untyped building blocks (10) | 2 | 5 |
The correlation is exact. The only five charts using the generic logo — cron-job, scylladb, service, uptime-monitoring, zookeeper — are all untyped, so they never appear in the zop.dev listing at all. Every chart that carries an annotations.type ships its own product logo, with no exceptions.
Chart.yaml:9 sets type: datasource, so qdrant lands in the Datasources catalog next to postgres, redis, kafka and clickhouse — each showing its real logo — while qdrant shows a generic zop.dev mark. Qdrant has a perfectly good logo of its own; this is the case the icon: field exists for.
Fix: upload the Qdrant logo to storage.googleapis.com/zopdev-blog-resources/ — the bucket clickhouse, litellm, localai, holmesgpt and n8n use — and point icon: at it. I cannot upload to that bucket, so this one needs you.
Everything else in 6525d29 still checks out; this is the only item I got wrong.
- icon: point to the Qdrant logo in the zopdev-blog-resources bucket, matching the datasource-chart convention (every typed chart ships its own product logo) - NOTES.txt: correct the auth note — /metrics is not open; only the health endpoints (/livez, /readyz, /healthz) are reachable without the key - repackage qdrant-v0.0.1.tgz and refresh its index digest
|
@arunesh-j thanks for the second pass — both remaining items are addressed in
Repackaged |
arunesh-j
left a comment
arunesh-j
left a comment
There was a problem hiding this comment.
Re-review of 1f18326 — everything I raised is now resolved
Both remaining items are fixed, and I verified each rather than reading the diff.
Icon
icon: now points at zopdev-blog-resources/.../qdrant.png. I fetched it and looked at the image itself rather than trusting the filename — it is the genuine Qdrant mark (the isometric red q cube), a 600×600 PNG, HTTP/2 200, content-type: image/png.
That also puts qdrant on the newer bucket alongside clickhouse, litellm, localai, holmesgpt and n8n, and restores the invariant that made me reopen this: every chart carrying an annotations.type ships its own product logo. The repo-wide off-domain audit is clean.
NOTES.txt
The corrected text renders on a live install, and I re-measured every endpoint it names to confirm the text is now true rather than merely different:
| Endpoint | No key | NOTES claims |
|---|---|---|
/livez |
200 | open ✓ |
/readyz |
200 | open ✓ |
/healthz |
200 | open ✓ |
/metrics |
401 | rejected ✓ |
/metrics with the key returns 200. All three places that describe this — README.md, the statefulset.yaml comment, and now NOTES.txt — finally agree with each other and with the server.
Re-verified on this commit
helm lint→ 0 failed;verify-values.py→ 0 blocking, 0 should-fix.docs/index.yaml:added: 1, removed: 0, changed: 0. The repackaged tarball unpacks to exactly the current source, digest matches,vprefix intact, and the packagedNOTES.txtcarries the corrected wording.- Umbrella install with
qdrant+postgres: 0 duplicate object names — the PrometheusRule collision stays fixed. - Cluster: Ready with 0 restarts; ServiceMonitor still selects exactly 1 Service, so no double scrape.
Summary across the three rounds
| Severity | Raised | Open |
|---|---|---|
| Blocking | 3 | 0 |
| Should fix | 6 | 0 |
| Note | 4 | 0 |
Nothing outstanding from me. Two things worth recording, neither a change request:
- The
_helpers.tplset still omits.name,.chartand.labels. Several datastore charts in the repo are the same, so this is consistent with where the chart lives — noting it only so it is a deliberate choice rather than an oversight. - helm.zop.dev renders from the hardcoded list in
docs/src/js/config.js, so qdrant will not appear on the site after merge until that list is updated (repo-wide drift, issue #308) — worth a follow-up now that the chart has a proper logo to show.
Nice work turning these around. I am not approving — my earlier CHANGES_REQUESTED is still what is blocking the merge, and clearing it is @zopdev's call, not mine. From a review standpoint there is nothing left to fix.
arunesh-j
left a comment
arunesh-j
left a comment
There was a problem hiding this comment.
Three publishing items — plus a correction to something I told you earlier
Re-ran the review against 32c7f71c. The chart itself is still clean; these are all about publishing, and I should have caught the first two in round one.
First, a correction
In earlier rounds I told you qdrant "will not appear on helm.zop.dev until docs/src/js/config.js is updated (issue #308)". That is no longer true — the main you just merged replaced config.js with helm-index.js, and the site now builds its grid from docs/index.yaml. Nothing needs adding to a site list. Sorry for the noise; the merge changed the answer under me, and it makes item 3 below relevant instead.
1. docs/index.yaml was hand-edited, not regenerated — should fix
CONTRIBUTING Step 6 requires cd docs && helm repo index . --url https://helm.zop.dev, which rewrites generated: and every created: timestamp. This PR shows neither:
| Signal | This PR | Every other chart PR |
|---|---|---|
docs/index.yaml diff |
+16 −0 | ~+190 −175 |
generated: changed |
no — byte-identical to main |
yes |
created: rewritten |
0 of 180 | all 180 |
I ran Step 6 locally: it produces 366 changed lines, 362 of them created:. The last 12 commits touching this file — 5f318dd, ff3ad93, f9b595b, 4512540, 9691853, 72cd8f3, d19c5d8 and back — all show the full churn. This PR is the only exception.
The entry itself is correct — I diffed it field-by-field against generated output: digest verified against the tarball, urls, annotations, appVersion and alphabetical position all right. The only mismatch is created:, which reads 2026-08-12T15:56:00.000000+05:30 (round seconds, zero microseconds; helm emits values like 15:41:22.241313).
So nothing is broken today. It matters because the digest is 64 hand-copied hex characters — right this time, with nothing but the tool guaranteeing the next one, and a wrong digest fails at helm install on a user's machine rather than in CI. Detail inline.
2. Root README.md chart table is missing qdrant — should fix
Available Charts → 1. DATASOURCES lists 15 charts; the repo has 16 with annotations.type: datasource. qdrant is the only one absent, and the table is otherwise exhaustive:
| **Qdrant** | [helm.zop.dev/qdrant](https://helm.zop.dev/qdrant) | ✅ |The ✅ is right — the chart ships a ServiceMonitor and exports Prometheus metrics.
In fairness: CONTRIBUTING never mentions the root README, so this is an unwritten convention rather than a documented step you skipped. Precedent is nonetheless consistent — holmesgpt (#307), litellm and clickhouse (#287) all updated it in the same PR. Might be worth a Step 7 in CONTRIBUTING so the next contributor is not left to infer it.
3. description: is now the site card text — should fix
This one only became a finding with the merge. Per the new CONTRIBUTING section Showing up on helm.zop.dev, Chart.yaml's description is what renders on the integrations grid, and it should be written "for a reader who is choosing an integration".
Yours currently reads:
Helm chart for deploying qdrant vector database datastore
That is chart-implementation copy, and it lowercases the brand mid-sentence. On the grid it will sit beside:
- ChromaDB — The AI-native embedding database
- ClickHouse — Column-oriented database for real-time analytics at scale
Something like "High-performance vector database for AI search, recommendations and RAG" would match. You do not need a docs/src/js/display.js entry — I checked the fallback and qdrant title-cases to Qdrant correctly on its own.
All three are should fix, none blocking, and the chart itself is unchanged from my last review: 0 open findings on the templates, schema, and cluster behaviour.
| urls: | ||
| - https://helm.zop.dev/postgres-v0.0.1.tgz | ||
| version: v0.0.1 | ||
| qdrant: |
There was a problem hiding this comment.
Should fix — this entry was written by hand; CONTRIBUTING Step 6 was not run.
The block is correct — I diffed it field-by-field against what helm repo index generates and everything matches, digest included. But the file shows the command was never executed:
generated:at the top of the file is byte-identical tomain(2026-08-11T21:22:14.966412+05:30).helm repo indexalways rewrites it.- 0 of 180 existing
created:timestamps changed; the command rewrites every one. - The whole diff is +16 −0. Running Step 6 here produces 366 changed lines.
The created: value below is a further giveaway — 2026-08-12T15:56:00.000000+05:30 has round seconds and zero microseconds, where helm emits e.g. 15:41:22.241313.
Fix:
cd docs && helm repo index . --url https://helm.zop.devExpect the diff to grow to ~370 lines. That is normal here — every other chart PR in this repo looks like that. (--merge is not a shortcut; it rewrites the timestamps too.)
- docs/index.yaml: regenerate with 'helm repo index' (CONTRIBUTING Step 6) instead of a hand-edited entry, so the digest is tool-computed - Chart.yaml: rewrite description as site-card copy — 'High-performance vector database for AI search, recommendations and RAG' (was implementation text that lowercased the brand); repackage v0.0.1 - README.md: add Qdrant to the DATASOURCES table
|
@arunesh-j all three publishing items addressed in
|
# Conflicts: # docs/index.yaml
Review —
|
| Check | Result |
|---|---|
CI reproduction (helm dependency update + helm lint) |
clean |
| Defaults render | clean |
| Icon on a zopdev-controlled bucket, and resolves | storage.googleapis.com/zopdev-blog-resources/… → HTTP 200 |
annotations.type |
datasource |
| Schema checker (categories, mutable, defaults, descriptions) | 0 blocking, 0 should-fix |
index.yaml isolation |
added: 1 (qdrant v0.0.1), removed: 0, changed: 0; 183 digests verified |
index.yaml genuinely regenerated |
0/182 created: timestamps carried → regenerated, generated: touched twice, diff +200/−183 |
v-prefixed tarball, version matches Chart.yaml, no committed subchart tarball |
✓ |
| PrometheusRule name collision trap | avoided — named <release>-qdrant, not {{ .Release.Name }}, with a comment saying why |
| Duplicate kind+name in the release | none |
| Root README DATASOURCES table | Qdrant listed |
| Install on minikube | Ready, 0 restarts, clean logs |
| Upgrade | ×2 clean; API key preserved via lookup |
Several things here are better than the repo's average and worth saying so:
- The CRD prerequisite is documented with the exact failure text.
ServiceMonitor/PrometheusRulerender unconditionally — the norm for datasource charts here (redis, postgres, mysql, mariadb, kafka, clickhouse, solr, solrcloud all do the same) — but this is the first chart I've seen state it as a hard prerequisite and quoteno matches for kind "ServiceMonitor". That is exactly the honest-prerequisites bar; other charts silently send people into a failed install. - The non-root story is complete rather than aspirational.
runAsNonRoot+fsGroup: 3000for the provisioned volume, and — the part that is usually missed —QDRANT__STORAGE__SNAPSHOTS_PATHandQDRANT_INIT_FILE_PATHredirected onto the PVC because the image's own directories are root-owned. - The ServiceMonitor's Bearer credentials point at a secret that actually exists. Rendered:
credentials.name: t-qdrant-apikey-secret,key: api-key, matching the rendered Secret exactly. Easy thing to get subtly wrong; it is right. - NOTES.txt covers the trap I went looking for: uninstall deletes the Secret but retains the PVC, so a reinstall against the retained volume comes up with a new key.
1. [Note] The chart is single-node, and nothing says so
templates/statefulset.yaml:15 hardcodes replicas: 1, there is no scaling value, and Qdrant logs Distributed mode disabled on boot. README.md and values.yaml never mention it.
Readers arriving from the sibling datastores will assume otherwise — charts/postgres exposes replication.enabled/replication.count, so the absence of any such key here reads as an oversight rather than a decision. One line in the README (and/or a comment in values.yaml) saying the chart runs a single node with no HA/distributed-mode support would settle it, and would also tell a reader evaluating this for production what they are choosing.
2. [Note] appVersion can silently drift from the image actually deployed
Chart.yaml:2—appVersion: "1.19.0"values.yaml:2—version: "v1.19.0"(the image tag)templates/statefulset.yaml:37—image: qdrant/qdrant:{{ .Values.version }}
The image tag comes from .Values.version, and appVersion feeds nothing. They also differ in form (v prefix), so they cannot be compared at a glance. Bump one and forget the other and the chart advertises a version it is not running — on helm.zop.dev and in helm list.
Both spellings are correct in isolation (Qdrant's tags are v-prefixed), so this is only about keeping them in step. Either derive the default — qdrant/qdrant:{{ .Values.version | default (printf "v%s" .Chart.AppVersion) }} — or leave a comment at both sites noting they must move together.
3. [Note, trivial] livenessProbe.initialDelaySeconds: 30 is inert
templates/statefulset.yaml:66-71. A startupProbe is defined, and the kubelet does not run liveness or readiness until the startup probe first succeeds — so the 30s delay never applies. Harmless, but it reads as protection that is not doing anything. Dropping it makes the probe block say what it means.
What I verified versus what I read
Verified on minikube (fresh namespace, per-PR release name, explicit --context on every call): install reached Ready with 0 restarts and no warnings in the logs; /metrics → 401 without the key and 200 with it; /livez, /readyz, /healthz → 200 unauthenticated, so the probes work as claimed; /collections → 401; two helm upgrades clean, with the API key byte-identical before and after; --set customConfig='log_level: DEBUG' mounted at /qdrant/config/local.yaml and Qdrant emitted DEBUG lines, so the subPath mount and the reloader annotation target are correct; the plain ConfigMap carries only QDRANT_HOST/QDRANT_HTTP_PORT/QDRANT_GRPC_PORT — nothing sensitive.
Read, not executed: the alert rule expressions themselves (QdrantDown) were not fired against a live Prometheus, and Reloader is not installed on my test cluster, so the annotation's target was verified but not an actual reload.
Two things I checked and specifically am not reporting, since verification showed they would have been unfair: the ungated CRD objects (the established datasource pattern here, and honestly documented), and the feat/qdrant branch name against CONTRIBUTING §2's feature/|fix/|docs/ — merged history shows service-cronjob-v0.0.3, chart-mysql-v0.0.1 and similar, so that convention is not actually in force.
…robe cleanup - README/values.yaml: state the chart is single-node (replicas: 1, no HA) - Chart.yaml/values.yaml: comment that appVersion and the image tag (version) must move together - statefulset: drop inert initialDelaySeconds from liveness/readiness probes — the startupProbe already gates both - repackage v0.0.1 and regenerate docs/index.yaml
|
@arunesh-j thanks — all three notes addressed in
Repackaged |
arunesh-j
left a comment
arunesh-j
left a comment
There was a problem hiding this comment.
Re-review of 29efec9
All three notes resolved. Nothing new, and nothing outstanding from my side.
| Note | Shipped | Verified |
|---|---|---|
| 1. Single-node undocumented | README callout at line 5 — immediately under the intro, not buried — plus a matching comment in values.yaml |
✓ |
2. appVersion / image-tag drift |
Comments at both sites; the Chart.yaml one also says appVersion is what shows on helm.zop.dev and helm list |
✓ |
3. Inert initialDelaySeconds |
Removed from liveness and readiness, with a comment explaining the startupProbe gates both |
✓ rendered pod has neither field; startupProbe still 30×10s |
On skipping the | default (printf "v%s" .Chart.AppVersion) derive — you're right, and for the right reason. values.yaml always ships an explicit version, so that branch could only fire if someone set version: ""; it would be exactly the inert-config pattern note 3 was about. My note offered "derive or comment at both sites", and the comments are the better half of that choice here. Extending note 3's reasoning to readinessProbe was also correct — the startup probe gates readiness identically, so that delay was equally dead.
Re-ran, not assumed
helm dependency update+helm lint: clean. Defaults render. Schema checker: 0 blocking, 0 should-fix.- Cluster: fresh namespace on minikube,
helm install --waitreturned in 12s, pod Ready with 0 restarts. Rendered probe spec confirmsinitialDelaySecondsis gone from both and the startup budget is unchanged — so removing it did not cost you readiness responsiveness. Torn down afterwards. - Publishing: 183 tarball digests verified;
added: 1 (qdrant v0.0.1), removed: 0, changed: 0; index regenerated properly again (0/182created:timestamps carried). Packaged tarball matches source apart from helm's ownChart.yamlreserialization. - On repackaging
v0.0.1rather than bumping: correct here, and I checked rather than assumed — qdrant is absent fromorigin/main's index andhelm.zop.devserves noqdrant-v0.0.1.tgz, so nothing live can be broken by replacing that artifact. The rule against republishing a version with different content only bites once the version has actually shipped.
Not re-verified this round (unchanged since the first pass, and outside 29efec9's diff): the API-key auth behaviour (/metrics 401 → 200, health endpoints open), key preservation across upgrade, and the customConfig subPath mount. All three were confirmed on a cluster during the first review.
Looks good to me.
jatintalgotra-zd
left a comment
jatintalgotra-zd
left a comment
There was a problem hiding this comment.
Independent pass on 29efec9 — 3 findings
Reviewed in a separate worktree and installed on minikube with the full kube-prometheus-stack rather than just the CRDs, so the ServiceMonitor path could actually be exercised end to end. The chart is in good shape and most of the load-bearing claims hold up under test — what I confirmed working is listed at the bottom.
Three things still look open to me. I traced each to primary sources (Qdrant's own source, the official docs, the official qdrant-helm chart) rather than reasoning from the diff, and reproduced the two behavioural ones on the cluster.
1. QdrantInRecoveryMode cannot fire as the chart ships — templates/alerts.yaml:32-36
The alert fires on app_status_recovery_mode == 1. Nothing in this chart can produce that value. The chain, closed end to end in the Qdrant tree:
| Step | Evidence |
|---|---|
| The image runs the wrapper script | Dockerfile:246 — CMD ["./entrypoint.sh"] |
| Its recovery branch is the only injector in the image | tools/entrypoint.sh:52 — QDRANT__STORAGE__RECOVERY_MODE="$RECOVERY_MESSAGE" ./qdrant $@ |
| …and it is gated off by default | tools/entrypoint.sh:29 — QDRANT_ALLOW_RECOVERY_MODE=${QDRANT_ALLOW_RECOVERY_MODE:-false}tools/entrypoint.sh:32 — if [ "$QDRANT_ALLOW_RECOVERY_MODE" != true ]; then exit $EXIT_CODE; fi |
| No default in shipped config | recovery_mode appears in neither config/config.yaml nor config/production.yaml, so Option<String> stays None (lib/storage/src/types.rs:127) |
| The metric derives from nothing else | src/common/telemetry_ops/app_telemetry.rs:117 — recovery_mode: settings.storage.recovery_mode.is_some()src/common/metrics.rs:235 — gauge(if self.recovery_mode { 1.0 } else { 0.0 }, &[]) |
| The chart never sets the gate | grep -rn ALLOW_RECOVERY_MODE charts/qdrant/ → no matches; rendered env is only QDRANT__SERVICE__API_KEY, QDRANT__STORAGE__SNAPSHOTS_PATH, QDRANT_INIT_FILE_PATH |
A grep of the whole Qdrant source shows no other writer of recovery_mode — no API, no flag.
Reproduced. Installed with resources.limits.memory=16Mi to force the OOM this alert exists for:
lastState: terminated{ exitCode: 137, reason: OOMKilled }
./entrypoint.sh: line 25: 7 Killed ./qdrant $@
r3-qdrant-0 0/1 CrashLoopBackOff 3 (30s ago)
Straight to CrashLoopBackOff, no recovery attempt. And the alert's expression is fine — forcing storage.recovery_mode through customConfig gave me app_status_recovery_mode 1 and the rule loaded health: ok. The trigger is the only missing piece.
Worth noting this is the same class of problem as the QdrantHighRestResponseErrors finding from an earlier round, so it may be worth a rule of thumb: an alert ships with a reproduction of the state it fires on.
Suggested fix — one line, which resolves three things at once:
- name: QDRANT_ALLOW_RECOVERY_MODE
value: "true"That makes an OOM boot into recovery mode (where collections can be dropped to get back under the limit — the remedy Qdrant designed for this) instead of crashlooping; it gives QDRANT_INIT_FILE_PATH a consumer, since the entrypoint's init-file handshake is otherwise dead code in this chart; and it makes the alert reachable. Otherwise the honest move is to drop the alert.
The official chart corroborates that this is the intended knob — it is the one documented example env var in its values.yaml:14:
env: []
# - name: QDRANT_ALLOW_RECOVERY_MODE
# value: true(Upstream leaves it off, but upstream also ships no PrometheusRule at all, so it has nothing to be inconsistent with.)
2. Changing customConfig on helm upgrade is a silent no-op
Reproduced on the cluster. Installed with telemetry_disabled: true, then upgraded to telemetry_disabled: false, log_level: DEBUG:
upgraded
ConfigMap now holds: telemetry_disabled: false / log_level: DEBUG
sts generation: 1 -> 1 (no rollout triggered)
pod uid: ce64f1ce-… -> ce64f1ce-… (unchanged)
file inside the running pod: telemetry_disabled: true / log_level: INFO <- stale
helm upgrade reports success, the ConfigMap updates, and nothing reaches the process. So customConfig is effectively write-only after install.
Two upstream confirmations of the mechanism:
- The Kubernetes docs are explicit, and the chart's own comment at
statefulset.yaml:8-9already says as much: "A container using a ConfigMap as asubPathvolume mount will not receive updates when the ConfigMap changes." - The Reloader annotation is correctly placed — Stakater's docs put it on the workload's
metadata.annotations, not the pod template, and StatefulSet is a supported kind.statefulset.yaml:7-11is right.
So this isn't a broken annotation. The gap is that the only mechanism is conditional on an operator that README.md:12 itself calls "(optional, recommended)", and the pod template (statefulset.yaml:24-26) carries labels only. On any cluster without Reloader — including a plain helm install from helm.zop.dev — the documented customConfig knob silently stops working after install.
Suggested fix — additive, keep the Reloader annotation and add a checksum to the pod template:
template:
metadata:
annotations:
checksum/custom-config: {{ include (print $.Template.BasePath "/custom-config-configmap.yaml") . | sha256sum }}This is what the official Qdrant chart does (qdrant-helm/charts/qdrant/templates/statefulset.yaml:29-31, checksum/config + checksum/secret), and it is already the idiom in four charts here — holmesgpt, litellm, opentsdb, openobserve-standalone. It needs no cluster add-ons.
clickhouse has the same gap, but qdrant is unreleased, so fixing it here costs nothing.
3. Telemetry is on by default, while chromadb turns it off — judgment call
Confirmed on a default install:
INFO qdrant: Telemetry reporting enabled, id: fb734b81-e3e6-4082-b454-b1731365fc10
From the source: config/config.yaml:433 sets telemetry_disabled: false; config/production.yaml (the RUN_MODE layer the image sets at Dockerfile:234) overrides only log_level and service, so it does not touch it; src/main.rs:390 gates on !settings.telemetry_disabled && !args.disable_telemetry; and src/common/telemetry_reporting.rs:41,60 POSTs to https://telemetry.qdrant.io on a REPORTING_INTERVAL of one hour.
I want to be fair about the severity here. The official docs say the payload is system info, timings/counters and error backtraces, and explicitly not IP addresses, identifying information, stored data, collection names or URLs — so it is anonymous, not a data leak. And the official Qdrant chart does not disable it either. So this is not an upstream-practice divergence, and I would not call it a defect.
What does stand is the repo's own convention for this exact class of chart: charts/chromadb/templates/configmap.yaml:10 sets ANONYMIZED_TELEMETRY: "False", and it is load-bearing — consumed at charts/chromadb/templates/statefulset.yaml:22-24 via envFrom.configMapRef. Since this PR positions qdrant as the production counterpart to chromadb, it seems worth the two chart's telemetry postures matching, whichever way you pick. Hourly egress to a third-party endpoint from a customer namespace is also the kind of thing that trips egress policy.
If you want it off, the official docs name the exact env var (ops-configuration/usage-statistics/):
- name: QDRANT__TELEMETRY_DISABLED
value: "true"I checked precedence rather than assuming it: with that env set and telemetry_disabled: false in the mounted customConfig, the log flips to Telemetry reporting disabled. Env wins, consistent with values.yaml:23-24. Note that also makes it non-overridable via customConfig, same as the API key — which I think is the behaviour you want.
Entirely your call — flagging it as a consistency decision to make deliberately rather than something to fix.
Verified working (not read — measured)
Listing these so the findings above aren't mistaken for a poor overall impression.
- Install clean: 22s, pod
1/1, 0 restarts, non-rootuid=1000 gid=2000 groups=2000,3000. - Auth matrix matches all three places that document it —
/livez/readyz/healthz→ 200 with no key;/metrics/collections/telemetry→ 401 without, 200 with eitherapi-keyorAuthorization: Bearer. - The
ServiceMonitorBearer auth genuinely works. Prometheus targethealth: up,lastError: '', andup{namespace="qd",service="r1-qdrant"} = 1— so the label selectors in the alert expressions are correct. Exactly one target, confirming the headless-Service relabel fixed the double scrape. Both rules loadedhealth: ok. - Snapshots and init file correctly relocated onto the PVC. I also checked that this doesn't defeat the entrypoint's OOM heuristic:
src/startup.rscallsremove_started_file_indicator()before initialization ("Use before server initialization to avoid false positives"), so persisting the marker is right. - Data plane: collection create, upsert and vector search all correct;
points_count: 2surviveskubectl delete pod. - API key preserved across
helm upgrade; pattern matchespostgresexactly. - Umbrella install with
postgresin one release works —myappandmyapp-qdrantPrometheusRules coexist,myapp-postgresandmyapp-qdrantServiceMonitors coexist. The original blocking collision is genuinely gone. - PVC carries
app=<release>-qdrant, so the README'skubectl delete pvc -l app=…actually works. - The README's CRD prerequisite reproduces verbatim, including the quoted error text.
- Uninstall clean; only the PVC remains, which both
README.mdandNOTES.txtdocument. - Publishing:
docs/index.yamlshows qdrant added, 0 removed, 0 changed, and all 183 digests in the index match their tarballs on disk.v-prefixed tarball matches repo convention. - Conventions all line up with siblings:
mutable+editDisabledondiskSize,patternon cpu/memory,generation: 1, ServiceMonitor withoutnamespace, flatappselector label, PVC template naming,startupProbe30×10s (identical to chromadb), thelookupsecret pattern, root README row,annotations.type: datasource, icon on the zopdev bucket. On hardening it is ahead of the repo — almost no other datastore chart here setsrunAsNonRoot.
Nice chart overall. (2) is the one I'd treat as blocking, since a documented knob that silently stops working generates support load; (1) is a small change with a real payoff; (3) is yours to decide.
- statefulset: set QDRANT_ALLOW_RECOVERY_MODE=true so an OOM boots into recovery mode (drop a collection to recover) instead of crash-looping, and so the QdrantInRecoveryMode alert is actually reachable - statefulset: add a checksum/custom-config pod-template annotation so changing customConfig on 'helm upgrade' rolls the pod even without Reloader installed (subPath mounts don't live-update); verified the pod rolls and picks up config - statefulset: set QDRANT__TELEMETRY_DISABLED=true to match the chromadb chart and avoid third-party egress from customer namespaces - repackage v0.0.1 and regenerate docs/index.yaml
|
@jatintalgotra-zd excellent review — the source-traced findings were exactly right. All three addressed in 1.
|
jatintalgotra-zd
left a comment
jatintalgotra-zd
left a comment
There was a problem hiding this comment.
Re-review of 2d33b2e — all three fixed correctly; one new trade-off, and one older item
Re-ran everything on a fresh minikube with the full kube-prometheus-stack. The diff is exactly the three changes and nothing else, and all three do what they claim. Two things I'd still like to raise — the first is a consequence of the fix I asked for, so it's on me for not thinking it through in the original comment.
The three fixes — verified
customConfig rollout (the blocking one) — measured with no Reloader on the cluster:
| Scenario | Result |
|---|---|
| config A → B | checksum changed, sts generation 1→2, pod rolled, running pod holds config B, and DEBUG lines in the log prove it reached the process |
| re-upgrade, same values | pod stable — checksum is deterministic, so no restart churn on every upgrade |
customConfig removed |
annotation, volume and ConfigMap all cleanly removed, pod rolled |
| default release | no checksum annotation — the if .Values.customConfig gate works |
One correction to something I nearly reported as a bug: my first "removal" test looked like the ConfigMap was lingering. That was my error — helm upgrade with no value flags reuses the previous release's values, so customConfig was never actually cleared. With --reset-values the teardown is clean. The chart is right; my test was wrong.
Telemetry — log reads Telemetry reporting disabled. I specifically checked the regression I was worried about, since /metrics and app_status_recovery_mode both come off the telemetry collector: /metrics still 200 with 60 metric lines, app_info and app_status_recovery_mode both still exported, /telemetry still 200, Prometheus target health: up / lastError: '' / up = 1, one target per release. Nothing broke — consistent with main.rs:405,689, where reporting_enabled gates only the panic hook and the reporter spawn, not the collector.
Recovery mode — QDRANT_ALLOW_RECOVERY_MODE=true present in the rendered env, and QdrantInRecoveryMode now genuinely reaches state: pending with the gauge at 1. No longer inert.
1. Recovery mode is a silent-degradation state, and warning now under-calls it
My original comment argued for QDRANT_ALLOW_RECOVERY_MODE=true and stopped there. I should have followed the consequence through to the alert, because the change moves the OOM outcome from loud to quiet.
I seeded a collection, then booted into recovery mode with data already on disk — the realistic post-OOM shape, rather than the empty-PVC case:
| Signal | Value |
|---|---|
pod Ready |
True |
| restarts | 0 |
| in Service endpoints | yes — traffic routes to it |
/readyz |
200 |
GET /collections/docs |
500 |
| search on existing data | 400 — "Failed to search, Qdrant was killed during initialization. Most likely it's Out-of-Memory." |
| write to existing collection | 400 |
DELETE /collections/docs |
200 — the documented remedy does work |
up |
1, so QdrantDown (critical) does not fire |
| only alert that fires | QdrantInRecoveryMode, severity: warning |
Before this change, an OOM produced CrashLoopBackOff: pod not Ready, out of the Service, QdrantDown firing critical. After it, the database sits Ready, stays in the Service, accepts connections, and fails every read and write against existing collections — with a warning as the only signal.
The env var is still the right call; recovery mode is upstream's designed remedy and being able to DELETE a collection without fighting a crashlooping pod is worth having. It's the alert that no longer matches the state.
Suggested change — alerts.yaml, raise it and say what it actually means:
- alert: QdrantInRecoveryMode
expr: app_status_recovery_mode{namespace="…", service="…"} == 1
for: 2m
labels:
severity: critical
annotations:
summary: 'Qdrant instance {{ .Release.Name }} is in recovery mode'
description: 'Qdrant instance {{ .Release.Name }} booted into recovery mode. It reports Ready and stays in the Service, but every read and write against existing collections fails. Drop a collection to recover, or raise the memory limit.'This matches the repo's own severity convention rather than inventing one: warning is used for capacity and latency (ClickHouseTooManyConnections, MariaDBSlowQueries, KafkaConsumerGroupLag, KafkaUnderReplicatedPartitions), while up-but-broken states are critical — KafkaOfflinePartitions and MariaDBReplicationNotRunning are the closest analogues and both sit at critical.
Worth a line in the README's Monitoring section too, since "Ready but refusing all work" is surprising behaviour to meet for the first time during an incident.
To be explicit about what I'm not suggesting: the readiness probe is the wrong lever. /readyz returning 200 in recovery mode is Qdrant's own behaviour, and you want the pod reachable so an operator can drop the collection. Severity is the fix.
2. Release names ≥38 chars: helm install reports success but no pod is ever created
Not from this commit — it predates it, and I left it out of my first comment. Raising it now because it is free to fix only while the chart is unreleased.
Reproduced at a 47-char release name:
STATUS: deployed
sts: <name>-qdrant 0/1 (forever)
pods: No resources found
event: spec.volumes[0].name: Invalid value:
"<47-chars>-qdrant-persistent-storage": must be no more than 63 characters
leaked PVCs: 1
helm install exits 0, a PVC is provisioned and orphaned, and the only signal is a FailedCreate event. Helm allows release names up to 53 chars; at ≥48 it at least fails loudly on the headless Service name, but 38–47 is the silent window.
Cause is volumeClaimTemplates[].metadata.name: {{ .Release.Name }}-qdrant-persistent-storage, where the release name is redundant — volume names are pod-scoped, and the StatefulSet controller already appends the pod name to make the PVC unique.
volumeClaimTemplates:
- metadata:
name: qdrant-storageThe official Qdrant chart doesn't put the release name in the volume name either, which is why it has no such threshold. clickhouse and postgres here share the bug (clickhouse's threshold is 33), but renaming those now would orphan live PVCs — qdrant is the one chart where it costs nothing. Entirely reasonable to defer as a follow-up if you'd rather keep this PR tight; it just gets more expensive after release.
Regression sweep — clean
helm lint clean. docs/qdrant-v0.0.1.tgz unpacks to exactly the current source (only the intentional v version prefix differs). docs/index.yaml: qdrant added, 0 removed, 0 changed, all 183 digests match their tarballs, genuinely regenerated (0 of 182 created: lines carried over), single tarball. Auth matrix unchanged. Data plane correct — create, upsert, search. API key preserved across upgrade. Umbrella install with postgres still coexists (myapp + myapp-qdrant rules, myapp-postgres + myapp-qdrant monitors).
Nice turnaround on all three — the checksum fix in particular is exactly the shape upstream uses.
Address the latest review (jatintalgotra-zd) on the qdrant chart:
- QdrantInRecoveryMode: raise severity warning -> critical and rewrite the
description. Recovery mode is an up-but-broken state — the pod reports Ready
and stays in the Service (so QdrantDown never fires), yet every read/write
against existing collections fails. This matches the repo's own convention
(KafkaOfflinePartitions, MariaDBReplicationNotRunning are critical). Document
the behaviour in the README Monitoring section.
- volumeClaimTemplate: drop the redundant "{{ .Release.Name }}-qdrant-" prefix
from the volume name (now "qdrant-storage"). Volume names are pod-scoped and
the StatefulSet controller already makes the PVC unique; the prefix pushed the
volume name past the 63-char limit on release names >=38 chars, where
helm install reports success but no pod is ever created. The app selector
label the controller copies onto the PVC still backs the documented
`kubectl delete pvc -l app=<release>-qdrant`.
Repackaged docs/qdrant-v0.0.1.tgz and regenerated docs/index.yaml.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@jatintalgotra-zd thanks for the deep, cluster-verified pass — both items are addressed in 1. Recovery mode severity → You are right that the 2. Release-name length / silent no-pod ( Fixed now while the chart is unreleased. Repackaged On the two recorded non-change-requests: leaving the flat |
# Conflicts: # README.md
There was a problem hiding this comment.
Re-review of a02906e9 — both items fixed, approving
Re-ran the suite on a fresh minikube with the full kube-prometheus-stack. 487a965 is exactly the two changes asked for; a02906e9 merges main cleanly.
Recovery-mode alert — loaded in Prometheus as severity: critical, health: ok, with the new description intact. The README note is the useful half: "reports Ready and stays in the Service, but every read and write against existing collections fails" wasn't discoverable anywhere before.
Volume rename — the PVC no longer carries the release name, so the documented cleanup had to keep working off the label. It does: PVC qdrant-storage-z1-qdrant-0, labels {"app":"z1-qdrant"}, kubectl delete pvc -l app=z1-qdrant deletes it, data lands on the volume, points_count survives a pod delete.
Also confirmed the rename is safe rather than assuming it: v0.0.1 was never published — absent from the live index.yaml, tarball 404s. Renaming a volumeClaimTemplate in a released version would orphan live PVCs.
Silent window closed where it counts (plain helm install, no --wait):
| Release name | helm reports |
Pod |
|---|---|---|
| 44, 45 | STATUS: deployed |
Running |
| 46, 47 | STATUS: deployed |
none (+1 leaked PVC) |
| 48+ | INSTALLATION FAILED |
none — loud, no leak |
38–45 now works; it used to fail silently across all of 38–47.
Two residual notes — neither worth another round
- 46–47 still fails silently, from a different limit: the
controller-revision-hashlabel is<release>-qdrant+-+ 10-char hash, so the StatefulSet name must be ≤52 → release ≤45. This still leaves qdrant ahead of its siblings (clickhouse's longer suffix caps it at 41). Recording the number rather than asking for a fix. - Upgrading from a mid-review commit fails —
volumeClaimTemplatesis immutable, so2d33b2e→a02906e9givesspec: Forbidden: updates to statefulset spec for fields other than 'replicas', … are forbidden. Uninstall/reinstall. Moot after merge; noted so nobody is surprised on their own cluster.
Merge + regressions
The merge brought only README changes from #319. The Qdrant row picked up main's new src/readme.html?id= link format and matches its neighbours, with no Deploy button — correct, that column is APPLICATIONS-only. A table reformat landing on a freshly-added row is a normal place for a merge to go quietly wrong, and this one didn't.
Sweep clean: helm lint passes; telemetry still disabled with all five env vars present; auth matrix intact; data plane correct; API key preserved on upgrade; customConfig checksum still rolls the pod; ServiceMonitor health: up, up = 1; tgz matches source; index.yaml shows qdrant added with 0 removed, 0 changed and all 183 digests matching.
Approving. Scope, so this isn't read as broader than it is: I reviewed the qdrant chart, its packaging, and its behaviour alongside sibling charts in an umbrella release — not the main merge beyond confirming it left this chart alone. The two notes above are knowingly left open.
docs/index.yaml conflicted again because both sides regenerated it wholesale - main gained the qdrant chart (#310) and the README fixes (#319). Resolved the same way: discard both versions and re-run helm repo index . --url https://helm.zop.dev over the merged docs/ directory. Verified the only semantic change against main is the added n8n v0.0.1 entry - 184 tarball digests match, nothing removed, nothing altered, and qdrant v0.0.1 from main is intact. Also adds n8n to the applications table in the top-level README. #319 landed that table with a Deploy column while this branch was open, so without this the chart would merge unlisted. Both of its links were checked live.
3 participants
Description
Adds a new datasource Helm chart for Qdrant — a high-performance vector database for AI workloads (semantic search, recommendations, RAG). This fills a genuine gap in the catalog: we ship ChromaDB (prototype-grade) but had no production vector store, and Qdrant is the on-trend choice teams actually ship.
The chart deploys a single-node Qdrant with persistence, API-key authentication, and built-in Prometheus monitoring — following the richer ClickHouse-style pattern already established in this repo.
Architecture
flowchart TB subgraph ns["Namespace"] subgraph sts["StatefulSet: <release>-qdrant (replicas: 1, non-root uid 1000)"] pod["qdrant/qdrant:v1.19.0<br/>REST :6333 · gRPC :6334<br/>probes: /livez /readyz /healthz"] end pvc[("PVC 10Gi<br/>/qdrant/storage<br/>(data + snapshots + init file)")] sec["Secret<br/><release>-qdrant-apikey-secret<br/>api-key (generated, preserved on upgrade)"] cm["ConfigMap<br/><release>-qdrant-configmap<br/>QDRANT_HOST / HTTP_PORT / GRPC_PORT"] ccm["ConfigMap (optional)<br/>custom-config → /qdrant/config/local.yaml"] svc["Service (ClusterIP)<br/>6333 http · 6334 grpc"] hsvc["Service (headless)<br/>stable StatefulSet identity"] sm["ServiceMonitor<br/>scrapes /metrics with Bearer key"] pr["PrometheusRule<br/>QdrantDown alert"] end pod --- pvc sec -->|QDRANT__SERVICE__API_KEY env| pod ccm -.->|mounted| pod svc --> pod hsvc --> pod sm -->|Authorization: Bearer| svc pr -.->|up == 0| sm app["Application pod"] -->|host + ports from ConfigMap<br/>api-key header| svc prom["Prometheus"] --> smConsuming applications read
QDRANT_HOST/QDRANT_HTTP_PORT/QDRANT_GRPC_PORTfrom the connection ConfigMap and pass the API key (from the Secret) in theapi-keyheader. Qdrant has no databases/users — apps create collections at runtime — so there is no SQL-style init job.What's included
Also packaged as
docs/qdrant-v0.0.1.tgzand added todocs/index.yaml(merged in by hand to avoid churning existing entries' timestamps).Configuration
versionqdrant/qdrant:<version>)v1.19.0diskSize/qdrant/storage10Giresources.requests.cpu/.memory250m/512Miresources.limits.cpu/.memory1000m/2GicustomConfig/qdrant/config/local.yaml""Testing — end-to-end on a real cluster
Deployed on a local minikube cluster (with the Prometheus-Operator CRDs installed to mirror production). This surfaced two real bugs that
helm lintalone would never catch, both now fixed:/qdrant/storagePVC is writable; the image's/qdrant/snapshotsand working dir are root-owned, so Qdrant panicked on startup. Fixed by pinningQDRANT__STORAGE__SNAPSHOTS_PATHandQDRANT_INIT_FILE_PATHonto the PVC./metricsrequires the API key when auth is enabled (only the health endpoints stay open). Fixed by scraping with the key as a Bearer token from the Secret.All 12 cases pass after the fixes:
1/1 Running, clean startup/livez/readyz/healthzopen without key/metricsreturns Prometheus format (authenticated)api-keyheader → 200Authorization: Bearer→ 200customConfighonored (telemetry log flips to disabled)helm uninstallProduct integration
No
kube-management-apiorapp-uichanges required. Themaintainers[0].name: ZopDev+annotations: type: datasourcemetadata makes the chart classify as a datasource via the existing fallback path, and the UI form renders generically fromvalues.schema.json.Type of Change
Checklist
helm lintpasses without errorsdocs/README.md) added