Creating a Readme documentation for testing aggregate subinterfaces#5109
Creating a Readme documentation for testing aggregate subinterfaces#5109toadeniy-g merged 4 commits intoopenconfig:mainfrom
Conversation
Summary of ChangesHello @toadeniy-g, 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 new documentation for testing aggregate subinterfaces, providing a detailed guide for validating their operation. It outlines the test environment setup, including network topology and configuration parameters for both the Device Under Test (DUT) and Automated Test Equipment (ATE). The documentation also specifies the testing steps, pass criteria, canonical OpenConfig JSON for configuration, and lists the covered OpenConfig paths and RPCs, ensuring thorough validation of aggregate subinterface functionality in various network instances. 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 RT-5.14, a test for aggregate subinterfaces in default and non-default network instances. My review focuses on improving the clarity and structure of the test plan to align with repository guidelines. The most significant feedback is to restructure the Procedure section to properly represent the two distinct test cases (default and non-default network instances) as separate subtests, as required by the test plan template. I've also noted that the provided Canonical OC is incomplete. Additionally, I've pointed out several minor typos and formatting issues in both the README.md and metadata.textproto files to enhance readability and consistency. None of the comments contradicted the provided rules, so all original comments have been retained.
| @@ -0,0 +1,266 @@ | |||
| # RT-5.14: Aggregate Subinterface in Default and Non default Network Instance | |||
There was a problem hiding this comment.
There is a typo in the title. Non default should be hyphenated as Non-default for consistency and clarity.
| # RT-5.14: Aggregate Subinterface in Default and Non default Network Instance | |
| # RT-5.14: Aggregate Subinterface in Default and Non-default Network Instance |
|
|
||
| ##### IPv4 Addresses | ||
| Device | Lag 1 vlan 10 | lag1 vlan 20 | lag2 vlan 10 | lag2 vlan20 | ||
| :------| :----------| :-------- | :---------| :-------: |
There was a problem hiding this comment.
The markdown table formatting is inconsistent. The last column's alignment specifier (:-------:) indicates right-alignment, while others are left-aligned. For consistency, all columns should have the same alignment.
| :------| :----------| :-------- | :---------| :-------: | |
| :------| :--------------| :-------------| :--------------| :--------------- |
|
|
||
| ##### IPv6 Addresses | ||
| Device | Lag 1 vlan 10 | lag1 vlan 20 | lag2 vlan 10 | lag2 vlan20 | ||
| :------| :----------| :-------- | :---------| :-------: |
There was a problem hiding this comment.
The markdown table formatting is inconsistent. The last column's alignment specifier (:-------:) indicates right-alignment, while others are left-aligned. For consistency, all columns should have the same alignment.
| :------| :----------| :-------- | :---------| :-------: | |
| :------| :--------------| :-------------| :--------------| :--------------- |
| 2. Add port 1 and port 2 to aggregate interface 1 | ||
| 3. Add port 3 and port 4 to aggregate interface 2 | ||
| 4. Create two emulated routers with ethernet interfaces on both routers | ||
| 5. Connect the ethernet interfaces aggregate interfaces |
There was a problem hiding this comment.
This sentence is grammatically incorrect and unclear. Please rephrase for clarity.
| 5. Connect the ethernet interfaces aggregate interfaces | |
| 5. Connect the ethernet interfaces to the aggregate interfaces |
| * Validate that the Aggregate Interfaces are UP on the OTG | ||
| * Start Traffic flows | ||
| * Wait for another 60 seconds | ||
| * stop Traffic Flow |
There was a problem hiding this comment.
Typo. 'stop' should be capitalized for consistency with 'Start Traffic flows'.
| * stop Traffic Flow | |
| * Stop Traffic Flow |
toadeniy-g
requested review from
a team and
AmrNJ
and removed request for
AmrNJ
|
This test is nearly 100% duplicate to RT-5.2 (https://github.com/openconfig/featureprofiles/tree/main/feature/interface/aggregate/otg_tests/aggregate_test). Can you update RT-5.2 to simply add an additional (non-default) network instance? It is rather inefficient to create another _test.go just for this case. |
dplore
left a comment
dplore
left a comment
There was a problem hiding this comment.
Please update https://github.com/openconfig/featureprofiles/tree/main/feature/interface/aggregate/otg_tests/aggregate_test with your test requirements.
| DUT | 2001:db8::1/126 | 2001:db8::5/126 | 2001:db8::9/126 | 2001:db8::13/126 | ||
| ATE | 2001:db8::2/126 | 2001:db8::6/126 | 2001:db8::10/126 | 2001:db8::14/126 | ||
|
|
||
|
|
There was a problem hiding this comment.
@toadeniy-g - Request you to add a section here defining the traffic profile. Expecting the following
- Traffic source, Traffic destination
- Traffic flow details like frame size, duration of traffic, fixed packet count / line rate traffic (recommended)
|
|
||
| ### Test Pass Criteria | ||
|
|
||
| * Packet drop from the flows must be less than 2 percent. |
There was a problem hiding this comment.
@toadeniy-g - Based on the traffic profile. If the traffic flow is of size 64 frame size then 2% tolerance is fine. If the frame size is higher than 64 and >512 bytes. then the tolerance should be <1%.
| /interfaces/interface/config/name: | ||
| /interfaces/interface/aggregation/config/lag-type: | ||
|
|
||
| # Telemetry Parameter Coverage |
There was a problem hiding this comment.
The current README only lists /interfaces/interface/state/admin-status and /interfaces/interface/state/oper-status for telemetry. To fully verify the health of the aggregate interface, you should include LACP member state telemetry such as:
/lacp/interfaces/interface/members/member/state/collecting
/lacp/interfaces/interface/members/member/state/distributing
/lacp/interfaces/interface/members/member/state/partner-id.
These paths are already utilized in other aggregate tests like RT-5.2
| * Validate that all the 4 ports are Operationally UP on the OTG | ||
| * Validate that the Aggregate Interfaces are UP on the OTG | ||
| * Start Traffic flows | ||
| * Wait for another 60 seconds |
There was a problem hiding this comment.
@toadeniy-g - Instead of a static "Wait 60 seconds" the test should use gNMI telemetry to wait specifically until the LAG and its subinterfaces are operationally UP. This is required to reduce the test duration and gives a predictible results
There was a problem hiding this comment.
this is the wait time between when the traffic is started on the OTG and when it is stopped, have nothing to do with telemetry yet at this point in the test
|
|
||
| ### Test Pass Criteria | ||
|
|
||
| * Packet drop from the flows must be less than 2 percent. |
There was a problem hiding this comment.
@toadeniy-g - The pass criteria should also have telemetry validation. All the counters should be validated without any failure.
| * Stop Traffic Flow | ||
| * Collect the receive and the transmit packet information from the | ||
| flows and validate it against the test pass criteria | ||
|
|
There was a problem hiding this comment.
I see there are some coverage that is missing in this README. Please below network events
- Flap the LAG at least 10 times and then wait for the interfaces to be back up using telemetry. Validate traffic and counters.
- A critical aspect of aggregate interfaces is their ability to handle member link failures. Configure LAG with minimum member link set to 1 if not already configured. Bring down one of the member link for one of the LAG and see if the interface still stays up and traffic flow is not impacted. Bring back the member link and see if the traffic is still flowing.
- MTU validation. Ensure the test validates traffic at a configured MTU as the VLAN traffic adds a overhead of 4 bytes. It is required for the DUT to handle the maximum sized packets on the subinterfaces.
…penconfig#5109) Co-authored-by: Darren Loher <dloher@google.com>
…penconfig#5109) Co-authored-by: Darren Loher <dloher@google.com>
6 participants
Creating a Readme documentation for testing aggregate subinterfaces