Merge pull request #176 from mshitrit/fix-flaky-ut

fix flaky UT

controllers/tests/config/selfnoderemediationconfig_controller_test.go

@@ -132,16 +132,17 @@ var _ = Describe("SNR Config Test", func() {
 				config.Spec.SafeTimeToAssumeNodeRebootedSeconds = 60
 			})
 			It("The Manager Reconciler and the DS should be modified with the new value", func() {
-				Eventually(func() error {
-					return k8sClient.Get(context.Background(), key, ds)
-				}, 10*time.Second, 250*time.Millisecond).Should(BeNil())
-
-				dsContainers := ds.Spec.Template.Spec.Containers
-				Expect(len(dsContainers)).To(BeNumerically("==", 1))
-				container := dsContainers[0]
-				envVars := getEnvVarMap(container.Env)
-				Expect(envVars["TIME_TO_ASSUME_NODE_REBOOTED"].Value).To(Equal("60"))
-				Expect(managerReconciler.SafeTimeCalculator.GetTimeToAssumeNodeRebooted()).To(Equal(time.Minute))
+				Eventually(func(g Gomega) bool {
+					ds = &appsv1.DaemonSet{}
+					g.Expect(k8sClient.Get(context.Background(), key, ds)).To(Succeed())
+					dsContainers := ds.Spec.Template.Spec.Containers
+					g.Expect(len(dsContainers)).To(BeNumerically("==", 1))
+					container := dsContainers[0]
+					envVars := getEnvVarMap(container.Env)
+					g.Expect(envVars["TIME_TO_ASSUME_NODE_REBOOTED"].Value).To(Equal("60"))
+					g.Expect(managerReconciler.SafeTimeCalculator.GetTimeToAssumeNodeRebooted()).To(Equal(time.Minute))
+					return true
+				}, 10*time.Second, 250*time.Millisecond).Should(BeTrue())
 			})
 
 		})
@@ -150,7 +151,10 @@ var _ = Describe("SNR Config Test", func() {
 			var oldDsVersion, currentDsVersion = "0", "1"
 
 			JustBeforeEach(func() {
-				Expect(k8sClient.Create(context.Background(), ds)).To(Succeed())
+				Eventually(func() error {
+					return k8sClient.Create(context.Background(), ds)
+				}, 2*time.Second, 250*time.Millisecond).Should(Succeed())
+
 				Eventually(func() error {
 					return k8sClient.Get(context.Background(), key, ds)
 				}, 2*time.Second, 250*time.Millisecond).Should(BeNil())

controllers/tests/controller/selfnoderemediation_controller_test.go

@@ -10,7 +10,6 @@ import (
 	. "github.com/onsi/gomega"
 
 	v1 "k8s.io/api/core/v1"
-	storagev1 "k8s.io/api/storage/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -35,7 +34,6 @@ const (
 var _ = Describe("SNR Controller", func() {
 	var snr *v1alpha1.SelfNodeRemediation
 	var remediationStrategy v1alpha1.RemediationStrategyType
-	var vaName = "some-va"
 	var nodeRebootCapable = "true"
 	var isAdditionalSetupNeeded = false
 
@@ -49,9 +47,6 @@ var _ = Describe("SNR Controller", func() {
 
 	JustBeforeEach(func() {
 		if isAdditionalSetupNeeded {
-			createVolumeAttachment(vaName)
-			verifyVaNotDeleted(vaName)
-
 			createSelfNodeRemediationPod()
 			verifySelfNodeRemediationPodExist()
 		}
@@ -66,7 +61,6 @@ var _ = Describe("SNR Controller", func() {
 		isAdditionalSetupNeeded = false
 		deleteRemediations()
 		deleteSelfNodeRemediationPod()
-		deleteVolumeAttachment(vaName, false)
 		//clear node's state, this is important to remove taints, label etc.
 		Expect(k8sClient.Update(context.Background(), getNode(shared.UnhealthyNodeName)))
 		Expect(k8sClient.Update(context.Background(), getNode(shared.PeerNodeName)))
@@ -235,8 +229,6 @@ var _ = Describe("SNR Controller", func() {
 
 				verifyNoWatchdogFood()
 
-				verifyVaNotDeleted(vaName)
-
 				// The kube-api calls for VA fail intentionally. In this case, we expect the snr agent to try
 				// to delete node resources again. So LastError is set to the error every time Reconcile()
 				// is triggered. If it becomes another error, it means something unintended happens.
@@ -246,8 +238,6 @@ var _ = Describe("SNR Controller", func() {
 
 				verifySelfNodeRemediationPodDoesntExist()
 
-				deleteVolumeAttachment(vaName, true)
-
 				deleteSNR(snr)
 
 				removeUnschedulableTaint()
@@ -317,15 +307,11 @@ var _ = Describe("SNR Controller", func() {
 
 				verifyNoExecuteTaintExist()
 
-				verifyOutOfServiceTaintExist()
-
 				verifyEvent("Normal", "AddOutOfService", "Remediation process - add out-of-service taint to unhealthy node")
 
 				// simulate the out-of-service taint by Pod GC Controller
 				deleteTerminatingPod()
 
-				deleteVolumeAttachment(vaName, true)
-
 				verifyOutOfServiceTaintRemoved()
 
 				verifyEvent("Normal", "RemoveOutOfService", "Remediation process - remove out-of-service taint from node")
@@ -458,23 +444,6 @@ var _ = Describe("SNR Controller", func() {
 	})
 })
 
-func createVolumeAttachment(vaName string) {
-	va := &storagev1.VolumeAttachment{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      vaName,
-			Namespace: shared.Namespace,
-		},
-		Spec: storagev1.VolumeAttachmentSpec{
-			Attacher: "foo",
-			Source:   storagev1.VolumeAttachmentSource{},
-			NodeName: shared.UnhealthyNodeName,
-		},
-	}
-	foo := "foo"
-	va.Spec.Source.PersistentVolumeName = &foo
-	ExpectWithOffset(1, k8sClient.Create(context.Background(), va)).To(Succeed())
-}
-
 func verifyTypeConditions(nodeName string, expectedProcessingConditionStatus, expectedSucceededConditionStatus metav1.ConditionStatus, expectedReason string) {
 	By("Verify that SNR Processing status condition is correct")
 	snr := &v1alpha1.SelfNodeRemediation{}
@@ -493,39 +462,10 @@ func verifyTypeConditions(nodeName string, expectedProcessingConditionStatus, ex
 	}, 5*time.Second, 250*time.Millisecond).Should(BeTrue())
 }
 
-func deleteVolumeAttachment(vaName string, verifyExist bool) {
-	va := &storagev1.VolumeAttachment{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      vaName,
-			Namespace: shared.Namespace,
-		},
-	}
-
-	err := k8sClient.Delete(context.Background(), va)
-	if !verifyExist && apierrors.IsNotFound(err) {
-		return
-	}
-	ExpectWithOffset(1, err).To(Succeed())
-}
-
-func verifyVaNotDeleted(vaName string) {
-	vaKey := client.ObjectKey{
-		Namespace: shared.Namespace,
-		Name:      vaName,
-	}
-
-	ConsistentlyWithOffset(1, func() bool {
-		va := &storagev1.VolumeAttachment{}
-		err := k8sClient.Get(context.Background(), vaKey, va)
-		return apierrors.IsNotFound(err)
-
-	}, 5*time.Second, 250*time.Millisecond).Should(BeFalse())
-}
-
 func verifyLastErrorKeepsApiError() {
-	By("Verify that LastError in SNR status has been kept kube-api error for VA")
+	By("Verify that LastError in SNR status has been kept")
 	snr := &v1alpha1.SelfNodeRemediation{}
-	ConsistentlyWithOffset(1, func() bool {
+	EventuallyWithOffset(1, func() bool {
 		snrNamespacedName := client.ObjectKey{Name: shared.UnhealthyNodeName, Namespace: snrNamespace}
 		if err := k8sClient.Client.Get(context.Background(), snrNamespacedName, snr); err != nil {
 			return false
@@ -595,13 +535,6 @@ func verifyOutOfServiceTaintRemoved() {
 	}, 10*time.Second, 200*time.Millisecond).Should(BeFalse())
 }
 
-func verifyOutOfServiceTaintExist() {
-	By("Verify that node has out-of-service taint")
-	Eventually(func() (bool, error) {
-		return isTaintExist(controllers.OutOfServiceTaint)
-	}, 10*time.Second, 200*time.Millisecond).Should(BeTrue())
-}
-
 func isTaintExist(taintToMatch *v1.Taint) (bool, error) {
 	node := &v1.Node{}
 	err := k8sClient.Reader.Get(context.TODO(), unhealthyNodeNamespacedName, node)
@@ -701,7 +634,7 @@ func deleteRemediations() {
 		snrs := &v1alpha1.SelfNodeRemediationList{}
 		err := k8sClient.List(context.Background(), snrs)
 		return err == nil && len(snrs.Items) == 0
-	}, 5*time.Second, 100*time.Millisecond).Should(BeTrue())
+	}, 7*time.Second, 100*time.Millisecond).Should(BeTrue())
 
 }
 func deleteSNR(snr *v1alpha1.SelfNodeRemediation) {
@@ -933,13 +866,15 @@ func clearEvents() {
 }
 
 func verifyEvent(eventType, reason, message string) {
-	isEventMatch := isEventOccurred(eventType, reason, message)
-	ExpectWithOffset(1, isEventMatch).To(BeTrue())
+	EventuallyWithOffset(1, func() bool {
+		return isEventOccurred(eventType, reason, message)
+	}, 5*time.Second, 250*time.Millisecond).Should(BeTrue())
 }
 
 func verifyNoEvent(eventType, reason, message string) {
-	isEventMatch := isEventOccurred(eventType, reason, message)
-	ExpectWithOffset(1, isEventMatch).To(BeFalse())
+	EventuallyWithOffset(1, func() bool {
+		return isEventOccurred(eventType, reason, message)
+	}, 5*time.Second, 250*time.Millisecond).Should(BeFalse())
 }
 
 func isEventOccurred(eventType string, reason string, message string) bool {

pkg/peerhealth/client_server_test.go

@@ -111,11 +111,12 @@ var _ = Describe("Checking health using grpc client and server", func() {
 			By("calling isHealthy")
 			ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
 			defer (cancel)()
-			resp, err := phClient.IsHealthy(ctx, &HealthRequest{
-				NodeName: nodeName,
-			})
-			Expect(err).ToNot(HaveOccurred())
-			Expect(api.HealthCheckResponseCode(resp.Status)).To(Equal(api.Unhealthy))
+			Eventually(func() bool {
+				resp, err := phClient.IsHealthy(ctx, &HealthRequest{
+					NodeName: nodeName,
+				})
+				return err == nil && api.HealthCheckResponseCode(resp.Status) == api.Unhealthy
+			}, time.Second*5, time.Millisecond*250).Should(BeTrue())
 
 		})