fix: Correct kill command. Fixes #8687 (#8908)

* fix: Correct kill command. Fixes #8687 Signed-off-by: Alex Collins <[email protected]> * fix: ok Signed-off-by: Alex Collins <[email protected]> * fix: ok Signed-off-by: Alex Collins <[email protected]>
argoproj · Jun 13, 2022 · e31ffcd · e31ffcd
1 parent ba6e190
commit e31ffcd
Show file tree

Hide file tree

Showing 7 changed files with 63 additions and 46 deletions.
diff --git a/cmd/argoexec/commands/kill.go b/cmd/argoexec/commands/kill.go
@@ -0,0 +1,37 @@
+package commands
+
+import (
+	"fmt"
+	"os"
+	"strconv"
+	"syscall"
+
+	"github.com/spf13/cobra"
+)
+
+func NewKillCommand() *cobra.Command {
+	return &cobra.Command{
+		Use:          "kill SIGNAL PID",
+		SilenceUsage: true,
+		RunE: func(cmd *cobra.Command, args []string) error {
+			signum, err := strconv.Atoi(args[0])
+			if err != nil {
+				return err
+			}
+			pid, err := strconv.Atoi(args[1])
+			if err != nil {
+				return err
+			}
+			sig := syscall.Signal(signum)
+			p, err := os.FindProcess(pid)
+			if err != nil {
+				return err
+			}
+			fmt.Printf("killing %d with %v\n", pid, sig)
+			if err := p.Signal(sig); err != nil {
+				return err
+			}
+			return nil
+		},
+	}
+}
diff --git a/cmd/argoexec/commands/root.go b/cmd/argoexec/commands/root.go
@@ -61,6 +61,7 @@ func NewRootCommand() *cobra.Command {
 	command.AddCommand(NewAgentCommand())
 	command.AddCommand(NewEmissaryCommand())
 	command.AddCommand(NewInitCommand())
+	command.AddCommand(NewKillCommand())
 	command.AddCommand(NewResourceCommand())
 	command.AddCommand(NewWaitCommand())
 	command.AddCommand(NewDataCommand())

diff --git a/cmd/argoexec/commands/wait.go b/cmd/argoexec/commands/wait.go
@@ -2,6 +2,8 @@ package commands
 
 import (
 	"context"
+	"os/signal"
+	"syscall"
 	"time"
 
 	"github.com/argoproj/pkg/stats"
@@ -30,13 +32,20 @@ func waitContainer(ctx context.Context) error {
 	defer stats.LogStats()
 	stats.StartStatsTicker(5 * time.Minute)
 
-	// Wait for main container to complete
-	err := wfExecutor.Wait(ctx)
-	if err != nil {
-		wfExecutor.AddError(err)
+	// use a block to constrain the scope of ctx
+	{
+		// this allows us to gracefully shutdown, capturing artifacts
+		ctx, cancel := signal.NotifyContext(ctx, syscall.SIGTERM)
+		defer cancel()
+
+		// Wait for main container to complete
+		err := wfExecutor.Wait(ctx)
+		if err != nil {
+			wfExecutor.AddError(err)
+		}
 	}
 	// Capture output script result
-	err = wfExecutor.CaptureScriptResult(ctx)
+	err := wfExecutor.CaptureScriptResult(ctx)
 	if err != nil {
 		wfExecutor.AddError(err)
 	}

diff --git a/test/e2e/signals_test.go b/test/e2e/signals_test.go
@@ -128,14 +128,6 @@ func (s *SignalsSuite) TestInjectedSidecar() {
 		WaitForWorkflow(fixtures.ToBeSucceeded, kill2xDuration)
 }
 
-func (s *SignalsSuite) TestInjectedSidecarKillAnnotation() {
-	s.Given().
-		Workflow("@testdata/sidecar-injected-kill-annotation-workflow.yaml").
-		When().
-		SubmitWorkflow().
-		WaitForWorkflow(fixtures.ToBeSucceeded, kill2xDuration)
-}
-
 func (s *SignalsSuite) TestSubProcess() {
 	s.Given().
 		Workflow("@testdata/subprocess-workflow.yaml").

diff --git a/test/e2e/testdata/sidecar-injected-kill-annotation-workflow.yaml b/test/e2e/testdata/sidecar-injected-kill-annotation-workflow.yaml
diff --git a/workflow/executor/executor.go b/workflow/executor/executor.go
@@ -13,13 +13,11 @@ import (
 	"io/fs"
 	"io/ioutil"
 	"os"
-	"os/signal"
 	"path"
 	"path/filepath"
 	"runtime/debug"
 	"strconv"
 	"strings"
-	"syscall"
 	"time"
 
 	"github.com/argoproj/argo-workflows/v3/util/file"
@@ -1022,10 +1020,6 @@ func (we *WorkflowExecutor) Wait(ctx context.Context) error {
 		log.WithField("annotationPatchTickDuration", we.annotationPatchTickDuration).WithField("readProgressFileTickDuration", we.readProgressFileTickDuration).Info("monitoring progress disabled")
 	}
 
-	// this allows us to gracefully shutdown, capturing artifacts
-	ctx, cancel := signal.NotifyContext(ctx, syscall.SIGTERM)
-	defer cancel()
-
 	go we.monitorDeadline(ctx, containerNames)
 
 	err := retryutil.OnError(executorretry.ExecutorRetry, errorsutil.IsTransientErr, func() error {

diff --git a/workflow/signal/signal.go b/workflow/signal/signal.go
@@ -3,6 +3,7 @@ package signal
 import (
 	"encoding/json"
 	"fmt"
+	"path/filepath"
 	"strings"
 	"syscall"
 
@@ -15,8 +16,16 @@ import (
 
 func SignalContainer(restConfig *rest.Config, pod *corev1.Pod, container string, s syscall.Signal) error {
 	command := []string{"/bin/sh", "-c", "kill -%d 1"}
-	if container == common.WaitContainerName {
-		command = []string{"/bin/sh", "-c", "kill -%d $(pidof argoexec)"}
+
+	// If the container has the /var/run/argo volume mounted, this it will have access to `argoexec`.
+	for _, c := range pod.Spec.Containers {
+		if c.Name == container {
+			for _, m := range c.VolumeMounts {
+				if m.MountPath == common.VarRunArgoPath {
+					command = []string{filepath.Join(common.VarRunArgoPath, "argoexec"), "kill", "%d", "1"}
+				}
+			}
+		}
 	}
 
 	if v, ok := pod.Annotations[common.AnnotationKeyKillCmd(container)]; ok {