Merge pull request #56959 from lichuqiang/lifecycleHandlerFix

Automatic merge from submit-queue (batch tested with PRs 56599, 56824, 56918, 56967, 56959). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Fix bug in container lifecycle event message generation

**What this PR does / why we need it**:
In HandlerRunner of container lifecycle, the event msg is re-declared. Thus, the event message we returned would always be empty.

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #56962

**Special notes for your reviewer**:
/sig node
**Release note**:

```release-note
Fix bug in container lifecycle event messaging
```

pkg/kubelet/lifecycle/BUILD

@@ -39,6 +39,7 @@ go_test(
     library = ":go_default_library",
     deps = [
         "//pkg/kubelet/container:go_default_library",
+        "//pkg/kubelet/util/format:go_default_library",
         "//vendor/k8s.io/api/core/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/intstr:go_default_library",
     ],

pkg/kubelet/lifecycle/handlers.go

@@ -58,14 +58,14 @@ func (hr *HandlerRunner) Run(containerID kubecontainer.ContainerID, pod *v1.Pod,
 		// TODO(tallclair): Pass a proper timeout value.
 		output, err := hr.commandRunner.RunInContainer(containerID, handler.Exec.Command, 0)
 		if err != nil {
-			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, string(output))
+			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, string(output))
 			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 - error: %v, message: %q", handler.HTTPGet.Path, container.Name, format.Pod(pod), err, 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

pkg/kubelet/lifecycle/handlers_test.go

@@ -28,6 +28,7 @@ import (
 	"k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	kubecontainer "k8s.io/kubernetes/pkg/kubelet/container"
+	"k8s.io/kubernetes/pkg/kubelet/util/format"
 )
 
 func TestResolvePortInt(t *testing.T) {
@@ -78,12 +79,14 @@ func TestResolvePortStringUnknown(t *testing.T) {
 type fakeContainerCommandRunner struct {
 	Cmd []string
 	ID  kubecontainer.ContainerID
+	Err error
+	Msg string
 }
 
 func (f *fakeContainerCommandRunner) RunInContainer(id kubecontainer.ContainerID, cmd []string, timeout time.Duration) ([]byte, error) {
 	f.Cmd = cmd
 	f.ID = id
-	return nil, nil
+	return []byte(f.Msg), f.Err
 }
 
 func TestRunHandlerExec(t *testing.T) {
@@ -185,6 +188,40 @@ func TestRunHandlerNil(t *testing.T) {
 	}
 }
 
+func TestRunHandlerExecFailure(t *testing.T) {
+	expectedErr := fmt.Errorf("invalid command")
+	fakeCommandRunner := fakeContainerCommandRunner{Err: expectedErr, Msg: expectedErr.Error()}
+	handlerRunner := NewHandlerRunner(&fakeHTTP{}, &fakeCommandRunner, nil)
+
+	containerID := kubecontainer.ContainerID{Type: "test", ID: "abc1234"}
+	containerName := "containerFoo"
+	command := []string{"ls", "--a"}
+
+	container := v1.Container{
+		Name: containerName,
+		Lifecycle: &v1.Lifecycle{
+			PostStart: &v1.Handler{
+				Exec: &v1.ExecAction{
+					Command: command,
+				},
+			},
+		},
+	}
+
+	pod := v1.Pod{}
+	pod.ObjectMeta.Name = "podFoo"
+	pod.ObjectMeta.Namespace = "nsFoo"
+	pod.Spec.Containers = []v1.Container{container}
+	expectedErrMsg := fmt.Sprintf("Exec lifecycle hook (%s) for Container %q in Pod %q failed - error: %v, message: %q", command, containerName, format.Pod(&pod), expectedErr, expectedErr.Error())
+	msg, err := handlerRunner.Run(containerID, &pod, &container, container.Lifecycle.PostStart)
+	if err == nil {
+		t.Errorf("expected error: %v", expectedErr)
+	}
+	if msg != expectedErrMsg {
+		t.Errorf("unexpected error message: %q; expected %q", msg, expectedErrMsg)
+	}
+}
+
 func TestRunHandlerHttpFailure(t *testing.T) {
 	expectedErr := fmt.Errorf("fake http error")
 	expectedResp := http.Response{
@@ -210,12 +247,13 @@ func TestRunHandlerHttpFailure(t *testing.T) {
 	pod.ObjectMeta.Name = "podFoo"
 	pod.ObjectMeta.Namespace = "nsFoo"
 	pod.Spec.Containers = []v1.Container{container}
+	expectedErrMsg := fmt.Sprintf("Http lifecycle hook (%s) for Container %q in Pod %q failed - error: %v, message: %q", "bar", containerName, format.Pod(&pod), expectedErr, expectedErr.Error())
 	msg, err := handlerRunner.Run(containerID, &pod, &container, container.Lifecycle.PostStart)
 	if err == nil {
 		t.Errorf("expected error: %v", expectedErr)
 	}
-	if msg != expectedErr.Error() {
+	if msg != expectedErrMsg {
-		t.Errorf("unexpected error message: %q; expected %q", msg, expectedErr)
+		t.Errorf("unexpected error message: %q; expected %q", msg, expectedErrMsg)
 	}
 	if fakeHttp.url != "http://foo:8080/bar" {
 		t.Errorf("unexpected url: %s", fakeHttp.url)