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

nixos/nginx: don't hide nginx config errors on nixos-rebuild --switch with reload enabled #76179

Merged
merged 8 commits into from Jan 4, 2020

Conversation

@danbst
Copy link
Contributor

@danbst danbst commented Dec 22, 2019

Closes #73455

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

cc @domenkozar @Izorkin @aanderse @ckauhaus

with reload enabled

Closes #73455
@danbst
Copy link
Contributor Author

@danbst danbst commented Dec 22, 2019

I know about #74072, but I have a test working so it is not blocked by that PR.

Copy link
Member

@domenkozar domenkozar left a comment

😍

@Ma27
Ma27 approved these changes Dec 23, 2019
danbst added 2 commits Dec 23, 2019
@danbst
Copy link
Contributor Author

@danbst danbst commented Dec 23, 2019

@Mic92 you are right, the systemctl reload was redundant. However I now changed it a bit - it doesn't reload nginx, but only checks config file for errors.

The reason I still do systemctl reload is to create an entry in log file for nginx.service:

# journalctl -u nginx | tail -n2
Dec 23 09:35:07 c1 systemd[1]: Reloading Nginx Web Server.
Dec 23 09:35:07 c1 systemd[1]: Reloaded Nginx Web Server.

I can implement that with running logger manually from reload script. Do you think it is better to run logger explicitly, rather than relying on systemd reload?

I also removed stopIfChanged = false;. Due to how nixos activation works, on nginx confiuration change it tries to a) restart nginx-config-reload.service and b) start multi-user.target. If there is an error in config, it will fail twice - first time during restart, second time during start, because those are done as separate systemctl commands. In some cases (DNS lookup error) this may cause very long activation delay.

However, if unit is stopped first, then activate starts both nginx-config-reload.service and multi-user.target together, which produces only one failure.

@ckauhaus
Copy link
Contributor

@ckauhaus ckauhaus commented Jan 2, 2020

@danbst Looks good to me. Please rebase to current master.

@danbst danbst merged commit cef68c4 into NixOS:master Jan 4, 2020
@danbst danbst deleted the danbst:nginx-reload-warn-errors branch Jan 4, 2020
@danbst
Copy link
Contributor Author

@danbst danbst commented Jan 4, 2020

@domenkozar @ckauhaus what do you think, this has to be backported to 19.09? I believe it should, but I need confirmation

@danbst danbst mentioned this pull request Jan 4, 2020
3 of 10 tasks complete
@domenkozar
Copy link
Member

@domenkozar domenkozar commented Jan 5, 2020

Yup.

dtzWill added a commit to dtzWill/nixpkgs that referenced this pull request Jan 16, 2020
… with reload enabled (NixOS#76179)

nixos/nginx: don't hide nginx config errors on nixos-rebuild --switch
with reload enabled

Closes NixOS#73455

(cherry picked from commit cef68c4)
dtzWill added a commit to dtzWill/nixpkgs that referenced this pull request Mar 31, 2020
… with reload enabled (NixOS#76179)

nixos/nginx: don't hide nginx config errors on nixos-rebuild --switch
with reload enabled

Closes NixOS#73455

(cherry picked from commit cef68c4)
${execCommand} -t && \
${pkgs.systemd}/bin/systemctl reload nginx.service
fi
'';
Comment on lines +721 to 724

This comment has been minimized.

@Mic92

Mic92 Aug 12, 2020
Contributor

This line breaks nginx. It causes directories in /var/cache/nginx/ to be owned by nobody, which breaks large POST uploads. I think this might due the fact that nginx is no longer started as root. cc @Izorkin

This comment has been minimized.

@Izorkin

Izorkin Aug 12, 2020
Contributor

#85820 - fix variant and #85820 (comment)

This comment has been minimized.

@Mic92

Mic92 Aug 12, 2020
Contributor

Ok. I have a simpler version. Will post.

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.