fix(executor): Increase pod patch backoff. Fixes argoproj#4339 (argop…

…roj#4340)
jlegrone · Oct 23, 2020 · b76246e · b76246e
1 parent 7bfe303
commit b76246e
Show file tree

Hide file tree

Showing 2 changed files with 35 additions and 18 deletions.
diff --git a/workflow/common/util.go b/workflow/common/util.go
@@ -20,6 +20,7 @@ import (
  apierr "k8s.io/apimachinery/pkg/api/errors"
  metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
  "k8s.io/apimachinery/pkg/types"
+ "k8s.io/apimachinery/pkg/util/wait"
  "k8s.io/client-go/kubernetes"
  "k8s.io/client-go/rest"
  "k8s.io/client-go/tools/remotecommand"
@@ -434,44 +435,54 @@ func RunShellCommand(arg ...string) ([]byte, error) {
  return RunCommand(name, arg...)
 }
 
-const patchRetries = 5
+// Run Seconds
+// 0 0.000
+// 1 1.000
+// 2 2.000
+// 3 3.000
+// 4 4.000
+var defaultPatchBackoff = wait.Backoff{
+ Steps: 5,
+ Duration: 1 * time.Second,
+ Factor: 1,
+}
 
 // AddPodAnnotation adds an annotation to pod
-func AddPodAnnotation(c kubernetes.Interface, podName, namespace, key, value string) error {
- return addPodMetadata(c, "annotations", podName, namespace, key, value)
+func AddPodAnnotation(c kubernetes.Interface, podName, namespace, key, value string, options ...interface{}) error {
+ backoff := defaultPatchBackoff
+ for _, option := range options {
+ switch v := option.(type) {
+ case wait.Backoff:
+ backoff = v
+ default:
+ panic("unknown option type")
+ }
+ }
+ return addPodMetadata(c, "annotations", podName, namespace, key, value, backoff)
 }
 
 // AddPodLabel adds an label to pod
 func AddPodLabel(c kubernetes.Interface, podName, namespace, key, value string) error {
- return addPodMetadata(c, "labels", podName, namespace, key, value)
+ return addPodMetadata(c, "labels", podName, namespace, key, value, defaultPatchBackoff)
 }
 
 // addPodMetadata is helper to either add a pod label or annotation to the pod
-func addPodMetadata(c kubernetes.Interface, field, podName, namespace, key, value string) error {
+func addPodMetadata(c kubernetes.Interface, field, podName, namespace, key, value string, backoff wait.Backoff) error {
  metadata := map[string]interface{}{
  "metadata": map[string]interface{}{
  field: map[string]string{
  key: value,
  },
  },
  }
- var err error
  patch, err := json.Marshal(metadata)
  if err != nil {
  return errors.InternalWrapError(err)
  }
- for attempt := 0; attempt < patchRetries; attempt++ {
+ return wait.ExponentialBackoff(backoff, func() (bool, error) {
  _, err = c.CoreV1().Pods(namespace).Patch(podName, types.MergePatchType, patch)
- if err != nil {
- if !apierr.IsConflict(err) {
- return err
- }
- } else {
- break
- }
- time.Sleep(100 * time.Millisecond)
- }
- return err
+ return err == nil, err
+ })
 }
 
 const deleteRetries = 3

diff --git a/workflow/executor/executor.go b/workflow/executor/executor.go
@@ -40,6 +40,12 @@ import (
 )
 
 // ExecutorRetry is a retry backoff settings for WorkflowExecutor
+// Run Seconds
+// 0 0.000
+// 1 1.000
+// 2 2.600
+// 3 5.160
+// 4 9.256
 var ExecutorRetry = wait.Backoff{
  Steps: 5,
  Duration: 1 * time.Second,
@@ -829,7 +835,7 @@ func (we *WorkflowExecutor) AddError(err error) {
 
 // AddAnnotation adds an annotation to the workflow pod
 func (we *WorkflowExecutor) AddAnnotation(key, value string) error {
- return common.AddPodAnnotation(we.ClientSet, we.PodName, we.Namespace, key, value)
+ return common.AddPodAnnotation(we.ClientSet, we.PodName, we.Namespace, key, value, ExecutorRetry)
 }
 
 // isTarball returns whether or not the file is a tarball