Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added Cadence clusterMetadata configuration #1278

Merged
merged 2 commits into from
Aug 18, 2021

Conversation

pregnor
Copy link
Member

@pregnor pregnor commented Aug 2, 2021

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

  • Code meets the Developer Guide
  • User guide and development docs updated (if needed)
  • Related Helm chart(s) updated (if needed)

@pregnor pregnor self-assigned this Aug 2, 2021
@pregnor pregnor force-pushed the feat/parametrize-cadence-cluster-names branch from 6851765 to d4c77e5 Compare August 2, 2021 09:16
cadence/values.yaml Outdated Show resolved Hide resolved
cadence/values.yaml Outdated Show resolved Hide resolved
@pregnor pregnor force-pushed the feat/parametrize-cadence-cluster-names branch 3 times, most recently from ecca195 to e18536f Compare August 2, 2021 20:01
cadence/values.yaml Outdated Show resolved Hide resolved
@pregnor pregnor force-pushed the feat/parametrize-cadence-cluster-names branch 2 times, most recently from b312d5e to 4824108 Compare August 2, 2021 21:20
@longquanzheng
Copy link

Do you have any updates on this? BTW I created a topic about it: cadence-workflow/cadence#4356 Hope this can help you understand the config

@pregnor
Copy link
Member Author

pregnor commented Aug 13, 2021

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.

It was broken since chart 0.18.0.

.Values.server.frontend.service.port is required
for the Cadence K8s service in the Helm chart,
thus it will always be defined, so I changed the
defaulting behavior to always use that, because it
would never fall back to the dockerize env var
.Env.FRONTEND_PORT or even back to literal 7933.
@pregnor pregnor force-pushed the feat/parametrize-cadence-cluster-names branch 2 times, most recently from badb911 to e626796 Compare August 16, 2021 20:19
@pregnor
Copy link
Member Author

pregnor commented Aug 16, 2021

@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 #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
Copy link

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

@pregnor pregnor force-pushed the feat/parametrize-cadence-cluster-names branch from e626796 to d7a21e7 Compare August 16, 2021 22:48
@pregnor pregnor force-pushed the feat/parametrize-cadence-cluster-names branch 2 times, most recently from febd48f to 5a0499c Compare August 18, 2021 05:39
@longquanzheng
Copy link

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
Copy link
Member Author

pregnor commented Aug 18, 2021

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 pregnor force-pushed the feat/parametrize-cadence-cluster-names branch from 5a0499c to d3a3be9 Compare August 18, 2021 06:37
@pregnor
Copy link
Member Author

pregnor commented Aug 18, 2021

@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).

@pregnor pregnor merged commit c80e3f8 into master Aug 18, 2021
@pregnor pregnor deleted the feat/parametrize-cadence-cluster-names branch August 18, 2021 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Cadence] Allow passing clusterNames instead of hardcoded
4 participants