Improve usability of switch port settings by internet-diglett · Pull Request #7966 · oxidecomputer/omicron

internet-diglett · 2025-04-11T21:13:58Z

This PR aims to improve the usability of the switch port settings API by returning data of similar shape and content as what users provide when the create the switch port settings.

Issues resolved:

Before this PR, you only need one call to create a complete switch port settings object, but you have to perform several lookups to get the complete switch port configuration. We were already returning the object ids of the nested objects, so it makes a lot of sense to just return the actual object (especially since the user supplied the entire object when they configured it)
The HashMap<String_of_link_name, T> pattern also did not give the expected behavior in our go client generation. Changing this to a Vec<T_with_link_name_inside> matches what the user provides during creation and also resolves this behavior.
Some of our Vec<T> fields can be empty but must be present. In the Go client, an empty array is omitted from the json object. We needed to add a default annotation to these fields so Serde handles this situation correctly.

Switch port settings create params takes a hashmap of link name -> configuration value. However, we return a struct that has the configuration value and link name inside of a single type. We are updating this to be the same for create params and returned types. This change accomplishes two things: * The API is more consistent for users * The generated go client doesn't seem to work properly with the hash map style params. This change allows us to properly create settings using the go client.

david-crespo · 2025-05-13T19:05:39Z

    pub dst: oxnet::IpNet,

    /// The route's gateway address.
-    pub gw: oxnet::IpNet,


why can this go from IpNet to IpAddr?

This was likely never supposed to be an IpNet. The user provides an IpAddr when they create the configuration:

omicron/nexus/types/src/external_api/params.rs

Line 1900 in abc98f2

pub gw: IpAddr,

The DB stores singular ipv4 addresses as a full CIDR with /32, so that's probably how we ended up returning an IpNet instead of an IpAddr like what the user supplied.

david-crespo · 2025-05-13T19:12:38Z


+/// An IP address configuration for a port settings object.
+#[derive(Clone, Debug, Deserialize, JsonSchema, Serialize, PartialEq)]
+pub struct SwitchPortAddressView {


I see this matches up with SwitchPortSettingsView, but we don't usually put View on the end of these structs we call views. In this case I think SwitchPortAddress might be ok, though it makes it sound like it's just the address. But either nothing or something more descriptive than View would be an improvement.

SwitchPortSettingsView can't be changed to SwitchPortSettings because there is already a SwitchPortSettings, and SwitchPortSettingsConfig is ridiculous. What if instead of the settings key, you took that #[serde(flatten)] identity bit and stuck it directly in SwitchPortSettingsView? Then you could rename the struct SwitchPortSettings.

Giving this a try now

It looks like the existing SwitchPortSettings struct that wraps the IdentityMetadata is what we return when we list the SwitchPortSettings resource. I'm not sure on the original motivation for this, but the complete SwitchPortSettingsView struct is quite large and requires a bit of work to populate, so maybe we didn't want to do that when listing the resource.

Would SwitchPortSettingsIdentity make more sense for the wrapper struct? That would allow us to use SwitchPortSettings for the "view" struct.

I see. That's a naming conundrum. I don't think we distinguish the full view from the list item anywhere else, so we are inventing the convention here. *Identity feels natural because we have structs in the Rust code that are named like that, though we don't have any existing view schemas in the API ending with Identity. I think your proposal is solid: the list endpoint would return ResultsPage<SwitchPortSettingsIdentity> and the detail would be SwitchPortSettings. An alternative pair of names could be SwitchPortSettings and SwitchPortSettingsDetail. But I think I like the first one better.

Yet another option would be to return the full thing for the list as well, but you weren't kidding about it being a lot, so we probably shouldn't do that.

@ahl what do you think?

I've updated these type names to SwitchPortSettings and SwitchPortSettingsIdentity.

Cool, I think it looks good.

david-crespo · 2025-05-13T19:14:36Z

The hashmap to vec changes look good in the TS client. It's more intuitive this way.

internet-diglett · 2025-05-13T19:17:54Z

The hashmap to vec changes look good in the TS client. It's more intuitive this way.

I'm VERY happy to hear this!

rcgoodfellow

Thanks Levon. Just a few questions about testing at this point.

rcgoodfellow · 2025-05-15T22:11:37Z

-                },
-            )]),
-            addresses: HashMap::new(),
+            links: vec![],


Can you update this test to assert full round trip consistency? We're just looking at the number of ports below.

The test should be ensuring full round trip consistency now

rcgoodfellow · 2025-05-15T22:17:08Z

    assert_eq!(link0.mtu, 4700);

-    let lldp0 = &roundtrip.link_lldp[0];
+    let lldp0 = link0.lldp_link_config.clone().unwrap();


Is it possible to define (or derive) PartialEq for these structures to make the round trip testing more concise and less error prone?

I'll look into this

Done. Looks like this test was disabled back when we didn't have mgs in the test context. I re-enabled it.

@internet-diglett maybe we can close out this one!?

Make MGS available in nexus test suite #5780

Looks like it!

rcgoodfellow · 2025-05-15T22:21:35Z

@@ -0,0 +1 @@
+


Mistakenly added file?

This PR aims to improve the usability of the switch port settings API by returning data of similar shape and content as what users provide when the create the switch port settings. Issues resolved: * Before this PR, you only need one call to create a complete switch port settings object, but you have to perform several lookups to get the complete switch port configuration. We were already returning the object ids of the nested objects, so it makes a lot of sense to just return the actual object (especially since the user supplied the entire object when they configured it) * The `HashMap<String_of_link_name, T>` pattern also did not give the expected behavior in our go client generation. Changing this to a `Vec<T_with_link_name_inside>` matches what the user provides during creation and also resolves this behavior. * Some of our `Vec<T>` fields can be empty but must be present. In the Go client, an empty array is omitted from the json object. We needed to add a `default` annotation to these fields so Serde handles this situation correctly. ## Related - oxidecomputer/oxide.go#278 - oxidecomputer/terraform-provider-oxide#426 Closes #5780

internet-diglett added 5 commits April 11, 2025 21:12

wip

8290e75

Reshape returned LLDP and TxEq fields

334ba30

put actual TxEqConfig in returned link config

0e59683

Merge branch 'main' into return-similar-types-for-network-config

174667d

internet-diglett changed the title ~~wip~~ Add missing data to switch port settings May 13, 2025

internet-diglett marked this pull request as ready for review May 13, 2025 18:33

internet-diglett changed the title ~~Add missing data to switch port settings~~ Improve useability of switch port settings May 13, 2025

internet-diglett changed the title ~~Improve useability of switch port settings~~ Improve usability of switch port settings May 13, 2025

david-crespo reviewed May 13, 2025

View reviewed changes

change external type names for better clarity

9b54b3c

rcgoodfellow added this to the 15 milestone May 15, 2025

morlandi7 assigned internet-diglett May 15, 2025

Nieuwejaar self-requested a review May 15, 2025 17:02

rcgoodfellow reviewed May 15, 2025

View reviewed changes

Nieuwejaar approved these changes May 16, 2025

View reviewed changes

david-crespo approved these changes May 16, 2025

View reviewed changes

internet-diglett added 7 commits May 23, 2025 21:27

implement partial eq for lldp params

79b0adc

fixup! implement partial eq for lldp params

26c66f3

remove empty file

3f3e881

beef up testing for switch port settings

2519b4d

resolve id to name

09dcf74

testing: check length of collections

1b6580c

Merge branch 'main' into return-similar-types-for-network-config

1a1ed77

rcgoodfellow approved these changes May 30, 2025

View reviewed changes

internet-diglett merged commit 7547bfa into main May 31, 2025
17 checks passed

internet-diglett deleted the return-similar-types-for-network-config branch May 31, 2025 00:00

Conversation

internet-diglett commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-crespo commented May 13, 2025

Uh oh!

internet-diglett commented May 13, 2025

Uh oh!

rcgoodfellow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

internet-diglett commented Apr 11, 2025 •

edited

Loading