-
Notifications
You must be signed in to change notification settings - Fork 428
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
Run riak in a docker container for CI #1747
Conversation
Add healthcheck for Docker MySQL container
9e1ee78
to
4d069f2
Compare
@@ -1,8 +1,11 @@ | |||
#!/bin/bash | |||
#!/usr/bin/env bash |
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.
Is env
is more likely to be in /usr/bin
than bash
?
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.
@kzemek we use /usr/bin/env
in many scripts, it's just another way to do the same thing.
Never seen a system without /bin/bash
or without /usr/bin/env
.
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.
/usr/bin/env <sth>
is a trick to find <sth>
in caller's $PATH
.
I'm not saying that the change is bad. I'm not saying it's good either.
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.
Feeling the same.
But there was some conversation on hipchat about using env everywhere. It was more about "one way to do this", and env was chosen.
Maybe we should document it.
cat "${DB_CONF_DIR}/riak.conf.ssl" >> "$TEMP_RIAK_CONF" | ||
# Import config back into container | ||
docker cp "$TEMP_RIAK_CONF" "mongooseim-riak:/etc/riak/riak.conf" | ||
# Erase temporary config file |
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.
You're really going overboard with the comments here.
sudo tools/setup_riak mongooseim_secret | ||
|
||
elif [ $DB = 'cassandra' ]; then | ||
# Instead of docker run, use "docker create" + "docker start". |
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.
What if instead of doing create
+ fileswap + start
trick we did the following?:
RIAK_CONF=$(mktemp)
docker run --rm --entrypoint cat michalwski/docker-riak:1.0.6 /etc/riak/riak.conf \
sed 's/^search = \(.*\)/search = on' \
> "${RIAK_CONF}"
cat "${DB_CONF_DIR}/riak.conf.ssl" >> "${RIAK_CONF}"
docker create -p 8087:8087 -p 8098:8098 \
-e DOCKER_RIAK_BACKEND=leveldb \
-e DOCKER_RIAK_CLUSTER_SIZE=1 \
--name=mongooseim-riak \
-v "${DB_CONF_DIR}/advanced.config:/etc/riak/advanced.config:ro" \
-v "${SSLDIR}/fake_cert.pem:/etc/riak/cert.pem:ro" \
-v "${SSLDIR}/fake_key.pem:/etc/riak/key.pem:ro" \
-v "${SSLDIR}/ca/cacert.pem:/etc/riak/ca/cacertfile.pem:ro" \
-v "${RIAK_CONF}:/etc/riak/riak.conf:ro"
--health-cmd='riak-admin status' \
"michalwski/docker-riak:1.0.6"
Thoughts?
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.
Two times container creation in your case vs one time container creation in mine.
I want to measure both, but yours version would crash.
Because of the reason, described in a comment here in the code:
+ # We can't use volumes for riak.conf, because:
+ # - we want to change it
+ # - container starting code runs sed on it and gets IO error,
+ # if it's a volume (THIS)
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.
Two times container creation in your case vs one time container creation in mine.
I don't think this is a problem
I want to measure both, but yours version would crash.
Yep, that's a problem
tools/wait_for_healthcheck.sh
Outdated
|
||
# Stop waiting after timeout using a background task | ||
MAIN_PID=$BASHPID | ||
(sleep "$TIMEOUT"; echo ""; echo "Killed by timeout"; kill $MAIN_PID) & |
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.
Why go through the complications of setting up a separate timeout process? We can achieve less fidelity but much more clarity with simply manipulating the loop:
for i in $(seq ${TIMEOUT}); do
if [ $(health_status "$CONTAINER")"" = "\"healthy\"" ]; then
echo "\nWaiting is done"
exit 0
fi
echo -n "."
sleep 1
done
echo "\nKilled by timeout"
exit 1
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.
ok, I like your code more.
We can use also timeout
utility.
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.
There are two main problems with timeout
:
- still requires trapping exits, otherwise
SIGINT
from user will be lost - not present on macOS unless you install coreutils (and then present as
gtimeout
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.
echo -e "\n"
heh, it's tricky
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.
Will not kill or print progress, if docker call does not return. But we can live with it I guess.
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.
I definitely can.
It's also possible that kill
wouldn't kill the process if it hanged in docker call, anyway.
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.
OK, rewrote using cycle.
blocking docker is super rare, I would care, if it would cause problems for me.
Until then it should be enough.
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.
Looks good to me.
I like the idea with docker health checks.
This PR allows to run Riak in a container.
Also adds:
See previous work: