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

[Maxime Devos]

> > > +
> > > +(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
Description: OpenPGP public key

OpenPGP_signature
Description: OpenPGP digital signature