Deviation DecapNHWithoutNextHopNIUnsupported proposal#5637
Conversation
…se in TE-14.2 - As mentioned in https://partnerissuetracker.corp.google.com/issues/529388485 proposing deviation name DecapNHWithoutNextHopNIUnsupported. "This code is a Contribution to the OpenConfig Feature Profiles project ("Work") made under the Google Software Grant and Corporate Contributor License Agreement ("CLA") and governed by the Apache License 2.0. No other rights or licenses in or to any of Nokia's intellectual property are granted for any other purpose. This code is provided on an "as is" basis without any warranties of any kind."
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 device deviation to support specific vendor behaviors regarding decapsulation next-hop configurations. By adding this flag to the metadata and updating the relevant test logic, the change ensures that the encap/decap scale tests can correctly adapt to platforms that do not support certain next-hop network instance settings. 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
|
Pull Request Functional Test Report for #5637 / ea9f866Virtual 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 new deviation decap_nh_without_nexthop_ni_unsupported to handle platforms that do not support Decap NH without NextHopNetworkInstance. However, there are two issues: first, the new deviation accessor in deviations.go incorrectly calls the getter for a different deviation (GetDecapNhWithNexthopNiUnsupported instead of GetDecapNhWithoutNexthopNiUnsupported); second, the logic change in encap_decap_scale_test.go alters the default behavior for other platforms, disabling the configuration of WithNextHopNetworkInstance when it should be enabled by default.
| // DecapNHWithoutNextHopNIUnsupported returns true if Decap NH without NextHopNetworkInstance is not supported | ||
| // Nokia: https://partnerissuetracker.corp.google.com/issues/529388485 | ||
| func DecapNHWithoutNextHopNIUnsupported(dut *ondatra.DUTDevice) bool { | ||
| return lookupDUTDeviations(dut).GetDecapNhWithNexthopNiUnsupported() |
There was a problem hiding this comment.
The function DecapNHWithoutNextHopNIUnsupported is incorrectly calling GetDecapNhWithNexthopNiUnsupported() instead of GetDecapNhWithoutNexthopNiUnsupported(). This will cause it to return the value of the wrong deviation field (decap_nh_with_nexthop_ni_unsupported instead of decap_nh_without_nexthop_ni_unsupported). Additionally, please ensure this deviation accessor function includes a comment with a URL link to an issue tracker to document and track the behavior.
| return lookupDUTDeviations(dut).GetDecapNhWithNexthopNiUnsupported() | |
| return lookupDUTDeviations(dut).GetDecapNhWithoutNexthopNiUnsupported() |
References
- Every deviation accessor function, including those for permanent deviations, must include a comment with a URL link to an issue tracker (e.g., a tracking FR) to document and track the behavior.
| if deviations.DecapNHWithoutNextHopNIUnsupported(args.dut) { | ||
| decapNH.WithNextHopNetworkInstance(deviations.DefaultNetworkInstance(args.dut)) | ||
| } |
There was a problem hiding this comment.
By changing the condition from !deviations.DecapNHWithNextHopNIUnsupported(args.dut) to deviations.DecapNHWithoutNextHopNIUnsupported(args.dut), you are disabling the configuration of WithNextHopNetworkInstance by default for all other platforms where DecapNHWithoutNextHopNIUnsupported is false/unset.
If the standard/preferred behavior is to configure WithNextHopNetworkInstance unless unsupported, we should keep the original logic. The original logic already correctly configures it for Nokia (since DecapNHWithNextHopNIUnsupported is false for Nokia) and omits it for Arista (where it is true).
| if deviations.DecapNHWithoutNextHopNIUnsupported(args.dut) { | |
| decapNH.WithNextHopNetworkInstance(deviations.DefaultNetworkInstance(args.dut)) | |
| } | |
| if !deviations.DecapNHWithNextHopNIUnsupported(args.dut) { | |
| decapNH.WithNextHopNetworkInstance(deviations.DefaultNetworkInstance(args.dut)) | |
| } |
References
- When using the gribigo fluent builder API in Go, it is not necessary to reassign the builder variable when calling its methods, as the builder modifies the state in-place.
"This code is a Contribution to the OpenConfig Feature Profiles project ("Work") made under the Google Software Grant and Corporate Contributor License Agreement ("CLA") and governed by the Apache License 2.0. No other rights or licenses in or to any of Nokia's intellectual property are granted for any other purpose. This code is provided on an "as is" basis without any warranties of any kind."
Coverage Report for CI Build 28436196097Coverage decreased (-0.002%) to 8.645%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
snaragund
force-pushed
the
SND-TE14dot2Dev
branch
from
b11310f to
db2797d
Compare
6 participants
"This code is a Contribution to the OpenConfig Feature Profiles project ("Work") made under the Google Software Grant and Corporate Contributor License Agreement ("CLA") and governed by the Apache License 2.0. No other rights or licenses in or to any of Nokia's intellectual property are granted for any other purpose. This code is provided on an "as is" basis without any warranties of any kind."