Merge pull request #123 from numtide/ssa-cluster-handler

refactor(cluster-handler): implement Server-Side Apply for all controllers

Author: fernando-villalba

pkg/cluster-handler/controller/multigrescluster/integration_test.go

@@ -70,8 +70,9 @@ func setupIntegration(t *testing.T) (client.Client, *testutil.ResourceWatcher) {
 
 	// 3. Setup Controller
 	reconciler := &multigrescluster.MultigresClusterReconciler{
-		Client: mgr.GetClient(),
-		Scheme: mgr.GetScheme(),
+		Client:   mgr.GetClient(),
+		Scheme:   mgr.GetScheme(),
+		Recorder: mgr.GetEventRecorderFor("multigres-cluster-controller"),
 	}
 
 	if err := reconciler.SetupWithManager(mgr, controller.Options{

pkg/cluster-handler/controller/multigrescluster/multigrescluster_controller_test.go

@@ -550,12 +550,12 @@ func TestMultigresClusterReconciler_Lifecycle(t *testing.T) {
 			existingObjects: []client.Object{coreTpl, cellTpl, shardTpl},
 			wantErrMsg:      "exceeds 50 characters",
 		},
-		"Error: Create TableGroup Failed": {
+		"Error: Apply TableGroup Failed": {
 			existingObjects: []client.Object{coreTpl, cellTpl, shardTpl},
 			failureConfig: &testutil.FailureConfig{
-				OnCreate: testutil.FailOnObjectName(clusterName+"-db1-tg1", errSimulated),
+				OnPatch: testutil.FailOnObjectName(clusterName+"-db1-tg1", errSimulated),
 			},
-			wantErrMsg: "failed to create tablegroup",
+			wantErrMsg: "failed to apply tablegroup",
 		},
 		"Error: Delete Orphan TableGroup Failed": {
 			existingObjects: []client.Object{

pkg/cluster-handler/controller/multigrescluster/reconcile_cells.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"fmt"
 
-	"k8s.io/apimachinery/pkg/api/errors"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	multigresv1alpha1 "github.com/numtide/multigres-operator/api/v1alpha1"
@@ -55,28 +54,19 @@ func (r *MultigresClusterReconciler) reconcileCells(
 			return fmt.Errorf("failed to build cell '%s': %w", cellCfg.Name, err)
 		}
 
-		existing := &multigresv1alpha1.Cell{}
-		err = r.Get(
+		// Server Side Apply
+		desired.SetGroupVersionKind(multigresv1alpha1.GroupVersion.WithKind("Cell"))
+		if err := r.Patch(
 			ctx,
-			client.ObjectKey{Namespace: cluster.Namespace, Name: desired.Name},
-			existing,
-		)
-		if err != nil {
-			if errors.IsNotFound(err) {
-				if err := r.Create(ctx, desired); err != nil {
-					return fmt.Errorf("failed to create cell '%s': %w", cellCfg.Name, err)
-				}
-				r.Recorder.Eventf(cluster, "Normal", "Created", "Created Cell %s", desired.Name)
-				continue
-			}
-			return fmt.Errorf("failed to get cell '%s': %w", cellCfg.Name, err)
+			desired,
+			client.Apply,
+			client.ForceOwnership,
+			client.FieldOwner("multigres-operator"),
+		); err != nil {
+			return fmt.Errorf("failed to apply cell '%s': %w", cellCfg.Name, err)
 		}
+		r.Recorder.Eventf(cluster, "Normal", "Applied", "Applied Cell %s", desired.Name)
 
-		existing.Spec = desired.Spec
-		existing.Labels = desired.Labels
-		if err := r.Update(ctx, existing); err != nil {
-			return fmt.Errorf("failed to update cell '%s': %w", cellCfg.Name, err)
-		}
 	}
 
 	for _, item := range existingCells.Items {

pkg/cluster-handler/controller/multigrescluster/reconcile_cells_test.go

@@ -7,14 +7,8 @@ import (
 
 	multigresv1alpha1 "github.com/numtide/multigres-operator/api/v1alpha1"
 	"github.com/numtide/multigres-operator/pkg/testutil"
-	appsv1 "k8s.io/api/apps/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/client-go/tools/record"
-	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 )
 
 func TestReconcile_Cells(t *testing.T) {
@@ -46,34 +40,13 @@ func TestReconcile_Cells(t *testing.T) {
 			},
 			wantErrMsg: "failed to list existing cells",
 		},
-		"Error: Create Cell Failed": {
+		"Error: Apply Cell Failed": {
 			failureConfig: &testutil.FailureConfig{
-				OnCreate: testutil.FailOnObjectName(clusterName+"-zone-a", errSimulated),
+				OnPatch: testutil.FailOnObjectName(clusterName+"-zone-a", errSimulated),
 			},
-			wantErrMsg: "failed to create cell",
-		},
-		"Error: Get Cell Failed (Unexpected Error)": {
-			failureConfig: &testutil.FailureConfig{
-				OnGet: testutil.FailOnKeyName(clusterName+"-zone-a", errSimulated),
-			},
-			wantErrMsg: "failed to get cell",
-		},
-		"Error: Update Cell Failed": {
-			existingObjects: []client.Object{
-				coreTpl, cellTpl, shardTpl,
-				&multigresv1alpha1.Cell{
-					ObjectMeta: metav1.ObjectMeta{
-						Name:      clusterName + "-zone-a",
-						Namespace: namespace,
-						Labels:    map[string]string{"multigres.com/cluster": clusterName},
-					},
-				},
-			},
-			failureConfig: &testutil.FailureConfig{
-				OnUpdate: testutil.FailOnObjectName(clusterName+"-zone-a", errSimulated),
-			},
-			wantErrMsg: "failed to update cell",
+			wantErrMsg: "failed to apply cell",
 		},
+
 		"Error: Prune Cell Failed": {
 			existingObjects: []client.Object{
 				coreTpl, shardTpl,
@@ -170,78 +143,25 @@ func TestReconcile_Cells(t *testing.T) {
 				},
 			},
 		},
-	}
-
-	runReconcileTest(t, tests)
-}
-
-func TestReconcile_Cells_BuildFailure(t *testing.T) {
-	t.Parallel()
-	scheme := runtime.NewScheme()
-	_ = multigresv1alpha1.AddToScheme(scheme)
-	_ = appsv1.AddToScheme(scheme)
-
-	cluster := &multigresv1alpha1.MultigresCluster{
-		TypeMeta: metav1.TypeMeta{
-			Kind:       "MultigresCluster",
-			APIVersion: multigresv1alpha1.GroupVersion.String(),
-		},
-		ObjectMeta: metav1.ObjectMeta{
-			Name:       "c1",
-			Namespace:  "ns1",
-			Finalizers: []string{"multigres.com/finalizer"},
-		},
-		Spec: multigresv1alpha1.MultigresClusterSpec{
-			// Need to disable global components to avoid them failing first (on cluster name if long? No, we use normal cluster name here)
-			// But for Cells, we use Long Cell Name.
-			GlobalTopoServer: &multigresv1alpha1.GlobalTopoServerSpec{
-				External: &multigresv1alpha1.ExternalTopoServerSpec{
-					Endpoints: []multigresv1alpha1.EndpointUrl{"http://ext:2379"},
-				},
-			},
-			MultiAdmin: nil,
-			Cells: []multigresv1alpha1.CellConfig{ // This will trigger Build failure due to long name
-				{Name: strings.Repeat("a", 64), CellTemplate: "cell-tpl"},
+		"Error: Build Cell Failed (Name Too Long)": {
+			preReconcileUpdate: func(t testing.TB, c *multigresv1alpha1.MultigresCluster) {
+				// Use normal cluster name, but put a long name in Cells config
+				c.Spec.Cells = []multigresv1alpha1.CellConfig{
+					{Name: strings.Repeat("a", 64), CellTemplate: "default-cell"},
+				}
+				// Ensure GlobalTopo doesn't fail
+				c.Spec.GlobalTopoServer = &multigresv1alpha1.GlobalTopoServerSpec{
+					External: &multigresv1alpha1.ExternalTopoServerSpec{
+						Endpoints: []multigresv1alpha1.EndpointUrl{"http://ext:2379"},
+					},
+				}
+				// Disable MultiAdmin to avoid distractions
+				c.Spec.MultiAdmin = nil
 			},
+			existingObjects: []client.Object{coreTpl, cellTpl, shardTpl},
+			wantErrMsg:      "failed to build cell",
 		},
 	}
 
-	tmpl := &multigresv1alpha1.CellTemplate{
-		TypeMeta: metav1.TypeMeta{
-			Kind:       "CellTemplate",
-			APIVersion: multigresv1alpha1.GroupVersion.String(),
-		},
-		ObjectMeta: metav1.ObjectMeta{Name: "cell-tpl", Namespace: "ns1"},
-		Spec:       multigresv1alpha1.CellTemplateSpec{},
-	}
-
-	c := fake.NewClientBuilder().
-		WithScheme(scheme).
-		WithObjects(cluster, tmpl).
-		Build()
-
-	r := &MultigresClusterReconciler{
-		Client:   c,
-		Scheme:   scheme,
-		Recorder: record.NewFakeRecorder(100),
-	}
-
-	req := ctrl.Request{
-		NamespacedName: types.NamespacedName{Name: cluster.Name, Namespace: cluster.Namespace},
-	}
-
-	_, err := r.Reconcile(t.Context(), req)
-	if err == nil {
-		t.Fatal("Expected error from Reconcile, got nil")
-	}
-	if !contains(err.Error(), "failed to build cell") {
-		// If MultiAdmin fails, we will see it here. But since we used normal cluster name and nil MultiAdmin (which likely defaults),
-		// we might hit MultiAdmin build.
-		// If MultiAdmin defaults, it builds.
-		// Does MultiAdmin validation fail? No.
-		// So MultiAdmin succeeds.
-		// Then Cells run. Cell name is long. Validation fails.
-		// WE SHOULD SEE "failed to build cell".
-		t.Errorf("Unexpected error: %v", err)
-	}
+	runReconcileTest(t, tests)
 }

pkg/cluster-handler/controller/multigrescluster/reconcile_database_test.go

@@ -6,17 +6,12 @@ import (
 	"testing"
 
 	"github.com/numtide/multigres-operator/pkg/testutil"
-	appsv1 "k8s.io/api/apps/v1"
-	"k8s.io/client-go/tools/record"
 	"k8s.io/utils/ptr"
 
 	multigresv1alpha1 "github.com/numtide/multigres-operator/api/v1alpha1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
-	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 )
 
 func TestReconcile_Databases(t *testing.T) {
@@ -145,34 +140,13 @@ func TestReconcile_Databases(t *testing.T) {
 			},
 			wantErrMsg: "failed to resolve shard",
 		},
-		"Error: Create TableGroup Failed": {
+		"Error: Apply TableGroup Failed": {
 			failureConfig: &testutil.FailureConfig{
-				OnCreate: testutil.FailOnObjectName(clusterName+"-db1-tg1", errSimulated),
+				OnPatch: testutil.FailOnObjectName(clusterName+"-db1-tg1", errSimulated),
 			},
-			wantErrMsg: "failed to create tablegroup",
-		},
-		"Error: Get TableGroup Failed (Unexpected Error)": {
-			failureConfig: &testutil.FailureConfig{
-				OnGet: testutil.FailOnKeyName(clusterName+"-db1-tg1", errSimulated),
-			},
-			wantErrMsg: "failed to get tablegroup",
-		},
-		"Error: Update TableGroup Failed": {
-			existingObjects: []client.Object{
-				coreTpl, cellTpl, shardTpl,
-				&multigresv1alpha1.TableGroup{
-					ObjectMeta: metav1.ObjectMeta{
-						Name:      clusterName + "-db1-tg1",
-						Namespace: namespace,
-						Labels:    map[string]string{"multigres.com/cluster": clusterName},
-					},
-				},
-			},
-			failureConfig: &testutil.FailureConfig{
-				OnUpdate: testutil.FailOnObjectName(clusterName+"-db1-tg1", errSimulated),
-			},
-			wantErrMsg: "failed to update tablegroup",
+			wantErrMsg: "failed to apply tablegroup",
 		},
+
 		"Error: Prune TableGroup Failed": {
 			existingObjects: []client.Object{
 				coreTpl, cellTpl, shardTpl,
@@ -235,79 +209,32 @@ func TestReconcile_Databases(t *testing.T) {
 			},
 			wantErrMsg: "exceeds 50 characters",
 		},
-	}
-
-	runReconcileTest(t, tests)
-}
-
-func TestReconcile_Databases_BuildFailure(t *testing.T) {
-	t.Parallel()
-	scheme := runtime.NewScheme()
-	_ = multigresv1alpha1.AddToScheme(scheme)
-	_ = appsv1.AddToScheme(scheme)
-
-	cluster := &multigresv1alpha1.MultigresCluster{
-		TypeMeta: metav1.TypeMeta{
-			Kind:       "MultigresCluster",
-			APIVersion: multigresv1alpha1.GroupVersion.String(),
-		},
-		ObjectMeta: metav1.ObjectMeta{
-			Name:       "c1",
-			Namespace:  "ns1",
-			Finalizers: []string{"multigres.com/finalizer"},
-		},
-		Spec: multigresv1alpha1.MultigresClusterSpec{
-			GlobalTopoServer: &multigresv1alpha1.GlobalTopoServerSpec{
-				External: &multigresv1alpha1.ExternalTopoServerSpec{
-					Endpoints: []multigresv1alpha1.EndpointUrl{"http://ext:2379"},
-				},
-			},
-			MultiAdmin: nil, // MultiAdmin will default and succeed.
-			Databases: []multigresv1alpha1.DatabaseConfig{
-				{
-					Name: strings.Repeat("a", 64), // Long Name triggers failure in BuildTableGroup
-					TableGroups: []multigresv1alpha1.TableGroupConfig{
-						{
-							Name: "tg1",
-							Shards: []multigresv1alpha1.ShardConfig{
-								{Name: "0", ShardTemplate: "shard-tpl"},
+		"Error: Build TableGroup Failed (Name Too Long)": {
+			preReconcileUpdate: func(t testing.TB, c *multigresv1alpha1.MultigresCluster) {
+				c.Spec.GlobalTopoServer = &multigresv1alpha1.GlobalTopoServerSpec{
+					External: &multigresv1alpha1.ExternalTopoServerSpec{
+						Endpoints: []multigresv1alpha1.EndpointUrl{"http://ext:2379"},
+					},
+				}
+				c.Spec.MultiAdmin = nil
+				c.Spec.Databases = []multigresv1alpha1.DatabaseConfig{
+					{
+						Name: strings.Repeat("a", 64),
+						TableGroups: []multigresv1alpha1.TableGroupConfig{
+							{
+								Name: "tg1",
+								Shards: []multigresv1alpha1.ShardConfig{
+									{Name: "0", ShardTemplate: "default-shard"},
+								},
 							},
 						},
 					},
-				},
+				}
 			},
+			existingObjects: []client.Object{coreTpl, cellTpl, shardTpl},
+			wantErrMsg:      "failed to build tablegroup",
 		},
 	}
 
-	tmpl := &multigresv1alpha1.ShardTemplate{
-		TypeMeta: metav1.TypeMeta{
-			Kind:       "ShardTemplate",
-			APIVersion: multigresv1alpha1.GroupVersion.String(),
-		},
-		ObjectMeta: metav1.ObjectMeta{Name: "shard-tpl", Namespace: "ns1"},
-		Spec:       multigresv1alpha1.ShardTemplateSpec{},
-	}
-
-	c := fake.NewClientBuilder().
-		WithScheme(scheme).
-		WithObjects(cluster, tmpl).
-		Build()
-
-	r := &MultigresClusterReconciler{
-		Client:   c,
-		Scheme:   scheme,
-		Recorder: record.NewFakeRecorder(100),
-	}
-
-	req := ctrl.Request{
-		NamespacedName: types.NamespacedName{Name: cluster.Name, Namespace: cluster.Namespace},
-	}
-
-	_, err := r.Reconcile(t.Context(), req)
-	if err == nil {
-		t.Fatal("Expected error from Reconcile, got nil")
-	}
-	if !contains(err.Error(), "failed to build tablegroup") {
-		t.Errorf("Unexpected error: %v", err)
-	}
+	runReconcileTest(t, tests)
 }