Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[1.1] runc list: fix race with runc delete #4231

Open
wants to merge 2 commits into
base: release-1.1
Choose a base branch
from
Open

[1.1] runc list: fix race with runc delete #4231

wants to merge 2 commits into from

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Mar 27, 2024 •
edited

This is a backport of #3349 to release-1.1. Original description follows.

It is possible that parallel execution of runc list with runc delete will result in runc list fatal failure. Fix this.

Bonus: some refactoring.

Please review with --ignore-all-space ("Hide whitespace" checked on GH).

@kolyshkin kolyshkin added this to the 1.1.13 milestone Mar 27, 2024
@lifubang
Copy link
Member

lifubang commented Mar 28, 2024 •
edited

I think we should also change the go version to 1.21 in github workflow definition file because of #4193.
But I'm curious about why there is no failure in the main branch.

Setup go version spec 1.x
Found in cache @ /opt/hostedtoolcache/go/1.22.1/x64
Added go to the path
Successfully set up Go version 1.x
/opt/hostedtoolcache/go/1.22.1/x64/bin/go env GOMODCACHE
/opt/hostedtoolcache/go/1.22.1/x64/bin/go env GOCACHE
/home/runner/go/pkg/mod
/home/runner/.cache/go-build
Cache is not found
go version go1.22.1 linux/amd64

@kolyshkin
Copy link
Contributor Author

I think we should also change the go version to 1.21 in github workflow definition file because of #4193.

Yes, done.

But I'm curious about why there is no failure in the main branch.

I see that for cross-386 job we use Ubuntu 22.04 in main and 20.04 here, plus we only run unit tests (for some reason unit tests do not fail in 22.04 with Go 1.22, I guess we use nsenter in a slightly different manner).

Hmm.

@kolyshkin
Copy link
Contributor Author

kolyshkin commented Mar 28, 2024 •
edited

I see that for cross-386 job we use Ubuntu 22.04 in main and 20.04 here, plus we only run unit tests (for some reason unit tests do not fail in 22.04 with Go 1.22, I guess we use nsenter in a slightly different manner).

Ah, nevermind, Ubuntu 22.04 fails for different reason (spec validator test, see here) and its glibc seems to be fine wrt Go 1.22 issue.

@lifubang
Copy link
Member

Please see 6fb24a2 in #4193 , I think maybe we can resolve this clone(2) issue. Please help to see what's wrong with the remain failure test cases.

@kolyshkin kolyshkin mentioned this pull request Apr 2, 2024
Closed
@kolyshkin kolyshkin changed the title [1.1] runc list: fix race with runc delete [1.1] runc list: fix race with runc delete; fix 1.1 CI Apr 2, 2024
@kolyshkin kolyshkin mentioned this pull request Apr 2, 2024
Closed
@kolyshkin kolyshkin added the backport/1.1-pr A backport to 1.1.x release. label Apr 2, 2024
@kolyshkin kolyshkin changed the title [1.1] runc list: fix race with runc delete; fix 1.1 CI [1.1] runc list: fix race with runc delete Apr 3, 2024
@medyagh
Copy link

medyagh commented Apr 9, 2024

we look forward to use this PR once it is included in the release in minikube, currently causing integration test failures

@kolyshkin kolyshkin mentioned this pull request Apr 23, 2024
Open
lifubang
lifubang approved these changes Apr 26, 2024
View reviewed changes
Copy link
Member

@lifubang lifubang left a comment

Choose a reason for hiding this comment

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

still LGTM

@kolyshkin
list: getContainers: less indentation
09214f2 
Instead of a huge if {} block, use continue.

Best reviewed with --ignore-all-space.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 095929b)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
list: use Info(), fix race with delete
986edbe 
Since commit 5516294 we can (and should) use Info() to get access to
file stat. Do this.

While going over directory entries, a parallel runc delete can remove
an entry, and with the current code it results in a fatal error (which
was not observed in practice, but looks quite possible). To fix,
add a special case to continue on ErrNotExist.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit 1a3ee49)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lifubang lifubang lifubang approved these changes

@AkihiroSuda AkihiroSuda Awaiting requested review from AkihiroSuda

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
backport/1.1-pr A backport to 1.1.x release.
Projects
None yet
Milestone
1.1.13
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kolyshkin @lifubang @medyagh