Add LLDP configuration support #6185

Nieuwejaar · 2024-07-30T15:27:19Z

Several months ago, we added basic support for running lldp in the switch zone. That support consisted of plumbing through the chassis ID and sidecar revision, and starting the daemon.

This change adds support for enabling LLDP on individual ports, and for configuring per-port settings. These settings are consumed by wicket and are plumbed through the bootstore and into crdb. They are passed on to the daemon in the switch zone via SMF properties.

With this new functionality, we will be announcing our presence on a customer's network and should be visible through their network management tools. The daemon running in the switch zone will be collecting information from its peers on their network, but that collected data is not yet plumbed back through the oxide API/CLI. That will be addressed as follow-on work.

Nieuwejaar · 2024-07-30T15:28:42Z

illumos-utils/src/smf_helper.rs

@@ -77,7 +77,7 @@ impl<'t> SmfHelper<'t> {
                "addpropvalue",
                &prop.to_string(),
                &format!("{}:", valtype.to_string()),
-                &val.to_string(),
+                &format!("\"{}\"", val.to_string()),


This is needed to allow the settings of property values containing spaces, e.g., "oxide switch 1".

Nieuwejaar · 2024-07-30T15:30:21Z

nexus/db-model/src/schema.rs

    }
 }

 table! {
-    lldp_service_config (id) {
+    lldp_link_config (id) {


This makes the per-link lldp config data directly visible from that link's configuration. There was a level of indirection through a service_config which wasn't necessary.

Nieuwejaar · 2024-07-30T15:32:21Z

package-manifest.toml

@@ -620,8 +620,8 @@ output.intermediate_only = true
 service_name = "lldp"
 source.type = "prebuilt"
 source.repo = "lldp"
-source.commit = "30e5d89fae9190c69258ca77d5d5a1acec064742"
-source.sha256 = "f58bfd1b77748544b5b1a99a07e52bab8dc5673b9bd3a745ebbfdd614d492328"
+source.commit = "2a9f92dd55817af792670cd86e6a01676f5c79a0"


This LLDP update gives us a daemon that can understand the new per-port properties.

Nieuwejaar · 2024-07-30T15:34:00Z

schema/crdb/collapse_lldp_settings/up1.sql

+/*
+ * The old lldp_service_config_id is being replaced with lldp_link_config_id.
+ */
+ALTER TABLE omicron.public.switch_port_settings_link_config DROP COLUMN IF EXISTS lldp_service_config_id;


This column and the tables below can be dropped without losing any data, as no prior release has had any way to populate any of them. They were added as placeholders/stakes-in-the-ground last year.

Nieuwejaar · 2024-07-30T15:35:27Z

sled-agent/src/rack_setup/service.rs

@@ -749,7 +750,7 @@ impl ServiceInner {
                    .iter()
                    .map(|config| NexusTypes::PortConfigV2 {
                        port: config.port.clone(),
-			routes: config
+                        routes: config


There are a bunch of spacing-only changes here, which cargo fmt added, and which cargo fmt -- -check now requires. Only the lldp clause at 799 represents a substantive change.

Nieuwejaar · 2024-07-30T15:37:11Z

sled-agent/src/services.rs

-        info!(self.inner.log, "Setting up uplinkd service");
-        let smfh = SmfHelper::new(&zone, &SwitchService::Uplink);
+        info!(self.inner.log, "ensuring scrimlet uplinks");
+        let usmfh = SmfHelper::new(&zone, &SwitchService::Uplink);


We have two different SMF helpers, as we are updating the properties for two different services in the loop below.

smklein · 2024-08-01T17:08:59Z

nexus/db-queries/src/db/datastore/switch_port.rs

+    diesel::delete(lldp_link_config::dsl::lldp_link_config)
+        .filter(lldp_link_config::id.eq_any(lldp_link_ids))


Not a huge deal, but out of curiosity -- lldp_link_config has a time_deleted field, but it looks like we're doing hard deletion here instead of soft-deletion. When do you plan on using soft vs hard deletion for this table?

We don't actually have a way to delete the LLDP config at all - other than deleting the whole port, I guess. The soft delete field was carried over from the earlier, never-used, tables. I'm not really sure why we would use a soft delete for this. I'll see if Ry had a reason for going this way initially, but I expect I'll take it out.

smklein · 2024-08-01T17:10:22Z

nexus/db-queries/src/db/datastore/switch_port.rs

-                    .select(LldpServiceConfig::as_select())
+                use db::schema::lldp_link_config;
+                result.link_lldp = lldp_link_config::dsl::lldp_link_config
+                    .filter(lldp_link_config::id.eq_any(lldp_link_ids))


If we want to use soft-deletion, this should also probably:

.filter(lldp_link_config::dsl::time_deleted.is_null())

If we want to use hard-deletion, this isn't necessary, but then we should drop the time_deleted column

smklein · 2024-08-01T17:13:40Z

nexus/src/app/rack.rs

+                    ..Default::default()
+                },
+                Some(l) => LldpLinkConfigCreate {
+                    enabled: l.status == LldpAdminStatus::Enabled,


Is this translation problematic? Seems weird to me to report "enabled: false" when we might be still be allowing rx_only or tx_only traffic.

(This isn't my domain of expertise, so I trust your judgement, just seems odd to have those settings convert to "LLDP if off!")

There is a mismatch between what we support, and what the LLDP RFC supports. The protocol allows a node to be rx-only or tx-only, but the old LLDP tables only included enabled/disabled. It looks like Cisco at least does support all 4 options, so maybe we should as well. I'll run this by Ry and/or Trey to see whether we should offer that flexibility.

smklein · 2024-08-01T17:14:22Z

nexus/tests/integration_tests/switch_port.rs

+            lldp: LldpLinkConfigCreate {
+                enabled: true,
+                link_name: Some("Link Name".into()),
+                link_description: Some("link_ Dscription".into()),


super nit, because this is a test-only string and really doesn't matter, but:

Suggested change

link_description: Some("link_ Dscription".into()),

link_description: Some("link description".into()),

smklein · 2024-08-01T17:20:38Z

smf/sled-agent/non-gimlet/config-rss.toml

@@ -118,6 +118,22 @@ switch = "switch0"
 # Neighbors we expect to peer with over BGP on this port.
 bgp_peers = []

+# LLDP settings for this port
+#[rack_network_config.switch0.qsfp0.lldp]


Is it intentional that this config is commented out?

andrewjstone · 2024-08-01T18:57:58Z

common/src/api/internal/shared.rs

@@ -393,6 +471,8 @@ pub struct PortConfigV2 {
    /// Whether or not to set autonegotiation
    #[serde(default)]
    pub autoneg: bool,
+    /// LLDP configuration for this port
+    pub lldp: Option<LldpPortConfig>,


Does this require a #[serde(default)] to be backwards compatible?

Nope. The fact that it's an Option means that it's legal for it to be missing entirely. Essentially, the [serde(default)] is implicitly None for Options.

I tested this with both fresh installs and mupdateing from older, pre-llldp instances as well.

Awesome, ignore my comments then! I went searching for details about this online before leaving that comment but could not find an answer. Thanks for the dilligence.

Like Great White says, "My, my, my, I'm once bitten, twice shy, babe".

andrewjstone

Looks good @Nieuwejaar. I was mainly looking for bootstore related backwards compat issues, but also took a quick look elsewhere. I think since we may read a PortConfigV2 in the bootstore that is lacking the new lldp field, we need to tag it with #[serde(default)]. Otherwise, I would expect deserialization to fail on upgrades. I'd suggest testing out an upgrade on madrid or london to be sure everything works as expected if you haven't already.

#6185 introduced a schema change which added a non-nullable column `lldp_link_config_id` to the `switch_port_settings_link_config` table. However, on systems where `switch_port_settings_link_config` had rows, there was no "default" value, and the schema update appears to fail. This is currently the case on our dogfood system. To mitigate: this PR makes the `lldp_link_config_id` column nullable, by updating the existing schema change (for cases where it could not complete previously) and by adding a new schema change (for cases where the `switch_port_settings_link_config` table was empty, and the schema change **did** previously complete). Fixes #6433

oxidecomputer#6185 introduced a schema change which added a non-nullable column `lldp_link_config_id` to the `switch_port_settings_link_config` table. However, on systems where `switch_port_settings_link_config` had rows, there was no "default" value, and the schema update appears to fail. This is currently the case on our dogfood system. To mitigate: this PR makes the `lldp_link_config_id` column nullable, by updating the existing schema change (for cases where it could not complete previously) and by adding a new schema change (for cases where the `switch_port_settings_link_config` table was empty, and the schema change **did** previously complete). Fixes oxidecomputer#6433

Nieuwejaar commented Jul 30, 2024

View reviewed changes

Nieuwejaar requested review from smklein and andrewjstone July 30, 2024 15:37

smklein approved these changes Aug 1, 2024

View reviewed changes

andrewjstone reviewed Aug 1, 2024

View reviewed changes

andrewjstone approved these changes Aug 1, 2024

View reviewed changes

Nieuwejaar force-pushed the lldp branch from 7a451a0 to a61f6b0 Compare August 20, 2024 18:01

Add LLDP configuration support

161443f

Nieuwejaar force-pushed the lldp branch from a61f6b0 to 161443f Compare August 21, 2024 20:52

Nieuwejaar merged commit 45fb575 into main Aug 22, 2024
23 checks passed

Nieuwejaar deleted the lldp branch August 22, 2024 01:34

This was referenced Aug 26, 2024

Upgrade from CRDB schema version 88.0.0 failed: null value in column "lldp_link_config_id" violates not-null constraint #6433

Closed

Nullable lldp_link_config_id column #6436

Merged

davepacheco mentioned this pull request Aug 27, 2024

want automated test to catch known kinds of problematic schema updates #6451

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add LLDP configuration support #6185

Add LLDP configuration support #6185

Nieuwejaar commented Jul 30, 2024

Nieuwejaar Jul 30, 2024

Nieuwejaar Jul 30, 2024

Nieuwejaar Jul 30, 2024

Nieuwejaar Jul 30, 2024

Nieuwejaar Jul 30, 2024

Nieuwejaar Jul 30, 2024

smklein Aug 1, 2024

Nieuwejaar Aug 1, 2024

smklein Aug 1, 2024

smklein Aug 1, 2024

Nieuwejaar Aug 1, 2024

smklein Aug 1, 2024

smklein Aug 1, 2024

andrewjstone Aug 1, 2024

Nieuwejaar Aug 1, 2024

andrewjstone Aug 1, 2024

Nieuwejaar Aug 1, 2024

andrewjstone left a comment

		diesel::delete(lldp_link_config::dsl::lldp_link_config)
		.filter(lldp_link_config::id.eq_any(lldp_link_ids))

	link_description: Some("link_ Dscription".into()),
	link_description: Some("link description".into()),

Add LLDP configuration support #6185

Add LLDP configuration support #6185

Conversation

Nieuwejaar commented Jul 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewjstone left a comment

Choose a reason for hiding this comment