nginx: expose generated config and allow nginx reloads #57429
Conversation
Fixes: NixOS#15906 Another try was done, but not yet merged in NixOS#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.
|
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 |
|
BTW it is not exposed in 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 |
|
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? |
There was a problem hiding this 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.
|
@bobvanderlinden unfortunately and have different behavior. In both cases config will be reloaded, but in second case service will never be restarted. I also think about making
no, it was stated a few times already that |
|
This pull request has been mentioned on Nix community. There might be relevant details there: |
|
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. |
|
@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 The main reason I did this in 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 |
|
Nice PR! I'm also for the rename to An internal |
|
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. |
danbst
changed the title
|
cc @yorickvP @yegortimoshenko @aszlig The current blocker for this PR is that ETag test fails when |
|
@danbst: All the ETag test does is doing a switch-to-configuration to another config (btw. moving that to |
| ExecReload = "${pkgs.coreutils}/bin/kill -HUP $MAINPID"; | ||
| Restart = "always"; | ||
| RestartSec = "10s"; | ||
| StartLimitInterval = "1min"; | ||
| }; | ||
| }; | ||
|
|
||
| environment.etc."nginx/nginx.conf" = mkIf cfg.enableReload { |
There was a problem hiding this comment.
Hm, why is this only enabled if enableReload is true? Apart from that, I do have mixed feelings about exposing the config in /etc, because I do not want other applications to make certain assumptions about files /etc and generally want to keep as much things out of global paths to ensure they don't behave differently in a Nix build sandbox. That said, I can still mkForce it to an empty attrset, but maybe adding exposeConfigFile with a default of false might be a good idea? Maybe depends on how many users want it to occur in /etc versus how many do not want that behaviour.
There was a problem hiding this comment.
ohhh, the original commit actually called that right 8b036d5#diff-795fa65a7c41a479084de826e4e2e65cR460
It just people suggested that exposeConfig isn't a good name and it should actually be enableReload
Actually, I'd like to write that file in some other place, but only /etc has atomic replaces in NixOS. I don't want to move nginx.conf to runtime dir, as I'd like to have post-mortem (after nginx doesn't start) config checks.
yes, but previously this led to nginx restart, and with this PR it leads to |
|
@GrahamcOfBorg test nginx |
|
|
@danbst: Hmmm... maybe it's a race condition, so the reload happens after the curl... did you try to do |
|
@aszlig that was brilliant! Sleep helped indeed! |
|
@danbst: Okay, so we obviously don't want sleeps in test code, so I think we should figure out how to make sure that we properly wait for the reload to finish without sleeping for a fixed timespan. |
|
Eeesh, even nginx itself sleeps X-D /* allow new processes to start */
ngx_msleep(100); |
… using `nesting.clone`
|
@danbst: Okay, it seems we can't easily avoid the sleep, because nginx doesn't have any code for notifying that a reload has finished, which is a bummer... maybe annotate the sleep with an |
danbst
changed the title
| restartTriggers = [ configFile ]; | ||
| script = '' | ||
| if ${pkgs.systemd}/bin/systemctl -q is-active nginx.service ; then | ||
| ${pkgs.systemd}/bin/systemctl reload nginx.service |
There was a problem hiding this comment.
This should be config.systemd.package, but I think systemctl is in PATH by default, so it can be just a non-absolute systemctl.
| @@ -431,6 +435,16 @@ in | |||
| "; | |||
| }; | |||
|
|
|||
| enableReload = mkOption { | |||
| default = false; | |||
There was a problem hiding this comment.
I thought everybody agreed to make this true by default?
There was a problem hiding this comment.
🤔 Maybe I'm just too cautious to change default behavior. Would you like to do that? 😁
|
(Sorry for the just late review) |
|
Any reason this is not the default? I have always been bothered by the fact that a config change was restarting nginx and I have just discovered this new option. |
|
It could very well be for backwards compatibility. This option wasn't available before and back then it would always restart. I'm not sure whether it is possible to switch the default without potentially breaking existing configs. |
|
@vincentbernat note that there is a problem with this implementation. I just merged the fix #76179, but it is still in 19.09 That's another reason why it's not yet default - not polished enough. |
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
sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)nix path-info -Sbefore and after)