From 72ed3b0f23fa2f21795c01bba025ed4499e896f4 Mon Sep 17 00:00:00 2001 From: "Johannes M. Scheuermann" Date: Sat, 16 Jul 2022 11:20:18 +0100 Subject: [PATCH 1/2] Reduce API call to Kubernetes to fetch process group ID information --- internal/pod_models.go | 11 ++ internal/pod_models_test.go | 17 ++ kubectl-fdb/cmd/analyze.go | 2 +- kubectl-fdb/cmd/buggify_crash_loop.go | 14 +- kubectl-fdb/cmd/buggify_crash_loop_test.go | 35 ++--- kubectl-fdb/cmd/buggify_no_schedule.go | 14 +- kubectl-fdb/cmd/buggify_no_schedule_test.go | 34 ++-- kubectl-fdb/cmd/cordon.go | 4 +- kubectl-fdb/cmd/k8s_client.go | 30 ++-- kubectl-fdb/cmd/k8s_client_test.go | 81 ++++++++++ kubectl-fdb/cmd/remove_process_group.go | 49 ++---- kubectl-fdb/cmd/remove_process_group_test.go | 155 +++---------------- 12 files changed, 205 insertions(+), 241 deletions(-) diff --git a/internal/pod_models.go b/internal/pod_models.go index 6e97d65c2..185076aff 100644 --- a/internal/pod_models.go +++ b/internal/pod_models.go @@ -37,6 +37,17 @@ import ( var processClassSanitizationPattern = regexp.MustCompile("[^a-z0-9-]") +// GetProcessGroupIDFromPodName returns the process group ID for a given Pod name. +func GetProcessGroupIDFromPodName(cluster *fdbv1beta2.FoundationDBCluster, podName string) string { + tmpName := strings.ReplaceAll(podName, cluster.Name, "")[1:] + + if cluster.Spec.ProcessGroupIDPrefix != "" { + return fmt.Sprintf("%s-%s", cluster.Spec.ProcessGroupIDPrefix, tmpName) + } + + return tmpName +} + // GetProcessGroupID generates an ID for a process group. // // This will return the pod name and the processGroupID ID. diff --git a/internal/pod_models_test.go b/internal/pod_models_test.go index ce331797b..566a7a070 100644 --- a/internal/pod_models_test.go +++ b/internal/pod_models_test.go @@ -3181,4 +3181,21 @@ var _ = Describe("pod_models", func() { Expect(GetPodDNSName(cluster, "operator-test-storage-1")).To(Equal("operator-test-storage-1.operator-test-1.my-ns.svc.cluster.example")) }) }) + + DescribeTable("getting the process group ID from the Pod name", func(cluster *fdbv1beta2.FoundationDBCluster, podName string, expected string) { + Expect(GetProcessGroupIDFromPodName(cluster, podName)).To(Equal(expected)) + }, + Entry("cluster without prefix", &fdbv1beta2.FoundationDBCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + }, + }, "test-storage-1", "storage-1"), + Entry("cluster with prefix", &fdbv1beta2.FoundationDBCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + }, + Spec: fdbv1beta2.FoundationDBClusterSpec{ + ProcessGroupIDPrefix: "prefix", + }, + }, "test-storage-1", "prefix-storage-1")) }) diff --git a/kubectl-fdb/cmd/analyze.go b/kubectl-fdb/cmd/analyze.go index ca719a34b..bd5fd4ac4 100644 --- a/kubectl-fdb/cmd/analyze.go +++ b/kubectl-fdb/cmd/analyze.go @@ -399,7 +399,7 @@ func analyzeCluster(cmd *cobra.Command, kubeClient client.Client, clusterName st confirmed := false if len(failedProcessGroups) > 0 { - err := replaceProcessGroups(kubeClient, cluster.Name, failedProcessGroups, namespace, true, false, wait, false) + err := replaceProcessGroups(kubeClient, cluster.Name, failedProcessGroups, namespace, true, wait, false, true) if err != nil { return err } diff --git a/kubectl-fdb/cmd/buggify_crash_loop.go b/kubectl-fdb/cmd/buggify_crash_loop.go index 3d7a4008b..3f699fe03 100644 --- a/kubectl-fdb/cmd/buggify_crash_loop.go +++ b/kubectl-fdb/cmd/buggify_crash_loop.go @@ -66,12 +66,7 @@ func newBuggifyCrashLoop(streams genericclioptions.IOStreams) *cobra.Command { return err } - processGroups, err := getProcessGroupIDsFromPod(kubeClient, cluster, args, namespace) - if err != nil { - return err - } - - return updateCrashLoopList(kubeClient, cluster, processGroups, namespace, wait, clear, clean) + return updateCrashLoopList(kubeClient, cluster, args, namespace, wait, clear, clean) }, Example: ` # Add process groups into crash loop state for a cluster in the current namespace @@ -104,7 +99,7 @@ kubectl fdb -n default buggify crash-loop -c cluster pod-1 pod-2 } // updateCrashLoopList updates the crash-loop list of the cluster -func updateCrashLoopList(kubeClient client.Client, clusterName string, processGroups []string, namespace string, wait bool, clear bool, clean bool) error { +func updateCrashLoopList(kubeClient client.Client, clusterName string, pods []string, namespace string, wait bool, clear bool, clean bool) error { cluster, err := loadCluster(kubeClient, namespace, clusterName) if err != nil { if k8serrors.IsNotFound(err) { @@ -113,6 +108,11 @@ func updateCrashLoopList(kubeClient client.Client, clusterName string, processGr return err } + processGroups, err := getProcessGroupIDsFromPodName(cluster, pods) + if err != nil { + return err + } + patch := client.MergeFrom(cluster.DeepCopy()) if clean { if wait { diff --git a/kubectl-fdb/cmd/buggify_crash_loop_test.go b/kubectl-fdb/cmd/buggify_crash_loop_test.go index b8809f86f..bc020eee2 100644 --- a/kubectl-fdb/cmd/buggify_crash_loop_test.go +++ b/kubectl-fdb/cmd/buggify_crash_loop_test.go @@ -56,7 +56,6 @@ var _ = Describe("[plugin] buggify crash-loop instances command", func() { When("running buggify crash-loop instances command", func() { When("adding instances to crash-loop list from a cluster", func() { - type testCase struct { Instances []string ExpectedInstancesInCrashLoop []string @@ -83,13 +82,13 @@ var _ = Describe("[plugin] buggify crash-loop instances command", func() { }, Entry("Adding single instance.", testCase{ - Instances: []string{"instance-1"}, - ExpectedInstancesInCrashLoop: []string{"instance-1"}, + Instances: []string{"test-storage-1"}, + ExpectedInstancesInCrashLoop: []string{"storage-1"}, }), Entry("Adding multiple instances.", testCase{ - Instances: []string{"instance-1", "instance-2"}, - ExpectedInstancesInCrashLoop: []string{"instance-1", "instance-2"}, + Instances: []string{"test-storage-1", "test-storage-2"}, + ExpectedInstancesInCrashLoop: []string{"storage-1", "storage-2"}, }), ) @@ -97,7 +96,7 @@ var _ = Describe("[plugin] buggify crash-loop instances command", func() { var kubeClient client.Client BeforeEach(func() { - cluster.Spec.Buggify.CrashLoop = []string{"instance-1"} + cluster.Spec.Buggify.CrashLoop = []string{"storage-1"} scheme := runtime.NewScheme() _ = clientgoscheme.AddToScheme(scheme) _ = fdbv1beta2.AddToScheme(scheme) @@ -125,18 +124,18 @@ var _ = Describe("[plugin] buggify crash-loop instances command", func() { }, Entry("Adding the same instance.", testCase{ - Instances: []string{"instance-1"}, - ExpectedInstancesInCrashLoop: []string{"instance-1"}, + Instances: []string{"test-storage-1"}, + ExpectedInstancesInCrashLoop: []string{"storage-1"}, }), Entry("Adding different instance.", testCase{ - Instances: []string{"instance-2"}, - ExpectedInstancesInCrashLoop: []string{"instance-1", "instance-2"}, + Instances: []string{"test-storage-2"}, + ExpectedInstancesInCrashLoop: []string{"storage-1", "storage-2"}, }), Entry("Adding multiple instances.", testCase{ - Instances: []string{"instance-2", "instance-3"}, - ExpectedInstancesInCrashLoop: []string{"instance-1", "instance-2", "instance-3"}, + Instances: []string{"test-storage-2", "test-storage-3"}, + ExpectedInstancesInCrashLoop: []string{"storage-1", "storage-2", "storage-3"}, }), ) }) @@ -146,7 +145,7 @@ var _ = Describe("[plugin] buggify crash-loop instances command", func() { var kubeClient client.Client BeforeEach(func() { - cluster.Spec.Buggify.CrashLoop = []string{"instance-1", "instance-2", "instance-3"} + cluster.Spec.Buggify.CrashLoop = []string{"storage-1", "storage-2", "storage-3"} scheme := runtime.NewScheme() _ = clientgoscheme.AddToScheme(scheme) _ = fdbv1beta2.AddToScheme(scheme) @@ -174,13 +173,13 @@ var _ = Describe("[plugin] buggify crash-loop instances command", func() { }, Entry("Removing single instance.", testCase{ - Instances: []string{"instance-1"}, - ExpectedInstancesInCrashLoop: []string{"instance-2", "instance-3"}, + Instances: []string{"test-storage-1"}, + ExpectedInstancesInCrashLoop: []string{"storage-2", "storage-3"}, }), Entry("Removing multiple instances.", testCase{ - Instances: []string{"instance-2", "instance-3"}, - ExpectedInstancesInCrashLoop: []string{"instance-1"}, + Instances: []string{"test-storage-2", "test-storage-3"}, + ExpectedInstancesInCrashLoop: []string{"storage-1"}, }), ) @@ -190,7 +189,7 @@ var _ = Describe("[plugin] buggify crash-loop instances command", func() { var kubeClient client.Client BeforeEach(func() { - cluster.Spec.Buggify.CrashLoop = []string{"instance-1", "instance-2", "instance-3"} + cluster.Spec.Buggify.CrashLoop = []string{"storage-1", "storage-2", "storage-3"} scheme := runtime.NewScheme() _ = clientgoscheme.AddToScheme(scheme) _ = fdbv1beta2.AddToScheme(scheme) diff --git a/kubectl-fdb/cmd/buggify_no_schedule.go b/kubectl-fdb/cmd/buggify_no_schedule.go index 4d47af352..be07d1bb4 100644 --- a/kubectl-fdb/cmd/buggify_no_schedule.go +++ b/kubectl-fdb/cmd/buggify_no_schedule.go @@ -66,12 +66,7 @@ func newBuggifyNoSchedule(streams genericclioptions.IOStreams) *cobra.Command { return err } - processGroups, err := getProcessGroupIDsFromPod(kubeClient, cluster, args, namespace) - if err != nil { - return err - } - - return updateNoScheduleList(kubeClient, cluster, processGroups, namespace, wait, clear, clean) + return updateNoScheduleList(kubeClient, cluster, args, namespace, wait, clear, clean) }, Example: ` # Add process groups into no-schedule state for a cluster in the current namespace @@ -105,7 +100,7 @@ kubectl fdb -n default buggify no-schedule -c cluster pod-1 pod-2 } // updateNoScheduleList updates the removal list of the cluster -func updateNoScheduleList(kubeClient client.Client, clusterName string, processGroups []string, namespace string, wait bool, clear bool, clean bool) error { +func updateNoScheduleList(kubeClient client.Client, clusterName string, pods []string, namespace string, wait bool, clear bool, clean bool) error { cluster, err := loadCluster(kubeClient, namespace, clusterName) if err != nil { if k8serrors.IsNotFound(err) { @@ -114,6 +109,11 @@ func updateNoScheduleList(kubeClient client.Client, clusterName string, processG return err } + processGroups, err := getProcessGroupIDsFromPodName(cluster, pods) + if err != nil { + return err + } + patch := client.MergeFrom(cluster.DeepCopy()) if clean { if wait { diff --git a/kubectl-fdb/cmd/buggify_no_schedule_test.go b/kubectl-fdb/cmd/buggify_no_schedule_test.go index 448612f55..e1788323d 100644 --- a/kubectl-fdb/cmd/buggify_no_schedule_test.go +++ b/kubectl-fdb/cmd/buggify_no_schedule_test.go @@ -82,13 +82,13 @@ var _ = Describe("[plugin] buggify no-schedule instances command", func() { }, Entry("Adding single instance.", testCase{ - Instances: []string{"instance-1"}, - ExpectedInstancesInNoSchedule: []string{"instance-1"}, + Instances: []string{"test-storage-1"}, + ExpectedInstancesInNoSchedule: []string{"storage-1"}, }), Entry("Adding multiple instances.", testCase{ - Instances: []string{"instance-1", "instance-2"}, - ExpectedInstancesInNoSchedule: []string{"instance-1", "instance-2"}, + Instances: []string{"test-storage-1", "test-storage-2"}, + ExpectedInstancesInNoSchedule: []string{"storage-1", "storage-2"}, }), ) @@ -96,7 +96,7 @@ var _ = Describe("[plugin] buggify no-schedule instances command", func() { var kubeClient client.Client BeforeEach(func() { - cluster.Spec.Buggify.NoSchedule = []string{"instance-1"} + cluster.Spec.Buggify.NoSchedule = []string{"storage-1"} scheme := runtime.NewScheme() _ = clientgoscheme.AddToScheme(scheme) _ = fdbv1beta2.AddToScheme(scheme) @@ -124,18 +124,18 @@ var _ = Describe("[plugin] buggify no-schedule instances command", func() { }, Entry("Adding the same instance.", testCase{ - Instances: []string{"instance-1"}, - ExpectedInstancesInNoSchedule: []string{"instance-1"}, + Instances: []string{"test-storage-1"}, + ExpectedInstancesInNoSchedule: []string{"storage-1"}, }), Entry("Adding different instance.", testCase{ - Instances: []string{"instance-2"}, - ExpectedInstancesInNoSchedule: []string{"instance-1", "instance-2"}, + Instances: []string{"test-storage-2"}, + ExpectedInstancesInNoSchedule: []string{"storage-1", "storage-2"}, }), Entry("Adding multiple instances.", testCase{ - Instances: []string{"instance-2", "instance-3"}, - ExpectedInstancesInNoSchedule: []string{"instance-1", "instance-2", "instance-3"}, + Instances: []string{"test-storage-2", "test-storage-3"}, + ExpectedInstancesInNoSchedule: []string{"storage-1", "storage-2", "storage-3"}, }), ) }) @@ -145,7 +145,7 @@ var _ = Describe("[plugin] buggify no-schedule instances command", func() { var kubeClient client.Client BeforeEach(func() { - cluster.Spec.Buggify.NoSchedule = []string{"instance-1", "instance-2", "instance-3"} + cluster.Spec.Buggify.NoSchedule = []string{"storage-1", "storage-2", "storage-3"} scheme := runtime.NewScheme() _ = clientgoscheme.AddToScheme(scheme) _ = fdbv1beta2.AddToScheme(scheme) @@ -173,13 +173,13 @@ var _ = Describe("[plugin] buggify no-schedule instances command", func() { }, Entry("Removing single instance.", testCase{ - Instances: []string{"instance-1"}, - ExpectedInstancesInNoSchedule: []string{"instance-2", "instance-3"}, + Instances: []string{"test-storage-1"}, + ExpectedInstancesInNoSchedule: []string{"storage-2", "storage-3"}, }), Entry("Removing multiple instances.", testCase{ - Instances: []string{"instance-2", "instance-3"}, - ExpectedInstancesInNoSchedule: []string{"instance-1"}, + Instances: []string{"test-storage-2", "test-storage-3"}, + ExpectedInstancesInNoSchedule: []string{"storage-1"}, }), ) @@ -189,7 +189,7 @@ var _ = Describe("[plugin] buggify no-schedule instances command", func() { var kubeClient client.Client BeforeEach(func() { - cluster.Spec.Buggify.NoSchedule = []string{"instance-1", "instance-2", "instance-3"} + cluster.Spec.Buggify.NoSchedule = []string{"storage-1", "storage-2", "storage-3"} scheme := runtime.NewScheme() _ = clientgoscheme.AddToScheme(scheme) _ = fdbv1beta2.AddToScheme(scheme) diff --git a/kubectl-fdb/cmd/cordon.go b/kubectl-fdb/cmd/cordon.go index 2dd4ea531..4b7f761ee 100644 --- a/kubectl-fdb/cmd/cordon.go +++ b/kubectl-fdb/cmd/cordon.go @@ -145,7 +145,7 @@ func cordonNode(kubeClient client.Client, cluster *fdbv1beta2.FoundationDBCluste } for _, pod := range pods.Items { - // With the field selector above this shouldn't be required but it's good to + // With the field selector above this shouldn't be required, but it's good to // have a second check. if pod.Spec.NodeName != node { fmt.Printf("Pod: %s is not running on node %s will be ignored\n", pod.Name, node) @@ -161,5 +161,5 @@ func cordonNode(kubeClient client.Client, cluster *fdbv1beta2.FoundationDBCluste } } - return replaceProcessGroups(kubeClient, cluster.Name, processGroups, namespace, withExclusion, false, wait, false) + return replaceProcessGroups(kubeClient, cluster.Name, processGroups, namespace, withExclusion, wait, false, true) } diff --git a/kubectl-fdb/cmd/k8s_client.go b/kubectl-fdb/cmd/k8s_client.go index 71714a493..d2300588f 100644 --- a/kubectl-fdb/cmd/k8s_client.go +++ b/kubectl-fdb/cmd/k8s_client.go @@ -23,6 +23,8 @@ package cmd import ( "bytes" ctx "context" + "fmt" + "strings" fdbv1beta1 "github.com/FoundationDB/fdb-kubernetes-operator/api/v1beta1" fdbv1beta2 "github.com/FoundationDB/fdb-kubernetes-operator/api/v1beta2" @@ -199,29 +201,21 @@ func getAllPodsFromClusterWithCondition(kubeClient client.Client, clusterName st return processes, nil } -func getProcessGroupIDsFromPod(kubeClient client.Client, clusterName string, podNames []string, namespace string) ([]string, error) { +// getProcessGroupIDsFromPodName returns the process group IDs based on the cluster configuration. +func getProcessGroupIDsFromPodName(cluster *fdbv1beta2.FoundationDBCluster, podNames []string) ([]string, error) { processGroups := make([]string, 0, len(podNames)) - // Build a map to filter faster - podNameMap := map[string]struct{}{} - for _, name := range podNames { - podNameMap[name] = struct{}{} - } - cluster, err := loadCluster(kubeClient, namespace, clusterName) - if err != nil { - return processGroups, err - } - pods, err := getPodsForCluster(kubeClient, cluster, namespace) - if err != nil { - return processGroups, err - } - - for _, pod := range pods.Items { - if _, ok := podNameMap[pod.Name]; !ok { + // TODO(johscheuer): We could validate if the provided process group is actually part of the cluster + for _, podName := range podNames { + if podName == "" { continue } - processGroups = append(processGroups, pod.Labels[cluster.GetProcessGroupIDLabel()]) + if !strings.HasPrefix(podName, cluster.Name) { + return nil, fmt.Errorf("cluster name %s is not set as prefix for Pod name %s, please ensure the specified Pod is part of the cluster", cluster.Name, podName) + } + + processGroups = append(processGroups, internal.GetProcessGroupIDFromPodName(cluster, podName)) } return processGroups, nil diff --git a/kubectl-fdb/cmd/k8s_client_test.go b/kubectl-fdb/cmd/k8s_client_test.go index 0033870be..8114ba360 100644 --- a/kubectl-fdb/cmd/k8s_client_test.go +++ b/kubectl-fdb/cmd/k8s_client_test.go @@ -182,4 +182,85 @@ var _ = Describe("[plugin] using the Kubernetes client", func() { }), ) }) + + When("getting the process groups IDs from Pods", func() { + clusterName := "test" + namespace := "test" + var cluster fdbv1beta2.FoundationDBCluster + + When("the cluster doesn't have a prefix", func() { + BeforeEach(func() { + cluster = fdbv1beta2.FoundationDBCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: clusterName, + Namespace: namespace, + }, + Spec: fdbv1beta2.FoundationDBClusterSpec{ + ProcessCounts: fdbv1beta2.ProcessCounts{ + Storage: 1, + }, + }, + } + }) + + DescribeTable("should get all process groups IDs", + func(podNames []string, expected []string) { + instances, err := getProcessGroupIDsFromPodName(&cluster, podNames) + Expect(err).NotTo(HaveOccurred()) + Expect(instances).To(ContainElements(expected)) + Expect(len(instances)).To(BeNumerically("==", len(expected))) + }, + Entry("Filter one instance", + []string{"test-storage-1"}, + []string{"storage-1"}, + ), + Entry("Filter two instances", + []string{"test-storage-1", "test-storage-2"}, + []string{"storage-1", "storage-2"}, + ), + Entry("Filter no instance", + []string{""}, + []string{}, + ), + ) + }) + + When("the cluster has a prefix", func() { + BeforeEach(func() { + cluster = fdbv1beta2.FoundationDBCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: clusterName, + Namespace: namespace, + }, + Spec: fdbv1beta2.FoundationDBClusterSpec{ + ProcessGroupIDPrefix: "banana", + ProcessCounts: fdbv1beta2.ProcessCounts{ + Storage: 1, + }, + }, + } + }) + + DescribeTable("should get all process groups IDs", + func(podNames []string, expected []string) { + instances, err := getProcessGroupIDsFromPodName(&cluster, podNames) + Expect(err).NotTo(HaveOccurred()) + Expect(instances).To(ContainElements(expected)) + Expect(len(instances)).To(BeNumerically("==", len(expected))) + }, + Entry("Filter one instance", + []string{"test-storage-1"}, + []string{"banana-storage-1"}, + ), + Entry("Filter two instances", + []string{"test-storage-1", "test-storage-2"}, + []string{"banana-storage-1", "banana-storage-2"}, + ), + Entry("Filter no instance", + []string{""}, + []string{}, + ), + ) + }) + }) }) diff --git a/kubectl-fdb/cmd/remove_process_group.go b/kubectl-fdb/cmd/remove_process_group.go index 797f72093..63fa1b880 100644 --- a/kubectl-fdb/cmd/remove_process_group.go +++ b/kubectl-fdb/cmd/remove_process_group.go @@ -26,9 +26,7 @@ import ( "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" "sigs.k8s.io/controller-runtime/pkg/client" @@ -54,10 +52,6 @@ func newRemoveProcessGroupCmd(streams genericclioptions.IOStreams) *cobra.Comman if err != nil { return err } - withShrink, err := cmd.Flags().GetBool("shrink") - if err != nil { - return err - } useProcessGroupID, err := cmd.Flags().GetBool("use-process-group-id") if err != nil { return err @@ -77,15 +71,7 @@ func newRemoveProcessGroupCmd(streams genericclioptions.IOStreams) *cobra.Comman return err } - processGroups := args - if !useProcessGroupID { - processGroups, err = getProcessGroupIDsFromPod(kubeClient, cluster, processGroups, namespace) - if err != nil { - return err - } - } - - return replaceProcessGroups(kubeClient, cluster, processGroups, namespace, withExclusion, withShrink, wait, removeAllFailed) + return replaceProcessGroups(kubeClient, cluster, args, namespace, withExclusion, wait, removeAllFailed, useProcessGroupID) }, Example: ` # Remove process groups for a cluster in the current namespace @@ -105,7 +91,6 @@ kubectl fdb -n default remove process-group -c cluster --remove-all-failed cmd.Flags().StringP("fdb-cluster", "c", "", "remove process groupss from the provided cluster.") cmd.Flags().BoolP("exclusion", "e", true, "define if the process groups should be removed with exclusion.") - cmd.Flags().Bool("shrink", false, "define if the removed process groups should not be replaced.") cmd.Flags().Bool("remove-all-failed", false, "define if all failed processes should be replaced.") cmd.Flags().Bool("use-process-group-id", false, "define if the process-group should be used instead of the Pod name.") err := cmd.MarkFlagRequired("fdb-cluster") @@ -122,7 +107,11 @@ kubectl fdb -n default remove process-group -c cluster --remove-all-failed } // 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, wait bool, removeAllFailed bool) error { +func replaceProcessGroups(kubeClient client.Client, clusterName string, processGroups []string, namespace string, withExclusion bool, wait bool, removeAllFailed bool, useProcessGroupID bool) error { + if len(processGroups) == 0 && !removeAllFailed { + return nil + } + cluster, err := loadCluster(kubeClient, namespace, clusterName) if err != nil { @@ -132,26 +121,12 @@ func replaceProcessGroups(kubeClient client.Client, clusterName string, processG return err } - if len(processGroups) == 0 && !removeAllFailed { - return nil - } - - shrinkMap := make(map[fdbv1beta2.ProcessClass]int) - - if withShrink { - var pods corev1.PodList - err := kubeClient.List(ctx.Background(), &pods, - client.InNamespace(namespace), - client.MatchingLabels(cluster.GetMatchLabels()), - ) + // In this case the user has Pod name specified + if !useProcessGroupID { + processGroups, err = getProcessGroupIDsFromPodName(cluster, processGroups) if err != nil { return err } - - for _, pod := range pods.Items { - class := internal.GetProcessClassFromMeta(cluster, pod.ObjectMeta) - shrinkMap[class]++ - } } patch := client.MergeFrom(cluster.DeepCopy()) @@ -175,12 +150,8 @@ func replaceProcessGroups(kubeClient client.Client, clusterName string, processG } } - for class, amount := range shrinkMap { - cluster.Spec.ProcessCounts.DecreaseCount(class, amount) - } - if wait { - confirmed := confirmAction(fmt.Sprintf("Remove %v from cluster %s/%s with exclude: %t and shrink: %t", processGroups, namespace, clusterName, withExclusion, withShrink)) + confirmed := confirmAction(fmt.Sprintf("Remove %v from cluster %s/%s with exclude: %t", processGroups, namespace, clusterName, withExclusion)) if !confirmed { return fmt.Errorf("user aborted the removal") } diff --git a/kubectl-fdb/cmd/remove_process_group_test.go b/kubectl-fdb/cmd/remove_process_group_test.go index b38054cf9..0444d6a01 100644 --- a/kubectl-fdb/cmd/remove_process_group_test.go +++ b/kubectl-fdb/cmd/remove_process_group_test.go @@ -24,7 +24,6 @@ import ( ctx "context" fdbv1beta2 "github.com/FoundationDB/fdb-kubernetes-operator/api/v1beta2" - corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" clientgoscheme "k8s.io/client-go/kubernetes/scheme" @@ -35,7 +34,7 @@ import ( . "github.com/onsi/gomega" ) -var _ = Describe("[plugin] remove instances command", func() { +var _ = Describe("[plugin] remove process groups command", func() { clusterName := "test" namespace := "test" @@ -55,99 +54,20 @@ var _ = Describe("[plugin] remove instances command", func() { } }) - When("running remove instances command", func() { - When("getting the instance IDs from Pods", func() { - var podList corev1.PodList - - BeforeEach(func() { - podList = corev1.PodList{ - Items: []corev1.Pod{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "instance-1", - Namespace: namespace, - Labels: map[string]string{ - fdbv1beta2.FDBProcessClassLabel: string(fdbv1beta2.ProcessClassStorage), - fdbv1beta2.FDBClusterLabel: clusterName, - fdbv1beta2.FDBProcessGroupIDLabel: "storage-1", - }, - }, - }, - { - ObjectMeta: metav1.ObjectMeta{ - Name: "instance-2", - Namespace: namespace, - Labels: map[string]string{ - fdbv1beta2.FDBProcessClassLabel: string(fdbv1beta2.ProcessClassStorage), - fdbv1beta2.FDBClusterLabel: clusterName, - fdbv1beta2.FDBProcessGroupIDLabel: "storage-2", - }, - }, - }, - }, - } - }) - - type testCase struct { - Instances []string - ExpectedInstances []string - } - - DescribeTable("should get all instance IDs", - func(input testCase) { - scheme := runtime.NewScheme() - _ = clientgoscheme.AddToScheme(scheme) - _ = fdbv1beta2.AddToScheme(scheme) - kubeClient := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(cluster, &podList).Build() - - instances, err := getProcessGroupIDsFromPod(kubeClient, clusterName, input.Instances, namespace) - Expect(err).NotTo(HaveOccurred()) - Expect(input.ExpectedInstances).To(Equal(instances)) - }, - Entry("Filter one instance", - testCase{ - Instances: []string{"instance-1"}, - ExpectedInstances: []string{"storage-1"}, - }), - Entry("Filter two instances", - testCase{ - Instances: []string{"instance-1", "instance-2"}, - ExpectedInstances: []string{"storage-1", "storage-2"}, - }), - Entry("Filter no instance", - testCase{ - Instances: []string{""}, - ExpectedInstances: []string{}, - }), - ) - }) - - When("removing instances from a cluster", func() { - var podList corev1.PodList - + When("running remove process groups command", func() { + When("removing process groups from a cluster", func() { BeforeEach(func() { cluster.Status = fdbv1beta2.FoundationDBClusterStatus{ ProcessGroups: []*fdbv1beta2.ProcessGroupStatus{ { - ProcessGroupID: "failed", + ProcessGroupID: "storage-42", Addresses: []string{"1.2.3.4"}, ProcessGroupConditions: []*fdbv1beta2.ProcessGroupCondition{ fdbv1beta2.NewProcessGroupCondition(fdbv1beta2.MissingProcesses), }, }, - }, - } - podList = corev1.PodList{ - Items: []corev1.Pod{ { - ObjectMeta: metav1.ObjectMeta{ - Name: "instance-1", - Namespace: namespace, - Labels: map[string]string{ - fdbv1beta2.FDBProcessClassLabel: string(fdbv1beta2.ProcessClassStorage), - fdbv1beta2.FDBClusterLabel: clusterName, - }, - }, + ProcessGroupID: "storage-1", }, }, } @@ -156,7 +76,6 @@ var _ = Describe("[plugin] remove instances command", func() { type testCase struct { Instances []string WithExclusion bool - WithShrink bool ExpectedInstancesToRemove []string ExpectedInstancesToRemoveWithoutExclusion []string ExpectedProcessCounts fdbv1beta2.ProcessCounts @@ -168,9 +87,9 @@ var _ = Describe("[plugin] remove instances command", func() { scheme := runtime.NewScheme() _ = clientgoscheme.AddToScheme(scheme) _ = fdbv1beta2.AddToScheme(scheme) - kubeClient := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(cluster, &podList).Build() + kubeClient := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(cluster).Build() - err := replaceProcessGroups(kubeClient, clusterName, tc.Instances, namespace, tc.WithExclusion, tc.WithShrink, false, tc.RemoveAllFailed) + err := replaceProcessGroups(kubeClient, clusterName, tc.Instances, namespace, tc.WithExclusion, false, tc.RemoveAllFailed, false) Expect(err).NotTo(HaveOccurred()) var resCluster fdbv1beta2.FoundationDBCluster @@ -185,60 +104,32 @@ var _ = Describe("[plugin] remove instances command", func() { Expect(len(tc.ExpectedInstancesToRemoveWithoutExclusion)).To(BeNumerically("==", len(resCluster.Spec.ProcessGroupsToRemoveWithoutExclusion))) Expect(tc.ExpectedProcessCounts.Storage).To(Equal(resCluster.Spec.ProcessCounts.Storage)) }, - Entry("Remove instance with exclusion", + Entry("Remove process group with exclusion", testCase{ - Instances: []string{"instance-1"}, + Instances: []string{"test-storage-1"}, WithExclusion: true, - WithShrink: false, - ExpectedInstancesToRemove: []string{"instance-1"}, + ExpectedInstancesToRemove: []string{"storage-1"}, ExpectedInstancesToRemoveWithoutExclusion: []string{}, ExpectedProcessCounts: fdbv1beta2.ProcessCounts{ Storage: 1, }, RemoveAllFailed: false, }), - Entry("Remove instance without exclusion", + Entry("Remove process group without exclusion", testCase{ - Instances: []string{"instance-1"}, + Instances: []string{"test-storage-1"}, WithExclusion: false, - WithShrink: false, ExpectedInstancesToRemove: []string{}, - ExpectedInstancesToRemoveWithoutExclusion: []string{"instance-1"}, + ExpectedInstancesToRemoveWithoutExclusion: []string{"storage-1"}, ExpectedProcessCounts: fdbv1beta2.ProcessCounts{ Storage: 1, }, }), - Entry("Remove instance with exclusion and shrink", - testCase{ - Instances: []string{"instance-1"}, - WithExclusion: true, - WithShrink: true, - ExpectedInstancesToRemove: []string{"instance-1"}, - ExpectedInstancesToRemoveWithoutExclusion: []string{}, - ExpectedProcessCounts: fdbv1beta2.ProcessCounts{ - Storage: 0, - }, - RemoveAllFailed: false, - }), - - Entry("Remove instance without exclusion and shrink", - testCase{ - Instances: []string{"instance-1"}, - WithExclusion: false, - WithShrink: true, - ExpectedInstancesToRemove: []string{}, - ExpectedInstancesToRemoveWithoutExclusion: []string{"instance-1"}, - ExpectedProcessCounts: fdbv1beta2.ProcessCounts{ - Storage: 0, - }, - RemoveAllFailed: false, - }), Entry("Remove all failed instances", testCase{ - Instances: []string{"failed"}, + Instances: []string{"test-storage-42"}, WithExclusion: true, - WithShrink: false, - ExpectedInstancesToRemove: []string{"failed"}, + ExpectedInstancesToRemove: []string{"storage-42"}, ExpectedInstancesToRemoveWithoutExclusion: []string{}, ExpectedProcessCounts: fdbv1beta2.ProcessCounts{ Storage: 1, @@ -247,20 +138,20 @@ var _ = Describe("[plugin] remove instances command", func() { }), ) - When("a procress group was already marked for removal", func() { + When("a process group was already marked for removal", func() { var kubeClient client.Client BeforeEach(func() { - cluster.Spec.ProcessGroupsToRemove = []string{"instance-1"} + cluster.Spec.ProcessGroupsToRemove = []string{"storage-1"} scheme := runtime.NewScheme() _ = clientgoscheme.AddToScheme(scheme) _ = fdbv1beta2.AddToScheme(scheme) - kubeClient = fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(cluster, &podList).Build() + kubeClient = fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(cluster).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"} + removals := []string{"test-storage-1"} err := replaceProcessGroups(kubeClient, clusterName, removals, namespace, false, false, false, false) Expect(err).NotTo(HaveOccurred()) @@ -271,16 +162,16 @@ var _ = Describe("[plugin] remove instances command", func() { }, &resCluster) Expect(err).NotTo(HaveOccurred()) - Expect(resCluster.Spec.ProcessGroupsToRemove).To(ContainElements(removals)) + Expect(resCluster.Spec.ProcessGroupsToRemove).To(ContainElements("storage-1")) Expect(len(resCluster.Spec.ProcessGroupsToRemove)).To(BeNumerically("==", len(removals))) - Expect(resCluster.Spec.ProcessGroupsToRemoveWithoutExclusion).To(ContainElements(removals)) + Expect(resCluster.Spec.ProcessGroupsToRemoveWithoutExclusion).To(ContainElements("storage-1")) 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"} + removals := []string{"test-storage-1"} err := replaceProcessGroups(kubeClient, clusterName, removals, namespace, true, false, false, false) Expect(err).NotTo(HaveOccurred()) @@ -291,7 +182,7 @@ var _ = Describe("[plugin] remove instances command", func() { }, &resCluster) Expect(err).NotTo(HaveOccurred()) - Expect(resCluster.Spec.ProcessGroupsToRemove).To(ContainElements(removals)) + Expect(resCluster.Spec.ProcessGroupsToRemove).To(ContainElements("storage-1")) Expect(len(resCluster.Spec.ProcessGroupsToRemove)).To(BeNumerically("==", len(removals))) Expect(len(resCluster.Spec.ProcessGroupsToRemoveWithoutExclusion)).To(BeNumerically("==", 0)) }) From 06239afe07f349be1850896e141016b455cfc9f4 Mon Sep 17 00:00:00 2001 From: "Johannes M. Scheuermann" Date: Fri, 22 Jul 2022 08:53:19 +0100 Subject: [PATCH 2/2] Update variable names to match usage --- kubectl-fdb/cmd/buggify_crash_loop.go | 12 ++++++------ kubectl-fdb/cmd/buggify_no_schedule.go | 12 ++++++------ kubectl-fdb/cmd/k8s_client.go | 6 +++--- kubectl-fdb/cmd/remove_process_group.go | 17 ++++++++--------- 4 files changed, 23 insertions(+), 24 deletions(-) diff --git a/kubectl-fdb/cmd/buggify_crash_loop.go b/kubectl-fdb/cmd/buggify_crash_loop.go index 3f699fe03..87df8251e 100644 --- a/kubectl-fdb/cmd/buggify_crash_loop.go +++ b/kubectl-fdb/cmd/buggify_crash_loop.go @@ -108,7 +108,7 @@ func updateCrashLoopList(kubeClient client.Client, clusterName string, pods []st return err } - processGroups, err := getProcessGroupIDsFromPodName(cluster, pods) + processGroupIDs, err := getProcessGroupIDsFromPodName(cluster, pods) if err != nil { return err } @@ -124,26 +124,26 @@ func updateCrashLoopList(kubeClient client.Client, clusterName string, pods []st return kubeClient.Patch(ctx.TODO(), cluster, patch) } - if len(processGroups) == 0 { + if len(processGroupIDs) == 0 { return fmt.Errorf("please provide atleast one pod") } if wait { if clear { - if !confirmAction(fmt.Sprintf("Removing %v to crash-loop from cluster %s/%s", processGroups, namespace, clusterName)) { + if !confirmAction(fmt.Sprintf("Removing %v to crash-loop from cluster %s/%s", processGroupIDs, namespace, clusterName)) { return fmt.Errorf("user aborted the removal") } } else { - if !confirmAction(fmt.Sprintf("Adding %v to crash-loop from cluster %s/%s", processGroups, namespace, clusterName)) { + if !confirmAction(fmt.Sprintf("Adding %v to crash-loop from cluster %s/%s", processGroupIDs, namespace, clusterName)) { return fmt.Errorf("user aborted the removal") } } } if clear { - cluster.RemoveProcessGroupsFromCrashLoopList(processGroups) + cluster.RemoveProcessGroupsFromCrashLoopList(processGroupIDs) } else { - cluster.AddProcessGroupsToCrashLoopList(processGroups) + cluster.AddProcessGroupsToCrashLoopList(processGroupIDs) } return kubeClient.Patch(ctx.TODO(), cluster, patch) diff --git a/kubectl-fdb/cmd/buggify_no_schedule.go b/kubectl-fdb/cmd/buggify_no_schedule.go index be07d1bb4..486565215 100644 --- a/kubectl-fdb/cmd/buggify_no_schedule.go +++ b/kubectl-fdb/cmd/buggify_no_schedule.go @@ -109,7 +109,7 @@ func updateNoScheduleList(kubeClient client.Client, clusterName string, pods []s return err } - processGroups, err := getProcessGroupIDsFromPodName(cluster, pods) + processGroupIDs, err := getProcessGroupIDsFromPodName(cluster, pods) if err != nil { return err } @@ -125,26 +125,26 @@ func updateNoScheduleList(kubeClient client.Client, clusterName string, pods []s return kubeClient.Patch(ctx.TODO(), cluster, patch) } - if len(processGroups) == 0 { + if len(processGroupIDs) == 0 { return fmt.Errorf("please provide atleast one pod") } if wait { if clear { - if !confirmAction(fmt.Sprintf("Removing %v from no-schedule from cluster %s/%s", processGroups, namespace, clusterName)) { + if !confirmAction(fmt.Sprintf("Removing %v from no-schedule from cluster %s/%s", processGroupIDs, namespace, clusterName)) { return fmt.Errorf("user aborted the removal") } } else { - if !confirmAction(fmt.Sprintf("Adding %v from no-schedule from cluster %s/%s", processGroups, namespace, clusterName)) { + if !confirmAction(fmt.Sprintf("Adding %v from no-schedule from cluster %s/%s", processGroupIDs, namespace, clusterName)) { return fmt.Errorf("user aborted the removal") } } } if clear { - cluster.RemoveProcessGroupsFromNoScheduleList(processGroups) + cluster.RemoveProcessGroupsFromNoScheduleList(processGroupIDs) } else { - cluster.AddProcessGroupsToNoScheduleList(processGroups) + cluster.AddProcessGroupsToNoScheduleList(processGroupIDs) } return kubeClient.Patch(ctx.TODO(), cluster, patch) diff --git a/kubectl-fdb/cmd/k8s_client.go b/kubectl-fdb/cmd/k8s_client.go index d2300588f..35b874b95 100644 --- a/kubectl-fdb/cmd/k8s_client.go +++ b/kubectl-fdb/cmd/k8s_client.go @@ -203,7 +203,7 @@ func getAllPodsFromClusterWithCondition(kubeClient client.Client, clusterName st // getProcessGroupIDsFromPodName returns the process group IDs based on the cluster configuration. func getProcessGroupIDsFromPodName(cluster *fdbv1beta2.FoundationDBCluster, podNames []string) ([]string, error) { - processGroups := make([]string, 0, len(podNames)) + processGroupIDs := make([]string, 0, len(podNames)) // TODO(johscheuer): We could validate if the provided process group is actually part of the cluster for _, podName := range podNames { @@ -215,8 +215,8 @@ func getProcessGroupIDsFromPodName(cluster *fdbv1beta2.FoundationDBCluster, podN return nil, fmt.Errorf("cluster name %s is not set as prefix for Pod name %s, please ensure the specified Pod is part of the cluster", cluster.Name, podName) } - processGroups = append(processGroups, internal.GetProcessGroupIDFromPodName(cluster, podName)) + processGroupIDs = append(processGroupIDs, internal.GetProcessGroupIDFromPodName(cluster, podName)) } - return processGroups, nil + return processGroupIDs, nil } diff --git a/kubectl-fdb/cmd/remove_process_group.go b/kubectl-fdb/cmd/remove_process_group.go index 63fa1b880..41ec570c3 100644 --- a/kubectl-fdb/cmd/remove_process_group.go +++ b/kubectl-fdb/cmd/remove_process_group.go @@ -107,8 +107,8 @@ kubectl fdb -n default remove process-group -c cluster --remove-all-failed } // replaceProcessGroups adds process groups to the removal list of the cluster -func replaceProcessGroups(kubeClient client.Client, clusterName string, processGroups []string, namespace string, withExclusion bool, wait bool, removeAllFailed bool, useProcessGroupID bool) error { - if len(processGroups) == 0 && !removeAllFailed { +func replaceProcessGroups(kubeClient client.Client, clusterName string, processGroupIDs []string, namespace string, withExclusion bool, wait bool, removeAllFailed bool, useProcessGroupID bool) error { + if len(processGroupIDs) == 0 && !removeAllFailed { return nil } @@ -123,7 +123,7 @@ func replaceProcessGroups(kubeClient client.Client, clusterName string, processG // In this case the user has Pod name specified if !useProcessGroupID { - processGroups, err = getProcessGroupIDsFromPodName(cluster, processGroups) + processGroupIDs, err = getProcessGroupIDsFromPodName(cluster, processGroupIDs) if err != nil { return err } @@ -132,7 +132,7 @@ func replaceProcessGroups(kubeClient client.Client, clusterName string, processG patch := client.MergeFrom(cluster.DeepCopy()) processGroupSet := map[string]fdbv1beta2.None{} - for _, processGroup := range processGroups { + for _, processGroup := range processGroupIDs { processGroupSet[processGroup] = fdbv1beta2.None{} } @@ -145,22 +145,21 @@ func replaceProcessGroups(kubeClient client.Client, clusterName string, processG needsReplacement, _ := processGroupStatus.NeedsReplacement(0) if needsReplacement { - processGroups = append(processGroups, processGroupStatus.ProcessGroupID) + processGroupIDs = append(processGroupIDs, processGroupStatus.ProcessGroupID) } } } if wait { - confirmed := confirmAction(fmt.Sprintf("Remove %v from cluster %s/%s with exclude: %t", processGroups, namespace, clusterName, withExclusion)) - if !confirmed { + if !confirmAction(fmt.Sprintf("Remove %v from cluster %s/%s with exclude: %t", processGroupIDs, namespace, clusterName, withExclusion)) { return fmt.Errorf("user aborted the removal") } } if withExclusion { - cluster.AddProcessGroupsToRemovalList(processGroups) + cluster.AddProcessGroupsToRemovalList(processGroupIDs) } else { - cluster.AddProcessGroupsToRemovalWithoutExclusionList(processGroups) + cluster.AddProcessGroupsToRemovalWithoutExclusionList(processGroupIDs) } return kubeClient.Patch(ctx.TODO(), cluster, patch)