-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
init.d script improvements #3115
init.d script improvements #3115
Conversation
Thanks @KoeSystems -- we'll definitely take a look. Have you signed the CLA? |
case $1 in | ||
start) | ||
# Check if config file exists | ||
if [ ! -r $CONFIG ]; then | ||
log_failure_msg "config file doesn't exists" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exists -> exist
@otoolep I've just sign the CLA. |
# Daemon name, where is the actual executable | ||
# If the daemon is not there, then exit. | ||
DAEMON=/opt/influxdb/influxd | ||
[ -x $DAEMON ] || exit 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5 is the standard return code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
Thanks again @KoeSystems -- can you rebase? We had to merge in some other changes to this file and there is now a conflict. |
if [ -r $DEFAULT ]; then | ||
source $DEFAULT | ||
if [ -r /lib/lsb/init-functions ]; then | ||
source /lib/lsb/init-functions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this going to break RedHat installations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems fair
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
@KoeSystems if you can rebase we'll merge this in right away. Thanks! |
@KoeSystems -- unfortunately the patch has conflict markers embedded within it i.e. |
Allow to overwrite all the variables with environment variables. Check if config file exists and it's readable. Check if pidfile is writable.
Done, sorry for the fail when I did the rebase. Now should works. |
Tested this on a Digital Ocean droplet and all functionality remains in place. Thanks @KoeSystems |
else | ||
log_success_msg "$name process was started" | ||
exit 0 | ||
nohup $DAEMON -pidfile $PIDFILE -config $CONFIG $INFLUXD_OPTS >>$STDOUT 2>>$STDERR & |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
I've just open an issue for this 3803 |
I created a new PR 3804 with the solution. |
Objectives
Motivation
Changes
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 some config in /etc/default/influxdb