ci: make crictl install faster and independent of CRI-O by saschagrunert · Pull Request #1778 · kata-containers/tests

saschagrunert · 2019-07-02T12:52:32Z

Fixes #1777

chavafg · 2019-07-02T13:10:14Z

oh cool!
Can you please amend your commit to add a body and the Fixes: #1777?

Thanks.

saschagrunert · 2019-07-02T13:11:30Z

Can you please amend your commit to add a body and the Fixes: #1777?

Yes, sure :)

grahamwhaley · 2019-07-02T13:21:51Z

.ci/install_crio.sh

-sudo install "${GOPATH}/bin/crictl" /usr/bin
-popd
+crictl_version=v1.14.0
+wget -qO- https://github.com/kubernetes-sigs/cri-tools/releases/download/$crictl_version/crictl-$crictl_version-linux-amd64.tar.gz \


@chavafg -do we only test on x86 - or do we need to make this arch independent/tolerant ?

oh, you are right... We currently test only x86, but I know that @Pennyzct is adding support for arm, so I think it would be good to add support for different architectures.
@saschagrunert do you think you can make the change?
I think you can use .ci/kata-arch.sh -g to get the correct arch name.

Alright, I will do it like this.

chavafg · 2019-07-02T14:02:38Z

.ci/install_crio.sh

-go install ./cmd/crictl
-sudo install "${GOPATH}/bin/crictl" /usr/bin
-popd
+crictl_version=v1.14.0


also, can you please move this to https://github.com/kata-containers/tests/blob/master/versions.yaml and then get the value here using something like: crictl_version=$(get_test_version "external.crictl.version")

Yes, I will add that.

GabyCT · 2019-07-03T14:50:12Z

/test-ubuntu

chavafg · 2019-07-04T17:23:43Z

.ci/install_crio.sh

-go install ./cmd/crictl
-sudo install "${GOPATH}/bin/crictl" /usr/bin
-popd
+crictl_version=$(get_test_version "externals.critools.version")


I went through the failure on the ubuntu job and see that it cannot find the versions.yaml file:

10:15:41 ERROR: cannot find .ci/../versions.yaml 10:15:41 Failed at 67: crictl_version=$(get_test_version "externals.critools.version")

@saschagrunert can you please move these lines before we pushd into the cri-o directory (L42)?, so that we are still on our CI working directory and can find the versions.yaml file.

Yes, thanks for pointing me to the issue 🙏

chavafg · 2019-07-05T12:27:01Z

/test

chavafg · 2019-07-05T12:59:57Z

.ci/install_crio.sh

+echo "Get CRI Tools"
+crictl_version=$(get_test_version "externals.critools.version")
+wget -qO- https://github.com/kubernetes-sigs/cri-tools/releases/download/$crictl_version/crictl-$crictl_version-linux-"$(${cidir}/kata-arch.sh -g)".tar.gz \
+	| tar xfz - -C /usr/bin


seems that this needs to be sudo tar ...

Makes sense, I change it.

chavafg · 2019-07-05T13:23:32Z

/test

chavafg · 2019-07-09T00:01:17Z

@saschagrunert I went trough the issues on the CI:
The first one is that we have a failure on one of the tests

not ok 24 ctr execsync exit code
# (in test file ctr.bats, line 918)
#   `[ "$status" -ne 0 ]' failed

Taking a look at the tests, the test fails making an exec:
https://github.com/cri-o/cri-o/blob/v1.14.1/test/ctr.bats#L918

I see here that they changed the behavior of the test in this commit:
cri-o/cri-o@1551436#diff-c5753339854697f7106d65aea5be6788

So maybe the crictl version is not compatible with cri-o v1.14.1 ?
Maybe we should try with cri-o v1.14.5.

The second issue is that we an older version of cri-o (branch release-1.10) on the Fedora job as we run openshift (3.10) there and the v1.14 version of crictl is not compatible with the old cri-o version.
For this one, I think that we can add another entry for the old cri-tool (which is release-1.10)version inside the yaml file.

saschagrunert · 2019-07-09T07:14:37Z

So maybe the crictl version is not compatible with cri-o v1.14.1 ?
Maybe we should try with cri-o v1.14.5.

Yeah might be. I opened up an issue in the runtime repository to bump the version.

saschagrunert · 2019-07-10T07:34:19Z

/test

saschagrunert · 2019-07-10T07:59:50Z

@chavafg I changed the cri tools implementation accordingly to your suggestions, do we want to go for an additional test again? :)

jodh-intel · 2019-07-10T08:35:21Z

.ci/install_crio.sh

-sudo install "${GOPATH}/bin/crictl" /usr/bin
-popd
+echo "Installing CRI Tools"
+wget -qO- "$crictl_url" | tar xfz - -C /usr/bin


I see you are updating already existing code but I think we need a check in here to test for KATA_DEV_MODE as devs probably don't want their /usr/bin/ stamped on (but /usr/local/ would be "fair game" I think).

We avoid wget where possible so please could you convert this to use curl.

