Issue 4073 - Improve Nginx integration with "upstart" service provider

modules/nginx/manifests/init.pp

Index: modules/nginx/manifests/init.pp
diff --git a/modules/nginx/manifests/init.pp b/modules/nginx/manifests/init.pp
index 78a1cb537e07ca92796f11d308c0934bc5d6e040..398154e2a6dac7bf00913a4f38b3c554aaf77331 100644
--- a/modules/nginx/manifests/init.pp
+++ b/modules/nginx/manifests/init.pp
@@ -47,7 +47,6 @@ class nginx (
   file {'/etc/nginx/nginx.conf':
     content => template('nginx/nginx.conf.erb'),
     require => Package['nginx'],

[Wladimir Palant, 2016/05/27 11:15:08]

If I see it correctly, you replaced notify here by subscribe on the service
itself, and this effectively doesn't change anything. Why did you do it? It's
now inconsistent with other configuration files which still use notify.

[mathias, 2016/05/27 11:43:53]

Beside the notify for the service in this section here I have also removed
require for this file here in the service section - with the notification the
latter was implicit. 

Regarding consistency: That part of comment was a joke, right? Even if not:
There is no such rule regarding the ordering in Puppet files, neither in the
common coding style nor in any of our extension. (If you want to introduce a
preference for consistency I am open to that thought, though I actually prefer
explicit operand notation.)

But in fact there was still a rationale: Removing this line here implicitly
fixes the missing comma at the end of the line, in contrast to removing the
obsolete require in the other resource.

[Wladimir Palant, 2016/05/27 12:33:01]

Well, you require the package now - it should install nginx.conf so I don't see
a problem here, and I don't see why that change should be related.
I'm not talking about rules. However, there are multiple configuration files
listed here, all notifying the Nginx service. Now you took one of them and made
the service subscribe to it. I don't really see why you did it.

As it is now, the code suggests that Nginx depends on a single configuration
file. That's simply not true - there are multiple configuration files and also
certificates which will cause a service restart.
I hope you forgive me if I don't accept missing commas as rationale for
unrelated changes.

[mathias, 2016/05/27 14:16:16]

I do forgive you, but again this is not the full picture: The change was
related, and the chosen phenotype does not matter technically, at which exact
point I start considering rationale like this one - not earlier.

Please stop trying to micro-optimize such stuff in reviews; it's the wrong
channel, and just blocking operations. Especially because you seem to sometimes
still value opinion arguments the same as technical fact ones. In this example
it's the "unrelated" part: You did not make any argument to ratify that
attribute, let me articulate and write my motivation, but then state nothing
more than "I don't see how" in reply. That's quite some overhead for a possible
change that does not change anything in behavior.

But don't get me wrong, I do appreciate feedback in this direction. So, while
we're at it anyway, let's go down that road another time:

In fact I do see from your comments that there is a need for a convention on how
to denote relationships in Puppet. But that won't be as simple as "just prefer
notify over subscribe": I would argue that I care way more about what a service
could be triggered from (or, more general, depends on), and not want to
potentially search through the entire code-base to find notification
declarations. One could reply that there are plenty of cases where one cannot
know at service definition time what will trigger the resource later, and that's
correct. One could continue that the scheme that always works is preferable, and
that's correct as well.

But that does not implicitly favor notify over subscribe either: There are other
options, in particular the notation using (arrow) operators, which is not only
applicable in *every* case, but also way more explicit, grep- and readable. In
addition, it does not require definition with the underlying resource itself,
thus allows for flexible combination with i.e. function create_resources(). It's
clearly the most sophisticated approach.

Usually I would make a cut here and push towards a follow-up, leaving that
patch-set as it is in that regard and finally being able to continue with the
tasks depending on this one. But since you seem to feel pretty strongly about
some of these aspects, I let you decide:

a) introducing more unrelated changes by letting this very patch-set become an
example for the to-be-introduced convention of using operator notation or
b) creating an explicit follow-up ticket with the exact same intention.

So, what should it be?

[Wladimir Palant, 2016/05/27 14:41:34]

You are free to remove unrelated changes from this review. That's generally
advisable as unrelated changes tend to make reviews unnecessarily complicated.
Besides, the way I see things we really have a one-line change change here -
something that's not really obvious due to all the unrelated changes.
Yes, when it comes to code readability I value opinion arguments.
In fact, I verified myself that the behavior doesn't change (not being a Puppet
expert, this wasn't really obvious to me). The change does make some aspects of
what is happening here less obvious however, that's why I'm asking for
rationale.
As I said, I don't care about global conventions. I only care about
relationships being specified differently for one and the same service, within
one and the same file.
I didn't suggest anything like that.
Clearly b). The intention of this change is cleaning up nginx restart behavior,
there shouldn't be any additional non-trivial changes.

-    notify => Service['nginx']
   }
 
   file {'/etc/nginx/sites-available':
@@ -189,9 +188,10 @@ class nginx (
   service {'nginx':
     ensure => running,
     enable => true,
-    restart => '/etc/init.d/nginx reload',
+    hasrestart => true,

[Wladimir Palant, 2016/05/27 11:15:08]

Do I see it correctly that we will now restart Nginx for every configuration
change? In other words, whenever we deploy a configuration change Nginx will
stop accepting connections for some time. This is rather suboptimal, isn't there
any better solution?

[mathias, 2016/05/27 11:43:53]

No, not with Puppet, at least not without a custom service provider. But this is
not as bad as it may seem: Nginx restarts way faster than any common timeout,
and with hasrestart => true we avoid the common pitfall of first stopping and
then starting, which often leads to port collision (resp. false-positives) and
similar issues, thus is often done with a delay in between.

[Wladimir Palant, 2016/05/27 12:33:01]

That's not how I think it works - the original process has to stop before the
new one can start. And stopping takes a while on heavily loaded servers due to
some connections taking much time to be handled, during that time the server
won't accept any new connections.

How about specifying an explicit restart command, one that won't produce any
dropped connections?

  restart => 'PID=`cat /run/nginx.pid` && kill -USR2 $PID && sleep 1 && kill
-QUIT $PID',

This will start a new nginx instance before instructing the old one to shut
down. Shutting down the old instance will take minutes, the new instance will be
processing incoming connections already however.

[mathias, 2016/05/27 14:16:16]

Using the restart parameter for this approach is not the cleanest of solutions,
and there are some pitfalls; i.e. the restart being reported as successful
despite still being in progress and potentially failing. But I do see how the
resulting behavior could be preferable. Hence I'd say we try it out, and if it
works in this or adjusted form we can subsequently integrate it as a service
provider overlay for Nginx, covering most pitfalls.

Will update and test the patch-set aligned to your suggestion while waiting for
your feedback regarding the topic above.

[Wladimir Palant, 2016/05/27 14:41:34]

Thank you.

     hasstatus => true,
-    require => File['/etc/nginx/nginx.conf']
+    require => Package['nginx'],
+    subscribe => File['/etc/nginx/nginx.conf'],
   }
 
   file {'/usr/share/nginx/html/50x.html':