unicorn Ruby/Rack server user+dev discussion/patches/pulls/bugs/help
 help / color / Atom feed
* Re: [PATCH] `unicorn upgrade` script resilience against exceptions
       [not found] <CAL-rKu6CZ731c=uHyZ8+Fg2fhC30e-3-J26XODacUK=YrfX+5Q@mail.gmail.com>
@ 2016-06-07 13:41 ` Eric Wong
  2016-06-07 15:17   ` Jesper Rønn-Jensen
  0 siblings, 1 reply; 4+ messages in thread
From: Eric Wong @ 2016-06-07 13:41 UTC (permalink / raw)
  To: Jesper Rønn-Jensen; +Cc: unicorn-public

Jesper Rønn-Jensen <jesperrr@gmail.com> wrote:
> This is actually a change proposal "by accident". But first, some
> background:

	Please don't send HTML mail.  I guess the bounce message didn't
	include the reason for the bounce, but should be fixed; now;
	and the bounce will tell you to not send HTML.

	Anyways if you stick to old-school Usenet/mailing list posting
	conventions, I'm more than happy to help you :)

> My server uses a standard version of this `init.sh` script. I found that
> executing `unicorn upgrade` often gives me problems.

Hm... I haven't looked at that script in a while given all
the new-fangled init replacements...

> For example, it breaks my Capistrano deploy script to use `unicorn:upgrade`
> to deploy. This is a shame, since it's the correct
> command for Unicorn to pick up any code changes. But unicorn upgrade in the
> script fails with messages like:
> 
> ```
> sudo /etc/init.d/unicorn upgrade
> Couldn't upgrade, starting 'export HOME=/home/deploy ; cd
> /var/www/my_app/current && /home/deploy/.rvm/wrappers/my_app/bundle exec
> unicorn -D -c /var/www/my_app/shared/config/unicorn.rb -E production'
> instead
> master failed to start, check stderr log for details
> ```
> 
> And the stderror log:
> ```
> E, [2016-06-07T09:03:27.517267 #27583] ERROR -- : reaped #<Process::Status:
> pid 27621 exit 1> exec()-ed
> /var/www/my_app/shared/vendor/bundle/ruby/2.3.0/gems/unicorn-5.1.0/lib/unicorn/http_server.rb:195:in
>     `pid=': Already running on PID:27583 (or
> pid=/var/www/my_app/shared/tmp/pids/unicorn.pid is stale)
>     (ArgumentError)
> ```
> 
> I am proposing this change because I misread the documentation for the
> SIGNALS!
> I thought that Unicorn itself sends a QUIT after all subprocesses has ended
> via SIG USR2.
> 
> The change is to remove the QUIT signal to the PID.oldbin process, and that
> apparently stops all the failures I ran into. So I made this change on a
> server, and it works!
> 
> I just don't understand why, since reading the documentation again it says:
> https://github.com/defunkt/unicorn/blob/d23d4713dc9ab9732c574f5aa34a4b6740b43164/SIGNALS#L35
> 
> > * USR2 - reexecute the running binary.  A separate QUIT
> >  should be sent to the original process once the child is verified to
> >  be up and running.
> 
> So clearly, I should send the QUIT signal.

Yes.

> My goal is to have a resilient, robust deployment, where unicorn picks up
> any code changes.

Unfortunately, PID files have always been racy with USR2.
Nowadays systemd is fairly standardized and seems to work
pretty well for managing sockets and services.

I'm still not a fan of some systemd things, but I think the socket
activation part is very nice.  Example @.service and .socket files
are distributed nowadays:

	https://unicorn.bogomips.org/examples/unicorn@.service
	https://unicorn.bogomips.org/examples/unicorn.socket

> Can you help me and enlighten me as to why this proposed change works?

Anyways, things are racy in that script and increasing the
"sleep 2" to a higher number will help if your system is really
overloaded.

> ---
>  examples/init.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/examples/init.sh b/examples/init.sh
> index 1f0e035..cbadc11 100644
> --- a/examples/init.sh
> +++ b/examples/init.sh
> @@ -45,7 +45,7 @@ restart|reload)
>   $CMD
>   ;;
>  upgrade)
> - if sig USR2 && sleep 2 && sig 0 && oldsig QUIT
> + if sig USR2 && sleep 2 && sig 0
>   then
>   n=$TIMEOUT
>   while test -s $old_pid && test $n -ge 0
> -- 

I actually wrote a better init script for a similar server,
patch coming in a bit (or later, close to falling asleep).

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

* Re: [PATCH] `unicorn upgrade` script resilience against exceptions
  2016-06-07 13:41 ` [PATCH] `unicorn upgrade` script resilience against exceptions Eric Wong
@ 2016-06-07 15:17   ` Jesper Rønn-Jensen
  2016-06-07 21:13     ` [PATCH] examples/init.sh: update to reduce upgrade raciness Eric Wong
  0 siblings, 1 reply; 4+ messages in thread
From: Jesper Rønn-Jensen @ 2016-06-07 15:17 UTC (permalink / raw)
  To: Eric Wong; +Cc: unicorn-public

Thanks for input.

I will close this issue and wait for your patch later, then.

Will also close pull-request I put at github with reference to this thread.

PS. Thanks for tip on HTML vs Plain text mails. Didn't know Gmail
could send plain text :)



On Tue, Jun 7, 2016 at 3:41 PM, Eric Wong <e@80x24.org> wrote:
> Jesper Rønn-Jensen <jesperrr@gmail.com> wrote:
>> This is actually a change proposal "by accident". But first, some
>> background:
>
>         Please don't send HTML mail.  I guess the bounce message didn't
>         include the reason for the bounce, but should be fixed; now;
>         and the bounce will tell you to not send HTML.
>
>         Anyways if you stick to old-school Usenet/mailing list posting
>         conventions, I'm more than happy to help you :)
>
>> My server uses a standard version of this `init.sh` script. I found that
>> executing `unicorn upgrade` often gives me problems.
>
> Hm... I haven't looked at that script in a while given all
> the new-fangled init replacements...
>
>> For example, it breaks my Capistrano deploy script to use `unicorn:upgrade`
>> to deploy. This is a shame, since it's the correct
>> command for Unicorn to pick up any code changes. But unicorn upgrade in the
>> script fails with messages like:
>>
>> ```
>> sudo /etc/init.d/unicorn upgrade
>> Couldn't upgrade, starting 'export HOME=/home/deploy ; cd
>> /var/www/my_app/current && /home/deploy/.rvm/wrappers/my_app/bundle exec
>> unicorn -D -c /var/www/my_app/shared/config/unicorn.rb -E production'
>> instead
>> master failed to start, check stderr log for details
>> ```
>>
>> And the stderror log:
>> ```
>> E, [2016-06-07T09:03:27.517267 #27583] ERROR -- : reaped #<Process::Status:
>> pid 27621 exit 1> exec()-ed
>> /var/www/my_app/shared/vendor/bundle/ruby/2.3.0/gems/unicorn-5.1.0/lib/unicorn/http_server.rb:195:in
>>     `pid=': Already running on PID:27583 (or
>> pid=/var/www/my_app/shared/tmp/pids/unicorn.pid is stale)
>>     (ArgumentError)
>> ```
>>
>> I am proposing this change because I misread the documentation for the
>> SIGNALS!
>> I thought that Unicorn itself sends a QUIT after all subprocesses has ended
>> via SIG USR2.
>>
>> The change is to remove the QUIT signal to the PID.oldbin process, and that
>> apparently stops all the failures I ran into. So I made this change on a
>> server, and it works!
>>
>> I just don't understand why, since reading the documentation again it says:
>> https://github.com/defunkt/unicorn/blob/d23d4713dc9ab9732c574f5aa34a4b6740b43164/SIGNALS#L35
>>
>> > * USR2 - reexecute the running binary.  A separate QUIT
>> >  should be sent to the original process once the child is verified to
>> >  be up and running.
>>
>> So clearly, I should send the QUIT signal.
>
> Yes.
>
>> My goal is to have a resilient, robust deployment, where unicorn picks up
>> any code changes.
>
> Unfortunately, PID files have always been racy with USR2.
> Nowadays systemd is fairly standardized and seems to work
> pretty well for managing sockets and services.
>
> I'm still not a fan of some systemd things, but I think the socket
> activation part is very nice.  Example @.service and .socket files
> are distributed nowadays:
>
>         https://unicorn.bogomips.org/examples/unicorn@.service
>         https://unicorn.bogomips.org/examples/unicorn.socket
>
>> Can you help me and enlighten me as to why this proposed change works?
>
> Anyways, things are racy in that script and increasing the
> "sleep 2" to a higher number will help if your system is really
> overloaded.
>
>> ---
>>  examples/init.sh | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/examples/init.sh b/examples/init.sh
>> index 1f0e035..cbadc11 100644
>> --- a/examples/init.sh
>> +++ b/examples/init.sh
>> @@ -45,7 +45,7 @@ restart|reload)
>>   $CMD
>>   ;;
>>  upgrade)
>> - if sig USR2 && sleep 2 && sig 0 && oldsig QUIT
>> + if sig USR2 && sleep 2 && sig 0
>>   then
>>   n=$TIMEOUT
>>   while test -s $old_pid && test $n -ge 0
>> --
>
> I actually wrote a better init script for a similar server,
> patch coming in a bit (or later, close to falling asleep).



-- 

Jesper Rønn-Jensen
Nine A/S
Mobile: +45 2989 1822
Blog http://justaddwater.dk/
jesperrr@gmail.com (Private e-mail and Google Talk IM)

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

* [PATCH] examples/init.sh: update to reduce upgrade raciness
  2016-06-07 15:17   ` Jesper Rønn-Jensen
@ 2016-06-07 21:13     ` Eric Wong
  2016-06-15 20:21       ` Eric Wong
  0 siblings, 1 reply; 4+ messages in thread
From: Eric Wong @ 2016-06-07 21:13 UTC (permalink / raw)
  To: Jesper Rønn-Jensen; +Cc: unicorn-public

Jesper Rønn-Jensen <jesperrr@gmail.com> wrote:
> On Tue, Jun 7, 2016 at 3:41 PM, Eric Wong <e@80x24.org> wrote:
> > Jesper Rønn-Jensen <jesperrr@gmail.com> wrote:
> >> +++ b/examples/init.sh
> >> @@ -45,7 +45,7 @@ restart|reload)
> >>   $CMD
> >>   ;;
> >>  upgrade)
> >> - if sig USR2 && sleep 2 && sig 0 && oldsig QUIT
> >> + if sig USR2 && sleep 2 && sig 0
> >>   then
> >>   n=$TIMEOUT
> >>   while test -s $old_pid && test $n -ge 0
> >> --
> >
> > I actually wrote a better init script for a similar server,
> > patch coming in a bit (or later, close to falling asleep).
>
> Thanks for input.
> 
> I will close this issue and wait for your patch later, then.
 
Proposed patch below (for "git am --scissors" users)
I've pushed this out to the "jr/init" branch on the git repo
and you can download the blob directly, here:

https://bogomips.org/unicorn.git/plain/examples/init.sh?h=jr/init

Will rebase/amend this branch as necessary until merged into master.

> Will also close pull-request I put at github with reference to this thread.

Please do, thank you.

Their terms-of-service doesn't allow bots to auto-close and redirect
things; and I can't support any centralized messaging system.

> >         Anyways if you stick to old-school Usenet/mailing list posting
> >         conventions, I'm more than happy to help you :)
 
> PS. Thanks for tip on HTML vs Plain text mails. Didn't know Gmail
> could send plain text :)

I think the only problem with gmail I've heard was from mobile
phone app users.  At least the git ML and LKML gets text-only
gmail posts all the time.

We old-school conventions mean we also trim our quotes and
avoid top-posting :)

------------8<----------------
Subject: [PATCH] examples/init.sh: update to reduce upgrade raciness
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Rework the "upgrade" target to only read the PID files once to
avoid misreading the wrong PID files in the middle of the
upgrade.

Additionally, introduce the UPGRADE_DELAY environment parameter
so users can increase/decrease according to their application
startup time.

PID files are inherently racy and people should be using a
process manager (systemd or similar) instead, but this should
mitigate most of the problems with the old target.

While we're at it, add LSB tags for systems which complain
about the lack of them and modernize things a bit using
$(command) construct instead of the more fragile `command`.

Thanks-to: Jesper Rønn-Jensen <jesperrr@gmail.com>
---
 examples/init.sh | 44 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 36 insertions(+), 8 deletions(-)

diff --git a/examples/init.sh b/examples/init.sh
index 1f0e035..4ef6cdc 100644
--- a/examples/init.sh
+++ b/examples/init.sh
@@ -1,7 +1,16 @@
 #!/bin/sh
 set -e
+### BEGIN INIT INFO
+# Provides:          unicorn
+# Required-Start:    $local_fs $network
+# Required-Stop:     $local_fs $network
+# Default-Start:     2 3 4 5
+# Default-Stop:      0 1 6
+# Short-Description: Start/stop unicorn Rack app server
+### END INIT INFO
+
 # Example init script, this can be used with nginx, too,
