[nexus] Add tests for instance_create unwind safety #1843
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
smklein
commented
Oct 20, 2022
common/Cargo.toml
Outdated
| @@ -25,7 +25,7 @@ serde_json = "1.0" | |||
| serde_with = "2.0.1" | |||
| slog = { version = "2.5", features = [ "max_level_trace", "release_max_level_debug" ] } | |||
| smf = "0.2" | |||
| steno = "0.2" | |||
| steno = { git = "https://github.com/oxidecomputer/steno", branch = "node-access" } | |||
There was a problem hiding this comment.
Ideally, I'd use a new rev of steno instead before merging. Same elsewhere in this PR
smklein
commented
Oct 20, 2022
| let params = new_test_params(&opctx, project_id); | ||
| let dag = create_saga_dag::<SagaInstanceCreate>(params).unwrap(); | ||
|
|
||
| for i in 0..dag.get_node_count() { |
There was a problem hiding this comment.
This is the "more automated" version of what I tried doing in #1835 - I can definitely consolidate the testing here, and create a "saga unwind tester", if that would be useful.
I basically see three phases:
- Setup the saga. Create any orgs / projects / disks / etc needed to run the saga.
- Iterate over every single node in the DAG, injecting failures at each one. Log which ones we're working on, to make it clear what went wrong if an error occurs. Run the saga.
- Assert that no detritus remains, and that the system is in the same state before / after saga execution.
This was referenced Oct 20, 2022
leftwo
approved these changes
Oct 21, 2022
Not actually. There is definitely still a dependency, but the "test setup" part can be re-used with Nexus tests without causing any real circular dependency issues. See #1835 (comment) for an example of one such issue. Frankly the rest of nexus-test-utils probably *should* continue to be refactored such that there is no real dependency on Nexus; doing so will reduce the risk of "mis-matched versions of Nexus" classes of errors in the future. This required the following changes: - Move `nexus`'s config information into `common/src/nexus_config.rs`, so that it can be used by `nexus-test-utils`. - Make `nexus-test-utils` operate on a generic version of `NexusServer`, exposed by a new crate named `nexus-test-interface`. This crate can contain any information used to operate Nexus during shared test setup, without actually requiring a full dependency on `nexus`. - Note, adding this generic required making `ControlPlaneTestContext` generic, which ended up bubbling up to a lot of test setup functions, including the one used in the `#[nexus_test]` macro... - ... So, make it possible to customize which version of `NexusServer` is used by `#[nexus_test]`. This means callers can supply their *own* version of `NexusServer`, either from their own crate (see: Nexus unit tests) or the version from the `omicron_nexus` crate (see: Nexus integration tests).
13 tasks
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.
Builds on #1835 and oxidecomputer/steno#67
Adds a test for the "instance create" saga's unwind safety by: