| Message ID | 87v9l3zjg7.fsf@m4x.org |
|---|---|
| State | New |
| Headers | show |
| Series | [bug#41180] Add cachefilesd service. | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/comparison | success | View comparision |
| cbaines/git branch | success | View Git branch |
| cbaines/applying patch | fail | View Laminar job |
Hello Jean-Baptiste, Thanks for this service! > - I gathered that #~ / #$ kinds of suspends evaluation / forces it -- is > there documentation about this somewhere ? #~ and #$ are related to the Gexp mechanism. It's documented here: https://guix.gnu.org/manual/en/html_node/G_002dExpressions.html. > - There's something that looks like a splat operator (only seen in > conjuction with forcing evaluation in #$@) -- again i'd be interested > in more documentation about this feature -- is this a guix-specific > operator? #$@ is a shortcut for ungexp-splicing. It's also documented in the link above. It can be a bit puzzling at start, don't hesitate to ask some help on #guix channel. > - I don't understand why there are ^L separating services in the scheme > files -- is this necessary? A convention? What purpose does it serve? Yes, see the "Pagination" section in https://mumble.net/~campbell/scheme/style.txt. You can install "emacs-page-break-lines" to replace it by cleaner lines. > Regarding the patch itself: > > - i'm not entirely sure the service belongs to services/linux.scm I think it's fine. > - there are no automated tests (beyond what I have done by hand > locally!), and there's no lint, so I don't really feel confident about > it :) Are there tests for services to alleviate my fears? It would be nice to implement tests along with the new service definition. You can have a look to (gnu tests cups) module for instance. It tests the cups service by spawning a virtual-machine called a "marionette". You could create a (gnu tests cachefilesd) doing a similar job. See "Running the Test Suite" in the info page for more details on how to run the test suite. > - I've copied some other service for modprobing the required kernel > modules before launching the daemon with a one-shot shepherd > service. Frankly i'm not happy about this solution, it seems to me > that it unnecessarily pollutes the shepherd configuration; maybe some > other mechanism (graft?) adjusting the modprobe configuration could be > better (better still, autoload the file). Any guidance would be nice > (including, that this solution is sufficient for now :)) The ideal would be that cachefilesd loads the appropriated module. If this is not possible, we can discuss extending kernel-module-loader-service-type service. But for now I guess it's ok. I hope it answers your questions, I'll review the rest of the service later on. Thanks, Mathieu
Hello, Overall, this looks nice! A few comments below. Note that you can merge this patch with the documentation patch. It would also be nice to add the associated system tests. > +(define-record-type* <cachefilesd-configuration> > + cachefilesd-configuration make-cachefilesd-configuration > + cachefilesd-configuration? > + > + ;; <package-path> > + (cachefilesd cachefilesd-configuration-cachefilesd > + (default cachefilesd)) > + You could write something more concise here by removing empty lines and adding the 'type' comment on the same line. > + (let ((secctx #$(cachefilesd-configuration-secctx config))) > + (if secctx (format port "secctx ~a" secctx))) You can use 'when' for one arm if conditions. > + > + ;; XXX factor this > + (format port "brun ~a%\n" > + #$(number->string > + (cachefilesd-configuration-brun config))) It would indeed be nice to factor it, maybe by creating an association table with the symbol name as CAR and the matching procedure as CDR. Something like: --8<---------------cut here---------------start------------->8--- '(("frun" . cachefilesd-configuration-frun) ("bcull" . cachefilesd-configuration-bcull)) --8<---------------cut here---------------end--------------->8--- then you could iterate on that list. > + (if #$(cachefilesd-configuration-nocull? config) > + (display "nocull\n" port)) Same as above. You can use 'when' or 'unless' instead of "(if test stmt)". > + ;; Make sure the cache directory and pid dir exists "dir" -> "directory". > + ;; XXX shepherd pid file handling: no idea how shepherd does it > + ;; and if it's going to conflict with cachefilesd's Shepherd documentation says: --8<---------------cut here---------------start------------->8--- When PID-FILE is true, it must be the name of a PID file associated with the process being launched; the return value is the PID once that file has been created. If PID-FILE does not show up in less than PID-FILE-TIMEOUT seconds, the service is considered as failing to start. --8<---------------cut here---------------end--------------->8--- So I think you can remove this comment. Thanks, Mathieu
Hi Mathieu, Thanks a lot for taking so much time to help me out and review this patch. Following your previous mail, I'm currently writing a test. I may follow NFS's tests rather than CUPS -- (some of) NFS' tests just checks from within the marionette that the service has been started, while CUPS verifies from the outside that an external service is correctly running. While I do prefer the CUPS test -- verify at the "user level" that the service is provided -- it seems very complex to me to test cachefilesd at a high level with a binary decision and no false positive. Venues for this could be: - maybe an NFS mount with the fsc option when cachefilesd is not activated will fail -- I need to check that; - or maybe mounting some NFS share with fsc, accessing it, and checking afterwards that the cache has been filled by cached data -- this is getting complex though, and probably not a clear-cut scenario. I don't know how high you will set the bar and if a simple 'check that the daemon is running' would be sufficient to you :) Thanks a lot for the various documentation pointers and style recommendation. I really appreciate all of this! I will provide a comprehensive patch with your changes integrated, and a test, as soon as i am happy with it -- or just give up writing the perfect test. Kind regards, Jean-Baptiste
Hello Jean-Baptiste, > Thanks a lot for the various documentation pointers and style > recommendation. I really appreciate all of this! > > I will provide a comprehensive patch with your changes integrated, and a > test, as soon as i am happy with it -- or just give up writing the > perfect test. Great :) I think that checking that the module is loaded, and the cachefilesd service is running is already a first step. You can also check for some properties in the sysfs, but unless you're already there, maybe you don't need to push much further! Thanks, Mathieu
Hello Jean-Baptiste, > Thanks a lot for the various documentation pointers and style > recommendation. I really appreciate all of this! > > I will provide a comprehensive patch with your changes integrated, and a > test, as soon as i am happy with it -- or just give up writing the > perfect test. Any progress on that one :)? Do not hesitate to ask some help here or on #guix if needed. Thanks, Mathieu
Hi, This bug is too old to trigger a CI job. The patch also needed adjustments. Please check Bug#63182 for more progress. Closing this bug. Thanks! Kind regards Felix
From 729d43d541e8dcb41b36a7522ec291b0c3f8dd14 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Note <jean-baptiste.note@m4x.org> Date: Sat, 9 May 2020 15:14:26 +0000 Subject: [PATCH 1/2] gnu: Add cachefilesd-service. * doc/guix.texi (Linux Services): Add a new subsection and document the new service and its configuration. * gnu/services/linux.scm (cachefilesd-service-type): New type. (cachefilesd-configuration): New type. --- gnu/services/linux.scm | 210 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 209 insertions(+), 1 deletion(-) diff --git a/gnu/services/linux.scm b/gnu/services/linux.scm index 12934c2084..810901e0ca 100644 --- a/gnu/services/linux.scm +++ b/gnu/services/linux.scm @@ -42,7 +42,11 @@ earlyoom-configuration-send-notification-command earlyoom-service-type - kernel-module-loader-service-type)) + kernel-module-loader-service-type + + cachefilesd-configuration + cachefilesd-configuration? + cachefilesd-service-type)) ;;; @@ -177,3 +181,207 @@ representation." (compose concatenate) (extend append) (default-value '()))) + + +;;; +;;; cachefilesd. +;;; + +(define-record-type* <cachefilesd-configuration> + cachefilesd-configuration make-cachefilesd-configuration + cachefilesd-configuration? + + ;; <package-path> + (cachefilesd cachefilesd-configuration-cachefilesd + (default cachefilesd)) + + ;; cmdline flags + ;; Boolean + (daemonic? cachefilesd-configuration-daemonic? + (default #t)) + + ;; string + (pid-file cachefilesd-configuration-pid-file + (default "/var/run/cachefilesd.pid")) + + ;; Boolean + (debug? cachefilesd-configuration-debug? + (default #f)) + ;; Boolean + (syslog? cachefilesd-configuration-syslog? + (default #t)) + ;; Boolean + (culling-and-scanning? cachefilesd-configuration-culling-and-scanning? + (default #t)) + + ;; configuration file contents + ;; String + (dir cachefilesd-configuration-dir + (default "/var/cache/fscache")) + + ;; String + (tag cachefilesd-configuration-tag + (default "CacheFiles")) + + ;; String + (secctx cachefilesd-configuration-secctx + (default #f)) + + ;; integers + (brun cachefilesd-configuration-brun + (default 7)) + (frun cachefilesd-configuration-frun + (default 7)) + (bcull cachefilesd-configuration-bcull + (default 5)) + (fcull cachefilesd-configuration-fcull + (default 5)) + (bstop cachefilesd-configuration-bstop + (default 1)) + (fstop cachefilesd-configuration-fstop + (default 1)) + + ;; integer + (culltable cachefilesd-configuration-culltable + (default 12)) + + ;; integer / debug mask + (kernel-debug cachefilesd-configuration-kernel-debug + (default 0)) + + ;; boolean + (nocull? cachefilesd-configuration-nocull? + (default #f)) + ;; Boolean + ;; XXX: This should really be handled in an orthogonal way, for instance as + ;; proposed in <https://bugs.gnu.org/27155>. Keep it internal/undocumented + ;; for now. + (%auto-start? cachefilesd-auto-start? + (default #t))) + +(define (cachefilesd-configuration-file config) + "Return the cachefilesd configuration file corresponding to CONFIG." + (define secctx + (cachefilesd-configuration-dir config)) + + (computed-file + "cachefilesd.conf" + #~(begin + (use-modules (ice-9 match)) + (call-with-output-file #$output + (lambda (port) + (display "# Generated by 'cachefilesd-service'.\n" port) + (format port "dir ~a\n" #$(cachefilesd-configuration-dir config)) + + (let ((secctx #$(cachefilesd-configuration-secctx config))) + (if secctx (format port "secctx ~a" secctx))) + + ;; XXX factor this + (format port "brun ~a%\n" + #$(number->string + (cachefilesd-configuration-brun config))) + (format port "frun ~a%\n" + #$(number->string + (cachefilesd-configuration-frun config))) + (format port "bcull ~a%\n" + #$(number->string + (cachefilesd-configuration-bcull config))) + (format port "fcull ~a%\n" + #$(number->string + (cachefilesd-configuration-fcull config))) + (format port "bstop ~a%\n" + #$(number->string + (cachefilesd-configuration-bstop config))) + (format port "fstop ~a%\n" + #$(number->string + (cachefilesd-configuration-fstop config))) + + (format port "tag ~a\n" #$(cachefilesd-configuration-tag config)) + + (format port "culltable ~a\n" + #$(number->string + (cachefilesd-configuration-culltable config))) + + (if #$(cachefilesd-configuration-nocull? config) + (display "nocull\n" port)) + + (format port "debug ~a\n" + #$(number->string + (cachefilesd-configuration-kernel-debug config))) + + #t))))) + +(define (cachefilesd-activation config) + "Return cachefilesd's activation GEXP for CONFIG." + (with-imported-modules '((guix build utils)) + #~(begin + (use-modules (guix build utils)) + ;; Make sure the cache directory and pid dir exists + (mkdir-p #$(cachefilesd-configuration-dir config)) + (mkdir-p (dirname #$(cachefilesd-configuration-pid-file config)))))) + +(define (cachefilesd-shepherd-service config) + "Return a <shepherd-service> for cachefilesd with CONFIG." + + (define cachefilesdpath + (cachefilesd-configuration-cachefilesd config)) + (define pid-file + (cachefilesd-configuration-pid-file config)) + (define syslog? + (cachefilesd-configuration-syslog? config)) + (define culling-and-scanning? + (cachefilesd-configuration-culling-and-scanning? config)) + (define debug? + (cachefilesd-configuration-debug? config)) + + (define cachefilesd-command + #~(list #$(file-append cachefilesdpath "/sbin/cachefilesd") + #$@(if (cachefilesd-configuration-daemonic? config) '() '("-n")) + ;; XXX shepherd pid file handling: no idea how shepherd does it + ;; and if it's going to conflict with cachefilesd's + #$@(if debug? '("-d") '()) + #$@(if syslog? '() '("-s")) + #$@(if culling-and-scanning? '() '("-N")) + "-p" #$pid-file + "-f" #$(cachefilesd-configuration-file config))) + + (list (shepherd-service + (documentation "Start cachefilesd daemon.") + (requirement (append '(file-systems cachefiles-module) + (if syslog? '(syslogd) '()))) + (provision '(cachefilesd)) + (start #~(make-forkexec-constructor #$cachefilesd-command + #:pid-file #$pid-file)) + (stop #~(make-kill-destructor)) + (auto-start? (cachefilesd-auto-start? config))) + + (shepherd-service + (provision '(cachefiles-module)) + (requirement '(file-systems)) + (modules '((guix build utils))) + (documentation + "Load the cachefiles Linux kernel module.") + (start (with-imported-modules '((guix build utils)) + #~(lambda _ + ;; XXX: duplicated from networking + ;; -- factor this into a modprobe command + ;; XXX: We can't use 'load-linux-module*' here because it + ;; expects a flat module directory. + (setenv "LINUX_MODULE_DIRECTORY" + "/run/booted-system/kernel/lib/modules") + (invoke #$(file-append kmod "/bin/modprobe") + "cachefiles")))) + (one-shot? #t)))) + +(define cachefilesd-service-type + (service-type (name 'cachefilesd) + (description + "Run the CacheFile backend daemon, @command{cachefilesd}.") + (extensions + (list + (service-extension shepherd-root-service-type + cachefilesd-shepherd-service) + (service-extension activation-service-type + cachefilesd-activation))) + (compose concatenate) + (default-value (cachefilesd-configuration)))) -- 2.26.2