Skip to content

Commit

Permalink
Improve the way to check port type of RJ45 port (sonic-net#2249)
Browse files Browse the repository at this point in the history 
* 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 <stephens@nvidia.com>

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

Signed-off-by: Stephen Sun <stephens@nvidia.com>

* Adjust sfputil and testcases

Signed-off-by: Stephen Sun <stephens@nvidia.com>

* Adjust sfpshow

Signed-off-by: Stephen Sun <stephens@nvidia.com>

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

Signed-off-by: Stephen Sun <stephens@nvidia.com>

* Fall back to old way for checking RJ45 port

Signed-off-by: Stephen Sun <stephens@nvidia.com>

* Move RJ45 part to platform_sfputil_helper

Signed-off-by: Stephen Sun <stephens@nvidia.com>

* Remove fallback mechanism in is_rj45_port

Signed-off-by: Stephen Sun <stephens@nvidia.com>

* Remove get_child_ports which is not used

Signed-off-by: Stephen Sun <stephens@nvidia.com>

* Temporarily commit

Signed-off-by: Stephen Sun <stephens@nvidia.com>

* Update unit test

Signed-off-by: stephens <stephens@contoso.com>

* Adjust unit test

Signed-off-by: Stephen Sun <stephens@nvidia.com>

* Commit missed files

Signed-off-by: Stephen Sun <stephens@nvidia.com>

* Add missing files

Signed-off-by: stephens <stephens@contoso.com>

* Fix typo

Signed-off-by: Stephen Sun <stephens@nvidia.com>

* Remove code that was committed by mistake.

Signed-off-by: Stephen Sun <stephens@nvidia.com>

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

Signed-off-by: Stephen Sun <stephens@nvidia.com>

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

Signed-off-by: Stephen Sun <stephens@nvidia.com>

* LGTM warning supression

Signed-off-by: Stephen Sun <stephens@nvidia.com>

* LGTM warning supression

Signed-off-by: Stephen Sun <stephens@nvidia.com>

* Move present state part into another PR

Signed-off-by: Stephen Sun <stephens@nvidia.com>

* Fix review comments

Signed-off-by: Stephen Sun <stephens@nvidia.com>

Co-authored-by: stephens <stephens@contoso.com>
Conflicts:
	scripts/intfutil
	scripts/sfpshow
	sfputil/main.py
	tests/mock_platform_sfputil/mock_platform_sfputil.py
	tests/sfputil_test.py
  • Loading branch information
@stephenxs @keboliu
stephenxs authored and keboliu committed Aug 1, 2022
1 parent a3e8f5e commit f0a3e5b
Show file tree
Hide file tree
Showing 6 changed files with 51 additions and 52 deletions.
3 changes: 1 addition & 2 deletions scripts/intfutil
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
14 changes: 3 additions & 11 deletions scripts/sfpshow
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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


Expand Down
45 changes: 25 additions & 20 deletions sfputil/main.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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])])
Expand All @@ -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')
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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))
Expand All @@ -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))
Expand All @@ -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))
Expand Down Expand Up @@ -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))
Expand All @@ -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))
Expand Down
3 changes: 2 additions & 1 deletion tests/mock_platform_sfputil/mock_platform_sfputil.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
8 changes: 8 additions & 0 deletions tests/sfp_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
---------- ----------
Expand Down
30 changes: 12 additions & 18 deletions tests/sfputil_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'],
Expand All @@ -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'],
Expand All @@ -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')
Expand All @@ -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']]
Expand Down Expand Up @@ -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()
Expand All @@ -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()
Expand All @@ -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()
Expand All @@ -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()
Expand Down Expand Up @@ -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()
Expand All @@ -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()
Expand Down

0 comments on commit f0a3e5b

Please sign in to comment.