-# since nginx and unicorn accept the same signals
+# since nginx and unicorn accept the same signals.
 
 # Feel free to change any of the following variables for your app:
 TIMEOUT=${TIMEOUT-60}
@@ -9,21 +18,22 @@ APP_ROOT=/home/x/my_app/current
 PID=$APP_ROOT/tmp/pids/unicorn.pid
 CMD="/usr/bin/unicorn -D -c $APP_ROOT/config/unicorn.rb"
 INIT_CONF=$APP_ROOT/config/init.conf
+UPGRADE_DELAY=${UPGRADE_DELAY-2}
 action="$1"
 set -u
 
 test -f "$INIT_CONF" && . $INIT_CONF
 
-old_pid="$PID.oldbin"
+OLD="$PID.oldbin"
 
 cd $APP_ROOT || exit 1
 
 sig () {
-	test -s "$PID" && kill -$1 `cat $PID`
+	test -s "$PID" && kill -$1 $(cat $PID)
 }
 
 oldsig () {
-	test -s $old_pid && kill -$1 `cat $old_pid`
+	test -s "$OLD" && kill -$1 $(cat $OLD)
 }
 
 case $action in
@@ -45,18 +55,36 @@ restart|reload)
 	$CMD
 	;;
 upgrade)
-	if sig USR2 && sleep 2 && sig 0 && oldsig QUIT
+	if oldsig 0
+	then
+		echo >&2 "Old upgraded process still running with $OLD"
+		exit 1
+	fi
+
+	cur_pid=
+	if test -s "$PID"
+	then
+		cur_pid=$(cat $PID)
+	fi
+
+	if test -n "$cur_pid" &&
+			kill -USR2 "$cur_pid" &&
+			sleep $UPGRADE_DELAY &&
+			new_pid=$(cat $PID) &&
+			test x"$new_pid" != x"$cur_pid" &&
+			kill -0 "$new_pid" &&
+			kill -QUIT "$cur_pid"
 	then
 		n=$TIMEOUT
-		while test -s $old_pid && test $n -ge 0
+		while kill -0 "$cur_pid" 2>/dev/null && test $n -ge 0
 		do
 			printf '.' && sleep 1 && n=$(( $n - 1 ))
 		done
 		echo
 
-		if test $n -lt 0 && test -s $old_pid
+		if test $n -lt 0 && kill -0 "$cur_pid" 2>/dev/null
 		then
-			echo >&2 "$old_pid still exists after $TIMEOUT seconds"
+			echo >&2 "$cur_pid still running after $TIMEOUT seconds"
 			exit 1
 		fi
 		exit 0
-- 
EW

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

* Re: [PATCH] examples/init.sh: update to reduce upgrade raciness
  2016-06-07 21:13     ` [PATCH] examples/init.sh: update to reduce upgrade raciness Eric Wong
@ 2016-06-15 20:21       ` Eric Wong
  0 siblings, 0 replies; 4+ messages in thread
From: Eric Wong @ 2016-06-15 20:21 UTC (permalink / raw)
  To: Jesper Rønn-Jensen; +Cc: unicorn-public

Eric Wong <e@80x24.org> wrote:
> https://bogomips.org/unicorn.git/plain/examples/init.sh?h=jr/init

Hi Jesper, did you get a chance to test this updated init script?
Any feedback would be appreciated, thanks.

ref: https://bogomips.org/unicorn-public/20160607211354.GA21174@dcvr.yhbt.net/

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

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CAL-rKu6CZ731c=uHyZ8+Fg2fhC30e-3-J26XODacUK=YrfX+5Q@mail.gmail.com>
2016-06-07 13:41 ` [PATCH] `unicorn upgrade` script resilience against exceptions Eric Wong
2016-06-07 15:17   ` Jesper Rønn-Jensen
2016-06-07 21:13     ` [PATCH] examples/init.sh: update to reduce upgrade raciness Eric Wong
2016-06-15 20:21       ` Eric Wong

unicorn Ruby/Rack server user+dev discussion/patches/pulls/bugs/help

Archives are clonable:
	git clone --mirror https://bogomips.org/unicorn-public
	git clone --mirror http://ou63pmih66umazou.onion/unicorn-public

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.lang.ruby.unicorn
	nntp://ou63pmih66umazou.onion/inbox.comp.lang.ruby.unicorn

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox