roachtest: port tpcc/mixed-headroom to the new framework by DarrylWong · Pull Request #113706 · cockroachdb/cockroach

DarrylWong · 2023-11-02T20:06:22Z

Note that the new framework randomizes how many version upgrades occur. This removes the need for both a single upgrade and multiple upgrade test and the two were merged.

Release note: None
Fixes: #110537

cockroach-teamcity · 2023-11-02T20:06:31Z

This change is

DarrylWong · 2023-11-06T19:54:55Z

RFAL

Passed on gceworker + TC job

RaduBerinde

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DarrylWong, @renatolabs, and @srosenberg)

pkg/cmd/roachtest/tests/tpcc.go line 359 at r1 (raw file):

// `bank` dataset, and runs one or multiple database upgrades while a
// TPCC workload is running. The number of database upgrades is
// controlled by the `versionsToUpgrade` parameter.

This needs updating (no more versionsToUpgrade). It would be good to include a high level idea of how it works (I assume mixedversion chooses a random older but supported version to start with?)

pkg/cmd/roachtest/tests/tpcc.go line 430 at r1 (raw file):

	}

	uploadVersion(ctx, t, c, workloadNode, clusterupgrade.CurrentVersion())

At which point in this test sequence do we start the upgrade process? Is it at some point during the runTPCCWorkload because that is the InMixedVersion step?

DarrylWong

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @RaduBerinde, @renatolabs, and @srosenberg)

pkg/cmd/roachtest/tests/tpcc.go line 359 at r1 (raw file):

Previously, RaduBerinde wrote…

This needs updating (no more versionsToUpgrade). It would be good to include a high level idea of how it works (I assume mixedversion chooses a random older but supported version to start with?)

Good catch. Done.

pkg/cmd/roachtest/tests/tpcc.go line 430 at r1 (raw file):

Previously, RaduBerinde wrote…

At which point in this test sequence do we start the upgrade process? Is it at some point during the runTPCCWorkload because that is the InMixedVersion step?

The upgrade process is started after OnStartup but before InMixedVersion, where InMixedVersion hooks will be called once per upgrade step. It's randomized when, but the general idea is if we are going from v23.1 to v23.2, then at any point before it upgrades an individual node, it could call runTPCCWorkload, doing so once per cluster upgrade/downgrade step.

Taking a peek at the planning output in test.log probably gives a better idea of a potential plan than what I can explain. (I had to save it locally for the formatting to be nice btw)

renatolabs · 2023-11-07T20:28:12Z

pkg/cmd/roachtest/tests/tpcc.go

+	runTPCCWorkload := func(ctx context.Context, l *logger.Logger, rng *rand.Rand, h *mixedversion.Helper) error {
+		cmd := roachtestutil.NewCommand("./cockroach workload run tpcc").
+			Arg("{pgurl%s}", crdbNodes).
+			Flag("duration", rampDuration(c.IsLocal())).


Probably better to have the duration be separate from rampDuration.

Also, I think the following change would be welcome in order to test the migrations:

workloadDur := 10 * time.Minute rampDur := rampDuration(c.IsLocal()) if h.Context().Finalizing && !c.IsLocal() { rampDur = 1 * time.Minute workloadDur = 100 * time.Minute }

We want the workload to ramp up faster when migrations are running to expose them to more concurrent load. Also letting the workload run for longer in that case is good, and mimics what the previous version of this test was doing.

Does that make sense?

Done.

I'll give 100 minutes a test run but are we concerned at all that this will take too long? I'm not sure what the chances are but could it possibly call runTPCCWorkload every time while migrations are happening and all of a sudden this test takes 10+ hours to finish?

renatolabs

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DarrylWong, @RaduBerinde, and @srosenberg)

pkg/cmd/roachtest/tests/tpcc.go line 430 at r1 (raw file):

I had to save it locally for the formatting to be nice btw

If anyone uses firefox, the Repair Text Encoding option should fix it:

Not sure if there is a way to tell TC an artifact is UTF-8 encoded so that this wouldn't be necessary.

RaduBerinde

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DarrylWong, @renatolabs, and @srosenberg)

pkg/cmd/roachtest/tests/tpcc.go line 359 at r1 (raw file):

Previously, DarrylWong wrote…

Good catch. Done.

I don't think you pushed the new version.

pkg/cmd/roachtest/tests/tpcc.go line 430 at r1 (raw file):

Previously, renatolabs (Renato Costa) wrote…

I had to save it locally for the formatting to be nice btw

If anyone uses firefox, the Repair Text Encoding option should fix it:

Not sure if there is a way to tell TC an artifact is UTF-8 encoded so that this wouldn't be necessary.

Very cool, thanks!

DarrylWong

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @RaduBerinde, @renatolabs, and @srosenberg)

pkg/cmd/roachtest/tests/tpcc.go line 359 at r1 (raw file):

Previously, RaduBerinde wrote…

I don't think you pushed the new version.

Ah my bad, saw Renato's comment last second and got distracted, it's pushed now.

RaduBerinde

Reviewed 1 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @DarrylWong, @renatolabs, and @srosenberg)

pkg/cmd/roachtest/tests/tpcc.go line 359 at r1 (raw file):

Previously, DarrylWong wrote…

Ah my bad, saw Renato's comment last second and got distracted, it's pushed now.

Thanks, looks great!

renatolabs

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @DarrylWong and @srosenberg)

pkg/cmd/roachtest/tests/tpcc.go line 412 at r1 (raw file):

every time while migrations are happening and all of a sudden this test takes 10+ hours to finish

We perform up to 3 upgrades by default, so worst case scenario would be 300 minutes or 5h, which is long but not the longest roachtest to exist. That said, I didn't realize the existing roachtest only uses 100 minutes if we are upgrading to current, so we can do that here too, i.e., only bump the duration if we are upgrading to the current version:

