trinodb / charts

Apache License 2.0
142 stars 165 forks source link

Duplicate mount when defining `/etc/trino/catalog` in additionalVolumeMounts #210

Open pr0ton11 opened 3 weeks ago

pr0ton11 commented 3 weeks ago

Seems like a bug when trying to add volumes to the helm chart on the coordinator:

  additionalVolumes:
  - name: {{ kubernetes_trino__catalog_pvc_name }}
    persistentVolumeClaim:
      claimName: {{ kubernetes_trino__catalog_pvc_name }}

  additionalVolumeMounts:
  - name: {{ kubernetes_trino__catalog_pvc_name }}
    mountPath: {{ kubernetes_trino__catalog_mount_path }}
    readOnly: false

Error from server: failed to create typed patch object (kube-trino/trino-coordinator; apps/v1, Kind=Deployment): .spec.template.spec.containers[name="trino-coordinator"].volumeMounts: duplicate entries for key [mountPath="/etc/trino/catalog"]

This is how it is defined in the default values (the volume is defined on both keys):

  additionalVolumes: []
  # coordinator.additionalVolumes -- One or more additional volumes to add to the coordinator.
  # @raw
  # Example:
  # ```yaml
  #  - name: extras
  #    emptyDir: {}
  # ```

  additionalVolumeMounts: []
  # coordinator.additionalVolumeMounts -- One or more additional volume mounts to add to the coordinator.
  # @raw
  # Example:
  #  - name: extras
  #    mountPath: /usr/share/extras
  #    readOnly: true
nineinchnick commented 3 weeks ago

/etc/trino/catalog is always mounted from the configmap, and you can't override it this way. This is not a bug, but it is a reasonable feature request to be able not to mount it. The trick is that there are some default catalogs (tpch and tpcds).

pr0ton11 commented 3 weeks ago

Do I understand correctly, that dynamic catalog management doesn't work with the Helm Chart?

nineinchnick commented 3 weeks ago

It's not being tested, but it should work. Because of the issue I described above, it requires some workarounds, like changing the location of the catalog files in Trino.

See https://github.com/trinodb/charts/pull/178

pr0ton11 commented 3 weeks ago

Ok thanks for your help, do the default catalogs and additional catalogs still work when I redefine this path?

nineinchnick commented 3 weeks ago

Of course not, they're mounted under the original location. You need to manually initialize the volume where catalogs are stored. This could be done using an init container, if the volume is provisioned automatically.

pr0ton11 commented 3 weeks ago

Then I could copy over the catalogs with an init container to the dynamic volume I guess. This should be the simplest solution.

pr0ton11 commented 3 weeks ago

To close this issue here: I post my new configuration:

initContainers:
  coordinator:
    # Workaround for statically defined catalogs
    # Copies over the additional catalogs to the dynamic catalog path
    - name: init-copy-additional-catalogs
      image: "busybox:latest"
      command:
        - "sh"
        - "-c"
        - "cp {{ kubernetes_trino__catalog_default_path }}/*.properties {{ kubernetes_trino__catalog_mount_path }}/"
      volumeMounts:
        - name: catalog-volume
          mountPath: {{ kubernetes_trino__catalog_default_path }}
        - name: {{ kubernetes_trino__catalog_pvc_name }}
          mountPath: {{ kubernetes_trino__catalog_mount_path }}
  coordinatorExtraConfig: |
    catalog.store=file
    catalog.config-dir={{ kubernetes_trino__catalog_mount_path }}
    catalog.read-only=true
additionalConfigProperties:
  - catalog.management=dynamic
nineinchnick commented 3 weeks ago

Let's keep this open, we don't have any other issue to address these default catalogs. I was thinking about introducing a new property for defining all catalogs explicitly.