-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add the multiAddress.enabled cluster option present in zk >3.6.0 #25
base: master
Are you sure you want to change the base?
Conversation
…d up by the deployments
…ookeeperReady functionality in progress
docker/zookeeper-image/zkServer.sh
Outdated
|
||
# | ||
# added -Dzookeeper.multiAddress.enabled=true option to allow for multiple | ||
# zookeeper addresses in the zookeeper-operator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we avoid duplicating the upstream script and instead leverage SERVER_JVMFLAGS
env var to pass in -Dzookeeper.multiAddress.enabled=true
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we use something similar internally at https://git.corp.adobe.com/experience-platform/pipeline-kafka/blob/master/k8s/charts/zookeeper-cluster/values.yaml#L112-L113
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deploying with the SERVER_JVMFLAGS works. Removed this file and now theres no longer a need for a new apache image as well.
I noted the pod env requirement in a comment in generators.go. Should I add a section to the README about multiaddress deployment configuration as well?
echo "Extra server addresses present" | ||
ORIGINALADDRESS=${ZKCONFIG%"$suffix"} | ||
echo "server.${MYID}=${ORIGINALADDRESS}|${EXTRACONFIG}" > $DYNCONFIG | ||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we haven't used this before - is this required for cases where EXTRACONFIG is empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not. When you run zkconfig it sends out a format that is address:quorum-port:leade-port:role;client-port which follows the zk formatting for addresses. When you have multiple addresses the leader and quorum ports need to be duplicated, the role is optional but you can only have ";client-port" once at the end of the line. So if there is no extra address address:<quorum-port:leader-port:role;client--port works, but adding that extra address we need to trim off the ;client-port in the first one so we have address1:quorum-port:leader-port:role|address2:quorum-port:leader-port:role;client--port.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the doc on multiple address formatting: https://zookeeper.apache.org/doc/current/zookeeperReconfig.html#sc_multiaddress
While it doesn't explicitly say the client port can't be duplicated, in development this caused an error that was something to the effect of "address format invalid, should be host:port:port|host:port:port;port..."
…ess enabled flag in a server_jvmflag for the pod vars
docker/zookeeper-image/Dockerfile
Outdated
@@ -153,6 +153,12 @@ COPY opt/zkEnv.sh $DISTRO_NAME/bin/zkEnv.sh | |||
WORKDIR $DISTRO_NAME | |||
VOLUME ["$ZOO_DATA_DIR", "$ZOO_DATA_LOG_DIR", "$ZOO_LOG_DIR"] | |||
|
|||
# overwrite default zkserver.sh in here to add multiaddress.enabled | |||
RUN rm /$DISTRO_NAME/bin/zkServer.sh | |||
COPY zkServer.sh /$DISTRO_NAME/bin/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is this zkServer.sh
coming from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was left over from before you pointed out that I could set multiAddress.enabled with a jvm flag. I just removed this.
Change log description
(2-3 concise points about the changes in this PR. When committing this PR, the committer is expected to copy the content of this section to the merge description box)
Purpose of the change
In zookeeper > 3.6.0 you can specify multiple addresses for a singular zookeeper server. here is the description of that cluster option. This update allows you to leverage that feature through the operator. This can also be useful if you want to expose individual zk servers through external addresses.
(e.g., Fixes #666, Closes #1234)
What the code does
(Detailed description of the code changes)
How to verify it
You can add addresses for some, all or no servers. You can also add multiple addresses for the same server in a pipe separated list.