Fix #392

pkg/internal/controller/controller.go

@@ -214,6 +214,13 @@ func (c *Controller) processNextWorkItem() bool {
 		ctrlmetrics.ReconcileTotal.WithLabelValues(c.Name, "error").Inc()
 		return false
 	} else if result.RequeueAfter > 0 {
+		// The result.RequeueAfter request will be lost, if it is returned
+		// along with a non-nil error. But this is intended as
+		// We need to drive to stable reconcile loops before queuing due
+		// to result.RequestAfter
+		if err == nil {
+			c.Queue.Forget(obj)
+		}
 		c.Queue.AddAfter(req, result.RequeueAfter)
 		ctrlmetrics.ReconcileTotal.WithLabelValues(c.Name, "requeue_after").Inc()
 		return true

pkg/internal/controller/controller_test.go

@@ -360,7 +360,7 @@ var _ = Describe("controller", func() {
 		})
 
 		It("should requeue a Request after a duration if the Result sets Requeue:true and "+
-			"RequeueAfter is set", func() {
+			"RequeueAfter is set and forget the item", func() {
 			fakeReconcile.Result.RequeueAfter = time.Millisecond * 100
 			go func() {
 				defer GinkgoRecover()
@@ -375,14 +375,14 @@ var _ = Describe("controller", func() {
 
 			By("Invoking Reconciler which will ask for requeue")
 			Expect(<-reconciled).To(Equal(request))
-			Expect(dq.countAdd).To(Equal(1))
+			Expect(dq.countAdd).To(Equal(0))
 			Expect(dq.countAddAfter).To(Equal(1))
 			Expect(dq.countAddRateLimited).To(Equal(0))
 
 			By("Invoking Reconciler a second time without asking for requeue")
 			fakeReconcile.Result.Requeue = false
 			Expect(<-reconciled).To(Equal(request))
-			Expect(dq.countAdd).To(Equal(1))
+			Expect(dq.countAdd).To(Equal(0))
 			Expect(dq.countAddAfter).To(Equal(1))
 			Expect(dq.countAddRateLimited).To(Equal(0))
 
@@ -391,6 +391,42 @@ var _ = Describe("controller", func() {
 			Eventually(func() int { return ctrl.Queue.NumRequeues(request) }).Should(Equal(0))
 		})
 
+		PIt("should not requeue a Request after a duration if the Result sets Requeue:true and "+
+			"RequeueAfter is set and err is not nil", func() {
+
+			fakeReconcile.Result.RequeueAfter = time.Millisecond * 100
+			fakeReconcile.Err = fmt.Errorf("expected error: reconcile")
+			go func() {
+				defer GinkgoRecover()
+				Expect(ctrl.Start(stop)).NotTo(HaveOccurred())
+			}()
+			dq := &DelegatingQueue{RateLimitingInterface: ctrl.Queue}
+			ctrl.Queue = dq
+			ctrl.JitterPeriod = time.Millisecond
+			ctrl.Queue.Add(request)
+			Expect(dq.countAdd).To(Equal(1))
+			Expect(dq.countAddAfter).To(Equal(0))
+			Expect(dq.countAddRateLimited).To(Equal(0))
+
+			By("Invoking Reconciler which will ask for requeue with an error")
+			Expect(<-reconciled).To(Equal(request))
+			Expect(dq.countAdd).To(Equal(1))
+			Expect(dq.countAddAfter).To(Equal(0))
+			Expect(dq.countAddRateLimited).To(Equal(1))
+
+			fakeReconcile.Result.RequeueAfter = time.Millisecond * 100
+			fakeReconcile.Err = nil
+			By("Invoking Reconciler a second time asking for requeue without errors")
+			Expect(<-reconciled).To(Equal(request))
+			Expect(dq.countAdd).To(Equal(0))
+			Expect(dq.countAddAfter).To(Equal(1))
+			Expect(dq.countAddRateLimited).To(Equal(1))
+
+			By("Removing the item from the queue")
+			Eventually(ctrl.Queue.Len).Should(Equal(0))
+			Eventually(func() int { return ctrl.Queue.NumRequeues(request) }).Should(Equal(0))
+		})
+
 		PIt("should forget the Request if Reconciler is successful", func() {
 			// TODO(community): write this test
 		})
@@ -639,3 +675,8 @@ func (q *DelegatingQueue) Add(item interface{}) {
 	q.countAdd++
 	q.RateLimitingInterface.Add(item)
 }
+
+func (q *DelegatingQueue) Forget(item interface{}) {
+	q.countAdd--
+	q.RateLimitingInterface.Forget(item)
+}