Merge pull request #61847 from mengqiy/patch_conflict

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

fix patch conflict detection in apiserver

Patching conflict for merging list with mergeKey is not determined in the correct way. 

```release-note
None
```

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

@@ -330,7 +330,11 @@ func patchResource(
 				return nil, err
 			}
 
-			hasConflicts, err := mergepatch.HasConflicts(originalPatchMap, currentPatchMap)
+			patchMetaFromStruct, err := strategicpatch.NewPatchMetaFromStruct(versionedObj)
+			if err != nil {
+				return nil, err
+			}
+			hasConflicts, err := strategicpatch.MergingMapsHaveConflicts(originalPatchMap, currentPatchMap, patchMetaFromStruct)
 			if err != nil {
 				return nil, err
 			}

test/integration/apiserver/patch_test.go

@@ -24,8 +24,6 @@ import (
 
 	"github.com/pborman/uuid"
 
-	"reflect"
-
 	"k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
@@ -43,14 +41,30 @@ func TestPatchConflicts(t *testing.T) {
 	ns := framework.CreateTestingNamespace("status-code", s, t)
 	defer framework.DeleteTestingNamespace(ns, s, t)
 
-	// Create the object we're going to conflict on
+	numOfConcurrentPatches := 2 * handlers.MaxRetryWhenPatchConflicts
-	clientSet.CoreV1().Secrets(ns.Name).Create(&v1.Secret{
+
+	UIDs := make([]types.UID, numOfConcurrentPatches)
+	ownerRefs := []metav1.OwnerReference{}
+	for i := 0; i < numOfConcurrentPatches; i++ {
+		uid := types.UID(uuid.NewRandom().String())
+		ownerName := fmt.Sprintf("owner-%d", i)
+		UIDs[i] = uid
+		ownerRefs = append(ownerRefs, metav1.OwnerReference{
+			APIVersion: "example.com/v1",
+			Kind:       "Foo",
+			Name:       ownerName,
+			UID:        uid,
+		})
+	}
+	secret := &v1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
-			Name: "test",
+			Name:            "test",
-			// Populate annotations so the strategic patch descends, compares, and notices the $patch directive
+			OwnerReferences: ownerRefs,
-			Annotations: map[string]string{"initial": "value"},
 		},
-	})
+	}
+
+	// Create the object we're going to conflict on
+	clientSet.CoreV1().Secrets(ns.Name).Create(secret)
 	client := clientSet.CoreV1().RESTClient()
 
 	successes := int32(0)
@@ -60,11 +74,10 @@ func TestPatchConflicts(t *testing.T) {
 	// If the resource version of the object changed between attempts, that means another one of our patch requests succeeded.
 	// That means if we run 2*MaxRetryWhenPatchConflicts patch attempts, we should see at least MaxRetryWhenPatchConflicts succeed.
 	wg := sync.WaitGroup{}
-	for i := 0; i < (2 * handlers.MaxRetryWhenPatchConflicts); i++ {
+	for i := 0; i < numOfConcurrentPatches; i++ {
 		wg.Add(1)
 		go func(i int) {
 			defer wg.Done()
-			annotationName := fmt.Sprintf("annotation-%d", i)
 			labelName := fmt.Sprintf("label-%d", i)
 			value := uuid.NewRandom().String()
 
@@ -72,7 +85,7 @@ func TestPatchConflicts(t *testing.T) {
 				Namespace(ns.Name).
 				Resource("secrets").
 				Name("test").
-				Body([]byte(fmt.Sprintf(`{"metadata":{"labels":{"%s":"%s"}, "annotations":{"$patch":"replace","%s":"%s"}}}`, labelName, value, annotationName, value))).
+				Body([]byte(fmt.Sprintf(`{"metadata":{"labels":{"%s":"%s"}, "ownerReferences":[{"$patch":"delete","uid":"%s"}]}}`, labelName, value, UIDs[i]))).
 				Do().
 				Get()
 
@@ -95,9 +108,14 @@ func TestPatchConflicts(t *testing.T) {
 				t.Errorf("patch of %s was ineffective, expected %s=%s, got labels %#v", "secrets", labelName, value, accessor.GetLabels())
 				return
 			}
-			// make sure the patch directive didn't get lost, and that the entire annotation map was replaced
+			// make sure the patch directive didn't get lost, and that an entry in the ownerReference list was deleted.
-			if !reflect.DeepEqual(accessor.GetAnnotations(), map[string]string{annotationName: value}) {
+			found := findOwnerRefByUID(accessor.GetOwnerReferences(), UIDs[i])
-				t.Errorf("patch of %s with $patch directive was ineffective, didn't replace entire annotations map: %#v", "secrets", accessor.GetAnnotations())
+			if err != nil {
+				t.Errorf("%v", err)
+				return
+			}
+			if found {
+				t.Errorf("patch of %s with $patch directive was ineffective, didn't delete the entry in the ownerReference slice: %#v", "secrets", UIDs[i])
 			}
 
 			atomic.AddInt32(&successes, 1)
@@ -112,3 +130,12 @@ func TestPatchConflicts(t *testing.T) {
 	}
 
 }
+
+func findOwnerRefByUID(ownerRefs []metav1.OwnerReference, uid types.UID) bool {
+	for _, of := range ownerRefs {
+		if of.UID == uid {
+			return true
+		}
+	}
+	return false
+}