temporalio
[Bug] persistence.visibility shouldn't be included in 'range' when `elasticsearch.external` store is used
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 Thanks for this rework.
I can confirm that with #522 we finally removed all of the in-house workarounds which been made so far.
🎉
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: @.***>