[elbe-devel] [PATCH] dockerfile: make elbe-testing run in docker

[Manuel Traut]

Hi John,

On Thu, Jul 19, 2018 at 05:36:01PM +0200, John Ogness wrote:
> Hi Manuel,
> 
> See my comments inline...

thanks for your review! I'll send a v2 soon.

[..]

> > diff --git a/contrib/dockerfile/Makefile b/contrib/dockerfile/Makefile
> > index 1a7c48fa..569deda3 100644
> > --- a/contrib/dockerfile/Makefile
> > +++ b/contrib/dockerfile/Makefile
> > @@ -4,61 +4,49 @@
> >  #
> >  # SPDX-License-Identifier: GPL-3.0-or-later
> >  
> > -IMAGENAME ?= elbe-image
> > -CONTAINERNAME ?= elbe
> > -
> > -ifdef SSH_AUTH_SOCK
> > -	sshauth = -e SSH_AUTH_SOCK=/ssh-auth-sock -v $(SSH_AUTH_SOCK):/ssh-auth-sock
> > -endif
> > +IMAGENAME ?= elbe-devel-image
> > +CONTAINERNAME ?= elbe-devel
> > +KVMGID ?= `ls -n /dev/kvm | awk '{ print $$4 }'`
> > +FUSEGID ?= `ls -n /dev/fuse | awk '{ print $$4 }'`
> 
> I recommend evaluating the expressions during initial Makefile
> expansion, rather than as inline build commands. This makes it a little
> easier to debug because you can see the values expanded in the build
> commands.

ack

> And while at it, I would add UID to the list:
> 
> KVMGID ?= $(shell ls -n /dev/kvm | awk '{ print $$4 }')
> FUSEGID ?= $(shell ls -n /dev/fuse | awk '{ print $$4 }')

i realized that fusegid is not needed, because /dev/fuse is always root:root
but others are allowed to write.

> UID = $(shell id -u)

i'll use:

UID := $(shell id -u)

> >  
> >  # docker commands
> >  build:
> 
> On my test system, there was initially no /dev/fuse. Debugging this took
> longer than it should have because FUSEGID got set to "", which
> eventually caused the docker build to explode. I recommend adding a
> couple tests here:
> 
>         test -n "$(KVMGID)" || ( echo "/dev/kvm not found" && false )

I'll add tests for /dev/kvm, /dev/fuse to be a char-devicenode and a check
for the group.

>         test -n "$(FUSEGID)" || ( echo "/dev/fuse not found" && false )

But skip this.

> > +	sed -e "s#@KVMGID@#$(KVMGID)#g" Dockerfile.in  > Dockerfile.in2
> > +	sed -e "s#@FUSEGID@#$(FUSEGID)#g" Dockerfile.in2  > Dockerfile.in3
> > +	sed -e "s#@USERID@#`id -u`#g" Dockerfile.in3 > Dockerfile
> 
> sed can evaluate multiple expressions in 1 command. It looks a little
> nicer IMHO:
> 
>         sed -e "s#@KVMGID@#$(KVMGID)#g" \
>             -e "s#@FUSEGID@#$(FUSEGID)#g" \
>             -e "s#@USERID@#$(UID)#g" \
>             Dockerfile.in > Dockerfile
> 
> >  	docker build --build-arg http_proxy=$(http_proxy) \
> >  	             --build-arg https_proxy=$(https_proxy) \
> >  	             --build-arg no_proxy=$(no_proxy) \
> >  	             -t $(IMAGENAME) .
> > +	rm Dockerfile Dockerfile.in?
> 
> ... which then reduces the cleanup to:
> 
>         rm Dockerfile
> 
> >  
> > -run:
> > -	docker run \
> > +start:
> > +	docker run --name $(CONTAINERNAME) -d \
> > +		-e container=docker \
> > +		-e http_proxy=$(http_proxy) \
> > +		-e https_proxy=$(https_proxy) \
> > +		-e no_proxy=$(no_proxy) \
> >  		-v $(realpath ../../.):/elbe \
> > -		--env http_proxy=$(http_proxy) \
> > -		--env https_proxy=$(https_proxy) \
> > -		--env no_proxy=$(no_proxy) \
> > -		--device /dev/kvm:/dev/kvm \
> > +		-v `pwd`/../..:/home -w /home \
> 
> I do not understand what you want here.
>
> I suppose it should look like
> this:
> 
>                 -v $(realpath ../..):/elbe \
>                 -v $(realpath ../..):/home -w /home \
> 
> But what is the point of that? You won't have a /home/elbe
> directory. Maybe you meant:
> 
>                 -v $(realpath ../..):/elbe \
>                 -v $(realpath ../../..):/home -w /home \
> 
> with the assumption that the source directory is called elbe. Then there
> would be a /home/elbe. But that is a big assumption.

This code was (accidently) copied from my personal test-environment. It ensured
that elbe-src is available in /elbe and the folder that contains elbe and some
other scripts is in /home that is used as working directory.

I agree that this is not useful for general purpose and suggest:

             -v $(realpath ../../.):/elbe  -w /elbe \

in a v2.

> >  		-v /sys/fs/cgroup:/sys/fs/cgroup:ro \
> >  		--cap-add SYS_ADMIN \
> >  		--security-opt seccomp:unconfined \
> > +		--group-add kvm \
> > +		--device /dev/kvm:/dev/kvm \
> > +		--device /dev/fuse:/dev/fuse \
> >  		--tmpfs /tmp \
> >  		--tmpfs /run \
> >  		--tmpfs /run/lock \
> > -		-e container=docker \
> > -		$(sshauth) \
> > -		-d \
> > -		-ti \
> > -		--group-add kvm \
> > -		--name \
> > -		$(CONTAINERNAME) \
> > -		$(IMAGENAME) \
> > -		/lib/systemd/systemd
> > -
> > -getip:
> > -	docker inspect -f '{{ .NetworkSettings.IPAddress }}' $(CONTAINERNAME)
> > -
> > -start: run getip
> > +		$(IMAGENAME)
> >  
> >  stop:
> > -	docker stop $(CONTAINERNAME)
> > +	-docker stop $(CONTAINERNAME)
> >  
> >  stoprm: stop
> > -	docker rm $(CONTAINERNAME)
> > +	-docker rm $(CONTAINERNAME)
> >  
> > -# ssh related functions
> > -cleanssh:
> > -	IP=$(shell docker inspect -f '{{ .NetworkSettings.IPAddress }}' ${CONTAINERNAME}) ;\
> > -	[[ -n "$${IP}" ]] && ssh-keygen -R $${IP}
> > +clean: stoprm
> > +	-docker rmi $(IMAGENAME)
> >  
> > -connect:
> > -	SBC=$(shell which sbc) ;\
> > -	IP=$(shell docker inspect -f '{{ .NetworkSettings.IPAddress }}' ${CONTAINERNAME}) ;\
> > -	ssh-copy-id elbe@$${IP} ;\
> > -	$$SBC ssh -XA elbe@$${IP}
> > +connect: start
> 
> start as a dependency seems really strange. It means connect will fail
> if it is already started. I would expect the following sequece of
> command to work:
> 
> make build
> make start
> make connect
> make connect
> make connect
> make stop

ack; but i'll change the start target:

start:
  docker ps | grep $(CONTAINERNAME)$$ || \
  docker run --name $(CONTAINERNAME) -d \
  ..

> > +	docker exec -tiu `id -u` $(CONTAINERNAME) /bin/bash
> 
> And if UID is set during expansion, this can be:
> 
>         docker exec -tiu $(UID) $(CONTAINERNAME) /bin/bash

ack

> Also, please add .PHONY targets.

ack

> > diff --git a/contrib/dockerfile/README.md b/contrib/dockerfile/README.md
> > index 58219a4f..20407502 100644
> > --- a/contrib/dockerfile/README.md
> > +++ b/contrib/dockerfile/README.md
> > @@ -12,8 +12,8 @@ devices.
> >  [docker][doc] is an open-source project to easily create lightweight, portable,
> >  self-sufficient containers from any application.
> >  
> > -This is a Dockerfile to generate a elbe development environment for systems
> > -other than debian based.
> > +This is a Dockerfile to generate a elbe development and runtime environment for
> > +systems other than debian based.
> >  
> >  [doc]: https://www.docker.io "Docker Homepage"
> >  [elb]: http://elbe-rfs.org   "ELBE Homepage"
> > @@ -30,13 +30,10 @@ is `elbe-image` and a started container name is `elbe`. This names are
> >  changeable via `IMAGENAME` and `CONTAINERNAME` environment variables.
> >  
> >  * `build`: build the image
> > -* `start` start a container, mounts the elbe git-archive to `/elbe` and gives
> > -  back the ip address
> > +* `start` start a container, mounts the elbe git-archive to `/elbe`
> >  * `stop`: stop a running container
> >  * `stoprm`: stop and remove the container
> > -* `getip`: return ip address of a running container
> > -* `connect`: connect via ssh to the container
> > -* `cleanssh`: remove the used ip address (see `getip`) from your `${HOME}/.ssh/known_host`
> > +* `connect`: attach to a running container
> >  
> >  After `connect` you can find the elbe git repository under `/elbe`.
> >  
> > diff --git a/contrib/dockerfile/adds/supervisord.conf b/contrib/dockerfile/adds/supervisord.conf
> > deleted file mode 100644
> > index f59ce051..00000000
> > --- a/contrib/dockerfile/adds/supervisord.conf
> > +++ /dev/null
> > @@ -1,26 +0,0 @@
> > -; taken by https://github.com/sullof/docker-sshd
> > -; SPDX-License-Identifier: GPL-3.0-or-later
> > -[unix_http_server]
> > -file=/tmp/supervisor.sock ; (the path to the socket file)
> > -
> > -[supervisord]
> > -logfile=/tmp/supervisord.log ; (main log file;default $CWD/supervisord.log)
> > -logfile_maxbytes=50MB ; (max main logfile bytes b4 rotation;default 50MB)
> > -logfile_backups=10 ; (num of main logfile rotation backups;default 10)
> > -loglevel=info ; (log level;default info; others: debug,warn,trace)
> > -pidfile=/tmp/supervisord.pid ; (supervisord pidfile;default supervisord.pid)
> > -nodaemon=false ; (start in foreground if true;default false)
> > -minfds=1024 ; (min. avail startup file descriptors;default 1024)
> > -minprocs=200 ; (min. avail process descriptors;default 200)
> > -
> > -; the below section must remain in the config file for RPC
> > -; (supervisorctl/web interface) to work, additional interfaces may be
> > -; added by defining them in separate rpcinterface: sections
> > -[rpcinterface:supervisor]
> > -supervisor.rpcinterface_factory = supervisor.rpcinterface:make_main_rpcinterface
> > -
> > -[supervisorctl]
> > -serverurl=unix:///tmp/supervisor.sock ; use a unix:// URL for a unix socket
> > -
> > -[program:openssh]
> > -command=/usr/sbin/sshd
> 
> John Ogness