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

Enhanced Dockerfile for cross compilation #43529

Closed
wants to merge 24 commits into from
Closed

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented Apr 26, 2022

follow-up #43474 (comment)
closes #43492
closes #43613
closes #41590
closes #41614 (should be already fixed atm)
fixes #41862
fixes #41519
fixes #38307 (make binary instead of make all)
fixes #39380

*- What I did

Better support for cross compilation so we can rely on --platform flag of buildx for a seamless integration. This removes unnecessary extra cross logic in the Dockerfile as well as reducing hack scripts. Tried my best to reduce the footprint of changes but modifying one bit in the Dockerfile involves other changes in ./hack scripts. Non-sandboxed build invocation is still supported.

dev stages have been updated accordingly to changes for cross comp as well as linked tools (swagger, tomll, gotestsum, ...)

frozen-images stage doesn't use the download-frozen-image-v2.sh anymore so we can effectively use TARGETPLATFORM from global scope. The test util has been updated accordingly. Will be able to fix:

fixes #42701
fixes #31320
closes #31435

In a follow-up we can remove download-frozen-image-v2.sh script but needs to look first at Dockerfile.e2e which seems not used anymore in our pipeline.

vpnkit stage only supports linux/amd64 and linux/arm64 platforms when building dev image and will crash if we try building against another platform. With this change we can still build the dev image against any platform using dummy scratch base.

pin criu for better reproducibility and build from source so we can use it across any platform. will also fix issues with repo mirror we often encounter: #43682 (comment)

containerd, runc, tini and rootlesskit builds in Dockerfile are limited to host platform and could not be cross-built for other platforms. This change allows to build against any platforms if we want to smoke test in a follow-up but also enhance e2e tests for linux and windows in our pipeline. This also introduced DOCKER_LINKMODE to be able to build dynamic or static binaries.

To add support for riscv64 builds we need crossbuild packages for riscv64 but current golang image with debian bullseye does not support it. Ubuntu 22.04 supports riscv64 but unfortunately drops support for armel arch. Therefore we need a multi base image that will be picked up based on the target platform we want to build.

The current bake definition has been updated to take the changes into account as well as the ci gha workflow.

cc @tonistiigi @thaJeztah @cpuguy83 @tianon

- How to verify it

# build binaries for the current host platform
# output to ./bundles/binary by default
docker buildx bake

# build binaries for the current host platform
# output to ./bin
DESTDIR=./bin docker buildx bake

# build dynamically linked binaries
# output to ./bundles/dynbinary by default
DOCKER_LINKMODE=dynamic docker buildx bake

# build binaries for all supported platforms
docker buildx bake binary-cross

# build binaries for a specific platform
docker buildx bake --set *.platform=linux/arm64

# build all for the current host platform (binaries + containerd, runc, tini, ...)
# output to ./bundles/all by default
docker buildx bake all

# build all for the current host platform
# output to ./bin
DESTDIR=./bin docker buildx bake all

# build all for all supported platforms
docker buildx bake all-cross

- Description for the changelog

  • Enhanced Dockerfile for cross compilation

Dockerfile.cross Outdated Show resolved Hide resolved
Dockerfile.cross Outdated Show resolved Hide resolved
Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

Is the support for multiple distros and static/dynamic a requirement?

Dockerfile.cross Outdated Show resolved Hide resolved
Dockerfile.cross Outdated Show resolved Hide resolved
Dockerfile.cross Outdated Show resolved Hide resolved
Dockerfile.cross Outdated Show resolved Hide resolved
Dockerfile.cross Outdated Show resolved Hide resolved
Dockerfile.cross Outdated Show resolved Hide resolved
@crazy-max
Copy link
Member Author

Is the support for multiple distros and static/dynamic a requirement?

Not necessary imo so if debian variant is the one we want to rely on, I can remove the BUILD_VARIANT logic.

@cpuguy83
Copy link
Member

What's the goal of this? Who is going to use it?
The only purpose the current cross target serves, to my knowledge, is to make sure we don't break compilation on other platforms... maybe it's used for static builds?

I'm all for refactoring/rewriting our build system, I don't think anyone really likes it.
That said, the caveat I would say here is that the build system should have no dependency on docker or its components except as a means of setting up a build environment.
I also do not think it should depend on obscure tools like xx.

Let CC and CXX be, and if someone wants to use xx they could do something like a hypothetical CC=$(xx-env CC) to get the correct compiler.
Dockerfile can use xx, scripts should use only standard tooling.

All in all, this looks like packaging. As a packager, I cannot use this toolchain.

@crazy-max
Copy link
Member Author

crazy-max commented Apr 26, 2022

What's the goal of this? Who is going to use it? The only purpose the current cross target serves, to my knowledge, is to make sure we don't break compilation on other platforms...

The primary goal is to improve the maintainability of our build scripts. The cross compilation is just a first step, dissociated for the moment from what is used by Jenkins or the Makefile but later we should not depend on the multitude of scripts present in the hack folder (or at least a large amount of them).

maybe it's used for static builds?

And yes also for this for static bundles in docker-ce-packaging.

I'm all for refactoring/rewriting our build system, I don't think anyone really likes it. That said, the caveat I would say here is that the build system should have no dependency on docker or its components except as a means of setting up a build environment. I also do not think it should depend on obscure tools like xx.

Let CC and CXX be, and if someone wants to use xx they could do something like a hypothetical CC=$(xx-env CC) to get the correct compiler. Dockerfile can use xx, scripts should use only standard tooling.

