GNU bug report logs - #29466
[PATCH] services: web: Add support for configuring the nginx server names hash.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 29466 in the body.
You can then email your comments to 29466 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: guix-patches <at> gnu.org
Subject: [PATCH] services: web: Add support for configuring the nginx server
 names hash.
Date: Mon, 27 Nov 2017 08:23:38 +0000

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

Message #8 received at 29466 <at> debbugs.gnu.org (full text, mbox):

From: ludo <at> gnu.org (Ludovic Courtès)
To: Christopher Baines <mail <at> cbaines.net>
Cc: 29466 <at> debbugs.gnu.org
Subject: Re: [bug#29466] [PATCH] services: web: Add support for configuring
 the nginx server names hash.
Date: Mon, 27 Nov 2017 15:06:48 +0100

Hi!

Christopher Baines <mail <at> 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’.

Message #11 received at 29466 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: 29466 <at> debbugs.gnu.org
Subject: [PATCH 1/2] services: web: Switch nginx related functions to use
 match-record.
Date: Sun, 10 Dec 2017 08:44:20 +0000

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

Message #14 received at 29466 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: 29466 <at> debbugs.gnu.org
Subject: [PATCH 2/2] WIP: Split the config file out of the
 <nginx-configuration> record.
Date: Sun, 10 Dec 2017 08:44:21 +0000

---
 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

Message #17 received at 29466 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 29466 <at> debbugs.gnu.org
Subject: Re: [bug#29466] [PATCH] services: web: Add support for configuring
 the nginx server names hash.
Date: Sun, 10 Dec 2017 09:00:28 +0000

[Message part 1 (text/plain, inline)]

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

[signature.asc (application/pgp-signature, inline)]

Message #20 received at 29466 <at> debbugs.gnu.org (full text, mbox):

From: ludo <at> gnu.org (Ludovic Courtès)
To: Christopher Baines <mail <at> cbaines.net>
Cc: 29466 <at> debbugs.gnu.org
Subject: Re: [bug#29466] [PATCH 1/2] services: web: Switch nginx related
 functions to use match-record.
Date: Mon, 11 Dec 2017 17:05:41 +0100

Christopher Baines <mail <at> 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.

Message #25 received at 29466-done <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 29466-done <at> debbugs.gnu.org
Subject: Re: [bug#29466] [PATCH 1/2] services: web: Switch nginx related
 functions to use match-record.
Date: Mon, 11 Dec 2017 21:01:22 +0000

[Message part 1 (text/plain, inline)]

Ludovic Courtès <ludo <at> gnu.org> writes:

> Christopher Baines <mail <at> 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.

[signature.asc (application/pgp-signature, inline)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.