Refactor to have better common-library reuse

[bo0tzz]

This PR tries to remove all the duplication from copy-pasting the common-library chart. It thus closes #15. It also adds Typesense, closing #18.

I've made a start on #3, and persistence object for the various caches can now be created by the chart. The main library volume still isn't auto-created as I'm not yet sure how to handle specifying that once and using it multiple times across the various components.

Any comments are welcome as I've never really made a helm chart before, so I don't entirely know what I'm doing 😅.

Merging this would be a breaking change, but as we're still on 0.x for the chart version I think that's OK.

[rbo]

I switched from tag immich-0.0.4 to the refactor branch and have the following problems:

proxy

/docker-entrypoint.sh: /docker-entrypoint.d/ is not empty, will attempt to perform configuration
/docker-entrypoint.sh: Looking for shell scripts in /docker-entrypoint.d/
/docker-entrypoint.sh: Launching /docker-entrypoint.d/10-listen-on-ipv6-by-default.sh
10-listen-on-ipv6-by-default.sh: info: Enabled listen on IPv6 in /etc/nginx/conf.d/default.conf
/docker-entrypoint.sh: Sourcing /docker-entrypoint.d/15-set-env-variables.envsh
/docker-entrypoint.sh: Launching /docker-entrypoint.d/20-envsubst-on-templates.sh
20-envsubst-on-templates.sh: Running envsubst on /etc/nginx/templates/default.conf.template to /etc/nginx/conf.d/default.conf
/docker-entrypoint.sh: Launching /docker-entrypoint.d/30-tune-worker-processes.sh
/docker-entrypoint.sh: Configuration complete; ready for start up
2023/04/02 11:33:09 [emerg] 1#1: host not found in upstream "immich-server:3001" in /etc/nginx/conf.d/default.conf:15
nginx: [emerg] host not found in upstream "immich-server:3001" in /etc/nginx/conf.d/default.conf:15

Service names with tag immich-0.0.4

 oc get svc
NAME                              TYPE        CLUSTER-IP       EXTERNAL-IP   PORT(S)    AGE
app-immich-chart-redis-headless   ClusterIP   None             <none>        6379/TCP   6h2m
app-immich-chart-redis-master     ClusterIP   172.30.14.95     <none>        6379/TCP   6h2m
app-immich-postgresql-chart       ClusterIP   172.30.9.200     <none>        5432/TCP   6h2m
app-immich-postgresql-chart-hl    ClusterIP   None             <none>        5432/TCP   6h2m
immich-machine-learning           ClusterIP   172.30.190.16    <none>        3003/TCP   25s
immich-proxy                      ClusterIP   172.30.59.214    <none>        8080/TCP   25s
immich-server                     ClusterIP   172.30.87.182    <none>        3001/TCP   25s
immich-web                        ClusterIP   172.30.125.250   <none>        3000/TCP   25s

Service names with tag refactor

$ oc get svc
NAME                                TYPE        CLUSTER-IP       EXTERNAL-IP   PORT(S)    AGE
app-immich-chart-machine-learning   ClusterIP   172.30.59.30     <none>        3003/TCP   7m48s
app-immich-chart-proxy              ClusterIP   172.30.226.25    <none>        8080/TCP   7m48s
app-immich-chart-redis-headless     ClusterIP   None             <none>        6379/TCP   6h1m
app-immich-chart-redis-master       ClusterIP   172.30.14.95     <none>        6379/TCP   6h1m
app-immich-chart-server             ClusterIP   172.30.49.182    <none>        3001/TCP   7m48s
app-immich-chart-web                ClusterIP   172.30.107.116   <none>        3000/TCP   7m48s
app-immich-postgresql-chart         ClusterIP   172.30.9.200     <none>        5432/TCP   6h1m
app-immich-postgresql-chart-hl      ClusterIP   None             <none>        5432/TCP   6h1m

It looks like the name generation in my setup with argocd changed a bit.

My values

configuration for the helm chart, configured/deployed via ArgoCD: https://github.com/rbo/gitops-cluster/blob/6e4ee0c887b88fa995451d11fdfa0d6a9b67d5de/apps/onlogic/immich/Application/app-immich-chart.yaml

server

[Nest] 1  - 04/02/2023, 11:35:01 AM   ERROR [TypeOrmModule] Unable to connect to the database. Retrying (1)...
Error: getaddrinfo ENOTFOUND app-immich-chart-postgresql
    at GetAddrInfoReqWrap.onlookup [as oncomplete] (node:dns:71:26)
[Nest] 1  - 04/02/2023, 11:35:04 AM   ERROR [TypeOrmModule] Unable to connect to the database. Retrying (2)...
Error: getaddrinfo ENOTFOUND app-immich-chart-postgresql
    at GetAddrInfoReqWrap.onlookup [as oncomplete] (node:dns:71:26)
[Nest] 1  - 04/02/2023, 11:35:07 AM   ERROR [TypeOrmModule] Unable to connect to the database. Retrying (3)...
Error: getaddrinfo ENOTFOUND app-immich-chart-postgresql
    at GetAddrInfoReqWrap.onlookup [as oncomplete] (node:dns:71:26)
[Nest] 1  - 04/02/2023, 11:35:11 AM   ERROR [TypeOrmModule] Unable to connect to the database. Retrying (4)...
Error: getaddrinfo ENOTFOUND app-immich-chart-postgresql
    at GetAddrInfoReqWrap.onlookup [as oncomplete] (node:dns:71:26)
[Nest] 1  - 04/02/2023, 11:35:14 AM   ERROR [TypeOrmModule] Unable to connect to the database. Retrying (5)...
Error: getaddrinfo ENOTFOUND app-immich-chart-postgresql
    at GetAddrInfoReqWrap.onlookup [as oncomplete] (node:dns:71:26)

common_env.DBHOSTNAME is configured but it looks like ignored:

oc get pod app-immich-chart-server-58749c86b4-9xw9b -o yaml | grep -A1 DB_HOSTNAME
    - name: DB_HOSTNAME
      value: app-immich-chart-postgresql

[bo0tzz]

I haven't yet looked into getting the service names right, so it's expected that that'll be broken right now. I need to see if there's a way to extract the generated service name from inside the common-library call, or whether I'll just need to "guess" at the right value.

[rbo]

Looks like this solved the service naming problem.

Next problem I ran into, it cannot create the microservice service because port is 0:

apiVersion: v1
kind: Service
metadata:
  labels:
    app.kubernetes.io/instance: app-immich-chart
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/name: microservices
    app.kubernetes.io/service: app-immich-chart-microservices
    app.kubernetes.io/version: v1.30.0_46-dev
    helm.sh/chart: immich-0.0.4
  name: app-immich-chart-microservices
  namespace: photos
spec:
  ports:
    - name: http
      port: 0
      protocol: TCP
      targetPort: http
  selector:
    app.kubernetes.io/instance: app-immich-chart
    app.kubernetes.io/name: microservices
  type: ClusterIP

[rbo]

Fixed, disabled the k8s service for the microservice I guess that's not needed: https://github.com/rbo/immich-charts/commit/2c19a03a021d38323f808e600334ff694a8b9a67

[rbo]

To fix the Postgres hostname problem, I renamed common_env into env: https://github.com/rbo/gitops-cluster/commit/1db08365569c7306dc2c3ba620e8c2eac0a61d69

[pcmid]

Can we use a StatefulSet instead of a Deployment to automatically create the main volume?

[bo0tzz]

Thanks for the tweaks @rbo, I've included them in the chart now.

@pcmid the main library volume needs to be shared between multiple deployments, so I don't think using an sts would help.

[rbo]

@bo0tzz thanks

Now I'm working the storage/persistence, for me, the defaults are not very handy. My idea, specify a global upload volume and disable at the component where we don't need this.

@bo0tzz which component needs the shared upload volume? All except proxy? Right?

[rbo]

@bo0tzz by default typesense.enabled: false does it make sense to add TYPESENSE_ENABLED: false as well? Because with typesense.enabled: false without TYPESENSE_ENABLED: false the server won't start.

[bo0tzz]

I agree it's unlikely that people will use the chart defaults for the library volume. It might be better to just enforce entering an existingClaim, I was already experimenting with that a little bit yesterday.

The library needs to be mounted to the server, microservices, and machine-learning deployments, all at /usr/src/app/upload.

I've fixed the typesense stuff, thanks for pointing that out!

[pcmid]

I really think there is a need for a pvc that is not managed by helm, so that the data can still be retained after helm uninstall

[bo0tzz]

I experimented with having an existingClaim defined in the values.yaml, that is then included from the hardcodedValues sections in the templates like so:

persistence:
  library:
    enabled: true
    mountPath: /usr/src/app/upload
    existingClaim: {{ .Values.immich.persistence.library.existingClaim }}

Unfortunately, this ends up overriding the persistence sections that specify the cache volumes and such. I'm not sure how to combine the two.

[rbo]

I was thinking something like that:

persistence:
  # Main data store for all photos shared between different components.
  library:
    enabled: true
    mountPath: /usr/src/app/upload
    accessMode: ReadWriteOnce
    storageClass: lvms-vg1
    size: 100Gi

postgresql:
  persistence:
    library:
      enabled: false

redis:
  persistence:
    library:
      enabled: false

typesense:
  persistence:
    library:
      enabled: false

server:
  persistence:
    library:
      enabled: false

microservices:
  persistence:
    library:
      enabled: false

machine-learning:
  persistence:
    library:
      enabled: true
      type: pvc
      mountPath: /usr/src/app/upload
      existingClaim: '{{ printf "%s-library" .Release.Name }}'

web:
  persistence:
    library:
      enabled: false

proxy:
  persistence:
    library:
      enabled: false

Does not work:

Error: YAML parse error on immich/templates/machine-learning.yaml: error converting YAML to JSON: yaml: invalid map key: map[interface {}]interface {}{"printf \"%s-library\" .Release.Name":interface {}(nil)}

[bo0tzz]

Something like that would be ideal, but unfortunately I can't get it to merge correctly with the other persistence entries.

[pcmid]

I experimented with having an existingClaim defined in the values.yaml, that is then included from the hardcodedValues sections in the templates like so:

persistence:
  library:
    enabled: true
    mountPath: /usr/src/app/upload
    existingClaim: {{ .Values.immich.persistence.library.existingClaim }}

Unfortunately, this ends up overriding the persistence sections that specify the cache volumes and such. I'm not sure how to combine the two.

I tried this, and I didn't find such a case in the yaml file of the inspection result.

# Source: immich/templates/microservices.yaml
apiVersion: apps/v1
kind: Deployment
metadata:
  name: immich-microservices
  labels:
    app.kubernetes.io/instance: immich
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/name: microservices
    app.kubernetes.io/version: v1.30.0_46-dev
    helm.sh/chart: immich-0.0.4
spec:
  revisionHistoryLimit: 3
  replicas: 1
  strategy:
    type: Recreate
  selector:
    matchLabels:
      app.kubernetes.io/name: microservices
      app.kubernetes.io/instance: immich
  template:
    metadata:
      labels:
        app.kubernetes.io/name: microservices
        app.kubernetes.io/instance: immich
    spec:

      serviceAccountName: default
      automountServiceAccountToken: true
      dnsPolicy: ClusterFirst
      enableServiceLinks: true
      containers:
        - name: immich-microservices
          image: "ghcr.io/immich-app/immich-server:v1.52.1"
          imagePullPolicy: IfNotPresent
          command:
            - /bin/sh
          args:

            - ./start-microservices.sh
          env:
            - name: DB_DATABASE_NAME
              value: immich
            - name: DB_HOSTNAME
              value: immich-postgresql
            - name: DB_PASSWORD
              value: immich
            - name: DB_USERNAME
              value: immich
            - name: IMMICH_MACHINE_LEARNING_URL
              value: http://immich-machine-learning:3001
            - name: IMMICH_SERVER_URL
              value: http://immich-server:3001
            - name: IMMICH_WEB_URL
              value: http://immich-web:3000
            - name: NODE_ENV
              value: production
            - name: REDIS_HOSTNAME
              value: immich-redis-master
            - name: REVERSE_GEOCODING_DUMP_DIRECTORY
              value: /geodata-cache
            - name: TYPESENSE_API_KEY
              value: typesense
          volumeMounts:
            - name: geodata-cache
              mountPath: /geodata-cache
            - name: library
              mountPath: /usr/src/app/upload

      volumes:
        - name: geodata-cache
          persistentVolumeClaim:
            claimName: immich-microservices-geodata-cache
        - name: library
          persistentVolumeClaim:
            claimName: test-pvc

[pcmid]

Here is my case. https://github.com/immich-app/immich-charts/compare/refactor...pcmid:immich-charts:refactor

[bo0tzz]

Yup, turns out the reason I wasn't seeing the other volumes was because I forgot to enable them, lol. It does indeed work fine. Thanks!

I've added in a check that fails the templating if persistence.library.existingClaim is not set, to enforce externally creating a PVC.

Left to do is (at least):

• Adding startup/liveness probes
• Maybe adding some default resource requests?
• Currently the env values are applied to all deployments, even where they aren't needed. It would be cleaner to appropriately set them where necessary.

[pcmid]

It's a bit confusing whether to use the library volume absolutely in each component, it would be better to add a persistent block only for hardcodedValues ​​in the components that need it.

[bo0tzz]

I tried disabling the library in hardcodedValues where applicable, but that then got overridden by the entry from the values.yaml root.

[pcmid]

@bo0tzz I added persistence.library to hardcodedValues in server, microservices, and machine-learning deployments, it seems to work fine.

Now the pods are waiting for the library volume to be created.

[bo0tzz]

Aha, I see what you mean.

[pcmid]

There is a problem here, the default immich.persistence.library.existingClaim must exist, preferably empty, otherwise executing helm install will panic.

[pcmid]

Should we add some example annotations for ingress? Such as cert-manager.io/cluster-issuer or nginx.ingress.kubernetes.io/proxy-body-size. Maybe custom ingress class expamle is equally important.

[bo0tzz]

The panic is deliberate, to force users to specify the existingClaim. I think that makes sense?

I don't think we need to add example annotations as people will know when they need them, however we should indeed add nginx.ingress.kubernetes.io/proxy-body-size: "0". Having that then immediately makes it clear where all the other annotations should go as well.

[pcmid]

We already have checks, the extra panic might be confusing as well.

[bo0tzz]

Gotcha. Sorry for the back and forth, I'm learning as we go 😅

[bo0tzz]

IMO this PR is ready to go, although it could probably do with more testing.

[pcmid]

I have a question, should we keep the tsdata volume of type sense in helm install? If so, we can add annotations such as

annotations:
  helm.sh/resource-policy: "keep"

under typesense.persistence.tsdata .

Or we can also use the retain option, ref: retain.

[rbo]

Stupid question, why .Values.immich.persistence.library.existingClaim and not .Values.persistence.library.existingClaim?

[bo0tzz]

should we keep the tsdata volume of type sense in helm install?

The only data that is essential to keep is the library volume and the postgres database. The typesense data is derived from postgres and can be regenerated when reinstalling/restoring from backup/etc.

why .Values.immich.persistence.library.existingClaim and not .Values.persistence.library.existingClaim?

Using a top level persistence key causes that to be applied to every component via the common-library. Using an (arbitrarily chosen) immich key gives a namespace that isn't used by the library.

(edit: I did initially use a top level persistence key for the library volume, but changed that in https://github.com/immich-app/immich-charts/pull/20/commits/8946f0baa95e4abdcbbaf3d194b30c88c927963f)

[rbo]

should we keep the tsdata volume of type sense in helm install?

The only data that is essential to keep is the library volume and the postgres database. The typesense data is derived from postgres and can be regenerated when reinstalling/restoring from backup/etc.

why .Values.immich.persistence.library.existingClaim and not .Values.persistence.library.existingClaim?

Using a top level persistence key causes that to be applied to every component via the common-library. Using an (arbitrarily chosen) immich key gives a namespace that isn't used by the library.

(edit: I did initially use a top level persistence key for the library volume, but changed that in 8946f0b)

Got it, then I would like to use a variable name complete outside of commons library usage. But I don't have any good ideas or examples.

[rbo]

Updated my deployment with the latest version of refactor branch with following values:

        dnsConfig:
          options:
            - name: ndots
              value: "1"
        immich:
          persistence:
            # Main data store for all photos shared between different components.
            library:
              # Automatically creating the library volume is not supported by this chart
              # You have to specify an existing PVC to use
              existingClaim: app-immich-chart-upload
        redis:
          enabled: true
        postgresql:
          enabled: false
        typesense:
          enabled: true
          persistence:
            tsdata:
              enabled: true
              accessMode: ReadWriteOnce
              size: 8Gi
              storageClass: lvms-vg1

        env:
          DB_HOSTNAME: "app-immich-postgresql-chart.photos.svc.cluster.local."
          DB_PASSWORD:
            valueFrom:
              secretKeyRef:
                name: immich-postgresql
                key: password
          JWT_SECRET:
            valueFrom:
              secretKeyRef:
                name: immich-jwt
                key: password

        machine_learning:
          persistence:
            cache:
              enabled: true
              size: 10Gi
              type: pvc
              accessMode: ReadWriteOnce
              storageClass: lvms-vg1

Works like charm!

[rbo]

Comment deleted, my fault. Ingress works like a charm.

[PixelJonas]

Wow! I want to express my sincere gratitude to each and every one of you for your valuable contributions to this.

Your efforts have truly made a significant impact on the chart, and I am thrilled to see how @bjw-s common-library has been utilized to enhance it. This will make future contributions easier. Thank you again for your hard work and dedication! @rbo @bo0tzz @pcmid @onedr0p

LGTM