[swss/cfgmgr] teammgr configure lacp rate #2121

globaltrouble · 2022-01-25T14:07:39Z

What I did

Make lacp_rate configurable for portchannel.

	Option specifying the rate in which we'll ask our link partner
	to transmit LACPDU packets in 802.3ad mode.  Possible values
	are:

	slow
		Request partner to transmit LACPDUs every 30 seconds

	fast
		Request partner to transmit LACPDUs every 1 second

	The default is slow.

Why I did it

In case of slow lacp_rate configuration link down will be detected in 60-90 seconds, it may be to long (for example for MCLAG high availability), in case of using --fast-rate=true link down will be detected in 2-3 seconds.

How I did it

add optional argument to config portchannel command, default=slow for backward compatibility. add lacp_rate to portchannel sonic-utilities#2036
parse argument in teammgr and forward it to teamd (this PR)
update docs Feature/add lacp rate SONiC#937

How to verify it

Confgiure bond on other side, then configure portchannel and sniff the traffic from it.

config portchannel add PortChannel0001  --fast-rate=true
config portchannel member add PortChannel0001 Ethernet0
config interface ip add PortChannel0001  192.168.1.2/24
tcpdump -ne

globaltrouble · 2022-01-25T16:03:50Z

doc/Configuration.md

@@ -1139,21 +1139,25 @@ name as object key and member list as attribute.
 ```
 {
 "PORTCHANNEL": {
-        "PortChannel0003": {


fix formatting and reorder as in config utility for better readability, "PortChannel0003" contains all mandatory attrs, "PortChannel0004" additionally contains options attrs

globaltrouble · 2022-01-31T10:05:57Z

@stcheng could you please review it?

ArthiSivanantham · 2022-01-31T20:19:30Z

cfgmgr/teammgr.cpp

@@ -553,6 +559,11 @@ task_process_status TeamMgr::addLag(const string &alias, int min_links, bool fal
        conf << ",\"fallback\":true";
    }

+    if (lacp_rate == "fast")


In "teamd.conf", (LACP RUNNER SPECIFIC OPTIONS), fast_rate is defined to be a boolean,

runner.fast_rate (bool) Option specifies the rate at which our link partner is asked to transmit LACPDU packets. If this is true then packets will be sent once per second. Otherwise they will be sent every 30 seconds.

https://www.unix.com/man-page/centos/5/teamd.conf/
http://manpages.ubuntu.com/manpages/xenial/man5/teamd.conf.5.html

While spawning the teamd process with it's config file, "slow|fast" should be converted to "false|true". Instead why can't we define "fast_rate/lacp_rate" to be a boolean in consistent with "teamd" configuration ?

I agree with Arthi, please use fast_rate in the config-DB as well.

Answering your question:

It's easier to read concreate status than to read a boolean flag and convert it to concreate status.

Boolean flags are harder to maintain (update code base), in case new type need to be added - all code need to be change (boolean flags and conditions using them).

If it's possible: modules should not depend on low level details. In this example for persion unfarmiliar with teamd it will be easier to find info only about lacp_rate, without dig in teamd.

@venkatmahalingam @ArthiSivanantham, thank you for the review, changed it to be consistent with links you pointed. Also changed config tool too.

@venkatmahalingam / @ArthiSivanantham IMHO, the arguments provided by @globaltrouble seems to be reasonable. Do we have a strong opinion here? Although, the original approach is compliant with the teamd.conf but it is not user friendly.

IMHO, lacp_rate with SLOW/FAST looks better to me

We don't have any strong opinion @nazariig it's just a suggestion as well known teamd uses fast_rate to control the LACP rate and also we can avoid conversion (lacp_rate to fast_rate) in the back-end, some NOS vendors do use "[no] lacp fast rate" config knobs.

msosyak · 2022-02-16T18:36:16Z

@ArthiSivanantham @venkatmahalingam @prsunny Are there any other comments? If not, please, help to merge this and related PR

msosyak · 2022-03-24T08:47:34Z

@ArthiSivanantham @venkatmahalingam @prsunny Could we merge this and related?

msosyak · 2022-03-29T08:59:29Z

@venkatmahalingam @ArthiSivanantham do you have other comments? If not, please approve.

msosyak · 2022-04-05T08:49:37Z

@qiluo-msft @prsunny @nazariig Please help review and merge

nazariig · 2022-04-05T09:27:32Z

tests/test_portchannel.py

+
+        # Create PortChannels
+        tbl = swsscommon.Table(self.cdb, "PORTCHANNEL")
+        fvs_base = [("admin_status", "up"), ("mtu", "9100"), ("oper_status", "up"), ("lacp_key", "auto")]


@msosyak why do we need oper_status in Config DB?

nazariig · 2022-04-05T09:28:24Z

tests/test_portchannel.py

+        self.cdb = swsscommon.DBConnector(4, dvs.redis_sock, 0)
+
+        # Create PortChannels
+        tbl = swsscommon.Table(self.cdb, "PORTCHANNEL")


@msosyak please reuse LAG testing framework: https://github.com/Azure/sonic-swss/blob/master/tests/dvslib/dvs_lag.py

nazariig · 2022-04-05T09:31:40Z

tests/test_portchannel.py

+        time.sleep(1)
+
+        # Add members to PortChannels
+        tbl = swsscommon.Table(self.cdb, "PORTCHANNEL_MEMBER")


@msosyak please reuse LAG testing framework: https://github.com/Azure/sonic-swss/blob/master/tests/dvslib/dvs_lag.py

nazariig · 2022-04-05T09:32:35Z

tests/test_portchannel.py

+        time.sleep(1)
+
+        # test fast rate was not set on portchannel_slow
+        output = dvs.runcmd("teamdctl {} state dump".format(portchannel_slow[0]))[1]


@msosyak suggest extending and reusing LAG testing framework: https://github.com/Azure/sonic-swss/blob/master/tests/dvslib/dvs_lag.py

nazariig · 2022-04-05T09:33:40Z

tests/test_portchannel.py

+        tbl = swsscommon.Table(self.cdb, "PORTCHANNEL_MEMBER")
+        for portchannel in portchannel_slow, portchannel_fast:
+            tbl._del(portchannel[0] + "|" + portchannel[1])
+        time.sleep(1)


@msosyak get_and_verify_port_channel_members assert?

nazariig · 2022-04-05T09:34:20Z

tests/test_portchannel.py

+        tbl = swsscommon.Table(self.cdb, "PORTCHANNEL")
+        for portchannel in portchannel_slow, portchannel_fast:
+            tbl._del(portchannel[0])
+        time.sleep(1)


@msosyak get_and_verify_port_channel assert?

prsunny · 2022-04-06T01:51:11Z

@qiluo-msft @prsunny @nazariig Please help review and merge

@msosyak , changes looks good to me. Please address the outstanding comments and also get the dependent PRs in sonic-utilities, HLD merged.

@nazariig , @venkatmahalingam , @ArthiSivanantham to signoff

bratashX · 2022-05-13T21:22:13Z

will be good to include these changes into 202111 branch

msosyak · 2022-07-13T08:35:18Z

Sorry for the late reaction.

@prsunny @nazariig Conflicts resolved. UT updated.

nazariig · 2022-07-13T08:40:55Z

tests/test_portchannel.py

@@ -89,6 +91,30 @@ def test_Portchannel(self, dvs, testlog):
        lagms = lagmtbl.getKeys()
        assert len(lagms) == 0

+    def test_Portchannel_fast_rate(self, dvs, testlog):


@msosyak please use test parametrization to avoid loops

msosyak · 2022-07-19T08:58:21Z

@nazariig Do you have other comments?

msosyak · 2022-07-20T14:45:47Z

@prsunny Could we merge this?

prsunny · 2022-07-22T02:15:52Z

@judyjoseph , can you please review/signoff?

judyjoseph

LGTM, Can you also add the test results (the o/p of teamdctl state dump) in PR description with local/peer device having lacp rate configured as fast/slow rate respectively.

* add lacp_rate config * add lacp_rate to teammgr addLag params * docs for portchannel db config * add test Co-authored-by: Anton <antonx.novikau@intel.com> Co-authored-by: Myron Sosyak <myronx.sosyak@intel.com>

#### What I did Make lacp_rate configurable for portchannel. ``` Option specifying the rate in which we'll ask our link partner to transmit LACPDU packets in 802.3ad mode. Possible values are: slow Request partner to transmit LACPDUs every 30 seconds fast Request partner to transmit LACPDUs every 1 second The default is slow. ``` #### Why I did it In case of slow lacp_rate configuration link down will be detected in 60-90 seconds, it may be to long (for example for MCLAG high availability), in case of using ` --fast-rate=true` link down will be detected in 2-3 seconds. #### How I did it * add optional argument to `config portchannel` command, default=slow for backward compatibility. (this PR) * parse argument in `teammgr` and forward it to `teamd` (other PR: sonic-net/sonic-swss#2121) * update docs sonic-net/SONiC#937 #### How to verify it Confgiure bond on other side, then configure portchannel and sniff the traffic from it. ``` config portchannel add PortChannel0001 --fast-rate=true config portchannel member add PortChannel0001 Ethernet0 config interface ip add PortChannel0001 192.168.1.2/24 tcpdump -ne ```

