Move the update status into a single place

johscheuer · johscheuer · commit f6d3b23512aa · 2025-11-10T10:57:47.000+01:00
diff --git a/controllers/add_process_groups.go b/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}
 	}
diff --git a/controllers/add_process_groups_test.go b/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,
diff --git a/controllers/change_coordinators.go b/controllers/change_coordinators.go
@@ -39,7 +39,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,
@@ -130,10 +130,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
 }
diff --git a/controllers/choose_removals.go b/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
diff --git a/controllers/choose_removals_test.go b/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 {
diff --git a/controllers/cluster_controller.go b/controllers/cluster_controller.go
@@ -31,6 +31,7 @@ import (
 	"time"
 
 	"k8s.io/utils/ptr"
+	"k8s.io/apimachinery/pkg/api/equality"
 
 	"github.com/FoundationDB/fdb-kubernetes-operator/v2/pkg/fdbadminclient"
 	"github.com/FoundationDB/fdb-kubernetes-operator/v2/pkg/podmanager"
@@ -202,9 +203,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",
diff --git a/controllers/exclude_processes.go b/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.
diff --git a/controllers/exclude_processes_test.go b/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))
diff --git a/controllers/generate_initial_cluster_file.go b/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
 }
diff --git a/controllers/remove_process_groups.go b/controllers/remove_process_groups.go
@@ -81,7 +81,7 @@ func (u removeProcessGroups) reconcile(
 	}
 
 	coordinators := fdbstatus.GetCoordinatorsFromStatus(status)
