Optimize CockroachDB setup for tests (96% reduction) by smklein · Pull Request #493 · oxidecomputer/omicron

smklein · 2021-12-09T02:04:07Z

Optimizes setup time for CRDB-based tests by seeding + copying a seeded database, rather than populating the database anew during each test. This significantly cuts down on the setup time for any test which wants to use even a simple, single-node Cockroach instance.

Comparing the benchmark originally added in #492:

Before:

Test Setup/do_full_setup
                        time:   [7.1417 s 7.2563 s 7.3714 s]
Test Setup/do_crdb_setup
                        time:   [6.4927 s 6.8566 s 7.2067 s]
Test Setup/do_clickhouse_setup
                        time:   [521.88 ms 527.69 ms 533.80 ms]

After:

Test Setup/do_full_setup
                        time:   [1.1145 s 1.1477 s 1.1832 s]
                        change: [-84.712% -84.183% -83.623%] (p = 0.00 < 0.05)
                        Performance has improved. 
Test Setup/do_crdb_setup
                        time:   [229.49 ms 242.09 ms 253.62 ms]
                        change: [-96.706% -96.469% -96.203%] (p = 0.00 < 0.05)
                        Performance has improved.
Test Setup/do_clickhouse_setup
                        time:   [521.93 ms 526.22 ms 530.98 ms]
                        change: [-1.6256% -0.2787% +1.1729%] (p = 0.71 > 0.05)
                        No change in performance detected.

…base

david-crespo · 2021-12-09T02:24:52Z

Life-changing! Tried it myself and got this on build, which I don't get on main.

error[E0658]: sym operands for inline assembly are unstable
   --> /Users/david/.cargo/git/checkouts/dropshot-a4a923d29dccc492/ff33033/dropshot/src/lib.rs:574:1
    |
574 |   #[usdt::provider(provider = "dropshot")]
    |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
   ::: /Users/david/.cargo/git/checkouts/dropshot-a4a923d29dccc492/ff33033/dropshot/src/server.rs:387:13

david-crespo · 2021-12-09T02:29:30Z

Ah good, at least the macOS CI build can reproduce that.

david-crespo · 2021-12-09T02:39:03Z

Confirmed that fixed it.

# before
cargo test --package omicron-nexus --test test_console_api -- test_sessions
9.48s user 2.08s system 31% cpu 36.163 total

# after
cargo test --package omicron-nexus --test test_console_api -- test_sessions
1.71s user 0.58s system 53% cpu 4.303 total

This is the best day of my life.

smklein · 2021-12-09T05:06:37Z

Cargo.toml

    "common",
    "nexus",
    "nexus/src/db/db-macros",
-    "nexus/test-utils",


Note: this has been removed from the default members, but we're definitely still using / testing it.

This just means that cargo build doesn't, by default, build a "test-only" helper crate - doing so would also require "cockroachdb" to be on the PATH earlier for some of our checks.

Note: This still gets built with cargo build --all-targets and cargo test, so no worries on coverage.

smklein · 2021-12-09T05:07:42Z

.github/buildomat/jobs/build-and-test.sh

@@ -14,6 +14,18 @@ set -o xtrace
 cargo --version


The change here - and in the ".github/workflows/rust.yml" file - just make sure cockroach is downloaded and on the PATH before we cargo build --all-targets, since we're now using it in a build script.

Could you update the README to reflect this? Basically this part:

CockroachDB v21.1.10.
The test suite expects to be able to start a single-node CockroachDB cluster using the cockroach executable on your PATH.

I think that should now say "the build and test suite expect..."

davepacheco

Very nice improvement!

davepacheco · 2021-12-09T17:04:50Z

test-utils/src/dev/mod.rs

+/// By creating a "seed" version of the database, we can cut down
+/// on the time spent performing this operation. Instead, we opt
+/// to copy the database from this seed location.
+pub const SEED_DB_DIR: &str = "/tmp/crdb-base";


It'd be good to make this use one of the crates that finds a temp dir for you. I think elsewhere we do this, and it winds up using $TMPDIR from the environment in at least some cases.

Context: /tmp on Helios is generally tmpfs, which comes from swap. This uses up physical memory. We already consume a lot of that during builds and tests and it's led to exhaustion even on decently large systems. There's no need for this to be in-memory. Plus, allowing people to override this in a standard way allows folks to put this on a ZFS dataset that they've configured differently.

Updated to use https://doc.rust-lang.org/std/env/fn.temp_dir.html

davepacheco · 2021-12-09T17:06:28Z

test-utils/src/dev/mod.rs

+fn copy_dir(
+    src: impl AsRef<Path>,
+    dst: impl AsRef<Path>,
+) -> std::io::Result<()> {


I'm surprised there's not a standard crate for doing this. There are a lot of edge cases, which I guess we haven't run into in this case? (what if there's a named pipe here or a symlink?)

Also, this is all synchronous, even though the caller is async. I guess that's okay here?

Yeah, I looked around, and couldn't find anything much better than doing this myself.

I can make it async, but given the "test setup" context, I'm not sure how worthwhile this is to optimize for concurrency?

davepacheco · 2021-12-09T17:11:18Z

.github/buildomat/jobs/build-and-test.sh

@@ -14,6 +14,18 @@ set -o xtrace
 cargo --version


Could you update the README to reflect this? Basically this part:

CockroachDB v21.1.10.
The test suite expects to be able to start a single-node CockroachDB cluster using the cockroach executable on your PATH.

I think that should now say "the build and test suite expect..."

This reverts commit ab8872a.

smklein added 8 commits December 8, 2021 14:05

[nexus] Refactor test-utilities to helper crate, add test benchmarks

082c009

No need to be screamy about disk posting, that's for another PR

444aaa0

Optimize CRDB setup by using 'compile-time' population of a seed data…

42727a7

…base

Merge branch 'main' into benchmark-tests

d1c1345

Add to top-level workspace

54b6f68

Merge branch 'benchmark-tests' into optimize-crdb-setup

6850f67

Add some explanations

c736fe0

re-run if build.rs changes

05aed1b

smklein changed the title ~~Optimize CockroachDB setup time for tests~~ Optimize CockroachDB setup time for tests (-96% reduction of DB setup time) Dec 9, 2021

smklein changed the title ~~Optimize CockroachDB setup time for tests (-96% reduction of DB setup time)~~ Optimize CockroachDB setup for tests (96% reduction) Dec 9, 2021

The temporary directory might not exist

304d03d

smklein added 2 commits December 8, 2021 21:26

Merge branch 'main' into benchmark-tests

61d3fd3

Merge branch 'benchmark-tests' into optimize-crdb-setup

a992ee8

proby dropshot

c5427aa

smklein added 5 commits December 8, 2021 23:00

Remove test-utils from default workspace

fea3cd9

Download database executables *before* we build/test all targets

0153e19

Merge branch 'main' into benchmark-tests

0bcdc28

Merge branch 'benchmark-tests' into optimize-crdb-setup

52b31c6

Adjust path when building, we want that executable for our build script

a0c88a4

smklein commented Dec 9, 2021

View reviewed changes

smklein requested review from davepacheco and david-crespo December 9, 2021 05:08

davepacheco reviewed Dec 9, 2021

View reviewed changes

davepacheco mentioned this pull request Dec 9, 2021

Merge integration tests together #497

Merged

david-crespo approved these changes Dec 9, 2021

View reviewed changes

Merge branch 'main' into benchmark-tests

43b75c9

smklein added 3 commits December 9, 2021 15:53

Merge branch 'benchmark-tests' into optimize-crdb-setup

3dd98d8

Merge branch 'main' into benchmark-tests

419d89e

Merge branch 'benchmark-tests' into optimize-crdb-setup

7b9dc1b

Base automatically changed from benchmark-tests to main December 9, 2021 21:51

smklein added 4 commits December 9, 2021 17:22

Merge branch 'main' into optimize-crdb-setup

9ad763c

fixup

1b19c57

review feedback

4f9feef

OUT of the default members again gah

8eb0549

smklein merged commit ab8872a into main Dec 9, 2021

smklein deleted the optimize-crdb-setup branch December 9, 2021 23:23

davepacheco mentioned this pull request Dec 22, 2021

test_paginated_single_column_descending is flaky #540

Closed

smklein added a commit that referenced this pull request Dec 23, 2021

Revert "Optimize CockroachDB setup for tests (96% reduction) (#493)"

6bfd868

This reverts commit ab8872a.

Conversation

smklein commented Dec 9, 2021

Uh oh!

david-crespo commented Dec 9, 2021

Uh oh!

david-crespo commented Dec 9, 2021

Uh oh!

david-crespo commented Dec 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davepacheco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants