Add unit-test for lease held by another entity scenario

Check that we enter this scenario and act properly by exceeding max retries and having failed manitenance event. Before the fix the obtainLease function always returned (false, err) for any error, including AlreadyHeldError. In the reconciler, the condition err != nil && updateOwnedLeaseFailed was never true, so ErrorOnLeaseCount was never incremented — it was actually reset to 0 in the fallthrough if err != nil block. Now obtainLease function returns (true, err) specifically for AlreadyHeldError, so ErrorOnLeaseCount properly increments on each reconcile. After 4 iterations (> 3), the node is uncordoned and maintenance transitions to Failed.

Author: razo7

controllers/controllers_suite_test.go

@@ -94,7 +94,7 @@ var _ = BeforeSuite(func() {
 		Client:       k8sClient,
 		Scheme:       scheme.Scheme,
 		MgrConfig:    cfg,
-		LeaseManager: &mockLeaseManager{mockManager},
+		LeaseManager: &mockLeaseManager{Manager: mockManager},
 		Recorder:     fakeRecorder,
 		logger:       ctrl.Log.WithName("unit test"),
 	}

controllers/nodemaintenance_controller_test.go

@@ -260,6 +260,37 @@ var _ = Describe("Node Maintenance", func() {
 				verifyNoEvent(corev1.EventTypeNormal, utils.EventReasonSucceedMaintenance, utils.EventMessageSucceedMaintenance)
 			})
 		})
+		When("lease is already held by another entity", func() {
+			BeforeEach(func() {
+				originalLeaseManager := r.LeaseManager
+				r.LeaseManager = &mockLeaseManager{
+					Manager:         originalLeaseManager.(*mockLeaseManager).Manager,
+					requestLeaseErr: lease.AlreadyHeldError{},
+				}
+				DeferCleanup(func() {
+					r.LeaseManager = originalLeaseManager
+				})
+
+				nm = getTestNM("node-maintenance-lease-held", taintedNodeName)
+				Expect(k8sClient.Create(ctx, nm)).To(Succeed())
+				DeferCleanup(k8sClient.Delete, ctx, nm)
+			})
+			It("should increment ErrorOnLeaseCount and fail maintenance after exceeding max retries", func() {
+				By("Waiting for ErrorOnLeaseCount to exceed the threshold and phase to become Failed")
+				maintenance := &v1beta1.NodeMaintenance{}
+				Eventually(func(g Gomega) {
+					g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(nm), maintenance)).To(Succeed())
+					g.Expect(maintenance.Status.ErrorOnLeaseCount).To(BeNumerically(">", maxAllowedErrorToUpdateOwnedLease))
+					g.Expect(maintenance.Status.Phase).To(Equal(v1beta1.MaintenanceFailed))
+				}, "3s", "250ms").Should(Succeed())
+
+				By("Verifying maintenance failed event was emitted")
+				verifyEvent(corev1.EventTypeWarning, utils.EventReasonFailedMaintenance, utils.EventMessageFailedMaintenance)
+
+				By("Verifying LastError mentions lease contention")
+				Expect(maintenance.Status.LastError).To(ContainSubstring("failed to extend lease owned by us"))
+			})
+		})
 	})
 })
 
@@ -413,8 +444,9 @@ func clearEvents() {
 
 type mockLeaseManager struct {
 	lease.Manager
+	requestLeaseErr error
 }
 
 func (mock *mockLeaseManager) RequestLease(_ context.Context, _ client.Object, _ time.Duration) error {
-	return nil
+	return mock.requestLeaseErr
 }