-
Notifications
You must be signed in to change notification settings - Fork 635
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
add lacp_rate to portchannel #2036
add lacp_rate to portchannel #2036
Conversation
5f6f702
to
86f00eb
Compare
@stcheng could you please review it? |
7b7a5fe
to
cc62984
Compare
cf3037d
to
d5eeccd
Compare
@qiluo-msft @bingwang-ms Help to merge this and related PRs, Please. |
@click.pass_context | ||
def add_portchannel(ctx, portchannel_name, min_links, fallback): | ||
def add_portchannel(ctx, portchannel_name, min_links, fallback, fast_rate): |
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.
@globaltrouble please add a UTs to cover this case
will be good to include these changes into 202111 branch |
Please solve the conflicts |
34549bb
to
9037aa8
Compare
Sorry for the late reaction. @qiluo-msft Conflicts resolved. |
tests/portchannel_test.py
Outdated
obj = {'db':db.cfgdb} | ||
|
||
# add a portchannel with fats rate | ||
result = runner.invoke(config.config.commands["portchannel"].commands["add"], ["PortChannel0005", "--fast-rate", "True"], obj=obj) |
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.
@globaltrouble please remove all created objects on teardown. Also suggest to have parametrization for True/False
values
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.
@nazariig Parametrization added. Regarding "remove all created objects on teardown" why it is needed? The DB is instantiated for each test from the template https://github.com/Azure/sonic-utilities/blob/3600639c16079f17089efd655c818022987032bb/tests/mock_tables/config_db.json
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.
Regarding "remove all created objects on teardown" why it is needed?
@msosyak just a common practise, can be omitted if you make sure auto cleanup is taking place
f266dfb
to
c76a2f4
Compare
@@ -1946,8 +1946,11 @@ def portchannel(db, ctx, namespace): | |||
@click.argument('portchannel_name', metavar='<portchannel_name>', required=True) | |||
@click.option('--min-links', default=1, type=click.IntRange(1,1024)) | |||
@click.option('--fallback', default='false') | |||
@click.option('--fast-rate', default='false', | |||
type=click.Choice(['true', 'false'], |
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.
@globaltrouble IMHO, better to use flags or click.BOOL
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.
In the current implementation, the syntax is the same for fallback
and for fast-rate
. Booth requires an argument. If change type to bool or flags then I think it should be done for both options together is separate PR
@nazariig Do you have other comments? |
c76a2f4
to
a085315
Compare
@nazariig Could you approve, please? |
doc/Command-Reference.md
Outdated
@@ -6925,12 +6925,13 @@ When any port is already member of any other portchannel and if user tries to ad | |||
Command takes two optional arguements given below. | |||
1) min-links - minimum number of links required to bring up the portchannel | |||
2) fallback - true/false. LACP fallback feature can be enabled / disabled. When it is set to true, only one member port will be selected as active per portchannel during fallback mode. Refer https://github.com/Azure/SONiC/blob/master/doc/lag/LACP%20Fallback%20Feature%20for%20SONiC_v0.5.md for more details about fallback feature. | |||
3) fast-rate - true/false, default is flase (slow). Option specifying the rate in which we'll ask our link partner to transmit LACPDU packets in 802.3ad mode. slow - request partner to transmit LACPDUs every 30 seconds, fast - request partner to transmit LACPDUs every 1 second. In slow mode 60-90 seconds needed to detect linkdown, in fast mode only 2-3 seconds. |
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.
typo: flase
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.
Fixed
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.
LGTM. Thinking from db migration angle as this is a new field being added -- I guess is ok as the default is false.
/easycla |
@judyjoseph Could you help to merge this? |
@qiluo-msft Please, help to merge. |
#### 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 ```
What I did
Make lacp_rate configurable for portchannel.
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
config portchannel
command, default=slow for backward compatibility. (this PR)teammgr
and forward it toteamd
(other PR: [swss/cfgmgr] teammgr configure lacp rate sonic-swss#2121)How to verify it
Confgiure bond on other side, then configure portchannel and sniff the traffic from it.