banzaicloud / banzai-charts

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

Added Cadence clusterMetadata configuration #1278

Closed pregnor closed 3 years ago

pregnor commented 3 years ago
Q A
Bug fix? no
New feature? yes
API breaks? yes
Deprecations? no
Related tickets resolves #1274
License Apache 2.0

What's in this PR?

Added Cadence clusterMetadata configuration. Extended the values.yaml with a corresponding configuration section in order to be able to overwrite the default configuration. Also let the old default configuration be used whenever the values.yaml.server.config.clusterMetadata section is not defined to be backward compatible with old values.yaml files.

Related to #1275, I also fixed the legacy clusterMetadata default values, it was broken since 0.18, because of the conditional set on a raw string of embedded template which always resolved to true.

Why?

It was requested in #1274.

Additional context

Tested with the regular chart update test.

Checklist

longquanzheng commented 3 years ago

Do you have any updates on this? BTW I created a topic about it: https://github.com/uber/cadence/discussions/4356 Hope this can help you understand the config

pregnor commented 3 years ago

Thanks.

No, unfortunately I don't. I have prioritization issues right now. Haven't forgotten it, just can't find the time to finish this.

pregnor commented 3 years ago

@longquanzheng I have done the rework in https://github.com/banzaicloud/banzai-charts/compare/482410871de6c5c05fa0f51a928ce9f996adf696..badb911b28f51ee99b5650d8601a230a27cb4da5. (And fixed a comment in https://github.com/banzaicloud/banzai-charts/compare/badb911b28f51ee99b5650d8601a230a27cb4da5..e62679633c3cd17e00bb255b1487d508a01f13cf.)

I believe this approach is going to work fine for both single and multiple Cadence cluster setups and accounts for release name customization and also currentClusterName for different Cadence/Kubernetes clusters (XDC, multicluster setup).

Also auto-assigns failover version (only the absolute maximum number of clusters ever added to the group has to be fixed beforehand and the increment and initial versions are calculated from there).

publicClient is always going to be set to the currentClusterName's configured rpcAddress, the latter can be customized in the config if needed and defaults to the automatically set up local Kubernetes Cadence frontend service address ({{ .releaseName }}-frontend:{{ .Values.server.frontend.service.port }}). Once the rpcAddress is customized in the values.yaml, it can be set to an assigned clusterIP (k8s-cluster-internal exposure only) or a nodeIP with nodePort configuration which would also make it available from outside the cluster and thus in theory support multicluster setups.

I have also tested these changes with one and two Cadence cluster configs (the latter with only firing up 1 actual cluster to see if the template config is generated well), both with defaults and manual values (even erroneous ones), they looked good in my tests, but if you have the capacity to double check it, that would be appreciated. I'm planning to do a full 2 cluster setup as well with a nodePort service publishing setting.

I have created an issue for extending the service publishing support in https://github.com/banzaicloud/banzai-charts/issues/1284, so load balancers, external names and ingresses may also be supported. Please be advised this is a feature implementation (similarly to the elastic-search chart support) and therefore I cannot provide any ETA. It is certainly not expected in the next couple of weeks and very unlikely for the next couple of months as well - unless heavy interest is indicated by the community - due to the resource constraints and prioritization issues.

longquanzheng commented 3 years ago

@pregnor Thank you for the great work! I have left some comments about ranging map and the frontend service that you mentioned.

longquanzheng commented 3 years ago

When releasing this one, probably should announce that for upgrading , the cluster name value needs to be explicitly defined so that it is backward compatible with existing cluster

pregnor commented 3 years ago

When releasing this one, probably should announce that for upgrading , the cluster name value needs to be explicitly defined so that it is backward compatible with existing cluster

I'm going to add it to the release notes as a note.

pregnor commented 3 years ago

@longquanzheng I'm also adding explanatory upgrade configuration examples to the cadence/README.md in #1279 for the 0.21.0 release in addition to the release tag annotation (kudos to Mark for the idea).