add maxrequestworkers parameter for mpm_worker module #2331

trefzer · 2022-10-12T07:46:33Z

No description provided.

CLAassistant · 2022-10-12T07:46:38Z

All committers have signed the CLA.

puppet-community-rangefinder · 2022-10-12T07:46:40Z

apache::mod::worker is a class

that may have no external impact to Forge modules.

This module is declared in 174 of 579 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.

trefzer · 2022-10-12T08:41:54Z

check seems to fail provisioning SLES-15 which ist not a problem of this commit.

ekohl

In other places (prefork/event mpm) we prefer MaxRequestWorkers and don't output MaxClients if it's set. Could you implement the same here?

trefzer · 2022-10-30T14:49:17Z

check seems to fail provisioning RedHat 7 and 9, not the change request.

david22swan · 2022-10-31T14:20:18Z

@ekohl Are you happy with the changes made?

ekohl

The changes look good, but spec/classes/mod/worker_spec.rb should be updated to pass maxrequestworkers and verify it outputs MaxRequestWorkers and not MaxClients.

ekohl · 2022-11-07T13:41:46Z

spec/classes/mod/worker_spec.rb

@@ -126,7 +126,8 @@
      it { is_expected.to contain_file('/etc/httpd/conf.modules.d/worker.conf').with(content: %r{^<IfModule mpm_worker_module>$}) }
      it { is_expected.to contain_file('/etc/httpd/conf.modules.d/worker.conf').with(content: %r{^\s+ServerLimit\s+10$}) }
      it { is_expected.to contain_file('/etc/httpd/conf.modules.d/worker.conf').with(content: %r{^\s+StartServers\s+11$}) }
-      it { is_expected.to contain_file('/etc/httpd/conf.modules.d/worker.conf').with(content: %r{^\s+MaxClients\s+12$}) }
+      it { is_expected.not_to contain_file('/etc/httpd/conf.modules.d/worker.conf').with(content: %r{^\s+MaxClients}) }


It's better to expect a file to be there without certain content. The big reason is that your example will also pass if the file isn't present in the catalog at all.

Suggested change

it { is_expected.not_to contain_file('/etc/httpd/conf.modules.d/worker.conf').with(content: %r{^\s+MaxClients}) }

it { is_expected.to contain_file('/etc/httpd/conf.modules.d/worker.conf').without(content: %r{MaxClients}) }

On a side note: this whole file should be rewritten IMHO to have a single it statement with one long content block that has the full generated template, but that's not part of this PR.

ekohl · 2022-11-07T13:42:01Z

manifests/mod/worker.pp

@@ -8,6 +8,7 @@
 #   The max number of simultaneous requests that will be served.
 #   This is the old name and is still supported. The new name is
 #   MaxRequestWorkers as of 2.3.13.
+#   If maxreuestworkers is set, this value is ignored.


Suggested change

# If maxreuestworkers is set, this value is ignored.

# If maxrequestworkers is set, this value is ignored.

david22swan

Little concerned with maxclients no longer having a test where it is explicitly set, but given it has a default value covered in the other tests this should be fine.

LGTM

@ekohl Are you happy that the changes that you requested have been implemented?

ekohl

One tiny comment, but generally 👍

ekohl · 2022-11-07T14:58:04Z

manifests/mod/worker.pp

@@ -40,6 +41,10 @@
 # @param apache_version
 #   Used to verify that the Apache version you have requested is compatible with the module.
 #
+# @param maxrequestworkers
+#   Maximum number of connections that will be processed simultaneously


I think this will show up slightly better in the rendered strings documentation

Suggested change

# Maximum number of connections that will be processed simultaneously

# Maximum number of connections that will be processed simultaneously.

david22swan · 2022-11-14T11:28:18Z

Gonna go ahead and merge.
@trefzer Thanks for putting in the work.
@ekohl Thanks for helping to review

trefzer requested a review from a team as a code owner October 12, 2022 07:46

ia-content added the community label Oct 12, 2022

ekohl requested changes Oct 26, 2022

View reviewed changes

david22swan added the feature label Oct 31, 2022

ekohl reviewed Oct 31, 2022

View reviewed changes

ekohl reviewed Nov 7, 2022

View reviewed changes

trefzer added 4 commits November 7, 2022 14:50

add maxrequestworkers parameter for mpm_worker module

102d459

add logic to ignore maxclients if maxrequestworkers is set

a6a70b4

add spec test for MaxRequestWorkers

246617f

fix typo

58e3a35

david22swan approved these changes Nov 7, 2022

View reviewed changes

ekohl approved these changes Nov 7, 2022

View reviewed changes

david22swan merged commit 0cb1c07 into puppetlabs:main Nov 14, 2022

ekohl mentioned this pull request Dec 14, 2022

Drop Apache 2.2 support #2329

Merged

trefzer deleted the fix_maxrequestworkers branch April 6, 2024 09:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

add maxrequestworkers parameter for mpm_worker module #2331

add maxrequestworkers parameter for mpm_worker module #2331

Uh oh!

trefzer commented Oct 12, 2022

Uh oh!

CLAassistant commented Oct 12, 2022 •

edited

Loading

Uh oh!

puppet-community-rangefinder bot commented Oct 12, 2022

Uh oh!

trefzer commented Oct 12, 2022

Uh oh!

ekohl left a comment

Uh oh!

trefzer commented Oct 30, 2022

Uh oh!

david22swan commented Oct 31, 2022

Uh oh!

ekohl left a comment

Uh oh!

ekohl Nov 7, 2022

Uh oh!

trefzer Nov 7, 2022

Uh oh!

ekohl Nov 7, 2022

Uh oh!

trefzer Nov 7, 2022

Uh oh!

david22swan left a comment

Uh oh!

ekohl left a comment

Uh oh!

ekohl Nov 7, 2022

Uh oh!

david22swan commented Nov 14, 2022

Uh oh!

Uh oh!

	it { is_expected.not_to contain_file('/etc/httpd/conf.modules.d/worker.conf').with(content: %r{^\s+MaxClients}) }
	it { is_expected.to contain_file('/etc/httpd/conf.modules.d/worker.conf').without(content: %r{MaxClients}) }

	# If maxreuestworkers is set, this value is ignored.
	# If maxrequestworkers is set, this value is ignored.

	# Maximum number of connections that will be processed simultaneously
	# Maximum number of connections that will be processed simultaneously.

add maxrequestworkers parameter for mpm_worker module #2331

add maxrequestworkers parameter for mpm_worker module #2331

Uh oh!

Conversation

trefzer commented Oct 12, 2022

Uh oh!

CLAassistant commented Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

puppet-community-rangefinder bot commented Oct 12, 2022

apache::mod::worker is a class

This module is declared in 174 of 579 indexed public Puppetfiles.

Uh oh!

trefzer commented Oct 12, 2022

Uh oh!

ekohl left a comment

Choose a reason for hiding this comment

Uh oh!

trefzer commented Oct 30, 2022

Uh oh!

david22swan commented Oct 31, 2022

Uh oh!

ekohl left a comment

Choose a reason for hiding this comment

Uh oh!

ekohl Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

trefzer Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

ekohl Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

trefzer Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

david22swan left a comment

Choose a reason for hiding this comment

Uh oh!

ekohl left a comment

Choose a reason for hiding this comment

Uh oh!

ekohl Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

david22swan commented Nov 14, 2022

Uh oh!

Uh oh!

CLAassistant commented Oct 12, 2022 •

edited

Loading

This module is declared in 174 of 579 indexed public `Puppetfiles`.