Add test RT-1.102 - #5818
Conversation
Pull Request Functional Test Report for #5818 / 5181db6Virtual 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 introduces a new test suite, RT-1.102, aimed at verifying BGP VRF L3VPN parameters. The implementation includes comprehensive test cases for eBGP session establishment, attribute validation, prefix limit enforcement, isolation boundaries, and graceful restart functionality. The PR also provides necessary infrastructure updates to support vendor-specific CLI configurations and platform deviations, ensuring compatibility with Arista devices. 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
|
![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 introduces a new test suite for BGP VRF L3VPN parameters, along with Arista platform exceptions and helper functions for configuring route targets via CLI. The reviewer feedback highlights several critical and high-priority improvements: a potential nil pointer dereference when checking fatal messages, a missing state reversion cleanup using t.Cleanup(), multiple performance bottlenecks due to serial gnmi.Get calls that should be batched using gnmi.OCBatch(), and a long static sleep during Graceful Restart tests that should be minimized to optimize pipeline execution speed.
| }); *fatalMsg != "" { | ||
| return fmt.Errorf("ATE core VPN prefix absence verification failed: %s", *fatalMsg) | ||
| } |
There was a problem hiding this comment.
The code dereferences fatalMsg directly without checking if it is nil. If no fatal error is captured, testt.CaptureFatal returns nil, which will cause a nil pointer dereference panic. Please check if fatalMsg != nil before dereferencing it, similar to awaitATECoreVPNPrefixPresence.
| }); *fatalMsg != "" { | |
| return fmt.Errorf("ATE core VPN prefix absence verification failed: %s", *fatalMsg) | |
| } | |
| }); fatalMsg != nil { | |
| return fmt.Errorf("ATE core VPN prefix absence verification failed: %s", *fatalMsg) | |
| } |
| defaultNI = deviations.DefaultNetworkInstance(dut) | ||
| configureDUT(t, dut) | ||
| configureATE(t, ate) |
There was a problem hiding this comment.
According to the repository style guide (Section 1, Configuration & Cleanup), tests must always leave the system in the exact original state it was in prior to the test execution, and all cleanup operations must be registered using t.Cleanup(). Please register a cleanup function to delete the configured BGP protocols, VRFs, and routing policies.
defaultNI = deviations.DefaultNetworkInstance(dut)
configureDUT(t, dut)
t.Cleanup(func() {
cleanBatch := &gnmi.SetBatch{}
gnmi.BatchDelete(cleanBatch, gnmi.OC().NetworkInstance(defaultNI).Protocol(oc.PolicyTypes_INSTALL_PROTOCOL_TYPE_BGP, bgpProtocolName).Config())
gnmi.BatchDelete(cleanBatch, gnmi.OC().NetworkInstance(vrf100).Config())
gnmi.BatchDelete(cleanBatch, gnmi.OC().NetworkInstance(vrf200).Config())
gnmi.BatchDelete(cleanBatch, gnmi.OC().RoutingPolicy().PolicyDefinition(rplName).Config())
cleanBatch.Set(t, dut)
})
configureATE(t, ate)References
- Mandatory State Reversion: Tests must always leave the system in the exact original state it was in prior to the test execution, regardless of whether the test passes or fails. Use t.Cleanup() to guarantee they are executed. (link)
| func verifyL3VPNExportConfig(t *testing.T, dut *ondatra.DUTDevice) error { | ||
| t.Helper() | ||
| var errs []error | ||
| if !deviations.NetworkInstanceImportExportPolicyOCUnsupported(dut) { | ||
| ni := gnmi.OC().NetworkInstance(vrf100) | ||
| if got := gnmi.Get(t, dut, ni.RouteDistinguisher().State()); got != vrf100RD { | ||
| errs = append(errs, fmt.Errorf("VRF_100 route distinguisher: got %q, want %q", got, vrf100RD)) | ||
| } | ||
| policy := ni.InterInstancePolicies().ImportExportPolicy() | ||
| errs = append(errs, verifyRouteTarget("import", gnmi.Get(t, dut, policy.ImportRouteTarget().State()), vrf100RT)) | ||
| errs = append(errs, verifyRouteTarget("export", gnmi.Get(t, dut, policy.ExportRouteTarget().State()), vrf100RT)) | ||
| } | ||
|
|
||
| bgpPath := gnmi.OC().NetworkInstance(defaultNI).Protocol(oc.PolicyTypes_INSTALL_PROTOCOL_TYPE_BGP, bgpProtocolName).Bgp() | ||
| for _, pgName := range []string{pgCoreV4, pgCoreV6} { | ||
| communities := gnmi.Get(t, dut, bgpPath.PeerGroup(pgName).SendCommunityType().State()) | ||
| if !containsCommunity(communities, oc.Bgp_CommunityType_EXTENDED) && !containsCommunity(communities, oc.Bgp_CommunityType_BOTH) { | ||
| errs = append(errs, fmt.Errorf("core peer-group %s does not send extended communities: got %v", pgName, communities)) | ||
| } | ||
| } | ||
| return errors.Join(errs...) | ||
| } |
There was a problem hiding this comment.
According to the repository style guide (Section 4, Performance & Execution Optimizations), serial gnmi.Get calls should be avoided to prevent execution slowness. Please use gnmi.OCBatch() to aggregate paths and perform a single RPC call.
func verifyL3VPNExportConfig(t *testing.T, dut *ondatra.DUTDevice) error {
t.Helper()
var errs []error
batch := gnmi.OCBatch()
if !deviations.NetworkInstanceImportExportPolicyOCUnsupported(dut) {
batch.AddPaths(
gnmi.OC().NetworkInstance(vrf100).RouteDistinguisher(),
gnmi.OC().NetworkInstance(vrf100).InterInstancePolicies().ImportExportPolicy().ImportRouteTarget(),
gnmi.OC().NetworkInstance(vrf100).InterInstancePolicies().ImportExportPolicy().ExportRouteTarget(),
)
}
bgpPath := gnmi.OC().NetworkInstance(defaultNI).Protocol(oc.PolicyTypes_INSTALL_PROTOCOL_TYPE_BGP, bgpProtocolName).Bgp()
for _, pgName := range []string{pgCoreV4, pgCoreV6} {
batch.AddPaths(bgpPath.PeerGroup(pgName).SendCommunityType())
}
root := gnmi.Get(t, dut, batch.State())
if !deviations.NetworkInstanceImportExportPolicyOCUnsupported(dut) {
ni := root.GetNetworkInstance(vrf100)
if got := ni.GetRouteDistinguisher(); got != vrf100RD {
errs = append(errs, fmt.Errorf("VRF_100 route distinguisher: got %q, want %q", got, vrf100RD))
}
policy := ni.GetInterInstancePolicies().GetImportExportPolicy()
errs = append(errs, verifyRouteTarget("import", policy.GetImportRouteTarget(), vrf100RT))
errs = append(errs, verifyRouteTarget("export", policy.GetExportRouteTarget(), vrf100RT))
}
for _, pgName := range []string{pgCoreV4, pgCoreV6} {
communities := root.GetNetworkInstance(defaultNI).GetProtocol(oc.PolicyTypes_INSTALL_PROTOCOL_TYPE_BGP, bgpProtocolName).GetBgp().GetPeerGroup(pgName).GetSendCommunityType()
if !containsCommunity(communities, oc.Bgp_CommunityType_EXTENDED) && !containsCommunity(communities, oc.Bgp_CommunityType_BOTH) {
errs = append(errs, fmt.Errorf("core peer-group %s does not send extended communities: got %v", pgName, communities))
}
}
return errors.Join(errs...)
}References
- Batching gNMI Operations (Avoid 'N+1' Queries): Use gnmi.OCBatch() to aggregate paths and perform a single RPC call. (link)
| func verifyPrefixLimitConfig(t *testing.T, dut *ondatra.DUTDevice, want uint32) error { | ||
| t.Helper() | ||
| bgpPath := gnmi.OC().NetworkInstance(vrf100).Protocol(oc.PolicyTypes_INSTALL_PROTOCOL_TYPE_BGP, bgpProtocolName).Bgp() | ||
|
|
||
| v4 := bgpPath.Neighbor(atePort1.IPv4).AfiSafi(oc.BgpTypes_AFI_SAFI_TYPE_IPV4_UNICAST) | ||
| v6 := bgpPath.Neighbor(atePort1.IPv6).AfiSafi(oc.BgpTypes_AFI_SAFI_TYPE_IPV6_UNICAST) | ||
|
|
||
| var gotV4, gotV6 uint32 | ||
| if deviations.BGPExplicitPrefixLimitReceived(dut) { | ||
| gotV4 = gnmi.Get(t, dut, v4.Ipv4Unicast().PrefixLimitReceived().MaxPrefixes().State()) | ||
| gotV6 = gnmi.Get(t, dut, v6.Ipv6Unicast().PrefixLimitReceived().MaxPrefixes().State()) | ||
| } else { | ||
| gotV4 = gnmi.Get(t, dut, v4.Ipv4Unicast().PrefixLimit().MaxPrefixes().State()) | ||
| gotV6 = gnmi.Get(t, dut, v6.Ipv6Unicast().PrefixLimit().MaxPrefixes().State()) | ||
| } | ||
| var errs []error | ||
| if gotV4 != want { | ||
| errs = append(errs, fmt.Errorf("IPv4 prefix-limit max-prefixes: got %d, want %d", gotV4, want)) | ||
| } | ||
| if gotV6 != want { | ||
| errs = append(errs, fmt.Errorf("IPv6 prefix-limit max-prefixes: got %d, want %d", gotV6, want)) | ||
| } | ||
| return errors.Join(errs...) | ||
| } |
There was a problem hiding this comment.
Avoid serial gnmi.Get calls by batching them using gnmi.OCBatch() to improve test execution speed.
func verifyPrefixLimitConfig(t *testing.T, dut *ondatra.DUTDevice, want uint32) error {
t.Helper()
bgpPath := gnmi.OC().NetworkInstance(vrf100).Protocol(oc.PolicyTypes_INSTALL_PROTOCOL_TYPE_BGP, bgpProtocolName).Bgp()
v4 := bgpPath.Neighbor(atePort1.IPv4).AfiSafi(oc.BgpTypes_AFI_SAFI_TYPE_IPV4_UNICAST)
v6 := bgpPath.Neighbor(atePort1.IPv6).AfiSafi(oc.BgpTypes_AFI_SAFI_TYPE_IPV6_UNICAST)
batch := gnmi.OCBatch()
if deviations.BGPExplicitPrefixLimitReceived(dut) {
batch.AddPaths(v4.Ipv4Unicast().PrefixLimitReceived().MaxPrefixes(), v6.Ipv6Unicast().PrefixLimitReceived().MaxPrefixes())
} else {
batch.AddPaths(v4.Ipv4Unicast().PrefixLimit().MaxPrefixes(), v6.Ipv6Unicast().PrefixLimit().MaxPrefixes())
}
root := gnmi.Get(t, dut, batch.State())
var gotV4, gotV6 uint32
nbrV4 := root.GetNetworkInstance(vrf100).GetProtocol(oc.PolicyTypes_INSTALL_PROTOCOL_TYPE_BGP, bgpProtocolName).GetBgp().GetNeighbor(atePort1.IPv4).GetAfiSafi(oc.BgpTypes_AFI_SAFI_TYPE_IPV4_UNICAST)
nbrV6 := root.GetNetworkInstance(vrf100).GetProtocol(oc.PolicyTypes_INSTALL_PROTOCOL_TYPE_BGP, bgpProtocolName).GetBgp().GetNeighbor(atePort1.IPv6).GetAfiSafi(oc.BgpTypes_AFI_SAFI_TYPE_IPV6_UNICAST)
if deviations.BGPExplicitPrefixLimitReceived(dut) {
gotV4 = nbrV4.GetIpv4Unicast().GetPrefixLimitReceived().GetMaxPrefixes()
gotV6 = nbrV6.GetIpv6Unicast().GetPrefixLimitReceived().GetMaxPrefixes()
} else {
gotV4 = nbrV4.GetIpv4Unicast().GetPrefixLimit().GetMaxPrefixes()
gotV6 = nbrV6.GetIpv6Unicast().GetPrefixLimit().GetMaxPrefixes()
}
var errs []error
if gotV4 != want {
errs = append(errs, fmt.Errorf("IPv4 prefix-limit max-prefixes: got %d, want %d", gotV4, want))
}
if gotV6 != want {
errs = append(errs, fmt.Errorf("IPv6 prefix-limit max-prefixes: got %d, want %d", gotV6, want))
}
return errors.Join(errs...)
}References
- Batching gNMI Operations (Avoid 'N+1' Queries): Use gnmi.OCBatch() to aggregate paths and perform a single RPC call. (link)
| func establishedTransitions(t *testing.T, dut *ondatra.DUTDevice) map[string]uint64 { | ||
| t.Helper() | ||
| bgpPath := gnmi.OC().NetworkInstance(vrf100).Protocol(oc.PolicyTypes_INSTALL_PROTOCOL_TYPE_BGP, bgpProtocolName).Bgp() | ||
| return map[string]uint64{ | ||
| atePort1.IPv4: gnmi.Get(t, dut, bgpPath.Neighbor(atePort1.IPv4).EstablishedTransitions().State()), | ||
| atePort1.IPv6: gnmi.Get(t, dut, bgpPath.Neighbor(atePort1.IPv6).EstablishedTransitions().State()), | ||
| } | ||
| } |
There was a problem hiding this comment.
Batch the serial gnmi.Get calls using gnmi.OCBatch() to optimize performance.
func establishedTransitions(t *testing.T, dut *ondatra.DUTDevice) map[string]uint64 {
t.Helper()
bgpPath := gnmi.OC().NetworkInstance(vrf100).Protocol(oc.PolicyTypes_INSTALL_PROTOCOL_TYPE_BGP, bgpProtocolName).Bgp()
batch := gnmi.OCBatch()
batch.AddPaths(
bgpPath.Neighbor(atePort1.IPv4).EstablishedTransitions(),
bgpPath.Neighbor(atePort1.IPv6).EstablishedTransitions(),
)
root := gnmi.Get(t, dut, batch.State())
return map[string]uint64{
atePort1.IPv4: root.GetNetworkInstance(vrf100).GetProtocol(oc.PolicyTypes_INSTALL_PROTOCOL_TYPE_BGP, bgpProtocolName).GetBgp().GetNeighbor(atePort1.IPv4).GetEstablishedTransitions(),
atePort1.IPv6: root.GetNetworkInstance(vrf100).GetProtocol(oc.PolicyTypes_INSTALL_PROTOCOL_TYPE_BGP, bgpProtocolName).GetBgp().GetNeighbor(atePort1.IPv6).GetEstablishedTransitions(),
}
}References
- Batching gNMI Operations (Avoid 'N+1' Queries): Use gnmi.OCBatch() to aggregate paths and perform a single RPC call. (link)
| func receivedNotificationCounts(t *testing.T, dut *ondatra.DUTDevice) map[string]uint64 { | ||
| t.Helper() | ||
| bgpPath := gnmi.OC().NetworkInstance(vrf100).Protocol(oc.PolicyTypes_INSTALL_PROTOCOL_TYPE_BGP, bgpProtocolName).Bgp() | ||
| return map[string]uint64{ | ||
| atePort1.IPv4: gnmi.Get(t, dut, bgpPath.Neighbor(atePort1.IPv4).Messages().Received().NOTIFICATION().State()), | ||
| atePort1.IPv6: gnmi.Get(t, dut, bgpPath.Neighbor(atePort1.IPv6).Messages().Received().NOTIFICATION().State()), | ||
| } | ||
| } |
There was a problem hiding this comment.
Batch the serial gnmi.Get calls using gnmi.OCBatch() to optimize performance.
func receivedNotificationCounts(t *testing.T, dut *ondatra.DUTDevice) map[string]uint64 {
t.Helper()
bgpPath := gnmi.OC().NetworkInstance(vrf100).Protocol(oc.PolicyTypes_INSTALL_PROTOCOL_TYPE_BGP, bgpProtocolName).Bgp()
batch := gnmi.OCBatch()
batch.AddPaths(
bgpPath.Neighbor(atePort1.IPv4).Messages().Received().NOTIFICATION(),
bgpPath.Neighbor(atePort1.IPv6).Messages().Received().NOTIFICATION(),
)
root := gnmi.Get(t, dut, batch.State())
return map[string]uint64{
atePort1.IPv4: root.GetNetworkInstance(vrf100).GetProtocol(oc.PolicyTypes_INSTALL_PROTOCOL_TYPE_BGP, bgpProtocolName).GetBgp().GetNeighbor(atePort1.IPv4).GetMessages().GetReceived().GetNOTIFICATION(),
atePort1.IPv6: root.GetNetworkInstance(vrf100).GetProtocol(oc.PolicyTypes_INSTALL_PROTOCOL_TYPE_BGP, bgpProtocolName).GetBgp().GetNeighbor(atePort1.IPv6).GetMessages().GetReceived().GetNOTIFICATION(),
}
}References
- Batching gNMI Operations (Avoid 'N+1' Queries): Use gnmi.OCBatch() to aggregate paths and perform a single RPC call. (link)
| grRestartTime = uint16(120) | ||
| grHelperExtraWait = 30 * time.Second |
There was a problem hiding this comment.
The Graceful Restart restart-time is currently set to 120 seconds, which results in a static sleep of 150 seconds (2.5 minutes) during the timeout scenario in testGracefulRestartInVRF. To speed up test execution and prevent pipeline bottlenecks, consider reducing grRestartTime to a smaller value (e.g., 30 seconds) and adjusting grHelperExtraWait accordingly.
| grRestartTime = uint16(120) | |
| grHelperExtraWait = 30 * time.Second | |
| grRestartTime = uint16(30) | |
| grHelperExtraWait = 10 * time.Second |
References
- Eliminating Static Sleeps: Static sleeps become a fixed tax on the pipeline. Minimize wait times where possible. (link)
2 participants
No description provided.