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

Better upstart script #120

Merged
merged 3 commits into from
Jan 21, 2014
Merged

Better upstart script #120

merged 3 commits into from
Jan 21, 2014

Conversation

dbathily
Copy link
Contributor

@dbathily dbathily commented Jan 4, 2014

Add a stop stanzas (http://upstart.ubuntu.com/cookbook/#job-with-start-on-but-no-stop-on) and use start-stop-daemon as recommended in the upstart cookbook (http://upstart.ubuntu.com/cookbook/#changing-user)

Some server applications (like those builded with play framework) will not launch after a reboot/shutdown without a proper stop because they create a pid file and don't boot if this file exists. For those applications, a stop before system's shutdown is mandatory.

Also, for those applications, we can improve the script by adding a "pre start" Stanzas that will delete the pid file if it exist and if there haven't a running process with the corresponding id. That's why I think the feature #118 will be useful. Playframework will provide his upstart script.

@lightbend-cla-validator
Copy link

Hi @dbathily,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Typesafe Contributors License Agreement:

http://typesafe.com/contribute/cla

@dbathily
Copy link
Contributor Author

dbathily commented Jan 4, 2014

Done

@@ -11,17 +11,20 @@ author "${{author}}"
# When to start the service
start on runlevel [2345]

# When to stop the service
stop on runlevel [016]

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

@aparkinson
Copy link
Contributor

By default Java processes aren't daemonized and you would have to use start-stop-daemon or Apache Daemon to do it. Upstart is designed so you don't have to daemonize processes:

'Upstart will keep track of the process ID that it thinks belongs to a job. If a job has specified the instance stanza, Upstart will track the PIDs for each unique instance of that job.' [1]

'If your daemon has a "don't daemonize" or "run in the foreground" mode, then it's much simpler to use that and not run with fork following. One issue with that though, is that Upstart will emit the started JOB=yourjob event as soon as it has executed your daemon, which may be before it has had time to listen for incoming connections or fully initialize.' [1]

[1] http://upstart.ubuntu.com/cookbook/#expect

@dbathily
Copy link
Contributor Author

dbathily commented Jan 5, 2014

Keep only the stop Stanzas?

@muuki88
Copy link
Contributor

muuki88 commented Jan 5, 2014

IMHO we should keep the stop stanza ( I ran into the same issue like you ).
However the other peace of code may be of interest for people with legacy systems ( are there that many @jsuereth ? )

A small hint in the documentation/Readme ( especially how it works with play ) would be awesome too.
I try to work on #98 next week, so it should be easier to append Java_opts.

@jsuereth
Copy link
Member

jsuereth commented Jan 5, 2014

Haven't really decided on a support policy. For Ubuntu, I think the default should support the last LTS release. Anything past that is "gravy" (if it's easy, good, but otherwise folks using older Ubuntu could provide their own upstart scripts, as they would have had to before this plugin).

Also, thanks much for this patch! I'll be looking into it and doing a milestone release for this plugin on monday.

@muuki88
Copy link
Contributor

muuki88 commented Jan 16, 2014

@dbathily can make the change, so only the

# When to stop the service
stop on runlevel [016]

remains. The rest could be discussed in another ticket/pull request.

@dbathily
Copy link
Contributor Author

Done ;)

@muuki88
Copy link
Contributor

muuki88 commented Jan 16, 2014

Thanks :-)

@jsuereth
Copy link
Member

Looks great! Thanks.

jsuereth added a commit that referenced this pull request Jan 21, 2014
@jsuereth jsuereth merged commit 1832e47 into sbt:master Jan 21, 2014
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.

None yet

5 participants