Merge pull request #4756 from vishh/kubelet

Relax constraints on container status while fetching container logs

pkg/kubelet/kubelet.go

@@ -1538,6 +1538,26 @@ func (kl *Kubelet) GetDockerVersion() ([]uint, error) {
 	return dockerRunner.GetDockerServerVersion()
 }
 
+func (kl *Kubelet) validatePodPhase(podStatus *api.PodStatus) error {
+	switch podStatus.Phase {
+	case api.PodRunning, api.PodSucceeded, api.PodFailed:
+		return nil
+	}
+	return fmt.Errorf("pod is not in 'Running', 'Succeeded' or 'Failed' state - State: %q", podStatus.Phase)
+}
+
+func (kl *Kubelet) validateContainerStatus(podStatus *api.PodStatus, containerName string) (dockerID string, err error) {
+	for cName, cStatus := range podStatus.Info {
+		if containerName == cName {
+			if cStatus.State.Waiting != nil {
+				return "", fmt.Errorf("container %q is in waiting state.", containerName)
+			}
+			return strings.Replace(podStatus.Info[containerName].ContainerID, dockertools.DockerPrefix, "", 1), nil
+		}
+	}
+	return "", fmt.Errorf("container %q not found in pod", containerName)
+}
+
 // GetKubeletContainerLogs returns logs from the container
 // The second parameter of GetPodStatus and FindPodContainer methods represents pod UUID, which is allowed to be blank
 // TODO: this method is returning logs of random container attempts, when it should be returning the most recent attempt
@@ -1551,27 +1571,14 @@ func (kl *Kubelet) GetKubeletContainerLogs(podFullName, containerName, tail stri
 			return fmt.Errorf("failed to get status for pod %q - %v", podFullName, err)
 		}
 	}
-	switch podStatus.Phase {
-	case api.PodRunning, api.PodSucceeded, api.PodFailed:
-		break
-	default:
-		return fmt.Errorf("pod %q is not in 'Running', 'Succeeded' or 'Failed' state - State: %q", podFullName, podStatus.Phase)
-	}
-	exists := false
-	dockerContainerID := ""
-	for cName, cStatus := range podStatus.Info {
-		if containerName == cName {
-			exists = true
-			if !cStatus.Ready {
-				return fmt.Errorf("container %q is not ready.", containerName)
-			}
-			dockerContainerID = strings.Replace(podStatus.Info[containerName].ContainerID, dockertools.DockerPrefix, "", 1)
-		}
-	}
-	if !exists {
-		return fmt.Errorf("container %q not found in pod %q", containerName, podFullName)
-	}
 
+	if err := kl.validatePodPhase(&podStatus); err != nil {
+		return err
+	}
+	dockerContainerID, err := kl.validateContainerStatus(&podStatus, containerName)
+	if err != nil {
+		return err
+	}
 	return dockertools.GetKubeletDockerContainerLogs(kl.dockerClient, dockerContainerID, tail, follow, stdout, stderr)
 }
 

pkg/kubelet/kubelet_test.go

@@ -3122,5 +3122,69 @@ func TestFilterHostPortConflicts(t *testing.T) {
 	if filteredPods[0].Name != "oldpod" {
 		t.Fatalf("Expected pod %#v. Got pod %#v", pods[1], filteredPods[0])
 	}
-
+}
+
+func TestValidatePodStatus(t *testing.T) {
+	kubelet, _, _ := newTestKubelet(t)
+	testCases := []struct {
+		podPhase api.PodPhase
+		success  bool
+	}{
+		{api.PodRunning, true},
+		{api.PodSucceeded, true},
+		{api.PodFailed, true},
+		{api.PodPending, false},
+		{api.PodUnknown, false},
+	}
+
+	for i, tc := range testCases {
+		err := kubelet.validatePodPhase(&api.PodStatus{Phase: tc.podPhase})
+		if tc.success {
+			if err != nil {
+				t.Errorf("[case %d]: unexpected failure - %v", i, err)
+			}
+		} else if err == nil {
+			t.Errorf("[case %d]: unexpected success", i)
+		}
+	}
+}
+
+func TestValidateContainerStatus(t *testing.T) {
+	kubelet, _, _ := newTestKubelet(t)
+	containerName := "x"
+	testCases := []struct {
+		podInfo api.PodInfo
+		success bool
+	}{
+		{
+			podInfo: api.PodInfo{containerName: api.ContainerStatus{State: api.ContainerState{Running: &api.ContainerStateRunning{}}}},
+			success: true,
+		},
+		{
+			podInfo: api.PodInfo{containerName: api.ContainerStatus{State: api.ContainerState{Termination: &api.ContainerStateTerminated{}}}},
+			success: true,
+		},
+		{
+			podInfo: api.PodInfo{containerName: api.ContainerStatus{State: api.ContainerState{Waiting: &api.ContainerStateWaiting{}}}},
+			success: false,
+		},
+	}
+
+	for i, tc := range testCases {
+		_, err := kubelet.validateContainerStatus(&api.PodStatus{
+			Info: tc.podInfo,
+		}, containerName)
+		if tc.success {
+			if err != nil {
+				t.Errorf("[case %d]: unexpected failure - %v", i, err)
+			}
+		} else if err == nil {
+			t.Errorf("[case %d]: unexpected success", i)
+		}
+	}
+	if _, err := kubelet.validateContainerStatus(&api.PodStatus{
+		Info: testCases[0].podInfo,
+	}, "blah"); err == nil {
+		t.Errorf("expected error with invalid container name")
+	}
 }