Merge #114594

114594: roachtest: always send SIGKILL after SIGUSR1 r=RaduBerinde a=RaduBerinde

In coverage mode we send SIGUSR1 instead of SIGKILL so that we can
obtain the coverage data. However, upgrade tests use older Cockroach
versions which ignore SIGUSR1.

With this change we now always send SIGKILL if SIGUSR1 did not end the
process.

Epic: none
Release note: None

Fixes #114558
Fixes #114562

Co-authored-by: Radu Berinde <radu@cockroachlabs.com>

pkg/cmd/roachtest/cluster.go

@@ -2079,17 +2079,20 @@ func (c *clusterImpl) StopE(
 	if c.spec.NodeCount == 0 {
 		return nil // unit tests
 	}
-	if c.goCoverDir != "" {
-		// Never kill processes if we're trying to collect coverage; use SIGUSR1
-		// which dumps coverage data and exits.
-		if stopOpts.RoachprodOpts.Sig == 9 {
-			stopOpts.RoachprodOpts.Sig = 10 // SIGUSR1
-			stopOpts.RoachprodOpts.Wait = true
-			stopOpts.RoachprodOpts.MaxWait = 10
-		}
-	}
 	c.setStatusForClusterOpt("stopping", stopOpts.RoachtestOpts.Worker, nodes...)
 	defer c.clearStatusForClusterOpt(stopOpts.RoachtestOpts.Worker)
+
+	if c.goCoverDir != "" && stopOpts.RoachprodOpts.Sig == 9 /* SIGKILL */ {
+		// If we are trying to collect coverage, we don't want to kill processes;
+		// use SIGUSR1 which dumps coverage data and exits. Note that Cockroach
+		// v23.1 and earlier ignore SIGUSR1, so we still want to send SIGKILL.
+		l.Printf("coverage mode: first trying to stop using SIGUSR1")
+		opts := stopOpts.RoachprodOpts
+		opts.Sig = 10 // SIGUSR1
+		opts.Wait = true
+		opts.MaxWait = 10
+		_ = roachprod.Stop(ctx, l, c.MakeNodes(nodes...), opts)
+	}
 	return errors.Wrap(roachprod.Stop(ctx, l, c.MakeNodes(nodes...), stopOpts.RoachprodOpts), "cluster.StopE")
 }