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 value overrides for master broadcastaddress #110

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ssung-yugabyte
Copy link

FOR GKE MCS support.
GKE MCS will need the override of server_broadcast_addresses.

@tvesely tvesely requested a review from iSignal April 12, 2022 15:43
@tvesely
Copy link

tvesely commented Apr 12, 2022

+1. Just like the tservers, the masters need to be able to set the broadcast address. Small nit: this should be in the values file as well.

@@ -386,7 +386,7 @@ spec:
--undefok=num_cpus,enable_ysql \
--use_node_hostname_for_local_tserver=true \
{{- if $root.Values.authCredentials.ysql.password }}
--ysql_enable_auth=true \
--ysql_enable_auth=true
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a typo. This line should be removed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tvesely : is Stanley around to update this PR? Otherwise, can you please send a new PR with the typo removed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tvesely you mean we should not remove the trailing slash, right?
Cause if we remove this line, a feature used by community users, might stop working.

# For more https://docs.yugabyte.com/latest/reference/configuration/yugabyted/#environment-variables
authCredentials:
ysql:
user: ""
password: ""
database: ""
ycql:
user: ""
password: ""
keyspace: ""

@iSignal iSignal requested a review from bhavin192 April 12, 2022 23:18
@bhavin192
Copy link
Contributor

While I support this change, I'm trying to understand how do we plan to use it for GKE MCS.

When we set a value of master.serverBroadcastAddresses, the same value is passed to all the master pods created by the chart release. So, we will have to specifically set it for each release in each region/AZ to something like $(HOSTNAME).us-east-a.yb-masters.us-east-a-ns.svc.clusterset.local.

Other option is to keep the broadcast address unchanged and use masterAddresses like this: {<service name global across clusters - master-0>, <service name local to the cluster - master-0>}, {yb-master-1.us-east-a.yb-masters.ans.svc.clusterset.local,yb-master-1.yb-masters.ans.svc.cluster.local}....

This format makes it possible to tell TServer and Masters that each master has two ways to reach it. Need to test if this method works with GKE MCS though, that is something similar which we used for Istio multi-cluster setup. https://gist.github.com/bhavin192/ad3e0a7be5e23c00784fca864db9e1fc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants