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

Adding the option to set process.dumpable #511

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mhagstrand
Copy link

Pull Request (PR) description

Adding the option to set process.dumpable

This Pull Request (PR) fixes the following issues

If process.dumpable is disabled core dumps cannot be captured on segfaults even if rlimit_core is set because of the following change to PHP:

php/php-src@276d19f#diff-10cc4db817271640849c40a7fc1f0569

@mhagstrand
Copy link
Author

mhagstrand commented Mar 19, 2019

process.dumpable isn't added until php-7.0.29. Need to add a version check.

@mhagstrand mhagstrand force-pushed the AddProcessDumpableOption branch 2 times, most recently from b16ebe1 to 85f0ff8 Compare March 19, 2019 19:12
@rayward
Copy link

rayward commented Oct 12, 2020

@bastelfreak any chance of a review on this one?

@bastelfreak
Copy link
Member

hey @rayward. We're currently doing a rewrite of the module (check https://github.com/voxpupuli/puppet-php/projects/1 and #604 and #603). @SimonHoenscheid what's your opinion on this PR?

@SimonHoenscheid
Copy link
Member

@bastelfreak @rayward makes sense to me, please add option to manage rlimit_core set automatically if core dumps are set.

@mhagstrand
Copy link
Author

@SimonHoenscheid Sounds good, I will take care of that and update the PR.

; or group is differrent than the master process user. It allows to create process
; core dump and ptrace the process for the pool user.
; Default Value: no
<% if @process_dumpable -%>
Copy link
Member

Choose a reason for hiding this comment

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

whitespace typo?

@@ -168,6 +173,12 @@
warning('You must include the php base class before using any php defined resources')
}

if ($php::globals::php_version != undef) {
$php_version_major = regsubst($php::globals::php_version, '^(\d+)\.(\d+)$','\1')
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't the calculation of the major php version be done in the params or in a more global location?

@@ -144,6 +148,7 @@
$security_limit_extensions = undef,
$slowlog = "/var/log/php-fpm/${name}-slow.log",
$template = 'php/fpm/pool.conf.erb',
$process_dumpable = false,
Copy link
Member

Choose a reason for hiding this comment

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

just a thought: does it make sense, in the matter of maintainability, to have every possible setting as parameter in the class? wouldn't it be better having a "custom code" parameter where the users can add whatever code they like?

@vox-pupuli-tasks
Copy link

Dear @mhagstrand, thanks for the PR!

This is Vox Pupuli Tasks, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants