Skip to content

feat(immich): add Immich chart - #313

Open
Gursewakzopdev wants to merge 5 commits into
mainfrom
feature/immich-chart
Open

feat(immich): add Immich chart#313
Gursewakzopdev wants to merge 5 commits into
mainfrom
feature/immich-chart

Conversation

@Gursewakzopdev

Copy link
Copy Markdown
Contributor

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:

Immich mobile app / web UI
        │
        ▼
  immich-server (2283) ──┬──▶ Postgres (bespoke image w/ vector extension)
        │                └──▶ Redis (zopdev's redis chart)
        ▼
  immich-machine-learning (3003, internal only — face detection/smart search)

Why Postgres is bespoke, not the zopdev postgres chart: 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 zopdev postgres chart 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

  • New feature (new chart)

Testing

Verified end-to-end on a local minikube cluster:

  • helm lint — clean
  • All four components (server, machine-learning, postgres, redis) reach 1/1 Running
  • curl /api/server/ping{"res":"pong"}, web UI (/) → 200
  • Server logs confirm real Postgres/Redis/machine-learning connections (not just its own HTTP server being up) — Nest application successfully started, ML marked healthy by name
  • psql \dt on the Postgres pod shows the full migrated Immich schema (asset, album, activity, etc.)
  • Deleted all three stateful pods (server, postgres, ml) simultaneously — all recovered cleanly, data intact (row counts, schema, ping all still correct)
  • Clean helm uninstall

Two real bugs caught during testing and fixed, not just assumed away:

  • Same PrometheusRule null-label bug as postgres.name (seen in localai/ollama) also hits the redis subchart's alerts.yaml — fixed with an explicit redis.name: immich in values.yaml.
  • The machine-learning StatefulSet had a livenessProbe but no startupProbe. Cold-start model loading took long enough that liveness killed the container before it ever finished starting. Added a startupProbe with a generous budget, matching the pattern already used for server.

Checklist

  • I have performed a self-review of my code
  • helm lint passes without errors
  • My changes generate no new warnings
  • I have updated documentation accordingly (chart README, root README, docs/index.yaml)

🤖 Generated with Claude Code

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.

@arunesh-j arunesh-j left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 lint clean; defaults render; renders with ingress on,
    with TLS, and the ingress.host guard 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_HOST output.
  • Labels match localai and holmesgpt, 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/ping 200, /api/server/version reports 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).
@Gursewakzopdev

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review — addressed in ef8271f:

  1. [Blocking] Uninstall + reinstall bricks the database — fixed: the postgres password Secret is now annotated helm.sh/resource-policy: keep, matching the PVC's own retention, so lookup finds the same password on reinstall. Also added postgres.existingSecret/postgres.password to pin the credential outside the release lifecycle. Reproduced your exact repro (uninstall, then reinstall, same namespace, no volume deletion) — postgres now comes back with 0 restarts and the same data, instead of permanently failing auth.
  2. [Blocking] redis missing from values.schema.json — fixed, added with "category": "advanced", matching how outline/superset declare redis and localai/litellm declare postgres.
  3. [Should fix] Prerequisites omit the Prometheus Operator CRDs — fixed, added to the README.
  4. [Should fix] Server crash-loops on every fresh install — fixed: added a wait-for-deps init container (busybox, nc -z) that waits for both redis and postgres to actually accept a connection before the server starts, absorbing the DNS-not-ready race on the redis headless Service. Verified: 0 restarts on a fresh install (previously 2–5).
  5. [Should fix] Uninstall command leaves the redis volume behind — fixed, README now lists all four PVCs.
  6. [Should fix] Postgres mounts the PVC directly at PGDATA — fixed: PGDATA now points at a subdirectory of the mount instead of the mount root, per the official postgres image's own guidance. Not reproducible on minikube's hostpath volumes as you noted, but applied per your diagnosis.
  7. [Note] Icon is the only off-domain icon — acknowledged, not fixed yet, same as the icon situation on feat(ollama): add Ollama chart #312 — no upload access to the zopdev bucket; will be swapped separately.
  8. [Note] category on nested schema properties — fixed, removed from all 7 nested occurrences; only top-level properties carry it now.
  9. [Note] 13 schema leaves with no description — fixed, all now have one.
  10. [Note] Default footprint large — fixed, added a Minimum node size note to the README with the actual summed request/limit figures.

Also proactively applied the same v-prefix packaging fix as #312 (same underlying issue, just not flagged in this review).

Re-verified all of the above on minikube, including the full uninstall → reinstall cycle.

@jatintalgotra-zd jatintalgotra-zd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 .immich marker 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-v2 microarchitecture 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.
@Gursewakzopdev

Copy link
Copy Markdown
Contributor Author

Thanks for the review — verified all 9 findings against the code first, then addressed in d555cae:

  1. Ingress will reject uploads — fixed: ingress.annotations now defaults to the ingress-nginx annotations Immich's own reverse-proxy docs recommend (proxy-body-size: "0", proxy-request-buffering: "off", 600s read/send timeouts). No-ops on a non-nginx controller.
  2. Name length overflow — fixed: serverFullname/mlFullname/postgresFullname now re-truncate to 63 after appending their suffix, and the -headless Service names are computed once via new helpers so a StatefulSet's serviceName can't drift from its Service's metadata.name. Verified by rendering with a release name at Helm's own 53-char max — every Service/StatefulSet this chart owns comes out at exactly 63.
  3. Postgres has no startupProbe — fixed: added one (pg_isready, matching readiness/liveness), budget configurable via postgres.startupProbe.
  4. Startup probe budgets too tight, not configurable — fixed: server.startupProbe/machineLearning.startupProbe/postgres.startupProbe are now values, defaulted to 300s/600s/180s.
  5. postgres.password editable in UI — fixed: editDisabled: true, matching username/database.
  6. Generated password drifts under helm template/GitOps — documented rather than "fixed": I initially tried gating on .Release.IsInstall (matching charts/qdrant's secret), but that broke the documented uninstall→reinstall password retention — a reinstall after helm uninstall is genuinely IsInstall: true from Helm's perspective even though the PVC/Secret survived, so it started generating a fresh password against the still-initialized volume. Caught this by re-running the uninstall/reinstall cycle on minikube, reverted to the original unconditional lookup check (only simplified: no more decode/re-encode round trip), and documented the residual helm template-only limitation in the README instead.
  7. No postgres env passthrough — fixed: added postgres.env, same pattern as server.env.
  8. Postgres memory below documented floor — fixed: request/limit raised to 2Gi/3Gi.
  9. README gaps — fixed: added the library-PVC/database pairing rule and the x86-64-v2 ML requirement, both confirmed word-for-word against Immich's own docs before writing them in.

Re-verified end-to-end on minikube: fresh install, postgres.env passthrough, ingress annotations, and the uninstall→reinstall password-retention cycle (re-ran it twice after catching the regression above — password matches before/after both times, server authenticates and responds).

@jatintalgotra-zd jatintalgotra-zd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants