Repository: FoundationDB/fdb-kubernetes-operator
Pull request #1868 (2023-10-27 15:19:44)

Correct the version check for the locality based exclusions
# Description

Make sure the operator only uses locality based exclusions for versions that properly support locality based exclusions.

## Type of change

*Please select one of the options below.*

- Bug fix (non-breaking change which fixes an issue)

## Discussion

-

## Testing

Added unit tests and updated the FDB versions used for testing.

## Documentation

Updated the cluster docs.

## Follow-up

-


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

* Commit ID: bcf036a8f2dcfc93434ffa7d8144c20897ab4f61
* Duration 2:43:45
* Result: :x: **FAILED**
* Error: `Error while executing command: if $fail_test; then exit 1; fi. Reason: exit status 1`
* [Build Log](https://d1e0l78xbkh3xa.cloudfront.net/fdb-kubernetes-operator/fdb-kubernetes-operator-pr/e199e83a-abb0-4717-8b1c-ef09cff286a7.gz) terminal output (available for 30 days)
* [Build Workspace](https://d1e0l78xbkh3xa.cloudfront.net/fdb-kubernetes-operator-pr/e199e83a-abb0-4717-8b1c-ef09cff286a7/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 e3fb247d..a40dd1d8 100644
--- a/api/v1beta2/foundationdb_database_configuration.go
+++ b/api/v1beta2/foundationdb_database_configuration.go
@@ -718,10 +718,8 @@ func MinimumFaultDomains(redundancyMode RedundancyMode) int {
 		return 1
 	case RedundancyModeDouble, RedundancyModeUnset:
 		return 2
-	case RedundancyModeTriple:
+	case RedundancyModeTriple, RedundancyModeThreeDataHall:
 		return 3
-	case RedundancyModeThreeDataHall:
-		return 4
 	default:
 		return 1
 	}
diff --git a/api/v1beta2/foundationdb_version.go b/api/v1beta2/foundationdb_version.go
index 706a8f14..30021b1c 100644
--- a/api/v1beta2/foundationdb_version.go
+++ b/api/v1beta2/foundationdb_version.go
@@ -221,6 +221,17 @@ func (version Version) SupportsVersionChange(other Version) bool {
 	return version.IsProtocolCompatible(other) || other.IsAtLeast(version)
 }
 
+// SupportsLocalityBasedExclusions returns true if the current version supports locality based exclusions.
+func (version Version) SupportsLocalityBasedExclusions() bool {
+	// If the version is 7.1.* we have to check if it supports locality based exclusions. For all newer versions
+	// we will check against the 7.3 version.
+	if version.IsProtocolCompatible(Version{Major: 7, Minor: 1, Patch: 0}) {
+		return version.IsAtLeast(Versions.SupportsLocalityBasedExclusions71)
+	}
+
+	return version.IsAtLeast(Versions.SupportsLocalityBasedExclusions)
+}
+
 // Versions provides a shorthand for known versions.
 // This is only to be used in testing.
 var Versions = struct {
@@ -236,19 +247,23 @@ var Versions = struct {
 	PreviousPatchVersion,
 	SupportsRecoveryState,
 	SupportsDNSInClusterFile,
+	SupportsLocalityBasedExclusions71,
+	SupportsLocalityBasedExclusions,
 	Default Version
 }{
-	Default:                      Version{Major: 6, Minor: 2, Patch: 21},
-	IncompatibleVersion:          Version{Major: 6, Minor: 1, Patch: 0},
-	PreviousPatchVersion:         Version{Major: 6, Minor: 2, Patch: 20},
-	NextPatchVersion:             Version{Major: 6, Minor: 2, Patch: 22},
-	NextMajorVersion:             Version{Major: 7, Minor: 0, Patch: 0},
-	MinimumVersion:               Version{Major: 6, Minor: 2, Patch: 20},
-	SupportsRocksDBV1:            Version{Major: 7, Minor: 1, Patch: 0, ReleaseCandidate: 4},
-	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},
+	Default:                           Version{Major: 6, Minor: 2, Patch: 21},
+	IncompatibleVersion:               Version{Major: 6, Minor: 1, Patch: 0},
+	PreviousPatchVersion:              Version{Major: 6, Minor: 2, Patch: 20},
+	NextPatchVersion:                  Version{Major: 6, Minor: 2, Patch: 22},
+	NextMajorVersion:                  Version{Major: 7, Minor: 0, Patch: 0},
+	MinimumVersion:                    Version{Major: 6, Minor: 2, Patch: 20},
+	SupportsRocksDBV1:                 Version{Major: 7, Minor: 1, Patch: 0, ReleaseCandidate: 4},
+	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},
+	SupportsLocalityBasedExclusions71: Version{Major: 7, Minor: 1, Patch: 42},
+	SupportsLocalityBasedExclusions:   Version{Major: 7, Minor: 3, Patch: 26},
 }
diff --git a/api/v1beta2/foundationdbcluster_types.go b/api/v1beta2/foundationdbcluster_types.go
index dd9afeee..d0a2244c 100644
--- a/api/v1beta2/foundationdbcluster_types.go
+++ b/api/v1beta2/foundationdbcluster_types.go
@@ -1086,7 +1086,8 @@ type FoundationDBClusterAutomationOptions struct {
 	// The default is false.
 	UseNonBlockingExcludes *bool `json:"useNonBlockingExcludes,omitempty"`
 
-	// UseLocalitiesForExclusion defines whether the exclusions are done using localities instead of IP addresses.
+	// UseLocalitiesForExclusion defines whether the exclusions are done using localities instead of IP addresses. This
+	// feature requires at least FDB 7.1.42 or 7.3.26.
 	// The default is false.
 	UseLocalitiesForExclusion *bool `json:"useLocalitiesForExclusion,omitempty"`
 
@@ -2248,13 +2249,14 @@ func (cluster *FoundationDBCluster) GetUseNonBlockingExcludes() bool {
 
 // UseLocalitiesForExclusion returns the value of UseLocalitiesForExclusion or false if unset.
 func (cluster *FoundationDBCluster) UseLocalitiesForExclusion() bool {
-	fdbVersion, err := ParseFdbVersion(cluster.Spec.Version)
+	fdbVersion, err := ParseFdbVersion(cluster.GetRunningVersion())
 	if err != nil {
 		// Fall back to use exclusions with IP if we can't parse the version.
 		// This should never happen since the version is validated in earlier steps.
 		return false
 	}
-	return fdbVersion.IsAtLeast(Versions.NextMajorVersion) && pointer.BoolDeref(cluster.Spec.AutomationOptions.UseLocalitiesForExclusion, false)
+
+	return fdbVersion.SupportsLocalityBasedExclusions() && pointer.BoolDeref(cluster.Spec.AutomationOptions.UseLocalitiesForExclusion, false)
 }
 
 // GetProcessClassLabel provides the label that this cluster is using for the
diff --git a/docs/cluster_spec.md b/docs/cluster_spec.md
index 2b42b90d..896f251e 100644
--- a/docs/cluster_spec.md
+++ b/docs/cluster_spec.md
@@ -183,7 +183,7 @@ FoundationDBClusterAutomationOptions provides flags for enabling or disabling op
 | replacements | Replacements contains options for automatically replacing failed processes. | [AutomaticReplacementOptions](#automaticreplacementoptions) | false |
 | ignorePendingPodsDuration | IgnorePendingPodsDuration defines how long a Pod has to be in the Pending Phase before ignore it during reconciliation. This prevents Pod that are stuck in Pending to block further reconciliation. | time.Duration | false |
 | useNonBlockingExcludes | UseNonBlockingExcludes defines whether the operator is allowed to use non blocking exclude commands. The default is false. | *bool | false |
-| useLocalitiesForExclusion | UseLocalitiesForExclusion defines whether the exclusions are done using localities instead of IP addresses. The default is false. | *bool | false |
+| useLocalitiesForExclusion | UseLocalitiesForExclusion defines whether the exclusions are done using localities instead of IP addresses. This feature requires at least FDB 7.1.42 or 7.3.26. The default is false. | *bool | false |
 | ignoreTerminatingPodsSeconds | IgnoreTerminatingPodsSeconds defines how long a Pod has to be in the Terminating Phase before we ignore it during reconciliation. This prevents Pod that are stuck in Terminating to block further reconciliation. | *int | false |
 | ignoreMissingProcessesSeconds | IgnoreMissingProcessesSeconds defines how long a process group has to be in the MissingProcess condition until it will be ignored during reconciliation. This prevents that a process will block reconciliation. | *int | false |
 | failedPodDurationSeconds | FailedPodDurationSeconds defines the duration a Pod can stay in the deleted state (deletionTimestamp != 0) before it gets marked as PodFailed. This is important in cases where a fdbserver process is still reporting but the Pod resource is marked for deletion. This can happen when the kubelet or a node fails. Setting this condition will ensure that the operator is replacing affected Pods. | *int | false |


Test patch:
diff --git a/api/v1beta2/foundationdb_version_test.go b/api/v1beta2/foundationdb_version_test.go
index 132a2314..a3f9bff5 100644
--- a/api/v1beta2/foundationdb_version_test.go
+++ b/api/v1beta2/foundationdb_version_test.go
@@ -251,4 +251,54 @@ var _ = Describe("[api] FDBVersion", func() {
 			false,
 		),
 	)
+
+	DescribeTable("validating if the provided version supports locality based exclusions", func(version Version, expected bool) {
+		Expect(version.SupportsLocalityBasedExclusions()).To(Equal(expected))
+	},
+		Entry(
+			"Version is 6.3",
+			Version{
+				Major: 6,
+				Minor: 3,
+				Patch: 0,
+			},
+			false,
+		),
+		Entry(
+			"Version is 7.1.41",
+			Version{
+				Major: 7,
+				Minor: 1,
+				Patch: 41,
+			},
+			false,
+		),
+		Entry(
+			"Version is 7.1.42",
+			Version{
+				Major: 7,
+				Minor: 1,
+				Patch: 42,
+			},
+			true,
+		),
+		Entry(
+			"Version is 7.3.25",
+			Version{
+				Major: 7,
+				Minor: 3,
+				Patch: 25,
+			},
+			false,
+		),
+		Entry(
+			"Version is 7.3.36",
+			Version{
+				Major: 7,
+				Minor: 3,
+				Patch: 26,
+			},
+			true,
+		),
+	)
 })
diff --git a/controllers/exclude_processes_test.go b/controllers/exclude_processes_test.go
index 95b9b184..a9da13f2 100644
--- a/controllers/exclude_processes_test.go
+++ b/controllers/exclude_processes_test.go
@@ -223,7 +223,7 @@ var _ = Describe("exclude_processes", func() {
 					Expect(missingProcesses).To(Equal([]fdbv1beta2.ProcessGroupID{"storage-1", "storage-2"}))
 				})
 
-				FWhen("the missing timestamp is older than 5 minutes", func() {
+				When("the missing timestamp is older than 5 minutes", func() {
 					BeforeEach(func() {
 						for idx, processGroup := range cluster.Status.ProcessGroups {
 							timestamp := processGroup.GetConditionTime(fdbv1beta2.MissingProcesses)
@@ -509,7 +509,7 @@ var _ = Describe("exclude_processes", func() {
 
 		Context("cluster supports locality based exclusions", func() {
 			BeforeEach(func() {
-				cluster.Spec.Version = fdbv1beta2.Versions.NextMajorVersion.String()
+				cluster.Spec.Version = fdbv1beta2.Versions.SupportsLocalityBasedExclusions.String()
 			})
 
 			When("there are no exclusions", func() {
diff --git a/controllers/remove_process_groups_test.go b/controllers/remove_process_groups_test.go
index 314b356c..f1460a4d 100644
--- a/controllers/remove_process_groups_test.go
+++ b/controllers/remove_process_groups_test.go
@@ -117,7 +117,7 @@ var _ = Describe("remove_process_groups", func() {
 					})
 				})
 
-				When("the cluster has degraded stroage fault tolerance", func() {
+				When("the cluster has degraded storage fault tolerance", func() {
 					BeforeEach(func() {
 						adminClient, err := mock.NewMockAdminClientUncast(cluster, k8sClient)
 						Expect(err).NotTo(HaveOccurred())
@@ -192,6 +192,62 @@ var _ = Describe("remove_process_groups", func() {
 				})
 			})
 
+			When("the cluster has three_data_hall redundancy", func() {
+				BeforeEach(func() {
+					adminClient, err := mock.NewMockAdminClientUncast(cluster, k8sClient)
+					Expect(err).NotTo(HaveOccurred())
+					adminClient.DatabaseConfiguration.RedundancyMode = fdbv1beta2.RedundancyModeThreeDataHall
+
+				})
+				When("storage have 3 replicas", func() {
+					BeforeEach(func() {
+						adminClient, err := mock.NewMockAdminClientUncast(cluster, k8sClient)
+						Expect(err).NotTo(HaveOccurred())
+						adminClient.TeamTracker = []fdbv1beta2.FoundationDBStatusTeamTracker{
+							{
+								Primary: true,
+								State: fdbv1beta2.FoundationDBStatusDataState{
+									Healthy:              true,
+									MinReplicasRemaining: 3,
+								},
+							},
+						}
+					})
+					It("should successfully remove that process group", func() {
+						Expect(result).To(BeNil())
+						// Ensure resources are deleted
+						removed, include, err := confirmRemoval(context.Background(), globalControllerLogger, clusterReconciler, cluster, removedProcessGroup)
+						Expect(err).To(BeNil())
+						Expect(removed).To(BeTrue())
+						Expect(include).To(BeTrue())
+					})
+				})
+				When("storage have 2 replicas", func() {
+					BeforeEach(func() {
+						adminClient, err := mock.NewMockAdminClientUncast(cluster, k8sClient)
+						Expect(err).NotTo(HaveOccurred())
+						adminClient.TeamTracker = []fdbv1beta2.FoundationDBStatusTeamTracker{
+							{
+								Primary: true,
+								State: fdbv1beta2.FoundationDBStatusDataState{
+									Healthy:              true,
+									MinReplicasRemaining: 2,
+								},
+							},
+						}
+					})
+					It("should not remove the process group and should not exclude processes", func() {
+						Expect(result).NotTo(BeNil())
+						Expect(result.message).To(Equal("Removals cannot proceed because cluster has degraded fault tolerance"))
+						// Ensure resources are not deleted
+						removed, include, err := confirmRemoval(context.Background(), globalControllerLogger, clusterReconciler, cluster, removedProcessGroup)
+						Expect(err).To(BeNil())
+						Expect(removed).To(BeFalse())
+						Expect(include).To(BeFalse())
+					})
+				})
+			})
+
 			When("removing multiple process groups", func() {
 				var initialCnt int
 				var secondRemovedProcessGroup *fdbv1beta2.ProcessGroupStatus
@@ -470,7 +526,7 @@ var _ = Describe("remove_process_groups", func() {
 
 		Context("cluster support inclusions using locality", func() {
 			BeforeEach(func() {
-				cluster.Spec.Version = fdbv1beta2.Versions.NextMajorVersion.String()
+				cluster.Spec.Version = fdbv1beta2.Versions.SupportsLocalityBasedExclusions.String()
 			})
 
 			When("including no process", func() {
diff --git a/e2e/Makefile b/e2e/Makefile
index b44d6821..3030a134 100644
--- a/e2e/Makefile
+++ b/e2e/Makefile
@@ -10,9 +10,9 @@ TIMEOUT?=168h
 NAMESPACE?=
 PREFIX?=
 CONTEXT?=
-FDB_VERSION?=7.1.37
+FDB_VERSION?=7.1.43
 # This will be the version used for upgrade tests.
-NEXT_FDB_VERSION?=7.3.15
+NEXT_FDB_VERSION?=7.3.25
 ## Expectation is that you are running standard build image which generates both regular and debug (Symbols) images.
 FDB_IMAGE?=foundationdb/foundationdb:$(FDB_VERSION)
 SIDECAR_IMAGE?=foundationdb/foundationdb-kubernetes-sidecar:$(FDB_VERSION)-1