I think we could generally use /usr/local, then an additional check would not be needed right?
I changed the command to use curl instead of wget

jodh-intel · 2019-07-10T08:35:24Z

.ci/install_crio.sh

 	else
 		crio_version=$(get_version "externals.crio.version")
+		crictl_version=$(get_test_version "externals.critools.version")
+		crictl_url=https://github.com/kubernetes-sigs/cri-tools/releases/download/$crictl_version/crictl-$crictl_version-linux-"$(${cidir}/kata-arch.sh -g)".tar.gz


These two crictl_url variables look identical but on closer inspection one contains a v as a version prefix and the other doesn't. Is that really correct?

If it is, please could you make this clearer by reworking the code to something like:

version_prefix="" if [ ... ]; then version_prefix="v" # ... else # ... fi cri_tools_site=https://github.com/kubernetes-sigs/cri-tools cri_tools_file="crictl-${crictl_version}-linux-$(${cidir}/kata-arch.sh -g).tar.gz" crictl_url="${cri_tools_site}/releases/download/${version_prefix}${crictl_version}/${cri_tools_file}

Yes, I adapt it as suggested.

jodh-intel · 2019-07-10T08:37:30Z

versions.yaml

+
+  critools:
+    version: v1.14.0
+    openShift3Version: 1.0.0-beta.2


We already encode openshift in the main versions database, so should these details go there?:

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

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

Okay, I will add it there.

Created another PR to address this issue: kata-containers/runtime#1873

grahamwhaley · 2019-07-10T08:47:20Z

@saschagrunert - CI resets when you push changes, so we would need to test again - but, I'll leave triggering that until you assess the feedback from @jodh-intel ;-)

jodh-intel · 2019-07-10T09:03:14Z

.ci/install_crio.sh

-sudo install "${GOPATH}/bin/crictl" /usr/bin
-popd
+echo "Installing CRI Tools"
+crictl_url=https://github.com/kubernetes-sigs/cri-tools/releases/download/v$crictl_version/crictl-$crictl_tag_prefix$crictl_version-linux-"$(${cidir}/kata-arch.sh -g)".tar.gz


This variable still contains v$crictl_version ...?

Yes, this is a little skew (tag vs file name) in cri-tools but it should work fine.

So in the end we have both urls, which are working:

https://github.com/kubernetes-sigs/cri-tools/releases/download/v1.14.0/crictl-v1.14.0-linux-amd64.tar.gz https://github.com/kubernetes-sigs/cri-tools/releases/download/v1.0.0-beta.2/crictl-1.0.0-beta.2-linux-amd64.tar.gz

@saschagrunert can you move the Installing CRI tools section before we pushd into the cri-o repo?

10:58:02 .ci/install_crio.sh: line 70: .ci/kata-arch.sh: No such file or directory 10:58:02 Failed at 70: crictl_url=https://github.com/kubernetes-sigs/cri-tools/releases/download/v$crictl_version/crictl-$crictl_tag_prefix$crictl_version-linux-"$(${cidir}/kata-arch.sh -g)".tar.gz

Hm, alright I moved it above.

jodh-intel · 2019-07-10T09:07:50Z

.ci/install_crio.sh

-popd
+echo "Installing CRI Tools"
+crictl_url=https://github.com/kubernetes-sigs/cri-tools/releases/download/v$crictl_version/crictl-$crictl_tag_prefix$crictl_version-linux-"$(${cidir}/kata-arch.sh -g)".tar.gz
+wget -Ls "$crictl_url" | tar xfz - -C /usr/local/bin


I think you might need to re-push as this is still showing as wget.

Thanks for the hint, you're right. :) Re-pushed my changes.

jodh-intel

Thanks @saschagrunert.

lgtm

chavafg · 2019-07-10T15:14:56Z

/test

chavafg · 2019-07-10T15:48:24Z

10:33:41 .ci/lib.sh: line 121: .ci/install_yq.sh: No such file or directory
10:33:41 ERROR: cannot find .ci/../versions.yaml
10:33:41 Failed at 62: crictl_version=$(get_test_version "externals.critools.version")

I think we need to move the pushd on L43 along with the git commands on L66-67 after the if finishes on L68 to find the files needed to get the versions

saschagrunert · 2019-07-11T07:08:40Z

10:33:41 .ci/lib.sh: line 121: .ci/install_yq.sh: No such file or directory
10:33:41 ERROR: cannot find .ci/../versions.yaml
10:33:41 Failed at 62: crictl_version=$(get_test_version "externals.critools.version")
I think we need to move the pushd on L43 along with the git commands on L66-67 after the if finishes on L68 to find the files needed to get the versions

Yes, I moved it below.

saschagrunert · 2019-07-12T11:58:00Z

@chavafg may I ask you to trigger a /test ? :)

grahamwhaley · 2019-07-12T12:03:26Z

/test

chavafg · 2019-07-15T15:45:03Z

/test

chavafg · 2019-07-15T16:12:19Z

/test

chavafg · 2019-07-15T16:36:32Z

.ci/install_crio.sh

-popd
+echo "Installing CRI Tools"
+crictl_url=https://github.com/kubernetes-sigs/cri-tools/releases/download/v$crictl_version/crictl-$crictl_tag_prefix$crictl_version-linux-"$(${cidir}/kata-arch.sh -g)".tar.gz
+curl -Ls "$crictl_url" | tar xfz - -C /usr/local/bin


can you please add sudo here?

Yes, added it. I thought it was already there 🤔

yep, don't worry, I think this should be fine now 😄
/test

Fixes: kata-containers#1777 Signed-off-by: Sascha Grunert <sgrunert@suse.com>

saschagrunert · 2019-07-15T17:22:37Z

I guess we have to drop another /test here. 😇

chavafg · 2019-07-15T17:27:36Z

ha, the trigger phrase didn't work on the review comment. Thanks for noticing it. Should work now:
/test

saschagrunert · 2019-07-15T18:00:41Z

Hm, the Kernel Build seems to fail:

19:51:41 Failed at 62: "${kernel_repo_dir}/kernel/build-kernel.sh" -v "${kernel_version}" "build"
19:51:47 Failed at 21: "${cidir}/install_kata_kernel.sh"

saschagrunert · 2019-07-16T06:33:40Z

Awesome, it works. Thanks for all the support guys! :)

chavafg · 2019-07-16T13:53:42Z

thanks @saschagrunert

saschagrunert force-pushed the crictl branch from cd9cd77 to cb280ac Compare July 2, 2019 12:53

saschagrunert force-pushed the crictl branch from cb280ac to 9847178 Compare July 2, 2019 13:11

grahamwhaley reviewed Jul 2, 2019

View reviewed changes

chavafg reviewed Jul 2, 2019

View reviewed changes

saschagrunert force-pushed the crictl branch 2 times, most recently from 6b8f0df to 4bd9b07 Compare July 2, 2019 14:19

saschagrunert mentioned this pull request Jul 3, 2019

Bump cri-tools to v1.15.0 cri-o/cri-o#2566

Merged

chavafg reviewed Jul 4, 2019

View reviewed changes

saschagrunert force-pushed the crictl branch from 4bd9b07 to a74d381 Compare July 5, 2019 10:21

chavafg approved these changes Jul 5, 2019

View reviewed changes

chavafg reviewed Jul 5, 2019

View reviewed changes

saschagrunert force-pushed the crictl branch 3 times, most recently from 8bd6bc1 to 2ccfe5e Compare July 5, 2019 13:12

This was referenced Jul 9, 2019

Update CRI-O to v1.14.6 kata-containers/runtime#1866

Closed

versions: update CRI-O to v1.14.6 kata-containers/runtime#1867

Merged

saschagrunert force-pushed the crictl branch from 2ccfe5e to 4c93a79 Compare July 10, 2019 07:13

jodh-intel reviewed Jul 10, 2019

View reviewed changes

saschagrunert force-pushed the crictl branch from 4c93a79 to b761e59 Compare July 10, 2019 09:01

jodh-intel reviewed Jul 10, 2019

View reviewed changes

This was referenced Jul 10, 2019

Add crictl version compatible with OpenShift kata-containers/runtime#1872

Closed

versions: add crictl version which is compatible with OpenShift kata-containers/runtime#1873

Merged

jodh-intel reviewed Jul 10, 2019

View reviewed changes

saschagrunert force-pushed the crictl branch from b761e59 to 89c356b Compare July 10, 2019 09:09

jodh-intel approved these changes Jul 10, 2019

View reviewed changes

saschagrunert force-pushed the crictl branch from 89c356b to 30c0697 Compare July 11, 2019 07:08

saschagrunert force-pushed the crictl branch from 30c0697 to b9e469b Compare July 12, 2019 12:24

saschagrunert force-pushed the crictl branch from b9e469b to a01270e Compare July 15, 2019 16:06

chavafg reviewed Jul 15, 2019

View reviewed changes

ci: make crictl install faster and independent of CRI-O

9251583

Fixes: kata-containers#1777 Signed-off-by: Sascha Grunert <sgrunert@suse.com>

saschagrunert force-pushed the crictl branch from a01270e to 9251583 Compare July 15, 2019 16:48

chavafg merged commit 5d0e2a8 into kata-containers:master Jul 16, 2019

Conversation

saschagrunert commented Jul 2, 2019

Uh oh!

chavafg commented Jul 2, 2019

Uh oh!

saschagrunert commented Jul 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GabyCT commented Jul 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chavafg commented Jul 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chavafg commented Jul 5, 2019

Uh oh!

chavafg commented Jul 9, 2019

Uh oh!

saschagrunert commented Jul 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saschagrunert commented Jul 10, 2019

Uh oh!

saschagrunert commented Jul 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grahamwhaley commented Jul 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jodh-intel left a comment

Choose a reason for hiding this comment

Uh oh!

chavafg commented Jul 10, 2019

Uh oh!

chavafg commented Jul 10, 2019

Uh oh!

saschagrunert commented Jul 11, 2019

saschagrunert commented Jul 9, 2019 •

edited

Loading