Merge pull request #110058 from glebiller/managed-fields-time

Update managedFields time when field value is modified

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

@@ -45,7 +45,6 @@ func NewManagedFieldsUpdater(fieldManager Manager) Manager {
 // Update implements Manager.
 func (f *managedFieldsUpdater) Update(liveObj, newObj runtime.Object, managed Managed, manager string) (runtime.Object, Managed, error) {
 	self := "current-operation"
-	formerSet := managed.Fields()[manager]
 	object, managed, err := f.fieldManager.Update(liveObj, newObj, managed, self)
 	if err != nil {
 		return object, managed, err
@@ -61,10 +60,8 @@ func (f *managedFieldsUpdater) Update(liveObj, newObj runtime.Object, managed Ma
 		} else {
 			managed.Fields()[manager] = vs
 		}
-		// Update the time only if the manager's fieldSet has changed.
+
-		if formerSet == nil || !managed.Fields()[manager].Set().Equals(formerSet.Set()) {
+		managed.Times()[manager] = &metav1.Time{Time: time.Now().UTC()}
-			managed.Times()[manager] = &metav1.Time{Time: time.Now().UTC()}
-		}
 	}
 
 	return object, managed, nil
@@ -72,15 +69,13 @@ func (f *managedFieldsUpdater) Update(liveObj, newObj runtime.Object, managed Ma
 
 // Apply implements Manager.
 func (f *managedFieldsUpdater) Apply(liveObj, appliedObj runtime.Object, managed Managed, fieldManager string, force bool) (runtime.Object, Managed, error) {
-	formerManaged := managed.Fields().Copy()
 	object, managed, err := f.fieldManager.Apply(liveObj, appliedObj, managed, fieldManager, force)
 	if err != nil {
 		return object, managed, err
 	}
-	if object != nil || !managed.Fields().Equals(formerManaged) {
+	if object != nil {
 		managed.Times()[fieldManager] = &metav1.Time{Time: time.Now().UTC()}
-	}
+	} else {
-	if object == nil {
 		object = liveObj.DeepCopyObject()
 		internal.RemoveObjectManagedFields(object)
 	}

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

@@ -17,6 +17,8 @@ limitations under the License.
 package fieldmanager
 
 import (
+	"fmt"
+	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"reflect"
 	"testing"
 	"time"
@@ -29,42 +31,395 @@ import (
 	"sigs.k8s.io/yaml"
 )
 
-func TestNoManagedFieldsUpdateDoesntUpdateTime(t *testing.T) {
+func TestManagedFieldsUpdateDoesModifyTime(t *testing.T) {
-	f := NewTestFieldManager(schema.FromAPIVersionAndKind("v1", "Pod"), "", nil)
+	var err error
+	f := NewTestFieldManager(schema.FromAPIVersionAndKind("v1", "ConfigMap"), "", nil)
 
-	obj := &unstructured.Unstructured{Object: map[string]interface{}{}}
+	err = updateObject(&f, "fieldmanager_test", []byte(`{
-	if err := yaml.Unmarshal([]byte(`{
 		"apiVersion": "v1",
-		"kind": "Pod",
+		"kind": "ConfigMap",
 		"metadata": {
-			"name": "pod",
+			"name": "configmap"
-			"labels": {"app": "nginx"}
 		},
-	}`), &obj.Object); err != nil {
+		"data": {
-		t.Fatalf("error decoding YAML: %v", err)
+			"key": "value"
+		}
+	}`))
+	if err != nil {
+		t.Fatal(err)
 	}
+	previousManagedFields := f.ManagedFields()
 
-	if err := f.Update(obj, "fieldmanager_test"); err != nil {
-		t.Fatalf("failed to update object: %v", err)
-	}
-	managed := f.ManagedFields()
-	obj2 := &unstructured.Unstructured{Object: map[string]interface{}{}}
-	if err := yaml.Unmarshal([]byte(`{
-		"apiVersion": "v1",
-		"kind": "Pod",
-		"metadata": {
-			"name": "pod",
-			"labels": {"app": "nginx2"}
-		},
-	}`), &obj2.Object); err != nil {
-		t.Fatalf("error decoding YAML: %v", err)
-	}
 	time.Sleep(time.Second)
-	if err := f.Update(obj2, "fieldmanager_test"); err != nil {
+
-		t.Fatalf("failed to update object: %v", err)
+	err = updateObject(&f, "fieldmanager_test", []byte(`{
+		"apiVersion": "v1",
+		"kind": "ConfigMap",
+		"metadata": {
+			"name": "configmap"
+		},
+		"data": {
+			"key": "new-value"
+		}
+	}`))
+	if err != nil {
+		t.Fatal(err)
 	}
-	if !reflect.DeepEqual(managed, f.ManagedFields()) {
+	newManagedFields := f.ManagedFields()
-		t.Errorf("ManagedFields changed:\nBefore:\n%v\nAfter:\n%v", managed, f.ManagedFields())
+
+	if previousManagedFields[0].Time.Equal(newManagedFields[0].Time) {
+		t.Errorf("ManagedFields time has not been updated:\n%v", newManagedFields)
+	}
+}
+
+func TestManagedFieldsApplyDoesModifyTime(t *testing.T) {
+	var err error
+	f := NewTestFieldManager(schema.FromAPIVersionAndKind("v1", "ConfigMap"), "", nil)
+
+	err = applyObject(&f, "fieldmanager_test", []byte(`{
+		"apiVersion": "v1",
+		"kind": "ConfigMap",
+		"metadata": {
+			"name": "configmap"
+		},
+		"data": {
+			"key": "value"
+		}
+	}`))
+	if err != nil {
+		t.Fatal(err)
+	}
+	previousManagedFields := f.ManagedFields()
+
+	time.Sleep(time.Second)
+
+	err = applyObject(&f, "fieldmanager_test", []byte(`{
+		"apiVersion": "v1",
+		"kind": "ConfigMap",
+		"metadata": {
+			"name": "configmap"
+		},
+		"data": {
+			"key": "new-value"
+		}
+	}`))
+	if err != nil {
+		t.Fatal(err)
+	}
+	newManagedFields := f.ManagedFields()
+
+	if previousManagedFields[0].Time.Equal(newManagedFields[0].Time) {
+		t.Errorf("ManagedFields time has not been updated:\n%v", newManagedFields)
+	}
+}
+
+func TestManagedFieldsUpdateWithoutChangesDoesNotModifyTime(t *testing.T) {
+	var err error
+	f := NewTestFieldManager(schema.FromAPIVersionAndKind("v1", "ConfigMap"), "", nil)
+
+	err = updateObject(&f, "fieldmanager_test", []byte(`{
+		"apiVersion": "v1",
+		"kind": "ConfigMap",
+		"metadata": {
+			"name": "configmap"
+		},
+		"data": {
+			"key": "value"
+		}
+	}`))
+	if err != nil {
+		t.Fatal(err)
+	}
+	previousManagedFields := f.ManagedFields()
+
+	time.Sleep(time.Second)
+
+	err = updateObject(&f, "fieldmanager_test", []byte(`{
+		"apiVersion": "v1",
+		"kind": "ConfigMap",
+		"metadata": {
+			"name": "configmap"
+		},
+		"data": {
+			"key": "value"
+		}
+	}`))
+	if err != nil {
+		t.Fatal(err)
+	}
+	newManagedFields := f.ManagedFields()
+
+	if !previousManagedFields[0].Time.Equal(newManagedFields[0].Time) {
+		t.Errorf("ManagedFields time has changed:\nBefore:\n%v\nAfter:\n%v", previousManagedFields, newManagedFields)
+	}
+}
+
+func TestManagedFieldsApplyWithoutChangesDoesNotModifyTime(t *testing.T) {
+	var err error
+	f := NewTestFieldManager(schema.FromAPIVersionAndKind("v1", "ConfigMap"), "", nil)
+
+	err = applyObject(&f, "fieldmanager_test", []byte(`{
+		"apiVersion": "v1",
+		"kind": "ConfigMap",
+		"metadata": {
+			"name": "configmap"
+		},
+		"data": {
+			"key": "value"
+		}
+	}`))
+	if err != nil {
+		t.Fatal(err)
+	}
+	previousManagedFields := f.ManagedFields()
+
+	time.Sleep(time.Second)
+
+	err = applyObject(&f, "fieldmanager_test", []byte(`{
+		"apiVersion": "v1",
+		"kind": "ConfigMap",
+		"metadata": {
+			"name": "configmap"
+		},
+		"data": {
+			"key": "value"
+		}
+	}`))
+	if err != nil {
+		t.Fatal(err)
+	}
+	newManagedFields := f.ManagedFields()
+
+	if !previousManagedFields[0].Time.Equal(newManagedFields[0].Time) {
+		t.Errorf("ManagedFields time has changed:\nBefore:\n%v\nAfter:\n%v", previousManagedFields, newManagedFields)
+	}
+}
+
+func TestNonManagedFieldsUpdateDoesNotModifyTime(t *testing.T) {
+	var err error
+	f := NewTestFieldManager(schema.FromAPIVersionAndKind("v1", "ConfigMap"), "", nil)
+
+	err = updateObject(&f, "fieldmanager_a_test", []byte(`{
+		"apiVersion": "v1",
+		"kind": "ConfigMap",
+		"metadata": {
+			"name": "configmap"
+		},
+		"data": {
+			"key_a": "value"
+		}
+	}`))
+	if err != nil {
+		t.Fatal(err)
+	}
+	err = updateObject(&f, "fieldmanager_b_test", []byte(`{
+		"apiVersion": "v1",
+		"kind": "ConfigMap",
+		"metadata": {
+			"name": "configmap"
+		},
+		"data": {
+			"key_b": "value"
+		}
+	}`))
+	if err != nil {
+		t.Fatal(err)
+	}
+	previousManagedFields := f.ManagedFields()
+	previousEntries := map[string]v1.ManagedFieldsEntry{}
+	for _, entry := range previousManagedFields {
+		previousEntries[entry.Manager] = entry
+	}
+
+	time.Sleep(time.Second)
+
+	err = updateObject(&f, "fieldmanager_a_test", []byte(`{
+		"apiVersion": "v1",
+		"kind": "ConfigMap",
+		"metadata": {
+			"name": "configmap"
+		},
+		"data": {
+			"key_a": "value",
+			"key_b": "new-value"
+		}
+	}`))
+	if err != nil {
+		t.Fatal(err)
+	}
+	newManagedFields := f.ManagedFields()
+	newEntries := map[string]v1.ManagedFieldsEntry{}
+	for _, entry := range newManagedFields {
+		newEntries[entry.Manager] = entry
+	}
+
+	if _, ok := newEntries["fieldmanager_b_test"]; ok {
+		t.Errorf("FieldManager B ManagedFields has changed:\n%v", newEntries["fieldmanager_b_test"])
+	}
+}
+
+func TestNonManagedFieldsApplyDoesNotModifyTime(t *testing.T) {
+	var err error
+	f := NewTestFieldManager(schema.FromAPIVersionAndKind("v1", "ConfigMap"), "", nil)
+
+	err = applyObject(&f, "fieldmanager_a_test", []byte(`{
+		"apiVersion": "v1",
+		"kind": "ConfigMap",
+		"metadata": {
+			"name": "configmap"
+		},
+		"data": {
+			"key_a": "value"
+		}
+	}`))
+	if err != nil {
+		t.Fatal(err)
+	}
+	err = applyObject(&f, "fieldmanager_b_test", []byte(`{
+		"apiVersion": "v1",
+		"kind": "ConfigMap",
+		"metadata": {
+			"name": "configmap"
+		},
+		"data": {
+			"key_b": "value"
+		}
+	}`))
+	if err != nil {
+		t.Fatal(err)
+	}
+	previousManagedFields := f.ManagedFields()
+	previousEntries := map[string]v1.ManagedFieldsEntry{}
+	for _, entry := range previousManagedFields {
+		previousEntries[entry.Manager] = entry
+	}
+
+	time.Sleep(time.Second)
+
+	err = applyObject(&f, "fieldmanager_a_test", []byte(`{
+		"apiVersion": "v1",
+		"kind": "ConfigMap",
+		"metadata": {
+			"name": "configmap"
+		},
+		"data": {
+			"key_a": "new-value"
+		}
+	}`))
+	if err != nil {
+		t.Fatal(err)
+	}
+	newManagedFields := f.ManagedFields()
+	newEntries := map[string]v1.ManagedFieldsEntry{}
+	for _, entry := range newManagedFields {
+		newEntries[entry.Manager] = entry
+	}
+
+	if !previousEntries["fieldmanager_b_test"].Time.Equal(newEntries["fieldmanager_b_test"].Time) {
+		t.Errorf("FieldManager B ManagedFields time changed:\nBefore:\n%v\nAfter:\n%v",
+			previousEntries["fieldmanager_b_test"], newEntries["fieldmanager_b_test"])
+	}
+}
+
+func TestTakingOverManagedFieldsDuringUpdateDoesNotModifyPreviousManagerTime(t *testing.T) {
+	var err error
+	f := NewTestFieldManager(schema.FromAPIVersionAndKind("v1", "ConfigMap"), "", nil)
+
+	err = updateObject(&f, "fieldmanager_a_test", []byte(`{
+		"apiVersion": "v1",
+		"kind": "ConfigMap",
+		"metadata": {
+			"name": "configmap"
+		},
+		"data": {
+			"key_a": "value",
+			"key_b": value"
+		}
+	}`))
+	if err != nil {
+		t.Fatal(err)
+	}
+	previousManagedFields := f.ManagedFields()
+	previousEntries := map[string]v1.ManagedFieldsEntry{}
+	for _, entry := range previousManagedFields {
+		previousEntries[entry.Manager] = entry
+	}
+
+	time.Sleep(time.Second)
+
+	err = updateObject(&f, "fieldmanager_b_test", []byte(`{
+		"apiVersion": "v1",
+		"kind": "ConfigMap",
+		"metadata": {
+			"name": "configmap"
+		},
+		"data": {
+			"key_b": "new-value"
+		}
+	}`))
+	if err != nil {
+		t.Fatal(err)
+	}
+	newManagedFields := f.ManagedFields()
+	newEntries := map[string]v1.ManagedFieldsEntry{}
+	for _, entry := range newManagedFields {
+		newEntries[entry.Manager] = entry
+	}
+
+	if !previousEntries["fieldmanager_a_test"].Time.Equal(newEntries["fieldmanager_a_test"].Time) {
+		t.Errorf("FieldManager A ManagedFields time has been updated:\nBefore:\n%v\nAfter:\n%v",
+			previousEntries["fieldmanager_a_test"], newEntries["fieldmanager_a_test"])
+	}
+}
+
+func TestTakingOverManagedFieldsDuringApplyDoesNotModifyPreviousManagerTime(t *testing.T) {
+	var err error
+	f := NewTestFieldManager(schema.FromAPIVersionAndKind("v1", "ConfigMap"), "", nil)
+
+	err = applyObject(&f, "fieldmanager_a_test", []byte(`{
+		"apiVersion": "v1",
+		"kind": "ConfigMap",
+		"metadata": {
+			"name": "configmap"
+		},
+		"data": {
+			"key_a": "value",
+			"key_b": value"
+		}
+	}`))
+	if err != nil {
+		t.Fatal(err)
+	}
+	previousManagedFields := f.ManagedFields()
+	previousEntries := map[string]v1.ManagedFieldsEntry{}
+	for _, entry := range previousManagedFields {
+		previousEntries[entry.Manager] = entry
+	}
+
+	time.Sleep(time.Second)
+
+	err = applyObject(&f, "fieldmanager_b_test", []byte(`{
+		"apiVersion": "v1",
+		"kind": "ConfigMap",
+		"metadata": {
+			"name": "configmap"
+		},
+		"data": {
+			"key_b": "new-value"
+		}
+	}`))
+	if err != nil {
+		t.Fatal(err)
+	}
+	newManagedFields := f.ManagedFields()
+	newEntries := map[string]v1.ManagedFieldsEntry{}
+	for _, entry := range newManagedFields {
+		newEntries[entry.Manager] = entry
+	}
+
+	if !previousEntries["fieldmanager_a_test"].Time.Equal(newEntries["fieldmanager_a_test"].Time) {
+		t.Errorf("FieldManager A ManagedFields time has been updated:\nBefore:\n%v\nAfter:\n%v",
+			previousEntries["fieldmanager_a_test"], newEntries["fieldmanager_a_test"])
 	}
 }
 
@@ -78,6 +433,28 @@ func (NoopManager) Update(liveObj, newObj runtime.Object, managed Managed, manag
 	return nil, nil, nil
 }
 
+func updateObject(f *TestFieldManager, fieldManagerName string, object []byte) error {
+	obj := &unstructured.Unstructured{Object: map[string]interface{}{}}
+	if err := yaml.Unmarshal(object, &obj.Object); err != nil {
+		return fmt.Errorf("error decoding YAML: %v", err)
+	}
+	if err := f.Update(obj, fieldManagerName); err != nil {
+		return fmt.Errorf("failed to update object: %v", err)
+	}
+	return nil
+}
+
+func applyObject(f *TestFieldManager, fieldManagerName string, object []byte) error {
+	obj := &unstructured.Unstructured{Object: map[string]interface{}{}}
+	if err := yaml.Unmarshal(object, &obj.Object); err != nil {
+		return fmt.Errorf("error decoding YAML: %v", err)
+	}
+	if err := f.Apply(obj, fieldManagerName, true); err != nil {
+		return fmt.Errorf("failed to apply object: %v", err)
+	}
+	return nil
+}
+
 // Ensures that if ManagedFieldsUpdater gets a nil value from its nested manager
 // chain (meaning the operation was a no-op), then the ManagedFieldsUpdater
 // itself will return a copy of the input live object, with its managed fields