Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nginx service: reload instead of restart #24476

Closed
wants to merge 1 commit into from

Conversation

@fpletz
Copy link
Member

commented Mar 30, 2017

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 #15906.

@mention-bot

This comment has been minimized.

Copy link

commented Mar 30, 2017

@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 #15906.
@fpletz fpletz force-pushed the mayflower:feature/nginx-reload branch to b12a3d1 Mar 30, 2017
@fpletz fpletz requested review from domenkozar, edolstra and globin Mar 30, 2017
Restart = "always";
RestartSec = "10s";
RestartSec = "1s";

This comment has been minimized.

Copy link
@fadenb

fadenb Mar 31, 2017

Member

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.

This comment has been minimized.

Copy link
@fpletz

fpletz Mar 31, 2017

Author Member

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
Mic92 approved these changes Mar 31, 2017
'"request":"$request",'
'"http_referer":"$http_referer",'
'"upstream_addr":"$upstream_addr"}';
'';

This comment has been minimized.

Copy link
@Mic92

Mic92 Mar 31, 2017

Contributor

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

This comment has been minimized.

Copy link
@fpletz

fpletz Mar 31, 2017

Author Member

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

Copy link
Contributor

left a comment

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.

@@ -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

This comment has been minimized.

Copy link
@danbst

danbst Apr 25, 2017

Contributor

typo confi

@fpletz

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2017

@fpletz Did you make any progress on this?

@bobvanderlinden

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2017

@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?

@ilya-kolpakov

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2017

@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.

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

This comment has been minimized.

Copy link
Member

commented Mar 25, 2018

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

@Mic92 Mic92 removed this from the 18.03 milestone Mar 26, 2018
@Infinisil

This comment has been minimized.

Copy link
Member

commented Oct 13, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2018

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

@fpletz

This comment has been minimized.

Copy link
Member Author

commented Oct 26, 2018

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

Copy link
Contributor

left a comment

Mentioned some issues, not very critical though.

# 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

This comment has been minimized.

Copy link
@danbst

danbst Jan 14, 2019

Contributor

I'd like this to respect restartIfChanged option

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

This comment has been minimized.

Copy link
@danbst

danbst Jan 14, 2019

Contributor

👍 Maybe also make RuntimeDirectoryMode=0750?

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}";

This comment has been minimized.

Copy link
@danbst

danbst Jan 14, 2019

Contributor

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.

This comment has been minimized.

Copy link
@ckauhaus

ckauhaus Jan 20, 2019

Contributor

Sounds like a very good idea

@Infinisil

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

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

danbst added a commit to danbst/nixpkgs that referenced this pull request Mar 11, 2019
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.
@danbst danbst referenced this pull request Mar 11, 2019
3 of 10 tasks complete
@mmahut

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

What is the status of this pull request?

@ckauhaus

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

Would be nice to get this finished sometimes...

dpausp added a commit to flyingcircusio/nixpkgs that referenced this pull request Jul 29, 2019
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 #15906.

(cherry-picked from PR NixOS/nixpkgs#24476)
dpausp added a commit to flyingcircusio/nixpkgs that referenced this pull request Jul 29, 2019
add a X-ConfigFile option that points to the last running config

NixOS/nixpkgs#24476
dpausp added a commit to flyingcircusio/nixpkgs that referenced this pull request Jul 30, 2019
add a X-ConfigFile option that points to the last running config

NixOS/nixpkgs#24476
dpausp added a commit to flyingcircusio/fc-nixos that referenced this pull request Jul 30, 2019
* 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
dpausp added a commit to flyingcircusio/fc-nixos that referenced this pull request Jul 31, 2019
* 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
dpausp added a commit to flyingcircusio/fc-nixos that referenced this pull request Aug 2, 2019
* 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
dpausp added a commit to flyingcircusio/fc-nixos that referenced this pull request Aug 2, 2019
* 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

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2019

The alternative PR #57429 is about to be merged.

@ckauhaus

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

Then we should close this one in favour of #57429

danbst added a commit that referenced this pull request Aug 21, 2019
* 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

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

#57429 is merged

@danbst danbst closed this Aug 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.