chore: patch CR metadata without owning spec (#10264)

(cherry picked from commit 7e2fd5a)

Author: weicao

controllers/apps/componentversion_controller.go

@@ -265,8 +265,9 @@ func (r *ComponentVersionReconciler) supportedServiceVersions(compVersion *appsv
 	return strings.Join(versions, ",") // TODO(API): service versions length
 }
 
-func (r *ComponentVersionReconciler) updateSupportedCompDefLabels(cli client.Client, rctx intctrlutil.RequestCtx,
+func (r *ComponentVersionReconciler) updateSupportedCompDefLabels(cli client.Writer, rctx intctrlutil.RequestCtx,
 	compVersion *appsv1.ComponentVersion, releaseToCompDefinitions map[string]map[string]*appsv1.ComponentDefinition) error {
+	patch := client.MergeFromWithOptions(compVersion.DeepCopy(), client.MergeFromWithOptimisticLock{})
 	if compVersion.Annotations == nil {
 		compVersion.Annotations = make(map[string]string)
 	}
@@ -294,7 +295,7 @@ func (r *ComponentVersionReconciler) updateSupportedCompDefLabels(cli client.Cli
 	}
 	compVersion.Labels = labels
 	compVersion.Annotations[compatibleDefinitionsKey] = strings.Join(labelKeys, ",")
-	return cli.Update(rctx.Ctx, compVersion)
+	return cli.Patch(rctx.Ctx, compVersion, patch)
 }
 
 func (r *ComponentVersionReconciler) validate(compVersion *appsv1.ComponentVersion,

controllers/apps/componentversion_controller_test.go

@@ -20,21 +20,120 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
 package apps
 
 import (
+	"context"
 	"strings"
+	"testing"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 
 	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	appsv1 "github.com/apecloud/kubeblocks/apis/apps/v1"
+	intctrlutil "github.com/apecloud/kubeblocks/pkg/controllerutil"
 	"github.com/apecloud/kubeblocks/pkg/generics"
 	testapps "github.com/apecloud/kubeblocks/pkg/testutil/apps"
 )
 
+type componentVersionPatchWriter struct {
+	patchCalls  int
+	updateCalls int
+	patchData   []byte
+}
+
+func (w *componentVersionPatchWriter) Create(context.Context, client.Object, ...client.CreateOption) error {
+	return nil
+}
+
+func (w *componentVersionPatchWriter) Delete(context.Context, client.Object, ...client.DeleteOption) error {
+	return nil
+}
+
+func (w *componentVersionPatchWriter) Update(context.Context, client.Object, ...client.UpdateOption) error {
+	w.updateCalls++
+	return nil
+}
+
+func (w *componentVersionPatchWriter) Patch(_ context.Context, obj client.Object, patch client.Patch, _ ...client.PatchOption) error {
+	var err error
+	w.patchCalls++
+	w.patchData, err = patch.Data(obj)
+	return err
+}
+
+func (w *componentVersionPatchWriter) DeleteAllOf(context.Context, client.Object, ...client.DeleteAllOfOption) error {
+	return nil
+}
+
+func TestComponentVersionSupportedCompDefLabelsUsesMetadataPatch(t *testing.T) {
+	compVersion := &appsv1.ComponentVersion{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:            "mssql",
+			ResourceVersion: "7",
+			Labels: map[string]string{
+				"old-comp-def": "old-comp-def",
+				"chart-label":  "keep",
+			},
+			Annotations: map[string]string{
+				compatibleDefinitionsKey: "old-comp-def",
+			},
+		},
+		Spec: appsv1.ComponentVersionSpec{
+			Releases: []appsv1.ComponentVersionRelease{{
+				Name:           "mssql-2022",
+				ServiceVersion: "16.0.0",
+				Images: map[string]string{
+					"mssql": "mssql:old",
+				},
+			}},
+		},
+	}
+	writer := &componentVersionPatchWriter{}
+	releaseToCompDefinitions := map[string]map[string]*appsv1.ComponentDefinition{
+		"mssql-2022": {
+			"mssql-new": {ObjectMeta: metav1.ObjectMeta{Name: "mssql-new"}},
+		},
+	}
+
+	err := (&ComponentVersionReconciler{}).updateSupportedCompDefLabels(writer,
+		intctrlutil.RequestCtx{Ctx: context.Background()}, compVersion, releaseToCompDefinitions)
+	if err != nil {
+		t.Fatalf("updateSupportedCompDefLabels returned error: %v", err)
+	}
+	if writer.patchCalls != 1 {
+		t.Fatalf("expected one patch, got %d", writer.patchCalls)
+	}
+	if writer.updateCalls != 0 {
+		t.Fatalf("expected no whole-object update, got %d", writer.updateCalls)
+	}
+	if got := compVersion.Labels["mssql-new"]; got != "mssql-new" {
+		t.Fatalf("expected new compatible label, got %q", got)
+	}
+	if got := compVersion.Labels["chart-label"]; got != "keep" {
+		t.Fatalf("expected existing chart label to be preserved, got %q", got)
+	}
+	if _, ok := compVersion.Labels["old-comp-def"]; ok {
+		t.Fatalf("expected old compatible label to be removed")
+	}
+	if got := compVersion.Annotations[compatibleDefinitionsKey]; got != "mssql-new" {
+		t.Fatalf("expected compatible definitions annotation to be updated, got %q", got)
+	}
+	patchData := string(writer.patchData)
+	if !strings.Contains(patchData, `"metadata"`) {
+		t.Fatalf("expected metadata patch, got %s", patchData)
+	}
+	if strings.Contains(patchData, `"spec"`) {
+		t.Fatalf("metadata patch must not include spec, got %s", patchData)
+	}
+	if !strings.Contains(patchData, `"resourceVersion":"7"`) {
+		t.Fatalf("metadata patch must preserve optimistic-locking resourceVersion, got %s", patchData)
+	}
+}
+
 var _ = Describe("ComponentVersion Controller", func() {
 	var (
 		compVersionObj *appsv1.ComponentVersion

pkg/controllerutil/controller_common.go

@@ -105,7 +105,8 @@ func Requeue(logger logr.Logger, msg string, keysAndValues ...interface{}) (reco
 }
 
 // HandleCRDeletion handles CR deletion, adds finalizer if found a non-deleting object and removes finalizer during
-// deletion process. Passes optional 'deletionHandler' func for external dependency deletion. Returns Result pointer
+// deletion process. It patches only finalizer metadata so chart-owned spec fields keep their field ownership.
+// Passes optional 'deletionHandler' func for external dependency deletion. Returns Result pointer
 // if required to return out of outer 'Reconcile' reconciliation loop.
 func HandleCRDeletion(reqCtx RequestCtx,
 	r client.Writer,
@@ -118,8 +119,9 @@ func HandleCRDeletion(reqCtx RequestCtx,
 		// then add the finalizer and update the object. This is equivalent to
 		// registering our finalizer.
 		if !controllerutil.ContainsFinalizer(cr, finalizer) {
+			patch := client.MergeFromWithOptions(cr.DeepCopyObject().(client.Object), client.MergeFromWithOptimisticLock{})
 			controllerutil.AddFinalizer(cr, finalizer)
-			if err := r.Update(reqCtx.Ctx, cr); err != nil {
+			if err := r.Patch(reqCtx.Ctx, cr, patch); err != nil {
 				return ResultToP(CheckedRequeueWithError(err, reqCtx.Log, ""))
 			}
 		}
@@ -156,8 +158,9 @@ func HandleCRDeletion(reqCtx RequestCtx,
 				}
 			}
 			// remove our finalizer from the list and update it.
+			patch := client.MergeFromWithOptions(cr.DeepCopyObject().(client.Object), client.MergeFromWithOptimisticLock{})
 			if controllerutil.RemoveFinalizer(cr, finalizer) {
-				if err := r.Update(reqCtx.Ctx, cr); err != nil {
+				if err := r.Patch(reqCtx.Ctx, cr, patch); err != nil {
 					return ResultToP(CheckedRequeueWithError(err, reqCtx.Log, ""))
 				}
 				// record resources deleted event

pkg/controllerutil/controller_common_test.go

@@ -24,6 +24,7 @@ import (
 	"errors"
 	"fmt"
 	"reflect"
+	"strings"
 	"testing"
 	"time"
 
@@ -44,6 +45,53 @@ import (
 
 var tlog = ctrl.Log.WithName("controller_testing")
 
+type patchRecordingWriter struct {
+	patchCalls  int
+	updateCalls int
+	patchData   []byte
+}
+
+func (w *patchRecordingWriter) Create(context.Context, client.Object, ...client.CreateOption) error {
+	return nil
+}
+
+func (w *patchRecordingWriter) Delete(context.Context, client.Object, ...client.DeleteOption) error {
+	return nil
+}
+
+func (w *patchRecordingWriter) Update(context.Context, client.Object, ...client.UpdateOption) error {
+	w.updateCalls++
+	return nil
+}
+
+func (w *patchRecordingWriter) Patch(_ context.Context, obj client.Object, patch client.Patch, _ ...client.PatchOption) error {
+	var err error
+	w.patchCalls++
+	w.patchData, err = patch.Data(obj)
+	return err
+}
+
+func (w *patchRecordingWriter) DeleteAllOf(context.Context, client.Object, ...client.DeleteAllOfOption) error {
+	return nil
+}
+
+type concurrentFinalizerWriter struct {
+	patchRecordingWriter
+	currentResourceVersion string
+}
+
+func (w *concurrentFinalizerWriter) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error {
+	if err := w.patchRecordingWriter.Patch(ctx, obj, patch, opts...); err != nil {
+		return err
+	}
+	staleResourceVersion := fmt.Sprintf(`"resourceVersion":"%s"`, obj.GetResourceVersion())
+	if strings.Contains(string(w.patchData), staleResourceVersion) && obj.GetResourceVersion() != w.currentResourceVersion {
+		return apierrors.NewConflict(schema.GroupResource{Resource: "configmaps"}, obj.GetName(),
+			fmt.Errorf("object resourceVersion changed to %s", w.currentResourceVersion))
+	}
+	return nil
+}
+
 func TestRequeueWithError(t *testing.T) {
 	_, err := CheckedRequeueWithError(errors.New("test error"), tlog, "test")
 	if err == nil {
@@ -116,6 +164,97 @@ func TestResultToP(t *testing.T) {
 	}
 }
 
+func TestHandleCRDeletionPatchesFinalizerMetadata(t *testing.T) {
+	const finalizer = "finalizer/protection"
+	reqCtx := RequestCtx{Ctx: context.Background(), Log: tlog}
+
+	t.Run("add finalizer", func(t *testing.T) {
+		writer := &patchRecordingWriter{}
+		obj := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "cm", ResourceVersion: "1"}}
+
+		res, err := HandleCRDeletion(reqCtx, writer, obj, finalizer, nil)
+		if err != nil {
+			t.Fatalf("HandleCRDeletion returned error: %v", err)
+		}
+		if res != nil {
+			t.Fatalf("expected no reconcile result, got %v", res)
+		}
+		if writer.patchCalls != 1 || writer.updateCalls != 0 {
+			t.Fatalf("expected one patch and no update, got patch=%d update=%d", writer.patchCalls, writer.updateCalls)
+		}
+		assertMetadataOnlyPatch(t, writer.patchData)
+		assertOptimisticLockPatch(t, writer.patchData, obj.GetResourceVersion())
+		if !controllerutil.ContainsFinalizer(obj, finalizer) {
+			t.Fatalf("expected finalizer %q to be added", finalizer)
+		}
+	})
+
+	t.Run("remove finalizer", func(t *testing.T) {
+		writer := &patchRecordingWriter{}
+		now := metav1.Now()
+		obj := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{
+			Name:              "cm",
+			ResourceVersion:   "2",
+			Finalizers:        []string{finalizer},
+			DeletionTimestamp: &now,
+		}}
+
+		res, err := HandleCRDeletion(reqCtx, writer, obj, finalizer, nil)
+		if err != nil {
+			t.Fatalf("HandleCRDeletion returned error: %v", err)
+		}
+		if res == nil {
+			t.Fatalf("expected reconciled result")
+		}
+		if writer.patchCalls != 1 || writer.updateCalls != 0 {
+			t.Fatalf("expected one patch and no update, got patch=%d update=%d", writer.patchCalls, writer.updateCalls)
+		}
+		assertMetadataOnlyPatch(t, writer.patchData)
+		assertOptimisticLockPatch(t, writer.patchData, obj.GetResourceVersion())
+		if controllerutil.ContainsFinalizer(obj, finalizer) {
+			t.Fatalf("expected finalizer %q to be removed", finalizer)
+		}
+	})
+
+	t.Run("returns conflict on concurrent finalizer update", func(t *testing.T) {
+		writer := &concurrentFinalizerWriter{currentResourceVersion: "2"}
+		obj := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{
+			Name:            "cm",
+			ResourceVersion: "1",
+			Finalizers:      []string{"other/finalizer"},
+		}}
+
+		res, err := HandleCRDeletion(reqCtx, writer, obj, finalizer, nil)
+		if !apierrors.IsConflict(err) {
+			t.Fatalf("expected conflict from stale finalizer patch, got res=%v err=%v patch=%s", res, err, writer.patchData)
+		}
+		if writer.patchCalls != 1 || writer.updateCalls != 0 {
+			t.Fatalf("expected one patch and no update, got patch=%d update=%d", writer.patchCalls, writer.updateCalls)
+		}
+		assertMetadataOnlyPatch(t, writer.patchData)
+		assertOptimisticLockPatch(t, writer.patchData, "1")
+	})
+}
+
+func assertMetadataOnlyPatch(t *testing.T, data []byte) {
+	t.Helper()
+	patchData := string(data)
+	if !strings.Contains(patchData, `"metadata"`) {
+		t.Fatalf("expected metadata patch, got %s", patchData)
+	}
+	if strings.Contains(patchData, `"spec"`) {
+		t.Fatalf("metadata patch must not include spec, got %s", patchData)
+	}
+}
+
+func assertOptimisticLockPatch(t *testing.T, data []byte, resourceVersion string) {
+	t.Helper()
+	expected := fmt.Sprintf(`"resourceVersion":"%s"`, resourceVersion)
+	if !strings.Contains(string(data), expected) {
+		t.Fatalf("expected optimistic-locking resourceVersion %s in patch, got %s", resourceVersion, data)
+	}
+}
+
 var _ = Describe("Cluster Controller", func() {
 
 	const finalizer = "finalizer/protection"