globaltrouble requested a review from prsunny as a code owner January 25, 2022 14:07

globaltrouble marked this pull request as draft January 25, 2022 14:07

globaltrouble mentioned this pull request Jan 25, 2022

add lacp_rate to portchannel sonic-net/sonic-utilities#2036

Merged

globaltrouble force-pushed the feature/teamd-configure-lacp-rate branch from b75507c to 7915fdf Compare January 25, 2022 15:48

globaltrouble commented Jan 25, 2022

View reviewed changes

globaltrouble mentioned this pull request Jan 28, 2022

Feature/add lacp rate sonic-net/SONiC#937

Open

globaltrouble marked this pull request as ready for review January 28, 2022 15:01

ArthiSivanantham reviewed Jan 31, 2022

View reviewed changes

prsunny requested a review from judyjoseph February 1, 2022 01:02

globaltrouble force-pushed the feature/teamd-configure-lacp-rate branch from 0cdaec9 to 74c52f4 Compare February 1, 2022 09:15

msosyak force-pushed the feature/teamd-configure-lacp-rate branch 3 times, most recently from 3e38104 to 1107049 Compare March 22, 2022 09:29

nazariig suggested changes Apr 5, 2022

View reviewed changes

msosyak force-pushed the feature/teamd-configure-lacp-rate branch 3 times, most recently from de2d6d3 to 1a64d22 Compare July 5, 2022 13:14

