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.

Sign up for GitHub

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
+32 −5

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

Sorry, something went wrong.

@danbst
nixos/nginx: don't hide nginx config errors on nixos-rebuild --switch

Unverified

This user has not yet uploaded their public signing key.
GPG key ID: CED392DE0C483D00
Learn about vigilant mode
27777cf 
with reload enabled

Closes NixOS#73455
@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.

@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Dec 22, 2019
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
few fixes

Unverified

This user has not yet uploaded their public signing key.
GPG key ID: CED392DE0C483D00
Learn about vigilant mode
14b65e5
@danbst
another fix for extra delay during switch

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
dbf5137
@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

@domenkozar domenkozar

@Izorkin Izorkin

@Ma27 Ma27

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
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