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

Sedna control plane supports amd64/arm64 #196

Merged
merged 1 commit into from
Sep 28, 2021

Conversation

JimmyYang20
Copy link

@JimmyYang20 JimmyYang20 commented Sep 15, 2021

fix issues: #170

Signed-off-by: JimmyYang20 yangjin39@huawei.com

@kubeedge-bot kubeedge-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 15, 2021
@JimmyYang20 JimmyYang20 changed the title Sedna supports arm64 arch Sedna supports mult-arch Sep 15, 2021
@JimmyYang20 JimmyYang20 force-pushed the optimize-platform branch 6 times, most recently from 22826f1 to 2666fd9 Compare September 16, 2021 03:29
https://github.com/docker/buildx/releases/download/v0.6.3/buildx-v0.6.3.linux-amd64
chmod a+x ~/.docker/cli-plugins/docker-buildx

cat > ~/.docker/config.json <<EOF
Copy link

Choose a reason for hiding this comment

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

this would overwrite the user original docker config

Copy link
Author

Choose a reason for hiding this comment

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

fix it

Makefile Outdated
for target in "gm" "lc"; do \
docker buildx build --push \
--build-arg GO_LDFLAGS=${GO_LDFLAGS} \
--platform linux/amd64,linux/arm64 \
Copy link

Choose a reason for hiding this comment

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

make the platforms configurable.

Copy link
Author

Choose a reason for hiding this comment

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

fix it


# Update buildx plugin to version 6.0+.
mkdir -pv ~/.docker/cli-plugins/
wget -O ~/.docker/cli-plugins/docker-buildx \
Copy link

Choose a reason for hiding this comment

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

please check it first, buildx maybe already built in docker, or installed by this way.

Copy link
Author

Choose a reason for hiding this comment

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

fix it

# Update buildx plugin to version 6.0+.
mkdir -pv ~/.docker/cli-plugins/
wget -O ~/.docker/cli-plugins/docker-buildx \
https://github.com/docker/buildx/releases/download/v0.6.3/buildx-v0.6.3.linux-amd64
Copy link

Choose a reason for hiding this comment

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

please get arch from current build machine first instead of hard amd64

Suggested change
https://github.com/docker/buildx/releases/download/v0.6.3/buildx-v0.6.3.linux-amd64
https://github.com/docker/buildx/releases/download/v0.6.3/buildx-v0.6.3.linux-$arch

Copy link
Author

Choose a reason for hiding this comment

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

fix it

@@ -207,6 +207,9 @@ sedna::golang::build_binaries() {
goldflags="${GOLDFLAGS=-s -w -buildid=} $(sedna::version::ldflags)"
gogcflags="${GOGCFLAGS:-}"

CGO_ENABLED=0
GOARCH=arm64
Copy link

Choose a reason for hiding this comment

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

Only build arm64, how about other archs, e.g. x86?

Copy link
Author

Choose a reason for hiding this comment

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

delete it

@JimmyYang20 JimmyYang20 force-pushed the optimize-platform branch 3 times, most recently from 7780eaf to 4017c62 Compare September 18, 2021 09:53
@JimmyYang20 JimmyYang20 changed the title Sedna supports mult-arch Sedna component images support amd64/arm64 arch Sep 18, 2021
docker_buildx=~/.docker/cli-plugins/docker-buildx
if [ ! -x $docker_buildx ];then
mkdir -pv $(dirname $docker_buildx)
arch=$(dpkg --print-architecture)
Copy link

Choose a reason for hiding this comment

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

dpkg only available in debain like system, use uname -m

Copy link
Author

Choose a reason for hiding this comment

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

delete install buildx

arch=$(dpkg --print-architecture)
wget -O $docker_buildx \
https://github.com/docker/buildx/releases/download/v0.6.3/buildx-v0.6.3.linux-$arch
chmod a+x ~/.docker/cli-plugins/docker-buildx
Copy link

Choose a reason for hiding this comment

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

Suggested change
chmod a+x ~/.docker/cli-plugins/docker-buildx
chmod a+x $docker_buildx

Copy link
Author

Choose a reason for hiding this comment

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

delete install buildx

@@ -13,8 +13,14 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
set -x
Copy link

Choose a reason for hiding this comment

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

Suggested change
set -x

Copy link
Author

Choose a reason for hiding this comment

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

ok

mkdir -pv $(dirname $docker_buildx)
arch=$(dpkg --print-architecture)
wget -O $docker_buildx \
https://github.com/docker/buildx/releases/download/v0.6.3/buildx-v0.6.3.linux-$arch
Copy link

Choose a reason for hiding this comment

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

make buildx version as variable

Copy link
Author

Choose a reason for hiding this comment

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

delete install buildx

@JimmyYang20 JimmyYang20 force-pushed the optimize-platform branch 2 times, most recently from 1211c18 to 365641d Compare September 22, 2021 02:29
Makefile Outdated
@@ -18,6 +18,10 @@ OUT_DIR ?= _output
OUT_BINPATH := $(OUT_DIR)/bin

IMAGE_REPO ?= kubeedge

PLATFORMS ?= "linux/amd64,linux/arm64"
Copy link

Choose a reason for hiding this comment

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

Can you add a description about full list of platforms?

Copy link
Author

Choose a reason for hiding this comment

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

OK

@@ -17,4 +17,9 @@
cd "$(dirname "${BASH_SOURCE[0]}")"

source build_image.sh
docker push $IMAGE

if [ ! -n "$1" ]; then
Copy link

Choose a reason for hiding this comment

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

Use environment variable instead of argument list to toggle cross build capability.

Copy link

Choose a reason for hiding this comment

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

Such as

Suggested change
if [ ! -n "$1" ]; then
if [ -z "${CROSS_PLATFORMS:-}" ]; then

Copy link
Author

Choose a reason for hiding this comment

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

ok

@@ -0,0 +1,47 @@
#!/usr/bin/env bash
Copy link

Choose a reason for hiding this comment

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

delete this script

Copy link

Choose a reason for hiding this comment

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

No need since we can leave the buildx download/config to developer.

Copy link
Author

Choose a reason for hiding this comment

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

we need to have buildx multiple platforms features config, leave the buildx download to developer.

@llhuii
Copy link

llhuii commented Sep 22, 2021

Add a cross build target in Makefile, not only image push action

@JimmyYang20
Copy link
Author

@llhuii
the current limitation of buildx is combination of --load and multiple values to --platform.
This means that buildx uses --load only for single platform。


# Check whether buildx exists.
if ! docker buildx >/dev/null 2>&1; then
echo "ERROR: docker buildx not available. Docker 19.03 or higher is required with experimental features enabled"
Copy link

Choose a reason for hiding this comment

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

log error to stderr

Suggested change
echo "ERROR: docker buildx not available. Docker 19.03 or higher is required with experimental features enabled"
echo "ERROR: docker buildx not available. Docker 19.03 or higher is required with experimental features enabled" >&2

Copy link
Author

Choose a reason for hiding this comment

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

fix it

docker run --privileged --rm tonistiigi/binfmt --install all

# Create a new builder which gives access to the new multi-architecture features.
builder_instance="build-node-sedna"
Copy link

Choose a reason for hiding this comment

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

prefix sedna?

Suggested change
builder_instance="build-node-sedna"
builder_instance="sedna-buildx"

Copy link
Author

Choose a reason for hiding this comment

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

ok


# Create a new builder which gives access to the new multi-architecture features.
builder_instance="build-node-sedna"
temp=$(docker buildx ls | awk NF=1)
Copy link

Choose a reason for hiding this comment

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

How about using docker buildx inspect $builder_instance?

Copy link

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

ok, use if ! docker buildx inspect $build_instance

Copy link

Choose a reason for hiding this comment

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

remember to discard stdout/stdin

Copy link
Author

Choose a reason for hiding this comment

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

ok

@llhuii
Copy link

llhuii commented Sep 23, 2021

@llhuii
the current limitation of buildx is combination of --load and multiple values to --platform.
This means that buildx uses --load only for single platform。

Use the -o option for local build, https://github.com/docker/buildx/blob/master/docs/reference/buildx_build.md#output

@JimmyYang20
Copy link
Author

@llhuii
the current limitation of buildx is combination of --load and multiple values to --platform.
This means that buildx uses --load only for single platform。

Use the -o option for local build, https://github.com/docker/buildx/blob/master/docs/reference/buildx_build.md#output

can not do it by my test, https://github.com/docker/buildx/blob/master/docs/reference/buildx_build.md#image

@JimmyYang20
Copy link
Author

JimmyYang20 commented Sep 26, 2021

@llhuii
the current limitation of buildx is combination of --load and multiple values to --platform.
This means that buildx uses --load only for single platform。

Use the -o option for local build, https://github.com/docker/buildx/blob/master/docs/reference/buildx_build.md#output

can not do it by my test, https://github.com/docker/buildx/blob/master/docs/reference/buildx_build.md#image

I mean cross built local file instead of pushing image to registry directly.

You can refer the kubernetes repo:

# make DBG_MAKEFILE=1 release-images KUBE_BUILD_CONFORMANCE=n KUBE_BUILD_PLATFORMS=linux/amd64
                                         
# tree _output/release-stage/server/       
_output/release-stage/server/                         
├── linux-amd64                                
│   └── kubernetes                         
│       └── server                                    
│           └── bin                            
│               ├── kube-apiserver         
│               ├── kube-apiserver.docker_tag         
│               ├── kube-apiserver.tar         
│               ├── kube-controller-manager
│               ├── kube-controller-manager.docker_tag
│               ├── kube-controller-manager.tar
│               ├── kube-proxy
│               ├── kube-proxy.docker_tag
│               ├── kube-proxy.tar
│               ├── kube-scheduler
│               ├── kube-scheduler.docker_tag
│               └── kube-scheduler.tar
├── linux-arm
│   └── kubernetes
... omit here ...
_output/
└── images
    ├── gm-v0.3.0-linux-amd64.tar
    ├── gm-v0.3.0-linux-arm64.tar
    ├── lc-v0.3.0-linux-amd64.tar
    └── lc-v0.3.0-linux-arm64.tar

@JimmyYang20 JimmyYang20 force-pushed the optimize-platform branch 3 times, most recently from b3dcbba to 776e71b Compare September 26, 2021 11:47
@JimmyYang20 JimmyYang20 changed the title Sedna component images support amd64/arm64 arch Sedna control plane supports amd64/arm64 Sep 26, 2021
@JimmyYang20 JimmyYang20 force-pushed the optimize-platform branch 3 times, most recently from 98a76af to 1a65964 Compare September 27, 2021 02:28
@llhuii
Copy link

llhuii commented Sep 27, 2021

good job
/lgtm

@kubeedge-bot kubeedge-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2021
@llhuii
Copy link

llhuii commented Sep 27, 2021

The checker docker cross build images for gm/lc/kb caused 38min, too slow.

@llhuii
Copy link

llhuii commented Sep 27, 2021

/hold

@kubeedge-bot kubeedge-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 27, 2021
@kubeedge-bot kubeedge-bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2021
@JimmyYang20
Copy link
Author

JimmyYang20 commented Sep 27, 2021

The checker docker cross build images for gm/lc/kb caused 38min, too slow.

fix it ,
The checker docker cross build images for gm/lc/kb caused 8min 11s

reference: https://docs.docker.com/buildx/working-with-buildx/#build-multi-platform-images
FROM --platform=$BUILDPLATFORM golang:alpine AS build

for component in ${COMPONENTS[@]}; do
echo "building ${PLATFORMS} image for ${component}"

sed "/AS builder/s/FROM/FROM --platform=\$BUILDPLATFORM/g" build/${component}/Dockerfile > ${temp_dockerfile}
Copy link

Choose a reason for hiding this comment

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

The cross-build-dockerfile generating code happend at two places, use function generate-cross-build-dockerfile

Copy link
Author

Choose a reason for hiding this comment

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

fix it

control plane images support amd64/arm64
add a workerflow action: docker_cross_build

Signed-off-by: JimmyYang20 <yangjin39@huawei.com>
@llhuii
Copy link

llhuii commented Sep 28, 2021

/lgtm
/hold cancel

@kubeedge-bot kubeedge-bot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Sep 28, 2021
@llhuii
Copy link

llhuii commented Sep 28, 2021

/approve

@kubeedge-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: llhuii

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubeedge-bot kubeedge-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 28, 2021
@kubeedge-bot kubeedge-bot merged commit 1d7bd48 into kubeedge:main Sep 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants