-
Notifications
You must be signed in to change notification settings - Fork 54
Replace bootstrap-agent dropshot server with sprockets session #1173
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. It's awesome how easy an async sprockets session is to use when it implements AsyncRead/AsyncWrite. Really happy we made that change!
sled-agent/src/bootstrap/params.rs
Outdated
#[derive(Clone, Copy, Debug, Serialize_repr, Deserialize_repr, PartialEq)] | ||
#[repr(u32)] | ||
pub enum Version { | ||
V1 = 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest not using an enum here with such a large repr. The problem is that if we really have many versions, each one will have to be maintained in this enum, even if our code no longer supports that version. Otherwise, it won't deserialize successfully if we are talking to an unsupported version and we won't report what version it's looking for. It's probably easier to just use a u32 or newtype.
The other option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to switch this to a plain u32
, although it looks like your comment got cut off partway through. What's the other option? 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha, probably left over from something I started typing and thought I deleted. There are probably other options, but u32 is my suggestion :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switched in e076dd6
.await | ||
.map_err(Error::WriteLengthPrefix)?; | ||
stream.write_all(&buf).await.map_err(Error::WriteRequest)?; | ||
stream.flush().await.map_err(Error::FlushRequest)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice flush 😆
// allocating based on the length prefix they send, so it should be fine to | ||
// be a little sloppy here and just pick something far larger than we ever | ||
// expect to see. | ||
const MAX_REQUEST_LEN: u32 = 128 << 20; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe share this between client and server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I quasi-intentionally picked different sizes here, although maybe that was being too clever. Requests will certainly be larger than responses in general (especially as the init request grows to include the list of devices in the quorum), but in practice we don't expect this bound to ever be hit in either case. I have a slight preference for keeping them separate only because it allows them to be local to their respective functions, but if you'd rather I share one I'm fine with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I didn't even notice they were different. I'm fine with this as is. Carry on!
Removes sprockets proxies, fixing #1161.
ae75789
to
e076dd6
Compare
Removes sprockets proxies, fixing #1161.
The messages sent inside the sprockets session are
[u32-length-prefix | u32-version-number | json]
.This PR does not replace the trust quorum client/server that performs share collection to rebuild the rack secret, but that will come in a followup PR; the intent is that
request_share
(currently a placeholder, and marked with#[allow(dead_code)]
as of this PR) will become not-a-placeholder, and it will be served inside the sprockets session instead of shares being fetched via SPDM.Minor notes on testing:
(The double logs are because we're seeing both the client and server note that no sprockets are in play.)
sled 0 (running RSS, simulated SP serial 1000...)
sled 1 (simulated SP serial 2000...)