ci: Add installation scripts #71

GabyCT · 2018-02-01T19:32:08Z

This will install runtime, shim, agent, proxy, qemu, and all dependencies
to run kata-containers.

Fixes #70

Signed-off-by: Gabriela Cervantes [email protected]

jodh-intel · 2018-02-02T08:12:40Z

.ci/install_kata_image.sh

+go get -d ${osbuilder_repo} || true
+
+pushd "${GOPATH}/src/${osbuilder_repo}/rootfs-builder"
+script -fec 'sudo -E GOPATH=$GOPATH USE_DOCKER=true ./rootfs.sh clearlinux'


Can you remove the script -fec calls they don't really add much when called from the CI.

Could you make a variable for clearlinux as I think these scripts should be callable by developers.
Something like:

OSBUILDER_DISTRO=${OSBUILDER_DISTRO:-clearlinux}

jodh-intel · 2018-02-02T08:33:36Z

.ci/install_kata_image.sh

+# Install the image
+commit=$(git log --format=%h -1 HEAD)
+date=$(date +%Y-%m-%d-%T.%N%z)
+image="kata-containers-${date}-${commit}"


My naming might be overkill here :-), but if we are going to use this, assuming users might want to run this script, how about making it:

agent_commit=$("$GOPATH/src/github.com/kata-containers/agent/kata-agent" --version | awk '{print $NF}') image="kata-containers-${date}-osbuilder-${commit}-agent-${agent_commit}"

Admittedly, that's going to give a pretty long filename, but it captures some useful information (particularly since the agent is crucial and that agent version will include "dirty" if the code has been locally-modified).

Note that we could simplify this a lot if we implemented:

Consider creating a minimal summary file *inside* the image osbuilder#35

jodh-intel · 2018-02-02T08:34:18Z

.ci/install_kata_image.sh

+date=$(date +%Y-%m-%d-%T.%N%z)
+image="kata-containers-${date}-${commit}"
+
+sudo install -o root -g root -m 0640 -D kata-containers.img "/usr/share/kata-containers/${image}"


Could you create a variable for kata-containers.img as it's used multiple times?

jodh-intel · 2018-02-02T08:34:41Z

.ci/install_kata_kernel.sh

+repo_name="linux"
+cc_linux_releases_url="https://github.com/${repo_owner}/${repo_name}/releases"
+#fake repository dir to query kernel version from remote
+fake_repo_dir=$(mktemp -t -d cc-kernel.XXXX)


I appreciate we're using the CC kernel for now, but I'd still drop the cc- here (or change it to kata-).

jodh-intel · 2018-02-02T09:59:04Z

.ci/install_kata_kernel.sh

+# SPDX-License-Identifier: Apache-2.0
+#
+
+repo_owner="clearcontainers"


This needs a comment explaining that we're using CC kernels for now as we haven't started creating kata kernels yet.

I've raised an issue on the new Kata packaging repo for this btw (kata-containers/packaging#1), so could you add a "see $url" for that issue?

jodh-intel · 2018-02-02T11:10:42Z

.ci/setup_env_ubuntu.sh

+
+if [ "$VERSION_ID" == "16.04" ]; then
+	echo "Install os-tree"
+	sudo -E add-apt-repository ppa:alexlarsson/flatpak -y


I just checked and this is available for all arches ;)

jodh-intel · 2018-02-02T11:11:38Z

.ci/setup_env_ubuntu.sh

+sudo -E apt install -y libostree-dev
+
+if ! command -v docker > /dev/null; then
+	"${cidir}/../cmd/container-manager/manage_ctr_mgr.sh" docker install


This tool needs to be imported into kata, but note that a fix for the following is going to be required:

manage_ctr_mgr.sh assumes amd64 clearcontainers/tests#881

@jodh-intel , I removed the comment about this issue

I think this is the last issue then as this won't work currently afaics?

jodh-intel · 2018-02-02T11:19:36Z

.ci/install_kata_kernel.sh

+	local binaries_dir="${version}-binaries"
+	local binaries_tarball="${binaries_dir}.tar.gz"
+	local shasum_file="SHA512SUMS"
+	curl -OL "${cc_linux_releases_url}/download/${version}/${binaries_tarball}"


This is architecture-specific and will break if arch != x86_64.

@jcvenegas - could you raise a bug and update your tooling so that the binaries file includes x86_64 in the name please?

jodh-intel · 2018-02-02T11:22:22Z

.ci/install_qemu_lite.sh

+qemu_lite_version="$2"
+distro="$3"
+qemu_lite_bin="qemu-lite-bin-${qemu_lite_version}.x86_64.rpm"
+qemu_lite_data="qemu-lite-data-${qemu_lite_version}.x86_64.rpm"


