[reconfigurator] Add rendezvous_debug_dataset table #7341

jgallagher · 2025-01-14T19:14:23Z

Also adds datastore methods and a library crate to populate it.

This is PR 1 of 2; the second PR will add both an RPW that calls this library crate and adds a consumer of this table.

Also adds datastore methods and a library crate to populate it.

smklein

Nicely done, this structure makes sense to me

smklein · 2025-01-14T19:40:27Z

nexus/reconfigurator/rendezvous/src/debug_dataset.rs

+    // We want to insert any in-service datasets (according to the blueprint)
+    // that are also present in `inventory_datasets` but that are not already
+    // present in the database (described by `existing_datasets`).
+    let datasets_to_insert = inventory_datasets


Suggested change

let datasets_to_insert = inventory_datasets

let new_inventory_datasets = inventory_datasets

This is a nitpick, but I think it's worthwhile. We don't necessarily want to insert these debug datasets unless they're also in the blueprint as in-service.

I reworked this in 08315cf to not even construct this intermediate set.

andrewjstone · 2025-01-14T20:41:13Z

schema/crdb/dbinit.sql

+     * Hard deletion of tombstoned datasets will require some care with respect
+     * to the problem above. For now we keep tombstoned datasets around forever.
+     */
+    time_tombstoned TIMESTAMPTZ,


It may be worth adding a comment here or in the corresponding rust type that we should not rely on the fact that time_tombstoned is later than time_created for correctness. While it's highly unlikely that is going to not be true, it still shouldn't be relied upon.

Hmm. Do you think I should add a blueprint_when_tombstoned column? That's what we're really operating on; the time is only useful to a human to narrow down when things might have happened.

I think that would indeed be useful. I don't think you want to get rid of the timestamps, just add that additional column and a comment. Thank you!

Added in 8372bb6

andrewjstone · 2025-01-14T20:56:30Z

nexus/reconfigurator/rendezvous/src/debug_dataset.rs

+    // This is a minor performance optimization. If we removed this fetch, the
+    // code below would still be correct, but it would issue a bunch of
+    // do-nothing inserts for already-existing datasets.
+    let existing_datasets = datastore


I realize we don't have multi-rack support yet, but I'm a little leery of this optimization. In our max expected scale out to 1k racks, this could result in over 300,000 datasets being pulled each time, if there is a debug dataset on each disk in a sled. I was thinking that we could instead diff this blueprint with it's parent instead, and then add or remove those datasets as needed. This could also end up reading in all these datasets, but that may also be true already if those blueprints are loaded.

Probably nothing to worry about right now, but long term these blueprints might get huge and we may not want to access them all at once even...

Oh, it looks like I'm probably completely wrong about a debug dataset per disk. In that case, ignore me (at least for debug datasets).

Every U.2 gets a Debug dataset and a Transient Zone Root dataset.

Source:
ensure_disk, in nexus/reconfigurator/planning/src/blueprint_editor/sled_editor.rs

Ah, thank you @smklein. Well, I'm still concerned, but it's probably not something we can worry about now. Scale will have more issues we can worry about later ;)

Yeah, I think this function actually has both cases and neither of them seem good (but I'm not sure what to do about it):

For in-service datasets, we do one (or technically "a small number", since it's paginated into batches) big query up front to list all the things, then only issue individual "insert if not exists" queries for datasets that weren't in that big list. This means we almost always only issue the one big query, since we only have to issue the small inserts when new datasets are added (which is very rare).

For expunged datasets, we don't do that: instead we always issue a "mark this tombstoned" individual query for every single expunged dataset.

The second one seems worse to me; that means any time we expunge a debug dataset, we're now issuing 1 more query on every execution of this RPW.

After writing this down, maybe the expunge case should also have a one-big-query-up-front-to-avoid-extra-work thing?

The second one seems worse to me; that means any time we expunge a debug dataset, we're now issuing 1 more query on every execution of this RPW.

This is only true until we prune the expunged nodes from the blueprint though, right? In theory those could be limited, while the total set size will remain at the the number of disks in the system.

This is only true until we prune the expunged nodes from the blueprint though, right?

Yes, but given pruning expunged nodes from the blueprint is a "don't need to solve any time soon" problem, in practice those will stick around for quite a while, I think.

Sure, but I assume we'll solve it before we have multirack :D

So when I wrote this function and my comment above, I was thinking about the original implementation of debug_dataset_list_all_batched() which only returned non-tombstoned rows. But I changed that because we know planning input is going to care about tombstoned rows too, so I changed the "expunged" side of things to reuse the results of that query too in 08315cf. Doesn't address the problem of "is it okay to fetch all debug datasets in a multirack world", but as you point out we can sort that out later!

andrewjstone

I left a few small comments, but this looks really good. Thanks for getting it out so quickly @jgallagher!

…ndezvous-1

…7342) This is PR 2 of 2 and builds on #7341; it adds an RPW that calls the library added in that PR to actually reconcile blueprint+inventory and update the debug dataset rendezvous table, and changes the support bundle query that picks a debug dataset to use this new table.

[reconfigurator] Add rendezvous_debug_dataset table

8c7db6d

Also adds datastore methods and a library crate to populate it.

jgallagher requested review from andrewjstone, davepacheco and smklein January 14, 2025 19:14

jgallagher mentioned this pull request Jan 14, 2025

[reconfigurator] Add RPW to reconcile debug dataset rendezvous table #7342

Merged

rustdoc fix

541a22e

smklein approved these changes Jan 14, 2025

View reviewed changes

andrewjstone reviewed Jan 14, 2025

View reviewed changes

andrewjstone approved these changes Jan 14, 2025

View reviewed changes

jgallagher added 3 commits January 15, 2025 10:50

add blueprint_id_when_tombstoned column

8372bb6

clean up RPW implementation

08315cf

Merge remote-tracking branch 'origin/main' into john/debug-dataset-re…

c9c8463

…ndezvous-1

jgallagher merged commit fbe043a into main Jan 15, 2025
18 checks passed

jgallagher deleted the john/debug-dataset-rendezvous-1 branch January 15, 2025 21:26

jgallagher mentioned this pull request Jan 24, 2025

Change dataset table to only hold Crucible datasets #7386

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[reconfigurator] Add rendezvous_debug_dataset table #7341

[reconfigurator] Add rendezvous_debug_dataset table #7341

jgallagher commented Jan 14, 2025

smklein left a comment

smklein Jan 14, 2025

jgallagher Jan 15, 2025 •

edited

Loading

andrewjstone Jan 14, 2025

jgallagher Jan 14, 2025

andrewjstone Jan 14, 2025 •

edited

Loading

jgallagher Jan 15, 2025

andrewjstone Jan 14, 2025

andrewjstone Jan 14, 2025 •

edited

Loading

smklein Jan 14, 2025

andrewjstone Jan 14, 2025

jgallagher Jan 14, 2025

andrewjstone Jan 14, 2025

jgallagher Jan 15, 2025

andrewjstone Jan 15, 2025

jgallagher Jan 15, 2025

andrewjstone left a comment

	let datasets_to_insert = inventory_datasets
	let new_inventory_datasets = inventory_datasets

[reconfigurator] Add rendezvous_debug_dataset table #7341

[reconfigurator] Add rendezvous_debug_dataset table #7341

Conversation

jgallagher commented Jan 14, 2025

smklein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgallagher Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewjstone Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewjstone Jan 14, 2025 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewjstone left a comment

Choose a reason for hiding this comment

jgallagher Jan 15, 2025 •

edited

Loading

andrewjstone Jan 14, 2025 •

edited

Loading

andrewjstone Jan 14, 2025 •

edited

Loading