✨ Add TransformStripManagedFields

[alvaroaleman]

This change adds TransformStripManagedFields to the Cache, a transform func that strips off all managed fields for objects. There are no known issues when using this as a DefaultTransform unless there is explicit code to access objects ManagedFields and it can lead to a significant reduction in memory usage.

[Danil-Grigorev]

This is very useful when using client.Patch with Apply logic, thanks
/lgtm
/hold
if anyone wants to take a look

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 8bfda55969144726bdabcfb9c7cad9a597b588a0

[sbueringer]

Thx!!

/approve
/lgtm

/hold cancel

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, Danil-Grigorev, sbueringer

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 [alvaroaleman,sbueringer]

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

[akutz]

Howdy @alvaroaleman,

How did you arrive at:

There are no known issues when using this as a DefaultTransform unless there is explicit code to access objects ManagedFields

Per https://kubernetes.io/docs/reference/using-api/server-side-apply/:

It is however possible to change .metadata.managedFields through an update, or through a patch operation that does not use Server-Side Apply. Doing so is highly discouraged, but might be a reasonable option to try if, for example, the .metadata.managedFields get into an inconsistent state (which should not happen in normal operations).

Is the transformer from this PR not going to result in controllers sending patches/updates that no longer have the managed fields? For example, none of our controllers directly access managed fields, but we do write objects back to etcd, and if the cached object no longer has the managed fields, won't an update (we do use patch, but for example) remove the managed fields?

Is it because an update from a controller-runtime manager client will have field manager ID of manager (by default) and thus not impact the managed fields of another field manager, like the client kubectl, which uses the field manager ID of kubectl?

[sbueringer]

I think with patch, if you don't modify the managed fields in your code they simply won't be part of the delta and thus not part of the patch sent to the apiserver (so doesn't matter if they are there and don't have a diff or if they are not there and don't have a diff).

It is definitely easily possible to Patch managedFields if you actually modify managed fields in your code. We're using this in some places in Cluster API.

I have no experience with update. There I could imagine it might lead to problems

[akutz]

I think with patch, if you don't modify the managed fields in your code they simply won't be part of the delta and thus not part of the patch sent to the apiserver (so doesn't matter if they are there and don't have a diff or if they are not there and don't have a diff).

It is definitely easily possible to Patch managedFields if you actually modify managed fields in your code. We're using this in some places in Cluster API.

I have no experience with update. There I could imagine it might lead to problems

If the above is true, then I do think the documentation for the transformer and description of this PR should be updated. Otherwise the current documentation makes it sound like there is zero downside for using the stripping transformer, but that may not be the case if folks are using client.Update or controllerutil.CreateOrUpdate with a caching client that strips managed fields.

[sbueringer]

Let's see if someone else knows more. I simply never tried what happens with Update, we are always using Patch

[alvaroaleman]

So a quick test shows that update with nil managed fields does not clear them:

k get deployment coredns -ojson --show-managed-fields=true|jq .metadata.managedFields >/tmp/original
cat main.go
package main

import (
	"context"

	appsv1 "k8s.io/api/apps/v1"
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
	ctrl "sigs.k8s.io/controller-runtime"
	"sigs.k8s.io/controller-runtime/pkg/cache"
	"sigs.k8s.io/controller-runtime/pkg/client"
)

func main() {
	mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{
		Cache: cache.Options{DefaultTransform: cache.TransformStripManagedFields()},
	})
	if err != nil {
		panic(err)
	}

	go func() {
		if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil {
			panic(err)
		}
	}()

	ctx := context.Background()
	if _, err := mgr.GetCache().GetInformer(ctx, &appsv1.Deployment{}); err != nil {
		panic(err)
	}
	dep := &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{
		Name:      "coredns",
		Namespace: "kube-system",
	}}
	if err := mgr.GetClient().Get(ctx, client.ObjectKeyFromObject(dep), dep); err != nil {
		panic(err)
	}

	dep.Annotations["foo"] = "bar"
	if err := mgr.GetClient().Update(ctx, dep); err != nil {
		panic(err)
	}
}

go run ./
k get deployment coredns -ojson --show-managed-fields=true|jq .metadata.managedFields >/tmp/updated
$ diff -u /tmp/original /tmp/updated
--- /tmp/original	2024-09-11 21:42:10
+++ /tmp/updated	2024-09-11 21:44:14
@@ -164,6 +164,20 @@
     "fieldsV1": {
       "f:metadata": {
         "f:annotations": {
+          "f:foo": {}
+        }
+      }
+    },
+    "manager": "foo.bar",
+    "operation": "Update",
+    "time": "2024-09-12T01:44:07Z"
+  },
+  {
+    "apiVersion": "apps/v1",
+    "fieldsType": "FieldsV1",
+    "fieldsV1": {
+      "f:metadata": {
+        "f:annotations": {
           ".": {},
           "f:deployment.kubernetes.io/revision": {}
         }
@@ -200,6 +214,6 @@
     "manager": "kube-controller-manager",
     "operation": "Update",
     "subresource": "status",
-    "time": "2024-08-31T22:23:53Z"
+    "time": "2024-09-12T01:44:07Z"
   }
 ]

It should also be mentioned that upstream did this change for the controller-manager and scheduler), those two are what prompted me to do this change.

According to that scheduler PR even if scheduler use Update method on Pod in the future, updating Pod with managedFields being nil has no impact as kube-apiserver calculates the managed fields from the live object so I do think this is safe.

[sbueringer]

Nice! Thx for testing! So there is special handling for nil. I assume if some managedfields are set/modified it would update accordingly
(As mentioned in Cluster API we are using only patch, with patch we were able to modify managed fields)