-
Notifications
You must be signed in to change notification settings - Fork 326
Use a distroless based runtime image #18208
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
as there is no risk of pulling these from a public registry
5f38cfe
to
9ffa230
Compare
command=/usr/local/bin/prefix-log /usr/local/bin/redis-server --unixsocket /tmp/redis.sock | ||
command=/usr/local/bin/prefix-log /usr/bin/redis-server --unixsocket /tmp/redis.sock | ||
{% else %} | ||
command=/usr/local/bin/prefix-log /usr/local/bin/redis-server | ||
command=/usr/local/bin/prefix-log /usr/bin/redis-server |
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 should have been its own commit to make this fix more clear, but this is needed because of the changes to how Dockerfile-workers
installs Redis. This fixes the Complement run for workers+Postgres.
The failed Complement run in SQLite is because of the application-defined synapse/synapse/storage/engines/sqlite.py Line 112 in 08c56c3
synapse/synapse/storage/engines/sqlite.py Lines 190 to 204 in 08c56c3
For whatever reason, when queries call that function (as is done by |
Sadly, this might be because of how sqlite is compiled in the builds we're grabbing… That particular call uses
Edit: nevermind, tried executing a simple FTS query with the Python we have in this image and it works |
Alright I figured out the difference! Using the current image:
With the one from this branch:
Which is ultimately what the test is doing: https://github.com/matrix-org/complement/blob/81c0d9ad96823e92cef2a9d146a23bf32d186126/tests/csapi/apidoc_search_test.go#L93 |
I've opened an issue on their side: astral-sh/python-build-standalone#550 |
The Python distribution from uv comes with the SQLite extension compiled without the `ENABLE_FTS3_PARENTHESIS` flag, which Synapse requires for its full-text search queries. So, revert to using the Python from the "python-slim" image, which comes with SQLite compiled with that flag.
Downstream container images that use the Synapse image as a base will break due to the change to distroless.
Exclude pip from the runtime image, and don't copy unused binaries & unused/empty directories. This also requires to go back to using uv in a build stage to install supervisor in the workers image.
as it is useless without a shell anyways
Also add a comment explaining why it's used, and install it
Add back zlib1g, and add libs that uv's python builds statically
Don't append an arch to package names that already have an arch
COPYing a symlink resolves the link, so explicitly make links instead
Instead of manually creating links to the python binary in the final runtime image, copy them from a builder image.
Do this instead of appending package names with ":{arch}", which fails when downloading architecture-independent packages for a non-host arch.
The last round of commits should wrap this up, barring further review. |
@AndrewFerr ftr, I've pushed a patch to python-build-standalone to enable the flag we needed, and it just got merged, so if you want to switch back to the uv-based python install, you could :) |
Now that uv's prebuilt Python uses the `ENABLE_FTS3_PARENTHESIS` flag for its SQLite extension, it is once again usable for Synapse (see astral-sh/python-build-standalone#550). Still skip copying unused binaries & directories into the runtime image.
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 had some comments but not sure all of them still apply because the history is a little windy and I think some of them got handled since ...
docker/Dockerfile-workers
Outdated
| grep '^\w' > /tmp/pkg-list && \ | ||
mkdir -p /tmp/debs && \ | ||
cat /tmp/pkg-list && \ | ||
cd /tmp/debs && \ | ||
xargs apt-get download </tmp/pkg-list | ||
|
||
# Extract the debs for each architecture | ||
RUN \ | ||
mkdir -p /install/var/lib/dpkg/status.d/ && \ | ||
for deb in /tmp/debs/*.deb; do \ | ||
package_name=$(dpkg-deb -I ${deb} | awk '/^ Package: .*$/ {print $2}'); \ | ||
echo "Extracting: ${package_name}"; \ | ||
dpkg --ctrl-tarfile $deb | tar -Ox ./control > /install/var/lib/dpkg/status.d/${package_name}; \ | ||
dpkg --extract $deb /install; \ | ||
done; |
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 not super elegant. Not sure if we can do better though.
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.
FWIW the same strategy is done in stage 2 of docker/Dockerfile
.
prefixer = ['mawk', '-W', 'interactive', f'{{print "{os.environ['SUPERVISOR_PROCESS_NAME']} | "$0; fflush() }}'] | ||
|
||
r_out, w_out = os.pipe() | ||
if os.fork() == 0: | ||
os.close(w_out) | ||
os.dup2(r_out, sys.stdin.fileno()) | ||
os.execvp(prefixer[0], prefixer) | ||
os.close(r_out) | ||
|
||
r_err, w_err = os.pipe() | ||
if os.fork() == 0: | ||
os.close(w_out) | ||
os.close(w_err) | ||
os.dup2(r_err, sys.stdin.fileno()) | ||
os.dup2(sys.stderr.fileno(), sys.stdout.fileno()) | ||
os.execvp(prefixer[0], prefixer) | ||
os.close(r_err) | ||
|
||
os.dup2(w_out, sys.stdout.fileno()) | ||
os.dup2(w_err, sys.stderr.fileno()) | ||
os.execvp(sys.argv[1], sys.argv[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.
I have to say I'd prefer to use the shell for this, as this has really suffered in clarity.
I am also not confident in vouching for its correctness here.
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.
To help with clarity, I'll add some comments to this.
It's a lot more code than a sh
script, but it really just boils down to redirecting output streams.
These changes help with local builds, but aren't needed for distroless
This PR has grown to be quite large, and after having played with it some more, I realize now that the impacts of switching to distroless could be more than originally anticipated (like breaking downstream image builds & environments that expect more tools to be installed). So, closing this until it's potentially revisited later. There are some changes in here that aren't strictly related to distroless, though, which I intend to split into other PRs. |
Based on #18039
Pull Request Checklist
EventStore
toEventWorkerStore
.".code blocks
.(run the linters)