unicorn Ruby/Rack server user+dev discussion/patches/pulls/bugs/help
 help / color / mirror / code / Atom feed
* Master hooks needed
@ 2014-10-03 11:34 Bráulio Bhavamitra
  2014-10-03 12:22 ` Eric Wong
  0 siblings, 1 reply; 11+ messages in thread
From: Bráulio Bhavamitra @ 2014-10-03 11:34 UTC (permalink / raw)
  To: unicorn-public

Hello all,

If I need to hook something after master load, I'm currently doing:

before_fork do |server, worker|
  # worker 0 is the first to init, so hold the master here
  if worker.nr == 0
     #warm up server...

     #kill old pid...
  end

  # other stuff for each worker
end

Both operations I currently do (server warm up and old pid kill) need
to be run only once, and not for every worker as before_fork does,
that's why I had to put the condition seen above.

So hooks for master is needed, something like
master_after_load(server) and master_init(server).

What do you think?

cheers,
bráulio

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Master hooks needed
  2014-10-03 11:34 Master hooks needed Bráulio Bhavamitra
@ 2014-10-03 12:22 ` Eric Wong
  2014-10-03 12:36   ` Valentin Mihov
  2014-10-04  0:53   ` Bráulio Bhavamitra
  0 siblings, 2 replies; 11+ messages in thread
From: Eric Wong @ 2014-10-03 12:22 UTC (permalink / raw)
  To: Bráulio Bhavamitra; +Cc: unicorn-public

Bráulio Bhavamitra <braulio@eita.org.br> wrote:
> Hello all,
> 
> If I need to hook something after master load, I'm currently doing:
> 
> before_fork do |server, worker|
>   # worker 0 is the first to init, so hold the master here
>   if worker.nr == 0
>      #warm up server...
> 
>      #kill old pid...
>   end
> 
>   # other stuff for each worker
> end
> 
> Both operations I currently do (server warm up and old pid kill) need
> to be run only once, and not for every worker as before_fork does,
> that's why I had to put the condition seen above.

The above is fine if your first worker never dies.  I think you can add
a local variable to ensure it only runs the first time worker.nr == 0 is
started, in case a worker dies.  Something like:

    first = true
    before_fork do |server, worker|
      # worker 0 is the first to init, so hold the master here
      if worker.nr == 0 && first
         first = false
         #warm up server...

         #kill old pid...
      end

      # other stuff for each worker
    end

For what it's worth, I'm not a fan of auto-killing the old PID in the
unicorn config and regret having it in the example config.  It's only
for the most memory-constrained configs and fragile (because anything
with pid files is always fragile).

> So hooks for master is needed, something like
> master_after_load(server) and master_init(server).
> 
> What do you think?

rack.git also has a Rack::Builder#warmup method.  Aman originally
proposed it for unicorn, but it's useful outside of unicorn so
we moved it to Rack.

In general, I'm against adding new hooks/options because they tend to
make maintainability and documentation harder for ops folks.
I still have nightmares of some Capistrano config filled with hooks
from years ago :x

Features like these also makes migrating away from unicorn harder, so
that is another reason we ended up adding #warmup to Rack and not
unicorn.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Master hooks needed
  2014-10-03 12:22 ` Eric Wong
@ 2014-10-03 12:36   ` Valentin Mihov
  2014-10-03 17:50     ` Eric Wong
  2014-10-04  0:53   ` Bráulio Bhavamitra
  1 sibling, 1 reply; 11+ messages in thread
From: Valentin Mihov @ 2014-10-03 12:36 UTC (permalink / raw)
  To: Eric Wong; +Cc: Bráulio Bhavamitra, unicorn-public

On Fri, Oct 3, 2014 at 3:22 PM, Eric Wong <e@80x24.org> wrote:
>
> Bráulio Bhavamitra <braulio@eita.org.br> wrote:
> > Hello all,
> >
> > If I need to hook something after master load, I'm currently doing:
> >
> > before_fork do |server, worker|
> >   # worker 0 is the first to init, so hold the master here
> >   if worker.nr == 0
> >      #warm up server...
> >
> >      #kill old pid...
> >   end
> >
> >   # other stuff for each worker
> > end
> >
> > Both operations I currently do (server warm up and old pid kill) need
> > to be run only once, and not for every worker as before_fork does,
> > that's why I had to put the condition seen above.
>
> The above is fine if your first worker never dies.  I think you can add
> a local variable to ensure it only runs the first time worker.nr == 0 is
> started, in case a worker dies.  Something like:
>
>     first = true
>     before_fork do |server, worker|
>       # worker 0 is the first to init, so hold the master here
>       if worker.nr == 0 && first
>          first = false
>          #warm up server...
>
>          #kill old pid...
>       end
>
>       # other stuff for each worker
>     end
>
> For what it's worth, I'm not a fan of auto-killing the old PID in the
> unicorn config and regret having it in the example config.  It's only
> for the most memory-constrained configs and fragile (because anything
> with pid files is always fragile).
>
> > So hooks for master is needed, something like
> > master_after_load(server) and master_init(server).
> >
> > What do you think?
>
> rack.git also has a Rack::Builder#warmup method.  Aman originally
> proposed it for unicorn, but it's useful outside of unicorn so
> we moved it to Rack.
>
> In general, I'm against adding new hooks/options because they tend to
> make maintainability and documentation harder for ops folks.
> I still have nightmares of some Capistrano config filled with hooks
> from years ago :x
>
> Features like these also makes migrating away from unicorn harder, so
> that is another reason we ended up adding #warmup to Rack and not
> unicorn.
>

Isn't it much better to do the warmup in an initializer instead in
unicorn? This way you can preload_app=true and the master will execute
the warmup code and fork. Killing the old pid is probably stopping you
from do that, right?

--Valentin

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Master hooks needed
  2014-10-03 12:36   ` Valentin Mihov
@ 2014-10-03 17:50     ` Eric Wong
  2014-10-03 18:12       ` Valentin Mihov
  0 siblings, 1 reply; 11+ messages in thread
From: Eric Wong @ 2014-10-03 17:50 UTC (permalink / raw)
  To: Valentin Mihov; +Cc: Bráulio Bhavamitra, unicorn-public

Valentin Mihov <valentin.mihov@gmail.com> wrote:
> On Fri, Oct 3, 2014 at 3:22 PM, Eric Wong <e@80x24.org> wrote:
> > Bráulio Bhavamitra <braulio@eita.org.br> wrote:
> > > Both operations I currently do (server warm up and old pid kill) need
> > > to be run only once, and not for every worker as before_fork does,
> > > that's why I had to put the condition seen above.
> >
> > rack.git also has a Rack::Builder#warmup method.  Aman originally
> > proposed it for unicorn, but it's useful outside of unicorn so
> > we moved it to Rack.
> 
> Isn't it much better to do the warmup in an initializer instead in
> unicorn? This way you can preload_app=true and the master will execute
> the warmup code and fork. Killing the old pid is probably stopping you
> from do that, right?

Right, that's exactly what Rack::Builder#warmup does with
preload_app=true.  If by "initializer" you mean within the application,
the problem was it lacked the visibility of the entire Rack middleware
stack which Rack::Builder has.

ref:
http://bogomips.org/unicorn-public/m/571f2d6c6f4b8df644bd84f069fafa5bde3cde2e
http://bogomips.org/unicorn-public/m/20130923105807.GA24712%40dcvr.yhbt.net

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Master hooks needed
  2014-10-03 17:50     ` Eric Wong
