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 health check #28

Closed
wants to merge 1 commit into from
Closed

Conversation

thall
Copy link

@thall thall commented Nov 10, 2017

Think the official Zookeeper image should include an health check.

@31z4 31z4 self-requested a review November 11, 2017 03:58
@31z4
Copy link
Owner

31z4 commented Nov 11, 2017

Hey @thall, thanks for the PR! I wonder how would this compare to

[[ `echo ruok | nc localhost $ZOO_PORT` == 'imok' ]]

Note, that zkServer.sh status calls srvr command (https://github.com/apache/zookeeper/blob/branch-3.4/bin/zkServer.sh#L211) which lists full details for the server. While ruok tests if server is running in a non-error state. Not sure which one works best for healthchecking.

What do you think?

@thall
Copy link
Author

thall commented Nov 13, 2017

@31z4 Your are right! The documentation also state that, https://zookeeper.apache.org/doc/r3.3.6/zookeeperAdmin.html#sc_zkCommands . I have pushed a new commit.

Thanks for the feedback!

@31z4
Copy link
Owner

31z4 commented Nov 13, 2017

Hey @thall, sorry to say that, but it looks like there is no chance for this change to be merged into the official library due to @yosifkit position. Please see docker-library/postgres#282 for details.

@thall thall closed this Dec 5, 2017
@thall thall deleted the zookeeper_healthcheck branch December 5, 2017 08:17
@dsw9742
Copy link

dsw9742 commented Feb 2, 2018

Hi, anyone have any tips to implement our own healthcheck at the docker service create level if this won't move forward? Additionally, I'd opt for the zkServer.sh-based solution versus the ruok-based one, since the former is more reliable.

I've been trying --health-cmd='SERVER_JVMFLAGS= ; bin/zkServer.sh status || exit 1' \ --health-start-period=300s \ but it seems to hang on startup and then fail.

What's odd is that the service will start -- I can see leader election and everything using docker service logs -- but docker thinks the service is not ready. It just spins on "overall progress: 0 out of 1 tasks"

@31z4
Copy link
Owner

31z4 commented Feb 4, 2018

@dsw9742 have you tried to exec bin/zkServer.sh status within a running container to make sure that it works? I bet it simply doesn't find bin/zkServer.sh, try zkServer.sh instead. Also, why would you add SERVER_JVMFLAGS= ; there?

@dsw9742
Copy link

dsw9742 commented Feb 4, 2018

@31z4 clearly I should have provided more context about some of this.

The env var for SERVER_JVMFLAGS is used in my configuration to set java Xmx and the prometheus JMX agent. I have to clear that env var any time I docker exec into a running container.

I'll try removing bin from the command to see if that makes a difference. Will report back shortly.

@dsw9742
Copy link

dsw9742 commented Feb 6, 2018

@31z4 so tested this a bit with docker exec.

If I exec into the running container and execute bin/zkServer.sh status from the default /zookeeper-3.4.10 directory, I get "Error contacting service. It is probably not running." Same result if I execute zkServer.sh status.

If I exec into the running container and execute SERVER_JVMFLAGS= && bin/zkServer.sh status from the default /zookeeper-3.4.10 directory, I get a success response, something like "Mode: follower". Same result if I execute SERVER_JVMFLAGS= && zkServer.sh status.

So seems to me the cause is one or more of the flags I'm setting with the env var SERVER_JVMFLAGS, probably the -javaagent and probably the port it uses. Since using a javaagent with zookeeper isn't completely edge case, I'd really like to find a solution that works under this scenario.

What I don't understand is why setting the SERVER_JVMFLAGS to empty in the --health-cmd command value doesn't have the same result there as it does in docker exec. Any ideas?

@31z4 31z4 mentioned this pull request Oct 20, 2019
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.

None yet

3 participants