Issue 29344646: Issue 4073 - Improve Nginx integration with "upstart" service provider (Closed)
|
Created:
May 26, 2016, 8:47 p.m. by mathias Modified:
June 7, 2016, 10:04 a.m. Reviewers:
Wladimir Palant CC:
darkue, Felix Dahlke Visibility:
Public. |
DescriptionIssue 4073 - Improve Nginx integration with "upstart" service provider
Patch Set 1 #
Total comments: 10
Patch Set 2 : Issue 4073 - Address feedback from code-review #Patch Set 3 : Issue 4073 - Address feedback from code-review #MessagesTotal messages: 9
https://codereview.adblockplus.org/29344646/diff/29344647/modules/nginx/manif... File modules/nginx/manifests/init.pp (right): https://codereview.adblockplus.org/29344646/diff/29344647/modules/nginx/manif... modules/nginx/manifests/init.pp:49: require => Package['nginx'], 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. https://codereview.adblockplus.org/29344646/diff/29344647/modules/nginx/manif... modules/nginx/manifests/init.pp:191: hasrestart => true, 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?
https://codereview.adblockplus.org/29344646/diff/29344647/modules/nginx/manif... File modules/nginx/manifests/init.pp (right): https://codereview.adblockplus.org/29344646/diff/29344647/modules/nginx/manif... modules/nginx/manifests/init.pp:49: require => Package['nginx'], On 2016/05/27 11:15:08, Wladimir Palant wrote: > 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. 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. https://codereview.adblockplus.org/29344646/diff/29344647/modules/nginx/manif... modules/nginx/manifests/init.pp:191: hasrestart => true, On 2016/05/27 11:15:08, Wladimir Palant wrote: > 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? 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.
https://codereview.adblockplus.org/29344646/diff/29344647/modules/nginx/manif... File modules/nginx/manifests/init.pp (right): https://codereview.adblockplus.org/29344646/diff/29344647/modules/nginx/manif... modules/nginx/manifests/init.pp:49: require => Package['nginx'], On 2016/05/27 11:43:53, mathias wrote: > Beside the notify for the service in this section here I have also removed > require for this file here in the service section 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. > 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.) 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. > 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. I hope you forgive me if I don't accept missing commas as rationale for unrelated changes. https://codereview.adblockplus.org/29344646/diff/29344647/modules/nginx/manif... modules/nginx/manifests/init.pp:191: hasrestart => true, On 2016/05/27 11:43:53, mathias wrote: > Nginx restarts way faster than any common timeout, > and with hasrestart => true we avoid the common pitfall of first stopping and > then starting, 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.
https://codereview.adblockplus.org/29344646/diff/29344647/modules/nginx/manif... File modules/nginx/manifests/init.pp (right): https://codereview.adblockplus.org/29344646/diff/29344647/modules/nginx/manif... modules/nginx/manifests/init.pp:49: require => Package['nginx'], On 2016/05/27 12:33:01, Wladimir Palant wrote: > 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. 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? https://codereview.adblockplus.org/29344646/diff/29344647/modules/nginx/manif... modules/nginx/manifests/init.pp:191: hasrestart => true, On 2016/05/27 12:33:01, Wladimir Palant wrote: > 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. 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.
https://codereview.adblockplus.org/29344646/diff/29344647/modules/nginx/manif... File modules/nginx/manifests/init.pp (right): https://codereview.adblockplus.org/29344646/diff/29344647/modules/nginx/manif... modules/nginx/manifests/init.pp:49: require => Package['nginx'], On 2016/05/27 14:16:16, mathias wrote: > Please stop trying to micro-optimize such stuff in reviews; it's the wrong > channel, and just blocking operations. 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. > Especially because you seem to sometimes > still value opinion arguments the same as technical fact ones. Yes, when it comes to code readability I value opinion arguments. > 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. 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. > In fact I do see from your comments that there is a need for a convention on how > to denote relationships in Puppet. 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. > But that won't be as simple as "just prefer > notify over subscribe": I didn't suggest anything like that. > 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. Clearly b). The intention of this change is cleaning up nginx restart behavior, there shouldn't be any additional non-trivial changes. https://codereview.adblockplus.org/29344646/diff/29344647/modules/nginx/manif... modules/nginx/manifests/init.pp:191: hasrestart => true, On 2016/05/27 14:16:16, mathias wrote: > Will update and test the patch-set aligned to your suggestion Thank you.
See also https://issues.adblockplus.org/ticket/4082 --
Avoiding seemingly unrelated change, though I still insist on this not being unlrelated to "improving integration" of the service resource. Tests succeed, however, two possible issues still remain: The sleep command might require a longer delay under load, but always be a good guessed balance between log spamming and luck anyway, at best. Also the path to the pid-file being hard-coded is of course not optimal, but fixing that one requires yet another bunch of "unrelated" changes (not only hard-coded here, and not the only related entity hard-coded in the module). But with the revamp of module nginx (while porting it into a new namespace, analogous to the ones we've already done) being scheduled anyway, this version of the patch-set should be enough, satisfying all currently addressed issues.
On 2016/05/27 16:03:42, mathias wrote: > The sleep command might require a > longer delay under load, but always be a good guessed balance between log > spamming and luck anyway, at best. Frankly, I'm not sure whether the sleep command is necessary at all, I added it merely as a precaution. If the signals are never reordered (which I think they are) then the nginx process will always run the new process before shutting down. The only issue might be that for a short period the new process won't be ready to respond to requests, yet the old one won't be responding any more. However, the server socket won't be shut down meaning that connection requests will simply queue up. These will be processed either way as soon as the new process is ready. > Also the path to the pid-file being hard-coded is of course not optimal, Another option would be running `pgrep -u root nginx` which will not be affected by stale PID files as a nice side-effect (note that the master process is the only one running under user root). LGTM but you are free to implement additional changes to address the issues above. 
|
