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: expose generated config and allow nginx reloads #57429

Open
wants to merge 1 commit into
base: master
from

Conversation

@danbst
Copy link
Contributor

commented Mar 11, 2019

Motivation for this change

Fixes: #15906
Another try was done, but not yet merged in #24476

This add 2 new features: ability to review generated Nginx config
(and NixOS has sophisticated generation!) and reloading
of nginx on config changes. This preserves nginx restart on package
updates.

I've modified nginx test to use this new feature and check reload/restart
behavior.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.
nginx: expose generated config and allow nginx reloads
Fixes: #15906
Another try was done, but not yet merged in #24476

This add 2 new features: ability to review generated Nginx config
(and NixOS has sophisticated generation!) and reloading
of nginx on config changes. This preserves nginx restart on package
updates.

I've modified nginx test to use this new feature and check reload/restart
behavior.
@roberth

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2019

I don't care much about exposing a config file, so my eyes would skip past this option. In a way it's always exposed, through systemctl show, for example, or using nix repl or probably just nixos-option.
So I think the reloading behavior is more relevant than exposed or not. Perhaps the option should be enableReload. Also I'd like the default to be true, although that might break someone's workflow. Either way, this should be in the changelog.

@danbst

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2019

BTW it is not exposed in nix repl, configFile is a private let-binding when making virtual hosts via module system. And systemctl show isn't very fun for debugging...

But I agree this should be default! I just don't want to push this until there is some decision over #24476. I can rename option to enableReload if this will be chosen as a way to go.

cc @fpletz @Infinisil @fadenb @bobvanderlinden

@roberth

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2019

I guess I was getting philosophical about what expose means :)

Perhaps the rename can be done, and the change of default behavior can wait until a decision is made?

@bobvanderlinden
Copy link
Contributor

left a comment

This needs to move forward :(. The current implementation feels broken and we have had quite a few PRs proposed, all 'died' because we couldn't come up with a solution where everyone agrees on all aspects.

This PR looks good. I like the tests 👍.

The name exposeConfig doesn't seem to match the intent, but we might want to split exposeConfig from enableReload and assert enableReload -> exposeConfig. That way a user can explicitly set whether to reload/restart in addition to explicitly defining whether to 'expose' the config.

Another suggestion for the name exposeConfig: configInEtc.

As for the enableReload suggestion, how about reloadIfChanged, inheriting the naming from the systemd configuration?

Lastly, the default should be true to lessen the number of 'wtfs'. But for backwards compatibility it is better to have it on false. Is this a use-case where relying on stateVersion is an option?

You've got my approval. All my suggestions are optional and may be ignored if it helps moving this forward. I feel this PR is at least a step in the right direction, if not the solution to the issue.

@danbst

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2019

@bobvanderlinden unfortunately reloadIfChanged isn't correct name, as service can be restarted instead of reloaded. So

services.nginx.exposeConfig = true;
systemd.services.nginx.reloadIfChanged = false;

and

services.nginx.exposeConfig = true;
systemd.services.nginx.reloadIfChanged = true;

have different behavior. In both cases config will be reloaded, but in second case service will never be restarted.

I also think about making exposeConfig internal and true by default. Not sure people like such breaking changes, but I hope most will be fine with that. @fpletz @Mic92 ?

Is this a use-case where relying on stateVersion is an option?

no, it was stated a few times already that stateVersion is not schemaVersion.

@nixos-discourse

This comment has been minimized.

Copy link

commented Apr 19, 2019

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/20

@wmertens

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

I'm not sure if I am a fan of the expose config - on the one hand, it's nice to see a system-global configuration in /etc (read-only of course), but on the other hand it hides complexity and will make /etc a cesspit if everybody starts doing that.

The rest of the PR is great. Would be cool if you could also do a config check via nginx during the build. I made something similar for Apache but it was problematic because it didn't work if the apache user didn't exist yet.

@danbst

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2019

@wmertens This is more of pragmatic approach. Configuring Nginx via Nix language is fun and nice, but there is a big problem that one can't see the generated result. I had to look into generated config several times, and it is a pain to do when nginx.conf is only in nix store.

The main reason I did this in /etc is because /etc changes are transactional and rollbackable out of box.

Config check is not required for this usecase: if config has problem, reload will fail, and old config will be used by Nginx internally. You'll still get a warning on rebuild-switch that nginx reload failed. But I'm not against it in separate PR

@Infinisil

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Nice PR! I'm also for the rename to enableReload and for defaulting it to true. This behavior is preferable for most people by far. We can use the release notes to mention the change in behavior.

An internal exposeConfig option might be nice yeah, but I think this can be done in a separate PR to move this one along fast.

@danbst

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

I've rebased this branch on top of master, and test from #48337 stopped working. I'm still investigating into "why". Looks like nginx restart is different from SIGHUP with respect to caching.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.