@ 2014-10-03 18:12       ` Valentin Mihov
  0 siblings, 0 replies; 11+ messages in thread
From: Valentin Mihov @ 2014-10-03 18:12 UTC (permalink / raw)
  To: Eric Wong; +Cc: Bráulio Bhavamitra, unicorn-public

On Fri, Oct 3, 2014 at 8:50 PM, Eric Wong <e@80x24.org> wrote:
> Valentin Mihov <valentin.mihov@gmail.com> wrote:
>> On Fri, Oct 3, 2014 at 3:22 PM, Eric Wong <e@80x24.org> wrote:
>> > Bráulio Bhavamitra <braulio@eita.org.br> wrote:
>> > > Both operations I currently do (server warm up and old pid kill) need
>> > > to be run only once, and not for every worker as before_fork does,
>> > > that's why I had to put the condition seen above.
>> >
>> > rack.git also has a Rack::Builder#warmup method.  Aman originally
>> > proposed it for unicorn, but it's useful outside of unicorn so
>> > we moved it to Rack.
>>
>> Isn't it much better to do the warmup in an initializer instead in
>> unicorn? This way you can preload_app=true and the master will execute
>> the warmup code and fork. Killing the old pid is probably stopping you
>> from do that, right?
>
> Right, that's exactly what Rack::Builder#warmup does with
> preload_app=true.  If by "initializer" you mean within the application,
> the problem was it lacked the visibility of the entire Rack middleware
> stack which Rack::Builder has.
>
> ref:
> http://bogomips.org/unicorn-public/m/571f2d6c6f4b8df644bd84f069fafa5bde3cde2e
> http://bogomips.org/unicorn-public/m/20130923105807.GA24712%40dcvr.yhbt.net

I was thinking about a Rails initializer, but what you pointed to is a
generalization of that. So, we are talking about the same thing.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Master hooks needed
  2014-10-03 12:22 ` Eric Wong
  2014-10-03 12:36   ` Valentin Mihov
@ 2014-10-04  0:53   ` Bráulio Bhavamitra
  2014-10-04  1:22     ` Eric Wong
  1 sibling, 1 reply; 11+ messages in thread
From: Bráulio Bhavamitra @ 2014-10-04  0:53 UTC (permalink / raw)
  To: Eric Wong; +Cc: unicorn-public

The problem is actually worser, and the worker.nr == 0 can't be used.
I had to do something like this:

master_run = true

before_fork do |server, worker|
  if master_run
     #warm up server...

     #kill old pid...

     #disconnect active record

     master_run = false
  end

  # other stuff for each worker
end

In the last example, using worker.nr == 0 would make the server crash
in case the worker 0 was killed/restarted.

Also the AR's disconnect and *many other stuff* people put on
before_fork should only be run once the master was preloaded, not for
every worker.

So I still think at least a hook like master_preloaded(server) is necessary.

cheers,
bráulio

On Fri, Oct 3, 2014 at 9:22 AM, Eric Wong <e@80x24.org> wrote:
> Bráulio Bhavamitra <braulio@eita.org.br> wrote:
>> Hello all,
>>
>> If I need to hook something after master load, I'm currently doing:
>>
>> before_fork do |server, worker|
>>   # worker 0 is the first to init, so hold the master here
>>   if worker.nr == 0
>>      #warm up server...
>>
>>      #kill old pid...
>>   end
>>
>>   # other stuff for each worker
>> end
>>
>> Both operations I currently do (server warm up and old pid kill) need
>> to be run only once, and not for every worker as before_fork does,
>> that's why I had to put the condition seen above.
>
> The above is fine if your first worker never dies.  I think you can add
> a local variable to ensure it only runs the first time worker.nr == 0 is
> started, in case a worker dies.  Something like:
>
>     first = true
>     before_fork do |server, worker|
>       # worker 0 is the first to init, so hold the master here
>       if worker.nr == 0 && first
>          first = false
>          #warm up server...
>
>          #kill old pid...
>       end
>
>       # other stuff for each worker
>     end
>
> For what it's worth, I'm not a fan of auto-killing the old PID in the
> unicorn config and regret having it in the example config.  It's only
> for the most memory-constrained configs and fragile (because anything
> with pid files is always fragile).
>
>> So hooks for master is needed, something like
>> master_after_load(server) and master_init(server).
>>
>> What do you think?
>
> rack.git also has a Rack::Builder#warmup method.  Aman originally
> proposed it for unicorn, but it's useful outside of unicorn so
> we moved it to Rack.
>
> In general, I'm against adding new hooks/options because they tend to
> make maintainability and documentation harder for ops folks.
> I still have nightmares of some Capistrano config filled with hooks
> from years ago :x
>
> Features like these also makes migrating away from unicorn harder, so
> that is another reason we ended up adding #warmup to Rack and not
> unicorn.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Master hooks needed
  2014-10-04  0:53   ` Bráulio Bhavamitra
@ 2014-10-04  1:22     ` Eric Wong
  2014-10-04  1:35       ` Bráulio Bhavamitra
  0 siblings, 1 reply; 11+ messages in thread
From: Eric Wong @ 2014-10-04  1:22 UTC (permalink / raw)
  To: Bráulio Bhavamitra; +Cc: unicorn-public

Bráulio Bhavamitra <braulio@eita.org.br> wrote:
> The problem is actually worser, and the worker.nr == 0 can't be used.
> I had to do something like this:
> 
> master_run = true
> 
> before_fork do |server, worker|
>   if master_run
>      #warm up server...
> 
>      #kill old pid...
> 
>      #disconnect active record
> 
>      master_run = false
>   end
> 
>   # other stuff for each worker
> end
> 
> In the last example, using worker.nr == 0 would make the server crash
> in case the worker 0 was killed/restarted.
> 
> Also the AR's disconnect and *many other stuff* people put on
> before_fork should only be run once the master was preloaded, not for
> every worker.

AR disconnect! is idempotent and has been since unicorn existed.
I suspect most other things people run in before_fork are already
idempotent, too.

> So I still think at least a hook like master_preloaded(server) is necessary.

Using the local 'master_run' variable in your before_fork already
accomplishes everything you needed, right?

