Check PF counters through cli if not supported in OC #5002
Conversation
Summary of ChangesHello @ijernoiu, 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 testing framework by introducing a fallback mechanism for verifying Policy Forwarding (PF) counters. Previously, tests relied solely on OpenConfig (OC) for this data. With this change, if a device (specifically Arista, as implemented) does not support PF counters via OC, the system will now query the device using CLI commands. This ensures test coverage for PF counters on platforms with incomplete OC implementations, addressing a known issue. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 #5002 / 0328a82Virtual 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 the ability to check policy forwarding counters via CLI for devices that do not support OpenConfig (OC) telemetry. This is a temporary measure to enable testing on specific platforms. The changes involve adding new Go functions to execute CLI commands and parse their output, updating the metadata to reflect this deviation, and regenerating protobuf files. Overall, the approach addresses the immediate need, but some areas could be made more robust.
| if err != nil { | ||
| t.Errorf("Failed to execute CLI command '%s': %v", cliCommand, err) |
There was a problem hiding this comment.
If the CLI command fails to execute, it's a critical issue that prevents the test from proceeding meaningfully. Using t.Fatalf here would immediately stop the test and mark it as failed, which is generally preferred for unrecoverable errors during test setup or execution.
| if err != nil { | |
| t.Errorf("Failed to execute CLI command '%s': %v", cliCommand, err) | |
| if err != nil { | |
| t.Fatalf("Failed to execute CLI command '%s': %v", cliCommand, err) | |
| } |
| if err != nil { | ||
| t.Errorf("Invalid response for CLI command '%s': %v", cliOutput, err) |
There was a problem hiding this comment.
Similar to the runCliCommand error handling, if strconv.ParseUint fails, it indicates a fundamental issue with parsing the CLI output, making further test assertions unreliable. Using t.Fatalf here would ensure the test stops immediately, preventing misleading results.
if err != nil {
t.Fatalf("Invalid response for CLI command '%s': %v", cliOutput, err)
}| switch dut.Vendor() { | ||
| case ondatra.ARISTA: | ||
| //extract text from CLI output between rule name and packets | ||
| policyCountersCommand := fmt.Sprintf(`show traffic-policy %s interface counters | grep %s | sed -e 's/.*%s:\(.*\)packets.*/\1/'`, trafficPolicyName, tc.policyRule, tc.policyRule) |
There was a problem hiding this comment.
The use of sed with a complex regular expression to extract data from CLI output can be brittle. CLI output formats can change, leading to test failures. Consider a more robust parsing method, such as splitting the output into lines and then fields, or using a more flexible regex if direct parsing is unavoidable. This improves maintainability and reduces the risk of future breakage.
For example, if the output format changes slightly, this regex might fail. A more resilient approach might involve:
- Splitting the
cliOutputby newline to get individual lines. - Iterating through lines to find the one containing
tc.policyRule. - Using
strings.Fieldsor a simpler regex on that specific line to extract the number.
| // Code generated by protoc-gen-go. DO NOT EDIT. | ||
| // versions: | ||
| // protoc-gen-go v1.36.7 | ||
| // protoc-gen-go v1.36.6 |
There was a problem hiding this comment.
The protoc-gen-go version has been downgraded from v1.36.7 to v1.36.6. This might indicate an unintentional change or a local environment mismatch. It's important to ensure that the protobuf generator versions are consistent across the project to avoid potential build issues or unexpected behavior. Please verify if this downgrade was intentional; otherwise, it should be reverted to v1.36.7 or updated to the latest stable version if appropriate.
| // protoc-gen-go v1.36.6 | |
| // protoc-gen-go v1.36.7 |
Pull Request Test Coverage Report for Build 21225941840Details
💛 - Coveralls |
Iosif-Jernoiu
force-pushed
the
pf-1.2-cliTelemetryUpdate
branch
from
2ebe96e to
901f36f
Compare
|
We cannot accept CLI for counters. Counters must always be via gnmi streaming. A deviation could be used for paths that are not openconfig (vendor native paths). See the guide for deviations. featureprofiles/internal/deviations/README.md Lines 9 to 11 in 9bc0a4b |
5 participants
Temporarily enabling PF counters check through CLI as per discussion in issue 454567032