diff --git a/pkg/kubelet/container/fake_runtime.go b/pkg/kubelet/container/fake_runtime.go index 8a64dc959cc..3602a01405b 100644 --- a/pkg/kubelet/container/fake_runtime.go +++ b/pkg/kubelet/container/fake_runtime.go @@ -236,15 +236,6 @@ func (f *FakeRuntime) KillContainerInPod(container api.Container, pod *api.Pod) return f.Err } -func (f *FakeRuntime) GetAPIPodStatus(*api.Pod) (*api.PodStatus, error) { - f.Lock() - defer f.Unlock() - - f.CalledFunctions = append(f.CalledFunctions, "GetAPIPodStatus") - status := f.APIPodStatus - return &status, f.Err -} - func (f *FakeRuntime) GetPodStatus(uid types.UID, name, namespace string) (*PodStatus, error) { f.Lock() defer f.Unlock() diff --git a/pkg/kubelet/container/runtime.go b/pkg/kubelet/container/runtime.go index 4f3dabadbbd..5afa916df24 100644 --- a/pkg/kubelet/container/runtime.go +++ b/pkg/kubelet/container/runtime.go @@ -69,11 +69,6 @@ type Runtime interface { // KillPod kills all the containers of a pod. Pod may be nil, running pod must not be. // TODO(random-liu): Return PodSyncResult in KillPod. KillPod(pod *api.Pod, runningPod Pod) error - // GetAPIPodStatus retrieves the api.PodStatus of the pod, including the information of - // all containers in the pod. Clients of this interface assume the - // containers' statuses in a pod always have a deterministic ordering - // (e.g., sorted by name). - GetAPIPodStatus(*api.Pod) (*api.PodStatus, error) // GetPodStatus retrieves the status of the pod, including the // information of all containers in the pod that are visble in Runtime. GetPodStatus(uid types.UID, name, namespace string) (*PodStatus, error) diff --git a/pkg/kubelet/container/runtime_mock.go b/pkg/kubelet/container/runtime_mock.go index fd8eaec88eb..a4843ccca0d 100644 --- a/pkg/kubelet/container/runtime_mock.go +++ b/pkg/kubelet/container/runtime_mock.go @@ -77,11 +77,6 @@ func (r *Mock) KillContainerInPod(container api.Container, pod *api.Pod) error { return args.Error(0) } -func (r *Mock) GetAPIPodStatus(pod *api.Pod) (*api.PodStatus, error) { - args := r.Called(pod) - return args.Get(0).(*api.PodStatus), args.Error(1) -} - func (r *Mock) GetPodStatus(uid types.UID, name, namespace string) (*PodStatus, error) { args := r.Called(uid, name, namespace) return args.Get(0).(*PodStatus), args.Error(1) diff --git a/pkg/kubelet/dockertools/fake_docker_client.go b/pkg/kubelet/dockertools/fake_docker_client.go index f73eebffdee..2b7364ed9cb 100644 --- a/pkg/kubelet/dockertools/fake_docker_client.go +++ b/pkg/kubelet/dockertools/fake_docker_client.go @@ -250,7 +250,7 @@ func (f *FakeDockerClient) CreateContainer(c docker.CreateContainerOptions) (*do // Docker likes to add a '/', so copy that behavior. name := "/" + c.Name f.Created = append(f.Created, name) - // The newest container should be in front, because we assume so in GetAPIPodStatus() + // The newest container should be in front, because we assume so in GetPodStatus() f.ContainerList = append([]docker.APIContainers{ {ID: name, Names: []string{name}, Image: c.Config.Image, Labels: c.Config.Labels}, }, f.ContainerList...) @@ -300,7 +300,7 @@ func (f *FakeDockerClient) StopContainer(id string, timeout uint) error { var newList []docker.APIContainers for _, container := range f.ContainerList { if container.ID == id { - // The newest exited container should be in front. Because we assume so in GetAPIPodStatus() + // The newest exited container should be in front. Because we assume so in GetPodStatus() f.ExitedContainerList = append([]docker.APIContainers{container}, f.ExitedContainerList...) continue } diff --git a/pkg/kubelet/dockertools/manager.go b/pkg/kubelet/dockertools/manager.go index d4130f028f9..7235f89a0fc 100644 --- a/pkg/kubelet/dockertools/manager.go +++ b/pkg/kubelet/dockertools/manager.go @@ -104,8 +104,7 @@ type DockerManager struct { // means that some entries may be recycled before a pod has been // deleted. reasonCache reasonInfoCache - // TODO(yifan): Record the pull failure so we can eliminate the image checking - // in GetAPIPodStatus()? + // TODO(yifan): Record the pull failure so we can eliminate the image checking? // Lower level docker image puller. dockerPuller DockerPuller @@ -419,17 +418,6 @@ func (dm *DockerManager) inspectContainer(id string, podName, podNamespace strin return &status, "", nil } -// GetAPIPodStatus returns docker related status for all containers in the pod -// spec. -func (dm *DockerManager) GetAPIPodStatus(pod *api.Pod) (*api.PodStatus, error) { - // Get the pod status. - podStatus, err := dm.GetPodStatus(pod.UID, pod.Name, pod.Namespace) - if err != nil { - return nil, err - } - return dm.ConvertPodStatusToAPIPodStatus(pod, podStatus) -} - func (dm *DockerManager) ConvertPodStatusToAPIPodStatus(pod *api.Pod, podStatus *kubecontainer.PodStatus) (*api.PodStatus, error) { var apiPodStatus api.PodStatus uid := pod.UID diff --git a/pkg/kubelet/dockertools/manager_test.go b/pkg/kubelet/dockertools/manager_test.go index a360e978455..6c11aa66919 100644 --- a/pkg/kubelet/dockertools/manager_test.go +++ b/pkg/kubelet/dockertools/manager_test.go @@ -31,10 +31,8 @@ import ( docker "github.com/fsouza/go-dockerclient" cadvisorapi "github.com/google/cadvisor/info/v1" - "github.com/stretchr/testify/assert" "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/api/testapi" - "k8s.io/kubernetes/pkg/api/unversioned" "k8s.io/kubernetes/pkg/client/record" kubecontainer "k8s.io/kubernetes/pkg/kubelet/container" "k8s.io/kubernetes/pkg/kubelet/network" @@ -557,7 +555,6 @@ func runSyncPod(t *testing.T, dm *DockerManager, fakeDocker *FakeDockerClient, p if backOff == nil { backOff = util.NewBackOff(time.Second, time.Minute) } - //TODO(random-liu): Add test for PodSyncResult result := dm.SyncPod(pod, *apiPodStatus, podStatus, []api.Secret{}, backOff) err = result.Error() if err != nil && !expectErr { @@ -890,65 +887,6 @@ func TestSyncPodsDoesNothing(t *testing.T) { }) } -func TestSyncPodWithPullPolicy(t *testing.T) { - dm, fakeDocker := newTestDockerManager() - puller := dm.dockerPuller.(*FakeDockerPuller) - puller.HasImages = []string{"existing_one", "want:latest"} - dm.podInfraContainerImage = "pod_infra_image" - - pod := &api.Pod{ - ObjectMeta: api.ObjectMeta{ - UID: "12345678", - Name: "foo", - Namespace: "new", - }, - Spec: api.PodSpec{ - Containers: []api.Container{ - {Name: "bar", Image: "pull_always_image", ImagePullPolicy: api.PullAlways}, - {Name: "bar2", Image: "pull_if_not_present_image", ImagePullPolicy: api.PullIfNotPresent}, - {Name: "bar3", Image: "existing_one", ImagePullPolicy: api.PullIfNotPresent}, - {Name: "bar4", Image: "want:latest", ImagePullPolicy: api.PullIfNotPresent}, - {Name: "bar5", Image: "pull_never_image", ImagePullPolicy: api.PullNever}, - }, - }, - } - - expectedStatusMap := map[string]api.ContainerState{ - "bar": {Running: &api.ContainerStateRunning{unversioned.Now()}}, - "bar2": {Running: &api.ContainerStateRunning{unversioned.Now()}}, - "bar3": {Running: &api.ContainerStateRunning{unversioned.Now()}}, - "bar4": {Running: &api.ContainerStateRunning{unversioned.Now()}}, - "bar5": {Waiting: &api.ContainerStateWaiting{Reason: kubecontainer.ErrImageNeverPull.Error(), - Message: "Container image \"pull_never_image\" is not present with pull policy of Never"}}, - } - - runSyncPod(t, dm, fakeDocker, pod, nil, true) - statuses, err := dm.GetAPIPodStatus(pod) - if err != nil { - t.Errorf("unable to get pod status") - } - for _, c := range pod.Spec.Containers { - if containerStatus, ok := api.GetContainerStatus(statuses.ContainerStatuses, c.Name); ok { - // copy the StartedAt time, to make the structs match - if containerStatus.State.Running != nil && expectedStatusMap[c.Name].Running != nil { - expectedStatusMap[c.Name].Running.StartedAt = containerStatus.State.Running.StartedAt - } - assert.Equal(t, expectedStatusMap[c.Name], containerStatus.State, "for container %s", c.Name) - } - } - - fakeDocker.Lock() - defer fakeDocker.Unlock() - - pulledImageSorted := puller.ImagesPulled[:] - sort.Strings(pulledImageSorted) - assert.Equal(t, []string{"pod_infra_image", "pull_always_image", "pull_if_not_present_image"}, pulledImageSorted) - - if len(fakeDocker.Created) != 5 { - t.Errorf("Unexpected containers created %v", fakeDocker.Created) - } -} - func TestSyncPodWithRestartPolicy(t *testing.T) { dm, fakeDocker := newTestDockerManager() containers := []api.Container{ @@ -1053,112 +991,6 @@ func TestSyncPodWithRestartPolicy(t *testing.T) { } } -func TestGetAPIPodStatusWithLastTermination(t *testing.T) { - dm, fakeDocker := newTestDockerManager() - containers := []api.Container{ - {Name: "succeeded"}, - {Name: "failed"}, - } - - pod := &api.Pod{ - ObjectMeta: api.ObjectMeta{ - UID: "12345678", - Name: "foo", - Namespace: "new", - }, - Spec: api.PodSpec{ - Containers: containers, - }, - } - - dockerContainers := []*docker.Container{ - { - ID: "9876", - Name: "/k8s_POD." + strconv.FormatUint(generatePodInfraContainerHash(pod), 16) + "_foo_new_12345678_0", - State: docker.State{ - StartedAt: time.Now(), - FinishedAt: time.Now(), - Running: true, - }, - }, - { - ID: "1234", - Name: "/k8s_succeeded." + strconv.FormatUint(kubecontainer.HashContainer(&containers[0]), 16) + "_foo_new_12345678_0", - State: docker.State{ - ExitCode: 0, - StartedAt: time.Now(), - FinishedAt: time.Now(), - }, - }, - { - ID: "5678", - Name: "/k8s_failed." + strconv.FormatUint(kubecontainer.HashContainer(&containers[1]), 16) + "_foo_new_12345678_0", - State: docker.State{ - ExitCode: 42, - StartedAt: time.Now(), - FinishedAt: time.Now(), - }, - }, - } - - tests := []struct { - policy api.RestartPolicy - created []string - stopped []string - lastTerminations []string - }{ - { - api.RestartPolicyAlways, - []string{"succeeded", "failed"}, - []string{}, - []string{"docker://1234", "docker://5678"}, - }, - { - api.RestartPolicyOnFailure, - []string{"failed"}, - []string{}, - []string{"docker://5678"}, - }, - { - api.RestartPolicyNever, - []string{}, - []string{"9876"}, - []string{}, - }, - } - - for i, tt := range tests { - fakeDocker.SetFakeContainers(dockerContainers) - fakeDocker.ClearCalls() - pod.Spec.RestartPolicy = tt.policy - runSyncPod(t, dm, fakeDocker, pod, nil, false) - - // Check if we can retrieve the pod status. - status, err := dm.GetAPIPodStatus(pod) - if err != nil { - t.Fatalf("unexpected error %v", err) - } - terminatedContainers := []string{} - for _, cs := range status.ContainerStatuses { - if cs.LastTerminationState.Terminated != nil { - terminatedContainers = append(terminatedContainers, cs.LastTerminationState.Terminated.ContainerID) - } - } - sort.StringSlice(terminatedContainers).Sort() - sort.StringSlice(tt.lastTerminations).Sort() - if !reflect.DeepEqual(terminatedContainers, tt.lastTerminations) { - t.Errorf("Expected(sorted): %#v, Actual(sorted): %#v", tt.lastTerminations, terminatedContainers) - } - - if err := fakeDocker.AssertCreated(tt.created); err != nil { - t.Errorf("%d: %v", i, err) - } - if err := fakeDocker.AssertStopped(tt.stopped); err != nil { - t.Errorf("%d: %v", i, err) - } - } -} - func TestSyncPodBackoff(t *testing.T) { var fakeClock = &util.FakeClock{Time: time.Now()} startTime := fakeClock.Now() @@ -1248,101 +1080,10 @@ func TestSyncPodBackoff(t *testing.T) { } } } -func TestGetPodCreationFailureReason(t *testing.T) { - dm, fakeDocker := newTestDockerManager() - // Inject the creation failure error to docker. - failureReason := "RunContainerError" - fakeDocker.Errors = map[string]error{ - "create": fmt.Errorf("%s", failureReason), - } - - pod := &api.Pod{ - ObjectMeta: api.ObjectMeta{ - UID: "12345678", - Name: "foo", - Namespace: "new", - }, - Spec: api.PodSpec{ - Containers: []api.Container{{Name: "bar"}}, - }, - } - - // Pretend that the pod infra container has already been created, so that - // we can run the user containers. - fakeDocker.SetFakeRunningContainers([]*docker.Container{{ - ID: "9876", - Name: "/k8s_POD." + strconv.FormatUint(generatePodInfraContainerHash(pod), 16) + "_foo_new_12345678_0", - }}) - - runSyncPod(t, dm, fakeDocker, pod, nil, true) - // Check if we can retrieve the pod status. - status, err := dm.GetAPIPodStatus(pod) - if err != nil { - t.Fatalf("unexpected error %v", err) - } - - if len(status.ContainerStatuses) < 1 { - t.Errorf("expected 1 container status, got %d", len(status.ContainerStatuses)) - } else { - state := status.ContainerStatuses[0].State - if state.Waiting == nil { - t.Errorf("expected waiting state, got %#v", state) - } else if state.Waiting.Reason != failureReason { - t.Errorf("expected reason %q, got %q", failureReason, state.Waiting.Reason) - } - } -} - -func TestGetPodPullImageFailureReason(t *testing.T) { - dm, fakeDocker := newTestDockerManager() - // Initialize the FakeDockerPuller so that it'd try to pull non-existent - // images. - puller := dm.dockerPuller.(*FakeDockerPuller) - puller.HasImages = []string{} - // Inject the pull image failure error. - failureReason := kubecontainer.ErrImagePull.Error() - puller.ErrorsToInject = []error{fmt.Errorf("%s", failureReason)} - - pod := &api.Pod{ - ObjectMeta: api.ObjectMeta{ - UID: "12345678", - Name: "foo", - Namespace: "new", - }, - Spec: api.PodSpec{ - Containers: []api.Container{{Name: "bar", Image: "realImage", ImagePullPolicy: api.PullAlways}}, - }, - } - // Pretend that the pod infra container has already been created, so that - // we can run the user containers. - fakeDocker.SetFakeRunningContainers([]*docker.Container{{ - ID: "9876", - Name: "/k8s_POD." + strconv.FormatUint(generatePodInfraContainerHash(pod), 16) + "_foo_new_12345678_0", - }}) - runSyncPod(t, dm, fakeDocker, pod, nil, true) - // Check if we can retrieve the pod status. - status, err := dm.GetAPIPodStatus(pod) - if err != nil { - t.Fatalf("unexpected error %v", err) - } - - if len(status.ContainerStatuses) < 1 { - t.Errorf("expected 1 container status, got %d", len(status.ContainerStatuses)) - } else { - state := status.ContainerStatuses[0].State - if state.Waiting == nil { - t.Errorf("expected waiting state, got %#v", state) - } else if state.Waiting.Reason != failureReason { - t.Errorf("expected reason %q, got %q", failureReason, state.Waiting.Reason) - } - } -} func TestGetRestartCount(t *testing.T) { dm, fakeDocker := newTestDockerManager() - containers := []api.Container{ - {Name: "bar"}, - } + containerName := "bar" pod := api.Pod{ ObjectMeta: api.ObjectMeta{ UID: "12345678", @@ -1350,67 +1091,83 @@ func TestGetRestartCount(t *testing.T) { Namespace: "new", }, Spec: api.PodSpec{ - Containers: containers, + Containers: []api.Container{ + {Name: containerName}, + }, RestartPolicy: "Always", }, + Status: api.PodStatus{ + ContainerStatuses: []api.ContainerStatus{ + { + Name: containerName, + RestartCount: 3, + }, + }, + }, } // Helper function for verifying the restart count. - verifyRestartCount := func(pod *api.Pod, expectedCount int) api.PodStatus { + verifyRestartCount := func(pod *api.Pod, expectedCount int) { runSyncPod(t, dm, fakeDocker, pod, nil, false) - status, err := dm.GetAPIPodStatus(pod) + status, err := dm.GetPodStatus(pod.UID, pod.Name, pod.Namespace) if err != nil { t.Fatalf("Unexpected error %v", err) } - restartCount := status.ContainerStatuses[0].RestartCount + cs := status.FindContainerStatusByName(containerName) + if cs == nil { + t.Fatal("Can't find status for container %q", containerName) + } + restartCount := cs.RestartCount if restartCount != expectedCount { t.Errorf("expected %d restart count, got %d", expectedCount, restartCount) } - return *status } killOneContainer := func(pod *api.Pod) { - status, err := dm.GetAPIPodStatus(pod) + status, err := dm.GetPodStatus(pod.UID, pod.Name, pod.Namespace) if err != nil { t.Fatalf("Unexpected error %v", err) } - containerID := kubecontainer.ParseContainerID(status.ContainerStatuses[0].ContainerID) - dm.KillContainerInPod(containerID, &pod.Spec.Containers[0], pod, "test container restart count.") + cs := status.FindContainerStatusByName(containerName) + if cs == nil { + t.Fatal("Can't find status for container %q", containerName) + } + dm.KillContainerInPod(cs.ID, &pod.Spec.Containers[0], pod, "test container restart count.") } // Container "bar" starts the first time. // TODO: container lists are expected to be sorted reversely by time. // We should fix FakeDockerClient to sort the list before returning. // (randome-liu) Just partially sorted now. - pod.Status = verifyRestartCount(&pod, 0) + verifyRestartCount(&pod, 0) killOneContainer(&pod) // Poor container "bar" has been killed, and should be restarted with restart count 1 - pod.Status = verifyRestartCount(&pod, 1) + verifyRestartCount(&pod, 1) killOneContainer(&pod) // Poor container "bar" has been killed again, and should be restarted with restart count 2 - pod.Status = verifyRestartCount(&pod, 2) + verifyRestartCount(&pod, 2) killOneContainer(&pod) // Poor container "bar" has been killed again ang again, and should be restarted with restart count 3 - pod.Status = verifyRestartCount(&pod, 3) + verifyRestartCount(&pod, 3) // The oldest container has been garbage collected exitedContainers := fakeDocker.ExitedContainerList fakeDocker.ExitedContainerList = exitedContainers[:len(exitedContainers)-1] - pod.Status = verifyRestartCount(&pod, 3) + verifyRestartCount(&pod, 3) // The last two oldest containers have been garbage collected fakeDocker.ExitedContainerList = exitedContainers[:len(exitedContainers)-2] - pod.Status = verifyRestartCount(&pod, 3) + verifyRestartCount(&pod, 3) - // All exited containers have been garbage collected + // All exited containers have been garbage collected, restart count should be got from old api pod status fakeDocker.ExitedContainerList = []docker.APIContainers{} - pod.Status = verifyRestartCount(&pod, 3) + verifyRestartCount(&pod, 3) killOneContainer(&pod) // Poor container "bar" has been killed again ang again and again, and should be restarted with restart count 4 - pod.Status = verifyRestartCount(&pod, 4) + verifyRestartCount(&pod, 4) } func TestGetTerminationMessagePath(t *testing.T) { @@ -1681,57 +1438,6 @@ func TestSyncPodWithHostNetwork(t *testing.T) { } } -func TestGetAPIPodStatusSortedContainers(t *testing.T) { - dm, fakeDocker := newTestDockerManager() - specContainerList := []api.Container{} - expectedOrder := []string{} - - numContainers := 10 - podName := "foo" - podNs := "test" - podUID := "uid1" - fakeConfig := &docker.Config{ - Image: "some:latest", - } - - dockerContainers := []*docker.Container{} - for i := 0; i < numContainers; i++ { - id := fmt.Sprintf("%v", i) - containerName := fmt.Sprintf("%vcontainer", id) - expectedOrder = append(expectedOrder, containerName) - dockerContainers = append(dockerContainers, &docker.Container{ - ID: id, - Name: fmt.Sprintf("/k8s_%v_%v_%v_%v_42", containerName, podName, podNs, podUID), - Config: fakeConfig, - Image: fmt.Sprintf("%vimageid", id), - }) - specContainerList = append(specContainerList, api.Container{Name: containerName}) - } - fakeDocker.SetFakeRunningContainers(dockerContainers) - fakeDocker.ClearCalls() - pod := &api.Pod{ - ObjectMeta: api.ObjectMeta{ - UID: types.UID(podUID), - Name: podName, - Namespace: podNs, - }, - Spec: api.PodSpec{ - Containers: specContainerList, - }, - } - for i := 0; i < 5; i++ { - status, err := dm.GetAPIPodStatus(pod) - if err != nil { - t.Fatalf("unexpected error %v", err) - } - for i, c := range status.ContainerStatuses { - if expectedOrder[i] != c.Name { - t.Fatalf("Container status not sorted, expected %v at index %d, but found %v", expectedOrder[i], i, c.Name) - } - } - } -} - func TestVerifyNonRoot(t *testing.T) { dm, fakeDocker := newTestDockerManager() @@ -1893,3 +1599,5 @@ func TestGetIPCMode(t *testing.T) { t.Errorf("expected host ipc mode for pod but got %v", ipcMode) } } + +// TODO(random-liu): Add unit test for returned PodSyncResult (issue #20478) diff --git a/pkg/kubelet/kubelet_test.go b/pkg/kubelet/kubelet_test.go index 8a5c11d6d82..ac3bb8e84db 100644 --- a/pkg/kubelet/kubelet_test.go +++ b/pkg/kubelet/kubelet_test.go @@ -4348,3 +4348,5 @@ func TestGetPodsToSync(t *testing.T) { t.Errorf("expected %d pods to sync, got %d", 3, len(podsToSync)) } } + +// TODO(random-liu): Add unit test for convertStatusToAPIStatus (issue #20478) diff --git a/pkg/kubelet/rkt/rkt.go b/pkg/kubelet/rkt/rkt.go index fa9f6004722..98d3fd070ea 100644 --- a/pkg/kubelet/rkt/rkt.go +++ b/pkg/kubelet/rkt/rkt.go @@ -1009,16 +1009,6 @@ func (r *Runtime) KillPod(pod *api.Pod, runningPod kubecontainer.Pod) error { return nil } -// GetAPIPodStatus returns the status of the given pod. -func (r *Runtime) GetAPIPodStatus(pod *api.Pod) (*api.PodStatus, error) { - // Get the pod status. - podStatus, err := r.GetPodStatus(pod.UID, pod.Name, pod.Namespace) - if err != nil { - return nil, err - } - return r.ConvertPodStatusToAPIPodStatus(pod, podStatus) -} - func (r *Runtime) Type() string { return RktType }