[bug#60788] [PATCH] services: Add vnstat-service-type.

[Bruno Victal <mirai@makinata.eu>]

Hi  Ludo’,

On 2023-04-07 16:22, Ludovic Courtès wrote:
> I think a system test would be nice, we generally require it upfront,
> but since Maxim wrote it can come later, let’s not let it block this
> patch any longer.

Originally I thought it was unfeasible to write a test for this service
since it required network activity within the VM and it seemed to take more
than 10 minutes for it to pick it up.

I did some manual experiments on it much later and I managed to get it down to
approx. between 2 and 3 minutes for a partially automated test.

What's blocking the test from being implemented in Guix is that my (incomplete) test-suite
for this depends on guile 'spawn', which isn't available yet,
For reference, I've attached the test-suite here [1], the plan is to finish it up and
add it to Guix after the 'spawn' issue is resolved. (or perhaps refactor this to use another approach?)

> 
> One comment:
> 
>> +@item @code{database-dir} (default: @code{"/var/lib/vnstat"}) (type: string)
> 
> [...]
> 
>> +@item @code{create-dirs?} (default: @code{#t}) (type: maybe-boolean)
> 
> For consistency, both within this record and with the rest of Guix, I
> suggest avoiding abbreviations.  Since this will be part of the API,
> better fix it now than later.

I should mention that almost all of the field names here are near verbatim
vnstat config-file directives, i.e. a near 1-1 Scheme translation of vnstat config.
This has the benefit that it makes serialization pretty much straightforward.

It's possible to override their names by the use of the custom serializer parameter
but would it be acceptable to leave them as-is?

>> +@item @code{max-bandwidth} (type: maybe-integer)
>> +Maximum bandwidth for all interfaces.  If the interface specific traffic
>> +exceeds the given value then the data is assumed to be invalid and
>> +rejected.  Set to 0 in order to disable the feature.  Value range:
>> +@samp{0}..@samp{50000}
>> +
>> +@item @code{max-bw} (type: maybe-alist)
>> +Same as @var{max-bandwidth} but can be used for setting individual
>> +limits for selected interfaces.  This is an association list of
>> +interfaces as symbols/strings to integer values.  For example,
>> +@lisp
>> +(max-bw `((eth0 .  15000)
>> +          (ppp0 .  10000)))
>> +@end lisp
> 
> Both the naming and semantics are a bit confusing to me.
> 
> How about s/max-bw/per-interface-max-bandwidth/ ?

I found it a bit confusing as well but I'm not too familiar with this part of
the config to comment about it.
I lifted most of the field-names and documentations straight from the manpage.

> Side note: I’d represent interfaces as strings because there’s no
> guarantee they “fit” in a symbol.

Thanks! I'll have this amended in the next revision.


[1]: Listing of vnstat-test.scm

--8<---------------cut here---------------start------------->8---
(define-module (gnu tests vnstat)
  #:use-module (gnu tests)
  #:use-module ((gnu packages networking) #:select (socat vnstat))
  #:use-module (gnu services)
  #:use-module (gnu services networking)
  #:use-module (gnu services monitoring)
  #:use-module (gnu system)
  #:use-module (gnu system vm)
  #:use-module (guix gexp)
  #:use-module (ice-9 format)
  #:export (%test-vnstat))


(define (run-vnstat-test)
  "Run tests in a vm which has vnstat running."

  (define vnstat-config
    (vnstat-configuration
     (max-bandwidth 0)
     (time-sync-wait 0)
     (bandwidth-detection-interval 0)))

  (define os
    (marionette-operating-system
     (simple-operating-system
      (service dhcp-client-service-type)
      (service vnstat-service-type
               vnstat-config)
      (service inetd-service-type
               (inetd-configuration
                (entries
                 (list (inetd-entry
                        (name "discard")
                        (socket-type 'stream)
                        (protocol "tcp")   ;; FIXME: originally this was UDP but port-forwardings hardcodes TCP
                        (wait? #t)
                        (user "nobody")))))))
     #:imported-modules '((gnu services herd))))

  (define forwarded-port 9999)

  (define vm
    (virtual-machine
     (operating-system os)
     ;; Note: port 9 corresponds to "discard" service.
     (port-forwardings `((,forwarded-port . 9)))))   ;; FIXME: Allow UDP forward.

  (define test-timeout (* 60 2))  ; wait for 2 minutes tops.

  (define test
    (with-imported-modules '((gnu build marionette))
      #~(begin
          (use-modules (gnu build marionette)
                       (srfi srfi-64))

          (let ((marionette (make-marionette (list #$vm)))
                (pid-file #$(vnstat-configuration-pid-file vnstat-config)))

            (test-runner-current (system-test-runner #$output))
            (test-begin "vnstat")

            (test-assert "service is running"
              (marionette-eval
               '(begin
                  (use-modules (gnu services herd))
                  (start-service 'vnstatd))
               marionette))

            (test-assert "vnstatd ready"
              (wait-for-file pid-file marionette))

            (test-assert "vnstatd is logging"
              ;; pump garbage into the "discard" service within the vm
              ;; TODO: guile socket client instead? Is it feasible?
              (let* ((socat #$(file-append socat "/bin/socat"))
                     (dest-addr #$(format #f "TCP4:localhost:~d"
                                          forwarded-port))
                     (args `("socat" "-u" "/dev/zero" ,dest-addr))
                     ;; XXX: Guile bug (22/03/2023, Guile 3.0.9)
                     ;;      Fixed in main: <https://issues.guix.gnu.org/61073>
                     #;(output-port (%make-void-port "w"))
                     (garbage-pump-pid (spawn socat args)))

                (let ((retval
                       (marionette-eval
                        '(begin
                           (use-modules (ice-9 popen)
                                        ;(ice-9 rdelim)
                                        (ice-9 match)
                                        (sxml simple)
                                        (sxml xpath))

                           (define selector
                             (let ((xpath '(vnstat interface traffic total)))
                               (compose (node-pos 1) (sxpath xpath))))

                           (let loop ((i 0))
                             (let* ((vnstat #$(file-append vnstat "/bin/vnstat"))
                                    (query-cmd (format #f "~a --xml") vnstat)
                                    (result
                                     #;(call-with-port
                                     (open-input-pipe query-cmd) read-line)
                                     (call-with-port
                                         (open-input-pipe query-cmd) xml->sxml))
                                    (iface-stats (selector result)))
                               (match iface-stats
                                 ((('total ('rx "0") ('tx "0")))
                                  (sleep 1)
                                  (if (< i #$test-timeout)
                                      (loop (+ i 1))
                                      #f))
                                 ((('total ('rx rx) ('tx tx)))
                                  #t)
                                 (_ #f)))))
                        marionette)))
                  ;; shutdown garbage pump
                  (kill garbage-pump-pid SIGTERM)
                  retval)))

            (test-end)))))

  (gexp->derivation "vnstat-test" test))

(define %test-vnstat
  (system-test
   (name "vnstat")
   (description "Basic tests for vnstat service.")
   (value (run-vnstat-test))))
--8<---------------cut here---------------end--------------->8---


Cheers,
Bruno