I try to keep unicorn as lean as possible and not bloat it with
redundant features.  The current hooks already fulfill the minimal
requirements for supporting apps which do not behave properly under
preload_app=true, so I am not convinced why a new hook is necessary.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Master hooks needed
  2014-10-04  1:22     ` Eric Wong
@ 2014-10-04  1:35       ` Bráulio Bhavamitra
  2014-10-04  1:57         ` Eric Wong
  0 siblings, 1 reply; 11+ messages in thread
From: Bráulio Bhavamitra @ 2014-10-04  1:35 UTC (permalink / raw)
  To: Eric Wong; +Cc: unicorn-public

I think the hook is needed because I took too much time to figure out
the problem and much more time to figure out the solution (this
master_run variable). Also, I don't think this master_run solution is
elegant.

cheers,
bráulio

On Fri, Oct 3, 2014 at 10:22 PM, Eric Wong <e@80x24.org> wrote:
> Bráulio Bhavamitra <braulio@eita.org.br> wrote:
>> The problem is actually worser, and the worker.nr == 0 can't be used.
>> I had to do something like this:
>>
>> master_run = true
>>
>> before_fork do |server, worker|
>>   if master_run
>>      #warm up server...
>>
>>      #kill old pid...
>>
>>      #disconnect active record
>>
>>      master_run = false
>>   end
>>
>>   # other stuff for each worker
>> end
>>
>> In the last example, using worker.nr == 0 would make the server crash
>> in case the worker 0 was killed/restarted.
>>
>> Also the AR's disconnect and *many other stuff* people put on
>> before_fork should only be run once the master was preloaded, not for
>> every worker.
>
> AR disconnect! is idempotent and has been since unicorn existed.
> I suspect most other things people run in before_fork are already
> idempotent, too.
>
>> So I still think at least a hook like master_preloaded(server) is necessary.
>
> Using the local 'master_run' variable in your before_fork already
> accomplishes everything you needed, right?
>
> I try to keep unicorn as lean as possible and not bloat it with
> redundant features.  The current hooks already fulfill the minimal
> requirements for supporting apps which do not behave properly under
> preload_app=true, so I am not convinced why a new hook is necessary.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Master hooks needed
  2014-10-04  1:35       ` Bráulio Bhavamitra
@ 2014-10-04  1:57         ` Eric Wong
  2014-10-04  2:04           ` Bráulio Bhavamitra
  0 siblings, 1 reply; 11+ messages in thread
From: Eric Wong @ 2014-10-04  1:57 UTC (permalink / raw)
  To: Bráulio Bhavamitra; +Cc: unicorn-public

Bráulio Bhavamitra <braulio@eita.org.br> wrote:
> I think the hook is needed because I took too much time to figure out
> the problem and much more time to figure out the solution (this
> master_run variable). Also, I don't think this master_run solution is
> elegant.

A guard variable is fairly common practice for initialization.
It's not always nice, but I do not consider the existing hooks
to be elegant, either; they're only unfortunately necessary.

I consider having redundant features to be even worse.

How about the following documentation change instead?

--- a/examples/unicorn.conf.rb
+++ b/examples/unicorn.conf.rb
@@ -54,12 +54,23 @@ GC.respond_to?(:copy_on_write_friendly=) and
 # fast LAN.
 check_client_connection false
 
+# local variable to guard against running a hook multiple times
+run_once = true
+
 before_fork do |server, worker|
   # the following is highly recomended for Rails + "preload_app true"
   # as there's no need for the master process to hold a connection
   defined?(ActiveRecord::Base) and
     ActiveRecord::Base.connection.disconnect!
 
