MCLAG enhacements ICCPd initial code commit#4819
Conversation
|
Can you please resolve the conflicts |
src/iccpd/src/iccp_ifm.c
Outdated
There was a problem hiding this comment.
Are these code changes needed?
All the code in this function has been marked under "#if 0"
There was a problem hiding this comment.
@prvattem: Thanks for the comment. The unused function iccp_parse_if_vlan_info_from_netlink is being removed.
src/iccpd/src/iccp_ifm.c
Outdated
There was a problem hiding this comment.
Are the changes in this function needed?
There was a problem hiding this comment.
@prvattem: Thanks for the comment. The unused function iccp_parse_if_vlan_info_from_netlink is being removed.
src/iccpd/src/mlacp_link_handler.c
Outdated
There was a problem hiding this comment.
@prvattem: Yes, This flush is not required when ICCPd session is coming up. The unwanted flush is being commented.
src/iccpd/src/mlacp_link_handler.c
Outdated
There was a problem hiding this comment.
Is this functionality going to get enabled later?
There was a problem hiding this comment.
Thanks for the comment. No unused code is deleted
src/iccpd/src/mlacp_link_handler.c
Outdated
There was a problem hiding this comment.
Is this functionality going to get enabled later?
References to this function are also commented.
There was a problem hiding this comment.
Thanks for the comment. Yes with latest enhancements this code is not used. But the plan is to keep the change in case if required to enable back.
src/iccpd/src/mlacp_link_handler.c
Outdated
There was a problem hiding this comment.
Thanks for the comment. The code kept to enable in the future.
|
Please resolve the conflicts, if any. |
5745928 to
4e78479
Compare
Resolved the merge conflicts. |
Merged conflicts are resolved |
|
@rathnasabapathyv , @prvattem |
src/iccpd/src/scheduler.c
Outdated
| return 1; | ||
|
|
||
| recv_err: | ||
| recv_err: |
src/iccpd/src/scheduler.c
Outdated
| goto accept_client; | ||
|
|
||
| reject_client: | ||
| reject_client: |
src/iccpd/src/scheduler.c
Outdated
| return MCLAG_ERROR; | ||
|
|
||
| accept_client: | ||
| accept_client: |
src/iccpd/src/scheduler.c
Outdated
| } | ||
|
|
||
| out: | ||
| out: |
src/iccpd/src/scheduler.c
Outdated
| } | ||
|
|
||
| conn_fail: | ||
| conn_fail: |
src/iccpd/src/scheduler.c
Outdated
| close(connFd); | ||
| } | ||
| conn_ok: | ||
| conn_ok: |
src/iccpd/src/scheduler.c
Outdated
| } | ||
|
|
||
| time_update: | ||
| time_update: |
src/iccpd/src/scheduler.c
Outdated
| return 0; | ||
|
|
||
| no_time_update: | ||
| no_time_update: |
src/iccpd/src/mlacp_sync_update.c
Outdated
| mlacp_peer_mlag_intf_delete_handler(csm, pif->name); | ||
|
|
||
| /* Delete remote interface info from STATE_DB */ | ||
| if (csm) |
There was a problem hiding this comment.
This checkis not needed. line 92 already ensure cms is not NILL
There was a problem hiding this comment.
Thanks, Done.!
src/iccpd/src/mlacp_sync_update.c
Outdated
| arp_entry->ifname, show_ip_str(arp_entry->ipv4_addr), mac_str); | ||
| return MCLAG_ERROR; | ||
| if (err != ICCP_NLE_SEQ_MISMATCH) { | ||
| ICCPD_LOG_NOTICE(__FUNCTION__, "ARP add failure for %s %s %s, status %d", |
There was a problem hiding this comment.
An error should be logged as ERROR and not NOTICE right?
There was a problem hiding this comment.
Thanks, Done.!
src/iccpd/src/mlacp_sync_update.c
Outdated
| arp_entry->ifname, show_ip_str(arp_entry->ipv4_addr), mac_str); | ||
| return MCLAG_ERROR; | ||
| if (err != ICCP_NLE_SEQ_MISMATCH) { | ||
| ICCPD_LOG_NOTICE(__FUNCTION__, "ARP delete failure for %s %s %s, status %d", |
There was a problem hiding this comment.
Error condition should be logged with ERROR and not NOTICE.
There was a problem hiding this comment.
Thanks, Done.!
src/iccpd/src/mclagdctl/mclagdctl.c
Outdated
| ret = EXIT_SUCCESS; | ||
|
|
||
| mclagdctl_disconnect: | ||
| mclagdctl_disconnect: |
|
@Praveen-Brcm can you help check why mclagsyncd socket sometimes receive close but actually iccpd just send some message to socket: "Connection lost" , seems mclagsyncd send fine, but receive exist some problem. is my local build miss some thing?[another device seems fine] |
|
server receive q's size is not empty, seems read() issue: |
@pettershao-ragilenetworks , The fix for this has been addressed part of PR# sonic-net/sonic-swss#1832 .? Thanks, - Prvaeen |
|
@gechiang : Can this be merged now as the PR 1338 is merged, and PR 1331 is blocked on VS tests to re-run. Please confirm and merge.? Thanks - Praveen. |
yes. |
@Praveen-Brcm, @rlhui I think we discussed this in the meeting. This PR also needs (sonic-net/sonic-swss#1331) or else the existing MCLAG will be broken as soon as this PR is merged... So it was decided that we will also wait for the SWSS(1331) to complete and at that time merge both in parallel to reduce the feature broken time... Is this not what we agreed? |
@gechiang : Yes that was the agreement. Earlier we had the dependency with PR#1338 and we had to let that merge for the VS tests to pass for PR#1331. While we already merge 4 of the related PR's my request was to consider the current PR to merge as well, while we hope to have the PR #1331 merge soon once the VS tests pass. |
IF the MC LAG feature in current master branch is already broken then we might as well go ahead get this merged... but if it is not, then I prefer to not break the feature for an unknown (potentially long) period of time... |
|
Is there a way to download an image (sonic-aboot-broadcom.swi) built from master? Otherwise, where do I get an image for testing MCLAG? |
* MCLAG enhacements ICCPd initial code commit * Resolving the merge conflicts with orighin * L3 MCLAG Enhancements and Unique IP Changes. * Addressed review comments Co-authored-by: Tapash Das <tapash.das@broadcom.com>
|
| ICCPD_LOG_DEBUG("ICCP_FDB", "MAC update from mclagsyncd: Update MAC remote to local %s, vlan %d" | ||
| " ifname %s, del entry from MCLAG_FDB_TABLE", | ||
| mac_addr_to_str(mac_msg->mac_addr), mac_msg->vid, mac_msg->ifname); | ||
| del_mac_from_chip(mac_msg); |
There was a problem hiding this comment.
how do we distinguish this ADD is local learn or by "add back" action?
* MCLAG enhacements ICCPd initial code commit * Resolving the merge conflicts with orighin * L3 MCLAG Enhancements and Unique IP Changes. * Addressed review comments Co-authored-by: Tapash Das <tapash.das@broadcom.com>
|
|
||
| /*Update local item*/ | ||
| memcpy(&mac_msg->ifname, MacData->ifname, MAX_L_PORT_NAME); | ||
| add_mac_to_chip(mac_msg, mac_msg->fdb_type); |
There was a problem hiding this comment.
@Praveen-Brcm
You changed the behavior from del_mac_from_chip() to add_mac_to_chip() even when the peerlink is down. Is it intended? Why?
Because the behavior is different from the behavior at line#531, the mac address is added when the peerlink is up.
Thank you!
| /*Redirect the mac to peer-link*/ | ||
| /*must redirect but if peerlink is down FdbOrch will delete MAC */ | ||
| memcpy(&mac_msg->ifname, csm->peer_itf_name, MAX_L_PORT_NAME); | ||
| add_mac_to_chip(mac_msg, mac_msg->fdb_type); |
There was a problem hiding this comment.
@Praveen-Brcm
You add the mac even when the peerlink is down. Is it intended? Why?
Because the behavior is different from the behavior at line#531, the mac address is added when the peerlink is up.
Thank you!
| @@ -347,11 +530,22 @@ int mlacp_fsm_update_mac_entry_from_peer( struct CSM* csm, struct mLACPMACData * | |||
| if (csm->peer_link_if && csm->peer_link_if->state == PORT_STATE_UP) | |||
| add_mac_to_chip(mac_msg, mac_msg->fdb_type); | |||
There was a problem hiding this comment.
@Praveen-Brcm
The mac address is added only when peer-link is configured and the link is up.
This PR contains the ICCPd changes to support MCLAG enhancements feature.
Implemented as per the MCLAG enhancements HLD mentioned at : MCLAG Enhancements HLD SONiC#596
Compile and verify MCLAG behavior.
The PR includes the ICCPD code changes for the MCLAG enhancements support.
This PR must work with submitted PR's in other sonic repositories which are listed below.