[crdb-seed] use a tarball, fix omicron-dev run-all #4208

sunshowers · 2023-10-05T00:58:47Z

Several changes:

In Make crdb-seed more robust to missing files #4193, @david-crespo observed some missing files in the crdb-seed generated directory. My suspicion is that that is due to the /tmp cleaner that runs on macOS. @davepacheco suggested using a tarball to get atomicity (either the file exists or it doesn't), and it ended up being pretty simple to do that at the end.
Teach nexus-test-utils to ensure that the seed tarball exists, fixing omicron-dev run-all and anything else that uses nexus-test-utils (and avoiding a dependency on the environment).
Move crdb-seed to dev-tools (thanks Dave for pointing it out!)
Add a way to invalidate the cache with CRDB_SEED_INVALIDATE=1 in the environment.
Add a readme for crdb-seed.

Fixes #4206.

Hopefully addresses #4193.

Created using spr 1.3.4

sunshowers · 2023-10-05T01:12:43Z

I tried migrating the function to anyhow so I could get better debugging, but couldn't get all the way -- there are still some .expects littered in there. Hopefully that isn't too much of a problem since this is test-only code.

smklein

LGTM, leaving this as a single artifact seems simpler to manage.

Seems like we'd still be at-risk of a /tmp cleaner deleting this directory, say, "after nextest setup begins, but before the test executes", but I'm not sure we can ever deal with that problem while still using /tmp.

crdb-seed/Cargo.toml

smklein · 2023-10-05T01:12:24Z

test-utils/src/dev/mod.rs

 }

+// TODO: switch to anyhow entirely -- this function is currently a mishmash


Just to clarify, this appears to already be using an anyhow::Result?

So this is currently a mishmash of anyhow::Result and unwrap/expect. I've expanded the comment to make this clearer.

Created using spr 1.3.4

sunshowers · 2023-10-05T01:22:03Z

I also added a way to invalidate the cache with CRDB_SEED_INVALIDATE=1. Any idea if/where this is worth documenting?

Created using spr 1.3.4

smklein · 2023-10-05T01:36:28Z

I also added a way to invalidate the cache with CRDB_SEED_INVALIDATE=1. Any idea if/where this is worth documenting?

A help message for the crdb-seed binary would help -- we could also document NEXTEST_ENV usage there too?

sunshowers · 2023-10-05T01:48:26Z

A help message for the crdb-seed binary would help -- we could also document NEXTEST_ENV usage there too?

Hmm, any thoughts on checking that argv[1] is --help vs just pulling in clap for that? clap seems a little overkill.

Maybe we could also add a README.

Created using spr 1.3.4

sunshowers · 2023-10-05T23:41:48Z

Requesting review again because I added a bunch of new stuff to the PR.

Created using spr 1.3.4

smklein

LGTM, mod one comment clarifying mtimes!

smklein · 2023-10-06T18:33:38Z

test-utils/src/dev/seed.rs

+/// This method should _not_ be used by tests. Instead, rely on the `crdb-seed`
+/// setup script.
+pub async fn ensure_seed_tarball_exists(
+    log: &Logger,
+    why_invalidate: Option<&str>,
+) -> Result<(Utf8PathBuf, SeedTarballStatus)> {
+    // If the CRDB_SEED_TAR_ENV variable is set, return an error.
+    if let Ok(val) = std::env::var(CRDB_SEED_TAR_ENV) {
+        anyhow::bail!(
+            "{CRDB_SEED_TAR_ENV} is set to `{val}` -- implying that a test called \
+             ensure_seed_tarball_exists. Instead, tests should rely on the `crdb-seed` \
+             setup script."
+        );
+    }


This seems like a bit of a proxy for #[ifcfg(not(test))] -- is that too extreme of a conditional qualifier to put on this method?

It does seem like it would make crdb-seed, the binary, untestable, but I think it would enforce more accurately what the comments are describing.

This is a dependency (not a leaf node) so I don't think cfg(not(test)) would work? And if I try gating it behind a feature flag, the issue would be that omicron-dev needs this function and so feature unification would mean this gets included in test binaries if you're doing a top-level cargo test.

Actually, I ended up both preserving this check and gating it behind a feature flag. So we're triply sure this module isn't used by code other than omicron-dev and crdb-seed.

smklein · 2023-10-06T18:45:42Z

test-utils/src/dev/seed.rs

+            std::fs::OpenOptions::new()
+                .append(true)
+                .open(&desired_seed_tar)
+                .context("failed to touch seed tarball")?;


Is this sufficient to update mtime, if the file exists? If the file already exists, but nothing is written, I would have suspected that this does not update the underlying timestamp.

Really really good catch, thank you. I think I may end up using the filetime crate as in https://github.com/uutils/coreutils/blob/a596cda51612e72b5245ccb8c2fa6c05a89569a7/src/uu/touch/src/touch.rs#L205-L207.

Done, the filetime crate works great.

smklein · 2023-10-06T18:48:20Z

test-utils/src/dev/seed.rs

+    // The tarball didn't exist when we started, so try to create it.
+    //


Nit: the tarball no longer exists

If why_invalidated is Some, it might have existed, but been removed

Expanded this comment:

// At this point the tarball does not exist (either because it didn't exist // in the first place or because it was deleted above), so try to create it.

Created using spr 1.3.4

sunshowers added 3 commits October 4, 2023 17:58

[𝘀𝗽𝗿] initial version

4d670aa

Created using spr 1.3.4

Rebase

e93b194

Created using spr 1.3.4

Fix some comments

87536bf

Created using spr 1.3.4

sunshowers requested a review from smklein October 5, 2023 01:04

Remove nextest.toml changes that aren't relevant

f3727ab

Created using spr 1.3.4

smklein approved these changes Oct 5, 2023

View reviewed changes

sunshowers added 2 commits October 4, 2023 18:18

Also add a mechanism to invalidate the seed tarball

12912ae

Created using spr 1.3.4

Address review comments

b456f42

Created using spr 1.3.4

Make invalidated logic clearer

edd6b16

Created using spr 1.3.4

sunshowers mentioned this pull request Oct 5, 2023

omicron-dev run-all relies on CRDB_SEED_DIR #4206

Closed

More changes -- will update summary

9f7bddd

Created using spr 1.3.4

sunshowers changed the title ~~[crdb-seed] use a tarball rather than a directory~~ [crdb-seed] use a tarball, fix omicron-dev run-all Oct 5, 2023

Make it as hard as possible for tests to run ensure_seed_tarball_exists

5e7528c

Created using spr 1.3.4

sunshowers requested a review from smklein October 5, 2023 23:41

Fix docs, add note about permissions

396af26

Created using spr 1.3.4

smklein approved these changes Oct 6, 2023

View reviewed changes

Address final review comments

27013e4

Created using spr 1.3.4

sunshowers enabled auto-merge (squash) October 6, 2023 23:48

Fix CRDB_SEED_TAR_ENV import

494a1c4

Created using spr 1.3.4

sunshowers merged commit 9f004d2 into main Oct 7, 2023
23 checks passed

sunshowers deleted the sunshowers/spr/crdb-seed-use-a-tarball-rather-than-a-directory branch October 7, 2023 04:11

sunshowers mentioned this pull request Oct 10, 2023

Make crdb-seed more robust to missing files #4193

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[crdb-seed] use a tarball, fix omicron-dev run-all #4208

[crdb-seed] use a tarball, fix omicron-dev run-all #4208

sunshowers commented Oct 5, 2023 •

edited

Loading

sunshowers commented Oct 5, 2023

smklein left a comment

smklein Oct 5, 2023

sunshowers Oct 5, 2023

sunshowers commented Oct 5, 2023

smklein commented Oct 5, 2023

sunshowers commented Oct 5, 2023

sunshowers commented Oct 5, 2023

smklein left a comment

smklein Oct 6, 2023

sunshowers Oct 6, 2023

sunshowers Oct 6, 2023 •

edited

Loading

smklein Oct 6, 2023

sunshowers Oct 6, 2023

sunshowers Oct 6, 2023

smklein Oct 6, 2023

sunshowers Oct 6, 2023

		}

		// TODO: switch to anyhow entirely -- this function is currently a mishmash

		// The tarball didn't exist when we started, so try to create it.
		//

[crdb-seed] use a tarball, fix omicron-dev run-all #4208

[crdb-seed] use a tarball, fix omicron-dev run-all #4208

Conversation

sunshowers commented Oct 5, 2023 • edited Loading

sunshowers commented Oct 5, 2023

smklein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunshowers commented Oct 5, 2023

smklein commented Oct 5, 2023

sunshowers commented Oct 5, 2023

sunshowers commented Oct 5, 2023

smklein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunshowers Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunshowers commented Oct 5, 2023 •

edited

Loading

sunshowers Oct 6, 2023 •

edited

Loading