Opened 8 years ago

Closed 8 years ago

#953 closed defect (fixed)

Error on logrotate when Nginx is stopped

Reported by: indy2kro@… Owned by:
Priority: minor Milestone:
Component: nginx-package Version: 1.9.x
Keywords: Cc:
uname -a: Linux - 2.6.32-573.18.1.el6.x86_64 #1 SMP Wed Jan 6 11:20:49 EST 2016 x86_64 x86_64 x86_64 GNU/Linux
nginx -V: nginx version: nginx/1.7.9
built by gcc 4.4.7 20120313 (Red Hat 4.4.7-3) (GCC)
TLS SNI support enabled
configure arguments: --prefix=/etc/nginx --sbin-path=/usr/sbin/nginx --conf-path=/etc/nginx/nginx.conf --error-log-path=/var/log/nginx/error.log --http-log-path=/var/log/nginx/access.log --pid-path=/var/run/nginx.pid --lock-path=/var/run/nginx.lock --http-client-body-temp-path=/var/cache/nginx/client_temp --http-proxy-temp-path=/var/cache/nginx/proxy_temp --http-fastcgi-temp-path=/var/cache/nginx/fastcgi_temp --http-uwsgi-temp-path=/var/cache/nginx/uwsgi_temp --http-scgi-temp-path=/var/cache/nginx/scgi_temp --user=nginx --group=nginx --with-http_ssl_module --with-http_realip_module --with-http_addition_module --with-http_sub_module --with-http_dav_module --with-http_flv_module --with-http_mp4_module --with-http_gunzip_module --with-http_gzip_static_module --with-http_random_index_module --with-http_secure_link_module --with-http_stub_status_module --with-http_auth_request_module --with-mail --with-mail_ssl_module --with-file-aio --with-ipv6 --with-http_spdy_module --with-cc-opt='-O2 -g -pipe -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic'

Description

Steps to reproduce:

  1. Start Nginx
  2. Have some traffic on Nginx (logs must not be empty)
  3. Stop Nginx
  4. Logrotate is executed (can be forced)

Expected results:

No error, Nginx will not be reloaded since it is not running anyway.

Actual results:

The following error is returned:

[root@dev logrotate.d]# /etc/init.d/nginx stop
Stopping nginx:                                            [  OK  ]
[root@dev logrotate.d]# logrotate --force nginx 
error: error running shared postrotate script for '/var/log/nginx/*.log '

This happens because the && operator does not work exactly like an if/then statement, anything after && is executed and the final result is then computed. Because of this, it tries to do cat /var/run/nginx.pid even if this file does not exist.

Current logrotate configuration /etc/logrotate.d/nginx:

postrotate
  [ -f /var/run/nginx.pid ] && kill -USR1 `cat /var/run/nginx.pid`
endscript

Proposed logrotate configuration:

postrotate
  if [ -f /var/run/nginx.pid ]; then
    kill -USR1 `cat /var/run/nginx.pid`
  fi
endscript

Change History (2)

comment:1 by Maxim Dounin, 8 years ago

Component: other → nginx-package
Status: new → accepted

Thanks for the report, the error clearly shouldn't be reported in this situation. The explanation why the error happens you've suggested looks wrong though.

According to POSIX, the && operator is not expected to execute any commands unless previous commands returned zero, see ​here.

The "error running shared postrotate script..." message appears for a different reason: the postrotate script returns non-zero exit status if there is no pid file, and logrotate sees it and complains. Adding || true to the script should fix it.

Patch for the ​package repository:

# HG changeset patch
# User Maxim Dounin <mdounin@mdounin.ru>
# Date 1463266829 -10800
#      Sun May 15 02:00:29 2016 +0300
# Node ID 81c8ae85472bd7bec24b38dc14a00dd507063aa6
# Parent  2043093e0956f36fda1660239d20ac86db65c945
Fixed logrotate error if nginx not running (ticket #953).

diff --git a/debian/logrotate b/debian/logrotate
--- a/debian/logrotate
+++ b/debian/logrotate
@@ -8,6 +8,7 @@
         create 640 nginx adm
         sharedscripts
         postrotate
-                [ -f /var/run/nginx.pid ] && kill -USR1 `cat /var/run/nginx.pid`
+                [ -f /var/run/nginx.pid ] \
+                        && kill -USR1 `cat /var/run/nginx.pid` || true
         endscript
 }
diff --git a/rpm/SOURCES/logrotate b/rpm/SOURCES/logrotate
--- a/rpm/SOURCES/logrotate
+++ b/rpm/SOURCES/logrotate
@@ -8,6 +8,7 @@
         create 640 nginx adm
         sharedscripts
         postrotate
-                [ -f /var/run/nginx.pid ] && kill -USR1 `cat /var/run/nginx.pid`
+                [ -f /var/run/nginx.pid ] \
+                        && kill -USR1 `cat /var/run/nginx.pid` || true
         endscript
 }
diff --git a/rpm/SOURCES/nginx.suse.logrotate b/rpm/SOURCES/nginx.suse.logrotate
--- a/rpm/SOURCES/nginx.suse.logrotate
+++ b/rpm/SOURCES/nginx.suse.logrotate
@@ -8,6 +8,7 @@
         create 640 nginx trusted
         sharedscripts
         postrotate
-                [ -f /var/run/nginx.pid ] && kill -USR1 `cat /var/run/nginx.pid`
+                [ -f /var/run/nginx.pid ] \
+                        && kill -USR1 `cat /var/run/nginx.pid` || true
         endscript
 }

comment:2 by Sergey Budnevitch, 8 years ago

Resolution: → fixed
Status: accepted → closed

Patch with || true will not work correctly if pid file exists, but has a wrong pid within - logrotate should return error in this case, but it will end silently. Patch with originally proposed approach was committed.

Note: See TracTickets for help on using tickets.