build: check golang version meets min req. #806

grahamwhaley · 2018-10-04T10:19:34Z

Check that the system golang version is new enough to build with
according to the data from the versions.yaml file.

Update the verions in the versions.yaml accordingly, and add a note
describing what the 'newest-version' item represents.
Note, we only do a minimum requirement check, and are not checking
against the 'newest-version' info from the yaml.

Fixes: #148

Inspired-by: Wei Zhang zhangwei555@huawei.com
Idea-by: James O. D. Hunt james.o.hunt@intel.com
Signed-off-by: Graham Whaley graham.whaley@intel.com

grahamwhaley · 2018-10-04T10:20:53Z

Replaces #762
@jodh-intel @chavafg and if you are around @WeiZhang555 ptal.
Let's see what the CI thinks of this....
/test

grahamwhaley · 2018-10-04T10:58:04Z

Oh, this has hit the same struct align issue as #744

virtcontainers/pkg/mock/cc_proxy_mock.go:24:18:warning: struct of size 160 could be 152 (maligned)
virtcontainers/monitor.go:15:14:warning: struct of size 80 could be 72 (maligned)
Build step 'Execute shell' marked build as failure
Performing Post build task...

@jodh-intel - did we have a plan?

jodh-intel · 2018-10-05T08:22:43Z

Hi @grahamwhaley - you can steal my fix from #744 for the lint issues but the reason my PR is failing seems to be because cri-containerd's tests don't yet work with the new version of golang :( We could help them fix, but it isn't high up my priority list I'm afraid.

grahamwhaley · 2018-10-05T09:56:27Z

@jodh-intel thanks for the info. Hmm, I suspect both our PRs are going to sit for a short while then until we/somebody can fix up the cri-containerd tests :-( but, at least we found that before merging!

WeiZhang555 · 2018-10-06T05:20:12Z

Thanks @grahamwhaley and @jodh-intel for the picking up! This looks good to me, except that CI doesn't agree with me 😃

amshinde · 2018-10-08T18:56:51Z

Adding dnm till cri tests are fixed.

amshinde · 2018-10-09T00:00:10Z

@grahamwhaley I submitted a fix to cri-containerd that has been merged in master: containerd/cri#941

I am not sure which version of cri we are using, we can consider applying that patch till it becomes part of a release.

jodh-intel · 2018-10-09T08:49:58Z

The cri-containerd version information is in versions.yaml so we should be able to bump the version on this PR to have the tests pull in that version I think:

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

grahamwhaley · 2018-10-09T13:18:20Z

OK, let me see if I can bump that version. But, the patch from @amshinde (btw, thank you! ;-) ) is not in a release yet, and I suspect may not be for some time (hard to tell) - so, I'll have to see if our yaml maybe supports pinning to a commit version.

jodh-intel · 2018-10-09T13:34:05Z

It should do, yes.

grahamwhaley · 2018-10-09T13:36:33Z

/test

grahamwhaley · 2018-10-09T13:41:42Z

Looks like we need to add yq to travis as well.

$ make
make: yq: Command not found
golang.mk:12: *** "ERROR: cannot determine minimum golang version".  Stop.

What's the recommended way there @jodh-intel @chavafg - do I try to add it into the .travis.yml or somehow tie it into .ci/static-checks.sh? I suspect figuring out the 'correct' way to get travis to add packages is preferable?

grahamwhaley · 2018-10-09T13:51:45Z

Hmm, and strange osbuilder fail on 16.04initrd:

Digest: sha256:c26d7183c5d6341ef2eb8a8c7e6e2053a7f9d03e7a156bca2dd17c46629cff44
Status: Downloaded newer image for golang:1.9.2-alpine3.7
 ---> f1c23f11852e
Step 2/2 : RUN apk update && apk add git make bash gcc musl-dev linux-headers apk-tools-static libseccomp libseccomp-dev &&     echo -e '#!/bin/sh\nuname -m' > /usr/bin/arch && chmod +x /usr/bin/arch
 ---> Running in 8985e9408842
fetch http://dl-cdn.alpinelinux.org/alpine/v3.7/main/x86_64/APKINDEX.tar.gz
Build timed out (after 5 minutes). Marking the build as aborted.
Build was aborted

and seems to have hit the no-nesting issue on 16.04 image.
mumble...

grahamwhaley · 2018-10-09T14:11:24Z

and.... seems I blew the line length check for the meta description in the versions.yaml. spinning again...

versions.yaml
  188:81    error    line too long (92 > 80 characters)  (line-length)

grahamwhaley · 2018-10-09T14:12:58Z

/test

grahamwhaley · 2018-10-09T14:28:41Z

I had a look at the Travis item. They show using apt in the before_install section, and yq does not have a standard ubuntu package, but on their page they show either a snap install or adding a ppa like (here to our before_script section):

before_script:
  -sudo add-apt-repository ppa:rmescandon/yq
  -sudo apt update
  -sudo apt install yq -y
...

Thoughts @jodh-intel @chavafg ? I'm ready to add that patch to this PR and spin it...

marcov · 2018-10-09T14:32:04Z

I had a look at the Travis item. They show using apt in the before_install section, and yq does not have a standard ubuntu package, but on their page they show either a snap install or adding a ppa like (here to our before_script section):
before_script:
  -sudo add-apt-repository ppa:rmescandon/yq
  -sudo apt update
  -sudo apt install yq -y
...
Thoughts @jodh-intel @chavafg ? I'm ready to add that patch to this PR and spin it...

fwiw there's a install_yq function in tests .ci/lib.sh you may use too, but I checked and there's no way to call it without some wrapper script.

codecov · 2018-10-09T14:56:09Z

Codecov Report

❗ No coverage uploaded for pull request base (master@58ce1b8). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #806   +/-   ##
=========================================
  Coverage          ?   65.66%           
=========================================
  Files             ?       87           
  Lines             ?    10798           
  Branches          ?        0           
=========================================
  Hits              ?     7090           
  Misses            ?     3002           
  Partials          ?      706

grahamwhaley · 2018-10-10T09:34:52Z

/test
let's see if the yq travis apt fix works...

marcov · 2018-10-15T13:37:10Z

@grahamwhaley, about the travis failure, it's failing because before_script is run after install. before_install seems to be the right one.
https://docs.travis-ci.com/user/customizing-the-build/

raravena80 · 2018-10-22T16:47:25Z

@grahamwhaley ping :)

grahamwhaley · 2018-10-30T10:47:47Z

Updated. rebased to pick up the relevant golang version.yaml changes. Bumped the known max version from 1.11 to 1.11.1. Moved the travis apt yq init into the 'install' section, so let's see if that fixes travis.
/cc @marcov @jodh-intel

marcov · 2018-10-30T14:10:48Z

@grahamwhaley Travis still failing, you need to add the -y flag to both apt and add-apt-repository to avoid them waiting for a user input.

grahamwhaley · 2018-10-30T14:19:44Z

Thanks @marcov . A quick surf says that -y for update is redundant (but accepted as a global option), as update does not prompt for anything ever - but, I went 'full hog' and added -y -qq to all three lines. Let's see how it flies...

grahamwhaley · 2018-10-30T14:28:42Z

Well, I guess that didn't fly - try again...

add-apt-repository: error: no such option: -q
The command "sudo add-apt-repository -y -qq ppa:rmescandon/yq" failed and exited with 2 during

grahamwhaley · 2018-10-30T14:36:31Z

/me starts to wave arms.....
OK, is this telling me that there is no yq package for trusty - probably coz we'd really like to be on xenial, but Travis is not iirc....? @marcov @jodh-intel

gpg: keyring `/tmp/tmp8gq8qnjl/secring.gpg' created
gpg: keyring `/tmp/tmp8gq8qnjl/pubring.gpg' created
gpg: requesting key CC86BB64 from hkp server keyserver.ubuntu.com
gpg: /tmp/tmp8gq8qnjl/trustdb.gpg: trustdb created
gpg: key CC86BB64: public key "Launchpad PPA for Roberto Mier Escandón " imported
gpg: Total number processed: 1
gpg:               imported: 1  (RSA: 1)
OK
7.59s$ sudo apt update -y -qq
W: http://ppa.launchpad.net/couchdb/stable/ubuntu/dists/trusty/Release.gpg: Signature by key 15866BAFD9BCC4F3C1E0DFC7D69548E1C17EAB57 uses weak digest algorithm (SHA1)
E: The repository 'http://ppa.launchpad.net/rmescandon/yq/ubuntu trusty Release' does not have a Release file.
The command "sudo apt update -y -qq" failed and exited with 100 during .

marcov · 2018-10-30T14:53:57Z

@grahamwhaley hehe
The problem is that Travis config specifies trusty as distro, but this yq repo only provide packages for xenial and bionic (see here).

trusty is quite old (2014), could we bump the travis config to xenial or bionic (they are both LTS)?

grahamwhaley · 2018-10-30T14:58:30Z

Yep, I vote we try to bump Travis, presuming a newer version is available.
When we tried this waaaay back on ClearContainers iirc we ran into some issues, but that was like >18months ago probably, and hopefully the world has moved on a bit since then.
What do you think @jodh-intel @chavafg - go directly to a bionic if we can, otherwise try a xenial?

This feels like a new PR to me which I'm happy to try as a pre-cursor to getting this PR landed on day.

marcov · 2018-10-30T14:59:51Z

Unluckily we are still stuck in 2014: travis-ci/travis-ci#9460

grahamwhaley · 2018-10-30T15:07:47Z

Wow, ouch. mumble 😿
/me feels rather sad....
https://docs.travis-ci.com/user/reference/overview/#sudo-enabled

Well, much though I'd like to say we will move off of Travis, I don't think that is happening right now (but, I think that was just another nail in the coffin ⚰️
I'll see if I can find another way to get yq installed here, most likely from the go source.

marcov · 2018-10-30T15:09:16Z

@grahamwhaley, I'd suggest adding a before_install step that just install yq using a bash script. You can take inspiration from tests/.ci/lib.sh install_yq function.

We need to have `yq` installed before we can 'make', as we now use it for a version check in the build. But, we may not have golang installed. Add a script that installs `yq` via curl'ing from the github releases. This was cloned from the function in the tests repo .ci scripts that perform the same action. Signed-off-by: Graham Whaley <graham.whaley@intel.com>

Install `yq` before running the tests. The Makefile now uses `yq` to check the golang version against the versions file. Signed-off-by: Graham Whaley <graham.whaley@intel.com>

Check that the system golang version is new enough to build with according to the data from the `versions.yaml` file. Update the verions in the versions.yaml accordingly, and add a note describing what the 'newest-version' item represents. Note, we only do a minimum requirement check, and are not checking against the 'newest-version' info from the yaml. Fixes: kata-containers#148 Inspired-by: Wei Zhang <zhangwei555@huawei.com> Idea-by: James O. D. Hunt <james.o.hunt@intel.com> Signed-off-by: Graham Whaley <graham.whaley@intel.com>

grahamwhaley · 2018-10-30T15:48:16Z

/me hacks up a script, and fires off travis again - I will not be surprised if this gives me something that still needs to be fixed up.

grahamwhaley · 2018-10-30T16:06:06Z

Golly, travis seems to have passed!
/test

grahamwhaley · 2018-10-30T18:14:33Z

Centos 7.4 CI failed in a strange manner - I'll nudge a rebuild....

Stderr: 
Running command '/usr/bin/docker [docker ps -a -f name=6hzLfwdjtT2qHNBymvssMAasZZBmP5 --format {{.Status}}]'
[docker ps -a -f name=6hzLfwdjtT2qHNBymvssMAasZZBmP5 --format {{.Status}}]
Timeout: 60 seconds
Exit Code: 0
Stdout: 
Stderr: 
•

Summarizing 1 Failure:

[Fail] CPUs and CPU set updating cpus and cpuset of a running container [It] should have the right number of vCPUs 
/tmp/jenkins/workspace/kata-containers-runtime-centos-7-4-PR/go/src/github.com/kata-containers/tests/integration/docker/cpu_test.go:432

Ran 208 of 220 Specs in 1316.180 seconds
FAIL! -- 207 Passed | 1 Failed | 0 Pending | 12 Skipped --- FAIL: TestIntegration (1354.25s)
FAIL

sboeuf · 2018-11-05T19:44:22Z

@jodh-intel @WeiZhang555 PTAL and merge if you're fine with this :)

WeiZhang555 · 2018-11-06T08:11:28Z

LGTM

Thanks!

jodh-intel · 2019-07-04T15:44:25Z

Since it was probably copied from #762, note that the issue number in the commit is incorrect. Anyone know what it should be?

grahamwhaley · 2019-07-04T15:55:42Z

Golang 1.10 is mentioned as an issue in #148
So, maybe there should have been another Issue to fix just the go 1.10 item - I dunno - better ask the owners of #148 if it is a done deal or not.

For docker in docker scenario, the nested container created has entry "b *:* m" in the list of devices it is allowed to access under /sys/fs/cgroup/devices/docker/{ctrid}/devices.list. This entry was causing issues while starting a nested container as we were denying "m" access to the rootfs block devices. With this change we add back "m" access, the container would be allowed to create a device node for the rootfs device but will not have read-write access to the created device node. This fixes the docker in docker use case while still making sure the container is not allowed read/write access to the rootfs. Note, this could also be fixed by simply skipping {"Type : "b"} while creating the device cgroup with libcontainer. But this seems to be undocumented behaviour at this point, hence refrained from taking this approach. Fixes kata-containers#806 Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>

amshinde added the do-not-merge label Oct 8, 2018

amshinde removed the do-not-merge label Oct 9, 2018

grahamwhaley mentioned this pull request Oct 9, 2018

versions: Update golang to 1.10.4 #744

Merged

grahamwhaley force-pushed the 20181002_golang_110 branch from 91b98f3 to 44fd7e5 Compare October 9, 2018 14:12

grahamwhaley force-pushed the 20181002_golang_110 branch from 44fd7e5 to ed694f8 Compare October 10, 2018 09:32

WeiZhang555 mentioned this pull request Oct 23, 2018

build: add restriction Golang >= 1.10 #762

Closed

grahamwhaley force-pushed the 20181002_golang_110 branch from ed694f8 to b318759 Compare October 30, 2018 10:46

grahamwhaley force-pushed the 20181002_golang_110 branch from b318759 to 48b8f50 Compare October 30, 2018 14:18

grahamwhaley force-pushed the 20181002_golang_110 branch from 48b8f50 to 6ef5b21 Compare October 30, 2018 14:30

Graham Whaley added 3 commits October 30, 2018 15:44

CI: travis: call yq installer

eaa5c7a

Install `yq` before running the tests. The Makefile now uses `yq` to check the golang version against the versions file. Signed-off-by: Graham Whaley <graham.whaley@intel.com>

grahamwhaley force-pushed the 20181002_golang_110 branch from 6ef5b21 to 95f4fdb Compare October 30, 2018 15:47

sboeuf approved these changes Nov 5, 2018

View reviewed changes

WeiZhang555 merged commit d895cd0 into kata-containers:master Nov 6, 2018

build: check golang version meets min req. #806

build: check golang version meets min req. #806

Conversation

grahamwhaley commented Oct 4, 2018

grahamwhaley commented Oct 4, 2018

grahamwhaley commented Oct 4, 2018

jodh-intel commented Oct 5, 2018

grahamwhaley commented Oct 5, 2018

WeiZhang555 commented Oct 6, 2018

amshinde commented Oct 8, 2018

amshinde commented Oct 9, 2018

jodh-intel commented Oct 9, 2018

grahamwhaley commented Oct 9, 2018

jodh-intel commented Oct 9, 2018

grahamwhaley commented Oct 9, 2018

grahamwhaley commented Oct 9, 2018

grahamwhaley commented Oct 9, 2018

grahamwhaley commented Oct 9, 2018

grahamwhaley commented Oct 9, 2018

grahamwhaley commented Oct 9, 2018

marcov commented Oct 9, 2018

codecov bot commented Oct 9, 2018 • edited Loading

Codecov Report

grahamwhaley commented Oct 10, 2018

marcov commented Oct 15, 2018

raravena80 commented Oct 22, 2018

grahamwhaley commented Oct 30, 2018

marcov commented Oct 30, 2018

grahamwhaley commented Oct 30, 2018

grahamwhaley commented Oct 30, 2018

grahamwhaley commented Oct 30, 2018

marcov commented Oct 30, 2018

grahamwhaley commented Oct 30, 2018

marcov commented Oct 30, 2018

grahamwhaley commented Oct 30, 2018

marcov commented Oct 30, 2018

grahamwhaley commented Oct 30, 2018

grahamwhaley commented Oct 30, 2018

grahamwhaley commented Oct 30, 2018

sboeuf commented Nov 5, 2018

WeiZhang555 commented Nov 6, 2018 • edited by amshinde Loading

jodh-intel commented Jul 4, 2019

grahamwhaley commented Jul 4, 2019

codecov bot commented Oct 9, 2018 •

edited

Loading

WeiZhang555 commented Nov 6, 2018 •

edited by amshinde

Loading