[config] Add support of PortChannels to portconfig #1375

ghost · 2021-01-21T15:59:35Z

- What I did
Resolves sonic-net/sonic-buildimage#6430
Added support of PortChannels to portconfig script.
Added mocking of redis into portconfig script.
Added unit tests for subcommand config interface mtu.

- 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

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

Test result in case the portchannel has no members:

admin@sonic:~$ ip link show PortChannel0011
83: PortChannel0011: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 9000 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
    link/ether 1c:34:da:eb:ca:80 brd ff:ff:ff:ff:ff:ff
admin@sonic:~$ show int portchannel 
Flags: A - active, I - inactive, Up - up, Dw - Down, N/A - not available,
       S - selected, D - deselected, * - not synced
  No.  Team Dev         Protocol     Ports
-----  ---------------  -----------  -------
 0011  PortChannel0011  LACP(A)(Dw)  N/A
admin@sonic:~$ sudo config int mtu PortChannel0011 512
admin@sonic:~$ ip link show PortChannel0011
83: PortChannel0011: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 512 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
    link/ether 1c:34:da:eb:ca:80 brd ff:ff:ff:ff:ff:ff
admin@sonic:~$ sudo config portchannel member add PortChannel0011 Ethernet12
Usage: config portchannel member add [OPTIONS] <portchannel_name> <port_name>
Try "config portchannel member add -h" for help.

Error: Port MTU of Ethernet12 is different than the PortChannel0011 MTU size
admin@sonic:~$ sudo config int mtu Ethernet12 512
admin@sonic:~$ sudo config portchannel member add PortChannel0011 Ethernet12
admin@sonic:~$ show int portchannel 
Flags: A - active, I - inactive, Up - up, Dw - Down, N/A - not available,
       S - selected, D - deselected, * - not synced
  No.  Team Dev         Protocol     Ports
-----  ---------------  -----------  -------------
 0011  PortChannel0011  LACP(A)(Dw)  Ethernet12(D)

Test result in case the portchannel has a member:

admin@sonic:~$ show int portchannel 
Flags: A - active, I - inactive, Up - up, Dw - Down, N/A - not available,
       S - selected, D - deselected, * - not synced
  No.  Team Dev         Protocol     Ports
-----  ---------------  -----------  -------------
 0011  PortChannel0011  LACP(A)(Dw)  Ethernet12(D)
admin@sonic:~$ 
admin@sonic:~$ sudo config int mtu PortChannel0011 9100
admin@sonic:~$ ip link show PortChannel0011
83: PortChannel0011: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 9100 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
    link/ether 1c:34:da:eb:ca:80 brd ff:ff:ff:ff:ff:ff
admin@sonic:~$ ip link show Ethernet12
87: Ethernet12: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 9100 qdisc pfifo_fast master PortChannel0011 state DOWN mode DEFAULT group default qlen 1000
    link/ether 1c:34:da:eb:ca:80 brd ff:ff:ff:ff:ff:ff

- 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

anamehra · 2021-01-22T20:30:37Z

Diff looks good, works fine with Ethernet and PortChannel ports, will need this for 201911. Thanks

gechiang · 2021-02-12T21:49:30Z

@maksymbelei95
What if the port channel although configured but has no member ports in it?
Can you try it out and see what is the end result?
also, it is a policy that all code changes require unit test code that goes with it. Please include the unit test code for this change.
Thanks!

ghost · 2021-02-15T13:00:39Z

@gechiang

What if the port channel although configured but has no member ports in it?

I have tested the case. Here is my output:

admin@sonic:~$ ip link show PortChannel0011
83: PortChannel0011: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 9000 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
    link/ether 1c:34:da:eb:ca:80 brd ff:ff:ff:ff:ff:ff
admin@sonic:~$ show int portchannel 
Flags: A - active, I - inactive, Up - up, Dw - Down, N/A - not available,
       S - selected, D - deselected, * - not synced
  No.  Team Dev         Protocol     Ports
-----  ---------------  -----------  -------
 0011  PortChannel0011  LACP(A)(Dw)  N/A
admin@sonic:~$ sudo config int mtu PortChannel0011 512
admin@sonic:~$ ip link show PortChannel0011
83: PortChannel0011: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 512 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
    link/ether 1c:34:da:eb:ca:80 brd ff:ff:ff:ff:ff:ff
admin@sonic:~$ sudo config portchannel member add PortChannel0011 Ethernet12
Usage: config portchannel member add [OPTIONS] <portchannel_name> <port_name>
Try "config portchannel member add -h" for help.

