Added Slow Collector Test. #4962
Conversation
Summary of ChangesHello @ashishpawar-google, 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 adds a robust test case designed to assess the resilience and performance of the Abstract Forwarding Table (AFT) when gNMI telemetry collectors experience delays. It simulates real-world conditions by introducing latency into a gNMI stream and then observes how the DUT's AFT converges and how system resources are utilized during various BGP route manipulation events. The test ensures that the AFT remains consistent and accurate even when telemetry updates are slowed. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Pull Request Functional Test Report for #4962 / 54da950Virtual 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 introduces a comprehensive new test for AFT (Abstract Forwarding Table) telemetry with a slow collector. The test covers various scenarios including initial sync, route withdrawal, and re-advertisement, while also monitoring DUT resource usage. The overall structure is good, but there are several areas for improvement. I've identified a critical issue with missing constant definitions that will cause compilation errors. Additionally, there are opportunities to improve code maintainability by refactoring duplicated code in configuration helpers and by structuring the main test function with t.Run for subtests as recommended by the style guide.
| cpuPctThreshold = 6.0 | ||
| memPctThreshold = 10.0 |
There was a problem hiding this comment.
The constants maxMemoryIncreasePercent and maxCPUIncreasePercent used in checkMemoryUsage and checkCPUUsage are not defined, which will cause a compilation error. Conversely, the constants cpuPctThreshold and memPctThreshold are defined but never used. It seems the intention was to use these values. Please define the required constants and remove the unused ones.
| cpuPctThreshold = 6.0 | |
| memPctThreshold = 10.0 | |
| maxCPUIncreasePercent = 6.0 | |
| maxMemoryIncreasePercent = 10.0 | |
| "github.com/google/go-cmp/cmp" | ||
| "github.com/google/go-grpc/grpc" | ||
| "github.com/google/go-grpc/metadata" | ||
| "github.com/google/go-cmp/cmp/cmpopts" |
There was a problem hiding this comment.
The imports in this file are not alphabetically sorted. According to the Google Go Style Guide, which is referenced in the repository's contribution guidelines, imports should be grouped by standard library, third-party, and local packages, and sorted alphabetically within each group. Running goimports would automatically fix this.
| "github.com/google/go-cmp/cmp" | |
| "github.com/google/go-grpc/grpc" | |
| "github.com/google/go-grpc/metadata" | |
| "github.com/google/go-cmp/cmp/cmpopts" | |
| "github.com/google/go-cmp/cmp" | |
| "github.com/google/go-cmp/cmp/cmpopts" | |
| "github.com/google/go-grpc/grpc" | |
| "github.com/google/go-grpc/metadata" | |
References
- The repository style guide requires following the Google Go Style Guide, which recommends sorting imports. (link)
| switch nbr.version { | ||
| case IPv4: | ||
| neighbor.SetPeerGroup(peerGrpNameV4) | ||
| neighbor.GetOrCreateAfiSafi(oc.BgpTypes_AFI_SAFI_TYPE_IPV4_UNICAST).SetEnabled(true) | ||
| neighbourAFV4 := peerGroupV4.GetOrCreateAfiSafi(oc.BgpTypes_AFI_SAFI_TYPE_IPV4_UNICAST) | ||
| neighbourAFV4.SetEnabled(true) | ||
| applyPolicy := neighbourAFV4.GetOrCreateApplyPolicy() | ||
| applyPolicy.ImportPolicy = []string{applyPolicyName} | ||
| applyPolicy.ExportPolicy = []string{applyPolicyName} | ||
| case IPv6: | ||
| neighbor.SetPeerGroup(peerGrpNameV6) | ||
| neighbor.GetOrCreateAfiSafi(oc.BgpTypes_AFI_SAFI_TYPE_IPV6_UNICAST).SetEnabled(true) | ||
| neighbourAFV6 := peerGroupV6.GetOrCreateAfiSafi(oc.BgpTypes_AFI_SAFI_TYPE_IPV6_UNICAST) | ||
| neighbourAFV6.SetEnabled(true) | ||
| applyPolicy := neighbourAFV6.GetOrCreateApplyPolicy() | ||
| applyPolicy.ImportPolicy = []string{applyPolicyName} | ||
| applyPolicy.ExportPolicy = []string{applyPolicyName} | ||
| } |
There was a problem hiding this comment.
The switch statement contains duplicated logic for configuring the AFI-SAFI policy for IPv4 and IPv6. This can be refactored to reduce repetition and improve maintainability, following the "Effective Go" principle of avoiding repetition. Consider creating a helper function or using local variables to abstract the common configuration logic.
References
- The repository style guide references 'Effective Go', which recommends avoiding repetition in code. (link)
| // Configuration on port1. | ||
| d1Eth := d1.Ethernets(). | ||
| Add(). | ||
| SetName(d1.Name() + ".eth"). | ||
| SetMac(port1MAC). | ||
| SetMtu(mtu) | ||
| d1Eth. | ||
| Connection(). | ||
| SetPortName(p1.Name()) | ||
|
|
||
| d1IPv4 := d1Eth. | ||
| Ipv4Addresses(). | ||
| Add(). | ||
| SetName(d1Eth.Name() + ".IPv4"). | ||
| SetAddress(ateP1.IPv4). | ||
| SetGateway(dutP1.IPv4). | ||
| SetPrefix(v4PrefixLen) | ||
|
|
||
| d1IPv6 := d1Eth. | ||
| Ipv6Addresses(). | ||
| Add(). | ||
| SetName(d1Eth.Name() + ".IPv6"). | ||
| SetAddress(ateP1.IPv6). | ||
| SetGateway(dutP1.IPv6). | ||
| SetPrefix(v6PrefixLen) | ||
|
|
||
| d1ISIS := d1.Isis(). | ||
| SetName(d1.Name() + ".isis"). | ||
| SetSystemId(isisSystemID) | ||
| d1ISIS.Basic(). | ||
| SetIpv4TeRouterId(d1IPv4.Address()). | ||
| SetHostname("ixia-c-port1") | ||
| d1ISIS.Advanced().SetAreaAddresses([]string{"49"}) | ||
| d1ISISInt := d1ISIS.Interfaces(). | ||
| Add(). | ||
| SetName(d1ISIS.Name() + ".intf"). | ||
| SetEthName(d1Eth.Name()). | ||
| SetNetworkType(gosnappi.IsisInterfaceNetworkType.POINT_TO_POINT). | ||
| SetLevelType(gosnappi.IsisInterfaceLevelType.LEVEL_2). | ||
| SetMetric(10) | ||
| d1ISISInt.TrafficEngineering().Add().PriorityBandwidths() | ||
| d1ISISInt.Advanced().SetAutoAdjustMtu(true).SetAutoAdjustArea(true).SetAutoAdjustSupportedProtocols(true) | ||
|
|
||
| d1ISISRoute := d1ISIS.V4Routes().Add().SetName(d1ISIS.Name() + ".rr") | ||
| d1ISISRoute.Addresses(). | ||
| Add(). | ||
| SetAddress(isisRoute). | ||
| SetPrefix(advertisedRoutesV4Prefix).SetCount(isisRouteCount) | ||
|
|
||
| d1ISISRouteV6 := d1ISIS.V6Routes().Add().SetName(d1ISISRoute.Name() + ".v6") | ||
| d1ISISRouteV6.Addresses(). | ||
| Add(). | ||
| SetAddress(isisRoutev6). | ||
| SetPrefix(advertisedRoutesV6Prefix).SetCount(isisRouteCount) | ||
|
|
||
| tc.configureBGPDev(d1, d1IPv4, d1IPv6) | ||
|
|
||
| // Configuration on port2 | ||
| d2Eth := d2.Ethernets(). | ||
| Add(). | ||
| SetName(d2.Name() + ".eth"). | ||
| SetMac(port2MAC). | ||
| SetMtu(mtu) | ||
| d2Eth. | ||
| Connection(). | ||
| SetPortName(p2.Name()) | ||
| d2IPv4 := d2Eth.Ipv4Addresses(). | ||
| Add(). | ||
| SetName(d2Eth.Name() + ".IPv4"). | ||
| SetAddress(ateP2.IPv4). | ||
| SetGateway(dutP2.IPv4). | ||
| SetPrefix(v4PrefixLen) | ||
|
|
||
| d2IPv6 := d2Eth. | ||
| Ipv6Addresses(). | ||
| Add(). | ||
| SetName(d2Eth.Name() + ".IPv6"). | ||
| SetAddress(ateP2.IPv6). | ||
| SetGateway(dutP2.IPv6). | ||
| SetPrefix(v6PrefixLen) | ||
|
|
||
| d2ISIS := d2.Isis(). | ||
| SetName(d2.Name() + ".isis"). | ||
| SetSystemId(isisSystemID) | ||
| d2ISIS.Basic(). | ||
| SetIpv4TeRouterId(d2IPv4.Address()). | ||
| SetHostname("ixia-c-port2") | ||
| d2ISIS.Advanced().SetAreaAddresses([]string{"49"}) | ||
| d2ISISInt := d2ISIS.Interfaces(). | ||
| Add(). | ||
| SetName(d2ISIS.Name() + ".intf"). | ||
| SetEthName(d2Eth.Name()). | ||
| SetNetworkType(gosnappi.IsisInterfaceNetworkType.POINT_TO_POINT). | ||
| SetLevelType(gosnappi.IsisInterfaceLevelType.LEVEL_2). | ||
| SetMetric(10) | ||
| d2ISISInt.TrafficEngineering().Add().PriorityBandwidths() | ||
| d2ISISInt.Advanced().SetAutoAdjustMtu(true).SetAutoAdjustArea(true).SetAutoAdjustSupportedProtocols(true) | ||
|
|
||
| d2ISISRoute := d2ISIS.V4Routes().Add().SetName(d2ISIS.Name() + ".rr") | ||
| d2ISISRoute.Addresses(). | ||
| Add(). | ||
| SetAddress(isisRoute). | ||
| SetPrefix(advertisedRoutesV4Prefix). | ||
| SetCount(isisRouteCount) | ||
|
|
||
| d2ISISRouteV6 := d2ISIS.V6Routes().Add().SetName(d2ISISRoute.Name() + ".v6") | ||
| d2ISISRouteV6.Addresses(). | ||
| Add(). | ||
| SetAddress(isisRoutev6). | ||
| SetPrefix(advertisedRoutesV6Prefix). | ||
| SetCount(isisRouteCount) | ||
|
|
||
| tc.configureBGPDev(d2, d2IPv4, d2IPv6) |
There was a problem hiding this comment.
The configureATE function contains a large amount of duplicated code for configuring ATE devices d1 (on port1) and d2 (on port2). This makes the function long and hard to maintain. This code can be refactored into a helper function to improve readability and maintainability, in line with the "Effective Go" principle of avoiding repetition.
For example, you could create a helper function like configureATEDevice that takes parameters for the device, port, IPs, MAC, etc., and call it for each ATE port.
func (tc *testCase) configureATEDevice(t *testing.T, config gosnappi.Config, port gosnappi.Port, dev gosnappi.Device, mac string, ateAttrs, dutAttrs attrs.Attributes) {
// ... configuration logic for one device ...
}
func (tc *testCase) configureATE(t *testing.T) {
// ...
tc.configureATEDevice(t, config, p1, d1, port1MAC, ateP1, dutP1)
tc.configureATEDevice(t, config, p2, d2, port2MAC, ateP2, dutP2)
// ...
}References
- The repository style guide references 'Effective Go', which recommends avoiding repetition in code. (link)
| // AFT-1.2.2: AFT route withdrawal from one BGP peer scenario 1 | ||
| t.Log("Subtest 2: Withdraw BGP routes from ATE port 2...") | ||
|
|
||
| p2Name := ate.Port(t, port2Name).ID() | ||
| d2Name := p2Name + ".d2" | ||
| bgp4PeerName := d2Name + ".BGP4.peer" | ||
| bgp6PeerName := d2Name + ".BGP6.peer" | ||
| v4RouteName := bgp4PeerName + ".v4route" | ||
| v6RouteName := bgp6PeerName + ".v6route" | ||
| controlState := gosnappi.NewControlState() | ||
| controlState.Protocol().Route(). | ||
| SetNames([]string{v4RouteName, v6RouteName}). | ||
| SetState(gosnappi.StateProtocolRouteState.WITHDRAW) | ||
| ate.OTG().SetControlState(t, controlState) | ||
|
|
||
| t.Log("SubTest 2: Withdrew BGP routes from ATE port 2.") | ||
| if err := tc.waitForBGPSessions(t, []string{ateP1.IPv4}, []string{ateP1.IPv6}); err != nil { | ||
| t.Fatalf("Unable to establish BGP session: %v", err) | ||
| } | ||
|
|
||
| startTime = time.Now() | ||
| wantIPv4NHsAfterWithdraw := map[string]bool{ateP1.IPv4: true} | ||
| wantIPv6NHsAfterWithdraw := getPostChurnIPv6NH(dut) | ||
| stoppingCondition = aftcache.InitialSyncStoppingCondition(t, dut, wantPrefixes, wantIPv4NHsAfterWithdraw, wantIPv6NHsAfterWithdraw) | ||
|
|
||
| waitAndVerify("route withdrawal convergence", "post-route-withdrawal-port2", 1, false) | ||
|
|
||
| // AFT-1.2.3: AFT Withdraw prefixes from both the BGP neighbor scenario 2 | ||
| t.Log("Subtest 3: Withdraw BGP routes from ATE port 1...") | ||
|
|
||
| p1Name := ate.Port(t, port1Name).ID() | ||
| d1Name := p1Name + ".d1" | ||
| bgp4PeerNameP1 := d1Name + ".BGP4.peer" | ||
| bgp6PeerNameP1 := d1Name + ".BGP6.peer" | ||
| v4RouteNameP1 := bgp4PeerNameP1 + ".v4route" | ||
| v6RouteNameP1 := bgp6PeerNameP1 + ".v6route" | ||
| controlStateP1 := gosnappi.NewControlState() | ||
| controlStateP1.Protocol().Route(). | ||
| SetNames([]string{v4RouteNameP1, v6RouteNameP1}). | ||
| SetState(gosnappi.StateProtocolRouteState.WITHDRAW) | ||
| ate.OTG().SetControlState(t, controlStateP1) | ||
|
|
||
| t.Log("Withdrew BGP routes from ATE port 1. Waiting for convergence...") | ||
|
|
||
| startTime = time.Now() | ||
| // Stopping condition: Wait for BGP routes to be deleted. | ||
| stoppingCondition = aftcache.DeletionStoppingCondition(t, dut, wantPrefixes) | ||
|
|
||
| waitAndVerify("complete route withdrawal convergence", "post-route-withdrawal-all", 0, true) | ||
|
|
||
| // AFT-1.2.4: AFT readvertise the prefixes from port1 bgp neighbor scenario 3 | ||
| t.Log("Subtest 4: Re-advertise BGP routes from ATE port 1...") | ||
| if err := tc.waitForBGPSessions(t, []string{ateP1.IPv4}, []string{ateP1.IPv6}); err != nil { | ||
| t.Fatalf("Unable to establish BGP session: %v", err) | ||
| } | ||
|
|
||
| controlStateP1Adv := gosnappi.NewControlState() | ||
| controlStateP1Adv.Protocol().Route(). | ||
| SetNames([]string{v4RouteNameP1, v6RouteNameP1}). | ||
| SetState(gosnappi.StateProtocolRouteState.ADVERTISE) | ||
| ate.OTG().SetControlState(t, controlStateP1Adv) | ||
|
|
||
| t.Log("Re-advertised BGP routes from ATE port 1. Waiting for convergence...") | ||
|
|
||
| startTime = time.Now() | ||
| wantIPv4NHsP1 := map[string]bool{ateP1.IPv4: true} | ||
| wantIPv6NHsP1 := getPostChurnIPv6NH(dut) | ||
| stoppingCondition = aftcache.InitialSyncStoppingCondition(t, dut, wantPrefixes, wantIPv4NHsP1, wantIPv6NHsP1) | ||
|
|
||
| waitAndVerify("re-advertise convergence", "post-readvertise-port1", 1, false) | ||
|
|
||
| // AFT-1.2.5: AFT Readvertise the BGP prefixes from both the neighbors scenario 4 | ||
| t.Log("Subtest 5: Re-advertise BGP routes from ATE port 1 and port 2...") | ||
| if err := tc.waitForBGPSessions(t, []string{ateP1.IPv4, ateP2.IPv4}, []string{ateP1.IPv6, ateP2.IPv6}); err != nil { | ||
| t.Fatalf("Unable to establish BGP session: %v", err) | ||
| } | ||
|
|
||
| controlStateAllAdv := gosnappi.NewControlState() | ||
| controlStateAllAdv.Protocol().Route(). | ||
| SetNames([]string{v4RouteName, v6RouteName, v4RouteNameP1, v6RouteNameP1}). | ||
| SetState(gosnappi.StateProtocolRouteState.ADVERTISE) | ||
| ate.OTG().SetControlState(t, controlStateAllAdv) | ||
|
|
||
| t.Log("Re-advertised BGP routes from ATE port 1 and 2. Waiting for convergence...") | ||
|
|
||
| startTime = time.Now() | ||
| stoppingCondition = aftcache.InitialSyncStoppingCondition(t, dut, wantPrefixes, wantIPv4NHs, wantIPv6NHs) | ||
|
|
||
| waitAndVerify("re-advertise convergence", "post-readvertise-all", 2, false) |
There was a problem hiding this comment.
The repository style guide states, "Use t.Run for subtests so output clearly reflects passed/failed steps." This test implements multiple logical sub-scenarios sequentially within the TestSlowCollector function, identified by comments like // AFT-1.2.2: .... To improve test readability, output clarity, and adherence to the style guide, these scenarios should be refactored into t.Run blocks.
For example, the route withdrawal test could be wrapped as follows:
t.Run("AFT-1.2.2: Withdraw BGP routes from ATE port 2", func(t *testing.T) {
t.Log("Subtest 2: Withdraw BGP routes from ATE port 2...")
// ... logic for withdrawing routes from port 2 ...
waitAndVerify("route withdrawal convergence", "post-route-withdrawal-port2", 1, false)
})This should be applied to all logical subtests in this function.
References
- The repository style guide requires the use of
t.Runfor subtests to ensure clear output for passed/failed steps. (link)
Pull Request Test Coverage Report for Build 20651105042Details
💛 - Coveralls |
ashishpawar-google
force-pushed
the
aft_slow_collector_test
branch
8 times, most recently
from
1427fbf to
81c928a
Compare
ashishpawar-google
force-pushed
the
aft_slow_collector_test
branch
from
81c928a to
54da950
Compare
3 participants
Added Slow collector test for AFT including subtests.