[bug#29732] [PATCH 0/1] Add a DHCP daemon service

[bug#29732] [PATCH 0/1] Add a DHCP daemon service

[Chris Marusich]

Hi,

The following patch adds a DHCP daemon service.  It applies cleanly to
commit 341bddb31542d03bef35b1234e6e9466be337798.

I haven't found the time to add system tests yet, and I'm not sure how
easy it would be to do that (wouldn't such a system test require at
least two marionetted VMs?  Or is it possible to do it with just
one?).  Any thoughts about how to do that would be welcome.

In addition, I decided not to provide any kind of Guile scheme
representation for the daemon's config file, since that was the
quickest way for me to get a working DHCP daemon service.  I think it
makes sense to let a user specify a config file explicitly, like this
patch does.  I also think it would be neat to provide a way to
configure the daemon in Guile scheme, like we do for other services
such as openssh.  Any thoughts on this front would be welcome, too.

Chris Marusich (1):
  services: Add dhcpd-service-type and <dhcpd-configuration>.

 doc/guix.texi               | 17 +++++++++++
 gnu/services/networking.scm | 72 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 89 insertions(+)

-- 
2.15.1

[bug#29732] [PATCH 1/1] services: Add dhcpd-service-type and <dhcpd-configuration>.

[Chris Marusich]

* doc/guix.texi (Networking Services): Document it.
* gnu/services/networking.scm (dhcpd-service-type): Add it.
(dhcpd-configuration, dhcpd-configuration?): Add it.
(dhcpd-configuration-package): Add it.
(dhcpd-configuration-config-file): Add it.
(dhcpd-configuration-ip-version): Add it.
(dhcpd-configuration-run-directory): Add it.
(dhcpd-configuration-lease-file): Add it.
(dhcpd-configuration-pid-file): Add it.
(dhcpd-configuration-interfaces): Add it.
---
 doc/guix.texi               | 17 +++++++++++
 gnu/services/networking.scm | 72 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 89 insertions(+)

diff --git a/doc/guix.texi b/doc/guix.texi
index 64f73b38a..3b62a0578 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -10357,6 +10357,23 @@ Return a service that runs @var{dhcp}, a Dynamic Host Configuration
 Protocol (DHCP) client, on all the non-loopback network interfaces.
 @end deffn
 
+@deffn {Scheme Procedure} dhcpd-service-type
+This type defines a DHCP daemon.  To create a service of this type, you
+must supply a @code{<dhcpd-configuration>}.  For example:
+
+@example
+(service dhcpd-service-type
+         (dhcpd-configuration (config-file (local-file "my-dhcpd.conf"))
+                              (interfaces '("enp2s0f0"))))
+@end example
+
+Here, @file{my-dhcpd.conf} is a local file that defines a valid
+@command{dhcpd} configuration.  Any ``file-like'' object will do here.
+For example, you could use @code{plain-file} instead of
+@code{local-file} if you prefer to embed the @code{dhcpd} configuration
+file in your scheme code.
+@end deffn
+
 @defvr {Scheme Variable} static-networking-service-type
 This is the type for statically-configured network interfaces.
 @c TODO Document <static-networking> data structures.
diff --git a/gnu/services/networking.scm b/gnu/services/networking.scm
index b0c23aafc..d562b7011 100644
--- a/gnu/services/networking.scm
+++ b/gnu/services/networking.scm
@@ -55,6 +55,18 @@
             static-networking-service
             static-networking-service-type
             dhcp-client-service
+
+            dhcpd-service-type
+            dhcpd-configuration
+            dhcpd-configuration?
+            dhcpd-configuration-package
+            dhcpd-configuration-config-file
+            dhcpd-configuration-ip-version
+            dhcpd-configuration-run-directory
+            dhcpd-configuration-lease-file
+            dhcpd-configuration-pid-file
+            dhcpd-configuration-interfaces
+
             %ntp-servers
 
             ntp-configuration
@@ -338,6 +350,66 @@ to handle."
 Protocol (DHCP) client, on all the non-loopback network interfaces."
   (service dhcp-client-service-type dhcp))
 
+(define-record-type* <dhcpd-configuration> dhcpd-configuration
+  make-dhcpd-configuration
+  dhcpd-configuration?
+  (package   dhcpd-configuration-package ;<package>
+             (default isc-dhcp))
+  (config-file   dhcpd-configuration-config-file ;file-like
+                 (default #f))
+  (ip-version dhcpd-ip-version          ; either "4" or "6"
+              (default "4"))
+  (run-directory dhcpd-run-directory
+                 (default "/run/dhcpd"))
+  (lease-file dhcpd-lease-file
+              (default "/var/db/dhcpd.leases"))
+  (pid-file dhcpd-pid-file
+            (default "/run/dhcpd/dhcpd.pid"))
+  (interfaces dhcpd-interfaces ; list of strings, e.g. (list "enp0s25")
+              (default '())))
+
+(define dhcpd-shepherd-service
+  (match-lambda
+    (($ <dhcpd-configuration> package config-file ip-version _ lease-file pid-file interfaces)
+     (when (null-list? interfaces)
+       (error "Must specify at least one interface for DHCP daemon to use"))
+     (unless config-file
+       (error "Must supply a config-file"))
+     (list (shepherd-service
+            (provision '(dhcp-daemon))
+            (documentation "Run the DHCP daemon.")
+            (requirement '(networking))
+            (start #~(make-forkexec-constructor
+                      '(#$(file-append package "/sbin/dhcpd")
+                          #$(string-append "-" ip-version)
+                          "-lf" #$lease-file
+                          "-pf" #$pid-file
+                          "-cf" #$config-file
+                          #$@interfaces)
+                      #:pid-file #$pid-file))
+            (stop #~(make-kill-destructor)))))))
+
+(define dhcpd-activation
+  (match-lambda
+    (($ <dhcpd-configuration> package config-file _ run-directory lease-file _ _)
+     #~(begin
+         (unless (file-exists? #$run-directory)
+           (mkdir #$run-directory))
+         ;; According to the DHCP manual (man dhcpd.leases), the lease
+         ;; database must be present for dhcpd to start successfully.
+         (unless (file-exists? #$lease-file)
+           (with-output-to-file #$lease-file
+             (lambda _ (display ""))))
+         ;; Validate the config.
+         (zero? (system* #$(file-append package "/sbin/dhcpd") "-t" "-cf" #$config-file))))))
+
+(define dhcpd-service-type
+  (service-type
+   (name 'dhcpd)
+   (extensions
+    (list (service-extension shepherd-root-service-type dhcpd-shepherd-service)
+          (service-extension activation-service-type dhcpd-activation)))))
+
 (define %ntp-servers
   ;; Default set of NTP servers. These URLs are managed by the NTP Pool project.
   ;; Within Guix, Leo Famulari <leo@famulari.name> is the administrative contact
-- 
2.15.1

[bug#29732] [PATCH 0/1] Add a DHCP daemon service

[Christopher Baines]

Chris Marusich writes:

> Hi,
>
> The following patch adds a DHCP daemon service.  It applies cleanly to
> commit 341bddb31542d03bef35b1234e6e9466be337798.
>
> I haven't found the time to add system tests yet, and I'm not sure how
> easy it would be to do that (wouldn't such a system test require at
> least two marionetted VMs?  Or is it possible to do it with just
> one?).  Any thoughts about how to do that would be welcome.
>
> In addition, I decided not to provide any kind of Guile scheme
> representation for the daemon's config file, since that was the
> quickest way for me to get a working DHCP daemon service.  I think it
> makes sense to let a user specify a config file explicitly, like this
> patch does.  I also think it would be neat to provide a way to
> configure the daemon in Guile scheme, like we do for other services
> such as openssh.  Any thoughts on this front would be welcome, too.
>
> Chris Marusich (1):
>   services: Add dhcpd-service-type and <dhcpd-configuration>.
>
>  doc/guix.texi               | 17 +++++++++++
>  gnu/services/networking.scm | 72 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 89 insertions(+)

I had a quick look at the patch, and it looks good to me.

A system test would be nice. I think the important thing to test is the
Guix part of the service, so if it's possible to get it to start on a
VM, I think just checking the status of the shepherd service would be a
good test.

I think not providing a Guile configuration approach is fine as well.

Thanks,

Chris

[bug#29732] [PATCH 0/1] Add a DHCP daemon service

[Chris Marusich]

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

Christopher Baines <mail@cbaines.net> writes:

> I had a quick look at the patch, and it looks good to me.

Thank you for taking a look!

> A system test would be nice. I think the important thing to test is the
> Guix part of the service, so if it's possible to get it to start on a
> VM, I think just checking the status of the shepherd service would be a
> good test.

OK.  I'll find time to add something by the end of January.

-- 
Chris

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

[bug#29732] [PATCH 0/1] Add a DHCP daemon service

[Ludovic Courtès]

Heya Chris,

Looks like this patch is pretty much ready.  Would you have time to take
a look again?

TIA,
Ludo’.

Chris Marusich <cmmarusich@gmail.com> skribis:

> Christopher Baines <mail@cbaines.net> writes:
>
>> I had a quick look at the patch, and it looks good to me.
>
> Thank you for taking a look!
>
>> A system test would be nice. I think the important thing to test is the
>> Guix part of the service, so if it's possible to get it to start on a
>> VM, I think just checking the status of the shepherd service would be a
>> good test.
>
> OK.  I'll find time to add something by the end of January.

[bug#29732] [PATCH 1/1] services: Add dhcpd-service-type and <dhcpd-configuration>.

[Clément Lassieur]

Hi Chris, thank you for this service!

A few comments:

Chris Marusich <cmmarusich@gmail.com> writes:

> * doc/guix.texi (Networking Services): Document it.
> * gnu/services/networking.scm (dhcpd-service-type): Add it.
> (dhcpd-configuration, dhcpd-configuration?): Add it.
> (dhcpd-configuration-package): Add it.
> (dhcpd-configuration-config-file): Add it.
> (dhcpd-configuration-ip-version): Add it.
> (dhcpd-configuration-run-directory): Add it.
> (dhcpd-configuration-lease-file): Add it.
> (dhcpd-configuration-pid-file): Add it.
> (dhcpd-configuration-interfaces): Add it.
> ---
>  doc/guix.texi               | 17 +++++++++++
>  gnu/services/networking.scm | 72 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 89 insertions(+)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index 64f73b38a..3b62a0578 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -10357,6 +10357,23 @@ Return a service that runs @var{dhcp}, a Dynamic Host Configuration
>  Protocol (DHCP) client, on all the non-loopback network interfaces.
>  @end deffn
>  
> +@deffn {Scheme Procedure} dhcpd-service-type
> +This type defines a DHCP daemon.  To create a service of this type, you
> +must supply a @code{<dhcpd-configuration>}.  For example:
> +
> +@example
> +(service dhcpd-service-type
> +         (dhcpd-configuration (config-file (local-file "my-dhcpd.conf"))
> +                              (interfaces '("enp2s0f0"))))
> +@end example
> +
> +Here, @file{my-dhcpd.conf} is a local file that defines a valid
> +@command{dhcpd} configuration.  Any ``file-like'' object will do here.
> +For example, you could use @code{plain-file} instead of
> +@code{local-file} if you prefer to embed the @code{dhcpd} configuration
> +file in your scheme code.
> +@end deffn
> +
>  @defvr {Scheme Variable} static-networking-service-type
>  This is the type for statically-configured network interfaces.
>  @c TODO Document <static-networking> data structures.
> diff --git a/gnu/services/networking.scm b/gnu/services/networking.scm
> index b0c23aafc..d562b7011 100644
> --- a/gnu/services/networking.scm
> +++ b/gnu/services/networking.scm
> @@ -55,6 +55,18 @@
>              static-networking-service
>              static-networking-service-type
>              dhcp-client-service
> +
> +            dhcpd-service-type
> +            dhcpd-configuration
> +            dhcpd-configuration?
> +            dhcpd-configuration-package
> +            dhcpd-configuration-config-file
> +            dhcpd-configuration-ip-version
> +            dhcpd-configuration-run-directory
> +            dhcpd-configuration-lease-file
> +            dhcpd-configuration-pid-file
> +            dhcpd-configuration-interfaces
> +
>              %ntp-servers
>  
>              ntp-configuration
> @@ -338,6 +350,66 @@ to handle."
>  Protocol (DHCP) client, on all the non-loopback network interfaces."
>    (service dhcp-client-service-type dhcp))
>  
> +(define-record-type* <dhcpd-configuration> dhcpd-configuration
> +  make-dhcpd-configuration
> +  dhcpd-configuration?
> +  (package   dhcpd-configuration-package ;<package>
> +             (default isc-dhcp))
> +  (config-file   dhcpd-configuration-config-file ;file-like
> +                 (default #f))
> +  (ip-version dhcpd-ip-version          ; either "4" or "6"
> +              (default "4"))

Upstream defaults to "6".  I guess it's because they want to encourage
people to use IPv6.  Maybe we should respect their choice and default to
"6" as well?

> +  (run-directory dhcpd-run-directory
> +                 (default "/run/dhcpd"))
> +  (lease-file dhcpd-lease-file
> +              (default "/var/db/dhcpd.leases"))
> +  (pid-file dhcpd-pid-file
> +            (default "/run/dhcpd/dhcpd.pid"))

I wonder, does it make sense to run two instances of the daemon, one for
IPv4 and another for IPv6?  Would having the IP version included in the
pid file name make it possible?  (dhcpd-4.pid and dhcp-6.pid)

> +  (interfaces dhcpd-interfaces ; list of strings, e.g. (list "enp0s25")
> +              (default '())))

I don't really understand the logic of the indentation and alignment
of this whole block :-).

> +(define dhcpd-shepherd-service
> +  (match-lambda
> +    (($ <dhcpd-configuration> package config-file ip-version _ lease-file pid-file interfaces)
> +     (when (null-list? interfaces)
> +       (error "Must specify at least one interface for DHCP daemon to use"))
> +     (unless config-file
> +       (error "Must supply a config-file"))
> +     (list (shepherd-service
> +            (provision '(dhcp-daemon))
> +            (documentation "Run the DHCP daemon.")
> +            (requirement '(networking))
> +            (start #~(make-forkexec-constructor
> +                      '(#$(file-append package "/sbin/dhcpd")
> +                          #$(string-append "-" ip-version)
> +                          "-lf" #$lease-file
> +                          "-pf" #$pid-file
> +                          "-cf" #$config-file
> +                          #$@interfaces)
> +                      #:pid-file #$pid-file))
> +            (stop #~(make-kill-destructor)))))))
> +
> +(define dhcpd-activation
> +  (match-lambda
> +    (($ <dhcpd-configuration> package config-file _ run-directory lease-file _ _)
> +     #~(begin
> +         (unless (file-exists? #$run-directory)
> +           (mkdir #$run-directory))

mkdir-p I guess

> +         ;; According to the DHCP manual (man dhcpd.leases), the lease
> +         ;; database must be present for dhcpd to start successfully.
> +         (unless (file-exists? #$lease-file)
> +           (with-output-to-file #$lease-file
> +             (lambda _ (display ""))))
> +         ;; Validate the config.
> +         (zero? (system* #$(file-append package "/sbin/dhcpd") "-t" "-cf" #$config-file))))))
> +
> +(define dhcpd-service-type
> +  (service-type
> +   (name 'dhcpd)
> +   (extensions
> +    (list (service-extension shepherd-root-service-type dhcpd-shepherd-service)
> +          (service-extension activation-service-type dhcpd-activation)))))
> +
>  (define %ntp-servers
>    ;; Default set of NTP servers. These URLs are managed by the NTP Pool project.
>    ;; Within Guix, Leo Famulari <leo@famulari.name> is the administrative contact

Also, could you stick to 80 columns?

Thank you!
Clément

[bug#29732] [PATCH 1/1] services: Add dhcpd-service-type and <dhcpd-configuration>.

[Chris Marusich]

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

Hi Clément and Ludo,

Thank you for the review!

Clément Lassieur <clement@lassieur.org> writes:

>> +  (ip-version dhcpd-ip-version          ; either "4" or "6"
>> +              (default "4"))
>
> Upstream defaults to "6".  I guess it's because they want to encourage
> people to use IPv6.  Maybe we should respect their choice and default to
> "6" as well?

Are you sure about this?  The manual (man 8 dhcpd) says:

  COMMAND LINE OPTIONS
       -4     Run as a DHCP server. This is the default and cannot  be
              combined with -6.

       -6     Run as a DHCPv6 server. This cannot be combined with -4.

       -4o6 port
              Participate in the DHCPv4 over DHCPv6 protocol specified
              by RFC 7341.  This associates  a  DHCPv4  and  a  DHCPv6
              server  to  allow  the  v4 server to receive v4 requests
              that were encapsulated in a  v6  packet.   Communication
              between the two servers is done on a pair of UDP sockets
              bound to ::1 port and port + 1.  Both  servers  must  be
              launched using the same port argument.

I agree we should respect the default that upstream sets.  Where did you
see it indicated that upstream sets the default to 6?

In addition, I didn't even know about the -4o6 option, but thankfully my
patch would still allow someone to specify that as the ip-version, too.

> I wonder, does it make sense to run two instances of the daemon, one for
> IPv4 and another for IPv6?  Would having the IP version included in the
> pid file name make it possible?  (dhcpd-4.pid and dhcp-6.pid)

That would make it possible, yes.  However, I think that it should be up
to the user to decide whether or not to run two services.  If they want
to run two (or three) dhcpd services for the different IP protocol
versions, then they should be able to do so easily.  I might need to
adjust the "provision" part of the dhcpd-shepherd-service to allow that,
though.  I'm not sure what happens if two services try to provision the
same "provision" symbol - probably nothing good.

>> +  (interfaces dhcpd-interfaces ; list of strings, e.g. (list "enp0s25")
>> +              (default '())))
>
> I don't really understand the logic of the indentation and alignment
> of this whole block :-).

I try to follow the indentation style mentioned in the Guix manual (see:
(guix) Coding Style), but I may have let a few rough spots slip through.
If you have specific suggestions for how to improve the indentation, I
would be glad to hear them.

>> +(define dhcpd-activation
>> +  (match-lambda
>> +    (($ <dhcpd-configuration> package config-file _ run-directory lease-file _ _)
>> +     #~(begin
>> +         (unless (file-exists? #$run-directory)
>> +           (mkdir #$run-directory))
>
> mkdir-p I guess

Yeah, that's probably better.  I'll see about adjusting it.

>> +         ;; According to the DHCP manual (man dhcpd.leases), the lease
>> +         ;; database must be present for dhcpd to start successfully.
>> +         (unless (file-exists? #$lease-file)
>> +           (with-output-to-file #$lease-file
>> +             (lambda _ (display ""))))
>> +         ;; Validate the config.
>> +         (zero? (system* #$(file-append package "/sbin/dhcpd") "-t" "-cf" #$config-file))))))
>> +
>> +(define dhcpd-service-type
>> +  (service-type
>> +   (name 'dhcpd)
>> +   (extensions
>> +    (list (service-extension shepherd-root-service-type dhcpd-shepherd-service)
>> +          (service-extension activation-service-type dhcpd-activation)))))
>> +
>>  (define %ntp-servers
>>    ;; Default set of NTP servers. These URLs are managed by the NTP Pool project.
>>    ;; Within Guix, Leo Famulari <leo@famulari.name> is the administrative contact
>
> Also, could you stick to 80 columns?

Certainly - looks like I accidentally missed a few long lines.  I've got
a new patch almost ready to share which will improve the formatting and,
most importantly, add a couple tests!

I'll try to clean it up and post it by the end of January.  ;-)

-- 
Chris

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

[bug#29732] [PATCH 1/1] services: Add dhcpd-service-type and <dhcpd-configuration>.

[Clément Lassieur]

Chris Marusich <cmmarusich@gmail.com> writes:

> Hi Clément and Ludo,
>
> Thank you for the review!
>
> Clément Lassieur <clement@lassieur.org> writes:
>
>>> +  (ip-version dhcpd-ip-version          ; either "4" or "6"
>>> +              (default "4"))
>>
>> Upstream defaults to "6".  I guess it's because they want to encourage
>> people to use IPv6.  Maybe we should respect their choice and default to
>> "6" as well?
>
> Are you sure about this?  The manual (man 8 dhcpd) says:
>
>   COMMAND LINE OPTIONS
>        -4     Run as a DHCP server. This is the default and cannot  be
>               combined with -6.
>
>        -6     Run as a DHCPv6 server. This cannot be combined with -4.
>
>        -4o6 port
>               Participate in the DHCPv4 over DHCPv6 protocol specified
>               by RFC 7341.  This associates  a  DHCPv4  and  a  DHCPv6
>               server  to  allow  the  v4 server to receive v4 requests
>               that were encapsulated in a  v6  packet.   Communication
>               between the two servers is done on a pair of UDP sockets
>               bound to ::1 port and port + 1.  Both  servers  must  be
>               launched using the same port argument.
>
> I agree we should respect the default that upstream sets.  Where did you
> see it indicated that upstream sets the default to 6?

Indeed, you are right.  I saw it there:
https://linux.die.net/man/8/dhcpd, but it seems like it's an old version
of the documentation, which contains an error.  The error was fixed in
commit 802fdea172f0d2f59298a69efb7ccfd492feb7f0 of
https://source.isc.org/git/dhcp.git

    - Documentation cleanup
      [ISC-Bugs #23326] Updated References document, several man page updates

Which contains

--8<---------------cut here---------------start------------->8---
modified   server/dhcpd.8
@@ -28,7 +28,7 @@
 .\" Support and other services are available for ISC products - see
 .\" https://www.isc.org for more information or to learn more about ISC.
 .\"
-.\" $Id: dhcpd.8,v 1.34 2011/04/15 21:58:12 sar Exp $
+.\" $Id: dhcpd.8,v 1.35 2011/05/20 13:48:33 tomasz Exp $
 .\"
 .TH dhcpd 8
 .SH NAME
@@ -190,11 +190,11 @@ possible, and listen for DHCP broadcasts on each interface.
 .SH COMMAND LINE OPTIONS
 .TP
 .BI \-4
-Run as a DHCP server.  This cannot be combined with \fB\-6\fR.
+Run as a DHCP server. This is the default and cannot be combined with
+\fB\-6\fR.
 .TP
 .BI \-6
-Run as a DHCPv6 server.  This is the default and cannot be combined
-with \fB\-4\fR.
+Run as a DHCPv6 server. This cannot be combined with \fB\-4\fR.
 .TP
 .BI \-p \ port
 The udp port number on which 
--8<---------------cut here---------------end--------------->8---

> In addition, I didn't even know about the -4o6 option, but thankfully my
> patch would still allow someone to specify that as the ip-version, too.
>
>> I wonder, does it make sense to run two instances of the daemon, one for
>> IPv4 and another for IPv6?  Would having the IP version included in the
>> pid file name make it possible?  (dhcpd-4.pid and dhcp-6.pid)
>
> That would make it possible, yes.  However, I think that it should be up
> to the user to decide whether or not to run two services.  If they want
> to run two (or three) dhcpd services for the different IP protocol
> versions, then they should be able to do so easily.  I might need to
> adjust the "provision" part of the dhcpd-shepherd-service to allow that,
> though.  I'm not sure what happens if two services try to provision the
> same "provision" symbol - probably nothing good.
>
>>> +  (interfaces dhcpd-interfaces ; list of strings, e.g. (list "enp0s25")
>>> +              (default '())))
>>
>> I don't really understand the logic of the indentation and alignment
>> of this whole block :-).
>
> I try to follow the indentation style mentioned in the Guix manual (see:
> (guix) Coding Style), but I may have let a few rough spots slip through.
> If you have specific suggestions for how to improve the indentation, I
> would be glad to hear them.

I don't remember why I said this, sorry about it.

>>> +(define dhcpd-activation
>>> +  (match-lambda
>>> +    (($ <dhcpd-configuration> package config-file _ run-directory lease-file _ _)
>>> +     #~(begin
>>> +         (unless (file-exists? #$run-directory)
>>> +           (mkdir #$run-directory))
>>
>> mkdir-p I guess
>
> Yeah, that's probably better.  I'll see about adjusting it.
>
>>> +         ;; According to the DHCP manual (man dhcpd.leases), the lease
>>> +         ;; database must be present for dhcpd to start successfully.
>>> +         (unless (file-exists? #$lease-file)
>>> +           (with-output-to-file #$lease-file
>>> +             (lambda _ (display ""))))
>>> +         ;; Validate the config.
>>> +         (zero? (system* #$(file-append package "/sbin/dhcpd") "-t" "-cf" #$config-file))))))
>>> +
>>> +(define dhcpd-service-type
>>> +  (service-type
>>> +   (name 'dhcpd)
>>> +   (extensions
>>> +    (list (service-extension shepherd-root-service-type dhcpd-shepherd-service)
>>> +          (service-extension activation-service-type dhcpd-activation)))))
>>> +
>>>  (define %ntp-servers
>>>    ;; Default set of NTP servers. These URLs are managed by the NTP Pool project.
>>>    ;; Within Guix, Leo Famulari <leo@famulari.name> is the administrative contact
>>
>> Also, could you stick to 80 columns?
>
> Certainly - looks like I accidentally missed a few long lines.  I've got
> a new patch almost ready to share which will improve the formatting and,
> most importantly, add a couple tests!

Cool!  This one looks good to me anyway.

> I'll try to clean it up and post it by the end of January.  ;-)

That's very ambitious!  Take your time :-)

[bug#29732] [PATCH 1/1] services: Add dhcpd-service-type and <dhcpd-configuration>.

[Chris Marusich]

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


Hi Clément and Ludo!

Here's a new patch.  It mainly tidies up some formatting, makes it
possible to run more than one version of the DHCP daemon at the same
time (e.g., for IPv4, IPv6, and IPv4 over IPv6), and adds a system test
to verify that the DHCPv4 daemon can start up without error.

I tried adding a test case for DHCPv6, but I ran into complications.
Specifically, I'm not sure how to give an IPv6 address and subnet to the
eth0 interface within the test VM using the static-networking-service.
The DHCPv6 daemon requires the interface to have an IPv6 subnet
configured, so it refuses to run on that interface.

For now, it's nice enough, I think, that we have a DHCPv4 system test!
Please let me know what you think.

-- 
Chris

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-services-Add-dhcpd-service-type-and-dhcpd-configurat.patch --]
[-- Type: text/x-patch, Size: 9924 bytes --]

From 5dd2e6853f1a332e55f3a4ba69b9baf199458fcb Mon Sep 17 00:00:00 2001
From: Chris Marusich <cmmarusich@gmail.com>
Date: Sat, 16 Dec 2017 00:52:42 -0800
Subject: [PATCH] services: Add dhcpd-service-type and <dhcpd-configuration>.

* doc/guix.texi (Networking Services): Document it.
* gnu/services/networking.scm (dhcpd-service-type): Add it.
(dhcpd-configuration, dhcpd-configuration?): Add it.
(dhcpd-configuration-package): Add it.
(dhcpd-configuration-config-file): Add it.
(dhcpd-configuration-version): Add it.
(dhcpd-configuration-run-directory): Add it.
(dhcpd-configuration-lease-file): Add it.
(dhcpd-configuration-pid-file): Add it.
(dhcpd-configuration-interfaces): Add it.
---
 doc/guix.texi               | 17 +++++++
 gnu/services/networking.scm | 80 ++++++++++++++++++++++++++++++
 gnu/tests/networking.scm    | 97 ++++++++++++++++++++++++++++++++++++-
 3 files changed, 193 insertions(+), 1 deletion(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index d825f39e0..1875fb80a 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -10694,6 +10694,23 @@ Return a service that runs @var{dhcp}, a Dynamic Host Configuration
 Protocol (DHCP) client, on all the non-loopback network interfaces.
 @end deffn
 
+@deffn {Scheme Procedure} dhcpd-service-type
+This type defines a DHCP daemon.  To create a service of this type, you
+must supply a @code{<dhcpd-configuration>}.  For example:
+
+@example
+(service dhcpd-service-type
+         (dhcpd-configuration (config-file (local-file "my-dhcpd.conf"))
+                              (interfaces '("enp2s0f0"))))
+@end example
+
+Here, @file{my-dhcpd.conf} is a local file that defines a valid
+@command{dhcpd} configuration.  Any ``file-like'' object will do here.
+For example, you could use @code{plain-file} instead of
+@code{local-file} if you prefer to embed the @code{dhcpd} configuration
+file in your scheme code.
+@end deffn
+
 @defvr {Scheme Variable} static-networking-service-type
 This is the type for statically-configured network interfaces.
 @c TODO Document <static-networking> data structures.
diff --git a/gnu/services/networking.scm b/gnu/services/networking.scm
index 6ac440fd2..7eb031861 100644
--- a/gnu/services/networking.scm
+++ b/gnu/services/networking.scm
@@ -57,6 +57,18 @@
             static-networking-service
             static-networking-service-type
             dhcp-client-service
+
+            dhcpd-service-type
+            dhcpd-configuration
+            dhcpd-configuration?
+            dhcpd-configuration-package
+            dhcpd-configuration-config-file
+            dhcpd-configuration-version
+            dhcpd-configuration-run-directory
+            dhcpd-configuration-lease-file
+            dhcpd-configuration-pid-file
+            dhcpd-configuration-interfaces
+
             %ntp-servers
 
             ntp-configuration
@@ -341,6 +353,74 @@ to handle."
 Protocol (DHCP) client, on all the non-loopback network interfaces."
   (service dhcp-client-service-type dhcp))
 
+(define-record-type* <dhcpd-configuration>
+  dhcpd-configuration make-dhcpd-configuration
+  dhcpd-configuration?
+  (package   dhcpd-configuration-package ;<package>
+             (default isc-dhcp))
+  (config-file   dhcpd-configuration-config-file ;file-like
+                 (default #f))
+  (version dhcpd-configuration-version ;"4", "6", or "4o6"
+              (default "6"))
+  (run-directory dhcpd-configuration-run-directory
+                 (default "/run/dhcpd"))
+  (lease-file dhcpd-configuration-lease-file
+              (default "/var/db/dhcpd.leases"))
+  (pid-file dhcpd-configuration-pid-file
+            (default "/run/dhcpd/dhcpd.pid"))
+  ;; list of strings, e.g. (list "enp0s25")
+  (interfaces dhcpd-configuration-interfaces
+              (default '())))
+
+(define dhcpd-shepherd-service
+  (match-lambda
+    (($ <dhcpd-configuration> package config-file version run-directory
+                              lease-file pid-file interfaces)
+     (when (null-list? interfaces)
+       (error "Must specify at least one interface for DHCP daemon to use"))
+     (unless config-file
+       (error "Must supply a config-file"))
+     (list (shepherd-service
+            ;; Allow users to easily run multiple versions simultaneously.
+            (provision (list (string->symbol
+                              (string-append "dhcpv" version "-daemon"))))
+            (documentation (string-append "Run the DHCPv" version " daemon"))
+            (requirement '(networking))
+            (start #~(make-forkexec-constructor
+                      '(#$(file-append package "/sbin/dhcpd")
+                        #$(string-append "-" version)
+                        "-lf" #$lease-file
+                        "-pf" #$pid-file
+                        "-cf" #$config-file
+                        #$@interfaces)
+                      #:pid-file #$pid-file))
+            (stop #~(make-kill-destructor)))))))
+
+(define dhcpd-activation
+  (match-lambda
+    (($ <dhcpd-configuration> package config-file version run-directory
+                              lease-file pid-file interfaces)
+     (with-imported-modules '((guix build utils))
+       #~(begin
+           (unless (file-exists? #$run-directory)
+             (mkdir #$run-directory))
+           ;; According to the DHCP manual (man dhcpd.leases), the lease
+           ;; database must be present for dhcpd to start successfully.
+           (unless (file-exists? #$lease-file)
+             (with-output-to-file #$lease-file
+               (lambda _ (display ""))))
+           ;; Validate the config.
+           (invoke
+            #$(file-append package "/sbin/dhcpd") "-t" "-cf"
+            #$config-file))))))
+
+(define dhcpd-service-type
+  (service-type
+   (name 'dhcpd)
+   (extensions
+    (list (service-extension shepherd-root-service-type dhcpd-shepherd-service)
+          (service-extension activation-service-type dhcpd-activation)))))
+
 (define %ntp-servers
   ;; Default set of NTP servers. These URLs are managed by the NTP Pool project.
   ;; Within Guix, Leo Famulari <leo@famulari.name> is the administrative contact
diff --git a/gnu/tests/networking.scm b/gnu/tests/networking.scm
index d7d9166fa..171c636e5 100644
--- a/gnu/tests/networking.scm
+++ b/gnu/tests/networking.scm
@@ -29,7 +29,7 @@
   #:use-module (gnu packages bash)
   #:use-module (gnu packages networking)
   #:use-module (gnu services shepherd)
-  #:export (%test-inetd %test-openvswitch))
+  #:export (%test-inetd %test-openvswitch %test-dhcpd))
 
 (define %inetd-os
   ;; Operating system with 2 inetd services.
@@ -243,3 +243,98 @@ port 7, and a dict service on port 2628."
    (name "openvswitch")
    (description "Test a running OpenvSwitch configuration.")
    (value (run-openvswitch-test))))
+
+\f
+;;;
+;;; DHCP Daemon
+;;;
+
+(define minimal-dhcpd-v4-config-file
+  (plain-file "dhcpd.conf"
+              "\
+default-lease-time 600;
+max-lease-time 7200;
+
+subnet 192.168.1.0 netmask 255.255.255.0 {
+ range 192.168.1.100 192.168.1.200;
+ option routers 192.168.1.1;
+ option domain-name-servers 192.168.1.2, 192.168.1.3;
+ option domain-name \"dummy.domain.name.abc123xyz\";
+}
+"))
+
+(define dhcpd-v4-configuration
+  (dhcpd-configuration
+   (config-file minimal-dhcpd-v4-config-file)
+   (version "4")
+   (interfaces '("eth0"))))
+
+(define %dhcpd-os
+  (simple-operating-system
+   (static-networking-service "eth0" "192.168.1.4"
+                              #:netmask "255.255.255.0"
+                              #:gateway "192.168.1.1"
+                              #:name-servers '("192.168.1.2" "192.168.1.3"))
+   (service dhcpd-service-type dhcpd-v4-configuration)))
+
+(define (run-dhcpd-test)
+  (define os
+    (marionette-operating-system %dhcpd-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 "dhcpd")
+
+          (test-assert "pid file exists"
+            (marionette-eval
+             '(file-exists?
+               #$(dhcpd-configuration-pid-file dhcpd-v4-configuration))
+             marionette))
+
+          (test-assert "lease file exists"
+            (marionette-eval
+             '(file-exists?
+               #$(dhcpd-configuration-lease-file dhcpd-v4-configuration))
+             marionette))
+
+          (test-assert "run directory exists"
+            (marionette-eval
+             '(file-exists?
+               #$(dhcpd-configuration-run-directory dhcpd-v4-configuration))
+             marionette))
+
+          (test-assert "dhcpd is alive"
+            (marionette-eval
+             '(begin
+                (use-modules (gnu services herd)
+                             (srfi srfi-1))
+                (live-service-running
+                 (find (lambda (live)
+                         (memq 'dhcpv4-daemon
+                               (live-service-provision live)))
+                       (current-services))))
+             marionette))
+
+          (test-end)
+          (exit (= (test-runner-fail-count (test-runner-current)) 0)))))
+
+  (gexp->derivation "dhcpd-test" test))
+
+(define %test-dhcpd
+  (system-test
+   (name "dhcpd")
+   (description "Test a running DHCP daemon configuration.")
+   (value (run-dhcpd-test))))
-- 
2.17.0


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

[bug#29732] [PATCH 1/1] services: Add dhcpd-service-type and <dhcpd-configuration>.

[Ludovic Courtès]

Hello Chris,

Chris Marusich <cmmarusich@gmail.com> skribis:

> Here's a new patch.  It mainly tidies up some formatting, makes it
> possible to run more than one version of the DHCP daemon at the same
> time (e.g., for IPv4, IPv6, and IPv4 over IPv6), and adds a system test
> to verify that the DHCPv4 daemon can start up without error.
>
> I tried adding a test case for DHCPv6, but I ran into complications.
> Specifically, I'm not sure how to give an IPv6 address and subnet to the
> eth0 interface within the test VM using the static-networking-service.
> The DHCPv6 daemon requires the interface to have an IPv6 subnet
> configured, so it refuses to run on that interface.

OK.

> For now, it's nice enough, I think, that we have a DHCPv4 system test!
> Please let me know what you think.

I agree, nice job!

> From 5dd2e6853f1a332e55f3a4ba69b9baf199458fcb Mon Sep 17 00:00:00 2001
> From: Chris Marusich <cmmarusich@gmail.com>
> Date: Sat, 16 Dec 2017 00:52:42 -0800
> Subject: [PATCH] services: Add dhcpd-service-type and <dhcpd-configuration>.
>
> * doc/guix.texi (Networking Services): Document it.
> * gnu/services/networking.scm (dhcpd-service-type): Add it.
> (dhcpd-configuration, dhcpd-configuration?): Add it.
> (dhcpd-configuration-package): Add it.
> (dhcpd-configuration-config-file): Add it.
> (dhcpd-configuration-version): Add it.
> (dhcpd-configuration-run-directory): Add it.
> (dhcpd-configuration-lease-file): Add it.
> (dhcpd-configuration-pid-file): Add it.
> (dhcpd-configuration-interfaces): Add it.
> ---
>  doc/guix.texi               | 17 +++++++
>  gnu/services/networking.scm | 80 ++++++++++++++++++++++++++++++
>  gnu/tests/networking.scm    | 97 ++++++++++++++++++++++++++++++++++++-

Please mention the gnu/tests/networking.scm changes in the log.

Otherwise LGTM, thank you!

Ludo’.

bug#29732: [PATCH 1/1] services: Add dhcpd-service-type and <dhcpd-configuration>.

[Chris Marusich]

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

ludo@gnu.org (Ludovic Courtès) writes:

>> For now, it's nice enough, I think, that we have a DHCPv4 system test!
>> Please let me know what you think.
>
> I agree, nice job!

Thank you!  I hope this service proves useful to somebody besides just
me!

> Please mention the gnu/tests/networking.scm changes in the log.

Good catch; I've fixed the ChangeLog entry.  I've also added
documentation for <dhcpd-configuration> in the manual, since users
deserve good documentation.  I've committed all this as
f1104d900978900addad731dfec4e0e6e5765fbe.

Thank you both for your helpful review!

-- 
Chris

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