GNU bug report logs - #58123
[PATCH] gnu: services: docker: Add docker-container-service-type

Previous Next

Full log

Message #14 received at 58123 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Mája Tomášek <maya.tomasek <at> disroot.org>,
 58123 <at> debbugs.gnu.org
Subject: Re: [bug#58123] [PATCH] gnu: services: docker: Add
 docker-container-service-type
Date: Fri, 30 Sep 2022 20:47:47 +0200

[Message part 1 (text/plain, inline)]

>>> +
>>> +(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-}."

Oops, didn't notice that.  However, you could insert a check somewhere 
for uniqueness, to avoid accidents.

>>> +     (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.

Not saying it won't work, just that it deserves a comment (though 
apparently I misspelled 'comment'), even if it's only something like 
"return #false as done by 'make-kill-destructor'".

However, 'exec-command' runs 'exec' (replacing the shepherd process), I 
think you need something like 'invoke' or 'system*' instead.

Greetings,
MaximE.

[OpenPGP_0x49E3EE22191725EE.asc (application/pgp-keys, attachment)]

[OpenPGP_signature (application/pgp-signature, attachment)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.