Skip to content

[ADDED] Systemd unit file #587

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

Merged
merged 2 commits into from
Sep 25, 2017
Merged

[ADDED] Systemd unit file #587

merged 2 commits into from
Sep 25, 2017

Conversation

RedShift1
Copy link
Contributor

Changes proposed in this pull request:

  • Adds util/gnatsd.service which is a systemd unit file

/cc @nats-io/core

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 91.703% when pulling 1415511 on RedShift1:master into c3e0951 on nats-io:master.

@RedShift1
Copy link
Contributor Author

Can an exclusion be added to not cover this file?

@ColinSullivan1
Copy link
Member

@RedShift1 , thank you for the PR! I can see this having dependencies on the .rpm/.deb installations. Let's keep this open until those are created.

@RedShift1
Copy link
Contributor Author

RedShift1 commented Sep 20, 2017

Well here's the thing. If I add the RPM spec file before the systemd unit file, I have to reference (in the spec file) the unit file that's outside the source. So in order to successfully build the RPM, you'd need the specfile, the systemd unit file and the source tar. When the unit file is added to the source afterwards, I would have to change the spec file to reference the unit file that ships with the source tar.

If we add the systemd unit file to the source before we add the spec file, then I can immediately reference the unit file from the source in the spec file. Done.

Copy link
Contributor

@petemiron petemiron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Only recommendation is to add Reload and Stop commands. Once you've updated, I'll test and confirm.

PrivateTmp=true
Type=simple
PIDFile=/var/run/gnatsd/%i.pid
ExecStart=/usr/sbin/gnatsd -c /etc/gnatsd.conf
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend adding the following commands for standard reload and stop behavior:

ExecReload=/bin/kill -s HUP $MAINPID
ExecStop=/bin/kill -s QUIT $MAINPID

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure the QUIT signal is the right signal to terminate gnatsd correctly? Because gnatsd spams the logs when you send it the QUIT sig: https://pastebin.com/9D4R40b4

Copy link
Member

@kozlovic kozlovic Sep 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SIGINT would probably be better?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

other like TERM would not allow server to cleanly shutdown (which may be OK as a standalone process).

Removes PIDFile (gnatsd doesn't create a pid file) and implements the
Reload and Stop service commands
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 91.853% when pulling 89cbb96 on RedShift1:master into c3e0951 on nats-io:master.

@tylertreat
Copy link
Contributor

lgtm

Copy link
Contributor

@petemiron petemiron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@kozlovic kozlovic merged commit 26ff94c into nats-io:master Sep 25, 2017
@kozlovic kozlovic changed the title Add systemd unit file [ADDED] Systemd unit file Sep 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants