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: change log and cache directories #85862

Merged
merged 7 commits into from May 11, 2020
Merged

nginx: change log and cache directories #85862

merged 7 commits into from May 11, 2020

Conversation

@Izorkin
Copy link
Contributor

Izorkin commented Apr 23, 2020

Motivation for this change

Change locations log and cache folders

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

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@Izorkin
Copy link
Contributor Author

Izorkin commented Apr 23, 2020

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

@delroth
Copy link
Contributor

delroth commented Apr 23, 2020

What's the motivation for this change?

@Izorkin
Copy link
Contributor Author

Izorkin commented Apr 23, 2020

@jonringer
Copy link
Contributor

jonringer commented Apr 23, 2020

@jonringer
Copy link
Contributor

jonringer commented Apr 23, 2020

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

git pull -r origin/master
git push --force .. ..
@Izorkin Izorkin force-pushed the Izorkin:nginx-paths branch from 720d0fb to 68e8512 Apr 23, 2020
@Izorkin
Copy link
Contributor Author

Izorkin commented Apr 23, 2020

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

@Izorkin
Copy link
Contributor Author

Izorkin commented Apr 23, 2020

How to correct add patch?

    }) ++ [
      ./nix-skip-check-logs-path.patch
    ] ++ optionals (stdenv.hostPlatform != stdenv.buildPlatform) [
@flokli flokli marked this pull request as draft Apr 23, 2020
@Izorkin Izorkin force-pushed the Izorkin:nginx-paths branch 2 times, most recently from 39e571a to 9c5a5af Apr 24, 2020
@Izorkin
Copy link
Contributor Author

Izorkin commented Apr 25, 2020

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

@emilazy
Copy link
Member

emilazy commented Apr 25, 2020

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
Copy link
Contributor Author

Izorkin commented Apr 25, 2020

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?

@emilazy
Copy link
Member

emilazy commented Apr 25, 2020

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
Copy link
Member

emilazy commented Apr 25, 2020

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
Copy link
Member

emilazy commented Apr 25, 2020

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.

@Izorkin Izorkin force-pushed the Izorkin:nginx-paths branch from ae5adf5 to 691d7ea Apr 25, 2020
@Izorkin Izorkin marked this pull request as ready for review Apr 26, 2020
@flokli
Copy link
Contributor

flokli commented Apr 27, 2020

This PR definitely needs a changelog entry, and we need to think about how migration paths should look like.

At least the awstats NixOS module, and the service-runner NixOS test seem to contain references to the /var/spool/nginx, which would need to be updated - possibly more.

@Izorkin Izorkin force-pushed the Izorkin:nginx-paths branch from 691d7ea to dea2eed Apr 27, 2020
@jonringer
Copy link
Contributor

jonringer commented May 2, 2020

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

@Izorkin Izorkin force-pushed the Izorkin:nginx-paths branch from dea2eed to ad6ca93 May 2, 2020
@Izorkin
Copy link
Contributor Author

Izorkin commented May 2, 2020

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

@Izorkin
Copy link
Contributor Author

Izorkin commented May 2, 2020

@GrahamcOfBorg test service-runner

@Izorkin
Copy link
Contributor Author

Izorkin commented May 2, 2020

@GrahamcOfBorg build openresty

@jonringer jonringer requested a review from grahamc May 3, 2020
Copy link
Contributor

jonringer left a comment

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 Outdated Show resolved Hide resolved
nixos/modules/services/web-servers/nginx/default.nix Outdated Show resolved Hide resolved
pkgs/servers/http/nginx/generic.nix Show resolved Hide resolved
pkgs/servers/http/tengine/default.nix Show resolved Hide resolved
@aanderse
Copy link
Contributor

aanderse commented May 4, 2020

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:

@Izorkin Izorkin force-pushed the Izorkin:nginx-paths branch from ad6ca93 to 2d8d841 May 4, 2020
@Mic92 Mic92 merged commit 11c18fa into NixOS:master May 11, 2020
17 checks passed
17 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="2d8d841"; rev="2d8d8415c0c780307bcff62d1ba63f7ada46d1cb"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="2d8d841"; rev="2d8d8415c0c780307bcff62d1ba63f7ada46d1cb"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="2d8d841"; rev="2d8d8415c0c780307bcff62d1ba63f7ada46d1cb"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="2d8d841"; rev="2d8d8415c0c780307bcff62d1ba63f7ada46d1cb"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="2d8d841"; rev="2d8d8415c0c780307bcff62d1ba63f7ada46d1cb"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="2d8d841"; rev="2d8d8415c0c780307bcff62d1ba63f7ada46d1cb"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="2d8d841"; rev="2d8d8415c0c780307bcff62d1ba63f7ada46d1cb"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
nginx, nginx.passthru.tests, tengine, tengine.passthru.tests on aarch64-linux Success
Details
nginx, nginx.passthru.tests, tengine, tengine.passthru.tests on x86_64-darwin Success
Details
nginx, nginx.passthru.tests, tengine, tengine.passthru.tests on x86_64-linux Success
Details
@Izorkin Izorkin deleted the Izorkin:nginx-paths branch May 11, 2020
@aanderse
Copy link
Contributor

aanderse commented May 11, 2020

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
Copy link
Member

pbogdan commented May 15, 2020

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
Copy link
Contributor

Mic92 commented May 15, 2020

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.

@@ -23,7 +23,7 @@ import ./make-test-python.nix ({ pkgs, ... }: {
machine.fail(f"curl {url}")
machine.succeed(
"""
mkdir -p /run/nginx /var/spool/nginx/logs
mkdir -p /run/nginx /var/log/nginx /var/cache/nginx

This comment has been minimized.

Copy link
@andir

andir May 23, 2020

Member

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

This comment has been minimized.

Copy link
@andir

andir May 23, 2020

Member

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

This comment has been minimized.

Copy link
@Izorkin

Izorkin May 23, 2020

Author Contributor

Service nginx disabled by default:

systemd.services.nginx.enable = false;

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

This comment has been minimized.

Copy link
@flokli

flokli May 23, 2020

Contributor

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.

This comment has been minimized.

Copy link
@roberth

roberth May 23, 2020

Contributor

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.

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.