+  # Occasionally, it may be necessary to run non-idempotent code in the
+  # master before forking.  Keep in mind the above disconnect! example
+  # is idempotent and does not need a guard.
+  if run_once
+    # do_something_once_here ...
+    run_once = false # prevent from firing again
+  end
+
   # The following is only recommended for memory/DB-constrained
   # installations.  It is not needed if your system can house
   # twice as many worker_processes as you have configured.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Master hooks needed
  2014-10-04  1:57         ` Eric Wong
@ 2014-10-04  2:04           ` Bráulio Bhavamitra
  2014-10-04  2:25             ` Eric Wong
  0 siblings, 1 reply; 11+ messages in thread
From: Bráulio Bhavamitra @ 2014-10-04  2:04 UTC (permalink / raw)
  To: Eric Wong; +Cc: unicorn-public

Documentation is an excelent solution :)

On Fri, Oct 3, 2014 at 10:57 PM, Eric Wong <e@80x24.org> wrote:
> Bráulio Bhavamitra <braulio@eita.org.br> wrote:
>> I think the hook is needed because I took too much time to figure out
>> the problem and much more time to figure out the solution (this
>> master_run variable). Also, I don't think this master_run solution is
>> elegant.
>
> A guard variable is fairly common practice for initialization.
> It's not always nice, but I do not consider the existing hooks
> to be elegant, either; they're only unfortunately necessary.
>
> I consider having redundant features to be even worse.
>
> How about the following documentation change instead?
>
> --- a/examples/unicorn.conf.rb
> +++ b/examples/unicorn.conf.rb
> @@ -54,12 +54,23 @@ GC.respond_to?(:copy_on_write_friendly=) and
>  # fast LAN.
>  check_client_connection false
>
> +# local variable to guard against running a hook multiple times
> +run_once = true
> +
>  before_fork do |server, worker|
>    # the following is highly recomended for Rails + "preload_app true"
>    # as there's no need for the master process to hold a connection
>    defined?(ActiveRecord::Base) and
>      ActiveRecord::Base.connection.disconnect!
>
> +  # Occasionally, it may be necessary to run non-idempotent code in the
> +  # master before forking.  Keep in mind the above disconnect! example
> +  # is idempotent and does not need a guard.
> +  if run_once
> +    # do_something_once_here ...
> +    run_once = false # prevent from firing again
> +  end
> +
>    # The following is only recommended for memory/DB-constrained
>    # installations.  It is not needed if your system can house
>    # twice as many worker_processes as you have configured.



-- 
"Lute pela sua ideologia. Seja um com sua ideologia. Viva pela sua
ideologia. Morra por sua ideologia" P.R. Sarkar

EITA - Educação, Informação e Tecnologias para Autogestão
http://cirandas.net/brauliobo
http://eita.org.br

"Paramapurusha é meu pai e Parama Prakriti é minha mãe. O universo é
meu lar e todos nós somos cidadãos deste cosmo. Este universo é a
imaginação da Mente Macrocósmica, e todas as entidades estão sendo
criadas, preservadas e destruídas nas fases de extroversão e
introversão do fluxo imaginativo cósmico. No âmbito pessoal, quando
uma pessoa imagina algo em sua mente, naquele momento, essa pessoa é a
única proprietária daquilo que ela imagina, e ninguém mais. Quando um
ser humano criado mentalmente caminha por um milharal também
imaginado, a pessoa imaginada não é a propriedade desse milharal, pois
ele pertence ao indivíduo que o está imaginando. Este universo foi
criado na imaginação de Brahma, a Entidade Suprema, por isso a
propriedade deste universo é de Brahma, e não dos microcosmos que
também foram criados pela imaginação de Brahma. Nenhuma propriedade
deste mundo, mutável ou imutável, pertence a um indivíduo em
particular; tudo é o patrimônio comum de todos."
Restante do texto em
http://cirandas.net/brauliobo/blog/a-problematica-de-hoje-em-dia

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Master hooks needed
  2014-10-04  2:04           ` Bráulio Bhavamitra
@ 2014-10-04  2:25             ` Eric Wong
  0 siblings, 0 replies; 11+ messages in thread
From: Eric Wong @ 2014-10-04  2:25 UTC (permalink / raw)
  To: Bráulio Bhavamitra; +Cc: unicorn-public

Bráulio Bhavamitra <braulio@eita.org.br> wrote:
> Documentation is an excelent solution :)

OK, pushed to the website[1] and unicorn.git as
commit 3318b070c6513a3baa7ce7ac26f4835f46ccff1f

    examples: add run_once to before_fork hook example

    There may be code in a before_fork hook which should run only once,
    document an example using a guard variable since it may not be
    immediately obvious to all users.

    Inspired-by: Bráulio Bhavamitra <braulio@eita.org.br>

http://bogomips.org/unicorn-public/m/20141004015707.GA1951@dcvr.yhbt.net.html

[1] http://unicorn.bogomips.org/examples/

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2014-10-04  2:25 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-10-03 11:34 Master hooks needed Bráulio Bhavamitra
2014-10-03 12:22 ` Eric Wong
2014-10-03 12:36   ` Valentin Mihov
2014-10-03 17:50     ` Eric Wong
2014-10-03 18:12       ` Valentin Mihov
2014-10-04  0:53   ` Bráulio Bhavamitra
2014-10-04  1:22     ` Eric Wong
2014-10-04  1:35       ` Bráulio Bhavamitra
2014-10-04  1:57         ` Eric Wong
2014-10-04  2:04           ` Bráulio Bhavamitra
2014-10-04  2:25             ` Eric Wong

Code repositories for project(s) associated with this inbox:

	../../../unicorn.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).