I'm not very familiar with this code, but it looks strange to put this logic in the branch that is calling RemoveObjectManagedFields ... are we sure this is the branch where a new manager is adding co-ownership of some fields?

this definitely needs really good unit tests around the scenario being exercised

Thanks for the prompt review!

I have the same feeling that the two branches are a little confusing to follow. Let me try my best to clarify.

• Since managedFieldsUpdater implements the Manager interface, its method (f *managedFieldsUpdater) Apply should return ‘the new object with managedFields removed, and the object's new proposed managedFields separately’:
  

  kubernetes/staging/src/k8s.io/apimachinery/pkg/util/managedfields/internal/manager.go

  Lines 45 to 51 in 7ee17ce

  	// Apply is used when server-side apply is called, as it merges the
  	// object and updates the managed fields.
  	// * `liveObj` is not mutated by this function
  	// * `newObj` may be mutated by this function
  	// Returns the new object with managedFields removed, and the object's new
  	// proposed managedFields separately.
  	Apply(liveObj, appliedObj runtime.Object, managed Managed, fieldManager string, force bool) (runtime.Object, Managed, error)

• The else branch says: Since the live object is not touched (signaled by object == nil, detailed below), the merged object should be the same as the live object. So first make a deep copy of liveObj, then remove its managedFields to fit the Manager interface.

Details on object == nil:
These lines say that nil signals the situation that a merged object and the current live object being the same.

BTW I believe there was a typo in the go doc comments of the Manager interface’s Apply method. Latest push fixed the typo.

Fully agree that we need unit tests here. Will work on that.

ok... I see object comes back as nil for no-ops

constructs this:

leaving returnInputOnNoop set to false, so an apply that doesn't change anything returns nil for the object:

I think the logic in this PR is correct, but needs commenting to explain.

I'd suggest:

	if object != nil {
		// non-nil object means the apply operation modified the object, so update the manager's timestamp
		managed.Times()[fieldManager] = &metav1.Time{Time: time.Now().UTC()}
	} else {
		// nil object means the apply operation did not modify the input object
		// clone the input object and return it without managed fields
		object = liveObj.DeepCopyObject()
		RemoveObjectManagedFields(object)

		if _, managerInFields := managed.Fields()[fieldManager]; managerInFields {
			if _, managerInTimes := managed.Times()[fieldManager]; !managerInTimes {
				// if the manager owns fields, ensure it has an associated time.
				// a no-op apply can add co-ownership of existing field values, so record the time that occurred.
				managed.Times()[fieldManager] = &metav1.Time{Time: time.Now().UTC()}
			}
		}
}

Thanks for the suggestions! With the comments, we should save some time for the future visitors. I included them in latest push.

huh... these pre-existing test typos are distressing... did you just notice these and fix them while you were here, or did fixing these reveal that these tests were broken?

non-blocking for this PR, but cc @jpbetz for visibility to how accepting yaml parsing is of malformed json input sent as yaml. This was parsing as:

{
  "apiVersion": "v1",
  "data": {
    "key_a": "value",
    "key_b": "value\""
  },
  "kind": "ConfigMap",
  "metadata": {
    "name": "configmap"
  }
}

The former, I just noticed these so fixed them. I was wondering why these typos didn't break the testing, and now I get the answer from your explanations above. Thanks!

@jpbetz how does partitioning of field manager among subresources work? does managed.Fields() only return entries relevant to the subresource for this request?

The field manager relies on the GetResetFields() (example) of the subresource to indicate which fields the subresource applies to.

'GetResetFieldsFilter()' was also added a in 1.32 to make it easier to declare the fields (example) for cases where the exclude set of GetResetFields() is not expressive enough.

The specific test I'm thinking of is:

1. manager foo writes to pod, gets a managedFields entry for itself
2. manager bar writes to pods/status, gets a managedFields entry for itself under the status subresource
3. manager bar does no-op write to pod (e.g. empty patch {}) so that it doesn't actually own any fields

Will the change in this PR make processing of step 3 see and get confused by the managed fields entry from 2?

@jpbetz for existing managed entries that got created without timestamps, once this bug fix releases, the next no-op update will set the time to now on a pre-existing managed field entry. That's a little weird, and probably ok, but I wanted to call it out explicitly as a side-effect of the fix.

We fixed a few spurious resourceVersion bumps issues this year:

• Fix non-semantic apply requests to ignore empty maps #125317
• Fix check to ignore non-semantic changes to objects to handle unstructured #125263

So I'm keeping an eye out of anything that changes an apply request before the object is compared with the stored resource to decide if a write is needed. But this happens after fieldmanager.Apply() so appears safe, at least from that perspective.