gNMI-1.28: fix Arista telemetry interfaces test failures - #5729
gNMI-1.28: fix Arista telemetry interfaces test failures#5729pjacakArista wants to merge 2 commits into
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 addresses several test failures in the gNMI-1.28 telemetry_interfaces_test when running on Arista EOS. The changes introduce necessary platform deviations, correct interface naming conventions, and adjust subinterface configuration requirements to align with Arista's operational model, ensuring the test suite passes successfully. 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 #5729 / 88faa32Virtual 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 updates the telemetry interfaces test by generalizing platform skip messages, removing redundant comments, and avoiding lowercase conversion on port names. It also introduces a check for the RequireRoutedSubinterface0 deviation to conditionally enable IPv4 on subinterfaces. Feedback on this PR highlights that the newly used require_routed_subinterface_0 deviation must be added to the Arista platform exceptions in metadata.textproto to prevent test failures on Arista EOS.
| func TestTelemetryInterfaces(t *testing.T) { | ||
| dut := ondatra.DUT(t, "dut") | ||
| port1 := strings.ToLower(dut.Port(t, "port1").Name()) | ||
| port1 := dut.Port(t, "port1").Name() |
There was a problem hiding this comment.
Do we know why this was using tolower before? How do we know that this fix does not break other vendors?
There was a problem hiding this comment.
Hmm ... you might be right on this, but my thinking was that:
There are 311 ( as far as I could find ) places where dut.Port(...).Name() is used, from those there are only 2 places where strings.ToLower is used ( telemetry_interfaces_test.go and telemetry_high_availability_test.go )
If we should add deviation (to use strings.ToLower ), we probably should add it only in those 2 places for platforms that require it.
There are no tests run result for this PR. Is there any way to see those on 'main'?
I've looked at PR that added this test (#5313 ) and it failed on all virtual devices ( and was never run on physical ones )
There was a problem hiding this comment.
@singhavnish2516 what do you think of it?
a30a20b to
113962c
Compare
59cb660 to
d903a79
Compare
6b415b1 to
b80f376
Compare
|
@singhavnish2516 could you take a look at this PR? |
472639a to
977806e
Compare
2c30c04 to
0f64ad7
Compare
The gNMI-1.28 telemetry_interfaces_test fails on Arista EOS for several independent reasons: the test lowercases the Ondatra port name before using it in OpenConfig interface configuration, default description state is not present unless explicitly configured, the state/rate paths are handled through the test's existing state_path_unsupported deviation, routed subinterface 0 requires IPv4 to be explicitly enabled, and aggregation cannot be configured directly on the physical test port. gNMI-1.28, port names: use the port name exactly as returned by Ondatra instead of lowercasing it. EOS treats the lowercased name as a different interface, which causes the interface type Set to apply to the wrong interface object and fail validation. gNMI-1.28, default description state: add Arista to the existing missing_value_for_defaults deviation. This lets the state validation skip the default description leaf when the platform does not report a default value for it. gNMI-1.28, state/rate and aggregation paths: add Arista to the existing state_path_unsupported deviation. This uses the same test mechanism already used for platforms that do not support the tested state/rate paths or physical-port aggregation configuration. The expected handling of /interfaces/interface/state/in-rate and out-rate is still being clarified; the test README lists those paths, but they are not present in the current openconfig-interfaces model or in the test's canonical OC example. gNMI-1.28, aggregation skip text: make the aggregation skip message platform-generic instead of Nokia-specific because the skip is now driven by the platform deviation rather than a single vendor. gNMI-1.28, routed subinterface 0: when the existing RequireRoutedSubinterface0 deviation is set, include IPv4 enabled state in the subinterface config pushed by the test. EOS requires that explicit configuration before the subinterface state validation can succeed. With these changes, gNMI-1.28 passes on Arista EOS.
0f64ad7 to
d600574
Compare
4 participants
The gNMI-1.28 telemetry_interfaces_test fails on Arista EOS for several independent reasons: the test lowercases the Ondatra port name before using it in OpenConfig interface configuration, default description state is not present unless explicitly configured, the state/rate paths are handled through the test's existing state_path_unsupported deviation, routed subinterface 0 requires IPv4 to be explicitly enabled, and aggregation cannot be configured directly on the physical test port.
gNMI-1.28, port names: use the port name exactly as returned by Ondatra instead of lowercasing it. EOS treats the lowercased name as a different interface, which causes the interface type Set to apply to the wrong interface object and fail validation.
gNMI-1.28, default description state: add Arista to the existing missing_value_for_defaults deviation. This lets the state validation skip the default description leaf when the platform does not report a default value for it.
gNMI-1.28, state/rate and aggregation paths: add Arista to the existing state_path_unsupported deviation. This uses the same test mechanism already used for platforms that do not support the tested state/rate paths or physical-port aggregation configuration. The expected handling of /interfaces/interface/state/in-rate and out-rate is still being clarified; the test README lists those paths, but they are not present in the current openconfig-interfaces model or in the test's canonical OC example.
gNMI-1.28, aggregation skip text: make the aggregation skip message platform-generic instead of Nokia-specific because the skip is now driven by the platform deviation rather than a single vendor.
gNMI-1.28, routed subinterface 0: when the existing RequireRoutedSubinterface0 deviation is set, include IPv4 enabled state in the subinterface config pushed by the test. EOS requires that explicit configuration before the subinterface state validation can succeed.
With these changes, gNMI-1.28 passes on Arista EOS.