Error: Port MTU of Ethernet12 is different than the PortChannel0011 MTU size
admin@sonic:~$ sudo config int mtu Ethernet12 512
admin@sonic:~$ sudo config portchannel member add PortChannel0011 Ethernet12
admin@sonic:~$ show int portchannel 
Flags: A - active, I - inactive, Up - up, Dw - Down, N/A - not available,
       S - selected, D - deselected, * - not synced
  No.  Team Dev         Protocol     Ports
-----  ---------------  -----------  -------------
 0011  PortChannel0011  LACP(A)(Dw)  Ethernet12(D)

Another case(the port channel has a member):

admin@sonic:~$ show int portchannel 
Flags: A - active, I - inactive, Up - up, Dw - Down, N/A - not available,
       S - selected, D - deselected, * - not synced
  No.  Team Dev         Protocol     Ports
-----  ---------------  -----------  -------------
 0011  PortChannel0011  LACP(A)(Dw)  Ethernet12(D)
admin@sonic:~$ 
admin@sonic:~$ sudo config int mtu PortChannel0011 9100
admin@sonic:~$ ip link show PortChannel0011
83: PortChannel0011: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 9100 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
    link/ether 1c:34:da:eb:ca:80 brd ff:ff:ff:ff:ff:ff
admin@sonic:~$ ip link show Ethernet12
87: Ethernet12: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 9100 qdisc pfifo_fast master PortChannel0011 state DOWN mode DEFAULT group default qlen 1000
    link/ether 1c:34:da:eb:ca:80 brd ff:ff:ff:ff:ff:ff

Both cases look good.

also, it is a policy that all code changes require unit test code that goes with it. Please include the unit test code for this change.
Thanks!

Sure. But, as I mentioned before in the related ticket, I found out that there are no unit tests, related to config interface commands. I thought it is related to #1301, isn't it?

gechiang · 2021-02-16T18:35:30Z

@maksymbelei95
Thanks for the added result of various config scenario. Please update/add those under "How to verify it" of this PR.
As for the testcase, can you please refer to this PR which has both multi-asic support and single asic support for config:
#1248

Please follow the above PR and add the necessary test cases.
Thanks!

judyjoseph · 2021-02-18T16:16:41Z

scripts/portconfig

@@ -22,15 +22,20 @@ import argparse
 import swsssdk

 PORT_TABLE_NAME = "PORT"
+PORTCHANNEL_TABLE_NAME = "PORTCHANNEL"
+PORT_PREFIX_ETHERNET = "Ethernet"


Please use the API's defined in https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-py-common/sonic_py_common/interface.py. This prevents duplicate definitions of table names and prefixes.

judyjoseph · 2021-02-18T16:18:01Z

scripts/portconfig

+        if args.port.startswith(PORT_PREFIX_ETHERNET):
+            port_table_name = PORT_TABLE_NAME
+        elif args.port.startswith(PORT_PREFIX_PORTCHANNEL):
+            port_table_name = PORTCHANNEL_TABLE_NAME


This logic can also be replaced with https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-py-common/sonic_py_common/interface.py#L69

ghost · 2021-02-20T18:53:04Z

retest this please

ghost · 2021-02-22T15:44:34Z

@gechiang, @judyjoseph, I have added a related unit test cases to the PR. Could I ask you to review them?

ghost · 2021-03-23T12:51:32Z

@gechiang, @judyjoseph, could you review the PR?

judyjoseph · 2021-03-31T06:23:49Z

/AzurePipelines run

azure-pipelines · 2021-03-31T06:23:59Z

Azure Pipelines successfully started running 1 pipeline(s).

ghost · 2021-04-14T13:03:07Z

@gechiang, @judyjoseph, I have rebased the PR, unit tests done successfully. Could your check the PR?

judyjoseph · 2021-06-04T06:47:04Z

@maksymbelei95, Could you please resolve this conflict before we merge it in - thanks.

gechiang · 2021-06-04T15:47:14Z

@maksymbelei95 It appears that if a port is already a member of portchannel, user can still configure its MTU on its own? If there is a conflict between portchannel configured MTU and that of its member I believe the portchannel will force its value to its member. but after that if another MTU configured to one of its member with conflicting value, I believe it will again becomes a conflict. I think if we decide to support MTU configured on port-channel, we must add check to disallow user from configuring MTU onto the individual LAG members. Also, when the port is being converted to LAG, we should check that if its MTU is at default value and if it is not, we should disallow the user from adding it to portchannel until that is corrected and then let it inherit what is already configured for the LAG itself. We also need to consider what if a member is deleted from port-channel, what would its MTU be? Retain what it inherited from the port-channel? or go back to default MTU? I think all these requires some answer before we should allow MTU configuration for portchannel. Can you work on these so that we have a solid coverage of all these scenarios? Thanks!