Anton added 6 commits July 12, 2022 11:37

add lacp_rate config

4084fce

add lacp_rate to teammgr addLag params

4d58812

docs for portchannel db config

59bd028

add test

8dd849f

replace lacp_rate by fast_rate

9ad056f

alter docs

9dc60fc

Anton and others added 3 commits July 12, 2022 11:37

fix compiler warning

19408f3

FIx merge conflicts

c8e406a

Update test to use dvs

e0ba9f6

msosyak force-pushed the feature/teamd-configure-lacp-rate branch from 1a64d22 to 5565488 Compare July 12, 2022 18:38

nazariig reviewed Jul 13, 2022

View reviewed changes

Fix test

c38790c

msosyak force-pushed the feature/teamd-configure-lacp-rate branch from 5565488 to c38790c Compare July 14, 2022 10:49

Fix typo

71e4baf

nazariig approved these changes Jul 19, 2022

View reviewed changes

judyjoseph approved these changes Jul 29, 2022

View reviewed changes

judyjoseph merged commit dc477fb into sonic-net:master Aug 3, 2022

stephenxs mentioned this pull request Aug 9, 2022

[submodule] Advance sonic-swss and sonic-utilities pointer sonic-net/sonic-buildimage#11643

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[swss/cfgmgr] teammgr configure lacp rate #2121

[swss/cfgmgr] teammgr configure lacp rate #2121

globaltrouble commented Jan 25, 2022 •

edited

Loading

globaltrouble Jan 25, 2022 •

edited

Loading

globaltrouble commented Jan 31, 2022

ArthiSivanantham Jan 31, 2022

venkatmahalingam Jan 31, 2022

globaltrouble Feb 1, 2022 •

edited

Loading

nazariig Apr 5, 2022 •

edited

Loading

venkatmahalingam Apr 7, 2022 •

edited

Loading

msosyak commented Feb 16, 2022

msosyak commented Mar 24, 2022

msosyak commented Mar 29, 2022

msosyak commented Apr 5, 2022

nazariig Apr 5, 2022

nazariig Apr 5, 2022

nazariig Apr 5, 2022

nazariig Apr 5, 2022

nazariig Apr 5, 2022

nazariig Apr 5, 2022

prsunny commented Apr 6, 2022

bratashX commented May 13, 2022

msosyak commented Jul 13, 2022

nazariig Jul 13, 2022

msosyak Jul 14, 2022

msosyak commented Jul 19, 2022

msosyak commented Jul 20, 2022

prsunny commented Jul 22, 2022

judyjoseph left a comment

[swss/cfgmgr] teammgr configure lacp rate #2121

[swss/cfgmgr] teammgr configure lacp rate #2121

Conversation

globaltrouble commented Jan 25, 2022 • edited Loading

What I did

Why I did it

How I did it

How to verify it

globaltrouble Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

globaltrouble commented Jan 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

globaltrouble Feb 1, 2022 • edited Loading

Choose a reason for hiding this comment

nazariig Apr 5, 2022 • edited Loading

Choose a reason for hiding this comment

venkatmahalingam Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

msosyak commented Feb 16, 2022

msosyak commented Mar 24, 2022

msosyak commented Mar 29, 2022

msosyak commented Apr 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prsunny commented Apr 6, 2022

bratashX commented May 13, 2022

msosyak commented Jul 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msosyak commented Jul 19, 2022

msosyak commented Jul 20, 2022

prsunny commented Jul 22, 2022

judyjoseph left a comment

Choose a reason for hiding this comment

globaltrouble commented Jan 25, 2022 •

edited

Loading

globaltrouble Jan 25, 2022 •

edited

Loading

globaltrouble Feb 1, 2022 •

edited

Loading

nazariig Apr 5, 2022 •

edited

Loading

venkatmahalingam Apr 7, 2022 •

edited

Loading