Use sled inventory to build sled agent clients - #11197
Conversation
Instead of deriving sled agent addresses from switch zone service addresses (which does not work in the test context), pull the sled agent information from the inventory records in the database.
jgallagher
left a comment
jgallagher
left a comment
There was a problem hiding this comment.
Thanks, I think this is a lot better solution generally than 10958.
Just a few nits, mostly about test flakiness.
| let sled_agent_addrs: Vec<_> = sleds.into_iter().map(|s| s.address()).collect(); | ||
|
|
||
| // TODO https://github.com/oxidecomputer/omicron/issues/5201 | ||
| // build sled agent clients for sleds that are connected to the switches | ||
| let scrimlet_sled_agent_clients = build_sled_agent_clients(&mappings, &log); | ||
| let sled_agent_clients = build_sled_agent_clients(&sled_agent_addrs, &log); |
There was a problem hiding this comment.
Nitpicky, but we build two Vec<_>s here (a vec of addresses and then a vec of clients); we can definitely get away with just one, and I think if I'm reading it correctly we could get away with neither. build_sled_agent_clients() isn't buying us much anymore - I think if we inlined client creation we could construct an iterator here and avoid both vecs. Untested but something like
let sled_agent_clients = sleds.into_iter().map(|s| {
sled_agent_client::Client::new(
&format!("http://{}", s.address()),
log.clone(),
)
}); // no collect()which is then consumed by the loop below that uses the clients?
| // Check the sim sled-agent's in-memory bootstore was actually updated, | ||
| // confirming it was successfully contacted. | ||
| let bootstore_generation = ctx | ||
| .first_sled_agent() |
There was a problem hiding this comment.
Do we know that in the nexus-test / sim-sled-agent case, the first sled-agent is always going to be updated? This test has two sled-agents (from #[nexus_test(extra_sled_agents = 1)]) - even if the sim-sled-agents do bootstore replication (no idea if they do), this test might be flaky if the task sent the request to the second sled-agent then we look at the first one here before it replicates.
Maybe we should check both sled agents and succeed if either of them have the correct generation?
There was a problem hiding this comment.
I don't think the sim sled-agents are doing bootstore replication (which now that I think of it, this is going to be a problem for JBOR, because we need both scrimlet sled agents to get the updates so we can do proper integration testing)
There was a problem hiding this comment.
bringing an out-of-band convo here:
We will update this to fetch all sleds from inventory, filter on is_scrimlet(), and apply the updates to all scrimlets. The test should assert that both scrimlet sled agents have received the update.
This is similar to the current production behavior and will ensure that we can get a reliable test and dev environment working.
| .unwrap() | ||
| .generation; | ||
| assert!( | ||
| bootstore_generation > 0, |
There was a problem hiding this comment.
Can we check for the exact generation the task should have written? Even if sim-sled-agent starts out at generation 0 today (which seems a little weird / surprising), a future change could have it start out at 1, which means this test would always pass even if the task breaks.
|
@jgallagher I believe the latest commit hits all of the things you called out, let me know what you think |
| // confirming both scrimlets were successfully contacted. | ||
| for (i, sled_agent) in ctx.sled_agents.iter().enumerate() { | ||
| let sled_agent = sled_agent.sled_agent().clone(); | ||
| wait_for_condition( |
There was a problem hiding this comment.
Maaaaybe worth pulling this out into a helper function since we have it twice? wait_for_sled_agent_bootstore_generation(&sled_agent, 4).await or something like that? I don't feel strongly though, up to you.
2 participants
An alternative fix to #10958 (first attempt at a fix is #11172)
Instead of deriving sled agent addresses from switch zone service addresses (which does not work in the test context), pull the sled agent information from the inventory records in the database.