* 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]>

…Minor refactoring. * Adding mocking of redis DB into portconfig script for unit testing purpose. * Using of commom API from sonic_py_common.interface instead of defining of own constants and conditions. * Refactoring "mtu" function to be more consistant and efficient. Signed-off-by: Maksym Belei <[email protected]>

* Adding unit tests for subcommand 'config intf mtu' including testing of portconfig script. Signed-off-by: Maksym Belei <[email protected]>

NGorb-jabil · 2021-10-21T08:32:39Z

@gechiang I have checked all cases you mentioned above.
User unable to add port into port-channel with mtu different from port-channel mtu, also its impossible to change MTU onto the individual LAG members.
If port is deleted from port-channel, it inherits port-channel MTU.
Please, could your check last changes ?

lgtm-com · 2021-10-21T08:39:41Z

This pull request fixes 1 alert when merging f35327586450378678f7633fd2aae05112b299bd into 9017d99 - view on LGTM.com

fixed alerts:

1 for Unused import

Signed-off-by: Mykola Horb <[email protected]>

lgtm-com · 2021-10-22T10:56:31Z

This pull request fixes 1 alert when merging cfb23f6 into 9017d99 - view on LGTM.com

fixed alerts:

1 for Unused import

KonstiantynHalushka · 2021-11-08T09:48:14Z

@judyjoseph Please run this PR again for testing

This reverts commit 6d8387c.

lgtm-com · 2021-11-09T14:04:38Z

This pull request fixes 1 alert when merging e063eca into 63a5257 - view on LGTM.com

fixed alerts:

1 for Unused import

KonstiantynHalushka · 2021-11-09T15:32:43Z

fixed alert about unused import found by bot is not an error. It is designed for unit tests.

pshulik · 2021-11-12T07:02:07Z

@judyjoseph Could you please review the changes

pshulik · 2021-11-12T07:24:44Z

Dear @gechiang and @anamehra. Could you please review the changes

[sonic-swsss] Fix the issue of field "next_hop_ip" not getting updated in state DB in ERSPAN Mirror (sonic-net#1375) [vlanmgr] Support Jumbo Frame By Default (sonic-net#1393) [fec] added logic that put port down before applying fec onfiguration (sonic-net#1399) [fec] Get FEC mode when port is already admin down (sonic-net#1403) Refine getDbId() calling to fix build after swss-common change (sonic-net#1245)

gechiang · 2022-06-01T18:23:30Z

config/main.py

+        # The checks are only for Ethernet interfaces and their aliases
+        if clicommon.get_interface_naming_mode() == "alias":
+            name_from_alias = interface_alias_to_name(config_db, interface_name)
+            if interface_name is None or name_from_alias == interface_name:


With this added check you are requiring that once the device is configured with naming_mode alias it must use the alias name and not the SONiC interface name and it will cause this to fail? Can we preserve the original design/check and not treat the case where the look up resulted to the same name as a failure scenario?

gechiang · 2022-06-01T18:30:05Z

scripts/portconfig

@@ -258,26 +263,30 @@ def main():
    # Load database config files
    load_db_config()
    try:
+        port_table_name = interface.get_port_table_name(args.port)


have you tested this with device set to naming_mode as "alias" and if you passed the alias name instead would this still work? I think we should move the naming mode check to here instead of handled at set_mtu only so that it should be handling the alias case in case naming_mode is set to alias?

TafkaMax · 2024-06-17T12:06:16Z

Why is this not yet merged???

show interfaces portchannel
Flags: A - active, I - inactive, Up - up, Dw - Down, N/A - not available,
       S - selected, D - deselected, * - not synced
  No.  Team Dev       Protocol     Ports
-----  -------------  -----------  -------
   01  PortChannel01  LACP(A)(Dw)
   02  PortChannel02  LACP(A)(Dw)

sudo config interface mtu PortChannel02 1500
Invalid port PortChannel02

 sudo config portchannel member add PortChannel02 Ethernet0
Usage: config portchannel member add [OPTIONS] <portchannel_name> <port_name>
Try "config portchannel member add -h" for help.

Error: Port MTU of Ethernet0 is different than the PortChannel02 MTU size

ghost marked this pull request as ready for review January 21, 2021 16:03

ghost mentioned this pull request Jan 21, 2021

PortChannel mtu change fails with CLI sonic-net/sonic-buildimage#6430

Open

prsunny requested a review from judyjoseph January 29, 2021 01:59

judyjoseph reviewed Feb 18, 2021

View reviewed changes

ghost force-pushed the fix_portconfig_portchannel branch 3 times, most recently from cc1c90a to 9e50825 Compare February 19, 2021 17:19

ghost requested a review from judyjoseph February 22, 2021 15:44

ghost force-pushed the fix_portconfig_portchannel branch from 9e50825 to 3f07964 Compare April 14, 2021 10:28

Maksym Belei added 4 commits October 20, 2021 20:33

[config] Add support of PortChannels to portconfig

46485f9

* 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]>

