roachtestutil: disk stall Unstall returns error

Depending on the context of the test, Unstall erroring out
should result in a test failure. Some tests purposely induce
a node panic, in which case an error is expected and can be
safely ignored. However, some tests want to test that WAL
Failover works and any node death is unexpected.

This change modifies the disk stall API to return an error
for Unstall, and lets the test writer decide how to handle it.

Author: DarrylWong

pkg/cmd/roachtest/operations/disk_stall.go

@@ -24,7 +24,9 @@ type cleanupDiskStall struct {
 }
 
 func (cl *cleanupDiskStall) Cleanup(ctx context.Context, o operation.Operation, c cluster.Cluster) {
-	cl.staller.Unstall(ctx, cl.nodes)
+	if err := cl.staller.Unstall(ctx, cl.nodes); err != nil {
+		o.Fatalf("failed to unstall disk: %v", err)
+	}
 	o.Status("unstalled nodes; waiting 10 seconds before restarting")
 	time.Sleep(10 * time.Second)
 	// We might need to restart the node if it isn't live.

pkg/cmd/roachtest/roachtestutil/disk_stall.go

@@ -14,6 +14,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/spec"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/failureinjection/failures"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/logger"
+	"github.com/cockroachdb/errors"
 )
 
 // TODO(darryl): Once the failure injection library is a first class citizen of roachtest,
@@ -25,7 +26,7 @@ type DiskStaller interface {
 	Stall(ctx context.Context, nodes option.NodeListOption)
 	StallCycle(ctx context.Context, nodes option.NodeListOption, stallDuration, unstallDuration time.Duration)
 	Slow(ctx context.Context, nodes option.NodeListOption, bytesPerSecond int)
-	Unstall(ctx context.Context, nodes option.NodeListOption)
+	Unstall(ctx context.Context, nodes option.NodeListOption) error
 	DataDir() string
 	LogDir() string
 }
@@ -34,16 +35,17 @@ type NoopDiskStaller struct{}
 
 var _ DiskStaller = NoopDiskStaller{}
 
-func (n NoopDiskStaller) Cleanup(ctx context.Context)                                               {}
-func (n NoopDiskStaller) DataDir() string                                                           { return "{store-dir}" }
-func (n NoopDiskStaller) LogDir() string                                                            { return "logs" }
-func (n NoopDiskStaller) Setup(ctx context.Context)                                                 {}
-func (n NoopDiskStaller) Slow(_ context.Context, _ option.NodeListOption, _ int)                    {}
-func (n NoopDiskStaller) Stall(_ context.Context, _ option.NodeListOption)                          {}
+func (n NoopDiskStaller) Cleanup(ctx context.Context)                            {}
+func (n NoopDiskStaller) DataDir() string                                        { return "{store-dir}" }
+func (n NoopDiskStaller) LogDir() string                                         { return "logs" }
+func (n NoopDiskStaller) Setup(ctx context.Context)                              {}
+func (n NoopDiskStaller) Slow(_ context.Context, _ option.NodeListOption, _ int) {}
+func (n NoopDiskStaller) Stall(_ context.Context, _ option.NodeListOption)       {}
 func (n NoopDiskStaller) StallCycle(
 	_ context.Context, _ option.NodeListOption, _, _ time.Duration,
-) {}
-func (n NoopDiskStaller) Unstall(_ context.Context, _ option.NodeListOption)                        {}
+) {
+}
+func (n NoopDiskStaller) Unstall(_ context.Context, _ option.NodeListOption) error { return nil }
 
 type Fataler interface {
 	Fatal(args ...interface{})
@@ -140,11 +142,12 @@ func (s *cgroupDiskStaller) Slow(
 	}
 }
 
