[Bug] persistence.visibility shouldn't be included in 'range' when `elasticsearch.external` store is used

VLZZZ commented 2 weeks ago

What are you really trying to do?

I'm trying to use postgresql as a default store and external Elasticsearch as visibility storage.

Describe the bug

The installation will fail if I use the following configuration on values.yaml (you may notice additional workaround here already, but it's out scope for now)

config:      
  persistence:
    defaultStore: default

    # This is a workaround for broken values structure
    # Where `persistence.sql` key is required
    # https://github.com/temporalio/helm-charts/blob/79d0493fa274c70e6ddfdcf24afa030117f9d898/charts/temporal/templates/server-job.yaml#L90
    sql:
      driver: "postgres12"
      host: '<my_host>'
      port: 5432
      database: "temporal"
      user: '<my_user>'
      password: '<my_password>'
      maxConns: 20
      maxConnLifetime: "1h"
    default:
      driver: "sql"

      sql:
        driver: "postgres12"
        host: '<my_host>'
        port: 5432
        database: "temporal"
        user: '<my_user>'
        password: '<my_password>'
        maxConns: 20
        maxConnLifetime: "1h"

elasticsearch:
  enabled: false
  external: true
  host: <my_elasticsearch_host>
  port: "443"
  version: "v7"
  username: <my_elasticsearch_usernmae>
  password: <my_elasticsearch_password>
  scheme: "https"
  logLevel: "error"

prometheus:
  enabled: false  
grafana:
  enabled: false
cassandra:
  enabled: false
mysql:
  enabled: false

This setup will fail with:

Error: execution error at (helm-temporal/charts/temporal/templates/server-job.yaml:162:24): Please specify cassandra port for visibility store

However as I'm using elasticsearch.external: true the visibilityStore will be set to es-visibility already so I don't expect to define something else (especially something related to the cassandra when using only postgresql/elasticsearch ) https://github.com/temporalio/helm-charts/blob/aca701791eccd128f28dc584e3ecee48c1077795/charts/temporal/templates/server-configmap.yaml#L22C1-L26C15

    {{- if or $.Values.elasticsearch.enabled $.Values.elasticsearch.external }}
      visibilityStore: es-visibility
    {{- else }}
      visibilityStore: visibility
    {{- end }}

However I can mitigate it by faking the section for visibility storage:

config:      
  persistence:
    # This visibility section with SQL driver is not used
    # However it's required because of the chart issues
    # - https://github.com/temporalio/helm-charts/issues/466
    # - https://github.com/temporalio/helm-charts/issues/471
    visibility:
      driver: "sql"

      sql:
        driver: "postgres12"
        host: '<my_host>'
        port: 5432
        database: "temporal"
        user: '<my_user>'
        password: '<my_password>'
        maxConns: 20
        maxConnLifetime: "1h"

Previously I've been using custom workaround for my PoC by simply removing visibility persistence from range function for the server-job.yaml template:

Overview of my git-patch:

---
 charts/temporal/templates/server-job.yaml | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/charts/temporal/templates/server-job.yaml b/charts/temporal/templates/server-job.yaml
index 5e80530..ed72f35 100644
--- a/charts/temporal/templates/server-job.yaml
+++ b/charts/temporal/templates/server-job.yaml
@@ -50,7 +50,7 @@ spec:
           command: ['sh', '-c', 'until cqlsh {{ include "cassandra.host" $ }} {{ .Values.cassandra.config.ports.cql }} -e "SHOW VERSION"; do echo waiting for cassandra to start; sleep 1; done;']
         {{- end }}
         {{- if .Values.schema.createDatabase.enabled }}
-        {{- range $store := (list "default" "visibility") }}
+       {{- range $store := (list "default") }}
         {{- $storeConfig := index $.Values.server.config.persistence $store }}
         {{- if eq (include "temporal.persistence.driver" (list $ $store)) "cassandra" }}
         - name: create-{{ $store }}-store
@@ -82,7 +82,7 @@ spec:
         {{- end }}
         {{- end }}
         {{- else if or (eq (include "temporal.persistence.driver" (list $ "default")) "sql") (eq (include "temporal.persistence.driver" (list $ "visibility")) "sql") }}
-        {{- range $store := (list "default" "visibility") }}
+       {{- range $store := (list "default") }}
         {{- $storeConfig := index $.Values.server.config.persistence $store }}
         {{- if eq (include "temporal.persistence.driver" (list $ $store)) "sql" }}
         - name: create-{{ $store }}-store
@@ -118,7 +118,7 @@ spec:
           []
         {{- end }}
       containers:
-        {{- range $store := (list "default" "visibility") }}
+       {{- range $store := (list "default") }}
         {{- $storeConfig := index $.Values.server.config.persistence $store }}
         - name: {{ $store }}-schema
           image: "{{ $.Values.admintools.image.repository }}:{{ default $.Chart.AppVersion $.Values.admintools.image.tag }}"
@@ -255,7 +255,7 @@ spec:
           []
         {{- end }}
       containers:
-        {{- range $store := (list "default" "visibility") }}
+       {{- range $store := (list "default") }}
         {{- if or (eq $store "default") (eq (include "temporal.persistence.driver" (list $ $store)) "sql") }}
         {{- $storeConfig := index $.Values.server.config.persistence $store }}
         - name: {{ $store }}-schema
--

Minimal Reproduction

Environment/Versions

OS and processor: [e.g. M1 Mac, x86 Windows, Linux]
Temporal Version: helm-chart build from the latest master (aca7017)
Are you using Docker or Kubernetes or building Temporal from source?
Kubernetes

Additional context

robholland commented 3 days ago

This should be fixed by #522 , please test and confirm.

VLZZZ commented 3 days ago

@robholland Thanks for this rework.

I can confirm that with #522 we finally removed all of the in-house workarounds which been made so far.

robholland commented 3 days ago

🎉

On Thu, 4 Jul 2024 at 11:50, Valentin Zayash @.***> wrote:

@robholland https://github.com/robholland Thanks for this rework.

I can confirm that with #522 https://github.com/temporalio/helm-charts/pull/522 we finally removed all of the in-house workarounds which been made so far.

— Reply to this email directly, view it on GitHub https://github.com/temporalio/helm-charts/issues/508#issuecomment-2208677731, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAACKW4U5VTKPQYGYQNWRPLZKUSJBAVCNFSM6AAAAABJSDFFFCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMBYGY3TONZTGE . You are receiving this because you were mentioned.Message ID: @.***>

temporalio / helm-charts