* [bug#29466] [PATCH] services: web: Add support for configuring the nginx server names hash.
@ 2017-11-27 8:23 Christopher Baines
2017-11-27 14:06 ` Ludovic Courtès
0 siblings, 1 reply; 7+ messages in thread
From: Christopher Baines @ 2017-11-27 8:23 UTC (permalink / raw)
To: 29466
The nginx service can fail to start if the server names hash bucket size is
too small, which can happen on some systems, and when using QEMU, depending on
the CPU.
* gnu/services/web.scm (<nginx-configuration>): Add
server-names-hash-bucket-size and server-names-hash-bucket-max-size.
(default-nginx-config): Add support for the new hash bucket size parameters.
(nginx-service, nginx-activation): Pass the new hash bucket size parameters
through to the default-nginx-config procedure.
* doc/guix.texi (Web Services): Document the new hash bucket size parameters.
---
doc/guix.texi | 7 +++++++
gnu/services/web.scm | 36 +++++++++++++++++++++++++++++++-----
2 files changed, 38 insertions(+), 5 deletions(-)
diff --git a/doc/guix.texi b/doc/guix.texi
index 2a6825682..d0a00bdcb 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -14874,6 +14874,13 @@ This can be useful if you have an existing configuration file, or it's
not possible to do what is required through the other parts of the
nginx-configuration record.
+@item @code{server-names-hash-bucket-size} (default: @code{#f})
+Bucket size for the server names hash tables, defaults to @code{#f} to
+use the size of the processors cache line.
+
+@item @code{server-names-hash-bucket-max-size} (default: @code{#f})
+Maximum bucket size for the server names hash tables.
+
@end table
@end deffn
diff --git a/gnu/services/web.scm b/gnu/services/web.scm
index 9d713003c..b9acee762 100644
--- a/gnu/services/web.scm
+++ b/gnu/services/web.scm
@@ -38,6 +38,8 @@
nginx-configuration-run-directory
nginx-configuration-server-blocks
nginx-configuration-upstream-blocks
+ nginx-configuration-server-names-hash-bucket-size
+ nginx-configuration-server-names-hash-bucket-max-size
nginx-configuration-file
<nginx-server-configuration>
@@ -141,6 +143,10 @@
(default '())) ;list of <nginx-server-configuration>
(upstream-blocks nginx-configuration-upstream-blocks
(default '())) ;list of <nginx-upstream-configuration>
+ (server-names-hash-bucket-size nginx-configuration-server-names-hash-bucket-size
+ (default #f))
+ (server-names-hash-bucket-max-size nginx-configuration-server-names-hash-bucket-max-size
+ (default #f))
(file nginx-configuration-file ;#f | string | file-like
(default #f)))
@@ -235,7 +241,9 @@ of index files."
(cons head out)))
(fold-right flatten1 '() lst))
-(define (default-nginx-config nginx log-directory run-directory server-list upstream-list)
+(define (default-nginx-config nginx log-directory run-directory server-list
+ upstream-list server-names-hash-bucket-size
+ server-names-hash-bucket-max-size)
(apply mixed-text-file "nginx.conf"
(flatten
"user nginx nginx;\n"
@@ -249,6 +257,18 @@ of index files."
" scgi_temp_path " run-directory "/scgi_temp;\n"
" access_log " log-directory "/access.log;\n"
" include " nginx "/share/nginx/conf/mime.types;\n"
+ (if server-names-hash-bucket-size
+ (string-append
+ " server_names_hash_bucket_size "
+ (number->string server-names-hash-bucket-size)
+ ";\n")
+ "")
+ (if server-names-hash-bucket-max-size
+ (string-append
+ " server_names_hash_bucket_max_size "
+ (number->string server-names-hash-bucket-max-size)
+ ";\n")
+ "")
"\n"
(map emit-nginx-upstream-config upstream-list)
(map emit-nginx-server-config server-list)
@@ -268,7 +288,8 @@ of index files."
(define nginx-activation
(match-lambda
(($ <nginx-configuration> nginx log-directory run-directory server-blocks
- upstream-blocks file)
+ upstream-blocks server-names-hash-bucket-size
+ server-names-hash-bucket-max-size file)
#~(begin
(use-modules (guix build utils))
@@ -289,13 +310,16 @@ of index files."
(system* (string-append #$nginx "/sbin/nginx")
"-c" #$(or file
(default-nginx-config nginx log-directory
- run-directory server-blocks upstream-blocks))
+ run-directory server-blocks upstream-blocks
+ server-names-hash-bucket-size
+ server-names-hash-bucket-max-size))
"-t")))))
(define nginx-shepherd-service
(match-lambda
(($ <nginx-configuration> nginx log-directory run-directory server-blocks
- upstream-blocks file)
+ upstream-blocks server-names-hash-bucket-size
+ server-names-hash-bucket-max-size file)
(let* ((nginx-binary (file-append nginx "/sbin/nginx"))
(nginx-action
(lambda args
@@ -304,7 +328,9 @@ of index files."
(system* #$nginx-binary "-c"
#$(or file
(default-nginx-config nginx log-directory
- run-directory server-blocks upstream-blocks))
+ run-directory server-blocks upstream-blocks
+ server-names-hash-bucket-size
+ server-names-hash-bucket-max-size))
#$@args))))))
;; TODO: Add 'reload' action.
--
2.14.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [bug#29466] [PATCH] services: web: Add support for configuring the nginx server names hash.
2017-11-27 8:23 [bug#29466] [PATCH] services: web: Add support for configuring the nginx server names hash Christopher Baines
@ 2017-11-27 14:06 ` Ludovic Courtès
2017-12-10 8:44 ` [bug#29466] [PATCH 1/2] services: web: Switch nginx related functions to use match-record Christopher Baines
2017-12-10 9:00 ` [bug#29466] [PATCH] services: web: Add support for configuring the nginx server names hash Christopher Baines
0 siblings, 2 replies; 7+ messages in thread
From: Ludovic Courtès @ 2017-11-27 14:06 UTC (permalink / raw)
To: Christopher Baines; +Cc: 29466
Hi!
Christopher Baines <mail@cbaines.net> skribis:
> The nginx service can fail to start if the server names hash bucket size is
> too small, which can happen on some systems, and when using QEMU, depending on
> the CPU.
>
> * gnu/services/web.scm (<nginx-configuration>): Add
> server-names-hash-bucket-size and server-names-hash-bucket-max-size.
> (default-nginx-config): Add support for the new hash bucket size parameters.
> (nginx-service, nginx-activation): Pass the new hash bucket size parameters
> through to the default-nginx-config procedure.
> * doc/guix.texi (Web Services): Document the new hash bucket size parameters.
LGTM!
However…
> -(define (default-nginx-config nginx log-directory run-directory server-list upstream-list)
> +(define (default-nginx-config nginx log-directory run-directory server-list
> + upstream-list server-names-hash-bucket-size
> + server-names-hash-bucket-max-size)
That’s too many positional parameters. And should we use a gexp
compiler for <nginx-configuration> anyway?
> (define nginx-shepherd-service
> (match-lambda
> (($ <nginx-configuration> nginx log-directory run-directory server-blocks
> - upstream-blocks file)
> + upstream-blocks server-names-hash-bucket-size
> + server-names-hash-bucket-max-size file)
Likewise, at this stage, we should probably use ‘match-record’ to avoid
mistakes.
Thanks,
Ludo’.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [bug#29466] [PATCH 1/2] services: web: Switch nginx related functions to use match-record.
2017-11-27 14:06 ` Ludovic Courtès
@ 2017-12-10 8:44 ` Christopher Baines
2017-12-10 8:44 ` [bug#29466] [PATCH 2/2] WIP: Split the config file out of the <nginx-configuration> record Christopher Baines
2017-12-11 16:05 ` [bug#29466] [PATCH 1/2] services: web: Switch nginx related functions to use match-record Ludovic Courtès
2017-12-10 9:00 ` [bug#29466] [PATCH] services: web: Add support for configuring the nginx server names hash Christopher Baines
1 sibling, 2 replies; 7+ messages in thread
From: Christopher Baines @ 2017-12-10 8:44 UTC (permalink / raw)
To: 29466
As this is less prone to mistakes than match.
* gnu/services/web.scm (default-nginx-config, nginx-activation,
nginx-shepherd-service): Switch from using match-lambda to match-record.
---
gnu/services/web.scm | 166 +++++++++++++++++++++++++--------------------------
1 file changed, 81 insertions(+), 85 deletions(-)
diff --git a/gnu/services/web.scm b/gnu/services/web.scm
index b9acee762..477e43e8d 100644
--- a/gnu/services/web.scm
+++ b/gnu/services/web.scm
@@ -241,39 +241,43 @@ of index files."
(cons head out)))
(fold-right flatten1 '() lst))
-(define (default-nginx-config nginx log-directory run-directory server-list
- upstream-list server-names-hash-bucket-size
- server-names-hash-bucket-max-size)
- (apply mixed-text-file "nginx.conf"
- (flatten
- "user nginx nginx;\n"
- "pid " run-directory "/pid;\n"
- "error_log " log-directory "/error.log info;\n"
- "http {\n"
- " client_body_temp_path " run-directory "/client_body_temp;\n"
- " proxy_temp_path " run-directory "/proxy_temp;\n"
- " fastcgi_temp_path " run-directory "/fastcgi_temp;\n"
- " uwsgi_temp_path " run-directory "/uwsgi_temp;\n"
- " scgi_temp_path " run-directory "/scgi_temp;\n"
- " access_log " log-directory "/access.log;\n"
- " include " nginx "/share/nginx/conf/mime.types;\n"
- (if server-names-hash-bucket-size
- (string-append
- " server_names_hash_bucket_size "
- (number->string server-names-hash-bucket-size)
- ";\n")
- "")
- (if server-names-hash-bucket-max-size
- (string-append
- " server_names_hash_bucket_max_size "
- (number->string server-names-hash-bucket-max-size)
- ";\n")
- "")
- "\n"
- (map emit-nginx-upstream-config upstream-list)
- (map emit-nginx-server-config server-list)
- "}\n"
- "events {}\n")))
+(define (default-nginx-config config)
+ (match-record config
+ <nginx-configuration>
+ (nginx log-directory run-directory
+ server-blocks upstream-blocks
+ server-names-hash-bucket-size
+ server-names-hash-bucket-max-size)
+ (apply mixed-text-file "nginx.conf"
+ (flatten
+ "user nginx nginx;\n"
+ "pid " run-directory "/pid;\n"
+ "error_log " log-directory "/error.log info;\n"
+ "http {\n"
+ " client_body_temp_path " run-directory "/client_body_temp;\n"
+ " proxy_temp_path " run-directory "/proxy_temp;\n"
+ " fastcgi_temp_path " run-directory "/fastcgi_temp;\n"
+ " uwsgi_temp_path " run-directory "/uwsgi_temp;\n"
+ " scgi_temp_path " run-directory "/scgi_temp;\n"
+ " access_log " log-directory "/access.log;\n"
+ " include " nginx "/share/nginx/conf/mime.types;\n"
+ (if server-names-hash-bucket-size
+ (string-append
+ " server_names_hash_bucket_size "
+ (number->string server-names-hash-bucket-size)
+ ";\n")
+ "")
+ (if server-names-hash-bucket-max-size
+ (string-append
+ " server_names_hash_bucket_max_size "
+ (number->string server-names-hash-bucket-max-size)
+ ";\n")
+ "")
+ "\n"
+ (map emit-nginx-upstream-config upstream-blocks)
+ (map emit-nginx-server-config server-blocks)
+ "}\n"
+ "events {}\n"))))
(define %nginx-accounts
(list (user-group (name "nginx") (system? #t))
@@ -285,61 +289,53 @@ of index files."
(home-directory "/var/empty")
(shell (file-append shadow "/sbin/nologin")))))
-(define nginx-activation
- (match-lambda
- (($ <nginx-configuration> nginx log-directory run-directory server-blocks
- upstream-blocks server-names-hash-bucket-size
- server-names-hash-bucket-max-size file)
- #~(begin
- (use-modules (guix build utils))
+(define (nginx-activation config)
+ (match-record config
+ <nginx-configuration>
+ (nginx log-directory run-directory file)
+ #~(begin
+ (use-modules (guix build utils))
- (format #t "creating nginx log directory '~a'~%" #$log-directory)
- (mkdir-p #$log-directory)
- (format #t "creating nginx run directory '~a'~%" #$run-directory)
- (mkdir-p #$run-directory)
- (format #t "creating nginx temp directories '~a/{client_body,proxy,fastcgi,uwsgi,scgi}_temp'~%" #$run-directory)
- (mkdir-p (string-append #$run-directory "/client_body_temp"))
- (mkdir-p (string-append #$run-directory "/proxy_temp"))
- (mkdir-p (string-append #$run-directory "/fastcgi_temp"))
- (mkdir-p (string-append #$run-directory "/uwsgi_temp"))
- (mkdir-p (string-append #$run-directory "/scgi_temp"))
- ;; Start-up logs. Once configuration is loaded, nginx switches to
- ;; log-directory.
- (mkdir-p (string-append #$run-directory "/logs"))
- ;; Check configuration file syntax.
- (system* (string-append #$nginx "/sbin/nginx")
- "-c" #$(or file
- (default-nginx-config nginx log-directory
- run-directory server-blocks upstream-blocks
- server-names-hash-bucket-size
- server-names-hash-bucket-max-size))
- "-t")))))
+ (format #t "creating nginx log directory '~a'~%" #$log-directory)
+ (mkdir-p #$log-directory)
+ (format #t "creating nginx run directory '~a'~%" #$run-directory)
+ (mkdir-p #$run-directory)
+ (format #t "creating nginx temp directories '~a/{client_body,proxy,fastcgi,uwsgi,scgi}_temp'~%" #$run-directory)
+ (mkdir-p (string-append #$run-directory "/client_body_temp"))
+ (mkdir-p (string-append #$run-directory "/proxy_temp"))
+ (mkdir-p (string-append #$run-directory "/fastcgi_temp"))
+ (mkdir-p (string-append #$run-directory "/uwsgi_temp"))
+ (mkdir-p (string-append #$run-directory "/scgi_temp"))
+ ;; Start-up logs. Once configuration is loaded, nginx switches to
+ ;; log-directory.
+ (mkdir-p (string-append #$run-directory "/logs"))
+ ;; Check configuration file syntax.
+ (system* (string-append #$nginx "/sbin/nginx")
+ "-c" #$(or file
+ (default-nginx-config config))
+ "-t"))))
-(define nginx-shepherd-service
- (match-lambda
- (($ <nginx-configuration> nginx log-directory run-directory server-blocks
- upstream-blocks server-names-hash-bucket-size
- server-names-hash-bucket-max-size file)
- (let* ((nginx-binary (file-append nginx "/sbin/nginx"))
- (nginx-action
- (lambda args
- #~(lambda _
- (zero?
- (system* #$nginx-binary "-c"
- #$(or file
- (default-nginx-config nginx log-directory
- run-directory server-blocks upstream-blocks
- server-names-hash-bucket-size
- server-names-hash-bucket-max-size))
- #$@args))))))
+(define (nginx-shepherd-service config)
+ (match-record config
+ <nginx-configuration>
+ (nginx file run-directory)
+ (let* ((nginx-binary (file-append nginx "/sbin/nginx"))
+ (nginx-action
+ (lambda args
+ #~(lambda _
+ (zero?
+ (system* #$nginx-binary "-c"
+ #$(or file
+ (default-nginx-config config))
+ #$@args))))))
- ;; TODO: Add 'reload' action.
- (list (shepherd-service
- (provision '(nginx))
- (documentation "Run the nginx daemon.")
- (requirement '(user-processes loopback))
- (start (nginx-action "-p" run-directory))
- (stop (nginx-action "-s" "stop"))))))))
+ ;; TODO: Add 'reload' action.
+ (list (shepherd-service
+ (provision '(nginx))
+ (documentation "Run the nginx daemon.")
+ (requirement '(user-processes loopback))
+ (start (nginx-action "-p" run-directory))
+ (stop (nginx-action "-s" "stop")))))))
(define nginx-service-type
(service-type (name 'nginx)
--
2.15.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [bug#29466] [PATCH 2/2] WIP: Split the config file out of the <nginx-configuration> record.
2017-12-10 8:44 ` [bug#29466] [PATCH 1/2] services: web: Switch nginx related functions to use match-record Christopher Baines
@ 2017-12-10 8:44 ` Christopher Baines
2017-12-11 16:05 ` [bug#29466] [PATCH 1/2] services: web: Switch nginx related functions to use match-record Ludovic Courtès
1 sibling, 0 replies; 7+ messages in thread
From: Christopher Baines @ 2017-12-10 8:44 UTC (permalink / raw)
To: 29466
---
gnu/services/web.scm | 152 +++++++++++++++++++++++++++++++--------------------
gnu/tests/web.scm | 5 +-
2 files changed, 95 insertions(+), 62 deletions(-)
diff --git a/gnu/services/web.scm b/gnu/services/web.scm
index 477e43e8d..f11ac6817 100644
--- a/gnu/services/web.scm
+++ b/gnu/services/web.scm
@@ -33,14 +33,21 @@
#:export (<nginx-configuration>
nginx-configuration
nginx-configuration?
- nginx-configuartion-nginx
+ nginx-configuration-nginx
nginx-configuration-log-directory
nginx-configuration-run-directory
- nginx-configuration-server-blocks
- nginx-configuration-upstream-blocks
- nginx-configuration-server-names-hash-bucket-size
- nginx-configuration-server-names-hash-bucket-max-size
- nginx-configuration-file
+ nginx-configuration-config-file
+
+ <nginx-config-file>
+ nginx-config-file
+ nginx-config-file?
+ nginx-config-file-nginx
+ nginx-config-file-log-directory
+ nginx-config-file-run-directory
+ nginx-config-file-server-blocks
+ nginx-config-file-upstream-blocks
+ nginx-config-file-server-names-hash-bucket-size
+ nginx-config-file-server-names-hash-bucket-max-size
<nginx-server-configuration>
nginx-server-configuration
@@ -130,6 +137,24 @@
(default #f))
(body nginx-named-location-configuration-body))
+(define-record-type* <nginx-config-file>
+ nginx-config-file make-nginx-config-file
+ nginx-config-file?
+ (nginx nginx-configuration-nginx ;<package>
+ (default nginx))
+ (log-directory nginx-config-file-log-directory ;string
+ (default "/var/log/nginx"))
+ (run-directory nginx-config-file-run-directory ;string
+ (default "/var/run/nginx"))
+ (server-blocks nginx-config-file-server-blocks
+ (default '())) ;list of <nginx-server-config-file>
+ (upstream-blocks nginx-config-file-upstream-blocks
+ (default '())) ;list of <nginx-upstream-config-file>
+ (server-names-hash-bucket-size nginx-config-file-server-names-hash-bucket-size
+ (default #f))
+ (server-names-hash-bucket-max-size nginx-config-file-server-names-hash-bucket-max-size
+ (default #f)))
+
(define-record-type* <nginx-configuration>
nginx-configuration make-nginx-configuration
nginx-configuration?
@@ -139,16 +164,9 @@
(default "/var/log/nginx"))
(run-directory nginx-configuration-run-directory ;string
(default "/var/run/nginx"))
- (server-blocks nginx-configuration-server-blocks
- (default '())) ;list of <nginx-server-configuration>
- (upstream-blocks nginx-configuration-upstream-blocks
- (default '())) ;list of <nginx-upstream-configuration>
- (server-names-hash-bucket-size nginx-configuration-server-names-hash-bucket-size
- (default #f))
- (server-names-hash-bucket-max-size nginx-configuration-server-names-hash-bucket-max-size
- (default #f))
- (file nginx-configuration-file ;#f | string | file-like
- (default #f)))
+ (config-file nginx-configuration-config-file ;string | file-like
+ (default (nginx-config-file))))
+
(define (config-domain-strings names)
"Return a string denoting the nginx config representation of NAMES, a list
@@ -241,43 +259,49 @@ of index files."
(cons head out)))
(fold-right flatten1 '() lst))
-(define (default-nginx-config config)
+(define-gexp-compiler (nginx-config-file-compiler
+ (config <nginx-config-file>) system target)
(match-record config
- <nginx-configuration>
+ <nginx-config-file>
(nginx log-directory run-directory
server-blocks upstream-blocks
server-names-hash-bucket-size
server-names-hash-bucket-max-size)
- (apply mixed-text-file "nginx.conf"
- (flatten
- "user nginx nginx;\n"
- "pid " run-directory "/pid;\n"
- "error_log " log-directory "/error.log info;\n"
- "http {\n"
- " client_body_temp_path " run-directory "/client_body_temp;\n"
- " proxy_temp_path " run-directory "/proxy_temp;\n"
- " fastcgi_temp_path " run-directory "/fastcgi_temp;\n"
- " uwsgi_temp_path " run-directory "/uwsgi_temp;\n"
- " scgi_temp_path " run-directory "/scgi_temp;\n"
- " access_log " log-directory "/access.log;\n"
- " include " nginx "/share/nginx/conf/mime.types;\n"
- (if server-names-hash-bucket-size
- (string-append
- " server_names_hash_bucket_size "
- (number->string server-names-hash-bucket-size)
- ";\n")
- "")
- (if server-names-hash-bucket-max-size
- (string-append
- " server_names_hash_bucket_max_size "
- (number->string server-names-hash-bucket-max-size)
- ";\n")
- "")
- "\n"
- (map emit-nginx-upstream-config upstream-blocks)
- (map emit-nginx-server-config server-blocks)
+ (gexp->derivation
+ "nginx.conf"
+ #~(call-with-output-file (ungexp output "out")
+ (lambda (port)
+ (display
+ (string-append
+ "user nginx nginx;\n"
+ "pid " #$run-directory "/pid;\n"
+ "error_log " #$log-directory "/error.log info;\n"
+ "http {\n"
+ " client_body_temp_path " #$run-directory "/client_body_temp;\n"
+ " proxy_temp_path " #$run-directory "/proxy_temp;\n"
+ " fastcgi_temp_path " #$run-directory "/fastcgi_temp;\n"
+ " uwsgi_temp_path " #$run-directory "/uwsgi_temp;\n"
+ " scgi_temp_path " #$run-directory "/scgi_temp;\n"
+ " access_log " #$log-directory "/access.log;\n"
+ " include " #$nginx "/share/nginx/conf/mime.types;\n"
+ #$(if server-names-hash-bucket-size
+ (string-append
+ " server_names_hash_bucket_size "
+ (number->string server-names-hash-bucket-size)
+ ";\n")
+ "")
+ #$(if server-names-hash-bucket-max-size
+ (string-append
+ " server_names_hash_bucket_max_size "
+ (number->string server-names-hash-bucket-max-size)
+ ";\n")
+ "")
+ "\n"
+ #$@(flatten (map emit-nginx-upstream-config upstream-blocks))
+ #$@(flatten (map emit-nginx-server-config server-blocks))
"}\n"
- "events {}\n"))))
+ "events {}\n")
+ port))))))
(define %nginx-accounts
(list (user-group (name "nginx") (system? #t))
@@ -292,7 +316,7 @@ of index files."
(define (nginx-activation config)
(match-record config
<nginx-configuration>
- (nginx log-directory run-directory file)
+ (nginx log-directory run-directory config-file)
#~(begin
(use-modules (guix build utils))
@@ -311,22 +335,20 @@ of index files."
(mkdir-p (string-append #$run-directory "/logs"))
;; Check configuration file syntax.
(system* (string-append #$nginx "/sbin/nginx")
- "-c" #$(or file
- (default-nginx-config config))
- "-t"))))
+ "-c" #$config-file
+ "-t"))))
(define (nginx-shepherd-service config)
(match-record config
<nginx-configuration>
- (nginx file run-directory)
+ (nginx config-file run-directory)
(let* ((nginx-binary (file-append nginx "/sbin/nginx"))
(nginx-action
(lambda args
#~(lambda _
(zero?
- (system* #$nginx-binary "-c"
- #$(or file
- (default-nginx-config config))
+ (system* #$nginx-binary
+ "-c" #$config-file
#$@args))))))
;; TODO: Add 'reload' action.
@@ -347,12 +369,22 @@ of index files."
(service-extension account-service-type
(const %nginx-accounts))))
(compose concatenate)
- (extend (lambda (config servers)
- (nginx-configuration
- (inherit config)
+ (extend
+ (lambda (config servers)
+ (let ((config-file
+ (nginx-configuration-config-file config)))
+ (if (nginx-config-file? config-file)
+ (nginx-configuration
+ (inherit config)
+ (config-file
+ (nginx-config-file
+ (inherit config-file)
(server-blocks
- (append (nginx-configuration-server-blocks config)
- servers)))))
+ (append (nginx-config-file-server-blocks config-file)
+ servers)))))
+ (unless (null? servers)
+ (display "warning: cannot extend nginx with a custom config file\n")
+ config)))))
(default-value
(nginx-configuration))))
diff --git a/gnu/tests/web.scm b/gnu/tests/web.scm
index 3fa272c67..06776b2dd 100644
--- a/gnu/tests/web.scm
+++ b/gnu/tests/web.scm
@@ -56,8 +56,9 @@
(dhcp-client-service)
(service nginx-service-type
(nginx-configuration
- (log-directory "/var/log/nginx")
- (server-blocks %nginx-servers)))
+ (config-file
+ (nginx-config-file
+ (server-blocks %nginx-servers)))))
(simple-service 'make-http-root activation-service-type
%make-http-root)))
--
2.15.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [bug#29466] [PATCH] services: web: Add support for configuring the nginx server names hash.
2017-11-27 14:06 ` Ludovic Courtès
2017-12-10 8:44 ` [bug#29466] [PATCH 1/2] services: web: Switch nginx related functions to use match-record Christopher Baines
@ 2017-12-10 9:00 ` Christopher Baines
1 sibling, 0 replies; 7+ messages in thread
From: Christopher Baines @ 2017-12-10 9:00 UTC (permalink / raw)
To: Ludovic Courtès; +Cc: 29466
[-- Attachment #1: Type: text/plain, Size: 3850 bytes --]
Ludovic Courtès writes:
>> The nginx service can fail to start if the server names hash bucket size is
>> too small, which can happen on some systems, and when using QEMU, depending on
>> the CPU.
>>
>> * gnu/services/web.scm (<nginx-configuration>): Add
>> server-names-hash-bucket-size and server-names-hash-bucket-max-size.
>> (default-nginx-config): Add support for the new hash bucket size parameters.
>> (nginx-service, nginx-activation): Pass the new hash bucket size parameters
>> through to the default-nginx-config procedure.
>> * doc/guix.texi (Web Services): Document the new hash bucket size parameters.
>
> LGTM!
>
> However…
>
>> -(define (default-nginx-config nginx log-directory run-directory server-list upstream-list)
>> +(define (default-nginx-config nginx log-directory run-directory server-list
>> + upstream-list server-names-hash-bucket-size
>> + server-names-hash-bucket-max-size)
>
> That’s too many positional parameters. And should we use a gexp
> compiler for <nginx-configuration> anyway?
>
>> (define nginx-shepherd-service
>> (match-lambda
>> (($ <nginx-configuration> nginx log-directory run-directory server-blocks
>> - upstream-blocks file)
>> + upstream-blocks server-names-hash-bucket-size
>> + server-names-hash-bucket-max-size file)
>
> Likewise, at this stage, we should probably use ‘match-record’ to avoid
> mistakes.
I've sent a couple of additional patches now, the first makes the change
to using match-record, and the second splits out the configuration file
part of the <nginx-configuration> record, and adds a gexp compiler for
it.
Let me know what you think about the 2nd patch? I don't particularly
like the duplication between the two records, as both the
<nginx-configuration> and <nginx-config-file> records need the package,
log directory and run directory for different reasons.
In summary:
- <nginx-configuration>
nginx: used for the nginx binary in the shepherd service
log-directory: created in the activation service
run-directory: created in the activation service
- <nginx-config-file>
nginx: used for the mime.types file
log-directory: written in to the config file
run-directory: written in to the config file
It's important that it's possible to specify the log directory and run
directory if you don't use the <nginx-config-file>, and instead use
something opaque (like a <local-file>). But I dislike the duplication
that this is adding.
I wonder if there is a better way of supporting the use of a raw
configuration file, rather than the record. Possibly by providing an
easy way to create a custom nginx-service-type, with a different
activation phase? I think that would allow for making the original
<nginx-configuration> compile to the configuration file, but then have
something like this for a custom config file.
(service (nginx-service-type-with-custom-activation
nginx-service-type
#:run-directory "/var/run/nginx-custom"
#:log-directory "/var/run/nginx-custom")
(local-file "nginx-custom.conf"))
One downside with this approach is that service extensions use the
service-type, so modifying it would mean that services that extend nginx
wouldn't work with this service type (you'd have to have the original as
well, or modify the service with the extension).
In the 2nd patch, I put in some (bad) handling of the extension case, as
with an opaque config file, you can't do anything. Previously, the
configuration was changed, but then this is later ignored, as the file
takes precedence.
Thanks for reviewing,
Chris
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 962 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* [bug#29466] [PATCH 1/2] services: web: Switch nginx related functions to use match-record.
2017-12-10 8:44 ` [bug#29466] [PATCH 1/2] services: web: Switch nginx related functions to use match-record Christopher Baines
2017-12-10 8:44 ` [bug#29466] [PATCH 2/2] WIP: Split the config file out of the <nginx-configuration> record Christopher Baines
@ 2017-12-11 16:05 ` Ludovic Courtès
2017-12-11 21:01 ` bug#29466: " Christopher Baines
1 sibling, 1 reply; 7+ messages in thread
From: Ludovic Courtès @ 2017-12-11 16:05 UTC (permalink / raw)
To: Christopher Baines; +Cc: 29466
Christopher Baines <mail@cbaines.net> skribis:
> As this is less prone to mistakes than match.
>
> * gnu/services/web.scm (default-nginx-config, nginx-activation,
> nginx-shepherd-service): Switch from using match-lambda to match-record.
Awesome. LGTM!
Ludo’.
PS: I’d rather let Clément or some other nginx-savvy person comment on
the second patch.
^ permalink raw reply [flat|nested] 7+ messages in thread
* bug#29466: [PATCH 1/2] services: web: Switch nginx related functions to use match-record.
2017-12-11 16:05 ` [bug#29466] [PATCH 1/2] services: web: Switch nginx related functions to use match-record Ludovic Courtès
@ 2017-12-11 21:01 ` Christopher Baines
0 siblings, 0 replies; 7+ messages in thread
From: Christopher Baines @ 2017-12-11 21:01 UTC (permalink / raw)
To: Ludovic Courtès; +Cc: 29466-done
[-- Attachment #1: Type: text/plain, Size: 591 bytes --]
Ludovic Courtès <ludo@gnu.org> writes:
> Christopher Baines <mail@cbaines.net> skribis:
>
>> As this is less prone to mistakes than match.
>>
>> * gnu/services/web.scm (default-nginx-config, nginx-activation,
>> nginx-shepherd-service): Switch from using match-lambda to match-record.
>
> Awesome. LGTM!
Great, I've pushed this and the previous patch.
> PS: I’d rather let Clément or some other nginx-savvy person comment on
> the second patch.
Yeah, that's fine :) I'll close this bug anyway, as the remaining patch
I sent is not anywhere near ready yet.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 962 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-12-11 21:02 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-27 8:23 [bug#29466] [PATCH] services: web: Add support for configuring the nginx server names hash Christopher Baines
2017-11-27 14:06 ` Ludovic Courtès
2017-12-10 8:44 ` [bug#29466] [PATCH 1/2] services: web: Switch nginx related functions to use match-record Christopher Baines
2017-12-10 8:44 ` [bug#29466] [PATCH 2/2] WIP: Split the config file out of the <nginx-configuration> record Christopher Baines
2017-12-11 16:05 ` [bug#29466] [PATCH 1/2] services: web: Switch nginx related functions to use match-record Ludovic Courtès
2017-12-11 21:01 ` bug#29466: " Christopher Baines
2017-12-10 9:00 ` [bug#29466] [PATCH] services: web: Add support for configuring the nginx server names hash Christopher Baines
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/guix.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).