-func (s *cgroupDiskStaller) Unstall(ctx context.Context, nodes option.NodeListOption) {
+func (s *cgroupDiskStaller) Unstall(ctx context.Context, nodes option.NodeListOption) error {
 	l := newDiskStallLogger(s.f.L(), nodes, "Unstall")
-	if err := s.Failer.Recover(ctx, l); err != nil {
-		s.f.Fatalf("failed to unstall disk: %s", err)
-	}
+	// cgroup may fail when unstalling the disk, usually because the node already
+	// fataled and the cgroup is no longer available. Return the error and let
+	// the caller decide if a node fatal is expected or not.
+	return errors.Wrap(s.Failer.Recover(ctx, l), "failed to unstall disk")
 }
 
 type dmsetupDiskStaller struct {
@@ -211,11 +214,13 @@ func (s *dmsetupDiskStaller) Slow(
 	s.f.Fatal("Slow is not supported for dmsetupDiskStaller")
 }
 
-func (s *dmsetupDiskStaller) Unstall(ctx context.Context, nodes option.NodeListOption) {
+func (s *dmsetupDiskStaller) Unstall(ctx context.Context, nodes option.NodeListOption) error {
 	l := newDiskStallLogger(s.f.L(), nodes, "Unstall")
+	// Any unstall error for dmsetup is unexpected and should fail the test.
 	if err := s.Failer.Recover(ctx, l); err != nil {
 		s.f.Fatalf("failed to unstall disk: %s", err)
 	}
+	return nil
 }
 
 func (s *dmsetupDiskStaller) DataDir() string { return "{store-dir}" }

pkg/cmd/roachtest/tests/disk_stall.go

@@ -129,7 +129,9 @@ func runDiskStalledWALFailover(ctx context.Context, t test.Test, c cluster.Clust
 					ctx, cancel := context.WithTimeout(context.Background(), time.Minute)
 					defer cancel()
 					t.Status("Unstalling disk on n1")
-					s.Unstall(ctx, c.Node(1))
+					if err = s.Unstall(ctx, c.Node(1)); err != nil {
+						t.Fatal(err)
+					}
 					t.Status("Unstalled disk on n1")
 				}()
 
@@ -378,7 +380,11 @@ func runDiskStalledDetection(
 	}
 
 	// Unstall the stalled node. It should be able to be reaped.
-	s.Unstall(ctx, c.Node(1))
+	// Note we only log errors since cgroup unstall is expected to fail due to
+	// nodes panicking from a detected disk stall.
+	if err = s.Unstall(ctx, c.Node(1)); err != nil {
+		t.L().Printf("failed to unstall disk: %v", err)
+	}
 	time.Sleep(1 * time.Second)
 	exit, ok = getProcessExitMonotonic(ctx, t, c, 1)
 	if doStall {
@@ -622,7 +628,9 @@ func runDiskStalledWALFailoverWithProgress(ctx context.Context, t test.Test, c c
 				case <-ctx.Done():
 					t.Fatalf("context done while stall induced: %s", ctx.Err())
 				case <-time.After(operationDur):
-					s.Unstall(ctx, c.Node(1))
+					if err = s.Unstall(ctx, c.Node(1)); err != nil {
+						t.Fatal(err)
+					}
 					t.Status("disk stalls stopped")
 				}
 				return nil

pkg/cmd/roachtest/tests/failover.go

@@ -1605,7 +1605,6 @@ func (f *diskStallFailer) Setup(ctx context.Context) {
 }
 
 func (f *diskStallFailer) Cleanup(ctx context.Context) {
-	f.staller.Unstall(ctx, f.c.All())
 	// We have to stop the cluster before cleaning up the staller.
 	f.m.ExpectDeaths(int32(f.c.Spec().NodeCount))
 	f.c.Stop(ctx, f.t.L(), option.DefaultStopOpts(), f.c.All())
@@ -1628,7 +1627,9 @@ func (f *diskStallFailer) Fail(ctx context.Context, nodeID int) {
 }
 
 func (f *diskStallFailer) Recover(ctx context.Context, nodeID int) {
-	f.staller.Unstall(ctx, f.c.Node(nodeID))
+	if err := f.staller.Unstall(ctx, f.c.Node(nodeID)); err != nil {
+		f.t.Fatalf("failed to unstall disk %v", err)
+	}
 	// Pebble's disk stall detector should have terminated the node, but in case
 	// it didn't, we explicitly stop it first.
 	f.c.Stop(ctx, f.t.L(), option.DefaultStopOpts(), f.c.Node(nodeID))

pkg/cmd/roachtest/tests/perturbation/slow_disk.go

@@ -80,7 +80,9 @@ func (s *slowDisk) startPerturbation(ctx context.Context, t test.Test, v variati
 
 // endPerturbation implements perturbation.
 func (s *slowDisk) endPerturbation(ctx context.Context, t test.Test, v variations) time.Duration {
-	s.staller.Unstall(ctx, v.targetNodes())
+	if err := s.staller.Unstall(ctx, v.targetNodes()); err != nil {
+		t.Fatal("failed to unstall disk:", err)
+	}
 	waitDuration(ctx, v.validationDuration)
 	return v.validationDuration
 }