Conversation
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 addresses several critical issues within the aggregate_all_not_forwarding_viable_test to ensure its proper execution and accuracy on Cisco devices. The changes primarily focus on aligning test configurations with Cisco IOS-XR's specific behaviors regarding VRF assignment for LAG interfaces, ISIS loopback interface requirements, and correct ISIS interface naming conventions. A key fix involved removing an erroneous metadata deviation that was causing fundamental ISIS adjacency failures. 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 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 counter productive. 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
|
Pull Request Functional Test Report for #5263 / e313438Virtual Devices
Hardware Devices
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the interface_ref_interface_id_format: true deviation from metadata.textproto. However, the reviewer points out that the PR appears incomplete, as it lacks corresponding code changes in the Go test file to configure a loopback interface for ISIS, which is necessary given the isis_loopback_required: true deviation. Without these additional changes, the test is likely to remain broken.
| wecmp_auto_unsupported: true | ||
| isis_loopback_required: true | ||
| weighted_ecmp_fixed_packet_verification: true | ||
| interface_ref_interface_id_format: true |
There was a problem hiding this comment.
While removing interface_ref_interface_id_format: true is a correct step for Cisco devices, this pull request appears to be incomplete. The PR description details other necessary changes in aggregate_all_not_forwarding_viable_test.go to fully resolve the issue, specifically the configuration of a loopback interface for ISIS as required by the isis_loopback_required: true deviation on line 29. This logic seems to be missing from the Go test file. Without these accompanying code changes, the test is likely to remain broken.
2 participants
RT-5.7 AGGREGATE ALL NOT VIABLE TEST - CODE CHANGES EXPLANATION
CHANGE #1: VRF ASSIGNMENT FIX FOR CISCO LAG INTERFACES
Location: aggregate_all_not_forwarding_viable_test.go, lines 529-537
Problem:
The test was calling fptest.AssignToNetworkInstance() for all interfaces
including LAG (Bundle-Ether) interfaces. On Cisco devices, LAG interfaces
cannot be directly assigned to a VRF/Network Instance.
Error Message:
"'RSI' detected the 'warning' condition 'The specified interface type does not
support VRFs'"
Code Change:
BEFORE:
fptest.AssignToNetworkInstance(t, dut, p.Name(),
deviations.DefaultNetworkInstance(dut), 0)
AFTER:
// Assign the interface to the default network instance if not CISCO
// Cisco LAG interfaces do not support VRF assignment
if dut.Vendor() != ondatra.CISCO {
fptest.AssignToNetworkInstance(t, dut, p.Name(),
deviations.DefaultNetworkInstance(dut), 0)
}
Explanation:
Cisco IOS-XR handles LAG interfaces differently from other vendors. Bundle-Ether
interfaces are implicitly part of the default network instance and cannot be
explicitly assigned to a VRF. The fix adds a vendor check to skip this
assignment for Cisco devices only.
CHANGE #2: LOOPBACK INTERFACE CONFIGURATION FOR ISIS
Location: aggregate_all_not_forwarding_viable_test.go, lines 561-564, 590-612
Problem:
Cisco requires a loopback interface for ISIS to function properly. The test
was not configuring Loopback0 when the ISISLoopbackRequired deviation is true.
Code Added:
Conditional loopback configuration call:
// Configure Loopback0 for Cisco where ISIS requires a loopback interface
if deviations.ISISLoopbackRequired(dut) {
configureLoopback(t, dut)
aggIDs = append(aggIDs, "Loopback0")
}
New configureLoopback function:
func configureLoopback(t *testing.T, dut *ondatra.DUTDevice) {
t.Helper()
d := &oc.Root{}
loopbackIntf := d.GetOrCreateInterface("Loopback0")
loopbackIntf.Name = ygot.String("Loopback0")
loopbackIntf.Description = ygot.String("Loopback for ISIS")
loopbackIntf.Type = oc.IETFInterfaces_InterfaceType_softwareLoopback
if deviations.InterfaceEnabled(dut) {
loopbackIntf.Enabled = ygot.Bool(true)
}
s := loopbackIntf.GetOrCreateSubinterface(0)
s4 := s.GetOrCreateIpv4()
if deviations.InterfaceEnabled(dut) {
s4.Enabled = ygot.Bool(true)
}
s4.GetOrCreateAddress(dutLoopback.IPv4).PrefixLength = ygot.Uint8(dutLoopback.IPv4Len)
s6 := s.GetOrCreateIpv6()
if deviations.InterfaceEnabled(dut) {
s6.Enabled = ygot.Bool(true)
}
s6.GetOrCreateAddress(dutLoopback.IPv6).PrefixLength = ygot.Uint8(dutLoopback.IPv6Len)
gnmi.Update(t, dut, gnmi.OC().Interface("Loopback0").Config(), loopbackIntf)
}
Explanation:
Cisco IOS-XR ISIS implementation requires a loopback interface to be configured
and included in ISIS for proper route advertisement. The Loopback0 interface:
CHANGE #3: ISIS LOOPBACK INTERFACE HANDLING
Location: aggregate_all_not_forwarding_viable_test.go, lines 801-840
Problem:
The configureDUTISIS function was treating all interfaces the same:
This caused:
Code Change:
BEFORE:
for _, aggID := range aggIDs {
isisIntf := isis.GetOrCreateInterface(aggID)
if deviations.InterfaceRefInterfaceIDFormat(dut) {
isisIntf = isis.GetOrCreateInterface(aggID + ".0")
}
// ... same treatment for all interfaces
isisIntf.CircuitType = oc.Isis_CircuitType_POINT_TO_POINT
AFTER:
for _, aggID := range aggIDs {
isLoopback := strings.HasPrefix(strings.ToLower(aggID), "loopback")
intfID := aggID
// Only add .0 suffix for non-loopback interfaces
// Loopback interfaces on Cisco do not use subinterface notation
if deviations.InterfaceRefInterfaceIDFormat(dut) && !isLoopback {
intfID = aggID + ".0"
}
isisIntf := isis.GetOrCreateInterface(intfID)
// ...
isisIntf.Enabled = ygot.Bool(true)
// Loopback interfaces should be passive (they don't form ISIS adjacencies)
if isLoopback {
isisIntf.SetPassive(true)
} else {
isisIntf.CircuitType = oc.Isis_CircuitType_POINT_TO_POINT
}
Explanation:
Loopback Detection: strings.HasPrefix(strings.ToLower(aggID), "loopback")
Skip ".0" suffix for loopback:
Set Passive for loopback:
send/receive ISIS hello packets on this interface
CircuitType only for non-loopback:
CHANGE #4: ISIS ADJACENCY LOOKUP PATH FIX
Location: aggregate_all_not_forwarding_viable_test.go, lines 243-246, 396-399,
1253-1261
Problem:
The awaitAdjacency function and direct gnmi.LookupAll calls were using the base
interface name (e.g., "Bundle-Ether2") to query ISIS adjacency state. However,
when InterfaceRefInterfaceIDFormat deviation is true, ISIS interface would be
configured as "Bundle-Ether2.0", causing lookups to fail.
Code Change:
Modified awaitAdjacency function:
func awaitAdjacency(t *testing.T, dut *ondatra.DUTDevice, intfName string,
state []oc.E_Isis_IsisInterfaceAdjState) bool {
isisIntfID := intfName
// When InterfaceRefInterfaceIDFormat is true, ISIS interface ID has .0 suffix
if deviations.InterfaceRefInterfaceIDFormat(dut) &&
!strings.HasPrefix(strings.ToLower(intfName), "loopback") {
isisIntfID = intfName + ".0"
}
isisPath := ocpath.Root().NetworkInstance(deviations.DefaultNetworkInstance(dut)).
Protocol(oc.PolicyTypes_INSTALL_PROTOCOL_TYPE_ISIS, isisInstance).Isis()
intf := isisPath.Interface(isisIntfID)
// ...
}
Fixed direct gnmi.LookupAll calls:
if ok := awaitAdjacency(...); !ok {
isisIntfID := aggIDs[1]
if deviations.InterfaceRefInterfaceIDFormat(dut) {
isisIntfID = aggIDs[1] + ".0"
}
if presence := gnmi.LookupAll(t, dut, ocpath.Root()...
.Isis().Interface(isisIntfID).LevelAny()...); len(presence) > 0 {
// ...
}
}
Explanation:
The ISIS state query path must match the configured ISIS interface name. When
InterfaceRefInterfaceIDFormat deviation is enabled, the ISIS interface is
configured with ".0" suffix, so queries must also use the same suffix.
CHANGE #5: METADATA DEVIATION FIX (CRITICAL)
Location: metadata.textproto, Cisco platform_exceptions section
Problem:
The test had
interface_ref_interface_id_format: truedeviation set for Cisco,but comparing with WORKING Cisco ISIS tests revealed they don't use this deviation:
This deviation was causing:
Code Change:
BEFORE:
platform_exceptions: {
platform: {
vendor: CISCO
}
deviations: {
interface_ref_config_unsupported:true
wecmp_auto_unsupported: true
isis_loopback_required: true
weighted_ecmp_fixed_packet_verification: true
interface_ref_interface_id_format: true <-- REMOVED
}
}
AFTER:
platform_exceptions: {
platform: {
vendor: CISCO
}
deviations: {
interface_ref_config_unsupported:true
wecmp_auto_unsupported: true
isis_loopback_required: true
weighted_ecmp_fixed_packet_verification: true
}
}
Explanation:
The interface_ref_interface_id_format deviation is used for certain interfaces
that require subinterface notation in their configuration. However, for Cisco
LAG interfaces in ISIS, this is not needed. Working Cisco ISIS tests use only:
Removing this unnecessary deviation ensures ISIS interfaces are configured
with the correct names that match the actual interface names on the device.
FILES MODIFIED
aggregate_all_not_forwarding_viable_test.go
metadata.textproto