Changes in update database configuration to detect if the database was configured (#2237)

* Changes in update database configuration to detect if the database was configured

Author: johscheuer

controllers/cluster_controller.go

@@ -469,6 +469,10 @@ func (r *FoundationDBClusterReconciler) getLockClient(logger logr.Logger, cluste
 
 // takeLock attempts to acquire a lock.
 func (r *FoundationDBClusterReconciler) takeLock(logger logr.Logger, cluster *fdbv1beta2.FoundationDBCluster, action string) error {
+	if !cluster.ShouldUseLocks() {
+		return nil
+	}
+
 	logger.Info("Taking lock on cluster", "action", action)
 	lockClient, err := r.getLockClient(logger, cluster)
 	if err != nil {

controllers/cluster_controller_test.go

@@ -1033,96 +1033,6 @@ var _ = Describe("cluster_controller", func() {
 			})
 		})
 
-		Context("with a configuration change", func() {
-			var adminClient *mock.AdminClient
-			BeforeEach(func() {
-				adminClient, err = mock.NewMockAdminClientUncast(cluster, k8sClient)
-				Expect(err).NotTo(HaveOccurred())
-
-				status, err := adminClient.GetStatus()
-				Expect(err).NotTo(HaveOccurred())
-				Expect(status.Cluster.DatabaseConfiguration.RedundancyMode).To(Equal(fdbv1beta2.RedundancyModeDouble))
-
-				cluster.Spec.DatabaseConfiguration.RedundancyMode = fdbv1beta2.RedundancyModeTriple
-
-				status, err = adminClient.GetStatus()
-				Expect(err).NotTo(HaveOccurred())
-				Expect(status.Cluster.DatabaseConfiguration.RedundancyMode).To(Equal(fdbv1beta2.RedundancyModeDouble))
-			})
-
-			Context("with changes enabled", func() {
-				BeforeEach(func() {
-					err = k8sClient.Update(context.TODO(), cluster)
-					Expect(err).NotTo(HaveOccurred())
-				})
-
-				It("should configure the database", func() {
-					Expect(adminClient.DatabaseConfiguration.RedundancyMode).To(Equal(fdbv1beta2.RedundancyModeTriple))
-				})
-			})
-
-			Context("with a change to the log version", func() {
-				BeforeEach(func() {
-					cluster.Spec.DatabaseConfiguration.LogVersion = 3
-					cluster.Spec.DatabaseConfiguration.RedundancyMode = fdbv1beta2.RedundancyModeDouble
-					generationGap = 1
-					err = k8sClient.Update(context.TODO(), cluster)
-					Expect(err).NotTo(HaveOccurred())
-				})
-
-				It("should configure the database", func() {
-					Expect(adminClient.DatabaseConfiguration.RedundancyMode).To(Equal(fdbv1beta2.RedundancyModeDouble))
-					Expect(adminClient.DatabaseConfiguration.LogVersion).To(Equal(3))
-				})
-			})
-
-			Context("with a change to the log version that is not set in the spec", func() {
-				BeforeEach(func() {
-					cluster.Spec.DatabaseConfiguration.RedundancyMode = fdbv1beta2.RedundancyModeDouble
-					cluster.Spec.SeedConnectionString = "touch"
-
-					configuration := cluster.DesiredDatabaseConfiguration()
-					configuration.LogVersion = 3
-					err = adminClient.ConfigureDatabase(configuration, false)
-					Expect(err).NotTo(HaveOccurred())
-
-					generationGap = 1
-					err = k8sClient.Update(context.TODO(), cluster)
-					Expect(err).NotTo(HaveOccurred())
-				})
-
-				It("should not reconfigure the database", func() {
-					Expect(adminClient.DatabaseConfiguration.LogVersion).To(Equal(3))
-				})
-			})
-
-			Context("with changes disabled", func() {
-				BeforeEach(func() {
-					shouldCompleteReconciliation = false
-					cluster.Spec.AutomationOptions.ConfigureDatabase = pointer.Bool(false)
-					cluster.Spec.DatabaseConfiguration.RedundancyMode = fdbv1beta2.RedundancyModeTriple
-
-					err = k8sClient.Update(context.TODO(), cluster)
-					Expect(err).NotTo(HaveOccurred())
-				})
-
-				JustBeforeEach(func() {
-					generations, err := reloadClusterGenerations(cluster)
-					Expect(err).NotTo(HaveOccurred())
-					Expect(generations).To(Equal(fdbv1beta2.ClusterGenerationStatus{
-						Reconciled:               originalVersion,
-						NeedsConfigurationChange: originalVersion + 1,
-					}))
-				})
-
-				It("should not change the database configuration", func() {
-					adminClient, err := mock.NewMockAdminClientUncast(cluster, k8sClient)
-					Expect(err).NotTo(HaveOccurred())
-					Expect(adminClient.DatabaseConfiguration.RedundancyMode).To(Equal(fdbv1beta2.RedundancyModeDouble))
-				})
-			})
-		})
-
 		Context("with a change to pod labels", func() {
 			BeforeEach(func() {
 				cluster.Spec.Processes = map[fdbv1beta2.ProcessClass]fdbv1beta2.ProcessSettings{fdbv1beta2.ProcessClassGeneral: {PodTemplate: &corev1.PodTemplateSpec{

controllers/exclude_processes.go

@@ -74,11 +74,9 @@ func (e excludeProcesses) reconcile(ctx context.Context, r *FoundationDBClusterR
 	}
 
 	// Make sure the exclusions are coordinated across multiple operator instances.
-	if cluster.ShouldUseLocks() {
-		err = r.takeLock(logger, cluster, "exclude processes")
-		if err != nil {
-			return &requeue{curError: err, delayedRequeue: true}
-		}
+	err = r.takeLock(logger, cluster, "exclude processes")
+	if err != nil {
+		return &requeue{curError: err, delayedRequeue: true}
 	}
 
 	// We need the information below to check if the excluded processes are coordinators to make sure we can change the

controllers/remove_process_groups.go

@@ -279,11 +279,9 @@ func includeProcessGroup(ctx context.Context, logger logr.Logger, r *FoundationD
 	}
 
 	// Make sure the inclusion are coordinated across multiple operator instances.
-	if cluster.ShouldUseLocks() {
-		err = r.takeLock(logger, cluster, "remove process groups")
-		if err != nil {
-			return err
-		}
+	err = r.takeLock(logger, cluster, "remove process groups")
+	if err != nil {
+		return err
 	}
 
 	// Make sure it's safe to include processes.

controllers/update_database_configuration.go

@@ -25,15 +25,12 @@ import (
 	"fmt"
 	"time"
 
+	fdbv1beta2 "github.com/FoundationDB/fdb-kubernetes-operator/v2/api/v1beta2"
 	"github.com/FoundationDB/fdb-kubernetes-operator/v2/pkg/fdbstatus"
-
 	"github.com/go-logr/logr"
-
-	"k8s.io/utils/pointer"
-
-	fdbv1beta2 "github.com/FoundationDB/fdb-kubernetes-operator/v2/api/v1beta2"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
+	"k8s.io/utils/pointer"
 )
 
 // updateDatabaseConfiguration provides a reconciliation step for changing the
@@ -62,42 +59,38 @@ func (u updateDatabaseConfiguration) reconcile(_ context.Context, r *FoundationD
 		}
 	}
 
-	initialConfig := !cluster.Status.Configured
-	if !initialConfig && !status.Client.DatabaseStatus.Available {
-		logger.Info("Skipping database configuration change because database is unavailable")
-		return &requeue{message: "cluster is not available", delayedRequeue: true, delay: 5 * time.Second}
-	}
-
+	clusterIsConfigured := fdbstatus.ClusterIsConfigured(cluster, status)
 	desiredConfiguration := cluster.DesiredDatabaseConfiguration()
-	desiredConfiguration.RoleCounts.Storage = 0
 	currentConfiguration := status.Cluster.DatabaseConfiguration.NormalizeConfiguration(cluster)
 
-	runningVersion, err := fdbv1beta2.ParseFdbVersion(cluster.GetRunningVersion())
-	if err != nil {
-		return &requeue{curError: err, delayedRequeue: true}
-	}
-
-	if initialConfig || !equality.Semantic.DeepEqual(desiredConfiguration, currentConfiguration) {
+	if !clusterIsConfigured || !equality.Semantic.DeepEqual(desiredConfiguration, currentConfiguration) {
 		var nextConfiguration fdbv1beta2.DatabaseConfiguration
-		if initialConfig {
-			nextConfiguration = desiredConfiguration
-		} else {
+		if clusterIsConfigured {
 			nextConfiguration = currentConfiguration.GetNextConfigurationChange(desiredConfiguration)
+		} else {
+			nextConfiguration = desiredConfiguration
+		}
+		configurationString, configErr := nextConfiguration.GetConfigurationString()
+		if configErr != nil {
+			return &requeue{curError: err, delayedRequeue: true}
 		}
-		configurationString, _ := nextConfiguration.GetConfigurationString()
 
-		if !initialConfig {
+		// If the cluster is configured run additional safety checks before performing the database configuration change.
+		if clusterIsConfigured {
+			runningVersion, err := fdbv1beta2.ParseFdbVersion(cluster.GetRunningVersion())
+			if err != nil {
+				return &requeue{curError: err, delayedRequeue: true}
+			}
+
 			err = fdbstatus.ConfigurationChangeAllowed(status, runningVersion.SupportsRecoveryState() && r.EnableRecoveryState)
 			if err != nil {
 				logger.Info("Changing current configuration is not safe", "error", err, "current configuration", currentConfiguration, "desired configuration", desiredConfiguration)
 				r.Recorder.Event(cluster, corev1.EventTypeNormal, "NeedsConfigurationChange",
-					fmt.Sprintf("Spec require configuration change to `%s`, but configuration change is not safe: %s", configurationString, err.Error()))
-				return &requeue{message: "Configuration change is not safe, retry later", delayedRequeue: true, delay: 10 * time.Second}
+					fmt.Sprintf("Spec requires configuration change to `%s`, but configuration change is not safe: %s", configurationString, err.Error()))
+				return &requeue{message: fmt.Sprintf("Configuration change is not safe: %s, will retry", err.Error()), delayedRequeue: true, delay: 10 * time.Second}
 			}
-		}
 
-		if !initialConfig {
-			err := r.takeLock(logger, cluster,
+			err = r.takeLock(logger, cluster,
 				fmt.Sprintf("reconfiguring the database to `%s`", configurationString))
 			if err != nil {
 				return &requeue{curError: err, delayedRequeue: true}
@@ -108,18 +101,14 @@ func (u updateDatabaseConfiguration) reconcile(_ context.Context, r *FoundationD
 		r.Recorder.Event(cluster, corev1.EventTypeNormal, "ConfiguringDatabase",
 			fmt.Sprintf("Setting database configuration to `%s`", configurationString),
 		)
-		err = adminClient.ConfigureDatabase(nextConfiguration, initialConfig)
+		err = adminClient.ConfigureDatabase(nextConfiguration, !clusterIsConfigured)
 		if err != nil {
 			return &requeue{curError: err, delayedRequeue: true}
 		}
 
-		if initialConfig {
-			return &requeue{message: "Requeuing for fetching the initial configuration from FDB cluster", delay: 1 * time.Second}
-		}
-
-		logger.Info("Configured database", "initialConfig", initialConfig)
+		logger.Info("Configured database", "clusterIsConfigured", clusterIsConfigured)
 		if !equality.Semantic.DeepEqual(nextConfiguration, desiredConfiguration) {
-			return &requeue{message: "Requeuing for next stage of database configuration change", delayedRequeue: true}
+			return &requeue{message: "Requeuing for next stage of database configuration change", delay: 30 * time.Second, delayedRequeue: true}
 		}
 	}