From 7fe795db59bef58a487855eac4c76a5ed2a8ea71 Mon Sep 17 00:00:00 2001 From: Leonid Makarov Date: Tue, 6 Jun 2017 16:29:58 -0700 Subject: [PATCH 1/3] Replace sleep with a true healthcheck for cli Requires docksal/cli version edge (or 1.3.0+ eventually). Falls back to `sleep 5` for older versions --- bin/fin | 46 +++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 43 insertions(+), 3 deletions(-) diff --git a/bin/fin b/bin/fin index 171b14fb6..f7e4f9915 100755 --- a/bin/fin +++ b/bin/fin @@ -337,6 +337,7 @@ get_mysql_connect () # Get container id by service name # @param $1 docker compose service name (e.g. cli) # @return docker container id +# TODO: deprecate in favor of ${COMPOSE_PROJECT_NAME_SAFE}__1 get_container_id () { # Trim any control characters from the output, otherwise there will be issues passing it to the docker binary on Windows. @@ -716,9 +717,7 @@ _start_containers () check_docker_running echo-green "Starting services..." docker-compose up -d --remove-orphans && \ - # Give cli some time to start. Necessary for UID/GID overrides, optionally enabling xdebug/etc. - # TODO: replace this with a Docker native health check - sleep 2 && \ + _healthcheck_wait && \ # TODO: remove in September 2017, as this functionality was ported inside docksal/cli _set_cli_uid && \ _vhost_proxy_connect @@ -856,6 +855,42 @@ _vhost_proxy_connect () fi } +# Checks container health status (if available) +# Relies on healchecks introduced in docksal/cli v1.3.0+, uses `sleep` as a fallback +# @param $1 container id/name +_healthcheck () +{ + local health_status + health_status=$(docker inspect --format='{{json .State.Health.Status}}' "$1" 2>/dev/null) + + # Wait for 5s then exit with 0 if a container does not have a health status property + # Necessary for backward compatibility with images that do not support health checks + if [[ $? != 0 ]]; then + sleep 5 + return 0 + fi + + # If it does, check the status + echo $health_status | grep '"healthy"' >/dev/null 2>&1 +} + +# Waits for containers to become healthy +# For reasoning why we are not using `depends_on` `condition` see here: +# https://github.com/docksal/docksal/issues/225#issuecomment-306604063 +# TODO: make this universal. Currently hardcoded for cli only. +# TODO: add a timeout after which we give up trying the health check +_healthcheck_wait () +{ + # Wait for cli to become ready by watching its health status + local container_name="${COMPOSE_PROJECT_NAME_SAFE}_cli_1" + until _healthcheck "$container_name"; do + echo "Waiting for $container_name to become ready..." + sleep 5; + done + + return 0 +} + #------------------------------ Help functions -------------------------------- # Nicely prints command help @@ -3158,6 +3193,7 @@ _exec () [[ $1 == "-T" ]] && \ local no_tty=true && shift + # TODO: refactor to use ${COMPOSE_PROJECT_NAME_SAFE}__1 # CONTAINER_NAME can be used to override where to run. Used in _bash() CONTAINER_NAME=${CONTAINER_NAME:-cli} container_id=$(get_container_id "$CONTAINER_NAME") @@ -3263,6 +3299,7 @@ mysql () local __dump_password="${dbpassword:-\$MYSQL_ROOT_PASSWORD}" check_docksal_environment + # TODO: refactor to use ${COMPOSE_PROJECT_NAME_SAFE}__1 container_id=$(get_container_id "db") __mysql_command=$(docker exec "$container_id" bash -c "echo -u$__dump_user -p$__dump_password") ${winpty} docker exec -it "$container_id" mysql ${__mysql_command} @@ -3330,6 +3367,7 @@ mysql_import () echo "Importing from stdin..." || # We can not use _run here because we need to launch docker exec with only -i param # and only mysql command direclty (no bash wrapper) so that stdin could be received inside that exec + # TODO: refactor to use ${COMPOSE_PROJECT_NAME_SAFE}__1 container_id=$(get_container_id "db") __mysql_command=$(docker exec "$container_id" bash -c "echo -u$__dump_user -p$__dump_password") __mysql_command=$(echo "$__mysql_command" | sed -e 's/[^a-zA-Z0-9_-]$//') @@ -3368,6 +3406,7 @@ mysql_dump () echo-green "Exporting..." fi + # TODO: refactor to use ${COMPOSE_PROJECT_NAME_SAFE}__1 container_id=$(get_container_id "db") __mysql_command=$(docker exec -i "$container_id" bash -c "echo -u$__dump_user -p$__dump_password") if [[ "${ARGV[0]}" == "" ]]; then @@ -3479,6 +3518,7 @@ _set_cli_uid () # Let uid to be set with the FIN_SET_UID env variable local host_uid=${FIN_SET_UID:-$(id -u)} local host_gid=${FIN_SET_GID:-$(id -g)} + # TODO: refactor to use ${COMPOSE_PROJECT_NAME_SAFE}__1 local cli=$(get_container_id cli || true) # If there is no cli, move on. From 04149e78ec11f2254a3e7ff4b6ddfb4b74dafe0f Mon Sep 17 00:00:00 2001 From: Leonid Makarov Date: Wed, 7 Jun 2017 09:19:39 -0700 Subject: [PATCH 2/3] Added a timeout after which we give up trying the healthcheck --- bin/fin | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/bin/fin b/bin/fin index f7e4f9915..0358a0710 100755 --- a/bin/fin +++ b/bin/fin @@ -878,14 +878,28 @@ _healthcheck () # For reasoning why we are not using `depends_on` `condition` see here: # https://github.com/docksal/docksal/issues/225#issuecomment-306604063 # TODO: make this universal. Currently hardcoded for cli only. -# TODO: add a timeout after which we give up trying the health check _healthcheck_wait () { # Wait for cli to become ready by watching its health status local container_name="${COMPOSE_PROJECT_NAME_SAFE}_cli_1" + local delay=5 + local timeout=30 + local elapsed=0 + until _healthcheck "$container_name"; do echo "Waiting for $container_name to become ready..." - sleep 5; + sleep "$delay"; + + # Give the container 30s to become ready + elapsed=$((elapsed + delay)) + echo "elapsed: $elapsed" + echo "timeout: $timeout" + if ((elapsed > timeout)); then + echo-error "$container_name heathcheck failed" \ + "Container did not enter a healthy state within the expected amount of time." \ + "Try ${yellow}fin restart${NC}" + exit 1 + fi done return 0 From deab11087119a4e7af201d644ada0cad655251cb Mon Sep 17 00:00:00 2001 From: Leonid Makarov Date: Wed, 7 Jun 2017 09:29:39 -0700 Subject: [PATCH 3/3] Increase delay for legacy cli images startup to 10s --- bin/fin | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bin/fin b/bin/fin index 0358a0710..be1dba48b 100755 --- a/bin/fin +++ b/bin/fin @@ -866,7 +866,8 @@ _healthcheck () # Wait for 5s then exit with 0 if a container does not have a health status property # Necessary for backward compatibility with images that do not support health checks if [[ $? != 0 ]]; then - sleep 5 + echo "Waiting 10s for container to start..." + sleep 10 return 0 fi