Repository: knative/pkg
Pull request #1696 (2020-09-09 22:51:43)

Exclude the control-plane from our webhooks for psbinding webhook.
do not fight with azure's webhook for webhooks

Relates to #1590

knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1696#" title="Author self-approved">n3wscott</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **tcnghia**
You can assign the PR to them by writing `/assign @tcnghia` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

<details open>
Needs approval from an approver in each of these files:

- **[webhook/OWNERS](https://github.com/knative/pkg/blob/master/webhook/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["tcnghia"]} -->
mattmoor: /hold

Link the Azure issue?
n3wscott: It is linked from our issue, but it is this one : https://github.com/Azure/AKS/issues/1771

It is not clear it is an issue. They are doing a normal thing, and we are re-reconciling the object back to how we thing it should look. We should be smarter about mutation webhooks in general.

Patch:
diff --git a/reconciler/retry.go b/reconciler/retry.go
index dd9d41455..92ba69e1b 100644
--- a/reconciler/retry.go
+++ b/reconciler/retry.go
@@ -16,16 +16,51 @@ limitations under the License.
 package reconciler
 
 import (
+	"strings"
+
+	apierrs "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/client-go/util/retry"
 )
 
 // RetryUpdateConflicts retries the inner function if it returns conflict errors.
 // This can be used to retry status updates without constantly reenqueuing keys.
 func RetryUpdateConflicts(updater func(int) error) error {
+	return RetryErrors(updater, apierrs.IsConflict)
+}
+
+// RetryErrors retries the inner function if it returns matching errors.
+func RetryErrors(updater func(int) error, fns ...func(error) bool) error {
 	attempts := 0
-	return retry.RetryOnConflict(retry.DefaultRetry, func() error {
+	return retry.OnError(retry.DefaultRetry, func(err error) bool {
+		for _, fn := range fns {
+			if fn(err) {
+				return true
+			}
+		}
+		return false
+	}, func() error {
 		err := updater(attempts)
 		attempts++
 		return err
 	})
 }
+
+// RetryTestErrors retries the inner function if it hits an error type that is
+// common in our test environments.
+func RetryTestErrors(updater func(int) error, fns ...func(error) bool) error {
+	return RetryErrors(updater,
+		// Example: conflicts updating `gke-resource-quotas` (implicit on Service/Pod/Ingress creations)
+		apierrs.IsConflict,
+
+		// Example: https://github.com/knative/test-infra/issues/2346#issuecomment-687220045
+		func(err error) bool {
+			return strings.Contains(err.Error(), "gke-resource-quotas")
+		},
+
+		// Example: `etcdserver: request timed out`
+		// TODO(mattmoor): Does apierrs.IsServerTimeout catch the above?
+		func(err error) bool {
+			return strings.Contains(err.Error(), "etcdserver")
+		},
+	)
+}
diff --git a/webhook/psbinding/psbinding.go b/webhook/psbinding/psbinding.go
index 9472e16bd..ec94ebf87 100644
--- a/webhook/psbinding/psbinding.go
+++ b/webhook/psbinding/psbinding.go
@@ -134,6 +134,11 @@ var (
 			Key:      duck.BindingExcludeLabel,
 			Operator: metav1.LabelSelectorOpNotIn,
 			Values:   []string{"true"},
+		}, {
+			// "control-plane" is added to support Azure's AKS, otherwise the controllers fight.
+			// See knative/pkg#1590 for details.
+			Key:      "control-plane",
+			Operator: metav1.LabelSelectorOpDoesNotExist,
 		}},
 		// TODO(mattmoor): Consider also having a GVR-based one, e.g.
 		//    foobindings.blah.knative.dev/exclude: "true"
@@ -143,6 +148,11 @@ var (
 			Key:      duck.BindingIncludeLabel,
 			Operator: metav1.LabelSelectorOpIn,
 			Values:   []string{"true"},
+		}, {
+			// "control-plane" is added to support Azure's AKS, otherwise the controllers fight.
+			// See knative/pkg#1590 for details.
+			Key:      "control-plane",
+			Operator: metav1.LabelSelectorOpDoesNotExist,
 		}},
 		// TODO(mattmoor): Consider also having a GVR-based one, e.g.
 		//    foobindings.blah.knative.dev/include: "true"


Test patch:
diff --git a/reconciler/retry_test.go b/reconciler/retry_test.go
index b4bf6a12e..b10f41284 100644
--- a/reconciler/retry_test.go
+++ b/reconciler/retry_test.go
@@ -78,3 +78,59 @@ func TestRetryUpdateConflicts(t *testing.T) {
 		})
 	}
 }
+
+func TestRetryTestErrors(t *testing.T) {
+	errAny := errors.New("foo")
+	errGKE := errors.New(`Operation cannot be fulfilled on resourcequotas "gke-resource-quotas": StorageError: invalid object, Code: 4, Key: /registry/resourcequotas/serving-tests/gke-resource-quotas, ResourceVersion: 0, AdditionalErrorMsg: Precondition failed: UID in precondition: 7aaedbdf-caa8-41e7-94cb-f8c053038e86, UID in object meta:`)
+	errEtcd := errors.New("etcdserver: request timed out")
+	errConflict := apierrs.NewConflict(v1.Resource("foo"), "bar", errAny)
+
+	tests := []struct {
+		name         string
+		returns      []error
+		want         error
+		wantAttempts int
+	}{{
+		name:         "all good",
+		returns:      []error{nil},
+		want:         nil,
+		wantAttempts: 1,
+	}, {
+		name:         "not retry on non-conflict error",
+		returns:      []error{errAny},
+		want:         errAny,
+		wantAttempts: 1,
+	}, {
+		name:         "retry up to 5 times",
+		returns:      []error{errConflict, errGKE, errEtcd, errConflict, errGKE, errEtcd},
+		want:         errGKE,
+		wantAttempts: 5,
+	}, {
+		name:         "eventually succeed",
+		returns:      []error{errConflict, errGKE, errEtcd, nil},
+		want:         nil,
+		wantAttempts: 4,
+	}, {
+		name:         "eventually fail",
+		returns:      []error{errConflict, errConflict, errAny},
+		want:         errAny,
+		wantAttempts: 3,
+	}}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			attempts := 0
+			got := RetryTestErrors(func(i int) error {
+				attempts++
+				return test.returns[i]
+			})
+
+			if got != test.want {
+				t.Errorf("RetryTestErrors() = %v, want %v", got, test.want)
+			}
+			if attempts != test.wantAttempts {
+				t.Errorf("attempts = %d, want %d", attempts, test.wantAttempts)
+			}
+		})
+	}
+}