-
Notifications
You must be signed in to change notification settings - Fork 40
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
6750 Add support for post-install LLDP configuration #7132
base: main
Are you sure you want to change the base?
Conversation
6751 Add support for extracting LLDP neighbor information
// The LLDP configuration has been defined as a leaf of the switch-port-settings | ||
// tree, and is identified in the database with a UUID stored in that tree. | ||
// Using the uuid as the target argument for the config operations would be | ||
// reasonable, and similar in spirit to the link configuration operations. | ||
// | ||
// On the other hand, the neighbors are discovered on a configured link, but the | ||
// data is otherwise completely independent of the configuration. Furthermore, | ||
// the questions answered by the neighbor information have to do with the | ||
// physical connections between the Oxide rack and the upstream, datacenter | ||
// switch. Accordingly, it seems more appropriate to use the physical | ||
// rack/switch/port triple to identify the port of interest for the neighbors | ||
// query. | ||
// | ||
// For consistency across the lldp operations, all use rack/switch/port rather | ||
// than the uuid. | ||
// XXX: Is this the right call? The other options being: uuid for all | ||
// operations, or uuid for config and r/s/p for neighbors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason LLDP configuration should be a part of the switch-port-settings
object, aside from everything else also being there? I think it can be argued that we could migrate it out and make the CREATE
operation consistent with the other paths that have been defined for the GET
and UPDATE
methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could go back and look at the git history to be sure, but I think they've been there for as long as we've have switch port settings in the database. These are settings that belong to a specific port on the switch, so I'm not sure where else they would go? The settings themselves have their own table - the switch-port-settings just has a link to that table.
If you have something specific in mind, I'd be happy to tackle that as follow-on work. This is already a pretty chunky PR and doesn't include any database changes, so I'd rather not weigh it down with a db restructure as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good just a small comment
PortId::MacAddress(mac) => format!( | ||
"{:02x}:{:02x}:{:02x}:{:02x}:{:02x}:{:02x}", | ||
mac.a[0], | ||
mac.a[1], | ||
mac.a[2], | ||
mac.a[3], | ||
mac.a[4], | ||
mac.a[5] | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we implement a to_string()
method for the mac address type to reduce repetition?
6751 Add support for extracting LLDP neighbor information