[release/1.2 backport] Fix container pid race condition by thaJeztah · Pull Request #4025 · containerd/containerd

thaJeztah · 2020-02-14T10:04:43Z

alternative for #4024 ([1.2] Revert #3366)
closes #4024 ([1.2] Revert #3366)

backport of:

a6b6097 Fix container pid race condition. #3857 Fix container pid race condition
- relates to Robust pid locking for shim processes #3366 / [release/1.2 backport] backport exec fixes #3755 Robust pid locking for shim processes
- fixes [v1.2] regression in v1.2.12 leaves container/shim hanging #4023 [v1.2] regression in v1.2.12 leaves container/shim hanging
Minor conflict in container_test.go, because Handle large output in v2 shim with TTY #3743 and Shorten the unix socket path for shim #3046 are not yet backported to the 1.2 branch
- no real "conflict", but git having trouble finding a marker due to test of the above not being present
Conflict in runtime/v1/linux/proc/utils.go``, because files were renamed, and the StoppedPID` const is in a different location;

++>>>>>>> a6b6097c9... Fix container pid.
diff --cc runtime/v1/linux/proc/utils.go
index 08fefdb3e,3c4661770..000000000
--- a/runtime/v1/linux/proc/utils.go
+++ b/runtime/v1/linux/proc/utils.go
@@@ -34,6 -36,13 +34,16 @@@ import
        "golang.org/x/sys/unix"
  )

++<<<<<<< HEAD:runtime/v1/linux/proc/utils.go
++=======
+ const (
+       // RuncRoot is the path to the root runc state directory
+       RuncRoot = "/run/containerd/runc"
+       // InitPidFile name of the file that contains the init pid
+       InitPidFile = "init.pid"
+ )
+
++>>>>>>> a6b6097c9... Fix container pid.:pkg/process/utils.go
  // safePid is a thread safe wrapper for pid.
  type safePid struct {
        sync.Mutex

Resolved by removing the StoppedPID const from runtime/v1/linux/proc/process.go instead of pkg/process/utils.go

Also had to make a small modification to the test, because #3417 is not in the 1.2 branch;

diff --git a/container_test.go b/container_test.go
index 2a0828f4c..aa04a31ad 100644
--- a/container_test.go
+++ b/container_test.go
@@ -1540,7 +1540,7 @@ func TestShortRunningTaskPid(t *testing.T) {

        var (
                image       Image
-               ctx, cancel = testContext(t)
+               ctx, cancel = testContext()
                id          = t.Name()
        )
        defer cancel()

zhsj · 2020-02-14T10:08:17Z

Probably you mean to say "Closes: #4023 ([v1.2] regression in v1.2.12 leaves container/shim hanging)" in the PR description?

thaJeztah · 2020-02-14T10:25:56Z

That line is further down below in the description (fixes #4023 ...)
I added closes #4024, so that the PR for which this is an alternative would close if we pick this one 😄

thaJeztah · 2020-02-14T10:28:27Z

Hm.. I was afraid that would happen;

container_test.go:1543:29:warning: too many arguments (compile) (staticcheck)
container_test.go:1543:29:warning: too many arguments (compile) (staticcheck)
container_test.go:1543:29:warning: unused variable or constant too many arguments (varcheck)

I suspect we need #3417 to fix that, or make some changes to the test

Updating the test looks trivial, so let me go for that approach:

diff --git a/container_test.go b/container_test.go
index 2a0828f4c..aa04a31ad 100644
--- a/container_test.go
+++ b/container_test.go
@@ -1540,7 +1540,7 @@ func TestShortRunningTaskPid(t *testing.T) {

        var (
                image       Image
-               ctx, cancel = testContext(t)
+               ctx, cancel = testContext()
                id          = t.Name()
        )
        defer cancel()

thaJeztah · 2020-02-14T13:07:55Z

Looks like there's failures with CGO_ENABLED=1. Trying to find what the failure is (CI output is hard to dig through)

hakman · 2020-02-14T13:11:43Z

@thaJeztah I think I have the same failures in #4015, so at least seems unrelated to the fix.

thaJeztah · 2020-02-14T13:14:23Z

Thanks! Wasn't sure if it's related

fuweid · 2020-02-16T14:52:20Z

runtime/v1/linux/proc/init.go

I think we can keep safePid to prevent pid's data race and just remove StoppedPID and update kill logic. WDYT?

This is a backport, so if we want to change that, it's better to make those changes on master, then backport to release branches

fuweid · 2020-02-16T16:11:58Z

CI failure will be fixed by kubernetes-sigs/cri-tools#574

zhsj · 2020-02-16T16:27:58Z

CI failure will be fixed by kubernetes-sigs/cri-tools#574

But cri-tools is pinned at v1.12.0

containerd/script/setup/install-critools

Lines 24 to 27 in 35bd7a5

    
           CRITEST_COMMIT=v1.12.0 
        
           go get -d github.com/kubernetes-incubator/cri-tools/... 
        
           cd $GOPATH/src/github.com/kubernetes-incubator/cri-tools 
        
           git checkout $CRITEST_COMMIT

Not sure why the test result differs over time.

fuweid · 2020-02-17T02:43:44Z

CI failure will be fixed by kubernetes-sigs/cri-tools#574

But cri-tools is pinned at v1.12.0

containerd/script/setup/install-critools

Lines 24 to 27 in 35bd7a5

CRITEST_COMMIT=v1.12.0

go get -d github.com/kubernetes-incubator/cri-tools/...

cd $GOPATH/src/github.com/kubernetes-incubator/cri-tools

git checkout $CRITEST_COMMIT

Not sure why the test result differs over time.

@zhsj yes. I think the kubernetes-sigs/cri-tools package is always pulled in the machine.

# Makefile from github.com/kubernetes-incubator/cri-tools
GO ?= go
PROJECT := github.com/kubernetes-sigs/cri-tools
BINDIR := /usr/local/bin

That is why that commit change doesn't work and it still using master.
I think we should change it from github.com/kubernetes-incubator/cri-tools to github.com/ kubernetes-sigs/cri-tools.

cc @mikebrow @Random-Liu

thaJeztah · 2020-02-17T09:42:25Z

AppVeyor is failing now; could be a Windows issue (we've seen issues with latest Windows patch releases in moby/moby)

exit status 1
FAIL	github.com/containerd/containerd	100.813s
Makefile:158: recipe for target 'integration' failed
mingw32-make: *** [integration] Error 1
Command exited with code 2

Signed-off-by: Lantao Liu <lantaol@google.com> (cherry picked from commit a6b6097) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

codecov-io · 2020-02-17T16:39:47Z

Codecov Report

Merging #4025 into release/1.2 will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff              @@
##           release/1.2    #4025   +/-   ##
============================================
  Coverage        44.19%   44.19%           
============================================
  Files              100      100           
  Lines            10847    10847           
============================================
  Hits              4794     4794           
  Misses            5313     5313           
  Partials           740      740

Flag	Coverage Δ
#linux	`47.87% <ø> (ø)`	⬆️
#windows	`41% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89c46ed...b970987. Read the comment docs.

