Introduce qos_ft deviation to apply functional translator options to QoS counter queries#5187
Conversation
…ions to QoS counter queries.
RishabhAgarwal-2001
requested review from
AmrNJ,
alokmtri-g,
ram-mac and
rohit-rp
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 enhances the QoS testing framework by integrating functional translator capabilities for QoS counter queries. It introduces a mechanism to specify a functional translator via a deviation, ensuring that OpenConfig paths for QoS metrics are correctly translated to native device paths, particularly for CiscoXR devices. This change allows for accurate retrieval and validation of QoS counters like transmitted and dropped packets/octets, improving the robustness of QoS feature tests. 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
|
Pull Request Functional Test Report for #5187 / b71141cVirtual 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 qos_ft deviation to support functional translators for QoS counter queries, specifically for CiscoXR devices. The changes are well-implemented and correctly apply the functional translator options to gnmi.Watch and gnmi.Get calls. The suggestions to improve code quality by removing an unused parameter from a new helper function are valid and align with general Go best practices.
| inputIntf attrs.Attributes | ||
| } | ||
|
|
||
| func getOptsForFunctionalTranslator(t *testing.T, dut *ondatra.DUTDevice, functionalTranslatorName string) []ygnmi.Option { |
There was a problem hiding this comment.
The dut parameter is unused and can be removed to simplify the function signature.
| func getOptsForFunctionalTranslator(t *testing.T, dut *ondatra.DUTDevice, functionalTranslatorName string) []ygnmi.Option { | |
| func getOptsForFunctionalTranslator(t *testing.T, functionalTranslatorName string) []ygnmi.Option { |
References
- Functions should not have unused parameters. This is a common Go best practice for clean and maintainable code, as highlighted in style guides like Effective Go. (link)
| // Get QoS egress packet counters before the traffic. | ||
| const timeout = time.Minute | ||
| isPresent := func(val *ygnmi.Value[uint64]) bool { return val.IsPresent() } | ||
| opts := getOptsForFunctionalTranslator(t, dut, deviations.QosFt(dut)) |
There was a problem hiding this comment.
Removing the unused dut parameter from the getOptsForFunctionalTranslator call to align with the suggested change to the function's signature.
| opts := getOptsForFunctionalTranslator(t, dut, deviations.QosFt(dut)) | |
| opts := getOptsForFunctionalTranslator(t, deviations.QosFt(dut)) |
| ygnmi.WithSubscriptionMode(gpb.SubscriptionMode_SAMPLE), | ||
| ygnmi.WithSampleInterval(interval), | ||
| ) | ||
| opts := getOptsForFunctionalTranslator(t, dut, deviations.QosFt(dut)) |
There was a problem hiding this comment.
Removing the unused dut parameter from the getOptsForFunctionalTranslator call to align with the suggested change to the function's signature.
| opts := getOptsForFunctionalTranslator(t, dut, deviations.QosFt(dut)) | |
| opts := getOptsForFunctionalTranslator(t, deviations.QosFt(dut)) |
…o QoS counter queries (openconfig#5187) * feat: introduce `qos_ft` deviation to apply functional translator options to QoS counter queries. * Add Vendor Bug
…o QoS counter queries (openconfig#5187) * feat: introduce `qos_ft` deviation to apply functional translator options to QoS counter queries. * Add Vendor Bug
5 participants
Primary Reviewer: @rohit-rp / @AmrNJ / @ram-mac
Description
The
ciscoxr_qos_ftfunctional translator is used for translating native paths from CiscoXR devices to OC paths. Out of the 6 paths translated by this translator, our changes in this PR test that the following four paths are working when queried through the functional translator -qos/interfaces/interface/output/queues/queue/state/dropped-octetsqos/interfaces/interface/output/queues/queue/state/dropped-pktsqos/interfaces/interface/output/queues/queue/state/transmit-octetsqos/interfaces/interface/output/queues/queue/state/transmit-pktsTesting
Ensured that the test passes successfully on the Cisco device. No testing was done for other vendors since for them changes here should be no-op.