ci/gha: add go 1.16; run as root; fix some tests; add fedora #65

kolyshkin · 2021-02-17T01:43:52Z

A few tiny improvements to GHA CI:

add go 1.16 to testing matix 🔥 ;
explicitly specify ubuntu-20.04 (rather than ubuntu-latest);
move 'make lint' and 'make cross' to before 'make test';
update actions/setup-go to v2;
add 👒 fedora runner (to test functionality that needs openat2(2));
remove remaining travis-ci bits;
fix mount tests for kernel 5.9+;
run tests as root 🔥.

The last item revealed that tests that require openat2 do not check for it 🙈 :, so add the check.

kolyshkin · 2021-02-17T02:29:18Z

@cpuguy83 @thaJeztah PTAL. No change in package functionality, just CI infra and a bit of tests.

.github/workflows/test.yml

thaJeztah · 2021-02-17T13:04:35Z

We should probably also switch to actions/setup-go@v2; https://github.com/actions/setup-go/tree/v2.1.3#v2

The V2 offers:

Adds GOBIN to the PATH

Proxy Support

stable input

Bug Fixes (including issues around version matching and semver)

cpuguy83 · 2021-02-17T18:47:22Z

CI shows passing but:

kolyshkin · 2021-02-24T03:55:49Z

CI shows passing but

