From 20a9e0bc067b54dfb045415c3117e120d860446f Mon Sep 17 00:00:00 2001
From: Diego Pontoriero <dpontor@gmail.com>
Date: Mon, 17 Jul 2017 13:17:12 -0700
Subject: [PATCH] pod-checkpointer: isRunning check.

This could lead to a race where the pod is not running and therefore
the checkpoint isn't created, but the lack of checkpoint leaves us
vulnerable to a service disruption.

Theoretically the apiserver reconciliation loop should handle cleaning
the checkpoint up.
---
 cmd/checkpoint/main.go      | 21 ---------------
 cmd/checkpoint/main_test.go | 53 -------------------------------------
 2 files changed, 74 deletions(-)

diff --git a/cmd/checkpoint/main.go b/cmd/checkpoint/main.go
index 6bd9655de..cd42a1ec6 100644
--- a/cmd/checkpoint/main.go
+++ b/cmd/checkpoint/main.go
@@ -326,15 +326,6 @@ func process(localRunningPods, localParentPods, apiParentPods, activeCheckpoints
 //TODO(aaron): Add support for checkpointing configMaps
 func createCheckpointsForValidParents(client kubernetes.Interface, pods map[string]*v1.Pod) {
 	for _, pod := range pods {
-		// This merely check that the last kubelet pod state thinks this pod was running. It's possible that
-		// state is actually stale (only as recent as last successful contact with api-server). However, this
-		// does contain the full podSpec -- so we can still attempt to checkpoint with this "last known good state".
-		//
-		// We do not use the `localPodRunning` state, because while the runtime may think the pod/containers are running -
-		// they may actually be in a failing state - and we've not successfully sent that podStatus to any api-server.
-		if !isRunning(pod) {
-			continue
-		}
 		id := PodFullName(pod)
 
 		cp, err := copyPod(pod)
@@ -692,18 +683,6 @@ func handleStart(start []string) {
 	}
 }
 
-func isRunning(pod *v1.Pod) bool {
-	// Determine if a pod is "running" by checking if each container status is in a "ready" state
-	// TODO(aaron): Figure out best sets of data to inspect. PodConditions, PodPhase, containerStatus, containerState, etc.
-	for _, containerStatus := range pod.Status.ContainerStatuses {
-		if !containerStatus.Ready {
-			glog.Infof("Container %s in pod %s not ready. Will not checkpoint", containerStatus.Name, pod.Name)
-			return false
-		}
-	}
-	return true
-}
-
 func podListToParentPods(pl *v1.PodList) map[string]*v1.Pod {
 	return podListToMap(pl, isValidParent)
 }
diff --git a/cmd/checkpoint/main_test.go b/cmd/checkpoint/main_test.go
index 416e67477..7d3cecea8 100644
--- a/cmd/checkpoint/main_test.go
+++ b/cmd/checkpoint/main_test.go
@@ -472,59 +472,6 @@ func TestPodListToParentPods(t *testing.T) {
 	}
 }
 
-func TestIsRunning(t *testing.T) {
-	type testCase struct {
-		desc     string
-		pod      *v1.Pod
-		expected bool
-	}
-
-	cases := []testCase{
-		{
-			desc: "Single container ready",
-			pod: &v1.Pod{
-				Status: v1.PodStatus{ContainerStatuses: []v1.ContainerStatus{{Ready: true}}},
-			},
-			expected: true,
-		},
-		{
-			desc: "Multiple containers, all ready",
-			pod: &v1.Pod{
-				Status: v1.PodStatus{ContainerStatuses: []v1.ContainerStatus{{Ready: true}, {Ready: true}}},
-			},
-			expected: true,
-		},
-		{
-			desc: "Single container not ready",
-			pod: &v1.Pod{
-				Status: v1.PodStatus{ContainerStatuses: []v1.ContainerStatus{{Ready: false}}},
-			},
-			expected: false,
-		},
-		{
-			desc: "Multiple containers, some not ready",
-			pod: &v1.Pod{
-				Status: v1.PodStatus{ContainerStatuses: []v1.ContainerStatus{{Ready: true}, {Ready: false}}},
-			},
-			expected: false,
-		},
-		{
-			desc: "Multiple containers, all not ready",
-			pod: &v1.Pod{
-				Status: v1.PodStatus{ContainerStatuses: []v1.ContainerStatus{{Ready: false}, {Ready: false}}},
-			},
-			expected: false,
-		},
-	}
-
-	for _, tc := range cases {
-		got := isRunning(tc.pod)
-		if tc.expected != got {
-			t.Errorf("Expected: %t Got: %t for test: %s", tc.expected, got, tc.desc)
-		}
-	}
-}
-
 func podWithAnnotations(a map[string]string) *v1.Pod {
 	return &v1.Pod{
 		ObjectMeta: metav1.ObjectMeta{