Improve the way to check port type of RJ45 port (sonic-net#2249)

* Update the presence state of RJ45 port

Present/Not present => Link Up/Link Down
Use the new platform API to test whether the port is an RJ45 port

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

* Use new platform API to check whether a port is RJ45 and represent present status accordingly

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

* Adjust sfputil and testcases

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

* Adjust sfpshow

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

* Exact is_rj45_port to a common module shared between sfpshow and intfutil

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

* Fall back to old way for checking RJ45 port

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

* Move RJ45 part to platform_sfputil_helper

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

* Remove fallback mechanism in is_rj45_port

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

* Remove get_child_ports which is not used

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

* Temporarily commit

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

* Update unit test

Signed-off-by: stephens <[email protected]>

* Adjust unit test

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

* Commit missed files

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

* Add missing files

Signed-off-by: stephens <[email protected]>

* Fix typo

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

* Remove code that was committed by mistake.

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

* Fix an issue: the ports should be in nature order in sfputil show presence

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

* Fix present state for RJ45: Link Up/Down => Port Up/Down

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

* LGTM warning supression

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

* LGTM warning supression

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

* Move present state part into another PR

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

* Fix review comments

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

Co-authored-by: stephens <[email protected]>
Conflicts:
	scripts/intfutil
	scripts/sfpshow
	sfputil/main.py
	tests/mock_platform_sfputil/mock_platform_sfputil.py
	tests/sfputil_test.py

Author: 2 people

scripts/intfutil

@@ -14,8 +14,7 @@ try:
         sys.path.insert(0, tests_path)
         import mock_tables.dbconnector
         from mock_platform_sfputil.mock_platform_sfputil import mock_platform_sfputil_helper
-        import utilities_common.platform_sfputil_helper as platform_sfputil_helper
-        mock_platform_sfputil_helper(platform_sfputil_helper)
+        mock_platform_sfputil_helper()
     if os.environ["UTILITIES_UNIT_TESTING_TOPOLOGY"] == "multi_asic":
         import mock_tables.mock_multi_asic
         mock_tables.dbconnector.load_namespace_config()

scripts/sfpshow

@@ -27,8 +27,7 @@ try:
         sys.path.insert(0, test_path)
         import mock_tables.dbconnector
         from mock_platform_sfputil.mock_platform_sfputil import mock_platform_sfputil_helper
-        import utilities_common.platform_sfputil_helper as platform_sfputil_helper
-        mock_platform_sfputil_helper(platform_sfputil_helper)
+        mock_platform_sfputil_helper()
     if os.environ["UTILITIES_UNIT_TESTING_TOPOLOGY"] == "multi_asic":
         import mock_tables.mock_multi_asic
         mock_tables.dbconnector.load_namespace_config()
@@ -433,16 +432,9 @@ class SFPShow(object):
     def convert_interface_sfp_presence_state_to_cli_output_string(self, state_db, interface_name):
         sfp_info_dict = state_db.get_all(self.db.STATE_DB, 'TRANSCEIVER_INFO|{}'.format(interface_name))
         if sfp_info_dict:
-            if sfp_info_dict['type'] == RJ45_PORT_TYPE:
-                output = 'Link Up'
-            else:
-                output = 'Present'
+            output = 'Present'
         else:
-            # We have to check whether it is an RJ45 port via calling platform API in case it is absent
-            if is_rj45_port(interface_name):
-                output = 'Link Down'
-            else:
-                output = 'Not present'
+            output = 'Not present'
         return output
 
 

sfputil/main.py

@@ -292,22 +292,16 @@ def is_sfp_present(port_name):
     return bool(presence)
 
 
-def is_rj45_port_from_api(port_name):
+def is_port_type_rj45(port_name):
     physical_port = logical_port_to_physical_port_index(port_name)
 
     try:
         port_types = platform_chassis.get_port_or_cage_type(physical_port)
         return SfpBase.SFP_PORT_TYPE_BIT_RJ45 == port_types
     except NotImplementedError:
-        port_types = None
+        pass
 
     return False
-
-
-def skip_if_port_is_rj45(port_name): 
-    if is_rj45_port_from_api(port_name): 
-        click.echo("This functionality is not applicable for RJ45 port {}.".format(port_name)) 
-        sys.exit(EXIT_FAIL)
 # ========================== Methods for formatting output ==========================
 
 # Convert dict values to cli output string
@@ -649,7 +643,7 @@ def eeprom(port, dump_dom, namespace):
         for physical_port in physical_port_list:
             port_name = get_physical_port_name(logical_port_name, i, ganged)
 
-            if is_rj45_port_from_api(port_name):
+            if is_port_type_rj45(port_name):
                 output += "{}: SFP EEPROM is not applicable for RJ45 port\n".format(port_name)
                 output += '\n'
                 continue
@@ -715,6 +709,7 @@ def presence(port):
 
         logical_port_list = [port]
 
+    logical_port_list = natsort.natsorted(logical_port_list)
     for logical_port_name in logical_port_list:
         ganged = False
         i = 1
@@ -810,7 +805,7 @@ def fetch_error_status_from_platform_api(port):
         physical_port_list = logical_port_name_to_physical_port_list(logical_port_name)
         port_name = get_physical_port_name(logical_port_name, 1, False)
 
-        if is_rj45_port_from_api(logical_port_name):
+        if is_port_type_rj45(logical_port_name):
             output.append([port_name, "N/A"])
         else:
             output.append([port_name, output_dict.get(physical_port_list[0])])
@@ -836,7 +831,7 @@ def fetch_error_status_from_state_db(port, state_db):
     sorted_ports = natsort.natsorted(status)
     output = []
     for port in sorted_ports:
-        if is_rj45_port_from_api(port):
+        if is_port_type_rj45(port):
             description = "N/A"
         else:
             statestring = status[port].get('status')
@@ -912,7 +907,7 @@ def lpmode(port):
             click.echo("Error: No physical ports found for logical port '{}'".format(logical_port_name))
             return
 
-        if is_rj45_port_from_api(logical_port_name):
+        if is_port_type_rj45(logical_port_name):
             output_table.append([logical_port_name, "N/A"])
         else:
             if len(physical_port_list) > 1:
@@ -955,7 +950,7 @@ def fwversion(port_name):
     physical_port = logical_port_to_physical_port_index(port_name)
     sfp = platform_chassis.get_sfp(physical_port)
 
-    if is_rj45_port_from_api(port_name):
+    if is_port_type_rj45(port_name):
         click.echo("Show firmware version is not applicable for RJ45 port {}.".format(port_name))
         sys.exit(EXIT_FAIL)
 
@@ -994,7 +989,7 @@ def set_lpmode(logical_port, enable):
         click.echo("Error: No physical ports found for logical port '{}'".format(logical_port))
         return
 
-    if is_rj45_port_from_api(logical_port):
+    if is_port_type_rj45(logical_port):
         click.echo("{} low-power mode is not applicable for RJ45 port {}.".format("Enabling" if enable else "Disabling", logical_port))
         sys.exit(EXIT_FAIL)
 
@@ -1054,7 +1049,7 @@ def reset(port_name):
         click.echo("Error: No physical ports found for logical port '{}'".format(port_name))
         return
 
-    if is_rj45_port_from_api(port_name):
+    if is_port_type_rj45(port_name):
         click.echo("Reset is not applicable for RJ45 port {}.".format(port_name))
         sys.exit(EXIT_FAIL)
 
@@ -1219,7 +1214,9 @@ def download_firmware(port_name, filepath):
 def run(port_name, mode):
     """Run the firmware with default mode=1"""
 
-    skip_if_port_is_rj45(port_name)
+    if is_port_type_rj45(port_name): 
+        click.echo("This functionality is not applicable for RJ45 port {}.".format(port_name)) 
+        sys.exit(EXIT_FAIL)
 
     if not is_sfp_present(port_name):
         click.echo("{}: SFP EEPROM not detected\n".format(port_name))
@@ -1238,7 +1235,9 @@ def run(port_name, mode):
 def commit(port_name):
     """Commit the running firmware"""
 
-    skip_if_port_is_rj45(port_name)
+    if is_port_type_rj45(port_name): 
+        click.echo("This functionality is not applicable for RJ45 port {}.".format(port_name)) 
+        sys.exit(EXIT_FAIL)
 
     if not is_sfp_present(port_name):
         click.echo("{}: SFP EEPROM not detected\n".format(port_name))
@@ -1260,7 +1259,9 @@ def upgrade(port_name, filepath):
 
     physical_port = logical_port_to_physical_port_index(port_name)
 
-    skip_if_port_is_rj45(port_name)
+    if is_port_type_rj45(port_name): 
+        click.echo("This functionality is not applicable for RJ45 port {}.".format(port_name)) 
+        sys.exit(EXIT_FAIL)
 
     if not is_sfp_present(port_name):
         click.echo("{}: SFP EEPROM not detected\n".format(port_name))
@@ -1296,7 +1297,9 @@ def upgrade(port_name, filepath):
 def download(port_name, filepath):
     """Download firmware on the transceiver"""
 
-    skip_if_port_is_rj45(port_name)
+    if is_port_type_rj45(port_name): 
+        click.echo("This functionality is not applicable for RJ45 port {}.".format(port_name)) 
+        sys.exit(EXIT_FAIL)
 
     if not is_sfp_present(port_name):
        click.echo("{}: SFP EEPROM not detected\n".format(port_name))
@@ -1322,7 +1325,9 @@ def unlock(port_name, password):
     physical_port = logical_port_to_physical_port_index(port_name)
     sfp = platform_chassis.get_sfp(physical_port)
 
-    skip_if_port_is_rj45(port_name)
+    if is_port_type_rj45(port_name): 
+        click.echo("This functionality is not applicable for RJ45 port {}.".format(port_name)) 
+        sys.exit(EXIT_FAIL)
 
     if not is_sfp_present(port_name):
        click.echo("{}: SFP EEPROM not detected\n".format(port_name))

tests/mock_platform_sfputil/mock_platform_sfputil.py

@@ -3,6 +3,7 @@
 from sonic_platform_base.platform_base import PlatformBase
 from sonic_platform_base.chassis_base import ChassisBase
 from sonic_platform_base.sfp_base import SfpBase
+import utilities_common.platform_sfputil_helper as platform_sfputil_helper
 
 portMap = None
 RJ45Ports = None
@@ -32,7 +33,7 @@ def mock_platform_sfputil_read_porttab_mappings():
         portMap = jsonobj['portMap']
         RJ45Ports = jsonobj['RJ45Ports']
 
-def mock_platform_sfputil_helper(platform_sfputil_helper):
+def mock_platform_sfputil_helper():
     platform_sfputil_helper.platform_chassis = mock_Chassis()
     platform_sfputil_helper.platform_sfputil = True
     platform_sfputil_helper.platform_porttab_mapping_read = False

tests/sfp_test.py

@@ -372,6 +372,14 @@ def test_sfp_presence(self):
         assert result.exit_code == 0
         assert result.output == expected
 
+        result = runner.invoke(show.cli.commands["interfaces"].commands["transceiver"].commands["presence"], ["Ethernet29"])
+        expected = """Port        Presence
+----------  -----------
+Ethernet29  Not present
+"""
+        assert result.exit_code == 0
+        assert result.output == expected
+
         result = runner.invoke(show.cli.commands["interfaces"].commands["transceiver"].commands["presence"], ["Ethernet36"])
         expected = """Port        Presence
 ----------  ----------

tests/sfputil_test.py

@@ -268,7 +268,7 @@ def test_version(self):
         result = runner.invoke(sfputil.cli.commands['version'], [])
         assert result.output.rstrip() == 'sfputil version {}'.format(sfputil.VERSION)
 
-    @patch('sfputil.main.is_rj45_port_from_api', MagicMock(return_value=False))
+    @patch('sfputil.main.is_port_type_rj45', MagicMock(return_value=False))
     def test_error_status_from_db(self):
         db = Db()
         expected_output = [['Ethernet0', 'Blocking Error|High temperature'],
@@ -285,7 +285,7 @@ def test_error_status_from_db(self):
         output = sfputil.fetch_error_status_from_state_db('Ethernet0', db.db)
         assert output == expected_output_ethernet0
 
-    @patch('sfputil.main.is_rj45_port_from_api', MagicMock(return_value=True))
+    @patch('sfputil.main.is_port_type_rj45', MagicMock(return_value=True))
     def test_error_status_from_db_RJ45(self):
         db = Db()
         expected_output = [['Ethernet0', 'N/A'],
@@ -304,7 +304,7 @@ def test_error_status_from_db_RJ45(self):
 
     @patch('sfputil.main.logical_port_name_to_physical_port_list', MagicMock(return_value=[1]))
     @patch('sfputil.main.logical_port_to_physical_port_index', MagicMock(return_value=1))
-    @patch('sfputil.main.is_rj45_port_from_api', MagicMock(return_value=False))
+    @patch('sfputil.main.is_port_type_rj45', MagicMock(return_value=False))
     @patch('subprocess.check_output', MagicMock(return_value="['0:OK']"))
     def test_fetch_error_status_from_platform_api(self):
         output = sfputil.fetch_error_status_from_platform_api('Ethernet0')
@@ -313,7 +313,7 @@ def test_fetch_error_status_from_platform_api(self):
     @patch('sfputil.main.logical_port_name_to_physical_port_list', MagicMock(return_value=[1]))
     @patch('sfputil.main.logical_port_to_physical_port_index', MagicMock(return_value=1))
     @patch('subprocess.check_output', MagicMock(return_value="['0:OK']"))
-    @patch('sfputil.main.is_rj45_port_from_api', MagicMock(return_value=True))
+    @patch('sfputil.main.is_port_type_rj45', MagicMock(return_value=True))
     def test_fetch_error_status_from_platform_api_RJ45(self):
         output = sfputil.fetch_error_status_from_platform_api('Ethernet0')
         assert output == [['Ethernet0', 'N/A']]
@@ -407,7 +407,7 @@ def test_show_lpmode(self, mock_chassis):
     @patch('sfputil.main.logical_port_to_physical_port_index', MagicMock(return_value=1))
     @patch('sfputil.main.logical_port_name_to_physical_port_list', MagicMock(return_value=[1]))
     @patch('sfputil.main.platform_sfputil', MagicMock(is_logical_port=MagicMock(return_value=1)))
-    @patch('sfputil.main.is_rj45_port_from_api', MagicMock(return_value=True))
+    @patch('sfputil.main.is_port_type_rj45', MagicMock(return_value=True))
     def test_show_eeprom_RJ45(self, mock_chassis):
         mock_sfp = MagicMock()
         mock_api = MagicMock()
@@ -418,14 +418,8 @@ def test_show_eeprom_RJ45(self, mock_chassis):
         expected_output = "Ethernet16: SFP EEPROM is not applicable for RJ45 port\n\n\n"
         assert result.output == expected_output
 
-    @patch('sfputil.main.is_rj45_port_from_api', MagicMock(return_value=True))
-    @patch('sys.exit', MagicMock(return_value=EXIT_FAIL))
-    def test_skip_if_port_is_rj45(self):
-        result = sfputil.skip_if_port_is_rj45('Ethernet0')
-        assert result == None
-
     @patch('sfputil.main.logical_port_name_to_physical_port_list', MagicMock(return_value=1))
-    @patch('sfputil.main.is_rj45_port_from_api', MagicMock(return_value=True))
+    @patch('sfputil.main.is_port_type_rj45', MagicMock(return_value=True))
     @patch('sfputil.main.platform_sfputil', MagicMock(is_logical_port=MagicMock(return_value=1)))
     def test_lpmode_set(self):
         runner = CliRunner()
@@ -434,7 +428,7 @@ def test_lpmode_set(self):
         assert result.exit_code == EXIT_FAIL
 
     @patch('sfputil.main.logical_port_name_to_physical_port_list', MagicMock(return_value=1))
-    @patch('sfputil.main.is_rj45_port_from_api', MagicMock(return_value=True))
+    @patch('sfputil.main.is_port_type_rj45', MagicMock(return_value=True))
     @patch('sfputil.main.platform_sfputil', MagicMock(is_logical_port=MagicMock(return_value=1)))
     def test_reset_RJ45(self):
         runner = CliRunner()
@@ -457,7 +451,7 @@ def test_unlock_firmware(self, mock_chassis):
 
     @patch('sfputil.main.platform_chassis')
     @patch('sfputil.main.logical_port_to_physical_port_index', MagicMock(return_value=1))
-    @patch('sfputil.main.is_rj45_port_from_api', MagicMock(return_value=True))
+    @patch('sfputil.main.is_port_type_rj45', MagicMock(return_value=True))
     def test_show_fwversion_Rj45(self, mock_chassis):
         mock_sfp = MagicMock()
         mock_api = MagicMock()
@@ -494,23 +488,23 @@ def test_commit_firmwre(self, mock_chassis):
         assert status == 1
 
     @patch('sfputil.main.is_sfp_present', MagicMock(return_value=True))
-    @patch('sfputil.main.is_rj45_port_from_api', MagicMock(return_value=True))
+    @patch('sfputil.main.is_port_type_rj45', MagicMock(return_value=True))
     def test_firmware_run_RJ45(self):
         runner = CliRunner()
         result = runner.invoke(sfputil.cli.commands['firmware'].commands['run'], ["--mode", "0", "Ethernet0"])
         assert result.output == 'This functionality is not applicable for RJ45 port Ethernet0.\n'
         assert result.exit_code == EXIT_FAIL
 
     @patch('sfputil.main.is_sfp_present', MagicMock(return_value=True))
-    @patch('sfputil.main.is_rj45_port_from_api', MagicMock(return_value=True))
+    @patch('sfputil.main.is_port_type_rj45', MagicMock(return_value=True))
     def test_firmware_commit_RJ45(self):
         runner = CliRunner()
         result = runner.invoke(sfputil.cli.commands['firmware'].commands['commit'], ["Ethernet0"])
         assert result.output == 'This functionality is not applicable for RJ45 port Ethernet0.\n'
         assert result.exit_code == EXIT_FAIL
 
     @patch('sfputil.main.logical_port_to_physical_port_index', MagicMock(return_value=1))
-    @patch('sfputil.main.is_rj45_port_from_api', MagicMock(return_value=True))
+    @patch('sfputil.main.is_port_type_rj45', MagicMock(return_value=True))
     @patch('sfputil.main.is_sfp_present', MagicMock(return_value=1))
     def test_firmware_upgrade_RJ45(self):
         runner = CliRunner()
@@ -519,7 +513,7 @@ def test_firmware_upgrade_RJ45(self):
         assert result.exit_code == EXIT_FAIL
 
     @patch('sfputil.main.logical_port_to_physical_port_index', MagicMock(return_value=1))
-    @patch('sfputil.main.is_rj45_port_from_api', MagicMock(return_value=True))
+    @patch('sfputil.main.is_port_type_rj45', MagicMock(return_value=True))
     @patch('sfputil.main.is_sfp_present', MagicMock(return_value=1))
     def test_firmware_download_RJ45(self):
         runner = CliRunner()