workloadDur := 10 * time.Minute
rampDur := rampDuration(c.IsLocal())
if h.Context().Finalizing && !c.IsLocal() {
    rampDur = 1 * time.Minute
    if h.Context().ToVersion.IsCurrent() {
        workloadDur = 100 * time.Minute
    }
}

Note that the new framework randomizes how many version upgrades occur. This removes the need for both a single upgrade and multiple upgrade test and the two were merged. Release note: None Fixes: cockroachdb#110537

DarrylWong

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @RaduBerinde, @renatolabs, and @srosenberg)

pkg/cmd/roachtest/tests/tpcc.go line 412 at r1 (raw file):

Previously, renatolabs (Renato Costa) wrote…

every time while migrations are happening and all of a sudden this test takes 10+ hours to finish

We perform up to 3 upgrades by default, so worst case scenario would be 300 minutes or 5h, which is long but not the longest roachtest to exist. That said, I didn't realize the existing roachtest only uses 100 minutes if we are upgrading to current, so we can do that here too, i.e., only bump the duration if we are upgrading to the current version:
workloadDur := 10 * time.Minute
rampDur := rampDuration(c.IsLocal())
if h.Context().Finalizing && !c.IsLocal() {
    rampDur = 1 * time.Minute
    if h.Context().ToVersion.IsCurrent() {
        workloadDur = 100 * time.Minute
    }
}

Done.

Test run here

renatolabs

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @DarrylWong, @RaduBerinde, and @srosenberg)

DarrylWong · 2023-11-10T17:57:13Z

TFTRs!

bors r=renatolabs, RaduBerinde

craig · 2023-11-10T18:55:22Z

Build succeeded:

Bazel Essential CI (Cockroach)

renatolabs · 2023-11-17T15:25:39Z

blathers backport 23.1

blathers-crl · 2023-11-17T15:25:43Z

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 8cdcf77 to blathers/backport-release-23.1-113706: 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 23.1 failed. See errors above.

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

DarrylWong added the backport-23.2.x PAST MAINTENANCE SUPPORT: 23.2 patch releases via ER request only label Nov 2, 2023

DarrylWong force-pushed the port-tpcc branch 3 times, most recently from 48e0bc2 to 292a48c Compare November 6, 2023 19:53

DarrylWong force-pushed the port-tpcc branch from 292a48c to b8eeda0 Compare November 6, 2023 19:57

DarrylWong marked this pull request as ready for review November 6, 2023 19:57

DarrylWong requested a review from a team as a code owner November 6, 2023 19:57

DarrylWong requested review from renatolabs and srosenberg and removed request for a team November 6, 2023 19:57

RaduBerinde approved these changes Nov 7, 2023

View reviewed changes

DarrylWong commented Nov 7, 2023

View reviewed changes

renatolabs reviewed Nov 7, 2023

View reviewed changes

RaduBerinde reviewed Nov 7, 2023

View reviewed changes

DarrylWong force-pushed the port-tpcc branch from b8eeda0 to f40aaf9 Compare November 7, 2023 20:59

DarrylWong commented Nov 7, 2023

View reviewed changes

RaduBerinde approved these changes Nov 7, 2023

View reviewed changes

renatolabs reviewed Nov 7, 2023

View reviewed changes

roachtest: port tpcc/mixed-headroom to the new framework

8cdcf77

Note that the new framework randomizes how many version upgrades occur. This removes the need for both a single upgrade and multiple upgrade test and the two were merged. Release note: None Fixes: cockroachdb#110537

DarrylWong force-pushed the port-tpcc branch from f40aaf9 to 8cdcf77 Compare November 8, 2023 15:39

DarrylWong commented Nov 8, 2023

View reviewed changes

renatolabs approved these changes Nov 10, 2023

View reviewed changes

craig bot merged commit 194151c into cockroachdb:master Nov 10, 2023

blathers-crl bot mentioned this pull request Nov 10, 2023

release-23.2: roachtest: port tpcc/mixed-headroom to the new framework #114249

Merged

This was referenced Nov 17, 2023

release-23.1: roachtest: port tpcc/mixed-headroom to the new framework + refactor test context #114647

Merged

roachtest: port remaining mixed-version roachtests to the new framework #110528

Closed

Conversation

DarrylWong commented Nov 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Nov 2, 2023

Uh oh!

DarrylWong commented Nov 6, 2023

Uh oh!

RaduBerinde left a comment

Choose a reason for hiding this comment

Uh oh!

DarrylWong left a comment

Choose a reason for hiding this comment

Uh oh!

renatolabs Nov 7, 2023

Choose a reason for hiding this comment

Uh oh!

DarrylWong Nov 7, 2023

Choose a reason for hiding this comment

Uh oh!

renatolabs left a comment

Choose a reason for hiding this comment

Uh oh!

RaduBerinde left a comment

Choose a reason for hiding this comment

Uh oh!

DarrylWong left a comment

Choose a reason for hiding this comment

Uh oh!

RaduBerinde left a comment

Choose a reason for hiding this comment

Uh oh!

renatolabs left a comment

Choose a reason for hiding this comment

Uh oh!

DarrylWong left a comment

Choose a reason for hiding this comment

Uh oh!

renatolabs left a comment

Choose a reason for hiding this comment

Uh oh!

DarrylWong commented Nov 10, 2023

Uh oh!

craig bot commented Nov 10, 2023

Uh oh!

renatolabs commented Nov 17, 2023

Uh oh!

blathers-crl bot commented Nov 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DarrylWong commented Nov 2, 2023 •

edited

Loading