Mclag enhacements #1349
Mclag enhacements #1349
Conversation
|
|
gitsabari
force-pushed
the
mclag_enhacements
branch
from
310c312 to
3779438
Compare
|
retest vs please |
2 similar comments
|
retest vs please |
|
retest vs please |
|
@rathnasabapathyv , @prvattem |
|
akokhan i have addressed all your review comments. if you dont have futher comments, please help approve the changes |
| return vid < fdb.vid; | ||
| else | ||
| return port_name < fdb.port_name; | ||
| //else if (port_name != fdb.port_name) return port_name < fdb.port_name; |
There was a problem hiding this comment.
Please remove if not needed
|
|
||
| if (write <= 0) | ||
| { | ||
| SWSS_LOG_ERROR("mclagsycnd to ICCPD, domain cfg send, buffer full; write to m_connection_socket failed"); |
There was a problem hiding this comment.
What is the recovery for buffer full? should it be retried again later? returned to caller with error status? The code seems to jut continue as if no error occurred?
|
|
||
| if (write <= 0) | ||
| { | ||
| SWSS_LOG_ERROR("mclagsycnd to ICCPD, domain cfg send; write to m_connection_socket failed"); |
There was a problem hiding this comment.
Any recovery or somehow to abort this operation when such error occurred?
|
|
||
| if (write <= 0) | ||
| { | ||
| SWSS_LOG_ERROR("mclagsycnd to ICCPD, mclag iface cfg send, buffer full; write to m_connection_socket failed"); |
There was a problem hiding this comment.
After error detected just continue?
There was a problem hiding this comment.
@gechiang write call failures are rare and currently there is no handling at this point. haven't faced these issues in our testing.
Probably we can consider future enhancement to address it in further enhacements PRs
| count = 0; | ||
| if (write <= 0) | ||
| { | ||
| SWSS_LOG_ERROR("mclagsycnd to ICCPD, mclag vlan member updates send, buffer full; write to m_connection_socket failed"); |
There was a problem hiding this comment.
should it bail out? skip current iteration?
| break; | ||
|
|
||
| default: | ||
| SWSS_LOG_ERROR("Invalid option type %u", op_hdr->op_type); |
There was a problem hiding this comment.
Unexpected option perhaps should skip the rest of the code processing and bail out instead?
There was a problem hiding this comment.
Unexpected option perhaps should skip the rest of the code processing and bail out instead?
it is okay to continue. since this is option is not recognised by mclagsyncd although iccpd sends it. this is sub option within main option. next line increments the length and need to continue with next one
cur_len += (MCLAG_SUB_OPTION_HDR_LEN + op_hdr->op_len);
| break; | ||
|
|
||
| default: | ||
| SWSS_LOG_WARN("Invalid option type %u", op_hdr->op_type); |
There was a problem hiding this comment.
Invalid Option Ok to continue?
There was a problem hiding this comment.
it is okay to continue. since this is option is not recognised by mclagsyncd although iccpd sends it. this is sub option within main option. next line increments the length and need to continue with next one
cur_len += (MCLAG_SUB_OPTION_HDR_LEN + op_hdr->op_len);
|
I did not see any unit test code created for this set of changes. Can you please add the necessary unit test code? |
|
@gechiang for write call failures are rare and currently there is no handling at this point. haven't faced these issues in our testing. |
Sure. Since you did capture it with syslog we can defer this as future enhancement. |
How about additional test cases to cover this new set of changes? |
@gechiang |
Ok. Will go ahead approve this and merge... |
* mclagsyncd enhancements as per HLD at sonic-net/SONiC#596 * mclagsyncd enhancements as per HLD at sonic-net/SONiC#596 * mclagsyncd enhancements as per HLD at sonic-net/SONiC#596 * mclagsyncd enhancements as per HLD at sonic-net/SONiC#596 * updated mclag port isolate platform check function * MCLAG Unique IP Changes. * updated mclagsyncd * resolved compilation issues with master branch * updated mclagsyncd merge issue * addressed review comments * addressed review comments * fixed build issues with armhf platform * fixed build issues with armhf platform * fixed build issue * fixed build issue * addressed review comments * addressed review comments * removed unused code Co-authored-by: Tapash Das <tapash.das@broadcom.com>
| FieldValueTuple desc_attr("policy_desc", "Mclag egress port isolate acl"); | ||
| acl_attrs.push_back(desc_attr); | ||
|
|
||
| FieldValueTuple type_attr("type", "L3"); |
There was a problem hiding this comment.
@gitsabari From the HLD it looks that ACL is needed for L2 traffic isolation, but table type is L3.
Why do we use L3?
Is it a bug?
…net#1349) * [storyteller] adding a grep wrapper with predefined scenarios Introduce storyteller tool to grep syslog (or any other logs) for a story line of a certain scenario. Defined scenarios are: - reboot : device reboot related events. - service : service start/stop events. - link : link up/down events. - lag : LAG up/down events. - bgp : BGP config change events. - crash : process/service crash events. Unmatched cateory is used as grepping regex directly. Signed-off-by: Ying Xie <ying.xie@microsoft.com>
What I did
This PR contains the code changes to support Mclagsyncd changes for mclag enhancements as per HLD sonic-net/SONiC#596
Why I did it
Implemented as per the MCLAG enhancements HLD: sonic-net/SONiC#596
How I verified it
Mclag specific VS tests included part of check-in.
Details:
The PR includes the code changes for the mclagsyncd side of MCLAG enhancements
This PR must work with submitted PR's in other sonic repositories which are listed below.
> Iccpd: sonic-net/sonic-buildimage#4819
> Swss: #1331
> swss-common: sonic-net/sonic-swss-common#405
> Utilities: sonic-net/sonic-utilities#1138
> Mgmt-FW: sonic-net/sonic-mgmt-framework#59
added mclag sonic yang file sonic-net/sonic-buildimage#7622