This is OK, the kernel is not that new on GHA so some tests are skipped (unless inside a vagrant vm with fedora -- which is why I am adding it.

Now, I'm not sure why skipping is reported as error (I use t.Skip())

Update: this is how it looks if run from CLI on an older kernel

kir@ubu2004:~/git/moby/sys/mountinfo$ sudo go test -v -run MountedBy
=== RUN   TestMountedBy
--- SKIP: TestMountedBy (0.00s)
    mounted_linux_test.go:190: openat2: function not implemented (old kernel? need Linux 5.6+)
=== RUN   TestMountedByOpenat2VsMountinfo
--- SKIP: TestMountedByOpenat2VsMountinfo (0.00s)
    mounted_linux_test.go:237: openat2: function not implemented (old kernel? need Linux 5.6+)
PASS
ok  	github.com/moby/sys/mountinfo	0.002s

Update2: I think this is some unfortunate GHA cleverness (highlighting the function not implemented as an error or so)

kolyshkin · 2021-02-24T04:08:58Z

We should probably also switch to actions/setup-go@v2

added; PTAL @cpuguy83 @thaJeztah

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Otherwise GHA complains: > Ubuntu-latest workflows will use Ubuntu-20.04 soon. For more details, > see actions/runner-images#1816 Apparently, despite the wording, it is already using 20.04, but it is nice to silence the warning regardless. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Changes: https://github.com/actions/setup-go/tree/v2.1.3#v2 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

The idea is not to run any tests if the linter fails. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

thaJeztah · 2021-02-24T10:25:48Z

Update2: I think this is some unfortunate GHA cleverness (highlighting the function not implemented as an error or so)

Could it be it prepends Error: for any line on STDERR ?

thaJeztah

Overall looks good (thanks!) left some questions

thaJeztah · 2021-02-24T10:15:12Z

.github/workflows/test.yml

+        # fixup go cache screwed by sudo
+        sudo chown -R $(id -u):$(id -g) $HOME/.cache


Some questions;

Is the cache actually used (or "useful"), assuming these nodes are cleaned between runs?

Did this cause a problem in CI, or only on your local machine (testing sudo -E PATH=... , after which the files were owned by root)? (if so, it may not be needed in CI, as state would be cleared after this completes, correct?)

Alternatively, we can set GOCACHE so that the sudo variant writes to it's own home directory, not the home directory of the non-privileged user, e.g.

sudo -E PATH=$PATH GOCACHE=/root/.cache make test

Is the cache actually used (or "useful"), assuming these nodes are cleaned between runs?

It is used (based by the errors in CI I saw), and I guess it is mildly useful. Surely, we're not compiling docker here, so the savings are not that big.

I am fixing the .cache ownership not because the cache is useful, but for the sake of future contributors who might add some commands after that one, and discover that it fails.

Actually, the proper way would be to compile the tests as a user and run them as root, but I'm not sure how to do it in an elegant way.

Did this cause a problem in CI

Yes it did, as "make lint" and "make cross" were before "sudo make test" and they failed because they could not write to ~/.cache. So I mitigated it with two things -- (1) moved all non-sudo commands to before sudo ones (2) added this fixup.

we can set GOCACHE

Good idea, let me try. Judging by the amount of code to compile (our packages plus a bits of x/sys/unix), caching should not result in any dramatic improvements.

Ended up implementing decent make sudotest via Makefile, which compiles tests as a user and runs them as root. This way we don't have to care about gocache and other similar things (like re-downloading modules).

Scratch that bloody sudotest, I found a much better way, see the last commit.

thaJeztah · 2021-02-24T10:29:17Z

.ci/Vagrantfile.fedora33

@@ -20,8 +20,7 @@ Vagrant.configure("2") do |config|
      cat << EOF | dnf -y shell && break
 config exclude kernel,kernel-core


Wondering if this line was related to the update line you removed (i.e.; "update, but don't install kernel updates")?

Yes it is, but I'd like to keep this line, which basically says "no matter what we do below, exclude kernel from it".

Some Fedora version needed update in the past, and this need might re-surface, so there's a chance someone will add update later, and they will definitely forget about excluding the kernel, which is a big package for which there's almost always an update available.

So yes, the line does not make sense per se currently, but it might come handy later, and since it does not add any overhead or something, I'd rather keep it.

Alternatively, we can re-add update and comment it out, together with the exclude. Let me know if you prefer it this way.

(given that the comments are available in that context -- I'm not entirely sure)

cpuguy83

LGTM

Both tests need (1) root (2) openat2. Do skip if those are not available. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

I am seeing the following failures on my machine (running): === RUN TestMount/none-remount,size=128k mounter_linux_test.go:203: unexpected vfs option "inode64", expected "rw,size=128k" === RUN TestMount/none-remount,ro,size=128k mounter_linux_test.go:203: unexpected vfs option "inode64", expected "ro,size=128k" --- FAIL: TestMount (0.06s) Looking into /proc/self/mountinfo, I see that all tmpfs mounts have this option. Looking into the kernel, it was added by commit ea3271f7196c6 ("tmpfs: support 64-bit inums per-sb", 2020-08-06), which made its way into v5.9-rc1. Ignore the option, as well as "inode32". Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin · 2021-02-25T19:24:35Z

Could it be it prepends Error: for any line on STDERR ?

I am using t.Skipf, and the docs say Skipf is equivalent to Logf followed by SkipNow. This means that either any t.Log[f] goes to stderr, or it doesn't, iow there's nothing special about message printed by t.Skipf.

So I stll think there's some special cleverness in GHA to highlight a line containing 'function not implemented'. The only way to check is to remove this message, I'll try that (but I don't want to remove message forever).

Some recently added features require openat2 support to be tested. We used Travis for that, but it's not available now, so let's use a GHA VM. Code mostly copied from github.com/opencontainers/runc. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin · 2021-02-25T21:10:01Z

Scratch that, I found a better way to run go test via sudo

Some tests (those that do mounts etc.) require root. Amend the Makefile to check if sudo -n (non-interactive) works, and if yes, add -exec sudo to "go test" arguments. This trick makes test compile as normal user but run as root, eliminating the need to preserve environment (-E) and path (PATH=$PATH) when using sudo, as well as avoiding potential ownership problems after running something like compiling test binaries under sudo. With this, tests are run as root in GHA. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

addressed

kolyshkin · 2021-03-03T17:37:11Z

@thaJeztah PTAL

thaJeztah

LGTM ❤️

left one question (could do in a follow up)

thaJeztah · 2021-03-05T09:00:37Z

mountinfo/mounted_linux_test.go

+}
+
+func TestMountedBy(t *testing.T) {
+	requireOpenat2(t)


Do you think it would make sense to only skip the parts that require openat2, instead of the whole test? #67

I definitely had openat2 way in mind when writing this, mostly added mountinfo to test my test :)

But yeah, when I look at it, it makes some sense.

But as we already test in on an openat2-capable system that should be sufficient.

IOW it's up to you whether you want to update #67 to run this test partially or just close it.

kolyshkin added the area/ci label Feb 17, 2021

kolyshkin changed the title ~~ci/gha: add go 1.16~~ ci/gha: add go 1.16; run as root Feb 17, 2021

kolyshkin force-pushed the go116 branch from 17f05a2 to 3ae6219 Compare February 17, 2021 02:12

kolyshkin changed the title ~~ci/gha: add go 1.16; run as root~~ ci/gha: add go 1.16; run as root; fix some tests; add fedora Feb 17, 2021

thaJeztah previously requested changes Feb 17, 2021

View reviewed changes

.github/workflows/test.yml Outdated Show resolved Hide resolved

kolyshkin force-pushed the go116 branch from e4b7595 to 392012c Compare February 24, 2021 04:08

kolyshkin added 4 commits February 23, 2021 20:13

ci/gha: add go 1.16

2c1a2d9

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

ci/gha: bump actions/setup-go to v2

7fa6122

Changes: https://github.com/actions/setup-go/tree/v2.1.3#v2 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

ci/gha: move make test to after lint/cross

c8bf724

The idea is not to run any tests if the linter fails. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin force-pushed the go116 branch 4 times, most recently from 8d9c48a to 256ee02 Compare February 24, 2021 04:39

thaJeztah reviewed Feb 24, 2021

View reviewed changes

cpuguy83 approved these changes Feb 25, 2021

View reviewed changes

kolyshkin force-pushed the go116 branch from 256ee02 to 52c94a5 Compare February 25, 2021 19:08

kolyshkin added 2 commits February 25, 2021 11:15

mountinfo/TestMountedBy*: add missing pre-checks

ce8f425

Both tests need (1) root (2) openat2. Do skip if those are not available. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin force-pushed the go116 branch from 52c94a5 to 676b7ba Compare February 25, 2021 19:19

ci/gha: add fedora job

ce49bfe

Some recently added features require openat2 support to be tested. We used Travis for that, but it's not available now, so let's use a GHA VM. Code mostly copied from github.com/opencontainers/runc. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin force-pushed the go116 branch 2 times, most recently from 8c753ef to 89b55da Compare February 25, 2021 19:45

kolyshkin force-pushed the go116 branch from 89b55da to eb2a60a Compare February 25, 2021 21:33

kolyshkin requested a review from thaJeztah March 3, 2021 17:37

thaJeztah mentioned this pull request Mar 5, 2021

mountinfo: TestMountedBy: skip mountedByOpenat2() check if not supported #67

Merged

thaJeztah approved these changes Mar 5, 2021

View reviewed changes

AkihiroSuda approved these changes Mar 8, 2021

View reviewed changes

AkihiroSuda merged commit d038b10 into moby:master Mar 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci/gha: add go 1.16; run as root; fix some tests; add fedora #65

ci/gha: add go 1.16; run as root; fix some tests; add fedora #65

kolyshkin commented Feb 17, 2021 •

edited

Loading

kolyshkin commented Feb 17, 2021

thaJeztah commented Feb 17, 2021

cpuguy83 commented Feb 17, 2021

kolyshkin commented Feb 24, 2021 •

edited

Loading

kolyshkin commented Feb 24, 2021

thaJeztah commented Feb 24, 2021

thaJeztah left a comment

thaJeztah Feb 24, 2021

kolyshkin Feb 25, 2021

kolyshkin Feb 25, 2021

kolyshkin Feb 25, 2021

thaJeztah Feb 24, 2021

kolyshkin Feb 25, 2021

kolyshkin Feb 25, 2021

cpuguy83 left a comment

kolyshkin commented Feb 25, 2021

kolyshkin commented Feb 25, 2021

kolyshkin commented Mar 3, 2021

thaJeztah left a comment

thaJeztah Mar 5, 2021

kolyshkin Mar 8, 2021

kolyshkin Mar 8, 2021

		# fixup go cache screwed by sudo
		sudo chown -R $(id -u):$(id -g) $HOME/.cache

		@@ -20,8 +20,7 @@ Vagrant.configure("2") do \|config\|
		cat << EOF \| dnf -y shell && break
		config exclude kernel,kernel-core

ci/gha: add go 1.16; run as root; fix some tests; add fedora #65

ci/gha: add go 1.16; run as root; fix some tests; add fedora #65

Conversation

kolyshkin commented Feb 17, 2021 • edited Loading

kolyshkin commented Feb 17, 2021

thaJeztah commented Feb 17, 2021

cpuguy83 commented Feb 17, 2021

kolyshkin commented Feb 24, 2021 • edited Loading

kolyshkin commented Feb 24, 2021

thaJeztah commented Feb 24, 2021

thaJeztah 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

cpuguy83 left a comment

Choose a reason for hiding this comment

kolyshkin commented Feb 25, 2021

kolyshkin commented Feb 25, 2021

kolyshkin commented Mar 3, 2021

thaJeztah 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

kolyshkin commented Feb 17, 2021 •

edited

Loading

kolyshkin commented Feb 24, 2021 •

edited

Loading