Repository: FoundationDB/fdb-kubernetes-operator
Pull request #1124 (2022-03-19 10:43:38)

Remove the usage of go get and unused dep
# Description

Remove the `go get` command to install our dependencies and update our documentation

## Type of change

*Please select one of the options below.*

- Documentation

# Discussion

-

# Testing

Locally tested `make deps`

# Documentation

-

# Follow-up

-


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

* CodeBuild project: fdb-kubernetes-operator-pr
* Commit ID: 88058cd2a0b29355856c5acbd99de853665cf1f0
* 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/de5d454e-0b01-4b2f-935b-c68d37fb0798.gz) (available for 30 days)


Patch:
diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md
index f8eaee86..f806d26a 100644
--- a/.github/PULL_REQUEST_TEMPLATE.md
+++ b/.github/PULL_REQUEST_TEMPLATE.md
@@ -12,18 +12,18 @@
 - Documentation
 - Other
 
-# Discussion
+## Discussion
 
 *Are there any design details that you would like to discuss further?*
 
-# Testing
+## 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
+## Documentation
 
 *Did you update relevant documentation within this repository?*
 
@@ -33,7 +33,7 @@ Manual testing?*
 
 *If this change is adding new safety checks or new potential failure modes, have we documented and how to debug potential issues?*
 
-# Follow-up
+## Follow-up
 
 *Are there any follow-up issues that we should pursue in the future?*
 
diff --git a/Makefile b/Makefile
index 46b16960..c020154c 100644
--- a/Makefile
+++ b/Makefile
@@ -31,8 +31,6 @@ CONTROLLER_GEN_PKG?=sigs.k8s.io/controller-tools/cmd/controller-gen@v0.6.1
 CONTROLLER_GEN=$(GOBIN)/controller-gen
 KUSTOMIZE_PKG?=sigs.k8s.io/kustomize/kustomize/v3@v3.9.4
 KUSTOMIZE=$(GOBIN)/kustomize
-YQ_PKG?=github.com/mikefarah/yq/v4@v4.13.5
-YQ=$(GOBIN)/yq
 GOLANGCI_LINT_PKG=github.com/golangci/golangci-lint/cmd/golangci-lint@v1.42.1
 GOLANGCI_LINT=$(GOBIN)/golangci-lint
 BUILD_DEPS?=
@@ -43,7 +41,7 @@ BUILD_DEPS+=$(1)
 $($2):
 	(export TMP_DIR=$$$$(mktemp -d) ;\
 	cd $$$$TMP_DIR ;\
-	go get $$($2_PKG) ;\
+	go install $$($2_PKG) ;\
 	cd - ;\
 	rm -rf $$$$TMP_DIR )
 $(1): $$($2)
@@ -52,7 +50,6 @@ endef
 $(eval $(call godep,controller-gen,CONTROLLER_GEN))
 $(eval $(call godep,golangci-lint,GOLANGCI_LINT))
 $(eval $(call godep,kustomize,KUSTOMIZE))
-$(eval $(call godep,yq,YQ))
 
 GO_SRC=$(shell find . -name "*.go" -not -name "zz_generated.*.go")
 GENERATED_GO=api/v1beta2/zz_generated.deepcopy.go
