Master hooks needed

Master hooks needed

[Bráulio Bhavamitra]

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

Re: Master hooks needed

[Eric Wong]

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.

Re: Master hooks needed

[Valentin Mihov]

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

Re: Master hooks needed

[Eric Wong]

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

Re: Master hooks needed

[Valentin Mihov]

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.

Re: Master hooks needed

[Bráulio Bhavamitra]

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.

Re: Master hooks needed

[Eric Wong]

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.

Re: Master hooks needed

[Bráulio Bhavamitra]

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.

Re: Master hooks needed

[Eric Wong]

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.

Re: Master hooks needed

[Bráulio Bhavamitra]

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

Re: Master hooks needed

[Eric Wong]

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/