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

[master] fix and refactor static packages #665

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented Mar 29, 2022

fixes an issue introduced by #654 while adding buildx static package. also review the bits that generates the static packages:

  • with dockerd we need to detect if cross compilation is necessary. this is something we should fix upstream so we don't need this check and can full rely on --platform with buildx.
  • makefile has been simplified so we don't need to duplicate goals and rely only on TARGETPLATFORM.
  • Jenkinsfile is lighter and additionally generates static packages for additional platforms that were missing (linux/arm/v6, linux/arm/v7, linux/arm64)
  • adds a job matrix on GHA so we can also test its behavior on GitHub Runners.
  • static packages were missing for buildx against windows and darwin platforms.

Signed-off-by: CrazyMax [email protected]

@crazy-max crazy-max force-pushed the fix-static-pkgs branch 9 times, most recently from 826baa6 to a93d12b Compare March 29, 2022 13:18
@crazy-max
Copy link
Member Author

moby engine cross compilation fails for linux/arm/v6. Should be fixed upstream: https://github.com/docker/docker-ce-packaging/runs/5737440583?check_suite_focus=true#step:4:2372

#43 0.132 
#43 0.138 ---> Making bundle: cross (in /build/bundles/cross)
#43 0.139 Cross building: /build/bundles/cross/linux/arm/v6
#43 0.182 Building: /build/bundles/cross/linux/arm/v6-daemon/dockerd-0.0.0-20220329083112-174e51c
#43 0.182 GOOS="linux" GOARCH="arm" GOARM="6"
#43 76.80 # github.com/docker/docker/cmd/dockerd
#43 76.80 loadinternal: cannot find runtime/cgo
#43 76.80 /usr/local/go/pkg/tool/linux_amd64/link: running gcc failed: exit status 1
#43 76.80 gcc: error: unrecognized command-line option '-marm'; did you mean '-mabm'?

@crazy-max crazy-max force-pushed the fix-static-pkgs branch 6 times, most recently from fad8dbf to c34fe89 Compare March 29, 2022 21:42
@crazy-max crazy-max marked this pull request as ready for review March 29, 2022 22:41
@crazy-max crazy-max force-pushed the fix-static-pkgs branch 6 times, most recently from 10552c8 to 950306c Compare March 30, 2022 10:47
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.

Just a first glance over the changes, thought I'd post some initial "thinking out loud" comments


# current arch/variant
CUROS="linux"
case "$(uname -m)" in
Copy link
Member

Choose a reason for hiding this comment

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

didn't xx also have a utility for something like this? (wondering if we can somehow centralise this effort)

Copy link
Member Author

@crazy-max crazy-max Mar 30, 2022

Choose a reason for hiding this comment

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

yes there is the xx-info one and we should use it in a follow-up when we will be able to be sandboxed inside a Dockerfile. See #665 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better (and a lot simpler) to use go env GOARCH / go env GOOS here? 😇

