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

[Maxim Cournoyer]

Hello!

Bruno Victal <mirai@makinata.eu> writes:

[...]

> @@ -45,6 +48,10 @@ (define-module (gnu services monitoring)
>              prometheus-node-exporter-web-listen-address
>              prometheus-node-exporter-service-type
 
> +            vnstat-configuration
> +            vnstat-configuration?
> +            vnstat-service-type
> +

Normally, all the accessors are exported, otherwise there's no means to
introspect a vnstat-configuration, which may be useful at the REPL.

[...]

> +
> +;;;
> +;;; vnstat daemon
> +;;;
> +
> +(define* (camelfy-field-name field-name #:key (dromedary? #f))
> +  (match (string-split (symbol->string field-name) #\-)
> +    ((head tail ...)
> +     (string-join (cons (if dromedary? head (string-upcase head 0 1))
> +                        (map string-capitalize tail)) ""))))

I'd name this pascal-case-field-name, and drop the apparently unused
dromerady? argument (fun, though :-)).  If we need it, we could
introduce a 2nd camel-case-field-name.

> +(define (dock-field-name field-name)
> +  "Drop rightmost '?' character"
> +  (let ((str (symbol->string field-name)))
> +    (if (string-suffix? "?" str)
> +        (string->symbol (string-drop-right str 1))
> +        field-name)))

I'm not sure about the meaning of 'dock' in this procedure name.
Perhaps 'strip-trailing-?-character' would be better?

> +(define (vnstat-serialize-string field-name value)
> +  #~(format #f "~a ~s~%"
> +            #$(camelfy-field-name field-name)
> +            #$value))
> +
> +(define vnstat-serialize-integer vnstat-serialize-string)
> +
> +(define (vnstat-serialize-boolean field-name value)
> +  #~(format #f "~a ~a~%"
> +            #$(camelfy-field-name (dock-field-name field-name))
> +            #$(if value 1 0)))
> +
> +(define (vnstat-serialize-alist field-name value)
> +  (generic-serialize-alist string-append
> +                           (lambda (iface val)
> +                             (vnstat-serialize-integer
> +                              (format #f "MaxBW~a" iface) val))
> +                           value))
> +
> +(define-maybe string  (prefix vnstat-))
> +(define-maybe integer (prefix vnstat-))
> +(define-maybe boolean (prefix vnstat-))
> +(define-maybe alist   (prefix vnstat-))
> +
> +;; Documentation strings from vnstat.conf manpage adapted to texinfo.
> +;; vnstat checkout: v2.10, commit b3408af1c609aa6265d296cab7bfe59a61d7cf70
> +(define-configuration vnstat-configuration
> +  (package
> +    (file-like vnstat)
> +    "The vnstat package."
> +    empty-serializer)
> +
> +  (database-dir
> +   (string "/var/lib/vnstat")
> +   "\
> +Specifies the directory where the database is to be stored.
> +A full path must be given and a leading '/' isn't required.")  
> +
> +  (5-minute-hours
> +   (maybe-integer 48)
> +   "\
> +Data retention duration for the 5 minute resolution entries. The 
> configuration
> +defines for how many past hours entries will be stored. Set to @code{-1} for
> +unlimited entries or to @code{0} to disable the data collection of this
> +resolution.")
> +
> +  (64bit-interface-counters
> +   (maybe-integer -2)
> +   "\
> +Select interface counter handling. Set to @code{1} for defining that all 
> interfaces
> +use 64-bit counters on the kernel side and @code{0} for defining 32-bit 
> counter. Set
> +to @code{-1} for using the old style logic used in earlier versions where 
> counter
> +values within 32-bits are assumed to be 32-bit and anything larger is 
> assumed to
> +be a 64-bit counter. This may produce false results if a 64-bit counter is
> +reset within the 32-bits. Set to @code{-2} for using automatic detection 
> based on
> +available kernel datastructures.")

Please reflow these paragraphs so they fit under the 80 characters width
limit.  Perhaps drop the \ escape, as it doesn't provide much and looks
a bit worst, in my opinion.  Each sentence should be separated by two
spaces (that's a Texinfo convention).  This comment applies to all
similar documentation texts.

> +  (always-add-new-interfaces?
> +   (maybe-boolean #t)
> +   "\
> +Enable or disable automatic creation of new database entries for interfaces 
> not
> +currently in the database even if the database file already exists when the
> +daemon is started. New database entries will also get created for new 
> interfaces
> +seen while the daemon is running. Pseudo interfaces lo, lo0 and sit0 are 
> always
> +excluded from getting added.")

The lo, lo0 and sit0 could use a @samp{} or decorator.

[...]

> +(define (vnstat-serialize-configuration config)
> +  (mixed-text-file
> +   "vnstat.conf"
> +   (serialize-configuration config vnstat-configuration-fields)))
> +
> +(define (vnstat-shepherd-service config)
> +  (let ((config-file (vnstat-serialize-configuration config)))
> +    (match-record config <vnstat-configuration> (package pid-file)
> +      (shepherd-service
> +       (documentation "Run vnstatd.")
> +       (requirement `(networking))
> +       (provision '(vnstatd))
> +       (start #~(make-forkexec-constructor
> +                 (list #$(file-append package "/sbin/vnstatd")
> +                       "--daemon"
> +                       "--config" #$config-file)
> +                 #:pid-file #$pid-file))
> +       (stop #~(make-kill-destructor))
> +       (actions
> +        (list (shepherd-configuration-action config-file)

I don't understand what (shepherd-configuration-action config-file)
corresponds to?

> +              (shepherd-action
> +               (name 'reload)
> +               (documentation "Reload vnstatd.")
> +               (procedure
> +                #~(lambda (pid)
> +                    (if pid
> +                        (begin
> +                          (kill pid SIGHUP)
> +                          (format #t
> +                                  "Issued SIGHUP to vnstatd (PID ~a)."
> +                                  pid))
> +                        (format #t "vnstatd is not running.")))))))))))

> +(define (vnstat-account-service config)
> +  (match-record config <vnstat-configuration> (daemon-group daemon-user)
> +    (if (every maybe-value-set? (list daemon-group daemon-user))
> +        (list
> +         (user-group
> +          (name daemon-group)
> +          (system? #t))
> +         (user-account
> +          (name daemon-user)
> +          (group daemon-group)
> +          (system? #t)
> +          (home-directory "/var/empty")
> +          (shell (file-append shadow "/sbin/nologin"))))
> +        '())))
> +
> +(define vnstat-service-type
> +  (service-type
> +   (name 'vnstat)
> +   (description "vnStat network-traffic monitor service.")
> +   (extensions
> +    (list (service-extension shepherd-root-service-type
> +                             (compose list vnstat-shepherd-service))
> +          (service-extension account-service-type
> +                             vnstat-account-service)))
> +   (default-value (vnstat-configuration))))

The rest LGTM (a system test would be nice, but since the Shepherd
service definition is straightforward, it could come later, when the
need arise).

That appears carefully crafted, thank you!  Could you send a v2 with my
above comments addressed, keeping me in CC?

-- 
Thanks,
Maxim