[PATCH] services: web: Add support for configuring the nginx server names hash.

Done

Details

2 participants

Ludovic Courtès
Christopher Baines

Owner: unassigned

Submitted by: Christopher Baines

Severity: normal

Debbugs page

Christopher Baines wrote 7 years ago

Recipients:(address . guix-patches@gnu.org)

Message-ID:20171127082338.18504-1-mail@cbaines.net

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

Toggle diff (114 lines)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

Ludovic Courtès wrote 7 years ago

Recipients:(name . Christopher Baines)(address . mail@cbaines.net)(address . 29466@debbugs.gnu.org)

Message-ID:87zi77lssn.fsf@gnu.org

Hi!

Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (11 lines)> 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…

Toggle quote (5 lines)

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

Toggle quote (7 lines)

> (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’.

Christopher Baines wrote 7 years ago

[PATCH 1/2] services: web: Switch nginx related functions to use match-record.

Recipients:(address . 29466@debbugs.gnu.org)

Message-ID:20171210084421.26404-1-mail@cbaines.net

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

Toggle diff (189 lines)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

Christopher Baines wrote 7 years ago

[PATCH 2/2] WIP: Split the config file out of the <nginx-configuration> record.

Recipients:(address . 29466@debbugs.gnu.org)

Message-ID:20171210084421.26404-2-mail@cbaines.net

---

gnu/services/web.scm | 152 +++++++++++++++++++++++++++++++--------------------

gnu/tests/web.scm | 5 +-

2 files changed, 95 insertions(+), 62 deletions(-)

Toggle diff (241 lines)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

Christopher Baines wrote 7 years ago

Re: [bug#29466] [PATCH] services: web: Add support for configuring the nginx server names hash.

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 29466@debbugs.gnu.org)

Message-ID:87374j6jpv.fsf@cbaines.net

Ludovic Courtès writes:

Toggle quote (33 lines)>> 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

Ludovic Courtès wrote 7 years ago

Re: [bug#29466] [PATCH 1/2] services: web: Switch nginx related functions to use match-record.

Recipients:(name . Christopher Baines)(address . mail@cbaines.net)(address . 29466@debbugs.gnu.org)

Message-ID:87po7lxnai.fsf@gnu.org

Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (5 lines)

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

Christopher Baines wrote 7 years ago

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 29466-done@debbugs.gnu.org)

Message-ID:87wp1t568t.fsf@cbaines.net

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

Toggle quote (9 lines)

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

Toggle quote (3 lines)

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

Closed

Your comment

This issue is archived.

To comment on this conversation send an email to 29466@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it

mumi current 29466

Then, you may apply the latest patchset in this issue (with sign off)

mumi am -- -s

Or, compose a reply to this issue

mumi compose

Or, send patches to this issue

mumi send-email *.patch

You may also tag this issue. See list of standard tags. For example, to set the confirmed and easy tags

mumi command -t +confirmed -t +easy

Or, remove the moreinfo tag and set the help tag

mumi command -t -moreinfo -t +help

is:open	open issues
is:done	closed issues
submitter:<who>	search issue submitter
author:<who>	search by message author
date:yesterday..now	search by issue date
mdate:3m..2d	search by message date