nginx service: reload instead of restart #24476

fpletz · 2017-03-30T16:54:25Z

This ensures the nginx daemon is reloaded for zero downtime.

If the package changes, nginx needs to be restarted instead because the integrated on the fly upgrade does not work.

mention-bot · 2017-03-30T16:54:27Z

@fpletz, thanks for your PR! By analyzing the history of the files in this pull request, we identified @globin, @domenkozar and @fadenb to be potential reviewers.

This ensures the nginx daemon is reloaded for zero downtime. If the package changes, nginx needs to be restarted instead because the integrated on the fly upgrade does not work. Fixes NixOS#15906.

fadenb · 2017-03-31T08:39:53Z

nixos/modules/services/web-servers/nginx/default.nix

        Restart = "always";
-        RestartSec = "10s";
+        RestartSec = "1s";


Any specific reason to wait a whole second and not use the default value?

RestartSec=¶
Configures the time to sleep before restarting a service (as configured with Restart=). Takes a unit-less value in seconds, or a time span value such as "5min 20s". Defaults to 100ms.

Yeah, the default should be fine. I just wanted to reduce the default time of 10s, because that would be an unnecessary wait. I'll change that.

Mic92 · 2017-03-31T08:59:10Z

nixos/tests/nginx.nix

+            '"request":"$request",'
+            '"http_referer":"$http_referer",'
+            '"upstream_addr":"$upstream_addr"}';
+          '';


nitpick: do we actually need this complex log format in a test?

The test came originally from #23279. I agree that we could probably simplify it a bit.

danbst

As far as I understand, user overrides of config.systemd.services.nginx.* won't trigger service restart anymore. This should be noted in release notes.

Thought, I don't know of wild uses of such overrides for nginx.

danbst · 2017-04-25T11:13:27Z

nixos/tests/nginx.nix

@@ -1,42 +1,72 @@
 # verifies:
 #   1. nginx generates config file with shared http context definitions above
 #      generated virtual hosts config.
+#   2. configuration reload works and nginx keeps running with old confi on


typo confi

fpletz · 2017-04-27T15:41:53Z

State of this PR: I'll address the feedback soon, but killing nginx makes rebuilds fail. I'll check if we can maybe set some magic systemd service options to fix that (SuccessExitStatus).

bachp · 2017-08-19T21:50:13Z

@fpletz Did you make any progress on this?

bobvanderlinden · 2017-10-04T19:31:08Z

@fpletz The issue you were seeing during rebuilds (killing a process while doing a reload) causes systemd to think the process has failed? I can see how this is the intended behavior from systemd.

Would it be possible for NixOS itself to know whether to reload or restart the process during the switch? Maybe something like systemd.services.<name>.restartTriggers, but for reloads?

1pakch · 2017-10-26T15:48:31Z

@domenkozar I see this is probably the implementation of your proposal.

Shouldn't it be done for all services by default? It can make it much simpler to determine which services needed to be reloaded/restarted/stopped on configuration switch. The current implementation in switch-to-implementation.pl is really convoluted IMO.

infinisil · 2018-03-25T21:36:32Z

Any updates on this? It doesn't seem to be ready for 18.03 so the milestone should probably be moved (again).

infinisil · 2018-10-13T22:53:46Z

@fpletz

killing nginx makes rebuilds fail.

I can't confirm this, what error did you get? It seems to work for me.

I tested this PR and it works well, I suggest these changes though:

diff --git a/nixos/modules/services/web-servers/nginx/default.nix b/nixos/modules/services/web-servers/nginx/default.nix
index 2c2757747ec..bb5d29a005f 100644
--- a/nixos/modules/services/web-servers/nginx/default.nix
+++ b/nixos/modules/services/web-servers/nginx/default.nix
@@ -618,25 +618,23 @@ in
         ${cfg.package}/bin/nginx -t -c ${configFile} -p ${cfg.stateDir}
 
         # Check if the package changed
-        if [[ `readlink /run/nginx/package` != ${cfg.package} ]]; then
+        if [[ $(readlink /run/nginx/package) != ${cfg.package} ]]; then
           # If it changed, we need to restart nginx. So we kill nginx
           # gracefully. We can't send a restart to systemd while in the
           # reload script. Nginx will be restarted by systemd automatically.
+          echo "Nginx package changed, gracefully quitting so systemd can restart it"
           ${pkgs.coreutils}/bin/kill -QUIT $MAINPID
-          exit 0
+        else
+          # We only need to change the configuration, so update it and reload nginx
+          echo "Restart not needed, only reloading"
+          ln -sf ${configFile} /run/nginx/config
+          ${pkgs.coreutils}/bin/kill -HUP $MAINPID
         fi
-
-        # We only need to change the configuration, so update it and reload nginx
-        ln -sf ${configFile} /run/nginx/config
-        ${pkgs.coreutils}/bin/kill -HUP $MAINPID
       '';
-      restartTriggers = [ configFile ];
       reloadIfChanged = true;
       serviceConfig = {
         ExecStart = "${cfg.package}/bin/nginx -c /run/nginx/config -p ${cfg.stateDir}";
         Restart = "always";
-        RestartSec = "1s";
-        StartLimitInterval = "1min";
         RuntimeDirectory = "nginx";
       };
     };

restartTriggers isn't needed, because the reload script already contains the config file so it will automatically be a restart trigger.

Default RestartSec is 100ms, default StartLimitIntervalSec is 10sec, those seem good. Sidenote: I'm not sure why it's StartLimitInterval all over nixpkgs, when the setting is really called StartLimitIntervalSec, are we sure those settings apply?

Interestingly, nginx also supports exchanging the executable without downtime, but that would be a bit harder to implement, see https://www.nginx.com/resources/wiki/start/topics/tutorials/commandline/, out of scope for this PR imo

ckauhaus · 2018-10-26T16:14:43Z

my 2ct: rebase, clean up, merge please :-)

fpletz · 2018-10-26T16:19:56Z

I'll pick this up soon. We have this running in production for a while on our nixpkgs fork.

danbst

Mentioned some issues, not very critical though.

danbst · 2019-01-14T16:47:36Z

nixos/modules/services/web-servers/nginx/default.nix

+          # If it changed, we need to restart nginx. So we kill nginx
+          # gracefully. We can't send a restart to systemd while in the
+          # reload script. Nginx will be restarted by systemd automatically.
+          ${pkgs.coreutils}/bin/kill -QUIT $MAINPID


I'd like this to respect restartIfChanged option

danbst · 2019-01-14T16:51:54Z

nixos/modules/services/web-servers/nginx/default.nix

        StartLimitInterval = "1min";
+        RuntimeDirectory = "nginx";


👍 Maybe also make RuntimeDirectoryMode=0750?

danbst · 2019-01-14T16:57:58Z

nixos/modules/services/web-servers/nginx/default.nix

      serviceConfig = {
-        ExecStart = "${cfg.package}/bin/nginx -c ${configFile} -p ${cfg.stateDir}";
-        ExecReload = "${pkgs.coreutils}/bin/kill -HUP $MAINPID";
+        ExecStart = "${cfg.package}/bin/nginx -c /run/nginx/config -p ${cfg.stateDir}";


So, RuntimeDirectory is removed when service is stopped. This effectively makes impossible to inspect nginx config after systemctl stop nginx. I propose to inject config path somehow into systemd unit. For example, as a comment. So it would be possible:

$ sudo systemctl stop nginx $ less $(systemctl cat nginx | grep "#Configuration=" | cut -d= -f2)

Main usecase for this was to diagnose why can't it be started, but you also added nginx -t call, so this may be no longer of an urgent need.

Sounds like a very good idea

infinisil · 2019-01-23T20:35:49Z

Oh actually, one problem with this is that it breaks the workflow of some people for starting ad-hoc nginx instances, e.g. in a nix-shell, via systemd.services.nginx.runner

Fixes: NixOS#15906 Another try was done, but not yet merged in NixOS#24476 This add 2 new features: ability to review generated Nginx config (and NixOS has sophisticated generation!) and reloading of nginx on config changes. This preserves nginx restart on package updates. I've modified nginx test to use this new feature and check reload/restart behavior.

mmahut · 2019-06-27T12:30:21Z

What is the status of this pull request?

ckauhaus · 2019-07-02T16:33:24Z

Would be nice to get this finished sometimes...

This ensures the nginx daemon is reloaded for zero downtime. If the package changes, nginx needs to be restarted instead because the integrated on the fly upgrade does not work. Fixes NixOS#15906. (cherry-picked from PR NixOS#24476)

add a X-ConfigFile option that points to the last running config NixOS#24476

* make emailACME optional in json config * enableACME is activated automatically if one of forceSSL, onlySSL or addSSL is set to true. * JSON config can be now be split into multiple files * merge all snippets into a single set * prevents and reports duplicate vhost names * use nginx reload code from upstream PR NixOS/nixpkgs#24476 * our reload code is superseded by the changes in nixpkgs * reloading on fc-manage now works for json config * changes to *.conf are picked up after systemctl reload nginx * add nginx config helper commands * nginx-show-config shows the currently activated nginx config * nginx-check-config runs nginx -t for the current config * update readme, json config is preferred now * include modified nginx test from nixpkgs Case 110209

* make emailACME optional in json config * enableACME is activated automatically if one of forceSSL, onlySSL or addSSL is set to true. * JSON config can be now be split into multiple files * merge all snippets into a single set * prevents and reports duplicate vhost names * use nginx reload code from upstream PR NixOS/nixpkgs#24476 * our reload code is superseded by the changes in nixpkgs * reloading on fc-manage now works for json config * changes in /etc/local/nginx are picked up by fc-manage * add nginx config helper commands * nginx-show-config shows the currently activated nginx config * nginx-check-config runs nginx -t for the current config * update readme, json config is preferred now * include modified nginx test from nixpkgs Case 110209 wip conf files

* make emailACME optional in json config * enableACME is activated automatically if one of forceSSL, onlySSL or addSSL is set to true. * JSON config can be now be split into multiple files * merge all snippets into a single set * prevents and reports duplicate vhost names * plain config files are concatenated to nginx base config * use nginx reload code from upstream PR NixOS/nixpkgs#24476 * our reload code is superseded by the changes in nixpkgs * reloading on fc-manage now works for json config * changes in /etc/local/nginx are picked up by fc-manage * add nginx config helper commands * nginx-show-config shows the currently activated nginx config * nginx-check-config runs nginx -t for the current config * update readme, json config is preferred now * include modified nginx test from nixpkgs Case 110209

danbst · 2019-08-18T22:13:46Z

The alternative PR #57429 is about to be merged.

ckauhaus · 2019-08-21T13:38:05Z

Then we should close this one in favour of #57429

* nginx: expose generated config and allow nginx reloads Fixes: #15906 Another try was done, but not yet merged in #24476 This add 2 new features: ability to review generated Nginx config (and NixOS has sophisticated generation!) and reloading of nginx on config changes. This preserves nginx restart on package updates. I've modified nginx test to use this new feature and check reload/restart behavior. * rename to enableReload * add sleep(1) in ETag test (race condition) and rewrite rebuild-switch using `nesting.clone`

danbst · 2019-08-21T13:53:40Z

#57429 is merged

fpletz added 6.topic: nixos 8.has: module (update) labels Mar 30, 2017

fpletz added this to the 17.09 milestone Mar 30, 2017

nginx service: reload instead of restart

b12a3d1

This ensures the nginx daemon is reloaded for zero downtime. If the package changes, nginx needs to be restarted instead because the integrated on the fly upgrade does not work. Fixes NixOS#15906.

fpletz force-pushed the feature/nginx-reload branch from d44b35f to b12a3d1 Compare March 30, 2017 16:56

fpletz requested review from domenkozar, edolstra and globin March 30, 2017 17:18

fadenb reviewed Mar 31, 2017

View reviewed changes

Mic92 approved these changes Mar 31, 2017

View reviewed changes

domenkozar mentioned this pull request Apr 6, 2017

nginx: Check config syntax validity with nginx -t in preStart #24664

Merged

danbst suggested changes Apr 25, 2017

View reviewed changes

fpletz added the 2.status: work-in-progress label Apr 27, 2017

fpletz modified the milestones: 17.09, 18.03 Oct 26, 2017

Mic92 removed this from the 18.03 milestone Mar 26, 2018

danbst reviewed Jan 14, 2019

View reviewed changes

danbst mentioned this pull request Mar 11, 2019

nginx: expose generated config and allow nginx reloads #57429

Merged

10 tasks

dpausp added a commit to flyingcircusio/nixpkgs that referenced this pull request Jul 30, 2019

nginx reload: some cleanup suggestions from PR

8fd1c5a

add a X-ConfigFile option that points to the last running config NixOS#24476

dpausp mentioned this pull request Jul 30, 2019

Nginx: JSON config and reload production-ready flyingcircusio/fc-nixos#24

Merged

danbst closed this Aug 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nginx service: reload instead of restart #24476

nginx service: reload instead of restart #24476

fpletz commented Mar 30, 2017 •

edited

mention-bot commented Mar 30, 2017

fadenb Mar 31, 2017

fpletz Mar 31, 2017

Mic92 Mar 31, 2017

fpletz Mar 31, 2017

danbst left a comment

danbst Apr 25, 2017

fpletz commented Apr 27, 2017

bachp commented Aug 19, 2017

bobvanderlinden commented Oct 4, 2017

1pakch commented Oct 26, 2017 •

edited

infinisil commented Mar 25, 2018

infinisil commented Oct 13, 2018

ckauhaus commented Oct 26, 2018

fpletz commented Oct 26, 2018

danbst left a comment

danbst Jan 14, 2019

danbst Jan 14, 2019

danbst Jan 14, 2019

ckauhaus Jan 20, 2019

infinisil commented Jan 23, 2019

mmahut commented Jun 27, 2019

ckauhaus commented Jul 2, 2019

danbst commented Aug 18, 2019

ckauhaus commented Aug 21, 2019

danbst commented Aug 21, 2019

nginx service: reload instead of restart #24476

nginx service: reload instead of restart #24476

Conversation

fpletz commented Mar 30, 2017 • edited

mention-bot commented Mar 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danbst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpletz commented Apr 27, 2017

bachp commented Aug 19, 2017

bobvanderlinden commented Oct 4, 2017

1pakch commented Oct 26, 2017 • edited

infinisil commented Mar 25, 2018

infinisil commented Oct 13, 2018

ckauhaus commented Oct 26, 2018

fpletz commented Oct 26, 2018

danbst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infinisil commented Jan 23, 2019

mmahut commented Jun 27, 2019

ckauhaus commented Jul 2, 2019

danbst commented Aug 18, 2019

ckauhaus commented Aug 21, 2019

danbst commented Aug 21, 2019

fpletz commented Mar 30, 2017 •

edited

1pakch commented Oct 26, 2017 •

edited