[7/n] [reconfigurator] combine inventory and omicron zones #6739

sunshowers · 2024-10-01T20:03:59Z

Currently, when the collection process makes two separate queries to sled-agent: one to obtain basic inventory, and the other to obtain Omicron zones. With this approach, it is possible for one of the two queries to fail -- in that case, the sled_agents and omicron_zones maps will not have the same set of keys.

In general, consumers of collections must be able to handle these kinds of failing cases, because collections contain information from a variety of sources, not all of which are guaranteed to work all the time. But this specific case (sled-agent inventory vs Omicron zones) is not intrinsic to the system -- it is an artifact of our APIs.

Change the API so that the /inventory endpoint also returns Omicron zones. The format remains the same, so that PUT /omicron-zones mirrors GET /inventory | jq .omicron_zones.

I had to make a few changes to our tests, since we now have a unified notion of Omicron zones and sled agents. But this exposed what feels to me like a modeling deficiency in the way our tests work. I hope to fix that up at some point soon.

There is also one behavior change to the reconfigurator CLI -- now, inventory-generate preserves zone information passed into it from the system. This matches the behavior of inventory generation within tests themselves.

Depends on:

Created using spr 1.3.6-beta.1 [skip ci]

Created using spr 1.3.6-beta.1

nexus/reconfigurator/planning/src/example.rs

sunshowers · 2024-10-02T05:13:05Z

~~(realized we can simplify this a lot by creating a explicit series of system descriptions along with their derived outputs -- will take a look at that tomorrow)~~

(will tackle this separately)

Created using spr 1.3.6-beta.1 [skip ci]

Created using spr 1.3.6-beta.1

sunshowers · 2024-10-04T00:37:54Z

nexus/reconfigurator/planning/src/planner.rs

+        // TODO: mutating example.system doesn't automatically update
+        // example.collection -- this should be addressed via API improvements.


This is unfortunate -- users must remember to do this themselves. This is what drove me to investigate how to make feedback from tests to SystemDescription work.

Created using spr 1.3.6-beta.1 [skip ci]

Created using spr 1.3.6-beta.1

andrewjstone

Looks great.

andrewjstone · 2024-10-04T15:41:16Z

nexus/db-queries/src/db/datastore/inventory.rs

+                usable_physical_ram: s.usable_physical_ram.into(),
+                reservoir_size: s.reservoir_size.into(),
+                omicron_zones,
+                // TODO: the unwrap_or_defaults here look incorrect -- all the


Do you want to fix this in this PR? If not, mind opening an issue?

I can see two possibilities here:

If disks, zpools, datasets are like Omicron zones, where there's a "parent" record for the list itself (the inv_sled_omicron_zones), then I expect that this TODO is probably right and instead of unwrap_or_default() we probably want ok_or_else(|| Error::internal_error(&format!("sled {}: missing physical disks that we expected to see", sled_id))?.

If disks, zpools, and datasets don't have a separate record like this, then unwrap_or_default() seems correct? We would have only populated the maps for the rows that we found, but if a sled just had 0 disks, zpools, or datasets, then we wouldn't have found any rows, it wouldn't have shown up in the map, and then the empty Vec is correct here, right?

All of it makes me wonder if we have test coverage for serializing and deserializing a collection containing a sled with no disks, zpools, or datasets.

Ah yeah, it does look like the latter -- so yes, the unwrap_or_default is correct. Thanks.

Added this in a comment.

davepacheco

Cool!

Reviewing this prompted me to remember one reason I think I had separated these initially, which is that in the sled agent, the Omicron zones information was protected by a lock, and that lock gets held for the entire duration of the zone reconfiguration that happens when you PUT /omicron-zones. That could be a while. I was worried this would prevent us from getting any information about the sled while zone reconfiguration was happening. I'm not sure if the locking still works that way, but either way, it doesn't seem to have been a meaningful problem. (If it were, presumably we'd have a lot of partially failed inventory collections. I don't think we've noticed that.) If it is or becomes a problem, we can always fix the locking. All that's to say: this looks good.

nexus/db-queries/src/db/datastore/inventory.rs

davepacheco · 2024-10-04T19:54:49Z

nexus/db-queries/src/db/datastore/inventory.rs

+                usable_physical_ram: s.usable_physical_ram.into(),
+                reservoir_size: s.reservoir_size.into(),
+                omicron_zones,
+                // TODO: the unwrap_or_defaults here look incorrect -- all the


I can see two possibilities here:

If disks, zpools, datasets are like Omicron zones, where there's a "parent" record for the list itself (the inv_sled_omicron_zones), then I expect that this TODO is probably right and instead of unwrap_or_default() we probably want ok_or_else(|| Error::internal_error(&format!("sled {}: missing physical disks that we expected to see", sled_id))?.

If disks, zpools, and datasets don't have a separate record like this, then unwrap_or_default() seems correct? We would have only populated the maps for the rows that we found, but if a sled just had 0 disks, zpools, or datasets, then we wouldn't have found any rows, it wouldn't have shown up in the map, and then the empty Vec is correct here, right?

All of it makes me wonder if we have test coverage for serializing and deserializing a collection containing a sled with no disks, zpools, or datasets.

davepacheco · 2024-10-04T20:15:46Z

nexus/reconfigurator/planning/src/system.rs

+    /// # Notes
+    ///
+    /// While bootstrapping an example system, the order is:
+    ///
+    /// 1. Create a collection with sleds with no zones.
+    /// 2. From the collection, generate a blueprint with zones.
+    /// 3. Seed the system with zones from the blueprint.
+    ///
+    /// This method is used in step 3.


I was a little confused by this comment. What's the purpose of having it here? I feel like maybe you're trying to say that there's some assumptions it makes that are only true in this situation, and so it wouldn't be valid if you were calling it for some other reason? Or maybe that it wasn't suitable for other situations?

I do feel like it could use a comment explaining what it means to set the zones for a particular sled. I assume it means "these zones will be reported in the inventory for this sled" and that the caller is allowed to change this however it wants (e.g., even going backwards or transitions in other illegal ways, even though that shouldn't happen in a real system) -- nothing internal to SystemDescription will break. (An alternative reading would be: make these the zones that the sled should be running, though it might not yet be, which probably doesn't make sense in the context of SystemDescription.)

(My confusion is probably not relevant but in case it's useful: when I first read it, I thought it was going to describe the three-step process this function itself was going to carry out. Then I got to the end and realized it wasn't that. Then I thought "method" meant "approach". It took me a few readings to parse this as as "context about how this function is used".)

Sorry about not being clear here! Yes, it means "when Self::to_collection_builder is called next, it will contain these Omicron zones for the sled".

I agree that describing the bootstrap process isn't very useful (you can discover this by looking for the callers of sled_set_omicron_zones), so I've dropped it.

davepacheco · 2024-10-04T21:45:58Z

dev-tools/reconfigurator-cli/src/main.rs

        sim.system.to_collection_builder().context("generating inventory")?;
-    // For an inventory we just generated from thin air, pretend like each sled


Does this change the behavior here?

It does -- and I think what I was intending to do here was to try and figure out what this really means. I'll work on reconfigurator-cli in another PR that this depends on.

Put up a few more PRs in the series, and established this behavior change via new tests. I think it makes sense -- the system now carries around zone information so it's weird for the inventory to ignore it.

Created using spr 1.3.6-beta.1 [skip ci]

Created using spr 1.3.6-beta.1

sunshowers added 2 commits October 1, 2024 13:03

[spr] changes to main this commit is based on

442bf25

Created using spr 1.3.6-beta.1 [skip ci]

[spr] initial version

83d34b7

Created using spr 1.3.6-beta.1

sunshowers commented Oct 1, 2024

View reviewed changes

nexus/reconfigurator/planning/src/example.rs Outdated Show resolved Hide resolved

sunshowers added 2 commits October 3, 2024 17:02

[spr] changes introduced through rebase

29eaceb

Created using spr 1.3.6-beta.1 [skip ci]

Simplify this, do remaining work later

792f4df

Created using spr 1.3.6-beta.1

sunshowers mentioned this pull request Oct 4, 2024

Combine inv_sled_omicron_zones with inv_sled_agent #6770

Open

fix clippy

6b65896

Created using spr 1.3.6-beta.1

sunshowers commented Oct 4, 2024

View reviewed changes

sunshowers marked this pull request as ready for review October 4, 2024 00:38

sunshowers added 2 commits October 3, 2024 17:48

[spr] changes introduced through rebase

dc166e9

Created using spr 1.3.6-beta.1 [skip ci]

docs

fdc8de7

Created using spr 1.3.6-beta.1

sunshowers requested review from andrewjstone, smklein, jgallagher and davepacheco October 4, 2024 01:24

sunshowers changed the title ~~[reconfigurator] combine inventory and omicron zones~~ [3/n] [reconfigurator] combine inventory and omicron zones Oct 4, 2024

andrewjstone approved these changes Oct 4, 2024

View reviewed changes

davepacheco approved these changes Oct 4, 2024

View reviewed changes

sunshowers added 2 commits October 5, 2024 17:51

[spr] changes introduced through rebase

242434d

Created using spr 1.3.6-beta.1 [skip ci]

rebase on earlier commits

9f9a91a

Created using spr 1.3.6-beta.1

sunshowers changed the title ~~[3/n] [reconfigurator] combine inventory and omicron zones~~ [7/n] [reconfigurator] combine inventory and omicron zones Oct 6, 2024

sunshowers changed the base branch from sunshowers/spr/main.reconfigurator-combine-inventory-and-omicron-zones to main October 11, 2024 21:30

rebase on main

762051b

Created using spr 1.3.6-beta.1

sunshowers enabled auto-merge (squash) October 11, 2024 21:31

sunshowers merged commit c9fc674 into main Oct 12, 2024
17 checks passed

sunshowers deleted the sunshowers/spr/reconfigurator-combine-inventory-and-omicron-zones branch October 12, 2024 01:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[7/n] [reconfigurator] combine inventory and omicron zones #6739

[7/n] [reconfigurator] combine inventory and omicron zones #6739

sunshowers commented Oct 1, 2024 •

edited

Loading

sunshowers commented Oct 2, 2024 •

edited

Loading

sunshowers Oct 4, 2024

andrewjstone left a comment

andrewjstone Oct 4, 2024

davepacheco Oct 4, 2024

sunshowers Oct 4, 2024

sunshowers Oct 4, 2024

davepacheco left a comment

davepacheco Oct 4, 2024

davepacheco Oct 4, 2024

sunshowers Oct 4, 2024

davepacheco Oct 4, 2024

sunshowers Oct 4, 2024 •

edited

Loading

sunshowers Oct 6, 2024

		// TODO: mutating example.system doesn't automatically update
		// example.collection -- this should be addressed via API improvements.

		sim.system.to_collection_builder().context("generating inventory")?;
		// For an inventory we just generated from thin air, pretend like each sled

[7/n] [reconfigurator] combine inventory and omicron zones #6739

[7/n] [reconfigurator] combine inventory and omicron zones #6739

Conversation

sunshowers commented Oct 1, 2024 • edited Loading

sunshowers commented Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

andrewjstone left a 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

davepacheco left a 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 Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunshowers commented Oct 1, 2024 •

edited

Loading

sunshowers commented Oct 2, 2024 •

edited

Loading

sunshowers Oct 4, 2024 •

edited

Loading