Merge #130106

130106: roachtest/mixedversion: improvements for multiple clusters r=srosenberg a=renatolabs

Specifically:

* we include the `tag`, if any, in the error message returned by a
test, making it easier to understand which upgrade failed.
* improve the logging setup: if there are two mixedversion test
instances, say `A` and `B`, the log files `A/mixed-version-test.log`
will contain all the output for A's upgrade steps (similarly for B).

This PR also removes the `libGEOS` requirement from `acceptance/version-upgrade`.

Co-authored-by: Renato Costa <[email protected]>

pkg/cmd/roachtest/roachtestutil/mixedversion/mixedversion.go

@@ -1052,12 +1052,22 @@ func newSingleStep(context *Context, impl singleStepProtocol, rng *rand.Rand) *s
 }
 
 // prefixedLogger returns a logger instance off of the given `l`
-// parameter, and adds a prefix to everything logged by the retured
-// logger.
+// parameter. The path and prefix are the same.
 func prefixedLogger(l *logger.Logger, prefix string) (*logger.Logger, error) {
-	fileName := strings.ReplaceAll(prefix, " ", "-")
-	formattedPrefix := fmt.Sprintf("[%s] ", fileName)
-	return l.ChildLogger(fileName, logger.LogPrefix(formattedPrefix))
+	filename := sanitizePath(prefix)
+	return prefixedLoggerWithFilename(l, filename, filename)
+}
+
+// prefixedLoggerWithFilename returns a logger instance with the given
+// prefix. The logger will write to a file on the given `path`,
+// relative to the logger `l`'s location.
+func prefixedLoggerWithFilename(l *logger.Logger, prefix, path string) (*logger.Logger, error) {
+	formattedPrefix := fmt.Sprintf("[%s] ", prefix)
+	return l.ChildLogger(path, logger.LogPrefix(formattedPrefix))
+}
+
+func sanitizePath(s string) string {
+	return strings.ReplaceAll(s, " ", "-")
 }
 
 func (h hooks) Filter(testContext Context) hooks {

pkg/cmd/roachtest/roachtestutil/mixedversion/runner.go

@@ -195,11 +195,19 @@ func (tr *testRunner) run() (retErr error) {
 	stepsErr := make(chan error)
 	defer func() { tr.teardown(stepsErr, retErr != nil) }()
 	defer func() {
-		// If the test failed an we haven't run any user hooks up to this
-		// point, redirect the failure to Test Eng, as this indicates a
-		// setup problem that should be investigated separately.
-		if retErr != nil && !tr.ranUserHooks.Load() {
-			retErr = registry.ErrorWithOwner(registry.OwnerTestEng, retErr)
+		if retErr != nil {
+			// If the test failed an we haven't run any user hooks up to this
+			// point, redirect the failure to Test Eng, as this indicates a
+			// setup problem that should be investigated separately.
+			if !tr.ranUserHooks.Load() {
+				retErr = registry.ErrorWithOwner(registry.OwnerTestEng, retErr)
+			}
+
+			// If this test run had a tag assigned, wrap the error with that
+			// tag to make it more immediately clear which run failed.
+			if tr.tag != "" {
+				retErr = errors.Wrapf(retErr, "%s", tr.tag)
+			}
 		}
 	}()
 
@@ -525,9 +533,7 @@ func (tr *testRunner) loggerFor(step *singleStep) (*logger.Logger, error) {
 	name = fmt.Sprintf("%d_%s", step.ID, name)
 	prefix := filepath.Join(tr.tag, logPrefix, name)
 
-	// Use the root logger here as the `prefix` passed will already
-	// include the full path from the root, including the tag.
-	return prefixedLogger(tr.logger.RootLogger(), prefix)
+	return prefixedLoggerWithFilename(tr.logger, prefix, filepath.Join(logPrefix, name))
 }
 
 // refreshBinaryVersions updates the `binaryVersions` field for every

pkg/cmd/roachtest/tests/acceptance.go

@@ -36,7 +36,6 @@ func registerAcceptance(r registry.Registry) {
 		defaultLeases      bool
 		requiresLicense    bool
 		randomized         bool
-		nativeLibs         []string
 		workloadNode       bool
 		incompatibleClouds registry.CloudSet
 	}{
@@ -82,7 +81,6 @@ func registerAcceptance(r registry.Registry) {
 				timeout:       2 * time.Hour, // actually lower in local runs; see `runVersionUpgrade`
 				defaultLeases: true,
 				randomized:    true,
-				nativeLibs:    registry.LibGEOS,
 			},
 		},
 		registry.OwnerDisasterRecovery: {
@@ -164,9 +162,6 @@ func registerAcceptance(r registry.Registry) {
 			if !tc.defaultLeases {
 				testSpec.Leases = registry.MetamorphicLeases
 			}
-			if len(tc.nativeLibs) > 0 {
-				testSpec.NativeLibs = tc.nativeLibs
-			}
 			testSpec.Run = func(ctx context.Context, t test.Test, c cluster.Cluster) {
 				tc.fn(ctx, t, c)
 			}