Repository: argoproj/argo (go)

Issue:
[Bug] pns executor cannot retrieve correct containerId with runtime cri-containerd
## Summary 

What happened/what you expected to happen?

When I use pns executor with containerd runtime, `argo stop` couldn't stop workflow pod successfully. The root cause is that containerd cgroup structure is different from docker, and pns executor failed to parse container id from /proc/{pid}/cgroup file.

## Diagnostics

What Kubernetes provider are you using? 

bare metal v1.13.12 with containerd at v1.3.6

What version of Argo Workflows are you running? 

v2.11.3

/proc/{pid}/cgroup in docker container
```
root@workflow-4fd9de820af248518e1a168fc79e29c6:/# ps -ef
UID        PID  PPID  C STIME TTY          TIME CMD
root         1     0  0 09:22 ?        00:00:00 /pause
root         6     0  0 09:22 ?        00:00:00 argoexec wait
root        19     0  0 09:22 ?        00:00:00 /bin/sh -c sleep 86400
root        24    19  0 09:22 ?        00:00:00 sleep 86400
root        27     0  0 09:22 pts/0    00:00:00 bash
root        36    27  0 09:23 pts/0    00:00:00 ps -ef
root@workflow-4fd9de820af248518e1a168fc79e29c6:/# 
root@workflow-4fd9de820af248518e1a168fc79e29c6:/# cat /proc/19/cgroup 
11:blkio:/kubepods/besteffort/pod11f05807-0f91-11eb-b66a-023a093e2aa4/1b3872262e6e9ad77b5fabe3139023d10c58f370499055765f39b908d07ec679
10:cpuset:/kubepods/besteffort/pod11f05807-0f91-11eb-b66a-023a093e2aa4/1b3872262e6e9ad77b5fabe3139023d10c58f370499055765f39b908d07ec679
9:freezer:/kubepods/besteffort/pod11f05807-0f91-11eb-b66a-023a093e2aa4/1b3872262e6e9ad77b5fabe3139023d10c58f370499055765f39b908d07ec679
8:memory:/kubepods/besteffort/pod11f05807-0f91-11eb-b66a-023a093e2aa4/1b3872262e6e9ad77b5fabe3139023d10c58f370499055765f39b908d07ec679
7:pids:/kubepods/besteffort/pod11f05807-0f91-11eb-b66a-023a093e2aa4/1b3872262e6e9ad77b5fabe3139023d10c58f370499055765f39b908d07ec679
6:devices:/kubepods/besteffort/pod11f05807-0f91-11eb-b66a-023a093e2aa4/1b3872262e6e9ad77b5fabe3139023d10c58f370499055765f39b908d07ec679
5:net_cls,net_prio:/kubepods/besteffort/pod11f05807-0f91-11eb-b66a-023a093e2aa4/1b3872262e6e9ad77b5fabe3139023d10c58f370499055765f39b908d07ec679
4:hugetlb:/kubepods/besteffort/pod11f05807-0f91-11eb-b66a-023a093e2aa4/1b3872262e6e9ad77b5fabe3139023d10c58f370499055765f39b908d07ec679
3:cpu,cpuacct:/kubepods/besteffort/pod11f05807-0f91-11eb-b66a-023a093e2aa4/1b3872262e6e9ad77b5fabe3139023d10c58f370499055765f39b908d07ec679
2:perf_event:/kubepods/besteffort/pod11f05807-0f91-11eb-b66a-023a093e2aa4/1b3872262e6e9ad77b5fabe3139023d10c58f370499055765f39b908d07ec679
1:name=systemd:/kubepods/besteffort/pod11f05807-0f91-11eb-b66a-023a093e2aa4/1b3872262e6e9ad77b5fabe3139023d10c58f370499055765f39b908d07ec679
```

