EarlyNetworkConfigBody: make rack_network_config non-optional#9948
EarlyNetworkConfigBody: make rack_network_config non-optional#9948jgallagher wants to merge 5 commits intojohn/bootstore-versioningfrom
EarlyNetworkConfigBody: make rack_network_config non-optional#9948Conversation
Additional changes: * Remove `EarlyNetworkConfigBody::ntp_servers` (unused). * Flesh out instructions and implementation for adding new `EarlyNetworkConfigBody` versions.
jgallagher
changed the title
EarlyNetworkConfigBody: make rack_netork_config non-optionalEarlyNetworkConfigBody: make rack_network_config non-optional
Yeah, this is the case Dave and I discussed in the update watercooler today -- we don't consider API versions blessed until they land on main so they won't be converted to stubs. In this PR there's a change to v24 (the operation ID change), which wouldn't be allowed once you're on main. Once the previous PR lands on main, v24 will become a Git stub. You can preview what happens with: (Though trying this out locally made me realize that with our RFD 619 recommendation to update the operation ID, the API document will be different, and we'll ask you to restore the blessed version from Git instead of doing it ourselves -- this is an expected case which we should handle better. Will fix -- edit: fixed in #9947). |
Apologies for the super quick update after version 0.4.0, but the speedup from parallelism is quite nice: on my 16c/32t machine, `cargo xtask openapi check` goes from ~9.5s to just 2.15s, with much of the remaining time being taken up by the Cargo build itself. Version 0.5.1 has a fix for the scenario outlined in #9948 (comment).
sled-agent/types/versions/src/impls/early_networking/early_network_config_serialization.rs
Outdated
Show resolved
Hide resolved
| //! bootstore and CRDB. Many of these requirements are enforced statically by | ||
| //! the macros in this module. | ||
| //! | ||
| //! To add a new `EarlyNetworkConfigBody` version: |
There was a problem hiding this comment.
Great comment!
Is this something that we need to abstract out, or should abstract out at some point? Seems like a useful pattern though I'm not sure how broadly applicable it is.
There was a problem hiding this comment.
Maybe! This is pretty similar to the machinery for managing the ledgered OmicronSledConfigs, except there we still don't have an envelope specifying the version (so we have to try deserializing all the formats). When we move that to an envelope, maybe we can extract something common between the two?
andrewjstone
left a comment
andrewjstone
left a comment
There was a problem hiding this comment.
I mostly get the gist of those macros ;) Nice work.
| resolver: Resolver, | ||
| rx_blueprint: watch::Receiver<Option<LoadedTargetBlueprint>>, | ||
| ) -> Self { | ||
| Self { datastore, resolver, rx_blueprint } |
There was a problem hiding this comment.
Amazing that you have managed to get rid of reading from the bootstore and blueprint!
| .. | ||
| } = &self.inner.sled_info.get().ok_or(Error::SledAgentNotReady)?; | ||
|
|
||
| let Some(rack_network_config) = rack_network_config.as_ref() else { |
| @@ -5,10 +5,52 @@ | |||
| //! Support for deserializing the body stored inside an | |||
| //! [`EarlyNetworkConfigEnvelope`], as determined by the envelope's metadata | |||
| //! (particularly, [`EarlyNetworkConfigEnvelope::schema_version`]. | |||
| //! | |||
3 participants
The main thrust of this PR is that it gives us a coherent versioning story for the types kept in the bootstore. There's a block comment in the
sled-agent/types/versions/src/impls/early_networking/early_network_config_serialization.rsmodule that explains the steps necessary to add a newEarlyNetworkConfigBodyversion, allowing us to safely revRackNetworkConfig.I wanted to walk through this with a worked example, so I made two seemingly-backwards-incompatible changes to
EarlyNetworkConfigBodythat I'll claim are actually completely fine:EarlyNetworkConfigBody::rack_network_confignon-optionalEarlyNetworkConfigBody::ntp_serversFor the first: the only creators of
EarlyNetworkConfigBodyare RSS and Nexus'ssync_switch_configurationbackground task, and both of them unconditionally supplySome(rack_network_config). If there was some code path in the past where this could beNone, it's long gone, and any deployed rack withNonewould have already failed to come back online after a reboot.For the second, I'll submit three pieces of evidence that it's safe to remove
ntp_servers:OmicronZoneType::BoundaryNtp { .. }, the config for the boundary NTP zone. It includes both upstream NTP servers and upstream DNS servers.Staged on top of #9944. Closes #9801.
(I'm not sure why we're still seeing a 10,000+ line diff from OpenAPI; maybe because I'm stacked on top of another PR instead of main? I assume that will go away once #9944 lands.)