roachtest: allow adding extra github parameters #134885

DarrylWong · 2024-11-11T22:00:18Z

When github issue posting, we denote various parameters describing the test, i.e. cloud, metamorphic encryption, etc. This is useful as it allows one to easily determine properties of a test without digging into the logs.

However, this feature only works if github posting is enabled. We've seen some cases where it is not enabled and we have trouble figuring out the aforementioned parameters. This change makes it so the parameters are logged to the artifacts directory if github posting is not enabled.

It also exposes the notion of extra parameters to the test interface. This allows for tests that have metamorphic properties to easily list them in the issue itself.

One example of this is in mixed version tests, where we randomize the deployment mode and the versions used. We often run into issues that pertain to only a specific deployment mode or version, and it can be cumbersome to dig through the artifacts for each individual failure.

Release note: none
Epic: none
Fixes: none

cockroach-teamcity · 2024-11-11T22:00:25Z

This change is

DarrylWong · 2024-11-11T22:01:47Z

This change originates from us trying to figure out if metamorphic encryption at rest was on in a qualification build, which does not create github issues. It was fairly trivial to just log the value, but I noticed several other metamorphic parameters that were also not logged. Instead of just logging them all, I figured it was cleaner to group them all in one location when I realized we kind of already do that for github posting.

I took this opportunity to add the custom extra params feature which I've been wanting for a while for debugging MVTs.

What the params.log file looks like if github posting is disabled and we run a MVT:

What the github issue looks like:

herkolategan · 2024-11-12T09:11:23Z

pkg/cmd/roachtest/github.go

+			// is worth the noise.
+			paramLogger, err := l.ChildLogger("params", logger.QuietStdout, logger.QuietStderr)
+			if err == nil {
+				paramLogger.Printf("Roachtest Parameters:\n%s", jsonBytes)


Small Nit: Could close the logger paramLogger.Close() right after this if it's not used further on.

herkolategan · 2024-11-12T09:28:47Z

pkg/cmd/roachtest/github.go

 	skipReason := g.shouldPost(t)
 	if skipReason != "" {
 		l.Printf("skipping GitHub issue posting (%s)", skipReason)
+
+		// If we're skipping issue posting, we still want to log the parameters


Question around this logic: I think I would find it confusing if sometimes I find this log and other times not (when no issue was created). Would it make sense to lift this out as part of a test's regular logging and always log it? It's relatively small so I don't see the harm in having 'duplicate' data, but I might be missing some other context here.

Sounds reasonable, I've lifted it to be part of the test runner and it now unconditionally logs the parameters on failure.

herkolategan · 2024-11-12T09:36:24Z

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

@@ -750,6 +750,11 @@ func (t *Test) Run() {

 	t.logger.Printf("mixed-version test:\n%s", plan.PrettyPrint())

+	// Mark the deployment mode and versions, so they show up in the github issue. This makes
+	// it easier to group failures together without having to dig into the test logs.
+	t.rt.AddParam("MVT_DEPLOYMENT_MODE", string(plan.deploymentMode))


Super Nit: These look a bit different MVT_DEPLOYMENT_MODE vs mvtDeploymentMode as the other parameters are camelCase. Although we have the ROACHTEST_ prefix, so I'm not sure if there's a certain style to the parameters here we need to follow.

What do you think about MVT_deploymentMode and then we don't add the prefix? I'd argue the ROACHTEST_ prefix is redundant if we're just going to append it to everything so at least this way we can differentiate between roachtest framework parameters and test specific parameters.

I'd argue the ROACHTEST_ prefix is redundant if we're just going to append it to everything so at least this way we can differentiate between roachtest framework parameters and test specific parameters.

Yeah, I was thinking about the same thing when I wrote my other comment. I do think maybe we should drop ROACHTEST_; I don't think anyone really cares about it.

Okay, no prefix + camel case sounds good to me 👍

herkolategan

Nice! A few small questions and some minor nits, but nothing necessarily worth holding up the PR over.

srosenberg · 2024-11-12T23:24:43Z

pkg/cmd/roachtest/testdata/github/extra_parameters

+```
+
+Parameters:
+ - <code>MVT_deploymentMode=separate-process</code>


It breaks the symmetry... Since we have this historical prefix, ROACHTEST_, and since mixed version tests are technically embedded in the roachtest framework, ROACHTEST_MVT_deploymentMode seems reasonable.

srosenberg · 2024-11-12T23:29:50Z

pkg/cmd/roachtest/test_runner.go

+	}
+}
+
+func getTestParameters(t *testImpl, c *clusterImpl, createOpts *vm.CreateOpts) map[string]string {


Nice, I like its new place!

When github issue posting, we denote various parameters describing the test, i.e. cloud, metamorphic encryption, etc. This is useful as it allows one to easily determine properties of a test without digging into the logs. However, this feature only works if github posting is enabled. We've seen some cases where it is not enabled and we have trouble figuring out the aforementioned parameters. This change makes it so the parameters are logged to the artifacts directory in case github posting is not enabled or fails. It also exposes the notion of extra parameters to the test interface. This allows for tests that have metamorphic properties to easily list them in the issue itself. One example of this is in mixed version tests, where we randomize the deployment mode and the versions used. We often run into issues that pertain to only a specific deployment mode or version, and it can be cumbersome to dig through the artifacts for each individual failure. Release note: none Epic: none Fixes: none

DarrylWong · 2024-11-13T20:00:35Z

TFTRs!

bors r=herkolategan, srosenberg

craig · 2024-11-13T20:55:35Z

Build succeeded:

blathers-crl · 2024-11-13T20:55:46Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating merge commit from b29d6f6 to blathers/backport-release-24.1-134885: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 24.1.x failed. See errors above.

error creating merge commit from b29d6f6 to blathers/backport-release-24.2-134885: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 24.2.x failed. See errors above.

error creating merge commit from b29d6f6 to blathers/backport-release-24.3-134885: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 24.3.x failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

DarrylWong marked this pull request as ready for review November 11, 2024 22:08

DarrylWong requested a review from a team as a code owner November 11, 2024 22:08

DarrylWong requested review from herkolategan and srosenberg and removed request for a team November 11, 2024 22:08

DarrylWong added backport-24.1.x Flags PRs that need to be backported to 24.1. backport-24.2.x Flags PRs that need to be backported to 24.2 backport-24.3.x Flags PRs that need to be backported to 24.3 labels Nov 11, 2024

herkolategan reviewed Nov 12, 2024

View reviewed changes

herkolategan approved these changes Nov 12, 2024

View reviewed changes

DarrylWong force-pushed the allow-extra-labels branch 5 times, most recently from 6106d71 to 733852b Compare November 12, 2024 21:46

srosenberg reviewed Nov 12, 2024

View reviewed changes

srosenberg approved these changes Nov 12, 2024

View reviewed changes

DarrylWong force-pushed the allow-extra-labels branch from 733852b to f3f6d47 Compare November 13, 2024 16:23

DarrylWong force-pushed the allow-extra-labels branch from f3f6d47 to b29d6f6 Compare November 13, 2024 20:00

craig bot merged commit c18ccff into cockroachdb:master Nov 13, 2024
22 of 23 checks passed

DarrylWong mentioned this pull request Nov 13, 2024

release-24.3: roachtest: allow adding extra github parameters #135117

Merged

This was referenced Nov 13, 2024

release-24.2: roachtest: allow adding extra github parameters #135119

Merged

release-24.1: roachtest: allow adding extra github parameters #135187

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

roachtest: allow adding extra github parameters #134885

roachtest: allow adding extra github parameters #134885

DarrylWong commented Nov 11, 2024

cockroach-teamcity commented Nov 11, 2024

DarrylWong commented Nov 11, 2024 •

edited

Loading

herkolategan Nov 12, 2024 •

edited

Loading

herkolategan Nov 12, 2024

DarrylWong Nov 12, 2024

herkolategan Nov 12, 2024 •

edited

Loading

DarrylWong Nov 12, 2024

srosenberg Nov 12, 2024

DarrylWong Nov 13, 2024

herkolategan left a comment

srosenberg Nov 12, 2024

srosenberg Nov 12, 2024

DarrylWong commented Nov 13, 2024

craig bot commented Nov 13, 2024

blathers-crl bot commented Nov 13, 2024

roachtest: allow adding extra github parameters #134885

roachtest: allow adding extra github parameters #134885

Conversation

DarrylWong commented Nov 11, 2024

cockroach-teamcity commented Nov 11, 2024

DarrylWong commented Nov 11, 2024 • edited Loading

herkolategan Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

herkolategan Nov 12, 2024

Choose a reason for hiding this comment

DarrylWong Nov 12, 2024

Choose a reason for hiding this comment

herkolategan Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

DarrylWong Nov 12, 2024

Choose a reason for hiding this comment

srosenberg Nov 12, 2024

Choose a reason for hiding this comment

DarrylWong Nov 13, 2024

Choose a reason for hiding this comment

herkolategan left a comment

Choose a reason for hiding this comment

srosenberg Nov 12, 2024

Choose a reason for hiding this comment

srosenberg Nov 12, 2024

Choose a reason for hiding this comment

DarrylWong commented Nov 13, 2024

craig bot commented Nov 13, 2024

blathers-crl bot commented Nov 13, 2024

DarrylWong commented Nov 11, 2024 •

edited

Loading

herkolategan Nov 12, 2024 •

edited

Loading

herkolategan Nov 12, 2024 •

edited

Loading