Opened 17 months ago

Last modified 11 months ago

#4073 reviewing change

Improve Nginx integration with Puppet service resource

Reported by: matze Assignee: matze
Priority: P1 Milestone:
Module: Infrastructure Keywords:
Cc: palant, fhd Blocked By:
Blocking: #4145 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29344646

Description

Our Puppet/Nginx integration in module nginx, in particular the definition of the service resource (see below), is currently hindering us from efficient roll-outs in production: Upgrading the software package always requires subsequent, manual restarting of Nginx on the target systems, including manual intervention to ensure log file integrity.

Background

While there have been different packages and -sources been in use in the past, it's not really necessary to analyze what could have been improved with each respective version. The important part is that the current version and the Puppet code below have plenty of issues:

189   service {'nginx':
190     ensure => running,
191     enable => true,
192     restart => '/etc/init.d/nginx reload',
193     hasstatus => true,
194     require => File['/etc/nginx/nginx.conf']
195   }

Puppet resources (and authors) assume a notification to cause a restart, not a reload. This goes irregardless of a reload being considered better than a restart in many cases, simply because automating the decision with Puppet in reliable fashion is not something one can achieve without violating a bunch of other best practices. The motivation for this very ticket here is just one example of the issues something like our hack can cause.

In addition, it is recommended to avoid specifying a custom restart parameter for the sake of both portability and maintainability. Properly setting hasrestart where applicable is a way more straightforward approach, especially with more modern back-ends like upstart or systemd, in contrast to init. The latter, which is used in our code, is more or less a legacy wrapper for upstart in our current setups anyway.

What To Do

Re-configure the Nginx service and associated resources in order to ensure a restart being what's expected, and package upgrades being possible without additional, manual intervention on each host.

Note that the upstart configuration that ships with the Nginx package includes the respawn directive (/etc/init/nginx.conf), which renders our sitescripts service supervisor obsolete for this case. (In fact most modern init-alternatives provide a similar feature.)

Change History (4)

comment:1 Changed 17 months ago by matze

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:2 Changed 17 months ago by abpbot

comment:3 Changed 17 months ago by matze

  • Blocking 4145 added

comment:4 Changed 11 months ago by matze

  • Blocking 4068 removed
Note: See TracTickets for help on using tickets.