* 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 public inbox https://yhbt.net/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).