Do not configure physical attributes on port channels in portconfig (sonic-net#2456)

- What I did
portconfig is a utility designed to configure various attributes on a physical port or a port channel.
It should reject the operation that a user wants to configure an attribute that will be inserted into CONFIG_DB.PORT table on a port channel. Otherwise, it will generate an entry for the port channel, which causes orchagent to be aborted.

Eg. the following commands will cause an error

admin@sonic:~$ sudo config portchannel add PortChannel0001
admin@sonic:~$ sudo config interface speed PortChannel0001 100000

After the fix, configuring physical attributes on a port channel will be rejected

- How I did it

- How to verify it
Unit test

Signed-off-by: Stephen Sun <[email protected]>

Author: stephenxs

scripts/portconfig

@@ -118,6 +118,9 @@ class portconfig(object):
             print(port_tables[port])
 
     def set_speed(self, port, speed):
+        if self.is_lag:
+            raise Exception("Invalid port %s" % (port))
+
         if self.verbose:
             print("Setting speed %s on port %s" % (speed, port))
         supported_speeds_str = self.get_supported_speeds(port) or ''
@@ -129,6 +132,9 @@ class portconfig(object):
         self.db.mod_entry(PORT_TABLE_NAME, port, {PORT_SPEED_CONFIG_FIELD_NAME: speed})
 
     def set_fec(self, port, fec):
+        if self.is_lag:
+            raise Exception("Invalid port %s" % (port))
+
         if self.verbose:
             print("Setting fec %s on port %s" % (fec, port))
         supported_fecs = self.get_supported_fecs(port)
@@ -141,14 +147,17 @@ class portconfig(object):
         self.db.mod_entry(PORT_TABLE_NAME, port, {PORT_FEC_CONFIG_FIELD_NAME: fec})
 
     def set_mtu(self, port, mtu):
-        port_tables = self.db.get_table(PORT_TABLE_NAME)
-        if port not in port_tables:
+        if self.is_lag:
             raise Exception("Invalid port %s" % (port))
+
         if self.verbose:
             print("Setting mtu %s on port %s" % (mtu, port))
         self.db.mod_entry(PORT_TABLE_NAME, port, {PORT_MTU_CONFIG_FIELD_NAME: mtu})
 
     def set_link_training(self, port, mode):
+        if self.is_lag:
+            raise Exception("Invalid port %s" % (port))
+
         if self.verbose:
             print("Setting link-training %s on port %s" % (mode, port))
         lt_modes = ['on', 'off']
@@ -159,20 +168,32 @@ class portconfig(object):
         self.db.mod_entry(PORT_TABLE_NAME, port, {PORT_LINK_TRAINING_CONFIG_FIELD_NAME: mode})
 
     def set_autoneg(self, port, mode):
+        if self.is_lag:
+            raise Exception("Invalid port %s" % (port))
+
         if self.verbose:
             print("Setting autoneg %s on port %s" % (mode, port))
         mode = 'on' if mode == 'enabled' else 'off'
         self.db.mod_entry(PORT_TABLE_NAME, port, {PORT_AUTONEG_CONFIG_FIELD_NAME: mode})
 
     def set_tx_power(self, port, tx_power):
+        if self.is_lag:
+            raise Exception("Invalid port %s" % (port))
+
         print("Setting target Tx output power to %s dBm on port %s" % (tx_power, port))
         self.db.mod_entry(PORT_TABLE_NAME, port, {PORT_XCVR_TX_POWER_FIELD_NAME: tx_power})
 
     def set_laser_freq(self, port, laser_freq):
+        if self.is_lag:
+            raise Exception("Invalid port %s" % (port))
+
         print("Setting laser frequency to %s GHz on port %s" % (laser_freq, port))
         self.db.mod_entry(PORT_TABLE_NAME, port, {PORT_XCVR_LASER_FREQ_FIELD_NAME: laser_freq})
 
     def set_adv_speeds(self, port, adv_speeds):
+        if self.is_lag:
+            raise Exception("Invalid port %s" % (port))
+
         if self.verbose:
             print("Setting adv_speeds %s on port %s" % (adv_speeds, port))
 
@@ -194,6 +215,9 @@ class portconfig(object):
         self.db.mod_entry(PORT_TABLE_NAME, port, {PORT_ADV_SPEEDS_CONFIG_FIELD_NAME: adv_speeds})
 
     def set_interface_type(self, port, interface_type):
+        if self.is_lag:
+            raise Exception("Invalid port %s" % (port))
+
         if self.is_rj45_port:
             print("Setting RJ45 ports' type is not supported")
             exit(1)
@@ -206,6 +230,9 @@ class portconfig(object):
         self.db.mod_entry(PORT_TABLE_NAME, port, {PORT_INTERFACE_TYPE_CONFIG_FIELD_NAME: interface_type})
 
     def set_adv_interface_types(self, port, adv_interface_types):
+        if self.is_lag:
+            raise Exception("Invalid port %s" % (port))
+
         if self.is_rj45_port:
             print("Setting RJ45 ports' advertised types is not supported")
             exit(1)

tests/config_an_test.py

@@ -33,6 +33,9 @@ def test_config_autoneg(self, ctx):
         self.basic_check("autoneg", ["Ethernet0", "disabled"], ctx)
         self.basic_check("autoneg", ["Invalid", "enabled"], ctx, operator.ne)
         self.basic_check("autoneg", ["Ethernet0", "invalid"], ctx, operator.ne)
+        # Setting auto negotiation on a port channel is not supported
+        result = self.basic_check("autoneg", ["PortChannel0001", "enabled"], ctx, operator.ne)
+        assert 'Invalid port PortChannel0001' in result.output
 
     def test_config_speed(self, ctx):
         self.basic_check("speed", ["Ethernet0", "40000"], ctx)
@@ -42,6 +45,9 @@ def test_config_speed(self, ctx):
         assert 'Invalid speed' in result.output
         assert 'Valid speeds:' in result.output
         self.basic_check("speed", ["Ethernet0", "invalid"], ctx, operator.ne)
+        # Setting speed on a port channel is not supported
+        result = self.basic_check("speed", ["PortChannel0001", "100000"], ctx, operator.ne)
+        assert 'Invalid port PortChannel0001' in result.output
 
     def test_config_adv_speeds(self, ctx):
         self.basic_check("advertised-speeds", ["Ethernet0", "40000,100000"], ctx)
@@ -53,6 +59,9 @@ def test_config_adv_speeds(self, ctx):
         result = self.basic_check("advertised-speeds", ["Ethernet0", "50000,50000"], ctx, operator.ne)
         assert 'Invalid speed' in result.output
         assert 'duplicate' in result.output
+        # Setting advertised speeds on a port channel is not supported
+        result = self.basic_check("advertised-speeds", ["PortChannel0001", "40000,100000"], ctx, operator.ne)
+        assert 'Invalid port PortChannel0001' in result.output
 
     def test_config_type(self, ctx):
         self.basic_check("type", ["Ethernet0", "CR4"], ctx)
@@ -64,6 +73,9 @@ def test_config_type(self, ctx):
         assert 'Valid interface types:' in result.output
         result = self.basic_check("type", ["Ethernet16", "Invalid"], ctx, operator.ne)
         assert "Setting RJ45 ports' type is not supported" in result.output
+        # Setting type on a port channel is not supported
+        result = self.basic_check("type", ["PortChannel0001", "CR4"], ctx, operator.ne)
+        assert 'Invalid port PortChannel0001' in result.output
 
     def test_config_adv_types(self, ctx):
         self.basic_check("advertised-types", ["Ethernet0", "CR4,KR4"], ctx)
@@ -78,6 +90,9 @@ def test_config_adv_types(self, ctx):
         self.basic_check("advertised-types", ["Ethernet0", ""], ctx, operator.ne)
         result = self.basic_check("advertised-types", ["Ethernet16", "Invalid"], ctx, operator.ne)
         assert "Setting RJ45 ports' advertised types is not supported" in result.output
+        # Setting advertised types on a port channel is not supported
+        result = self.basic_check("advertised-types", ["PortChannel0001", "CR4,KR4"], ctx, operator.ne)
+        assert 'Invalid port PortChannel0001' in result.output
 
     def test_config_mtu(self, ctx):
         self.basic_check("mtu", ["Ethernet0", "1514"], ctx)
@@ -99,6 +114,9 @@ def test_config_fec(self, ctx):
         # Negative case: set a fec mode on a port where setting fec is not supported
         result = self.basic_check("fec", ["Ethernet112", "test"], ctx, operator.ne)
         assert "Setting fec is not supported" in result.output
+        # Negative case: set a fec mode on a port channel is not supported
+        result = self.basic_check("fec", ["PortChannel0001", "none"], ctx, operator.ne)
+        assert 'Invalid port PortChannel0001' in result.output        
 
     def basic_check(self, command_name, para_list, ctx, op=operator.eq, expect_result=0):
         runner = CliRunner()

tests/config_lt_test.py

@@ -34,6 +34,9 @@ def test_config_link_training(self, ctx):
         self.basic_check("link-training", ["Invalid", "on"], ctx, operator.ne)
         self.basic_check("link-training", ["Invalid", "off"], ctx, operator.ne)
         self.basic_check("link-training", ["Ethernet0", "invalid"], ctx, operator.ne)
+        # Setting link training on a port channel is not supported
+        result = self.basic_check("link-training", ["PortChannel0001", "on"], ctx, operator.ne)
+        assert 'Invalid port PortChannel0001' in result.output
 
     def basic_check(self, command_name, para_list, ctx, op=operator.eq, expect_result=0):
         runner = CliRunner()

tests/config_xcvr_test.py

@@ -34,12 +34,18 @@ def test_config_laser_frequency(self, ctx):
         assert "Setting laser frequency" in result.output
         result = self.basic_check("frequency", ["Ethernet0", "--", "-1"], ctx, op=operator.ne)
         assert "Error: Frequency must be > 0" in result.output
+        # Setting laser frequency on a port channel is not supported
+        result = self.basic_check("frequency", ["PortChannel0001", "191300"], ctx, operator.ne)
+        assert 'Invalid port PortChannel0001' in result.output
 
     def test_config_tx_power(self, ctx):
         result = self.basic_check("tx_power", ["Ethernet0", "11.3"], ctx)
         assert "Setting target Tx output power" in result.output
         result = self.basic_check("tx_power", ["Ethernet0", "11.34"], ctx, op=operator.ne)
         assert "Error: tx power must be with single decimal place" in result.output
+        # Setting tx power on a port channel is not supported
+        result = self.basic_check("tx_power", ["PortChannel0001", "11.3"], ctx, operator.ne)
+        assert 'Invalid port PortChannel0001' in result.output
 
     def basic_check(self, command_name, para_list, ctx, op=operator.eq, expect_result=0):
         runner = CliRunner()