Repository: FoundationDB/fdb-kubernetes-operator (go)

Issue:
Redwood does not work anymore with the operator in 7.3.x after it is was classified stable
### What happened?

Redwood was renamed from `ssd-redwood-1-experimental` to `ssd-redwood-1`. 

- Since the new value is not in the spec, i'm not able to enable it. 
- If I set it to `ssd-redwood-1-experimental`, the cluster will change its state, but the operator will forever error loop, since FoundationDB will report back that it is using `ssd-redwood-1` instead of `ssd-redwood-1-experimental` that the operator expects.

### What did you expect to happen?

`ssd-redwood-1` should be allowed in the spec as a valid value

### How can we reproduce it (as minimally and precisely as possible)?

use 7.3.x, change storage-engine to `ssd-redwood-1-experimental`

### Anything else we need to know?

_No response_

### FDB Kubernetes operator

1.26.0

### Kubernetes version

<details>

```console
Client Version: v1.28.2
Kustomize Version: v5.0.4-0.20230601165947-6ce0bf390ce3
Server Version: v1.28.1
```

</details>


### Cloud provider

<details>
microk8s
</details>


Pull request:
Add support for redwood storage engine
# Description

fixes #1856 

Added redwood-1

## Type of change

Not sure of the impact of adding a new value to the spec

## Discussion

*Are there any design details that you would like to discuss further?*

## Testing

*Please describe the tests that you ran to verify your changes. Unit tests?
Manual testing?*

*Do we need to perform additional testing once this is merged, or perform in a larger testing environment?*

## Documentation

*Did you update relevant documentation within this repository?*

*If this change is adding new functionality, do we need to describe it in our user manual?*

*If this change is adding or removing subreconcilers, have we updated the core technical design doc to reflect that?*

*If this change is adding new safety checks or new potential failure modes, have we documented and how to debug potential issues?*

## Follow-up

*Are there any follow-up issues that we should pursue in the future?*

*Does this introduce new defaults that we should re-evaluate in the future?*


Patch:
diff --git a/api/v1beta1/foundationdb_database_configuration.go b/api/v1beta1/foundationdb_database_configuration.go
index c44a552c..61c25b22 100644
--- a/api/v1beta1/foundationdb_database_configuration.go
+++ b/api/v1beta1/foundationdb_database_configuration.go
@@ -39,7 +39,7 @@ type DatabaseConfiguration struct {
 
 	// StorageEngine defines the storage engine the database uses.
 	// +kubebuilder:validation:Optional
-	// +kubebuilder:validation:Enum=ssd;ssd-1;ssd-2;memory;memory-1;memory-2;ssd-redwood-1-experimental;ssd-rocksdb-experimental;memory-radixtree-beta;custom
+	// +kubebuilder:validation:Enum=ssd;ssd-1;ssd-2;memory;memory-1;memory-2;ssd-redwood-1-experimental;ssd-redwood-1;ssd-rocksdb-experimental;memory-radixtree-beta;custom
 	// +kubebuilder:default:=ssd-2
 	StorageEngine StorageEngine `json:"storage_engine,omitempty"`
 
diff --git a/api/v1beta2/foundationdb_database_configuration.go b/api/v1beta2/foundationdb_database_configuration.go
index 103edc71..e3fb247d 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"
 )
 
@@ -39,7 +40,7 @@ type DatabaseConfiguration struct {
 
 	// StorageEngine defines the storage engine the database uses.
 	// +kubebuilder:validation:Optional
-	// +kubebuilder:validation:Enum=ssd;ssd-1;ssd-2;memory;memory-1;memory-2;ssd-redwood-1-experimental;ssd-rocksdb-experimental;ssd-rocksdb-v1;ssd-sharded-rocksdb;memory-radixtree-beta;custom
+	// +kubebuilder:validation:Enum=ssd;ssd-1;ssd-2;memory;memory-1;memory-2;ssd-redwood-1-experimental;ssd-redwood-1;ssd-rocksdb-experimental;ssd-rocksdb-v1;ssd-sharded-rocksdb;memory-radixtree-beta;custom
 	// +kubebuilder:default:=ssd-2
 	StorageEngine StorageEngine `json:"storage_engine,omitempty"`
 
@@ -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
@@ -767,6 +778,8 @@ const (
 	StorageEngineShardedRocksDB StorageEngine = "ssd-sharded-rocksdb"
 	// StorageEngineRedwood1Experimental defines the storage engine ssd-redwood-1-experimental.
 	StorageEngineRedwood1Experimental StorageEngine = "ssd-redwood-1-experimental"
+	// StorageEngineRedwood1 defines the storage engine ssd-redwood-1.
+	StorageEngineRedwood1 StorageEngine = "ssd-redwood-1"
 )
 
 // RoleCounts represents the roles whose counts can be customized.
@@ -791,6 +804,7 @@ func (counts RoleCounts) Map() map[ProcessClass]int {
 			countMap[role] = value
 		}
 	}
