karmada-io / karmada

Open, Multi-Cloud, Multi-Cluster Kubernetes Orchestration
https://karmada.io
Apache License 2.0
4.11k stars 805 forks source link

speed up docker build #1740

Closed ikaven1024 closed 1 year ago

ikaven1024 commented 1 year ago

Signed-off-by: yingjinhui yingjinhui@didiglobal.com

What type of PR is this? /kind feature

What this PR does / why we need it: Speed up docker image building.

Which issue(s) this PR fixes: Fixes #1729

Special notes for your reviewer: Implement of https://github.com/karmada-io/karmada/issues/1729#issuecomment-1120238596.

Does this PR introduce a user-facing change?:

NONE
ikaven1024 commented 1 year ago

10min click me for full logs. Not so bad. @RainbowMango

RainbowMango commented 1 year ago

Thanks @ikaven1024 , I see there is a WIP in the PR, does this ready for review?

ikaven1024 commented 1 year ago

Thanks @ikaven1024 , I see there is a WIP in the PR, does this ready for review?

You can review it now. Some remaining work is update doc and remove debugging code.

ikaven1024 commented 1 year ago
Step 4/5 : RUN apk add --no-cache ca-certificates
 ---> Running in 1026ba175798
fetch http://mirrors.ustc.edu.cn/alpine/v3.15/main/x86_64/APKINDEX.tar.gz
WARNING: Ignoring http://mirrors.ustc.edu.cn/alpine/v3.15/main/: temporary error (try again later)
ERROR: unable to select packages:
  ca-certificates (no such package):
    required by: world[ca-certificates]

It failed today. But seems the problem of CI environment. On my side, it work well

RainbowMango commented 1 year ago

I see it fetching images from fetch http://mirrors.ustc.edu.cn/alpine/v3.15/main/x86_64/APKINDEX.tar.gz, why fetching images from the mirror? Is it due to #1656 ?

https://github.com/karmada-io/karmada/runs/6342037726?check_suite_focus=true#step:4:50

ikaven1024 commented 1 year ago

It works well yesterday all the day successful log at 5.8, also fetch from mirrors.ustc.edu.cn.

But failures appear after 12 clock PM. https://github.com/karmada-io/karmada/runs/6341953279?check_suite_focus=true#step:4:286 https://github.com/karmada-io/karmada/runs/6342037726?check_suite_focus=true#step:4:51

ikaven1024 commented 1 year ago

I see it fetching images from fetch http://mirrors.ustc.edu.cn/alpine/v3.15/main/x86_64/APKINDEX.tar.gz, why fetching images from the mirror? Is it due to #1656 ?

https://github.com/karmada-io/karmada/runs/6342037726?check_suite_focus=true#step:4:50

Ping is forbidden in k8s.gcr.io? I cannot test it, as having no machine out of china mainland.

In #1656, fetching from mirrors.ustc.edu.cn. see log

RainbowMango commented 1 year ago

Yeah, I made a test by this commit. And found that ping not work on GitHub runners:

PING googlecode.l.googleusercontent.com (172.253.115.[8](https://github.com/RainbowMango/karmada/runs/6345765429?check_suite_focus=true#step:4:8)2) 56(84) bytes of data.

--- googlecode.l.googleusercontent.com ping statistics ---
3 packets transmitted, 0 received, [10](https://github.com/RainbowMango/karmada/runs/6345765429?check_suite_focus=true#step:4:10)0% packet loss, time 406ms

For full logs, you can refer to https://github.com/RainbowMango/karmada/runs/6345765429?check_suite_focus=true#step:4:10.

I don't know why, might be the security concerns. Should we revert #1656? I don't want CI load images from a mirror, that's not safe and might slow down the test process(Load image from China?).

How do you think? @my-git9

my-git9 commented 1 year ago

Yeah, I made a test by this commit. And found that ping not work on GitHub runners:

PING googlecode.l.googleusercontent.com (172.253.115.[8](https://github.com/RainbowMango/karmada/runs/6345765429?check_suite_focus=true#step:4:8)2) 56(84) bytes of data.

--- googlecode.l.googleusercontent.com ping statistics ---
3 packets transmitted, 0 received, [10](https://github.com/RainbowMango/karmada/runs/6345765429?check_suite_focus=true#step:4:10)0% packet loss, time 406ms

For full logs, you can refer to https://github.com/RainbowMango/karmada/runs/6345765429?check_suite_focus=true#step:4:10.

I don't know why, might be the security concerns. Should we revert #1656? I don't want CI load images from a mirror, that's not safe and might slow down the test process(Load image from China?).

How do you think? @my-git9

LGTM

ikaven1024 commented 1 year ago

@RainbowMango Ready for merge

RainbowMango commented 1 year ago

Hi @gy95 , can you help to take a look?

ikaven1024 commented 1 year ago

@RainbowMango Ready for merging.

RainbowMango commented 1 year ago

I'm afraid it'll break Release workflow.

ikaven1024 commented 1 year ago

I'm afraid it'll break Release workflow.

make kubectl-karmada will match this rule:

# It contains all componets, including karmadactl kubectl-karmada
CMD_TARGET=$(TARGETS) $(CTL_TARGETS)
$(CMD_TARGET): $(SOURCES)
    LDFLAGS=$(LDFLAGS) BUILD_PLATFORMS=$(GOOS)/$(GOARCH) hack/build.sh $@

But, sorry for failure of below job, as i change the binary path. I will fix it, and test all the job in Release workflow. https://github.com/karmada-io/karmada/blob/977725d479515c4c0f0b7a030b3bf5964290a0e4/.github/workflows/release.yml#L32-L33

ikaven1024 commented 1 year ago

Test make & tar, work well

➜  make clean
➜  mkdir -p _output/release
➜  
➜  make kubectl-karmada GOOS=linux GOARCH=amd64
➜  make kubectl-karmada GOOS=linux GOARCH=arm64
➜  make kubectl-karmada GOOS=darwin GOARCH=amd64
➜  make kubectl-karmada GOOS=darwin GOARCH=arm64
➜  
➜  os=linux arch=amd64 tar czf _output/release/kubectl-karmada-$os-$arch.tgz LICENSE -C _output/bin/$os/$arch kubectl-karmada
➜  os=linux arch=arm64 tar czf _output/release/kubectl-karmada-$os-$arch.tgz LICENSE -C _output/bin/$os/$arch kubectl-karmada
➜  os=darwin arch=amd64 tar czf _output/release/kubectl-karmada-$os-$arch.tgz LICENSE -C _output/bin/$os/$arch kubectl-karmada
➜  os=darwin arch=arm64 tar czf _output/release/kubectl-karmada-$os-$arch.tgz LICENSE -C _output/bin/$os/$arch kubectl-karmada
➜  
➜  tree _output/
_output/
├── bin
│   ├── darwin
│   │   ├── amd64
│   │   │   └── kubectl-karmada
│   │   └── arm64
│   │       └── kubectl-karmada
│   └── linux
│       ├── amd64
│       │   └── kubectl-karmada
│       └── arm64
│           └── kubectl-karmada
└── release
    ├── kubectl-karmada-darwin-amd64.tgz
    ├── kubectl-karmada-darwin-arm64.tgz
    ├── kubectl-karmada-linux-amd64.tgz
    └── kubectl-karmada-linux-arm64.tgz
ikaven1024 commented 1 year ago

@RainbowMango help please. When i accept commit suggestion, new commit has no sign-off. How can i set it?

RainbowMango commented 1 year ago

When i accept commit suggestion, new commit has no sign-off. How can i set it?

You should squash your commits.

RainbowMango commented 1 year ago

Generally looks good, before moving this forward, I have a question.

It takes about 8 minutes to get ready for CI. That's awesome!

I run hack/local-up-karmada.sh on my side, but it takes about 13min. It takes about 3~4min before, I'm sure it is the same machine.

@XiShanYongYe-Chang @huone1 Can you help to test how long it will take on your side? I remember you have tested it before.

ikaven1024 commented 1 year ago

I run hack/local-up-karmada.sh on my side, but it takes about 13min. It takes about 3~4min before, I'm sure it is the same machine.

Below affect it:

  1. go cache. If you have build before, then cache will work, we can get the binary few seconds.
  2. docker build cache. Pulling base image and apk add --no-cache ca-certificates will be skipped.
  3. pulling kind image. If local image is updated, this also very fast.

@RainbowMango you can re-run hack/local-up-karmada.sh for test.

Another tip, If we use the same version of kind node with k8s version. k8s images are already in kind image. This can accelerate the start of karmada.

diff --git a/artifacts/kindClusterConfig/karmada-host.yaml b/artifacts/kindClusterConfig/karmada-host.yaml
index 1342653a..aa7d8565 100644
--- a/artifacts/kindClusterConfig/karmada-host.yaml
+++ b/artifacts/kindClusterConfig/karmada-host.yaml
@@ -4,6 +4,7 @@ networking:
   apiServerAddress: "{{host_ipaddress}}"
 nodes:
   - role: control-plane
+    image: kindest/node:v1.21.7
     extraPortMappings:
       - containerPort: 5443
         hostPort: 5443

https://github.com/karmada-io/karmada/blob/b2ebfb60361a8ac54fb9e2da9728f08ae5c3a02f/artifacts/deploy/karmada-apiserver.yaml#L67

RainbowMango commented 1 year ago

@RainbowMango you can re-run hack/local-up-karmada.sh for test.

I run several times locally. Almost the same time each time. :( :(

XiShanYongYe-Chang commented 1 year ago

It cost 3m50.004s on my local site. (4u8g linux x86_64)

RainbowMango commented 1 year ago

That must be my problem. I'll debug it then.

/lgtm /approve

Thanks again @ikaven1024 for your excellent work!

karmada-bot commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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: - ~~[OWNERS](https://github.com/karmada-io/karmada/blob/master/OWNERS)~~ [RainbowMango] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
RainbowMango commented 1 year ago

The workflow latest image to DockerHub is failing on master. https://github.com/karmada-io/karmada/runs/6383487427?check_suite_focus=true

I've re-triggered once.

ikaven1024 commented 1 year ago

@RainbowMango I know the reason now. It run dockerhub-latest-image job with swr.ap-southeast-1.myhuaweicloud.com registry.

huone1 commented 1 year ago

there is a error when i run hack/local-up-karmada.sh https://github.com/karmada-io/karmada/blob/da78076e261f796a03c6bb2306cb72664af956e1/hack/docker.sh#L33-L34 image

ikaven1024 commented 1 year ago

there is a error when i run hack/local-up-karmada.sh

https://github.com/karmada-io/karmada/blob/da78076e261f796a03c6bb2306cb72664af956e1/hack/docker.sh#L33-L34

diff --git a/hack/docker.sh b/hack/docker.sh
index 683c4daf..ed69edd9 100755
--- a/hack/docker.sh
+++ b/hack/docker.sh
@@ -31,7 +31,7 @@ REGISTRY=${REGISTRY:-"swr.ap-southeast-1.myhuaweicloud.com/karmada"}
 VERSION=${VERSION:="unknown"}

 function build_images() {
-  local -ra target=$1
+  local target=$1
   local -r output_type=${OUTPUT_TYPE:-docker}
   local platforms="${BUILD_PLATFORMS:-"$(util:host_platform)"}"

@huone1 could you modify as above and test again. Waiting for you reply.

huone1 commented 1 year ago

there is a error when i run hack/local-up-karmada.sh https://github.com/karmada-io/karmada/blob/da78076e261f796a03c6bb2306cb72664af956e1/hack/docker.sh#L33-L34

diff --git a/hack/docker.sh b/hack/docker.sh
index 683c4daf..ed69edd9 100755
--- a/hack/docker.sh
+++ b/hack/docker.sh
@@ -31,7 +31,7 @@ REGISTRY=${REGISTRY:-"swr.ap-southeast-1.myhuaweicloud.com/karmada"}
 VERSION=${VERSION:="unknown"}

 function build_images() {
-  local -ra target=$1
+  local target=$1
   local -r output_type=${OUTPUT_TYPE:-docker}
   local platforms="${BUILD_PLATFORMS:-"$(util:host_platform)"}"

@huone1 could you modify as above and test again. Waiting for you reply.

it is OK after deleting the option -a

ikaven1024 commented 1 year ago

@huone1 you delete -ra or just a?

huone1 commented 1 year ago

@huone1 you delete -ra or just a?

just a

ikaven1024 commented 1 year ago

just a

Thank you for your reply. This a is really needless. ok i will delete it.

ikaven1024 commented 1 year ago

I reproduce this error in CentOS Linux release 7.6.1810 (Core). While it runs well on my mac.

local -ra target=$1 

As this say, values assigned to array must warp with '()'.

@RainbowMango didn't you occur this error?

RainbowMango commented 1 year ago

No, It works fine on my side.

-bash-4.2# cat /etc/redhat-release 
CentOS Linux release 7.7.1908 (Core)
huone1 commented 1 year ago

image