Repository: argoproj/argo
Pull request #4835 (2021-01-06 10:10:48)

fix(controller): Preserve the original slice when removing string
Checklist:

* [ ] My organization is added to [USERS.md](https://github.com/argoproj/argo/blob/master/USERS.md).

<!--

Please leave your PR in draft if you don't need a review yet. 

To fix failing `CI / Codegen`, run `make codegen`. 

-->


juchaosong: The previous `RemoveString()` will change the origin slice. When releasing all semaphores, it will change original `wf.Status.Synchronization.Semaphore.Holding` referring to https://github.com/argoproj/argo/blob/master/workflow/sync/sync_manager.go#L182. This will cause the iterated array to change, and some semaphores does't be released.
You can change https://github.com/argoproj/argo/blob/master/examples/synchronization-tmpl-level.yaml#L37 `sleep 10` to more time to reproduce the bug.
```
kubectl create -f examples/synchronization-tmpl-level.yaml
kubectl delete wf $(kubectl get wf | grep synchronization-tmpl-level | awk '{print$1}')
``` 
The controller logs:
```
time="2021-01-06T10:35:54.659Z" level=info msg="Processing workflow" namespace=default workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:54.659Z" level=info msg="Updated phase  -> Running" namespace=default workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:54.659Z" level=info msg="Steps node synchronization-tmpl-level-gfzqp initialized Running" namespace=default workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:54.659Z" level=info msg="StepGroup node synchronization-tmpl-level-gfzqp-3634344595 initialized Running" namespace=default workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:54.665Z" level=info msg="default/ConfigMap/my-config/template acquired by default/synchronization-tmpl-level-gfzqp/synchronization-tmpl-level-gfzqp-397032944 " semaphore=default/ConfigMap/my-config/template
time="2021-01-06T10:35:54.665Z" level=info msg="Node synchronization-tmpl-level-gfzqp[0].synchronization-acquire-lock(0:1) acquired synchronization lock" namespace=default workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:54.665Z" level=info msg="Pod node synchronization-tmpl-level-gfzqp-397032944 initialized Pending" namespace=default workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:54.683Z" level=info msg="Created pod: synchronization-tmpl-level-gfzqp[0].synchronization-acquire-lock(0:1) (synchronization-tmpl-level-gfzqp-397032944)" namespace=default workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:54.688Z" level=info msg="default/ConfigMap/my-config/template acquired by default/synchronization-tmpl-level-gfzqp/synchronization-tmpl-level-gfzqp-1627841290 " semaphore=default/ConfigMap/my-config/template
time="2021-01-06T10:35:54.688Z" level=info msg="Node synchronization-tmpl-level-gfzqp[0].synchronization-acquire-lock(1:2) acquired synchronization lock" namespace=default workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:54.688Z" level=info msg="Pod node synchronization-tmpl-level-gfzqp-1627841290 initialized Pending" namespace=default workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:54.703Z" level=info msg="Created pod: synchronization-tmpl-level-gfzqp[0].synchronization-acquire-lock(1:2) (synchronization-tmpl-level-gfzqp-1627841290)" namespace=default workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:54.708Z" level=info msg="default/ConfigMap/my-config/template acquired by default/synchronization-tmpl-level-gfzqp/synchronization-tmpl-level-gfzqp-614236760 " semaphore=default/ConfigMap/my-config/template
time="2021-01-06T10:35:54.708Z" level=info msg="Node synchronization-tmpl-level-gfzqp[0].synchronization-acquire-lock(2:3) acquired synchronization lock" namespace=default workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:54.708Z" level=info msg="Pod node synchronization-tmpl-level-gfzqp-614236760 initialized Pending" namespace=default workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:54.723Z" level=info msg="Created pod: synchronization-tmpl-level-gfzqp[0].synchronization-acquire-lock(2:3) (synchronization-tmpl-level-gfzqp-614236760)" namespace=default workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:54.728Z" level=info msg="Pod node synchronization-tmpl-level-gfzqp-4180462278 initialized Pending" namespace=default workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:54.733Z" level=info msg="Pod node synchronization-tmpl-level-gfzqp-2103118504 initialized Pending" namespace=default workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:54.733Z" level=info msg="Workflow step group node synchronization-tmpl-level-gfzqp-3634344595 not yet completed" namespace=default workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:54.742Z" level=info msg="Workflow update successful" namespace=default phase=Running resourceVersion=2415604849 workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:55.683Z" level=info msg="Processing workflow" namespace=default workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:55.683Z" level=info msg="Updating node synchronization-tmpl-level-gfzqp-614236760 message: ContainerCreating"
time="2021-01-06T10:35:55.683Z" level=info msg="Updating node synchronization-tmpl-level-gfzqp-1627841290 message: ContainerCreating"
time="2021-01-06T10:35:55.683Z" level=info msg="Updating node synchronization-tmpl-level-gfzqp-397032944 message: ContainerCreating"
time="2021-01-06T10:35:55.689Z" level=info msg="Node synchronization-tmpl-level-gfzqp[0].synchronization-acquire-lock(0:1) acquired synchronization lock" namespace=default workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:55.689Z" level=info msg="Skipped pod synchronization-tmpl-level-gfzqp[0].synchronization-acquire-lock(0:1) (synchronization-tmpl-level-gfzqp-397032944) creation: already exists" namespace=default podPhase=Pending workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:55.695Z" level=info msg="Node synchronization-tmpl-level-gfzqp[0].synchronization-acquire-lock(1:2) acquired synchronization lock" namespace=default workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:55.695Z" level=info msg="Skipped pod synchronization-tmpl-level-gfzqp[0].synchronization-acquire-lock(1:2) (synchronization-tmpl-level-gfzqp-1627841290) creation: already exists" namespace=default podPhase=Pending workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:55.700Z" level=info msg="Node synchronization-tmpl-level-gfzqp[0].synchronization-acquire-lock(2:3) acquired synchronization lock" namespace=default workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:55.700Z" level=info msg="Skipped pod synchronization-tmpl-level-gfzqp[0].synchronization-acquire-lock(2:3) (synchronization-tmpl-level-gfzqp-614236760) creation: already exists" namespace=default podPhase=Pending workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:55.711Z" level=info msg="Workflow step group node synchronization-tmpl-level-gfzqp-3634344595 not yet completed" namespace=default workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:55.720Z" level=info msg="Workflow update successful" namespace=default phase=Running resourceVersion=2415604911 workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:57.540Z" level=info msg="Processing workflow" namespace=default workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:57.540Z" level=info msg="Updating node synchronization-tmpl-level-gfzqp-614236760 status Pending -> Running"
time="2021-01-06T10:35:57.540Z" level=info msg="Updating node synchronization-tmpl-level-gfzqp-397032944 status Pending -> Running"
time="2021-01-06T10:35:57.540Z" level=info msg="Updating node synchronization-tmpl-level-gfzqp-1627841290 status Pending -> Running"
time="2021-01-06T10:35:57.546Z" level=info msg="Node synchronization-tmpl-level-gfzqp[0].synchronization-acquire-lock(0:1) acquired synchronization lock" namespace=default workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:57.547Z" level=info msg="Skipped pod synchronization-tmpl-level-gfzqp[0].synchronization-acquire-lock(0:1) (synchronization-tmpl-level-gfzqp-397032944) creation: already exists" namespace=default podPhase=Running workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:57.554Z" level=info msg="Node synchronization-tmpl-level-gfzqp[0].synchronization-acquire-lock(1:2) acquired synchronization lock" namespace=default workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:57.554Z" level=info msg="Skipped pod synchronization-tmpl-level-gfzqp[0].synchronization-acquire-lock(1:2) (synchronization-tmpl-level-gfzqp-1627841290) creation: already exists" namespace=default podPhase=Running workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:57.560Z" level=info msg="Node synchronization-tmpl-level-gfzqp[0].synchronization-acquire-lock(2:3) acquired synchronization lock" namespace=default workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:57.560Z" level=info msg="Skipped pod synchronization-tmpl-level-gfzqp[0].synchronization-acquire-lock(2:3) (synchronization-tmpl-level-gfzqp-614236760) creation: already exists" namespace=default podPhase=Running workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:57.572Z" level=info msg="Workflow step group node synchronization-tmpl-level-gfzqp-3634344595 not yet completed" namespace=default workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:35:57.581Z" level=info msg="Workflow update successful" namespace=default phase=Running resourceVersion=2415605037 workflow=synchronization-tmpl-level-gfzqp
time="2021-01-06T10:36:01.405Z" level=info msg="Lock has been released by default/synchronization-tmpl-level-gfzqp/synchronization-tmpl-level-gfzqp-397032944. Available locks: 1" semaphore=default/ConfigMap/my-config/template
time="2021-01-06T10:36:01.405Z" level=info msg="default/synchronization-tmpl-level-gfzqp/synchronization-tmpl-level-gfzqp-397032944 released a lock from default/ConfigMap/my-config/template"
time="2021-01-06T10:36:01.405Z" level=info msg="Lock has been released by default/synchronization-tmpl-level-gfzqp/synchronization-tmpl-level-gfzqp-614236760. Available locks: 2" semaphore=default/ConfigMap/my-config/template
time="2021-01-06T10:36:01.405Z" level=info msg="default/synchronization-tmpl-level-gfzqp/synchronization-tmpl-level-gfzqp-614236760 released a lock from default/ConfigMap/my-config/template"
time="2021-01-06T10:36:01.406Z" level=info msg="default/synchronization-tmpl-level-gfzqp/synchronization-tmpl-level-gfzqp-614236760 released a lock from default/ConfigMap/my-config/template"
time="2021-01-06T10:39:43.880Z" level=info msg="Alloc=10960 TotalAlloc=99747 Sys=70592 NumGC=15 Goroutines=176"
```
You will find `default/synchronization-tmpl-level-gfzqp/synchronization-tmpl-level-gfzqp-614236760` release lock twice, and only 2 locks available, but 3 locks acquired.
alexec: tips: https://github.com/golang/go/wiki/SliceTricks

Patch:
diff --git a/Makefile b/Makefile
index eb06e7b07..d64e849df 100644
--- a/Makefile
+++ b/Makefile
@@ -371,7 +371,7 @@ manifests/install.yaml: $(CRDS) /usr/local/bin/kustomize
 # lint/test/etc
 
 $(GOPATH)/bin/golangci-lint:
-	curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b `go env GOPATH`/bin v1.27.0
+	curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b `go env GOPATH`/bin v1.33.2
 
 .PHONY: lint
 lint: server/static/files.go $(GOPATH)/bin/golangci-lint
diff --git a/go.mod b/go.mod
index a78441f76..7202bb9af 100644
--- a/go.mod
+++ b/go.mod
@@ -3,7 +3,7 @@ module github.com/argoproj/argo
 go 1.13
 
 require (
-	bou.ke/staticfiles v0.0.0-20190225145250-827d7f6389cd
+	bou.ke/staticfiles v0.0.0-20210106104248-dd04075d4104
 	cloud.google.com/go v0.55.0 // indirect
 	cloud.google.com/go/storage v1.6.0
 	github.com/Knetic/govaluate v3.0.1-0.20171022003610-9aa49832a739+incompatible
diff --git a/go.sum b/go.sum
index a708e20f9..dad55790a 100644
--- a/go.sum
+++ b/go.sum
@@ -1,5 +1,5 @@
-bou.ke/staticfiles v0.0.0-20190225145250-827d7f6389cd h1:7C8NlNNaforPxSBAogtUdw9VrvgDpSNmbaruZsIxP4Q=
-bou.ke/staticfiles v0.0.0-20190225145250-827d7f6389cd/go.mod h1:JpKWzdMX3ZBe8DYU+jWnZyYT4tJR9pMee/kPGvap41o=
+bou.ke/staticfiles v0.0.0-20210106104248-dd04075d4104 h1:EX9woBfEDHA1l8fPOsdEyeSI5rNRPVBb8AqIHm7jPU4=
+bou.ke/staticfiles v0.0.0-20210106104248-dd04075d4104/go.mod h1:JpKWzdMX3ZBe8DYU+jWnZyYT4tJR9pMee/kPGvap41o=
 cloud.google.com/go v0.26.0/go.mod h1:aQUYkXzVsufM+DwF1aE+0xfcU+56JwCaLick0ClmMTw=
 cloud.google.com/go v0.34.0/go.mod h1:aQUYkXzVsufM+DwF1aE+0xfcU+56JwCaLick0ClmMTw=
 cloud.google.com/go v0.38.0/go.mod h1:990N+gfupTy94rShfmMCWGDn0LpTmnzTp2qbd1dvSRU=
diff --git a/pkg/apis/workflow/v1alpha1/item.go b/pkg/apis/workflow/v1alpha1/item.go
index 288cc57e6..142233f65 100644
--- a/pkg/apis/workflow/v1alpha1/item.go
+++ b/pkg/apis/workflow/v1alpha1/item.go
@@ -64,7 +64,9 @@ func (i *Item) String() string {
 	}
 	// this convenience to remove quotes from strings will cause many problems
 	if jsonBytes[0] == '"' {
-		return string(jsonBytes[1 : len(jsonBytes)-1])
+		var str string
+		_ = json.Unmarshal(jsonBytes, &str)
+		return str
 	}
 	return string(jsonBytes)
 }
diff --git a/util/errors/errors.go b/util/errors/errors.go
index c508d921c..e554b3d05 100644
--- a/util/errors/errors.go
+++ b/util/errors/errors.go
@@ -15,7 +15,7 @@ func IsTransientErr(err error) bool {
 		return false
 	}
 	err = argoerrs.Cause(err)
-	return isExceededQuotaErr(err) || apierr.IsTooManyRequests(err) || isResourceQuotaConflictErr(err) || isTransientNetworkErr(err)
+	return isExceededQuotaErr(err) || apierr.IsTooManyRequests(err) || isResourceQuotaConflictErr(err) || isTransientNetworkErr(err) || apierr.IsServerTimeout(err) || apierr.IsServiceUnavailable(err)
 }
 
 func isExceededQuotaErr(err error) bool {
diff --git a/util/slice/slice.go b/util/slice/slice.go
index 487b322fc..2787cce42 100644
--- a/util/slice/slice.go
+++ b/util/slice/slice.go
@@ -3,7 +3,9 @@ package slice
 func RemoveString(slice []string, element string) []string {
 	for i, v := range slice {
 		if element == v {
-			return append(slice[:i], slice[i+1:]...)
+			ret := make([]string, 0, len(slice)-1)
+			ret = append(ret, slice[:i]...)
+			return append(ret, slice[i+1:]...)
 		}
 	}
 	return slice


Test patch:
diff --git a/pkg/apis/workflow/v1alpha1/item_test.go b/pkg/apis/workflow/v1alpha1/item_test.go
index 9e511a519..c6459ffa9 100644
--- a/pkg/apis/workflow/v1alpha1/item_test.go
+++ b/pkg/apis/workflow/v1alpha1/item_test.go
@@ -65,3 +65,24 @@ func TestItem_GetStrVal(t *testing.T) {
 	val := item.GetStrVal()
 	assert.Equal(t, "foo", val)
 }
+
+var testItemStringTable = []struct {
+	name   string
+	origin interface{}
+	str    string
+}{
+	{"string", "http://www/any?query=1&query2=2", "http://www/any?query=1&query2=2"},
+	{"array", []int{1, 2, 3}, "[1,2,3]"},
+	{"number", 1.1, "1.1"},
+}
+
+func TestItem_String(t *testing.T) {
+	for _, s := range testItemStringTable {
+		t.Run(s.name, func(t *testing.T) {
+			bytes, _ := json.Marshal(s.origin)
+			var i Item
+			_ = json.Unmarshal(bytes, &i)
+			assert.Equal(t, s.str, i.String())
+		})
+	}
+}
diff --git a/util/util_test.go b/util/util_test.go
index 4c44d974d..d46c5dd5b 100644
--- a/util/util_test.go
+++ b/util/util_test.go
@@ -4,9 +4,8 @@ import (
 	"os"
 	"testing"
 
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-
 	"github.com/stretchr/testify/assert"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 func TestGenerateFieldSelectorFromWorkflowName(t *testing.T) {
@@ -80,4 +79,4 @@ func TestGetDeletePropagation(t *testing.T) {
 		os.Setenv("WF_DEL_PROPAGATION_POLICY", "")
 		assert.Equal(t, metav1.DeletePropagationBackground, *GetDeletePropagation())
 	})
-}
\ No newline at end of file
+}
diff --git a/workflow/controller/controller_test.go b/workflow/controller/controller_test.go
index 7e00ec206..e0d72e254 100644
--- a/workflow/controller/controller_test.go
+++ b/workflow/controller/controller_test.go
@@ -417,8 +417,8 @@ metadata:
 spec:
   entrypoint: main
   templates:
-    - name: main 
-      container: 
+    - name: main
+      container:
         image: my-image
 `),
 		unmarshalWF(`
@@ -428,7 +428,7 @@ spec:
   entrypoint: main
   templates:
     - name: main
-      container: 
+      container:
         image: my-image
 `),
 		func(controller *WorkflowController) { controller.Config.Parallelism = 1 },
diff --git a/workflow/sync/sync_manager_test.go b/workflow/sync/sync_manager_test.go
index d11ee32e4..835d5965c 100644
--- a/workflow/sync/sync_manager_test.go
+++ b/workflow/sync/sync_manager_test.go
@@ -660,5 +660,4 @@ func TestMutexWfLevel(t *testing.T) {
 		assert.Len(t, mutex.mutex.pending.items, 0)
 
 	})
-
-}
\ No newline at end of file
+}