Don't skip the exclusion for process groups without an address when using localities

controllers/replace_failed_process_groups_test.go

@@ -99,8 +99,7 @@ var _ = Describe("replace_failed_process_groups", func() {
 			cluster.Spec.AutomationOptions.Replacements.FailureDetectionTimeSeconds = pointer.Int(5)
 			cluster.Spec.AutomationOptions.Replacements.TaintReplacementTimeSeconds = pointer.Int(1)
 			// Update cluster config so that generic reconciliation will work
-			err := k8sClient.Update(ctx.TODO(), cluster)
-			Expect(err).NotTo(HaveOccurred())
+			Expect(k8sClient.Update(ctx.TODO(), cluster)).NotTo(HaveOccurred())
 
 			adminClient, err = mock.NewMockAdminClientUncast(cluster, k8sClient)
 
@@ -918,6 +917,38 @@ var _ = Describe("replace_failed_process_groups", func() {
 					Expect(getRemovedProcessGroupIDs(cluster)).To(Equal([]fdbv1beta2.ProcessGroupID{}))
 				})
 			})
+
+			When("the cluster uses localities for exclusions", func() {
+				BeforeEach(func() {
+					processGroup := fdbv1beta2.FindProcessGroupByID(cluster.Status.ProcessGroups, "storage-2")
+					processGroup.Addresses = nil
+
+					cluster.Spec.Version = fdbv1beta2.Versions.SupportsLocalityBasedExclusions71.String()
+					cluster.Status.RunningVersion = fdbv1beta2.Versions.SupportsLocalityBasedExclusions71.String()
+					cluster.Spec.AutomationOptions.UseLocalitiesForExclusion = pointer.Bool(true)
+					Expect(k8sClient.Update(ctx.TODO(), cluster)).NotTo(HaveOccurred())
+				})
+
+				It("should requeue", func() {
+					Expect(result).NotTo(BeNil())
+					Expect(result.message).To(Equal("Removals have been updated in the cluster status"))
+				})
+
+				It("should mark the process group for removal", func() {
+					Expect(getRemovedProcessGroupIDs(cluster)).To(Equal([]fdbv1beta2.ProcessGroupID{"storage-2"}))
+				})
+
+				It("should not skip the exclusion", func() {
+					for _, pg := range cluster.Status.ProcessGroups {
+						if pg.ProcessGroupID != "storage-2" {
+							continue
+						}
+
+						Expect(pg.ExclusionSkipped).To(BeFalse())
+					}
+				})
+
+			})
 		})
 
 		Context("with maintenance mode enabled", func() {

internal/replacements/replace_failed_process_groups.go

@@ -54,6 +54,7 @@ func ReplaceFailedProcessGroups(log logr.Logger, cluster *fdbv1beta2.FoundationD
 	maxReplacements := getMaxReplacements(cluster, cluster.GetMaxConcurrentAutomaticReplacements())
 	hasReplacement := false
 	crashLoopContainerProcessGroups := cluster.GetCrashLoopContainerProcessGroups()
+	localitiesUsedForExclusion := cluster.UseLocalitiesForExclusion()
 
 	for _, processGroupStatus := range cluster.Status.ProcessGroups {
 		// If a process group is already marked for removal we can skip it here.
@@ -92,7 +93,9 @@ func ReplaceFailedProcessGroups(log logr.Logger, cluster *fdbv1beta2.FoundationD
 		}
 
 		skipExclusion := false
-		if len(processGroupStatus.Addresses) == 0 {
+		// Only if localities are not used for exclusions we want to skip the exclusion. Skipping the exclusion could
+		// lead to a race condition, see: https://github.com/FoundationDB/fdb-kubernetes-operator/issues/1890
+		if len(processGroupStatus.Addresses) == 0 && !localitiesUsedForExclusion {
 			if !hasDesiredFaultTolerance {
 				log.Info(
 					"Skip process group with missing address",