Repository: FoundationDB/fdb-kubernetes-operator
Pull request #1129 (2022-03-19 12:20:01)

Fix flaky unit test and bug in process group removal
# Description

Fixes: https://github.com/FoundationDB/fdb-kubernetes-operator/issues/1078

## Type of change

*Please select one of the options below.*

- Bug fix (non-breaking change which fixes an issue)

# Discussion

There was actual a bug in the old implementation in a way that we didn't added a processes group to the removal list without exclusions if the process was already marked as removal.

# Testing

Local + unit.

# Documentation

-

# Follow-up

-


foundationdb-ci: 
### AWS CodeBuild CI Report for Linux CentOS 7

* CodeBuild project: fdb-kubernetes-operator-pr
* Commit ID: dc16a0eca9a71888283d6784fb0009876c63f2a9
* Result: FAILED
* Error: `Error while executing command: make -C tests -kj prOperator. Reason: exit status 2`
* [Build Logs](https://d1e0l78xbkh3xa.cloudfront.net/build-logs/fdb-kubernetes-operator/fdb-kubernetes-operator-pr/86d13607-eb9a-4e79-b685-bec195a405e8.gz) (available for 30 days)

foundationdb-ci: 
### AWS CodeBuild CI Report for Linux CentOS 7

* CodeBuild project: fdb-kubernetes-operator-pr
* Commit ID: 5445f35b2c6a05181eb2e36c7905b222382e73d9
* Result: FAILED
* Error: `Error while executing command: make -C tests -kj prOperator. Reason: exit status 2`
* [Build Logs](https://d1e0l78xbkh3xa.cloudfront.net/build-logs/fdb-kubernetes-operator/fdb-kubernetes-operator-pr/ad3fae77-08bc-4bac-b319-817ac8e3b433.gz) (available for 30 days)

foundationdb-ci: 
### AWS CodeBuild CI Report for Linux CentOS 7

* CodeBuild project: fdb-kubernetes-operator-pr
* Commit ID: 61c691c9dd807c75c78a3e6655125192b1da7649
* Result: FAILED
* Error: `Error while executing command: make -C tests -kj prOperator. Reason: exit status 2`
* [Build Logs](https://d1e0l78xbkh3xa.cloudfront.net/build-logs/fdb-kubernetes-operator/fdb-kubernetes-operator-pr/f0c712ae-265e-4e88-ae8e-cf273239226d.gz) (available for 30 days)


Patch:
diff --git a/api/v1beta2/foundationdbcluster_types.go b/api/v1beta2/foundationdbcluster_types.go
index be91a526..840d3223 100644
--- a/api/v1beta2/foundationdbcluster_types.go
+++ b/api/v1beta2/foundationdbcluster_types.go
@@ -1994,3 +1994,39 @@ func (cluster *FoundationDBCluster) GetUseUnifiedImage() bool {
 func (cluster *FoundationDBCluster) GetIgnoreTerminatingPodsSeconds() int {
 	return pointer.IntDeref(cluster.Spec.AutomationOptions.IgnoreTerminatingPodsSeconds, int((10 * time.Minute).Seconds()))
 }
+
+// AddProcessGroupsToRemovalList adds the provided process group IDs to the remove list.
+// If a process group ID is already present on that list it won't be added a second time.
+func (cluster *FoundationDBCluster) AddProcessGroupsToRemovalList(processGroupIDs []string) {
+	removals := map[string]None{}
+
+	for _, id := range cluster.Spec.ProcessGroupsToRemove {
+		removals[id] = None{}
+	}
+
+	for _, processGroupID := range processGroupIDs {
+		if _, ok := removals[processGroupID]; ok {
+			continue
+		}
+
+		cluster.Spec.ProcessGroupsToRemove = append(cluster.Spec.ProcessGroupsToRemove, processGroupID)
+	}
+}
+
+// AddProcessGroupsToRemovalWithoutExclusionList adds the provided process group IDs to the remove without exclusion list.
+// If a process group ID is already present on that list it won't be added a second time.
+func (cluster *FoundationDBCluster) AddProcessGroupsToRemovalWithoutExclusionList(processGroupIDs []string) {
+	removals := map[string]None{}
+
+	for _, id := range cluster.Spec.ProcessGroupsToRemoveWithoutExclusion {
+		removals[id] = None{}
+	}
+
+	for _, processGroupID := range processGroupIDs {
+		if _, ok := removals[processGroupID]; ok {
+			continue
+		}
+
+		cluster.Spec.ProcessGroupsToRemoveWithoutExclusion = append(cluster.Spec.ProcessGroupsToRemoveWithoutExclusion, processGroupID)
+	}
+}
diff --git a/kubectl-fdb/cmd/remove_process_group.go b/kubectl-fdb/cmd/remove_process_group.go
index b35036e0..79cbd747 100644
--- a/kubectl-fdb/cmd/remove_process_group.go
+++ b/kubectl-fdb/cmd/remove_process_group.go
@@ -21,23 +21,17 @@
 package cmd
 
 import (
+	ctx "context"
 	"fmt"
 	"log"
 
+	fdbv1beta2 "github.com/FoundationDB/fdb-kubernetes-operator/api/v1beta2"
 	"github.com/FoundationDB/fdb-kubernetes-operator/internal"
-
+	"github.com/spf13/cobra"
+	corev1 "k8s.io/api/core/v1"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
-
 	"k8s.io/cli-runtime/pkg/genericclioptions"
-
-	corev1 "k8s.io/api/core/v1"
-
-	"github.com/spf13/cobra"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-
-	ctx "context"
-
-	fdbv1beta2 "github.com/FoundationDB/fdb-kubernetes-operator/api/v1beta2"
 )
 
 func newRemoveProcessGroupCmd(streams genericclioptions.IOStreams) *cobra.Command {
@@ -155,18 +149,6 @@ func getProcessGroupIDsFromPod(kubeClient client.Client, clusterName string, pod
 	return processGroups, nil
 }
 
-func filterAlreadyMarkedProcessGroups(cluster *fdbv1beta2.FoundationDBCluster, processGroupIDs []string) []string {
-	res := make([]string, 0, len(processGroupIDs))
-
-	for _, processGroupID := range processGroupIDs {
-		if !cluster.ProcessGroupIsBeingRemoved(processGroupID) {
-			res = append(res, processGroupID)
-		}
-	}
-
-	return res
-}
-
 // replaceProcessGroups adds process groups to the removal list of the cluster
 func replaceProcessGroups(kubeClient client.Client, clusterName string, processGroups []string, namespace string, withExclusion bool, withShrink bool, force bool, removeAllFailed bool) error {
 	cluster, err := loadCluster(kubeClient, namespace, clusterName)
@@ -202,8 +184,18 @@ func replaceProcessGroups(kubeClient client.Client, clusterName string, processG
 
 	patch := client.MergeFrom(cluster.DeepCopy())
 
+	processGroupSet := map[string]fdbv1beta2.None{}
+	for _, processGroup := range processGroups {
+		processGroupSet[processGroup] = fdbv1beta2.None{}
+	}
+
 	if removeAllFailed {
 		for _, processGroupStatus := range cluster.Status.ProcessGroups {
+			// Those are already included so we can skip the check and don't add duplicates
+			if _, ok := processGroupSet[processGroupStatus.ProcessGroupID]; ok {
+				continue
+			}
+
 			needsReplacement, _ := processGroupStatus.NeedsReplacement(0)
 			if needsReplacement {
 				processGroups = append(processGroups, processGroupStatus.ProcessGroupID)
@@ -222,12 +214,10 @@ func replaceProcessGroups(kubeClient client.Client, clusterName string, processG
 		}
 	}
 
-	processGroups = filterAlreadyMarkedProcessGroups(cluster, processGroups)
-
 	if withExclusion {
-		cluster.Spec.ProcessGroupsToRemove = append(cluster.Spec.ProcessGroupsToRemove, processGroups...)
+		cluster.AddProcessGroupsToRemovalList(processGroups)
 	} else {
-		cluster.Spec.ProcessGroupsToRemoveWithoutExclusion = append(cluster.Spec.ProcessGroupsToRemoveWithoutExclusion, processGroups...)
+		cluster.AddProcessGroupsToRemovalWithoutExclusionList(processGroups)
 	}
 
 	return kubeClient.Patch(ctx.TODO(), cluster, patch)


Test patch:
diff --git a/api/v1beta2/foundationdbcluster_types_test.go b/api/v1beta2/foundationdbcluster_types_test.go
index 15873852..c7af7856 100644
--- a/api/v1beta2/foundationdbcluster_types_test.go
+++ b/api/v1beta2/foundationdbcluster_types_test.go
@@ -3691,4 +3691,82 @@ var _ = Describe("[api] FoundationDBCluster", func() {
 			),
 		)
 	})
+
+	When("adding a process to the removal list", func() {
+		var cluster *FoundationDBCluster
+
+		BeforeEach(func() {
+			cluster = &FoundationDBCluster{}
+		})
+
+		When("removing with exclusion", func() {
+			When("a np process group is already included in the list", func() {
+				It("should add the process group to the removal list", func() {
+					removals := []string{"test1"}
+					cluster.AddProcessGroupsToRemovalList(removals)
+					Expect(cluster.Spec.ProcessGroupsToRemove).To(ContainElements(removals))
+					Expect(len(cluster.Spec.ProcessGroupsToRemove)).To(Equal(len(removals)))
+					Expect(len(cluster.Spec.ProcessGroupsToRemoveWithoutExclusion)).To(Equal(0))
+				})
+			})
+
+			When("a process group is already included in the list", func() {
+				BeforeEach(func() {
+					cluster.Spec.ProcessGroupsToRemove = append(cluster.Spec.ProcessGroupsToRemove, "test1")
+				})
+
+				It("should only add the missing process groups", func() {
+					Expect(cluster.Spec.ProcessGroupsToRemove).To(ContainElements("test1"))
+					removals := []string{"test1", "test2"}
+					cluster.AddProcessGroupsToRemovalList(removals)
+					Expect(cluster.Spec.ProcessGroupsToRemove).To(ContainElements(removals))
+					Expect(len(cluster.Spec.ProcessGroupsToRemove)).To(Equal(len(removals)))
+					Expect(len(cluster.Spec.ProcessGroupsToRemoveWithoutExclusion)).To(Equal(0))
+				})
+			})
+		})
+
+		When("removing without exclusion", func() {
+			When("a process group is not already included in the list", func() {
+				It("should add the process group to the removal list", func() {
+					removals := []string{"test1"}
+					cluster.AddProcessGroupsToRemovalWithoutExclusionList(removals)
+					Expect(cluster.Spec.ProcessGroupsToRemoveWithoutExclusion).To(ContainElements(removals))
+					Expect(len(cluster.Spec.ProcessGroupsToRemoveWithoutExclusion)).To(Equal(len(removals)))
+					Expect(len(cluster.Spec.ProcessGroupsToRemove)).To(Equal(0))
+				})
+			})
+
+			When("a process group is already included in the list", func() {
+				BeforeEach(func() {
+					cluster.Spec.ProcessGroupsToRemoveWithoutExclusion = append(cluster.Spec.ProcessGroupsToRemoveWithoutExclusion, "test1")
+					Expect(cluster.Spec.ProcessGroupsToRemoveWithoutExclusion).To(ContainElements("test1"))
+				})
+
+				It("should only add the missing process groups", func() {
+					removals := []string{"test1", "test2"}
+					cluster.AddProcessGroupsToRemovalWithoutExclusionList(removals)
+					Expect(cluster.Spec.ProcessGroupsToRemoveWithoutExclusion).To(ContainElements(removals))
+					Expect(len(cluster.Spec.ProcessGroupsToRemoveWithoutExclusion)).To(Equal(len(removals)))
+					Expect(len(cluster.Spec.ProcessGroupsToRemove)).To(Equal(0))
+				})
+			})
+
+			When("a process group is already included in the with exclusion list", func() {
+				BeforeEach(func() {
+					cluster.Spec.ProcessGroupsToRemove = append(cluster.Spec.ProcessGroupsToRemove, "test1")
+					Expect(cluster.Spec.ProcessGroupsToRemove).To(ContainElements("test1"))
+					Expect(len(cluster.Spec.ProcessGroupsToRemoveWithoutExclusion)).To(Equal(0))
+				})
+
+				It("should only add the missing process groups", func() {
+					removals := []string{"test1", "test2"}
+					cluster.AddProcessGroupsToRemovalWithoutExclusionList(removals)
+					Expect(cluster.Spec.ProcessGroupsToRemoveWithoutExclusion).To(ContainElements(removals))
+					Expect(len(cluster.Spec.ProcessGroupsToRemoveWithoutExclusion)).To(Equal(len(removals)))
+					Expect(len(cluster.Spec.ProcessGroupsToRemove)).To(Equal(1))
+				})
+			})
+		})
+	})
 })
diff --git a/kubectl-fdb/cmd/remove_process_group_test.go b/kubectl-fdb/cmd/remove_process_group_test.go
index c37db095..df46e946 100644
--- a/kubectl-fdb/cmd/remove_process_group_test.go
+++ b/kubectl-fdb/cmd/remove_process_group_test.go
@@ -247,6 +247,57 @@ var _ = Describe("[plugin] remove instances command", func() {
 						RemoveAllFailed: true,
 					}),
 			)
+
+			When("a procress group was already marked for removal", func() {
+				var kubeClient client.Client
+
+				BeforeEach(func() {
+					cluster.Spec.ProcessGroupsToRemove = []string{"instance-1"}
+					scheme := runtime.NewScheme()
+					_ = clientgoscheme.AddToScheme(scheme)
+					_ = fdbv1beta2.AddToScheme(scheme)
+					kubeClient = fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(cluster, &podList).Build()
+				})
+
+				When("adding the same process group to the removal list without exclusion", func() {
+					It("should add the process group to the removal without exclusion list", func() {
+						removals := []string{"instance-1"}
+						err := replaceProcessGroups(kubeClient, clusterName, removals, namespace, false, false, true, false)
+						Expect(err).NotTo(HaveOccurred())
+
+						var resCluster fdbv1beta2.FoundationDBCluster
+						err = kubeClient.Get(ctx.Background(), client.ObjectKey{
+							Namespace: namespace,
+							Name:      clusterName,
+						}, &resCluster)
+
+						Expect(err).NotTo(HaveOccurred())
+						Expect(resCluster.Spec.ProcessGroupsToRemove).To(ContainElements(removals))
+						Expect(len(resCluster.Spec.ProcessGroupsToRemove)).To(BeNumerically("==", len(removals)))
+						Expect(resCluster.Spec.ProcessGroupsToRemoveWithoutExclusion).To(ContainElements(removals))
+						Expect(len(resCluster.Spec.ProcessGroupsToRemoveWithoutExclusion)).To(BeNumerically("==", len(removals)))
+					})
+				})
+
+				When("adding the same process group to the removal list", func() {
+					It("should add the process group to the removal without exclusion list", func() {
+						removals := []string{"instance-1"}
+						err := replaceProcessGroups(kubeClient, clusterName, removals, namespace, true, false, true, false)
+						Expect(err).NotTo(HaveOccurred())
+
+						var resCluster fdbv1beta2.FoundationDBCluster
+						err = kubeClient.Get(ctx.Background(), client.ObjectKey{
+							Namespace: namespace,
+							Name:      clusterName,
+						}, &resCluster)
+
+						Expect(err).NotTo(HaveOccurred())
+						Expect(resCluster.Spec.ProcessGroupsToRemove).To(ContainElements(removals))
+						Expect(len(resCluster.Spec.ProcessGroupsToRemove)).To(BeNumerically("==", len(removals)))
+						Expect(len(resCluster.Spec.ProcessGroupsToRemoveWithoutExclusion)).To(BeNumerically("==", 0))
+					})
+				})
+			})
 		})
 	})
 })