/proc/{pid}/cgroup in containerd
```
root@workflow-c1b7ee13dda64548820efb49f14d64e2:/# ps -ef
UID        PID  PPID  C STIME TTY          TIME CMD
root         1     0  0 09:30 ?        00:00:00 /pause
root         6     0  0 09:30 ?        00:00:00 argoexec wait
root        30     0  0 09:30 ?        00:00:00 /bin/sh -c sleep 86400
root        35    30  0 09:30 ?        00:00:00 sleep 86400
root        37     0  0 09:30 pts/0    00:00:00 bash
root        42    37  0 09:30 pts/0    00:00:00 ps -ef
root@workflow-c1b7ee13dda64548820efb49f14d64e2:/# 
root@workflow-c1b7ee13dda64548820efb49f14d64e2:/# cat /proc/30/cgroup 
11:blkio:/system.slice/containerd.service/kubepods-besteffort-pod30556cce_0f92_11eb_b36d_02623cf324c8.slice:cri-containerd:c688c856b21cfb29c1dbf6c14793435e44a1299dfc12add33283239bffed2620
10:memory:/system.slice/containerd.service/kubepods-besteffort-pod30556cce_0f92_11eb_b36d_02623cf324c8.slice:cri-containerd:c688c856b21cfb29c1dbf6c14793435e44a1299dfc12add33283239bffed2620
9:cpuset:/kubepods-besteffort-pod30556cce_0f92_11eb_b36d_02623cf324c8.slice:cri-containerd:c688c856b21cfb29c1dbf6c14793435e44a1299dfc12add33283239bffed2620
8:freezer:/kubepods-besteffort-pod30556cce_0f92_11eb_b36d_02623cf324c8.slice:cri-containerd:c688c856b21cfb29c1dbf6c14793435e44a1299dfc12add33283239bffed2620
7:net_cls,net_prio:/kubepods-besteffort-pod30556cce_0f92_11eb_b36d_02623cf324c8.slice:cri-containerd:c688c856b21cfb29c1dbf6c14793435e44a1299dfc12add33283239bffed2620
6:hugetlb:/kubepods-besteffort-pod30556cce_0f92_11eb_b36d_02623cf324c8.slice:cri-containerd:c688c856b21cfb29c1dbf6c14793435e44a1299dfc12add33283239bffed2620
5:devices:/system.slice/containerd.service/kubepods-besteffort-pod30556cce_0f92_11eb_b36d_02623cf324c8.slice:cri-containerd:c688c856b21cfb29c1dbf6c14793435e44a1299dfc12add33283239bffed2620
4:perf_event:/kubepods-besteffort-pod30556cce_0f92_11eb_b36d_02623cf324c8.slice:cri-containerd:c688c856b21cfb29c1dbf6c14793435e44a1299dfc12add33283239bffed2620
3:pids:/system.slice/containerd.service/kubepods-besteffort-pod30556cce_0f92_11eb_b36d_02623cf324c8.slice:cri-containerd:c688c856b21cfb29c1dbf6c14793435e44a1299dfc12add33283239bffed2620
2:cpu,cpuacct:/system.slice/containerd.service/kubepods-besteffort-pod30556cce_0f92_11eb_b36d_02623cf324c8.slice:cri-containerd:c688c856b21cfb29c1dbf6c14793435e44a1299dfc12add33283239bffed2620
1:name=systemd:/system.slice/containerd.service/kubepods-besteffort-pod30556cce_0f92_11eb_b36d_02623cf324c8.slice:cri-containerd:c688c856b21cfb29c1dbf6c14793435e44a1299dfc12add33283239bffed2620
```

```
Paste the logs from the pns executor:

root@vai-adsimulator-k8s-autoscaling-stage-master1:/home/CORP/chenyu.zheng# kubectl logs workflow-9fff344428454a82b821ccf0b5b2090e wait -n workflow-test

time="2020-10-16T06:21:28.653Z" level=info msg="Starting Workflow Executor" version=v2.11.3
time="2020-10-16T06:21:28.656Z" level=info msg="Creating PNS executor (namespace: workflow-test, pod: workflow-9fff344428454a82b821ccf0b5b2090e, pid: 6, hasOutputs: false)"
time="2020-10-16T06:21:28.656Z" level=info msg="Executor (version: v2.11.3, build_date: 2020-10-07T22:55:41Z) initialized (pod: workflow-test/workflow-9fff344428454a82b821ccf0b5b2090e) with template:\n{\"name\":\"main\",\"arguments\":{},\"inputs\":{},\"outputs\":{},\"metadata\":{\"annotations\":{\"cluster-autoscaler.kubernetes.io/safe-to-evict\":\"false\",\"instance-name\":\"las-mesos-agent-s075_39eae123b9ab458f85ad60cc06d6e798\"},\"labels\":{\"bucket-id\":\"58\",\"capos_id\":\"workflow-9fff344428454a82b821ccf0b5b2090e\",\"pod-relates-workflow\":\"true\"}},\"container\":{\"name\":\"main\",\"image\":\"docker.io/zcy19941015/pytest:v0.2\",\"command\":[\"/bin/sh\",\"-c\",\"sleep 86400\"],\"env\":[{\"name\":\"CAPOS_ID\",\"value\":\"workflow-9fff344428454a82b821ccf0b5b2090e\"},{\"name\":\"NAMESPACE\",\"value\":\"workflow-test\"}],\"resources\":{}}}"
time="2020-10-16T06:21:28.656Z" level=info msg="Waiting on main container"
time="2020-10-16T06:21:29.627Z" level=info msg="main container started with container ID: 8e5e9b55b790d3f797c7d7e0519e5b0e500cbbc03c5f85b1c196957fa8d47f5e"
time="2020-10-16T06:21:29.627Z" level=info msg="Starting annotations monitor"
time="2020-10-16T06:21:29.630Z" level=info msg="containerID kubepods-besteffort-podd2c0bbdb_0f77_11eb_b36d_02623cf324c8.slice:cri-containerd:8e5e9b55b790d3f797c7d7e0519e5b0e500cbbc03c5f85b1c196957fa8d47f5e mapped to pid 27"
time="2020-10-16T06:21:29.630Z" level=warning msg="Ignoring wait failure: Failed to determine pid for containerID 8e5e9b55b790d3f797c7d7e0519e5b0e500cbbc03c5f85b1c196957fa8d47f5e: container may have exited too quickly. Process assumed to have completed"
time="2020-10-16T06:21:29.630Z" level=info msg="Main container completed"
time="2020-10-16T06:21:29.630Z" level=info msg="No Script output reference in workflow. Capturing script output ignored"
time="2020-10-16T06:21:29.630Z" level=info msg="Capturing script exit code"
time="2020-10-16T06:21:29.630Z" level=info msg="Getting exit code of 8e5e9b55b790d3f797c7d7e0519e5b0e500cbbc03c5f85b1c196957fa8d47f5e"
time="2020-10-16T06:21:29.630Z" level=info msg="Starting deadline monitor"
time="2020-10-16T06:21:29.630Z" level=info msg="Deadline monitor stopped"
time="2020-10-16T06:21:29.630Z" level=info msg="/argo/podmetadata/annotations updated"
time="2020-10-16T06:21:29.633Z" level=info msg="No output parameters"
time="2020-10-16T06:21:29.633Z" level=info msg="No output artifacts"
time="2020-10-16T06:21:29.633Z" level=info msg="Killing sidecars"
time="2020-10-16T06:21:29.635Z" level=info msg="Alloc=6720 TotalAlloc=14913 Sys=70592 NumGC=5 Goroutines=8"

```

## Proposal

Add code below to https://github.com/argoproj/argo/blob/v2.11.3/workflow/executor/pns/pns.go#L407

```
if strings.Contains(containerID, "cri-containerd") {
    strList := strings.Split(containerID, ":")
    containerID = strList[len(strList) - 1]
}
```

---
<!-- Issue Author: Don't delete this message to encourage other users to support your issue! -->
**Message from the maintainers**:

Impacted by this bug? Give it a 👍. We prioritise the issues with the most 👍.


Pull request:
fix(executor): Retrieve containerId from cri-containerd /proc/{pid}/cgroup. Fixes #4302
Checklist:

* [x] Either (a) I've created an [enhancement proposal](https://github.com/argoproj/argo/issues/new/choose) and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
* [x] The title of the PR is (a) [conventional](https://www.conventionalcommits.org/en/v1.0.0/), (b) states what changed, and (c) suffixes the related issues number. E.g. `"fix(controller): Updates such and such. Fixes #1234"`.  
* [ ] My organization is added to [USERS.md](https://github.com/argoproj/argo/blob/master/USERS.md).
* [x] I've signed the CLA.
* [ ] I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
* [x] My builds are green. Try syncing with master if they are not. 

Fixes #4302

Patch:
diff --git a/Makefile b/Makefile
index f3b20f312..b4fd6e0d9 100644
--- a/Makefile
+++ b/Makefile
@@ -121,6 +121,11 @@ define docker_pull
 	if [ $(K3D) = true ]; then k3d image import $(1); fi
 endef
 
+ifndef $(GOPATH)
+	GOPATH=$(shell go env GOPATH)
+	export GOPATH
+endif
+
 .PHONY: build
 build: status clis images manifests
 
@@ -254,7 +259,7 @@ crds: $(GOPATH)/bin/controller-gen
 $(GOPATH)/bin/controller-gen:
 	trap 'rm -Rf vendor' EXIT
 	go mod vendor
-	go install ./vendor/sigs.k8s.io/controller-tools/cmd/controller-gen
+	go install -mod=vendor ./vendor/sigs.k8s.io/controller-tools/cmd/controller-gen
 
 $(GOPATH)/bin/go-to-protobuf:
 	trap 'rm -Rf vendor' EXIT
@@ -478,7 +483,7 @@ dist/mixed.swagger.json: $(GOPATH)/bin/swagger $(SWAGGER_FILES) dist/swagger-con
 dist/swaggifed.swagger.json: dist/mixed.swagger.json hack/swaggify.sh
 	cat dist/mixed.swagger.json | sed 's/VERSION/$(VERSION)/' | ./hack/swaggify.sh > dist/swaggifed.swagger.json
 
-dist/kubeified.swagger.json: dist/swaggifed.swagger.json dist/kubernetes.swagger.json 
+dist/kubeified.swagger.json: dist/swaggifed.swagger.json dist/kubernetes.swagger.json
 	go run ./hack/swagger kubeifyswagger dist/swaggifed.swagger.json dist/kubeified.swagger.json
 
 api/openapi-spec/swagger.json: dist/kubeified.swagger.json
@@ -520,4 +525,4 @@ check-version-warning:
 
 .PHONY: parse-examples
 parse-examples:
-	go run -tags fields ./hack parseexamples
\ No newline at end of file
+	go run -tags fields ./hack parseexamples
diff --git a/docs/variables.md b/docs/variables.md
index 3145542d6..5a0c1db95 100644
--- a/docs/variables.md
+++ b/docs/variables.md
@@ -1,9 +1,9 @@
 # Workflow Variables
 
-Some fields in a workflow specification allow for variable references which are automatically substituted by Argo. 
+Some fields in a workflow specification allow for variable references which are automatically substituted by Argo.
 
 ??? note "How to use variables"
-    Variables are enclosed in curly braces and **must not** include whitespace.
+    Variables are enclosed in curly braces and **may** include whitespace between the brackets and variable.
 
     ``` yaml
     apiVersion: argoproj.io/v1alpha1
@@ -24,7 +24,7 @@ Some fields in a workflow specification allow for variable references which are
         container:
           image: docker/whalesay
           command: [cowsay]
-        # args: ["{{ inputs.parameters.message }}"]       <- bad
+        # args: ["{{ inputs.parameters.message }}"]       <- good
           args: ["{{inputs.parameters.message}}"]         #  good
     ```
 
diff --git a/workflow/common/util.go b/workflow/common/util.go
index 0e6e36cc9..ffca81d8a 100644
--- a/workflow/common/util.go
+++ b/workflow/common/util.go
@@ -373,7 +373,7 @@ func SubstituteParams(tmpl *wfv1.Template, globalParams, localParams Parameters)
 func Replace(fstTmpl *fasttemplate.Template, replaceMap map[string]string, allowUnresolved bool) (string, error) {
 	var unresolvedErr error
 	replacedTmpl := fstTmpl.ExecuteFuncString(func(w io.Writer, tag string) (int, error) {
-		replacement, ok := replaceMap[tag]
+		replacement, ok := replaceMap[strings.TrimSpace(tag)]
 		if !ok {
 			// Attempt to resolve nested tags, if possible
 			if index := strings.LastIndex(tag, "{{"); index > 0 {
diff --git a/workflow/executor/pns/pns.go b/workflow/executor/pns/pns.go
index 7d20666a0..f3a0f5845 100644
--- a/workflow/executor/pns/pns.go
+++ b/workflow/executor/pns/pns.go
@@ -393,21 +393,38 @@ func parseContainerID(pid int) (string, error) {
 	defer func() { _ = cgroupFile.Close() }()
 	sc := bufio.NewScanner(cgroupFile)
 	for sc.Scan() {
-		// See https://www.systutorials.com/docs/linux/man/5-proc/ for /proc/XX/cgroup format. e.g.:
-		// 5:cpuacct,cpu,cpuset:/daemons
 		line := sc.Text()
 		log.Debugf("pid %d: %s", pid, line)
-		parts := strings.Split(line, "/")
-		if len(parts) > 1 {
-			if containerID := parts[len(parts)-1]; containerID != "" {
-				// need to check for empty string because the line may look like: 5:rdma:/
-
-				// for crio we need to get rid of "crio-" prefix and ".scope" suffix
-				// e.g. crio-7a92a067289f6197148912be1c15f20f0330c7f3c541473d3b9c4043ca137b42.scope
-				containerID := strings.TrimSuffix(strings.TrimPrefix(containerID, "crio-"), ".scope")
-				return containerID, nil
-			}
+		containerID := parseContainerIDFromCgroupLine(line)
+		if containerID != "" {
+			return containerID, nil
 		}
 	}
 	return "", errors.InternalErrorf("Failed to parse container ID from %s", cgroupPath)
 }
+
+func parseContainerIDFromCgroupLine(line string) string {
+	// See https://www.systutorials.com/docs/linux/man/5-proc/ for /proc/XX/cgroup format. e.g.:
+	// 5:cpuacct,cpu,cpuset:/daemons
+	parts := strings.Split(line, "/")
+	if len(parts) > 1 {
+		if containerID := parts[len(parts)-1]; containerID != "" {
+			// need to check for empty string because the line may look like: 5:rdma:/
+
+			// for crio we need to get rid of "crio-" prefix and ".scope" suffix
+			// e.g. crio-7a92a067289f6197148912be1c15f20f0330c7f3c541473d3b9c4043ca137b42.scope
+			containerID := strings.TrimSuffix(strings.TrimPrefix(containerID, "crio-"), ".scope")
+
+			// for compatibility with cri-containerd record format when using systemd cgroup path
+			// example record in /proc/{pid}/cgroup:
+			// 9:cpuset:/kubepods-besteffort-pod30556cce_0f92_11eb_b36d_02623cf324c8.slice:cri-containerd:c688c856b21cfb29c1dbf6c14793435e44a1299dfc12add33283239bffed2620
+			if strings.Contains(containerID, "cri-containerd") {
+				strList := strings.Split(containerID, ":")
+				containerID = strList[len(strList)-1]
+				containerID = strings.TrimPrefix(containerID, "cri-containerd-")
+			}
+			return containerID
+		}
+	}
+	return ""
+}


Test patch:
diff --git a/workflow/common/util_test.go b/workflow/common/util_test.go
index 0b66a00c7..13f741d62 100644
--- a/workflow/common/util_test.go
+++ b/workflow/common/util_test.go
@@ -157,3 +157,17 @@ func TestNestedReplaceString(t *testing.T) {
 		}
 	}
 }
+
+func TestReplaceStringWithWhiteSpace(t *testing.T) {
+
+	replaceMap := map[string]string{"inputs.parameters.message": "hello world"}
+
+	test := `{{ inputs.parameters.message }}`
+	fstTmpl, err := fasttemplate.NewTemplate(test, "{{", "}}")
+	if assert.NoError(t, err) {
+		replacement, err := Replace(fstTmpl, replaceMap, true)
+		if assert.NoError(t, err) {
+			assert.Equal(t, "hello world", replacement)
+		}
+	}
+}
diff --git a/workflow/executor/pns/pns_test.go b/workflow/executor/pns/pns_test.go
new file mode 100644
index 000000000..4c9aa37e1
--- /dev/null
+++ b/workflow/executor/pns/pns_test.go
@@ -0,0 +1,40 @@
+package pns
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestPNSExecutor_parseContainerIDFromCgroupLine(t *testing.T) {
+	testCases := []struct {
+		line     string
+		expected string
+	}{
+		{
+			line:     "",
+			expected: "",
+		},
+		{
+			line:     "5:rdma:/",
+			expected: "",
+		},
+		{
+			line:     "8:cpu,cpuacct:/kubepods/besteffort/pod2fad8aad-dcd0-4fef-b45a-151630b9a4b5/b844ef90162af07ad3fb2961ffb2f528f8bf7c9edb8c45465fd8651fab8a78c1",
+			expected: "b844ef90162af07ad3fb2961ffb2f528f8bf7c9edb8c45465fd8651fab8a78c1",
+		},
+		{
+			line:     "2:cpu,cpuacct:/system.slice/containerd.service/kubepods-burstable-podf61fae9b_d7a7_11ea_bb0c_12a065621d2b.slice:cri-containerd:b6b894a028b7ec8e0f98c57a5f7b9735ad792276c3ce52bc795fcd367d829de9",
+			expected: "b6b894a028b7ec8e0f98c57a5f7b9735ad792276c3ce52bc795fcd367d829de9",
+		},
+		{
+			line:     "8:cpu,cpuacct:/kubepods/besteffort/pod2fad8aad-dcd0-4fef-b45a-151630b9a4b5/crio-7a92a067289f6197148912be1c15f20f0330c7f3c541473d3b9c4043ca137b42.scope",
+			expected: "7a92a067289f6197148912be1c15f20f0330c7f3c541473d3b9c4043ca137b42",
+		},
+	}
+
+	for _, testCase := range testCases {
+		containerID := parseContainerIDFromCgroupLine(testCase.line)
+		assert.Equal(t, containerID, testCase.expected)
+	}
+}