[1.1] refresh ci #3791
[1.1] refresh ci #3791
Conversation
kolyshkin
force-pushed
the
1.1-refresh-ci
branch
3 times, most recently
from
f21046e
to
f1fe32b
Compare
|
Two questions for @opencontainers/runc-maintainers (cc @AkihiroSuda @thaJeztah @cyphar)
My answer to q 3 is yes, and that alone might force us to say no to q 2. |
kolyshkin
force-pushed
the
1.1-refresh-ci
branch
from
30fbf5b
to
014df90
Compare
1. Allow wrap_bad and wrap_good to have an optional arguments.
2. Remove unneeded echos; this fixes the shellcheck warnings like
In ./script/check-config.sh line 178:
echo "$(wrap_bad 'cgroup hierarchy' 'nonexistent??')"
^-- SC2005 (style): Useless echo? Instead of 'echo $(cmd)', just use 'cmd'.
3. Fix missing color argument in calls to wrap_color (when printing the
hint about how to install apparmor).
Signed-off-by: Kir Kolyshkin <[email protected]>
(cherry picked from commit dc73d23)
Signed-off-by: Kir Kolyshkin <[email protected]>
Like this one: In ./script/check-config.sh line 215: if [ "$kernelMajor" -lt 5 ] || [ "$kernelMajor" -eq 5 -a "$kernelMinor" -le 1 ]; then ^-- SC2166 (warning): Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. Signed-off-by: Kir Kolyshkin <[email protected]> (cherry picked from commit baa0622) Signed-off-by: Kir Kolyshkin <[email protected]>
…and fix a single format issue found. Signed-off-by: Kir Kolyshkin <[email protected]> (cherry picked from commit 6b16d00) Signed-off-by: Kir Kolyshkin <[email protected]>
... and add this file to shellcheck target in Makefile.
These:
In script/check-config.sh line 27:
kernelMinor="${kernelVersion#$kernelMajor.}"
^----------^ SC2295 (info): Expansions inside ${..} need to be quoted separately, otherwise they match as patterns.
Did you mean:
kernelMinor="${kernelVersion#"$kernelMajor".}"
In script/check-config.sh line 103:
source /etc/os-release 2>/dev/null || /bin/true
^-------------^ SC1091 (info): Not following: /etc/os-release was not specified as input (see shellcheck -x).
In script/check-config.sh line 267:
NET_CLS_CGROUP $netprio
^------^ SC2206 (warning): Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
Signed-off-by: Kir Kolyshkin <[email protected]>
(cherry picked from commit d66498e)
Signed-off-by: Kir Kolyshkin <[email protected]>
Now the only remaining file that needs shellcheck warnings to be fixed is bash-completion. Note that in Makefile's TODO. Signed-off-by: Kir Kolyshkin <[email protected]> (cherry picked from commit ae6cb65) Signed-off-by: Kir Kolyshkin <[email protected]>
1. Bump shfmt to v3.5.1. Release notes: https://github.com/mvdan/sh/releases 2. Since shfmt v3.5.0, specifying -l bash (or -l bats) is no longer necessary. Therefore, we can use shfmt to find all the files. Add .editorconfig to ignore vendor subdirectory. 3. Use shfmt docker image, so that we don't have to install anything explicitly. This greatly simplifies the shfmt CI job. Add localshfmt target so developers can still use a local shfmt binary when necessary. Signed-off-by: Kir Kolyshkin <[email protected]> (cherry picked from commit 56edc41) Signed-off-by: Kir Kolyshkin <[email protected]>
centos-9 unit test sometimes fails with: === RUN TestPodSkipDevicesUpdate systemd_test.go:114: container stderr not empty: basename: missing operand Try 'basename --help' for more information. --- FAIL: TestPodSkipDevicesUpdate (0.11s) I'm not sure why the container output is an error in basename. It seems likely that the bashrc in that distro is kind of broken. Let's just run a sleep command and forget about bash. Signed-off-by: Rodrigo Campos <[email protected]> (cherry picked from commit 4d0a60c) Signed-off-by: Kir Kolyshkin <[email protected]>
1. bump golang.org/x/sys to v0.6.0; 2. bump golang.org/x/net to v0.8.0 3. require Go 1.17. Newer x/sys is needed to fix [1]. Go 1.17 is needed because x/sys/unix is using unsafe.Slice which requires go1.17 or later. This reuses parts of main commit a0f8847. [1] opencontainers#3715 Signed-off-by: Kir Kolyshkin <[email protected]>
Bumps [actions/setup-go](https://github.com/actions/setup-go) from 2 to 3. - [Release notes](https://github.com/actions/setup-go/releases) - [Commits](actions/setup-go@v2...v3)
Since the recent bump of actions/setup-go to v3 (commit 9d2268b), specifying "stable:" is no longer needed when we want to try a beta or rc version of Go. Remove it. Signed-off-by: Kir Kolyshkin <[email protected]> (cherry picked from commit 66be704) Signed-off-by: Kir Kolyshkin <[email protected]>
With the updated git in golang:1.19-bullseye image, building fails with: make -C /go/src/github.com/opencontainers/runc PKG_CONFIG_PATH=/opt/libseccomp/lib/pkgconfig COMMIT_NO= EXTRA_FLAGS=-a 'EXTRA_LDFLAGS=-w -s -buildid=' static make[1]: Entering directory '/go/src/github.com/opencontainers/runc' fatal: detected dubious ownership in repository at '/go/src/github.com/opencontainers/runc' To add an exception for this directory, call: git config --global --add safe.directory /go/src/github.com/opencontainers/runc go build -trimpath -buildmode=pie -a -tags "seccomp urfave_cli_no_docs netgo osusergo" -ldflags "-X main.gitCommit= -X main.version=1.1.0+dev -linkmode external -extldflags --static-pie -w -s -buildid=" -o runc . error obtaining VCS status: exit status 128 Use -buildvcs=false to disable VCS stamping. This commit should fix it. Signed-off-by: Kir Kolyshkin <[email protected]> (cherry picked from commit 42dffaa) Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin
force-pushed
the
1.1-refresh-ci
branch
from
014df90
to
e275a34
Compare
- Dockerfile: use latest Go version (1.20.x) for release binaries - .github/workflows: add Go 1.19.x, 1.20.x - .cirrus.yml: switch to latest Go 1.19.x Signed-off-by: Kir Kolyshkin <[email protected]>
For release notes, see https://github.com/golangci/golangci-lint/releases/tag/v1.45.0 Notably, it adds support for Go 1.18. Signed-off-by: Kir Kolyshkin <[email protected]> (cherry picked from commit fcab941) Signed-off-by: Kir Kolyshkin <[email protected]>
Signed-off-by: Kir Kolyshkin <[email protected]>
There is no need to parallelize lint and lint-extra jobs, and they only differ with the arguments to golangci-lint. Given that the longest time spent in these jobs is installing libseccomp-dev, and that the second linter run can probably benefit a lot from caching, it makes sense to merge them. Move lint-extra from a separate job to a step in lint job. The implementation is motivated by [1] and relies on the fact that the last commit being fetched is the merge commit. So, we need to set fetch-depth to 2 to be able to see the diff of the merge commit -- and this is what golangci-lint is using. [1] golangci/golangci-lint-action#449 (comment) Signed-off-by: Kir Kolyshkin <[email protected]> (cherry picked from commit fa83a17) Signed-off-by: Kir Kolyshkin <[email protected]>
Function strings.Title is deprecated as of Go 1.18, because it does not handle some corner cases good enough. In this case, though, it is perfectly fine to use it since we have a single ASCII word as an argument, and strings.Title won't be removed until at least Go 2.0. Suppress the deprecation warning. The alternative is to not capitalize the namespace string; this will break restoring of a container checkpointed by earlier version of runc. Signed-off-by: Kir Kolyshkin <[email protected]> (cherry picked from commit 7cec81e) Signed-off-by: Kir Kolyshkin <[email protected]>
A new version of staticcheck (included into golangci-lint 1.46.2) gives
this new warning:
> libcontainer/factory_linux.go:230:59: SA9008: e refers to the result of a failed type assertion and is a zero value, not the value that was being type-asserted (staticcheck)
> err = fmt.Errorf("panic from initialization: %v, %s", e, debug.Stack())
> ^
> libcontainer/factory_linux.go:226:7: SA9008(related information): this is the variable being read (staticcheck)
> if e, ok := e.(error); ok {
> ^
Apparently, this is indeed a bug. Fix by using a different name for a
new variable, so we can access the old one under "else".
Signed-off-by: Kir Kolyshkin <[email protected]>
(cherry picked from commit 6662570)
Signed-off-by: Kir Kolyshkin <[email protected]>
Signed-off-by: Kir Kolyshkin <[email protected]> (cherry picked from commit 7481c3c) Signed-off-by: Kir Kolyshkin <[email protected]>
Since Go 1.19, godoc recognizes lists, code blocks, headings etc. It also reformats the sources making it more apparent that these features are used. Fix a few places where it misinterpreted the formatting (such as indented vs unindented), and format the result using the gofumpt from HEAD, which already incorporates gofmt 1.19 changes. Some more fixes (and enhancements) might be required. Signed-off-by: Kir Kolyshkin <[email protected]> (cherry picked from commit 45cc290) Signed-off-by: Kir Kolyshkin <[email protected]>
This version works with go 1.19, i.e. it fixes golangci/golangci-lint#2922. Signed-off-by: Kir Kolyshkin <[email protected]> (cherry picked from commit 0f4bf2c) Signed-off-by: Kir Kolyshkin <[email protected]>
Signed-off-by: Kir Kolyshkin <[email protected]>
This corresponds to commit 6bf2c3b in main branch. Signed-off-by: Kir Kolyshkin <[email protected]>
This is done to make sure the script is working correctly in different environments (distro and kernel versions). In addition, we can see in test logs which kernel features are enabled. Note that I didn't want to have a separate job for GHA CI, so I just added this to the end of shellcheck one. Signed-off-by: Kir Kolyshkin <[email protected]> (cherry picked from commit cacc823) Signed-off-by: Kir Kolyshkin <[email protected]>
This is to de-duplicate the code that checks that err is nil and that the exit code is zero. Signed-off-by: Kir Kolyshkin <[email protected]> (cherry picked from commit 7c75e84) Signed-off-by: Kir Kolyshkin <[email protected]>
Signed-off-by: Kir Kolyshkin <[email protected]> (cherry picked from commit be7e039) Signed-off-by: Kir Kolyshkin <[email protected]>
The purpose of this test is to check that there are no extra file descriptors left open after repeated calls to runContainer. In fact, the first call to runContainer leaves a few file descriptors opened, and this is by design. Previously, this test relied on two things: 1. some other tests were run before it (and thus all such opened-once file descriptors are already opened); 2. explicitly excluding fd opened to /sys/fs/cgroup. Now, if we run this test separately, it will fail (because of 1 above). The same may happen if the tests are run in a random order. To fix this, add a container run before collection the initial fd list, so those fds that are opened once are included and won't be reported. Signed-off-by: Kir Kolyshkin <[email protected]> (cherry picked from commit f2e71b0) Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin
force-pushed
the
1.1-refresh-ci
branch
from
e275a34
to
8d9d1d2
Compare
|
The CI is all green now. @opencontainers/runc-maintainers PTAL -- merging this will help to unblock other 1.1 PRs. |
|
Not a maintainer, but I am invested in 1.1.6, so I've decided to review and throw my 2c into the mix
an approach I've seen work answers these questions as yes and no respectively. I would personally advocate for keeping the lowest possible golang version supported on this branch. If distributions want the stability of patch releases, they likely are staying on lower versions of golang. that said, runc and more specifically libcontainer's latest release is in the 1.1 cycle, and other projects are likely building with newer golang versions, but still require a cut version. That would require minor releases of runc be cut more frequently to provide a matrix of support/compatiblity with newer golangs. If I were to get a vote, I would say the purpose of this PR should focus on bumping to golang 1.17 (and probably also 1.18 because runc generally supports two golang versions), but drop the pieces focusing on 1.19/1.20, and direct that energy towards cutting 1.2.0, which would have support for newer golangs. That would simplify this PR, and reduce the maintenance burden of keeping 1.1 working with newer libraries, while also fixing the issue with golang 1.16 and fixing the runc 1.1 CI generally |
My train of thoughts is this: in 1.1.x we either stick to the same Go releases as in 1.1.0, or we don't. It worked just fine with sticking until recently. It's different now -- sticking to Go 1.16 stands in a way of fixing #3715, so we have to choose one. Personally, I choose to fix (maybe because it's my bug), meaning we have to drop Go 1.16, meaning we do not stick to the same releases as in runc 1.1.0. This allows us to make one more step forward and actually support latest and non-obsoleted Go releases. For one thing, this makes sense as kubernetes, one of the biggest runc/libcontainer users, always uses latest Go. At the same time, I see no meaning in trying to add support for Go 1.18 as it is obsoleted. As to the complexity of this PR, believe me, I feel the pain. It's not as ugly as it looks though -- if you check the actual changes in .go files, those are minimal. The changes is mostly CI, and tests, and linters, that sort of thing. |
I understand the position of being able to rip off the bandaid of supporting old releases if we're dropping support for 1.16 anyway. However, a distribution that maintains legacy versions of packages is more likely to have support for 1.17 than 1.19 at this point, so the point of supporting an older release is not entirely moot.
FWIW: I did go through each commit, and they all collectively LGTM if this is the approach taken |
|
My quick takes;
As to distros providing LTS releases, which may include a custom LTS version of (EOL) Go versions;
From the above, the only part we could consider in this PR is to (if possible) keep the |
Thank you for your input, I concur we should support the non-obsoleted releases, plus maybe some of the obsoleted ones, on a best-effort basis. Regarding _go116/7.go files, the only one being removed is for libcontainer/devices unit tests. Since this package uses s/sys/unix, which, in the bumped version, uses |
|
I'll go ahead and merge this, to unblock the 1.1.6 release work. |
This is a prerequisite to fix #3715 in 1.1.6. Once merged, I will backport the fix (#3753).
This PR removes Go 1.16 support, and adds Go 1.19 and Go 1.20 support. Mostly touches CI and formatting.
permission denied: unknownwhen relying on capabilities #3715)The reason we have to drop Go 1.16 support is to fix #3715. It looks like we can still keep Go 1.17 support.
This also backports
go getjust stopped working in Ubuntu 20.04 gha imageThe CS9 CI failure is unrelated and is being fixed by #3806.Once merged, I need to modify the required CI jobs list: