chore: Use strategic merge patching for Core APIs

[jonathan-innis]

Fixes #N/A

Description

Core APIs support strategic merge patching. This provides a more advanced way to patch together lists from standard merge patching, where lists can be merged if they have a strategic-merge-patch-key to denote them. For instance, this is true for the status conditions API.

CRDs do not support strategic merge patching out of the box so we cannot get rid of standard merge patching for them. This will resolve some errors in AWS E2E testing, where we are seeing errors like patching node, Node \"ip-192-168-49-61.us-east-2.compute.internal\" is invalid: metadata.finalizers: Forbidden: no new finalizers can be added if the object is being deleted, found new finalizers []string{\"foregroundDeletion\"}"}

How was this change tested?

make presubmit

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[Bryce-Soghigian]

Excited to see this as we have been observing this on the AKS side as well 🥳🥳🥳🥳🥳🥳🥳🥳

AKS Sample error

{"level":"ERROR","time":"2024-03-07T05:14:53.843Z","logger":"controller","message":"Reconciler error","commit":"f4b5ac0-dirty","controller":"node.termination","controllerGroup":"","controllerKind":"Node","Node":{"name":"aks-sparrowchain-4-eiyynjonza"},"namespace":"","name":"aks-sparrowchain-4-eiyynjonza","reconcileID":"2052a160-1fa6-4f75-8ce6-8ab37f57368f","error":"patching node, Node \"aks-sparrowchain-4-eiyynjonza\" is invalid: metadata.finalizers: Forbidden: no new finalizers can be added if the object is being deleted, found new finalizers []string{\"foregroundDeletion\"}"}

[jonathan-innis]

Excited to see this as we have been observing this on the AKS side as well

Yeah, sadly still doesn't solve the problem for CRDs 😞 Maybe one day we will get strategic merge patch support for CRDs.

[coveralls]

Pull Request Test Coverage Report for Build 8229522584

Details

• 5 of 5 (100.0%) changed or added relevant lines in 5 files are covered.
• 4 unchanged lines in 1 file lost coverage.
• Overall coverage decreased (-0.02%) to 81.094%

---

Files with Coverage Reduction	New Missed Lines	%
pkg/test/expectations/expectations.go	4	95.51%

Totals
Change from base Build 8206973287:	-0.02%
Covered Lines:	8227
Relevant Lines:	10145

---

💛 - Coveralls

[jonathan-innis]

@Bryce-Soghigian Just in case you were curious: As I was thinking through how this would actually work (considering strategic merge patches are good at representing merge operations, but I wasn't sure how they would resolve delete operations), I stumbled upon this deep in the documentation on the Kubernetes issues. Apparently there's a bespoke directive ($deleteFromPrimitiveList) that allows you to delete from a primitive set list by key. The fact that this isn't documented doesn't help 😄

[Bryce-Soghigian]

@jonathan-innis interesting will dive into the depths tommorrow

[jonathan-innis]

I was a little curious about what the time difference would be for running Strategic Merge Patching vs. Merge Patching. Here's the results from benchmarking. Turns out that strategic merge patching is slower as expected, but it looks like it's running at 20ms slower, which seems reasonable to take as an overhead cost here

Strategic Merge Patching

func BenchmarkStrategicMergePatch(b *testing.B) {
	ctx = TestContextWithLogger(b)
	env = test.NewEnvironment(scheme.Scheme, test.WithCRDs(apis.CRDs...), test.WithFieldIndexers(test.NodeClaimFieldIndexer(ctx)))
	for i := 0; i < b.N; i++ {
		node := test.Node()
		controllerutil.AddFinalizer(node, v1beta1.TerminationFinalizer)
                controllerutil.AddFinalizer(node, "test-finalizer")
		stored := node.DeepCopy()
		controllerutil.RemoveFinalizer(node, v1beta1.TerminationFinalizer)
		patch := client.StrategicMergeFrom(stored)
		_, _ = patch.Data(node)
	}
}

BenchmarkStrategicMergePatch-10    	   35680	     42300 ns/op
PASS

Merge Patching

func BenchmarkMergePatch(b *testing.B) {
	for i := 0; i < b.N; i++ {
		node := test.Node()
		controllerutil.AddFinalizer(node, v1beta1.TerminationFinalizer)
                controllerutil.AddFinalizer(node, "test-finalizer")
		stored := node.DeepCopy()
		controllerutil.RemoveFinalizer(node, v1beta1.TerminationFinalizer)
		patch := client.MergeFrom(stored)
		_, _ = patch.Data(node)
	}
}

BenchmarkMergePatch-10    	   56526	     22676 ns/op
PASS

[njtran]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jonathan-innis, njtran

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [jonathan-innis,njtran]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment