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).

dockershim: Fix the race condition in ListPodSandbox
8328a66b · Yu-Ju Hong · 62766c69 · 8328a66b
Commit 8328a66b authored Mar 08, 2017 by Yu-Ju Hong
Hide whitespace changes
Inline Side-by-side

Showing with 24 additions and 18 deletions

docker_sandbox.go pkg/kubelet/dockershim/docker_sandbox.go +24 -18

No files found.
--- 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()
-		if err != nil {
-			glog.Errorf("Failed to list checkpoints: %v", err)
+	for _, id := range checkpoints {
+		if _, ok := sandboxIDs[id]; ok {
+			continue
 		}
-		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)
-
-				if err == errors.CorruptCheckpointError {
-					glog.V(2).Info("Removing corrupted checkpoint %q: %+v", id, *checkpoint)
-					ds.checkpointHandler.RemoveCheckpoint(id)
-				}
-				continue
+		checkpoint, err := ds.checkpointHandler.GetCheckpoint(id)
+		if err != nil {
+			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)
 			}
-			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.