ci: install docker 19.03 for arm64 to let build image go #862

jongwu · 2020-10-29T10:44:30Z

"make proto" will fail on arm64 using docker 18.06. This bug will be
gone if using docker 19.03. so upgrade docker before "make proto"
for arm64.

Fixes: #861
Signed-off-by: Jianyong Wu [email protected]

@jodh-intel @devimc

jongwu · 2020-10-29T10:45:29Z

/test

codecov · 2020-10-29T11:14:14Z

Codecov Report

Merging #862 (23bd1c7) into master (5cfb8ec) will increase coverage by 0.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #862      +/-   ##
==========================================
+ Coverage   57.72%   57.80%   +0.08%     
==========================================
  Files          17       17              
  Lines        2370     2370              
==========================================
+ Hits         1368     1370       +2     
+ Misses        840      839       -1     
+ Partials      162      161       -1

devimc

thanks @jongwu - I have some questions

devimc · 2020-10-29T13:31:41Z

.ci/run.sh

@@ -9,10 +9,27 @@ set -e
 cidir=$(dirname "$0")
 source "${cidir}/lib.sh"

+install_docker() {
+	docker_version=${docker_version:-19.03}


is this version of docker required only in this repo? why don't change in the tests repo?

yeah, I find this is necessary only in agent, also change in tests repo won't work the version is available for 18.06 for now.

This might work today, but what about when Docker 19.03 is no longer available? It would be much safer to check the installed version of docker and if it is "< 19.03", upgrade it.

But we store these sorts of version details in the versions database:

https://github.com/kata-containers/runtime/blob/master/versions.yaml#L211

And if it is architecture-specific, add an aarch64 block like here:

https://github.com/kata-containers/runtime/blob/master/versions.yaml#L101..L106

devimc · 2020-10-29T13:32:58Z

.ci/run.sh

+if [ $arch == "arm64" ]; then
+	command -v docker >/dev/null 2>&1 && "${testcidir}/../cmd/container-manager/manage_ctr_mgr.sh" docker remove
+	echo "reinstall docker 19.03 for arm64"
+	install_docker 19.03


this install_docker is distro specific, I think you should rename it to install_docker_ubuntu or something else that contains the ubuntu word

devimc · 2020-10-29T13:33:56Z

.ci/run.sh

+install_docker() {
+	docker_version=${docker_version:-19.03}
+	pkg_name="docker-ce"
+	repo_url="https://download.docker.com/linux/ubuntu"


before installing docker, could you please check that you are running on ubuntu?

c3d

Looks good to me, but I only checked there was no impact on x86.

jongwu · 2020-10-30T02:10:41Z

/test

jongwu · 2020-10-30T04:48:49Z

/test

jongwu · 2020-10-30T09:08:11Z

/test-arm

jongwu · 2020-10-30T14:33:31Z

/test

jongwu · 2020-11-02T02:26:31Z

/test-arm

jongwu · 2020-11-02T04:43:46Z

/test-arm

jongwu · 2020-11-02T07:13:38Z

/test

jodh-intel · 2020-11-02T09:10:39Z

.ci/run.sh

@@ -9,10 +9,27 @@ set -e
 cidir=$(dirname "$0")
 source "${cidir}/lib.sh"

+install_docker() {
+	docker_version=${docker_version:-19.03}


This might work today, but what about when Docker 19.03 is no longer available? It would be much safer to check the installed version of docker and if it is "< 19.03", upgrade it.

But we store these sorts of version details in the versions database:

https://github.com/kata-containers/runtime/blob/master/versions.yaml#L211

And if it is architecture-specific, add an aarch64 block like here:

https://github.com/kata-containers/runtime/blob/master/versions.yaml#L101..L106

jodh-intel · 2020-11-02T09:11:27Z

.ci/run.sh

+	echo "reinstall docker 19.03 for arm64"
+	install_docker_ubuntu 19.03


Please don't hard code version numbers in strings - use variables to make the scripts easier to maintain.

yeah, thanks

@jodh-intel - any comments?

jongwu · 2020-11-05T01:56:05Z

/test

jongwu · 2020-11-06T09:28:42Z

/test

jodh-intel

Thanks @jongwu. But imho these changes look confusing: they are architecture, distro and container manager specific. Further, there are no comments in the code explaining what is happening. Plus, they make the script overly complex for the non-ARM case.

It would be much cleaner I think if you created something like .ci/run-aarch64.sh and called it a function in it conditionally.

jodh-intel · 2020-11-11T09:21:24Z

.ci/run.sh

+install_docker_ubuntu() {
+	pkg_name="docker-ce"
+	repo_url="https://download.docker.com/linux/ubuntu"
+	curl -fsSL "${repo_url}/gpg" | sudo apt-key add -


These options look inconsistent to me:

-f, --fail Fail silently (no output at all) on HTTP errors -S, --show-error Show error even when -s is used -s, --silent Silent mode

Why are you suppressing errors anyway as that will make debugging a remote CI very hard?

thanks @jodh-intel -, I fold the change into a new file, also tweak this curl command.

jodh-intel

Thanks @jongwu. That's much clearer imho.

lgtm

jodh-intel · 2020-11-12T10:31:52Z

.ci/run.sh

+# docker version may need to be upgraded to let make proto go on arm64
+arch=$(go env GOARCH)
+if [ "$arch" == "arm64" ]; then
+	"../agent/.ci/run_arm64.sh"


This path looks odd - shouldn't it be something like the following?:

cidir=$(dirname "$0") "${cidir}/run_arm64.sh"

but it won't work as the current dir is .../tests. run_arm64.sh is not found in the path of ".ci/run_arm64.sh"

Ack! I was only looking at that line in isolation ;)

jodh-intel · 2020-11-16T09:50:28Z

/test-ubuntu

jongwu · 2020-11-17T01:21:24Z

/test-arm

jongwu · 2020-11-17T05:00:15Z

/test-ubuntu
/test-arm

"make proto" will fail on arm64 using docker 18.06. This bug will be gone if using docker 19.03. so upgrade docker before "make proto" for arm64. Depends-on: github.com/kata-containers/runtime#3057 Fixes: kata-containers#861 Signed-off-by: Jianyong Wu <[email protected]>

jongwu · 2020-11-17T06:46:34Z

/test-arm
/test-ubuntu

jongwu · 2020-11-18T13:32:33Z

hello @jodh-intel @devimc -, should this be merged?

jodh-intel · 2020-11-18T14:00:41Z

/test-vfio

devimc

thanks @jongwu

devimc reviewed Oct 29, 2020

View reviewed changes

c3d approved these changes Oct 29, 2020

View reviewed changes

jongwu force-pushed the docker branch from eaa3709 to 6c28294 Compare October 30, 2020 02:10

jongwu force-pushed the docker branch from 6c28294 to 8255419 Compare October 30, 2020 04:48

jongwu force-pushed the docker branch from 8255419 to 74aaeec Compare October 30, 2020 09:06

jongwu force-pushed the docker branch from 74aaeec to 5f7b77c Compare October 30, 2020 14:32

jongwu force-pushed the docker branch from 5f7b77c to ecb9ed6 Compare November 2, 2020 02:25

jongwu force-pushed the docker branch from ecb9ed6 to 1faadd8 Compare November 2, 2020 04:43

jodh-intel suggested changes Nov 2, 2020

View reviewed changes

jongwu force-pushed the docker branch 2 times, most recently from 89e2ef2 to 09b4f33 Compare November 3, 2020 02:23

jongwu force-pushed the docker branch from 09b4f33 to ad23eb3 Compare November 5, 2020 06:21

jongwu force-pushed the docker branch 3 times, most recently from 15516c3 to 05872f3 Compare November 11, 2020 09:19

jodh-intel suggested changes Nov 11, 2020

View reviewed changes

jongwu force-pushed the docker branch from 05872f3 to 41522ed Compare November 12, 2020 02:51

jodh-intel approved these changes Nov 12, 2020

View reviewed changes

jongwu force-pushed the docker branch from 41522ed to dad04fa Compare November 17, 2020 03:07

jongwu force-pushed the docker branch from dad04fa to 23bd1c7 Compare November 17, 2020 06:45

devimc approved these changes Nov 18, 2020

View reviewed changes

devimc merged commit 8c85bb5 into kata-containers:master Nov 18, 2020

		echo "reinstall docker 19.03 for arm64"
		install_docker_ubuntu 19.03

ci: install docker 19.03 for arm64 to let build image go #862

ci: install docker 19.03 for arm64 to let build image go #862

Conversation

jongwu commented Oct 29, 2020

jongwu commented Oct 29, 2020

codecov bot commented Oct 29, 2020 • edited Loading

Codecov Report

devimc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c3d left a comment

Choose a reason for hiding this comment

jongwu commented Oct 30, 2020

jongwu commented Oct 30, 2020

jongwu commented Oct 30, 2020

jongwu commented Oct 30, 2020

jongwu commented Nov 2, 2020

jongwu commented Nov 2, 2020

jongwu commented Nov 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jongwu commented Nov 5, 2020

jongwu commented Nov 6, 2020

jodh-intel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodh-intel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodh-intel commented Nov 16, 2020

jongwu commented Nov 17, 2020

jongwu commented Nov 17, 2020

jongwu commented Nov 17, 2020

jongwu commented Nov 18, 2020

jodh-intel commented Nov 18, 2020

devimc left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 29, 2020 •

edited

Loading

jodh-intel left a comment •

edited

Loading