fix(executor): Handle sidecar killing in a process-namespace-shared p…

…od (argoproj#4575)

Signed-off-by: Daisuke Taniwaki <[email protected]>

cmd/argoexec/commands/wait.go

@@ -24,15 +24,15 @@ func NewWaitCommand() *cobra.Command {
 
 func waitContainer() error {
  wfExecutor := initExecutor()
- defer wfExecutor.HandleError()
+ defer wfExecutor.HandleError() // Must be placed at the bottom of defers stack.
  defer stats.LogStats()
  stats.StartStatsTicker(5 * time.Minute)
 
  defer func() {
  // Killing sidecar containers
  err := wfExecutor.KillSidecars()
  if err != nil {
- log.Errorf("Failed to kill sidecars: %s", err.Error())
+ wfExecutor.AddError(err)
  }
  }()
 

workflow/executor/common/common.go

@@ -80,6 +80,9 @@ func TerminatePodWithContainerID(c KubernetesClientInterface, containerID string
  log.Infof("Container %s is already terminated: %v", container.ContainerID, container.State.Terminated.String())
  return nil
  }
+ if pod.Spec.ShareProcessNamespace != nil && *pod.Spec.ShareProcessNamespace {
+ return fmt.Errorf("cannot terminate a process-namespace-shared Pod %s", pod.Name)
+ }
  if pod.Spec.HostPID {
  return fmt.Errorf("cannot terminate a hostPID Pod %s", pod.Name)
  }

workflow/executor/common/common_test.go

@@ -0,0 +1,121 @@
+package common
+
+import (
+ "bytes"
+ "syscall"
+ "testing"
+
+ "github.com/stretchr/testify/assert"
+ v1 "k8s.io/api/core/v1"
+ metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+ "k8s.io/utils/pointer"
+)
+
+type MockKC struct {
+ getContainerStatusPod *v1.Pod
+ getContainerStatusContainerStatus *v1.ContainerStatus
+ getContainerStatusErr error
+ killContainerError error
+}
+
+func (m *MockKC) GetContainerStatus(containerID string) (*v1.Pod, *v1.ContainerStatus, error) {
+ return m.getContainerStatusPod, m.getContainerStatusContainerStatus, m.getContainerStatusErr
+}
+
+func (m *MockKC) KillContainer(pod *v1.Pod, container *v1.ContainerStatus, sig syscall.Signal) error {
+ return m.killContainerError
+}
+
+func (*MockKC) CreateArchive(containerID, sourcePath string) (*bytes.Buffer, error) {
+ return nil, nil
+}
+
+// TestScriptTemplateWithVolume ensure we can a script pod with input artifacts
+func TestTerminatePodWithContainerID(t *testing.T) {
+ // Already terminated.
+ mock := &MockKC{
+ getContainerStatusContainerStatus: &v1.ContainerStatus{
+ State: v1.ContainerState{
+ Terminated: &v1.ContainerStateTerminated{},
+ },
+ },
+ }
+ err := TerminatePodWithContainerID(mock, "container-id", syscall.SIGTERM)
+ assert.NoError(t, err)
+
+ // w/ ShareProcessNamespace.
+ mock = &MockKC{
+ getContainerStatusPod: &v1.Pod{
+ ObjectMeta: metav1.ObjectMeta{
+ Name: "foo",
+ },
+ Spec: v1.PodSpec{
+ ShareProcessNamespace: pointer.BoolPtr(true),
+ },
+ },
+ getContainerStatusContainerStatus: &v1.ContainerStatus{
+ State: v1.ContainerState{
+ Terminated: nil,
+ },
+ },
+ }
+ err = TerminatePodWithContainerID(mock, "container-id", syscall.SIGTERM)
+ assert.EqualError(t, err, "cannot terminate a process-namespace-shared Pod foo")
+
+ // w/ HostPID.
+ mock = &MockKC{
+ getContainerStatusPod: &v1.Pod{
+ ObjectMeta: metav1.ObjectMeta{
+ Name: "foo",
+ },
+ Spec: v1.PodSpec{
+ HostPID: true,
+ },
+ },
+ getContainerStatusContainerStatus: &v1.ContainerStatus{
+ State: v1.ContainerState{
+ Terminated: nil,
+ },
+ },
+ }
+ err = TerminatePodWithContainerID(mock, "container-id", syscall.SIGTERM)
+ assert.EqualError(t, err, "cannot terminate a hostPID Pod foo")
+
+ // w/ RestartPolicy.
+ mock = &MockKC{
+ getContainerStatusPod: &v1.Pod{
+ ObjectMeta: metav1.ObjectMeta{
+ Name: "foo",
+ },
+ Spec: v1.PodSpec{
+ RestartPolicy: "Always",
+ },
+ },
+ getContainerStatusContainerStatus: &v1.ContainerStatus{
+ State: v1.ContainerState{
+ Terminated: nil,
+ },
+ },
+ }
+ err = TerminatePodWithContainerID(mock, "container-id", syscall.SIGTERM)
+ assert.EqualError(t, err, "cannot terminate pod with a \"Always\" restart policy")
+
+ // Successfully call KillContainer of the client interface.
+ mock = &MockKC{
+ getContainerStatusPod: &v1.Pod{
+ ObjectMeta: metav1.ObjectMeta{
+ Name: "foo",
+ },
+ Spec: v1.PodSpec{
+ RestartPolicy: "Never",
+ },
+ },
+ getContainerStatusContainerStatus: &v1.ContainerStatus{
+ State: v1.ContainerState{
+ Terminated: nil,
+ },
+ },
+ }
+ err = TerminatePodWithContainerID(mock, "container-id", syscall.SIGTERM)
+ assert.NoError(t, err)
+}