Conversation
| pub async fn collect( | ||
| collection: &BundleCollection, | ||
| dir: &Utf8Path, | ||
| ) -> anyhow::Result<CollectionStepOutput> { |
There was a problem hiding this comment.
This command is currently being executed unconditionally; I'm interested in tweaking https://github.com/oxidecomputer/omicron/blob/main/nexus/src/app/background/tasks/support_bundle/request.rs#L21-L51 but was interested in feedback here...
- I could add a category for "omdb" commands. That seems easy. But the downside here is that "omdb commands can check a lot of different data"?
- So the alternative would be: Should I be changing the set of sub-commands we execute here, based on the BundleRequest? e.g.: should we have a category for "Nexus" (including background tasks, quiescing state, sagas), a category for "Updates" (including mgs-updates, update-status) and continue to have other commands be controlled by other more specific bundle requests?
TL;DR:
- I could have this be lumped under the broad "omdb" category
- Or I could split it up into different categories, depending on "what happens to be queried", and let the fact that it's queried by omdb be kinda an implementation detail
.config/nextest.toml
Outdated
| [[profile.default.scripts]] | ||
| # Build omdb for usage within Nexus integration tests. | ||
| # This was initially added for the support bundle integration tests. | ||
| filter = 'rdeps(nexus-test-utils) - package(omicron-dev) - package(omicron-live-tests)' |
There was a problem hiding this comment.
If I don't have access to an omdb binary, it's pretty hard to validate that the commands issues by the support bundle are actually valid.
nexus/test-utils/src/starter.rs
Outdated
| .clone(), | ||
| }; | ||
|
|
||
| // Configure the omdb binary path for tests. |
There was a problem hiding this comment.
This feels a little sketchy to me - it depends on the changes I made in config/nextest.toml - so if there's something more resilient I could / should be doing here, I'm open to it.
There was a problem hiding this comment.
Thoughts on using this -dup pattern? Basically build another copy of omdb for use within nexus's integration tests.
There was a problem hiding this comment.
The other option, and maybe the one we use more commonly, is to make the relevant tests live in the omdb crate.
There was a problem hiding this comment.
I think I'd prefer the -dup option (will look into it) - this is one spot Nexus calls omdb, but now that Nexus expects to "Find omdb at a path, and take that as input argument" it seems totally reasonable for Nexus tests to depend on omdb.
The reverse seems like a mess - omdb depending on the entirety of Nexus' test utils seems like a really heavy dependency for omdb to add?
There was a problem hiding this comment.
Yeah that does seem to be a pretty large dependency. That's kind of why we did this -dup thing for reconfigurator-cli, because most tests were purely in memory and we didn't want to have to compile all the db code each iteration.
There was a problem hiding this comment.
Yeah that does seem to be a pretty large dependency. That's kind of why we did this -dup thing for reconfigurator-cli, because most tests were purely in memory and we didn't want to have to compile all the db code each iteration.
There was a problem hiding this comment.
I gave this a shot in adc037a
Basically:
- Split up omdb into library/binary
- Adds
omdb-dupas a binary in nexus - Find the path to it in the
nexus_testmacro - Remove all the nextest scripts / workspace-based path navigation I was doing previously
There was a problem hiding this comment.
This is breaking ls-apis for a couple reasons:
- I implemented adc037a by creating an
omdb-dupbinary in thenexuscrate, which depends on the omdb crate as adependency - This creates a problematic API dependency:
Nexus Internal Lockstep API (client: nexus-lockstep-client)
consumed by: omicron-nexus (omicron/nexus) via 1 path
error: unexpected dependency
consumed by: omicron-sled-agent (omicron/sled-agent) via 1 path
status: expected, reason: Only RSS uses this API and it is not part of upgrade
I can't just make the omdb dependency a dev-dependency, because I don't think I have a way to build it as a "dev-dependency [[bin]]" target. I think nexus-reconfigurator-cli-integration-tests gets around this by being built as a totally distinct crate.
I think I might need to do the same here - build all nexus integration tests as their own crate to make this work?
|
|
||
| files_checked += 1; | ||
|
|
||
| // Validate that the omdb command is valid, even if it can't connect |
There was a problem hiding this comment.
I validated this manually - typo-ing the spelling of omdb nexus as omdb nah from the support bundle; I see this as an output, and the assertion gets tripped
2 participants
Adds a handful of omdb commands to support bundle collection.
Additionally, expands support bundle integration testing to ensure that commands are
actually valid omdb subcommands, even if they aren't executing against a real system.
Fixes #9509