Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

versions: Update golang to 1.10.4 #744

Merged

Conversation

jodh-intel
Copy link
Contributor

Move to golang version 1.10.4 -- the oldest stable golang release at the
time of writing -- since golang 1.10+ is needed to make namespace
handling safe.

See:

Also bumped languages.golang.meta.newest-version to golang version
1.11, which is the newest stable release at the time of writing.

Fixes #148.

Signed-off-by: James O. D. Hunt [email protected]

@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 162533 KB
Proxy: 4509 KB
Shim: 9259 KB

Memory inside container:
Total Memory: 2043460 KB
Free Memory: 2006704 KB

Copy link
Contributor

@grahamwhaley grahamwhaley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@grahamwhaley
Copy link
Contributor

But I think it blew the CIs... for instance, Centos 7.4:

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

@egernst
Copy link
Member

egernst commented Sep 18, 2018

Like the idea - let's see if the CI can be agreeable.

@caoruidong
Copy link
Member

I think this can only be fixed by reordering field in this structs.

Copy link

@sboeuf sboeuf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jodh-intel looks good to me, but you need to rework some of the Go structures there if you want to make maligned happy ;)

@caoruidong
Copy link
Member

+1. Due to golang release policy, 1.9 is not supported since 1.11 is released.

@gnawux
Copy link
Member

gnawux commented Sep 20, 2018

lgtm

I think the maligned crying is not related to this patch, right?

Approved with PullApprove

@jodh-intel
Copy link
Contributor Author

/me tweaks branch to hopefully stop the linters bleating...

@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 166579 KB
Proxy: 4398 KB
Shim: 9303 KB

Memory inside container:
Total Memory: 2043460 KB
Free Memory: 2006892 KB

@jodh-intel
Copy link
Contributor Author

jodh-intel commented Sep 20, 2018

CI failed due to a factory test failure:

--- FAIL: TestNewFactory (0.01s)
    testing.go:771: race detected during execution of test
==================
WARNING: DATA RACE
Read at 0x00c0000301e8 by goroutine 7:
  internal/race.Read()
      /usr/local/go/src/internal/race/race.go:37 +0x38
  sync.(*WaitGroup).Add()
      /usr/local/go/src/sync/waitgroup.go:71 +0x16b
  github.com/kata-containers/runtime/virtcontainers/factory/cache.New()
      /tmp/jenkins/workspace/kata-containers-runtime-ubuntu-16-04-PR/go/src/github.com/kata-containers/runtime/virtcontainers/factory/cache/cache.go:37 +0x176
  github.com/kata-containers/runtime/virtcontainers/factory.NewFactory()
      /tmp/jenkins/workspace/kata-containers-runtime-ubuntu-16-04-PR/go/src/github.com/kata-containers/runtime/virtcontainers/factory/factory.go:73 +0x385
  github.com/kata-containers/runtime/virtcontainers/factory.TestNewFactory()
      /tmp/jenkins/workspace/kata-containers-runtime-ubuntu-16-04-PR/go/src/github.com/kata-containers/runtime/virtcontainers/factory/factory_test.go:60 +0x759
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:827 +0x162

Previous write at 0x00c0000301e8 by goroutine 9:
  internal/race.Write()
      /usr/local/go/src/internal/race/race.go:41 +0x38
  sync.(*WaitGroup).Wait()
      /usr/local/go/src/sync/waitgroup.go:128 +0xef
  github.com/kata-containers/runtime/virtcontainers/factory/cache.(*cache).CloseFactory.func1()
      /tmp/jenkins/workspace/kata-containers-runtime-ubuntu-16-04-PR/go/src/github.com/kata-containers/runtime/virtcontainers/factory/cache/cache.go:80 +0x129
  sync.(*Once).Do()
      /usr/local/go/src/sync/once.go:44 +0xde
  github.com/kata-containers/runtime/virtcontainers/factory/cache.(*cache).CloseFactory()
      /tmp/jenkins/workspace/kata-containers-runtime-ubuntu-16-04-PR/go/src/github.com/kata-containers/runtime/virtcontainers/factory/cache/cache.go:76 +0x7a
  github.com/kata-containers/runtime/virtcontainers/factory/cache.New.func1()
      /tmp/jenkins/workspace/kata-containers-runtime-ubuntu-16-04-PR/go/src/github.com/kata-containers/runtime/virtcontainers/factory/cache/cache.go:43 +0x28b

Goroutine 7 (running) created at:
  testing.(*T).Run()
      /usr/local/go/src/testing/testing.go:878 +0x650
  testing.runTests.func1()
      /usr/local/go/src/testing/testing.go:1119 +0xa8
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:827 +0x162
  testing.runTests()
      /usr/local/go/src/testing/testing.go:1117 +0x4ee
  testing.(*M).Run()
      /usr/local/go/src/testing/testing.go:1034 +0x2ee
  main.main()
      _testmain.go:98 +0x332

Goroutine 9 (finished) created at:
  github.com/kata-containers/runtime/virtcontainers/factory/cache.New()
      /tmp/jenkins/workspace/kata-containers-runtime-ubuntu-16-04-PR/go/src/github.com/kata-containers/runtime/virtcontainers/factory/cache/cache.go:38 +0x1e0
  github.com/kata-containers/runtime/virtcontainers/factory.NewFactory()
      /tmp/jenkins/workspace/kata-containers-runtime-ubuntu-16-04-PR/go/src/github.com/kata-containers/runtime/virtcontainers/factory/factory.go:73 +0x385
  github.com/kata-containers/runtime/virtcontainers/factory.TestNewFactory()
      /tmp/jenkins/workspace/kata-containers-runtime-ubuntu-16-04-PR/go/src/github.com/kata-containers/runtime/virtcontainers/factory/factory_test.go:60 +0x759
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:827 +0x162
==================

/cc @bergwolf.

@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 170312 KB
Proxy: 4487 KB
Shim: 9131 KB

Memory inside container:
Total Memory: 2043460 KB
Free Memory: 2006828 KB

@jodh-intel
Copy link
Contributor Author

Sigh - go 1.10 is spotting issues in other projects code and failing our tests ;((

# github.com/containerd/cri/integration
integration/image_load_test.go:39: T.Skip call has possible formatting directive %v

@jodh-intel
Copy link
Contributor Author

/test

@jodh-intel
Copy link
Contributor Author

/me watches the CI turbines start spinning...

@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 166263 KB
Proxy: 4536 KB
Shim: 9301 KB

Memory inside container:
Total Memory: 2043448 KB
Free Memory: 2006820 KB

@grahamwhaley
Copy link
Contributor

CI turbines bombed @jodh-intel Is this still something you will pursue?

@bergwolf
Copy link
Member

bergwolf commented Oct 9, 2018

@jodh-intel Any updates?

@grahamwhaley
Copy link
Contributor

@bergwolf See #806 - @amshinde has posted some patches upstream to containerd/cri - we need to get them pulled in so we can bump the golang version and not fail the build.

@bergwolf
Copy link
Member

@grahamwhaley Are you referring to containerd/cri#941 ? It is already merged. Now we can update cri/containerd dependency and then bump golang version. cc @jodh-intel

@grahamwhaley
Copy link
Contributor

@bergwolf - yes, that is the merged PR. Over on #806 I have the patches to bump that versions.yaml and golang version change. I'll chase that up - I need to fix at least the yq dep for travis.

@raravena80
Copy link
Member

@jodh-intel ping from your weekly Kata herder.

@jodh-intel
Copy link
Contributor Author

/test

@jodh-intel
Copy link
Contributor Author

/test

@jodh-intel
Copy link
Contributor Author

/test

Updated `externals.cri-containerd.version` in `versions.yaml` to the
newest version that includes the fix for building on golang 1.10.2:

- https://github.com/containerd/cri/commits/8b0d53c09c41d9fbc3b3896548ecf011518e3c42

Signed-off-by: James O. D. Hunt <[email protected]>
Move to golang version 1.10.4 -- the oldest stable golang release at the
time of writing -- since golang 1.10+ is needed to make namespace
handling safe.

Re-ordered a couple of structs (moved `sync.WaitGroup` fields) to keep
the `maligned` linter happy. Previously:

``
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)
```

See:

- golang/go#20676
- golang/go@2595fe7

Also bumped `languages.golang.meta.newest-version` to golang version
1.11, which is the newest stable release at the time of writing.

Fixes kata-containers#148.

Signed-off-by: James O. D. Hunt <[email protected]>
@jodh-intel
Copy link
Contributor Author

/test

@jodh-intel
Copy link
Contributor Author

We now have a cyclic dependency since this PR is failing due to kata-containers/tests#843 (and vice versa).

Copy link
Contributor

@grahamwhaley grahamwhaley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still:
lgtm

@grahamwhaley
Copy link
Contributor

OK, we know this will fail on the xurl stuff, which we need to land this to fix:

INFO: Installing xurls utility
package mvdan.cc/xurls/v2: cannot find package "mvdan.cc/xurls/v2" in any of:
	/usr/local/go/src/mvdan.cc/xurls/v2 (from $GOROOT)
	/tmp/jenkins/workspace/kata-containers-runtime-ubuntu-16-04-PR/go/src/mvdan.cc/xurls/v2 (from $GOPATH)
Build step 'Execute shell' marked build as failure

@jodh-intel and I propose we force merge. Of course it carries some risk, but right now the CI is broken on the xurl item, and we can merge nothing else anyway...

@grahamwhaley grahamwhaley merged commit f3ef220 into kata-containers:master Oct 23, 2018
jodh-intel added a commit to jodh-intel/documentation that referenced this pull request Oct 23, 2018
Travis appears to be providing a version of golang that is too old for
https://mvdan.cc/xurls/cmd/xurls, which is used by the CI scripts in the
tests repo.

See:

- kata-containers/runtime#744
- kata-containers/tests#843 (comment)

Fixes kata-containers#281.

Signed-off-by: James O. D. Hunt <[email protected]>
jodh-intel added a commit to jodh-intel/documentation that referenced this pull request Oct 23, 2018
Travis appears to be providing a version of golang that is too old for
https://mvdan.cc/xurls/cmd/xurls, which is used by the CI scripts in the
tests repo.

See:

- kata-containers/runtime#744
- kata-containers/tests#843 (comment)

Fixes kata-containers#281.

Signed-off-by: James O. D. Hunt <[email protected]>
jodh-intel added a commit to jodh-intel/documentation that referenced this pull request Oct 23, 2018
Travis appears to be providing a version of golang that is too old for
https://mvdan.cc/xurls/cmd/xurls, which is used by the CI scripts in the
tests repo.

See:

- kata-containers/runtime#744
- kata-containers/tests#843 (comment)

Fixes kata-containers#281.

Signed-off-by: James O. D. Hunt <[email protected]>
jodh-intel added a commit to jodh-intel/documentation that referenced this pull request Oct 23, 2018
Travis appears to be providing a version of golang that is too old for
https://mvdan.cc/xurls/cmd/xurls, which is used by the CI scripts in the
tests repo.

See:

- kata-containers/runtime#744
- kata-containers/tests#843 (comment)

Fixes kata-containers#281.

Signed-off-by: James O. D. Hunt <[email protected]>
jodh-intel added a commit to jodh-intel/documentation that referenced this pull request Oct 23, 2018
Travis appears to be providing a version of golang that is too old for
https://mvdan.cc/xurls/cmd/xurls, which is used by the CI scripts in the
tests repo.

See:

- kata-containers/runtime#744
- kata-containers/tests#843 (comment)

Fixes kata-containers#281.

Signed-off-by: James O. D. Hunt <[email protected]>
jodh-intel added a commit to jodh-intel/documentation that referenced this pull request Oct 23, 2018
Travis appears to be providing a version of golang that is too old for
https://mvdan.cc/xurls/cmd/xurls, which is used by the CI scripts in the
tests repo.

See:

- kata-containers/runtime#744
- kata-containers/tests#843 (comment)

Required adding a NOP makefile to avoid Travis from trying to build
this repo using `go`.

Fixes kata-containers#281.

Signed-off-by: James O. D. Hunt <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants