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
|
![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 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.
|
/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.
|
/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.
paramasivamn1
left a comment
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 OpenConfig pull request is | ||
| > [#1441](https://github.com/openconfig/public/pull/1441). |
There was a problem hiding this comment.
As this pull request has been merged let's remove this text?
| > The OpenConfig pull request is | ||
| > [#1441](https://github.com/openconfig/public/pull/1441). |
There was a problem hiding this comment.
Not all portions of the feature profiles have synced with the new PR. Have to leave some missing sections for now.
|
|
||
| This test validates that the AFT filter dynamically responds to changes in the | ||
| underlying prefix sets without requiring a re-binding of the policy itself. Both | ||
| IPv4 and IPv6 prefix-set modifications are covered. |
There was a problem hiding this comment.
what about the test cases related to the routing-policy itself?
- changing the prefix-set from A to B with in the routing-policy?
- routing policy with multiple statements where each statement refers to a different prefix-set?
- routing policy statement action is deny with a prefix set, i.e., prefix set represents list of denied or
non-allowed prefixes. - Negative test case: routing-policy contains match criteria something other than prefix-set and
verify no prefixes are received or deletes received for previously sent prefixes.
4 participants
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]