search

banzaicloud / banzai-charts

Curated list of Banzai Cloud Helm charts used by the Pipeline Platform
Apache License 2.0
368 stars 283 forks source link

[Cadence] Allow passing clusterNames instead of hardcoded #1274

Closed longquanzheng closed 3 years ago

longquanzheng commented 3 years ago

Hi team, I saw you recently updated this clusterMetadata section: https://github.com/banzaicloud/banzai-charts/blob/031c49f4f1f542f60a88bb9f5fc56c22a518d773/cadence/templates/server-configmap.yaml#L154

Thanks for doing that. However, I would like to point out that this is not a good template to use.

You probably copied from https://github.com/uber/cadence/blob/40c5f1896565cee5f8f2a67e3ea73342b6fb4d6d/docker/config_template.yaml#L203

However, changing this hardcoded clusterNames would break existing clusters(if they have enabled global domain). https://github.com/banzaicloud/banzai-charts/blame/031c49f4f1f542f60a88bb9f5fc56c22a518d773/cadence/templates/server-configmap.yaml#L157

To be compatible, we need to provide options to change the cluster name from "primary/secondary" to "active" back.

Also, the name "primary/secondary" are not always good names in production.

Ideally they should be something like "ProdClusterA/ProdClusterB" or "ProdClusterDCA1" etc. In Cadence global domain, clusters are equally treated. Only a domain would choose which cluster to be active on. And the domain can be updated to become active in another cluster anytime. So users don't have to use "primary" concepts. We used that in docker-compose just for testing, because there is only one domain for testing.

longquanzheng commented 3 years ago

Also sorta related to https://github.com/banzaicloud/banzai-charts/issues/1275 probably they can be solved at the same time.

pregnor commented 3 years ago

From chart 0.21.0 Cadence cluster names are configurable through the chart.