Again, use $arch here.

jodh-intel · 2018-02-02T11:22:37Z

.ci/install_qemu_lite.sh

+echo -e "Install qemu-lite ${qemu_lite_version}"
+
+# download packages
+curl -LO "https://download.clearlinux.org/releases/${clear_release}/clear/x86_64/os/Packages/${qemu_lite_bin}"


Two more places to use $arch.

GabyCT · 2018-02-02T16:36:27Z

@jodh-intel changes applied

jodh-intel · 2018-02-05T11:08:21Z

.ci/install_kata_kernel.sh

+}
+
+function download_kernel() {
+	local version="$1"


I think we need a local arch="$2 here.

Alternatively, make this function check arch (or maybe go env GOARCH) and download the correct architecture version.

See:

image + kernel assets should be uploaded to github with architecture-specific names clearcontainers/packaging#264

Download assets based on architecture clearcontainers/tests#889

/cc @jcvenegas.

This issue is still outstanding.

@jodh-intel done

jodh-intel · 2018-02-05T11:10:28Z

.ci/install_runtime.sh

+export V=1
+
+# tell the runtime build to use sane defaults
+export CC_SYSTEM_BUILD="yes"


This should now be,

export SYSTEM_BUILD_TYPE=kata

jodh-intel · 2018-02-05T11:11:54Z

.ci/install_runtime.sh

+fi
+
+echo "Enabling global logging for runtime in file ${runtime_config_path}"
+sudo sed -i -e 's/^#\(\[runtime\]\|global_log_path =\)/\1/g' "${runtime_config_path}"


This isn't needed any more as all components now log to the system journal.

jodh-intel · 2018-02-05T11:12:12Z

.ci/install_runtime.sh

+sudo sed -i -e 's/^#\(enable_debug\).*=.*$/\1 = true/g' "${runtime_config_path}"
+
+echo "Add runtime as a new/default Docker runtime. Docker version \"$(docker --version)\" could change according to updates."
+docker_options="-D --add-runtime kata-runtime=/usr/local/bin/kata-runtime --default-runtime=kata-runtime"


/cc @grahamwhaley.

As discussed here: #71 (comment)
We can remove the --default-runtime flag

jodh-intel · 2018-02-05T11:14:19Z

.ci/setup_env_ubuntu.sh

@@ -0,0 +1,69 @@
+#!/bin/bash -e


What about https://github.com/clearcontainers/tests/blob/master/.ci/setup_env_fedora.sh ? Is the plan to add that after this PR lands maybe?

@jodh-intel yes that is the idea :)

jodh-intel · 2018-02-05T11:41:18Z

.ci/lib.sh

+	clone_and_build $1 $2
+	pushd "${GOPATH}/src/${1}"
+	echo "Install repository ${1}"
+	if [ $1 == "github.com/kata-containers/runtime" ]; then


Again, I think this can become one line as this will work for all repos:

sudo -E PATH=$PATH KATA_RUNTIME=${KATA_RUNTIME} make install

jodh-intel · 2018-02-05T11:41:48Z

.ci/lib.sh

+	popd
+}
+
+function get_cc_versions(){


This needs a comment explaining why we are querying the Clear Containers runtime repo.

jodh-intel · 2018-02-05T11:45:19Z

.ci/setup_env_ubuntu.sh

+		sudo modprobe kvm_intel nested=1
+	fi
+else
+	die "Unsupported architecture: $arch"


Not defined? In fact, could you maybe add this function to lib.sh as it's so useful? :)

@jodh-intel, I already did it

Great, thanks.

jodh-intel · 2018-02-05T11:46:05Z

.ci/setup.sh

+if [ "$ID" == ubuntu ];then
+	bash -f "${cidir}/setup_env_ubuntu.sh"
+else
+	echo >&2 "ERROR: Unrecognised distribution."


This could be a die call if you add die to lib.sh and source it before this line.

jodh-intel · 2018-02-05T11:47:39Z

.ci/setup_env_ubuntu.sh

+sudo -E apt install -y libostree-dev
+
+if ! command -v docker > /dev/null; then
+	# This will be fixed by https://github.com/clearcontainers/tests/issues/881


I don't think you need this comment actually. Or if you leave it, could you explain what will be fixed by that issue?

What would be useful to note is that this sets the default Docker runtime to kata-runtime.

/cc @grahamwhaley.

So I'll ask the question - should or do we need to have kata-runtime as the default runtime?
Most/many of our tests use the RUNTIME env varialble to determine which runtime to pass to --runtime=xxx on the docker command line - and, if they don't, they should, to make it easier for us to have multiple runtimes installed for checking etc., without having to have them as the default runtime.

I suspect if we change this right now maybe a couple of tests fail - but that's probably the only way we'll find such tests. I propose we do not make our runtime the default runtime, and ensure our test frameworks can cope with being told which runtime they should invoke during testing.

FYI, afaik all the metrics tests pick up RUNTIME from the env. (but I do suspect there might be one or two we don't run often or under automation that still need updating).

Agreed. And in fact if we rely on RUNTIME, we could always run the entire suite of tests multiples times without having to reconfigure Docker/k8s:

for runtime in runc kata-runtime-cc kata-runtime-runv do # run tests... done

Any further thoughts on this @chavafg ?

Agree with this approach.

jodh-intel · 2018-02-05T11:48:16Z

@jcvenegas, @grahamwhaley - could you take a look at this please?

GabyCT · 2018-02-06T17:36:25Z

@jodh-intel changes applied

jodh-intel

Thanks for updating @GabyCT. There are still a few outstanding issues to look at.

jodh-intel · 2018-02-07T10:11:36Z

.ci/install_kata_kernel.sh

+function cleanup {
+	rm  -rf "${fake_repo_dir}"
+}
+trap cleanup EXIT


Nit: missing newline.

jodh-intel · 2018-02-07T10:12:27Z

.ci/install_kata_kernel.sh

+function usage() {
+	cat << EOT
+Usage: $0 <version>
+Install the containers clear kernel image <version> from "${repo_owner}"/"${repo_name}".


Nit: You could remove some double-quotes here:

... from "${repo_owner}/${repo_name}".

jodh-intel · 2018-02-07T10:13:07Z

.ci/install_kata_kernel.sh

+function get_latest_version {
+	pushd "${fake_repo_dir}" >> /dev/null
+	git init -q
+	git remote add origin  https://github.com/clearcontainers/linux.git


This needs to use $repo_owner and $repo_name.

jodh-intel · 2018-02-07T10:13:45Z

.ci/install_kata_kernel.sh

+}
+
+function download_kernel() {
+	local version="$1"


This issue is still outstanding.

jodh-intel · 2018-02-07T10:16:47Z

.ci/install_runtime.sh

+	docker_https_proxy="HTTPS_PROXY=$https_proxy"
+fi
+
+cat << EOF | sudo tee ${config_path}/clear-containers.conf


s/clear-containers/kata-containers/.

But shouldn't we be using https://github.com/clearcontainers/tests/blob/master/cmd/container-manager/manage_ctr_mgr.sh here instead (in other words import that cmd on this PR too)?

jodh-intel · 2018-02-07T10:18:07Z

.ci/setup_env_ubuntu.sh

@@ -0,0 +1,69 @@
+#!/bin/bash -e


jodh-intel · 2018-02-07T10:18:17Z

.ci/setup_env_ubuntu.sh

+		sudo modprobe kvm_intel nested=1
+	fi
+else
+	die "Unsupported architecture: $arch"


Great, thanks.

GabyCT · 2018-02-07T15:55:10Z

@jodh-intel changes applied

GabyCT · 2018-02-07T16:41:35Z

@jodh-intel I remove the last lines as they did not work (as you mentioned) thanks :)

jodh-intel · 2018-02-07T17:07:16Z

Hi @GabyCT - I think you just need to add the code for manage_ctr_mgr.sh and remove the manual configuration you're doing for Docker.

GabyCT · 2018-02-07T17:13:12Z

@jodh-intel the code for manage_ctr_mgr.sh should be part of the .ci scripts or should I put it in another place? also I see that this code is doing a configuration for swarm should this be removed?

jodh-intel · 2018-02-07T17:19:34Z

Hi @GabyCT - I think you just need to copy the files in https://github.com/clearcontainers/tests/tree/master/cmd/container-manager into this PR (under the same location [`cmd/container-manager/]). You could do that on a separate commit but as the CI scripts are using the tool, it could be on the single commit you already have I think.

@chavafg - does that sound ok with you?

chavafg · 2018-02-07T17:40:59Z

@GabyCT @jodh-intel yes, sounds good, also we would need to open an issue here for the arm support, and close in cc test repo?

GabyCT · 2018-02-07T21:35:10Z

@chavafg and @jodh-intel changes applied

jodh-intel · 2018-02-08T07:47:47Z

Thanks @GabyCT, @chavafg.

lgtm

jodh-intel · 2018-02-08T07:53:51Z

Hmm, pullapprove isn't seeing my ack. Let's try again...

lgtm

This will install runtime, shim, agent, proxy, qemu, and all dependencies to run kata-containers. Fixes kata-containers#70 Signed-off-by: Gabriela Cervantes <[email protected]>

chavafg · 2018-02-08T14:39:44Z

.ci/setup_env_ubuntu.sh

@@ -0,0 +1,64 @@
+#!/bin/bash -e


While testing this PR found that the -e didn't had any effect when we call the script using:
bash -f ${cidir}/setup_env_ubuntu.sh" , when errors appeared, the setup continued without failing.
So I would recommend that we explicitly add set -e to every script.

One thing I've considered doing to the metrics scripts is invoking all (sub)scripts with bash -$- ...., which then passes on the parent script settings to the child. That way if you set, say, -x on the top level script for debug, it will trickle through to all sub-scripts. Maybe that will work well for your -e situation ?

chavafg · 2018-02-08T14:42:11Z

.ci/setup_env_ubuntu.sh

+"${cidir}/install_qemu_lite.sh" "${qemu_lite_clear_release}" "${qemu_lite_sha}" "$ID"
+
+echo "Install kata-containers image"
+"${cidir}/install_kata_image.sh"


When we run this script, we already need docker, and if it is not installed, this will fail. We should install docker using the manage_ctr_mgr.sh script before calling install_kata_image.sh.

chavafg · 2018-02-08T14:44:36Z

cmd/container-manager/manage_ctr_mgr.sh

@@ -0,0 +1,321 @@
+#!/bin/bash -e


Add execution permissions to this script

GabyCT · 2018-02-08T16:06:49Z

@chavafg changes applied

GabyCT force-pushed the topic/ci branch from 3e5fe1e to c233522 Compare February 1, 2018 19:34

jodh-intel mentioned this pull request Feb 2, 2018

manage_ctr_mgr.sh assumes amd64 clearcontainers/tests#881

Closed

jodh-intel suggested changes Feb 2, 2018

View reviewed changes

GabyCT force-pushed the topic/ci branch 2 times, most recently from 224105b to 8e1120a Compare February 2, 2018 16:34

GabyCT force-pushed the topic/ci branch 4 times, most recently from 854d028 to b8ed8ce Compare February 2, 2018 22:29

This was referenced Feb 5, 2018

image + kernel assets should be uploaded to github with architecture-specific names clearcontainers/packaging#264

Open

Download assets based on architecture clearcontainers/tests#889

Open

jodh-intel reviewed Feb 5, 2018

View reviewed changes

GabyCT force-pushed the topic/ci branch from b8ed8ce to 62283dd Compare February 6, 2018 17:35

jodh-intel reviewed Feb 7, 2018

View reviewed changes

GabyCT force-pushed the topic/ci branch 2 times, most recently from b3bd210 to f6a7270 Compare February 7, 2018 15:54

GabyCT force-pushed the topic/ci branch from f6a7270 to c3b7029 Compare February 7, 2018 16:40

GabyCT force-pushed the topic/ci branch from c3b7029 to 47fdfe2 Compare February 7, 2018 21:33

jodh-intel approved these changes Feb 8, 2018

View reviewed changes

jodh-intel mentioned this pull request Feb 8, 2018

manage_ctr_mgr.sh assumes amd64 #76

Closed

ci: Add installation scripts

00b4b25

This will install runtime, shim, agent, proxy, qemu, and all dependencies to run kata-containers. Fixes kata-containers#70 Signed-off-by: Gabriela Cervantes <[email protected]>

chavafg suggested changes Feb 8, 2018

View reviewed changes

GabyCT force-pushed the topic/ci branch 2 times, most recently from efc859c to 00b4b25 Compare February 8, 2018 16:06

chavafg approved these changes Feb 8, 2018

View reviewed changes

jcvenegas approved these changes Feb 8, 2018

View reviewed changes

chavafg merged commit 1797f61 into kata-containers:master Feb 8, 2018

jodh-intel mentioned this pull request Feb 9, 2018

Add a fedora CI setup script #79

Closed

GabyCT deleted the topic/ci branch November 5, 2019 15:41

ci: Add installation scripts #71

ci: Add installation scripts #71

Conversation

GabyCT commented Feb 1, 2018

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

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

GabyCT commented Feb 2, 2018

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

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

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

jodh-intel commented Feb 5, 2018

GabyCT commented Feb 6, 2018

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

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

GabyCT commented Feb 7, 2018

GabyCT commented Feb 7, 2018

jodh-intel commented Feb 7, 2018

GabyCT commented Feb 7, 2018

jodh-intel commented Feb 7, 2018

chavafg commented Feb 7, 2018

GabyCT commented Feb 7, 2018

jodh-intel commented Feb 8, 2018

jodh-intel commented Feb 8, 2018

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

GabyCT commented Feb 8, 2018