roachtest: handle VM overload under tpccbench

See #62039.

`tpccbench`, by design, pushes CRDB into overload territory. However,
tpccbench was not handling the resulting conditions at all. Any node
death or stalling VM would fail the whole test run, rather than retry
again with a lower warehouse count.

This commit makes most errors "recoverable" in the sense that they
will simply treat the run as failing, but continue the line search.
Exceptions are communicated via `t.Fatal`, which will abort the
whole run instead.

We also make the `c.Stop()` step at the beginning of each search
step resilient to VMs browning out under memory pressure (from
the previous run), by patiently retrying for a few minutes.

The hope is that this will allow the search to run to completion, even
in the face of overload-imposed temporary VM outages. It is not expected
to do this perfectly - after all, VMs don't generally return from
brown-out within any fixed time period - but at least anecdotally most
VMs seem to come back a few minutes in.

Release note: None

pkg/cmd/roachtest/tpcc.go

@@ -803,8 +803,42 @@ func runTPCCBench(ctx context.Context, t *test, c *cluster, b tpccBenchSpec) {
 	if res, err := s.Search(func(warehouses int) (bool, error) {
 		iteration++
 		t.l.Printf("initializing cluster for %d warehouses (search attempt: %d)", warehouses, iteration)
+
+		// NB: for goroutines in this monitor, handle errors via `t.Fatal` to
+		// *abort* the line search and whole tpccbench run. Return the errors
+		// to indicate that the specific warehouse count failed, but that the
+		// line search ought to continue.
 		m := newMonitor(ctx, c, roachNodes)
-		c.Stop(ctx, roachNodes)
+
+		// We overload the clusters in tpccbench, which can lead to transient infra
+		// failures. These are a) really annoying to debug and b) hide the actual
+		// passing warehouse count, making the line search sensitive to the choice
+		// of starting warehouses. Do a best-effort at waiting for the cloud VM(s)
+		// to recover without failing the line search.
+		var ok bool
+		for i := 0; i < 10; i++ {
+			if err := ctx.Err(); err != nil {
+				t.Fatal(err)
+			}
+			if err := c.StopE(ctx, roachNodes); err != nil {
+				t.l.Printf("unable to stop cluster; retrying to allow vm to recover: %s", err)
+				// We usually spend a long time blocking in StopE anyway, but just in case
+				// of a fast-failure mode, we still want to spend a little bit of time over
+				// the course of 10 retries to maximize the chances of things going back to
+				// working.
+				select {
+				case <-time.After(30 * time.Second):
+				case <-ctx.Done():
+				}
+				continue
+			}
+			ok = true
+			break
+		}
+		if !ok {
+			t.Fatalf("VM is hosed; giving up")
+		}
+
 		c.Start(ctx, t, append(b.startOpts(), roachNodes)...)
 		time.Sleep(restartWait)
 
@@ -858,7 +892,8 @@ func runTPCCBench(ctx context.Context, t *test, c *cluster, b tpccBenchSpec) {
 						b.partitions(), groupIdx)
 					activeWarehouses = warehouses / numLoadGroups
 				default:
-					panic("unexpected")
+					// Abort the whole test.
+					t.Fatalf("unimplemented LoadConfig %v", b.LoadConfig)
 				}
 				if b.Chaos {
 					// For chaos tests, we don't want to use the default method because it
@@ -874,33 +909,67 @@ func runTPCCBench(ctx context.Context, t *test, c *cluster, b tpccBenchSpec) {
 				err := c.RunE(ctx, group.loadNodes, cmd)
 				loadDone <- timeutil.Now()
 				if err != nil {
+					// NB: this will let the line search continue at a lower warehouse
+					// count.
 					return errors.Wrapf(err, "error running tpcc load generator")
 				}
 				roachtestHistogramsPath := filepath.Join(resultsDir, fmt.Sprintf("%d.%d-stats.json", warehouses, groupIdx))
 				if err := c.Get(
 					ctx, t.l, histogramsPath, roachtestHistogramsPath, group.loadNodes,
 				); err != nil {
-					t.Fatal(err)
+					// NB: this will let the line search continue. The reason we do this
+					// is because it's conceivable that we made it here, but a VM just
+					// froze up on us. The next search iteration will handle this state.
+					return err
 				}
 				snapshots, err := histogram.DecodeSnapshots(roachtestHistogramsPath)
 				if err != nil {
-					return errors.Wrapf(err, "failed to decode histogram snapshots")
+					// If we got this far, and can't decode data, it's not a case of
+					// overload but something that deserves failing the whole test.
+					t.Fatal(err)
 				}
 				result := tpcc.NewResultWithSnapshots(activeWarehouses, 0, snapshots)
 				resultChan <- result
 				return nil
 			})
 		}
-		if err = m.WaitE(); err != nil {
-			return false, err
-		}
+		failErr := m.WaitE()
 		close(resultChan)
-		var results []*tpcc.Result
-		for partial := range resultChan {
-			results = append(results, partial)
+
+		var res *tpcc.Result
+		if failErr != nil {
+			if t.Failed() {
+				// Someone called `t.Fatal` in a monitored goroutine,
+				// meaning that something went sideways in a way that
+				// indicates a general problem (i.e. not just that the
+				// current warehouse count overloaded the cluster.
+				// Abort the whole test.
+				return false, err
+			}
+			// A goroutine returned an error, but this means only
+			// that the given warehouse count did not run to completion,
+			// presumably because it overloaded the cluster. We thus
+			// "achieved" zero TpmC, but will continue the search.
+			//
+			// Note that it's also possible that we get here due to an
+			// actual bug in CRDB (for example a node crashing due to
+			// getting into an invalid state); we cannot distinguish
+			// those here and so tpccbench isn't a good test to rely
+			// on to catch crash-causing bugs.
+			res = &tpcc.Result{
+				ActiveWarehouses: warehouses,
+			}
+		} else {
+			// We managed to run TPCC, which means that we may or may
+			// not have "passed" TPCC.
+			var results []*tpcc.Result
+			for partial := range resultChan {
+				results = append(results, partial)
+			}
+			res = tpcc.MergeResults(results...)
+			failErr = res.FailureError()
 		}
-		res := tpcc.MergeResults(results...)
-		failErr := res.FailureError()
+
 		// Print the result.
 		if failErr == nil {
 			ttycolor.Stdout(ttycolor.Green)

pkg/workload/tpcc/result.go

@@ -55,7 +55,6 @@ var passing90ThPercentile = map[string]time.Duration{
 
 // Result represents the outcome of a TPCC run.
 type Result struct {
-
 	// ActiveWarehouses is the number of warehouses used in the TPC-C run.
 	ActiveWarehouses int
 
@@ -155,7 +154,11 @@ func NewResultWithSnapshots(
 // TpmC returns a tpmC value with a warehouse factor of 12.86.
 // TpmC will panic if r does not contain a "newOrder" histogram in Cumulative.
 func (r *Result) TpmC() float64 {
-	return float64(r.Cumulative["newOrder"].TotalCount()) / (r.Elapsed.Seconds() / 60)
+	no := r.Cumulative["newOrder"]
+	if no == nil {
+		return 0
+	}
+	return float64(no.TotalCount()) / (r.Elapsed.Seconds() / 60)
 }
 
 // Efficiency returns the efficiency of a TPC-C run.
@@ -181,7 +184,7 @@ func (r *Result) FailureError() error {
 	var err error
 	if eff := r.Efficiency(); eff < PassingEfficiency {
 		err = errors.CombineErrors(err,
-			errors.Errorf("efficiency value of %v is below ppassing threshold of %v",
+			errors.Errorf("efficiency value of %v is below passing threshold of %v",
 				eff, PassingEfficiency))
 	}
 	for query, max90th := range passing90ThPercentile {

pkg/workload/tpcc/result_test.go

@@ -0,0 +1,26 @@
+// Copyright 2021 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package tpcc
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestNewResult(t *testing.T) {
+	// Ensure you don't get panics when calling common methods
+	// on a trivial Result that doesn't have any data attached.
+	res := NewResult(1000, 0, 0, nil)
+	require.Error(t, res.FailureError())
+	require.Zero(t, res.Efficiency())
+	require.Zero(t, res.TpmC())
+}