-
Notifications
You must be signed in to change notification settings - Fork 66
contrib: dockerfile: Update to bookworm and drop libvirt compatibility #437
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to work nicely. I do have some questions, and some cruft can be trimmed.
contrib/dockerfile/Makefile
Outdated
-e http_proxy=$(http_proxy) \ | ||
-e https_proxy=$(https_proxy) \ | ||
-e no_proxy=$(no_proxy) \ | ||
-v $(shell git rev-parse --show-toplevel):/usr/elbe -w /usr/elbe\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
realpath
is better than git
contrib/dockerfile/Makefile
Outdated
--security-opt apparmor:unconfined \ | ||
--group-add kvm \ | ||
--device /dev/kvm \ | ||
--device /dev/fuse \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is copied from below, correct? Should be possible to remove.
--rm \ | ||
--interactive \ | ||
--tty \ | ||
$(IMAGENAME) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to deduplicate the two commands
contrib/dockerfile/Makefile
Outdated
start-devel: | ||
docker ps | grep $(CONTAINERNAME)$$ || \ | ||
docker run --name $(CONTAINERNAME) \ | ||
-e container=docker \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean that the start-devel option is unnecessary ? Or is it something else in those 4 lines ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- e container=docker
is unnecessary. At least it's not used by ELBE.
contrib/dockerfile/Makefile
Outdated
--device /dev/kvm \ | ||
--device /dev/fuse \ | ||
--rm \ | ||
--interactive \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A noninteractive container is quite useful, can it be kept?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a Docker expert, but after doing some research, I don't believe using a non-interactive Docker container is beneficial in this particular situation (and I wasn't able to make it work :( ).
In this case, the container is started with a shell process, unlike the previous container which ran systemd and required attaching a shell afterward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be useful to have it running in the background to run programs in it with docker exec
. The entrypoint would just sleep forever.
But we can leave it for now and I'll try to extend it later.
contrib/dockerfile/Dockerfile.in
Outdated
RUN echo "deb http://deb.debian.org/debian bullseye-backports main" >> /etc/apt/sources.list; \ | ||
echo "deb http://deb.debian.org/debian-security bullseye-security main" >> /etc/apt/sources.list | ||
RUN echo "deb http://deb.debian.org/debian bookworm-backports main" >> /etc/apt/sources.list; \ | ||
echo "deb http://deb.debian.org/debian-security bookworm-security main" >> /etc/apt/sources.list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These sources can probably be dropped completely.
The packages in bookworm are fine without backports.
The security repository is already preconfigured, leading to (red) warnings:
W: Target Packages (main/binary-amd64/Packages) is configured multiple times in /etc/apt/sources.list:2 and /etc/apt/sources.list.d/debian.sources:2
W: Target Packages (main/binary-all/Packages) is configured multiple times in /etc/apt/sources.list:2 and /etc/apt/sources.list.d/debian.sources:2
contrib/dockerfile/Makefile
Outdated
-e http_proxy=$(http_proxy) \ | ||
-e https_proxy=$(https_proxy) \ | ||
-e no_proxy=$(no_proxy) \ | ||
-v $(shell git rev-parse --show-toplevel):/usr/elbe -w /usr/elbe\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using /usr
for this breaks the is_devel
logic from elbepack/version.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was the whole point, when running the container with start-devel, you force the container to use the elbe repository you're modifying and not the packaged one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that I get. But putting it at /usr/
might confuse ELBE.
Can you put it at /var/cache/elbe/devel
instead? This it consistent with the initvm devel mode.
The current container is running debian bullseye and is starting systemd to run libvirtd in background. This container is broken since some times. This commit updates the dockerfile to run interactively, libvirt isn't supported in this version. There are two launch options, start to run in production which will use the packaged version of Elbe and start-devel which will use the version of Elbe currently being used by the repository. Signed-off-by: Thomas Bonnefille <[email protected]>
The current container is running debian bullseye and is starting systemd to run libvirtd in background. This container is broken since some times.
This commit updates the dockerfile to run interactively, libvirt isn't supported in this version.
There are two launch options, start to run in production which will use the packaged version of Elbe and start-devel which will use the version of Elbe currently being used by the repository.