[sled-agent][sim] Use a shared support bundle implementation#7264
Conversation
smklein
changed the title
wfchandler
left a comment
wfchandler
left a comment
There was a problem hiding this comment.
Thanks @smklein, this wrapper trait seems reasonable to me. Two related questions around how we're walking the path of nested datasets.
| // Final component of path -- remove it if it exists. | ||
| if !path_component.contains('/') { | ||
| if nested_dataset.children.remove(path_component).is_none() { | ||
| return Err(HttpError::for_not_found( | ||
| None, | ||
| "Nested Dataset not found".to_string(), | ||
| )); | ||
| }; | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
As above, no component will contain /, so we'll always exit early and fail to iterate past the first component.
There was a problem hiding this comment.
Good catch - patched and tested (see other comment)
| // Final component of path -- insert it here if it doesn't exist | ||
| // already. | ||
| if !path_component.contains('/') { | ||
| let entry = | ||
| nested_dataset.children.entry(path_component.to_string()); | ||
| entry | ||
| .and_modify(|storage| { | ||
| storage.config = config.clone(); | ||
| }) | ||
| .or_insert_with(|| { | ||
| NestedDatasetStorage::new( | ||
| &zpool_root, | ||
| config.name.root, | ||
| nested_path, | ||
| config.inner, | ||
| ) | ||
| }); | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
This condition will always evaluate to true since we're splitting path_component on /, so we'll never walk the full path.
There was a problem hiding this comment.
Good catch; this wasn't an issue for paths without /, which I prodded in #7063 , but clearly is an issue for all other nested datasets.
I've patched this, and added some tests.
wfchandler
left a comment
wfchandler
left a comment
There was a problem hiding this comment.
Thanks @smklein, this looks nice, just one stylistic suggestion.
| )); | ||
| }; | ||
|
|
||
| let mut path_components = nested_path.split('/').peekable(); |
There was a problem hiding this comment.
Personally I think this is a bit easier to read as an Iterator::last, but feel free to leave this as-is if you prefer.
if let Some(path_component) = nested_path.split('/').filter(|p| !p.is_empty()).last() {
...
}There was a problem hiding this comment.
I'm not sure this is the same as the existing code - the current version of the code uses a while loop to iterate through children.
For example, if one creates a debug dataset, and within that debug dataset, creates:
foo
and then later createsfoo/bar
I expect that the foo/bar is a dataset contained within the foo dataset.
Basically - if I take this suggestion, the following code never executes, which means we don't actually iterate through child datasets:
omicron/sled-agent/src/sim/storage.rs
Lines 1424 to 1432 in ac36cf6
There was a problem hiding this comment.
This also causes the test I added - nested_dataset_child_parent_relationship - to fail with:
thread 'sim::storage::test::nested_dataset_child_parent_relationship' panicked at sled-agent/src/sim/storage.rs:870:14:
Should have failed to provision foo/bar before foo
2 participants
PR 3 / ???
This PR aims to re-use the support bundle management logic in sled-agent/src/support_bundle/storage.rs for both the real and simulated sled agent.
It accomplishes this goal with the following:
LocalStorage, that abstracts access to storage. The "real" sled agent accesses real storage, the simulated sled agent can access the simulated storage APIs..awaitcalls throughout Omicron.As an end result of this PR, tests in subsequent PRs (e.g. #7063) can rely on the simulated sled agent to respond realistically to support bundle requests, rather than using a stub implementation.