feat(immich): add Immich chart - #313
Conversation
Adds a photo/video backup chart deploying all four components from Immich's own reference architecture: immich-server, immich-machine- learning, a bespoke Postgres StatefulSet, and the zopdev redis chart. Postgres can't reuse the zopdev postgres chart -- Immich's search features need a vector extension baked into the image (ghcr.io/immich-app/postgres), which the shared bitnami-based chart doesn't have. Env vars, health endpoints, and image tags were verified against Immich's actual source rather than assumed. Verified end-to-end on minikube: all four components reach Ready, /api/server/ping and the web UI respond, server logs confirm real Postgres/Redis/machine-learning connections (not just HTTP up), Postgres has the full migrated schema, and all three stateful pods recover cleanly with data intact after a simultaneous restart. Two bugs caught and fixed during testing: - Same PrometheusRule null-label issue as postgres (fixed via postgres.name) hits the redis subchart too -- fixed with redis.name. - The machine-learning StatefulSet had no startupProbe, so its liveness probe was killing the container before cold-start model loading finished. Added one, matching the server's pattern. Also packages the chart to docs/immich-v0.0.1.tgz, regenerates docs/index.yaml, and lists immich in the root README's Applications table, matching #307/#312.
17b41b3 to
7364817
Compare
arunesh-j
left a comment
There was a problem hiding this comment.
Review — feat(immich): add Immich chart (#313)
Reviewed on a worktree at 7364817, base origin/main. CI reproduced, chart installed
and uninstalled/reinstalled on a local minikube.
This is a careful chart. The wiring was transcribed faithfully from upstream's own
reference deployment — I checked /data, /cache, /var/lib/postgresql/data, the
128 MiB /dev/shm, POSTGRES_INITDB_ARGS: --data-checksums and the postgres image tag
against immich-app/immich@v3.1.0's docker/docker-compose.yml and they all match. All
three image tags exist in ghcr.io, and v3.1.0 is genuinely the current upstream release.
The comment claiming the redis chart publishes this exact hostname is correct — its
service-config-map really does emit REDIS_HOST: <release>-redis-headless-service.
Packaging and publishing are right: index regenerated with the real tool (0% carried-over
timestamps), added: 1, removed: 0, changed: 0, all 183 digests verified, v-prefixed
tarball, Chart.lock committed and no resolved subchart tarball. The root README.md
table was updated, in the correct section — the step that was missed on #310.
Two blocking items, four worth fixing before merge.
1. [Blocking] Uninstall + reinstall permanently bricks the database
charts/immich/templates/postgres-secret.yaml:1-15
The generated postgres password lives only in the Secret. helm uninstall deletes the
Secret but the PVCs survive — which the chart's own README.md:45-52 documents as
intended. On reinstall, lookup finds nothing, so a fresh random password is generated,
while the retained volume still holds the old role password. Postgres skips
initialization, and the server can never authenticate again.
Reproduced on the exact documented path (helm uninstall then helm install, same
namespace, no volume deletion):
after uninstall: 4 PVCs Bound, secret r313b-immich-postgres-secret -> NotFound
postgres: "Database directory appears to contain a database; Skipping initialization"
postgres: FATAL: password authentication failed for user "immich" (x5)
server: PostgresError: password authentication failed for user "immich", code 28P01
4 restarts, never Ready
This hits every operator who reinstalls while keeping their photo library — which is the
whole reason the library PVC is retained. There is no escape hatch: no
postgres.existingSecret and no postgres.password.
lookup does correctly cover helm upgrade; it is only uninstall/reinstall that breaks.
Fix: add a postgres.existingSecret (and/or explicit postgres.password) so the
credential can be pinned outside the release lifecycle — charts/localai already exposes
externalDatabase for the same reason. Pairing that with
helm.sh/resource-policy: keep on the Secret would make its lifetime match the PVC it is
tied to. At minimum, the README's uninstall section has to say that the postgres PVC and
the Secret must be deleted together or kept together, never split.
2. [Blocking] redis is in values.yaml but not in values.schema.json
charts/immich/values.yaml:16-18, charts/immich/values.schema.json
redis is the only top-level values key with no schema property, so it is unvalidated and
invisible in the zop.dev config UI. verify-values.py flags it blocking.
It is load-bearing, not decorative: redis.name: immich is what stops the redis chart
rendering service: null in its PrometheusRule, which the CRD rejects and which fails the
whole release. I confirmed the workaround works (the rendered rule carries
service: immich) — but a user cannot see or safely re-set the field it depends on.
Every other chart with a subchart block declares it: outline and superset both declare
redis; localai and litellm declare postgres. Add it with "category": "advanced".
3. [Should fix] Prerequisites omit the Prometheus Operator CRDs
charts/immich/README.md:12-15
Prerequisites list only Kubernetes 1.19+ and Helm 3+. The redis subchart renders
PrometheusRule and ServiceMonitor unconditionally — no enabled flag in redis
v0.0.5, and no condition: on the dependency in Chart.yaml, so there is no way to opt
out. On any cluster without monitoring.coreos.com CRDs, helm install fails outright.
Verified by render (both objects present in the default output). I did not reproduce the
install failure, because the test cluster already had the CRDs from earlier work.
State the CRDs as a prerequisite, as the trap is otherwise invisible until install time.
4. [Should fix] The server crash-loops on every fresh install
charts/immich/templates/server-statefulset.yaml:22-45
The server exits with code 1 rather than waiting for its dependencies, so a fresh install
always burns restarts. Measured 2–5 restarts across three installs before Ready — on a
healthy cluster with images already cached it was still 2.
Two distinct causes, both from container logs:
Error: getaddrinfo ENOTFOUND r313-redis-headless-service
microservices worker error: MaxRetriesPerRequestError: Reached the max retries per request limit (which is 20)
microservices worker exited with code 1 / Killing api process
and, once redis was up, MetadataService.init failing against postgres.
The DNS failure is worth calling out specifically: a headless Service has no A record at
all until it has a ready endpoint, and the redis chart's headless Service does not set
publishNotReadyAddresses. So the server gets NXDOMAIN, not a connection refusal, and
ioredis treats that as fatal after 20 tries. (This chart's own headless Services do set
publishNotReadyAddresses: true — the one that matters is the subchart's.)
charts/n8n/templates/deployment.yaml solves the same class of problem with an init
container that blocks until the dependency actually authenticates. Doing that here would
make the install clean. It converges either way, so this is not blocking — but a fresh
install currently looks broken to whoever is watching it.
5. [Should fix] The uninstall command leaves the redis volume behind
charts/immich/README.md:49-52
The release creates four PVCs; the documented cleanup deletes three. Missing:
<release>-redis-persistent-storage-<release>-redis-0
Verified against the running release. Following the README as written orphans a disk that
keeps billing.
6. [Should fix] Postgres mounts the PVC directly at PGDATA
charts/immich/templates/postgres-statefulset.yaml:43-45
mountPath: /var/lib/postgresql/data, and I confirmed from the image config that
PGDATA=/var/lib/postgresql/data — the mount root is the data directory. On an
ext4-formatted cloud volume (GCE PD, EBS) that directory comes up containing lost+found,
and initdb refuses a non-empty target, so postgres never initializes. The official
postgres image documents exactly this case and prescribes a subdirectory.
In-repo precedent: charts/postgres/templates/statefulset.yaml:126 mounts the parent
(/bitnami/postgresql) and keeps the data in a subdirectory, avoiding this.
Not reproduced — minikube's hostpath volumes have no lost+found, so this passes locally
and fails on the clusters that matter. Fix by setting PGDATA to a subdirectory of the
mount, or mounting with a subPath.
7. [Note] The icon is the only off-domain icon in the repo
charts/immich/Chart.yaml:7
icon: https://raw.githubusercontent.com/immich-app/immich/main/design/immich-logo-stacked-light.png
It returns 200 today, but it is a third-party host and it tracks main, so its contents can
change without anything in this repo moving — and it renders on a zop.dev product surface.
I audited all 33 charts: this is the only one not served from zopdev infrastructure. Recent
charts (clickhouse, litellm, localai, holmesgpt, n8n) use
storage.googleapis.com/zopdev-blog-resources. The asset needs uploading there; a reviewer
cannot do it.
8. [Note] category on nested schema properties
charts/immich/values.schema.json — 7 nested occurrences (server.diskSize,
server.resources, server.env, machineLearning.diskSize, machineLearning.resources,
postgres.diskSize, postgres.resources).
category is the zop.dev form's section key and the repo places it on top-level properties
only. I checked every schema in the repo: immich is the sole chart with nested ones. Harmless
today, but it reads as meaningful when it is not.
9. [Note] 13 schema leaves have no description
All four resources.{requests,limits}.{cpu,memory} leaves under server,
machineLearning and postgres, plus postgres.image.pullPolicy. The reference
implementation (charts/litellm/values.schema.json) describes these
("CPU request", "Memory request"), and they surface in the config UI.
10. [Note] Default footprint is large for a default
Summed across the release: 2.51 CPU / 4.3 GiB requested, 9.55 CPU / 11.1 GiB limits
(ML alone requests 1 CPU / 2 GiB and limits 4 CPU / 4 GiB). A 2-CPU node cannot run this —
it took down the API server on my first attempt. Worth a line in the README stating the
minimum node size, since there is no way to disable the ML component.
Checked and correct
helm dependency update+helm lintclean; defaults render; renders with ingress on,
with TLS, and theingress.hostguard fires with the intended message.- No duplicate kind+name in the release — no collision with the redis subchart.
- Redis hostname helper verified against the subchart's own
REDIS_HOSToutput. - Labels match
localaiandholmesgpt, which is the current convention for new charts. annotations.type: application; root README row in the APPLICATIONS table; 2-space
indents, no hard tabs.- Installed clean on minikube from an empty namespace: Ready in ~80 s,
/api/server/ping200,/api/server/versionreports 3.1.0.
- Postgres password Secret is now kept across `helm uninstall` (helm.sh/resource-policy: keep), matching the PVC's own retention, so a reinstall against the retained volume can still authenticate. Previously a reinstall generated a new random password that no longer matched the already-initialized database, permanently bricking it. Also adds postgres.existingSecret/postgres.password so the credential can be pinned outside the release lifecycle. - Added the missing `redis` entry to values.schema.json -- it was unvalidated and invisible in the config UI despite redis.name being load-bearing (it's what stops the redis subchart rendering a null PrometheusRule label that fails the whole install). - Added a wait-for-deps init container to the server so a fresh install doesn't crash-loop: the redis subchart's headless Service has no DNS record until it has a ready endpoint, so a server started at the same time as redis got NXDOMAIN and exited fatally instead of just retrying. - Postgres now sets PGDATA to a subdirectory of the volume mount instead of the mount root, matching the official postgres image's own guidance -- on real block storage (EBS, GCE PD) the mount root already contains lost+found, and initdb refuses a non-empty target. - Documented the Prometheus Operator CRD prerequisite (the redis dependency renders PrometheusRule/ServiceMonitor unconditionally), the redis PVC missing from the uninstall command, and the ~2.5 CPU / 4.3Gi default footprint. - Removed `category` from nested schema properties (only top-level properties carry it elsewhere in the repo) and added descriptions to the 13 schema leaves that were missing one. - Repackaged with the v-prefixed version stamp every other chart in the index uses (v0.0.1), matching the same fix applied to ollama. Verified on minikube: uninstall -> reinstall against retained volumes now reconnects with 0 restarts and intact data (previously bricked); fresh install now reaches Ready with 0 restarts on the server (previously 2-5 crash-loop restarts).
|
Thanks for the thorough review — addressed in
Also proactively applied the same Re-verified all of the above on minikube, including the full uninstall → reinstall cycle. |
# Conflicts: # docs/index.yaml
jatintalgotra-zd
left a comment
There was a problem hiding this comment.
Reviewed against the official Immich chart (immich-0.13.1) and Immich's docs at tag v3.1.0. The env var contracts, ports, probe paths and the /data library mount all check out. A few functional issues:
1. Ingress will reject photo uploads (ingress.yaml, values.yaml)
ingress.annotations defaults to {}. ingress-nginx defaults to proxy-body-size: 1m, proxy-read-timeout: 60, proxy-request-buffering: on (config.go#L37). So any photo over 1 MB returns 413, and videos taking over 60s time out. Immich's reverse proxy docs call for client_max_body_size 50000M, proxy_request_buffering off and proxy_read_timeout 600s, and the official chart ships nginx.ingress.kubernetes.io/proxy-body-size: "0" as a default annotation for exactly this. Suggest defaulting:
ingress:
annotations:
nginx.ingress.kubernetes.io/proxy-body-size: "0"
nginx.ingress.kubernetes.io/proxy-request-buffering: "off"
nginx.ingress.kubernetes.io/proxy-read-timeout: "600"
nginx.ingress.kubernetes.io/proxy-send-timeout: "600"2. Name length overflow (_helpers.tpl)
immich.fullname truncates at 63, but serverFullname/mlFullname/postgresFullname/postgresSecretName append their suffix after that with no re-truncation. A 53-char release name renders <release>-immich-postgres-headless at 78 chars; Service names must be a DNS-1035 label (max 63), so install fails server-side. Break-even is a release name over 37 chars. helm lint and helm template both pass, so CI won't catch it. Needs | trunc 63 | trimSuffix "-" on each of the four.
3. Postgres has no startupProbe (postgres-statefulset.yaml)
livenessProbe has no initialDelaySeconds, so the budget is 6 x 20s = 120s from container start. On a fresh PVC that has to cover initdb plus --data-checksums plus the vchord preload. On slow or throttled storage this crash-loops. Same class of bug the PR already fixed for machine-learning, just not applied here.
4. Startup probe budgets are tighter than upstream's, and not configurable
| this PR | official chart | |
|---|---|---|
| server | 24 x 5s = 120s | 30 x 10s = 300s |
| machine-learning | 36 x 5s = 180s | 60 x 10s = 600s |
Upstream's chart 0.10.0 notes add: "depending on the size of your library you may need to relax startup probes to allow the migration to complete." First install also runs the full schema migration and the VectorChord version check. The values are hardcoded in the templates, so anyone who hits this has no knob.
5. postgres.password is editable but breaks a running release (values.schema.json)
username and database are marked editDisabled: true; password is only mutable: true. Editing it in the UI rotates the Secret, the server picks up the new value, and the already-initialized database still has the old one. Auth breaks with no recovery path. Should be editDisabled: true for consistency with its two neighbours.
6. Generated password rotates under helm template / GitOps (postgres-secret.yaml)
The randAlpha/randNumeric value is recomputed on every render and only falls back to the stored one when lookup returns something. lookup returns nothing under helm template, --dry-run and ArgoCD manifest generation, so each sync shows drift, and applying it rotates the Secret against a volume that still holds the old password. charts/qdrant/templates/secret.yaml guards this with {{- if .Release.IsInstall }}.
7. No env passthrough for postgres (postgres-statefulset.yaml)
The image's immich-docker-entrypoint.sh stats $PGDATA's filesystem and exit 1s unless it is ext2/3/4, xfs, btrfs, zfs, f2fs or tmpfs. On EFS, Filestore or an nfs-subdir provisioner the pod crash-loops with no way to set IGNORE_DATABASE_FSTYPE, since postgres env is fully hardcoded. The same knob would expose DB_STORAGE_TYPE: HDD, which is documented and called out in upstream's compose. A postgres.env map mirroring the existing server.env covers both.
8. Postgres memory request is under the documented floor (values.yaml)
Immich's requirements: "if Docker resource limits are used, the Postgres database requires at least 2GB of RAM." The chart requests 1Gi with a 2Gi limit, so it can schedule onto a node that cannot satisfy it. Suggest raising the request to 2Gi.
9. Two README gaps that are operational hazards, not wording
- Immich refuses to start if a previously written
.immichmarker is missing from/data's subfolders. The README carefully pairs the postgres Secret with the postgres PVC — the same "delete together or keep together" rule applies to the library PVC and the database, and isn't mentioned. - Since v3 the ML container on amd64 requires the
>= x86-64-v2microarchitecture level. Older node pools and some VM CPU models will fail to run it. Worth listing next to the Prometheus CRD prerequisite.
Also worth noting in the description: the official chart removed its bundled postgres and redis subcharts in 0.10.0 and now points at CloudNativePG. Bundling is the right call for this platform, but it makes the divergence look deliberate rather than an oversight.
- Default ingress annotations to lift ingress-nginx's 1m body-size and 60s timeouts, matching Immich's own reverse-proxy guidance for large photo/video uploads. - Truncate the server/ml/postgres fullname helpers (and their derived headless Service names) to 63 chars so long release names can't overflow the DNS-1035 Service name limit -- helm lint/template never catch this, it only fails server-side on install. - Add a startupProbe to the postgres StatefulSet, matching the pattern already used on server and machine-learning, so a slow initdb/vchord preload doesn't get killed by the liveness probe. - Make all three components' startupProbe periodSeconds/failureThreshold configurable via values, with more generous defaults. - Mark postgres.password editDisabled in the schema, consistent with its username/database neighbors -- editing it post-install rotates the Secret without touching the already-initialized database. - Add postgres.env passthrough (e.g. IGNORE_DATABASE_FSTYPE for non-local storage), mirroring the existing server.env/ollama pattern. - Raise postgres' default memory request/limit to 2Gi/3Gi, matching Immich's documented floor. - Document the library-PVC/database pairing rule and the x86-64-v2 machine-learning requirement, both confirmed against Immich's docs. Repackaged docs/immich-v0.0.1.tgz and regenerated docs/index.yaml per CONTRIBUTING.md steps 4-6. Re-verified end-to-end on minikube: fresh install, ingress annotations, postgres.env passthrough, and -- since an earlier attempt at the secret GitOps-drift fix (gating on .Release.IsInstall, matching charts/qdrant) turned out to break the documented uninstall+reinstall password retention -- specifically re-ran that uninstall/reinstall cycle twice to confirm the final version still authenticates against the retained volume with the same password.
|
Thanks for the review — verified all 9 findings against the code first, then addressed in
Re-verified end-to-end on minikube: fresh install, |
jatintalgotra-zd
left a comment
There was a problem hiding this comment.
Re-reviewed d555cae. Ingress annotations, postgres startupProbe, configurable probe budgets, postgres.env, editDisabled on the password, the 2Gi memory floor and both README additions all check out — I rendered each. The probe budgets now match the official chart's exactly (server 300s, ML 600s). Packaging is clean too: the tgz sha256 matches the index digest, tgz content is identical to the branch source, and docs/index.yaml went 183 → 184 digests with nothing lost.
One new issue, and one correction to my own earlier review.
The truncation fix trades the overflow for a name collision (_helpers.tpl)
The component helpers truncate at 63, and the headless helpers then append -headless to that result and truncate at 63 again. Once a component name is already 63 chars, appending and re-truncating returns the same string — so the ClusterIP Service and the headless Service render with identical names:
release name 46 chars → 1 duplicate kind+name pair
release name 49 chars → 2
release name 53 chars → 3 (53 is Helm's max release name)
At 53 chars helm template emits two Services named <release>-immich-se, two named <release>-immich-po, and two named <release>-immich-ml. Install fails on AlreadyExists for the second create, and the StatefulSet's serviceName no longer points at a distinct headless Service. The failure window narrowed from ≥37 chars to ≥46, but it is still a hard failure, and helm lint/helm template still pass.
Truncating the base once, so every suffix is guaranteed to fit, avoids the second truncation entirely:
{{- define "immich.serverFullname" -}}
{{- printf "%s-server" (include "immich.fullname" . | trunc 45 | trimSuffix "-") }}
{{- end }}
{{- define "immich.serverHeadlessFullname" -}}
{{- printf "%s-headless" (include "immich.serverFullname" .) }}
{{- end }}
45 is the largest base that keeps the worst case (-postgres plus -headless, 18 chars) within 63. I tested this at release lengths 46, 49 and 53: longest immich-owned Service name is exactly 63, zero duplicates.
Correction: you were right to reject the .Release.IsInstall gate
My original comment 6 suggested matching charts/qdrant/templates/secret.yaml. That would have been a regression here. helm uninstall followed by helm install against the retained PVC is an install as far as Helm is concerned, so that gate would generate a fresh password and break exactly the retention this chart is designed around. Keeping the lookup-only logic and documenting the residual helm template/GitOps case is the right call. Flagging it so nobody later "fixes" this by reading my earlier comment.
Minor, and conditional on how the Zopday form consumes the schema
values.schema.json declares ingress.annotations with no default, while values.yaml now ships four annotations the chart needs to accept uploads. Every other field in that schema mirrors its values.yaml default. I can't verify how the form handles an object property with no declared default — but if it renders empty and submits {}, the upload-limit annotations get wiped and the 413 comes back silently. Adding the default to the schema closes that off cheaply.
Description
Adds a new
charts/immich/chart deploying Immich, a self-hosted photo/video backup solution (drop-in Google Photos/iCloud alternative, with a matching mobile app).Deploys all four components from Immich's own reference architecture:
Why Postgres is bespoke, not the zopdev
postgreschart: Immich's search features (smart search, duplicate detection) need a vector extension baked into the database image. Upstream ships their own image with it (ghcr.io/immich-app/postgres) — the zopdevpostgreschart runs plain bitnami Postgres, which doesn't have it. Writing a small dedicated StatefulSet for this exact image was simpler and more robust than trying to force an incompatible image into the shared chart's bitnami-specific templates.Env var contracts (
DB_HOSTNAME,IMMICH_MACHINE_LEARNING_URL, etc.), health endpoints (/api/server/ping,/ping), and image tags were all verified directly against Immich's source and GHCR — not assumed.Machine learning has no enable/disable toggle, matching upstream's own reference docker-compose, which always runs it.
Type of Change
Testing
Verified end-to-end on a local minikube cluster:
helm lint— cleanserver,machine-learning,postgres,redis) reach1/1 Runningcurl /api/server/ping→{"res":"pong"}, web UI (/) → 200Nest application successfully started, ML marked healthy by namepsql \dton the Postgres pod shows the full migrated Immich schema (asset,album,activity, etc.)helm uninstallTwo real bugs caught during testing and fixed, not just assumed away:
PrometheusRulenull-label bug aspostgres.name(seen inlocalai/ollama) also hits theredissubchart'salerts.yaml— fixed with an explicitredis.name: immichinvalues.yaml.machine-learningStatefulSet had alivenessProbebut nostartupProbe. Cold-start model loading took long enough that liveness killed the container before it ever finished starting. Added astartupProbewith a generous budget, matching the pattern already used forserver.Checklist
helm lintpasses without errorsdocs/index.yaml)🤖 Generated with Claude Code