falcon-lab: separate topologies from scenarios - #898
Conversation
cfzimmerman
left a comment
cfzimmerman
left a comment
There was a problem hiding this comment.
Looks like a nice improvement, but I don't have the context to make a full judgement on design. That said, if a CI test PR passes CI tests, that seems positive.
|
|
||
| #[test] | ||
| fn run_options_follow_scenario() { | ||
| let cli = Cli::try_parse_from([ |
There was a problem hiding this comment.
cool, TIL you can test clap like this.
| #[test] | ||
| fn deployment_names_are_falcon_compatible() { |
There was a problem hiding this comment.
In general, it would be cool if we could use types to make this test unnecessary. Although fair if that seems unnecessary effort/complexity.
But if falcon imposes constraints like this, it might behoove us to put that logic in falcon, document why it's imposed, and export it for clients to use.
There was a problem hiding this comment.
Yeah, let me double check. I thought this was a limitation stemming from falcon, but now I'm questioning my recollection :D
There was a problem hiding this comment.
Yeah, the limitation is in illumos, which libfalcon is sort of the shepherd of for these kinds of setups... so I think the type should be added to libfalcon and then trickled down to falcon-lab. I'll make a note to submit a PR to libfalcon to use better types + a separate follow-on to fixup maghemite to use the better types.
There was a problem hiding this comment.
Just pushed an update via jj-stack that replaces this test with a compile-time assertion. This is meant as a stop-gap between now and when the libfalcon changes are made + incorporated into falcon-lab.
nicolaskagami
left a comment
nicolaskagami
left a comment
There was a problem hiding this comment.
Very nice
✅ Improves our abstractions
✅ Clear intent
✅ Well compartmentalized changes
I think there's some opportunity for simplification in this code (e.g. RunTopology/CleanupTopology could be generics, and perhaps we could lose boilerplate matching scenarios to topologies as well), but it's fine the way it is.
Refactor falcon-lab to have separate abstractions for Topologies and Scenarios. A Topology is a set of nodes, their backend (VM image), and the links interconnecting them. A Scenario is layered atop a Topology, describing the configuration, diagnostics, and test cases being applied. The old run targets now look like this: - mgd-unnumbered -> (mgd-duo, bgp-unnumbered) - quartet-unnumbered -> (interop, bgp-unnumbered) - quartet-bfd-static-routing -> (interop, bfd-static-routing) Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
Revision history
Generated by jj-stack from this pull request's force-push history. |
a0af47b to
1cca630
Compare
Thanks! I took your advice and made things more generic, which even resulted in the falcon-lab |
3 participants
Refactor falcon-lab to have separate abstractions for Topologies and Scenarios. A Topology is a set of nodes, their backend (VM image), and the links interconnecting them. A Scenario is layered atop a Topology, describing the configuration, diagnostics, and test cases being applied.
The old run targets now look like this:
Signed-off-by: Trey Aspelund trey@oxidecomputer.com