Move the update status into a single place

Author: johscheuer

.gitignore

@@ -48,3 +48,4 @@ dist/
 # Used for testing locally build FDB library
 libfdb_c.so
 fdb-kubernetes-operator
+*.local.md

controllers/add_process_groups.go

@@ -35,7 +35,7 @@ type addProcessGroups struct{}
 
 // reconcile runs the reconciler's work.
 func (a addProcessGroups) reconcile(
-	ctx context.Context,
+	_ context.Context,
 	r *FoundationDBClusterReconciler,
 	cluster *fdbv1beta2.FoundationDBCluster,
 	status *fdbv1beta2.FoundationDBStatus,
@@ -64,7 +64,6 @@ func (a addProcessGroups) reconcile(
 		logger.Error(err, "Error getting exclusion list")
 	}
 
-	hasNewProcessGroups := false
 	for _, processClass := range fdbv1beta2.ProcessClasses {
 		desiredCount := desiredCounts[processClass]
 		if desiredCount < 0 {
@@ -80,7 +79,6 @@ func (a addProcessGroups) reconcile(
 			processGroupIDs[processClass] = map[int]bool{}
 		}
 
-		hasNewProcessGroups = true
 		logger.Info(
 			"Adding new Process Groups",
 			"processClass",
@@ -120,13 +118,6 @@ func (a addProcessGroups) reconcile(
 		}
 	}
 
-	if hasNewProcessGroups {
-		err = r.updateOrApply(ctx, cluster)
-		if err != nil {
-			return &requeue{curError: err}
-		}
-	}
-
 	if getLocalitiesErr != nil {
 		return &requeue{curError: getLocalitiesErr, delayedRequeue: true}
 	}

controllers/add_process_groups_test.go

@@ -71,8 +71,6 @@ var _ = Describe("add_process_groups", func() {
 			Expect(requeue.curError).NotTo(HaveOccurred())
 		}
 
-		_, err = reloadCluster(cluster)
-		Expect(err).NotTo(HaveOccurred())
 		newProcessCounts = fdbv1beta2.CreateProcessCountsFromProcessGroupStatus(
 			cluster.Status.ProcessGroups,
 			true,

controllers/change_coordinators.go

@@ -38,7 +38,7 @@ type changeCoordinators struct{}
 
 // reconcile runs the reconciler's work.
 func (c changeCoordinators) reconcile(
-	ctx context.Context,
+	_ context.Context,
 	r *FoundationDBClusterReconciler,
 	cluster *fdbv1beta2.FoundationDBCluster,
 	status *fdbv1beta2.FoundationDBStatus,
@@ -115,10 +115,5 @@ func (c changeCoordinators) reconcile(
 	// Reset the SecondsSinceLastRecovered sine the operator just changed the coordinators, which will cause a recovery.
 	status.Cluster.RecoveryState.SecondsSinceLastRecovered = 0.0
 
-	err = r.updateOrApply(ctx, cluster)
-	if err != nil {
-		return &requeue{curError: err, delayedRequeue: true}
-	}
-
 	return nil
 }

controllers/choose_removals.go

@@ -38,7 +38,7 @@ type chooseRemovals struct{}
 
 // reconcile runs the reconciler's work.
 func (c chooseRemovals) reconcile(
-	ctx context.Context,
+	_ context.Context,
 	r *FoundationDBClusterReconciler,
 	cluster *fdbv1beta2.FoundationDBCluster,
 	status *fdbv1beta2.FoundationDBStatus,
@@ -151,10 +151,6 @@ func (c chooseRemovals) reconcile(
 				processGroup.MarkForRemoval()
 			}
 		}
-		err := r.updateOrApply(ctx, cluster)
-		if err != nil {
-			return &requeue{curError: err, delayedRequeue: true}
-		}
 	}
 
 	return nil

controllers/choose_removals_test.go

@@ -35,7 +35,6 @@ import (
 var _ = Describe("choose_removals", func() {
 	var cluster *fdbv1beta2.FoundationDBCluster
 	var adminClient *mock.AdminClient
-	var err error
 	var requeue *requeue
 	var removals []fdbv1beta2.ProcessGroupID
 
@@ -63,9 +62,6 @@ var _ = Describe("choose_removals", func() {
 			nil,
 			globalControllerLogger,
 		)
-		Expect(err).NotTo(HaveOccurred())
-		_, err = reloadCluster(cluster)
-		Expect(err).NotTo(HaveOccurred())
 
 		removals = nil
 		for _, processGroup := range cluster.Status.ProcessGroups {

controllers/cluster_controller.go

@@ -30,6 +30,8 @@ import (
 	"syscall"
 	"time"
 
+	"k8s.io/apimachinery/pkg/api/equality"
+
 	"github.com/FoundationDB/fdb-kubernetes-operator/v2/pkg/fdbadminclient"
 	"github.com/FoundationDB/fdb-kubernetes-operator/v2/pkg/podmanager"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -188,9 +190,23 @@ func (r *FoundationDBClusterReconciler) Reconcile(
 	cacheStatus := cluster.CacheDatabaseStatusForReconciliation(
 		r.CacheDatabaseStatusForReconciliationDefault,
 	)
-	// Printout the duration of the reconciliation, independent if the reconciliation was successful or had an error.
+
+	originalStatus := cluster.Status.DeepCopy()
 	startTime := time.Now()
 	defer func() {
+		// If the cluster.Status has changed compared to the original Status, we have to update the status.
+		// See: https://github.com/kubernetes-sigs/kubebuilder/issues/592
+		// If we use the default reflect.DeepEqual method it will be recreating the clusterStatus multiple times
+		// because the pointers are different.
+		if !equality.Semantic.DeepEqual(cluster.Status, *originalStatus) {
+			clusterLog.Info("cluster status was changed, will be updating the cluster status")
+			err = r.updateOrApply(ctx, cluster)
+			if err != nil {
+				clusterLog.Error(err, "Error updating cluster clusterStatus")
+			}
+		}
+
+		// Printout the duration of the reconciliation, independent if the reconciliation was successful or had an error.
 		clusterLog.Info(
 			"Reconciliation run finished",
 			"duration_seconds",

controllers/exclude_processes.go

@@ -47,7 +47,7 @@ type excludeEntry struct {
 
 // reconcile runs the reconciler's work.
 func (e excludeProcesses) reconcile(
-	ctx context.Context,
+	_ context.Context,
 	r *FoundationDBClusterReconciler,
 	cluster *fdbv1beta2.FoundationDBCluster,
 	status *fdbv1beta2.FoundationDBStatus,
@@ -336,11 +336,6 @@ func (e excludeProcesses) reconcile(
 		if coordinatorErr != nil {
 			return &requeue{curError: coordinatorErr, delayedRequeue: true}
 		}
-
-		err = r.updateOrApply(ctx, cluster)
-		if err != nil {
-			return &requeue{curError: err, delayedRequeue: true}
-		}
 	}
 
 	// If not all processes are excluded, ensure we requeue after 5 minutes.

controllers/exclude_processes_test.go

@@ -1730,8 +1730,6 @@ var _ = Describe("exclude_processes", func() {
 					Expect(req).To(BeNil())
 					Expect(adminClient.ExcludedAddresses).To(HaveLen(1))
 
-					_, err = reloadCluster(cluster)
-					Expect(err).NotTo(HaveOccurred())
 					Expect(initialConnectionString).NotTo(Equal(cluster.Status.ConnectionString))
 				})
 
@@ -1749,8 +1747,6 @@ var _ = Describe("exclude_processes", func() {
 						Expect(req).To(BeNil())
 						Expect(adminClient.ExcludedAddresses).To(HaveLen(1))
 
-						_, err = reloadCluster(cluster)
-						Expect(err).NotTo(HaveOccurred())
 						Expect(
 							initialConnectionString,
 						).NotTo(Equal(cluster.Status.ConnectionString))
@@ -1918,9 +1914,6 @@ var _ = Describe("exclude_processes", func() {
 
 					Expect(req).To(BeNil())
 					Expect(adminClient.ExcludedAddresses).To(HaveLen(1))
-
-					_, err = reloadCluster(cluster)
-					Expect(err).NotTo(HaveOccurred())
 					Expect(initialConnectionString).NotTo(Equal(cluster.Status.ConnectionString))
 				})
 
@@ -1937,9 +1930,6 @@ var _ = Describe("exclude_processes", func() {
 
 						Expect(req).To(BeNil())
 						Expect(adminClient.ExcludedAddresses).To(HaveLen(1))
-
-						_, err = reloadCluster(cluster)
-						Expect(err).NotTo(HaveOccurred())
 						Expect(
 							initialConnectionString,
 						).NotTo(Equal(cluster.Status.ConnectionString))

controllers/generate_initial_cluster_file.go

@@ -206,10 +206,6 @@ func (g generateInitialClusterFile) reconcile(
 	}
 
 	cluster.Status.ConnectionString = connectionString.String()
-	err = r.updateOrApply(ctx, cluster)
-	if err != nil {
-		return &requeue{curError: err}
-	}
 
 	return nil
 }