init.d script improvements #3115

KoeSystems · 2015-06-23T23:58:54Z

Objectives

Clarify script changing variables to capital letter.
Allow to overwrite all the variables with default variables.
Check if config file exists and it's readable.
Check if pidfile is writable.

Motivation

The current init script use lower case for some variables which names match with the output messages, changing this variables to capital letter makes the script more standard and more readable.
The current init script does not allow the overwrite of all the variables with environment variables, this would make that some users need to change the init script (instead of add environment variables or edit /etc/default/influxdb) or would make harder to manage influxdb from automatisation systems like puppet.
The config file exists by default ( built from package.sh ), however could exist a race condition in some environments, better if we check if config file exists.
Like point 3, some race condition could happen, better if we check if pidfile is writable.

Changes

Change all the variables in lower case by capital letter.
Move the definition of the variables before load the $DEFAULT file
Check if config file is readable, if not exit 4.
If the pidfile doesn't exists, check if writable doing a touch with the proper user, exit 5 if it fails.

Backward compatibility

No changes in the way influxDB is launched neither how the init script is called neither in the value of default variables, so it's full backward compatible.

Testing

I didn't find integration test over the builded package and the CI test seems to test "only" the code, not the init script.

For this reason I run some manual test:

with empty /etc/default/influxdb

root@vm01:~# service influxdb version 
InfluxDB v0.9.0 (git: .... )

root@vm01:~# service influxdb status
influxdb Process is not running [ FAILED ]

root@vm01:~# service influxdb start
Starting the process influxdb [ OK ]
influxdb process was started [ OK ]

root@vm01:~# service influxdb start
influxdb process is running [ FAILED ]

root@vm01:~# service influxdb stop
influxdb process was stopped [ OK ]

root@vm01:~# service influxdb stop
influxdb process is not running [ FAILED ]

with some config in /etc/default/influxdb

root@vm01:~# cat /etc/default/influxdb 
CONFIG=/opt/influxdb/etc/influxdb.conf

root@vm01:~# service influxdb start
config file doesn't exists [ FAILED ]

root@vm01:~# cat /etc/default/influxdb 
PIDFILE=/var/run/influxdb.pid

root@vm01:~# service influxdb start
/var/run/influxdb.pid now writable, check permissions [ FAILED ]

root@vm01:~# cat /etc/default/influxdb
CONFIG=/etc/opt/influxdb/influxdb.conf
PIDFILE=/var/run/influxdb/influxdb.pid

root@vm01:~# service influxdb start
Starting the process influxdb [ OK ]
influxdb process was started [ OK ]

otoolep · 2015-06-24T00:00:34Z

Thanks @KoeSystems -- we'll definitely take a look. Have you signed the CLA?

otoolep · 2015-06-24T00:01:07Z

scripts/init.sh

 case $1 in
    start)
+        # Check if config file exists
+        if [ ! -r $CONFIG ]; then
+            log_failure_msg "config file doesn't exists"


exists -> exist

KoeSystems · 2015-06-24T21:39:16Z

@otoolep I've just sign the CLA.

otoolep · 2015-06-25T20:42:08Z

scripts/init.sh

+# Daemon name, where is the actual executable
+# If the daemon is not there, then exit.
+DAEMON=/opt/influxdb/influxd
+[ -x $DAEMON ] || exit 5


5 is the standard return code?

As far as I know, there isn't standard exit codes in bash except 0,1 and 2 ... I saw you were using exit 5 when the daemon doesn't exist so I use the same when PID is not writable. I add too "exit 4" when config file is not present. I didn't change the exit code when DAEMON doesn't exits.

I respected the exit codes to ensure the backward compatibility and because I think that it doesn't really matter (except for 0 , 1 or 2).

Sounds good.

otoolep · 2015-06-25T20:44:25Z

Thanks again @KoeSystems -- can you rebase? We had to merge in some other changes to this file and there is now a conflict.

rhoml · 2015-07-07T13:27:23Z

scripts/init.sh

-if [ -r $DEFAULT ]; then
-    source $DEFAULT
+if [ -r /lib/lsb/init-functions ]; then
+    source /lib/lsb/init-functions


Isn't this going to break RedHat installations?

This is still working for RedHat flavours as you can see here init script test

Btw, you can check in the current raw init file that we already load /lib/lsb/init-functions if exists, is something that this PR does not change.

I'm not a RedHat / CentOS user but may need an elif and source /etc/init.d/functions. Then below, also check for daemon function before falling back to starting $DAEMON directly. (more below)

pauldix · 2015-07-23T19:57:51Z

@KoeSystems if you can rebase we'll merge this in right away. Thanks!

KoeSystems · 2015-08-04T13:56:47Z

@pauldix @otoolep Rebase done, thanks!

otoolep · 2015-08-13T18:30:52Z

@KoeSystems -- unfortunately the patch has conflict markers embedded within it i.e. >>>> and this change is not mergeable. If you can fix that I will then merge.

Allow to overwrite all the variables with environment variables. Check if config file exists and it's readable. Check if pidfile is writable.

KoeSystems · 2015-08-13T22:10:05Z

Done, sorry for the fail when I did the rebase. Now should works.

otoolep · 2015-08-13T23:54:50Z

Tested this on a Digital Ocean droplet and all functionality remains in place. Thanks @KoeSystems

init.d script improvements

KiNgMaR · 2015-08-21T12:18:58Z

scripts/init.sh

-        else
-            log_success_msg "$name process was started"
-            exit 0
+            nohup $DAEMON -pidfile $PIDFILE -config $CONFIG $INFLUXD_OPTS >>$STDOUT 2>>$STDERR &


Is the removal of su here an intentional change? Now, on CentOS 6 and 7, where start-stop.daemon is not available, InfluxDB runs as root if started via init.d script. Prepending sudo -g $GROUP -u $USER to this line is a workaround.

May need another which daemon check here and if available, do something like...

daemon --user=$USER:$GROUP --pidfile=$PIDFILE ...

Then fall back to starting $DAEMON directly if daemon function isn't available. su may have problems if $USER is the same as the user running the init script.

KoeSystems · 2015-08-23T00:20:24Z

I've just open an issue for this 3803

KoeSystems · 2015-08-23T00:24:50Z

I created a new PR 3804 with the solution.

otoolep reviewed Jun 24, 2015
View reviewed changes

otoolep reviewed Jun 25, 2015
View reviewed changes

rhoml reviewed Jul 7, 2015
View reviewed changes

otoolep self-assigned this Aug 13, 2015

Clarify script changing variables to capital letter.

88307bf

Allow to overwrite all the variables with environment variables. Check if config file exists and it's readable. Check if pidfile is writable.

otoolep added a commit that referenced this pull request Aug 13, 2015

Merge pull request #3115 from KoeSystems/init-script-improvements

1037b17

init.d script improvements

otoolep merged commit 1037b17 into influxdata:master Aug 13, 2015

KiNgMaR reviewed Aug 21, 2015
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

init.d script improvements #3115

init.d script improvements #3115

KoeSystems commented Jun 23, 2015

otoolep commented Jun 24, 2015

otoolep Jun 24, 2015

KoeSystems commented Jun 24, 2015

otoolep Jun 25, 2015

KoeSystems Jun 25, 2015

otoolep Jun 25, 2015

otoolep commented Jun 25, 2015

rhoml Jul 7, 2015

KoeSystems Jul 7, 2015

rhoml Jul 8, 2015

dgnorton Aug 21, 2015

pauldix commented Jul 23, 2015

KoeSystems commented Aug 4, 2015

otoolep commented Aug 13, 2015

KoeSystems commented Aug 13, 2015

otoolep commented Aug 13, 2015

KiNgMaR Aug 21, 2015

dgnorton Aug 21, 2015

KoeSystems commented Aug 23, 2015

KoeSystems commented Aug 23, 2015

init.d script improvements #3115

init.d script improvements #3115

Conversation

KoeSystems commented Jun 23, 2015

Objectives

Motivation

Changes

Backward compatibility

Testing

otoolep commented Jun 24, 2015

Choose a reason for hiding this comment

KoeSystems commented Jun 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otoolep commented Jun 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pauldix commented Jul 23, 2015

KoeSystems commented Aug 4, 2015

otoolep commented Aug 13, 2015

KoeSystems commented Aug 13, 2015

otoolep commented Aug 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KoeSystems commented Aug 23, 2015

KoeSystems commented Aug 23, 2015