Add Cisco support#5622
Conversation
Pull Request Functional Test Report for #5622 / e29905eVirtual Devices
Hardware Devices
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the testing framework's compatibility with Cisco hardware by implementing robust breakout port management and interface-specific configuration adjustments. It introduces logic to correctly identify and verify breakout interfaces, improves static route handling for physical interfaces, and refines configuration sanitization to ensure cleaner test environments. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces support for 400GB and 800GB breakout speeds, adds static route recursion disabling for physical interfaces on Cisco devices, and implements a check to ensure breakout ports come online. It also clears IPv4 and IPv6 neighbors in fptest configuration. Feedback on the changes highlights a bug where the shared trackspeed variable is overwritten across loop iterations, potentially assigning incorrect speeds, along with a potential nil pointer dereference. Additionally, it is recommended to rename childInterfaceName to parentInterfaceName to accurately reflect its contents.
| var speed oc.E_IfEthernet_ETHERNET_SPEED | ||
|
|
||
| _, keyExists := breakoutPortsMap[intf.GetHardwarePort()] | ||
| if !keyExists && speed == oc.IfEthernet_ETHERNET_SPEED_UNSET { | ||
| if intf.GetEthernet().PortSpeed.String() == "SPEED_100GB" { | ||
| if intf.GetEthernet().PortSpeed.String() == "SPEED_800GB" { | ||
| trackspeed = oc.IfEthernet_ETHERNET_SPEED_SPEED_800GB | ||
| } else if intf.GetEthernet().PortSpeed.String() == "SPEED_400GB" { | ||
| trackspeed = oc.IfEthernet_ETHERNET_SPEED_SPEED_400GB | ||
| } else if intf.GetEthernet().PortSpeed.String() == "SPEED_100GB" { | ||
| trackspeed = oc.IfEthernet_ETHERNET_SPEED_SPEED_100GB | ||
| } else if intf.GetEthernet().PortSpeed.String() == "SPEED_10GB" { | ||
| trackspeed = oc.IfEthernet_ETHERNET_SPEED_SPEED_10GB | ||
| } | ||
| } | ||
| hwPort := intf.GetHardwarePort() |
There was a problem hiding this comment.
The trackspeed variable is declared outside the loop (at line 1144). When iterating over interfaces, if a new hardware port is encountered (!keyExists), trackspeed is updated to that port's speed. However, because breakoutPortsMap[hwPort] is updated on every iteration (including when keyExists is true) using the shared trackspeed variable, if the loop processes an interface for a different hardware port in between, the trackspeed variable will have been overwritten with the other port's speed. This results in incorrect breakout speeds being assigned to previously discovered ports.
Additionally, calling intf.GetEthernet().PortSpeed without checking if intf.GetEthernet() is nil can lead to a nil pointer dereference panic.
We can resolve both issues by retrieving the existing speed from the map if it exists, or safely checking and determining it from the current interface if it does not.
| var speed oc.E_IfEthernet_ETHERNET_SPEED | |
| _, keyExists := breakoutPortsMap[intf.GetHardwarePort()] | |
| if !keyExists && speed == oc.IfEthernet_ETHERNET_SPEED_UNSET { | |
| if intf.GetEthernet().PortSpeed.String() == "SPEED_100GB" { | |
| if intf.GetEthernet().PortSpeed.String() == "SPEED_800GB" { | |
| trackspeed = oc.IfEthernet_ETHERNET_SPEED_SPEED_800GB | |
| } else if intf.GetEthernet().PortSpeed.String() == "SPEED_400GB" { | |
| trackspeed = oc.IfEthernet_ETHERNET_SPEED_SPEED_400GB | |
| } else if intf.GetEthernet().PortSpeed.String() == "SPEED_100GB" { | |
| trackspeed = oc.IfEthernet_ETHERNET_SPEED_SPEED_100GB | |
| } else if intf.GetEthernet().PortSpeed.String() == "SPEED_10GB" { | |
| trackspeed = oc.IfEthernet_ETHERNET_SPEED_SPEED_10GB | |
| } | |
| } | |
| hwPort := intf.GetHardwarePort() | |
| hwPort := intf.GetHardwarePort() | |
| if existing, ok := breakoutPortsMap[hwPort]; ok { | |
| trackspeed = existing.breakoutSpeed | |
| } else if intf.GetEthernet() != nil { | |
| switch intf.GetEthernet().PortSpeed.String() { | |
| case "SPEED_800GB": | |
| trackspeed = oc.IfEthernet_ETHERNET_SPEED_SPEED_800GB | |
| case "SPEED_400GB": | |
| trackspeed = oc.IfEthernet_ETHERNET_SPEED_SPEED_400GB | |
| case "SPEED_100GB": | |
| trackspeed = oc.IfEthernet_ETHERNET_SPEED_SPEED_100GB | |
| case "SPEED_10GB": | |
| trackspeed = oc.IfEthernet_ETHERNET_SPEED_SPEED_10GB | |
| } | |
| } |
| breakoutSpeed oc.E_IfEthernet_ETHERNET_SPEED | ||
| numPhysicalChannels *uint8 | ||
| numPhysicalChannels uint8 | ||
| childInterfaceName string |
There was a problem hiding this comment.
The field childInterfaceName actually stores the parent interface name (e.g., FourHundredGigE0/0/0/22 after stripping the breakout index suffix). Naming it childInterfaceName is highly misleading. Please rename it to parentInterfaceName to improve code readability and maintainability.
Remember to also update its usages at lines 1195 and 1255.
| childInterfaceName string | |
| parentInterfaceName string |
1) Breakout channel count type
breakout.numPhysicalChannelschanged from*uint8touint8.data.numPhysicalChannels) rather than*data.numPhysicalChannels.2) NumBreakouts calculation in
containerOp.pushgp.NumBreakoutschanged from:ygot.Uint8(*data.numPhysicalChannels + 1)ygot.Uint8(data.numPhysicalChannels)+1offset and aligns breakout count with computed physical channels.3) Subport iteration update
for i := 0; i < int(*data.numPhysicalChannels); i++for i := 0; i < int(data.numPhysicalChannels); i++4) Breakout candidate matching logic in
addMissingConfigForContainerReplaceif hwp+"/"+channel == name { ... }if strings.HasPrefix(name, hwp+"/") { ... }5) Speed tracking support and counting logic
SPEED_800GBSPEED_400GBSPEED_100GBSPEED_10GBnumChannels := port[hwPort] + 1port[hwPort] = numChannelsbreakoutPortsMap[hwPort] = breakout{numPhysicalChannels: numChannels, breakoutSpeed: trackspeed}6) Static route recurse handling for interface next-hop
setStaticRouteRecurseForPhysicalInterface(config *oc.Root).nh.Recurse = ygot.Bool(false)isPhysicalInterface(...)returns true.7) Physical interface detection logic
isPhysicalInterface(config *oc.Root, ifName string) bool.config.GetInterface(ifName)ethernetCsmacdieee8023adLag8) Push path integration points
rootOp.pushcontainerOp.pushitemOp.pushsetStaticRouteRecurseForPhysicalInterface(config)9) Breakout mode struct enhancement
parentInterfaceNamefield to thebreakoutstruct:parentInterfaceName string // e.g., "FourHundredGigE0/0/0/22"FourHundredGigE,HundredGigE) separately from the normalized hardware port identifier.Port0/0/0/22/0) instead of actual interface names (e.g.,FourHundredGigE0/0/0/22/0).10) Breakout port discovery enhancement
addMissingConfigForContainerReplaceto extract and store parent interface name:/indexsuffix.FourHundredGigE0/0/0/22/0, extractFourHundredGigE0/0/0/22.breakout.parentInterfaceNamefor later use in child port name construction.11) Breakout port online verification
checkBreakoutPortsOnlinehelper function called after ContainerOpbatch.Set()for Cisco devices.gnmi.Lookupto check ifComponent/Port/BreakoutMode/Group/0/num-breakoutsstate is available.gnmi.Awaitwith 3-minute timeout to verify each child interface reachesUP.parentInterfaceName(e.g.,FourHundredGigE0/0/0/22/0).