fix use of app_name and exec vars in systemv start-rpm-template

[dwhjames]

It looks like a bug was introduced in commit 22a691a, where if executableScriptName in Linux and packageName in Linux are not equal, then an incorrect script is generated.

The exec template variable should only be used to build the run command. Everything else should be the app_name.

[muuki88]

why remove this and insert ${{app_name}} by hand?

[dwhjames]

I thought it helped the readability of the file. Seeing as it’s already templated, it seems easier to see where the app_name variable flows to, rather than having the indirection of prog.

[muuki88]

Hm. I think it doesn't matter. The main point for creating a new variable is to be able to rename the replacement placeholder in one place. However we shouldn't do this (as any client could rely on this). So readability wins :)

[dwhjames]

You mean change the var name app_name in the future? As you say, that would be pretty disastrous for backwards compatibility :)

[muuki88]

On which system did you test this?

[dwhjames]

On which system did you test this?

I haven’t; I figured I was safe as it’s just a renaming.

[muuki88]

optimist :D

[dwhjames]

On that note, there are some simple sbt tests projects, none of which were able to catch this issue with exec vs. app_name. Have you had any thoughts about how to do deeper testing of scripts?

On the one hand, these templates don’t need to change that often and code review is not replaceable, yet some means for doing dry runs on scripts would be great. It definitely seems hard though… and I’d admit that previously I’ve resorted to manual testing when developing sbt plugins.

[muuki88]

The problem is testing system loaders is that you need a real virtual machine to

• start
• restart
• stop

