Merge pull request #4025 from thaJeztah/1.2_backport_fix_container_pid

fuweid · web-flow · commit b9b4fa7f105a · 2020-02-18T09:29:31.000+08:00
[release/1.2 backport] Fix container pid race condition
diff --git a/container_test.go b/container_test.go
@@ -1528,3 +1528,52 @@ func TestContainerHook(t *testing.T) {
 	}
 	defer task.Delete(ctx, WithProcessKill)
 }
+
+func TestShortRunningTaskPid(t *testing.T) {
+	t.Parallel()
+
+	client, err := newClient(t, address)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer client.Close()
+
+	var (
+		image       Image
+		ctx, cancel = testContext()
+		id          = t.Name()
+	)
+	defer cancel()
+
+	image, err = client.GetImage(ctx, testImage)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	container, err := client.NewContainer(ctx, id, WithNewSnapshot(id, image), WithNewSpec(oci.WithImageConfig(image), withProcessArgs("true")))
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer container.Delete(ctx, WithSnapshotCleanup)
+
+	task, err := container.NewTask(ctx, empty())
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer task.Delete(ctx)
+
+	finishedC, err := task.Wait(ctx)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if err := task.Start(ctx); err != nil {
+		t.Fatal(err)
+	}
+
+	int32PID := int32(task.Pid())
+	if int32PID <= 0 {
+		t.Errorf("Unexpected task pid %d", int32PID)
+	}
+	<-finishedC
+}
diff --git a/runtime/v1/linux/proc/exec.go b/runtime/v1/linux/proc/exec.go
@@ -96,7 +96,6 @@ func (e *execProcess) setExited(status int) {
 	e.status = status
 	e.exited = time.Now()
 	e.parent.Platform.ShutdownConsole(context.Background(), e.console)
-	e.pid.set(StoppedPID)
 	close(e.waitBlock)
 }
 
@@ -147,7 +146,7 @@ func (e *execProcess) kill(ctx context.Context, sig uint32, _ bool) error {
 	switch {
 	case pid == 0:
 		return errors.Wrap(errdefs.ErrFailedPrecondition, "process not created")
-	case pid < 0:
+	case !e.exited.IsZero():
 		return errors.Wrapf(errdefs.ErrNotFound, "process already finished")
 	default:
 		if err := unix.Kill(pid, syscall.Signal(sig)); err != nil {
diff --git a/runtime/v1/linux/proc/init.go b/runtime/v1/linux/proc/init.go
@@ -69,7 +69,7 @@ type Init struct {
 	pausing      *atomicBool
 	status       int
 	exited       time.Time
-	pid          safePid
+	pid          int
 	closers      []io.Closer
 	stdin        io.Closer
 	stdio        proc.Stdio
@@ -116,8 +116,6 @@ func (p *Init) Create(ctx context.Context, r *CreateConfig) error {
 		err    error
 		socket *runc.Socket
 	)
-	p.pid.Lock()
-	defer p.pid.Unlock()
 
 	if r.Terminal {
 		if socket, err = runc.NewTempConsoleSocket(); err != nil {
@@ -197,7 +195,7 @@ func (p *Init) Create(ctx context.Context, r *CreateConfig) error {
 	if err != nil {
 		return errors.Wrap(err, "failed to retrieve OCI runtime container pid")
 	}
-	p.pid.pid = pid
+	p.pid = pid
 	return nil
 }
 
@@ -213,7 +211,7 @@ func (p *Init) ID() string {
 
 // Pid of the process
 func (p *Init) Pid() int {
-	return p.pid.get()
+	return p.pid
 }
 
 // ExitStatus of the process
@@ -269,7 +267,6 @@ func (p *Init) setExited(status int) {
 	p.exited = time.Now()
 	p.status = status
 	p.Platform.ShutdownConsole(context.Background(), p.console)
-	p.pid.set(StoppedPID)
 	close(p.waitBlock)
 }
 
diff --git a/runtime/v1/linux/proc/init_state.go b/runtime/v1/linux/proc/init_state.go
@@ -161,9 +161,6 @@ func (s *createdCheckpointState) Start(ctx context.Context) error {
 	p := s.p
 	sio := p.stdio
 
-	p.pid.Lock()
-	defer p.pid.Unlock()
-
 	var (
 		err    error
 		socket *runc.Socket
@@ -209,7 +206,7 @@ func (s *createdCheckpointState) Start(ctx context.Context) error {
 	if err != nil {
 		return errors.Wrap(err, "failed to retrieve OCI runtime container pid")
 	}
-	p.pid.pid = pid
+	p.pid = pid
 	return s.transition("running")
 }
 
diff --git a/runtime/v1/linux/proc/process.go b/runtime/v1/linux/proc/process.go
@@ -25,8 +25,6 @@ import (
 // RuncRoot is the path to the root runc state directory
 const (
 	RuncRoot = "/run/containerd/runc"
-	// StoppedPID is the pid assigned after a container has run and stopped
-	StoppedPID = -1
 )
 
 func stateName(v interface{}) string {
diff --git a/runtime/v1/linux/proc/utils.go b/runtime/v1/linux/proc/utils.go
@@ -46,12 +46,6 @@ func (s *safePid) get() int {
 	return s.pid
 }
 
-func (s *safePid) set(pid int) {
-	s.Lock()
-	s.pid = pid
-	s.Unlock()
-}
-
 type atomicBool int32
 
 func (ab *atomicBool) set(b bool) {

Original file line number	Diff line number	Diff line change
`@@ -25,8 +25,6 @@ import (`
`25`	`25`	`// RuncRoot is the path to the root runc state directory`
`26`	`26`	`const (`
`27`	`27`	`RuncRoot = "/run/containerd/runc"`
`28`		`- // StoppedPID is the pid assigned after a container has run and stopped`
`29`		`- StoppedPID = -1`
`30`	`28`	`)`
`31`	`29`
`32`	`30`	`func stateName(v interface{}) string {`