Mclag enhacements #1349

gitsabari · 2020-07-09T22:42:01Z

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

ghost · 2020-07-09T22:42:14Z

All CLA requirements met.

gitsabari · 2021-01-04T22:40:33Z

@gechiang, @rlhui , @lguohan
can you please help merge and approve this at the earliest

gitsabari · 2021-01-04T23:53:40Z

@gechiang , @lguohan, @rlhui
can you please help review and approve this merge request

gitsabari · 2021-01-05T07:32:01Z

retest vs please

gitsabari · 2021-01-06T23:55:21Z

retest vs please

gitsabari · 2021-02-23T07:41:46Z

retest vs please

gitsabari · 2021-02-23T07:44:45Z

@rathnasabapathyv , @prvattem
can you please approve this PR if there are no more questions

gitsabari · 2021-05-02T15:52:24Z

akokhan i have addressed all your review comments. if you dont have futher comments, please help approve the changes

akokhan · 2021-05-02T18:49:14Z

mclagsyncd/mclaglink.h

+                return vid < fdb.vid;
+            else
+                return port_name < fdb.port_name;
+            //else if (port_name != fdb.port_name) return port_name < fdb.port_name;


Please remove if not needed

mclagsyncd/mclag.h

gechiang · 2021-05-04T02:02:59Z

mclagsyncd/mclaglink.cpp

+
+            if (write <= 0)
+            {
+                SWSS_LOG_ERROR("mclagsycnd to ICCPD, domain cfg send, buffer full; write to m_connection_socket failed");


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?

gechiang · 2021-05-04T02:04:12Z

mclagsyncd/mclaglink.cpp

+
+    if (write <= 0)
+    {
+        SWSS_LOG_ERROR("mclagsycnd to ICCPD, domain cfg send; write to m_connection_socket failed");


Any recovery or somehow to abort this operation when such error occurred?

gechiang · 2021-05-04T02:05:35Z

mclagsyncd/mclaglink.cpp

+
+            if (write <= 0)
+            {
+                SWSS_LOG_ERROR("mclagsycnd to ICCPD, mclag iface cfg send, buffer full; write to m_connection_socket failed");


After error detected just continue?

@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

mclagsyncd/mclaglink.cpp

gechiang · 2021-05-04T02:09:28Z

mclagsyncd/mclaglink.cpp

+            count = 0;
+            if (write <= 0)
+            {
+                SWSS_LOG_ERROR("mclagsycnd to ICCPD, mclag vlan member updates send, buffer full; write to m_connection_socket failed");


should it bail out? skip current iteration?

gechiang · 2021-05-04T02:13:24Z

mclagsyncd/mclaglink.cpp

+                break;
+
+            default:
+                SWSS_LOG_ERROR("Invalid option type %u", op_hdr->op_type);


Unexpected option perhaps should skip the rest of the code processing and bail out instead?

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);

mclagsyncd/mclaglink.cpp

gechiang · 2021-05-04T02:15:10Z

mclagsyncd/mclaglink.cpp

+                break;
+
+            default:
+                SWSS_LOG_WARN("Invalid option type %u", op_hdr->op_type);


Invalid Option Ok to continue?

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);

mclagsyncd/mclaglink.cpp

gechiang · 2021-05-04T02:24:10Z

I did not see any unit test code created for this set of changes. Can you please add the necessary unit test code?
At least add test cases to exercise all the messaging interactions. Please see sample unit test cases in the tests directory for docker virtual switch tests:
https://github.com/Azure/sonic-swss/tree/master/tests

gitsabari · 2021-05-17T18:35:54Z

@gechiang for 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

gechiang · 2021-05-18T05:30:21Z

@gechiang for 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

Sure. Since you did capture it with syslog we can defer this as future enhancement.

gechiang · 2021-05-18T05:33:20Z

I did not see any unit test code created for this set of changes. Can you please add the necessary unit test code?
At least add test cases to exercise all the messaging interactions. Please see sample unit test cases in the tests directory for docker virtual switch tests:
https://github.com/Azure/sonic-swss/tree/master/tests

How about additional test cases to cover this new set of changes?
Can you please work on this to provide good coverage of your new code changes?

gitsabari · 2021-06-01T09:22:55Z

I did not see any unit test code created for this set of changes. Can you please add the necessary unit test code?
At least add test cases to exercise all the messaging interactions. Please see sample unit test cases in the tests directory for docker virtual switch tests:
https://github.com/Azure/sonic-swss/tree/master/tests

How about additional test cases to cover this new set of changes?
Can you please work on this to provide good coverage of your new code changes?

@gechiang
i checked again. there are already test cases added part of #1331. It covers cases for new set of changes
tests/test_mclag.py
tests/test_mclag_fdb.py
tests/test_mclag_cfg.py

gechiang · 2021-06-01T20:11:35Z

I did not see any unit test code created for this set of changes. Can you please add the necessary unit test code?
At least add test cases to exercise all the messaging interactions. Please see sample unit test cases in the tests directory for docker virtual switch tests:
https://github.com/Azure/sonic-swss/tree/master/tests

How about additional test cases to cover this new set of changes?
Can you please work on this to provide good coverage of your new code changes?

@gechiang
i checked again. there are already test cases added part of #1331. It covers cases for new set of changes
tests/test_mclag.py
tests/test_mclag_fdb.py
tests/test_mclag_cfg.py

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>

globaltrouble · 2021-10-11T15:45:19Z

@gitsabari FYI sonic-net/sonic-buildimage#8945

globaltrouble · 2021-11-15T18:06:43Z

mclagsyncd/mclaglink.cpp

+            FieldValueTuple desc_attr("policy_desc", "Mclag egress port isolate acl");
+            acl_attrs.push_back(desc_attr);
+
+            FieldValueTuple type_attr("type", "L3");


@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>

gitsabari added 4 commits July 9, 2020 14:31

mclagsyncd enhancements as per HLD at sonic-net/SONiC#596

3c48d5b

mclagsyncd enhancements as per HLD at sonic-net/SONiC#596

b902337

mclagsyncd enhancements as per HLD at sonic-net/SONiC#596

1999388

mclagsyncd enhancements as per HLD at sonic-net/SONiC#596

1fa765b

Praveen-Brcm mentioned this pull request Sep 22, 2020

iccpd service is not running in the image built from latest SONIC mainline code. sonic-net/sonic-buildimage#5310

Open

rathnasabapathyv previously approved these changes Nov 11, 2020

View reviewed changes

tapashdas dismissed rathnasabapathyv’s stale review via 310c312 November 11, 2020 08:46

updated mclag port isolate platform check function

3779438

gitsabari force-pushed the mclag_enhacements branch from 310c312 to 3779438 Compare November 17, 2020 00:22

gitsabari and others added 7 commits November 16, 2020 16:33

Merge branch 'master' into mclag_enhacements

29464f6

MCLAG Unique IP Changes.

c201ef9

updated mclagsyncd

a6e95c3

resolved compilation issues with master branch

4d3c253

updated mclagsyncd merge issue

3d6eb75

Merge branch 'master' into mclag_enhacements

4f9fd68

Merge branch 'master' into mclag_enhacements

1184638

akokhan reviewed May 2, 2021

View reviewed changes

akokhan previously approved these changes May 2, 2021

View reviewed changes