[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#28769] [PATCH] gnu: services: Add php-fpm.
|
From: |
Christopher Baines |
|
Subject: |
[bug#28769] [PATCH] gnu: services: Add php-fpm. |
|
Date: |
Fri, 13 Oct 2017 22:37:29 +0100 |
On Mon, 9 Oct 2017 23:54:24 +0200
nee <address@hidden> wrote:
> Subject: [PATCH 2/2] gnu: services: Add php-fpm.
Hey, I've never used php-fpm, but I'll have a go at reviewing this :)
> * gnu/services/web.scm (<php-fpm-configuration>,
> <php-fpm-process-manager-configuration>): New record types.
> (php-fpm-configuration?,
> php-fpm-process-manager-configuration?,
> php-fpm-service-type,
> nginx-php-location): New procedures.
> * doc/guix.texi (Web-Services): document php-fpm service.
Very minor, but I'd suggest "(Web Services): Document the php-fpm
service." here.
> ---
> doc/guix.texi | 93 +++++++++++++++++++++++++++
> gnu/services/web.scm | 173
> ++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed,
> 264 insertions(+), 2 deletions(-)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index f0a59a6b4..ed4336f64 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -14529,6 +14529,99 @@ capability also has to be configured on the
> front-end as well. @end deftp
>
>
> address@hidden php-fpm
> +PHP-FPM (FastCGI Process Manager) is an alternative PHP FastCGI
> implementation
> +with some additional features useful for sites of any size.
If these additional features are worth mentioning, it would be best to
be specific as to what they are, and what benefit they provide.
> address@hidden {Scheme Variable} php-fpm-service-type
> +A Service type for @code{php-fpm}.
> address@hidden defvr
> +
...
> address@hidden {Data type} php-fpm-process-manager-configuration
> +Data Type for php-fpm worker process limits.
> address@hidden @asis
> address@hidden @code {type} (default: @code{"dynamic"})
> address@hidden @asis
> address@hidden @code{"dynamic"}
> +Spare worker processes are kept around based on the set
> @code{php-fpm-process-manager-configuration} limits.
> address@hidden @code{"static"}
> +A static number of worker processes is created.
> address@hidden @code{"ondemand"}
> +Worker processes are only created on demand.
> address@hidden table
> address@hidden @code {max-children} (default: @code{5})
> +Maximum of worker processes. Applies when the type is @code{"static"},
> @code{"dynamic"}, or @code{"ondemand"}.
> address@hidden @code {start-servers} (default: @code{2})
> +How many worker processes should be started on start-up. Only applies when
> type is @code{"dynamic"}.
> address@hidden @code {min-spare-servers} (default: @code{1})
> +How many spare worker processes should be kept around at minimum. Only
> applies when type is @code{"dynamic"}.
> address@hidden @code {max-spare-servers} (default: @code{3})
> +How many spare worker processes should be kept around at maximum. Only
> applies when type is @code{"dynamic"}.
> address@hidden @code {process-idle-timeout} (default: @code{10})
> +The time in seconds after which a process with no requests is killed. Only
> applies when type is @code{"ondemand"}.
> address@hidden table
> address@hidden deftp
Reading this makes me think that maybe having record types per process
manager types might be useful, e.g.
<php-fpm-dynamic-process-manager-configuration>
<php-fpm-static-process-manager-configuration>
<php-fpm-process-manager-configuration>
> address@hidden {Scheme Variable} nginx-php-fpm-location
> +A helper function to quickly add php to an
> @code{nginx-server-configuration}. address@hidden defvr
> +
> +A simple services setup for nginx with php can look like this:
> address@hidden
> +(services (cons* (dhcp-client-service)
> + (service php-fpm-service-type
> + (php-fpm-configuration))
The default-value for the service type means that you can omit the
(php-fpm-configuration) here, as (service php-fpm-service-type) should
work.
> + (service nginx-service-type
> + (nginx-server-configuration
> + (server-name '("example.com"))
> + (root "/srv/http/")
> + (locations
> + (list (nginx-php-location)))
> + (https-port #f)
> + (ssl-certificate #f)
> + (ssl-certificate-key #f)))
> + %base-services))
> address@hidden example
> +
> +
> @node DNS Services
> @subsubsection DNS Services
> @cindex DNS (domain name system)
> diff --git a/gnu/services/web.scm b/gnu/services/web.scm
> index 9d713003c..fd63b15bb 100644
> --- a/gnu/services/web.scm
> +++ b/gnu/services/web.scm
> @@ -26,8 +26,11 @@
> #:use-module (gnu system shadow)
> #:use-module (gnu packages admin)
> #:use-module (gnu packages web)
> + #:use-module (gnu packages php)
> #:use-module (guix records)
> #:use-module (guix gexp)
> + #:use-module ((guix utils) #:select (version-major))
> + #:use-module ((guix packages) #:select (package-version))
> #:use-module (srfi srfi-1)
> #:use-module (ice-9 match)
> #:export (<nginx-configuration>
> @@ -76,7 +79,14 @@
>
> fcgiwrap-configuration
> fcgiwrap-configuration?
> - fcgiwrap-service-type))
> + fcgiwrap-service-type
> +
> + php-fpm-configuration
> + php-fpm-configuration?
> + php-fpm-process-manager-configuration
> + php-fpm-process-manager-configuration?
> + php-fpm-service-type
> + nginx-php-location))
When using other Guix services, I've run in to problems when field
accessors and record types were not exported. The biggest cost I can
think of is the lines of code, but I'd still suggest to export
everything by default here.
> ;;; Commentary:
> ;;;
> @@ -256,10 +266,12 @@ of index files."
> "events {}\n")))
>
> (define %nginx-accounts
> - (list (user-group (name "nginx") (system? #t))
> + (list (user-group (name "www-data") (system? #t))
> + (user-group (name "nginx") (system? #t))
> (user-account
> (name "nginx")
> (group "nginx")
> + (supplementary-groups '("www-data"))
> (system? #t)
> (comment "nginx server user")
> (home-directory "/var/empty")
Pulling in your comment about the accounts...
> About the accounts:
> Nginx needs write access to the unix socket of php-fpm. I didn't want
> to set nginx as default user for php-fpm in case we add other
> webservers, so I added the nginx to the newly created www-data group.
This sounds like it would work, however, the defaults for the
socket-user and socket-group of the php-fpm-configuration are currently
"nginx", which seems to disagree with what you say above?
Also, the idea that came to my mind for this is to add php-fpm as a
supplementary group for the nginx user. In my mind, this seems the
neatest way of giving nginx access to the socket. What do you think?
I suggest this, as I'm not sure the name of the www-data group does a
good job of describing that it's controlling access to a socket. Also,
giving nginx explicit access to things owned by the php-fpm group could
be more secure than using a more generic group, especially as other
services that might need access to the socket, could end up getting it
because they need access to other things using the www-data group.
If the above sounds like a good idea, I think it could be implemented
by adding a configurable list of supplementary groups to the
nginx-service-type. Then maybe even adding support for configuring this
via the service extensions mechanism, then having the
php-fpm-service-type extending the nginx-service-type...
The above is definitely to complicated to do all in one go, especially
since the nginx-service-type doesn't support anything more complicated
than adding server blocks through extension at the moment.
> @@ -385,3 +397,160 @@ of index files."
> (service-extension account-service-type
> fcgiwrap-accounts)))
> (default-value (fcgiwrap-configuration))))
> +
> +(define-record-type* <php-fpm-configuration> php-fpm-configuration
> + make-php-fpm-configuration
> + php-fpm-configuration?
> + (php php-fpm-configuration-php ;<package>
> + (default php))
> + (socket php-fpm-configuration-socket
> + (default (string-append "/var/run/php"
> + (version-major
> (package-version php))
> + "-fpm.sock")))
> + (user php-fpm-configuration-user
> + (default "php-fpm"))
> + (group php-fpm-configuration-group
> + (default "php-fpm"))
> + (socket-user php-fpm-configuration-socket-user
> + (default "nginx"))
> + (socket-group php-fpm-configuration-socket-group
> + (default "nginx"))
As above, I'm not sure the use of nginx here matches the
description you gave...?
> + (process-manager php-fpm-configuration-process-manager
> + (default (php-fpm-process-manager-configuration)))
> + (display-errors php-fpm-configuration-display-errors
> + (default #f))
> + (workers-logfile php-fpm-configuration-workers-logfile
> + (default (string-append "/var/log/php"
> + (version-major
> (package-version php))
> + "-fpm.log")))
I'm not sure the php is adding much to the log file name, but then
again I've never used php... what do you think?
> + (file php-fpm-configuration-file ;#f | file-like
> + (default #f)))
> +
> +(define-record-type* <php-fpm-process-manager-configuration>
> php-fpm-process-manager-configuration
> + make-php-fpm-process-manager-configuration
> + php-fpm-process-manager-configuration?
> + (type php-fpm-process-manager-configuration-type
> + (default "dynamic"))
> + (max-children
> php-fpm-process-manager-configuration-max-children
> + (default 5))
> + (start-servers
> php-fpm-process-manager-configuration-start-servers
> + (default 2))
> + (min-spare-servers
> php-fpm-process-manager-configuration-min-spare-servers
> + (default 1))
> + (max-spare-servers
> php-fpm-process-manager-configuration-max-spare-servers
> + (default 3))
> + (process-idle-timeout
> php-fpm-process-manager-configuration-process-idle-timeout
> + (default 10)))
> +
> +(define php-fpm-accounts
> + (match-lambda
> + (($ <php-fpm-configuration> php socket user group socket-user
> socket-group _ _ _ _)
> + (filter identity
> + (list
> + (user-group (name "www-data") (system? #t))
> + (and (equal? group "php-fpm")
> + (user-group
> + (name "php-fpm")
> + (system? #t)))
> + (and (equal? user "php-fpm")
> + (user-account
> + (name "php-fpm")
> + (group group)
> + (supplementary-groups '("www-data"))
> + (system? #t)
> + (comment "web services group")
> + (home-directory "/var/empty")
> + (shell (file-append shadow
> "/sbin/nologin"))))))))) +
As you can specify the user and group in the <php-fpm-configuration>, I
think it might be better to just use the user and group names from
there, and always setup a user and group with those names, I'm guessing
that this will be what the average user would expect to happen.
> +(define (default-php-fpm-config socket user group socket-user
> socket-group
> + pm display-errors workers-logfile)
> + (match
> + pm
> + (($ <php-fpm-process-manager-configuration> pm.type
> + pm.max-children
> + pm.start-servers
> + pm.min-spare-servers
> + pm.max-spare-servers
> +
> pm.process-idle-timeout)
> + (apply mixed-text-file "php-fpm.conf"
> + "[global]\n"
> + "[www]\n"
> + "user =" user "\n"
> + "group =" group "\n"
> + "listen =" socket "\n"
> + "listen.owner =" socket-user "\n"
> + "listen.group =" socket-group "\n"
> +
> + "pm =" pm.type "\n"
> + "pm.max_children =" (number->string pm.max-children) "\n"
> + "pm.start_servers =" (number->string pm.start-servers)
> "\n"
> + "pm.min_spare_servers =" (number->string
> pm.min-spare-servers) "\n"
> + "pm.max_spare_servers =" (number->string
> pm.max-spare-servers) "\n"
> + "pm.process_idle_timeout =" (number->string
> pm.process-idle-timeout) "s\n" +
> + "php_flag[display_errors] = " (if display-errors "on"
> "off") "\n" +
> + (if workers-logfile
> + (list "catch_workers_output = yes\n"
> + "php_admin_value[error_log] =" workers-logfile
> "\n"
> + "php_admin_flag[log_errors] = on\n")
> + (list "catch_workers_output = no\n"))))))
> +
> +(define php-fpm-shepherd-service
> + (match-lambda
> + (($ <php-fpm-configuration> php socket user group socket-user
> socket-group
> + pm display-errors workers-logfile
> file)
> + (list (shepherd-service
> + (provision '(php-fpm))
> + (documentation "Run the php-fpm daemon.")
> + (requirement '(networking))
> + (start #~(make-forkexec-constructor
> + '(#$(file-append php "/sbin/php-fpm")
> + "--nodaemonize" "-p" "/var" "--fpm-config"
> + #$(or file
> + (default-php-fpm-config socket user
> group
> + socket-user socket-group pm
> display-errors
> + workers-logfile)))))
> + (stop #~(make-kill-destructor)))))))
As php-fpm supports using a pid file, I'd recommend configuring both
php-fpm and shepherd to use this by default. It means that the shepherd
service will wait until php-fpm creates the PID file before starting
services that say they require it, which can prevent some issues.
> +(define php-fpm-activation
> + (match-lambda
> + (($ <php-fpm-configuration> _ _ user _ _ _ _ _ workers-logfile _)
> + #~(begin
> + (use-modules (guix build utils))
> + (let ((user (getpwnam #$user))
> + (touch (lambda (file-name)
> + (call-with-output-file file-name (const
> #t)))))
> + ;; prepare writable logfile
> + (when #$workers-logfile
> + (when (not (file-exists? #$workers-logfile))
> + (touch #$workers-logfile))
> + (chown #$workers-logfile (passwd:uid user) (passwd:gid
> user))
> + (chmod #$workers-logfile #o660)))))))
> +
> +
> +(define php-fpm-service-type
> + (service-type (name 'php-fpm)
> + (extensions
> + (list (service-extension shepherd-root-service-type
> + php-fpm-shepherd-service)
> + (service-extension activation-service-type
> + php-fpm-activation)
> + (service-extension account-service-type
> + php-fpm-accounts)))
> + (default-value (php-fpm-configuration))))
Filling in the description (a relatively new field on the service type)
would be a great addition here.
> +(define* (nginx-php-location
> + #:key
> + (nginx-package nginx)
> + (socket (string-append "/var/run/php"
> + (version-major (package-version
> php))
> + "-fpm.sock")))
> + "Return a nginx-location-configuration that makes nginx run .php
> files."
> + (nginx-location-configuration
> + (uri "~ \\.php$")
> + (body (list
> + "fastcgi_split_path_info ^(.+\\.php)(/.+)$;"
> + (string-append "fastcgi_pass unix:" socket ";")
> + "fastcgi_index index.php;"
> + (list "include " nginx-package
> "/share/nginx/conf/fastcgi.conf;")))))
This helper seems good, although I think putting the "include"
elsewhere would be better, and I recognise that this isn't possible
with the nginx service yet.
Overall, I think this is great. Excellent use of record types, gexps
and match expressions. I think it would be good to think more on the
accounts issue before merging, but that is all that comes to mind
currently.
Also, if you feel like it, as service test would be a great addition to
this patch. There is a test for nginx already in gnu/tests/web.scm, and
I think you could get most of the benefit by having a test for nginx
with php-fpm, as that would give you some coverage over the php-fpm
service, as well as the nginx configuration.
pgplUOUm8IAdx.pgp
Description: OpenPGP digital signature