Repository: FoundationDB/fdb-kubernetes-operator
Pull request #1866 (2023-10-25 15:37:46)

Add some buffer for the exclusion logic to mitigate dead locks
# Description

Otherwise we might hit a case where the exclusion is blocked by one missing process.

## Type of change

*Please select one of the options below.*

- Bug fix (non-breaking change which fixes an issue)

## Discussion

*Are there any design details that you would like to discuss further?*

## Testing

Unit tests.

## Documentation

-

## Follow-up

We have to add a dedicated e2e tests for such a scenario.



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/exclude_processes.go b/controllers/exclude_processes.go
index c3bab9c9..d1772406 100644
--- a/controllers/exclude_processes.go
+++ b/controllers/exclude_processes.go
@@ -74,7 +74,6 @@ func (e excludeProcesses) reconcile(_ context.Context, r *FoundationDBClusterRec
 		}
 
 		desiredProcessesMap := desiredProcesses.Map()
-
 		for processClass := range fdbProcessesToExcludeByClass {
 			ongoingExclusions := ongoingExclusionsByClass[processClass]
 			processesToExclude := fdbProcessesToExcludeByClass[processClass]
@@ -91,7 +90,7 @@ func (e excludeProcesses) reconcile(_ context.Context, r *FoundationDBClusterRec
 			}
 
 			if len(processesToExclude) < allowedExclusions {
-				allowedExclusions = len(processesToExclude) - 1
+				allowedExclusions = len(processesToExclude)
 			}
 
 			// TODO: As a next step we could exclude transaction (log + stateless) processes together and exclude
@@ -104,7 +103,7 @@ func (e excludeProcesses) reconcile(_ context.Context, r *FoundationDBClusterRec
 
 		if len(fdbProcessesToExclude) == 0 {
 			return &requeue{
-				message:        "more exclusions needed but not allowed have to wait for new processes to come up",
+				message:        "more exclusions needed but not allowed, have to wait for new processes to come up",
 				delayedRequeue: true,
 			}
 		}
@@ -222,12 +221,15 @@ func canExcludeNewProcesses(logger logr.Logger, cluster *fdbv1beta2.FoundationDB
 	}
 
 	if !exclusionsAllowed {
+		logger.Info("Found at least one missing process, that was not missing for a long time", "missingProcesses", missingProcesses)
 		return 0, missingProcesses
 	}
 
 	logger.V(1).Info("canExcludeNewProcesses", "validProcesses", len(validProcesses), "desiredProcessCount", desiredProcessCount, "ongoingExclusions", ongoingExclusions)
 
 	// The assumption here is that we will only exclude a process if there is a replacement ready for it. We could relax
-	// this requirement in the future and take the fault tolerance into account.
-	return len(validProcesses) - desiredProcessCount - ongoingExclusions, missingProcesses
+	// this requirement in the future and take the fault tolerance into account. We add the desired fault tolerance to
+	// have some buffer to prevent cases where the operator might need to exclude more processes but there are more missing
+	// processes.
+	return cluster.DesiredFaultTolerance() + len(validProcesses) - desiredProcessCount - ongoingExclusions, missingProcesses
 }
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)
diff --git a/internal/removals/remove.go b/internal/removals/remove.go
index 90bf7c64..e39a0a07 100644
--- a/internal/removals/remove.go
+++ b/internal/removals/remove.go
@@ -23,6 +23,7 @@ package removals
 import (
 	"fmt"
 	"github.com/FoundationDB/fdb-kubernetes-operator/pkg/fdbstatus"
+	"math"
 	"net"
 
 	"github.com/FoundationDB/fdb-kubernetes-operator/pkg/fdbadminclient"
@@ -63,15 +64,26 @@ func GetProcessGroupsToRemove(removalMode fdbv1beta2.PodUpdateMode, removals map
 	}
 
 	if removalMode == fdbv1beta2.PodUpdateModeZone {
+		var pickedZone fdbv1beta2.FaultDomain
+		currentMaxZone := math.MinInt
+
+		// Pick the zone with the most processes in.
 		for zoneName, zoneProcesses := range removals {
 			if zoneName == TerminatingZone {
 				continue
 			}
 
-			// Fetch the first zone and stop
-			return zoneName, zoneProcesses, nil
+			if len(zoneProcesses) > currentMaxZone {
+				currentMaxZone = len(zoneProcesses)
+				pickedZone = zoneName
+			}
 		}
-		return "", nil, nil
+
+		if pickedZone == "" {
+			return "", nil, nil
+		}
+
+		return pickedZone, removals[pickedZone], nil
 	}
 
 	if removalMode == fdbv1beta2.PodUpdateModeNone {


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/controllers/exclude_processes_test.go b/controllers/exclude_processes_test.go
index 8e7cfc62..7db7472c 100644
--- a/controllers/exclude_processes_test.go
+++ b/controllers/exclude_processes_test.go
@@ -51,6 +51,8 @@ var _ = Describe("exclude_processes", func() {
 			generation, err := reloadCluster(cluster)
 			Expect(err).NotTo(HaveOccurred())
 			Expect(generation).To(Equal(int64(1)))
+
+			cluster.Spec.DatabaseConfiguration.RedundancyMode = fdbv1beta2.RedundancyModeSingle
 		})
 
 		AfterEach(func() {
@@ -72,6 +74,37 @@ var _ = Describe("exclude_processes", func() {
 					})
 				})
 
+				When("no additional processes are running", func() {
+					When("the redundancy mode is single", func() {
+						It("should not allow the exclusion", func() {
+							Expect(allowedExclusions).To(BeNumerically("==", 0))
+							Expect(missingProcesses).To(BeEmpty())
+						})
+					})
+
+					When("the redundancy mode is double", func() {
+						BeforeEach(func() {
+							cluster.Spec.DatabaseConfiguration.RedundancyMode = fdbv1beta2.RedundancyModeDouble
+						})
+
+						It("should not allow the exclusion", func() {
+							Expect(allowedExclusions).To(BeNumerically("==", cluster.DesiredFaultTolerance()))
+							Expect(missingProcesses).To(BeEmpty())
+						})
+					})
+
+					When("the redundancy mode is triple", func() {
+						BeforeEach(func() {
+							cluster.Spec.DatabaseConfiguration.RedundancyMode = fdbv1beta2.RedundancyModeTriple
+						})
+
+						It("should not allow the exclusion", func() {
+							Expect(allowedExclusions).To(BeNumerically("==", cluster.DesiredFaultTolerance()))
+							Expect(missingProcesses).To(BeEmpty())
+						})
+					})
+				})
+
 				When("one additional process is running", func() {
 					BeforeEach(func() {
 						cluster.Status.ProcessGroups = append(cluster.Status.ProcessGroups, fdbv1beta2.NewProcessGroupStatus("storage-1337", fdbv1beta2.ProcessClassStorage, nil))