Group update managedFieldsEntries by manager name

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager.go

@@ -98,7 +98,7 @@ func (f *FieldManager) Update(liveObj, newObj runtime.Object, manager string) (r
 	// If the managed field is empty or we failed to decode it,
 	// let's try the live object. This is to prevent clients who
 	// don't understand managedFields from deleting it accidentally.
-	if err != nil || len(managed) == 0 {
+	if err != nil || len(managed.Fields) == 0 {
 		managed, err = internal.DecodeObjectManagedFields(liveObj)
 		if err != nil {
 			return nil, fmt.Errorf("failed to decode managed fields: %v", err)
@@ -133,11 +133,14 @@ func (f *FieldManager) Update(liveObj, newObj runtime.Object, manager string) (r
 	}
 
 	// TODO(apelisse) use the first return value when unions are implemented
-	_, managed, err = f.updater.Update(liveObjTyped, newObjTyped, apiVersion, managed, manager)
+	_, managed.Fields, err = f.updater.Update(liveObjTyped, newObjTyped, apiVersion, managed.Fields, manager)
 	if err != nil {
 		return nil, fmt.Errorf("failed to update ManagedFields: %v", err)
 	}
-	managed = f.stripFields(managed, manager)
+	managed.Fields = f.stripFields(managed.Fields, manager)
+
+	// Update the time in the managedFieldsEntry for this operation
+	managed.Times[manager] = &metav1.Time{Time: time.Now().UTC()}
 
 	if err := internal.EncodeObjectManagedFields(newObj, managed); err != nil {
 		return nil, fmt.Errorf("failed to encode managed fields: %v", err)
@@ -192,14 +195,17 @@ func (f *FieldManager) Apply(liveObj runtime.Object, patch []byte, fieldManager
 	}
 
 	apiVersion := fieldpath.APIVersion(f.groupVersion.String())
-	newObjTyped, managed, err := f.updater.Apply(liveObjTyped, patchObjTyped, apiVersion, managed, manager, force)
+	newObjTyped, managedFields, err := f.updater.Apply(liveObjTyped, patchObjTyped, apiVersion, managed.Fields, manager, force)
 	if err != nil {
 		if conflicts, ok := err.(merge.Conflicts); ok {
 			return nil, internal.NewConflictError(conflicts)
 		}
 		return nil, err
 	}
-	managed = f.stripFields(managed, manager)
+	managed.Fields = f.stripFields(managedFields, manager)
+
+	// Update the time in the managedFieldsEntry for this operation
+	managed.Times[manager] = &metav1.Time{Time: time.Now().UTC()}
 
 	newObj, err := f.typeConverter.TypedToObject(newObjTyped)
 	if err != nil {
@@ -236,7 +242,6 @@ func (f *FieldManager) buildManagerInfo(prefix string, operation metav1.ManagedF
 		Manager:    prefix,
 		Operation:  operation,
 		APIVersion: f.groupVersion.String(),
-		Time:       &metav1.Time{Time: time.Now().UTC()},
 	}
 	if managerInfo.Manager == "" {
 		managerInfo.Manager = "unknown"

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/managedfields.go

@@ -28,6 +28,12 @@ import (
 	"sigs.k8s.io/structured-merge-diff/fieldpath"
 )
 
+// Managed groups a fieldpath.ManagedFields together with the timestamps associated with each operation.
+type Managed struct {
+	Fields fieldpath.ManagedFields
+	Times  map[string]*metav1.Time
+}
+
 // RemoveObjectManagedFields removes the ManagedFields from the object
 // before we merge so that it doesn't appear in the ManagedFields
 // recursively.
@@ -40,9 +46,9 @@ func RemoveObjectManagedFields(obj runtime.Object) {
 }
 
 // DecodeObjectManagedFields extracts and converts the objects ManagedFields into a fieldpath.ManagedFields.
-func DecodeObjectManagedFields(from runtime.Object) (fieldpath.ManagedFields, error) {
+func DecodeObjectManagedFields(from runtime.Object) (Managed, error) {
 	if from == nil {
-		return fieldpath.ManagedFields{}, nil
+		return Managed{}, nil
 	}
 	accessor, err := meta.Accessor(from)
 	if err != nil {
@@ -51,42 +57,44 @@ func DecodeObjectManagedFields(from runtime.Object) (fieldpath.ManagedFields, er
 
 	managed, err := decodeManagedFields(accessor.GetManagedFields())
 	if err != nil {
-		return nil, fmt.Errorf("failed to convert managed fields from API: %v", err)
+		return Managed{}, fmt.Errorf("failed to convert managed fields from API: %v", err)
 	}
 	return managed, err
 }
 
 // EncodeObjectManagedFields converts and stores the fieldpathManagedFields into the objects ManagedFields
-func EncodeObjectManagedFields(obj runtime.Object, fields fieldpath.ManagedFields) error {
+func EncodeObjectManagedFields(obj runtime.Object, managed Managed) error {
 	accessor, err := meta.Accessor(obj)
 	if err != nil {
 		panic(fmt.Sprintf("couldn't get accessor: %v", err))
 	}
 
-	managed, err := encodeManagedFields(fields)
+	encodedManagedFields, err := encodeManagedFields(managed)
 	if err != nil {
 		return fmt.Errorf("failed to convert back managed fields to API: %v", err)
 	}
-	accessor.SetManagedFields(managed)
+	accessor.SetManagedFields(encodedManagedFields)
 
 	return nil
 }
 
 // decodeManagedFields converts ManagedFields from the wire format (api format)
 // to the format used by sigs.k8s.io/structured-merge-diff
-func decodeManagedFields(encodedManagedFields []metav1.ManagedFieldsEntry) (managedFields fieldpath.ManagedFields, err error) {
+func decodeManagedFields(encodedManagedFields []metav1.ManagedFieldsEntry) (managed Managed, err error) {
-	managedFields = make(fieldpath.ManagedFields, len(encodedManagedFields))
+	managed.Fields = make(fieldpath.ManagedFields, len(encodedManagedFields))
+	managed.Times = make(map[string]*metav1.Time, len(encodedManagedFields))
 	for _, encodedVersionedSet := range encodedManagedFields {
 		manager, err := BuildManagerIdentifier(&encodedVersionedSet)
 		if err != nil {
-			return nil, fmt.Errorf("error decoding manager from %v: %v", encodedVersionedSet, err)
+			return Managed{}, fmt.Errorf("error decoding manager from %v: %v", encodedVersionedSet, err)
 		}
-		managedFields[manager], err = decodeVersionedSet(&encodedVersionedSet)
+		managed.Fields[manager], err = decodeVersionedSet(&encodedVersionedSet)
 		if err != nil {
-			return nil, fmt.Errorf("error decoding versioned set from %v: %v", encodedVersionedSet, err)
+			return Managed{}, fmt.Errorf("error decoding versioned set from %v: %v", encodedVersionedSet, err)
 		}
+		managed.Times[manager] = encodedVersionedSet.Time
 	}
-	return managedFields, nil
+	return managed, nil
 }
 
 // BuildManagerIdentifier creates a manager identifier string from a ManagedFieldsEntry
@@ -96,11 +104,13 @@ func BuildManagerIdentifier(encodedManager *metav1.ManagedFieldsEntry) (manager
 	// Never include the fields in the manager identifier
 	encodedManagerCopy.Fields = nil
 
-	// For appliers, don't include the APIVersion or Time in the manager identifier,
+	// Never include the time in the manager identifier
+	encodedManagerCopy.Time = nil
+
+	// For appliers, don't include the APIVersion in the manager identifier,
 	// so it will always have the same manager identifier each time it applied.
 	if encodedManager.Operation == metav1.ManagedFieldsOperationApply {
 		encodedManagerCopy.APIVersion = ""
-		encodedManagerCopy.Time = nil
 	}
 
 	// Use the remaining fields to build the manager identifier
@@ -126,21 +136,17 @@ func decodeVersionedSet(encodedVersionedSet *metav1.ManagedFieldsEntry) (version
 
 // encodeManagedFields converts ManagedFields from the format used by
 // sigs.k8s.io/structured-merge-diff to the wire format (api format)
-func encodeManagedFields(managedFields fieldpath.ManagedFields) (encodedManagedFields []metav1.ManagedFieldsEntry, err error) {
+func encodeManagedFields(managed Managed) (encodedManagedFields []metav1.ManagedFieldsEntry, err error) {
-	// Sort the keys so a predictable order will be used.
-	managers := []string{}
-	for manager := range managedFields {
-		managers = append(managers, manager)
-	}
-	sort.Strings(managers)
-
 	encodedManagedFields = []metav1.ManagedFieldsEntry{}
-	for _, manager := range managers {
+	for manager := range managed.Fields {
-		versionedSet := managedFields[manager]
+		versionedSet := managed.Fields[manager]
 		v, err := encodeManagerVersionedSet(manager, versionedSet)
 		if err != nil {
 			return nil, fmt.Errorf("error encoding versioned set for %v: %v", manager, err)
 		}
+		if t, ok := managed.Times[manager]; ok {
+			v.Time = t
+		}
 		encodedManagedFields = append(encodedManagedFields, *v)
 	}
 	return sortEncodedManagedFields(encodedManagedFields)

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/managedfields_test.go

@@ -169,7 +169,7 @@ manager: foo
 operation: Update
 time: "2001-02-03T04:05:06Z"
 `,
-			expected: "{\"manager\":\"foo\",\"operation\":\"Update\",\"apiVersion\":\"v1\",\"time\":\"2001-02-03T04:05:06Z\"}",
+			expected: "{\"manager\":\"foo\",\"operation\":\"Update\",\"apiVersion\":\"v1\"}",
 		},
 		{
 			managedFieldsEntry: `

test/integration/apiserver/apply/apply_test.go

@@ -291,6 +291,21 @@ func TestApplyManagedFields(t *testing.T) {
 		t.Fatalf("Failed to create object using Apply patch: %v", err)
 	}
 
+	_, err = client.CoreV1().RESTClient().Patch(types.MergePatchType).
+		Namespace("default").
+		Resource("configmaps").
+		Name("test-cm").
+		Param("fieldManager", "updater").
+		Body([]byte(`{"data":{"new-key": "value"}}`)).Do().Get()
+	if err != nil {
+		t.Fatalf("Failed to patch object: %v", err)
+	}
+
+	// Sleep for one second to make sure that the times of each update operation is different.
+	// This will let us check that update entries with the same manager name are grouped together,
+	// and that the most recent update time is recorded in the grouped entry.
+	time.Sleep(1 * time.Second)
+
 	_, err = client.CoreV1().RESTClient().Patch(types.MergePatchType).
 		Namespace("default").
 		Resource("configmaps").
@@ -332,6 +347,7 @@ func TestApplyManagedFields(t *testing.T) {
 					"manager": "apply_test",
 					"operation": "Apply",
 					"apiVersion": "v1",
+					"time": "` + accessor.GetManagedFields()[0].Time.UTC().Format(time.RFC3339) + `",
 					"fields": {
 						"f:metadata": {
 							"f:labels": {
@@ -347,20 +363,26 @@ func TestApplyManagedFields(t *testing.T) {
 					"time": "` + accessor.GetManagedFields()[1].Time.UTC().Format(time.RFC3339) + `",
 					"fields": {
 						"f:data": {
-							"f:key": {}
+							"f:key": {},
+							"f:new-key": {}
 						}
 					}
 				}
 			]
 		},
 		"data": {
-			"key": "new value"
+			"key": "new value",
+			"new-key": "value"
 		}
 	}`)
 
 	if string(expected) != string(actual) {
 		t.Fatalf("Expected:\n%v\nGot:\n%v", string(expected), string(actual))
 	}
+
+	if accessor.GetManagedFields()[0].Time.UTC().Format(time.RFC3339) == accessor.GetManagedFields()[1].Time.UTC().Format(time.RFC3339) {
+		t.Fatalf("Expected times to be different but got:\n%v", string(actual))
+	}
 }
 
 // TestApplyRemovesEmptyManagedFields there are no empty managers in managedFields