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

Jump to bottom

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
Merged

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

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

@danbst
Copy link
Contributor Author

danbst commented Dec 22, 2019

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

domenkozar
domenkozar reviewed Dec 22, 2019
View reviewed changes
Copy link
Member

@domenkozar domenkozar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😍

@danbst
Copy link
Contributor Author

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 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 nginx-reload-warn-errors branch January 4, 2020 22:39
@danbst
Copy link
Contributor Author

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
Merged
10 tasks
@domenkozar
Copy link
Member

Yup.

dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request Jan 16, 2020
@danbst @dtzWill
nixos/nginx: don't hide nginx config errors on nixos-rebuild --switch…
cc78f93 
… 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 pushed a commit to dtzWill/nixpkgs that referenced this pull request Mar 31, 2020
@danbst @dtzWill
nixos/nginx: don't hide nginx config errors on nixos-rebuild --switch…
e712f84 
… 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)
Mic92
Mic92 reviewed Aug 12, 2020
View reviewed changes
nixos/modules/services/web-servers/nginx/default.nix
Comment on lines +721 to 724
${execCommand} -t && \
${pkgs.systemd}/bin/systemctl reload nginx.service
fi
'';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#85820 - fix variant and #85820 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I have a simpler version. Will post.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#95249

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92 Mic92 left review comments

@domenkozar domenkozar domenkozar left review comments

@Izorkin Izorkin Izorkin left review comments

@Ma27 Ma27 Ma27 approved these changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

nginx reloading is bad UX
6 participants
@danbst @ckauhaus @domenkozar @Mic92 @Ma27 @Izorkin