From 2a0117e649d3d85bf82cd058862d2c44be2a3c17 Mon Sep 17 00:00:00 2001 From: Tobias Stenzel Date: Thu, 12 Sep 2024 14:17:08 +0200 Subject: [PATCH 1/3] webproxy: explicitly set work dir for varnishncsa Before, we relied on varnishncsa's behaviour, looking for the default varnish work dir at /var/run/varnishd (where /var/run is linked to /run). /run/varnishd is a symlink created by us, pointing to the real location /var/spool/varnish/. Setting the work dir in the varnishncsa command line easier to understand and less error-prone when the state dir changes. PL-132901 --- nixos/roles/webproxy.nix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nixos/roles/webproxy.nix b/nixos/roles/webproxy.nix index dc2b6e582..ca2369269 100644 --- a/nixos/roles/webproxy.nix +++ b/nixos/roles/webproxy.nix @@ -112,7 +112,7 @@ in PIDFile = "/run/varnish/varnishncsa.pid"; User = "varnish"; Group = "varnish"; - ExecStart = "${cfg.package}/bin/varnishncsa -D -a -w /var/log/varnish.log -P /run/varnish/varnishncsa.pid"; + ExecStart = "${cfg.package}/bin/varnishncsa -n ${cfg.stateDir} -D -a -w /var/log/varnish.log -P /run/varnish/varnishncsa.pid"; ExecReload = "${kill} -HUP $MAINPID"; }; }; From 26f41c58b19ee9c5613407194383c3c2ed796e5e Mon Sep 17 00:00:00 2001 From: Tobias Stenzel Date: Thu, 12 Sep 2024 14:24:51 +0200 Subject: [PATCH 2/3] webproxy: varnishncsa should have a runtime dir named after the service Using /run/varnish for the varnishncsa was quite confusing and can lead to weird errors when varnish uses the same run time dir and restarting one of the services clears the runtime directory for both. We don't do that at the moment, but the varnish work dir can be changed. PL-132901 --- nixos/roles/webproxy.nix | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/nixos/roles/webproxy.nix b/nixos/roles/webproxy.nix index ca2369269..145657ddd 100644 --- a/nixos/roles/webproxy.nix +++ b/nixos/roles/webproxy.nix @@ -108,11 +108,11 @@ in serviceConfig = { Type = "forking"; Restart = "always"; - RuntimeDirectory = "varnish"; - PIDFile = "/run/varnish/varnishncsa.pid"; + RuntimeDirectory = "varnishncsa"; + PIDFile = "/run/varnishncsa/varnishncsa.pid"; User = "varnish"; Group = "varnish"; - ExecStart = "${cfg.package}/bin/varnishncsa -n ${cfg.stateDir} -D -a -w /var/log/varnish.log -P /run/varnish/varnishncsa.pid"; + ExecStart = "${cfg.package}/bin/varnishncsa -n ${cfg.stateDir} -D -a -w /var/log/varnish.log -P /run/varnishncsa/varnishncsa.pid"; ExecReload = "${kill} -HUP $MAINPID"; }; }; @@ -124,6 +124,9 @@ in # Link the default dir expected by varnish tools to # the actual location of the state dir. This makes the commands # usable without specifying the -n option every time. + + ### XXX: where do we rely on this symlink? Can be problematic when changing the state dir. + ### XXX: I think that platform code should explicitly set the work dir to force service restarts after changes. "L /run/varnishd - - - - ${cfg.stateDir}" ]; From 503e92552233808e634a3b0750208aaccd8081f1 Mon Sep 17 00:00:00 2001 From: Christian Theune Date: Thu, 12 Sep 2024 16:57:18 +0200 Subject: [PATCH 3/3] webproxy/varnish: final version of the fix to manage the varnish state dir properly a) use the real varnish upstream state directory b) only reload if it's a pure VCL change, restart otherwise For channel upgrades (e.g. when this statedir change is rolled out) this happens in maintenance anyway. We'll provide better public docs about the restart conditions in the near future. PL-132901 --- nixos/roles/webproxy.nix | 7 ------- nixos/services/varnish/default.nix | 15 +++++++++++---- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/nixos/roles/webproxy.nix b/nixos/roles/webproxy.nix index 145657ddd..8e5d65d0c 100644 --- a/nixos/roles/webproxy.nix +++ b/nixos/roles/webproxy.nix @@ -121,13 +121,6 @@ in systemd.tmpfiles.rules = [ "d /etc/local/varnish 2775 varnish service" "f /var/log/varnish.log 644 varnish varnish" - # Link the default dir expected by varnish tools to - # the actual location of the state dir. This makes the commands - # usable without specifying the -n option every time. - - ### XXX: where do we rely on this symlink? Can be problematic when changing the state dir. - ### XXX: I think that platform code should explicitly set the work dir to force service restarts after changes. - "L /run/varnishd - - - - ${cfg.stateDir}" ]; users.groups.varnish.members = [ diff --git a/nixos/services/varnish/default.nix b/nixos/services/varnish/default.nix index 4b4b12153..ab2311faa 100644 --- a/nixos/services/varnish/default.nix +++ b/nixos/services/varnish/default.nix @@ -103,7 +103,9 @@ in { services.varnish = { enable = true; enableConfigCheck = false; - extraCommandLine = lib.concatStringsSep " " [ cfg.extraCommandLine "-I ${commandsfile}" ]; + + stateDir = "/run/varnishd"; + extraCommandLine = lib.concatStringsSep " " [ cfg.extraCommandLine "-I /etc/varnish/startup" ]; inherit (cfg) http_address; config = '' vcl 4.0; @@ -120,14 +122,19 @@ in { ''; }; + environment.etc."varnish/startup".source = commandsfile; + systemd.services.varnish = let vcfg = config.services.varnish; in { - reloadIfChanged = true; - restartTriggers = [ cfg.extraCommandLine vcfg.package cfg.http_address ]; + preStart = lib.mkBefore '' + rm -rf ${vcfg.stateDir} + ''; + stopIfChanged = false; + reloadTriggers = [ commandsfile ]; reload = '' vadm="${vcfg.package}/bin/varnishadm -n ${vcfg.stateDir}" - cat ${commandsfile} | $vadm + cat /etc/varnish/startup | $vadm coldvcls=$($vadm vcl.list | grep " cold " | ${pkgs.gawk}/bin/awk {'print $5'})