Skip to content
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

Fix portconfig portchannel 201911 #1384

Open
wants to merge 2 commits into
base: 201911
Choose a base branch
from

Conversation

ghost
Copy link

@ghost ghost commented Jan 25, 2021

- What I did
Resolves sonic-net/sonic-buildimage#6430
Added support of PortChannels to portconfig script.

- How I did it
Added checking of prefix in name of port, passed to the script. If it is EthernetXX, the script uses table name PORT to operate with DB, if it is PortChannelXX, then the script will use table name PORTCHANNEL, otherwise it will throw an exception to notify that port type is wrong.

- How to verify it

  1. sudo config portchannel add PortChannel0001
  2. sudo config interface mtu PortChannel0001 1510
  3. see current MTU of the PortChannel with
    ip link show PortChannel0001

- Previous command output (if the output of a command-line utility has changed)

- New command output (if the output of a command-line utility has changed)
if to try to pass some trash as port argument, like this sudo config interface mtu Unknown0002 9100, you will receive error in console output: Invalid port type specified

Maksym Belei added 2 commits January 25, 2021 14:32
* Adding support of PortChannels to portconfig script
  to be able to configure PortChannels by CLI commands,
  like 'config interface mtu'

Signed-off-by: Maksym Belei <[email protected]>
* Reformat the script according to Python code style,
  fix typos.

Signed-off-by: Maksym Belei <[email protected]>
Copy link
Contributor

@gechiang gechiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes looks good. Can you add the unit test test cases to handle the scenarios to ensure MTU can be changed for port channels as well as error cases where the prefix of the port/portchannel is not correct and the handler fails the configuration request.
Also, I think this code has other changes already so please rebase to the latest. I will review and merge once these are done. Thanks!

@TafkaMax
Copy link

Changes looks good. Can you add the unit test test cases to handle the scenarios to ensure MTU can be changed for port channels as well as error cases where the prefix of the port/portchannel is not correct and the handler fails the configuration request. Also, I think this code has other changes already so please rebase to the latest. I will review and merge once these are done. Thanks!

What needs to be done here? I would like to this feature to be merged to the community sonic aswell.

@TafkaMax
Copy link

Currently I had to manually edit /etc/config/config_db.json and set MTU to 1500 then config load. Like WTF...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants