What follows are all the write actions related to unicorn pid file when doing a hot restart. Seems like a bug to me that unicorn is deleting the pid file before writing the new file. Is there a reason for it? It seems to go against that rename that aims for an atomic replace that would always ensure the pid file is there. Regards, Petteri 2012-11-26 00:54:12 +0200 [:delete] "unicorn_blue1.pid" 2012-11-26 00:54:12 +0200 [:create] "0.18100578716567672.15108" 2012-11-26 00:54:12 +0200 [:open] "0.18100578716567672.15108" 2012-11-26 00:54:12 +0200 [:modify] "0.18100578716567672.15108" 2012-11-26 00:54:12 +0200 [:moved_from, :move] "0.18100578716567672.15108" 2012-11-26 00:54:12 +0200 [:moved_to, :move] "unicorn_blue1.pid.oldbin" 2012-11-26 00:54:12 +0200 [:close_write, :close] "unicorn_blue1.pid.oldbin" 2012-11-26 00:54:14 +0200 [:create] "0.9762316483892712.16822" 2012-11-26 00:54:14 +0200 [:open] "0.9762316483892712.16822" 2012-11-26 00:54:14 +0200 [:modify] "0.9762316483892712.16822" 2012-11-26 00:54:14 +0200 [:moved_from, :move] "0.9762316483892712.16822" 2012-11-26 00:54:14 +0200 [:moved_to, :move] "unicorn_blue1.pid" 2012-11-26 00:54:14 +0200 [:close_write, :close] "unicorn_blue1.pid" 2012-11-26 00:54:14 +0200
Petteri R?ty <betelgeuse at gentoo.org> wrote:> What follows are all the write actions related to unicorn pid file when > doing a hot restart. Seems like a bug to me that unicorn is deleting the > pid file before writing the new file. Is there a reason for it? It seems > to go against that rename that aims for an atomic replace that would > always ensure the pid file is there.Unfortunately, pid files are inherently racy. However, I seem to recall a pid file not existing for a brief moment was needed to allow some nginx-based scripts to work. I think unicorn differs a bit from nginx here: nginx uses rename() to clear the way for a new pid file. Like unicorn, this still leaves a window where no pid file exists. Also, nginx does not create a randomly named pid file before renaming it, so there''s a possibility another process can read an existing, but zero-byte file. unicorn avoids this, if a pid file exists, it has a pid inside it.
On 26.11.2012 2.43, Eric Wong wrote:> Petteri R?ty <betelgeuse at gentoo.org> wrote: >> What follows are all the write actions related to unicorn pid file when >> doing a hot restart. Seems like a bug to me that unicorn is deleting the >> pid file before writing the new file. Is there a reason for it? It seems >> to go against that rename that aims for an atomic replace that would >> always ensure the pid file is there. > > Unfortunately, pid files are inherently racy. However, I > seem to recall a pid file not existing for a brief moment was needed > to allow some nginx-based scripts to work. >Would you be open for a config switch to avoid this behavior? Can you say if lib/unicorn/http_server.rb:196 is the correct spot? The use case here is that with health monitors wouldn''t have a window where a pid file does not exist. With a hot restart it should always be possible to have a pid file that points to either the old or the new master.> I think unicorn differs a bit from nginx here: > > nginx uses rename() to clear the way for a new pid file. Like unicorn, > this still leaves a window where no pid file exists. >Looking at the inotify log it seems the reason pid file does not exist is an explicit delete and not due to rename. It happens a couple seconds earlier also so the window is possible to hit even with a periodic poller. Regards, Petteri
Petteri R?ty <betelgeuse at gentoo.org> wrote:> On 26.11.2012 2.43, Eric Wong wrote: > > Petteri R?ty <betelgeuse at gentoo.org> wrote: > >> What follows are all the write actions related to unicorn pid file when > >> doing a hot restart. Seems like a bug to me that unicorn is deleting the > >> pid file before writing the new file. Is there a reason for it? It seems > >> to go against that rename that aims for an atomic replace that would > >> always ensure the pid file is there. > > > > Unfortunately, pid files are inherently racy. However, I > > seem to recall a pid file not existing for a brief moment was needed > > to allow some nginx-based scripts to work. > > > > Would you be open for a config switch to avoid this behavior?No, every config option makes the project more difficult to support.> Can you say if lib/unicorn/http_server.rb:196 is the correct spot?Yes.> The use case here is that with health monitors wouldn''t have a window > where a pid file does not exist. With a hot restart it should always be > possible to have a pid file that points to either the old or the new master.Then, doesn''t nginx have the same problem?> > I think unicorn differs a bit from nginx here: > > > > nginx uses rename() to clear the way for a new pid file. Like unicorn, > > this still leaves a window where no pid file exists. > > > > Looking at the inotify log it seems the reason pid file does not exist > is an explicit delete and not due to rename. It happens a couple seconds > earlier also so the window is possible to hit even with a periodic poller.Is matching nginx rename behavior enough to solve the problem? Matching nginx behavior can become the default if it solves your problem.
On 26.11.2012 20.24, Eric Wong wrote:> >> The use case here is that with health monitors wouldn''t have a window >> where a pid file does not exist. With a hot restart it should always be >> possible to have a pid file that points to either the old or the new master. > > Then, doesn''t nginx have the same problem? >nginx doens''t have the same problem as I show later. Even if it I restart/reload nginx very infrequently. Unicorn has to be hot restarted every time we change our application code and happens frequently.>>> I think unicorn differs a bit from nginx here: >>> >>> nginx uses rename() to clear the way for a new pid file. Like unicorn, >>> this still leaves a window where no pid file exists. >>> >> >> Looking at the inotify log it seems the reason pid file does not exist >> is an explicit delete and not due to rename. It happens a couple seconds >> earlier also so the window is possible to hit even with a periodic poller. > > Is matching nginx rename behavior enough to solve the problem? > > Matching nginx behavior can become the default if it solves your problem. >nginx does not explicitly unlink the old pid file before it renames it out of the way so yes matching nginx in that regard changes the behavior exactly how I originally asked but you were against that. Maybe the point is moot though. This is from a combo of USR2, WINCH, QUIT sent from htop to the master process: 2012-11-27 01:44:25 +0200 [:moved_from, :move] "nginx.pid" 2012-11-27 01:44:25 +0200 [:moved_to, :move] "nginx.pid.oldbin" 2012-11-27 01:44:25 +0200 [:create] "nginx.pid" 2012-11-27 01:44:25 +0200 [:open] "nginx.pid" 2012-11-27 01:44:25 +0200 [:modify] "nginx.pid" 2012-11-27 01:44:25 +0200 [:close_write, :close] "nginx.pid" 2012-11-27 01:45:31 +0200 [:delete] "nginx.pid.oldbin" The window here is much smaller than for the current unicorn behavior. Regards, Petteri
Petteri R?ty <betelgeuse at gentoo.org> wrote:> On 26.11.2012 20.24, Eric Wong wrote: > > > >> The use case here is that with health monitors wouldn''t have a window > >> where a pid file does not exist. With a hot restart it should always be > >> possible to have a pid file that points to either the old or the new master. > > > > Then, doesn''t nginx have the same problem? > > > > nginx doens''t have the same problem as I show later. Even if it I > restart/reload nginx very infrequently. Unicorn has to be hot restarted > every time we change our application code and happens frequently. > > >>> I think unicorn differs a bit from nginx here: > >>> > >>> nginx uses rename() to clear the way for a new pid file. Like unicorn, > >>> this still leaves a window where no pid file exists. > >>> > >> > >> Looking at the inotify log it seems the reason pid file does not exist > >> is an explicit delete and not due to rename. It happens a couple seconds > >> earlier also so the window is possible to hit even with a periodic poller. > > > > Is matching nginx rename behavior enough to solve the problem? > > > > Matching nginx behavior can become the default if it solves your problem. > > > > nginx does not explicitly unlink the old pid file before it renames it > out of the way so yes matching nginx in that regard changes the behavior > exactly how I originally asked but you were against that. Maybe the > point is moot though.Ah, I thought you wanted the pid file to be replaced without a window where the file is non-existent (or empty).> This is from a combo of USR2, WINCH, QUIT sent from htop to the master > process: > > 2012-11-27 01:44:25 +0200 > [:moved_from, :move] > "nginx.pid" > 2012-11-27 01:44:25 +0200 > [:moved_to, :move] > "nginx.pid.oldbin" > 2012-11-27 01:44:25 +0200OK, this is the window where file does not exist at all. unicorn has the same problem here, but obviously unicorn is slower than nginx.> [:create] > "nginx.pid" > 2012-11-27 01:44:25 +0200 > [:open] > "nginx.pid"This part of nginx makes me uncomfortable since nginx.pid is empty.> 2012-11-27 01:44:25 +0200 > [:modify] > "nginx.pid" > 2012-11-27 01:44:25 +0200 > [:close_write, :close] > "nginx.pid" > 2012-11-27 01:45:31 +0200 > [:delete] > "nginx.pid.oldbin" > > The window here is much smaller than for the current unicorn behavior.A small window is still a window. Can''t you make your health monitor check the state of the listening ports as well? There''s no point where a listening port will be unavailable.
On 27.11.2012 2.35, Eric Wong wrote:>> >> nginx does not explicitly unlink the old pid file before it renames it >> out of the way so yes matching nginx in that regard changes the behavior >> exactly how I originally asked but you were against that. Maybe the >> point is moot though. > > Ah, I thought you wanted the pid file to be replaced without > a window where the file is non-existent (or empty). >That would be ideal but I meant this thread to be explicitly about the unlink and resulting couple seconds window. Now that I have spend some thinking about the issue here''s an approach using hard links that can be used to replace the pid without window of non-existance: betelgeuse at mac ~/inotify/test_files $ echo "old" > a betelgeuse at mac ~/inotify/test_files $ ln a a.oldbin betelgeuse at mac ~/inotify/test_files $ for file in *; do echo $file: $(stat -f %i $file); done a: 3730129 a.oldbin: 3730129 betelgeuse at mac ~/inotify/test_files $ echo "new" > b betelgeuse at mac ~/inotify/test_files $ mv b a betelgeuse at mac ~/inotify/test_files $ for file in *; do echo $file: $(stat -f %i $file); done a: 3730137 a.oldbin: 3730129 betelgeuse at mac ~/inotify/test_files $ cat * new old Regards, Petteri
On 27.11.2012 2.35, Eric Wong wrote:> > A small window is still a window. > > Can''t you make your health monitor check the state of the listening > ports as well? There''s no point where a listening port will be > unavailable. >It''s running on a PaaS but seems like a decent thing to suggest for their stack. Regards, Petteri
Petteri R?ty <betelgeuse at gentoo.org> wrote:> On 27.11.2012 2.35, Eric Wong wrote: > > >> > >> nginx does not explicitly unlink the old pid file before it renames it > >> out of the way so yes matching nginx in that regard changes the behavior > >> exactly how I originally asked but you were against that. Maybe the > >> point is moot though. > > > > Ah, I thought you wanted the pid file to be replaced without > > a window where the file is non-existent (or empty). > > > > That would be ideal but I meant this thread to be explicitly about the > unlink and resulting couple seconds window. Now that I have spend some > thinking about the issue here''s an approach using hard links that can be > used to replace the pid without window of non-existance:Yes, this is ideal, but unfortunately, the window of non-existence is necessary for compatibility with some existing (nginx-based) scripts. PID files are horrible :<> betelgeuse at mac ~/inotify/test_files $ echo "old" > a > betelgeuse at mac ~/inotify/test_files $ ln a a.oldbin > betelgeuse at mac ~/inotify/test_files $ for file in *; do echo $file: > $(stat -f %i $file); done > a: 3730129 > a.oldbin: 3730129 > betelgeuse at mac ~/inotify/test_files $ echo "new" > b > betelgeuse at mac ~/inotify/test_files $ mv b a > betelgeuse at mac ~/inotify/test_files $ for file in *; do echo $file: > $(stat -f %i $file); done > a: 3730137 > a.oldbin: 3730129 > betelgeuse at mac ~/inotify/test_files $ cat * > new > old
On 27.11.2012 4.02, Eric Wong wrote:> Petteri R?ty <betelgeuse at gentoo.org> wrote: >> On 27.11.2012 2.35, Eric Wong wrote: >> >>>> >>>> nginx does not explicitly unlink the old pid file before it renames it >>>> out of the way so yes matching nginx in that regard changes the behavior >>>> exactly how I originally asked but you were against that. Maybe the >>>> point is moot though. >>> >>> Ah, I thought you wanted the pid file to be replaced without >>> a window where the file is non-existent (or empty). >>> >> >> That would be ideal but I meant this thread to be explicitly about the >> unlink and resulting couple seconds window. Now that I have spend some >> thinking about the issue here''s an approach using hard links that can be >> used to replace the pid without window of non-existance: > > Yes, this is ideal, but unfortunately, the window of non-existence is > necessary for compatibility with some existing (nginx-based) scripts. >Can you point out what those scripts are? Regards, Petteri
Petteri R?ty <betelgeuse at gentoo.org> wrote:> On 27.11.2012 4.02, Eric Wong wrote: > > Petteri R?ty <betelgeuse at gentoo.org> wrote: > >> On 27.11.2012 2.35, Eric Wong wrote: > >>>> > >>>> nginx does not explicitly unlink the old pid file before it renames it > >>>> out of the way so yes matching nginx in that regard changes the behavior > >>>> exactly how I originally asked but you were against that. Maybe the > >>>> point is moot though. > >>> > >>> Ah, I thought you wanted the pid file to be replaced without > >>> a window where the file is non-existent (or empty). > >> > >> That would be ideal but I meant this thread to be explicitly about the > >> unlink and resulting couple seconds window. Now that I have spend some > >> thinking about the issue here''s an approach using hard links that can be > >> used to replace the pid without window of non-existance: > > > > Yes, this is ideal, but unfortunately, the window of non-existence is > > necessary for compatibility with some existing (nginx-based) scripts. > > Can you point out what those scripts are?(Revisiting old archives ...) OK, I misremembered the situation... TL;DR: there''s too much historical baggage, as unicorn inherits traits from both mongrel and nginx. A change to fix one pid file monitoring solution will likely break something else. Of course PID files are broken _anyways_, so relying on them for monitoring is a terrible idea, *especially* when it''s something like nginx/unicorn where a master process gets replaced. 1) Before unicorn, there was Mongrel: it was important for somebody to create the PID file early in the process, before binding the listen socket. I seem to recall this being done to make some other health monitor work. 2) nginx prevents PID file clobbering by attempting to bind the listen socket before writing the pid file. This is the best way to prevent PID clobbering for a fresh process. Unfortunately, unicorn needed to inherit trait (1) from Mongrel to ease migration for Mongrel users. However, I also decided PID file clobber prevention in nginx was useful, so unicorn will not overwrite a PID file of a valid process. Unfortunately, the unicorn implementation of PID file clobber prevention is racy as it cannot copy nginx behavior here while preserving Mongrel compatibility. Thus, unicorn unlinks the PID file before writing a new one. unicorn could rename the PID file out of the way before writing a new one (as nginx does) and not suffer bad consequences, but the file still disappears momentarily. There could be a minor tweak to allow clobbering of an existing PID file iff it matches an expected value (of the original process), but I don''t think its worth the effort to implement at this point; as it will still be racy. Really, PID files are horrible, and MORE horrible than usual because we support process upgrade/backout via fork+exec().