the service and check. Still on the list, but needs a lot of effort :(

[muuki88]

I'll give this a shot on centos. @kardapoltsev could you take a look aswell?

[kardapoltsev]

I've already took. Looks reasonable but I'm not sure.

[muuki88]

Hm. I get

-bash: /var/log/test-project-simple/daemon.log: Permission denied

However this shouldn't be due to this PR. I cannot even run the command as root?!

[kardapoltsev]

Even root couldn't run non-executable file :)

[muuki88]

true story, bro :(

[dwhjames]

Huh, didn’t see that one coming. So what happened?

[muuki88]

Hm. Nope, everythings seems fine @kardapoltsev (regarding executable permissions)

> ls -Al /usr/share/test-project-simple/bin/
total 12
-rwxr-xr-x 1 root root 9187 Mar 30 13:34 test-project-simple

Running with

sudo runuser test-project-simple /usr/share/test-project-simple/bin/test-project-simple >> /var/log/test-project-simple/daemon.log 2>&1
-bash: /var/log/test-project-simple/daemon.log: Permission denied

I added touch and chown to the init script, but with no luck:

ls -Al /var/log/test-project-simple/
total 0
-rw-r--r-- 1 test-project-simple test-project-simple 0 Mar 30 13:50 daemon.log

[dwhjames]

Oh, is it just that we have incorrectly used runuser?

[muuki88]

yeah.... I'm pretty sure this is the case 😭
And I have yet no idea how to fix this.

[dwhjames]

I guess not many are using the systemv + rpm combo?

[muuki88]

looks like. Shouldn't hold us back from fixing this :)

[dwhjames]

Maybe now is the time to address the ‘FIXME’

# FIXME figure out how to use daemon correctly
nohup runuser $DAEMON_USER ${RUN_CMD} >> /var/log/${{app_name}}/daemon.log 2>&1 &

# The way to go, but doesn't work properly
# If the app creates the pid file this gets messy
# daemon --user $DAEMON_USER --pidfile $PIDFILE $RUN_CMD &

I’m not sure I follow the issue that you were trying to avoid (from d6f88be)

A few lines down the script is creating a PID file. If this is used with Play, which creates its own PID file, won’t this result in the PID file being written twice?

Now that I think about it, I’m guessing that if the script / daemon function writes the PID file before Play’s server fully initializes then Play will detect a PID file and abort?

Maybe we can copy and adapt the daemon function from an appropriate distro?

[muuki88]

Now that I think about it, I’m guessing that if the script / daemon function writes the PID file before Play’s server fully initializes then Play will detect a PID file and abort?

I think that was the problem (either this or in another flavour). I cannot quiet recall, why two PID files didn't work either. If play want's to have his own PID, so be it.

Maybe we can copy and adapt the daemon function from an appropriate distro?

I'm not sure if this is the way to go. I remember there was an idea to configure the
PID creation behaviour. So you either say

1. Let the systemloader create the PID file
2. My application creates it, it will be here

[dwhjames]

Here’s the daemon function from /etc/rc.d/init.d/functions on centos-6.6.

# A function to start a program.
daemon() {
        # Test syntax.
        local gotbase= force= nicelevel corelimit
        local pid base= user= nice= bg= pid_file=
        local cgroup=
        nicelevel=0
        while [ "$1" != "${1##[-+]}" ]; do
          case $1 in
            '')    echo $"$0: Usage: daemon [+/-nicelevel] {program}"
                   return 1;;
            --check)
                   base=$2
                   gotbase="yes"
                   shift 2
                   ;;
            --check=?*)
                   base=${1#--check=}
                   gotbase="yes"
                   shift
                   ;;
            --user)
                   user=$2
                   shift 2
                   ;;
            --user=?*)
                   user=${1#--user=}
                   shift
                   ;;
            --pidfile)
                   pid_file=$2
                   shift 2
                   ;;
            --pidfile=?*)
                   pid_file=${1#--pidfile=}
                   shift
                   ;;
            --force)
                   force="force"
                   shift
                   ;;
            [-+][0-9]*)
                   nice="nice -n $1"
                   shift
                   ;;
            *)     echo $"$0: Usage: daemon [+/-nicelevel] {program}"
                   return 1;;
          esac
        done

        # Save basename.
        [ -z "$gotbase" ] && base=${1##*/}

        # See if it's already running. Look *only* at the pid file.
        __pids_var_run "$base" "$pid_file"

        [ -n "$pid" -a -z "$force" ] && return

        # make sure it doesn't core dump anywhere unless requested
        corelimit="ulimit -S -c ${DAEMON_COREFILE_LIMIT:-0}"

        # if they set NICELEVEL in /etc/sysconfig/foo, honor it
        [ -n "${NICELEVEL:-}" ] && nice="nice -n $NICELEVEL"

        # if they set CGROUP_DAEMON in /etc/sysconfig/foo, honor it
        if [ -n "${CGROUP_DAEMON}" ]; then
                if [ ! -x /bin/cgexec ]; then
                        echo -n "Cgroups not installed"; warning
                        echo
                else
                        cgroup="/bin/cgexec";
                        for i in $CGROUP_DAEMON; do
                                cgroup="$cgroup -g $i";
                        done
                fi
        fi

        # Echo daemon
        [ "${BOOTUP:-}" = "verbose" -a -z "${LSB:-}" ] && echo -n " $base"

        # And start it up.
        if [ -z "$user" ]; then
           $cgroup $nice /bin/bash -c "$corelimit >/dev/null 2>&1 ; $*"
        else
           $cgroup $nice runuser -s /bin/bash $user -c "$corelimit >/dev/null 2>&1 ; $*"
        fi

        [ "$?" -eq 0 ] && success $"$base startup" || failure $"$base startup"
}

daemon doesn’t write a pid file, it assumes the program will take responsibility for managing its own pid file. This seems to be consistent from other scripts in init.d.

So, Play definitely does the right thing by creating a pid file on startup and deleting it on vm shutdown. It just needs a little help (-Dpidfile.path) with putting it in the right place on a Linux system.

And so it seems that for programs that don’t manage their own pid file, there should be an option to turn on pid tracking in the init script.

[dwhjames]

I should have perused the old issues… I now see that there have been plenty of discussions on this in the past!

[muuki88]

Yeah :( sorry for not pointing out. I'm currently a bit busy. IF you can fix this, it would really awesome. If not, no problem, then we merge this pr and fix it in another.

[muuki88]

So I'm merging this and try to fix this in another pr

[dwhjames]

@muuki88, sorry, I should have replied.

I have a fixed script that works nicely with a Play sample. I was going to work on the case where the process doesn’t manage its own pid file, so the script has to capture it instead.

Also, I’ve got a testing environment set up with Vagrant + Ansible provisioning, and was going to finish this by using Ansible to orchestrate package installation testing, etc. I’ll submit this as a PR too.

[dwhjames]

See #541

[muuki88]

Awesome! No problem :)