[bug#58405] [PATCH] services: nginx: Add reload action

[bug#58405] [PATCH] services: nginx: Add reload action

[EuAndreh via Guix-patches via]

In a new "reload" shepherd-action, send a SIGHUP to the NGINX master
process, so that it can re-read the configuration file and start new
worker processes.

* gnu/services/web.scm (nginx-shepherd-service): Add the "reload"
  shepherd-action
---
 gnu/services/web.scm | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/gnu/services/web.scm b/gnu/services/web.scm
index e5ab1a1180..227a577de3 100644
--- a/gnu/services/web.scm
+++ b/gnu/services/web.scm
@@ -807,7 +807,6 @@ (define (nginx-shepherd-service config)
                           #~#t
                           #~(read-pid-file #$pid-file))))))))
 
-     ;; TODO: Add 'reload' action.
      (list (shepherd-service
             (provision '(nginx))
             (documentation "Run the nginx daemon.")
@@ -815,7 +814,19 @@ (define (nginx-shepherd-service config)
             (modules `((ice-9 match)
                        ,@%default-modules))
             (start (nginx-action "-p" run-directory))
-            (stop (nginx-action "-s" "stop")))))))
+            (stop (nginx-action "-s" "stop"))
+            (actions
+              (list
+               (shepherd-action
+                 (name 'reload)
+                 (documentation "Reload NGINX configuration file and restart worker processes.")
+                 (procedure
+                   #~(lambda (pid)
+                       (if pid
+                         (begin
+                           (kill pid SIGHUP)
+                           (format #t "Service NGINX (PID ~a) has been reloaded." pid))
+                         (format #t "Service NGINX is not running."))))))))))))
 
 (define nginx-service-type
   (service-type (name 'nginx)
-- 
2.37.3

[bug#58405] [PATCH] services: nginx: Add reload action

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 664 bytes --]


EuAndreh via Guix-patches via <guix-patches@gnu.org> writes:

> In a new "reload" shepherd-action, send a SIGHUP to the NGINX master
> process, so that it can re-read the configuration file and start new
> worker processes.
>
> * gnu/services/web.scm (nginx-shepherd-service): Add the "reload"
>   shepherd-action
> ---
>  gnu/services/web.scm | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)

With the NGinx service currently, you need to restart it to change the
NGinx binary or configuration file.

What's the purpose of the reload action here given that neither the
binary or configuration file being used will change?

Thanks,

Chris

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 987 bytes --]

[bug#58405] [PATCH] services: nginx: Add reload action

[EuAndreh via Guix-patches via]

> With the NGinx service currently, you need to restart it to change the
> NGinx binary or configuration file.

It is true that you need to restart to change the NGINX binary, but this
is not true for changing the configuration file.

NGINX's master process reloads the configuration file, which could have
an "include" line that points to ad-hoc files in /etc.  So even though
the NGINX service is using the immutable file inside /gnu/store,
reloading it can have it change its runtime behaviour.

The same behaviour is relied upon for certbot certificates: the current
certificate lives in /etc/letsencrypt/live, but it is a symlink that
points to /etc/letsencrypt/archive.  When a certificate is renewed, a
SIGHUP ought to be sent to NGINX in order to reload the configuration
file, so that the certificates themselves can be reloaded, even though
neither the NGINX binary nor the configuration file changed, but only
what they point to did.


> What's the purpose of the reload action here given that neither the
> binary or configuration file being used will change?

I'm doing blue/green deployments on a web service.  I have the
equivalent of /etc/my-service/{blue,green,active}.conf files, and an
"include" line in the main NGINX configuration that includes the
"active" one.  Doing a deploy from blue to green is done by changing the
`active.conf` symlink to point to `green.conf` instead, and sending a
SIGHUP to NGINX.

[bug#58405] [PATCH] services: nginx: Add reload action

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 1276 bytes --]


EuAndreh <eu@euandre.org> writes:

>> With the NGinx service currently, you need to restart it to change the
>> NGinx binary or configuration file.
>
> It is true that you need to restart to change the NGINX binary, but this
> is not true for changing the configuration file.
>
> NGINX's master process reloads the configuration file, which could have
> an "include" line that points to ad-hoc files in /etc.  So even though
> the NGINX service is using the immutable file inside /gnu/store,
> reloading it can have it change its runtime behaviour.
>
> The same behaviour is relied upon for certbot certificates: the current
> certificate lives in /etc/letsencrypt/live, but it is a symlink that
> points to /etc/letsencrypt/archive.  When a certificate is renewed, a
> SIGHUP ought to be sent to NGINX in order to reload the configuration
> file, so that the certificates themselves can be reloaded, even though
> neither the NGINX binary nor the configuration file changed, but only
> what they point to did.

That makes sense. I do think this still might cause confusion, since I
think some will expect this to change NGinx to use the configuration
defined in the system configuration.

I'm not quite sure how to address that, but I think this can still be
merged.

Chris

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 987 bytes --]

bug#58405: [PATCH] services: nginx: Add reload action

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 224 bytes --]


Christopher Baines <mail@cbaines.net> writes:

> I'm not quite sure how to address that, but I think this can still be
> merged.

I've gone ahead and pushed this as
10d429f2fce321d8285684503094694ec3979865.

Thanks,

Chris

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 987 bytes --]

[bug#58405] [PATCH] services: nginx: Add reload action

[Ludovic Courtès]

Hi,

A late comment…

EuAndreh <eu@euandre.org> skribis:

> +               (shepherd-action
> +                 (name 'reload)
> +                 (documentation "Reload NGINX configuration file and restart worker processes.")
> +                 (procedure
> +                   #~(lambda (pid)
> +                       (if pid
> +                         (begin
> +                           (kill pid SIGHUP)

Isn’t ‘nginx -s reload’ the documented way to do that?  Or maybe it’s
completely equivalent?

> +                           (format #t "Service NGINX (PID ~a) has been reloaded." pid))
> +                         (format #t "Service NGINX is not running."))))))))))))

Nitpick: According to <https://nginx.org/en/> it seems that the correct
spelling is “nginx”, lowercase.  :-)

Thanks,
Ludo’.

[bug#58405] [PATCH] services: nginx: Add reload action

[EuAndreh via Guix-patches via]

> That makes sense. I do think this still might cause confusion, since I
> think some will expect this to change NGinx to use the configuration
> defined in the system configuration.

How about being more explicit in the action documentation about the scope of the
reload?

[bug#58405] [PATCH] services: nginx: Add reload action

[EuAndreh via Guix-patches via]

They're compleltely equivalent :)

Both are documented ways of doing the same, see:

- https://nginx.org/en/docs/control.html
- https://nginx.org/en/docs/beginners_guide.html

> Nitpick: According to <https://nginx.org/en/> it seems that the correct
> spelling is “nginx”, lowercase.  :-)

Oh, TIL.

I'll fix that.

[bug#58405] [PATCH] services: nginx: Add reload action

[EuAndreh via Guix-patches via]

I think it would address your concerns on confusion of users. 

I'm up for doing it if you agree.

[bug#58405] [PATCH] services: nginx: Add reload action

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 348 bytes --]


EuAndreh <eu@euandre.org> writes:

>> That makes sense. I do think this still might cause confusion, since I
>> think some will expect this to change NGinx to use the configuration
>> defined in the system configuration.
>
> How about being more explicit in the action documentation about the scope of the
> reload?

Yeah, that sounds good to me.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 987 bytes --]