Skip to content

Commit

Permalink
Revert "Re-introduce support for setting a VLAN for an uplink from RSS (
Browse files Browse the repository at this point in the history 
#4319)" (#4323)

This reverts commit 35034f7.

#4319 (comment):
Note that setting the VLAN id on routes is going away entirely. The implementation was too costly on the Tofino, and we had to remove it for another feature as it did not wind up getting used.
  • Loading branch information
@luqmana
luqmana authored Oct 24, 2023
1 parent 35034f7 commit 4555620
Show file tree
Hide file tree
Showing 15 changed files with 15 additions and 161 deletions.
3 changes: 0 additions & 3 deletions common/src/api/internal/shared.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,6 @@ pub struct RouteConfig {
pub destination: IpNetwork,
/// The nexthop/gateway address.
pub nexthop: IpAddr,
/// The VLAN ID the gateway is reachable over.
pub vid: Option<u16>,
}

#[derive(Clone, Debug, Deserialize, Serialize, PartialEq, JsonSchema)]
Expand All @@ -139,7 +137,6 @@ impl From<UplinkConfig> for PortConfigV1 {
routes: vec![RouteConfig {
destination: "0.0.0.0/0".parse().unwrap(),
nexthop: value.gateway_ip.into(),
vid: value.uplink_vid,
}],
addresses: vec![value.uplink_cidr.into()],
switch: value.switch,
Expand Down
1 change: 0 additions & 1 deletion nexus/src/app/rack.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -663,7 +663,6 @@ impl super::Nexus {
.map(|r| SledRouteConfig {
destination: r.dst,
nexthop: r.gw.ip(),
vid: r.vid.map(Into::into),
})
.collect(),
addresses: info.addresses.iter().map(|a| a.address).collect(),
Expand Down
6 changes: 1 addition & 5 deletions nexus/src/app/sagas/switch_port_settings_apply.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -911,11 +911,7 @@ pub(crate) async fn bootstore_update(
routes: settings
.routes
.iter()
.map(|r| RouteConfig {
destination: r.dst,
nexthop: r.gw.ip(),
vid: r.vid.map(Into::into),
})
.map(|r| RouteConfig { destination: r.dst, nexthop: r.gw.ip() })
.collect(),
addresses: settings.addresses.iter().map(|a| a.address).collect(),
switch: switch_location,
Expand Down
31 changes: 7 additions & 24 deletions nexus/tests/integration_tests/switch_port.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,18 +128,11 @@ async fn test_port_settings_basic_crud(ctx: &ControlPlaneTestContext) {
settings.routes.insert(
"phy0".into(),
RouteConfig {
routes: vec![
Route {
dst: "1.2.3.0/24".parse().unwrap(),
gw: "1.2.3.4".parse().unwrap(),
vid: None,
},
Route {
dst: "5.6.7.0/24".parse().unwrap(),
gw: "5.6.7.8".parse().unwrap(),
vid: Some(5),
},
],
routes: vec![Route {
dst: "1.2.3.0/24".parse().unwrap(),
gw: "1.2.3.4".parse().unwrap(),
vid: None,
}],
},
);
// addresses
Expand All @@ -166,7 +159,7 @@ async fn test_port_settings_basic_crud(ctx: &ControlPlaneTestContext) {
.unwrap();

assert_eq!(created.links.len(), 1);
assert_eq!(created.routes.len(), 2);
assert_eq!(created.routes.len(), 1);
assert_eq!(created.addresses.len(), 1);

let link0 = &created.links[0];
Expand All @@ -185,11 +178,6 @@ async fn test_port_settings_basic_crud(ctx: &ControlPlaneTestContext) {
let route0 = &created.routes[0];
assert_eq!(route0.dst, "1.2.3.0/24".parse().unwrap());
assert_eq!(route0.gw, "1.2.3.4".parse().unwrap());
assert_eq!(route0.vlan_id, None);
let route1 = &created.routes[1];
assert_eq!(route1.dst, "5.6.7.0/24".parse().unwrap());
assert_eq!(route1.gw, "5.6.7.8".parse().unwrap());
assert_eq!(route1.vlan_id, Some(5));

let addr0 = &created.addresses[0];
assert_eq!(addr0.address, "203.0.113.10/24".parse().unwrap());
Expand All @@ -207,7 +195,7 @@ async fn test_port_settings_basic_crud(ctx: &ControlPlaneTestContext) {
.unwrap();

assert_eq!(roundtrip.links.len(), 1);
assert_eq!(roundtrip.routes.len(), 2);
assert_eq!(roundtrip.routes.len(), 1);
assert_eq!(roundtrip.addresses.len(), 1);

let link0 = &roundtrip.links[0];
Expand All @@ -226,11 +214,6 @@ async fn test_port_settings_basic_crud(ctx: &ControlPlaneTestContext) {
let route0 = &roundtrip.routes[0];
assert_eq!(route0.dst, "1.2.3.0/24".parse().unwrap());
assert_eq!(route0.gw, "1.2.3.4".parse().unwrap());
assert_eq!(route0.vlan_id, None);
let route1 = &roundtrip.routes[1];
assert_eq!(route1.dst, "5.6.7.0/24".parse().unwrap());
assert_eq!(route1.gw, "5.6.7.8".parse().unwrap());
assert_eq!(route1.vlan_id, Some(5));

let addr0 = &roundtrip.addresses[0];
assert_eq!(addr0.address, "203.0.113.10/24".parse().unwrap());
Expand Down
7 changes: 0 additions & 7 deletions openapi/bootstrap-agent.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -908,13 +908,6 @@
"description": "The nexthop/gateway address.",
"type": "string",
"format": "ip"
},
"vid": {
"nullable": true,
"description": "The VLAN ID the gateway is reachable over.",
"type": "integer",
"format": "uint16",
"minimum": 0
}
},
"required": [
Expand Down
7 changes: 0 additions & 7 deletions openapi/nexus-internal.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4507,13 +4507,6 @@
"description": "The nexthop/gateway address.",
"type": "string",
"format": "ip"
},
"vid": {
"nullable": true,
"description": "The VLAN ID the gateway is reachable over.",
"type": "integer",
"format": "uint16",
"minimum": 0
}
},
"required": [
Expand Down
7 changes: 0 additions & 7 deletions openapi/sled-agent.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2669,13 +2669,6 @@
"description": "The nexthop/gateway address.",
"type": "string",
"format": "ip"
},
"vid": {
"nullable": true,
"description": "The VLAN ID the gateway is reachable over.",
"type": "integer",
"format": "uint16",
"minimum": 0
}
},
"required": [
Expand Down
7 changes: 0 additions & 7 deletions openapi/wicketd.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2493,13 +2493,6 @@
"description": "The nexthop/gateway address.",
"type": "string",
"format": "ip"
},
"vid": {
"nullable": true,
"description": "The VLAN ID the gateway is reachable over.",
"type": "integer",
"format": "uint16",
"minimum": 0
}
},
"required": [
Expand Down
9 changes: 0 additions & 9 deletions schema/rss-sled-plan.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -581,15 +581,6 @@
"description": "The nexthop/gateway address.",
"type": "string",
"format": "ip"
},
"vid": {
"description": "The VLAN ID the gateway is reachable over.",
"type": [
"integer",
"null"
],
"format": "uint16",
"minimum": 0.0
}
}
},
Expand Down
63 changes: 2 additions & 61 deletions sled-agent/src/bootstrap/early_networking.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -509,15 +509,15 @@ impl<'a> EarlyNetworkSetup<'a> {
{
dpd_port_settings.v4_routes.insert(
dst.to_string(),
RouteSettingsV4 { link_id: link_id.0, nexthop, vid: r.vid },
RouteSettingsV4 { link_id: link_id.0, nexthop, vid: None },
);
}
if let (IpNetwork::V6(dst), IpAddr::V6(nexthop)) =
(r.destination, r.nexthop)
{
dpd_port_settings.v6_routes.insert(
dst.to_string(),
RouteSettingsV6 { link_id: link_id.0, nexthop, vid: r.vid },
RouteSettingsV6 { link_id: link_id.0, nexthop, vid: None },
);
}
}
Expand Down Expand Up @@ -785,65 +785,6 @@ mod tests {
routes: vec![RouteConfig {
destination: "0.0.0.0/0".parse().unwrap(),
nexthop: uplink.gateway_ip.into(),
vid: None,
}],
addresses: vec![uplink.uplink_cidr.into()],
switch: uplink.switch,
port: uplink.uplink_port,
uplink_port_speed: uplink.uplink_port_speed,
uplink_port_fec: uplink.uplink_port_fec,
bgp_peers: vec![],
}],
bgp: vec![],
}),
},
};

