Skip to content

Issue #140: Added a memory-limit setting for the CLI #347

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Issue #140: Added a memory-limit setting for the CLI #347

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a56f185
Issue #140: Enable setting for CLI memory limit.
Oct 8, 2021
f72f19f
Issue #140: Removed extra blank line.
Oct 8, 2021
03e012e
Issue #140: Fixed Lint warnings and errors.
Oct 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions defaults/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ php_enable_apc: true
php_apc_shm_size: "96M"
php_apc_enable_cli: "0"

# Manage PHP CLI memory limit.
php_set_cli_memory_limit: False
php_cli_memory_limit: "512M"

# If this is set to false, none of the following options will have any effect.
# Any and all changes to /etc/php.ini will be your responsibility.
php_use_managed_ini: true
Expand Down
15 changes: 15 additions & 0 deletions tasks/configure-cli.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
---

- name: Set PHP CLI memory limit.
block:
- name: Ensure PHP CLI config directory exists.
file:
dest: "/etc/php/{{ php_default_version_debian }}/cli/conf.d/"
Copy link
Owner

Choose a reason for hiding this comment

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

Do we have this set up for RedHat-derivatives too?

Copy link
Author

Choose a reason for hiding this comment

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

@geerlingguy No, sorry. We've standardized on Ubuntu LTS over here; I don't typically work with non-Debian-based OSes (and wouldn't recommend them, but that's another story). What I'd recommend doing, which is what we do for Aegir, is get support for each alternative component as it comes in. So whenever this is good to go, merge it, but leave the issue open for someone else to come along and work on the Red Hat support (and then close it once that gets in). Would that work for you?

I'll take a look at the Lint problems and see if I can get those fixed.

Copy link
Author

Choose a reason for hiding this comment

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

Tests were passing for Red Hat though. So maybe it's all good?

Copy link
Owner

Choose a reason for hiding this comment

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

I'd still rather not merge it if it only works on Debian.

Copy link
Author

Choose a reason for hiding this comment

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

@geerlingguy But if tests are passing, doesn't this prove that it also works on Red Hat, or whatever?

Copy link

Choose a reason for hiding this comment

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

well

  • you're adding a feature that'll be only compatible with 1 out of X OS options
  • tests are not that extensive, you could make a test that shows it doesn't work on RHEL distros and fix it accordingly

Copy link
Owner

Choose a reason for hiding this comment

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

The role still works, but if I add a feature that has a default variable that only applies to one distro (or a few) of any that are supported, then I start having to remember (for myself) which features of the role work on only one distro but not the other, and I have to start adding complexity to the documentation for features which shouldn't need to be distro-specific.

I prefer for my own roles to have defaults that work across any distro the role supports, but I'm always happy for people to fork my roles and modify them according to their own needs. I typically only add features that work on all distros unless it's something extremely specific to one OS (e.g. apt PPA url settings).

Copy link
Author

Choose a reason for hiding this comment

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

Fair points. To benefit everyone, however, can we tag this as Planned and then let others push to the PR to complete it eventually, when someone else gets to it? GitLab allows this, but I don't work on GitHub enough to know if that's possible here.

Unless someone is able to provide funding (please contact us if interested), I have no plans to add Red Hat support here, but it would be a shame to simply throw away the work I did.

Copy link
Owner

Choose a reason for hiding this comment

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

Sure, I have no problem with that—it's useful functionality, and if someone can come along and make it work on RHEL/RockyLinux as well, I'd be happy to merge.

state: directory
- name: Set PHP CLI memory limit.
template:
src: memory_limit.ini.j2
dest: "/etc/php/{{ php_default_version_debian }}/cli/conf.d/20-memory_limit.ini"
become: yes
when: php_set_cli_memory_limit | bool

1 change: 1 addition & 0 deletions tasks/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,3 +75,4 @@
- include_tasks: configure-apcu.yml
- include_tasks: configure-opcache.yml
- include_tasks: configure-fpm.yml
- include_tasks: configure-cli.yml
5 changes: 5 additions & 0 deletions templates/memory_limit.ini.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
; {{ ansible_managed }}

; configuration for increasing PHP CLI memory limit
; priority=20
memory_limit = {{ php_cli_memory_limit }}