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

You should set the pid path via the command line... #7

Closed
docwhat opened this issue Feb 28, 2012 · 17 comments · Fixed by #70
Closed

You should set the pid path via the command line... #7

docwhat opened this issue Feb 28, 2012 · 17 comments · Fixed by #70

Comments

@docwhat
Copy link
Contributor

docwhat commented Feb 28, 2012

It'd be better if the pid path was set via the command line to match the value in the Capfile.

@sosedoff
Copy link
Owner

Maybe a little explanation here? Not getting the case

@docwhat
Copy link
Contributor Author

docwhat commented Feb 28, 2012

I converted a simple capistrano recipe to use unicorn via your gem.

Every time I ran cap deploy it said it couldn't find the PID file.

My config/unicorn/production.rb file only contained a listen and worker_processes directive.

I had to add pid File.expand_path('../../../tmp/pids/unicorn.pid', __FILE__) to make it able to find the pid file.

Before I added the above line, I couldn't find a *.pid file anyplace in my account, so I don't think unicorn even wrote a pidfile.

@sosedoff
Copy link
Owner

You can check out the example, https://github.com/sosedoff/capistrano-unicorn/blob/master/examples/rails3.rb

I dont think its a good idea to pass unicorn pid directly via command line, easier just to define it in your unicorn config.

@docwhat
Copy link
Contributor Author

docwhat commented Feb 28, 2012

Hmm.... if it could detect it was missing and complain, it would be good.

An idea:

In the code I just submitted, there is a while loop that looks for a new_pid. I could add that to the end of unicorn:start and if it doesn't appear, I could complain some how and include instructions for how to fix it (create a file config/unicorn/foo.rb and add pid ... to it).

Or maybe use a regex to see if it's set.

Ideally, the set :unicorn_pid directive would be obsolete because it could identify the pidfile somehow.

@docwhat
Copy link
Contributor Author

docwhat commented Feb 28, 2012

Okay, here's how you can get the pidfile out of the configuration.....

Assuming your real unicorn config file is config/unicorn/production.rb you can use this as a configuration file (pass it in via unicorn -c) and it'll print out the pidfile location:

config_file = "config/unicorn/production.rb"
instance_eval(File.read(config_file), config_file) if config_file

puts set[:pid]

exit(0)

You just have to verify it's set to something and if it isn't, then raise a stink (or error) about it.

@aspiers
Copy link
Contributor

aspiers commented May 8, 2012

Maybe I'm missing something because I'm not sure why this issue was closed - there seems to be a contradiction between the docs and actual out-of-the-box behaviour:

If I create an empty unicorn config file and do cap unicorn:start, unicorn starts without a pid file. This contradicts the README.md which says Default to current_path/tmp/pids/unicorn.pid. On the other hand if unicorn_pid is really a mandatory option then I agree 100% with @docwhat that cap unicorn:start should not succeed without it, and the docs should emphasise that it is required and not claim there is a default.

Furthermore, if unicorn_pid is set to a different value than what's in the unicorn config, then this gem will not work correctly. So again @docwhat is correct that this gem needs to automatically detect the value from the unicorn config, rather than expect the user to correctly configure the same value in deploy.rb, since the latter violates the two core principles of Rails: the DRY rule and Convention over Configuration.

So please can you reopen until this is fixed? Thanks!

@sosedoff
Copy link
Owner

sosedoff commented May 8, 2012

Alright, reopening.

Having something like generating the unicorn config file on the fly (in case if it does not exist) will help to get it started with zero configuration.

@sosedoff sosedoff reopened this May 8, 2012
@aspiers
Copy link
Contributor

aspiers commented May 8, 2012

Thanks ;-)

@aspiers
Copy link
Contributor

aspiers commented Feb 15, 2013

Having something like generating the unicorn config file on the fly (in case if it does not exist)
will help to get it started with zero configuration.

But that still won't fix the violation of the two core principles mentioned above, e.g. if the unicorn config already existed, or if either the unicorn config or capistrano config are modified so that the pid paths no longer match.

@aspiers
Copy link
Contributor

aspiers commented Aug 22, 2013

@sosedoff, @sfsekaran: the trick proposed by @docwhat above for automatically extracting the path to the pid file works great for me. However, there is the remaining question of whether it should be run client-side or server-side. It depends on knowing the path to the unicorn config file, which is currently calculated server-side at deploy time. So we can either:

  1. do the extraction server side just after calculating the config file path, or
  2. we can move both client-side.

I think the 2nd option would result in much cleaner code (and allow extending my unicorn:show_vars debug task to include the pid file path), but it would introduce requirements for the unicorn config file to be (a) present locally client-side, and (b) in a place which capistrano-unicorn knows about. I assume that best and standard practice is to have the unicorn config file checked into the repository we are deploying from, so in the majority of cases it should be possible to satisfy both requirements. In the case where unicorn_pid is not specified and either

  • the unicorn config file only exists server-side or
  • config/deploy.rb doesn't specify its client-side location

there would be no way to make auto-detection work, so at this point the code could simply fall back to the existing sensible default value, i.e. #{current_path}/tmp/pids/unicorn.pid.

Thoughts?

@sfsekaran
Copy link
Contributor

That sounds reasonable. The nice part about this feature is it's mostly unobtrusive to our users, so we can switch it to option 1 if 2 turns out to be a bust.

@aspiers
Copy link
Contributor

aspiers commented Aug 24, 2013

Hmm, I just realised that doing it client-side would also require unicorn to be installed client-side, and maybe that's not a reasonable assumption. And if we went server-side instead, unicorn:show_vars could just show autodetect which would be OK. Thoughts?

@aspiers
Copy link
Contributor

aspiers commented Aug 24, 2013

Ugh, server-side gets ugly fast due to remote_process_exists?. I suppose if unicorn isn't installed client-side it's not a disaster - we just fall back to the pre-existing default.

aspiers pushed a commit to aspiers/capistrano-unicorn that referenced this issue Aug 24, 2013
aspiers pushed a commit to aspiers/capistrano-unicorn that referenced this issue Aug 24, 2013
aspiers pushed a commit to aspiers/capistrano-unicorn that referenced this issue Aug 24, 2013
aspiers pushed a commit to aspiers/capistrano-unicorn that referenced this issue Aug 24, 2013
aspiers pushed a commit to aspiers/capistrano-unicorn that referenced this issue Aug 24, 2013
aspiers pushed a commit to aspiers/capistrano-unicorn that referenced this issue Aug 24, 2013
@aspiers
Copy link
Contributor

aspiers commented Aug 24, 2013

(Sorry about the spam here, had to rebase a few times.)

OK, I have a working fix for this - see #70.

aspiers pushed a commit to aspiers/capistrano-unicorn that referenced this issue Aug 24, 2013
aspiers pushed a commit to aspiers/capistrano-unicorn that referenced this issue Aug 24, 2013
aspiers pushed a commit to aspiers/capistrano-unicorn that referenced this issue Aug 24, 2013
aspiers pushed a commit to aspiers/capistrano-unicorn that referenced this issue Sep 2, 2013
aspiers pushed a commit to aspiers/capistrano-unicorn that referenced this issue Sep 2, 2013
@januszm
Copy link

januszm commented Aug 20, 2014

How this was finally solved?

I keep getting the following error:
... triggering before callbacks for `unicorn:stop' ...
... No such file or directory - config/unicorn/production.rb
*** err :: failed to auto-detect pid from config/unicorn/production.rb
*** err :: falling back to default: #{app_path}/tmp/pids/unicorn.pid

I think it's still trying to detect PID on the client-side which sounds odd. Remote and local configuration may vary and since (I think) most users will send updates to the remote, staging/production servers the expected behaviour is: Capistrano loads and evaluates config vars >>> there <<<

@aspiers
Copy link
Contributor

aspiers commented Aug 20, 2014

@januszm please see e5e4108 and discussions in comments above about client-side vs. server-side.

@docwhat
Copy link
Contributor Author

docwhat commented Aug 20, 2014

I ended up not using this plugin and creating a file on the server (in shared/services/unicorn) that starts, stops, etc. unicorn. This let me see what is going on as well as be able to run it from the server (e.g. on system startup).

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 a pull request may close this issue.

5 participants