diff --git a/plugins/apps/subcommands.go b/plugins/apps/subcommands.go index 96942a6e3..72a7946ac 100644 --- a/plugins/apps/subcommands.go +++ b/plugins/apps/subcommands.go @@ -137,8 +137,13 @@ func CommandLock(appName string) error { return err } + // the file is not truncated as it may hold the build id of an in-progress deploy lockPath := getLockPath(appName) - if _, err := os.Create(lockPath); err != nil { + lockFile, err := os.OpenFile(lockPath, os.O_CREATE|os.O_WRONLY, 0644) + if err != nil { + return errors.New("Unable to create deploy lock") + } + if err := lockFile.Close(); err != nil { return errors.New("Unable to create deploy lock") } diff --git a/plugins/common/functions b/plugins/common/functions index bb00b5016..f2645f448 100755 --- a/plugins/common/functions +++ b/plugins/common/functions @@ -980,11 +980,17 @@ dokku_discard_build_capture() { unset DOKKU_BUILD_ID DOKKU_BUILD_LOG_FILE DOKKU_BUILD_SOURCE } +app_deploy_lock_file() { + declare desc="outputs the path to the deploy lock file for an app" + declare APP="$1" + echo "$DOKKU_LIB_ROOT/data/apps/$APP/.deploy.lock" +} + acquire_app_deploy_lock() { declare desc="acquire advisory lock for use in deploys" local APP="$1" local LOCK_TYPE="${2:-waiting}" - local APP_DEPLOY_LOCK_FILE="$DOKKU_LIB_ROOT/data/apps/$APP/.deploy.lock" + local APP_DEPLOY_LOCK_FILE="$(app_deploy_lock_file "$APP")" local LOCK_WAITING_MSG="$APP currently has a deploy lock in place. Waiting..." local LOCK_FAILED_MSG="$APP currently has a deploy lock in place. Exiting..." @@ -1003,7 +1009,7 @@ release_app_deploy_lock() { declare desc="release advisory lock used in deploys" local APP="$1" local EXIT_CODE="${2:-0}" - local APP_DEPLOY_LOCK_FILE="$DOKKU_LIB_ROOT/data/apps/$APP/.deploy.lock" + local APP_DEPLOY_LOCK_FILE="$(app_deploy_lock_file "$APP")" if [[ -n "$DOKKU_BUILD_ID" ]]; then plugn trigger builds-record-finalize "$APP" "$DOKKU_BUILD_ID" "$EXIT_CODE" || true @@ -1018,24 +1024,35 @@ acquire_advisory_lock() { local LOCK_FD="200" local SHOW_MSG=true - eval "exec $LOCK_FD>$LOCK_FILE" - if [[ "$LOCK_TYPE" == "waiting" ]]; then - while [[ $( - flock -n "$LOCK_FD" &>/dev/null - echo $? - ) -ne 0 ]]; do - if [[ "$SHOW_MSG" == "true" ]]; then - echo "$LOCK_WAITING_MSG" - SHOW_MSG=false + while true; do + # opened for appending so a failed attempt does not truncate the build id + # written by the current lock holder + eval "exec $LOCK_FD>>$LOCK_FILE" + if [[ "$LOCK_TYPE" == "waiting" ]]; then + while [[ $( + flock -n "$LOCK_FD" &>/dev/null + echo $? + ) -ne 0 ]]; do + if [[ "$SHOW_MSG" == "true" ]]; then + echo "$LOCK_WAITING_MSG" + SHOW_MSG=false + fi + sleep 1 + done + else + if ! flock -n "$LOCK_FD" &>/dev/null; then + dokku_log_warn "$LOCK_FAILED_MSG" + dokku_log_fail "Run 'apps:unlock' to release the existing deploy lock" fi - sleep 1 - done - else - if ! flock -n "$LOCK_FD" &>/dev/null; then - dokku_log_warn "$LOCK_FAILED_MSG" - dokku_log_fail "Run 'apps:unlock' to release the existing deploy lock" fi - fi + + # the previous holder removes the lock file on release, so the file locked + # above may no longer be the one at the lock path + if [[ "/proc/self/fd/$LOCK_FD" -ef "$LOCK_FILE" ]]; then + break + fi + eval "exec $LOCK_FD>&-" + done echo "${DOKKU_BUILD_ID:-$DOKKU_PID}" >"$LOCK_FILE" } @@ -1044,7 +1061,13 @@ release_advisory_lock() { local LOCK_FILE="$1" local LOCK_FD="200" - flock -u "$LOCK_FD" && rm -f "$LOCK_FILE" &>/dev/null + # the file is removed before unlocking so a waiter that locks it afterwards + # can tell that its lock is stale. It is only removed if it is still the file + # this process locked, as apps:unlock may have handed the path to another deploy + if [[ "/proc/self/fd/$LOCK_FD" -ef "$LOCK_FILE" ]]; then + rm -f "$LOCK_FILE" &>/dev/null + fi + flock -u "$LOCK_FD" } suppress_output() { diff --git a/plugins/git/functions b/plugins/git/functions index 5886822bf..2b23a75e9 100755 --- a/plugins/git/functions +++ b/plugins/git/functions @@ -150,28 +150,36 @@ git_receive_app() { if [[ ! -d "$DOKKU_ROOT/$APP/refs" ]]; then true else - local exit_code=0 has_source_image=false source_image previous_source_image + local exit_code=0 acquire_app_deploy_lock "$APP" "exclusive" - - # DOKKU_GIT_SOURCE_IMAGE is only applied while the deploy lock is held. An - # empty value clears the property, while an unset variable leaves it as-is. - if [[ -n "${DOKKU_GIT_SOURCE_IMAGE+x}" ]]; then - has_source_image=true - source_image="$DOKKU_GIT_SOURCE_IMAGE" - unset DOKKU_GIT_SOURCE_IMAGE - previous_source_image="$(fn-git-source-image "$APP")" - fn-plugin-property-write "git" "$APP" "source-image" "$source_image" - fi - - (git_build "$APP" "$REV") || exit_code="$?" - if [[ "$exit_code" -ne 0 ]] && [[ "$has_source_image" == "true" ]]; then - fn-plugin-property-write "git" "$APP" "source-image" "$previous_source_image" - fi + fn-git-build-with-lock-held "$APP" "$REV" || exit_code="$?" release_app_deploy_lock "$APP" "$exit_code" return "$exit_code" fi } +fn-git-build-with-lock-held() { + declare desc="builds an app at a revision, expecting the caller to hold the app deploy lock" + declare APP="$1" REV="$2" + local exit_code=0 has_source_image=false source_image previous_source_image + + # DOKKU_GIT_SOURCE_IMAGE is only applied while the deploy lock is held. An + # empty value clears the property, while an unset variable leaves it as-is. + if [[ -n "${DOKKU_GIT_SOURCE_IMAGE+x}" ]]; then + has_source_image=true + source_image="$DOKKU_GIT_SOURCE_IMAGE" + unset DOKKU_GIT_SOURCE_IMAGE + previous_source_image="$(fn-git-source-image "$APP")" + fn-plugin-property-write "git" "$APP" "source-image" "$source_image" + fi + + (git_build "$APP" "$REV") || exit_code="$?" + if [[ "$exit_code" -ne 0 ]] && [[ "$has_source_image" == "true" ]]; then + fn-plugin-property-write "git" "$APP" "source-image" "$previous_source_image" + fi + return "$exit_code" +} + cmd-git-upload-pack() { declare desc="executes git-upload-pack" declare cmd="git-upload-pack" diff --git a/plugins/git/git-from-directory b/plugins/git/git-from-directory index 542090c41..25210ff0a 100755 --- a/plugins/git/git-from-directory +++ b/plugins/git/git-from-directory @@ -5,20 +5,19 @@ source "$PLUGIN_AVAILABLE_PATH/git/internal-functions" set -eo pipefail [[ $DOKKU_TRACE ]] && set -x -trigger-git-git-from-directory() { - declare desc="updates a repository from a directory" - declare trigger="git-from-directory" - declare APP="$1" SOURCECODE_WORK_DIR="$2" USER_NAME="${3:-Dokku}" USER_EMAIL="${4:-automated@dokku.sh}" +fn-git-from-directory-with-lock-held() { + declare desc="updates a repository from a directory and builds it, expecting the caller to hold the app deploy lock" + declare APP="$1" SOURCECODE_WORK_DIR="$2" USER_NAME="$3" USER_EMAIL="$4" local APP_ROOT="$DOKKU_ROOT/$APP" local DOKKU_DEPLOY_BRANCH="$(fn-git-computed-deploy-branch "$APP")" - local REV + local REV PREVIOUS_REV local TMP_WORK_DIR=$(mktemp -d "/tmp/dokku-${DOKKU_PID}-${FUNCNAME[0]}.XXXXXX") local TMP_WORK_DIR_2=$(mktemp -d "/tmp/dokku-${DOKKU_PID}-${FUNCNAME[0]}.XXXXXX") trap "rm -rf '$TMP_WORK_DIR' '$TMP_WORK_DIR_2' >/dev/null" RETURN INT TERM EXIT local has_code=false - if fn-git-cmd "$APP_ROOT" rev-parse "$DOKKU_DEPLOY_BRANCH" &>/dev/null; then + if PREVIOUS_REV="$(fn-git-cmd "$APP_ROOT" rev-parse "$DOKKU_DEPLOY_BRANCH" 2>/dev/null)"; then has_code=true fi @@ -60,19 +59,39 @@ trigger-git-git-from-directory() { fi rm -rf "$TMP_WORK_DIR" "$TMP_WORK_DIR_2" >/dev/null - # run in a subshell so a failure to acquire the deploy lock still reaches the rollback - if (git_receive_app "$APP" "$REV"); then + if fn-git-build-with-lock-held "$APP" "$REV"; then return else local exit_code=$? local DOKKU_DEPLOY_BRANCH="$(fn-git-computed-deploy-branch "$APP")" + # only roll back the deploy branch if it still points at the commit created above if [[ "$has_code" == "false" ]]; then - fn-git-cmd "$APP_ROOT" update-ref -d HEAD + fn-git-cmd "$APP_ROOT" update-ref -d "refs/heads/$DOKKU_DEPLOY_BRANCH" "$REV" || dokku_log_warn "Unable to roll back the $DOKKU_DEPLOY_BRANCH branch" else - fn-git-cmd "$APP_ROOT" update-ref "refs/heads/$DOKKU_DEPLOY_BRANCH" "$DOKKU_DEPLOY_BRANCH^" + fn-git-cmd "$APP_ROOT" update-ref "refs/heads/$DOKKU_DEPLOY_BRANCH" "$PREVIOUS_REV" "$REV" || dokku_log_warn "Unable to roll back the $DOKKU_DEPLOY_BRANCH branch" fi return "$exit_code" fi } +trigger-git-git-from-directory() { + declare desc="updates a repository from a directory" + declare trigger="git-from-directory" + declare APP="$1" SOURCECODE_WORK_DIR="$2" USER_NAME="${3:-Dokku}" USER_EMAIL="${4:-automated@dokku.sh}" + local exit_code=0 + + # the lock is held from before the deploy branch is updated until after it is + # rolled back, and the subshell ensures it is released on any failure + acquire_app_deploy_lock "$APP" "exclusive" + set +e + ( + set -e + fn-git-from-directory-with-lock-held "$APP" "$SOURCECODE_WORK_DIR" "$USER_NAME" "$USER_EMAIL" + ) + exit_code="$?" + set -e + release_app_deploy_lock "$APP" "$exit_code" + return "$exit_code" +} + trigger-git-git-from-directory "$@" diff --git a/plugins/git/internal-functions b/plugins/git/internal-functions index f841499e9..41f1f2b4e 100644 --- a/plugins/git/internal-functions +++ b/plugins/git/internal-functions @@ -279,7 +279,7 @@ cmd-git-sync() { fi local APP_ROOT="$DOKKU_ROOT/$APP" - local APP_DEPLOY_LOCK_FILE="$DOKKU_LIB_ROOT/data/apps/$APP/.deploy.lock" + local APP_DEPLOY_LOCK_FILE="$(app_deploy_lock_file "$APP")" local fetch_exit_code=0 dokku_setup_build_capture "$APP" "git:sync" diff --git a/tests/unit/apps_1.bats b/tests/unit/apps_1.bats index 5bbfcda55..6264065bb 100644 --- a/tests/unit/apps_1.bats +++ b/tests/unit/apps_1.bats @@ -402,6 +402,36 @@ teardown() { destroy_app } +@test "(apps) apps:lock does not truncate an existing lock file" { + run create_app + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "echo holder-id >$DOKKU_LIB_ROOT/data/apps/$TEST_APP/.deploy.lock && chown dokku:dokku $DOKKU_LIB_ROOT/data/apps/$TEST_APP/.deploy.lock" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku apps:lock $TEST_APP" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "cat $DOKKU_LIB_ROOT/data/apps/$TEST_APP/.deploy.lock" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "holder-id" + + run /bin/bash -c "dokku apps:unlock $TEST_APP" + echo "output: $output" + echo "status: $status" + assert_success + + destroy_app +} + @test "(apps:report) emits new stripped JSON keys alongside legacy" { create_app diff --git a/tests/unit/deploy-lock.bats b/tests/unit/deploy-lock.bats new file mode 100644 index 000000000..4abd87ff3 --- /dev/null +++ b/tests/unit/deploy-lock.bats @@ -0,0 +1,109 @@ +#!/usr/bin/env bats + +load test_helper + +setup() { + global_setup + LOCK_DIR="$(mktemp -d "/tmp/${DOKKU_DOMAIN}.XXXXX")" + LOCK_FILE="$LOCK_DIR/test.lock" + LOCK_PIDS=() +} + +teardown() { + local pid + touch "$LOCK_DIR/release" + for pid in "${LOCK_PIDS[@]}"; do + kill "$pid" &>/dev/null || true + done + rm -rf "$LOCK_DIR" + global_teardown +} + +lock_script() { + declare BUILD_ID="$1" SCRIPT="$2" + echo "source '$PLUGIN_CORE_AVAILABLE_PATH/common/functions'; export DOKKU_BUILD_ID='$BUILD_ID'; $SCRIPT" +} + +start_lock_holder() { + declare BUILD_ID="$1" + /bin/bash -c "$(lock_script "$BUILD_ID" "acquire_advisory_lock '$LOCK_FILE' exclusive 'waiting' 'lock held'; while [[ ! -f '$LOCK_DIR/release' ]]; do sleep 0.2; done; release_advisory_lock '$LOCK_FILE'")" >/dev/null 2>&1 3>&- & + LOCK_PIDS+=("$!") + wait_for "[[ \"\$(cat '$LOCK_FILE' 2>/dev/null)\" == '$BUILD_ID' ]]" +} + +wait_for() { + declare CONDITION="$1" + local i + for i in $(seq 1 50); do + if eval "$CONDITION"; then + return 0 + fi + sleep 0.2 + done + + echo "timed out waiting for: $CONDITION" + return 1 +} + +@test "(deploy-lock) a failed acquire does not truncate the lock holder's build id" { + start_lock_holder "holder-a" + + run /bin/bash -c "$(lock_script "other" "acquire_advisory_lock '$LOCK_FILE' exclusive 'waiting' 'lock held'")" + echo "output: $output" + echo "status: $status" + assert_failure + assert_output_contains "lock held" + + run /bin/bash -c "cat '$LOCK_FILE'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "holder-a" +} + +@test "(deploy-lock) a waiter does not keep a stale lock after the holder removes the lock file" { + start_lock_holder "holder-a" + + /bin/bash -c "$(lock_script "holder-b" "acquire_advisory_lock '$LOCK_FILE' waiting 'waiting' 'lock held'; touch '$LOCK_DIR/b.acquired'; exec sleep 60")" >"$LOCK_DIR/b.log" 2>&1 3>&- & + local B_PID="$!" + LOCK_PIDS+=("$B_PID") + wait_for "grep -q waiting '$LOCK_DIR/b.log'" + + touch "$LOCK_DIR/release" + wait_for "[[ -f '$LOCK_DIR/b.acquired' ]]" + + run /bin/bash -c "[[ /proc/$B_PID/fd/200 -ef '$LOCK_FILE' ]]" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "cat '$LOCK_FILE'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "holder-b" + + run /bin/bash -c "$(lock_script "holder-c" "acquire_advisory_lock '$LOCK_FILE' exclusive 'waiting' 'lock held'")" + echo "output: $output" + echo "status: $status" + assert_failure + assert_output_contains "lock held" +} + +@test "(deploy-lock) release only removes the lock file it holds" { + start_lock_holder "holder-a" + + run /bin/bash -c "rm -f '$LOCK_FILE' && echo 'replacement' >'$LOCK_FILE'" + echo "output: $output" + echo "status: $status" + assert_success + + touch "$LOCK_DIR/release" + wait_for "! kill -0 ${LOCK_PIDS[0]} 2>/dev/null" + + run /bin/bash -c "cat '$LOCK_FILE'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "replacement" +} diff --git a/tests/unit/git_4.bats b/tests/unit/git_4.bats index e29126095..0b3cd31c1 100644 --- a/tests/unit/git_4.bats +++ b/tests/unit/git_4.bats @@ -51,12 +51,16 @@ deploy_second_procfile_image() { hold_deploy_lock() { DEPLOY_LOCK_FILE="$DOKKU_LIB_ROOT/data/apps/$TEST_APP/.deploy.lock" - sudo -u dokku flock -o "$DEPLOY_LOCK_FILE" sleep 120 >/dev/null 2>&1 3>&- & - DEPLOY_LOCK_HOLDER_PID="$!" + DEPLOY_LOCK_HOLDER_PID_FILE="$(mktemp "/tmp/${DOKKU_DOMAIN}.XXXXX")" + chown dokku:dokku "$DEPLOY_LOCK_HOLDER_PID_FILE" + + # the holder writes a build id like a deploy would, then becomes the sleep + # process so that killing its pid releases the lock + sudo -u dokku bash -c "exec 200>>'$DEPLOY_LOCK_FILE'; flock -n 200 || exit 1; echo test-lock-holder >'$DEPLOY_LOCK_FILE'; echo \$\$ >'$DEPLOY_LOCK_HOLDER_PID_FILE'; exec sleep 120" >/dev/null 2>&1 3>&- & local i for i in $(seq 1 20); do - if ! flock -n "$DEPLOY_LOCK_FILE" true; then + if [[ -s "$DEPLOY_LOCK_HOLDER_PID_FILE" ]] && ! flock -n "$DEPLOY_LOCK_FILE" true; then return 0 fi sleep 0.5 @@ -67,15 +71,25 @@ hold_deploy_lock() { } release_deploy_lock() { - if [[ -z "$DEPLOY_LOCK_HOLDER_PID" ]]; then + if [[ -z "$DEPLOY_LOCK_HOLDER_PID_FILE" ]]; then return 0 fi - kill "$DEPLOY_LOCK_HOLDER_PID" &>/dev/null || true - wait "$DEPLOY_LOCK_HOLDER_PID" &>/dev/null || true - pkill -u dokku -f "flock -o $DEPLOY_LOCK_FILE" &>/dev/null || true - rm -f "$DEPLOY_LOCK_FILE" - DEPLOY_LOCK_HOLDER_PID="" + local holder_pid i + if [[ -s "$DEPLOY_LOCK_HOLDER_PID_FILE" ]]; then + holder_pid="$(cat "$DEPLOY_LOCK_HOLDER_PID_FILE")" + kill "$holder_pid" &>/dev/null || true + for i in $(seq 1 20); do + kill -0 "$holder_pid" &>/dev/null || break + sleep 0.5 + done + fi + rm -f "$DEPLOY_LOCK_FILE" "$DEPLOY_LOCK_HOLDER_PID_FILE" + DEPLOY_LOCK_HOLDER_PID_FILE="" +} + +deploy_branch_rev() { + sudo -u dokku git -C "/home/dokku/$TEST_APP" rev-parse master } @test "(git) git:from-image [missing]" { @@ -113,11 +127,21 @@ release_deploy_lock() { assert_output_contains "Image exists on host, skipping pull" assert_output_contains "No changes detected, skipping git commit" + run /bin/bash -c "dokku apps:locked $TEST_APP" + echo "output: $output" + echo "status: $status" + assert_failure + run /bin/bash -c "dokku git:from-image --force $TEST_APP linuxserver/foldingathome:7.6.21" echo "output: $output" echo "status: $status" assert_output_contains "Force pulling image" assert_output_contains "No changes detected, skipping git commit" + + run /bin/bash -c "dokku apps:locked $TEST_APP" + echo "output: $output" + echo "status: $status" + assert_failure } @test "(git) git:from-image [normal-custom-branch]" { @@ -210,6 +234,11 @@ release_deploy_lock() { assert_success assert_output "" + run /bin/bash -c "dokku apps:locked $TEST_APP" + echo "output: $output" + echo "status: $status" + assert_failure + run /bin/bash -c "dokku config:set --no-restart $TEST_APP FAIL_ON_STARTUP=false" echo "output: $output" echo "status: $status" @@ -236,6 +265,11 @@ release_deploy_lock() { echo "status: $status" assert_failure + run /bin/bash -c "dokku apps:locked $TEST_APP" + echo "output: $output" + echo "status: $status" + assert_failure + run /bin/bash -c "dokku git:report $TEST_APP --git-source-image" echo "output: $output" echo "status: $status" @@ -500,6 +534,10 @@ EOF echo "status: $status" assert_success + local DEPLOY_REV + DEPLOY_REV="$(deploy_branch_rev)" + echo "deploy branch rev: $DEPLOY_REV" + hold_deploy_lock run /bin/bash -c "dokku git:from-image $TEST_APP dokku-test/$TEST_APP:default-procfile" @@ -514,6 +552,18 @@ EOF assert_success assert_output "dokku-test/$TEST_APP:second-procfile" + run deploy_branch_rev + echo "output: $output" + echo "status: $status" + assert_success + assert_output "$DEPLOY_REV" + + run /bin/bash -c "cat $DEPLOY_LOCK_FILE" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "test-lock-holder" + run /bin/bash -c "dokku git:sync $TEST_APP https://github.com/dokku/smoke-test-app.git 2.0.0" echo "output: $output" echo "status: $status" @@ -526,6 +576,18 @@ EOF assert_success assert_output "dokku-test/$TEST_APP:second-procfile" + run deploy_branch_rev + echo "output: $output" + echo "status: $status" + assert_success + assert_output "$DEPLOY_REV" + + run /bin/bash -c "cat $DEPLOY_LOCK_FILE" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "test-lock-holder" + release_deploy_lock run /bin/bash -c "dokku git:from-image $TEST_APP dokku-test/$TEST_APP:default-procfile"