2
\$\begingroup\$

This script is to enable the remote management of a Rails app via one script. I pieced it together from various blog posts. I am not to familiar with some of the BASH functionality that is used. It seems to work okay, but I wonder if there is anything that I have done sub-optimally in my naïveté.

#!/bin/bash
# /etc/init.d/unicorn

# ### BEGIN INIT INFO
# Provides:          unicorn
# Required-Start:    $network $remote_fs $local_fs
# Required-Stop:     $network $remote_fs $local_fs
# Default-Start:     2 3 4 5
# Default-Stop:      0 1 6
# Short-Description: Stop/start unicorn
### END INIT INFO

export PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin

PROJ_ENV="staging"
RBENV_VERSION=2.2.2
RBENV_ROOT=/opt/rbenv

PROJ_NAME="myapp"

PID_FILE=/var/www/$PROJ_NAME/current/tmp/pids/unicorn.pid
OLD_PID_FILE=$PID_FILE.oldbin
PROJ_DIR=/var/www/$PROJ_NAME
TAG='[unicorn]'

UNICORN_CONFIG=$PROJ_DIR/current/config/unicorn/$PROJ_ENV.rb
DEPLOY_USER=ec2-user
[ -f $PID_FILE ] && KPID=`cat $PID_FILE`

# Start the service
start() {
     # [ is the 'test' command  -f file? -a and -n variable -d directory
    if [ -f $PID_FILE -a -n "${KPID}" -a -d "/proc/${KPID}" ]; then
          logger -sit $TAG "Process ${KPID} in PID file $PID_FILE exists. Service '${PROJ_NAME}' should be running"
        exit 1
    else
        logger -sit "$TAG" "Process doesn't exist. Trying to start server..."
        #su - $DEPLOY_USER -c "bash -c 'cd $PROJ_DIR/current ; pwd      ; /opt/rbenv/bin/rbenv exec bundle exec unicorn -v'"
        su - $DEPLOY_USER -c "bash -c 'cd $PROJ_DIR/current ; pwd       ; /opt/rbenv/bin/rbenv exec bundle exec unicorn -c $UNICORN_CONFIG -E $PROJ_ENV -D'"
        if [[ $? == 0 ]]; then
            logger -sit "$TAG" "server started"
        else
            logger -sit "$TAG" "FAILED to start server"
            exit 1
        fi
    fi
}

# Stop the service
stop() {
        su $DEPLOY_USER -c "kill -TERM $(cat $PID_FILE)"
        sleep 2
        if [[ ! -f $PID_FILE ]]; then
                logger -sit "$TAG" "Server stoped"
        else
                logger -sit "$TAG" "FAILED to stop server"
                exit 1
        fi
}

# Reload service
reload() {
    logger -sit "$TAG" "Trying to reload server"
    kill -USR2 $(cat $PID_FILE)
    sleep 1
    if [[ -f $OLD_PID_FILE ]]; then
        logger -sit "$TAG" "Server reloaded"
    else
        logger -sit "$TAG" "FAILED to reload server"
        exit 1
    fi
}

restart() {
    if [ -f $PID_FILE -a -n "${KPID}" -a -d "/proc/${KPID}" ]; then
        logger -sit "$TAG" "Restarting server"
        kill -HUP $(cat $PID_FILE)
    else
      logger -sit "$TAG" "Process doesn't exist. Running start"
       #$0 is the path of the script
      $0 start
    fi
}

### main logic ###
case "$1" in
    start)
        start
        ;;

    stop)
        stop
        ;;

    restart)
        restart
        ;;
    reload)
        reload
        ;;
    *)
        echo $"Usage: $0 {start|stop|restart|reload}"
        exit 1
esac
exit 0
\$\endgroup\$
0
\$\begingroup\$

Quoting command line arguments

This is not safe:

[ -f $PID_FILE ] && ...

Variables and the result of $(...) should be double-quoted to protect from parameter expansion and globbing:

[ -f "$PID_FILE" ] && ...

A longer example (fixed version, compare with your original):

if [ -f "$PID_FILE" -a -n "${KPID}" -a -d "/proc/${KPID}" ]; then
      logger -sit "$TAG" "Process ${KPID} in PID file $PID_FILE exists. Service '${PROJ_NAME}' should be running"

The lack of quoting is the most common violation in the posted code, review carefully. Copy-paste to https://www.shellcheck.net/, it should help.

Use $(...) instead of `...`

The backtick format is obsolete, and harder to read. Always use $(...).

Usability

The stop function waits for 2 seconds after sending the TERM signal to the process to check if the PID file still exists. That seems a bit arbitrary, and possibly fragile. It would be better to sleep for 1 second 5 times in a loop. This is also pretty arbitrary, but a bit more flexible.

Pointless [ -f $PID_FILE ] && KPID=`cat $PID_FILE`

This is executed always, regardless of the sub-command. And then most sub-commands reload the value in $PID_FILE. It would be better to create a function for getting the PID, and calling that function when you need it.

Signal to PID that might not exist

Some of the functions signal to $(cat "$PID_FILE"), without making sure that PID actually exists, or even that $PID_FILE actually exists. It would be better to validate the state more accurately, and raise errors as appropriate.

Recursive execution

The restart function calls $0 start to restart the script. It would be more natural to simply call the start function directly.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.