diff --git a/controller/workqueue/defaults.go b/controller/workqueue/defaults.go index 8cbbaf6..0b621f7 100644 --- a/controller/workqueue/defaults.go +++ b/controller/workqueue/defaults.go @@ -37,14 +37,13 @@ var ( DefaultNumOfPriorityLotteries = []int{1, 2, 4, 8, 16} ) -func DefaultGetPriorityFuncBuilder(cli client.Client, objectGetter func() client.Object) GetPriorityFunc { - return GetPriorityFuncBuilder(cli, objectGetter, DefaultWorkQueuePriorityLabel, DefaultWorkQueuePriority) +func DefaultGetPriorityFuncBuilder(cli client.Client) GetPriorityFunc { + return GetPriorityFuncBuilder(cli, DefaultWorkQueuePriorityLabel, DefaultWorkQueuePriority) } // GetPriorityFunc is the function to get the priority of an item -// We use the label to get the priority of an item -// If the label is not set in the item, we will get the priority from the namespace label -func GetPriorityFuncBuilder(cli client.Client, objectGetter func() client.Object, workQueuePriorityLabel string, defaultWorkQueuePriority int) GetPriorityFunc { +// We use the namespace label to get the priority of an item +func GetPriorityFuncBuilder(cli client.Client, workQueuePriorityLabel string, defaultWorkQueuePriority int) GetPriorityFunc { if cli == nil { panic("cli is required") } @@ -54,45 +53,26 @@ func GetPriorityFuncBuilder(cli client.Client, objectGetter func() client.Object return func(item interface{}) int { req, ok := item.(reconcile.Request) - if !ok { - return defaultWorkQueuePriority - } - - object := objectGetter() - err := cli.Get(context.Background(), req.NamespacedName, object) - if err != nil { - klog.Errorf("Failed to get object: %v, error: %v", req.NamespacedName, err) + if !ok || req.Namespace == "" { return defaultWorkQueuePriority } var priorityLableValue string - labels := object.GetLabels() - if len(labels) != 0 { - priorityLableValue = labels[workQueuePriorityLabel] - } - - if priorityLableValue == "" { - if req.Namespace == "" { + namespace := &corev1.Namespace{} + if err := cli.Get(context.Background(), client.ObjectKey{Name: req.Namespace}, namespace); err != nil { + klog.Errorf("Failed to get namespace: %v, error: %v", req.Namespace, err) + return defaultWorkQueuePriority + } else { + labels := namespace.GetLabels() + if len(labels) == 0 { return defaultWorkQueuePriority } - - namespace := &corev1.Namespace{} - if err := cli.Get(context.Background(), client.ObjectKey{Name: req.Namespace}, namespace); err != nil { - klog.Errorf("Failed to get namespace: %v, error: %v", req.Namespace, err) + priorityLableValue, ok = namespace.Labels[workQueuePriorityLabel] + if !ok { return defaultWorkQueuePriority - } else { - labels := namespace.GetLabels() - if len(labels) == 0 { - return defaultWorkQueuePriority - } - priorityLableValue = namespace.Labels[workQueuePriorityLabel] } } - if priorityLableValue == "" { - return defaultWorkQueuePriority - } - priority, err := strconv.Atoi(priorityLableValue) if err != nil { klog.Errorf("Failed to convert label value: %q to int, error: %v", priorityLableValue, err) diff --git a/controller/workqueue/defaults_test.go b/controller/workqueue/defaults_test.go index 22ffd10..5773ba5 100644 --- a/controller/workqueue/defaults_test.go +++ b/controller/workqueue/defaults_test.go @@ -32,142 +32,58 @@ import ( var _ = Describe("Test defaults", func() { const ( - testConfigmap = "configmap1" + testObject = "object1" testNamespace = "default" ) var ( - objectGetter = func() client.Object { - return &corev1.ConfigMap{} - } req = reconcile.Request{NamespacedName: client.ObjectKey{ - Name: testConfigmap, + Name: testObject, Namespace: testNamespace, }} ) - Context("Use default workqueue priority", func() { - It("Should get default workqueue priority if the item has no labels", func() { - err := ensureConfigmap(k8sClient, testNamespace, testConfigmap, DefaultWorkQueuePriorityLabel, "") + Context("Use default workqueue priority label", func() { + It("Should get default priority if default workqueue priority label nil", func() { + err := ensureNamespace(k8sClient, testNamespace, DefaultWorkQueuePriorityLabel, "") Expect(err).NotTo(HaveOccurred()) - getPriorityFunc := DefaultGetPriorityFuncBuilder(k8sClient, objectGetter) + getPriorityFunc := DefaultGetPriorityFuncBuilder(k8sClient) priority := getPriorityFunc(req) Expect(priority).To(Equal(DefaultWorkQueuePriority)) }) - It("Should get workqueue priority from default item priority label", func() { - err := ensureConfigmap(k8sClient, testNamespace, testConfigmap, DefaultWorkQueuePriorityLabel, "3") - Expect(err).NotTo(HaveOccurred()) - - getPriorityFunc := DefaultGetPriorityFuncBuilder(k8sClient, objectGetter) - priority := getPriorityFunc(req) - Expect(priority).To(Equal(3)) - }) - - It("Should get workqueue priority from default namesapce priority label", func() { - err := ensureConfigmap(k8sClient, testNamespace, testConfigmap, DefaultWorkQueuePriorityLabel, "") - Expect(err).NotTo(HaveOccurred()) - - err = ensureNamespace(k8sClient, testNamespace, DefaultWorkQueuePriorityLabel, "4") + It("Should get workqueue priority from default workqueue priority label", func() { + err := ensureNamespace(k8sClient, testNamespace, DefaultWorkQueuePriorityLabel, "4") Expect(err).NotTo(HaveOccurred()) - getPriorityFunc := DefaultGetPriorityFuncBuilder(k8sClient, objectGetter) + getPriorityFunc := DefaultGetPriorityFuncBuilder(k8sClient) priority := getPriorityFunc(req) Expect(priority).To(Equal(4)) }) }) - Context("Use custom workqueue priority", func() { - It("Should get default workqueue priority if the item has no labels", func() { - err := ensureConfigmap(k8sClient, testNamespace, testConfigmap, "custom-priority", "") + Context("Use custom workqueue priority label", func() { + It("Should get default priority if custom workqueue priority label nil", func() { + err := ensureNamespace(k8sClient, testNamespace, "custom-priority", "4") Expect(err).NotTo(HaveOccurred()) - getPriorityFunc := GetPriorityFuncBuilder(k8sClient, objectGetter, "custom-priority", 1) + getPriorityFunc := GetPriorityFuncBuilder(k8sClient, "custom-priority", 1) priority := getPriorityFunc(req) - Expect(priority).To(Equal(1)) - }) - - It("Should get workqueue priority from custom item priority label", func() { - err := ensureConfigmap(k8sClient, testNamespace, testConfigmap, "custom-priority", "3") - Expect(err).NotTo(HaveOccurred()) - - getPriorityFunc := GetPriorityFuncBuilder(k8sClient, objectGetter, "custom-priority", 1) - priority := getPriorityFunc(req) - Expect(priority).To(Equal(3)) + Expect(priority).To(Equal(4)) }) - It("Should get workqueue priority from custom namesapce priority label", func() { - err := ensureConfigmap(k8sClient, testNamespace, testConfigmap, "custom-priority", "") + It("Should get workqueue priority from custom workqueue priority label", func() { + err := ensureNamespace(k8sClient, testNamespace, "custom-priority", "4") Expect(err).NotTo(HaveOccurred()) - err = ensureNamespace(k8sClient, testNamespace, "custom-priority", "4") - Expect(err).NotTo(HaveOccurred()) - - getPriorityFunc := GetPriorityFuncBuilder(k8sClient, objectGetter, "custom-priority", 1) + getPriorityFunc := GetPriorityFuncBuilder(k8sClient, "custom-priority", 1) priority := getPriorityFunc(req) Expect(priority).To(Equal(4)) }) }) }) -func ensureConfigmap(cli client.Client, namespace, name, priorityLabelKey, priorityLabelValue string) error { - // Ensure the configmap exists - configmap := &corev1.ConfigMap{} - err := cli.Get(context.Background(), client.ObjectKey{Name: name, Namespace: namespace}, configmap) - if err != nil { - if errors.IsNotFound(err) { - labels := map[string]string{} - if priorityLabelValue != "" { - labels[priorityLabelKey] = priorityLabelValue - } - - err := cli.Create(context.Background(), &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: name, - Namespace: namespace, - Labels: labels, - }, - }) - return err - } - return err - } - - // If the label is already set, we don't need to update it - labelValue, ok := configmap.Labels[priorityLabelKey] - if !ok && priorityLabelValue == "" { - return nil - } else if ok && labelValue == priorityLabelValue { - return nil - } - - // If the label is not set, we need to set it - if priorityLabelValue == "" { - configmap.Labels = map[string]string{} - } else { - configmap.Labels = map[string]string{ - priorityLabelKey: priorityLabelValue, - } - } - - // Update the configmap - err = cli.Update(context.Background(), configmap) - if err != nil { - return err - } - Eventually(func() bool { - configmap1 := &corev1.ConfigMap{} - err := cli.Get(context.Background(), client.ObjectKey{Name: name, Namespace: namespace}, configmap1) - if err != nil { - return false - } - return configmap1.ResourceVersion >= configmap.ResourceVersion - }, time.Second*3, time.Millisecond*100).Should(BeTrue()) - - return nil -} - func ensureNamespace(cli client.Client, name, priorityLabelKey, priorityLabelValue string) error { // Ensure the namespace exists namespace := &corev1.Namespace{} @@ -190,17 +106,16 @@ func ensureNamespace(cli client.Client, name, priorityLabelKey, priorityLabelVal } // If the label is already set, we don't need to update it - labelValue, ok := namespace.Labels[priorityLabelKey] - if !ok && priorityLabelValue == "" { - return nil - } else if ok && labelValue == priorityLabelValue { - return nil - } - - // If the label is not set, we need to set it + priority, ok := namespace.Labels[priorityLabelKey] if priorityLabelValue == "" { + if !ok { + return nil + } namespace.Labels = map[string]string{} } else { + if ok && priority == priorityLabelValue { + return nil + } namespace.Labels = map[string]string{ priorityLabelKey: priorityLabelValue, } diff --git a/controller/workqueue/priority_queue_test.go b/controller/workqueue/priority_queue_test.go index 9939d02..a2f7107 100644 --- a/controller/workqueue/priority_queue_test.go +++ b/controller/workqueue/priority_queue_test.go @@ -23,7 +23,6 @@ import ( . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" - corev1 "k8s.io/api/core/v1" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/reconcile" ) @@ -31,16 +30,10 @@ import ( var _ = Describe("Test prioriry_queue", func() { const ( controllerName = "controller1" - testConfigmap = "configmap1" + testObject = "object1" testNamespace = "default" ) - var ( - objectGetter = func() client.Object { - return &corev1.ConfigMap{} - } - ) - Context("Get lotteries", func() { It("Invalid numOfPriorityLotteries", func() { _, err := getLotteries([]int{}) @@ -164,18 +157,15 @@ var _ = Describe("Test prioriry_queue", func() { It("Succeed to create PriorityQueue", func() { cfg := &PriorityQueueConfig{ Name: controllerName, - GetPriorityFunc: DefaultGetPriorityFuncBuilder(k8sClient, objectGetter), + GetPriorityFunc: DefaultGetPriorityFuncBuilder(k8sClient), UnfinishedWorkUpdatePeriod: 100, } priorityQueue, err := NewPriorityQueue(cfg) Expect(err).To(Succeed()) Expect(priorityQueue).NotTo(BeNil()) - err = ensureConfigmap(k8sClient, testNamespace, "configmap1", DefaultWorkQueuePriorityLabel, "10") - Expect(err).NotTo(HaveOccurred()) - priorityQueue.Add(reconcile.Request{NamespacedName: client.ObjectKey{ - Name: "configmap1", + Name: "object1", Namespace: testNamespace, }}) Expect(priorityQueue.Len()).To(Equal(1)) @@ -184,19 +174,12 @@ var _ = Describe("Test prioriry_queue", func() { Expect(item).NotTo(BeNil()) Expect(shutdown).To(BeFalse()) - err = ensureConfigmap(k8sClient, testNamespace, "configmap2", DefaultWorkQueuePriorityLabel, "10") - Expect(err).NotTo(HaveOccurred()) - priorityQueue.Add(reconcile.Request{NamespacedName: client.ObjectKey{ - Name: "configmap2", + Name: "object2", Namespace: testNamespace, }}) - - err = ensureConfigmap(k8sClient, testNamespace, "configmap3", DefaultWorkQueuePriorityLabel, "10") - Expect(err).NotTo(HaveOccurred()) - priorityQueue.Add(reconcile.Request{NamespacedName: client.ObjectKey{ - Name: "configmap3", + Name: "object3", Namespace: testNamespace, }}) Expect(priorityQueue.Len()).To(Equal(2)) @@ -213,7 +196,7 @@ var _ = Describe("Test prioriry_queue", func() { }) It("Do not discard item when the priority is invalid", func() { - getPriorityFunc := DefaultGetPriorityFuncBuilder(k8sClient, objectGetter) + getPriorityFunc := DefaultGetPriorityFuncBuilder(k8sClient) cfg := &PriorityQueueConfig{ Name: controllerName, @@ -224,24 +207,24 @@ var _ = Describe("Test prioriry_queue", func() { Expect(err).To(Succeed()) Expect(priorityQueue).NotTo(BeNil()) - err = ensureConfigmap(k8sClient, testNamespace, testConfigmap, DefaultWorkQueuePriorityLabel, strconv.Itoa(len(DefaultNumOfPriorityLotteries)+1)) + err = ensureNamespace(k8sClient, testNamespace, DefaultWorkQueuePriorityLabel, strconv.Itoa(len(DefaultNumOfPriorityLotteries)+1)) Expect(err).NotTo(HaveOccurred()) priority := getPriorityFunc(reconcile.Request{NamespacedName: client.ObjectKey{ - Name: "configmap1", + Name: testObject, Namespace: testNamespace, }}) Expect(priority).To(Equal(len(DefaultNumOfPriorityLotteries) + 1)) priorityQueue.Add(reconcile.Request{NamespacedName: client.ObjectKey{ - Name: testConfigmap, + Name: testObject, Namespace: testNamespace, }}) Expect(priorityQueue.Len()).To(Equal(1)) }) It("Higher priority items have a higher chance of being processed", func() { - getPriorityFunc := DefaultGetPriorityFuncBuilder(k8sClient, objectGetter) + getPriorityFunc := DefaultGetPriorityFuncBuilder(k8sClient) cfg := &PriorityQueueConfig{ Name: controllerName, @@ -253,40 +236,43 @@ var _ = Describe("Test prioriry_queue", func() { Expect(priorityQueue).NotTo(BeNil()) for i := 0; i < 100; i++ { - name := fmt.Sprintf("confitmap0%d", i) + namespace := "namespace0" + name := fmt.Sprintf("object0%d", i) - err := ensureConfigmap(k8sClient, testNamespace, name, DefaultWorkQueuePriorityLabel, strconv.Itoa(DefaultWorkQueuePriority)) + err := ensureNamespace(k8sClient, namespace, DefaultWorkQueuePriorityLabel, strconv.Itoa(DefaultWorkQueuePriority)) Expect(err).NotTo(HaveOccurred()) priorityQueue.Add(reconcile.Request{NamespacedName: client.ObjectKey{ Name: name, - Namespace: testNamespace, + Namespace: namespace, }}) } Expect(priorityQueue.Len()).To(Equal(100)) for i := 0; i < 100; i++ { - name := fmt.Sprintf("confitmap1%d", i) + namespace := "namespace1" + name := fmt.Sprintf("object1%d", i) - err := ensureConfigmap(k8sClient, testNamespace, name, DefaultWorkQueuePriorityLabel, strconv.Itoa(DefaultWorkQueuePriority+1)) + err := ensureNamespace(k8sClient, namespace, DefaultWorkQueuePriorityLabel, strconv.Itoa(DefaultWorkQueuePriority+1)) Expect(err).NotTo(HaveOccurred()) priorityQueue.Add(reconcile.Request{NamespacedName: client.ObjectKey{ Name: name, - Namespace: testNamespace, + Namespace: namespace, }}) } Expect(priorityQueue.Len()).To(Equal(200)) for i := 0; i < 100; i++ { - name := fmt.Sprintf("confitmap2%d", i) + namespace := "namespace2" + name := fmt.Sprintf("object2%d", i) - err := ensureConfigmap(k8sClient, testNamespace, name, DefaultWorkQueuePriorityLabel, strconv.Itoa(DefaultWorkQueuePriority+2)) + err := ensureNamespace(k8sClient, namespace, DefaultWorkQueuePriorityLabel, strconv.Itoa(DefaultWorkQueuePriority+2)) Expect(err).NotTo(HaveOccurred()) priorityQueue.Add(reconcile.Request{NamespacedName: client.ObjectKey{ Name: name, - Namespace: testNamespace, + Namespace: namespace, }}) } Expect(priorityQueue.Len()).To(Equal(300))