Repository: operator-framework/operator-sdk
Pull request #4957 (2021-05-28 19:21:24)

fix(helm-operator): do not send empty patch requests to kube apiserver
**Description of the change:**
This change is in the helm operator when generating patch requests during each reconcile cycle.  The change filters out empty patches generated from the 3-way merge, which could be in the form of the "{}" byte array that represents an empty map.  This byte array is neither nil or zero-length, so the release reconciler sees it as non-empty and sends out a patch request to the kube apiserver.

This addresses https://github.com/operator-framework/operator-sdk/issues/4956.

**Motivation for the change:**
We have a deployment scenario with hundreds of helm operators, each generating ~60 empty patch requests even there are no changes.  That caused heavy traffic onto the kube apiserver.

**Testing done:**
- Built the operator with this change and confirmed no more such empty requests reported in #4956.
- Also tested a normal reconciliation scenario, by manually modifying a resource and observing that the operator still picks up the modification and reverts it upon the reconcile event.
- Updated the unit tests as well.

jsm84: /lgtm
asmacdo: looks reasonable to me, allowing the tests to run
openshift-ci[bot]: New changes are detected. LGTM label has been removed.
openshift-cherrypick-robot: @estroz: once the present PR merges, I will cherry-pick it on top of v1.8.x in a new PR and assign it to you.

<details>

In response to [this](https://github.com/operator-framework/operator-sdk/pull/4957#issuecomment-856220920):

>/cherry-pick v1.8.x
>/cherry-pick v1.7.x


Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
estroz: /cherry-pick v1.8.x
/cherry-pick v1.7.x
openshift-ci[bot]: New changes are detected. LGTM label has been removed.
chlam4: @estroz I had to force push the last commit to fulfill the DCO sign-off requirement.  Do you mind re-authorizing the workflows?  Thanks in advance!

Patch:
diff --git a/changelog/fragments/empty-patch-requests.yaml b/changelog/fragments/empty-patch-requests.yaml
new file mode 100644
index 00000000..d48a1fd8
--- /dev/null
+++ b/changelog/fragments/empty-patch-requests.yaml
@@ -0,0 +1,4 @@
+entries:
+  - description: >
+      For helm-based operators, empty patch requests generated from the 3-way merge are filtered to avoid unnecessary requests to the kube apiserver.
+    kind: bugfix
diff --git a/internal/helm/release/manager.go b/internal/helm/release/manager.go
index 9489d377..7ee19a0b 100644
--- a/internal/helm/release/manager.go
+++ b/internal/helm/release/manager.go
@@ -322,7 +322,15 @@ func createPatch(existing runtime.Object, expected *resource.Info) ([]byte, apit
 	}
 
 	patch, err := strategicpatch.CreateThreeWayMergePatch(expectedJSON, expectedJSON, existingJSON, patchMeta, true)
-	return patch, apitypes.StrategicMergePatchType, err
+	if err != nil {
+		return nil, apitypes.StrategicMergePatchType, err
+	}
+	// An empty patch could be in the form of "{}" which represents an empty map out of the 3-way merge;
+	// filter them out here too to avoid sending the apiserver empty patch requests.
+	if len(patch) == 0 || bytes.Equal(patch, []byte("{}")) {
+		return nil, apitypes.StrategicMergePatchType, nil
+	}
+	return patch, apitypes.StrategicMergePatchType, nil
 }
 
 func createJSONMergePatch(existingJSON, expectedJSON []byte) ([]byte, error) {
diff --git a/website/content/en/docs/building-operators/golang/operator-scope.md b/website/content/en/docs/building-operators/golang/operator-scope.md
index 230b99d1..1717562b 100644
--- a/website/content/en/docs/building-operators/golang/operator-scope.md
+++ b/website/content/en/docs/building-operators/golang/operator-scope.md
@@ -310,5 +310,5 @@ If the operator can watch multiple namespaces, set the following in your `spec.i
 [rbac-markers]: https://book.kubebuilder.io/reference/markers/rbac.html
 [olm-integration]: /docs/olm-integration
 [csv-base]: /docs/olm-integration/generation/#kustomize-files
-[bundle]: /docs/olm-integration/quickstart-bundle
+[bundle-quickstart]: /docs/olm-integration/quickstart-bundle
 [packagemanifests-quickstart]: /docs/olm-integration/quickstart-package-manifests


Test patch:
diff --git a/internal/helm/release/manager_test.go b/internal/helm/release/manager_test.go
index 77ccca43..f930374e 100644
--- a/internal/helm/release/manager_test.go
+++ b/internal/helm/release/manager_test.go
@@ -164,7 +164,7 @@ func TestManagerGenerateStrategicMergePatch(t *testing.T) {
 			o2: newTestDeployment([]v1.Container{
 				{Name: "test1", LivenessProbe: nil},
 			}),
-			patch:     `{}`,
+			patch:     ``,
 			patchType: apitypes.StrategicMergePatchType,
 		},
 		{
@@ -197,7 +197,7 @@ func TestManagerGenerateStrategicMergePatch(t *testing.T) {
 				},
 				Spec: appsv1.DeploymentSpec{},
 			},
-			patch:     `{}`,
+			patch:     ``,
 			patchType: apitypes.StrategicMergePatchType,
 		},
 	}