[mirrororch] Port Mirroring implementation#1314
Conversation
Signed-off-by: Rupesh Kumar <rupesh-k.kumar@broadcom.com>
Modified to support LAG as source port.
| vector<Port> portv; | ||
| m_portsOrch->getLagMember(port, portv); | ||
| for (const auto p : portv) | ||
| { |
There was a problem hiding this comment.
Validate member port using 'gPortsOrch->getPort' before calling SAI set_port API
There was a problem hiding this comment.
Sure. will take care.
There was a problem hiding this comment.
And the port here is coming from internal LAG port . do u see extra validation is needed?
I just saw the other usage of this function in portsorch.cpp and don't see any validation.
So Please suggest. I can take care based on your response.
|
Hi @daall Can u please help with the review. I hope we can make this feature for 202006 release :) Thanks |
ack, will review this + utilities today :) |
daall
left a comment
daall
left a comment
There was a problem hiding this comment.
I think overall this looks pretty good, I just have a few clarifying questions about edge cases and some broad suggestions for the test code.
hi @daall , Thanks a lot for the review and good suggestions, I have taken care of the changes. Can you please help with review again. Thanks |
|
This pull request introduces 3 alerts when merging b92c3d4 into c05601c - view on LGTM.com new alerts:
|
daall
left a comment
daall
left a comment
There was a problem hiding this comment.
See comments, just a few minor things in the tests. :)
orchagent/mirrororch.cpp
Outdated
| Port port; | ||
| if (!m_portsOrch->getPort(dstPort, port) && port.m_type != Port::PHY) | ||
| { | ||
| SWSS_LOG_ERROR("Failed to locate port %s", dstPort.c_str()); |
There was a problem hiding this comment.
I think the log message should also specify if type is not phy. Can you add that to log and print type?
| bool MirrorOrch::validateSrcPort(const string& srcPort) | ||
| { | ||
| auto ports = tokenize(srcPort, ','); | ||
|
|
There was a problem hiding this comment.
Delete this line, to be consistent with above function
There was a problem hiding this comment.
This is source port list, so modified function name to reflect it.
orchagent/mirrororch.cpp
Outdated
| status = sai_port_api->set_port_attribute(port.m_port_id, &port_attr); | ||
| if (status != SAI_STATUS_SUCCESS) | ||
| { | ||
| SWSS_LOG_ERROR("Failed to configure %s session on port %s, status %d, sessionId %x\n", |
There was a problem hiding this comment.
You've used \n inconsistently. Some places have and some not. \n is not required, kindly remove
There was a problem hiding this comment.
Yes. taken care of this. There were some existing code also wrongly doing .fixed those too.
|
@prsunny, @kperumalbfn, @daall , Can this PR be merged and any further updates will be made in 202006 branch? Same for sonic-net/sonic-utilities#936 Thanks! |
daall
left a comment
daall
left a comment
There was a problem hiding this comment.
LGTM, double-check with other reviewers to make sure their feedback is addressed.
|
retest this please |
| const auto& name = it->first; | ||
| auto& session = it->second; | ||
|
|
||
| // Check the following conditions: |
There was a problem hiding this comment.
if mirror session src port is port+lag, then lag member update to null, the mirror session status will set to inactive ？
Signed-off-by: Rupesh Kumar <rupesh-k.kumar@broadcom.com>
7 participants
Signed-off-by: Rupesh Kumar rupesh-k.kumar@broadcom.com
What I did
Port mirroring implementation in swss.
HLD @ sonic-net/SONiC#580
Why I did it
To support port mirroring in SONiC
How I verified it
Verified using pytests and manual UT.
Details if related
pytests will updated in next commit.