Skip to content

Move the update status into a single place #2361

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Move the update status into a single place #2361

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,4 @@ dist/
# Used for testing locally build FDB library
libfdb_c.so
fdb-kubernetes-operator
*.local.md
11 changes: 1 addition & 10 deletions controllers/add_process_groups.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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 {
Expand All @@ -80,7 +79,6 @@ func (a addProcessGroups) reconcile(
processGroupIDs[processClass] = map[int]bool{}
}

hasNewProcessGroups = true
logger.Info(
"Adding new Process Groups",
"processClass",
Expand Down Expand Up @@ -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}
}
Expand Down
2 changes: 0 additions & 2 deletions controllers/add_process_groups_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
7 changes: 1 addition & 6 deletions controllers/change_coordinators.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
}
6 changes: 1 addition & 5 deletions controllers/choose_removals.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
4 changes: 0 additions & 4 deletions controllers/choose_removals_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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 {
Expand Down
18 changes: 17 additions & 1 deletion controllers/cluster_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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() {
Copy link
Contributor

@nicmorales9 nicmorales9 Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any scenario where we don't want to do the update? like if there is an error on a subreconciler?

// 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",
Expand Down
7 changes: 1 addition & 6 deletions controllers/exclude_processes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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.
Expand Down
10 changes: 0 additions & 10 deletions controllers/exclude_processes_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
})

Expand All @@ -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))
Expand Down Expand Up @@ -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))
})

Expand All @@ -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))
Expand Down
4 changes: 0 additions & 4 deletions controllers/generate_initial_cluster_file.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
17 changes: 3 additions & 14 deletions controllers/remove_process_groups.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ func (u removeProcessGroups) reconcile(
}

coordinators := fdbstatus.GetCoordinatorsFromStatus(status)
allExcluded, newExclusions, processGroupsToRemove := r.getProcessGroupsToRemove(
allExcluded, _, processGroupsToRemove := r.getProcessGroupsToRemove(
logger,
cluster,
remainingMap,
Expand All @@ -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.
Expand Down Expand Up @@ -352,7 +344,7 @@ func confirmRemoval(
}

func includeProcessGroup(
ctx context.Context,
_ context.Context,
logger logr.Logger,
r *FoundationDBClusterReconciler,
status *fdbv1beta2.FoundationDBStatus,
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
7 changes: 1 addition & 6 deletions controllers/replace_failed_process_groups.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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"}
}

Expand Down
11 changes: 1 addition & 10 deletions controllers/replace_misconfigured_process_groups.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ func (c replaceMisconfiguredProcessGroups) reconcile(
}
}

hasReplacements, err := replacements.ReplaceMisconfiguredProcessGroups(
_, err := replacements.ReplaceMisconfiguredProcessGroups(
ctx,
r.PodLifecycleManager,
r,
Expand All @@ -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
}
10 changes: 0 additions & 10 deletions controllers/update_pod_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -55,7 +53,6 @@ func (updatePodConfig) reconcile(
return &requeue{curError: err}
}

originalStatus := cluster.Status.DeepCopy()
allSynced := true
delayedRequeue := true
var errs []error
Expand Down Expand Up @@ -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 {
Expand Down
18 changes: 6 additions & 12 deletions controllers/update_status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand Down Expand Up @@ -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{
Expand Down
Loading