From 4361b9f39a81a6bdd121973bd9191700683987dd Mon Sep 17 00:00:00 2001
From: Philippe Martin <phmartin@redhat.com>
Date: Wed, 2 Aug 2023 14:44:04 +0200
Subject: [PATCH] Makes stopped command terminate normally (#7011)

---
 pkg/remotecmd/kubeexec.go      | 13 ++++++-------
 pkg/remotecmd/kubeexec_test.go | 16 ++++++++++++----
 2 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/pkg/remotecmd/kubeexec.go b/pkg/remotecmd/kubeexec.go
index 926f21e2e7f..5730c297f49 100644
--- a/pkg/remotecmd/kubeexec.go
+++ b/pkg/remotecmd/kubeexec.go
@@ -145,13 +145,6 @@ func (k *kubeExecProcessHandler) StartProcessForCommand(ctx context.Context, def
 // Then killing those children will exit the parent 'sh' process.
 func (k *kubeExecProcessHandler) StopProcessForCommand(ctx context.Context, def CommandDefinition, podName string, containerName string) error {
 	klog.V(4).Infof("StopProcessForCommand for %q", def.Id)
-	defer func() {
-		pidFile := getPidFileForCommand(def)
-		_, _, err := k.execClient.ExecuteCommand(ctx, []string{ShellExecutable, "-c", fmt.Sprintf("rm -f %s", pidFile)}, podName, containerName, false, nil, nil)
-		if err != nil {
-			klog.V(2).Infof("Could not remove file %q: %v", pidFile, err)
-		}
-	}()
 
 	kill := func(p int) error {
 		_, _, err := k.execClient.ExecuteCommand(ctx, []string{ShellExecutable, "-c", fmt.Sprintf("kill %d || true", p)}, podName, containerName, false, nil, nil)
@@ -201,6 +194,12 @@ func (k *kubeExecProcessHandler) StopProcessForCommand(ctx context.Context, def
 
 	klog.V(3).Infof("Found %d children (either direct and indirect) for parent process %d: %v", len(children), ppid, children)
 
+	pidFile := getPidFileForCommand(def)
+	_, _, err = k.execClient.ExecuteCommand(ctx, []string{ShellExecutable, "-c", fmt.Sprintf("rm -f %s", pidFile)}, podName, containerName, false, nil, nil)
+	if err != nil {
+		klog.V(2).Infof("Could not remove file %q: %v", pidFile, err)
+	}
+
 	for _, child := range children {
 		if err = kill(child); err != nil {
 			return err
diff --git a/pkg/remotecmd/kubeexec_test.go b/pkg/remotecmd/kubeexec_test.go
index e1497861de9..3aea97db7ac 100644
--- a/pkg/remotecmd/kubeexec_test.go
+++ b/pkg/remotecmd/kubeexec_test.go
@@ -368,6 +368,10 @@ func Test_kubeExecProcessHandler_StopProcessForCommand(t *testing.T) {
 		{
 			name: "no process children killed if no children file found",
 			kubeClientCustomizer: func(kclient *kclient.MockClientInterface) {
+				kclient.EXPECT().ExecCMDInContainer(gomock.Any(), gomock.Eq(_containerName), gomock.Eq(_podName),
+					gomock.Eq([]string{ShellExecutable, "-c", fmt.Sprintf("rm -f %s", getPidFileForCommand(cmdDef))}),
+					gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
+					Return(errors.New("an error which should be ignored"))
 				kclient.EXPECT().ExecCMDInContainer(gomock.Any(), gomock.Eq(_containerName), gomock.Eq(_podName),
 					gomock.Eq([]string{ShellExecutable, "-c", fmt.Sprintf("cat %s || true", getPidFileForCommand(cmdDef))}),
 					gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
@@ -396,6 +400,10 @@ func Test_kubeExecProcessHandler_StopProcessForCommand(t *testing.T) {
 		{
 			name: "process children should get killed",
 			kubeClientCustomizer: func(kclient *kclient.MockClientInterface) {
+				kclient.EXPECT().ExecCMDInContainer(gomock.Any(), gomock.Eq(_containerName), gomock.Eq(_podName),
+					gomock.Eq([]string{ShellExecutable, "-c", fmt.Sprintf("rm -f %s", getPidFileForCommand(cmdDef))}),
+					gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
+					Return(errors.New("an error which should be ignored"))
 				kclient.EXPECT().ExecCMDInContainer(gomock.Any(), gomock.Eq(_containerName), gomock.Eq(_podName),
 					gomock.Eq([]string{ShellExecutable, "-c", fmt.Sprintf("cat %s || true", getPidFileForCommand(cmdDef))}),
 					gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
@@ -426,6 +434,10 @@ func Test_kubeExecProcessHandler_StopProcessForCommand(t *testing.T) {
 		{
 			name: "error if any child process could not be killed",
 			kubeClientCustomizer: func(kclient *kclient.MockClientInterface) {
+				kclient.EXPECT().ExecCMDInContainer(gomock.Any(), gomock.Eq(_containerName), gomock.Eq(_podName),
+					gomock.Eq([]string{ShellExecutable, "-c", fmt.Sprintf("rm -f %s", getPidFileForCommand(cmdDef))}),
+					gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
+					Return(errors.New("an error which should be ignored"))
 				kclient.EXPECT().ExecCMDInContainer(gomock.Any(), gomock.Eq(_containerName), gomock.Eq(_podName),
 					gomock.Eq([]string{ShellExecutable, "-c", fmt.Sprintf("cat %s || true", getPidFileForCommand(cmdDef))}),
 					gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
@@ -460,10 +472,6 @@ func Test_kubeExecProcessHandler_StopProcessForCommand(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			ctrl := gomock.NewController(t)
 			kubeClient := kclient.NewMockClientInterface(ctrl)
-			kubeClient.EXPECT().ExecCMDInContainer(gomock.Any(), gomock.Eq(_containerName), gomock.Eq(_podName),
-				gomock.Eq([]string{ShellExecutable, "-c", fmt.Sprintf("rm -f %s", getPidFileForCommand(cmdDef))}),
-				gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
-				Return(errors.New("an error which should be ignored"))
 			if tt.kubeClientCustomizer != nil {
 				tt.kubeClientCustomizer(kubeClient)
 			}