From b6765ea743777083044a296bfd5d0de668952562 Mon Sep 17 00:00:00 2001 From: Miciah Masters Date: Wed, 14 Oct 2015 13:28:46 -0400 Subject: [PATCH] cartridge control scripts: Fix exit value handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix some problems with how we handle exit values in control scripts. Most control scripts have `#!/bin/bash -e` or `set -e` so that they exit immediately if a command exits with a non-zero return value if that command is not part of a compound command or conditional statement. Thus if we anticipate that a command may fail, we must put it in a compound command or a conditional statement. This commit makes the following changes: • Replace `foo; [ $? -eq 0 ] && bar` with `local rc=0; foo || rc=$?; [[ "$rc" = 0 ]] && bar` where foo is an especially long command. • Replace `foo; [ $? -eq 0 ] || bar` with `if ! foo; then bar; fi`. • Replace `foo; ret=$?; if [ $ret -eq 0 ]; then` with `if foo; then`. • Replace `foo; ret=$?; if [ $ret -ne 0 ]; then` with `if ! foo; then`. • Replace `foo; bar $?` with `local rc=0; foo || rc=$?; bar "$rc"`. • Replace `foo; return $?` with `local ret=0; foo || ret=$?; return $ret`. • If a script runs with `/bin/bash -e` and ends with `exit $?`, delete the `exit $?` because it is redundant. Moreover, several control scripts had inverted logic around the /bin/kill command: These scripts would run /bin/kill, check its exit value, and retry /bin/kill if the first attempt returned 0. However, an exit value of 0 from /bin/kill means that it succeeded, so this code was retrying exactly when it did not need to do so. This commit fixes this logic. --- .../bin/control | 4 +-- .../bin/control | 4 +-- .../bin/control | 25 +++++++-------- .../bin/control | 4 +-- .../bin/control | 5 +-- .../bin/control | 5 +-- .../bin/control | 31 ++++++++++++------- .../bin/control | 4 +-- .../usr/versions/shared/bin/control | 5 +-- .../bin/control | 5 +-- 10 files changed, 46 insertions(+), 46 deletions(-) diff --git a/cartridges/openshift-origin-cartridge-haproxy/bin/control b/cartridges/openshift-origin-cartridge-haproxy/bin/control index f157eb073c2..75939bc1fe1 100755 --- a/cartridges/openshift-origin-cartridge-haproxy/bin/control +++ b/cartridges/openshift-origin-cartridge-haproxy/bin/control @@ -134,9 +134,7 @@ function _stop_haproxy_service() { _stop_haproxy_ctld [ -f $HAPROXY_PID ] && pid=$( /bin/cat "${HAPROXY_PID}" ) if `ps -p $pid > /dev/null 2>&1`; then - /bin/kill $pid - ret=$? - if [ $ret -eq 0 ]; then + if ! /bin/kill $pid; then TIMEOUT="$STOPTIMEOUT" while [ $TIMEOUT -gt 0 ] && [ -f "$HAPROXY_PID" ]; do /bin/kill -0 "$pid" >/dev/null 2>&1 || break diff --git a/cartridges/openshift-origin-cartridge-mariadb/bin/control b/cartridges/openshift-origin-cartridge-mariadb/bin/control index 57d1d107b07..9076c0d15a2 100644 --- a/cartridges/openshift-origin-cartridge-mariadb/bin/control +++ b/cartridges/openshift-origin-cartridge-mariadb/bin/control @@ -65,9 +65,7 @@ function stop { if [ -f $pidfile ]; then echo "Stopping MariaDB cartridge" pid=$( /bin/cat $pidfile ) - /bin/kill $pid - ret=$? - if [ $ret -eq 0 ]; then + if ! /bin/kill $pid; then TIMEOUT="$STOPTIMEOUT" while [ $TIMEOUT -gt 0 ] && [ -f "$pidfile" ] do diff --git a/cartridges/openshift-origin-cartridge-mongodb/bin/control b/cartridges/openshift-origin-cartridge-mongodb/bin/control index f9979ce16a9..bdf56880cb8 100755 --- a/cartridges/openshift-origin-cartridge-mongodb/bin/control +++ b/cartridges/openshift-origin-cartridge-mongodb/bin/control @@ -39,8 +39,9 @@ function _repair_mongod() { client_result "Attempting to repair MongoDB ..." tmp_config="/tmp/mongodb.repair.conf" grep -ve "fork\s*=\s*true" $OPENSHIFT_MONGODB_DIR/conf/mongodb.conf > $tmp_config - mongodb_context "mongod ${authopts} -f ${tmp_config} --repair" - client_result "MongoDB repair status = $?" + local rc=0 + mongodb_context "mongod ${authopts} -f ${tmp_config} --repair" || rc=$? + client_result "MongoDB repair status = $rc" rm -f $tmp_config else echo "MongoDB already running - not running repair" @@ -84,16 +85,10 @@ function stop() { if [ -n "$pid" ]; then echo "Stopping MongoDB cartridge" - set +e - /bin/kill $pid 2>/dev/null - set -e - ret=$? - if [ $ret -eq 0 ]; then + if ! /bin/kill $pid 2>/dev/null; then TIMEOUT="$STOPTIMEOUT" while [ $TIMEOUT -gt 0 ] && [ -f "$OPENSHIFT_MONGODB_DIR/pid/mongodb.pid" ]; do - set +e /bin/kill -0 "$pid" >/dev/null 2>&1 || break - set -e sleep 1 let TIMEOUT=${TIMEOUT}-1 done @@ -125,8 +120,10 @@ function pre_snapshot { _wait_for_mongod_to_startup # Work in a temporary directory (create and cd to it). umask 077 - dumpdir=$(mktemp -d /tmp/mongodumpXXXXXXXX) - [ $? -eq 0 ] || die 0 "ERROR" "Failed to create working directory." + if ! dumpdir=$(mktemp -d /tmp/mongodumpXXXXXXXX) + then + die 0 "ERROR" "Failed to create working directory." + fi pushd $dumpdir > /dev/null # Take a "dump". @@ -187,8 +184,10 @@ WARNING: You may have possibly encountered the mongorestore bugs related to function restore_from_mongodb_snapshot { # Work in a temporary directory (create and cd to it). umask 077 - dumpdir=$(mktemp -d /tmp/mongodumpXXXXXXXX) - [ $? -eq 0 ] || die 0 "ERROR" "Failed to create working directory." + if ! dumpdir=$(mktemp -d /tmp/mongodumpXXXXXXXX) + then + die 0 "ERROR" "Failed to create working directory." + fi pushd $dumpdir > /dev/null # Extract dump from the snapshot. diff --git a/cartridges/openshift-origin-cartridge-mysql/bin/control b/cartridges/openshift-origin-cartridge-mysql/bin/control index 5cf142f3418..8a3f2be5c71 100755 --- a/cartridges/openshift-origin-cartridge-mysql/bin/control +++ b/cartridges/openshift-origin-cartridge-mysql/bin/control @@ -80,9 +80,7 @@ function stop { if [ -f $pidfile ]; then echo "Stopping MySQL ${OPENSHIFT_MYSQL_VERSION} cartridge" pid=$( /bin/cat $pidfile ) - /bin/kill $pid - ret=$? - if [ $ret -eq 0 ]; then + if ! /bin/kill $pid; then TIMEOUT="${stop_timeout}" while [[ "${TIMEOUT}" > 0 && -f "${pidfile}" ]] do diff --git a/cartridges/openshift-origin-cartridge-nodejs/bin/control b/cartridges/openshift-origin-cartridge-nodejs/bin/control index 5bff893bd87..dc1c3fff7a3 100755 --- a/cartridges/openshift-origin-cartridge-nodejs/bin/control +++ b/cartridges/openshift-origin-cartridge-nodejs/bin/control @@ -126,10 +126,7 @@ function stop() { echo "`date +"$FMT"`: Stopping application '$OPENSHIFT_APP_NAME' ..." - /bin/kill $cart_pid - ret=$? - - if [ $ret -eq 0 ]; then + if ! /bin/kill $cart_pid; then TIMEOUT="$STOPTIMEOUT" while [ $TIMEOUT -gt 0 ] && is_cartridge_running ; do /bin/kill -0 "$cart_pid" >/dev/null 2>&1 || break diff --git a/cartridges/openshift-origin-cartridge-perl/bin/control b/cartridges/openshift-origin-cartridge-perl/bin/control index 31a8c345f27..749126e5510 100755 --- a/cartridges/openshift-origin-cartridge-perl/bin/control +++ b/cartridges/openshift-origin-cartridge-perl/bin/control @@ -28,10 +28,11 @@ function start() { ensure_valid_httpd_process "$HTTPD_PID_FILE" "$HTTPD_CFG_FILE" # Force httpd into its own pgroup, as httpd is hard-coded to TERM everything in # its pgroup during shutdown (even while foregrounded) + local rc=0 set -m - eval "nohup /usr/sbin/httpd $HTTPD_CMD_CONF -D FOREGROUND |& /usr/bin/logshifter -tag perl &" + eval "nohup /usr/sbin/httpd $HTTPD_CMD_CONF -D FOREGROUND |& /usr/bin/logshifter -tag perl &" || rc=$? set +m - [ "$?" == "0" ] && wait_for_pid_file $HTTPD_PID_FILE + [[ "$rc" = 0 ]] && wait_for_pid_file $HTTPD_PID_FILE } function stop() { diff --git a/cartridges/openshift-origin-cartridge-php/bin/control b/cartridges/openshift-origin-cartridge-php/bin/control index 19aaed13f95..edce76abcef 100755 --- a/cartridges/openshift-origin-cartridge-php/bin/control +++ b/cartridges/openshift-origin-cartridge-php/bin/control @@ -25,37 +25,45 @@ function pre_start_httpd_config { } function start() { + local ret=0 + echo "Starting PHP ${OPENSHIFT_PHP_VERSION} cartridge (Apache+mod_php)" pre_start_httpd_config # Force httpd into its own pgroup, as httpd is hard-coded to TERM everything in # its pgroup during shutdown (even while foregrounded) set -m php_context "nohup /usr/sbin/httpd $HTTPD_CMD_CONF -D FOREGROUND |& /usr/bin/logshifter -tag php &" \ - && wait_for_pid_file $HTTPD_PID_FILE + && wait_for_pid_file $HTTPD_PID_FILE || ret=$? set +m - return $? + + return $ret } function reload() { + local ret=0 + echo "Reloading PHP ${OPENSHIFT_PHP_VERSION} cartridge (Apache+mod_php)" pre_start_httpd_config httpd_pid=`cat "$HTTPD_PID_FILE" 2> /dev/null` - kill -USR1 $httpd_pid && wait_for_pid_file $HTTPD_PID_FILE - return $? + kill -USR1 $httpd_pid && wait_for_pid_file $HTTPD_PID_FILE || ret=$? + + return $ret } function restart() { + local ret=0 + echo "Restarting PHP ${OPENSHIFT_PHP_VERSION} cartridge (Apache+mod_php)" ensure_httpd_restart_succeed "$HTTPD_PID_FILE" "$HTTPD_CFG_FILE" if [ -f "$HTTPD_PID_FILE" ]; then pre_start_httpd_config httpd_pid=`cat "$HTTPD_PID_FILE" 2> /dev/null` - kill -HUP $httpd_pid + kill -HUP $httpd_pid || ret=$? else - start + start || ret=$? fi - return $? + return $ret } function stop() { @@ -69,9 +77,12 @@ function stop() { } function configtest() { + local ret=0 + echo "Testing Apache *.conf files" - php_context "/usr/sbin/httpd $HTTPD_CMD_CONF -t" - return $? + php_context "/usr/sbin/httpd $HTTPD_CMD_CONF -t" || ret=$? + + return $ret } function status() { @@ -185,5 +196,3 @@ case "$1" in build) build ;; *) exit 0 esac - -exit $? diff --git a/cartridges/openshift-origin-cartridge-postgresql/bin/control b/cartridges/openshift-origin-cartridge-postgresql/bin/control index 44493a7fbcc..c855cfb83c4 100644 --- a/cartridges/openshift-origin-cartridge-postgresql/bin/control +++ b/cartridges/openshift-origin-cartridge-postgresql/bin/control @@ -163,9 +163,7 @@ function post_restore { local rexp="s#\(DROP DATABASE\) \(.*\)#\\1 IF EXISTS \\2#g;" # Restore old postgres database - zcat $dump_file | sed "${rexp}" | postgresql_context "psql -U postgres -d postgres" &> /dev/null - - if [ $? -ne 0 ] + if ! zcat $dump_file | sed "${rexp}" | postgresql_context "psql -U postgres -d postgres" &> /dev/null then warning "Error: Could not import Postgres Database! Continuing..." fi diff --git a/cartridges/openshift-origin-cartridge-python/usr/versions/shared/bin/control b/cartridges/openshift-origin-cartridge-python/usr/versions/shared/bin/control index 2e9ea2ecc2e..b6165239a1c 100755 --- a/cartridges/openshift-origin-cartridge-python/usr/versions/shared/bin/control +++ b/cartridges/openshift-origin-cartridge-python/usr/versions/shared/bin/control @@ -68,10 +68,11 @@ function start_apache() { ensure_valid_httpd_process "$HTTPD_PID_FILE" "$HTTPD_CFG_FILE" # Force httpd into its own pgroup, as httpd is hard-coded to TERM everything in # its pgroup during shutdown (even while foregrounded) + local rc=0 set -m - eval "nohup /usr/sbin/httpd $HTTPD_CMD_CONF -DFOREGROUND |& /usr/bin/logshifter -tag python &" + eval "nohup /usr/sbin/httpd $HTTPD_CMD_CONF -DFOREGROUND |& /usr/bin/logshifter -tag python &" || rc=$? set +m - [ "$?" == "0" ] && wait_for_pid_file $HTTPD_PID_FILE + [[ "$rc" = 0 ]] && wait_for_pid_file $HTTPD_PID_FILE } function start() { diff --git a/cartridges/openshift-origin-cartridge-ruby/bin/control b/cartridges/openshift-origin-cartridge-ruby/bin/control index d0eb908f112..f0869d80fdf 100755 --- a/cartridges/openshift-origin-cartridge-ruby/bin/control +++ b/cartridges/openshift-origin-cartridge-ruby/bin/control @@ -41,10 +41,11 @@ function start() { update_passenger_performance # Force httpd into its own pgroup, as httpd is hard-coded to TERM everything in # its pgroup during shutdown (even while foregrounded) + local rc=0 set -m - ruby_context "umask 002 &>/dev/null ; RAILS_ENV=${RAILS_ENV:-production} nohup /usr/sbin/httpd $HTTPD_CMD_CONF -D FOREGROUND |& /usr/bin/logshifter -tag ruby &" + ruby_context "umask 002 &>/dev/null ; RAILS_ENV=${RAILS_ENV:-production} nohup /usr/sbin/httpd $HTTPD_CMD_CONF -D FOREGROUND |& /usr/bin/logshifter -tag ruby &" || rc=$? set +m - [ "$?" == "0" ] && wait_for_pid_file $HTTPD_PID_FILE + [[ "$rc" = 0 ]] && wait_for_pid_file $HTTPD_PID_FILE } function stop() {