Skip to content

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

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
Merged
AkihiroSuda merged 2 commits into from
May 17, 2024
Merged

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

AkihiroSuda merged 2 commits into from
May 17, 2024

Conversation

@kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Mar 27, 2024 •
edited
Loading

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
Loading

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

kolyshkin commented Mar 28, 2024

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
Loading

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

lifubang commented Mar 30, 2024

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 PR to release-1.1 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
Closed
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 added 2 commits May 10, 2024 11:22
@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>
@AkihiroSuda AkihiroSuda merged commit 51dc972 into opencontainers:release-1.1 May 17, 2024
@lifubang lifubang mentioned this pull request Jun 10, 2024
Merged
@prezha prezha mentioned this pull request Jun 27, 2024
Closed
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 AkihiroSuda approved these changes

Assignees

No one assigned

Labels

backport/1.1-pr A backport PR to release-1.1

Projects

None yet

Milestone

1.1.13

Development

Successfully merging this pull request may close these issues.

4 participants

@kolyshkin @lifubang @medyagh @AkihiroSuda