Stop Heartbeat monitor jobs on cancelation

[jsoriano]

If a monitor is stopped, for example when using autodiscover, the
scheduled tasks should be stopped too. Scheduler was rescheduling tasks
forever once started, though these tasks were not being executed because
they are also aware of the context.

This change avoids the execution and rescheduling of tasks once its job
context is done.

Checklist

• My code follows the style guidelines of this project
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have made corresponding change to the default configuration files
• I have added tests that prove my fix is effective or that my feature works
• I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

• Start heartbeat with autodiscover enabled and -d scheduler.
• Start some container/pod.
• Wait for the monitor to be configured and executed.
• Stop the container/pod.
• Messages about jobs execution like the following one should eventually stop appearing:

2020-08-12T13:13:43.705+0200	DEBUG	[scheduler]	scheduler/scheduler.go:201	Job 'auto-http-0X2C5537D51C1B9524' returned at 2020-08-12 13:13:43.70518639 +0200 CEST m=+66.010433861

Related issues

• Closes Autodiscover based on services doesn't stop monitors when service is deleted #20544

[elasticmachine]

Pinging @elastic/uptime (Team:Uptime)

[elasticmachine]

💚 Build Succeeded

Expand to view the summary

Build stats

• Build Cause: [Pull request #20570 updated]

• Start Time: 2020-08-12T11:29:50.990+0000

• Duration: 49 min 28 sec

Test stats 🧪

Test	Results
Failed	0
Passed	1200
Skipped	28
Total	1228

[andrewvc]

Really interesting and good find @jsoriano ! I tried to add a test here, but I couldn't, because there was no externally visible thing to test. We are however missing cancellation tests. Would you mind adding this one below to this patch? This should be added to scheduler_test.go.

I guess it's kind of weird to add a test that doesn't really test the patch, but given that there's no good test to write, I think this will have to do. I suppose we could add more insight into the scheduler internals, but that feels like a weird design choice.

func TestCancellingJobs(t *testing.T) {
	s := NewWithLocation(10, monitoring.NewRegistry(), tarawaTime())

	require.NoError(t, s.Start())

	// Mutex to guard removeFn
	taskInitMtx := sync.Mutex{}
	// Let the job run once, then cancel it immediately
	taskInitMtx.Lock()
	var removeFn func ()
	timesRan := batomic.MakeInt(0)
	removeFn, err := s.Add(testSchedule{delay: 0}, "testCancel", func(ctx context.Context) []TaskFunc {
		timesRan.Inc()
		taskInitMtx.Lock()
		removeFn()
		taskInitMtx.Unlock()
		return nil
	})
	require.NoError(t, err)
	taskInitMtx.Unlock()

	// It's hard to tell if the job still exists since
	// we just recursively requeue them, but we should know after a second
	time.Sleep(time.Second)
	require.Equal(t, 1, timesRan.Load())

	require.NoError(t, s.Stop())
}

[jsoriano]

@andrewvc yeah, I also couldn't find a way to test this. As you said we would need to expose scheduler or timer queue internals and can be weird. I thought that a way to do it could be to expose the length of the timer queue, but this is not reliable because the task is not in the queue while it is being executed.

Regarding the test for cancelation, wdyt about discussing it in a separate PR? I think it is always complicated to automatically test for things that are not expected to happen. In this case I am concerned by the sleep, I would prefer not having to add it.

[andrewvc]

LGTM. We can discuss the added test in a separate PR