assert_eq!(expected, v1);
}

#[test]
fn serialized_early_network_config_v0_to_v1_conversion_with_vid() {
let v0 = EarlyNetworkConfigV0 {
generation: 1,
rack_subnet: Ipv6Addr::UNSPECIFIED,
ntp_servers: Vec::new(),
rack_network_config: Some(RackNetworkConfigV0 {
infra_ip_first: Ipv4Addr::UNSPECIFIED,
infra_ip_last: Ipv4Addr::UNSPECIFIED,
uplinks: vec![UplinkConfig {
gateway_ip: Ipv4Addr::UNSPECIFIED,
switch: SwitchLocation::Switch0,
uplink_port: "Port0".to_string(),
uplink_port_speed: PortSpeed::Speed100G,
uplink_port_fec: PortFec::None,
uplink_cidr: "192.168.0.1/16".parse().unwrap(),
uplink_vid: Some(10),
}],
}),
};

let v0_serialized = serde_json::to_vec(&v0).unwrap();
let bootstore_conf =
bootstore::NetworkConfig { generation: 1, blob: v0_serialized };

let v1 = EarlyNetworkConfig::try_from(bootstore_conf).unwrap();
let v0_rack_network_config = v0.rack_network_config.unwrap();
let uplink = v0_rack_network_config.uplinks[0].clone();
let expected = EarlyNetworkConfig {
generation: 1,
schema_version: 1,
body: EarlyNetworkConfigBody {
ntp_servers: v0.ntp_servers.clone(),
rack_network_config: Some(RackNetworkConfigV1 {
rack_subnet: Ipv6Network::new(v0.rack_subnet, 56).unwrap(),
infra_ip_first: v0_rack_network_config.infra_ip_first,
infra_ip_last: v0_rack_network_config.infra_ip_last,
ports: vec![PortConfigV1 {
routes: vec![RouteConfig {
destination: "0.0.0.0/0".parse().unwrap(),
nexthop: uplink.gateway_ip.into(),
vid: Some(10),
}],
addresses: vec![uplink.uplink_cidr.into()],
switch: uplink.switch,
Expand Down
1 change: 0 additions & 1 deletion sled-agent/src/rack_setup/service.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -591,7 +591,6 @@ impl ServiceInner {
.map(|r| NexusTypes::RouteConfig {
destination: r.destination,
nexthop: r.nexthop,
vid: r.vid,
})
.collect(),
addresses: config.addresses.clone(),
Expand Down
3 changes: 0 additions & 3 deletions wicket/src/rack_setup/config_template.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,6 @@ infra_ip_last = ""
[[rack_network_config.ports]]
# Routes associated with this port.
# { nexthop = "1.2.3.4", destination = "0.0.0.0/0" }
# Can also optionally specify a VLAN id if the next hop is reachable
# over an 802.1Q tagged L2 segment.
# { nexthop = "5.6.7.8", destination = "5.6.7.0/24", vid = 5 }
routes = []

# Addresses associated with this port.
Expand Down
23 changes: 4 additions & 19 deletions wicket/src/rack_setup/config_toml.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -245,12 +245,6 @@ fn populate_network_table(
r.destination.to_string(),
)),
);
if let Some(vid) = r.vid {
route.insert(
"vid",
Value::Integer(Formatted::new(vid.into())),
);
}
routes.push(Value::InlineTable(route));
}
uplink.insert("routes", Item::Value(Value::Array(routes)));
Expand Down Expand Up @@ -385,7 +379,6 @@ mod tests {
.map(|r| InternalRouteConfig {
destination: r.destination,
nexthop: r.nexthop,
vid: r.vid,
})
.collect(),
addresses: config.addresses.clone(),
Expand Down Expand Up @@ -485,18 +478,10 @@ mod tests {
infra_ip_last: "172.30.0.10".parse().unwrap(),
ports: vec![PortConfigV1 {
addresses: vec!["172.30.0.1/24".parse().unwrap()],
routes: vec![
RouteConfig {
destination: "0.0.0.0/0".parse().unwrap(),
nexthop: "172.30.0.10".parse().unwrap(),
vid: None,
},
RouteConfig {
destination: "10.20.0.0/16".parse().unwrap(),
nexthop: "10.0.0.20".parse().unwrap(),
vid: Some(20),
},
],
routes: vec![RouteConfig {
destination: "0.0.0.0/0".parse().unwrap(),
nexthop: "172.30.0.10".parse().unwrap(),
}],
bgp_peers: vec![BgpPeerConfig {
asn: 47,
addr: "10.2.3.4".parse().unwrap(),
Expand Down
7 changes: 1 addition & 6 deletions wicket/src/ui/panes/rack_setup.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -718,12 +718,7 @@ fn rss_config_text<'a>(
vec![
Span::styled(" • Route : ", label_style),
Span::styled(
format!(
"{} -> {} (vid={})",
r.destination,
r.nexthop,
r.vid.unwrap_or(0),
),
format!("{} -> {}", r.destination, r.nexthop),
ok_style,
),
]
Expand Down
1 change: 0 additions & 1 deletion wicketd/src/rss_config.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -511,7 +511,6 @@ fn validate_rack_network_config(
.map(|r| BaRouteConfig {
destination: r.destination,
nexthop: r.nexthop,
vid: r.vid,
})
.collect(),
addresses: config.addresses.clone(),
Expand Down

0 comments on commit 4555620

Please sign in to comment.