From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp10.migadu.com ([2001:41d0:2:bcc0::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms5.migadu.com with LMTPS id EGV+DGgeN2PEKAAAbAwnHQ (envelope-from ) for ; Fri, 30 Sep 2022 18:50:48 +0200 Received: from aspmx1.migadu.com ([2001:41d0:2:bcc0::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp10.migadu.com with LMTPS id yJp9C2geN2PjVQEAG6o9tA (envelope-from ) for ; Fri, 30 Sep 2022 18:50:48 +0200 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by aspmx1.migadu.com (Postfix) with ESMTPS id C166211271 for ; Fri, 30 Sep 2022 18:50:47 +0200 (CEST) Received: from localhost ([::1]:33670 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1oeJDq-0004cx-C1 for larch@yhetil.org; Fri, 30 Sep 2022 12:50:46 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:41942) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oeJD8-0004bS-Pd for guix-patches@gnu.org; Fri, 30 Sep 2022 12:50:03 -0400 Received: from debbugs.gnu.org ([209.51.188.43]:43729) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1oeJD8-0005zO-H1 for guix-patches@gnu.org; Fri, 30 Sep 2022 12:50:02 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1oeJD8-0004eU-Cg for guix-patches@gnu.org; Fri, 30 Sep 2022 12:50:02 -0400 X-Loop: help-debbugs@gnu.org Subject: [bug#58123] [PATCH] gnu: services: docker: Add docker-container-service-type Resent-From: =?UTF-8?Q?M=C3=A1ja_?= =?UTF-8?Q?Tom=C3=A1=C5=A1ek?= Original-Sender: "Debbugs-submit" Resent-CC: guix-patches@gnu.org Resent-Date: Fri, 30 Sep 2022 16:50:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 58123 X-GNU-PR-Package: guix-patches X-GNU-PR-Keywords: patch To: Maxime Devos , 58123@debbugs.gnu.org Received: via spool by 58123-submit@debbugs.gnu.org id=B58123.166455659417865 (code B ref 58123); Fri, 30 Sep 2022 16:50:02 +0000 Received: (at 58123) by debbugs.gnu.org; 30 Sep 2022 16:49:54 +0000 Received: from localhost ([127.0.0.1]:42807 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1oeJCy-0004e0-RE for submit@debbugs.gnu.org; Fri, 30 Sep 2022 12:49:53 -0400 Received: from knopi.disroot.org ([178.21.23.139]:39958) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1oeGFQ-00016g-O8 for 58123@debbugs.gnu.org; Fri, 30 Sep 2022 09:40:14 -0400 Received: from localhost (localhost [127.0.0.1]) by disroot.org (Postfix) with ESMTP id E920F4BCFD; Fri, 30 Sep 2022 15:40:10 +0200 (CEST) X-Virus-Scanned: SPAM Filter at disroot.org Received: from knopi.disroot.org ([127.0.0.1]) by localhost (disroot.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id I1XolEzqq_YU; Fri, 30 Sep 2022 15:40:09 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=disroot.org; s=mail; t=1664545209; bh=jCelJf2+Mj2uHbBQzLrx/u0huO1RBab1/hbIRQNWZbQ=; h=From:To:Subject:In-Reply-To:References:Date; b=j/qX+UtFHCQnkiqZ0nV3nb1EExeEIWvwqBC7Ms46fw42os6skOYGLfKC+2AoAdieY Y6y+311rw/n4rh9ot4MWmQmPxk9AS9EfF5eOTgy1xVf/goHw7FIeBY0g9nxDELBlri +N5ibkD3I643QhVunpYP1NNrMRofCeg9zymwBGcZr3DCW1MBngyFqEFzVQs3noII59 pHE4HQ6/bLfa0zNUgW0yRpCTHQuZnXNKgIbGPecgVX+rHlgM6kY/iCE3xE/07dohHL OD06DAQADB7/xuEpSn3JR49e8kE6L7f0B54b5CAV9cyRhusAzYxjCzr4spr7akkIha wRs8rLWyy3KHg== In-Reply-To: References: <87r0zwr9dv.fsf@disroot.org> Date: Fri, 30 Sep 2022 15:40:05 +0200 Message-ID: <87edvt9e16.fsf@disroot.org> Mime-Version: 1.0 Content-Type: text/plain X-Mailman-Approved-At: Fri, 30 Sep 2022 12:49:50 -0400 X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: guix-patches@gnu.org List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+larch=yhetil.org@gnu.org Sender: "Guix-patches" Reply-to: =?UTF-8?Q?M=C3=A1ja_?= =?UTF-8?Q?Tom=C3=A1=C5=A1ek?= X-ACL-Warn: , =?utf-8?q?M=C3=A1ja_Tom=C3=A1=C5=A1ek_via_Guix-patches?= From: guix-patches--- via X-Migadu-Flow: FLOW_IN X-Migadu-Country: US ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=yhetil.org; s=key1; t=1664556647; h=from:from:sender:sender:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:mime-version:mime-version: content-type:content-type:resent-cc:resent-from:resent-sender: resent-message-id:in-reply-to:in-reply-to:references:references: list-id:list-help:list-unsubscribe:list-subscribe:list-post: dkim-signature; bh=2l+ymCIfzRfTArirSgcHSTwhWd90zXWzqtn7mOH6kl4=; b=Dvs/hpsQaC7WMY3JZjY8cwgEo65NAVYXP0z9ZG/t8dhrZs9jd0Qm7/ucHnWS9Sm9UqK29T AyKQQxuSU7iPntfRhxSBFHD4oUeYV66XalAlaZ+jn/vGkTPNoZauCv+fXYTpCHX6aA72lt VXbr1ctbfFSFnnmjv4kBbdninhZuf5lUq+w5yVvt8+c3d6PDjfidO0INFnBXc8zZtYirwn x7s/hwYFmqJqCKnCL3kiHE1VlGxcAPkZx+WIB8TpzcenzHKh3AeMwnW3tchbsUNz/rlivn wtiOgEEcgNum+/hVgaFFE/LXZTYCXRBHAvIv0VRWBAPMRaH5o/efK0Hif/RFdw== ARC-Seal: i=1; s=key1; d=yhetil.org; t=1664556647; a=rsa-sha256; cv=none; b=ac6WFX5cZM7Q+3tfqv4qIeggWLp81V3BDDPWs9iBxSFonSqeJ3Im0nYWqAqkX9ndle+g98 DB8B1bPONCb8YVfQQgvMPSxQCV2s6YLFhekdoqPRSajgNXSAa7ruCT2HSqjUzhTs20SepY yRKvvO29yyiuOyjrTVriHW6t00UYhOdhYcMybFbXVJ2nZjRoaiGOA1BzBx8IzhW8SkJoyo jfPkAtemCxGdac9pZFOnhgjg1byI8apY+cU5M7pvTh7oh0lkVDeAoulUDZXkmQw896FTYw hnQs2eD7Kj0WgiYKe80FWnHAwgXeCT6v3T3ZWDbThdUlsPtTAAVyxMf0MAzi1g== ARC-Authentication-Results: i=1; aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=disroot.org header.s=mail header.b="j/qX+UtF"; dmarc=pass (policy=none) header.from=gnu.org; spf=pass (aspmx1.migadu.com: domain of "guix-patches-bounces+larch=yhetil.org@gnu.org" designates 209.51.188.17 as permitted sender) smtp.mailfrom="guix-patches-bounces+larch=yhetil.org@gnu.org" X-Migadu-Spam-Score: -2.84 Authentication-Results: aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=disroot.org header.s=mail header.b="j/qX+UtF"; dmarc=pass (policy=none) header.from=gnu.org; spf=pass (aspmx1.migadu.com: domain of "guix-patches-bounces+larch=yhetil.org@gnu.org" designates 209.51.188.17 as permitted sender) smtp.mailfrom="guix-patches-bounces+larch=yhetil.org@gnu.org" X-Migadu-Queue-Id: C166211271 X-Spam-Score: -2.84 X-Migadu-Scanner: scn1.migadu.com X-TUID: be6fALms9uFs Hi > On 27-09-2022 19:16, guix-patches--- via wrote: >> >> This patch provides a new service type, which allows the creation of shepherd >> services from docker containers. >> --- >> Hi, >> >> I have written a definition of the docker-container-service-type. It is >> a service that allows you to convert docker containers into shepherd >> services. [...] >> >> There is currently no documentation outside of docstrings, I plan to >> write it, but first I would welcome comments from you, maybe this >> service isn't suitable for guix, as it does imply breaking of the >> declarative guix model, but that goes for docker and flatpak too, so I >> thought I can try it. > > We already have a docker-service-type, why not a > docker-container-service-type, though I wouldn't recommend docker > myself. Can't really document on the docker bits, but some mostly > superficial comments: I was thinking that this isn't exactly a "needed" use and can be thought of as separate, but probably I was wrong. >> >> +(define (pair-of-strings? val) >> + (and (pair val) > > I think you meant 'pair?' here, not 'pair'. > Yes. >> + (string? (car val)) >> + (string? (cdr val)))) >> + >> +(define (list-of-pair-of-strings? val) >> + (list-of pair-of-strings?)) >> + >> +(define-configuration/no-serialization docker-container-configuration >> + (name >> + (symbol '()) >> + "Name of the docker container. Will be used to denote service to Shepherd and must be unique! >> +We recommend, that the name of the container is prefixed with @code{docker-}.") >> + (comment >> + (string "") >> + "A documentation on the docker container.") > > I don't think documentation is countable, maybe > > "Documentation on the Docker container (optional)." > > ? (I don't know what casing is appropriate here). > >> + (image-name >> + (string) >> + "A name of the image that will be used. (Note that the existence of the image >> +is not guaranteed by this daemon.)") >> + (volumes >> + (list-of-pair-of-strings '()) >> + "A list of volume binds. In (HOST_PATH CONTAINER_PATH) format.") > > binds -> bindings and HOST_PATH CONTAINER_PATH -> (HOST-PATH > CONTAINER-PATH) per Scheme conventions. > >> + (ports >> + (list-of-pair-of-strings '()) >> + "A list of port binds. In (HOST_PORT CONTAINER_PORT) or (HOST_PORT CONTAINER_PORT OPTIONS) format. >> +For example, both port bindings are valid: >> + >> +@lisp >> +(ports '((\"2222\" \"22\") (\"21\" \"21\" \"tcp\"))) >> +@end lisp" > > * binds -> bindings > >> + (environments >> + (list-of-pair-of-strings '()) >> + "A list of variable binds, inside the container enviornment. In (VARIABLE VALUE) format.")) > > 'enviornment' -> 'environment', 'variable binds' -> 'environment > variables', '. In' -> ', in'. > >> + (network >> + (string "none") >> + "Network type.") > > Can the available network types be listed or a reference to the Docker > documentation be added, to help users with determining what to set it to? > >> + (additional-arguments >> + (list-of-strings '()) >> + "Additional arguments to the docker command line interface.") >> + (container-command >> + (list-of-strings '()) >> + "Command to send into the container.") >> + (attached? >> + (boolean #t) >> + "Run the container as an normal attached process (sending SIGTERM). >> +Or run the container as a isolated environment that must be stopped with @code{docker stop}. >> + >> +Please verify first, that you container is indeed not attached, otherwise @code{shepherd} might >> +assume the process is dead, even when it is not. >> + >> +You can do that, by first running your container with @code{docker run image-name}. >> + >> +Then check @code{docker ps}, if the command shows beside your container the word @code{running}. >> +Your container is indeed detached, but if it shows @code{starting}, and it doesn't flip to >> +@code{running} after a while, it means that you container is attached, and you need to keep this >> +option turned @code{#t}.")) >> + >> +(define (serialize-volumes config) >> + "Serialize list of pairs into flat list of @code{(\"-v\" \"HOST_PATH:CONTAINER_PATH\" ...)}" >> + (append-map >> + (lambda (volume-bind) >> + (list "-v" (format #f "~?" "~a:~a" volume-bind))) >> + (docker-container-configuration-volumes config))) > > See following about pairs and simplification. > >> + >> +(define (serialize-ports config) >> + "Serialize list of either pairs, or lists into flat list of >> +@code{(\"-p\" \"NUMBER:NUMBER\" \"-p\" \"NUMBER:NUMBER/PROTOCOL\" ...)}" >> + (append-map >> + (lambda (port-bind) >> + (list "-p" (format #f "~?" "~a:~a~^/~a" port-bind))) >> + (docker-container-configuration-ports config))) > > See following about pairs and simplification. > >> + >> +(define (serialized-environments config) >> + "Serialize list of pairs into flat list of @code{(\"-e\" \"VAR=val\" \"-e\" \"VAR=val\" ...)}." >> + (append-map >> + (lambda (env-bind) >> + (list "-e" (format #f "~?" "~a=~a" env-bind))) >> + (docker-container-configuration-environments config))) > > I tried this out in a REPL, but found that it doesn't accept pairs but > 2-element lists: > > scheme@(guile-user)> (format #f "~?" "~a=~a" '("x" . "y")) > ice-9/boot-9.scm:1685:16: In procedure raise-exception: > In procedure length: Wrong type argument in position 1: ("x" . "y") > > Entering a new prompt. Type `,bt' for a backtrace or `,q' to continue. > scheme@(guile-user) [1]> ,q > scheme@(guile-user)> (format #f "~?" "~a=~a" '("x" "y")) > $1 = "x=y" > > Also, the 'format' can be simplified: > > (apply format #f "~a=~a" env-bind) > > >> + >> +(define (docker-container-startup-script docker-cli container-name config) >> + "Return a program file, that executes the startup sequence of the @code{docker-container-shepherd-service}." >> + (let* ((attached? (docker-container-configuration-attached? config)) >> + (image-name (docker-container-configuration-image config)) >> + (volumes (serialize-volumes config)) >> + (ports (serialize-ports config)) >> + (envs (serialize-environments config)) >> + (network (docker-container-configuration-network config)) >> + (additional-arguments (docker-container-configuration-additional-arguments config)) >> + (container-command (docker-container-configuration-container-command config))) >> + (program-file >> + (string-append "start-" container-name "-container") >> + #~(let ((docker (string-append #$docker-cli "/bin/docker"))) >> + (system* docker "stop" #$container-name) >> + (system* docker "rm" #$container-name) > + (apply system* `(,docker >> + "run" >> + ,(string-append "--name=" #$container-name) >> + ;; Automatically remove the container when stopping >> + ;; If you want persistent data, you need to use >> + ;; volume binds or other methods. >> + "--rm" >> + ,(string-append "--network=" #$network) >> + ;; TODO: >> + ;; Write to a cid file the container id, this allows >> + ;; for shepherd to manage container even when the process >> + ;; itself gets detached from the container >> + ,@(if (not #$attached) '("--cidfile" #$cid-file) '()) >> + #$@volumes >> + #$@ports >> + #$@envs >> + #$@additional-arguments >> + ,#$image-name >> + #$@container-command)))))) > > 'system*' can fail, which it does by returning some number (and not by > an exception). I recommend using 'invoke' from (guix build utils) > (which uses exceptions) instead; you may need use-modules + > with-imported-modules to use that module. > > >> + >> +(define (docker-container-shepherd-service docker-cli config) >> + "Return a shepherd-service that runs CONTAINER." >> + (let* ((container-name (symbol->string (docker-container-configuration-name config))) >> + (cid-file (string-append "/var/run/docker/" container-name ".pid")) > > This sounds like ".", ".." and anything containing a "/" or "\x00" would > be invalid container names, I recommend refining the type check for > 'container-name' a little. It also looks like container names must be > unique within a system, that sounds like something to mention in its > docstring to me. > There actually is mention of it! "Name of the docker container. Will be used to denote service to Shepherd and must be unique! We recommend, that the name of the container is prefixed with @code{docker-}." But I agree I need to modify it a bit. >> + (attached? (docker-container-configuration-attached? config))) >> + (shepherd-service >> + (provision (list (docker-container-configuration-name config))) >> + (requirement `(dockerd)) >> + (start #~(make-forkexec-constructor >> + (list #$(docker-container-startup-script docker-cli container-name config)) >> + ;; Watch the cid-file instead of the docker run command, as the daemon can >> + ;; still be running even when the command terminates >> + (if (not #$attached?) >> + #:pid-file #$cid-file))) > > I don't think this does what you want it to do -- when attached, it will > evaluate to #$cid-file, when not, it will evaluate to #:pid-flile. > > Try apply+list instead: > > (apply > make-forkexec-constructor > (list ...) > #$(if $attached > #~() > #~(list #:pid-file #$cid-file))) > > (Changing the staging is not required, though myself I prefer it this way.) > > I recommend writing a system test (in gnu/tests/docker.scm), to prevent > such problems, though I don't know how feasible it would be. > I overlooked some issues, as I was sending the patch after last minute changes and I forgot to check it, thank you for noticing that mistake. >> + (stop (if #$attached? >> + #~(make-kill-destructor) >> + #~(lambda _ >> + (exec-command (list >> + (string-append #$docker-cli "/bin/docker") >> + "stop" #$container-name)) >> + #f)))))) > > Not very familiar with how Shepherd works here, but I think that the > 'return #false' dseserves a command. > Well, I looked through the source code, and this is shepherd's own definition: (define* (make-kill-destructor #:optional (signal SIGTERM)) "Return a procedure that sends SIGNAL to the process group of the PID given as argument, where SIGNAL defaults to `SIGTERM'." (lambda (pid . args) ;; Kill the whole process group PID belongs to. Don't assume that PID is ;; a process group ID: that's not the case when using #:pid-file, where ;; the process group ID is the PID of the process that "daemonized". If ;; this procedure is called, between the process fork and exec, the PGID ;; will still be zero (the Shepherd PGID). In that case, use the PID. (let ((pgid (getpgid pid))) (if (= (getpgid 0) pgid) (kill pid signal) ;don't kill ourself (kill (- pgid) signal))) #f)) So I think that returning #f works. docker stop will send SIGKILL if the container takes too long, so it should succeed. I will send a new patch with this service moved into the docker-service-type and addressed your criticisms. Maya