If we don't get to assume access to go, maybe something like docker version --format '{{ .Server.Os }}/{{ .Server.Arch }}'?
(not sure how we reliably get "current variant" but that's pretty complicated/a guess regardless, so letting the auto-detection default to v7 seems pretty sane IMO 🙈)

if (arch == 'armhf') {
// Running armhf builds on EC2 requires --platform parameter
// Otherwise it accidentally pulls armel images which then breaks the verify step
platform = "--platform=linux/${arch}"
Copy link
Member

Choose a reason for hiding this comment

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

this no longer needed? was slightly wondering if we must do the reverse and always explicitly specify platform 🤔

Copy link
Member Author

@crazy-max crazy-max Mar 30, 2022

Choose a reason for hiding this comment

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

We can't with moby atm unfortunately. only cli, buildx supports it without effort. hence #665 (comment). if it's fixed on moby we can remove the docker_engine_cross logic in build-static script.

Makefile Outdated Show resolved Hide resolved
static/Makefile Outdated Show resolved Hide resolved
static/build-static Outdated Show resolved Hide resolved
static/README.md Outdated Show resolved Hide resolved
thaJeztah
thaJeztah previously approved these changes Jun 29, 2022
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

@thaJeztah thaJeztah changed the title fix and refactor static packages [master] fix and refactor static packages Jun 29, 2022
Copy link
Contributor

@tianon tianon left a comment

Choose a reason for hiding this comment

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

@thaJeztah asked me to take a look -- had more comments than I expected, but nothing super major 😅


# current arch/variant
CUROS="linux"
case "$(uname -m)" in
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better (and a lot simpler) to use go env GOARCH / go env GOOS here? 😇

If we don't get to assume access to go, maybe something like docker version --format '{{ .Server.Os }}/{{ .Server.Arch }}'?
(not sure how we reliably get "current variant" but that's pretty complicated/a guess regardless, so letting the auto-detection default to v7 seems pretty sane IMO 🙈)

scripts/target-platform Outdated Show resolved Hide resolved
scripts/target-platform Outdated Show resolved Hide resolved
scripts/target-platform Outdated Show resolved Hide resolved
scripts/target-platform Show resolved Hide resolved
[ -d "${ENGINE_DIR:?}/bundles" ] && rm -r "${ENGINE_DIR:?}/bundles"
(
cd "${ENGINE_DIR}"
mkdir -p autogen # FIXME: remove when https://github.com/moby/moby/pull/43431 merged
Copy link
Contributor

Choose a reason for hiding this comment

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

That PR is merged -- does that mean this line can go away now? 👀 (or does it need to point somewhere else for the FIXME now? 😅)

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 for master or 22.06+ but not with 20.10 😣. Can update the comment though.

Copy link
Member

Choose a reason for hiding this comment

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

We don't need to take 20.10 into account (we use the 20.10 branch for that)

static/build-static Show resolved Hide resolved
static/build-static Outdated Show resolved Hide resolved
scanBuildDir="${buildDir}/docker-scan"

# create docker-container builder
docker buildx inspect | grep -q 'Driver: docker-container' || docker buildx create --use
Copy link
Contributor

Choose a reason for hiding this comment

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

😬

This line has several assumptions baked into it and changes the user's default buildx builder -- at the very least, I think we should probably be explicit about --driver docker-container on the create command, right? (so that it explicitly matches what we checked for with inspect)

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 indeed good point

static/build-static Show resolved Hide resolved
@crazy-max
Copy link
Member Author

crazy-max commented Jun 29, 2022

Thanks a bunch for the review @tianon!

Wouldn't it be better (and a lot simpler) to use go env GOARCH / go env GOOS here? 😇

If we don't get to assume access to go, maybe something like docker version --format '{{ .Server.Os }}/{{ .Server.Arch }}'?
(not sure how we reliably get "current variant" but that's pretty complicated/a guess regardless, so letting the auto-detection default to v7 seems pretty sane IMO 🙈)

A bunch of the os, arch and variant detection logic is taken from xx which has bats tests to cover these cases. This project is used as cross comp helper in some of our repos. See https://github.com/tonistiigi/xx/blob/master/base/xx-info to have a better understanding of the logic. Maybe it would be better to fetch the scripts instead of just copying part of its logic. Will improve that and see if we can get rid of scripts/target-platform.

Signed-off-by: CrazyMax <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
Copy link
Member

So, looks like something's still broken; the cross-compiled versions are missing the containerd binaries. It looks like --target=cross in the moby repository does not include them in the bundles directory (need to check why)

docker buildx build \
    --build-arg CGO_ENABLED= \
    --build-arg CONTAINERD_VERSION \
    --build-arg CROSS=true \
    --build-arg DEFAULT_PRODUCT_LICENSE \
    --build-arg DOCKER_CROSSPLATFORMS=linux/arm64 \
    --build-arg PACKAGER_NAME \
    --build-arg PLATFORM \
    --build-arg PRODUCT \
    --build-arg RUNC_VERSION \
    --build-arg VERSION=22.06.0-beta.1 \
    --output ./bundles \
    --target cross .

tree bundles
bundles
└── cross
    └── linux
        └── arm64-daemon
            ├── docker-proxy -> docker-proxy-22.06.0-beta.1
            ├── docker-proxy-22.06.0-beta.1
            ├── docker-proxy-22.06.0-beta.1.md5
            ├── docker-proxy-22.06.0-beta.1.sha256
            ├── dockerd -> dockerd-22.06.0-beta.1
            ├── dockerd-22.06.0-beta.1
            ├── dockerd-22.06.0-beta.1.md5
            └── dockerd-22.06.0-beta.1.sha256

3 directories, 8 files

@thaJeztah
Copy link
Member

I dug a bit further, and currently;

  • the cross target on moby/moby only cross-compiles the docker daemon. The confusing bit was that containerd and runc are also built as part of the cross target, but not cross-compiled
  • I could make containerd cross-compile by passing the right env-vars, but runc looks to need a bit more work

So, I'll have to dig further to see how feasible that is (given that we hope to build both as part of containerd packaging soon, so it would be temporary).

For platforms that we have machines for in Jenkins this won't be an issue (linux/amd64 and linux/arm64), but for other platforms that may be an issue (arm32 variants including)

@crazy-max
Copy link
Member Author

@thaJeztah moby/moby#43529 would fix this

crazy-max and others added 10 commits July 4, 2022 11:45
Signed-off-by: CrazyMax <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Jenkins may set parameters to an empty value, in which case the build-args
may be overriding the default value with an empty value. This patch
explicitly unsets variables if they're empty (or not set).

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: CrazyMax <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: CrazyMax <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: CrazyMax <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: CrazyMax <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
This splits the CLI, Engine, and containerd packages to allow downloading
the cli separate from the daemon, as well as (in future) allowing us to
do a containerd release without also requiring an engine release.

With this patch:

    make REF=v22.06.0-beta.0 VERSION=v22.06.0-beta.0 TARGETPLATFORM=linux/amd64 static

    static/build
    ├── bundles-ce-static-linux-x86_64.tar.gz
    └── linux
        └── amd64
            ├── containerd-1.6.4.tgz
            ├── docker-buildx-plugin-0.8.2.tgz
            ├── docker-cli-22.06.0-beta.0.tgz
            ├── docker-engine-22.06.0-beta.0.tgz
            ├── docker-compose-plugin-2.6.1.tgz
            ├── docker-rootless-extras-22.06.0-beta.0.tgz
            └── docker-scan-plugin-0.17.0.tgz

    2 directories, 8 files

    ls -lh static/build/linux/amd64/
    total 215208
    -rw-r--r--  1 sebastiaan  staff    31M Jun 29 00:21 containerd-1.6.4.tgz
    -rw-r--r--  1 sebastiaan  staff    14M Jun 29 00:21 docker-buildx-plugin-0.8.2.tgz
    -rw-r--r--  1 sebastiaan  staff   8.2M Jun 29 00:21 docker-cli-22.06.0-beta.0.tgz
    -rw-r--r--  1 sebastiaan  staff    19M Jun 29 00:21 docker-engine-22.06.0-beta.0.tgz
    -rw-r--r--  1 sebastiaan  staff   8.8M Jun 29 00:21 docker-compose-plugin-2.6.1.tgz
    -rw-r--r--  1 sebastiaan  staff    19M Jun 29 00:21 docker-rootless-extras-22.06.0-beta.0.tgz
    -rw-r--r--  1 sebastiaan  staff   4.4M Jun 29 00:21 docker-scan-plugin-0.17.0.tgz

Signed-off-by: CrazyMax <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
GHA currently only has x86 machines, and the "cross" target in moby
does not include containerd and runc.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants