-
Notifications
You must be signed in to change notification settings - Fork 667
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
[config]: Add config port channel mtu command #564
base: master
Are you sure you want to change the base?
Conversation
Add below command: config portchannel mtu <portchannel_name> <mtu> Signed-off-by: Richard Wu <wutong23@baidu.com>
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.
As part of this PR, please also update the Command Reference guide to reflect this new command; https://github.com/Azure/sonic-utilities/blob/master/doc/Command-Reference.md
1. Update command reference document Signed-off-by: Richard Wu <wutong23@baidu.com>
1. update command reference document Signed-off-by: Richard Wu <wutong23@baidu.com>
Done |
def set_portchannel_mtu(ctx, portchannel_name, mtu): | ||
"""set port channel mtu""" | ||
db = ctx.obj['db'] | ||
db.mod_entry('PORTCHANNEL', portchannel_name, {"mtu":mtu}) |
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.
I see that almost all port channel related Cmds do not check much on the given parameter(s). I really think we should fix all previous Portchannel related commands to at least validate that the given parameter(s) make sense. For new PortChannel command being added should start implementing parameter checking as a basic requirment. For this particular case it should check that the specified portchannel exists in the Config DB and if not, reject the request and print something so that the user is aware that he has made a mistake and can try again with the correct parameter...
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.
Currently the port MTU is being configured the following way:
config interface mtu Ethernet0 5000
This is handled via portconfig.py
I browsed that code and it seems to be enhanced to handle Portchannel by checking the PORTCHANNEL in the config DB if the interface specified is LAG. so something as following can be done:
config interface mtu PortChannel0002 5000
I feel this may be a better approach instead. This also make the MTU config with consisten CLI format. portconfig.py has included basic parameter checks for the interface and display error correctly. So I would suggest that we handle this by changing portconfig.py instead.
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.
I have a question. Should we change mtu of interfaces included to the LAG also?
Will it work if mtu of the PortChannel would be 9000, and member interfaces just 1500?
The purpose of changing LAG MTU is to set the correct MTU of linux TUN/TAP device. |
Once a port becomes part of the LAG, it's MTU value should NOT be configurable any more. It is no longer an individual port. The MTU value should be configured on top of the LAG it belongs and all LAG members inherit this same LAG MTU value. This means we need to detect that the port itself is already part of a LAG and prohibit user from making any MTU configuration on that LAG member port directly... |
Add below command:
config portchannel mtu <portchannel_name>
Signed-off-by: Richard Wu wutong23@baidu.com
- What I did
add command to set the MTU of port channel
- How to verify it
1, config portchannel mtu <portchannel_name>
2, check the port channel MTU in APP DB and linux via "ip link show"