-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
MCLAG enhacements ICCPd initial code commit #4819
Conversation
Can you please resolve the conflicts |
src/iccpd/src/iccp_ifm.c
Outdated
{ | ||
if (vlan->vlan_removed == 1) | ||
{ | ||
ICCPD_LOG_DEBUG(__FUNCTION__, "Remove %s from VLAN %d", lif->name, vlan->vid); | ||
|
||
LIST_REMOVE(vlan, port_next); | ||
#if 0 |
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 these code changes needed?
All the code in this function has been marked under "#if 0"
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.
@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
} | ||
|
||
return; | ||
} | ||
|
||
//this function is no more required - vlan membership updates comes through | ||
//state db updates from mclagsyncd |
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 the changes in this function needed?
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.
@prvattem: Thanks for the comment. The unused function iccp_parse_if_vlan_info_from_netlink is being removed.
if (sys->warmboot_start != WARM_REBOOT) | ||
mlacp_clean_fdb(); | ||
// do not required to flush FDB | ||
//if (sys->warmboot_start != WARM_REBOOT) |
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.
Is this unwanted functionality?
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.
@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
@@ -104,13 +114,117 @@ static int getHwAddr(char *buff, char *mac) | |||
return 0; | |||
} | |||
|
|||
#if 0 |
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.
Is this functionality going to get enabled later?
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.
Thanks for the comment. No unused code is deleted
@@ -375,47 +490,67 @@ static int peer_po_is_alive(struct CSM *csm, int po_ifindex) | |||
return pif_active; | |||
} | |||
|
|||
#if 0 |
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.
Is this functionality going to get enabled later?
References to this function are also commented.
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.
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
{ | ||
/*TBD: vxlan tunnel port isolation will be supportted later*/ | ||
return; | ||
#if 0 |
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.
Please remove unwanted code.
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.
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/iccp_netlink.c
Outdated
@@ -1361,7 +1788,44 @@ int iccp_system_init_netlink_socket() | |||
ICCPD_LOG_ERR(__FUNCTION__, "Failed to set buffer size of netlink event sock."); | |||
goto err_return; | |||
} | |||
#if 0 |
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.
Please remove dead code.
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.
Done.!
src/iccpd/src/mlacp_fsm.c
Outdated
@@ -767,11 +992,11 @@ void mlacp_enqueue_msg(struct CSM* csm, struct Msg* msg) | |||
if (msg == NULL ) | |||
return; | |||
|
|||
#if 0 | |||
#if 0 |
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.
please remove dead code
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.
Done.!
* | ||
****************************************/ | ||
static void mlacp_fsm_send_if_up_ack( | ||
struct CSM *csm, |
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.
any possibility that csm may be NULL passed into this 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.
@gechiang , Thanks. ACK is sent as part of processing remote IF up event, and CSM can not be NULL at this point.
src/iccpd/src/scheduler.c
Outdated
@@ -188,7 +252,7 @@ int scheduler_csm_read_callback(struct CSM* csm) | |||
|
|||
return 1; | |||
|
|||
recv_err: | |||
recv_err: |
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.
Please fix alignment
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.
Thanks, Done.
src/iccpd/src/scheduler.c
Outdated
goto reject_client; | ||
} | ||
} | ||
|
||
/* Accept*/ | ||
goto accept_client; | ||
|
||
reject_client: | ||
reject_client: |
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.
fix alignment
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.
Thanks, Done.
src/iccpd/src/scheduler.c
Outdated
if (new_fd >= 0) | ||
close(new_fd); | ||
return MCLAG_ERROR; | ||
|
||
accept_client: | ||
accept_client: |
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.
fix alignment
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.
Thanks, Done.
src/iccpd/src/scheduler.c
Outdated
@@ -445,7 +522,7 @@ int mlacp_sync_with_kernel_callback() | |||
} | |||
} | |||
|
|||
out: | |||
out: |
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.
fix alignment
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.
Thanks, Done.
src/iccpd/src/scheduler.c
Outdated
FD_SET(connFd, &(sys->readfd)); | ||
sys->readfd_count++; | ||
ICCPD_LOG_INFO(__FUNCTION__, "Connect to server %s sucess .", csm->peer_ip); | ||
goto conn_ok; | ||
} | ||
|
||
conn_fail: | ||
conn_fail: |
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.
fix alignment
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.
Thanks, Done.
src/iccpd/src/scheduler.c
Outdated
if (connFd >= 0) | ||
{ | ||
csm->sock_fd = -1; | ||
close(connFd); | ||
} | ||
conn_ok: | ||
conn_ok: |
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.
fix alignment
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.
Thanks, Done.
src/iccpd/src/scheduler.c
Outdated
@@ -613,11 +701,11 @@ int scheduler_prepare_session(struct CSM* csm) | |||
session_conn_thread_unlock(&csm->conn_mutex); | |||
} | |||
|
|||
time_update: | |||
time_update: |
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.
fix alignment
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.
Thanks, Done.
src/iccpd/src/scheduler.c
Outdated
time(&csm->connTimePrev); | ||
return 0; | ||
|
||
no_time_update: | ||
no_time_update: |
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.
fix alignment
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.
Thanks, Done.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This checkis not needed. line 92 already ensure cms is not NILL
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An error should be logged as ERROR and not NOTICE right?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error condition should be logged with ERROR and not NOTICE.
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.
Thanks, Done.!
src/iccpd/src/mclagdctl/mclagdctl.c
Outdated
@@ -932,12 +1273,11 @@ int main(int argc, char **argv) | |||
|
|||
ret = EXIT_SUCCESS; | |||
|
|||
mclagdctl_disconnect: | |||
mclagdctl_disconnect: |
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.
fix alignment please
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.
Thanks, Done.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
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.