-
-
Notifications
You must be signed in to change notification settings - Fork 268
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
base: master
Are you sure you want to change the base?
Conversation
|
b16ebe1
to
85f0ff8
Compare
85f0ff8
to
d8759ee
Compare
@bastelfreak any chance of a review on this one? |
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? |
@bastelfreak @rayward makes sense to me, please add option to manage rlimit_core set automatically if core dumps are set. |
@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 -%> |
There was a problem hiding this comment.
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') |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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?
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 |
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 ifrlimit_core
is set because of the following change to PHP:php/php-src@276d19f#diff-10cc4db817271640849c40a7fc1f0569