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.

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

@@ -415,19 +415,39 @@ in
       description = "Nginx Web Server";
       after = [ "network.target" ];
       wantedBy = [ "multi-user.target" ];
-      stopIfChanged = false;
       preStart =
         ''
         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";
       };
     };
 

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 = "jenkins";
+  name = "nginx";
   meta = with pkgs.stdenv.lib.maintainers; {
-    maintainers = [ mbbx6spp ];
+    maintainers = [ mbbx6spp fpletz ];
   };
 
-  nodes = {
-    webserver =
-      { config, pkgs, ... }:
-      { 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\" ]]");
   '';
 })