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 detailed documentation for testing the AFT global filter mechanism, a critical feature designed to control the flow of gNMI streaming updates by applying routing policies. The added test plan ensures robust validation of this filtering capability across various scenarios, including dynamic changes, error conditions, and scalability, ultimately enhancing the reliability and precision of network state reporting. Highlights
Changelog
Activity
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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new test plan (README.md) and metadata for AFT prefix filtering. The test plan is detailed, but I've found several logical inconsistencies related to policy definitions and prefix matching examples. These inconsistencies could lead to incorrect test implementation. Specifically, POLICY-PREFIX-SET-B has conflicting definitions, and several dynamic update steps use prefixes that won't match the policies as they are defined in the Canonical OC section. I've added comments with suggestions to resolve these issues. The metadata.textproto file appears to be correct.
feature/afts/filtered_streaming/otg_tests/afts_prefix_filtering/README.md
Outdated
Show resolved
Hide resolved
feature/afts/filtered_streaming/otg_tests/afts_prefix_filtering/README.md
Outdated
Show resolved
Hide resolved
feature/afts/filtered_streaming/otg_tests/afts_prefix_filtering/README.md
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive test plan (README.md) and metadata for testing the AFT prefix filtering feature. The test plan is well-structured and detailed, covering a wide range of scenarios including initial sync, dynamic updates, error handling, reboots, scale, and multi-NI filtering. I have identified a minor inconsistency in the test plan's dynamic update section and provided a suggestion for correction. Overall, the documentation is clear and thorough.
feature/afts/filtered_streaming/otg_tests/afts_prefix_filtering/README.md
Outdated
Show resolved
Hide resolved
feature/afts/filtered_streaming/otg_tests/afts_prefix_filtering/README.md
Outdated
Show resolved
Hide resolved
feature/afts/filtered_streaming/otg_tests/afts_prefix_filtering/README.md
Outdated
Show resolved
Hide resolved
feature/afts/filtered_streaming/otg_tests/afts_prefix_filtering/README.md
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive test plan (README.md) and corresponding metadata for a new feature test, AFT-6.1: AFT Prefix Filtering. The test plan is well-structured and covers various aspects of the AFT prefix filtering feature, including basic validation, error handling, persistence, and scale. My review focuses on improving the clarity and completeness of the documentation. I've suggested an update to the TODO section in the README.md to include the relevant OpenConfig pull request link and correct a minor grammatical issue.
feature/afts/filtered_streaming/otg_tests/afts_prefix_filtering/README.md
Outdated
Show resolved
Hide resolved
feature/afts/filtered_streaming/otg_tests/afts_prefix_filtering/README.md
Outdated
Show resolved
Hide resolved
feature/afts/filtered_streaming/otg_tests/afts_prefix_filtering/README.md
Outdated
Show resolved
Hide resolved
feature/afts/filtered_streaming/otg_tests/afts_prefix_filtering/README.md
Outdated
Show resolved
Hide resolved
feature/afts/filtered_streaming/otg_tests/afts_prefix_filtering/README.md
Outdated
Show resolved
Hide resolved
feature/afts/filtered_streaming/otg_tests/afts_prefix_filtering/README.md
Outdated
Show resolved
Hide resolved
feature/afts/filtered_streaming/otg_tests/afts_prefix_filtering/README.md
Show resolved
Hide resolved
feature/afts/filtered_streaming/otg_tests/afts_prefix_filtering/README.md
Show resolved
Hide resolved
feature/afts/filtered_streaming/otg_tests/afts_prefix_filtering/README.md
Outdated
Show resolved
Hide resolved
feature/afts/filtered_streaming/otg_tests/afts_prefix_filtering/README.md
Outdated
Show resolved
Hide resolved
feature/afts/filtered_streaming/otg_tests/afts_prefix_filtering/README.md
Outdated
Show resolved
Hide resolved
paramasivamn1
left a comment
There was a problem hiding this comment.
Please find my input
|
(I know there are outstanding comments on this PR-- will address when I get back from OOO on March 30th.) |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive suite of test plans (AFT-6.1 through AFT-6.4) for validating AFT prefix filtering mechanisms via gNMI. The tests cover initial synchronization, dual-stack support, dynamic updates, and resilience scenarios such as device reboots and scale testing. The review feedback highlights a need for explicit subtests for recursive route resolution, identifies a logical inconsistency regarding the timing of prefix additions in the validation steps, and suggests completing the canonical OpenConfig JSON examples to include all referenced IPv6 prefixes for consistency.
| - The DUT is pre-configured with static routes to populate the AFT. Routes | ||
| include a mix of IPv4 and IPv6 prefixes drawn from RFC 5737 test address | ||
| space (see IP address conventions in `CONTRIBUTING.md`). At minimum the | ||
| following prefixes are installed in the `DEFAULT` network instance: | ||
| `198.51.100.0/24`, `203.0.113.0/28`, `100.64.0.0/24`, `2001:DB8:1::/64`, and | ||
| `2001:DB8:3::/64`. |
There was a problem hiding this comment.
The test plan mentions validating that "necessary recursive resolution prefixes" are received (line 150). However, the current test setup only includes direct static routes with interface next-hops. To properly test this requirement, a separate subtest should be added for recursive route resolution (e.g., a prefix whose next-hop is another prefix in the AFT), as test plans should be structured with separate subtests for distinct logical test cases even if the configuration is similar.
References
- Test plans should be structured with separate subtests for distinct logical test cases (e.g., default vs. non-default Network Instance), even if the underlying configuration sections are similar.
| - Verify that Notifications are received **only** for prefixes matching | ||
| `POLICY-PREFIX-SET-A` (`198.51.100.0/24`, `203.0.113.0/28`, | ||
| `198.51.100.1/32`), plus any necessary recursive resolution prefixes. |
There was a problem hiding this comment.
The list of expected prefixes in the initial sync includes 198.51.100.1/32. However, according to the "Validate Dynamic Updates" section (line 168), this prefix is added to the DUT after the initial subscription is established. It should be removed from the initial sync validation list to avoid confusion.
| ] | ||
| } | ||
| } | ||
| ] |
There was a problem hiding this comment.
The "Canonical OC" JSON for static routes is missing the IPv6 prefixes (2001:DB8:1::/64 and 2001:DB8:3::/64) that are specified in the "Test environment setup" (line 31). Adding them would ensure the JSON is complete and consistent with the test description.
},
{
"prefix": "2001:DB8:1::/64",
"config": { "prefix": "2001:DB8:1::/64" },
"next-hops": {
"next-hop": [
{
"index": "0",
"config": { "index": "0", "next-hop": "2001:DB8::2" }
}
]
}
},
{
"prefix": "2001:DB8:3::/64",
"config": { "prefix": "2001:DB8:3::/64" },
"next-hops": {
"next-hop": [
{
"index": "0",
"config": { "index": "0", "next-hop": "2001:DB8::2" }
}
]
}
}
Create initial READMEs for testing out the AFTs prefix filtering feature.
OpenConfig PR for model
This pull request adds new test plans and metadata for AFT prefix filtering, expanding coverage to dual-stack (IPv4/IPv6) filtering and dynamic updates to prefix sets. The new documentation describes test setups, validation steps, and OpenConfig model coverage for these scenarios.
New test plans and documentation:
README.mdthat covers independent and concurrent application of IPv4 and IPv6 prefix filters, validation steps, and proposed OpenConfig model extensions.README.mddescribing tests for dynamic addition and removal of prefixes from active sets and expected gNMI notifications, as well as OpenConfig path and RPC coverage.Metadata additions:
metadata.textprotofiles for the new test plans, specifying UUIDs, plan IDs, descriptions, and required testbed types for AFT Prefix Filtering (single-stack), Dual-Stack, and Dynamic Updates. [1] [2] [3]