Skip to content
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

Dockerfile: use TARGETPLATFORM #44513

Merged
merged 12 commits into from Dec 15, 2022
Merged

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented Nov 23, 2022

follow-up #43529

Prepare next follow-ups to properly handle cross compilation using TARGETPLATFORM global arg.

This will optimize build for each components as it doesn't depend anymore on dev-base stage with hardcoded linux/amd64 platform which installs cross packages for all platforms for cross compilation:

moby/Dockerfile

Lines 133 to 149 in d6d0e4c

FROM --platform=linux/amd64 runtime-dev-cross-false AS runtime-dev-cross-true
ARG DEBIAN_FRONTEND
# These crossbuild packages rely on gcc-<arch>, but this doesn't want to install
# on non-amd64 systems, so other architectures cannot crossbuild amd64.
RUN --mount=type=cache,sharing=locked,id=moby-cross-true-aptlib,target=/var/lib/apt \
--mount=type=cache,sharing=locked,id=moby-cross-true-aptcache,target=/var/cache/apt \
apt-get update && apt-get install -y --no-install-recommends \
libapparmor-dev:arm64 \
libapparmor-dev:armel \
libapparmor-dev:armhf \
libapparmor-dev:ppc64el \
libapparmor-dev:s390x \
libseccomp-dev:arm64 \
libseccomp-dev:armel \
libseccomp-dev:armhf \
libseccomp-dev:ppc64el \
libseccomp-dev:s390x

In follow-up remove CROSS and DOCKER_CROSSPLATFORMS arg and cross stages in Dockerfile. It also needs changes in hack scripts.

cc @rumpl @vvoland @thaJeztah

Dockerfile Outdated Show resolved Hide resolved
@crazy-max crazy-max force-pushed the dockerfile-targetplatform branch 5 times, most recently from a917f3f to 170e1f6 Compare November 24, 2022 11:02
Dockerfile Show resolved Hide resolved
@crazy-max crazy-max marked this pull request as ready for review November 24, 2022 12:39
@thaJeztah thaJeztah added this to the v-next milestone Nov 24, 2022
@crazy-max crazy-max force-pushed the dockerfile-targetplatform branch 6 times, most recently from 80a5303 to 02276cd Compare November 26, 2022 13:29
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, crap. Had a review that I didn't commit; comments may be outdated by now (haven't checked)

# runc
FROM base AS runc-src
WORKDIR /usr/src/runc
RUN git init . && git remote add origin "https://github.com/opencontainers/runc.git"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, maybe for a follow-up and wait for final release of Dockerfile frontend 1.5?

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
go build -buildmode=pie -o /build/registry-v2-schema1 github.com/docker/distribution/cmd/registry; \
;; \
esac
--mount=type=tmpfs,target=/go/src <<EOT
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this use <<-EOT, if the intent is to remove the first indentation? (we may have to change some spaces to tabs for that)

@crazy-max crazy-max force-pushed the dockerfile-targetplatform branch 2 times, most recently from efc3683 to 0970234 Compare November 27, 2022 13:34
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Dockerfile Outdated
Comment on lines 259 to 266
export CGO_ENABLED=0
export CGO_ENABLED=$([ "$DOCKER_STATIC" = "1" ] && echo "0" || echo "1")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this still needed? (if so; should we fix the containerd makefile to set it automatically?)

I see there's also a SHIM_CGO_ENABLED (but not sure if there's anything we need to do with that one); https://github.com/containerd/containerd/blob/e0be97ccee444c06dc1268df5a99ed80c2cfea76/Makefile#L36

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes needed otherwise ctr is not static and containerd build warns for native builds:

$ docker buildx build --target containerd .
...
#22 [containerd-build 3/3] RUN --mount=from=containerd-src,src=/usr/src/containerd,rw     --mount=type=cache,target=/root/.cache <<EOT (set -e...)
#22 0.862 + bin/ctr
#22 1.921 + bin/containerd
#22 3.631 # github.com/containerd/containerd/cmd/containerd
#22 3.631 /usr/bin/ld: /tmp/go-link-508382094/000019.o: in function `New':
#22 3.631 /go/src/github.com/containerd/containerd/vendor/github.com/miekg/pkcs11/pkcs11.go:77: warning: Using 'dlopen' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
#22 3.712 + bin/containerd-stress
#22 4.438 + bin/containerd-shim
#22 4.885 + bin/containerd-shim-runc-v1
#22 5.423 + bin/containerd-shim-runc-v2
#22 5.767 + binaries
#22 DONE 5.9s

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like something we should fix in containerd; they added the STATIC=1 make var; I think that one should ideally take care of that automatically.

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Comment on lines +377 to +425
xx-verify $([ "$DOCKER_STATIC" = "1" ] && echo "--static") /build/rootlesskit
xx-go build -o /build/rootlesskit-docker-proxy -ldflags="$([ "$DOCKER_STATIC" != "1" ] && echo "-linkmode=external")" ./cmd/rootlesskit-docker-proxy
xx-verify $([ "$DOCKER_STATIC" = "1" ] && echo "--static") /build/rootlesskit-docker-proxy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(not for this PR); perhaps we should have a xx-verify utility that looks for an environment variable, so that we can use XX_STATIC=1 and run xx-verify unconditionally (would save boiler plating in quite some projects now I think) 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes sgtm

Dockerfile Outdated Show resolved Hide resolved
@crazy-max crazy-max force-pushed the dockerfile-targetplatform branch 2 times, most recently from 2a81651 to 7298765 Compare December 15, 2022 13:59
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@thaJeztah
Copy link
Member

some failures in docker-py; could be some race condition, but posting, as we recently merged some fixes for docker ps in #44629 (in case it's related)

____________________________ ContainerTest.test_top ____________________________
tests/integration/models_containers_test.py:424: in test_top
    assert len(top['Processes']) == 1
E   AssertionError: assert 3 == 1
E    +  where 3 = len([['root', '32619', '32599', '0', '14:20', '?', ...], ['root', '32619', '32599', '0', '14:20', '?', ...], ['root', '32619', '32599', '0', '14:20', '?', ...]])
------- generated xml file: /src/bundles/test-docker-py/junit-report.xml -------
=========================== short test summary info ============================
XFAIL tests/integration/api_container_test.py::CreateContainerTest::test_create_with_cpu_rt_options
  CONFIG_RT_GROUP_SCHED isn't enabled
XFAIL tests/integration/api_container_test.py::CreateContainerTest::test_create_with_storage_opt
  Not supported on most drivers
XFAIL tests/integration/api_container_test.py::ContainerTopTest::test_top
  Output of docker top depends on host distro, and is not formalized.
XFAIL tests/integration/api_container_test.py::ContainerTopTest::test_top_with_psargs
  Output of docker top depends on host distro, and is not formalized.
XFAIL tests/integration/api_swarm_test.py::SwarmTest::test_init_swarm_data_path_addr
  Can fail if eth0 has multiple IP addresses
XFAIL tests/integration/api_swarm_test.py::SwarmTest::test_init_swarm_with_log_driver
  This doesn't seem to be taken into account by the engine
SKIPPED [1] tests/integration/api_image_test.py:291: Doesn't work inside a container - FIXME
SKIPPED [1] /src/tests/integration/api_swarm_test.py:31: Test stalls the engine on 1.12.0
= 1 failed, 381 passed, 2 skipped, 3 deselected, 6 xfailed, 2 xpassed in 257.39 seconds =

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Copy link
Contributor

@corhere corhere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah
Copy link
Member

Okay; let's get this one in, thanks! I'll push my cherry-pick PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants