Set an arbitrary number of sled agents for tests #7353
Merged
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
As part of testing for oxidecomputer/crucible#1594, I needed the ability to specify multiple extra sled agents. I pulled some work out of the fix for that issue into this commit, which adds the `extra_sled_agents` parameter to the `nexus_test` macro (also refactoring that code so that other parameters can be added easier in the future). Previously, `ControlPlaneTestContext` would configure two sled agents, and later the impl of nexus_test_interface::NexusServer for Nexus would mark the second sled agent as non-provisionable. This commit changes the default number of sled agents created as part of `ControlPlaneTestContext` to 1, and if more are created they are not marked non-provisionable. Many tests do not require two sled agents, and those that do can specify how many they need with the new the `extra_sled_agents` parameter. A lot of other changes in this diff are mechanical: for example changing from reaching into the internals of `ControlPlaneTestContext` to using functions.
andrewjstone
approved these changes
Jan 15, 2025
| name: syn::Path, | ||
| _eq_token: syn::token::Eq, | ||
| value: syn::Path, | ||
| pub(crate) enum NexusTestArg { |
There was a problem hiding this comment.
Nice: I like this structure much better.
nexus/test-utils/src/lib.rs
Outdated
| @@ -120,6 +122,39 @@ pub const RACK_SUBNET: &str = "fd00:1122:3344:0100::/56"; | |||
| /// both transient deployments with no sensitive data. | |||
| pub const TEST_SUITE_PASSWORD: &str = "oxide"; | |||
|
|
|||
| pub struct ControlPlaneTestContextSledAgent { | |||
| #[allow(unused)] | |||
There was a problem hiding this comment.
Is this field actually used anywhere, or can we delete it?
There was a problem hiding this comment.
It's storing the tempdir, so we can't delete it - I changed the field to _storage and removed the #[allow(unused)]
| } | ||
| } | ||
|
|
||
| blueprint_disks.insert( |
There was a problem hiding this comment.
Just an FYI, this just changed to use IdMaps in #7352, so you'll need a bit more cleanup here. Sorry!
jmpesp
added a commit
to jmpesp/omicron
that referenced
this pull request
Jan 22, 2025
When performing region allocation _again_ for a particular volume and increasing the redundancy, the region allocation query was not excluding the sleds that have already been used by the existing allocation as candidates for the new region (note that this only matters when the allocation strategy mandates distinct sleds!). This resulted in region replacement and region snapshot replacement having the potential to allocate the replacement region onto the same sled as an existing region. This commit fixes the region allocation query, and then fixes the fallout: many tests of region replacement or region snapshot replacement were not creating many simulated sled agents, and therefore could not properly satisfy the fixed region allocation query. The `extra_sled_agents` parameter added in oxidecomputer#7353 was used, along with changing the `DiskTestBuilder` to simulated one zpool on many sleds. Changing these replacement tests revealed the next problem: because all the simulated sleds share the same IP (::1) and range of ports for the fake Crucible agents, each region allocation would have the same target, and the volume construction request would look like: "target": [ "[::1]:1100", "[::1]:1100", "[::1]:1100" ] This is very confusing for the replacement routines! We can't change the simulated sled's IPs: they need to bind to localhost to listen for requests. Therefore this commit also adds the idea of "sled index", and this sled index is used to make each range of Crucible agent ports unique. This lead straight to the next problem: the simulated Pantry assumes that there is really only one simulated sled-agent, and this has all the storage! This is no longer true in a post-oxidecomputer#7353 world. Snapshots would not be able to complete because the simulated Pantry would only find one of the three regions when searching the first simulated sled-agent's storage. The real Pantry would construct a Crucible Upstairs, and delegate snapshot requests to that, so this commit also adds a simulated Crucible Upstairs that's aware of each of the simulated sled agent's `Storage`, and can therefore fulfill snapshot requests. Centralizing the place where all simulated sled agents (and Pantry servers!) are created makes this easy: the simulated Upstairs can be aware of all of the `Storage` objects, and the simulated Pantry can use that simulated Upstairs to take fake snapshots. Fixes oxidecomputer/crucible#1594.
jmpesp
added a commit
that referenced
this pull request
Jan 23, 2025
When performing region allocation _again_ for a particular volume and increasing the redundancy, the region allocation query was not excluding the sleds that have already been used by the existing allocation as candidates for the new region (note that this only matters when the allocation strategy mandates distinct sleds!). This resulted in region replacement and region snapshot replacement having the potential to allocate the replacement region onto the same sled as an existing region. This commit fixes the region allocation query, and then fixes the fallout: many tests of region replacement or region snapshot replacement were not creating many simulated sled agents, and therefore could not properly satisfy the fixed region allocation query. The `extra_sled_agents` parameter added in #7353 was used, along with changing the `DiskTestBuilder` to simulated one zpool on many sleds. Changing these replacement tests revealed the next problem: because all the simulated sleds share the same IP (::1) and range of ports for the fake Crucible agents, each region allocation would have the same target, and the volume construction request would look like: "target": [ "[::1]:1100", "[::1]:1100", "[::1]:1100" ] This is very confusing for the replacement routines! We can't change the simulated sled's IPs: they need to bind to localhost to listen for requests. Therefore this commit also adds the idea of "sled index", and this sled index is used to make each range of Crucible agent ports unique. This lead straight to the next problem: the simulated Pantry assumes that there is really only one simulated sled-agent, and this has all the storage! This is no longer true in a post-#7353 world. Snapshots would not be able to complete because the simulated Pantry would only find one of the three regions when searching the first simulated sled-agent's storage. The real Pantry would construct a Crucible Upstairs, and delegate snapshot requests to that, so this commit also adds a simulated Crucible Upstairs that's aware of each of the simulated sled agent's `Storage`, and can therefore fulfill snapshot requests. Centralizing the place where all simulated sled agents (and Pantry servers!) are created makes this easy: the simulated Upstairs can be aware of all of the `Storage` objects, and the simulated Pantry can use that simulated Upstairs to take fake snapshots. Fixes oxidecomputer/crucible#1594.
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.
As part of testing for oxidecomputer/crucible#1594, I needed the ability to specify multiple extra sled agents. I pulled some work out of the fix for that issue into this commit, which adds the
extra_sled_agentsparameter to thenexus_testmacro (also refactoring that code so that other parameters can be added easier in the future).Previously,
ControlPlaneTestContextwould configure two sled agents, and later the impl of nexus_test_interface::NexusServer for Nexus would mark the second sled agent as non-provisionable. This commit changes the default number of sled agents created as part ofControlPlaneTestContextto 1, and if more are created they are not marked non-provisionable.Many tests do not require two sled agents, and those that do can specify how many they need with the new the
extra_sled_agentsparameter.A lot of other changes in this diff are mechanical: for example changing from reaching into the internals of
ControlPlaneTestContextto using functions.