Skip to content

MGS: Rework serialization of large-data-bearing packets#1645

Merged
jgallagher merged 12 commits into
mainfrom
mgs-serialization-rework
Aug 25, 2022
Merged

MGS: Rework serialization of large-data-bearing packets#1645
jgallagher merged 12 commits into
mainfrom
mgs-serialization-rework

Conversation

@jgallagher

@jgallagher jgallagher commented Aug 24, 2022

Copy link
Copy Markdown
Contributor

Prior to this PR, we had two variants of MGS <-> SP messages that had large, fixed-size arrays in them (SerialConsole for serial console data and UpdateChunk for a part of an update image). This had a few undesirable properties:

  1. Memory usage on the SP was higher than it needed to be due to needing to instantiate an instance of these types and then copy the data into the arrays during deserialization; it should've been able to refer to the data as a borrowed slice from the receive buffer. hubpack is specifically designed with this in mind.
  2. We had to suppress clippy's large enum variant warning for similar reasons.
  3. Serializing these packets had some clever machinery to keep from sending zero-filled, full length arrays in every UDP message, but (a) clever serde code is not the most readable and (b) network packet size is not as much of a concern as SP memory usage.

This PR changes the messaging format to allow for packets to be immediately followed by (length-prefixed) binary data, which allows the SP to deserialize only the packet header and then borrow the binary data. This is most obvious in the change to the SpHandler trait. Before this PR, the serial_console_write() and update_chunk() methods took SerialConsole/UpdateChunks by value which contained large arrays; now they receive arguments for the metadata and a separate argument for the data (a &[u8] which is borrowed from the UDP receive buffer).

In the process of testing these changes I ran into a nasty async bug in MGS's serial console forwarding; it's fixed as part of this PR in a6b9659.

@jgallagher
UpdateChunk: Serialize data immediately after hubpack header
1de5ca3
@jgallagher
SerialConsole: Serialize data immediately after hubpack header
5a93000
@jgallagher
Update downstream crates with serialization changes
a467761
@jgallagher
Clippy-guided cleanup
e403eef
@jgallagher
Update faux-mgs with serialization changes
09a2b83
@jgallagher
Fix buffer sizes
d565469
@jgallagher
Fix async bug(s) in MGS serial console forwarding
a6b9659 
Our `select!` loop was pulling enqueued data out of in-memory buffers,
but then if the future they went to was cancelled by the `select!` the
data was lost. This splits the forwarding up into three separate tokio
tasks, and leaves the `select!` loop only polling on cancel-safe futures
that don't lose any data if dropped.
@jgallagher
cargo fmt
4cd2319
@jgallagher
Remove last usage of variable_packet serialization
21af520
@jgallagher
Comment cleanup
8610138
@jgallagher jgallagher mentioned this pull request Aug 25, 2022
Merged
andrewjstone
andrewjstone approved these changes Aug 25, 2022
View reviewed changes

@andrewjstone andrewjstone left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is quite an improvement! Love the net-negative line count.

Mostly just nits around naming from me.

Comment thread gateway-messages/src/lib.rs
pub struct UpdateChunk {
/// Offset in bytes of this chunk from the beginning of the update data.
pub offset: u32,
/// Length in bytes of this chunk.

@andrewjstone andrewjstone Aug 25, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👏

Comment thread gateway-messages/src/lib.rs
// unpopulated? Maybe this message shouldn't be variable at all. For now we
// leave it like it is; it's certainly "variable" in the sense that our
// simulated racks for tests have fewer than 36 targets.
// TODO-cleanup Is it okay to hard code this number to what we know the

@andrewjstone andrewjstone Aug 25, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Love to remove many questions!

Comment thread gateway-messages/src/lib.rs Outdated
Comment thread gateway-messages/src/lib.rs Outdated
Comment thread gateway-messages/src/lib.rs
// our serialized message headers haven't gotten too large. The specific value
// here is arbitrary; if this check starts failing, it's probably fine to reduce
// it some. The check is here to force us to think about it.
const_assert!(MAX_SERIALIZED_SIZE - Request::MAX_SIZE > 700);

@andrewjstone andrewjstone Aug 25, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CLEVER

Comment thread gateway-sp-comms/src/single_sp.rs Outdated
Comment thread gateway/faux-mgs/src/usart.rs Outdated
@jgallagher jgallagher enabled auto-merge (squash) August 25, 2022 18:07
@jgallagher jgallagher merged commit 631e6d7 into main Aug 25, 2022
@jgallagher jgallagher deleted the mgs-serialization-rework branch August 25, 2022 19:01
@jgallagher jgallagher mentioned this pull request Aug 25, 2022
Merged
leftwo pushed a commit that referenced this pull request Mar 26, 2025
Update crucible and propolis
1234576 
Crucible changes are:
Make `ImpactedBlocks` an absolute range (#1685)
update omicron (#1687)
Update Rust crate chrono to v0.4.40 (#1682)
Update Rust crate tempfile to v3.19.0 (#1676)
Log loops during crutest replay (#1674)
Refactor live-repair start and send (#1673)
Update Rust crate libc to v0.2.171 (#1684)
Update Rust crate clap to v4.5.32 (#1683)
Update Rust crate async-trait to 0.1.88 (#1680)
Update Rust crate bytes to v1.10.1 (#1681)
Update Rust crate anyhow to v1.0.97 (#1679)
Update Rust crate http to v1 (#1678)
Update Rust crate tokio-util to v0.7.14 (#1675)
Update Rust crate thiserror to v2 (#1657)
Update Rust crate omicron-zone-package to 0.12.0 (#1647)
Update Rust crate opentelemetry to 0.28.0 (#1543)
Update Rust crate itertools to 0.14.0 (#1646)
Update Rust crate clearscreen to v4 (#1656)
nightly test polish (#1665)
Update Rust crate strum_macros to 0.27 (#1654)
Update Rust crate strum to 0.27 (#1653)
Update Rust crate rusqlite to 0.34 (#1652)
Better return semantics from should_send
Trying to simplify enqueue
Remove unnecessary argument
Remove unused code from `ImpactedBlocks` (#1668)
Log when pantry/volume layers activate things (#1670)
Fix unused import (#1669)
Use `RangeSet` instead of tracking every complete job (#1663)
Update Rust crate dropshot to 0.16.0 (#1645)
Simplify pending work tracking (#1662)
Remove rusqlite dependency from crucible-common (#1659)
Update Rust crate reedline to 0.38.0 (#1651)
Update Rust crate proptest to 1.6.0 (#1648)
Update Rust crate bytes to v1.10.0 (#1644)
Update Rust crate tracing-subscriber to 0.3.19 (#1643)
Update Rust crate tracing to v0.1.41 (#1642)
Update Rust crate toml to v0.8.20 (#1641)
Update Rust crate thiserror to v1.0.69 (#1640)
Update Rust crate serde_json to v1.0.139 (#1639)
Update Rust crate serde to v1.0.218 (#1638)
Update Rust crate reqwest to v0.12.12 (#1637)
Update Rust crate libc to v0.2.170 (#1636)
Update Rust crate indicatif to 0.17.11 (#1635)
Update Rust crate httptest to 0.16.3 (#1634)
Update Rust crate clap to v4.5.31 (#1632)
Update Rust crate chrono to v0.4.39 (#1631)
Update Rust crate byte-unit to 5.1.6 (#1630)
Update Rust crate async-trait to 0.1.86 (#1629)
Update Rust crate csv to 1.3.1 (#1633)
Update Rust crate anyhow to v1.0.96 (#1628)
Fix a test flake (#1626)
Bitpack per-block slot data in memory (#1625)
Use `div_ceil` instead of `(x + 7) / 8` (#1624)
Add repair server dynamometer (#1618)

Propolis changes are:
Paper over minor() differences
Update crucible (#886)
Test oximeter metrics end to end (#855)
server: improve behavior of starting VMs that are waiting for Crucible activation (#873)
server: extend API request queue's memory of prior requests (#880)
Use production propolis-server flags in PHD CI builds (#878)

Added a new field to the Board struct that propolis requires.
@leftwo leftwo mentioned this pull request Mar 26, 2025
Merged
leftwo added a commit that referenced this pull request Mar 27, 2025
@leftwo
Update crucible and propolis (#7877)
becb5eb 
Crucible changes are:
Make `ImpactedBlocks` an absolute range (#1685)
update omicron (#1687)
Update Rust crate chrono to v0.4.40 (#1682)
Update Rust crate tempfile to v3.19.0 (#1676)
Log loops during crutest replay (#1674)
Refactor live-repair start and send (#1673)
Update Rust crate libc to v0.2.171 (#1684)
Update Rust crate clap to v4.5.32 (#1683)
Update Rust crate async-trait to 0.1.88 (#1680)
Update Rust crate bytes to v1.10.1 (#1681)
Update Rust crate anyhow to v1.0.97 (#1679)
Update Rust crate http to v1 (#1678)
Update Rust crate tokio-util to v0.7.14 (#1675)
Update Rust crate thiserror to v2 (#1657)
Update Rust crate omicron-zone-package to 0.12.0 (#1647) Update Rust
crate opentelemetry to 0.28.0 (#1543)
Update Rust crate itertools to 0.14.0 (#1646)
Update Rust crate clearscreen to v4 (#1656)
nightly test polish (#1665)
Update Rust crate strum_macros to 0.27 (#1654)
Update Rust crate strum to 0.27 (#1653)
Update Rust crate rusqlite to 0.34 (#1652)
Better return semantics from should_send
Trying to simplify enqueue
Remove unnecessary argument
Remove unused code from `ImpactedBlocks` (#1668)
Log when pantry/volume layers activate things (#1670) Fix unused import
(#1669)
Use `RangeSet` instead of tracking every complete job (#1663) Update
Rust crate dropshot to 0.16.0 (#1645)
Simplify pending work tracking (#1662)
Remove rusqlite dependency from crucible-common (#1659) Update Rust
crate reedline to 0.38.0 (#1651)
Update Rust crate proptest to 1.6.0 (#1648)
Update Rust crate bytes to v1.10.0 (#1644)
Update Rust crate tracing-subscriber to 0.3.19 (#1643) Update Rust crate
tracing to v0.1.41 (#1642)
Update Rust crate toml to v0.8.20 (#1641)
Update Rust crate thiserror to v1.0.69 (#1640)
Update Rust crate serde_json to v1.0.139 (#1639)
Update Rust crate serde to v1.0.218 (#1638)
Update Rust crate reqwest to v0.12.12 (#1637)
Update Rust crate libc to v0.2.170 (#1636)
Update Rust crate indicatif to 0.17.11 (#1635)
Update Rust crate httptest to 0.16.3 (#1634)
Update Rust crate clap to v4.5.31 (#1632)
Update Rust crate chrono to v0.4.39 (#1631)
Update Rust crate byte-unit to 5.1.6 (#1630)
Update Rust crate async-trait to 0.1.86 (#1629)
Update Rust crate csv to 1.3.1 (#1633)
Update Rust crate anyhow to v1.0.96 (#1628)
Fix a test flake (#1626)
Bitpack per-block slot data in memory (#1625)
Use `div_ceil` instead of `(x + 7) / 8` (#1624)
Add repair server dynamometer (#1618)

Propolis changes are:
Paper over minor() differences
Update crucible (#886)
Test oximeter metrics end to end (#855)
server: improve behavior of starting VMs that are waiting for Crucible
activation (#873) server: extend API request queue's memory of prior
requests (#880) Use production propolis-server flags in PHD CI builds
(#878)

---------

Co-authored-by: Alan Hanson <alan@oxide.computer>
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