diff mbox series

[bug#61587,6/8] services: network-manager: Await for NetworkManager to finish starting up.

Message ID 502d6efbde6d7e372ab6bc414fbbe2c065c23b4e.1676667941.git.mirai@makinata.eu
State New
Headers show
Series networking services refactoring | expand

Commit Message

Bruno Victal Feb. 17, 2023, 9:14 p.m. UTC 
This is similar to its NetworkManager-wait-online.service systemd counterpart,
with the main difference being that we handle it all in 'networking symbol, rather than
introduce a new 'networking-online symbol. (see discussion #47253)

As a result of this change, with opensmtpd-service-type as an example,
manual 'herd restart smtpd' after system bootups are no longer required
when opensmtpd is configured with a smtpd.conf containing non-loopback interfaces.
(this issue is described in more detail at #60300)

Addresses #60300.
Supersedes #47253. (Note: Shepherd no longer blocks since shepherd 0.9.3)

* gnu/services/networking.scm (network-manager-shepherd-service): Await for
NetworkManager to finish starting up.
---
 gnu/services/networking.scm | 36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

Comments

Ludovic Courtès March 3, 2023, 5:13 p.m. UTC | #1 
Hi,

Bruno Victal <mirai@makinata.eu> skribis:

> This is similar to its NetworkManager-wait-online.service systemd counterpart,
> with the main difference being that we handle it all in 'networking symbol, rather than
> introduce a new 'networking-online symbol. (see discussion #47253)
>
> As a result of this change, with opensmtpd-service-type as an example,
> manual 'herd restart smtpd' after system bootups are no longer required
> when opensmtpd is configured with a smtpd.conf containing non-loopback interfaces.
> (this issue is described in more detail at #60300)
>
> Addresses #60300.

Please write: “Fixes <https://issues.guix.gnu.org/60300>.”
Possibly along with a “Reported by” line (see the Git log for inspiration).

> Supersedes #47253. (Note: Shepherd no longer blocks since shepherd 0.9.3)

What does that mean?

> * gnu/services/networking.scm (network-manager-shepherd-service): Await for
> NetworkManager to finish starting up.

[...]

> +             (start
> +              #~(lambda args
> +                  (let ((constructor
> +                         (apply
> +                          (make-forkexec-constructor
> +                           (list #$(file-append network-manager
> +                                                "/sbin/NetworkManager")
> +                                 (string-append "--config=" #$conf)
> +                                 "--no-daemon")

Rather:

  (let ((pid (fork+exec-command (list …))))
    …
    pid)

Ludo’.
Bruno Victal March 3, 2023, 5:26 p.m. UTC | #2 
On 2023-03-03 17:13, Ludovic Courtès wrote:> Bruno Victal <mirai@makinata.eu> skribis:
> 
>> Supersedes #47253. (Note: Shepherd no longer blocks since shepherd 0.9.3)
> 
> What does that mean?

Oops, I didn't intend to include this in the commit message, it was for the mail body.
At the time I didn't notice that #47253 already had a patch to solve this issue which resulted
in independent rewrite of the same fix. I only noticed it while I was searching for open issues to
attach to the message as potential issues that would be closed by this.

The note refers to what made #47253 untenable back then.

>> * gnu/services/networking.scm (network-manager-shepherd-service): Await for
>> NetworkManager to finish starting up.
> 
> [...]
> 
>> +             (start
>> +              #~(lambda args
>> +                  (let ((constructor
>> +                         (apply
>> +                          (make-forkexec-constructor
>> +                           (list #$(file-append network-manager
>> +                                                "/sbin/NetworkManager")
>> +                                 (string-append "--config=" #$conf)
>> +                                 "--no-daemon")
> 
> Rather:
> 
>   (let ((pid (fork+exec-command (list …))))
>     …
>     pid)

I'll send a v2 with the required touch-ups.


Cheers,
Bruno
diff mbox series

Patch

diff --git a/gnu/services/networking.scm b/gnu/services/networking.scm
index 13816327b0..55bc2cf362 100644
--- a/gnu/services/networking.scm
+++ b/gnu/services/networking.scm
@@ -1235,17 +1235,31 @@  (define (network-manager-shepherd-service config)
                             ;; TODO: iwd? is deprecated and should be passed
                             ;; with shepherd-requirement, remove later.
                             ,@(if iwd? '(iwd) '())))
-             (start #~(make-forkexec-constructor
-                       (list (string-append #$network-manager
-                                            "/sbin/NetworkManager")
-                             (string-append "--config=" #$conf)
-                             "--no-daemon")
-                       #:environment-variables
-                       (list (string-append "NM_VPN_PLUGIN_DIR=" #$vpn
-                                            "/lib/NetworkManager/VPN")
-                             ;; Override non-existent default users
-                             "NM_OPENVPN_USER="
-                             "NM_OPENVPN_GROUP=")))
+             (start
+              #~(lambda args
+                  (let ((constructor
+                         (apply
+                          (make-forkexec-constructor
+                           (list #$(file-append network-manager
+                                                "/sbin/NetworkManager")
+                                 (string-append "--config=" #$conf)
+                                 "--no-daemon")
+                           #:environment-variables
+                           (list (string-append "NM_VPN_PLUGIN_DIR=" #$vpn
+                                                "/lib/NetworkManager/VPN")
+                                 ;; Override non-existent default users
+                                 "NM_OPENVPN_USER="
+                                 "NM_OPENVPN_GROUP=")) args)))
+                    ;; XXX: Despite the "online" name, this doesn't guarantee
+                    ;; WAN connectivity, it merely waits for NetworkManager
+                    ;; to finish starting-up. This is required otherwise
+                    ;; services will fail since the network interfaces be
+                    ;; absent until NetworkManager finishes setting them up.
+                    (system* #$(file-append network-manager "/bin/nm-online")
+                             "--wait-for-startup" "--quiet")
+                    ;; XXX: Finally, return the value from running
+                    ;; make-forkexec-constructor to shepherd.
+                    constructor)))
              (stop #~(make-kill-destructor)))))))
 
 (define network-manager-service-type