[nexus] Delete "inv_dataset" records from inventory collections #6632
Conversation
smklein
changed the title
| @@ -1241,6 +1242,17 @@ impl DataStore { | |||
| .await? | |||
| }; | |||
|
|
|||
| // Remove rows for datasets | |||
| let ndatasets = | |||
There was a problem hiding this comment.
This is the portion of the PR that actually fixes the underlying issue
| @@ -2731,6 +2746,12 @@ mod test { | |||
| .await | |||
| .unwrap(); | |||
| assert_eq!(0, count); | |||
| let count = schema::inv_dataset::dsl::inv_dataset | |||
There was a problem hiding this comment.
This is the portion of the test where I should have looked for the inv_dataset being gone
| /// NOTE: This test depends on the naming convention "inv_" prefix name | ||
| /// to identify pieces of the inventory. | ||
| #[tokio::test] | ||
| async fn test_inventory_deletion() { |
There was a problem hiding this comment.
This is a new test I added, out of paranoia, to try to help catch future cases like this, if any "inv_..." tables are added in the future, and they are not deleted.
There was a problem hiding this comment.
Yeah, it's not exactly a database snapshot, but querying for all present and future inv_ tables is an approximation
There was a problem hiding this comment.
Yeah the ticket isn't so much about that specific implementation so much as having a test to catch this problem.
I can't think of why you'd need a snapshot. A possible risk is that another inventory collection is inserted by a background task in the meantime, but since this is a datastore test and not an app-level test, there's no Nexus, and so that should be impossible.
Do you think we should have a test for the representative collection that it has rows for all inv_* tables? I'm a little afraid this test won't catch this problem in the future because someone might forget to add it to that, too. (It wouldn't have caught this specific instance for that reason, right?)
There was a problem hiding this comment.
I'm gonna merge this PR first, but yes, totally agreed, we should have a test for that. I'll file another issue.
| @@ -321,8 +321,19 @@ pub fn representative() -> Representative { | |||
| slot: 3, | |||
| }, | |||
| ]; | |||
| let zpools = vec![]; | |||
| let datasets = vec![]; | |||
| let zpools = vec
There was a problem hiding this comment.
LGTM. I'm on board with declaring #5305 fixed with this; it's by convention, but it's a pretty strong convention at this point.
Fixes #6615
Does not patch existing systems (dogfood) which may have unnecessary inventory entries