[bug#29117] Static networking vs Open vSwitch

[bug#29117] Static networking vs Open vSwitch

[Marius Bakke]

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

Hello Guix,

When using static-networking-service with OpenvSwitch-managed
interfaces, shepherd will often attempt to start the static interface
configuration before the virtual interface has been created by OVS,
requiring manual intervention after boot.

This first patch allows arbitrary services to override 'requirement' in
<static-networking>; the second adds some tests for Open vSwitch,
including whether static-networking started successfully on a virtual
interface.

Marius Bakke (2):
  services: networking: Add a dependency override mechanism to
    <static-networking>.
  tests: networking: Add tests for OpenvSwitch.

 doc/guix.texi               |   5 +-
 gnu/services/networking.scm |  14 ++++--
 gnu/tests/networking.scm    | 109 +++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 121 insertions(+), 7 deletions(-)

-- 
2.15.0

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

[bug#29117] [PATCH 1/2] services: networking: Add a dependency override mechanism to <static-networking>.

[Marius Bakke]

* gnu/services/networking.scm (<static-networking>): Add 'requirement' record.
(static-networking-shepherd-service): Make UDEV the default requirement.
(static-networking-service): Expose 'requirement' parameter.
* doc/guix.texi (Networking Services): Document it.
---
 doc/guix.texi               |  5 ++++-
 gnu/services/networking.scm | 14 +++++++++-----
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 983d0e52e..c501c77eb 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -10288,9 +10288,12 @@ This is the type for statically-configured network interfaces.
 
 @deffn {Scheme Procedure} static-networking-service @var{interface} @var{ip} @
        [#:netmask #f] [#:gateway #f] [#:name-servers @code{'()}]
+       [#:requirement @code{'(udev)}]
 Return a service that starts @var{interface} with address @var{ip}.  If
 @var{netmask} is true, use it as the network mask.  If @var{gateway} is true,
-it must be a string specifying the default network gateway.
+it must be a string specifying the default network gateway.  @var{requirement}
+can be used to declare a dependency on another service before configuring the
+interface.
 
 This procedure can be called several times, one for each network
 interface of interest.  Behind the scenes what it does is extend
diff --git a/gnu/services/networking.scm b/gnu/services/networking.scm
index b0c23aafc..e7d9d9457 100644
--- a/gnu/services/networking.scm
+++ b/gnu/services/networking.scm
@@ -51,6 +51,7 @@
             static-networking-ip
             static-networking-netmask
             static-networking-gateway
+            static-networking-requirement
 
             static-networking-service
             static-networking-service-type
@@ -145,19 +146,21 @@ fe80::1%lo0 apps.facebook.com\n")
            (default #f))
   (provision static-networking-provision
              (default #f))
+  ;; Wait for udev to be up so that interfaces are actually usable.
+  (requirement static-networking-requirement
+               (default '(udev)))
   (name-servers static-networking-name-servers    ;FIXME: doesn't belong here
                 (default '())))
 
 (define static-networking-shepherd-service
   (match-lambda
     (($ <static-networking> interface ip netmask gateway provision
-                            name-servers)
+                            requirement name-servers)
      (let ((loopback? (and provision (memq 'loopback provision))))
        (shepherd-service
 
-        ;; Unless we're providing the loopback interface, wait for udev to be up
-        ;; and running so that INTERFACE is actually usable.
-        (requirement (if loopback? '() '(udev)))
+        ;; Force the loopback interface to start early.
+        (requirement (if loopback? '() requirement))
 
         (documentation
          "Bring up the networking interface using a static IP address.")
@@ -262,7 +265,7 @@ network interface.")))
 
 (define* (static-networking-service interface ip
                                     #:key
-                                    netmask gateway provision
+                                    netmask gateway provision requirement
                                     (name-servers '()))
   "Return a service that starts @var{interface} with address @var{ip}.  If
 @var{netmask} is true, use it as the network mask.  If @var{gateway} is true,
@@ -277,6 +280,7 @@ to handle."
                   (list (static-networking (interface interface) (ip ip)
                                            (netmask netmask) (gateway gateway)
                                            (provision provision)
+                                           (requirement requirement)
                                            (name-servers name-servers)))))
 
 (define dhcp-client-service-type
-- 
2.15.0

[bug#29117] [PATCH 2/2] tests: networking: Add tests for OpenvSwitch.

[Marius Bakke]

* gnu/tests/networking.scm (setup-openvswitch,
openvswitch-configuration-service, %openvswitch-os, run-openvswitch-test,
%test-openvswitch): New variables.
---
 gnu/tests/networking.scm | 109 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 108 insertions(+), 1 deletion(-)

diff --git a/gnu/tests/networking.scm b/gnu/tests/networking.scm
index aeee105a1..750125ded 100644
--- a/gnu/tests/networking.scm
+++ b/gnu/tests/networking.scm
@@ -26,7 +26,9 @@
   #:use-module (guix store)
   #:use-module (guix monads)
   #:use-module (gnu packages bash)
-  #:export (%test-inetd))
+  #:use-module (gnu packages networking)
+  #:use-module (gnu services shepherd)
+  #:export (%test-inetd %test-openvswitch))
 
 (define %inetd-os
   ;; Operating system with 2 inetd services.
@@ -135,3 +137,108 @@ port 7, and a dict service on port 2628."
    (name "inetd")
    (description "Connect to a host with an INETD server.")
    (value (run-inetd-test))))
+
+\f
+;;;
+;;; Open vSwitch
+;;;
+
+(define setup-openvswitch
+  #~(let ((ovs-vsctl (lambda (str)
+                       (zero? (apply system*
+                                     #$(file-append openvswitch "/bin/ovs-vsctl")
+                                     (string-tokenize str)))))
+          (add-native-port (lambda (if)
+                             (string-append "--may-exist add-port br0 " if
+                                            " vlan_mode=native-untagged"
+                                            " -- set Interface " if
+                                            " type=internal"))))
+      (and (ovs-vsctl "--may-exist add-br br0")
+           ;; Connect eth0 as an "untagged" port (no VLANs).
+           (ovs-vsctl "--may-exist add-port br0 eth0 vlan_mode=native-untagged")
+           (ovs-vsctl (add-native-port "ovs0")))))
+
+(define openvswitch-configuration-service
+  (simple-service 'openvswitch-configuration shepherd-root-service-type
+                  (list (shepherd-service
+                         (provision '(openvswitch-configuration))
+                         (requirement '(vswitchd))
+                         (start #~(lambda ()
+                                    #$setup-openvswitch))
+                         (respawn? #f)))))
+
+(define %openvswitch-os
+  (simple-operating-system
+   (static-networking-service "ovs0" "10.1.1.1"
+                              #:netmask "255.255.255.252"
+                              #:requirement '(openvswitch-configuration))
+   (service openvswitch-service-type
+            (openvswitch-configuration
+             (package openvswitch)))
+   openvswitch-configuration-service))
+
+(define (run-openvswitch-test)
+  (define os
+    (marionette-operating-system %openvswitch-os
+                                 #:imported-modules '((gnu services herd))))
+
+  (define test
+    (with-imported-modules '((gnu build marionette))
+      #~(begin
+          (use-modules (gnu build marionette)
+                       (ice-9 popen)
+                       (ice-9 rdelim)
+                       (srfi srfi-64))
+
+          (define marionette
+            (make-marionette (list #$(virtual-machine os))))
+
+          (mkdir #$output)
+          (chdir #$output)
+
+          (test-begin "openvswitch")
+
+          ;; Make sure the bridge is created.
+          (test-assert "br0 exists"
+            (marionette-eval
+             '(zero? (system* "ovs-vsctl" "br-exists" "br0"))
+             marionette))
+
+          ;; Make sure eth0 is connected to the bridge.
+          (test-equal "eth0 is connected to br0"
+            "br0"
+            (marionette-eval
+             '(begin
+                (use-modules (ice-9 popen) (ice-9 rdelim))
+                (let* ((port (open-pipe*
+                              OPEN_READ
+                              (string-append #$openvswitch "/bin/ovs-vsctl")
+                              "port-to-br" "eth0"))
+                       (output (read-line port)))
+                  (close-pipe port)
+                  output))
+             marionette))
+
+          ;; Make sure the virtual interface got a static IP.
+          (test-assert "networking has started on ovs0"
+            (marionette-eval
+             '(begin
+                (use-modules (gnu services herd)
+                             (srfi srfi-1))
+                (live-service-running
+                 (find (lambda (live)
+                         (memq 'networking-ovs0
+                               (live-service-provision live)))
+                       (current-services))))
+             marionette))
+
+          (test-end)
+          (exit (= (test-runner-fail-count (test-runner-current)) 0)))))
+
+  (gexp->derivation "openvswitch-test" test))
+
+(define %test-openvswitch
+  (system-test
+   (name "openvswitch")
+   (description "Test a running OpenvSwitch configuration.")
+   (value (run-openvswitch-test))))
-- 
2.15.0

bug#29117: [PATCH 1/2] services: networking: Add a dependency override mechanism to <static-networking>.

[Marius Bakke]

[-- Attachment #1.1: Type: text/plain, Size: 624 bytes --]

Marius Bakke <mbakke@fastmail.com> writes:

> * gnu/services/networking.scm (<static-networking>): Add 'requirement' record.
> (static-networking-shepherd-service): Make UDEV the default requirement.
> (static-networking-service): Expose 'requirement' parameter.
> * doc/guix.texi (Networking Services): Document it.

I finally got around to properly test this patch on Real Systems.  There
was a problem with it: even though 'requirement' was added to the the
<static-networking> record with a default value, #:requirement became a
mandatory parameter when using (static-networking-service).

This fixup commit solves it:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: hmmm --]
[-- Type: text/x-patch, Size: 2457 bytes --]

diff --git a/gnu/services/networking.scm b/gnu/services/networking.scm
index dbfe15a31..c3ba0787c 100644
--- a/gnu/services/networking.scm
+++ b/gnu/services/networking.scm
@@ -147,9 +147,8 @@ fe80::1%lo0 apps.facebook.com\n")
            (default #f))
   (provision static-networking-provision
              (default #f))
-  ;; Wait for udev to be up so that interfaces are actually usable.
   (requirement static-networking-requirement
-               (default '(udev)))
+               (default '()))
   (name-servers static-networking-name-servers    ;FIXME: doesn't belong here
                 (default '())))
 
@@ -160,11 +159,9 @@ fe80::1%lo0 apps.facebook.com\n")
      (let ((loopback? (and provision (memq 'loopback provision))))
        (shepherd-service
 
-        ;; Force the loopback interface to start early.
-        (requirement (if loopback? '() requirement))
-
         (documentation
          "Bring up the networking interface using a static IP address.")
+        (requirement requirement)
         (provision (or provision
                        (list (symbol-append 'networking-
                                             (string->symbol interface)))))
@@ -266,7 +263,9 @@ network interface.")))
 
 (define* (static-networking-service interface ip
                                     #:key
-                                    netmask gateway provision requirement
+                                    netmask gateway provision
+                                    ;; Most interfaces require udev to be usable.
+                                    (requirement '(udev))
                                     (name-servers '()))
   "Return a service that starts @var{interface} with address @var{ip}.  If
 @var{netmask} is true, use it as the network mask.  If @var{gateway} is true,
diff --git a/gnu/services/base.scm b/gnu/services/base.scm
index 7fc8f6aa7..f4681c804 100644
--- a/gnu/services/base.scm
+++ b/gnu/services/base.scm
@@ -1986,6 +1986,7 @@ This service is not part of @var{%base-services}."
         (service static-networking-service-type
                  (list (static-networking (interface "lo")
                                           (ip "127.0.0.1")
+                                          (requirement '())
                                           (provision '(loopback)))))
         (syslog-service)
         (service urandom-seed-service-type)

[-- Attachment #1.3: Type: text/plain, Size: 253 bytes --]


<static-networking>[requirement] now defaults to the empty list, but
(static-network-service ...) adds udev if nothing is specified.

Since there were no complaints about the original patch, I assume the
intent was okay and went ahead and pushed this.

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