It doesn't need docker, and xx is only used inside the Dockerfile. You can invoke ./hack/build/release to build dockerd and docker-proxy with your local env. See https://github.com/moby/moby/pull/43529/files#diff-de4586e60f6c74e2d92477c3c7a7cfc1d3a8527260e6e179b2275f1bf1e6f2f3R21-R28

@crazy-max
Copy link
Member Author

All in all, this looks like packaging.

I agree that the binaries stage in Dockerfile.cross currently outputs project binaries and also external tools. I have changed that so now it will just build binaries linked to the project (dockerd and docker-proxy). Another stage named bundle will output binaries and linked tools. Is it what you mean by packaging?

hack/build/binary Outdated Show resolved Hide resolved
Dockerfile.cross Outdated Show resolved Hide resolved
@crazy-max
Copy link
Member Author

crazy-max commented May 4, 2022

I made some changes based on your comments.

  • Merged changes inside the current Dockerfile so there is no specific Dockerfile.cross
  • Dev stages have been updated accordingly to changes for cross comp as well as linked tools (swagger, tomll, gotestsum, ...)
  • The frozen-images stage doesn't use the download-frozen-image-v2.sh anymore so we can effectively use TARGETPLATFORM. The test util has been updated accordingly.
  • ci job dev has been added to check if dev image with or without systemd builds correctly.
  • DOCKER_CROSSPLATFORMS and CROSS vars removed
  • Dockerfile.simple has been merged to Dockerfile. See dev-light target.

I also changes the output dir from bundles to dist but I think I will revert this change as it brings more noise than anything else.

Changes in Jenkinsfile are not taken into account atm as it requires write access to the repo. @thaJeztah feel free to fork if you want to test these changes. I have another branch with a GHA test workflow that we could use crazy-max/moby@cross...crazy-max:cross-test-workflow, but didn't want to add too much in this PR.

I've also tested building against linux/riscv64 on another branch crazy-max/moby@cross...crazy-max:cross-riscv64 to check @AkihiroSuda PR #43553 and seems to work fine: https://github.com/crazy-max/moby/runs/6286993811?check_suite_focus=true but requires ubuntu.

image

I let this one for a follow-up.

@crazy-max crazy-max force-pushed the cross branch 4 times, most recently from 42a0c19 to ff2967e Compare May 4, 2022 22:23
@crazy-max crazy-max marked this pull request as ready for review May 5, 2022 14:12
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
needs to update supported platforms for pie buildmode
and adds smoke test

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@crazy-max
Copy link
Member Author

@neersighted Yes @corhere got the same issue yesterday. Rebasing should work, let's see.

@rumpl
Copy link
Member

rumpl commented Oct 24, 2022

We've been using this to bundle dockerd with containerd for months and it works like a charm!

@neersighted
Copy link
Member

I would really like to see this or a version of it merged; as we're basically blocked on 22.06 by this. Granted, this is a monster of a PR, but it's clear some major refactoring of the build has to happen, and no one has proposed a good way to split this work up into more digestible chunks (or rather, @thaJeztah tried with one or two PRs, but it didn't get too far).

@corhere
Copy link
Contributor

corhere commented Oct 24, 2022

one has proposed a good way to split this work up into more digestible chunks (or rather, @thaJeztah tried with one or two PRs, but it didn't get too far).

To me, it looks like nobody has really tried to get any split-up work reviewed and merged. #44114 is just sitting there with no "PTAL" pings.


How is 22.06 blocked? For argument's sake, what would the consequences be if we were to abandon this PR and try to ship 22.06 today as-is?

How much of what this PR touches is required to ship 22.06? Can we cherry-pick just that into a PR and leave the rest for follow-ups? I find it hard to believe that we can't ship 22.06 unless we e.g. added a GHA workflow which validates Dockerfile.simple, modified how delve is installed in the dev container image, collected all the dependency version arguments into one block, changed the default in the Dockerfile to GO111MODULE=on, or switched /build paths to /out.

@neersighted
Copy link
Member

As I understand it, 22.06 is blocked on this because cross-compilation as used by docker/ce-packaging is broken and relies on fixes in this PR to fix it. Docker CE is blocking the release of Moby as there is a want to release both (Docker CE binaries and Moby sources) at the same time.

@crazy-max
Copy link
Member Author

crazy-max commented Oct 25, 2022

Static packages with current stable (20.10) are indeed not in good shape in docker-ce-packaging but it should not be a blocker for 22.06.

If I recall correctly the idea was first to split static packages as atm docker-ce-cli ships both the cli and some plugins. Same with docker-ce that ships dockerd, runc and containerd. The cross compilation issue for static packages with moby is that --platform and TARGETPLATFORM is not possible with the current Dockerfile. We also have to download cross packages for all platforms for each build and we are limited to a subset of platforms. This is what this PR tries to solve.

There is also the new https://github.com/docker/packaging/ repo that currently solves the issue with some FIXME that would be solved by this PR: https://github.com/docker/packaging/blob/eb4e5de34e1096cd0ad8d7d0244c17ea1adb5d98/pkg/docker-engine/scripts/pkg-static-build.sh#L47-L116. But still a WIP as we have to also consume these new images to make releases on https://download.docker.com/.

But again not a blocker for me as we are in the same situation as 20.10 but would be great for v-next at least.

@sam-thibault
Copy link
Contributor

@crazy-max @thaJeztah It sounds like we can move this off the 22.06 milestone? Or is there part of this change we must include for 22.06?

@neersighted
Copy link
Member

Gonna go ahead and move this to v-next.

@crazy-max
Copy link
Member Author

I think we can close this one now

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