Drop run_dir and make client_body_temp_path/proxy_temp_path optional #1478

b4ldr · 2021-10-27T10:48:29Z

The general description of /run/ is fairly vague and not in the original spec however my high level reading is that it relates more to small files which are used for service orchestration and not data (temporary or otherwise) which is specific to the programs operations for the later /var/lib which states the following seems more appropriate:

Furthermore, this hierarchy holds state information pertaining to an application .... State information is data that programs modify while they run, and that pertains to one specific host.

https://tldp.org/LDP/Linux-Filesystem-Hierarchy/html/Linux-Filesystem-Hierarchy.html#var

And as we see from the debian packages this is the route they have gone (and likely don't consider this a bug). although it is worth noting they only override this only for the full package, the light package keeps the default which ends up being --prefix and thus /usr/share/nginx/client_temp/.

Of course there is a performance argument to make that says theses files should be in a ramdisk, however its also possible that this dir could become very large and putting it in a ramdisk could cause swapping or other system issues as such i think this decision should be left to the operator with sane defaults provided by the package maintain

As run_dir is only ever used for setting proxy_temp_path and client_body_temp_path i suggest that we completely drop run_dir and make client_body_temp_path and proxy_temp_path optional, thus ensuring the configuration uses the distro configured defaults. if we don't want to use the package defaults then we should ensure we ensure things work correctly annd in this case i think using the systemd dropin would make the most senses

Fixes #1372

puppet-community-rangefinder · 2021-10-27T10:48:41Z

nginx::config is a class

that may have no external impact to Forge modules.

nginx is a class

that may have no external impact to Forge modules.

nginx::params is a class

that may have no external impact to Forge modules.

This module is declared in 9 of 578 indexed public `Puppetfiles`.

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

…tional The [general description of `/run/`](https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch03s15.html) is fairly vague and not in the original spec however my high level reading is that it relates more to small files which are used for service orchestration and not data (temporary or otherwise) which is specific to the programs operations for the later /var/lib which states the following seems more appropriate: > Furthermore, this hierarchy holds state information pertaining to an application .... State information is data that programs modify while they run, and that pertains to one specific host. https://tldp.org/LDP/Linux-Filesystem-Hierarchy/html/Linux-Filesystem-Hierarchy.html#var And as we see from the [debian packages](https://salsa.debian.org/nginx-team/nginx/-/blob/master/debian/rules#L61-63) this is the route they have gone (and likely don't consider this a bug). although it is worth noting they only override this only for the full package, the light package keeps the default which ends up being `--prefix` and thus `/usr/share/nginx/client_temp/`. Of course there is a performance argument to make that says theses files should be in a ramdisk, however its also possible that this dir could become very large and putting it in a ramdisk could cause swapping or other system issues as such i think this decision should be left to the operator with sane defaults provided by the package maintain As run_dir is only ever used for setting `proxy_temp_path` and `client_body_temp_path` i suggest that we completely drop run_dir and make `client_body_temp_path` and `proxy_temp_path` optional, thus ensuring the configuration uses the distro configured defaults. if we don't want to use the package defaults then we should ensure we ensure things work correctly annd in this case i think using the systemd dropin would make the most senses This CR also adds resources to manage the creation of proxy_cache_path and fastcgi_cache_path directories Fixes #1372

…tional This is the same as #1478 however changes the spec tests to check for is.expect.not_to instead of removing Fixes: #1372

ekohl · 2021-10-28T12:39:58Z

templates/conf.d/nginx.conf.erb

@@ -216,7 +216,7 @@ http {
 <% end -%>
 <% if @proxy_cache_path.is_a?(Hash) -%>
 <% @proxy_cache_path.sort_by{|k,v| k}.each do |key,value| -%>
-  proxy_cache_path        <%= key %> keys_zone=<%= value %> levels=<%= @proxy_cache_levels %> max_size=<%= @proxy_cache_max_size %> inactive=<%= @proxy_cache_inactive -%>
+  proxy_cache_path        <%= key %> levels=<%= @proxy_cache_levels %> keys_zone=<%= value %> max_size=<%= @proxy_cache_max_size %> inactive=<%= @proxy_cache_inactive -%>


Is this change intended? Looks like it doesn't change anything.

this was changed to match the same output when proxy_cache_path is a string. it makes it easier to script the rpsec matching

ekohl · 2021-10-28T12:40:23Z

This would be considered backwards incompatible, right?

b4ldr · 2021-10-28T12:44:47Z

This would be considered backwards incompatible, right?

yes

b4ldr · 2021-10-28T12:53:17Z

spec/classes/nginx_spec.rb

+                      is_expected.to contain_file(param[:value]).with_ensure('directory')
+                    end
+                  end
+


theses tests identified that we never manage fastcgi_cache_path or proxy_cache_path perhaps that is interventional if so i can refactor

b4ldr · 2021-10-28T12:54:18Z

manifests/config.pp

+      mode   => '0700',
+    }
+  }
+


Just wanted to explicitly highlight this. This was identified by the rspec additions on line 1079 (comment also there). I can more this to a separate PR if desired or drop it entirely?

@ekohl just want to make sure you also saw theses as im not sure they where present when you did your first pass. thanks

I did. I don't have a strong opinion on these but I wonder how we can make it clear in the changelog. It's backwards incompatible, so it'd be OK to deal with for users (they may manage the directories themselves leading to duplicate resource declarations) but it'd be great if they can find out prior to updating.

kenyon

Let's fix this thing finally! Thanks for the work @b4ldr.

b4ldr · 2021-11-24T11:50:13Z

Hi all Is there anything outstanding on this one? didn't want to self merge as im not a user of this module, thx

ashish1099 · 2021-12-06T11:14:00Z

I think it LGTM

b4ldr mentioned this pull request Oct 27, 2021

Issue with run files for nginx start with system start #1372

Closed

ekohl reviewed Oct 27, 2021

View reviewed changes

spec/classes/nginx_spec.rb Show resolved Hide resolved

b4ldr force-pushed the debian_rundir branch from 691a0e3 to aef034a Compare October 27, 2021 13:38

b4ldr added a commit that referenced this pull request Oct 28, 2021

(1372) Drop run_dir and make client_body_temp_path/proxy_temp_path op…

77adeb1

…tional This is the same as #1478 however changes the spec tests to check for is.expect.not_to instead of removing Fixes: #1372

b4ldr added a commit that referenced this pull request Oct 28, 2021

(1372) Drop run_dir and make client_body_temp_path/proxy_temp_path op…

3f39dee

…tional This is the same as #1478 however changes the spec tests to check for is.expect.not_to instead of removing Fixes: #1372

b4ldr mentioned this pull request Oct 28, 2021

(1372) Drop run_dir and make client_body_temp_path/proxy_temp_path optional #1479

Closed

b4ldr added a commit that referenced this pull request Oct 28, 2021

(1372) Drop run_dir and make client_body_temp_path/proxy_temp_path op…

73a41d7

…tional This is the same as #1478 however changes the spec tests to check for is.expect.not_to instead of removing Fixes: #1372

ekohl reviewed Oct 28, 2021

View reviewed changes

ekohl added the backwards-incompatible label Oct 28, 2021

ekohl approved these changes Oct 28, 2021

View reviewed changes

b4ldr commented Oct 28, 2021

View reviewed changes

kenyon approved these changes Oct 29, 2021

View reviewed changes

ekohl merged commit f52fb7c into master Dec 6, 2021

ekohl deleted the debian_rundir branch December 6, 2021 11:22

smortex changed the title ~~(1372) Drop run_dir and make client_body_temp_path/proxy_temp_path optional~~ Drop run_dir and make client_body_temp_path/proxy_temp_path optional Aug 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop run_dir and make client_body_temp_path/proxy_temp_path optional #1478

Drop run_dir and make client_body_temp_path/proxy_temp_path optional #1478

b4ldr commented Oct 27, 2021

puppet-community-rangefinder bot commented Oct 27, 2021

ekohl Oct 28, 2021

b4ldr Oct 28, 2021

ekohl commented Oct 28, 2021

b4ldr commented Oct 28, 2021

b4ldr Oct 28, 2021

b4ldr Oct 28, 2021

b4ldr Oct 28, 2021

ekohl Oct 28, 2021

kenyon left a comment

b4ldr commented Nov 24, 2021

ashish1099 commented Dec 6, 2021

Drop run_dir and make client_body_temp_path/proxy_temp_path optional #1478

Drop run_dir and make client_body_temp_path/proxy_temp_path optional #1478

Conversation

b4ldr commented Oct 27, 2021

puppet-community-rangefinder bot commented Oct 27, 2021

nginx::config is a class

nginx is a class

nginx::params is a class

This module is declared in 9 of 578 indexed public Puppetfiles.

ekohl Oct 28, 2021

Choose a reason for hiding this comment

b4ldr Oct 28, 2021

Choose a reason for hiding this comment

ekohl commented Oct 28, 2021

b4ldr commented Oct 28, 2021

b4ldr Oct 28, 2021

Choose a reason for hiding this comment

b4ldr Oct 28, 2021

Choose a reason for hiding this comment

b4ldr Oct 28, 2021

Choose a reason for hiding this comment

ekohl Oct 28, 2021

Choose a reason for hiding this comment

kenyon left a comment

Choose a reason for hiding this comment

b4ldr commented Nov 24, 2021

ashish1099 commented Dec 6, 2021

This module is declared in 9 of 578 indexed public `Puppetfiles`.