Merge pull request kubernetes#4373 from vishh/3391

Improve error handling for '/containerLogs' API in kubelet.
duda · Feb 12, 2015 · 35a62fb · 35a62fb
2 parents c216b26 + 922881f
commit 35a62fb
Show file tree

Hide file tree

Showing 4 changed files with 80 additions and 54 deletions.
diff --git a/pkg/kubelet/dockertools/docker.go b/pkg/kubelet/dockertools/docker.go
@@ -513,7 +513,7 @@ func GetDockerPodInfo(client DockerInterface, manifest api.PodSpec, podFullName
 
 	if len(info) < (len(manifest.Containers) + 1) {
 		var containerStatus api.ContainerStatus
-		// Not all containers expected are created, verify if there are
+		// Not all containers expected are created, check if there are
 		// image related issues
 		for _, container := range manifest.Containers {
 			if _, found := info[container.Name]; found {

diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go
@@ -1415,19 +1415,33 @@ func (kl *Kubelet) GetDockerVersion() ([]uint, error) {
 // TODO: this method is returning logs of random container attempts, when it should be returning the most recent attempt
 // or all of them.
 func (kl *Kubelet) GetKubeletContainerLogs(podFullName, containerName, tail string, follow bool, stdout, stderr io.Writer) error {
-	_, err := kl.GetPodStatus(podFullName, "")
-	if err == dockertools.ErrNoContainersInPod {
-		return fmt.Errorf("pod not found (%q)\n", podFullName)
-	}
-	dockerContainers, err := dockertools.GetKubeletDockerContainers(kl.dockerClient, true)
+	podStatus, err := kl.GetPodStatus(podFullName, "")
 	if err != nil {
-		return err
+		if err == dockertools.ErrNoContainersInPod {
+			return fmt.Errorf("pod %q not found\n", podFullName)
+		} else {
+			return fmt.Errorf("failed to get status for pod %q - %v", podFullName, err)
+		}
 	}
-	dockerContainer, found, _ := dockerContainers.FindPodContainer(podFullName, "", containerName)
-	if !found {
-		return fmt.Errorf("container not found (%q)\n", containerName)
+	if podStatus.Phase != api.PodRunning {
+		return fmt.Errorf("pod %q is not in 'Running' state - State: %q", podFullName, podStatus.Phase)
 	}
-	return dockertools.GetKubeletDockerContainerLogs(kl.dockerClient, dockerContainer.ID, tail, follow, stdout, stderr)
+	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)
+	}
+
+	return dockertools.GetKubeletDockerContainerLogs(kl.dockerClient, dockerContainerID, tail, follow, stdout, stderr)
 }
 
 // GetBoundPods returns all pods bound to the kubelet and their spec

diff --git a/pkg/kubelet/server.go b/pkg/kubelet/server.go
@@ -185,7 +185,18 @@ func (s *Server) handleContainerLogs(w http.ResponseWriter, req *http.Request) {
 
 	pod, ok := s.host.GetPodByName(podNamespace, podID)
 	if !ok {
-		http.Error(w, "Pod does not exist", http.StatusNotFound)
+		http.Error(w, fmt.Sprintf("Pod %q does not exist", podID), http.StatusNotFound)
+		return
+	}
+	// Check if containerName is valid.
+	containerExists := false
+	for _, container := range pod.Spec.Containers {
+		if container.Name == containerName {
+			containerExists = true
+		}
+	}
+	if !containerExists {
+		http.Error(w, fmt.Sprintf("Container %q not found in Pod %q", containerName, podID), http.StatusNotFound)
 		return
 	}
 

diff --git a/pkg/kubelet/server_test.go b/pkg/kubelet/server_test.go
@@ -380,15 +380,28 @@ func TestServeRunInContainerWithUID(t *testing.T) {
 	}
 }
 
-func TestContainerLogs(t *testing.T) {
-	fw := newServerTest()
-	output := "foo bar"
-	podNamespace := "other"
-	podName := "foo"
-	expectedPodName := podName + ".other.etcd"
-	expectedContainerName := "baz"
-	expectedTail := ""
-	expectedFollow := false
+func setPodByNameFunc(fw *serverTestFramework, namespace, pod, container string) {
+	fw.fakeKubelet.podByNameFunc = func(namespace, name string) (*api.BoundPod, bool) {
+		return &api.BoundPod{
+			ObjectMeta: api.ObjectMeta{
+				Namespace: namespace,
+				Name:      pod,
+				Annotations: map[string]string{
+					ConfigSourceAnnotationKey: "etcd",
+				},
+			},
+			Spec: api.PodSpec{
+				Containers: []api.Container{
+					{
+						Name: container,
+					},
+				},
+			},
+		}, true
+	}
+}
+
+func setGetContainerLogsFunc(fw *serverTestFramework, t *testing.T, expectedPodName, expectedContainerName, expectedTail string, expectedFollow bool, output string) {
 	fw.fakeKubelet.containerLogsFunc = func(podFullName, containerName, tail string, follow bool, stdout, stderr io.Writer) error {
 		if podFullName != expectedPodName {
 			t.Errorf("expected %s, got %s", expectedPodName, podFullName)
@@ -402,8 +415,22 @@ func TestContainerLogs(t *testing.T) {
 		if follow != expectedFollow {
 			t.Errorf("expected %t, got %t", expectedFollow, follow)
 		}
+		io.WriteString(stdout, output)
 		return nil
 	}
+}
+
+func TestContainerLogs(t *testing.T) {
+	fw := newServerTest()
+	output := "foo bar"
+	podNamespace := "other"
+	podName := "foo"
+	expectedPodName := podName + ".other.etcd"
+	expectedContainerName := "baz"
+	expectedTail := ""
+	expectedFollow := false
+	setPodByNameFunc(fw, podNamespace, podName, expectedContainerName)
+	setGetContainerLogsFunc(fw, t, expectedPodName, expectedContainerName, expectedTail, expectedFollow, output)
 	resp, err := http.Get(fw.testHTTPServer.URL + "/containerLogs/" + podNamespace + "/" + podName + "/" + expectedContainerName)
 	if err != nil {
 		t.Errorf("Got error GETing: %v", err)
@@ -415,7 +442,7 @@ func TestContainerLogs(t *testing.T) {
 		t.Errorf("Error reading container logs: %v", err)
 	}
 	result := string(body)
-	if result != string(body) {
+	if result != output {
 		t.Errorf("Expected: '%v', got: '%v'", output, result)
 	}
 }
@@ -429,21 +456,8 @@ func TestContainerLogsWithTail(t *testing.T) {
 	expectedContainerName := "baz"
 	expectedTail := "5"
 	expectedFollow := false
-	fw.fakeKubelet.containerLogsFunc = func(podFullName, containerName, tail string, follow bool, stdout, stderr io.Writer) error {
-		if podFullName != expectedPodName {
-			t.Errorf("expected %s, got %s", expectedPodName, podFullName)
-		}
-		if containerName != expectedContainerName {
-			t.Errorf("expected %s, got %s", expectedContainerName, containerName)
-		}
-		if tail != expectedTail {
-			t.Errorf("expected %s, got %s", expectedTail, tail)
-		}
-		if follow != expectedFollow {
-			t.Errorf("expected %t, got %t", expectedFollow, follow)
-		}
-		return nil
-	}
+	setPodByNameFunc(fw, podNamespace, podName, expectedContainerName)
+	setGetContainerLogsFunc(fw, t, expectedPodName, expectedContainerName, expectedTail, expectedFollow, output)
 	resp, err := http.Get(fw.testHTTPServer.URL + "/containerLogs/" + podNamespace + "/" + podName + "/" + expectedContainerName + "?tail=5")
 	if err != nil {
 		t.Errorf("Got error GETing: %v", err)
@@ -455,7 +469,7 @@ func TestContainerLogsWithTail(t *testing.T) {
 		t.Errorf("Error reading container logs: %v", err)
 	}
 	result := string(body)
-	if result != string(body) {
+	if result != output {
 		t.Errorf("Expected: '%v', got: '%v'", output, result)
 	}
 }
@@ -469,21 +483,8 @@ func TestContainerLogsWithFollow(t *testing.T) {
 	expectedContainerName := "baz"
 	expectedTail := ""
 	expectedFollow := true
-	fw.fakeKubelet.containerLogsFunc = func(podFullName, containerName, tail string, follow bool, stdout, stderr io.Writer) error {
-		if podFullName != expectedPodName {
-			t.Errorf("expected %s, got %s", expectedPodName, podFullName)
-		}
-		if containerName != expectedContainerName {
-			t.Errorf("expected %s, got %s", expectedContainerName, containerName)
-		}
-		if tail != expectedTail {
-			t.Errorf("expected %s, got %s", expectedTail, tail)
-		}
-		if follow != expectedFollow {
-			t.Errorf("expected %t, got %t", expectedFollow, follow)
-		}
-		return nil
-	}
+	setPodByNameFunc(fw, podNamespace, podName, expectedContainerName)
+	setGetContainerLogsFunc(fw, t, expectedPodName, expectedContainerName, expectedTail, expectedFollow, output)
 	resp, err := http.Get(fw.testHTTPServer.URL + "/containerLogs/" + podNamespace + "/" + podName + "/" + expectedContainerName + "?follow=1")
 	if err != nil {
 		t.Errorf("Got error GETing: %v", err)
@@ -495,7 +496,7 @@ func TestContainerLogsWithFollow(t *testing.T) {
 		t.Errorf("Error reading container logs: %v", err)
 	}
 	result := string(body)
-	if result != string(body) {
+	if result != output {
 		t.Errorf("Expected: '%v', got: '%v'", output, result)
 	}
 }