From 8328a66bdfa7712925c729beb6f722f179f8d418 Mon Sep 17 00:00:00 2001
From: Yu-Ju Hong <yjhong@google.com>
Date: Wed, 8 Mar 2017 15:59:20 -0800
Subject: [PATCH] dockershim: Fix the race condition in ListPodSandbox

In ListPodSandbox(), we
 1. List all sandbox docker containers
 2. List all sandbox checkpoints. If the checkpoint does not have a
    corresponding container in (1), we return partial result based on
    the checkpoint.

The problem is that new PodSandboxes can be created between step (1) and
(2). In those cases, we will see the checkpoints, but not the sandbox
containers. This leads to strange behavior because the partial result
from the checkpoint does not include some critical information. For
example, the creation timestamp'd be zero, and that would cause kubelet's
garbage collector to immediately remove the sandbox.

This change fixes that by getting the list of checkpoints before listing
all the containers (since in RunPodSandbox we create them in the reverse
order).
---
 pkg/kubelet/dockershim/docker_sandbox.go | 40 ++++++++++++++----------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/pkg/kubelet/dockershim/docker_sandbox.go b/pkg/kubelet/dockershim/docker_sandbox.go
index 7c5298d0b65..9918e4556bb 100644
--- a/pkg/kubelet/dockershim/docker_sandbox.go
+++ b/pkg/kubelet/dockershim/docker_sandbox.go
@@ -341,6 +341,18 @@ func (ds *dockerService) ListPodSandbox(filter *runtimeapi.PodSandboxFilter) ([]
 			}
 		}
 	}
+
+	// Make sure we get the list of checkpoints first so that we don't include
+	// new PodSandboxes that are being created right now.
+	var err error
+	checkpoints := []string{}
+	if filter == nil {
+		checkpoints, err = ds.checkpointHandler.ListCheckpoints()
+		if err != nil {
+			glog.Errorf("Failed to list checkpoints: %v", err)
+		}
+	}
+
 	containers, err := ds.client.ListContainers(opts)
 	if err != nil {
 		return nil, err
@@ -367,27 +379,21 @@ func (ds *dockerService) ListPodSandbox(filter *runtimeapi.PodSandboxFilter) ([]
 	// Include sandbox that could only be found with its checkpoint if no filter is applied
 	// These PodSandbox will only include PodSandboxID, Name, Namespace.
 	// These PodSandbox will be in PodSandboxState_SANDBOX_NOTREADY state.
-	if filter == nil {
-		checkpoints, err := ds.checkpointHandler.ListCheckpoints()
+	for _, id := range checkpoints {
+		if _, ok := sandboxIDs[id]; ok {
+			continue
+		}
+		checkpoint, err := ds.checkpointHandler.GetCheckpoint(id)
 		if err != nil {
-			glog.Errorf("Failed to list checkpoints: %v", err)
-		}
-		for _, id := range checkpoints {
-			if _, ok := sandboxIDs[id]; ok {
-				continue
-			}
-			checkpoint, err := ds.checkpointHandler.GetCheckpoint(id)
-			if err != nil {
-				glog.Errorf("Failed to retrieve checkpoint for sandbox %q: %v", id, err)
+			glog.Errorf("Failed to retrieve checkpoint for sandbox %q: %v", id, err)
 
-				if err == errors.CorruptCheckpointError {
-					glog.V(2).Info("Removing corrupted checkpoint %q: %+v", id, *checkpoint)
-					ds.checkpointHandler.RemoveCheckpoint(id)
-				}
-				continue
+			if err == errors.CorruptCheckpointError {
+				glog.V(2).Info("Removing corrupted checkpoint %q: %+v", id, *checkpoint)
+				ds.checkpointHandler.RemoveCheckpoint(id)
 			}
-			result = append(result, checkpointToRuntimeAPISandbox(id, checkpoint))
+			continue
 		}
+		result = append(result, checkpointToRuntimeAPISandbox(id, checkpoint))
 	}
 
 	// Include legacy sandboxes if there are still legacy sandboxes not cleaned up yet.