Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add initial faux-mgs dev tool #1526

Merged
merged 3 commits into from
Aug 2, 2022
Merged

Add initial faux-mgs dev tool #1526

merged 3 commits into from
Aug 2, 2022

Conversation

jgallagher
Copy link
Contributor

faux-mgs is a standalone CLI app to poke SPs. It's primarily intended as a development tool to support hubris SP work, but at the moment I think it makes sense for it to live alongside the real MGS.

@jgallagher jgallagher requested a review from andrewjstone August 1, 2022 14:40
andrewjstone
andrewjstone approved these changes Aug 2, 2022
View reviewed changes
Copy link
Contributor

@andrewjstone andrewjstone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. It's pretty straightforward, barring all the termios chicanery.
I'm curious why you ended up using mio directly and not tokio, but not enough to hold up merging a dev tool.

gateway/faux-mgs/src/main.rs
addr: SocketAddrV6,
kind: RequestKind,
) -> Result<u32> {
static REQUEST_ID: AtomicU32 = AtomicU32::new(1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this atomic? For mutability? I assume you made it static so you wouldn't have to pass it as a parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, just for simplicity - if this function sends all packets, this allows it to keep its own monotonically increasing counter, freeing the caller up from worrying about it.

gateway/faux-mgs/src/usart.rs
debug!(log, "received unexpected ack");
}
}
Ok(other) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to imply that we are only handling responses from the SP. Does the SP ever send async messages?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It definitely can, and MGS proper has a lot more machinery to handle that. faux-mgs may eventually need to handle this better, but for now the only async messages the SP can send contain serial console data, which it won't send unless an MGS instance has already attached by sending it data first.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Makes sense.

@jgallagher
Copy link
Contributor Author

jgallagher commented Aug 2, 2022 •
edited
Loading

Yeahhh, I should probably go back and replace mio with tokio. I was ~90% done before I realized I needed timeouts when reading from stdin, and grabbed mio as a smaller change to implement that. I'll go ahead and merge for now but plan on updating that next time I'm in here making changes.

Thanks for the review!

@andrewjstone
Copy link
Contributor

Yeahhh, I should probably go back and replace mio with tokio. I was ~90% done before I realized I needed timeouts when reading from stdin, and grabbed mio as a smaller change to implement that. I'll go ahead and merge for now but plan on updating that next time I'm in here making changes.

Thanks for the review!

Honestly, I don't think it's urgent at all to convert. If you have time and feel like it feel free, but I'm certainly not going to complain if we use mio forever on this.

@jgallagher jgallagher merged commit 356c449 into main Aug 2, 2022
@jgallagher jgallagher deleted the faux-mgs branch August 2, 2022 20:21
leftwo pushed a commit that referenced this pull request Nov 19, 2024
Update Crucible to latest
962ac94 
No Propolis changes other than to update Crucible

Crucible changes are:
Add debug/timeout to test_memory.sh (#1563)
Consolidate ack checking (#1561)
Rename for crutest: RegionInfo -> DiskInfo (#1562)
Fix dtrace system level scripts (#1560)
Remove `ackable_work`; ack immediately instead (#1552)
No more New jobs, no more New jobs column (#1559)
Remove delay-based backpressure in favor of explicit queue limits (#1515)
Only send flushes when Downstairs is idle; send Barrier otherwise (#1505)
Update Rust crate reqwest to v0.12.9 (#1536)
Update Rust crate omicron-zone-package to 0.11.1 (#1535)
Remove separate validation array (#1522)
Remove more unnecessary `DsState` variants (#1550)
Consolidate `DownstairsClient::reinitialize` (#1549)
Update Rust crate uuid to v1.11.0 (#1546)
Update Rust crate reedline to 0.36.0 (#1544)
Update Rust crate bytes to v1.8.0 (#1541)
Update Rust crate thiserror to v1.0.66 (#1539)
Update Rust crate serde_json to v1.0.132 (#1538)
Update Rust crate serde to v1.0.214 (#1537)
Remove transient states in `DsState` (#1526)
Update Rust crate libc to v0.2.161 (#1534)
Update Rust crate futures to v0.3.31 (#1532)
Update Rust crate clap to v4.5.20 (#1531)
Update Rust crate async-trait to 0.1.83 (#1530)
Update Rust crate anyhow to v1.0.92 (#1529)
Remove obsolete crutest perf test (#1528)
Update dependency rust to v1.82.0 (#1512)
Still more updates to support Volume layer activities. (#1508)
Remove remaining IOPS/bandwidth limiting code (#1525)
Add unit test for VersionMismatch (#1524)
Removing panic paths by only destructuring once (#1523)
Update actions/checkout digest to 11bd719 (#1518)
Switch to using `Duration` for times (#1520)
@leftwo leftwo mentioned this pull request Nov 19, 2024
Merged
leftwo added a commit that referenced this pull request Nov 20, 2024
@leftwo
Update Crucible to latest (#7101)
ac3fb34 
No Propolis changes other than to update Crucible

Crucible changes are:
Add debug/timeout to test_memory.sh (#1563)
Consolidate ack checking (#1561)
Rename for crutest: RegionInfo -> DiskInfo (#1562) Fix dtrace system
level scripts (#1560)
Remove `ackable_work`; ack immediately instead (#1552) No more New jobs,
no more New jobs column (#1559)
Remove delay-based backpressure in favor of explicit queue limits
(#1515) Only send flushes when Downstairs is idle; send Barrier
otherwise (#1505) Update Rust crate reqwest to v0.12.9 (#1536)
Update Rust crate omicron-zone-package to 0.11.1 (#1535) Remove separate
validation array (#1522)
Remove more unnecessary `DsState` variants (#1550) Consolidate
`DownstairsClient::reinitialize` (#1549) Update Rust crate uuid to
v1.11.0 (#1546)
Update Rust crate reedline to 0.36.0 (#1544)
Update Rust crate bytes to v1.8.0 (#1541)
Update Rust crate thiserror to v1.0.66 (#1539)
Update Rust crate serde_json to v1.0.132 (#1538)
Update Rust crate serde to v1.0.214 (#1537)
Remove transient states in `DsState` (#1526)
Update Rust crate libc to v0.2.161 (#1534)
Update Rust crate futures to v0.3.31 (#1532)
Update Rust crate clap to v4.5.20 (#1531)
Update Rust crate async-trait to 0.1.83 (#1530)
Update Rust crate anyhow to v1.0.92 (#1529)
Remove obsolete crutest perf test (#1528)
Update dependency rust to v1.82.0 (#1512)
Still more updates to support Volume layer activities. (#1508) Remove
remaining IOPS/bandwidth limiting code (#1525) Add unit test for
VersionMismatch (#1524)
Removing panic paths by only destructuring once (#1523) Update
actions/checkout digest to 11bd719 (#1518)
Switch to using `Duration` for times (#1520)

Co-authored-by: Alan Hanson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewjstone andrewjstone andrewjstone approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jgallagher @andrewjstone