roachtest: split Monitor functionality in two components #118214

renatolabs · 2024-01-23T15:39:13Z

Typical uses of the roachtest monitor are as follows:

m := c.NewMonitor()
m.Go(func(context.Context) error {
    // user logic...
})

m.Wait()

This is a summary of the two main functions provided by the monitor:

Go: starts a goroutine. This is useful because we never want bare go calls in roachtests (or anywhere, really -- see *: ban the go keyword in non-test code #58164); having automated panic recovery is nice to have for all tests. Note that calling Go will start the goroutine immediately.
Wait: serves two purposes. First, it waits for all goroutines started with Go to return (canceling on error like errgroup). In addition, it also starts a "processs monitor" (hence the name) that will watch for unexpected cockroach process deaths in the nodes it monitors.

This API is less than ideal for the following reasons:

Monitoring processes and managing goroutines are two completely separate functionalities. It is unexpected that we will only monitor node deaths if we also have some goroutine to run. We recently had to patch this module in order to allow callers to just monitor nodes, without having to run a function to achieve that goal (roachtest: allow callers to listen for node events in monitor #108594).
We will only actually monitor nodes if the test calls Wait() on the monitor. While this is good practice and should happen every time, this is not enforced and tests can continue to pass even if they don't. In addition, node deaths that happen before the test calls Wait() go completely unnoticed.
Because goroutine management and process monitoring are coupled, it makes composition really difficult. Suppose a function A creates a new monitor in order to run a background task. Suppose also that we're writing a new test that wants to call A to reuse its functionality. If our test B wishes to restart nodes, we now have to find a way to pass that information down to A so that A's monitor can properly handle that node death event. In the general case, if our test involves creating N monitors (directly or indirectly), and we want to perform a node restart, we need to find a way to pass that information to all N monitors.

Proposal

A proposed way to deal with this is to promote node monitoring to the test runner as a default. Every test should have an active "node monitor" that checks for unexpected node deaths. We could have a single t.ExpectNodeDeath(n1) to indicate that this test expects n1 to die, and that can happen only once. What's currently the Monitor will be deprecated in tests in favour of a simple goroutine runner with builtin panic handling.

Jira issue: CRDB-35614

The text was updated successfully, but these errors were encountered:

blathers-crl · 2024-01-23T15:39:15Z

cc @cockroachdb/test-eng

Previously, roachtests would use bare goroutines `go func...` to start tasks during a test to perform various background functions. This is problematic due to the nature of roachtest, which runs multiple tests in parallel using workers. If one such bare goroutine panics the whole test suite is interrupted and the process exits. This change is the first step and introduces an interface that both the traditional and mixedversion test frameworks can utilise. It intends to target both regular and MVT tests by supplying the test with the interface either through a helper or the test interface. In addition to providing the `Go` function, there are additional options that can be passed. The test framework will generally supply the panic and error handler, but users can supply additional options such as naming the task, or passing a custom logger. This makes logging more useful to determine what task panic or encountered an error. Informs: cockroachdb#118214 Epic: None Release note: None

This change provides a task manager implementation that will drive the logic running behind the Tasker interface, as well as provide the implementation of it. The manager is meant to be used by the test framework(s), while the Tasker interface is supplied to tests. The framework will do additional logic such as terminating tasks at the end of a test, logging uncaught errors or issues and providing the appropriate panic and error handlers. Informs: cockroachdb#118214 Epic: None Release note: None

Previously, the mixedversion framework had its own implementation for managing background tasks. This implementation performed well, but in its current state it is difficult to share with tests that do not require the mixedversion framework. This change replace the mixedversion framework background runner with the newly implemented task manager. The implementation details are relatively similar making it close to a drop-in replacement. Additionally, some utilities relating to tasks have been replaced by the task package. Informs: cockroachdb#118214 Epic: None Release note: None

This change now utilises the Tasker interface supplied by the mixedversion helper to start a task. In essence a background function can also be utilised, but for low impact go routines the utility provides a quick way of starting a task. Informs: cockroachdb#118214 Epic: None Release note: None