BlueprintSledConfig: One generation number for sled-agent instead of three
#7788
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I think this is actually not fine; the I suspect that when the generation is bumped without any of the disks actually changing, the new generation is not propagated out and so we get stuck waiting for it here just like after expunging a nonexistent disk: omicron/sled-agent/src/sled_agent.rs Lines 927 to 930 in 3802ae4 |
andrewjstone
approved these changes
Mar 13, 2025
| @@ -371,17 +388,13 @@ impl BpTableData for BlueprintZonesConfig { | |||
|
|
|||
| // Useful implementation for printing two column tables stored in a `BTreeMap`, where | |||
| // each key and value impl `Display`. | |||
| impl<S1, S2> BpTableData for (Generation, &BTreeMap<S1, S2>) | |||
| .sleds | ||
| .added | ||
| .values() | ||
| .fold(0, |acc, c| acc + c.zones_config.zones.len()) |
There was a problem hiding this comment.
It's so nice to get rid of those intermediate types. This reads much better now.
| } | ||
| } | ||
| } | ||
|
|
||
| #[derive(Debug)] | ||
| pub struct DiffPhysicalDisksDetails { | ||
| // Newly added sleds don't have "before" disks |
There was a problem hiding this comment.
These generations were the most annoying things to work with. Very happy to get rid of them and simplify all the callers above.
| @@ -0,0 +1,2 @@ | |||
| ALTER INDEX IF EXISTS omicron.public.bp_sled_metadata @ bp_sled_state_pkey | |||
jgallagher
added a commit
that referenced
this pull request
Mar 14, 2025
…ources` (#7793) This isn't a very big PR, but I'm not super familiar or comfortable with the pieces surrounding these changes, so please review carefully. Rationale for these changes follow. --- In both #7622 and #7788 (comment), we see `omicron_physical_disks_ensure` get stuck (apparently indefinitely) here: https://github.com/oxidecomputer/omicron/blob/3802ae4736a26b25b66be028c0a2217f6428a722/sled-agent/src/sled_agent.rs#L927-L930 I believe this can happen when there's a generation number change to the disks config but the corresponding execution of `StorageResources::synchronize_disk_management()` does not set `updated` to true, which means it [will not send the new disk config on the `disks_updated` channel](https://github.com/oxidecomputer/omicron/blob/3802ae4736a26b25b66be028c0a2217f6428a722/sled-storage/src/resources.rs#L403-L405). This leaves `StorageMonitor::await_generation()` stuck [waiting for an update that is never coming](https://github.com/oxidecomputer/omicron/blob/8222b27e850d2c3adc496222a317fe5ff96d1289/sled-agent/src/storage_monitor.rs#L59-L64). Both of the above issues are examples of such paths: 1. In #7622, if the new config removes a disk that is physically not present, we [do not set `updated` to true](https://github.com/oxidecomputer/omicron/blob/3802ae4736a26b25b66be028c0a2217f6428a722/sled-storage/src/resources.rs#L331-L340). 2. In #7788 (comment), if the generation is increased but there have been no changes to the disk config, we never set `updated` to true. I think there are two design choices in `StorageResources` that make this difficult to reason about, and this PR attempts to address them both. 1. `StorageResources` holds both [`disks: AllDisks` and `disk_updates: watch::Sender<AllDisks>`](https://github.com/oxidecomputer/omicron/blob/3802ae4736a26b25b66be028c0a2217f6428a722/sled-storage/src/resources.rs#L221-L231), and allows them to diverge at times and has to choose when to sync them back up. This PR removes `disk_updates` and changes `disks` to be the `watch::Sender<AllDisks>`, removing the possibility of us having two different `AllDisks` that have diverged in contents. (We still have to make local clones of the `AllDisks` via `Arc::make_mut()`, so we temporarily have multiple copies that intentionally diverge, but they're local and dropped when the methods return.) I tried to make it so we didn't need to keep track of `updated` manually. 2. `StorageResources::set_config()` [updates the generation held inside `self.disks`](https://github.com/oxidecomputer/omicron/blob/3802ae4736a26b25b66be028c0a2217f6428a722/sled-storage/src/resources.rs#L274), but neither sends that information along the `disk_updates` channel nor informs `synchronize_disk_resources` that the generation has changed. This seems particularly dicey: after `set_config` returns, we've increased the generation inside `self.disks` but have not actually acted on the new config from that generation, so inspecting `self.disks` at that point would be very confusing. This PR changes `set_config` to record the generation "on the side" (i.e., not updating the generation inside `self.disks`), and defers updating the generation inside `self.disks` to `synchronize_disk_resources`. I tested this on a4x2 by manually performing a "spurious disk config generation bump", and confirmed sled-agent did not get wedged as it does on `main`. --- Fixes #7622.
This was fixed by #7793, and indeed after merging with |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the first half of #7774. I naively expected to do the blueprint side and the sled-agent side together, but the blueprint side alone is pretty big already, so here it is.
The primary changes are in
nexus-types:BlueprintZonesConfig,BlueprintDatasetsConfig, andBlueprintPhysicalDisksConfig; each of these was a generation number + an appropriateIdMapOmicronSledConfignow contains a single generation number and threeIdMapsand everything else is fallout from that. Most of the changes are mechanical (and I highly recommend viewing the diff ignoring whitespace; it cuts a few hundred +/- lines out). The bits that are less mechanical and deserve a closer view are:
One decision I think is fine but is also worth double checking: as of this PR, we still have to send 3 generation numbers to sled-agent in
OmicronSledConfig, so I'm replicating the singleBlueprintSledConfiggeneration for all three of the sub-configs. I think this is fine; in practice if deployed, it means a sled might see its disks or datasets generation number jump a few units up without any changes to the actual disks or datasets, but I believe that's effectively a no-op other than rewriting the ledger JSON.