Skip to content

Comments

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

Merged
AkihiroSuda merged 2 commits intoopencontainers:release-1.1from
kolyshkin:1.1-3349
May 17, 2024
Merged

[1.1] runc list: fix race with runc delete#4231
AkihiroSuda merged 2 commits intoopencontainers:release-1.1from
kolyshkin:1.1-3349

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 <[email protected]>
(cherry picked from commit 095929b)
Signed-off-by: Kir Kolyshkin <[email protected]>
@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 <[email protected]>
(cherry picked from commit 1a3ee49)
Signed-off-by: Kir Kolyshkin <[email protected]>
@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