From 929af4c38d8ca4754d2a3ccf087d359bb67c33f3 Mon Sep 17 00:00:00 2001 From: Tianon Gravi Date: Fri, 17 Apr 2015 16:19:34 -0600 Subject: [PATCH] Fix daemon start/stop logic in hack/make/* scripts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the Bash manual's `set -e` description: (https://www.gnu.org/software/bash/manual/bashref.html#index-set) > Exit immediately if a pipeline (see Pipelines), which may consist of a > single simple command (see Simple Commands), a list (see Lists), or a > compound command (see Compound Commands) returns a non-zero status. > The shell does not exit if the command that fails is part of the > command list immediately following a while or until keyword, part of > the test in an if statement, part of any command executed in a && or > || list except the command following the final && or ||, any command > in a pipeline but the last, or if the command’s return status is being > inverted with !. If a compound command other than a subshell returns a > non-zero status because a command failed while -e was being ignored, > the shell does not exit. Additionally, further down: > If a compound command or shell function executes in a context where -e > is being ignored, none of the commands executed within the compound > command or function body will be affected by the -e setting, even if > -e is set and a command returns a failure status. If a compound > command or shell function sets -e while executing in a context where > -e is ignored, that setting will not have any effect until the > compound command or the command containing the function call > completes. Thus, the only way to have our `.integration-daemon-stop` script actually run appropriately to clean up our daemon on test/script failure is to use `trap ... EXIT`, which we traditionally avoid because it does not have any stacking capabilities, but in this case is a reasonable compromise because it's going to be the only script using it (for now, at least; we can evaluate more complex solutions in the future if they actually become necessary). The alternatives were much less reasonable. One is to have the entire complex chains in any script wanting to use `.integration-daemon-start` / `.integration-daemon-stop` be chained together with `&&` in an `if` block, which is untenable. The other I could think of was taking the body of these scripts out into separate scripts, essentially meaning we'd need two files for each of these, which further complicates the maintenance. Add to that the fact that our `trap ... EXIT` is scoped to the enclosing subshell (`( ... )`) and we're in even more reasonable territory with this pattern. Signed-off-by: Andrew "Tianon" Page --- hack/make/.integration-daemon-start | 1 + hack/make/.integration-daemon-stop | 2 + hack/make/build-deb | 112 +++++++++++++--------------- hack/make/test-docker-py | 26 ++----- hack/make/test-integration-cli | 18 +---- 5 files changed, 65 insertions(+), 94 deletions(-) diff --git a/hack/make/.integration-daemon-start b/hack/make/.integration-daemon-start index 570c6c7a9a..57fd525028 100644 --- a/hack/make/.integration-daemon-start +++ b/hack/make/.integration-daemon-start @@ -25,6 +25,7 @@ if [ -z "$DOCKER_TEST_HOST" ]; then --pidfile "$DEST/docker.pid" \ &> "$DEST/docker.log" ) & + trap "source '${MAKEDIR}/.integration-daemon-stop'" EXIT # make sure that if the script exits unexpectedly, we stop this daemon we just started else export DOCKER_HOST="$DOCKER_TEST_HOST" fi diff --git a/hack/make/.integration-daemon-stop b/hack/make/.integration-daemon-stop index 7e4dc2353a..6e1dc844de 100644 --- a/hack/make/.integration-daemon-stop +++ b/hack/make/.integration-daemon-stop @@ -1,5 +1,7 @@ #!/bin/bash +trap - EXIT # reset EXIT trap applied in .integration-daemon-start + for pidFile in $(find "$DEST" -name docker.pid); do pid=$(set -x; cat "$pidFile") ( set -x; kill "$pid" ) diff --git a/hack/make/build-deb b/hack/make/build-deb index 90c4c16939..a5a6d43870 100644 --- a/hack/make/build-deb +++ b/hack/make/build-deb @@ -7,76 +7,64 @@ DEST=$1 ( source "${MAKEDIR}/.integration-daemon-start" - # we need to wrap up everything in between integration-daemon-start and - # integration-daemon-stop to make sure we kill the daemon and don't hang, - # even and especially on test failures - didFail= - if ! { - set -e + # TODO consider using frozen images for the dockercore/builder-deb tags - # TODO consider using frozen images for the dockercore/builder-deb tags + debVersion="${VERSION//-/'~'}" + # if we have a "-dev" suffix or have change in Git, let's make this package version more complex so it works better + if [[ "$VERSION" == *-dev ]] || [ -n "$(git status --porcelain)" ]; then + gitUnix="$(git log -1 --pretty='%at')" + gitDate="$(date --date "@$gitUnix" +'%Y%m%d.%H%M%S')" + gitCommit="$(git log -1 --pretty='%h')" + gitVersion="git${gitDate}.0.${gitCommit}" + # gitVersion is now something like 'git20150128.112847.0.17e840a' + debVersion="$debVersion~$gitVersion" - debVersion="${VERSION//-/'~'}" - # if we have a "-dev" suffix or have change in Git, let's make this package version more complex so it works better - if [[ "$VERSION" == *-dev ]] || [ -n "$(git status --porcelain)" ]; then - gitUnix="$(git log -1 --pretty='%at')" - gitDate="$(date --date "@$gitUnix" +'%Y%m%d.%H%M%S')" - gitCommit="$(git log -1 --pretty='%h')" - gitVersion="git${gitDate}.0.${gitCommit}" - # gitVersion is now something like 'git20150128.112847.0.17e840a' - debVersion="$debVersion~$gitVersion" + # $ dpkg --compare-versions 1.5.0 gt 1.5.0~rc1 && echo true || echo false + # true + # $ dpkg --compare-versions 1.5.0~rc1 gt 1.5.0~git20150128.112847.17e840a && echo true || echo false + # true + # $ dpkg --compare-versions 1.5.0~git20150128.112847.17e840a gt 1.5.0~dev~git20150128.112847.17e840a && echo true || echo false + # true - # $ dpkg --compare-versions 1.5.0 gt 1.5.0~rc1 && echo true || echo false - # true - # $ dpkg --compare-versions 1.5.0~rc1 gt 1.5.0~git20150128.112847.17e840a && echo true || echo false - # true - # $ dpkg --compare-versions 1.5.0~git20150128.112847.17e840a gt 1.5.0~dev~git20150128.112847.17e840a && echo true || echo false - # true + # ie, 1.5.0 > 1.5.0~rc1 > 1.5.0~git20150128.112847.17e840a > 1.5.0~dev~git20150128.112847.17e840a + fi - # ie, 1.5.0 > 1.5.0~rc1 > 1.5.0~git20150128.112847.17e840a > 1.5.0~dev~git20150128.112847.17e840a + debSource="$(awk -F ': ' '$1 == "Source" { print $2; exit }' hack/make/.build-deb/control)" + debMaintainer="$(awk -F ': ' '$1 == "Maintainer" { print $2; exit }' hack/make/.build-deb/control)" + debDate="$(date --rfc-2822)" + + # if go-md2man is available, pre-generate the man pages + ./docs/man/md2man-all.sh -q || true + # TODO decide if it's worth getting go-md2man in _each_ builder environment to avoid this + + # TODO add a configurable knob for _which_ debs to build so we don't have to modify the file or build all of them every time we need to test + for dir in contrib/builder/deb/*/; do + version="$(basename "$dir")" + suite="${version##*-}" + + image="dockercore/builder-deb:$version" + if ! docker inspect "$image" &> /dev/null; then + ( set -x && docker build -t "$image" "$dir" ) fi - debSource="$(awk -F ': ' '$1 == "Source" { print $2; exit }' hack/make/.build-deb/control)" - debMaintainer="$(awk -F ': ' '$1 == "Maintainer" { print $2; exit }' hack/make/.build-deb/control)" - debDate="$(date --rfc-2822)" - - # if go-md2man is available, pre-generate the man pages - ./docs/man/md2man-all.sh -q || true - # TODO decide if it's worth getting go-md2man in _each_ builder environment to avoid this - - # TODO add a configurable knob for _which_ debs to build so we don't have to modify the file or build all of them every time we need to test - for dir in contrib/builder/deb/*/; do - version="$(basename "$dir")" - suite="${version##*-}" - - image="dockercore/builder-deb:$version" - if ! docker inspect "$image" &> /dev/null; then - ( set -x && docker build -t "$image" "$dir" ) - fi - - mkdir -p "$DEST/$version" - cat > "$DEST/$version/Dockerfile.build" <<-EOF - FROM $image - WORKDIR /usr/src/docker - COPY . /usr/src/docker - RUN ln -sfv hack/make/.build-deb debian - RUN { echo '$debSource (${debVersion}-0~${suite}) $suite; urgency=low'; echo; echo ' * Version: $VERSION'; echo; echo " -- $debMaintainer $debDate"; } > debian/changelog && cat >&2 debian/changelog - RUN dpkg-buildpackage -uc -us - EOF - cp -a "$DEST/$version/Dockerfile.build" . # can't use $DEST because it's in .dockerignore... - tempImage="docker-temp/build-deb:$version" - ( set -x && docker build -t "$tempImage" -f Dockerfile.build . ) - docker run --rm "$tempImage" bash -c 'cd .. && tar -c *_*' | tar -xvC "$DEST/$version" - docker rmi "$tempImage" - done - }; then - didFail=1 - fi + mkdir -p "$DEST/$version" + cat > "$DEST/$version/Dockerfile.build" <<-EOF + FROM $image + WORKDIR /usr/src/docker + COPY . /usr/src/docker + RUN ln -sfv hack/make/.build-deb debian + RUN { echo '$debSource (${debVersion}-0~${suite}) $suite; urgency=low'; echo; echo ' * Version: $VERSION'; echo; echo " -- $debMaintainer $debDate"; } > debian/changelog && cat >&2 debian/changelog + RUN dpkg-buildpackage -uc -us + EOF + cp -a "$DEST/$version/Dockerfile.build" . # can't use $DEST because it's in .dockerignore... + tempImage="docker-temp/build-deb:$version" + ( set -x && docker build -t "$tempImage" -f Dockerfile.build . ) + docker run --rm "$tempImage" bash -c 'cd .. && tar -c *_*' | tar -xvC "$DEST/$version" + docker rmi "$tempImage" + done # clean up after ourselves rm -f Dockerfile.build source "${MAKEDIR}/.integration-daemon-stop" - - [ -z "$didFail" ] # "set -e" ftw -) 2>&1 | tee -a $DEST/test.log +) 2>&1 | tee -a "$DEST/test.log" diff --git a/hack/make/test-docker-py b/hack/make/test-docker-py index 409cee0e4e..ac5ef35833 100644 --- a/hack/make/test-docker-py +++ b/hack/make/test-docker-py @@ -7,24 +7,14 @@ DEST=$1 ( source "${MAKEDIR}/.integration-daemon-start" - # we need to wrap up everything in between integration-daemon-start and - # integration-daemon-stop to make sure we kill the daemon and don't hang, - # even and especially on test failures - didFail= - if ! { - dockerPy='/docker-py' - [ -d "$dockerPy" ] || { - dockerPy="$DEST/docker-py" - git clone https://github.com/docker/docker-py.git "$dockerPy" - } + dockerPy='/docker-py' + [ -d "$dockerPy" ] || { + dockerPy="$DEST/docker-py" + git clone https://github.com/docker/docker-py.git "$dockerPy" + } - # exporting PYTHONPATH to import "docker" from our local docker-py - test_env PYTHONPATH="$dockerPy" python "$dockerPy/tests/integration_test.py" - }; then - didFail=1 - fi + # exporting PYTHONPATH to import "docker" from our local docker-py + test_env PYTHONPATH="$dockerPy" python "$dockerPy/tests/integration_test.py" source "${MAKEDIR}/.integration-daemon-stop" - - [ -z "$didFail" ] # "set -e" ftw -) 2>&1 | tee -a $DEST/test.log +) 2>&1 | tee -a "$DEST/test.log" diff --git a/hack/make/test-integration-cli b/hack/make/test-integration-cli index 8e9b975704..db1cb298fd 100644 --- a/hack/make/test-integration-cli +++ b/hack/make/test-integration-cli @@ -11,21 +11,11 @@ bundle_test_integration_cli() { ( source "${MAKEDIR}/.integration-daemon-start" - # we need to wrap up everything in between integration-daemon-start and - # integration-daemon-stop to make sure we kill the daemon and don't hang, - # even and especially on test failures - didFail= - if ! { - source "${MAKEDIR}/.ensure-frozen-images" - source "${MAKEDIR}/.ensure-httpserver" - source "${MAKEDIR}/.ensure-emptyfs" + source "${MAKEDIR}/.ensure-frozen-images" + source "${MAKEDIR}/.ensure-httpserver" + source "${MAKEDIR}/.ensure-emptyfs" - bundle_test_integration_cli - }; then - didFail=1 - fi + bundle_test_integration_cli source "${MAKEDIR}/.integration-daemon-stop" - - [ -z "$didFail" ] # "set -e" ftw ) 2>&1 | tee -a "$DEST/test.log"