-
Notifications
You must be signed in to change notification settings - Fork 543
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
[orchagent] Add support for Path Tracing Midpoint #2903
Conversation
@prsunny @kperumalbfn could you please help and review the code of this PR? |
orchagent/portsorch.cpp
Outdated
{ | ||
SWSS_LOG_INFO("Path Tracing is not supported"); | ||
fvVector.emplace_back(SWITCH_CAPABILITY_TABLE_PATH_TRACING_CAPABLE, "false"); | ||
} |
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.
are we using this capability check before pushing the path tracing feature to SAI? Could you check that?
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.
@kperumalbfn The result of the Path Tracing capability check is saved to STATE DB. Then, the CLI checks if Path Tracing is supported or not before attempting to enable Path Tracing.
In addition, as requested, I added some checks to also verify if Path Tracing is supported or not before configuring it into the ASIC: 448d8a3
* Decrease TAM object ref count before removing the port, if the port | ||
* has a TAM object assigned | ||
*/ | ||
if (m_portPtTam.find(p.m_alias) != m_portPtTam.end()) |
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.
Can you also check if TAM module is supported in the switch using the capability check?
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.
@kperumalbfn I added the TAM capability check, as requested: 5b57173
orchagent/portsorch.cpp
Outdated
} | ||
} | ||
|
||
if (!setPortPtTam(p, m_ptTam)) |
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.
We could move TAM create, setting TAM to port and refcount update to a different function.
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.
@kperumalbfn I moved all the logic related to the TAM to separate functions: fc6b5ba
@@ -80,3 +85,5 @@ | |||
#define PORT_ROLE "role" | |||
#define PORT_ADMIN_STATUS "admin_status" | |||
#define PORT_DESCRIPTION "description" | |||
#define PORT_PT_INTF_ID "pt_interface_id" |
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.
can you update these new fields in doc/swss-schema.md
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.
@kperumalbfn I added the two fields to the documentation: c9ff12e
06b7459
to
b232132
Compare
Hi @kperumalbfn, many thanks for the review. I addressed all comments. |
} | ||
port.pt_intf_id.is_set = true; | ||
} | ||
catch (const std::exception &e) |
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.
should we reject this for port-channel?
Also check for the path-tracing interface id within the valid range. I believe it is between 1-4K as per HLD. Please add this case in swss tests.
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.
@kperumalbfn I did the requested changes:
- added a check to reject Path Tracing configuration on port channel
- added a check to ensure Path Tracing interface ID is in the valid range [1, 4095]
- added test cases to verify that OrchAgent behaves as expected when it receives an invalid Path Tracing configuration
@kperumalbfn Many thanks for the review. All comments have been addressed. |
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.
LGTM
@@ -86,3 +86,4 @@ extern sai_mpls_api_t* sai_mpls_api; | |||
extern sai_counter_api_t* sai_counter_api; | |||
extern sai_samplepacket_api_t *sai_samplepacket_api; | |||
extern sai_fdb_api_t* sai_fdb_api; | |||
extern sai_fdb_api_t* sai_tam_api; |
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.
typo sai_tam_api_t??
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.
@kperumalbfn Fixed, thanks!
@@ -217,6 +218,7 @@ void initSaiApi() | |||
sai_api_query((sai_api_t)SAI_API_DASH_ENI, (void**)&sai_dash_eni_api); | |||
sai_api_query((sai_api_t)SAI_API_DASH_VIP, (void**)&sai_dash_vip_api); | |||
sai_api_query((sai_api_t)SAI_API_DASH_DIRECTION_LOOKUP, (void**)&sai_dash_direction_lookup_api); | |||
sai_api_query(SAI_API_TAM, (void **)&sai_tam_api); | |||
|
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.
pls check the alignments.
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.
@kperumalbfn Fixed, thanks!
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.
@cscarpitta could you fix the alignment for sai_api_query
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.
@kperumalbfn Fixed, many thanks for the review.
@kperumalbfn , please signoff |
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.
LGTM
@prsunny the PR has been approved by @kperumalbfn. It is ready for merge. can you merge the PR? |
@prsunny The PR has been reviewed and approved. It is ready for merge. Could you please merge the PR? |
@dgsudharsan Did you have a chance to review this PR? Can you please have a look? |
Can you please fix coverage and rebase the code? |
e057773
to
f628fb7
Compare
@cscarpitta in order to merge the PR we need all checkers to pass and the coverage to be extended. |
92c7c33
to
0a61f3e
Compare
61dc3db
to
6cc0bae
Compare
@prsunny can you please help to check and merge this PR? Thanks. |
f5c6fd2
to
3e5bb9e
Compare
@liat-grozovik Many thanks for the review. I added some test cases to extend the coverage as requested. The CI checks are failing for reasons unrelated to this PR. Could you please take a look again? |
ack. will merge after rerunning the tests |
@cscarpitta , can you please resolve conflict and rebase? |
Extend PortsOrch to support PT Midpoint. Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
During PortsOrch initialization, SwitchOrch->set_switch_capability() is called to set Path Tracing capability in STATE DB. Currently, SwitchOrch is not initialized in FdbOrch UT, which causes a Segmentation Fault. Let's initialize SwitchOrch during the test case setup. Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
During PortsOrch initialization, SwitchOrch->set_switch_capability() is called to set Path Tracing capability in STATE DB. Currently, PortshOrch is initialized before initializing SwitchOrch in MuxOrch UT, which causes a Segmentation Fault. Let's move SwitchOrch initialization before PortsOrch initialization. Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
During its initialization, PortsOrch queries switch capabilities to verify if Path Tracing is supported or not. Currently, PortsOrch UTs fail because the TAM object type required by Path Tracing tests is not in the list of object types supported by the switch. This commit mocks get_switch_attribute() SAI API in PortsOrch UT to return TAM object type as part of the object types supported by the switch. Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
3e5bb9e
to
985f376
Compare
PR tests are passing now, can you please fix coverage? |
Signed-off-by: Carmine Scarpitta <cscarpit@cisco.com>
@prsunny Done. I extended the coverage, tests are passing and coverage requirements are met now. Can you please take a look again? |
What I did
Extended
PortsOrch
to process the attributes required for Path Tracing.Why I did it
This PR is required for the SRv6 Path Tracing Midpoint feature.
HLD: sonic-net/SONiC#1456
How I verified it
Added new unit tests to
portmgr_ut.cpp
,portsorch_ut.cpp
andtests/test_port.py
.