Merge pull request #3333 from sbueringer/pr-result-priority

✨ Add optional Priority field to reconcile.Result

Author: k8s-ci-robot

pkg/internal/controller/controller.go

@@ -459,6 +459,9 @@ func (c *Controller[request]) reconcileHandler(ctx context.Context, req request,
 	// resource to be synced.
 	log.V(5).Info("Reconciling")
 	result, err := c.Reconcile(ctx, req)
+	if result.Priority != nil {
+		priority = *result.Priority
+	}
 	switch {
 	case err != nil:
 		if errors.Is(err, reconcile.TerminalError(nil)) {
@@ -468,8 +471,8 @@ func (c *Controller[request]) reconcileHandler(ctx context.Context, req request,
 		}
 		ctrlmetrics.ReconcileErrors.WithLabelValues(c.Name).Inc()
 		ctrlmetrics.ReconcileTotal.WithLabelValues(c.Name, labelError).Inc()
-		if !result.IsZero() {
-			log.Info("Warning: Reconciler returned both a non-zero result and a non-nil error. The result will always be ignored if the error is non-nil and the non-nil error causes requeuing with exponential backoff. For more details, see: https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/reconcile#Reconciler")
+		if result.RequeueAfter > 0 || result.Requeue { //nolint: staticcheck // We have to handle Requeue until it is removed
+			log.Info("Warning: Reconciler returned both a result with either RequeueAfter or Requeue set and a non-nil error. RequeueAfter and Requeue will always be ignored if the error is non-nil. For more details, see: https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/reconcile#Reconciler")
 		}
 		log.Error(err, "Reconciler error")
 	case result.RequeueAfter > 0:

pkg/internal/controller/controller_test.go

@@ -745,7 +745,36 @@ var _ = Describe("controller", func() {
 			}}))
 		})
 
-		It("should requeue a Request after a duration (but not rate-limitted) if the Result sets RequeueAfter (regardless of Requeue)", func(ctx SpecContext) {
+		It("should use the priority from Result when the reconciler requests a requeue", func(ctx SpecContext) {
+			q := &fakePriorityQueue{PriorityQueue: priorityqueue.New[reconcile.Request]("controller1")}
+			ctrl.NewQueue = func(string, workqueue.TypedRateLimiter[reconcile.Request]) workqueue.TypedRateLimitingInterface[reconcile.Request] {
+				return q
+			}
+
+			go func() {
+				defer GinkgoRecover()
+				Expect(ctrl.Start(ctx)).NotTo(HaveOccurred())
+			}()
+
+			q.PriorityQueue.AddWithOpts(priorityqueue.AddOpts{Priority: ptr.To(10)}, request)
+
+			By("Invoking Reconciler which will request a requeue")
+			fakeReconcile.AddResult(reconcile.Result{Requeue: true, Priority: ptr.To(99)}, nil)
+			Expect(<-reconciled).To(Equal(request))
+			Eventually(func() []priorityQueueAddition {
+				q.lock.Lock()
+				defer q.lock.Unlock()
+				return q.added
+			}).Should(Equal([]priorityQueueAddition{{
+				AddOpts: priorityqueue.AddOpts{
+					RateLimited: true,
+					Priority:    ptr.To(99),
+				},
+				items: []reconcile.Request{request},
+			}}))
+		})
+
+		It("should requeue a Request after a duration (but not rate-limited) if the Result sets RequeueAfter (regardless of Requeue)", func(ctx SpecContext) {
 			dq := &DelegatingQueue{TypedRateLimitingInterface: ctrl.NewQueue("controller1", nil)}
 			ctrl.NewQueue = func(string, workqueue.TypedRateLimiter[reconcile.Request]) workqueue.TypedRateLimitingInterface[reconcile.Request] {
 				return dq
@@ -775,7 +804,7 @@ var _ = Describe("controller", func() {
 			Eventually(func() int { return dq.NumRequeues(request) }).Should(Equal(0))
 		})
 
-		It("should retain the priority with RequeAfter", func(ctx SpecContext) {
+		It("should retain the priority with RequeueAfter", func(ctx SpecContext) {
 			q := &fakePriorityQueue{PriorityQueue: priorityqueue.New[reconcile.Request]("controller1")}
 			ctrl.NewQueue = func(string, workqueue.TypedRateLimiter[reconcile.Request]) workqueue.TypedRateLimitingInterface[reconcile.Request] {
 				return q
@@ -804,6 +833,35 @@ var _ = Describe("controller", func() {
 			}}))
 		})
 
+		It("should use the priority from Result with RequeueAfter", func(ctx SpecContext) {
+			q := &fakePriorityQueue{PriorityQueue: priorityqueue.New[reconcile.Request]("controller1")}
+			ctrl.NewQueue = func(string, workqueue.TypedRateLimiter[reconcile.Request]) workqueue.TypedRateLimitingInterface[reconcile.Request] {
+				return q
+			}
+
+			go func() {
+				defer GinkgoRecover()
+				Expect(ctrl.Start(ctx)).NotTo(HaveOccurred())
+			}()
+
+			q.PriorityQueue.AddWithOpts(priorityqueue.AddOpts{Priority: ptr.To(10)}, request)
+
+			By("Invoking Reconciler which will ask for RequeueAfter")
+			fakeReconcile.AddResult(reconcile.Result{RequeueAfter: time.Millisecond * 100, Priority: ptr.To(99)}, nil)
+			Expect(<-reconciled).To(Equal(request))
+			Eventually(func() []priorityQueueAddition {
+				q.lock.Lock()
+				defer q.lock.Unlock()
+				return q.added
+			}).Should(Equal([]priorityQueueAddition{{
+				AddOpts: priorityqueue.AddOpts{
+					After:    time.Millisecond * 100,
+					Priority: ptr.To(99),
+				},
+				items: []reconcile.Request{request},
+			}}))
+		})
+
 		It("should perform error behavior if error is not nil, regardless of RequeueAfter", func(ctx SpecContext) {
 			dq := &DelegatingQueue{TypedRateLimitingInterface: ctrl.NewQueue("controller1", nil)}
 			ctrl.NewQueue = func(string, workqueue.TypedRateLimiter[reconcile.Request]) workqueue.TypedRateLimitingInterface[reconcile.Request] {
@@ -862,6 +920,35 @@ var _ = Describe("controller", func() {
 			}}))
 		})
 
+		It("should use the priority from Result when there was an error", func(ctx SpecContext) {
+			q := &fakePriorityQueue{PriorityQueue: priorityqueue.New[reconcile.Request]("controller1")}
+			ctrl.NewQueue = func(string, workqueue.TypedRateLimiter[reconcile.Request]) workqueue.TypedRateLimitingInterface[reconcile.Request] {
+				return q
+			}
+
+			go func() {
+				defer GinkgoRecover()
+				Expect(ctrl.Start(ctx)).NotTo(HaveOccurred())
+			}()
+
+			q.PriorityQueue.AddWithOpts(priorityqueue.AddOpts{Priority: ptr.To(10)}, request)
+
+			By("Invoking Reconciler which will return an error")
+			fakeReconcile.AddResult(reconcile.Result{Priority: ptr.To(99)}, errors.New("oups, I did it again"))
+			Expect(<-reconciled).To(Equal(request))
+			Eventually(func() []priorityQueueAddition {
+				q.lock.Lock()
+				defer q.lock.Unlock()
+				return q.added
+			}).Should(Equal([]priorityQueueAddition{{
+				AddOpts: priorityqueue.AddOpts{
+					RateLimited: true,
+					Priority:    ptr.To(99),
+				},
+				items: []reconcile.Request{request},
+			}}))
+		})
+
 		PIt("should return if the queue is shutdown", func() {
 			// TODO(community): write this test
 		})

pkg/reconcile/reconcile.go

@@ -44,6 +44,11 @@ type Result struct {
 	// RequeueAfter if greater than 0, tells the Controller to requeue the reconcile key after the Duration.
 	// Implies that Requeue is true, there is no need to set Requeue to true at the same time as RequeueAfter.
 	RequeueAfter time.Duration
+
+	// Priority is the priority that will be used if the item gets re-enqueued (also if an error is returned).
+	// If Priority is not set the original Priority of the request is preserved.
+	// Note: Priority is only respected if the controller is using a priorityqueue.PriorityQueue.
+	Priority *int
 }
 
 // IsZero returns true if this result is empty.