unofficial mirror of guix-patches@gnu.org 
 help / color / mirror / code / Atom feed
* [bug#58405] [PATCH] services: nginx: Add reload action
@ 2022-10-10  4:39 EuAndreh via Guix-patches via
  2022-10-11 10:51 ` Christopher Baines
  2022-10-13 14:02 ` Ludovic Courtès
  0 siblings, 2 replies; 10+ messages in thread
From: EuAndreh via Guix-patches via @ 2022-10-10  4:39 UTC (permalink / raw)
  To: 58405; +Cc: EuAndreh

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





^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [bug#58405] [PATCH] services: nginx: Add reload action
  2022-10-10  4:39 [bug#58405] [PATCH] services: nginx: Add reload action EuAndreh via Guix-patches via
@ 2022-10-11 10:51 ` Christopher Baines
  2022-10-12  7:00   ` EuAndreh via Guix-patches via
  2022-10-13 14:02 ` Ludovic Courtès
  1 sibling, 1 reply; 10+ messages in thread
From: Christopher Baines @ 2022-10-11 10:51 UTC (permalink / raw)
  To: EuAndreh; +Cc: 58405

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [bug#58405] [PATCH] services: nginx: Add reload action
  2022-10-11 10:51 ` Christopher Baines
@ 2022-10-12  7:00   ` EuAndreh via Guix-patches via
  2022-10-13 10:40     ` Christopher Baines
  0 siblings, 1 reply; 10+ messages in thread
From: EuAndreh via Guix-patches via @ 2022-10-12  7:00 UTC (permalink / raw)
  To: Christopher Baines; +Cc: 58405

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




^ permalink raw reply	[flat|nested] 10+ messages in thread

* [bug#58405] [PATCH] services: nginx: Add reload action
  2022-10-12  7:00   ` EuAndreh via Guix-patches via
@ 2022-10-13 10:40     ` Christopher Baines
  2022-10-13 11:38       ` bug#58405: " Christopher Baines
  2022-10-13 16:02       ` [bug#58405] " EuAndreh via Guix-patches via
  0 siblings, 2 replies; 10+ messages in thread
From: Christopher Baines @ 2022-10-13 10:40 UTC (permalink / raw)
  To: EuAndreh; +Cc: 58405

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#58405: [PATCH] services: nginx: Add reload action
  2022-10-13 10:40     ` Christopher Baines
@ 2022-10-13 11:38       ` Christopher Baines
  2022-10-13 16:02       ` [bug#58405] " EuAndreh via Guix-patches via
  1 sibling, 0 replies; 10+ messages in thread
From: Christopher Baines @ 2022-10-13 11:38 UTC (permalink / raw)
  To: EuAndreh; +Cc: 58405-done

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [bug#58405] [PATCH] services: nginx: Add reload action
  2022-10-10  4:39 [bug#58405] [PATCH] services: nginx: Add reload action EuAndreh via Guix-patches via
  2022-10-11 10:51 ` Christopher Baines
@ 2022-10-13 14:02 ` Ludovic Courtès
  2022-10-13 16:05   ` EuAndreh via Guix-patches via
  1 sibling, 1 reply; 10+ messages in thread
From: Ludovic Courtès @ 2022-10-13 14:02 UTC (permalink / raw)
  To: EuAndreh; +Cc: 58405, Christopher Baines

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




^ permalink raw reply	[flat|nested] 10+ messages in thread

* [bug#58405] [PATCH] services: nginx: Add reload action
  2022-10-13 10:40     ` Christopher Baines
  2022-10-13 11:38       ` bug#58405: " Christopher Baines
@ 2022-10-13 16:02       ` EuAndreh via Guix-patches via
  2022-10-13 16:38         ` EuAndreh via Guix-patches via
  2022-10-14 10:43         ` Christopher Baines
  1 sibling, 2 replies; 10+ messages in thread
From: EuAndreh via Guix-patches via @ 2022-10-13 16:02 UTC (permalink / raw)
  To: Christopher Baines; +Cc: 58405

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




^ permalink raw reply	[flat|nested] 10+ messages in thread

* [bug#58405] [PATCH] services: nginx: Add reload action
  2022-10-13 14:02 ` Ludovic Courtès
@ 2022-10-13 16:05   ` EuAndreh via Guix-patches via
  0 siblings, 0 replies; 10+ messages in thread
From: EuAndreh via Guix-patches via @ 2022-10-13 16:05 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: 58405, Christopher Baines

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.




^ permalink raw reply	[flat|nested] 10+ messages in thread

* [bug#58405] [PATCH] services: nginx: Add reload action
  2022-10-13 16:02       ` [bug#58405] " EuAndreh via Guix-patches via
@ 2022-10-13 16:38         ` EuAndreh via Guix-patches via
  2022-10-14 10:43         ` Christopher Baines
  1 sibling, 0 replies; 10+ messages in thread
From: EuAndreh via Guix-patches via @ 2022-10-13 16:38 UTC (permalink / raw)
  To: Christopher Baines; +Cc: 58405

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

I'm up for doing it if you agree.




^ permalink raw reply	[flat|nested] 10+ messages in thread

* [bug#58405] [PATCH] services: nginx: Add reload action
  2022-10-13 16:02       ` [bug#58405] " EuAndreh via Guix-patches via
  2022-10-13 16:38         ` EuAndreh via Guix-patches via
@ 2022-10-14 10:43         ` Christopher Baines
  1 sibling, 0 replies; 10+ messages in thread
From: Christopher Baines @ 2022-10-14 10:43 UTC (permalink / raw)
  To: EuAndreh; +Cc: 58405

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-10-14 10:44 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-10-10  4:39 [bug#58405] [PATCH] services: nginx: Add reload action EuAndreh via Guix-patches via
2022-10-11 10:51 ` Christopher Baines
2022-10-12  7:00   ` EuAndreh via Guix-patches via
2022-10-13 10:40     ` Christopher Baines
2022-10-13 11:38       ` bug#58405: " Christopher Baines
2022-10-13 16:02       ` [bug#58405] " EuAndreh via Guix-patches via
2022-10-13 16:38         ` EuAndreh via Guix-patches via
2022-10-14 10:43         ` Christopher Baines
2022-10-13 14:02 ` Ludovic Courtès
2022-10-13 16:05   ` EuAndreh via Guix-patches via

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/guix.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).