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.

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

link fec is now an optional setting #6992

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion common/src/api/external/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2514,7 +2514,7 @@ pub struct SwitchPortLinkConfig {
pub mtu: u16,

/// The forward error correction mode of the link.
pub fec: LinkFec,
pub fec: Option<LinkFec>,

/// The configured speed of the link.
pub speed: LinkSpeed,
Expand Down
2 changes: 1 addition & 1 deletion common/src/api/internal/shared.rs
Original file line number Diff line number Diff line change
Expand Up @@ -489,7 +489,7 @@ pub struct PortConfigV2 {
/// Port speed.
pub uplink_port_speed: PortSpeed,
/// Port forward error correction type.
pub uplink_port_fec: PortFec,
pub uplink_port_fec: Option<PortFec>,
/// BGP peers on this port
pub bgp_peers: Vec<BgpPeerConfig>,
/// Whether or not to set autonegotiation
Expand Down
2 changes: 1 addition & 1 deletion nexus/db-model/src/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ table! {
port_settings_id -> Uuid,
link_name -> Text,
mtu -> Int4,
fec -> crate::SwitchLinkFecEnum,
fec -> Nullable<crate::SwitchLinkFecEnum>,
speed -> crate::SwitchLinkSpeedEnum,
autoneg -> Bool,
lldp_link_config_id -> Nullable<Uuid>,
Expand Down
4 changes: 2 additions & 2 deletions nexus/db-model/src/schema_versions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@ use std::collections::BTreeMap;
///
/// This must be updated when you change the database schema. Refer to
/// schema/crdb/README.adoc in the root of this repository for details.
pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(113, 0, 0);

pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(114, 0, 0);
/// List of all past database schema versions, in *reverse* order
///
/// If you want to change the Omicron database schema, you must update this.
Expand All @@ -29,6 +28,7 @@ static KNOWN_VERSIONS: Lazy<Vec<KnownVersion>> = Lazy::new(|| {
// | leaving the first copy as an example for the next person.
// v
// KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"),
KnownVersion::new(114, "make-link-settings-non-nullable"),
KnownVersion::new(113, "add-tx-eq"),
KnownVersion::new(112, "blueprint-dataset"),
KnownVersion::new(111, "drop-omicron-zone-underlay-address"),
Expand Down
6 changes: 3 additions & 3 deletions nexus/db-model/src/switch_port.rs
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,7 @@ pub struct SwitchPortLinkConfig {
pub lldp_link_config_id: Option<Uuid>,
pub link_name: String,
pub mtu: SqlU16,
pub fec: SwitchLinkFec,
pub fec: Option<SwitchLinkFec>,
pub speed: SwitchLinkSpeed,
pub autoneg: bool,
pub tx_eq_config_id: Option<Uuid>,
Expand All @@ -398,7 +398,7 @@ impl SwitchPortLinkConfig {
lldp_link_config_id: Uuid,
link_name: String,
mtu: u16,
fec: SwitchLinkFec,
fec: Option<SwitchLinkFec>,
speed: SwitchLinkSpeed,
autoneg: bool,
tx_eq_config_id: Option<Uuid>,
Expand All @@ -424,7 +424,7 @@ impl Into<external::SwitchPortLinkConfig> for SwitchPortLinkConfig {
tx_eq_config_id: self.tx_eq_config_id,
link_name: self.link_name.clone(),
mtu: self.mtu.into(),
fec: self.fec.into(),
fec: self.fec.map(|fec| fec.into()),
speed: self.speed.into(),
autoneg: self.autoneg,
}
Expand Down
2 changes: 1 addition & 1 deletion nexus/db-queries/src/db/datastore/switch_port.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1229,7 +1229,7 @@ async fn do_switch_port_settings_create(
lldp_config_id,
link_name.clone(),
c.mtu,
c.fec.into(),
c.fec.map(|fec| fec.into()),
c.speed.into(),
c.autoneg,
tx_eq_config_id,
Expand Down
4 changes: 2 additions & 2 deletions nexus/src/app/background/tasks/networking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,11 +83,11 @@ pub(crate) fn api_to_dpd_port_settings(
lane: Some(LinkId(0)),
kr: false,
tx_eq: tx_eq.clone(),
fec: match l.fec {
fec: l.fec.map(|fec| match fec {
SwitchLinkFec::Firecode => PortFec::Firecode,
SwitchLinkFec::Rs => PortFec::Rs,
SwitchLinkFec::None => PortFec::None,
},
}),
speed: match l.speed {
SwitchLinkSpeed::Speed0G => PortSpeed::Speed0G,
SwitchLinkSpeed::Speed1G => PortSpeed::Speed1G,
Expand Down
7 changes: 3 additions & 4 deletions nexus/src/app/background/tasks/sync_switch_configuration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use internal_dns_types::names::ServiceName;
use ipnetwork::IpNetwork;
use nexus_db_model::{
AddressLotBlock, BgpConfig, BootstoreConfig, LoopbackAddress,
SwitchLinkFec, SwitchLinkSpeed, INFRA_LOT, NETWORK_KEY,
SwitchLinkSpeed, INFRA_LOT, NETWORK_KEY,
};
use uuid::Uuid;

Expand Down Expand Up @@ -1002,9 +1002,8 @@ impl BackgroundTask for SwitchPortSettingsManager {
uplink_port_fec: info
.links
.get(0) //TODO https://github.com/oxidecomputer/omicron/issues/3062
.map(|l| l.fec)
.unwrap_or(SwitchLinkFec::None)
.into(),
.map(|l| l.fec.map(|fec| fec.into()))
.unwrap_or(None),
uplink_port_speed: info
.links
.get(0) //TODO https://github.com/oxidecomputer/omicron/issues/3062
Expand Down
2 changes: 1 addition & 1 deletion nexus/src/app/rack.rs
Original file line number Diff line number Diff line change
Expand Up @@ -654,7 +654,7 @@ impl super::Nexus {
let link = LinkConfigCreate {
//TODO https://github.com/oxidecomputer/omicron/issues/2274
mtu: 1500,
fec: uplink_config.uplink_port_fec.into(),
fec: uplink_config.uplink_port_fec.map(|fec| fec.into()),
speed: uplink_config.uplink_port_speed.into(),
autoneg: uplink_config.autoneg,
lldp,
Expand Down
2 changes: 1 addition & 1 deletion nexus/tests/integration_tests/switch_port.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ async fn test_port_settings_basic_crud(ctx: &ControlPlaneTestContext) {
system_description: Some("System description".into()),
management_ip: None,
},
fec: LinkFec::None,
fec: Some(LinkFec::None),
speed: LinkSpeed::Speed100G,
autoneg: false,
tx_eq: None,
Expand Down
2 changes: 1 addition & 1 deletion nexus/types/src/external_api/params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1716,7 +1716,7 @@ pub struct LinkConfigCreate {
pub lldp: LldpLinkConfigCreate,

/// The forward error correction mode of the link.
pub fec: LinkFec,
pub fec: Option<LinkFec>,

/// The speed of the link.
pub speed: LinkSpeed,
Expand Down
2 changes: 1 addition & 1 deletion openapi/bootstrap-agent.json
Original file line number Diff line number Diff line change
Expand Up @@ -866,6 +866,7 @@
]
},
"uplink_port_fec": {
"nullable": true,
"description": "Port forward error correction type.",
"allOf": [
{
Expand All @@ -888,7 +889,6 @@
"port",
"routes",
"switch",
"uplink_port_fec",
"uplink_port_speed"
]
},
Expand Down
2 changes: 1 addition & 1 deletion openapi/nexus-internal.json
Original file line number Diff line number Diff line change
Expand Up @@ -4687,6 +4687,7 @@
]
},
"uplink_port_fec": {
"nullable": true,
"description": "Port forward error correction type.",
"allOf": [
{
Expand All @@ -4709,7 +4710,6 @@
"port",
"routes",
"switch",
"uplink_port_fec",
"uplink_port_speed"
]
},
Expand Down
4 changes: 2 additions & 2 deletions openapi/nexus.json
Original file line number Diff line number Diff line change
Expand Up @@ -17166,6 +17166,7 @@
"type": "boolean"
},
"fec": {
"nullable": true,
"description": "The forward error correction mode of the link.",
"allOf": [
{
Expand Down Expand Up @@ -17207,7 +17208,6 @@
},
"required": [
"autoneg",
"fec",
"lldp",
"mtu",
"speed"
Expand Down Expand Up @@ -20410,6 +20410,7 @@
"type": "boolean"
},
"fec": {
"nullable": true,
"description": "The forward error correction mode of the link.",
"allOf": [
{
Expand Down Expand Up @@ -20455,7 +20456,6 @@
},
"required": [
"autoneg",
"fec",
"link_name",
"mtu",
"port_settings_id",
Expand Down
2 changes: 1 addition & 1 deletion openapi/sled-agent.json
Original file line number Diff line number Diff line change
Expand Up @@ -4644,6 +4644,7 @@
]
},
"uplink_port_fec": {
"nullable": true,
"description": "Port forward error correction type.",
"allOf": [
{
Expand All @@ -4666,7 +4667,6 @@
"port",
"routes",
"switch",
"uplink_port_fec",
"uplink_port_speed"
]
},
Expand Down
8 changes: 6 additions & 2 deletions openapi/wicketd.json
Original file line number Diff line number Diff line change
Expand Up @@ -6676,7 +6676,12 @@
]
},
"uplink_port_fec": {
"$ref": "#/components/schemas/PortFec"
"nullable": true,
"allOf": [
{
"$ref": "#/components/schemas/PortFec"
}
]
},
"uplink_port_speed": {
"$ref": "#/components/schemas/PortSpeed"
Expand All @@ -6686,7 +6691,6 @@
"addresses",
"autoneg",
"routes",
"uplink_port_fec",
"uplink_port_speed"
],
"additionalProperties": false
Expand Down
12 changes: 6 additions & 6 deletions package-manifest.toml
Original file line number Diff line number Diff line change
Expand Up @@ -719,8 +719,8 @@ only_for_targets.image = "standard"
# the other `source.*` keys.
source.type = "prebuilt"
source.repo = "dendrite"
source.commit = "4067d742d832fa434217b95e4b149048d01ef54e"
source.sha256 = "5e9ccc42e5ac31f4be24025d2afd5978aef33d618f3cb7caa260eff73b7e6a79"
source.commit = "4d97f30b61e54c8e14adf582092d89969cb4a66d"
source.sha256 = "5c055b3b065454be1fc0c89d28af99b860742d4ee8ea71864de339427aaad690"
output.type = "zone"
output.intermediate_only = true

Expand All @@ -746,8 +746,8 @@ only_for_targets.image = "standard"
# the other `source.*` keys.
source.type = "prebuilt"
source.repo = "dendrite"
source.commit = "4067d742d832fa434217b95e4b149048d01ef54e"
source.sha256 = "9d3156b7895126b9df5460dd0c34668738a7f2d5894a4be0229644820e732895"
source.commit = "4d97f30b61e54c8e14adf582092d89969cb4a66d"
source.sha256 = "b7e0a0828a17dcc134f60451f0d80c50c3ed97cd1d9bf9c27751711a7f5cc028"
output.type = "zone"
output.intermediate_only = true

Expand All @@ -766,8 +766,8 @@ only_for_targets.image = "standard"
# the other `source.*` keys.
source.type = "prebuilt"
source.repo = "dendrite"
source.commit = "4067d742d832fa434217b95e4b149048d01ef54e"
source.sha256 = "4eff4f00201ab8373510644693d066dbec2497142d48964be9844f0b30c147e8"
source.commit = "4d97f30b61e54c8e14adf582092d89969cb4a66d"
source.sha256 = "e7753bf199b6a16a92a65864e2ecbb20327fd3717a42de51b6fda88d0f71058c"
output.type = "zone"
output.intermediate_only = true

Expand Down
10 changes: 5 additions & 5 deletions schema/crdb/dbinit.sql
Original file line number Diff line number Diff line change
Expand Up @@ -2840,11 +2840,11 @@ CREATE TYPE IF NOT EXISTS omicron.public.switch_link_speed AS ENUM (
);

CREATE TABLE IF NOT EXISTS omicron.public.switch_port_settings_link_config (
port_settings_id UUID,
link_name TEXT,
mtu INT4,
port_settings_id UUID NOT NULL,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These changes aren't strictly related to the FEC part of this PR. These fields should always have been NOT NULL, and that oversight came to light when I realized that the fec field was already nullable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to split this into two PRs? Marking the "rust side" of things as Nullable is an easy change to approve, but altering these columns to be non-NULL is a more involved change

link_name TEXT NOT NULL,
mtu INT4 NOT NULL,
fec omicron.public.switch_link_fec,
speed omicron.public.switch_link_speed,
speed omicron.public.switch_link_speed NOT NULL,
autoneg BOOL NOT NULL DEFAULT false,
lldp_link_config_id UUID,
tx_eq_config_id UUID,
Expand Down Expand Up @@ -4601,7 +4601,7 @@ INSERT INTO omicron.public.db_metadata (
version,
target_version
) VALUES
(TRUE, NOW(), NOW(), '113.0.0', NULL)
(TRUE, NOW(), NOW(), '114.0.0', NULL)
ON CONFLICT DO NOTHING;

COMMIT;
3 changes: 3 additions & 0 deletions schema/crdb/make-link-settings-non-nullable/up1.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
ALTER TABLE omicron.public.switch_port_settings_link_config ALTER COLUMN speed SET NOT NULL;
ALTER TABLE omicron.public.switch_port_settings_link_config ALTER COLUMN mtu SET NOT NULL;
ALTER TABLE omicron.public.switch_port_settings_link_config ALTER COLUMN port_settings_id SET NOT NULL;
Comment on lines +1 to +3
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks a lot like https://github.com/oxidecomputer/omicron/tree/main/schema/crdb#211-adding-a-new-column-without-a-default-value -- if there are any databases which exist, where any of these values are NULL, then we'd fail to apply the schema upgrade and the rack would be stuck.

  1. Are we confident that these are non-NULL across all deployments? Why?
  2. Could we use a default value here, in the case that there are NULL values?

6 changes: 4 additions & 2 deletions schema/rss-sled-plan.json
Original file line number Diff line number Diff line change
Expand Up @@ -698,7 +698,6 @@
"port",
"routes",
"switch",
"uplink_port_fec",
"uplink_port_speed"
],
"properties": {
Expand Down Expand Up @@ -764,9 +763,12 @@
},
"uplink_port_fec": {
"description": "Port forward error correction type.",
"allOf": [
"anyOf": [
{
"$ref": "#/definitions/PortFec"
},
{
"type": "null"
}
]
},
Expand Down
4 changes: 2 additions & 2 deletions sled-agent/src/bootstrap/early_networking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -699,7 +699,7 @@ impl<'a> EarlyNetworkSetup<'a> {
params: LinkCreate {
autoneg: port_config.autoneg,
kr: false, //NOTE: kr does not apply to user configurable links.
fec: convert_fec(&port_config.uplink_port_fec),
fec: port_config.uplink_port_fec.map(convert_fec),
speed: convert_speed(&port_config.uplink_port_speed),
lane: Some(LinkId(0)),
tx_eq: port_config.tx_eq.map(|x| TxEq {
Expand Down Expand Up @@ -783,7 +783,7 @@ fn convert_speed(speed: &PortSpeed) -> dpd_client::types::PortSpeed {
}
}

fn convert_fec(fec: &PortFec) -> dpd_client::types::PortFec {
fn convert_fec(fec: PortFec) -> dpd_client::types::PortFec {
match fec {
PortFec::Firecode => dpd_client::types::PortFec::Firecode,
PortFec::None => dpd_client::types::PortFec::None,
Expand Down
4 changes: 3 additions & 1 deletion sled-agent/src/rack_setup/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -791,7 +791,9 @@ impl ServiceInner {
.collect(),
switch: config.switch.into(),
uplink_port_speed: config.uplink_port_speed.into(),
uplink_port_fec: config.uplink_port_fec.into(),
uplink_port_fec: config
.uplink_port_fec
.map(|fec| fec.into()),
autoneg: config.autoneg,
bgp_peers: config
.bgp_peers
Expand Down
2 changes: 1 addition & 1 deletion sled-agent/tests/integration_tests/early_network.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ fn current_config_example() -> (&'static str, EarlyNetworkConfig) {
switch: SwitchLocation::Switch0,
port: "foo".to_owned(),
uplink_port_speed: PortSpeed::Speed200G,
uplink_port_fec: PortFec::Firecode,
uplink_port_fec: Some(PortFec::Firecode),
bgp_peers: vec![BgpPeerConfig {
asn: 65000,
port: "bar".to_owned(),
Expand Down
Loading
Loading