[PATCH] services: nginx: Add reload action

Done

Details

3 participants

EuAndreh
Ludovic Courtès
Christopher Baines

Owner: unassigned

Submitted by: EuAndreh

Severity: normal

EuAndreh wrote on 10 Oct 2022 06:39

Recipients:(address . guix-patches@gnu.org)(name . EuAndreh)(address . eu@euandre.org)

Message-ID:20221010043932.28384-1-eu@euandre.org

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

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

Christopher Baines wrote on 11 Oct 2022 12:51

Recipients:(name . EuAndreh)(address . eu@euandre.org)(address . 58405@debbugs.gnu.org)

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

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

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

EuAndreh wrote on 12 Oct 2022 09:00

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

Message-ID:166555804644.2805.1693234721157753050@localhost

Toggle quote (3 lines)

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

Toggle quote (3 lines)

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

Christopher Baines wrote on 13 Oct 2022 12:40

Recipients:(name . EuAndreh)(address . eu@euandre.org)(address . 58405@debbugs.gnu.org)

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

EuAndreh <eu@euandre.org> writes:

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

Christopher Baines wrote on 13 Oct 2022 13:38

Recipients:(name . EuAndreh)(address . eu@euandre.org)(address . 58405-done@debbugs.gnu.org)

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

Christopher Baines <mail@cbaines.net> writes:

Toggle quote (3 lines)

> 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

Closed

Ludovic Courtès wrote on 13 Oct 2022 16:02

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

Recipients:(name . EuAndreh)(address . eu@euandre.org)

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

Hi,

A late comment…

EuAndreh <eu@euandre.org> skribis:

Toggle quote (9 lines)

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

Toggle quote (3 lines)

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

EuAndreh wrote on 13 Oct 2022 18:02

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

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

Message-ID:166567692392.1002.1128529192389237246@localhost

Toggle quote (4 lines)

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

EuAndreh wrote on 13 Oct 2022 18:05

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

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

Message-ID:166567715189.1002.10471412833004207259@localhost

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

Toggle quote (3 lines)

> Nitpick: According to https://nginx.org/en/ it seems that the correct

> spelling is “nginx”, lowercase. :-)

Oh, TIL.

I'll fix that.

EuAndreh wrote on 13 Oct 2022 18:38

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

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

Message-ID:166567909990.18703.909407946097887605@localhost

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

I'm up for doing it if you agree.

Christopher Baines wrote on 14 Oct 2022 12:43

Recipients:(name . EuAndreh)(address . eu@euandre.org)(address . 58405@debbugs.gnu.org)

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

EuAndreh <eu@euandre.org> writes:

Toggle quote (7 lines)

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

Your comment

This issue is archived.

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

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

mumi current 58405

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

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