Adding test for AFT-6.2#5523
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 introduces a new test case for AFT-6.2, focusing on dual-stack prefix filtering within the AFT (Abstract Forwarding Table). It includes the necessary test logic to configure routing policies, static routes, and verify filtering outcomes. Additionally, it adds infrastructure support for platform-specific deviations, allowing the test to run on hardware that requires CLI-based configuration for global filter policies instead of standard gNMI/OpenConfig paths. 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 implements the AFT Prefix Filtering Dual-Stack test (AFT-6.2), adding a new test file, metadata, and a deviation for Arista devices where the AFT global filter policy OC path is unsupported. The feedback recommends replacing t.Context() with context.Background() for Go compatibility, verifying IPv6 prefixes directly from the streamed prefixes rather than querying the DUT state, optimizing the state polling loop using gnmiC.Get instead of repeated ONCE subscriptions, and reusing existing sessions to verify prefix deletion. Additionally, the deviation comment should be updated to fix a path typo and include the required issue tracker URL tracking its removal.
| *aftcache.AFTStreamSession, *aftcache.AFTStreamSession, aftcache.PeriodicHook, | ||
| ) { | ||
| t.Helper() | ||
| ctx := t.Context() |
There was a problem hiding this comment.
t.Context() was introduced in Go 1.24. Since the repository may be compiled in environments running older Go versions (such as Go 1.22 or 1.23), using t.Context() will cause compilation failures. Use context.Background() or pass a context to ensure compatibility.
| ctx := t.Context() | |
| ctx := context.Background() |
| // mustRunAFTSessions runs both sessions concurrently and returns the converged AFTData from session1. | ||
| func mustRunAFTSessions(t *testing.T, cfg aftSessionConfig) *aftcache.AFTData { | ||
| t.Helper() | ||
| ctx := t.Context() |
There was a problem hiding this comment.
t.Context() was introduced in Go 1.24. Since the repository may be compiled in environments running older Go versions (such as Go 1.22 or 1.23), using t.Context() will cause compilation failures. Use context.Background() or pass a context to ensure compatibility.
| ctx := t.Context() | |
| ctx := context.Background() |
| // Verify IPv6 prefixes exist in AFT state. | ||
| t.Log("Step 4: Verifying IPv6 entries present in AFT state") | ||
| niAfts := gnmi.OC().NetworkInstance(deviations.DefaultNetworkInstance(dut)).Afts() | ||
| for _, pfx := range ipv6MatchPrefixes { | ||
| _, ok := gnmi.Watch(t, dut, niAfts.Ipv6Entry(pfx).State(), operationTimeout, | ||
| func(v *ygnmi.Value[*oc.NetworkInstance_Afts_Ipv6Entry]) bool { | ||
| entry, present := v.Val() | ||
| return present && entry != nil | ||
| }).Await(t) | ||
| if !ok { | ||
| t.Errorf("ipv6 prefix %s not found in AFT state", pfx) | ||
| } | ||
| } |
There was a problem hiding this comment.
The test is designed to validate dual-stack AFT prefix filtering over the streamed gNMI ON_CHANGE subscription. However, in Step 4, the IPv6 prefixes are verified by querying the DUT's state directly using gnmi.Watch instead of verifying them from the streamed aft.Prefixes (as done for IPv4 in Step 3). This bypasses the streaming validation for IPv6. Verify the IPv6 prefixes from the streamed aft.Prefixes to ensure the streaming filter works correctly for both address families.
// Verify IPv6 prefixes matched by POLICY-PREFIX-SET-B.
t.Log("Step 4: Verifying IPv6 entries match POLICY-PREFIX-SET-B")
for _, pfx := range ipv6MatchPrefixes {
if _, ok := aft.Prefixes[pfx]; !ok {
t.Errorf("ipv6 prefix %s expected in AFT stream but not received", pfx)
}
}| for time.Now().Before(deadline) { | ||
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | ||
| stream, err := gnmiC.Subscribe(ctx) | ||
| if err != nil { | ||
| cancel() | ||
| time.Sleep(pollInterval) | ||
| continue | ||
| } | ||
| err = stream.Send(&gpb.SubscribeRequest{ | ||
| Request: &gpb.SubscribeRequest_Subscribe{ | ||
| Subscribe: &gpb.SubscriptionList{ | ||
| Mode: gpb.SubscriptionList_ONCE, | ||
| Subscription: []*gpb.Subscription{{Path: statePath}}, | ||
| }, | ||
| }, | ||
| }) | ||
| if err != nil { | ||
| cancel() | ||
| time.Sleep(pollInterval) | ||
| continue | ||
| } | ||
| resp, err := stream.Recv() | ||
| cancel() | ||
| if err != nil { | ||
| time.Sleep(pollInterval) | ||
| continue | ||
| } | ||
| for _, u := range resp.GetUpdate().GetUpdate() { | ||
| if u.GetVal().GetStringVal() == wantVal { | ||
| t.Logf("global-filter state/%s = %q confirmed", leaf, wantVal) | ||
| return nil | ||
| } | ||
| } | ||
| time.Sleep(pollInterval) |
There was a problem hiding this comment.
Using gnmiC.Subscribe with ONCE mode inside a manual polling loop is highly inefficient as it repeatedly establishes and tears down gNMI subscription streams. Instead, use gnmiC.Get to retrieve the state value directly in each iteration of the loop.
for time.Now().Before(deadline) {
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
resp, err := gnmiC.Get(ctx, &gpb.GetRequest{
Path: []*gpb.Path{statePath},
Type: gpb.GetRequest_STATE,
})
cancel()
if err != nil {
time.Sleep(pollInterval)
continue
}
for _, n := range resp.GetNotification() {
for _, u := range n.GetUpdate() {
if u.GetVal().GetStringVal() == wantVal {
t.Logf("global-filter state/%s = %q confirmed", leaf, wantVal)
return nil
}
}
}
time.Sleep(pollInterval)
}| // Open a new subscription and verify matched prefixes are removed. | ||
| t.Log("Step 7: Opening AFT subscription with swapped policy") | ||
| session1Swapped, session2Swapped, stoppingCondSwapped := mustOpenAFTSessions(t, dut, map[string]bool{}) | ||
| aftSwapped := mustRunAFTSessions(t, aftSessionConfig{session1: session1Swapped, session2: session2Swapped, stop: stoppingCondSwapped, dut: dut}) |
There was a problem hiding this comment.
Opening a new gNMI subscription to verify that prefixes are removed is inefficient and adds unnecessary overhead. Since the existing sessions (session1 and session2) are ON_CHANGE subscriptions, they will automatically receive delete notifications when the policy is swapped. You can reuse the existing sessions and use aftcache.DeletionStoppingCondition to verify that the prefixes are deleted. This is faster, more efficient, and directly validates the deletion streaming behavior of the existing subscription.
| // Open a new subscription and verify matched prefixes are removed. | |
| t.Log("Step 7: Opening AFT subscription with swapped policy") | |
| session1Swapped, session2Swapped, stoppingCondSwapped := mustOpenAFTSessions(t, dut, map[string]bool{}) | |
| aftSwapped := mustRunAFTSessions(t, aftSessionConfig{session1: session1Swapped, session2: session2Swapped, stop: stoppingCondSwapped, dut: dut}) | |
| // Verify prefixes are deleted from the existing AFT stream. | |
| t.Log("Step 7: Verifying prefixes are deleted from the existing AFT stream") | |
| stopDelete := aftcache.DeletionStoppingCondition(t, dut, wantPrefixes) | |
| aftSwapped := mustRunAFTSessions(t, aftSessionConfig{session1: session1, session2: session2, stop: stopDelete, dut: dut}) |
| // AftsGlobalFilterPolicyOCUnsupported returns true if "/network-instances/network-instance/afts/global-filter-policy" OC path is not supported. | ||
| func AftsGlobalFilterPolicyOCUnsupported(dut *ondatra.DUTDevice) bool { |
There was a problem hiding this comment.
The comment for the deviation accessor function AftsGlobalFilterPolicyOCUnsupported is missing the required issue tracker URL tracking its removal, which violates the Repository Style Guide (Deviation Guidelines). Additionally, the path in the comment contains a typo: /network-instances/network-instance/afts/global-filter-policy should be /network-instances/network-instance/afts/global-filter. Please use the standard issue tracker URL format: https://issuetracker.google.com/xxxxx.
| // AftsGlobalFilterPolicyOCUnsupported returns true if "/network-instances/network-instance/afts/global-filter-policy" OC path is not supported. | |
| func AftsGlobalFilterPolicyOCUnsupported(dut *ondatra.DUTDevice) bool { | |
| // AftsGlobalFilterPolicyOCUnsupported returns true if "/network-instances/network-instance/afts/global-filter" OC path is not supported. | |
| // Tracked at: https://issuetracker.google.com/514565554 | |
| func AftsGlobalFilterPolicyOCUnsupported(dut *ondatra.DUTDevice) bool { |
References
- Deviation accessor functions must have a comment containing a URL link to an issue tracker tracking its removal. (link)
- Issue tracker URLs in deviation comments are acceptable in the format 'https://issuetracker.google.com/xxxxx' (omitting the '/issues/' segment), provided they are validated.
2 participants
README: https://github.com/openconfig/featureprofiles/blob/main/feature/afts/filtered_streaming/otg_tests/afts_prefix_filtering_dualstack/README.md
Issues:
https://partnerissuetracker.corp.google.com/u/1/issues/514565554
https://partnerissuetracker.corp.google.com/u/1/issues/517809756
Logs: https://partnerissuetracker.corp.google.com/u/1/issues/517838946