diff --git a/README.md b/README.md
index 2dd5360f..8a261031 100644
--- a/README.md
+++ b/README.md
@@ -4,8 +4,7 @@
 ![GitHub](https://img.shields.io/github/license/FoundationDB/fdb-kubernetes-operator)
 [![CI for main branch](https://github.com/FoundationDB/fdb-kubernetes-operator/actions/workflows/pull_request.yml/badge.svg)](https://github.com/FoundationDB/fdb-kubernetes-operator/actions/workflows/pull_request.yml)
 
-This project provides an operator for managing FoundationDB
-clusters on Kubernetes.
+This project provides an operator for managing FoundationDB clusters on Kubernetes.
 
 ## Running the Operator
 
@@ -26,10 +25,10 @@ kubectl apply -f https://raw.githubusercontent.com/foundationdb/fdb-kubernetes-o
 ```
 
 You can see logs from the operator by running
-`kubectl logs -f -l app=fdb-kubernetes-operator-controller-manager --container=manager`. To determine whether the reconciliation has completed, you can run `kubectl get foundationdbcluster my-cluster`. This will show the latest generation of the
+`kubectl logs -f -l app=fdb-kubernetes-operator-controller-manager --container=manager`. To determine whether the reconciliation has completed, you can run `kubectl get foundationdbcluster test-cluster`. This will show the latest generation of the
 spec and the last reconciled generation of the spec. Once reconciliation has completed, these values will be the same.
 
-Once the reconciliation is complete, you can run `kubectl exec -it my-cluster-log-1 -- fdbcli` to open up a CLI on your cluster.
+Once the reconciliation is complete, you can run `kubectl exec -it test-cluster-log-1 -- fdbcli` to open up a CLI on your cluster.
 
 You can also browse the [sample directory](config/samples) for more examples of different resource configurations.
 
@@ -37,36 +36,31 @@ For more information about using the operator, including detailed discussion of
 
 For more information on version compatibility, see our [compatibility guide](docs/compatibility.md).
 
-For more information on the fields you can define on the cluster resource, see
-the [API documentation](docs/cluster_spec.md).
+For more information on the fields you can define on the cluster resource, see the [API documentation](docs/cluster_spec.md).
 
 ## Local Development
 
 ### Environment Set-up
 
-1. Install GO on your machine, see the [Getting Started](https://golang.org/doc/install) guide for more information.
-2. Install KubeBuilder and its dependencies on your machine, see [The KubeBuilder Book](https://book.kubebuilder.io/quick-start.html) for more information (currently version `2.3.2` is used).
-3. Set your $GOPATH, e.x. `/Users/me/Code/go`.
-4. Install [kustomize](https://github.com/kubernetes-sigs/kustomize).
-5. Install the [foundationDB client package](https://www.foundationdb.org/download).
+1. Install Go on your machine, see the [Getting Started](https://golang.org/doc/install) guide for more information.
+1. Install the required dependencies with `make deps`.
+1. Install the [foundationDB client package](https://www.foundationdb.org/download).
 
 ### Running Locally
 
 To get this controller running in a local Kubernetes cluster:
 
-1. Change your current directory to $GOPATH/src/github.com using the
-   command `cd $GOPATH/src/github.com` and run `mkdir foundationdb`
-   to create the directory `foundationdb`.
-2. CD into the newly created directory and clone this github repo inside
-   `$GOPATH/src/github.com/foundationdb`.
-3. Run `config/test-certs/generate_secrets.bash` to set up a secret with
+1. The assumption is that you have local Kubernetes cluster running.
+   Depending on what solution you use some of the following steps might differ.
+1. Clone this repository onto your local machine.
+1. Run `config/test-certs/generate_secrets.bash` to set up a secret with
    self-signed test certs.
-4. Run `make rebuild-operator` to install the operator. By default, the
-   docker image is built for the platform where this command is executed.
-   To override the platform, for example, to build amd64 image on Apple M1,
-   you can set the BUILD_PLATFORM env variable
+1. Run `make rebuild-operator` to install the operator. By default, the
+   container image is built for the platform where this command is executed.
+   To override the platform, for example, to build an amd64 image on Apple M1,
+   you can set the `BUILD_PLATFORM` env variable
    `BUILD_PLATFORM="linux/amd64" make rebuild-operator`.
-5. Run `kubectl apply -k ./config/tests/base`
+1. Run `kubectl apply -k ./config/tests/base`
    to create a new FoundationDB cluster with the operator.
 
 ### Running locally with nerdctl
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))
+					})
+				})
+			})
 		})
 	})
 })