Duplicate mount when defining /etc/trino/catalog in additionalVolumeMounts

[pr0ton11]

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]

/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]

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

[nineinchnick]

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 #178

[pr0ton11]

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

[nineinchnick]

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]

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

[pr0ton11]

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]

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.

[sdaberdaku]

@nineinchnick I am very interested in the dynamic catalog support feature. Can I help you with this somehow?

[nineinchnick]

Sure! We first need to refactor the chart a bit to allow not adding any default catalogs like tpch here: https://github.com/trinodb/charts/blob/main/charts/trino/templates/configmap-catalog.yaml#L10 - ideally in a backward-compatible way. Then we can avoid mounting the catalog configmap if no static catalogs are defined, and instead allow mounting a volume.

I'd be careful with the chart providing any specific options for the PVs, since it's a very generic Kubernetes feature, so let's try working on this in small increments.

[sdaberdaku]

So to understand, the issue here is that Trino should have read-write permissions in the folder where the catalogs are located. If I mount the configmap-catalog as using subPath in the catalogs folder, where I will also mount my PV, would this work? The rest of the folder would retain read-write permissions while the mounted catalog would be read only. Would Trino need write permissions on this file for any reason?

[nineinchnick]

We don't want to mix mounting the configmap and a volume at the same path.

[sdaberdaku]

If you don't mind me asking, why not? Is it considered a bad practice?

[nineinchnick]

We don't have a reason for this and it's to keep things simple.

[sdaberdaku]

I am thinking about my use-case, where I would like to have a set of "default" catalogs defined in the chart values, and then be able to also add new catalogs dynamically. Or is it too specific?

In this regard I have a question: if I enable the dynamic catalogs feature, are the catalogs present in the catalog.config-dir "visible" to Trino if created or changed externally during runtime? Say I have a sidecar container that copies a set of catalog files from a configmap to the PV mounted on catalog.config-dir, would Trino be able to pick those up if they change during runtime, or this happens only on startup?

Regarding the default catalogs, we could move them from hardcoded in the configmap to the default value of the additionalCatalogs variable in values.yaml. Users can then decide if they want to keep them or not.

[nineinchnick]

Maybe create a separate issue for it.