+
 	return countMap
 }
 
diff --git a/api/v1beta2/foundationdb_version.go b/api/v1beta2/foundationdb_version.go
index b68aff8a..706a8f14 100644
--- a/api/v1beta2/foundationdb_version.go
+++ b/api/v1beta2/foundationdb_version.go
@@ -190,6 +190,8 @@ func (version Version) IsStorageEngineSupported(storageEngine StorageEngine) boo
 		return version.IsAtLeast(Versions.SupportsShardedRocksDB)
 	} else if storageEngine == StorageEngineRedwood1Experimental {
 		return version.IsAtLeast(Versions.SupportsRedwood1Experimental)
+	} else if storageEngine == StorageEngineRedwood1 {
+		return version.IsAtLeast(Versions.SupportsRedwood1)
 	}
 	return true
 }
@@ -229,6 +231,7 @@ var Versions = struct {
 	SupportsIsPresent,
 	SupportsShardedRocksDB,
 	SupportsRedwood1Experimental,
+	SupportsRedwood1,
 	IncompatibleVersion,
 	PreviousPatchVersion,
 	SupportsRecoveryState,
@@ -245,6 +248,7 @@ var Versions = struct {
 	SupportsIsPresent:            Version{Major: 7, Minor: 1, Patch: 4},
 	SupportsShardedRocksDB:       Version{Major: 7, Minor: 2, Patch: 0},
 	SupportsRedwood1Experimental: Version{Major: 7, Minor: 0, Patch: 0},
+	SupportsRedwood1:             Version{Major: 7, Minor: 3, Patch: 0},
 	SupportsRecoveryState:        Version{Major: 7, Minor: 1, Patch: 22},
 	SupportsDNSInClusterFile:     Version{Major: 7, Minor: 0, Patch: 0},
 }
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/config/crd/bases/apps.foundationdb.org_foundationdbclusters.yaml b/config/crd/bases/apps.foundationdb.org_foundationdbclusters.yaml
index 7c2371cb..e02c00dd 100644
--- a/config/crd/bases/apps.foundationdb.org_foundationdbclusters.yaml
+++ b/config/crd/bases/apps.foundationdb.org_foundationdbclusters.yaml
@@ -847,6 +847,7 @@ spec:
                     - memory-1
                     - memory-2
                     - ssd-redwood-1-experimental
+                    - ssd-redwood-1
                     - ssd-rocksdb-experimental
                     - memory-radixtree-beta
                     - custom
@@ -9863,6 +9864,7 @@ spec:
                     - memory-1
                     - memory-2
                     - ssd-redwood-1-experimental
+                    - ssd-redwood-1
                     - ssd-rocksdb-experimental
                     - memory-radixtree-beta
                     - custom
@@ -10397,6 +10399,7 @@ spec:
                     - memory-1
                     - memory-2
                     - ssd-redwood-1-experimental
+                    - ssd-redwood-1
                     - ssd-rocksdb-experimental
                     - ssd-rocksdb-v1
                     - ssd-sharded-rocksdb
@@ -14133,6 +14136,7 @@ spec:
                     - memory-1
                     - memory-2
                     - ssd-redwood-1-experimental
+                    - ssd-redwood-1
                     - ssd-rocksdb-experimental
                     - ssd-rocksdb-v1
                     - ssd-sharded-rocksdb
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))