Skip to content
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

Add support for a RedHat sysv init script #196

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Add support for a RedHat sysv init script #196

wants to merge 11 commits into from

Conversation

dwerder
Copy link

@dwerder dwerder commented Apr 26, 2014

This patch seperates init scripts into deb LSB and rpm SYSV versions. The sysv version uses nohup to work around the daemonizing problem discussed in #195 (program should daemonized itself)

There are also rpm init scripts out there, that implement systemd support (#175) . So there is the question if redhat <6 native init or 7+ should be supported. Better would be to split the rpm build process into rpms for redhat 5,6 and 7+.

This PR replaces #169 .

@ShimiTaNaka
Copy link

I have tested these changes with the latest code and the build process looks fine, can this be re-tested for pull?

@pmq
Copy link

pmq commented Jul 1, 2014

Hi Daniel, thank you for this patch. I had to change the use of status to be more clear : pmq@445eb6c

I also had to make the init.d script executable - didn't have the time to investigate so we corrected this with Ansible for now. The script could also be patched to display the [OK] when starting.

Apart from that, we run this on CentOS 6.5 and it's working.

@pmq
Copy link

pmq commented Jul 1, 2014

About the chmod a+x : done in pmq@3a783e6

@vestimir
Copy link

Hello, what's the status of this PR. Is this going to be merged anytime soon? /cc @jordansissel

@windowsrefund
Copy link

Regarding the redhat init script:

Should DAEMON_ARGS on line 18 look for $LOGSTASH_FORWARDER_OPTIONS? I believe that is the name of the variable which is maintained in /etc/sysconfig/logstash-forwarder

@m01
Copy link

m01 commented Aug 7, 2014

I'd also be interested in this being merged.

@windowsrefund: I think the DAEMON_ARGS were based on the existing init script code; if that should be changed it should probably be changed in both files.

PS: it looks like all Travis CI builds are failing for this project..

@jordansissel
Copy link
Contributor

PS: it looks like all Travis CI builds are failing for this project..

Travis hasn't been used for this project in quite some time. I just can't figure out how to disable travis and delete its history, so it continues to confuse users. Apologies for the confusion.

@jordansissel
Copy link
Contributor

btw, my intent is to generate all the possible "init" scripts for this project with pleaserun.

For folks interested in redhat support, I highly encourage you to test and even deploy the patch in this PR.

If you want it merged, I would love folks to say "I am using this in production and it works for me." instead of "I want this idea implemented" :P

@m01
Copy link

m01 commented Aug 7, 2014

So, in addition to the code as it is in the pull request we've made 2 trivial changes:

  1. chmod +x on the RedHat init script, as suggested by @pmq
  2. change the config file name in the DAEMON_ARGS to /etc/logstashforwarder/config.json for compatibility with the logstashforwarder puppet module's logstashforwarder::config class). I think @windowsrefund' suggestion of using $LOGSTASH_FORWARDER_OPTIONS sounds like a more correct way of implementing this change, but I haven't tested that.

Based on my testing it (latest master + this pull request + the 2 changes mentioned above) works on RHEL5.9, 6.5 and CentOS6.5. I wouldn't expect it to work on RHEL7/CentOS7 since I believe they introduced systemd there.

I think it'd be good to merge this pull request, and then to set the executable bit. That would be a worthwhile improvement over the status quo :-)

I don't think it's necessary to do anything about 2. for this pull request to be merged.

Thanks for clarifying the TravisCI status!

@dwerder
Copy link
Author

dwerder commented Aug 8, 2014

@jordansissel Ok, i use this in production on CentOS 6.5 machines. ;-)

@pmq
Copy link

pmq commented Aug 8, 2014

As I previously wrote, we use this in production on CentOS 6.5. No problem arose (size of the sample, 200 or so servers). We use our 'mirakl' branch at https://github.com/pmq/logstash-forwarder but it boils down to two trivial changes for this specific issue:

@geerlingguy
Copy link

Another 'me too' - using this on production and dev environments with CentOS 6.5, works perfectly, please merge the change :)

For now, in my Ansible logstash-forwarder role, I'm simply including the init scripts directly, but I'd like to be able to link to the main elasticsearch/logstash-forwarder repo so I don't have to keep those init scripts up to date.

@geerlingguy
Copy link

One quick note on the init script: In the official init script, the DAEMON_ARGS line defines the config file without .conf, whereas the rpm file in this PR uses .conf. To make everything consistent across distributions, one of these two standards should be used (and I'm guessing the official method is more used—without the .conf file).

Main repository init file (link):

DAEMON_ARGS="-config /etc/logstash-forwarder -spool-size 100 -log-to-syslog"

.rpm init file in this PR (link):

DAEMON_ARGS="${DAEMON_ARGS:--config /etc/logstash-forwarder.conf -spool-size 100 -log-to-syslog}"

I don't know if we'd necessarily want to move the file into /etc/logstashforwarder/config.json to comply with a CM-tool-specific convention—that would be a separate discussion/PR, I think. It would be best to follow whatever convention is used by Logstash itself.

@jerrac
Copy link

jerrac commented Aug 29, 2014

I just tried to run make rpm with this pull request checked out. It failed.

[root@vm logstash-forwarder]# make rpm
Makefile:28: warning: overriding commands for target `clean'
Makefile.ext:15: warning: ignoring old commands for target `clean'
Go version 1.1.x (or higher) required, you have a version of go that is too old See http://golang.org/doc/install for upgrading.
make: *** [go-check] Error 1


[root@vm logstash-forwarder]# go version
go version go1.3.1 linux/amd64
[root@vm logstash-forwarder]# 

Yesterday, I successfully built a working rpm from HEAD on this Centos 6.5 vm.

Suggestions?

@alphazero
Copy link
Contributor

@jerrac Hi David. The patch's make file is doing a simple grep. Change that line to conform to master.

@jerrac
Copy link

jerrac commented Aug 30, 2014

@alphazero That let me compile it. Thanks.

Unfortunately, the init script doesn't work without some edits. (Like in #196 (comment) and #196 (comment))

First, I had to give it +x perms. Then I had to remove the .conf from the daemon args line.

That let it start, but /var/log/messages is now complaining about tls issues...

Aug 30 13:16:44 shipperhost logstash-forwarder[24772]: 2014/08/30 13:16:44.652052 Connecting to testindexerip:5043 (log-test-node-01.lanecc.edu) 
Aug 30 13:16:44 shipperhost logstash-forwarder[24772]: 2014/08/30 13:16:44.652305 Failed to tls handshake with testindexerip tls: either ServerName or InsecureSkipVerify must be specified in the tls.Config

@jordansissel What exactly needs to be done to get pleaserun working for this?

@pmq
Copy link

pmq commented Aug 30, 2014

@jerrac as the message explains, you will probably need to patch like this: pmq@969626e

@driskell
Copy link
Contributor

@jerrac make sure to use the master verson of logstash-forwarder and it will work assuming certificates are correct.

Looks like you may be compiling an old version.

@pmq suggestion is to disable TLS security altogether which I don't agree with.

@jerrac
Copy link

jerrac commented Sep 1, 2014

@driskell Ah, yeah, first time testing a pull request, so I just checked it out directly. Didn't think to merge it into a copy of master. :) Doing that, plus adding +x perms, and deleting the .conf in the daemon args made it work. Thanks.

So, it seems like @dwerder needs to update the request with those changes? Or someone needs to make a new pull request?

@ejber-ozkan
Copy link

Erm 👍
... CentOS 6.5 also in production , a merge would be nice :)

@wjimenez5271
Copy link

Yes, using this currently and it works well. +1 for merge.

@driskell
Copy link
Contributor

driskell commented Nov 6, 2014

Just to add, the rpm script isn't compliant with the sysv standard.

condrestart shouldn't call restart unless the service is started, so if it's stopped it shouldn't start it. This needs a status check before calling restart where it looks to just hammer the subsys lock and restart blindly.

It should also set logstash forwarder stdin to </dev/null just in case there's a stdin config set, otherwise if someone bypasses /sbin/service and runs the init.d directly so it inherits their env and fds (dirty but lots of ppl don't realise the difference) it might cause funky things to happen heh.

I'm a fan of LSB stanza in the sysv script too. Chkconfig on RedHat/CentOS understands it.

@driskell
Copy link
Contributor

driskell commented Nov 6, 2014

To elaborate on condrestart - if subsys is locked but service not running (killed or crashed) it still shouldn't restart as far as I know.

@driskell
Copy link
Contributor

driskell commented Nov 6, 2014

Also a start failure or condrestart failure doesn't seem to propagate exit code. (False positive, sorry)

Be good to also add try-restart as identical to condrestart - it's fedora packaging compatible then and could be used in future packaging 👍

http://fedoraproject.org/wiki/Packaging:SysVInitScript

@Rumbles
Copy link

Rumbles commented Nov 6, 2014

I have just been trying this script out, as described above I had to remove .conf from the DAEMON_ARGS line, I have tested the script in CentOS 5.11 and 6.5 and it works well, I have tested it after a reboot as well, as the issue I was having was that the init script I had been using wasn't working correctly with chkconfig.

I have noticed one minor issue though, when I invoke start on this script I don't get a confirmation that the service has started, I just get "Starting logstash-forwarder" using the stop command returns the [ ok ] at the end as is normal with an init script. I know it's not a major problem, but I thought it should be noted regardless.

Thanks for the great software people!

@driskell
Copy link
Contributor

driskell commented Nov 6, 2014

Calling "success" between line 36 and 37, if RET is 0, will fix Rumbles problem.

@Rumbles
Copy link

Rumbles commented Nov 6, 2014

I've just noticed that when using --config /etc/logstash-forwarder the agent dies upon launch on both CentOS 5 and 6, but if I set it to --config /etc/logstash-forwarder/logstash-forwarder.conf it works fine.

@Rumbles
Copy link

Rumbles commented Nov 6, 2014

Ahhhhh yeah, that last commit fixed the reporting of whether the service started up okay. I had to make a small change as described before. I had to change my DEAMON_ARGS line to:

DAEMON_ARGS="${DAEMON_ARGS:--config /etc/logstash-forwarder/logstash-forwarder.conf -spool-size 100}"

Partly because I don't want to log to syslog as I harvest them, this creates a feedback loop (reporting 1 log collected, which is then collected and reported, etc)

If I leave it just pointing to the folder my logstash-forwarder dies leaving the pid file (dead but pid file exists)

Thanks for the update!

@dwerder
Copy link
Author

dwerder commented Nov 6, 2014

@driskell Ok, no more excuses :D The script is fully SysV compatible now.

@Rumbles You dont have to change the init script defaults. The clean way ist to set your own DAEMON_ARGS in
/etc/default/logstash-forwarder or
/etc/sysconfig/logstash-forwarder

Thats how you should configure your service.

Regards

@Rumbles
Copy link

Rumbles commented Nov 6, 2014

@dwerder you're completely right, I have added that now and it invokes perfectly. Thanks for teaching me something!

@stdweird
Copy link

thanks for this. much appreciated and works like a charm.
(although unrelated to RH sysv init) a Makefile variable to control the fpm iteration would also be nice, then i don't have to do ugly things like

tag=`git log --format=%ct.%h -1`
version=`grep VERSION= Makefile |sed "s/.*=//"`
make rpm VERSION="$version --iteration $tag.ug"

when making rpms from master.

@stdweird
Copy link

hmm, tiny remark. the logstash-forwarder.rpm.init should have executable permission as a result, so either set the executable permisison in git repo (as with the deb.init) or add similar make target as e.g. build/bin/logstash-forwarder.sh

@tomsommer
Copy link

Any chance we can get this merged and a new .rpm packaged? The current 0.3.1 rpm is useless on redhat-like OS'es with the debian init file.

@Rumbles
Copy link

Rumbles commented Dec 9, 2014

