Conversation
Summary of ChangesHello @mnp-juniper, 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 enhancements to the gNPSI (gRPC Network Packet Sampling Interface) test suite to better accommodate vendor-specific behaviors, particularly for Juniper devices. It refines how SFlow packet sizes are verified and how gNPSI is configured, ensuring more accurate and compatible testing across different network equipment vendors. Highlights
Changelog
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 #5094 / 28128e0Virtual 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 fixes for gNPSI tests, primarily by adding vendor-specific handling for Juniper devices. This includes adding a process name for the gNPSI daemon on Juniper, skipping gNPSI configuration when sFlow is configured, and providing vendor-specific frame size adjustments for sFlow packet verification. A fallback mechanism for frame sizes is also introduced for unknown vendors. The changes improve the test's adaptability to different vendor platforms. I've suggested a small refactoring to improve code clarity and reduce duplication in the frame size lookup logic.
| if vendorMap, found := adjustedFrameSizeMap[vendor]; found { | ||
| if adjustedValues, found := vendorMap[flowConfig.frameSize]; found { | ||
| if size, found := adjustedValues[flowConfig.ipType]; found { | ||
| adjustedSize = size | ||
| } | ||
| } | ||
| } else if fallbackMap, found := adjustedFrameSizeMap[vendorFallback]; found { | ||
| if adjustedValues, found := fallbackMap[flowConfig.frameSize]; found { | ||
| if size, found := adjustedValues[flowConfig.ipType]; found { | ||
| adjustedSize = size | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for looking up the adjusted frame size is duplicated for the vendor-specific case and the fallback case. This can be simplified to reduce duplication and improve readability.
vendorMap, ok := adjustedFrameSizeMap[vendor]
if !ok {
vendorMap = adjustedFrameSizeMap[vendorFallback]
}
if frameMap, ok := vendorMap[flowConfig.frameSize]; ok {
if size, ok := frameMap[flowConfig.ipType]; ok {
adjustedSize = size
}
}
Pull Request Test Coverage Report for Build 22108615141Details
💛 - Coveralls |
|
Changes made on this commit : [increasing traffic volume] 15fed9e ============================ Variance = np(1-p) = 99.99999 ≈ 100 ** Added handling for Juniper's "gNPSI client subscribe request is closed by server" error message during gNPSI service restarts ============================ |
5 participants
** Added vendor-specific sflow frame size adjustments - stripping 4byte FCS as per our architecture
** Skipping gnpsi configuration for juniper vendor since its implicitly enabled with sflow configs.
** Added gNPSI service mapping (svcsproxy) for Juniper devices.