Fix general watchdog usage in unit test, and 1 specific wrong test

- Do not test watchdog feeding (is in new watchdog unit tests now)
- Test watchdog status instead
- Reset watchdog before each test! Otherwise we get false positives,
  because earlier tests triggered the watchdog already!
- Fix "Unhealthy node without api-server access" test: the watchdog
  should NOT be triggered!

Signed-off-by: Marc Sluiter <[email protected]>

Author: slintes

controllers/tests/controller/selfnoderemediation_controller_test.go

@@ -25,6 +25,7 @@ import (
 	"github.com/medik8s/self-node-remediation/controllers"
 	"github.com/medik8s/self-node-remediation/controllers/tests/shared"
 	"github.com/medik8s/self-node-remediation/pkg/utils"
+	"github.com/medik8s/self-node-remediation/pkg/watchdog"
 )
 
 const (
@@ -44,6 +45,9 @@ var _ = Describe("SNR Controller", func() {
 		snr.Namespace = snrNamespace
 		snrConfig = shared.GenerateTestConfig()
 		time.Sleep(time.Second * 2)
+
+		// reset watchdog for each test!
+		dummyDog.Reset()
 	})
 
 	JustBeforeEach(func() {
@@ -183,7 +187,7 @@ var _ = Describe("SNR Controller", func() {
 
 				verifyTimeHasBeenRebootedExists(snr)
 
-				verifyNoWatchdogFood()
+				verifyWatchdogTriggered()
 
 				verifyEvent("Normal", "NodeReboot", "Remediation process - about to attempt fencing the unhealthy node by rebooting it")
 
@@ -233,7 +237,7 @@ var _ = Describe("SNR Controller", func() {
 
 				verifyTimeHasBeenRebootedExists(snr)
 
-				verifyNoWatchdogFood()
+				verifyWatchdogTriggered()
 
 				// 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()
@@ -315,7 +319,7 @@ var _ = Describe("SNR Controller", func() {
 
 					verifyTimeHasBeenRebootedExists(snr)
 
-					verifyNoWatchdogFood()
+					verifyWatchdogTriggered()
 
 					verifyFinalizerExists(snr)
 
@@ -436,30 +440,16 @@ var _ = Describe("SNR Controller", func() {
 	})
 
 	Context("Unhealthy node without api-server access", func() {
-
-		// this is not a controller test anymore... it's testing peers. But keep it here for now...
-
 		BeforeEach(func() {
 			By("Simulate api-server failure")
 			k8sClient.ShouldSimulateFailure = true
 			remediationStrategy = v1alpha1.ResourceDeletionRemediationStrategy
 		})
 
-		It("Verify that watchdog is not receiving food after some time", func() {
-			lastFoodTime := dummyDog.LastFoodTime()
-			timeout := dummyDog.GetTimeout()
-			Eventually(func() bool {
-				newTime := dummyDog.LastFoodTime()
-				// ensure the timeout passed
-				timeoutPassed := time.Now().After(lastFoodTime.Add(3 * timeout))
-				// ensure wd wasn't feeded
-				missedFeed := newTime.Before(lastFoodTime.Add(timeout))
-				if timeoutPassed && missedFeed {
-					return true
-				}
-				lastFoodTime = newTime
-				return false
-			}, 10*shared.PeerUpdateInterval, timeout).Should(BeTrue())
+		Context("no peer found", func() {
+			It("Verify that watchdog is not triggered", func() {
+				verifyWatchdogNotTriggered()
+			})
 		})
 	})
 
@@ -532,12 +522,16 @@ func verifyNodeIsSchedulable() {
 	}, 95*time.Second, 250*time.Millisecond).Should(BeFalse())
 }
 
-func verifyNoWatchdogFood() {
-	By("Verify that watchdog is not receiving food")
-	currentLastFoodTime := dummyDog.LastFoodTime()
-	ConsistentlyWithOffset(1, func() time.Time {
-		return dummyDog.LastFoodTime()
-	}, 5*dummyDog.GetTimeout(), 1*time.Second).Should(Equal(currentLastFoodTime), "watchdog should not receive food anymore")
+func verifyWatchdogTriggered() {
+	EventuallyWithOffset(1, func(g Gomega) {
+		g.Expect(dummyDog.Status()).To(Equal(watchdog.Triggered))
+	}, 5*dummyDog.GetTimeout(), 1*time.Second).Should(Succeed(), "watchdog should be triggered")
+}
+
+func verifyWatchdogNotTriggered() {
+	ConsistentlyWithOffset(1, func(g Gomega) {
+		g.Expect(dummyDog.Status()).To(Equal(watchdog.Armed))
+	}, 5*dummyDog.GetTimeout(), 1*time.Second).Should(Succeed(), "watchdog should not be triggered")
 }
 
 func verifyFinalizerExists(snr *v1alpha1.SelfNodeRemediation) {

controllers/tests/controller/suite_test.go

@@ -54,7 +54,7 @@ import (
 
 var (
 	testEnv                 *envtest.Environment
-	dummyDog                watchdog.Watchdog
+	dummyDog                watchdog.FakeWatchdog
 	unhealthyNode, peerNode = &v1.Node{}, &v1.Node{}
 	cancelFunc              context.CancelFunc
 	k8sClient               *shared.K8sClientWrapper

pkg/watchdog/fake.go

@@ -11,14 +11,22 @@ const (
 	fakeTimeout = 1 * time.Second
 )
 
+type FakeWatchdog interface {
+	Watchdog
+	Reset()
+}
+
 // fakeWatchdogImpl provides the fake implementation of the watchdogImpl interface for tests
 type fakeWatchdogImpl struct {
 	IsStartSuccessful bool
+	*synchronizedWatchdog
 }
 
-func NewFake(isStartSuccessful bool) Watchdog {
+func NewFake(isStartSuccessful bool) FakeWatchdog {
 	fakeWDImpl := &fakeWatchdogImpl{IsStartSuccessful: isStartSuccessful}
-	return newSynced(ctrl.Log.WithName("fake watchdog"), fakeWDImpl)
+	syncedWD := newSynced(ctrl.Log.WithName("fake watchdog"), fakeWDImpl)
+	fakeWDImpl.synchronizedWatchdog = syncedWD
+	return fakeWDImpl
 }
 
 func (f *fakeWatchdogImpl) start() (*time.Duration, error) {
@@ -36,3 +44,14 @@ func (f *fakeWatchdogImpl) feed() error {
 func (f *fakeWatchdogImpl) disarm() error {
 	return nil
 }
+
+func (f *fakeWatchdogImpl) Reset() {
+	swd := f.synchronizedWatchdog
+	swd.mutex.Lock()
+	defer swd.mutex.Unlock()
+	if swd.status != Armed {
+		swd.stop()
+		swd.status = Armed
+		swd.startFeeding()
+	}
+}

pkg/watchdog/synchronized.go

@@ -65,22 +65,7 @@ func (swd *synchronizedWatchdog) Start(ctx context.Context) error {
 	swd.log.Info("watchdog started")
 	swd.mutex.Unlock()
 
-	feedCtx, cancel := context.WithCancel(context.Background())
-	swd.stop = cancel
-	// feed until stopped
-	go wait.NonSlidingUntilWithContext(feedCtx, func(feedCtx context.Context) {
-		swd.mutex.Lock()
-		defer swd.mutex.Unlock()
-		// prevent feeding of a disarmed watchdog in case the context isn't cancelled yet
-		if swd.status != Armed {
-			return
-		}
-		if err := swd.impl.feed(); err != nil {
-			swd.log.Error(err, "failed to feed watchdog!")
-		} else {
-			swd.lastFoodTime = time.Now()
-		}
-	}, swd.timeout/3)
+	swd.startFeeding()
 
 	<-ctx.Done()
 
@@ -100,6 +85,25 @@ func (swd *synchronizedWatchdog) Start(ctx context.Context) error {
 	return nil
 }
 
+func (swd *synchronizedWatchdog) startFeeding() {
+	feedCtx, cancel := context.WithCancel(context.Background())
+	swd.stop = cancel
+	// feed until stopped
+	go wait.NonSlidingUntilWithContext(feedCtx, func(feedCtx context.Context) {
+		swd.mutex.Lock()
+		defer swd.mutex.Unlock()
+		// prevent feeding of a disarmed watchdog in case the context isn't cancelled yet
+		if swd.status != Armed {
+			return
+		}
+		if err := swd.impl.feed(); err != nil {
+			swd.log.Error(err, "failed to feed watchdog!")
+		} else {
+			swd.lastFoodTime = time.Now()
+		}
+	}, swd.timeout/3)
+}
+
 func (swd *synchronizedWatchdog) Stop() {
 	swd.mutex.Lock()
 	defer swd.mutex.Unlock()

pkg/watchdog/watchdog_test.go

@@ -12,7 +12,7 @@ import (
 
 var _ = Describe("Watchdog", func() {
 
-	var wd watchdog.Watchdog
+	var wd watchdog.FakeWatchdog
 	var cancel context.CancelFunc
 
 	BeforeEach(func() {
@@ -65,6 +65,20 @@ var _ = Describe("Watchdog", func() {
 			verifyNoWatchdogFood(wd)
 		})
 	})
+
+	Context("Triggered watchdog reset", func() {
+		BeforeEach(func() {
+			wd.Stop()
+			wd.Reset()
+		})
+
+		It("should be armed and fed", func() {
+			Eventually(func(g Gomega) {
+				g.Expect(wd.Status()).To(Equal(watchdog.Armed))
+			}, 1*time.Second, 100*time.Millisecond).Should(Succeed(), "watchdog should be armed")
+			verifyWatchdogFood(wd)
+		})
+	})
 })
 
 func verifyWatchdogFood(wd watchdog.Watchdog) {