Repository: FoundationDB/fdb-kubernetes-operator
Pull request #1863 (2023-10-25 09:24:23)

Make use of string builder for getting the configuration string
# Description

Refactoring of the get configuration string to make use of the string builder and directly iterate over the map.

## Type of change

*Please select one of the options below.*

- Other

## Discussion

-

## Testing

Ran unit tests and CI will run e2e tests.

## Documentation

-

## Follow-up

-



Patch:
diff --git a/api/v1beta2/foundationdb_database_configuration.go b/api/v1beta2/foundationdb_database_configuration.go
index 103edc71..c07e7794 100644
--- a/api/v1beta2/foundationdb_database_configuration.go
+++ b/api/v1beta2/foundationdb_database_configuration.go
@@ -26,6 +26,7 @@ import (
 	"html/template"
 	"reflect"
 	"sort"
+	"strconv"
 	"strings"
 )
 
@@ -233,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, areSeparatedProxiesConfigured bool) DatabaseConfiguration {
+func (configuration DatabaseConfiguration) NormalizeConfigurationWithSeparatedProxies(version string) DatabaseConfiguration {
 	result := configuration.NormalizeConfiguration()
 
 	parsedVersion, _ := ParseFdbVersion(version)
 	if parsedVersion.HasSeparatedProxies() {
-		if !areSeparatedProxiesConfigured {
+		if !configuration.AreSeparatedProxiesConfigured() {
 			result.GrvProxies = 0
 			result.CommitProxies = 0
 		} else {
@@ -579,8 +580,7 @@ func (configuration DatabaseConfiguration) getRegionPriorities() map[string]int
 // commit_proxies are greater than 0 (explicitly set) and Proxies is set
 // to 0
 func (configuration DatabaseConfiguration) AreSeparatedProxiesConfigured() bool {
-	counts := configuration.RoleCounts
-	return counts.GrvProxies > 0 || counts.CommitProxies > 0
+	return configuration.RoleCounts.GrvProxies > 0 || configuration.RoleCounts.CommitProxies > 0
 }
 
 // GetProxiesString returns a string that contains the correct fdbcli
@@ -620,32 +620,42 @@ func (configuration DatabaseConfiguration) FillInDefaultsFromStatus() DatabaseCo
 
 // GetConfigurationString gets the CLI command for configuring a database.
 func (configuration DatabaseConfiguration) GetConfigurationString(version string) (string, error) {
-	configurationString := fmt.Sprintf("%s %s", configuration.RedundancyMode, configuration.StorageEngine)
+	var configurationString strings.Builder
+	configurationString.WriteString(string(configuration.RedundancyMode))
+	configurationString.WriteString(" ")
+	configurationString.WriteString(string(configuration.StorageEngine))
 
 	fdbVersion, err := ParseFdbVersion(version)
 	if err != nil {
-		return configurationString, err
+		return "", err
 	}
 
-	counts := configuration.RoleCounts.Map()
-	configurationString += fmt.Sprintf(" usable_regions=%d", configuration.UsableRegions)
-	// TODO: roleNames !
+	configurationString.WriteString(" usable_regions=")
+	configurationString.WriteString(strconv.Itoa(configuration.UsableRegions))
+
+	roleCounts := configuration.RoleCounts.Map()
 	for _, role := range roleNames {
 		if role == "proxies" || role == "commit_proxies" || role == "grv_proxies" {
 			continue
 		}
 
 		if role != ProcessClassStorage {
-			configurationString += fmt.Sprintf(" %s=%d", role, counts[role])
+			configurationString.WriteString(" ")
+			configurationString.WriteString(string(role))
+			configurationString.WriteString("=")
+			configurationString.WriteString(strconv.Itoa(roleCounts[role]))
 		}
 	}
 
-	configurationString += configuration.GetProxiesString(fdbVersion)
+	configurationString.WriteString(configuration.GetProxiesString(fdbVersion))
 
 	flags := configuration.VersionFlags.Map()
 	for flag, value := range flags {
 		if value != 0 {
-			configurationString += fmt.Sprintf(" %s:=%d", flag, value)
+			configurationString.WriteString(" ")
+			configurationString.WriteString(flag)
+			configurationString.WriteString(":=")
+			configurationString.WriteString(strconv.Itoa(value))
 		}
 	}
 
@@ -660,9 +670,10 @@ func (configuration DatabaseConfiguration) GetConfigurationString(version string
 		regionString = template.JSEscapeString(string(regionBytes))
 	}
 
-	configurationString += " regions=" + regionString
+	configurationString.WriteString(" regions=")
+	configurationString.WriteString(regionString)
 
-	return configurationString, nil
+	return configurationString.String(), nil
 }
 
 // FillInDefaultVersionFlags adds in missing version flags so they match the
@@ -791,6 +802,7 @@ func (counts RoleCounts) Map() map[ProcessClass]int {
 			countMap[role] = value
 		}
 	}
+
 	return countMap
 }
 
diff --git a/api/v1beta2/foundationdbcluster_types.go b/api/v1beta2/foundationdbcluster_types.go
index aeaa1d5c..dd9afeee 100644
--- a/api/v1beta2/foundationdbcluster_types.go
+++ b/api/v1beta2/foundationdbcluster_types.go
@@ -1814,7 +1814,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(), cluster.Spec.DatabaseConfiguration.AreSeparatedProxiesConfigured())
+	configuration := cluster.Spec.DatabaseConfiguration.NormalizeConfigurationWithSeparatedProxies(cluster.GetRunningVersion())
 	configuration.RoleCounts = cluster.GetRoleCountsWithDefaults()
 	configuration.RoleCounts.Storage = 0
 
diff --git a/controllers/add_process_groups.go b/controllers/add_process_groups.go
index 200dc99a..24fe5f2e 100644
--- a/controllers/add_process_groups.go
+++ b/controllers/add_process_groups.go
@@ -56,7 +56,7 @@ func (a addProcessGroups) reconcile(ctx context.Context, r *FoundationDBClusterR
 			continue
 		}
 
-		logger.Info("Adding new Process Groups", "processClass", processClass, "newCount", newCount)
+		logger.Info("Adding new Process Groups", "processClass", processClass, "newCount", newCount, "desiredCount", desiredCount, "currentCount", processCounts[processClass])
 		r.Recorder.Event(cluster, corev1.EventTypeNormal, "AddingProcesses", fmt.Sprintf("Adding %d %s processes", newCount, processClass))
 		idNum := 1
 
diff --git a/controllers/update_database_configuration.go b/controllers/update_database_configuration.go
index e65e5d0b..e93c993d 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, cluster.Spec.DatabaseConfiguration.AreSeparatedProxiesConfigured())
+	currentConfiguration := status.Cluster.DatabaseConfiguration.NormalizeConfigurationWithSeparatedProxies(cluster.Spec.Version)
 	// 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 2e05fbda..047d5191 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, cluster.Spec.DatabaseConfiguration.AreSeparatedProxiesConfigured())
+		clusterStatus.DatabaseConfiguration = databaseStatus.Cluster.DatabaseConfiguration.NormalizeConfigurationWithSeparatedProxies(cluster.Spec.Version)
 		// Removing excluded servers as we don't want them during comparison.
 		clusterStatus.DatabaseConfiguration.ExcludedServers = nil
 		cluster.ClearMissingVersionFlags(&clusterStatus.DatabaseConfiguration)


Test patch:
diff --git a/api/v1beta2/foundationdbcluster_types_test.go b/api/v1beta2/foundationdbcluster_types_test.go
index aa747e2a..bca0a5d5 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, false)
+				normalized := spec.NormalizeConfigurationWithSeparatedProxies(version)
 				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, false)
+				normalized := spec.NormalizeConfigurationWithSeparatedProxies(version)
 				Expect(normalized.Regions).To(Equal([]Region{
 					{
 						DataCenters: []DataCenter{
@@ -2666,7 +2666,7 @@ var _ = Describe("[api] FoundationDBCluster", func() {
 						},
 					},
 				}
-				normalized := spec.NormalizeConfigurationWithSeparatedProxies(version, false)
+				normalized := spec.NormalizeConfigurationWithSeparatedProxies(version)
 				Expect(normalized.Regions).To(Equal([]Region{
 					{
 						DataCenters: []DataCenter{
diff --git a/e2e/fixtures/fdb_cluster_specs.go b/e2e/fixtures/fdb_cluster_specs.go
index 283e1438..fbab45ce 100644
--- a/e2e/fixtures/fdb_cluster_specs.go
+++ b/e2e/fixtures/fdb_cluster_specs.go
@@ -23,6 +23,7 @@ package fixtures
 import (
 	fdbv1beta2 "github.com/FoundationDB/fdb-kubernetes-operator/api/v1beta2"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/utils/pointer"
 )
@@ -103,11 +104,16 @@ func (factory *Factory) createPodTemplate(
 ) *corev1.PodTemplateSpec {
 	// the operator is causing this to not work as desired reference:
 	// https://github.com/FoundationDB/fdb-kubernetes-operator/blob/main/internal/deprecations.go#L75-L77
-	fdbPodResources := corev1.ResourceRequirements{
+	mainContainerResources := corev1.ResourceRequirements{
 		Requests: resources,
 	}
-	if setLimits {
-		fdbPodResources.Limits = resources
+
+	// See: https://github.com/FoundationDB/fdb-kubernetes-operator/blob/main/docs/manual/warnings.md#resource-requirements otherwise
+	// the operator will set the default limits.
+	if !setLimits {
+		mainContainerResources.Limits = corev1.ResourceList{
+			"org.foundationdb/empty": resource.MustParse("0"),
+		}
 	}
 	var initContainers []corev1.Container
 	var annotations map[string]string
@@ -120,7 +126,7 @@ func (factory *Factory) createPodTemplate(
 				// We use fdbResources here to speed up the cluster creation and prevent some OOM kills when the sidecar
 				// copies the binaries. In addition to that the init container won't count to the required resources
 				// since the init container requires less than the running containers later (fdbResources + sideResources)
-				Resources: fdbPodResources,
+				Resources: mainContainerResources,
 			},
 		}
 		annotations = map[string]string{
@@ -174,7 +180,7 @@ func (factory *Factory) createPodTemplate(
 				{
 					Name:            fdbv1beta2.MainContainerName,
 					ImagePullPolicy: factory.getImagePullPolicy(),
-					Resources:       fdbPodResources,
+					Resources:       mainContainerResources,
 					SecurityContext: &corev1.SecurityContext{
 						Privileged:               pointer.Bool(true),
 						AllowPrivilegeEscalation: pointer.Bool(true), // for performance profiling
diff --git a/e2e/test_operator_upgrades/operator_upgrades_test.go b/e2e/test_operator_upgrades/operator_upgrades_test.go
index 063b0628..95514438 100644
--- a/e2e/test_operator_upgrades/operator_upgrades_test.go
+++ b/e2e/test_operator_upgrades/operator_upgrades_test.go
@@ -328,7 +328,7 @@ var _ = Describe("Operator Upgrades", Label("e2e", "pr"), func() {
 			// Make sure that the incompatible connections are cleaned up after some time.
 			Eventually(func() []string {
 				return fdbCluster.GetStatus().Cluster.IncompatibleConnections
-			}).WithTimeout(5 * time.Minute).WithPolling(5 * time.Second).MustPassRepeatedly(5).Should(BeEmpty())
+			}).WithTimeout(10 * time.Minute).WithPolling(5 * time.Second).MustPassRepeatedly(5).Should(BeEmpty())
 
 			// Make sure the cluster has no data loss.
 			fdbCluster.EnsureTeamTrackersHaveMinReplicas()