thaJeztah · 2020-02-17T16:41:22Z

Yay, green now!

estesp

LGTM

mikebrow

LGTM
confirmed cherry-pick of #3857 to 1.2

fuweid

LGTM

The thirteenth patch release for `containerd` 1.2 fixes a regression introduced in v1.2.12 that caused container/shim to hang on single core machines, fixes an issue with blkio, and updates the Golang runtime to 1.12.17. Notable Updates ---------------------------------- * Fix container pid race condition [containerd#4025](containerd#4025) * Update containerd/cgroups dependency to address blkio issue [containerd#4001](containerd#4001) * Set octet-stream content-type on PUT request [containerd#4028](containerd#4028) * Pin to libseccomp 2.3.3 to preserve compatibility with hosts that do not have libseccomp 2.4 or higher installed [containerd#4015](containerd#4015) * Update Golang runtime to 1.12.17, which includes a fix to the runtime [containerd#4031](containerd#4031) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah mentioned this pull request Feb 14, 2020

[1.2] Revert #3366 #4024

Closed

thaJeztah force-pushed the 1.2_backport_fix_container_pid branch from 622b91e to 710007a Compare February 14, 2020 10:32

fuweid reviewed Feb 16, 2020

View reviewed changes

thaJeztah closed this Feb 17, 2020

thaJeztah reopened this Feb 17, 2020

thaJeztah closed this Feb 17, 2020

thaJeztah reopened this Feb 17, 2020

thaJeztah mentioned this pull request Feb 17, 2020

[v1.2] regression in v1.2.12 leaves container/shim hanging #4023

Closed

thaJeztah changed the title ~~[release/1.2 backport] Fix container pid.~~ [release/1.2 backport] Fix container pid race condition Feb 17, 2020

thaJeztah mentioned this pull request Feb 17, 2020

[release/1.2] Prepare v1.2.13 release #4033

Merged

5 tasks

thaJeztah force-pushed the 1.2_backport_fix_container_pid branch from 710007a to 88e633f Compare February 17, 2020 15:08

Fix container pid.

b970987

Signed-off-by: Lantao Liu <lantaol@google.com> (cherry picked from commit a6b6097) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah force-pushed the 1.2_backport_fix_container_pid branch from 88e633f to b970987 Compare February 17, 2020 15:34

estesp approved these changes Feb 17, 2020

View reviewed changes

thaJeztah mentioned this pull request Feb 17, 2020

Docker-in-Docker 19.03.6 hangs during execution docker-library/docker#216

Closed

mikebrow approved these changes Feb 17, 2020

View reviewed changes

fuweid approved these changes Feb 18, 2020

View reviewed changes

fuweid merged commit b9b4fa7 into containerd:release/1.2 Feb 18, 2020

thaJeztah deleted the 1.2_backport_fix_container_pid branch February 18, 2020 07:34

Conversation

thaJeztah commented Feb 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhsj commented Feb 14, 2020

Uh oh!

thaJeztah commented Feb 14, 2020

Uh oh!

thaJeztah commented Feb 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Feb 14, 2020

Uh oh!

hakman commented Feb 14, 2020

Uh oh!

thaJeztah commented Feb 14, 2020

Uh oh!

fuweid Feb 16, 2020

Choose a reason for hiding this comment

Uh oh!

thaJeztah Feb 16, 2020

Choose a reason for hiding this comment

Uh oh!

fuweid Feb 17, 2020

Choose a reason for hiding this comment

Uh oh!

fuweid commented Feb 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhsj commented Feb 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fuweid commented Feb 17, 2020

Uh oh!

thaJeztah commented Feb 17, 2020

Uh oh!

codecov-io commented Feb 17, 2020

Codecov Report

Uh oh!

thaJeztah commented Feb 17, 2020

Uh oh!

estesp left a comment

Choose a reason for hiding this comment

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

fuweid left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

thaJeztah commented Feb 14, 2020 •

edited

Loading

thaJeztah commented Feb 14, 2020 •

edited

Loading

fuweid commented Feb 16, 2020 •

edited

Loading

zhsj commented Feb 16, 2020 •

edited

Loading