GNU bug report logs - #58405
[PATCH] services: nginx: Add reload action

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 58405 in the body.
You can then email your comments to 58405 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: EuAndreh <eu <at> euandre.org>
To: guix-patches <at> gnu.org
Cc: EuAndreh <eu <at> euandre.org>
Subject: [PATCH] services: nginx: Add reload action
Date: Mon, 10 Oct 2022 01:39:32 -0300

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

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

From: Christopher Baines <mail <at> cbaines.net>
To: EuAndreh <eu <at> euandre.org>
Cc: 58405 <at> debbugs.gnu.org
Subject: Re: [bug#58405] [PATCH] services: nginx: Add reload action
Date: Tue, 11 Oct 2022 11:51:50 +0100

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

EuAndreh via Guix-patches via <guix-patches <at> 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

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

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

From: EuAndreh <eu <at> euandre.org>
To: Christopher Baines <mail <at> cbaines.net>
Cc: 58405 <at> debbugs.gnu.org
Subject: Re: [bug#58405] [PATCH] services: nginx: Add reload action
Date: Wed, 12 Oct 2022 04:00:46 -0300

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

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

From: Christopher Baines <mail <at> cbaines.net>
To: EuAndreh <eu <at> euandre.org>
Cc: 58405 <at> debbugs.gnu.org
Subject: Re: [bug#58405] [PATCH] services: nginx: Add reload action
Date: Thu, 13 Oct 2022 11:40:23 +0100

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

EuAndreh <eu <at> 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

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

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

From: Christopher Baines <mail <at> cbaines.net>
To: EuAndreh <eu <at> euandre.org>
Cc: 58405-done <at> debbugs.gnu.org
Subject: Re: [bug#58405] [PATCH] services: nginx: Add reload action
Date: Thu, 13 Oct 2022 12:38:14 +0100

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

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

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

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: EuAndreh <eu <at> euandre.org>
Cc: 58405 <at> debbugs.gnu.org, Christopher Baines <mail <at> cbaines.net>
Subject: Re: bug#58405: [PATCH] services: nginx: Add reload action
Date: Thu, 13 Oct 2022 16:02:03 +0200

Hi,

A late comment…

EuAndreh <eu <at> 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’.

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

From: EuAndreh <eu <at> euandre.org>
To: Christopher Baines <mail <at> cbaines.net>
Cc: 58405 <at> debbugs.gnu.org
Subject: Re: [bug#58405] [PATCH] services: nginx: Add reload action
Date: Thu, 13 Oct 2022 13:02:03 -0300

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

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

From: EuAndreh <eu <at> euandre.org>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 58405 <at> debbugs.gnu.org, Christopher Baines <mail <at> cbaines.net>
Subject: Re: bug#58405: [PATCH] services: nginx: Add reload action
Date: Thu, 13 Oct 2022 13:05:51 -0300

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.

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

From: EuAndreh <eu <at> euandre.org>
To: Christopher Baines <mail <at> cbaines.net>
Cc: 58405 <at> debbugs.gnu.org
Subject: Re: [bug#58405] [PATCH] services: nginx: Add reload action
Date: Thu, 13 Oct 2022 13:38:19 -0300

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

I'm up for doing it if you agree.

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

From: Christopher Baines <mail <at> cbaines.net>
To: EuAndreh <eu <at> euandre.org>
Cc: 58405 <at> debbugs.gnu.org
Subject: Re: [bug#58405] [PATCH] services: nginx: Add reload action
Date: Fri, 14 Oct 2022 11:43:19 +0100

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

EuAndreh <eu <at> 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.

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