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

[php] core/php: Updated to run PHP-FPM as a service #1146

Closed
wants to merge 3 commits into from

Conversation

baggerspion
Copy link
Contributor

Signed-off-by: Paul Adams paul@baggerspion.net

This update to the core/php package allows the user to optionally run php-fpm as a service to be bound to, and not simply as a library for import. This should not break any existing package that is based upon core/php, but open the door to building composite packages for PHP applications that deploy both nginx/apache + php-fpm. This is part of my overall mission to improve core/drupal.

@baggerspion baggerspion requested a review from a team as a code owner January 22, 2018 18:18
@thesentinels
Copy link
Contributor

Thanks for the pull request! Here is what will happen next:

  1. Your PR will be reviewed by the maintainers
  2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

Signed-off-by: Paul Adams <paul@baggerspion.net>
@eeyun eeyun changed the title core/php: Updated to run PHP-FPM as a service [php] core/php: Updated to run PHP-FPM as a service Jan 26, 2018
Copy link

@lancewf lancewf left a comment

Choose a reason for hiding this comment

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

Thank you for adding this. I was planning on adding this same service but as a separate package like this one here.
https://github.com/lancewf/habitat-plans/tree/master/php-fpm/.

The way you are doing it here with updating the php plan seems like a better option.

We need this update to also fix the core/wordpress library that currently does not work, because it tries to spawn the php service from the hook/init (https://github.com/habitat-sh/core-plans/blob/master/wordpress/hooks/init#L25).

I have a small example of a LEMP that is using this PR’s code for the PHP service.
https://github.com/lancewf/habitat_lemp_dev_environment

Please try to get this merged in. I have 3 more project I would like to start packaging with this service. Two of them are with wordpress and I would love to get a good example up.

php/default.toml Outdated
@@ -0,0 +1,10 @@
listen = "0.0.0.0"
Copy link

Choose a reason for hiding this comment

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

Should we default this to "127.0.0.1"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No strong opinion, so happy to change this.

php/plan.sh Outdated
)
pkg_bin_dirs=(bin sbin)
pkg_lib_dirs=(lib)
pkg_include_dirs=(include)
pkg_interpreters=(bin/php)
pkg_exports=([port]=port)
Copy link

Choose a reason for hiding this comment

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

Can we add the 'listen' parameter also here. If the user is binding to localhost, checking the listen parameter is the only way to know that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Easily added.

Signed-off-by: Paul Adams <paul@baggerspion.net>
Copy link

@afiune afiune left a comment

Choose a reason for hiding this comment

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

tenor-155273884

php/plan.sh Outdated
)
pkg_bin_dirs=(bin sbin)
pkg_lib_dirs=(lib)
pkg_include_dirs=(include)
pkg_interpreters=(bin/php)
pkg_exports=(
[port]=port
[listen]=listen
Copy link

Choose a reason for hiding this comment

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

What do you think about replacing the 'listen' export for a 'local_only' boolean export? The problem I am trying to solve is when the service is bound to localhost only. In that case the dependent service cannot connect with the IP address ('bind.php.first.sys.ip') it has to use 127.0.0.1. I have another PR here where I am trying to add the 'local_only' flag for mysql.

Signed-off-by: Paul Adams <paul@baggerspion.net>
@nellshamrell
Copy link
Contributor

@therealpadams ty so much for this contribution! Initially, the code looks fine and super useful. However, because this is a change in behavior, I would like to test this. Do you have any sample apps you could point me to that deploy both nginx/apache + php-fpm so I can test this?

@rsertelon
Copy link
Contributor

@baggerspion
Copy link
Contributor Author

I really want to make a composite plan for core/drupal. Let me give you a build of that using my baggerspion origin and you can then see this package in action. The package that @rsertelon linked is not built using this version of this plan. Eta sometime this week.

@lancewf
Copy link

lancewf commented Mar 27, 2018

@nellshamrell I have an example application with nginx/mysql/php here that uses these php changes published to my origin (lancewf/php).
https://github.com/lancewf/habitat_lemp_dev_environment

Also, below is a real nginx/mysql/php application that is using the same lancewf/php package.
https://github.com/lancewf/Marine-Mammal-Stranding-Network-Server

To test them out, enter the studio from the root directory and run the 'start' function.

@rsertelon rsertelon self-assigned this Apr 4, 2018
@nellshamrell
Copy link
Contributor

TY for the testing tips! Testing this now.

@nellshamrell
Copy link
Contributor

nellshamrell commented Apr 12, 2018

@therealpadams I'm having some difficulty getting this tested. I first tried to create a plan similar to core/drupal or core/wordpress that used both core/nginx and the current core/php, which turned out to be an absolute nightmare (I can definitely see the reason for this PR and why people are so enthusiastic about it!).

This afternoon I took a different approach and attempted to create a new php site based on a build of this plan, a server plan based on core/nginx, and combined the two. I haven't quite got them binding yet, but if you are interested all of my current code is here.

Could you give me some guidance in how to use/test this? Additionally, since this is now a service package, rather than a binary package, could you be sure to update the README with the same information (using this template)?

This seems tremendously useful to our community, and I look forward to getting this tested, updated, and out into the world!

@baggerspion
Copy link
Contributor Author

Hi @nellshamrell, "yes" to all of the above. I'm a little swamped at work just now, so it probably next week before I can get to this. I think the best thing I can do is also submit a PR for core/drupal that makes use of this. Then it should be easier to see how everything glues together.
With that "example" out of the way, separately I will tackle improved docs within this PR.

@nellshamrell
Copy link
Contributor

Sounds great! TY!

@rsertelon
Copy link
Contributor

@therealpadams Gentle ping ;)

@themightychris
Copy link
Contributor

I have been working on something similar, and have a pretty thorough battery of code and practice working for testing/verifying PHP-FPM. A Caddy server spawned in your .studiorc is a great quick way to test it. And the the cgi-fcgi binary I'm providing in #1527 is a big help too. Unfortunately I can't post the project I'm piloting it on wholesale yet so I'll have to smuggle things out gradually for now

I recommend adding some additional config, but it would probably be safest to make at least the status path be off by default:

[path]
ping = "/.well-known/php-fpm/ping"
status = "/.well-known/php-fpm/status"
{{#if cfg.path.ping~}}
    ping.path                                      = {{ cfg.path.ping }}
{{~/if}}

{{#if cfg.path.status~}}
    pm.status_path                                 = {{ cfg.path.status }}
{{~/if}}

With that and the libfcgi package added as a pkg_dep, the php-fpm service could then come with a health_check script like this:

#!/bin/bash -e

{{#if cfg.path.ping}}
    SCRIPT_NAME="{{ cfg.path.ping }}" \
    SCRIPT_FILENAME="{{ cfg.path.ping }}" \
    REQUEST_METHOD="GET" \
        {{pkgPathFor "core/libfcgi"}}/bin/cgi-fcgi \
            -bind \
            -connect {{ sys.ip }}:{{ cfg.network.port }} \
        | grep pong
{{/if}}

I haven't written it yet but I think the health_check should also come with a test of the status page when enabled and output some of its key metrics like core/mysql's health_check

@themightychris
Copy link
Contributor

Here's some useful snippets for testing with Caddy in a studio:

/src/Caddyfile:

localhost:2015 
 
errors errors.log 
 
fastcgi / 127.0.0.1:9000 { 
    root /src 
    index phpinfo.php 
    env SITE_ROOT /src 
    env SCRIPT_FILENAME /src/phpinfo.php 
} 

/src/phpinfo.php:

<?php

phpinfo();

/src/.studiorc:

echo
echo "--> Installing useful packages for development..."
hab pkg install core/caddy


echo
echo "--> Setting up development commands..."

echo "    * Use 'start-caddy' to launch a Caddy web server to expose the PHP-FPM application at http://localhost:2015"
start-caddy() { 
    hab pkg exec core/caddy caddy & 
}


echo

@baggerspion
Copy link
Contributor Author

baggerspion commented May 22, 2018

Just a quick poke to show that I am still alive and working on this! Especially during the Chefconf hackday. So here is my TODO list for ChefConf:

  • Add libfcgi and create healthcheck
  • PRovide (at least) a baggerspion/drupal based upon this package as an example of it in use

@nellshamrell
Copy link
Contributor

@therealpadams - have you had a chance to look further into "Add libfcgi and create healthcheck"?

@nellshamrell
Copy link
Contributor

@therealpadams - thank you so much again for this contribution. Because it's starting to drift from master pretty significantly, I'm going to go ahead and close this for the time being, but please feel free to re-open it at any time when you have some time to work on it.

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.

None yet

7 participants