systemd: PID file /var/run/nginx.pid not readable (yet?) after start.

Maxim Dounin mdounin на mdounin.ru
Пт Ноя 24 04:12:31 UTC 2017


Hello!

On Fri, Nov 24, 2017 at 01:12:46AM +0200, Gena Makhomed wrote:

> On 23.11.2017 23:00, Maxim Dounin wrote:
> 
> >>> Это всё замечательно (за вычетом того, предлагаемое использование
> >>> daemon(3) почему-то не учитывает, что после вызова daemon(3)
> >>> parent-процесса уже нет, а "ошибка" - не ошибка), но не отменяет
> >>> того, что чуть менее, чем все существующие демоны делают именно
> >>> "daemon(); write_pidfile();", и при таком подходе - ситуацию не
> >>> изменить.
> 
> >> А при каком подходе ситуацию c nginx изменить можно?
> >> Если говорить конструктивно.
> 
> > Чтобы изменить ситуацию конкретно с nginx - нужно сесть и сделать
> > хороший патч.  Очевидно, это сделать можно, и даже не очень
> > сложно.  Я, как уже неоднократно сказал, не возражаю.
> 
> Для меня это будет слишком сложный патч, в разумные сроки не напишу.
> 
> > Но сама идея, что все должны сесть и заняться выпиливанием
> > стандартного паттерна, который работал десятки лет, и делать
> > вместо это что-то своё с синхронизацией - не взлетит.
> 
> Эта идея уже взлетела. Если демон состоит из одного процесса
> - systemd может однозначно узнать его pid, проблемы могут возникать
> только с теми демонами, которые состоят из нескольких процессов.
> Из известных мне сервисов состоящих из более чем одного процесса:
> 
> * postfix - сделали синхронизацию и проблем с systemd больше нет.
> * httpd - перевели на Type=notify и проблем с systemd больше нет.
> * php-fpm - перевели на Type=notify и проблем с systemd больше нет.
> * nginx - только с этим сервисом наблюдаются проблемы под systemd.

Давайте, всё-таки, опеределимся: мы за всё хорошее против всего 
плохого (== чтобы демоны писали pid-файлы до выхода запущенного 
процесса, потому что по другому - плохо), или вопрос исключительно 
в том, чтобы systemd не ругался в логи?

Если за всё хорошее - то проблема, очевидно, не ограничевается 
сервисами из более чем одного процесса, и не решается переводом на 
Type=notify.  Она вообще ортогональна существованию systemd.  И 
идея её решения, очевидно, не взлетела, и уже наверное не взлетит.

Если чтобы systemd не ругался - то проблема, очевидно, не в том, 
чтобы сделать хорошо, а в том, убрать конкретную ругань (которая 
не несёт практического смысла, см. ниже).  И предолженный ранее 
workaround про sleep 0.1 - вполне себе в этом ключе же решение?

> >> О каких именно "чуть менее, чем все существующие демоны"
> >> сервисах Вы говорите? Есть еще кроме nginx примеры некорректного
> >> поведения systemd-сервисов с Type=forking которые запускают много
> >> дочерних процессов как это делает nginx или postfix?
> 
> > Не вижу причин, почему демоны с "много дочерних процессов" должны
> > отличаться от сервисов с "мало дочерних процессов".
> 
> systemd однозначно определяет pid демонов состоящих из одного процесса
> и поэтому для них в юнит-файле можно вообще не указывать опцию PIDFile=
> - все будет работать как надо даже если они стартуют без синхронизации.
> 
> Вот что говорит Lennart Poettering из Red Hat:
> 
> If you use Type=forking, then you'll get away with not specifiying a
> PID file in most cases, but it's racy as soon as you have more than
> one daemon process, and nginx appears to be one of this kind, hence
> please specify PIDFile=.
> 
> https://lists.freedesktop.org/archives/systemd-devel/2017-November/039833.html

Ну вот я посмотрел на это место чуть подробнее, и имею сказать, 
что это не совсем соответствует действительности.  Единственное, 
для чего нужен PIDFile в случае nginx'а - это чтобы systemd 
нормально реагировал на binary upgrade.

Для правильного детектирования основного процесса при запуске 
PIDFile не нужен, так как master-процесс - единственный, у кого 
parent'ом systemd, у остальных процессов parent'ом будет master.  
И соответственно все остальные процессы успешно отфильтрует вот 
этот код,
https://github.com/systemd/systemd/blob/master/src/core/cgroup.c#L1727:

                /* Ignore processes that aren't our kids */
                if (get_process_ppid(npid, &ppid) >= 0 && ppid != mypid)
                        continue;

Однако если PIDFile не указывать, то "service nginx upgrade" 
приведёт к тому, что после выхода старого мастера systemd будет 
считать, что nginx умер, и убьёт все новые процессы.  Поэтому 
PIDFile указывать таки надо.

Соответственно имеем то что имеем: PIDFile указывать надо, от 
этого на старте могут появляться сообщения про "PID file not ... yet?".  
Сообщения эти безвредные, и ни на что не влияют, кроме собственно 
появления самих сообщений.

Если идти по пути синхронизации через pipe, то патч получается 
как-то такой.  Не могу сказать, что он мне нравится, особенно в 
контексте решения задачи "чтобы у systemd в логе стало на одну 
строчку меньше".

diff -r 325b3042edd6 src/core/nginx.c
--- a/src/core/nginx.c	Thu Nov 23 16:33:40 2017 +0300
+++ b/src/core/nginx.c	Fri Nov 24 07:07:44 2017 +0300
@@ -361,6 +361,10 @@
         return 1;
     }
 
+    if (ngx_daemon_sync(cycle->log) != NGX_OK) {
+        return 1;
+    }
+
     if (ngx_log_redirect_stderr(cycle) != NGX_OK) {
         return 1;
     }
diff -r 325b3042edd6 src/os/unix/ngx_daemon.c
--- a/src/os/unix/ngx_daemon.c	Thu Nov 23 16:33:40 2017 +0300
+++ b/src/os/unix/ngx_daemon.c	Fri Nov 24 07:07:44 2017 +0300
@@ -9,10 +9,19 @@
 #include <ngx_core.h>
 
 
+static ngx_fd_t  ngx_daemon_fd = NGX_INVALID_FILE;
+
+
 ngx_int_t
 ngx_daemon(ngx_log_t *log)
 {
-    int  fd;
+    u_char    buf[1];
+    ngx_fd_t  fd, pp[2];
+
+    if (pipe(pp) == -1) {
+        ngx_log_error(NGX_LOG_EMERG, log, ngx_errno, "pipe() failed");
+        return NGX_ERROR;
+    }
 
     switch (fork()) {
     case -1:
@@ -20,9 +29,30 @@
         return NGX_ERROR;
 
     case 0:
+        if (close(pp[0]) == -1) {
+            ngx_log_error(NGX_LOG_EMERG, log, ngx_errno, "close() pipe failed");
+            return NGX_ERROR;
+        } 
+
+        ngx_daemon_fd = pp[1];
         break;
 
     default:
+        if (close(pp[1]) == -1) {
+            ngx_log_error(NGX_LOG_EMERG, log, ngx_errno, "close() pipe failed");
+            return NGX_ERROR;
+        } 
+
+        if (read(pp[0], buf, 1) != 1) {
+            ngx_log_error(NGX_LOG_EMERG, log, ngx_errno, "read() pipe failed");
+            return NGX_ERROR;
+        }
+
+        if (close(pp[0]) == -1) {
+            ngx_log_error(NGX_LOG_EMERG, log, ngx_errno, "close() failed");
+            return NGX_ERROR;
+        }
+
         exit(0);
     }
 
@@ -68,3 +98,26 @@
 
     return NGX_OK;
 }
+
+
+ngx_int_t
+ngx_daemon_sync(ngx_log_t *log)
+{
+    if (ngx_daemon_fd == NGX_INVALID_FILE) {
+        return NGX_OK;
+    }
+
+    if (write(ngx_daemon_fd, "", 1) != 1) {
+        ngx_log_error(NGX_LOG_EMERG, log, ngx_errno, "write() failed");
+        return NGX_ERROR;
+    }
+
+    if (close(ngx_daemon_fd) == -1) {
+        ngx_log_error(NGX_LOG_EMERG, log, ngx_errno, "close() failed");
+        return NGX_ERROR;
+    }
+
+    ngx_daemon_fd = NGX_INVALID_FILE;
+
+    return NGX_OK;
+}
diff -r 325b3042edd6 src/os/unix/ngx_os.h
--- a/src/os/unix/ngx_os.h	Thu Nov 23 16:33:40 2017 +0300
+++ b/src/os/unix/ngx_os.h	Fri Nov 24 07:07:44 2017 +0300
@@ -40,6 +40,7 @@
 ngx_int_t ngx_os_specific_init(ngx_log_t *log);
 void ngx_os_specific_status(ngx_log_t *log);
 ngx_int_t ngx_daemon(ngx_log_t *log);
+ngx_int_t ngx_daemon_sync(ngx_log_t *log);
 ngx_int_t ngx_os_signal_process(ngx_cycle_t *cycle, char *sig, ngx_pid_t pid);
 
 

-- 
Maxim Dounin
http://mdounin.ru/


Подробная информация о списке рассылки nginx-ru