Repository: FoundationDB/fdb-kubernetes-operator
Pull request #1883 (2023-11-12 09:15:54)

Fix database configuration bug if only proxies are specified
# Description

In https://github.com/FoundationDB/fdb-kubernetes-operator/pull/1863 we did some refactoring of the database configuration generation. This refactoring introduced a bug when `proxies` are specified in FDB versions 7.1+.

## Type of change

*Please select one of the options below.*

- Bug fix (non-breaking change which fixes an issue)

## Discussion

-

## Testing

I added a dedicated e2e test to make sure the cluster is able to reconcile and has the targeted configuration.

## Documentation

-

## Follow-up

Updated the documentation hoe the different proxy configuration interact.


foundationdb-ci: 
### Result of fdb-kubernetes-operator-pr on Linux CentOS 7

* Commit ID: 4eb1763f2d4b1f6f399553bb3f9698698cf4056d
* Duration 1:47:15
* Result: :white_check_mark: **SUCCEEDED**
* Error: `N/A`
* [Build Log](https://d1e0l78xbkh3xa.cloudfront.net/fdb-kubernetes-operator/fdb-kubernetes-operator-pr/3579291d-f110-4011-8104-86dd17ea2d27.gz) terminal output (available for 30 days)
* [Build Workspace](https://d1e0l78xbkh3xa.cloudfront.net/fdb-kubernetes-operator-pr/3579291d-f110-4011-8104-86dd17ea2d27/workspace.zip) zip file of the working directory (available for 30 days)

foundationdb-ci: 
### Result of fdb-kubernetes-operator-pr on Linux CentOS 7

* Commit ID: 579e20cf09cf5e043ecef053b212bdfb161b4ff2
* Duration 1:57:35
* Result: :white_check_mark: **SUCCEEDED**
* Error: `N/A`
* [Build Log](https://d1e0l78xbkh3xa.cloudfront.net/fdb-kubernetes-operator/fdb-kubernetes-operator-pr/37ea49b0-69c8-4ee1-b3da-a8c36ec8aecc.gz) terminal output (available for 30 days)
* [Build Workspace](https://d1e0l78xbkh3xa.cloudfront.net/fdb-kubernetes-operator-pr/37ea49b0-69c8-4ee1-b3da-a8c36ec8aecc/workspace.zip) zip file of the working directory (available for 30 days)


Patch:
diff --git a/api/v1beta2/foundationdb_database_configuration.go b/api/v1beta2/foundationdb_database_configuration.go
index a40dd1d8..60076d5a 100644
--- a/api/v1beta2/foundationdb_database_configuration.go
+++ b/api/v1beta2/foundationdb_database_configuration.go
@@ -234,12 +234,12 @@ func (configuration *DatabaseConfiguration) CountUniqueDataCenters() int {
 // This will fill in defaults of -1 for some fields that have a default
 // of 0, and will ensure that the region configuration is ordered
 // consistently.
-func (configuration DatabaseConfiguration) NormalizeConfigurationWithSeparatedProxies(version string) DatabaseConfiguration {
+func (configuration DatabaseConfiguration) NormalizeConfigurationWithSeparatedProxies(version string, areSeparatedProxiesConfigured bool) DatabaseConfiguration {
 	result := configuration.NormalizeConfiguration()
 
 	parsedVersion, _ := ParseFdbVersion(version)
 	if parsedVersion.HasSeparatedProxies() {
-		if !configuration.AreSeparatedProxiesConfigured() {
+		if !areSeparatedProxiesConfigured {
 			result.GrvProxies = 0
 			result.CommitProxies = 0
 		} else {
diff --git a/api/v1beta2/foundationdbcluster_types.go b/api/v1beta2/foundationdbcluster_types.go
index d0a2244c..e9a1c0ff 100644
--- a/api/v1beta2/foundationdbcluster_types.go
+++ b/api/v1beta2/foundationdbcluster_types.go
@@ -1815,7 +1815,7 @@ type ContainerOverrides struct {
 // DesiredDatabaseConfiguration builds the database configuration for the
 // cluster based on its spec.
 func (cluster *FoundationDBCluster) DesiredDatabaseConfiguration() DatabaseConfiguration {
-	configuration := cluster.Spec.DatabaseConfiguration.NormalizeConfigurationWithSeparatedProxies(cluster.GetRunningVersion())
+	configuration := cluster.Spec.DatabaseConfiguration.NormalizeConfigurationWithSeparatedProxies(cluster.GetRunningVersion(), cluster.Spec.DatabaseConfiguration.AreSeparatedProxiesConfigured())
 	configuration.RoleCounts = cluster.GetRoleCountsWithDefaults()
 	configuration.RoleCounts.Storage = 0
 
diff --git a/controllers/cluster_controller.go b/controllers/cluster_controller.go
index 5ef48034..58d83098 100644
--- a/controllers/cluster_controller.go
+++ b/controllers/cluster_controller.go
@@ -476,9 +476,14 @@ func (r *FoundationDBClusterReconciler) getStatusFromClusterOrDummyStatus(logger
 	if err != nil {
 		return nil, err
 	}
-
 	defer adminClient.Close()
 
+	// If the cluster is not yet configured, we can reduce the timeout to make sure the initial reconcile steps
+	// are faster.
+	if !cluster.Status.Configured {
+		adminClient.SetTimeout(10 * time.Second)
+	}
+
 	status, err := adminClient.GetStatus()
 	if err == nil {
 		return status, nil
diff --git a/controllers/update_database_configuration.go b/controllers/update_database_configuration.go
index e93c993d..e65e5d0b 100644
--- a/controllers/update_database_configuration.go
+++ b/controllers/update_database_configuration.go
@@ -65,7 +65,7 @@ func (u updateDatabaseConfiguration) reconcile(ctx context.Context, r *Foundatio
 
 	desiredConfiguration := cluster.DesiredDatabaseConfiguration()
 	desiredConfiguration.RoleCounts.Storage = 0
-	currentConfiguration := status.Cluster.DatabaseConfiguration.NormalizeConfigurationWithSeparatedProxies(cluster.Spec.Version)
+	currentConfiguration := status.Cluster.DatabaseConfiguration.NormalizeConfigurationWithSeparatedProxies(cluster.Spec.Version, cluster.Spec.DatabaseConfiguration.AreSeparatedProxiesConfigured())
 	// We have to reset the excluded servers here otherwise we will trigger a reconfiguration if one or more servers
 	// are excluded.
 	currentConfiguration.ExcludedServers = nil
diff --git a/controllers/update_status.go b/controllers/update_status.go
index 047d5191..e361e9bd 100644
--- a/controllers/update_status.go
+++ b/controllers/update_status.go
@@ -99,7 +99,7 @@ func (updateStatus) reconcile(ctx context.Context, r *FoundationDBClusterReconci
 	// about the current database configuration, leading to a wrong signal that the database configuration must be changed as
 	// the configuration will be overwritten with the default values.
 	if databaseStatus.Client.DatabaseStatus.Available {
-		clusterStatus.DatabaseConfiguration = databaseStatus.Cluster.DatabaseConfiguration.NormalizeConfigurationWithSeparatedProxies(cluster.Spec.Version)
+		clusterStatus.DatabaseConfiguration = databaseStatus.Cluster.DatabaseConfiguration.NormalizeConfigurationWithSeparatedProxies(cluster.Spec.Version, cluster.Spec.DatabaseConfiguration.AreSeparatedProxiesConfigured())
 		// Removing excluded servers as we don't want them during comparison.
 		clusterStatus.DatabaseConfiguration.ExcludedServers = nil
 		cluster.ClearMissingVersionFlags(&clusterStatus.DatabaseConfiguration)
@@ -318,6 +318,12 @@ func tryConnectionOptions(logger logr.Logger, cluster *fdbv1beta2.FoundationDBCl
 			return originalConnectionString, clientErr
 		}
 
+		// If the cluster is not yet configured, we can reduce the timeout to make sure the initial reconcile steps
+		// are faster.
+		if !cluster.Status.Configured {
+			adminClient.SetTimeout(10 * time.Second)
+		}
+
 		var activeConnectionString string
 		activeConnectionString, err = adminClient.GetConnectionString()
 
diff --git a/fdbclient/admin_client.go b/fdbclient/admin_client.go
index 01a84775..43778ef1 100644
--- a/fdbclient/admin_client.go
+++ b/fdbclient/admin_client.go
@@ -89,6 +89,9 @@ type cliAdminClient struct {
 	// fdbLibClient is an interface to interact with a FDB cluster over the FDB client libraries. In the real fdb lib client
 	// we will issue the actual requests against FDB. In the mock runner we will return predefined output.
 	fdbLibClient fdbLibClient
+
+	// timeout defines the timeout that should be used for interacting with FDB.
+	timeout time.Duration
 }
 
 // NewCliAdminClient generates an Admin client for a cluster
@@ -275,7 +278,7 @@ func (client *cliAdminClient) runCommand(command cliCommand) (string, error) {
 func (client *cliAdminClient) getStatusFromCli() (*fdbv1beta2.FoundationDBStatus, error) {
 	// Always use the max timeout here. Otherwise we will retry multiple times with an increasing timeout. As the
 	// timeout is only the upper bound using directly the max timeout reduces the calls to a single call.
-	output, err := client.runCommand(cliCommand{command: "status json", timeout: MaxCliTimeout})
+	output, err := client.runCommand(cliCommand{command: "status json", timeout: client.getTimeout()})
 	if err != nil {
 		return nil, err
 	}
@@ -313,8 +316,9 @@ func (client *cliAdminClient) GetStatus() (*fdbv1beta2.FoundationDBStatus, error
 	adminClientMutex.Lock()
 	defer adminClientMutex.Unlock()
 
+	startTime := time.Now()
 	// This will call directly the database and fetch the status information from the system key space.
-	status, err := getStatusFromDB(client.fdbLibClient, client.log, MaxCliTimeout)
+	status, err := getStatusFromDB(client.fdbLibClient, client.log, client.getTimeout())
 	// There is a limitation in the multi version client if the cluster is only partially upgraded e.g. because not
 	// all fdbserver processes are restarted, then the multi version client sometimes picks the wrong version
 	// to connect to the cluster. This will result in an empty status only reporting the unreachable coordinators.
@@ -326,7 +330,7 @@ func (client *cliAdminClient) GetStatus() (*fdbv1beta2.FoundationDBStatus, error
 		status, err = client.getStatus()
 	}
 
-	client.log.V(1).Info("Completed GetStatus() call", "error", err, "status", status)
+	client.log.V(1).Info("Completed GetStatus() call", "error", err, "status", status, "duration", time.Since(startTime).String())
 
 	return status, err
 }
@@ -348,7 +352,7 @@ func (client *cliAdminClient) ConfigureDatabase(configuration fdbv1beta2.Databas
 
 // GetMaintenanceZone gets current maintenance zone, if any. Returns empty string if maintenance mode is off
 func (client *cliAdminClient) GetMaintenanceZone() (string, error) {
-	mode, err := client.fdbLibClient.getValueFromDBUsingKey("\xff/maintenance", DefaultCLITimeout)
+	mode, err := client.fdbLibClient.getValueFromDBUsingKey("\xff/maintenance", client.getTimeout())
 	if err != nil {
 		return "", err
 	}
@@ -394,7 +398,7 @@ func (client *cliAdminClient) ExcludeProcesses(addresses []fdbv1beta2.ProcessAdd
 
 	excludeCommand.WriteString(fdbv1beta2.ProcessAddressesString(addresses, " "))
 
-	_, err = client.runCommand(cliCommand{command: excludeCommand.String(), timeout: MaxCliTimeout})
+	_, err = client.runCommand(cliCommand{command: excludeCommand.String(), timeout: client.getTimeout()})
 
 	return err
 }
@@ -445,7 +449,7 @@ func (client *cliAdminClient) KillProcesses(addresses []fdbv1beta2.ProcessAddres
 		fdbv1beta2.ProcessAddressesStringWithoutFlags(addresses, " "),
 	)
 	// Run the kill command once with the max timeout to reduce the risk of multiple recoveries happening.
-	_, err := client.runCommand(cliCommand{command: killCommand, timeout: MaxCliTimeout})
+	_, err := client.runCommand(cliCommand{command: killCommand, timeout: client.getTimeout()})
 
 	return err
 }
@@ -488,7 +492,7 @@ func cleanConnectionStringOutput(input string) string {
 func (client *cliAdminClient) GetConnectionString() (string, error) {
 	// This will call directly the database and fetch the connection string
 	// from the system key space.
-	outputBytes, err := getConnectionStringFromDB(client.fdbLibClient, MaxCliTimeout)
+	outputBytes, err := getConnectionStringFromDB(client.fdbLibClient, client.getTimeout())
 
 	if err != nil {
 		return "", err
@@ -694,3 +698,17 @@ func (client *cliAdminClient) WithValues(keysAndValues ...interface{}) {
 	}
 	cmdRunner.log = newLogger
 }
+
+// SetTimeout will overwrite the default timeout for interacting the FDB cluster.
+func (client *cliAdminClient) SetTimeout(timeout time.Duration) {
+	client.timeout = timeout
+}
+
+// getTimeout will return the timeout that is specified for the admin client or otherwise the MaxCliTimeout.
+func (client *cliAdminClient) getTimeout() time.Duration {
+	if client.timeout == 0 {
+		return MaxCliTimeout
+	}
+
+	return client.timeout
+}
diff --git a/fdbclient/common.go b/fdbclient/common.go
index 6ecec8b1..ddd1a906 100644
--- a/fdbclient/common.go
+++ b/fdbclient/common.go
@@ -40,7 +40,7 @@ import (
 var DefaultCLITimeout = 10 * time.Second
 
 // MaxCliTimeout is the maximum CLI timeout that will be used for requests that might be slower to respond.
-var MaxCliTimeout = 60 * time.Second
+var MaxCliTimeout = 40 * time.Second
 
 const (
 	defaultTransactionTimeout = 5 * time.Second
diff --git a/pkg/fdbadminclient/admin_client.go b/pkg/fdbadminclient/admin_client.go
index 8d92b116..5e22701e 100644
--- a/pkg/fdbadminclient/admin_client.go
+++ b/pkg/fdbadminclient/admin_client.go
@@ -22,6 +22,7 @@ package fdbadminclient
 
 import (
 	fdbv1beta2 "github.com/FoundationDB/fdb-kubernetes-operator/api/v1beta2"
+	"time"
 )
 
 // AdminClient describes an interface for running administrative commands on a
@@ -115,4 +116,7 @@ type AdminClient interface {
 	// WithValues will update the logger used by the current AdminClient to contain the provided key value pairs. The provided
 	// arguments must be even.
 	WithValues(keysAndValues ...interface{})
+
+	// SetTimeout will overwrite the default timeout for interacting the FDB cluster.
+	SetTimeout(timeout time.Duration)
 }
diff --git a/pkg/fdbadminclient/mock/admin_client_mock.go b/pkg/fdbadminclient/mock/admin_client_mock.go
index 53eab762..b2bdcfa6 100644
--- a/pkg/fdbadminclient/mock/admin_client_mock.go
+++ b/pkg/fdbadminclient/mock/admin_client_mock.go
@@ -1039,3 +1039,6 @@ func (client *AdminClient) GetWorstDurabilityLag() (fdbv1beta2.FoundationDBStatu
 // WithValues will update the logger used by the current AdminClient to contain the provided key value pairs. The provided
 // arguments must be even.
 func (client *AdminClient) WithValues(_ ...interface{}) {}
+
+// SetTimeout will overwrite the default timeout for interacting the FDB cluster.
+func (client *AdminClient) SetTimeout(_ time.Duration) {}
diff --git a/setup/setup.go b/setup/setup.go
index 7abedd99..4963dc1d 100644
--- a/setup/setup.go
+++ b/setup/setup.go
@@ -91,7 +91,7 @@ func (o *Options) BindFlags(fs *flag.FlagSet) {
 	)
 	fs.StringVar(&o.LogFile, "log-file", "", "The path to a file to write logs to.")
 	fs.IntVar(&o.CliTimeout, "cli-timeout", 10, "The timeout to use for CLI commands in seconds.")
-	fs.IntVar(&o.MaxCliTimeout, "max-cli-timeout", 60, "The maximum timeout to use for CLI commands in seconds. This timeout is used for CLI requests that are known to be potentially slow like get status or exclude.")
+	fs.IntVar(&o.MaxCliTimeout, "max-cli-timeout", 40, "The maximum timeout to use for CLI commands in seconds. This timeout is used for CLI requests that are known to be potentially slow like get status or exclude.")
 	fs.IntVar(&o.MaxConcurrentReconciles, "max-concurrent-reconciles", 1, "Defines the maximum number of concurrent reconciles for all controllers.")
 	fs.BoolVar(&o.CleanUpOldLogFile, "cleanup-old-cli-logs", true, "Defines if the operator should delete old fdbcli log files.")
 	fs.DurationVar(&o.LogFileMinAge, "log-file-min-age", 5*time.Minute, "Defines the minimum age of fdbcli log files before removing when \"--cleanup-old-cli-logs\" is set.")


Test patch:
diff --git a/api/v1beta2/foundationdbcluster_types_test.go b/api/v1beta2/foundationdbcluster_types_test.go
index bca0a5d5..aa747e2a 100644
--- a/api/v1beta2/foundationdbcluster_types_test.go
+++ b/api/v1beta2/foundationdbcluster_types_test.go
@@ -2534,7 +2534,7 @@ var _ = Describe("[api] FoundationDBCluster", func() {
 			It("should set the correct value (-1) for log routers", func() {
 				spec := DatabaseConfiguration{}
 				spec.RemoteLogs = 9
-				normalized := spec.NormalizeConfigurationWithSeparatedProxies(version)
+				normalized := spec.NormalizeConfigurationWithSeparatedProxies(version, false)
 				Expect(normalized.LogRouters).To(Equal(-1))
 				Expect(normalized.RemoteLogs).To(Equal(9))
 			})
@@ -2582,7 +2582,7 @@ var _ = Describe("[api] FoundationDBCluster", func() {
 						},
 					},
 				}
-				normalized := spec.NormalizeConfigurationWithSeparatedProxies(version)
+				normalized := spec.NormalizeConfigurationWithSeparatedProxies(version, false)
 				Expect(normalized.Regions).To(Equal([]Region{
 					{
 						DataCenters: []DataCenter{
@@ -2666,7 +2666,7 @@ var _ = Describe("[api] FoundationDBCluster", func() {
 						},
 					},
 				}
-				normalized := spec.NormalizeConfigurationWithSeparatedProxies(version)
+				normalized := spec.NormalizeConfigurationWithSeparatedProxies(version, false)
 				Expect(normalized.Regions).To(Equal([]Region{
 					{
 						DataCenters: []DataCenter{
diff --git a/e2e/test_operator/operator_test.go b/e2e/test_operator/operator_test.go
index 4f658a42..1ac9315a 100644
--- a/e2e/test_operator/operator_test.go
+++ b/e2e/test_operator/operator_test.go
@@ -1691,4 +1691,48 @@ var _ = Describe("Operator", Label("e2e", "pr"), func() {
 			}).WithTimeout(10 * time.Minute).WithPolling(5 * time.Second).Should(BeNumerically("==", 1))
 		})
 	})
+
+	When("using proxies instead of grv and commit proxies", func() {
+		var originalRoleCounts fdbv1beta2.RoleCounts
+
+		BeforeEach(func() {
+			spec := fdbCluster.GetCluster().Spec.DeepCopy()
+			originalRoleCounts = spec.DatabaseConfiguration.RoleCounts
+			spec.DatabaseConfiguration.RoleCounts.Proxies = spec.DatabaseConfiguration.RoleCounts.GrvProxies + spec.DatabaseConfiguration.RoleCounts.CommitProxies
+			// Reset the more specific GRV and Commit proxy count.
+			spec.DatabaseConfiguration.RoleCounts.GrvProxies = 0
+			spec.DatabaseConfiguration.RoleCounts.CommitProxies = 0
+
+			fmt.Println("Original:", fixtures.ToJSON(originalRoleCounts), "new roleCounts:", fixtures.ToJSON(spec.DatabaseConfiguration.RoleCounts))
+			fdbCluster.UpdateClusterSpecWithSpec(spec)
+			Expect(fdbCluster.WaitForReconciliation()).NotTo(HaveOccurred())
+		})
+
+		AfterEach(func() {
+			spec := fdbCluster.GetCluster().Spec.DeepCopy()
+			spec.DatabaseConfiguration.RoleCounts = originalRoleCounts
+			fdbCluster.UpdateClusterSpecWithSpec(spec)
+			Expect(fdbCluster.WaitForReconciliation()).NotTo(HaveOccurred())
+		})
+
+		It("should configure the database to run with GRV and commit proxies but keep the proxies in the status field of the FoundationDB resource", func() {
+			// Make sure the FoundationDB configured GRV and commit proxies.
+			status := fdbCluster.GetStatus()
+			Expect(status.Cluster.DatabaseConfiguration.RoleCounts.CommitProxies).To(BeNumerically(">", 0))
+			Expect(status.Cluster.DatabaseConfiguration.RoleCounts.GrvProxies).To(BeNumerically(">", 0))
+			Expect(status.Cluster.DatabaseConfiguration.RoleCounts.Proxies).NotTo(BeZero())
+
+			fmt.Println("Status configuration:", fixtures.ToJSON(status.Cluster.DatabaseConfiguration))
+			// Verify the FoundationDB Cluster resource
+			cluster := fdbCluster.GetCluster()
+			// Make sure that the spec of the FoundationDB resource only has proxies defined.
+			Expect(cluster.Spec.DatabaseConfiguration.RoleCounts.GrvProxies).To(BeZero())
+			Expect(cluster.Spec.DatabaseConfiguration.RoleCounts.CommitProxies).To(BeZero())
+			Expect(cluster.Spec.DatabaseConfiguration.RoleCounts.Proxies).NotTo(BeZero())
+			// Make sure that the status of the FoundationDB resource only has proxies defined.
+			Expect(cluster.Status.DatabaseConfiguration.RoleCounts.GrvProxies).To(BeZero())
+			Expect(cluster.Status.DatabaseConfiguration.RoleCounts.CommitProxies).To(BeZero())
+			Expect(cluster.Status.DatabaseConfiguration.RoleCounts.Proxies).NotTo(BeZero())
+		})
+	})
 })