-
Notifications
You must be signed in to change notification settings - Fork 779
Fetch capability of mirror before configuring it #4089
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fetch capability of mirror before configuring it #4089
Conversation
Signed-off-by: Stephen Sun <stephens@nvidia.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Stephen Sun <stephens@nvidia.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Failure is caused by rebasing the commit to the latest master. Investigating. |
Depends on sonic-net/sonic-swss#3934 |
Signed-off-by: Stephen Sun <stephens@nvidia.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Depends on sonic-net/sonic-swss#3934 |
|
@qiluo-msft Can you help review this change? Thanks |
config/main.py
Outdated
| def is_port_mirror_capability_supported(direction, namespace=None): | ||
| """ Check if port mirror capability is supported for the given direction """ | ||
| try: | ||
| state_db = SonicV2Connector(host='127.0.0.1') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use unix socket which has better performance? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
config/main.py
Outdated
| return False | ||
|
|
||
| return True | ||
| except Exception: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use more specific exception type? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed as we do not expect any exception here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds ASIC capability checking for port mirror direction support before allowing mirror session configuration. The change prevents users from configuring mirror sessions with directions (rx/tx/both) that are not supported by the underlying ASIC hardware.
- Added
is_port_mirror_capability_supported()function to query STATE_DB for ASIC capabilities - Integrated capability validation into
validate_mirror_session_config() - Added comprehensive test coverage for capability checking functionality
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| config/main.py | Added capability checking function and validation logic to prevent unsupported mirror directions |
| tests/config_mirror_session_test.py | Added test cases for capability checking and removed trailing whitespace |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Stephen Sun <stephens@nvidia.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
It's weird that pretest passed locally but failed in the github |
Signed-off-by: Stephen Sun <stephens@nvidia.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@qiluo-msft can you please check if you are ok with comments handling and approve this PR? |
What I did Fetch capability of ingress/egress mirror before configuring it and avoid configuring ingress/egress mirror on a platform that does not support it. How I did it Check the capability in PORT_INGRESS_MIRROR_CAPABLE and PORT_EGRESS_MIRROR_CAPABLE in STATE_DB table SWITCH_CAPABILITY. The capability of ingress/egress mirror is inserted to STATE_DB by orchagent during initialization. How to verify it Manual test and unit test
)" This reverts commit ad0a3d3.
|
@stephenxs this change causes test failure when advancing sonic-utilities submodule. We need to revert to move forward. |
|
@stephenxs this change is causing show_techsupport/test_techsupport.py to fail at multi-asic deployment. But this is a good change to have. I will see if that is the only failure. if it is, then I am leaning towards file an issue to xfail it and let you work on a fix. Please stay tuned. |
Could you please share the test link? Thanks |
Draft a PR to fix it: #4159 |
What I did Fetch capability of ingress/egress mirror before configuring it and avoid configuring ingress/egress mirror on a platform that does not support it. How I did it Check the capability in PORT_INGRESS_MIRROR_CAPABLE and PORT_EGRESS_MIRROR_CAPABLE in STATE_DB table SWITCH_CAPABILITY. The capability of ingress/egress mirror is inserted to STATE_DB by orchagent during initialization. How to verify it Manual test and unit test
|
Cherry-pick PR to 202511: #4204 |
What I did
Fetch capability of ingress/egress mirror before configuring it and avoid configuring ingress/egress mirror on a platform that does not support it.
How I did it
Check the capability in
PORT_INGRESS_MIRROR_CAPABLEandPORT_EGRESS_MIRROR_CAPABLEinSTATE_DBtableSWITCH_CAPABILITY.The capability of ingress/egress mirror is inserted to STATE_DB by orchagent during initialization.
How to verify it
Manual test and unit test
Previous command output (if the output of a command-line utility has changed)
New command output (if the output of a command-line utility has changed)