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

[Maxim Cournoyer]

Hi Bruno,

Bruno Victal <mirai@makinata.eu> writes:

> On 2023-01-16 18:42, Maxim Cournoyer wrote:
>
>>> Bruno Victal <mirai@makinata.eu> writes:
>>> +;;;
>>> +;;; 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.
>
> I was anticipating that this snippet would get reused (i.e. copy-pasted) by
> other services so I wanted to generalize it before it starts spawning
> its dromedary cousin.
>
> If this isn't preferred, I can drop it and just leave it as
> pascal-case-field-name. (maybe these kinds of procedures could go into
> a new "configuration utils" module with useful auxiliary functions?)
>
>> 
>>> +(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?
>
> I couldn't think of any other word that would essentially describe
> whats amounts to "dropping" the tail of a list/string.
> (here, 'dock': "the practice of cutting off or trimming the tail of an 
> animal")

Ouch.  I'm even more in favor of an alternative name now that I know the
meaning of 'dock' :-).

>>> +;; 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.
>
> They're formatted this way as I find it easier to compare against upstream 
> troff sources.
> If they're reflowed or if the \ escape is dropped then the diffs get larger 
> to compare against
> (most of the changes here are easier to compare using word-diffs)

I think we should take ownership of that text, and update it when it
makes sense without necessarily having to keep every details the same
with upstream (especially for the double space rule and other
conventions); in this view I'd opt to adjust it so the Guix source reads
beautifully rather than for diffing against the orginal source :-).

If this truly provides value to you (to be able to more easily diff
against the troff source), then perhaps how to do so should be detailed
in a comment for the next person to understand how this works and why it
is this way.

>>> +  (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.
>
> Noted.
>
>> 
>> [...]
>> 
>>> +(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?
>
> It shows the path to the config file with `herd configuration vnstatd'.
> It was introduced with 'ebc7de6a1efb702fd0364128cbde19697966c4f4'.

Thanks!  That's news to me :-).

>>> +              (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).
>
> I've thought a bit about the system test though the way vnstat works is that
> it needs to collect some data and it also takes a while for the daemon to 
> flag it
> as ready (~5 minutes). This would be a somewhat slow and non-trivial test to 
> write.

Hm, not nice indeed.  Then I guess it's fine without a test.

One last thing: in general I think we should try to have our services
run in a Linux namespace (unprivileged); have you tried running vnstat
in a wrapper produced with least-authority-wrapper (from (guix
least-authority)) ?

-- 
Thanks,
Maxim