In the mean time you can just copy and past the new init script in to your init script on CentOS, copy from here:

https://raw.githubusercontent.com/echocat/logstash-forwarder/d0727cd17b99775ea4bec7c1546c730be8b30cdc/logstash-forwarder.rpm.init

@stdweird
Copy link

stdweird commented Dec 9, 2014

or build your own

yum install -y golang ruby-devel rubygems
git clone https://github.com/elasticsearch/logstash-forwarder.git
cd logstash-forwarder

# merge this PR
git fetch origin pull/196/head:redhatinit
git merge redhatinit

go build
gem install fpm
tag=`git log --format=%ct.%h -1`
version=`grep VERSION= Makefile |sed "s/.*=//"`
make rpm VERSION="$version --iteration $tag.myownrpm"

@tomsommer
Copy link

I realise both, but that wasn't my point.

@beneggett
Copy link

+1 wanting this to merge ...and commenting to watch ;)

@tomsommer
Copy link

I'm wondering if the script should not set the current working directory aswell? For the .logstash-forwarder file?

@huiyiqun
Copy link

work on centos-6.6

@huiyiqun
Copy link

This pull request does not work on centos 7:

# systemctl start logstash-forwarder
# systemctl status logstash-forwarder
logstash-forwarder.service - LSB: start and stop logstash-forwarder
   Loaded: loaded (/etc/rc.d/init.d/logstash-forwarder)
   Active: failed (Result: exit-code) since Sat 2015-01-31 22:53:55 CST; 4s ago
  Process: 10601 ExecStop=/etc/rc.d/init.d/logstash-forwarder stop (code=exited, status=0/SUCCESS)
  Process: 10597 ExecStart=/etc/rc.d/init.d/logstash-forwarder start (code=exited, status=0/SUCCESS)
 Main PID: 10598 (code=exited, status=2)

Jan 31 22:53:55 localhost systemd[1]: Starting LSB: start and stop logstash-forwarder...
Jan 31 22:53:55 localhost logstash-forwarder[10597]: Starting logstash-forwarder: [  OK  ]
Jan 31 22:53:55 localhost systemd[1]: Started LSB: start and stop logstash-forwarder.
Jan 31 22:53:55 localhost systemd[1]: logstash-forwarder.service: main process exited, code=exited, status=2/INVALIDARGUMENT
Jan 31 22:53:55 localhost systemd[1]: Unit logstash-forwarder.service entered failed state.

and when I try to restart it, it failed directly:

# systemctl restart logstash-forwarder
Job for logstash-forwarder.service failed. See 'systemctl status logstash-forwarder.service' and 'journalctl -xn' for details.
# systemctl status logstash-forwarder
...
Jan 31 22:54:06 shinken4 systemd[1]: Starting LSB: start and stop logstash-forwarder...
Jan 31 22:54:06 shinken4 logstash-forwarder[10613]: Starting logstash-forwarder: [  OK  ]
Jan 31 22:54:06 shinken4 systemd[1]: PID 10614 read from file /var/run/logstash-forwarder.pid is a zombie.
Jan 31 22:54:06 shinken4 systemd[1]: logstash-forwarder.service never wrote its PID file. Failing.
Jan 31 22:54:06 shinken4 systemd[1]: Failed to start LSB: start and stop logstash-forwarder.
Jan 31 22:54:06 shinken4 systemd[1]: Unit logstash-forwarder.service entered failed state.
...

I doubt if it failed when I tried to start because I had not configured logstash-forwarder, while systemd was supposed to reponse with a error.

The failure happened when I tried to restart seems failed to deal with pid file.

@m01
Copy link

m01 commented Apr 17, 2015

@jordansissel I saw you comment in PR #330 that init scripts are no longer needed inside the repo, I assume that means this PR is also obsolete then?

@andrewkroh
Copy link
Member

Filebeat provides an SysV-style init script for RHEL/CentOS. It does not yet have systemd scripts for RHEL/CentOS 7. The packaging code and init scripts are in the beats-packer repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.