-	allExcluded, newExclusions, processGroupsToRemove := r.getProcessGroupsToRemove(
+	allExcluded, _, processGroupsToRemove := r.getProcessGroupsToRemove(
 		logger,
 		cluster,
 		remainingMap,
@@ -99,14 +99,6 @@ func (u removeProcessGroups) reconcile(
 		return nil
 	}
 
-	// Update the cluster to reflect the new exclusions in our status
-	if newExclusions {
-		err = r.updateOrApply(ctx, cluster)
-		if err != nil {
-			return &requeue{curError: err}
-		}
-	}
-
 	// Ensure we only remove process groups that are not blocked to be removed by the buggify config.
 	processGroupsToRemove = buggify.FilterBlockedRemovals(cluster, processGroupsToRemove)
 	// If all of the process groups are filtered out we can stop doing the next steps.
@@ -352,7 +344,7 @@ func confirmRemoval(
 }
 
 func includeProcessGroup(
-	ctx context.Context,
+	_ context.Context,
 	logger logr.Logger,
 	r *FoundationDBClusterReconciler,
 	status *fdbv1beta2.FoundationDBStatus,
@@ -366,11 +358,9 @@ func includeProcessGroup(
 		return err
 	}
 
-	// Update here for ready inclusion --> Check here
 	var readyForInclusion map[fdbv1beta2.ProcessGroupID]time.Time
 	readyForInclusionUpdates := map[fdbv1beta2.ProcessGroupID]fdbv1beta2.UpdateAction{}
 	if cluster.GetSynchronizationMode() == fdbv1beta2.SynchronizationModeGlobal {
-		var err error
 		readyForInclusion, err = adminClient.GetReadyForInclusion(cluster.Spec.ProcessGroupIDPrefix)
 		if err != nil {
 			return err
@@ -398,7 +388,6 @@ func includeProcessGroup(
 		// We can update the process groups at this stage, as no other processes must be included.
 		if len(cluster.Status.ProcessGroups) != len(newProcessGroups) && len(newProcessGroups) > 0 {
 			cluster.Status.ProcessGroups = newProcessGroups
-			return r.updateOrApply(ctx, cluster)
 		}
 
 		return nil
@@ -467,7 +456,7 @@ func includeProcessGroup(
 	// Update the process group list and remove all removed and included process groups.
 	cluster.Status.ProcessGroups = newProcessGroups
 
-	return r.updateOrApply(ctx, cluster)
+	return nil
 }
 
 // filterAddressesToInclude will remove all addresses that are part of the fdbProcessesToInclude slice but are not excluded in FDB itself.
diff --git a/controllers/replace_failed_process_groups.go b/controllers/replace_failed_process_groups.go
@@ -37,7 +37,7 @@ type replaceFailedProcessGroups struct{}
 
 // return non-nil requeue if a process has been replaced
 func (c replaceFailedProcessGroups) reconcile(
-	ctx context.Context,
+	_ context.Context,
 	r *FoundationDBClusterReconciler,
 	cluster *fdbv1beta2.FoundationDBCluster,
 	status *fdbv1beta2.FoundationDBStatus,
@@ -93,11 +93,6 @@ func (c replaceFailedProcessGroups) reconcile(
 	)
 	// If the reconciler replaced at least one process group we want to update the status and requeue.
 	if hasReplacement {
-		err := r.updateOrApply(ctx, cluster)
-		if err != nil {
-			return &requeue{curError: err}
-		}
-
 		return &requeue{message: "Removals have been updated in the cluster status"}
 	}
 
diff --git a/controllers/replace_misconfigured_process_groups.go b/controllers/replace_misconfigured_process_groups.go
@@ -70,7 +70,7 @@ func (c replaceMisconfiguredProcessGroups) reconcile(
 		}
 	}
 
-	hasReplacements, err := replacements.ReplaceMisconfiguredProcessGroups(
+	_, err := replacements.ReplaceMisconfiguredProcessGroups(
 		ctx,
 		r.PodLifecycleManager,
 		r,
@@ -82,14 +82,5 @@ func (c replaceMisconfiguredProcessGroups) reconcile(
 		return &requeue{curError: err}
 	}
 
-	if hasReplacements {
-		err = r.updateOrApply(ctx, cluster)
-		if err != nil {
-			return &requeue{curError: err}
-		}
-
-		logger.Info("Replacements have been updated in the cluster status")
-	}
-
 	return nil
 }
diff --git a/controllers/update_pod_config.go b/controllers/update_pod_config.go
@@ -29,8 +29,6 @@ import (
 
 	"github.com/go-logr/logr"
 
-	"k8s.io/apimachinery/pkg/api/equality"
-
 	"github.com/FoundationDB/fdb-kubernetes-operator/v2/pkg/podmanager"
 
 	"github.com/FoundationDB/fdb-kubernetes-operator/v2/internal"
@@ -55,7 +53,6 @@ func (updatePodConfig) reconcile(
 		return &requeue{curError: err}
 	}
 
-	originalStatus := cluster.Status.DeepCopy()
 	allSynced := true
 	delayedRequeue := true
 	var errs []error
@@ -165,13 +162,6 @@ func (updatePodConfig) reconcile(
 		processGroup.UpdateCondition(fdbv1beta2.SidecarUnreachable, false)
 	}
 
-	if !equality.Semantic.DeepEqual(cluster.Status, *originalStatus) {
-		err = r.updateOrApply(ctx, cluster)
-		if err != nil {
-			return &requeue{curError: err}
-		}
-	}
-
 	// If any error has happened requeue.
 	// We don't provide an error here since we log all errors above.
 	if len(errs) > 0 {
diff --git a/controllers/update_status.go b/controllers/update_status.go
@@ -56,7 +56,6 @@ func (c updateStatus) reconcile(
 	databaseStatus *fdbv1beta2.FoundationDBStatus,
 	logger logr.Logger,
 ) *requeue {
-	originalStatus := cluster.Status.DeepCopy()
 	clusterStatus := fdbv1beta2.FoundationDBClusterStatus{}
 	clusterStatus.Generations.Reconciled = cluster.Status.Generations.Reconciled
 	clusterStatus.ProcessGroups = cluster.Status.ProcessGroups
@@ -71,6 +70,12 @@ func (c updateStatus) reconcile(
 		var err error
 		databaseStatus, err = r.getStatusFromClusterOrDummyStatus(logger, cluster)
 		if err != nil {
+			// If the machine-readable status cannot be fetched we have to update it here.
+			clusterStatus.Health.Available = false
+			clusterStatus.Health.Healthy = false
+			clusterStatus.Health.FullReplication = false
+			clusterStatus.Health.DataMovementPriority = 0
+
 			return &requeue{
 				curError:       fmt.Errorf("update_status error fetching status: %w", err),
 				delayedRequeue: true,
@@ -330,17 +335,6 @@ func (c updateStatus) reconcile(
 		}
 	}
 
-	// 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) {
-		err = r.updateOrApply(ctx, cluster)
-		if err != nil {
-			logger.Error(err, "Error updating cluster clusterStatus")
-			return &requeue{curError: err}
-		}
-	}
-
 	// If the cluster is not reconciled, make sure we trigger a new reconciliation loop.
 	if !reconciled {
 		return &requeue{
diff --git a/controllers/update_status_test.go b/controllers/update_status_test.go

Original file line number	Diff line number	Diff line change
`@@ -71,8 +71,6 @@ var _ = Describe("add_process_groups", func() {`
`71`	`71`	`Expect(requeue.curError).NotTo(HaveOccurred())`
`72`	`72`	`}`
`73`	`73`
`74`		`- _, err = reloadCluster(cluster)`
`75`		`- Expect(err).NotTo(HaveOccurred())`
`76`	`74`	`newProcessCounts = fdbv1beta2.CreateProcessCountsFromProcessGroupStatus(`
`77`	`75`	`cluster.Status.ProcessGroups,`
`78`	`76`	`true,`
Original file line number	Diff line number	Diff line change
`@@ -38,7 +38,7 @@ type chooseRemovals struct{}`
`38`	`38`
`39`	`39`	`// reconcile runs the reconciler's work.`
`40`	`40`	`func (c chooseRemovals) reconcile(`
`41`		`- ctx context.Context,`
	`41`	`+ _ context.Context,`
`42`	`42`	`r *FoundationDBClusterReconciler,`
`43`	`43`	`cluster *fdbv1beta2.FoundationDBCluster,`
`44`	`44`	`status *fdbv1beta2.FoundationDBStatus,`
`@@ -151,10 +151,6 @@ func (c chooseRemovals) reconcile(`
`151`	`151`	`processGroup.MarkForRemoval()`
`152`	`152`	`}`
`153`	`153`	`}`
`154`		`- err := r.updateOrApply(ctx, cluster)`
`155`		`- if err != nil {`
`156`		`- return &requeue{curError: err, delayedRequeue: true}`
`157`		`- }`
`158`	`154`	`}`
`159`	`155`
`160`	`156`	`return nil`
Original file line number	Diff line number	Diff line change
`@@ -206,10 +206,6 @@ func (g generateInitialClusterFile) reconcile(`
`206`	`206`	`}`
`207`	`207`
`208`	`208`	`cluster.Status.ConnectionString = connectionString.String()`
`209`		`- err = r.updateOrApply(ctx, cluster)`
`210`		`- if err != nil {`
`211`		`- return &requeue{curError: err}`
`212`		`- }`
`213`	`209`
`214`	`210`	`return nil`
`215`	`211`	`}`