Merge pull request #33389 from Random-Liu/lifecycle-hook

Automatic merge from submit-queue

CRI: Fix lifecycle hook and add container lifecycle node e2e test

This PR:
1) Adds pod spec missing handling in kuberuntime. (1st commit)
2) Adds container lifecycle hook node e2e test. (2nd commit)

@yujuhong @feiskyer

pkg/kubelet/kuberuntime/kuberuntime_container.go

@@ -104,7 +104,7 @@ func (m *kubeGenericRuntimeManager) startContainer(podSandboxID string, podSandb
 		if handlerErr != nil {
 			err := fmt.Errorf("PostStart handler: %v", handlerErr)
 			m.generateContainerEvent(kubeContainerID, api.EventTypeWarning, events.FailedPostStartHook, msg)
-			m.killContainer(pod, kubeContainerID, container, "FailedPostStartHook", nil)
+			m.killContainer(pod, kubeContainerID, container.Name, "FailedPostStartHook", nil)
 			return "PostStart Hook Failed", err
 		}
 	}
@@ -429,24 +429,83 @@ func (m *kubeGenericRuntimeManager) executePreStopHook(pod *api.Pod, containerID
 	return int64(unversioned.Now().Sub(start.Time).Seconds())
 }
 
+// restoreSpecsFromContainerLabels restores all information needed for killing a container. In some
+// case we may not have pod and container spec when killing a container, e.g. pod is deleted during
+// kubelet restart.
+// To solve this problem, we've already written necessary information into container labels. Here we
+// just need to retrieve them from container labels and restore the specs.
+// TODO(random-liu): Add a node e2e test to test this behaviour.
+// TODO(random-liu): Change the lifecycle handler to just accept information needed, so that we can
+// just pass the needed function not create the fake object.
+func (m *kubeGenericRuntimeManager) restoreSpecsFromContainerLabels(containerID kubecontainer.ContainerID) (*api.Pod, *api.Container, error) {
+	var pod *api.Pod
+	var container *api.Container
+	s, err := m.runtimeService.ContainerStatus(containerID.ID)
+	if err != nil {
+		return nil, nil, err
+	}
+
+	l := getContainerInfoFromLabels(s.Labels)
+	a := getContainerInfoFromAnnotations(s.Annotations)
+	// Notice that the followings are not full spec. The container killing code should not use
+	// un-restored fields.
+	pod = &api.Pod{
+		ObjectMeta: api.ObjectMeta{
+			UID:                        l.PodUID,
+			Name:                       l.PodName,
+			Namespace:                  l.PodNamespace,
+			DeletionGracePeriodSeconds: a.PodDeletionGracePeriod,
+		},
+		Spec: api.PodSpec{
+			TerminationGracePeriodSeconds: a.PodTerminationGracePeriod,
+		},
+	}
+	container = &api.Container{
+		Name:  l.ContainerName,
+		Ports: a.ContainerPorts,
+		TerminationMessagePath: a.TerminationMessagePath,
+	}
+	if a.PreStopHandler != nil {
+		container.Lifecycle = &api.Lifecycle{
+			PreStop: a.PreStopHandler,
+		}
+	}
+	return pod, container, nil
+}
+
 // killContainer kills a container through the following steps:
 // * Run the pre-stop lifecycle hooks (if applicable).
 // * Stop the container.
-func (m *kubeGenericRuntimeManager) killContainer(pod *api.Pod, containerID kubecontainer.ContainerID, containerSpec *api.Container, reason string, gracePeriodOverride *int64) error {
+func (m *kubeGenericRuntimeManager) killContainer(pod *api.Pod, containerID kubecontainer.ContainerID, containerName string, reason string, gracePeriodOverride *int64) error {
-	gracePeriod := int64(minimumGracePeriodInSeconds)
+	var containerSpec *api.Container
 	if pod != nil {
+		for i, c := range pod.Spec.Containers {
+			if containerName == c.Name {
+				containerSpec = &pod.Spec.Containers[i]
+				break
+			}
+		}
+	} else {
+		// Restore necessary information if one of the specs is nil.
+		restoredPod, restoredContainer, err := m.restoreSpecsFromContainerLabels(containerID)
+		if err != nil {
+			return err
+		}
+		pod, containerSpec = restoredPod, restoredContainer
+	}
+	// From this point , pod and container must be non-nil.
+	gracePeriod := int64(minimumGracePeriodInSeconds)
 	switch {
 	case pod.DeletionGracePeriodSeconds != nil:
 		gracePeriod = *pod.DeletionGracePeriodSeconds
 	case pod.Spec.TerminationGracePeriodSeconds != nil:
 		gracePeriod = *pod.Spec.TerminationGracePeriodSeconds
 	}
-	}
 
 	glog.V(2).Infof("Killing container %q with %d second grace period", containerID.String(), gracePeriod)
 
 	// Run the pre-stop lifecycle hooks if applicable.
-	if pod != nil && containerSpec != nil && containerSpec.Lifecycle != nil && containerSpec.Lifecycle.PreStop != nil {
+	if containerSpec.Lifecycle != nil && containerSpec.Lifecycle.PreStop != nil {
 		gracePeriod = gracePeriod - m.executePreStopHook(pod, containerID, containerSpec, gracePeriod)
 	}
 	// always give containers a minimal shutdown window to avoid unnecessary SIGKILLs
@@ -485,19 +544,8 @@ func (m *kubeGenericRuntimeManager) killContainersWithSyncResult(pod *api.Pod, r
 		go func(container *kubecontainer.Container) {
 			defer utilruntime.HandleCrash()
 			defer wg.Done()
-
-			var containerSpec *api.Container
-			if pod != nil {
-				for i, c := range pod.Spec.Containers {
-					if container.Name == c.Name {
-						containerSpec = &pod.Spec.Containers[i]
-						break
-					}
-				}
-			}
-
 			killContainerResult := kubecontainer.NewSyncResult(kubecontainer.KillContainer, container.Name)
-			if err := m.killContainer(pod, container.ID, containerSpec, "Need to kill Pod", gracePeriodOverride); err != nil {
+			if err := m.killContainer(pod, container.ID, container.Name, "Need to kill Pod", gracePeriodOverride); err != nil {
 				killContainerResult.Fail(kubecontainer.ErrKillContainer, err.Error())
 			}
 			containerResults <- killContainerResult

pkg/kubelet/kuberuntime/kuberuntime_manager.go

@@ -499,7 +499,7 @@ func (m *kubeGenericRuntimeManager) SyncPod(pod *api.Pod, _ api.PodStatus, podSt
 			glog.V(3).Infof("Killing unwanted container %q(id=%q) for pod %q", containerInfo.name, containerID, format.Pod(pod))
 			killContainerResult := kubecontainer.NewSyncResult(kubecontainer.KillContainer, containerInfo.name)
 			result.AddSyncResult(killContainerResult)
-			if err := m.killContainer(pod, containerID, containerInfo.container, containerInfo.message, nil); err != nil {
+			if err := m.killContainer(pod, containerID, containerInfo.name, containerInfo.message, nil); err != nil {
 				killContainerResult.Fail(kubecontainer.ErrKillContainer, err.Error())
 				glog.Errorf("killContainer %q(id=%q) for pod %q failed: %v", containerInfo.name, containerID, format.Pod(pod), err)
 				return

pkg/kubelet/lifecycle/handlers.go

@@ -63,14 +63,14 @@ func (hr *HandlerRunner) Run(containerID kubecontainer.ContainerID, pod *api.Pod
 		output := ioutils.WriteCloserWrapper(&buffer)
 		err := hr.commandRunner.ExecInContainer(containerID, handler.Exec.Command, nil, output, output, false, nil)
 		if err != nil {
-			msg := fmt.Sprintf("Exec lifecycle hook (%v) for Container %q in Pod %q failed - %q", handler.Exec.Command, container.Name, format.Pod(pod), buffer.String())
+			msg := fmt.Sprintf("Exec lifecycle hook (%v) for Container %q in Pod %q failed - error: %v, message: %q", handler.Exec.Command, container.Name, format.Pod(pod), err, buffer.String())
 			glog.V(1).Infof(msg)
 		}
 		return msg, err
 	case handler.HTTPGet != nil:
 		msg, err := hr.runHTTPHandler(pod, container, handler)
 		if err != nil {
-			msg := fmt.Sprintf("Http lifecycle hook (%s) for Container %q in Pod %q failed - %q", handler.HTTPGet.Path, container.Name, format.Pod(pod), msg)
+			msg := fmt.Sprintf("Http lifecycle hook (%s) for Container %q in Pod %q failed - error: %v, message: %q", handler.HTTPGet.Path, container.Name, format.Pod(pod), err, msg)
 			glog.V(1).Infof(msg)
 		}
 		return msg, err

test/e2e/framework/pods.go

@@ -149,3 +149,19 @@ func (c *PodClient) mungeSpec(pod *api.Pod) {
 }
 
 // TODO(random-liu): Move pod wait function into this file
+// WaitForSuccess waits for pod to success.
+func (c *PodClient) WaitForSuccess(name string, timeout time.Duration) {
+	f := c.f
+	Expect(waitForPodCondition(f.Client, f.Namespace.Name, name, "success or failure", timeout,
+		func(pod *api.Pod) (bool, error) {
+			switch pod.Status.Phase {
+			case api.PodFailed:
+				return true, fmt.Errorf("pod %q failed with reason: %q, message: %q", name, pod.Status.Reason, pod.Status.Message)
+			case api.PodSucceeded:
+				return true, nil
+			default:
+				return false, nil
+			}
+		},
+	)).To(Succeed(), "wait for pod %q to success", name)
+}

test/e2e/framework/util.go

@@ -1455,6 +1455,7 @@ func waitForPodTerminatedInNamespace(c *client.Client, podName, reason, namespac
 func waitForPodSuccessInNamespaceTimeout(c *client.Client, podName string, contName string, namespace string, timeout time.Duration) error {
 	return waitForPodCondition(c, namespace, podName, "success or failure", timeout, func(pod *api.Pod) (bool, error) {
 		// Cannot use pod.Status.Phase == api.PodSucceeded/api.PodFailed due to #2632
+		// TODO: This was not true from long time ago. We can use api.PodSucceeded now.
 		ci, ok := api.GetContainerStatus(pod.Status.ContainerStatuses, contName)
 		if !ok {
 			Logf("No Status.Info for container '%s' in pod '%s' yet", contName, podName)

test/e2e_node/lifecycle_hook_test.go

@@ -0,0 +1,182 @@
+/*
+Copyright 2016 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package e2e_node
+
+import (
+	"fmt"
+	"time"
+
+	"k8s.io/kubernetes/pkg/api"
+	"k8s.io/kubernetes/pkg/util/intstr"
+	"k8s.io/kubernetes/pkg/util/uuid"
+	"k8s.io/kubernetes/test/e2e/framework"
+
+	. "github.com/onsi/ginkgo"
+)
+
+var _ = framework.KubeDescribe("Container Lifecycle Hook", func() {
+	f := framework.NewDefaultFramework("container-lifecycle-hook")
+	var podClient *framework.PodClient
+	var file string
+	const podWaitTimeout = 2 * time.Minute
+
+	testPodWithHook := func(podWithHook *api.Pod) {
+		podCheckHook := getLifecycleHookTestPod("pod-check-hook",
+			// Wait until the file is created.
+			[]string{"sh", "-c", fmt.Sprintf("while [ ! -e %s ]; do sleep 1; done", file)},
+		)
+		By("create the pod with lifecycle hook")
+		podClient.CreateSync(podWithHook)
+		if podWithHook.Spec.Containers[0].Lifecycle.PostStart != nil {
+			By("create the hook check pod")
+			podClient.Create(podCheckHook)
+			By("wait for the hook check pod to success")
+			podClient.WaitForSuccess(podCheckHook.Name, podWaitTimeout)
+		}
+		By("delete the pod with lifecycle hook")
+		podClient.DeleteSync(podWithHook.Name, api.NewDeleteOptions(15), podWaitTimeout)
+		if podWithHook.Spec.Containers[0].Lifecycle.PreStop != nil {
+			By("create the hook check pod")
+			podClient.Create(podCheckHook)
+			By("wait for the prestop check pod to success")
+			podClient.WaitForSuccess(podCheckHook.Name, podWaitTimeout)
+		}
+	}
+
+	Context("when create a pod with lifecycle hook", func() {
+		BeforeEach(func() {
+			podClient = f.PodClient()
+			file = "/tmp/test-" + string(uuid.NewUUID())
+		})
+
+		AfterEach(func() {
+			By("cleanup the temporary file created in the test.")
+			cleanupPod := getLifecycleHookTestPod("pod-clean-up", []string{"rm", file})
+			podClient.Create(cleanupPod)
+			podClient.WaitForSuccess(cleanupPod.Name, podWaitTimeout)
+		})
+
+		Context("when it is exec hook", func() {
+			It("should execute poststart exec hook properly", func() {
+				podWithHook := getLifecycleHookTestPod("pod-with-poststart-exec-hook",
+					// Block forever
+					[]string{"tail", "-f", "/dev/null"},
+				)
+				podWithHook.Spec.Containers[0].Lifecycle = &api.Lifecycle{
+					PostStart: &api.Handler{
+						Exec: &api.ExecAction{Command: []string{"touch", file}},
+					},
+				}
+				testPodWithHook(podWithHook)
+			})
+
+			It("should execute prestop exec hook properly", func() {
+				podWithHook := getLifecycleHookTestPod("pod-with-prestop-exec-hook",
+					// Block forever
+					[]string{"tail", "-f", "/dev/null"},
+				)
+				podWithHook.Spec.Containers[0].Lifecycle = &api.Lifecycle{
+					PreStop: &api.Handler{
+						Exec: &api.ExecAction{Command: []string{"touch", file}},
+					},
+				}
+				testPodWithHook(podWithHook)
+			})
+		})
+
+		Context("when it is http hook", func() {
+			var targetIP string
+			BeforeEach(func() {
+				By("cleanup the container to handle the HTTPGet hook request.")
+				podHandleHookRequest := getLifecycleHookTestPod("pod-handle-http-request",
+					[]string{"sh", "-c",
+						// Create test file when receive request on 1234.
+						fmt.Sprintf("echo -e \"HTTP/1.1 200 OK\n\" | nc -l -p 1234; touch %s", file),
+					},
+				)
+				podHandleHookRequest.Spec.Containers[0].Ports = []api.ContainerPort{
+					{
+						ContainerPort: 1234,
+						Protocol:      api.ProtocolTCP,
+					},
+				}
+				podHandleHookRequest = podClient.CreateSync(podHandleHookRequest)
+				targetIP = podHandleHookRequest.Status.PodIP
+			})
+			It("should execute poststart http hook properly", func() {
+				podWithHook := getLifecycleHookTestPod("pod-with-poststart-http-hook",
+					// Block forever
+					[]string{"tail", "-f", "/dev/null"},
+				)
+				podWithHook.Spec.Containers[0].Lifecycle = &api.Lifecycle{
+					PostStart: &api.Handler{
+						HTTPGet: &api.HTTPGetAction{
+							Host: targetIP,
+							Port: intstr.FromInt(1234),
+						},
+					},
+				}
+				testPodWithHook(podWithHook)
+			})
+			It("should execute prestop http hook properly", func() {
+				podWithHook := getLifecycleHookTestPod("pod-with-prestop-http-hook",
+					// Block forever
+					[]string{"tail", "-f", "/dev/null"},
+				)
+				podWithHook.Spec.Containers[0].Lifecycle = &api.Lifecycle{
+					PreStop: &api.Handler{
+						HTTPGet: &api.HTTPGetAction{
+							Host: targetIP,
+							Port: intstr.FromInt(1234),
+						},
+					},
+				}
+				testPodWithHook(podWithHook)
+			})
+		})
+	})
+})
+
+func getLifecycleHookTestPod(name string, cmd []string) *api.Pod {
+	return &api.Pod{
+		ObjectMeta: api.ObjectMeta{
+			Name: name,
+		},
+		Spec: api.PodSpec{
+			Containers: []api.Container{
+				{
+					Name:  name,
+					Image: "gcr.io/google_containers/busybox:1.24",
+					VolumeMounts: []api.VolumeMount{
+						{
+							Name:      "tmpfs",
+							MountPath: "/tmp",
+						},
+					},
+					Command: cmd,
+				},
+			},
+			RestartPolicy: api.RestartPolicyNever,
+			Volumes: []api.Volume{
+				{
+					Name:         "tmpfs",
+					VolumeSource: api.VolumeSource{HostPath: &api.HostPathVolumeSource{Path: "/tmp"}},
+				},
+			},
+		},
+	}
+}