From a40138673580f1120f6b39c6946c20805f84f4d7 Mon Sep 17 00:00:00 2001
From: Franz Pletz <fpletz@fnordicwalking.de>
Date: Thu, 30 Mar 2017 18:13:37 +0200
Subject: [PATCH] nginx service: reload instead of restart

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 https://github.com/NixOS/nixpkgs/pull/24476)
---
 .../services/web-servers/nginx/default.nix    | 34 ++++++--
 nixos/tests/nginx.nix                         | 78 +++++++++++++------
 2 files changed, 82 insertions(+), 30 deletions(-)

diff --git a/nixos/modules/services/web-servers/nginx/default.nix b/nixos/modules/services/web-servers/nginx/default.nix
index 98c0833b4b521a2..76476f3f15e8e1b 100644
--- a/nixos/modules/services/web-servers/nginx/default.nix
+++ b/nixos/modules/services/web-servers/nginx/default.nix
@@ -650,18 +650,40 @@ in
       description = "Nginx Web Server";
       after = [ "network.target" ];
       wantedBy = [ "multi-user.target" ];
-      stopIfChanged = false;
       preStart =
         ''
         ${cfg.preStart}
-        ${cfg.package}/bin/nginx -c ${configFile} -p ${cfg.stateDir} -t
-        '';
+        mkdir -p ${cfg.stateDir}/logs
+        chmod 700 ${cfg.stateDir}
+        chown -R ${cfg.user}:${cfg.group} ${cfg.stateDir}
+        ln -sf ${configFile} /run/nginx/config
+        ln -sf ${cfg.package} /run/nginx/package
+      '';
+      reload = ''
+        # Check if the new config is valid
+        ${cfg.package}/bin/nginx -t -c ${configFile} -p ${cfg.stateDir}
+
+        # Check if the package changed
+        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.
+          ${pkgs.coreutils}/bin/kill -QUIT $MAINPID
+          exit 0
+        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 ${configFile} -p ${cfg.stateDir}";
-        ExecReload = "${pkgs.coreutils}/bin/kill -HUP $MAINPID";
+        ExecStart = "${cfg.package}/bin/nginx -c /run/nginx/config -p ${cfg.stateDir}";
         Restart = "always";
-        RestartSec = "10s";
+        RestartSec = "1s";
         StartLimitInterval = "1min";
+        RuntimeDirectory = "nginx";
       };
     };
 
diff --git a/nixos/tests/nginx.nix b/nixos/tests/nginx.nix
index 32b113649237a86..630fc8d9c7e988f 100644
--- a/nixos/tests/nginx.nix
+++ b/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
+#      syntax errors
 
 import ./make-test.nix ({ pkgs, ...} : {
   name = "nginx";
   meta = with pkgs.stdenv.lib.maintainers; {
-    maintainers = [ mbbx6spp ];
+    maintainers = [ mbbx6spp fpletz ];
   };
 
-  nodes = {
-    webserver =
-      { ... }:
-      { services.nginx.enable = true;
-        services.nginx.commonHttpConfig = ''
-        log_format ceeformat '@cee: {"status":"$status",'
-          '"request_time":$request_time,'
-          '"upstream_response_time":$upstream_response_time,'
-          '"pipe":"$pipe","bytes_sent":$bytes_sent,'
-          '"connection":"$connection",'
-          '"remote_addr":"$remote_addr",'
-          '"host":"$host",'
-          '"timestamp":"$time_iso8601",'
-          '"request":"$request",'
-          '"http_referer":"$http_referer",'
-          '"upstream_addr":"$upstream_addr"}';
-        '';
-        services.nginx.virtualHosts."0.my.test" = {
-          extraConfig = ''
+  nodes = let
+    commonConfig = customCfg:
+      { lib, ... }:
+      lib.mkMerge [
+        { services.nginx.enable = true;
+          services.nginx.commonHttpConfig = ''
+          log_format ceeformat '@cee: {"status":"$status",'
+            '"request_time":$request_time,'
+            '"upstream_response_time":$upstream_response_time,'
+            '"pipe":"$pipe","bytes_sent":$bytes_sent,'
+            '"connection":"$connection",'
+            '"remote_addr":"$remote_addr",'
+            '"host":"$host",'
+            '"timestamp":"$time_iso8601",'
+            '"request":"$request",'
+            '"http_referer":"$http_referer",'
+            '"upstream_addr":"$upstream_addr"}';
+          '';
+          services.nginx.virtualHosts."_".extraConfig = ''
             access_log syslog:server=unix:/dev/log,facility=user,tag=mytag,severity=info ceeformat;
           '';
-        };
+        }
+        customCfg
+      ];
+    in {
+      webserver = commonConfig {
+        services.nginx.virtualHosts."_".extraConfig = "return 200 works;";
       };
-  };
+      webserver_new = commonConfig {
+        services.nginx.virtualHosts."_".extraConfig = "return 200 new-content;";
+        services.nginx.package = pkgs.nginxMainline;
+      };
+      webserver_invalid = commonConfig {
+        services.nginx.appendHttpConfig = "foo";
+      };
+    };
+
+  testScript = { nodes, ... }: let
+      webserver_new = nodes.webserver_new.config.system.build.toplevel;
+      webserver_invalid = nodes.webserver_invalid.config.system.build.toplevel;
+    in ''
+    $webserver->start;
+
+    $webserver->waitForUnit("nginx");
+    $webserver->waitForOpenPort("80");
+    $webserver->succeed("[[ `curl http://localhost/` == \"works\" ]]");
+
+    $webserver->succeed("${webserver_new}/bin/switch-to-configuration test");
+
+    $webserver->waitForUnit("nginx");
+    $webserver->waitForOpenPort("80");
+    $webserver->succeed("[[ `curl http://localhost/` == \"new-content\" ]]");
 
-  testScript = ''
-    startAll;
+    $webserver->fail("${webserver_invalid}/bin/switch-to-configuration test");
 
     $webserver->waitForUnit("nginx");
     $webserver->waitForOpenPort("80");
+    $webserver->succeed("[[ `curl http://localhost/` == \"new-content\" ]]");
   '';
 })