nginx: change log and cache directories by Izorkin · Pull Request #85862 · NixOS/nixpkgs

Izorkin · 2020-04-23T13:42:19Z

Motivation for this change

Change locations log and cache folders

cc @flokli @aanderse @Mic92 @delroth @jtojnar @danbst

Things done

Izorkin · 2020-04-23T13:43:03Z

@GrahamcOfBorg test nginx nginx-etag nginx-pubhtml nginx-sso

delroth · 2020-04-23T13:44:38Z

What's the motivation for this change?

Izorkin · 2020-04-23T13:48:19Z

#85752 (comment)
#34378

jonringer · 2020-04-23T17:05:49Z

@GrahamcOfBorg eval

jonringer · 2020-04-23T17:08:29Z

please rebase your branch ontop of master, and force push to fix eval:

git pull -r origin/master
git push --force .. ..

Izorkin · 2020-04-23T17:15:47Z

@GrahamcOfBorg test nginx nginx-etag nginx-pubhtml nginx-sso

Izorkin · 2020-04-23T17:28:52Z

How to correct add patch?

    }) ++ [
      ./nix-skip-check-logs-path.patch
    ] ++ optionals (stdenv.hostPlatform != stdenv.buildPlatform) [

pkgs/servers/http/nginx/nix-skip-check-logs-path.patch

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

Izorkin · 2020-04-25T13:07:13Z

@Emily reverted commit openresty. I did not find another variant. Please check.

emilazy · 2020-04-25T13:13:51Z

Why was 6d046e1 reverted? OpenResty is a distribution of nginx with additional patches and modules; keeping the derivations wholly separate will lead to code duplication and drift between the two, making maintenance harder.

If they're going to be separate then the third-party module functionality should at least be ported over to the OpenResty derivation; with the revert, there's no way to e.g. use OpenResty with the brotli module. But I don't see the benefit at all currently.

Izorkin · 2020-04-25T13:19:43Z

With this variant

    }) ++ [
      ./nix-skip-check-logs-path.patch
    ] ++ optionals (stdenv.hostPlatform != stdenv.buildPlatform) [

Error build openresty:

error: value is a path while a set was expected, at nixpkgs/pkgs/servers/http/openresty/default.nix:20:29

How to fix the error?

pkgs/servers/http/nginx/generic.nix

emilazy · 2020-04-25T13:37:56Z

The evaluation error can be fixed with:

diff --git a/pkgs/servers/http/openresty/default.nix b/pkgs/servers/http/openresty/default.nix
index 0e87b971985..01ee521a2a2 100644
--- a/pkgs/servers/http/openresty/default.nix
+++ b/pkgs/servers/http/openresty/default.nix
@@ -16,8 +16,8 @@ callPackage ../nginx/generic.nix args rec {
     sha256 = "1a1la7vszv1parsnhphydblz64ffhycazncn3ividnvqg2mg735n";
   };
 
-  fixPatch = patch:
-    runCommand "openresty-${patch.name}" { src = patch; } ''
+  fixPatch = patch: let name = patch.name or (builtins.baseNameOf patch); in
+    runCommand "openresty-${name}" { src = patch; } ''
       substitute $src $out \
         --replace "src/" "bundle/nginx-${nginxVersion}/src/"
     '';

The patch still fails to apply, though; you could either condition on the patch name in fixPatch and replace it with a local OpenResty version (gross), or I think the better approach would be to add skipLogsCheck ? ./nix-skip-checks-log-path.patch to the nginx derivation and pass in an alternate patch path for packages that need it.

emilazy · 2020-04-25T13:41:10Z

Actually, there's no need for a separate patch at all; just need to make OpenResty's fixPatch more robust:

diff --git a/pkgs/servers/http/openresty/default.nix b/pkgs/servers/http/openresty/default.nix
index 0e87b971985..9c01cfb19e1 100644
--- a/pkgs/servers/http/openresty/default.nix
+++ b/pkgs/servers/http/openresty/default.nix
@@ -16,10 +16,11 @@ callPackage ../nginx/generic.nix args rec {
     sha256 = "1a1la7vszv1parsnhphydblz64ffhycazncn3ividnvqg2mg735n";
   };
 
-  fixPatch = patch:
-    runCommand "openresty-${patch.name}" { src = patch; } ''
+  fixPatch = patch: let name = patch.name or (builtins.baseNameOf patch); in
+    runCommand "openresty-${name}" { src = patch; } ''
       substitute $src $out \
-        --replace "src/" "bundle/nginx-${nginxVersion}/src/"
+        --replace "a/" "a/bundle/nginx-${nginxVersion}/" \
+        --replace "b/" "b/bundle/nginx-${nginxVersion}/"
     '';
 
   buildInputs = [ postgresql ];

emilazy · 2020-04-25T13:43:00Z

As a general comment, I think it would be better to remove the directories in a postInstall phase than to patch the nginx build system to not create them at all; the latter is more brittle for updates, forks and variants, and should be handled upstream if possible, as @flokli suggested.

jonringer · 2020-05-02T01:33:50Z

@GrahamcOfBorg test nginx
@GrahamcOfBorg test nginx-etag
@GrahamcOfBorg test nginx-pubhtml
@GrahamcOfBorg test nginx-sso

Izorkin · 2020-05-02T04:32:45Z

@GrahamcOfBorg test nginx
@GrahamcOfBorg test nginx-etag
@GrahamcOfBorg test nginx-pubhtml
@GrahamcOfBorg test nginx-sso

Izorkin · 2020-05-02T04:43:16Z

@GrahamcOfBorg test service-runner

Izorkin · 2020-05-02T04:55:28Z

@GrahamcOfBorg build openresty

jonringer

I'm not an nginx user, but diff LGTM

also, tests pass.

I would like a few other reviews though, seeing as nginx is a pretty critical package.

nixos/doc/manual/release-notes/rl-2009.xml

nixos/modules/services/logging/awstats.nix

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

pkgs/servers/http/nginx/generic.nix

pkgs/servers/http/tengine/default.nix

aanderse · 2020-05-04T10:53:30Z

While I completely agree with this change I've had encounters with people in the nix community who feel very strong about not removing stateDir like options so I'm curious how well received this change will be after it is merged and starts hitting people's servers :man_thinking:

aanderse · 2020-05-11T11:54:20Z

I'm not an nginx user so there is only so much I can say about this specific change, but I'll reiterate that the overall idea of removing stateDir like options has not been well received in the past, and I expect that this change will cause a fair bit of frustration from some users. Just because you or I agree with a change doesn't mean we have consensus and should proceed.

I think this change represents a larger idea that hasn't received enough discussion from all parties impacted by it and merging prematurely wasn't the best way to proceed. At this point I guess we can just wait and see... time will tell how well accepted or not this change is.

pbogdan · 2020-05-15T10:06:27Z

Apologies if this is a simple question but as I'm not well versed with nginx / systemd I'm hoping someone might be able to help - is the log directory location guaranteed to be stable ie. always present under /var/log/nginx? Can I simply safely hardcode path in places where I previously used the value of the stateDir option? Sorry if I missed a note / changelog entry somewhere on how to deal with the removal of the option.

Mic92 · 2020-05-15T10:17:47Z

Apologies if this is a simple question but as I'm not well versed with nginx / systemd I'm hoping someone might be able to help - is the log directory location guaranteed to be stable ie. always present under /var/log/nginx? Can I simply safely hardcode path in places where I previously used the value of the stateDir option? Sorry if I missed a note / changelog entry somewhere on how to deal with the removal of the option.

Yes it's now in /var/log/nginx and /var/cache/nginx always.

andir · 2020-05-23T14:03:48Z

nixos/tests/service-runner.nix

        machine.succeed(
            """
-            mkdir -p /run/nginx /var/spool/nginx/logs
+            mkdir -p /run/nginx /var/log/nginx /var/cache/nginx


Why do we have to create the directories in the test?
I just moved system to the latest nixos-unstable bump and the log directory wasn't created automatically. I think we shouldn't fixup the tests like this. It is hiding the real errors...

I think the comment is still true but I was able to solve my issue. It was unrelated to this change.

Service nginx disabled by default:

systemd.services.nginx.enable = false;

Folders /run/nginx, /var/log/nginx and /var/cache/nginx not created automatically.

That service-runner module seems to be a "startup script generator", by parsing unit attributes.

It was added in 2013, and hasn't really seen a lot of changes to get on par with systemd.

I'm not sure who is using the service-runner thing today (@roberth maybe?), but I expect to not really work with many of the modules we use, and the systemd features we make use of.

Creating the directories was mostly added as a hack to not break the test, and I'd say adding support for all of systemd's features into a startup script generator was out of scope for this PR.

So the confusion here is that this is not a test for nginx but a test for the service-runner script.

service-runner is a mostly convenience for testing, but you could use it experimentally for other purposes. I don't use it for anything critical. I wrote this test to avoid bitrot in service-runner. I picked nginx because it stresses the argument handling in service-runner.

As flokli said, this changed line is a perfectly acceptable solution.

Roughly four years ago the default log directory for `nginx` changed from `/var/spool/nginx/logs` to `/var/log/nginx`: NixOS/nixpkgs#85862 This caused `/var/log/nginx/access.log` to accumulate four years worth of logs (currently ≈27 GB at the time of this writing). The fix is to update the `logrotate` script to point to the new log directory.

Izorkin force-pushed the nginx-paths branch from 720d0fb to 68e8512 Compare April 23, 2020 17:15

ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Apr 23, 2020

flokli reviewed Apr 23, 2020

View reviewed changes

pkgs/servers/http/nginx/nix-skip-check-logs-path.patch Outdated Show resolved Hide resolved

nixos/modules/services/web-servers/nginx/default.nix Outdated Show resolved Hide resolved

flokli marked this pull request as draft April 23, 2020 21:16

Izorkin force-pushed the nginx-paths branch 2 times, most recently from 39e571a to 9c5a5af Compare April 25, 2020 13:03

ofborg bot requested review from 7c6f434c, emilazy, fpletz, globin, lblasc and thoughtpolice April 25, 2020 13:10

ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Apr 25, 2020

emilazy reviewed Apr 25, 2020

View reviewed changes

pkgs/servers/http/nginx/generic.nix Outdated Show resolved Hide resolved

Izorkin force-pushed the nginx-paths branch from ae5adf5 to 691d7ea Compare April 25, 2020 14:10

ofborg bot added 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation labels Apr 27, 2020

Izorkin force-pushed the nginx-paths branch from dea2eed to ad6ca93 Compare May 2, 2020 04:32

jonringer approved these changes May 3, 2020

View reviewed changes

flokli requested changes May 4, 2020

View reviewed changes

Izorkin added 7 commits May 4, 2020 16:36

nixos/nginx: change log and cache directories

4d988ff

nginx: change logs path

a19800f

tengine: add ETag patch

1d71150

tengine: change logs path

98e0cba

nixos/awstats: change path to nginx logs

9f09914

nixos/tests/service-runner: change paths to nginx logs and cache

7b368b3

nixos/nginx: add release notes

2d8d841

Izorkin force-pushed the nginx-paths branch from ad6ca93 to 2d8d841 Compare May 4, 2020 13:37

Mic92 merged commit 11c18fa into NixOS:master May 11, 2020

Izorkin deleted the nginx-paths branch May 11, 2020 10:25

andir reviewed May 23, 2020

View reviewed changes

Kiwi mentioned this pull request Jun 15, 2020

nixos-unstable log file unwriteable nh2/nix-binary-cache-proxy#3

Open

flokli mentioned this pull request Jul 15, 2020

nginx: Not creating stateDir on nginx systemd service unit restart #85752

Closed

koslambrou mentioned this pull request Aug 24, 2020

nginx: can't change cache and log directories #96191

Closed

Gabriella439 mentioned this pull request Jun 12, 2024

Fix logrotate for nginx dhall-lang/dhall-lang#1380

Merged

Uh oh!

Conversation

Izorkin commented Apr 23, 2020

Motivation for this change

Things done

Uh oh!

Izorkin commented Apr 23, 2020

Uh oh!

delroth commented Apr 23, 2020

Uh oh!

Izorkin commented Apr 23, 2020

Uh oh!

jonringer commented Apr 23, 2020

Uh oh!

jonringer commented Apr 23, 2020

Uh oh!

Izorkin commented Apr 23, 2020

Uh oh!

Izorkin commented Apr 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Izorkin commented Apr 25, 2020

Uh oh!

emilazy commented Apr 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Izorkin commented Apr 25, 2020

Uh oh!

Uh oh!

emilazy commented Apr 25, 2020

Uh oh!

emilazy commented Apr 25, 2020

Uh oh!

emilazy commented Apr 25, 2020

Uh oh!

jonringer commented May 2, 2020

Uh oh!

Izorkin commented May 2, 2020

Uh oh!

Izorkin commented May 2, 2020

Uh oh!

Izorkin commented May 2, 2020

Uh oh!

jonringer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aanderse commented May 4, 2020

Uh oh!

aanderse commented May 11, 2020

Uh oh!

pbogdan commented May 15, 2020

Uh oh!

Mic92 commented May 15, 2020

Uh oh!

andir May 23, 2020

Choose a reason for hiding this comment

Uh oh!

andir May 23, 2020

Choose a reason for hiding this comment

Uh oh!

Izorkin May 23, 2020

Choose a reason for hiding this comment

Uh oh!

flokli May 23, 2020

Choose a reason for hiding this comment

Uh oh!

roberth May 23, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Izorkin commented Apr 23, 2020 •

edited

Loading

emilazy commented Apr 25, 2020 •

edited

Loading