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

Debian run_dir should be in /var/run/nginx #1352

Merged
merged 1 commit into from
Oct 31, 2019

Conversation

anarcat
Copy link
Contributor

@anarcat anarcat commented Oct 31, 2019

I am not sure if it's correct anywhere else, but at the very least on
Debian, the "run dir" is not /var/nginx, but /var/run/nginx. That
seems like a better default everywhere, but I'm just going to scratch
that itch for now.

Ideally, this would actually be in /run/nginx since that's the new
standard (and the default in the Debian package), but I suspect this
will not work properly in older Debian releases (e.g. 7) still marked
as supported here.

This is important because /var is a real, on-disk, filesystem while /var/run points to /run which is a tmpfs, and therefore much faster.

This is a WIP because I suspect tests will fail with just this naive commit, but gotta start the conversation somewhere.

@puppet-community-rangefinder
Copy link

nginx::params is a class

The enclosing module is declared in 11 of 577 indexed public Puppetfiles.

Breaking changes to this file MAY impact these modules (near match):


These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

spec/classes/nginx_spec.rb Outdated Show resolved Hide resolved
@ekohl
Copy link
Member

ekohl commented Oct 31, 2019

I also considered /run. If Debian 7 is still listed then we should drop it.

@anarcat
Copy link
Contributor Author

anarcat commented Oct 31, 2019

I also considered /run. If Debian 7 is still listed then we should drop it.

agreed, but that seems like another ballgame completely.

@anarcat anarcat changed the title WIP: Debian run_dir should be in /var/run/nginx Debian run_dir should be in /var/run/nginx Oct 31, 2019
@anarcat anarcat requested a review from ekohl October 31, 2019 20:02
@anarcat
Copy link
Contributor Author

anarcat commented Oct 31, 2019

i've squashed this into a single commit, and this is ready for merge/review/etc. tests should pass now.

@bastelfreak bastelfreak added the bug Something isn't working label Oct 31, 2019
This is important because /var is a real, on-disk, filesystem while
/var/run points to /run which is a tmpfs, and therefore much faster.

I am not sure if it's correct anywhere else, but at the very least on
Debian, the "run dir" is *not* /var/nginx, but /var/run/nginx. That
seems like a better default everywhere, but I'm just going to scratch
that itch for now.

Ideally, this would actually be in /run/nginx since that's the new
standard (and the default in the Debian package), but I suspect this
will not work properly in older Debian releases (e.g. 7) still marked
as supported here.
@bastelfreak bastelfreak merged commit 5160e71 into voxpupuli:master Oct 31, 2019
@anarcat anarcat deleted the debian-run-dir branch November 1, 2019 20:09
@anarcat
Copy link
Contributor Author

anarcat commented Nov 16, 2019

i made #1359 to keep track of the other divergences remaining such as this one. there are three "critical" ones that might affect performance, and two needless changes that should probably be fixed as well. but the diff is pretty big even after those...

@anarcat
Copy link
Contributor Author

anarcat commented Nov 26, 2019

note that this might make nginx servers stumble upon a bug where the server does not reboot correctly because /run/nginx doesn't exist on boot. this can be fixed with the following override file in etc/systemd/system/nginx.service.d/runtime.conf:

[Service]
RuntimeDirectory=nginx

I have filed this against the Debian package (#945551), but maybe it would be worth fixing in this module as well?

@smortex
Copy link
Member

smortex commented Jan 9, 2020

@anarcat the systemd override indeed helps, but the sub-directories are created with 0700 permissions by nginx, and the module change them to 0755 which notifies the service. When the service restart, the directories get removed and everything starts again: the configuration never converge to a stable state.

  1. I feel like nginx is right and 0700 is more suitable than 0755 for these directories, I'd like to propose a fix.
  2. Yet, with the default configuration, the web server is unable to start on Debian after a reboot and before Puppet is run, which looks really bad. @bastelfreak shouldn't we revert to use Debian's default directories?

@jonemo
Copy link

jonemo commented Mar 30, 2020

the web server is unable to start on Debian after a reboot and before Puppet is run

@anarcat @smortex I observed this behavior after upgrading to puppet-nginx 1.1.0 and after a few hours of investigation ended up here. I understand the conundrum of wanting to follow the Debian convention for paths and Nginx not being quite cooperative. Is there a suggested workaround, for the time being?

@anarcat
Copy link
Contributor Author

anarcat commented Mar 30, 2020

@jonemo i did suggest a workaround in #1352 (comment)

i suggest someone motivated by this issue open at least a bug report describing the issue in the short term.

@mgurjanov-cti
Copy link

Hi, I faced this problem on Ubuntu bionic with Vagrant. First time Vagrant is run or provisioned puppet creates "/run/nginx/client_body_temp" folders, but when I halt vagrant and run it without provision option this folder is then missing as /run folder is tmpfs and therefore nginx fails to start.

@jonemo
Copy link

jonemo commented Apr 9, 2020

i suggest someone motivated by this issue open at least a bug report describing the issue in the short term.

@anarcat: The issue already exists in #1372. In order to have the entire conversation in one place, I added a lengthy comment to #1372 to summarize the discussion from this thread, show workarounds, and review paths forward I am aware of: #1372 (comment)

Rubueno pushed a commit to Rubueno/puppet-nginx that referenced this pull request Oct 19, 2020
Debian run_dir should be in /var/run/nginx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants