-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Update dockerfiles to do staged builds #19952
base: master
Are you sure you want to change the base?
Changes from all commits
b288a99
d910457
5e61a87
1cb7f15
1e2a34b
edf701d
e153fa4
939af50
21c9591
74cc236
ef23450
1e31a68
7ea1005
0b85785
0f81931
6443892
1678fbd
2bdcd69
da5c8a8
07f8c02
803314c
87e2373
7127dc2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,26 +1,14 @@ | ||
{% set prefix = DEFAULT_CONTAINER_REGISTRY %} | ||
{% from "dockers/dockerfile-macros.j2" import install_debian_packages, install_python_wheels, copy_files %} | ||
{% if CONFIGURED_ARCH == "armhf" and (MULTIARCH_QEMU_ENVIRON == "y" or CROSS_BUILD_ENVIRON == "y") %} | ||
FROM --platform=linux/arm/v7 {{ prefix }}debian:bookworm | ||
ARG BASE=--platform=linux/arm/v7 {{ prefix }}debian:bookworm | ||
{% elif CONFIGURED_ARCH == "arm64" and (MULTIARCH_QEMU_ENVIRON == "y" or CROSS_BUILD_ENVIRON == "y") %} | ||
FROM --platform=linux/arm64 {{ prefix }}debian:bookworm | ||
ARG BASE=--platform=linux/arm64 {{ prefix }}debian:bookworm | ||
{% else %} | ||
FROM {{ prefix }}{{DOCKER_BASE_ARCH}}/debian:bookworm | ||
ARG BASE={{ prefix }}{{DOCKER_BASE_ARCH}}/debian:bookworm | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @saiarcot895 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. I want to keep the focus of this PR on unblocking Docker upgrades, but I had to bring in some space optimization stuff (see the |
||
{% endif %} | ||
|
||
# Clean documentation in FROM image | ||
RUN find /usr/share/doc -depth \( -type f -o -type l \) ! -name copyright | xargs rm || true | ||
|
||
# Clean doc directories that are empty or only contain empty directories | ||
RUN while [ -n "$(find /usr/share/doc -depth -type d -empty -print -exec rmdir {} +)" ]; do :; done && \ | ||
rm -rf \ | ||
/usr/share/man/* \ | ||
/usr/share/groff/* \ | ||
/usr/share/info/* \ | ||
/usr/share/lintian/* \ | ||
/usr/share/linda/* \ | ||
/var/cache/man/* \ | ||
/usr/share/locale/* | ||
FROM $BASE AS base | ||
|
||
# Make apt-get non-interactive | ||
ENV DEBIAN_FRONTEND=noninteractive | ||
|
@@ -47,6 +35,8 @@ RUN apt update && \ | |
python-is-python3 \ | ||
vim-tiny \ | ||
rsyslog \ | ||
# Install rsync for copying over only changes between layers | ||
rsync \ | ||
# Install redis-tools | ||
redis-tools \ | ||
# common dependencies | ||
|
@@ -98,17 +88,14 @@ RUN apt-get -y purge \ | |
{{ install_debian_packages(docker_base_bookworm_debs.split(' ')) }} | ||
{%- endif %} | ||
|
||
# Clean up apt | ||
# Remove /var/lib/apt/lists/*, could be obsoleted for derived images | ||
RUN apt-get clean -y && \ | ||
apt-get autoclean -y && \ | ||
apt-get autoremove -y && \ | ||
rm -rf /var/lib/apt/lists/* /tmp/* ~/.cache | ||
|
||
COPY ["etc/rsyslog.conf", "/etc/rsyslog.conf"] | ||
COPY ["etc/rsyslog.d/*", "/etc/rsyslog.d/"] | ||
COPY ["root/.vimrc", "/root/.vimrc"] | ||
|
||
RUN ln /usr/bin/vim.tiny /usr/bin/vim | ||
|
||
COPY ["etc/supervisor/supervisord.conf", "/etc/supervisor/"] | ||
|
||
FROM scratch | ||
|
||
COPY --from=base / / | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why docker-base use COPY but other dockers use RUN rsync? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
While it might be technically possible to run
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
{% from "dockers/dockerfile-macros.j2" import install_debian_packages, install_python_wheels, copy_files %} | ||
FROM docker-base-bookworm-{{DOCKER_USERNAME}}:{{DOCKER_USERTAG}} | ||
ARG BASE=docker-base-bookworm-{{DOCKER_USERNAME}}:{{DOCKER_USERTAG}} | ||
|
||
FROM $BASE as base | ||
|
||
## Make apt-get non-interactive | ||
ENV DEBIAN_FRONTEND=noninteractive | ||
|
@@ -49,10 +51,17 @@ COPY ["files/readiness_probe.sh", "/usr/bin/"] | |
COPY ["files/container_startup.py", "/usr/share/sonic/scripts/"] | ||
|
||
## Clean up | ||
RUN apt-get purge -y \ | ||
python3-dev \ | ||
build-essential && \ | ||
apt-get clean -y && \ | ||
apt-get autoclean -y && \ | ||
apt-get autoremove -y && \ | ||
rm -rf /debs /python-wheels ~/.cache | ||
|
||
{%- if CONFIGURED_ARCH == "armhf" or CONFIGURED_ARCH == "arm64" %} | ||
RUN apt-get purge -y \ | ||
libxslt-dev \ | ||
libz-dev | ||
{%- endif %} | ||
|
||
RUN apt-get purge -y \ | ||
python3-dev \ | ||
build-essential | ||
|
||
FROM $BASE | ||
|
||
RUN --mount=type=bind,from=base,target=/changes-to-image rsync -axAX --no-D --exclude=/sys --exclude=resolv.conf /changes-to-image/ / | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you have a test result that shows the how much the disk saves after this PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Taking a I need to resolve new merge conflicts, so I can get updated numbers after doing 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.
Why bullseye needs this option but bookworm doesn't?
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 was needed with an older version of changes that included a Docker daemon version upgrade, but isn't needed anymore. It may be needed in the future when the upgrade is done.
There was previously a commit to upgrade Docker to version 25, for the purposes of using containerd image store. With that upgrade, docker-in-docker in the Bullseye slave container did not start up because of a ulimit error. In the Bullseye slave container, the ulimit for the max open files was set to 1048576:1048576 (as in, 1048576 for both the soft and hard limit). However, Docker 25 and newer lowered just the hard limit to 524288 in moby/moby@c8930105b. This caused the soft limit to be higher than the hard limit, which is an issue, and so starting up docker failed. Based on local testing, the Bookworm slave container there appeared to be not affected because this ulimit was 1024:1048576, and so even lowering just the hard limit to 524288 would be fine.
Rechecking my testing now, when starting up a slave container locally, I see that both Bullseye and Bookworm have 1048576:1048576; I'm not sure what changed to also cause Bookworm to be potentially affected.
This issue affects us only after we upgrade to Docker 25 or newer; we're still on Docker 24 for now. I can either keep this change or take it out of the PR.