From a655ff2a26b1c58335d4ecc17e8263df73045063 Mon Sep 17 00:00:00 2001 From: Levon Tarver <11586085+internet-diglett@users.noreply.github.com> Date: Wed, 20 Mar 2024 13:30:18 -0500 Subject: [PATCH] Networking rpw bugfixes (#5286) --- clients/dpd-client/src/lib.rs | 40 ++++ clients/sled-agent-client/src/lib.rs | 46 +++++ common/src/api/internal/shared.rs | 8 +- nexus/src/app/background/nat_cleanup.rs | 16 +- .../background/sync_switch_configuration.rs | 177 +++++++++++++++--- package-manifest.toml | 12 +- tools/dendrite_openapi_version | 2 +- tools/dendrite_stub_checksums | 4 +- 8 files changed, 266 insertions(+), 39 deletions(-) diff --git a/clients/dpd-client/src/lib.rs b/clients/dpd-client/src/lib.rs index 1186e1722f..a898c31781 100644 --- a/clients/dpd-client/src/lib.rs +++ b/clients/dpd-client/src/lib.rs @@ -17,6 +17,10 @@ use slog::info; use slog::Logger; +use types::LinkCreate; +use types::LinkId; +use types::LinkSettings; +use types::PortSettings; include!(concat!(env!("OUT_DIR"), "/dpd-client.rs")); @@ -787,3 +791,39 @@ impl From for MacAddr { } } } + +impl Eq for PortSettings {} + +impl PartialEq for PortSettings { + fn eq(&self, other: &Self) -> bool { + self.links == other.links + } +} + +impl Eq for LinkSettings {} + +impl PartialEq for LinkSettings { + fn eq(&self, other: &Self) -> bool { + self.addrs == other.addrs && self.params == other.params + } +} + +impl Eq for LinkCreate {} + +impl PartialEq for LinkCreate { + fn eq(&self, other: &Self) -> bool { + self.autoneg == other.autoneg + && self.fec == other.fec + && self.kr == other.kr + && self.lane == other.lane + && self.speed == other.speed + } +} + +impl Eq for LinkId {} + +impl PartialEq for LinkId { + fn eq(&self, other: &Self) -> bool { + self.0 == other.0 + } +} diff --git a/clients/sled-agent-client/src/lib.rs b/clients/sled-agent-client/src/lib.rs index 5303a7bafd..2e9174c16c 100644 --- a/clients/sled-agent-client/src/lib.rs +++ b/clients/sled-agent-client/src/lib.rs @@ -8,8 +8,10 @@ use anyhow::Context; use async_trait::async_trait; use omicron_common::api::internal::shared::NetworkInterface; use std::convert::TryFrom; +use std::hash::Hash; use std::net::IpAddr; use std::net::SocketAddr; +use types::{BgpConfig, BgpPeerConfig, PortConfigV1, RouteConfig}; use uuid::Uuid; progenitor::generate_api!( @@ -605,3 +607,47 @@ impl TestInterfaces for Client { .expect("disk_finish_transition() failed unexpectedly"); } } + +impl Eq for BgpConfig {} + +impl Hash for BgpConfig { + fn hash(&self, state: &mut H) { + self.asn.hash(state); + self.originate.hash(state); + } +} + +impl Hash for BgpPeerConfig { + fn hash(&self, state: &mut H) { + self.addr.hash(state); + self.asn.hash(state); + self.port.hash(state); + self.hold_time.hash(state); + self.connect_retry.hash(state); + self.delay_open.hash(state); + self.idle_hold_time.hash(state); + self.keepalive.hash(state); + } +} + +impl Hash for RouteConfig { + fn hash(&self, state: &mut H) { + self.destination.hash(state); + self.nexthop.hash(state); + } +} + +impl Eq for PortConfigV1 {} + +impl Hash for PortConfigV1 { + fn hash(&self, state: &mut H) { + self.addresses.hash(state); + self.autoneg.hash(state); + self.bgp_peers.hash(state); + self.port.hash(state); + self.routes.hash(state); + self.switch.hash(state); + self.uplink_port_fec.hash(state); + self.uplink_port_speed.hash(state); + } +} diff --git a/common/src/api/internal/shared.rs b/common/src/api/internal/shared.rs index bf825fd2e7..3fc1eb9879 100644 --- a/common/src/api/internal/shared.rs +++ b/common/src/api/internal/shared.rs @@ -266,7 +266,9 @@ pub enum ExternalPortDiscovery { } /// Switchport Speed options -#[derive(Copy, Clone, Debug, Deserialize, Serialize, PartialEq, JsonSchema)] +#[derive( + Copy, Clone, Debug, Deserialize, Serialize, PartialEq, JsonSchema, Hash, +)] #[serde(rename_all = "snake_case")] pub enum PortSpeed { #[serde(alias = "0G")] @@ -290,7 +292,9 @@ pub enum PortSpeed { } /// Switchport FEC options -#[derive(Copy, Clone, Debug, Deserialize, Serialize, PartialEq, JsonSchema)] +#[derive( + Copy, Clone, Debug, Deserialize, Serialize, PartialEq, JsonSchema, Hash, +)] #[serde(rename_all = "snake_case")] pub enum PortFec { Firecode, diff --git a/nexus/src/app/background/nat_cleanup.rs b/nexus/src/app/background/nat_cleanup.rs index 16b1b7e357..844dbffefe 100644 --- a/nexus/src/app/background/nat_cleanup.rs +++ b/nexus/src/app/background/nat_cleanup.rs @@ -112,11 +112,21 @@ impl BackgroundTask for Ipv4NatGarbageCollector { let retention_threshold = Utc::now() - Duration::weeks(2); - let result = self + let result = match self .datastore .ipv4_nat_cleanup(opctx, min_gen, retention_threshold) - .await - .unwrap(); + .await { + Ok(v) => v, + Err(e) => { + return json!({ + "error": + format!( + "failed to perform cleanup operation: {:#}", + e + ) + }); + }, + }; let rv = serde_json::to_value(&result).unwrap_or_else(|error| { json!({ diff --git a/nexus/src/app/background/sync_switch_configuration.rs b/nexus/src/app/background/sync_switch_configuration.rs index e21f82b007..67d1d9fc1b 100644 --- a/nexus/src/app/background/sync_switch_configuration.rs +++ b/nexus/src/app/background/sync_switch_configuration.rs @@ -11,6 +11,7 @@ use crate::app::{ }, map_switch_zone_addrs, }; +use slog::o; use internal_dns::resolver::Resolver; use internal_dns::ServiceName; @@ -35,6 +36,7 @@ use nexus_db_queries::{ context::OpContext, db::{datastore::SwitchPortSettingsCombinedResult, DataStore}, }; +use nexus_types::identity::Asset; use nexus_types::{external_api::params, identity::Resource}; use omicron_common::OMICRON_DPD_TAG; use omicron_common::{ @@ -49,6 +51,7 @@ use sled_agent_client::types::{ }; use std::{ collections::{hash_map::Entry, HashMap, HashSet}, + hash::Hash, net::{IpAddr, Ipv4Addr}, str::FromStr, sync::Arc, @@ -211,6 +214,7 @@ impl SwitchPortSettingsManager { } } +#[derive(Debug)] enum PortSettingsChange { Apply(Box), Clear, @@ -222,7 +226,7 @@ impl BackgroundTask for SwitchPortSettingsManager { opctx: &'a OpContext, ) -> BoxFuture<'a, serde_json::Value> { async move { - let log = &opctx.log; + let log = opctx.log.clone(); let racks = match self.datastore.rack_list_initialized(opctx, &DataPageParams::max_page()).await { Ok(racks) => racks, @@ -244,6 +248,8 @@ impl BackgroundTask for SwitchPortSettingsManager { // but our logic for pulling switch ports and their related configurations // *isn't* per-rack, so that's something we'll need to revisit in the future. for rack in &racks { + let rack_id = rack.id().to_string(); + let log = log.new(o!("rack_id" => rack_id)); // lookup switch zones via DNS // TODO https://github.com/oxidecomputer/omicron/issues/5201 @@ -261,21 +267,21 @@ impl BackgroundTask for SwitchPortSettingsManager { // TODO https://github.com/oxidecomputer/omicron/issues/5201 let mappings = - map_switch_zone_addrs(log, switch_zone_addresses).await; + map_switch_zone_addrs(&log, switch_zone_addresses).await; // TODO https://github.com/oxidecomputer/omicron/issues/5201 // build sled agent clients - let sled_agent_clients = build_sled_agent_clients(&mappings, log); + let sled_agent_clients = build_sled_agent_clients(&mappings, &log); // TODO https://github.com/oxidecomputer/omicron/issues/5201 // build dpd clients - let dpd_clients = build_dpd_clients(&mappings, log); + let dpd_clients = build_dpd_clients(&mappings, &log); // TODO https://github.com/oxidecomputer/omicron/issues/5201 // build mgd clients - let mgd_clients = build_mgd_clients(mappings, log); + let mgd_clients = build_mgd_clients(mappings, &log); - let port_list = match self.switch_ports(opctx, log).await { + let port_list = match self.switch_ports(opctx, &log).await { Ok(value) => value, Err(e) => { error!(log, "failed to generate switchports for rack"; "error" => %e); @@ -287,7 +293,7 @@ impl BackgroundTask for SwitchPortSettingsManager { // calculate and apply switch port changes // - let changes = match self.changes(port_list, opctx, log).await { + let changes = match self.changes(port_list, opctx, &log).await { Ok(value) => value, Err(e) => { error!(log, "failed to generate changeset for switchport settings"; "error" => %e); @@ -295,7 +301,8 @@ impl BackgroundTask for SwitchPortSettingsManager { }, }; - apply_switch_port_changes(&dpd_clients, &changes, log).await; + info!(&log, "applying switch port config changes"; "changes" => ?changes); + apply_switch_port_changes(&dpd_clients, &changes, &log).await; // // calculate and apply routing changes @@ -303,7 +310,7 @@ impl BackgroundTask for SwitchPortSettingsManager { // get the static routes on each switch let current_static_routes = - static_routes_on_switch(&mgd_clients, log).await; + static_routes_on_switch(&mgd_clients, &log).await; info!(&log, "retrieved existing routes"; "routes" => ?current_static_routes); // generate the complete set of static routes that should be on a given switch @@ -315,7 +322,7 @@ impl BackgroundTask for SwitchPortSettingsManager { let routes_to_add = static_routes_to_add( &desired_static_routes, ¤t_static_routes, - log, + &log, ); info!(&log, "calculated static routes to add"; "routes" => ?routes_to_add); @@ -327,21 +334,26 @@ impl BackgroundTask for SwitchPortSettingsManager { // delete the unneeded routes first, just in case there is a conflicting route for // one we need to add - info!(&log, "deleting static routes"; "routes" => ?routes_to_del); - delete_static_routes(&mgd_clients, routes_to_del, log).await; + if !routes_to_del.is_empty() { + info!(&log, "deleting static routes"; "routes" => ?routes_to_del); + delete_static_routes(&mgd_clients, routes_to_del, &log).await; + } // add the new routes - info!(&log, "adding static routes"; "routes" => ?routes_to_add); - add_static_routes(&mgd_clients, routes_to_add, log).await; + if !routes_to_add.is_empty() { + info!(&log, "adding static routes"; "routes" => ?routes_to_add); + add_static_routes(&mgd_clients, routes_to_add, &log).await; + } // // calculate and apply loopback address changes // - match self.db_loopback_addresses(opctx, log).await { + info!(&log, "checking for changes to loopback addresses"); + match self.db_loopback_addresses(opctx, &log).await { Ok(desired_loopback_addresses) => { - let current_loopback_addresses = switch_loopback_addresses(&dpd_clients, log).await; + let current_loopback_addresses = switch_loopback_addresses(&dpd_clients, &log).await; let loopbacks_to_add: Vec<(SwitchLocation, IpAddr)> = desired_loopback_addresses .difference(¤t_loopback_addresses) @@ -352,8 +364,15 @@ impl BackgroundTask for SwitchPortSettingsManager { .map(|i| (i.0, i.1)) .collect(); - delete_loopback_addresses_from_switch(&loopbacks_to_del, &dpd_clients, log).await; - add_loopback_addresses_to_switch(&loopbacks_to_add, dpd_clients, log).await; + if !loopbacks_to_del.is_empty() { + info!(&log, "deleting loopback addresses"; "addresses" => ?loopbacks_to_del); + delete_loopback_addresses_from_switch(&loopbacks_to_del, &dpd_clients, &log).await; + } + + if !loopbacks_to_add.is_empty() { + info!(&log, "adding loopback addresses"; "addresses" => ?loopbacks_to_add); + add_loopback_addresses_to_switch(&loopbacks_to_add, dpd_clients, &log).await; + } }, Err(e) => { error!( @@ -769,7 +788,9 @@ impl BackgroundTask for SwitchPortSettingsManager { error!(log, "no blocks assigned to infra lot"); continue; }, - }; + } + ; + let mut desired_config = EarlyNetworkConfig { generation: 0, @@ -786,7 +807,7 @@ impl BackgroundTask for SwitchPortSettingsManager { }, }; - // should_update is a boolean value that determines whether or not we need to + // bootstore_needs_update is a boolean value that determines whether or not we need to // increment the bootstore version and push a new config to the sled agents. // // * If the config we've built from the switchport configuration information is @@ -804,15 +825,32 @@ impl BackgroundTask for SwitchPortSettingsManager { Ok(Some(BootstoreConfig { data, .. })) => { match serde_json::from_value::(data.clone()) { Ok(config) => { - if config.body.ntp_servers != desired_config.body.ntp_servers { + let current_ntp_servers: HashSet = config.body.ntp_servers.clone().into_iter().collect(); + let desired_ntp_servers: HashSet = desired_config.body.ntp_servers.clone().into_iter().collect(); + + let rnc_differs = match (config.body.rack_network_config.clone(), desired_config.body.rack_network_config.clone()) { + (Some(current_rnc), Some(desired_rnc)) => { + !hashset_eq(current_rnc.bgp.clone(), desired_rnc.bgp.clone()) || + !hashset_eq(current_rnc.ports.clone(), desired_rnc.ports.clone()) || + current_rnc.rack_subnet != desired_rnc.rack_subnet || + current_rnc.infra_ip_first != desired_rnc.infra_ip_first || + current_rnc.infra_ip_last != desired_rnc.infra_ip_last + }, + (None, Some(_)) => true, + _ => { + todo!("error") + } + }; + + if current_ntp_servers != desired_ntp_servers { info!( log, "ntp servers have changed"; - "old" => ?config.body.ntp_servers, - "new" => ?desired_config.body.ntp_servers, + "old" => ?current_ntp_servers, + "new" => ?desired_ntp_servers, ); true - } else if config.body.rack_network_config != desired_config.body.rack_network_config { + } else if rnc_differs { info!( log, "rack network config has changed"; @@ -855,6 +893,28 @@ impl BackgroundTask for SwitchPortSettingsManager { }, }; + // The following code is designed to give us the following + // properties + // * We only push updates to the bootstore (sled-agents) if + // configuration on our side (nexus) has relevant changes. + // * If the RPW encounters a critical error or crashes at any + // point of the operation, it will retry the configuration + // again during the next run + // * We are able to accomplish the above without inspecting + // the bootstore on the sled-agents + // + // For example, in the event that we crash after pushing to + // the sled-agents successfully, but before writing the + // results to the db + // 1. RPW will restart + // 2. RPW will build a new network config + // 3. RPW will compare against the last version stored in the db + // 4. RPW will decide to apply the config (again) + // 5. RPW will bump the version (again) + // 6. RPW will send a new bootstore update to the agents (with + // the same info as last time, but with a new version) + // 7. RPW will record the update in the db + // 8. We are now back on the happy path if bootstore_needs_update { let generation = match self.datastore .bump_bootstore_generation(opctx, NETWORK_KEY.into()) @@ -878,7 +938,7 @@ impl BackgroundTask for SwitchPortSettingsManager { "config" => ?desired_config, ); - // spush the updates to both scrimlets + // push the updates to both scrimlets // if both scrimlets are down, bootstore updates aren't happening anyway let mut one_succeeded = false; for (location, client) in &sled_agent_clients { @@ -923,6 +983,15 @@ impl BackgroundTask for SwitchPortSettingsManager { } } +fn hashset_eq(left: Vec, right: Vec) -> bool +where + T: Hash + Eq, +{ + let left = left.into_iter().collect::>(); + let right = right.into_iter().collect::>(); + left == right +} + async fn add_loopback_addresses_to_switch( loopbacks_to_add: &[(SwitchLocation, IpAddr)], dpd_clients: HashMap, @@ -1112,6 +1181,13 @@ fn static_routes_to_del( continue; }; } + + // filter out switches with no routes to remove + let routes_to_del = routes_to_del + .into_iter() + .filter(|(_location, request)| !request.routes.list.is_empty()) + .collect(); + routes_to_del } @@ -1158,6 +1234,13 @@ fn static_routes_to_add( }, ); } + + // filter out switches with no routes to add + let routes_to_add = routes_to_add + .into_iter() + .filter(|(_location, request)| !request.routes.list.is_empty()) + .collect(); + routes_to_add } @@ -1246,6 +1329,28 @@ async fn apply_switch_port_changes( } }; + let config_on_switch = + match client.port_settings_get(&dpd_port_id, DPD_TAG).await { + Ok(v) => v, + Err(e) => { + error!( + log, + "failed to retrieve port setttings from switch"; + "switch_port_id" => ?port_name, + "switch_location" => ?location, + "error" => format!("{:#}", e) + ); + continue; + } + }; + + info!( + log, + "retrieved port settings from switch"; + "switch_port_id" => ?port_name, + "settings" => ?config_on_switch, + ); + match change { PortSettingsChange::Apply(settings) => { let dpd_port_settings = match api_to_dpd_port_settings( @@ -1265,6 +1370,17 @@ async fn apply_switch_port_changes( } }; + if config_on_switch.into_inner() == dpd_port_settings { + info!( + &log, + "port settings up to date, skipping"; + "switch_port_id" => ?port_name, + "switch_location" => ?location, + "switch_port_settings_id" => ?settings.settings.id(), + ); + continue; + } + // apply settings via dpd client info!( &log, @@ -1301,6 +1417,17 @@ async fn apply_switch_port_changes( "switch_location" => ?location, "port_id" => ?dpd_port_id, ); + + if config_on_switch.into_inner().links.is_empty() { + info!( + &log, + "port settings up to date, skipping"; + "switch_port_id" => ?port_name, + "switch_location" => ?location, + ); + continue; + } + match client.port_settings_clear(&dpd_port_id, DPD_TAG).await { Ok(_) => {} Err(e) => { diff --git a/package-manifest.toml b/package-manifest.toml index e88f135985..0cc2e4939c 100644 --- a/package-manifest.toml +++ b/package-manifest.toml @@ -595,8 +595,8 @@ only_for_targets.image = "standard" # 2. Copy dendrite.tar.gz from dendrite/out to omicron/out source.type = "prebuilt" source.repo = "dendrite" -source.commit = "41ddeab9d43d90a51e6fc1c236dc9982fc76f922" -source.sha256 = "8ebb889a555ce59cb0373a1ec9595536e015c951f6fc4d89308b4e3f09c83b20" +source.commit = "b128500231b916802a9436dd438742424aa5c6ce" +source.sha256 = "47e0b279cf5babb199ea4d7d1654ccef6f463eeae7cef24e8138658c57affb4a" output.type = "zone" output.intermediate_only = true @@ -620,8 +620,8 @@ only_for_targets.image = "standard" # 2. Copy the output zone image from dendrite/out to omicron/out source.type = "prebuilt" source.repo = "dendrite" -source.commit = "41ddeab9d43d90a51e6fc1c236dc9982fc76f922" -source.sha256 = "3e8aa5483d22316e1fd629c77277190dafa875938a9ab3900e92a210c5e91e91" +source.commit = "b128500231b916802a9436dd438742424aa5c6ce" +source.sha256 = "3e6e785a3cfbf12dd9752c3bb026e4e3e75aa9d4433ba65045c7396d31e970a9" output.type = "zone" output.intermediate_only = true @@ -638,8 +638,8 @@ only_for_targets.image = "standard" # 2. Copy dendrite.tar.gz from dendrite/out to omicron/out/dendrite-softnpu.tar.gz source.type = "prebuilt" source.repo = "dendrite" -source.commit = "41ddeab9d43d90a51e6fc1c236dc9982fc76f922" -source.sha256 = "5e5f2831f3c46253828ea237f701f1fa174061ab0bf73c200d31d09e94890ae7" +source.commit = "b128500231b916802a9436dd438742424aa5c6ce" +source.sha256 = "64cb9c62d3516c613ea70c36dff241cba2e1138e25a82a6367c40da262ca55ee" output.type = "zone" output.intermediate_only = true diff --git a/tools/dendrite_openapi_version b/tools/dendrite_openapi_version index bbc2110a0a..3d5f21ffe0 100644 --- a/tools/dendrite_openapi_version +++ b/tools/dendrite_openapi_version @@ -1,2 +1,2 @@ -COMMIT="41ddeab9d43d90a51e6fc1c236dc9982fc76f922" +COMMIT="b128500231b916802a9436dd438742424aa5c6ce" SHA2="50eff6d9f986b7b1af5970d11d8d01b812de37269731c6c691a244b3fdae82ae" diff --git a/tools/dendrite_stub_checksums b/tools/dendrite_stub_checksums index 86cf1a56ec..3bca6cd174 100644 --- a/tools/dendrite_stub_checksums +++ b/tools/dendrite_stub_checksums @@ -1,3 +1,3 @@ -CIDL_SHA256_ILLUMOS="8ebb889a555ce59cb0373a1ec9595536e015c951f6fc4d89308b4e3f09c83b20" -CIDL_SHA256_LINUX_DPD="f753444cae478cdedcde743a20a9df5965ed28cddab0f9632f3c263c66cd6397" +CIDL_SHA256_ILLUMOS="47e0b279cf5babb199ea4d7d1654ccef6f463eeae7cef24e8138658c57affb4a" +CIDL_SHA256_LINUX_DPD="11e464a38fa0858c3d896e82d7ee3123aee5f0cf4e3c2a029a0dd7cfd54d3adf" CIDL_SHA256_LINUX_SWADM="66eab497b955751d0704c3cd97ac5c1ed373aa656fc37ccba86ae9900b5ae96d"