[sp-sim] configurable initial contents of the cabooses #8734
Conversation
There was a problem hiding this comment.
Left some nits but in general this looks fine to me. In terms of #7913 - did you try updating an sp-sim running with a real board with a real artifact from a TUF repo?
| @@ -62,7 +64,9 @@ impl GatewayTestContext { | |||
| } | |||
| } | |||
|
|
|||
| pub fn load_test_config() -> (omicron_gateway::Config, sp_sim::Config) { | |||
| pub fn load_test_config( | |||
There was a problem hiding this comment.
It seems a little unfortunate this touches a bunch of callers and most of them are just passing DEFAULT_SP_SIM_CONFIG. I wonder if instead this should be a separate function? I.e., keep load_test_config() as it was, and add something like load_test_config_from(path) for the handful of callers that want to be able to pass a specific config?
There was a problem hiding this comment.
Unfortunately, changing that specific function only removes the need for DEFAULT_SP_SIM_CONFIG in like two tests 🙃 . I was looking to avoid using Option<T> , but I've had to add a few, specifically in setup_with_config_impl<N: NexusServer>(), which kinda bubbles up to a few more methods.
I don't love it? But I guess if you're writing tests for the console API you really don't need to know where the configuration for the SP sim is coming from. This whole test and simulated component set-up probably needs a clean up, but I'm not sure this PR is the place to do it. It seems like it's not a trivial endeavour.
| @@ -62,7 +64,9 @@ impl GatewayTestContext { | |||
| } | |||
| } | |||
|
|
|||
| pub fn load_test_config() -> (omicron_gateway::Config, sp_sim::Config) { | |||
| pub fn load_test_config( | |||
There was a problem hiding this comment.
Unfortunately, changing that specific function only removes the need for DEFAULT_SP_SIM_CONFIG in like two tests 🙃 . I was looking to avoid using Option<T> , but I've had to add a few, specifically in setup_with_config_impl<N: NexusServer>(), which kinda bubbles up to a few more methods.
I don't love it? But I guess if you're writing tests for the console API you really don't need to know where the configuration for the SP sim is coming from. This whole test and simulated component set-up probably needs a clean up, but I'm not sure this PR is the place to do it. It seems like it's not a trivial endeavour.
@jgallagher yep! $ ./target/debug/faux-mgs --sp-sim-addr [::1]:51033 update sp 0 /var/tmp/R13/repo/targets/47266ede81e13f5f1e36623ea8dd963842606b783397e4809a9a5f0bda0f8170.gimlet_sp-gimlet-e-1.0.34.tar.gz
Aug 18 08:45:04.212 INFO creating SP handle on to talk to SP simulator at [::1]:51033, component: faux-mgs
Aug 18 08:45:04.213 INFO initial discovery complete, addr: [::1]:51033, component: faux-mgs
Aug 18 08:45:04.215 INFO generated update ID, id: e62d3282-b45e-4ffd-8f3c-7a09aca446fb, component: faux-mgs
Aug 18 08:45:04.353 INFO starting SP update, sp_image_size: 655616, aux_flash_size: 0, aux_flash_chck: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0], id: e62d3282-b45e-4ffd-8f3c-7a09aca446fb, component: faux-mgs
Aug 18 08:45:04.354 INFO update in progress, total_size: 655616, bytes_received: 0, component: faux-mgs
Aug 18 08:45:04.354 INFO update preparation complete, update_id: e62d3282-b45e-4ffd-8f3c-7a09aca446fb, component: faux-mgs
Aug 18 08:45:04.434 INFO update complete, id: e62d3282-b45e-4ffd-8f3c-7a09aca446fb, component: faux-mgs
update complete
|
nexus/test-utils/src/lib.rs
Outdated
| @@ -673,10 +675,14 @@ impl<'a, N: NexusServer> ControlPlaneTestContextBuilder<'a, N> { | |||
| &mut self, | |||
| switch_location: SwitchLocation, | |||
| port: Option<u16>, | |||
| sp_sim_config_file: Option<Utf8PathBuf>, | |||
There was a problem hiding this comment.
Ug, yeah, I don't love callers passing None here. I think I'd prefer either:
- two methods (
start_gatewaythat doesn't take this arg,start_gateway_with_sp_sim_configthat takes this arg non-optionally) - go back to making this required, and make the callers pass the
DEFAULT...constant
but in general I agree that all of this test setup is pretty rough and this isn't the PR to do major cleanup.
There was a problem hiding this comment.
- two methods (start_gateway that doesn't take this arg, start_gateway_with_sp_sim_config that takes this arg non-optionally)
- go back to making this required, and make the callers pass the DEFAULT... constant
I think I'd rather go back to making this required. Having two methods for start_gateway feels like it would add even more complexity to an already complex and convoluted testing setup. WDYT?
This is a proposal for configurable cabooses in
sp-simandomicron-dev run-allenvironments. I wasn't really sure how configurable we wanted the tests that start up an sp-sim to be. I'm open to opinions/suggestions :)Manual testing with the following configuration file
Against a simulated omicron environment
Closes: #7913