[portconfig] Refactor portconfig script

ca3c2c7

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

[config][unit tests] Add unit tests for subcommand 'config intf mtu'

1f44432

* Adding unit tests for subcommand 'config intf mtu' including testing of portconfig script. Signed-off-by: Maksym Belei <[email protected]>

NGorb-jabil force-pushed the fix_portconfig_portchannel branch from 3f07964 to f353275 Compare October 21, 2021 08:29

NGorb-jabil force-pushed the fix_portconfig_portchannel branch from f353275 to f696658 Compare October 22, 2021 07:00

[portconfig] Refactor portconfig script after rebase

cfb23f6

Signed-off-by: Mykola Horb <[email protected]>

NGorb-jabil force-pushed the fix_portconfig_portchannel branch from f696658 to cfb23f6 Compare October 22, 2021 10:44

KonstiantynHalushka and others added 3 commits November 9, 2021 11:11

[portconfig] Delete unused import

6d8387c

Merge branch 'master' into fix_portconfig_portchannel

ed55f2c

Revert "[portconfig] Delete unused import"

e063eca

This reverts commit 6d8387c.

gechiang reviewed Jun 1, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[config] Add support of PortChannels to portconfig #1375

[config] Add support of PortChannels to portconfig #1375

ghost commented Jan 21, 2021 •

edited by ghost

Loading

anamehra commented Jan 22, 2021

gechiang commented Feb 12, 2021

ghost commented Feb 15, 2021

gechiang commented Feb 16, 2021

judyjoseph Feb 18, 2021

ghost Feb 19, 2021

judyjoseph Feb 18, 2021

ghost Feb 19, 2021

ghost commented Feb 20, 2021

ghost commented Feb 22, 2021

ghost commented Mar 23, 2021

judyjoseph commented Mar 31, 2021

azure-pipelines bot commented Mar 31, 2021

ghost commented Apr 14, 2021

judyjoseph commented Jun 4, 2021

gechiang commented Jun 4, 2021

NGorb-jabil commented Oct 21, 2021

lgtm-com bot commented Oct 21, 2021

lgtm-com bot commented Oct 22, 2021

KonstiantynHalushka commented Nov 8, 2021

lgtm-com bot commented Nov 9, 2021

KonstiantynHalushka commented Nov 9, 2021

pshulik commented Nov 12, 2021

pshulik commented Nov 12, 2021

gechiang Jun 1, 2022

gechiang Jun 1, 2022

TafkaMax commented Jun 17, 2024 •

edited

Loading

[config] Add support of PortChannels to portconfig #1375

Are you sure you want to change the base?

[config] Add support of PortChannels to portconfig #1375

Conversation

ghost commented Jan 21, 2021 • edited by ghost Loading

anamehra commented Jan 22, 2021

gechiang commented Feb 12, 2021

ghost commented Feb 15, 2021

gechiang commented Feb 16, 2021

judyjoseph Feb 18, 2021

Choose a reason for hiding this comment

ghost Feb 19, 2021

Choose a reason for hiding this comment

judyjoseph Feb 18, 2021

Choose a reason for hiding this comment

ghost Feb 19, 2021

Choose a reason for hiding this comment

ghost commented Feb 20, 2021

ghost commented Feb 22, 2021

ghost commented Mar 23, 2021

judyjoseph commented Mar 31, 2021

azure-pipelines bot commented Mar 31, 2021

ghost commented Apr 14, 2021

judyjoseph commented Jun 4, 2021

gechiang commented Jun 4, 2021

NGorb-jabil commented Oct 21, 2021

lgtm-com bot commented Oct 21, 2021

lgtm-com bot commented Oct 22, 2021

KonstiantynHalushka commented Nov 8, 2021

lgtm-com bot commented Nov 9, 2021

KonstiantynHalushka commented Nov 9, 2021

pshulik commented Nov 12, 2021

pshulik commented Nov 12, 2021

gechiang Jun 1, 2022

Choose a reason for hiding this comment

gechiang Jun 1, 2022

Choose a reason for hiding this comment

TafkaMax commented Jun 17, 2024 • edited Loading

ghost commented Jan 21, 2021 •

edited by ghost

Loading

TafkaMax commented Jun 17, 2024 •

edited

Loading