Merge #144411 #147908

144411: failure-injection: add node kill failure r=herkolategan a=DarrylWong

This change adds a node kill failure mode which can either gracefully or ungracefully shutdown a cockroach process.

Informs: #138970
Release note: none

147908: workload/schemachange: allow dep error as potential error for ALTER PK r=spilchen a=spilchen

ALTER PRIMARY KEY has been observed to fail with the following error in the RSW (#147514 (comment)):
```
ERROR: cannot drop column "rowid" because trigger "trigger_w0_190" on table "table_w1_104" depends on it (SQLSTATE 2BP01)
```

This change adds that dependency error to the set of potential errors for ALTER PRIMARY KEY operations.

Informs #147514

Epic: none
Release note: none

Co-authored-by: DarrylWong <[email protected]>
Co-authored-by: Matt Spilchen <[email protected]>

Author: 3 people

pkg/cmd/roachtest/tests/failure_injection.go

@@ -9,6 +9,8 @@ import (
 	"context"
 	"fmt"
 	"math/rand"
+	"os"
+	"regexp"
 	"strings"
 	"time"
 
@@ -23,6 +25,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/roachprod/failureinjection/failures"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/install"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/logger"
+	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/util/randutil"
 	"github.com/cockroachdb/errors"
 )
@@ -52,6 +55,8 @@ type failureSmokeTest struct {
 	validateRecover func(ctx context.Context, l *logger.Logger, c cluster.Cluster, f *failures.Failer) error
 	// The workload to be run during the failureSmokeTest, if nil, defaultSmokeTestWorkload is used.
 	workload func(ctx context.Context, c cluster.Cluster, args ...string) error
+	// The duration to run the workload for before injecting the failure.
+	workloadRamp time.Duration
 }
 
 func (t *failureSmokeTest) run(
@@ -82,6 +87,15 @@ func (t *failureSmokeTest) run(
 		return err
 	}
 
+	if t.workloadRamp > 0 {
+		l.Printf("sleeping for %s before injecting failure", t.workloadRamp)
+		select {
+		case <-ctx.Done():
+			return ctx.Err()
+		case <-time.After(t.workloadRamp):
+		}
+	}
+
 	quietLogger, file, err = roachtestutil.LoggerForCmd(l, c.CRDBNodes(), t.testName, "inject")
 	if err != nil {
 		return err
@@ -596,6 +610,122 @@ var dmsetupDiskStallTest = func(c cluster.Cluster) failureSmokeTest {
 	}
 }
 
+var processKillTests = func(c cluster.Cluster) []failureSmokeTest {
+	rng, _ := randutil.NewPseudoRand()
+	var tests []failureSmokeTest
+	for _, gracefulShutdown := range []bool{true, false} {
+		groups, _ := c.CRDBNodes().SeededRandGroups(rng, 2 /* numGroups */)
+		killedNodeGroup := groups[0]
+		unaffectedNodeGroup := groups[1]
+
+		// These are the nodes that we will run validation on.
+		killedNode := killedNodeGroup.SeededRandNode(rng)
+		unaffectedNode := unaffectedNodeGroup.SeededRandNode(rng)
+
+		tests = append(tests, failureSmokeTest{
+			testName:    fmt.Sprintf("%s/GracefulShutdown=%t", failures.ProcessKillFailureName, gracefulShutdown),
+			failureName: failures.ProcessKillFailureName,
+			args: failures.ProcessKillArgs{
+				Nodes:            killedNodeGroup.InstallNodes(),
+				GracefulShutdown: gracefulShutdown,
+				GracePeriod:      time.Minute,
+			},
+			validateFailure: func(ctx context.Context, l *logger.Logger, c cluster.Cluster, f *failures.Failer) error {
+				// If we initiate a graceful shutdown, the cockroach process should
+				// intercept it and start draining the node.
+				if gracefulShutdown {
+					err := testutils.SucceedsSoonError(func() error {
+						if ctx.Err() != nil {
+							return ctx.Err()
+						}
+						res, err := c.RunWithDetailsSingleNode(ctx, l, option.WithNodes(unaffectedNode), fmt.Sprintf("./cockroach node status %d --decommission --certs-dir=%s | sed -n '2p' | awk '{print $NF}'", killedNode[0], install.CockroachNodeCertsDir))
+						if err != nil {
+							return err
+						}
+						isDraining := strings.TrimSpace(res.Stdout)
+						if isDraining != "true" {
+							return errors.Errorf("expected node %d to be draining", killedNode[0])
+						}
+						return nil
+					})
+					if err != nil {
+						return err
+					}
+				}
+
+				// Check that we aren't able to establish a SQL connection to the killed node.
+				// waitForFailureToPropagate already checks system death for us, which is a
+				// stronger assertion than checking SQL connections are unavailable. We
+				// are mostly doing this to satisfy the smoke test framework since this is
+				// a fairly simple failure mode with less to validate.
+				err := testutils.SucceedsSoonError(func() error {
+					if ctx.Err() != nil {
+						return ctx.Err()
+					}
+
+					killedDB, err := c.ConnE(ctx, l, killedNode[0])
+					if err == nil {
+						defer killedDB.Close()
+						if err := killedDB.Ping(); err == nil {
+							return errors.Errorf("expected node %d to be dead, but it is alive", killedNode)
+						} else {
+							l.Printf("failed to connect to node %d: %v", killedNode, err)
+						}
+					} else {
+						l.Printf("unable to establish SQL connection to node %d", killedNode)
+					}
+					return nil
+				})
+
+				return err
+			},
+			// Similar to validateFailure, there is not much to validate here that isn't
+			// covered by WaitForFailureToRecover, so just skip it.
+			validateRecover: func(ctx context.Context, l *logger.Logger, c cluster.Cluster, f *failures.Failer) error {
+				return nil
+			},
+			workload: func(ctx context.Context, c cluster.Cluster, args ...string) error {
+				return defaultFailureSmokeTestWorkload(ctx, c, "--tolerate-errors")
+			},
+			// Shutting down the server right after it's started can cause draining to be skipped.
+			workloadRamp: 30 * time.Second,
+		})
+	}
+
+	groups, _ := c.CRDBNodes().SeededRandGroups(rng, 2 /* numGroups */)
+	killedNodeGroup := groups[0]
+	// This is the node that we will run validation on.
+	killedNode := killedNodeGroup.SeededRandNode(rng)
+	noopSignal := 0
+
+	// Test that the GracePeriod logic will kick in if the SIGTERM hangs.
+	tests = append(tests, failureSmokeTest{
+		testName:    fmt.Sprintf("%s/hanging-drain", failures.ProcessKillFailureName),
+		failureName: failures.ProcessKillFailureName,
+		args: failures.ProcessKillArgs{
+			Nodes:       killedNode.InstallNodes(),
+			Signal:      &noopSignal,
+			GracePeriod: 30 * time.Second,
+		},
+		// There isn't anything to validate here because our failure is effectively
+		// a noop at first. Only after the GracePeriod will we see anything happen.
+		// We could block for 30 seconds and then check that the node is dead, but
+		// this is the same thing WaitForFailureToPropagate does for us.
+		validateFailure: func(ctx context.Context, l *logger.Logger, c cluster.Cluster, f *failures.Failer) error {
+			return nil
+		},
+		validateRecover: func(ctx context.Context, l *logger.Logger, c cluster.Cluster, f *failures.Failer) error {
+			return nil
+		},
+		workload: func(ctx context.Context, c cluster.Cluster, args ...string) error {
+			return defaultFailureSmokeTestWorkload(ctx, c, "--tolerate-errors")
+		},
+		// Shutting down the server right after it's started can cause draining to be skipped.
+		workloadRamp: 30 * time.Second,
+	})
+	return tests
+}
+
 func defaultFailureSmokeTestWorkload(ctx context.Context, c cluster.Cluster, args ...string) error {
 	workloadArgs := strings.Join(args, " ")
 	cmd := roachtestutil.NewCommand("./cockroach workload run kv %s", workloadArgs).
@@ -644,6 +774,7 @@ func runFailureSmokeTest(ctx context.Context, t test.Test, c cluster.Cluster, no
 		dmsetupDiskStallTest(c),
 	}
 	failureSmokeTests = append(failureSmokeTests, cgroupsDiskStallTests(c)...)
+	failureSmokeTests = append(failureSmokeTests, processKillTests(c)...)
 
 	// Randomize the order of the tests in case any of the failures have unexpected side
 	// effects that may mask failures, e.g. a cgroups disk stall isn't properly recovered
@@ -652,6 +783,22 @@ func runFailureSmokeTest(ctx context.Context, t test.Test, c cluster.Cluster, no
 		failureSmokeTests[i], failureSmokeTests[j] = failureSmokeTests[j], failureSmokeTests[i]
 	})
 
+	// For testing new failure modes, it may be useful to run only a subset of
+	// tests to increase iteration speed.
+	if regex := os.Getenv("FAILURE_INJECTION_SMOKE_TEST_FILTER"); regex != "" {
+		filter, err := regexp.Compile(regex)
+		if err != nil {
+			t.Fatal(err)
+		}
+		var filteredTests []failureSmokeTest
+		for _, test := range failureSmokeTests {
+			if filter.MatchString(test.testName) {
+				filteredTests = append(filteredTests, test)
+			}
+		}
+		failureSmokeTests = filteredTests
+	}
+
 	for _, test := range failureSmokeTests {
 		t.L().Printf("\n=====running %s test=====", test.testName)
 		if noopFailer {

pkg/roachprod/failureinjection/failures/BUILD.bazel

@@ -9,6 +9,7 @@ go_library(
         "latency.go",
         "network_partition.go",
         "noop.go",
+        "process_kill.go",
         "registry.go",
     ],
     importpath = "github.com/cockroachdb/cockroach/pkg/roachprod/failureinjection/failures",
@@ -17,10 +18,13 @@ go_library(
         "//pkg/roachprod",
         "//pkg/roachprod/install",
         "//pkg/roachprod/logger",
+        "//pkg/roachprod/roachprodutil",
         "//pkg/util/retry",
         "//pkg/util/syncutil",
+        "//pkg/util/sysutil",
         "//pkg/util/timeutil",
         "@com_github_cockroachdb_errors//:errors",
+        "@org_golang_x_sys//unix",
     ],
 )
 

pkg/roachprod/failureinjection/failures/failure.go

@@ -15,6 +15,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/roachprod"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/install"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/logger"
+	"github.com/cockroachdb/cockroach/pkg/roachprod/roachprodutil"
 	"github.com/cockroachdb/cockroach/pkg/util/retry"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/cockroachdb/errors"
@@ -217,6 +218,28 @@ func (f *GenericFailure) WaitForSQLUnavailable(
 	return errors.Wrapf(err, "connections to node %d never unavailable after %s", node, timeout)
 }
 
+// WaitForProcessDeath checks systemd until the cockroach process is no longer running
+// or the timeout is reached.
+func (f *GenericFailure) WaitForProcessDeath(
+	ctx context.Context, l *logger.Logger, node install.Nodes, timeout time.Duration,
+) error {
+	start := timeutil.Now()
+	err := retryForDuration(ctx, timeout, func() error {
+		res, err := f.RunWithDetails(ctx, l, node, "systemctl is-active cockroach-system.service")
+		if err != nil {
+			return err
+		}
+		status := strings.TrimSpace(res.Stdout)
+		if status != "active" {
+			l.Printf("n%d cockroach process exited after %s: %s", node, timeutil.Since(start), status)
+			return nil
+		}
+		return errors.Newf("systemd reported n%d cockroach process as %s", node, status)
+	})
+
+	return errors.Wrapf(err, "n%d process never exited after %s", node, timeout)
+}
+
 func (f *GenericFailure) StopCluster(
 	ctx context.Context, l *logger.Logger, stopOpts roachprod.StopOpts,
 ) error {
@@ -261,3 +284,15 @@ func forEachNode(nodes install.Nodes, fn func(install.Nodes) error) error {
 	}
 	return nil
 }
+
+func runAsync(ctx context.Context, l *logger.Logger, f func(context.Context) error) <-chan error {
+	errCh := make(chan error, 1)
+	go func() {
+		err := roachprodutil.PanicAsError(ctx, l, func(context.Context) error {
+			return f(ctx)
+		})
+		errCh <- err
+		close(errCh)
+	}()
+	return errCh
+}