Bug 12655

Summary: upgrade() is broken (wrong pidfile handling)
Product: Sisyphus Reporter: Michael Shigorin <mike>
Component: nginxAssignee: Michael Shigorin <mike>
Status: CLOSED FIXED QA Contact: qa-sisyphus
Severity: normal    
Priority: P2 CC: icesik, rider
Version: unstable   
Hardware: all   
OS: Linux   

Description Michael Shigorin 2007-08-30 18:12:30 MSD
Возможно, из-за отсутсвия /var/run/nginx/nginx.pid странно себя ведёт service
nginx condrestart: после двух запусков подряд при запущенном сервисе наблюдаем
два комплекта master+workers.  После трёх -- всё равно два.

Кстати!  Там в спеке /var/run/nginx/nginx.pid, а на файловой системе после
service nginx start наблюдается /var/run/nginx.pid.  Сейчас у себя буду ровнять
на /var/run/nginx.pid.

Слайд:

# killall nginx; service nginx restart; pidof nginx; cat /var/run/nginx.pid
Service nginx is not running. [PASSED]
Starting nginx service: [ DONE ]
13660 13659 13658
13658
# service nginx condrestart; ps auxw | grep nginx; cat /var/run/nginx.pid  
2007/08/30 17:08:45 [info] 13674#0: the configuration file /etc/nginx/nginx.conf
syntax is ok
2007/08/30 17:08:45 [info] 13674#0: the configuration file /etc/nginx/nginx.conf
was tested successfully
Upgrading nginx: [ DONE ]

root     11889  0.1  0.8  17984  4568 pts/16   S+   16:34   0:03 vim nginx
root     13658  0.0  0.2   5928  1144 ?        Ss   17:08   0:00 nginx: master
process /usr/sbin/nginx
_nginx   13659  0.0  0.8   9120  4616 ?        S    17:08   0:00 nginx: worker
process
_nginx   13660  0.1  0.8   9120  4640 ?        S    17:08   0:00 nginx: worker
process
root     13683  0.0  0.6   5932  3172 ?        S    17:08   0:00 nginx: master
process /usr/sbin/nginx
root     13691  0.0  0.1   1764   604 pts/17   S+   17:08   0:00 grep nginx
_nginx   13692  0.0  0.8   9124  4620 ?        S    17:08   0:00 nginx: worker
process
_nginx   13693  0.0  0.8   9124  4616 ?        S    17:08   0:00 nginx: worker
process
13683
# service nginx condrestart; ps auxw | grep nginx; cat /var/run/nginx.pid
2007/08/30 17:08:59 [info] 13699#0: the configuration file /etc/nginx/nginx.conf
syntax is ok
2007/08/30 17:08:59 [info] 13699#0: the configuration file /etc/nginx/nginx.conf
was tested successfully
Upgrading nginx: [ DONE ]

root     11889  0.1  0.8  17984  4568 pts/16   S+   16:34   0:03 vim nginx
root     13658  0.0  0.2   5928  1144 ?        Ss   17:08   0:00 nginx: master
process /usr/sbin/nginx
_nginx   13659  0.0  0.8   9120  4616 ?        S    17:08   0:00 nginx: worker
process
_nginx   13660  0.0  0.8   9120  4640 ?        S    17:08   0:00 nginx: worker
process
root     13683  0.4  0.6   5932  3176 ?        S    17:08   0:00 nginx: master
process /usr/sbin/nginx
_nginx   13692  0.0  0.8   9124  4620 ?        S    17:08   0:00 nginx: worker
process
_nginx   13693  0.0  0.9   9124  4644 ?        S    17:08   0:00 nginx: worker
process
root     13715  0.0  0.1   1760   588 pts/17   R+   17:09   0:00 grep nginx
13683
#
Comment 1 Michael Shigorin 2007-08-30 18:22:51 MSD
Не, гоню -- в спеке так:
--pid-path=%buildroot%_var/run/nginx.pid

Всё равно не понял, зачем тут %buildroot... (как и с --*-path) счас буду
пробовать это дело фиксить.
Comment 2 Denis Smirnov 2007-08-30 18:29:25 MSD
Спасибо!
Comment 3 Michael Shigorin 2007-08-30 21:31:23 MSD
Кажется, готово -- забирай у меня в git.
Comment 4 Denis Smirnov 2007-08-31 01:17:49 MSD
Спасибо. Ушло в incoming/

Кстати о -- попробуй git commit -a --amend, оно есть рулез (будет история почище).

Как нибудь в выходные поймай меня -- я расскажу как такие штуки делать совсем
красиво и прозрачно (хоть и заметно медленнее) с несколькими бранчами и
переписыванием истории rebase'ом.
Comment 5 Michael Shigorin 2007-08-31 02:12:45 MSD
Поймаю-поймаю, только рассказывать придётся с фиксацией на вики, поскольку Дима
мастер-класс по гиту на конференции тихонько скипнул ;)
Comment 6 Denis Smirnov 2007-08-31 02:25:48 MSD
Вот и будет замечательный обмен -- я тебе расскажу на пальцах, покажу, пример
приведу, и т.д. -- а ты на wiki зафиксируешь :)

Кстати о. Какое у нас есть удобное средство для записи в виде какого-нибудь
видео происходящего в конкретном окне? А то я могу действительно иногда такие
мастер-классы попробовать позаписывать. А ещё лучше, если бы на это можно было
бы накладывать звук (благо более-менее приличная аппаратура для этого у меня есть).
Comment 7 Denis Smirnov 2007-08-31 02:26:13 MSD
Кстати о. Ушло в incoming/ == fixed.
Comment 8 Dmitry V. Levin 2007-08-31 04:22:38 MSD
В nginx.init остались вызовы

kill -USR2 `cat "$PIDFILE"`
и
kill -TERM `cat "$OLDBINPID"`

Пока они есть, скрипт нельзя считать рабочим.

Функция upgrade() вообще ужасна:
- помимо небезопасной рассылки сигналов,
- sleep 5 выглядит дико, вместо этого должен быть цикл ожидания завершения
процесса;
- диагностика не соответствует происходящему.
Comment 9 Michael Shigorin 2007-08-31 04:45:32 MSD
(In reply to comment #8)
> В nginx.init остались вызовы kill -USR2 `cat "$PIDFILE"`
> и kill -TERM `cat "$OLDBINPID"`
> Пока они есть, скрипт нельзя считать рабочим.

Они чуть лучше, чем был (первый) -- я начал было всовывать туда stop_daemon, но
не закончил и откатил, поскольку на самом деле было ещё 2/3 остального дофиксить.

Это в TODO и блокер [моей] просьбы перенести в бранч, где оно мне тоже надо.

Если подсобишь сразу с правильной формой -- буду сильно признателен.

> Функция upgrade() вообще ужасна:

Да.

> - помимо небезопасной рассылки сигналов,

Можно подробнее для чайников?  Эти файлы имеют фиксированное положение, проверка
на существование/ненулевую длину осуществляется перед чтением (да, там есть
race), и пишутся они с uid==0.

Что именно там небезопасно и куда предлагается посмотреть в качестве примера
дёргания потенциально двух связок процессов с двумя pidfiles?

> - sleep 5 выглядит дико, вместо этого должен быть цикл ожидания завершения
> процесса;

Покажи пример?  Я смутно такие помню, но решил изобразить тривиальную форму.

Цикл, который я видел (жаба какая-то в 2001, точек на сорок) -- напрашивался в
функции.

> - диагностика не соответствует происходящему.

Вот редиска, заметил ;)  Опять же -- постарался чуточку поправить (по крайней
мере отпарировать success failure'ом), но сам ещё недоволен.

---

Ergo: погоди с рюшиками, то, что stop() не отрабатывало и upgrade() отрабатывало
странно, было действительно хуже.

И я до сих пор не уверен, что это не особенность/бага stop_daemon().
Comment 10 Dmitry V. Levin 2007-08-31 05:08:21 MSD
(In reply to comment #9)
> (In reply to comment #8)
> > В nginx.init остались вызовы kill -USR2 `cat "$PIDFILE"`
> > и kill -TERM `cat "$OLDBINPID"`
> > Пока они есть, скрипт нельзя считать рабочим.
> 
> Они чуть лучше, чем был (первый) -- я начал было всовывать туда stop_daemon, но
> не закончил и откатил, поскольку на самом деле было ещё 2/3 остального дофиксить.

Значит сейчас самое время ввернуть stop_daemon, пример есть в
/etc/init.d/template:reload

> > - помимо небезопасной рассылки сигналов,
> 
> Можно подробнее для чайников?  Эти файлы имеют фиксированное положение, проверка
> на существование/ненулевую длину осуществляется перед чтением (да, там есть
> race), и пишутся они с uid==0.
> Что именно там небезопасно

У меня нет в этом уверенности, да и нет гарантии того, что они не протухли.
stop_daemon существует для того, чтобы посылать такие сигналы безопасно.

> и куда предлагается посмотреть в качестве примера
> дёргания потенциально двух связок процессов с двумя pidfiles?

Обычный stop_daemon с разными значениями --pidfile и разными сигналами,
пример есть в /etc/init.d/template:reload

> > - sleep 5 выглядит дико, вместо этого должен быть цикл ожидания завершения
> > процесса;
> 
> Покажи пример?  Я смутно такие помню, но решил изобразить тривиальную форму.
> 
> Цикл, который я видел (жаба какая-то в 2001, точек на сорок) -- напрашивался в
> функции.

Пример есть в реализации функции stop_daemon.

> Ergo: погоди с рюшиками, то, что stop() не отрабатывало и upgrade() отрабатывало
> странно, было действительно хуже.

Для меня со стороны это не рюшики.
Представь себе s/nginx/sshd/ и станет совсем не смешно.

> И я до сих пор не уверен, что это не особенность/бага stop_daemon().

Ключ --name у stop_daemon предназначен для отправки сигналов
и остановки интерпретаторов скриптов по имени скрипта.
Так что это больше похоже на неправильное использование.
Comment 11 Michael Shigorin 2007-08-31 15:34:34 MSD
(In reply to comment #10)
> > > В nginx.init остались вызовы kill -USR2 `cat "$PIDFILE"`
> > Они чуть лучше, чем был (первый) -- я начал было всовывать туда stop_daemon,
> Значит сейчас самое время ввернуть stop_daemon, пример есть в
> /etc/init.d/template:reload

OK, будто работает (пока оставил sleep, см. ниже):

upgrade()
{
    conftest
    RETVAL=$?
    if [ $RETVAL -eq 0 ]; then
        echo -n "Upgrading running nginx binary: "
        [ -s "$PIDFILE" ] \
        && stop_daemon --pidfile "$PIDFILE" --expect-user root -USR2 -- nginx
        RETVAL=$?
        echo -n "Phasing out older nginx instance: "
        [ -s "$OLDBINPID" ] || sleep 5
        [ -s "$OLDBINPID" ] \
        && stop_daemon --pidfile "$OLDBINPID" --expect-user root -TERM -- nginx
    else
        RETVAL=1
    fi
    return $RETVAL
}

> > Что именно там небезопасно?
> нет гарантии того, что они не протухли.

Принято.

> > и куда предлагается посмотреть в качестве примера
> > дёргания потенциально двух связок процессов с двумя pidfiles?
> Обычный stop_daemon с разными значениями --pidfile и разными сигналами,
> пример есть в /etc/init.d/template:reload

Так было и начал писать; сделал опять, работает (надо ещё при реальном
обновлении пакета проверить, но пока будто да).

> > > - sleep 5 выглядит дико, вместо этого должен быть цикл ожидания
> > > завершения процесса;
> > Покажи пример?  Я смутно такие помню, но решил изобразить тривиальную форму.
> Пример есть в реализации функции stop_daemon.

Это?
# We really want to be sure

Дим, я без труда при помощи burnK6 и ls -lR / организовал ситуацию, когда
имеющийся код не сработал (не прибил TERM'ом старый экземпляр):

        echo -n "Phasing out older nginx instance: "
        stop_daemon --pidfile "$OLDBINPID" --expect-user root --no-announce -- nginx

В данном случае проблема в произвольно заданном небольшом значении задержки в
usleep (BTW если не изменяет склероз, то сейчас sleep 0.1 эффективней, чем
usleep 100000).  Копипастить этот код можно, но было бы логично обобщить его в
functions -- FR повесить?

Выглядит так:

# service nginx upgrade; ps aux | grep nginx
Checking configuration sanity for nginx: [ DONE ]
Upgrading running nginx binary: [ DONE ]
Phasing out older nginx instance: Service nginx is not running. [PASSED]
# ps auxw | grep nginx
root     23422  0.1  1.0  18004  5308 pts/10   S+   14:00   0:03 vim nginx
root     24582  0.0  0.5   5936  3004 ?        S    14:26   0:00 nginx: master
process /usr/sbin/nginx
_nginx   24587  0.0  0.8   9128  4620 ?        S    14:26   0:00 nginx: worker
process
_nginx   24588  0.0  0.8   9128  4620 ?        S    14:26   0:00 nginx: worker
process
root     24645  0.0  0.5   5932  2996 ?        S    14:27   0:00 nginx: master
process /usr/sbin/nginx
_nginx   24646  0.0  0.8   9124  4592 ?        S    14:27   0:00 nginx: worker
process
_nginx   24647  0.0  0.8   9124  4616 ?        S    14:27   0:00 nginx: worker
process
root     24661  0.0  0.1   1764   592 pts/3    S+   14:28   0:00 grep nginx
# head /var/run/nginx.pid*
==> /var/run/nginx.pid <==
24645

==> /var/run/nginx.pid.oldbin <==
24582

> Для меня со стороны это не рюшики.
> Представь себе s/nginx/sshd/ и станет совсем не смешно.
Критичность служб сильно разная.  Будет для меня nginx критичней sshd -- ...сам
понимаешь ;-)

> > И я до сих пор не уверен, что это не особенность/бага stop_daemon().
> Ключ --name у stop_daemon предназначен для отправки сигналов
> и остановки интерпретаторов скриптов по имени скрипта.
> Так что это больше похоже на неправильное использование.
Спасибо, исправлено.
Comment 12 Michael Shigorin 2007-08-31 16:57:55 MSD
Вот с таким перед погашением старого экземпляра работает:

waitpidfile()
{
        [ -z "$1" ] && exit 1
        MAXCOUNT=50
        counter=0
        until [ -s "$1" ]; do
                [ "$((counter++))" -eq "$MAXCOUNT" ] && break
                sleep 0.1
        done
}

Предлагаю MAXCOUNT в десятых секунды пытаться сперва взять из $2, по дефолту --
50, и всунуть это безобразие в functions.

Только тут ещё один вопрос -- я помню, что sleep 0.1 эффективнее, а led@ вот
говорит, что как раз наоборот -- usleep 100000.  sr@ сказал, что без разницы.

Как оно там на самом деле? :) (пока приведу к твоему виду)
Comment 13 Michael Shigorin 2007-08-31 20:21:03 MSD
IMHO FIXED