A negative configtest should be reported as a fail/error

[tux-o-matic]

Today a broken configuration can go unnoticed, even with 'configtest' enabled unless you go check /var/log/messages on the Nginx node.
The Puppet module should be able to handle 'configtest' as an Exec resource which failure can be reported on the Puppet master.

[ThomasLohner]

This works as expected for me with:

nginx::configtest_enable: true
nginx::service_restart: 'service nginx configtest && service nginx restart'

Puppet fails with:

Error: /Stage[main]/Nginx::Service/Service[nginx]: Failed to call refresh: Could not restart Service[nginx]: Execution of 'service nginx configtest && service nginx restart' returned 1: * Testing nginx configuration
   ...fail!
Error: /Stage[main]/Nginx::Service/Service[nginx]: Could not restart Service[nginx]: Execution of 'service nginx configtest && service nginx restart' returned 1: * Testing nginx configuration
   ...fail!

When testing / debugging, keep in mind that puppet only triggers refresh if resources change. so you need to do a change before every puppet run.

[tux-o-matic]

@ThomasLohner This is an error thrown by the agent on the node, won't be visible in Puppet DB.
Today configtest is not declared in the module as a resource, it seems hidden in a command call in some Ruby code.
What I'd rather see is two separate Puppet resources. If a configuration change triggers a refresh then configtest should be executed and if it fails then the service restart would be skipped. Such an implementation would be properly reported by the agent all the way to the MASTER

[tux-o-matic]

An extra benefit of splitting those steps into resources would be to insert in between if needed a cache clean up which might be needed by some people.
Example for proxy_cache:

if $nginx::config::clean_cache_on_restart {
  Exec{ 'cache_clean_up':
    before       => Service['nginx'],
    command  => "rm -fr ${nginx::config::proxy_cache_path}",
    path           => '/bin',
    refreshonly => true,
    subscribe   => Exec['configtest']
  }
}

[wyardley]

This makes some sense to me, @tux-o-matic: is this still something you're interested in (and if so, are you willing to contribute a PR and / or help test)?

Is it possible to "trap" the service failure from the puppet run rather than actually making an exec call to nginx -t?

Honestly, I have no idea how this is working at all:
https://github.com/voxpupuli/puppet-nginx/blob/master/manifests/service.pp#L18
https://github.com/voxpupuli/puppet-nginx/blob/master/manifests/service.pp#L51-L66
::nginx::service is directly being called, but in my test run on a RHEL 7 system, it seems to be (correctly) using systemd.

Info: Class[Nginx::Config]: Scheduling refresh of Class[Nginx::Service]
Info: Class[Nginx::Service]: Scheduling refresh of Service[nginx]
Debug: Executing '/bin/systemctl is-active nginx'
Debug: Executing '/bin/systemctl is-enabled nginx'
Debug: Executing '/bin/systemctl start nginx'
Debug: Executing '/bin/systemctl is-enabled nginx'
Notice: /Stage[main]/Nginx::Service/Service[nginx]/ensure: ensure changed 'stopped' to 'running'
Debug: /Stage[main]/Nginx::Service/Service[nginx]: The container Class[Nginx::Service] will propagate my refresh event
Info: /Stage[main]/Nginx::Service/Service[nginx]: Unscheduling refresh on Service[nginx]

With some custom modules written in-house (with a different config management system), we used to have a thing where the webserver actually wouldn't restart either if the config was bad. That would be kind of cool (if there was an option not to do the service reload if Puppet notices a config failure), but probably not easy to implement in this module, and could result in unpredictable behavior.

[wyardley]

Ah, got it, only overriding Service if $configtest is true, which it isn't by default. That stuff is clearly totally broken 😭😭😭, which doesn't help with this ticket, but the $configtest parameter and the code related to it should probably be ripped out and simplified.

[tux-o-matic]

The point of this issue is to ensure that any failed config test gets properly reported all the way up to PuppetDB. And hiding configtest within Ruby code or within the 'reload' function of the service script might not guarantee this level of reporting.

[wyardley]

The configtest_enable parameter is now gone. I think systems which do a config check as part of the failure should report a failure if there's a service failure currently, correct?

If someone wants to make a PR to add a (separate) nginx configtest call, and report failures, I can see some value in that, but for now, I'm going to close this issue.

[tux-o-matic]

As long as configtest is not an Exec resource called before a Service resource meant to restart Nginx, you won't have proper reporting.