-
Notifications
You must be signed in to change notification settings - Fork 5.9k
feature - Build alternate ubuntu images #7691
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
Conversation
10462fd
to
78509e5
Compare
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.
Overall LGTM
@@ -208,7 +214,7 @@ jobs: | |||
strategy: | |||
fail-fast: false | |||
matrix: | |||
base_image: ['nikolaik'] | |||
base_image: ['nikolaik', 'ubuntu'] |
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.
Should we just remove nikolaik
- or do you think we can keep both and using it for a while and if everything goes well in ubuntu, we do the switch then?
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.
Good question. So the concern with changing over completely is that it might have unforeseen impact, either on the runtime image behavior itself or for people using the build process with other custom base images.
Personally long term I could see making ubuntu the default and phasing the other out as you suggest, this PR is just avoiding committing to that.
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.
Would python:3.12
work? It's also debian so it's presumably more compatible with the nikolaik 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.
@li-boxuan Actually, it wouldn't work exactly because it's Debian-based, since the goal is to fix the security findings. If that were possible, we could just stay with nikolaik base and run upgrades as was tried first.
To show what I mean, for instance looking at a run of trivy image --severity HIGH,CRITICAL --scanners vuln python:3.12
python:3.12 (debian 12.10)
==========================
Total: 101 (HIGH: 96, CRITICAL: 5)
Zooming in on one of these criticals, the Debian page for CVE-2023-52425 shows that in the stable distro (Bookworm) there is no upgrade available. There happens to be a patch in the older distro Bullseye, but that's not true in general, for instance the CVE's associated with the package linux-libc-dev
.
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.
Ah I see, thanks
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.
BTW its worth noting that Debian Trixie will freeze in a few months and move from the current Testing to Stable, so possibly there will be a Debian with newer packages at that point. If we're lucky we'll be able to consider the option of just using the upgraded Debian to meet the need instead of deciding if we want to make Ubuntu the default.
I'll have to troubleshoot the runtime test failures, looks like |
@tofarr It does slightly better, probably because Bookworm's stock image starts out larger than Ubuntu's, but most of the space we add ourselves. runtime:78509e5-nikolaik - 10.5 gb |
.github/workflows/ghcr-build.yml
Outdated
|
||
TEST_RUNTIME=docker \ | ||
SANDBOX_USER_ID=$(id -u) \ |
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.
Edit: This user id is for file permissions when mounting a volume and needs to stay that way
For some reason, a lot of our docs suggest using id -u
for the user_id. When running in Docker mode, that makes no sense to me.
But the effect when running from Ubuntu using an Ubuntu image is: It tries to create an openhands
user with uid 1000, the same id as the ubuntu
user. This fails. Therefore, to use the Ubuntu runtime, we should set SANDBOX_USER_ID to some other number.
It's confusing for our users to have to know that, and we should revisit it - maybe see if we can eliminate the need to specify a uid at all.
As a workaround I tried to just remove the ubuntu
user in the image build or change its ID, but wasn't able to get that working.
4c5b9e7
to
9d48b70
Compare
'VSCODE_PORT': str(self._vscode_port), | ||
'PIP_BREAK_SYSTEM_PACKAGES': '1', |
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 gets around this issue where we can't pip install in Ubuntu. There may be better ways we can look at.
$ pip install requests
error: externally-managed-environment
× This environment is externally managed
╰─> To install Python packages system-wide, try apt install
python3-xyz, where xyz is the package you are trying to
install.
If you wish to install a non-Debian-packaged Python package,
create a virtual environment using python3 -m venv path/to/venv.
Then use path/to/venv/bin/python and path/to/venv/bin/pip. Make
sure you have python3-full installed.
If you wish to install a non-Debian packaged Python application,
it may be easiest to use pipx install xyz, which will manage a
virtual environment for you. Make sure you have pipx installed.
See /usr/share/doc/python3.12/README.venv for more information.
note: If you believe this is a mistake, please contact your Python installation or OS distribution provider. You can override this, at the risk of breaking your Python installation or OS, by passing --break-system-packages.
hint: See PEP 668 for the detailed specification.
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.
Curious, why was this necessary when it wasn't in Debian?
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.
@enyst Good question, I think it's because the official Python docker image (used by Nikolaik) installs python from source rather than with the APT package.
https://github.com/docker-library/python/blob/master/Dockerfile-linux.template
I think if we followed this pattern in Ubuntu we could also avoid it, that's an option. Generally it's probably not great to assemble the Python installation within the OpenHands build process, we could build an image with node+python in a separate repo maybe.
@@ -293,6 +294,8 @@ def _init_container(self): | |||
self.container = self.docker_client.containers.run( | |||
self.runtime_container_image, | |||
command=command, | |||
# Override the default 'bash' entrypoint because the command is a binary. | |||
entrypoint=[], |
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.
Oddly this specifically is needed in Ubuntu when the default ubuntu
user has been removed - but the updated config seems more correct all around.
End-user friendly description of the problem this fixes or functionality that this introduces.
Add a new docker image option based on Ubuntu
Give a summary of what the PR does, explaining any non-trivial design decisions.
We are trying to reduce the vulnerabilities found in image scans using SCA tools like trivy. Unfortunately, the current Debian Stable (Bookworm) which our
nikolaik
bases image use has vulnerabilities with no patches available showing up.This PR introduces Ubuntu 24.04 to address this but retains the existing
nikolaik
build as the default.For reference, here is the nikolaik dockerfile template:
https://github.com/nikolaik/docker-python-nodejs/blob/main/templates/debian.Dockerfile
Other changes
Removing the apt-get upgrade (which is done by upstream images regularly)
Adding another cache dir to to the ci config.
Removing several extensions dirs from the openvscode-server install because they show up as false positives (extension version is confused for npm package of the same name. These are handlebars, pug, json, diff, grunt, ini, npm.
Docker template logic
Since the Ubuntu base image does not have python and node installed by default, there are commands added to install them. Right now these activate when base_image contains "ubuntu", but perhaps people might try to configure a custom image with that in the name. Should we introduce another flag to toggle this? I'm not sure what the interface should be, maybe this is ok for now.
Link of any specific issues this addresses.
To run this PR locally, use the following command: