From 31345576d922b5e17ff139abc0778792a226a954 Mon Sep 17 00:00:00 2001 From: Clive Ni Date: Thu, 1 Sep 2022 18:18:32 +0800 Subject: [PATCH 1/6] [sfputil] Firmware download/upgrade CLI support for QSFP-DD (#1947) - Description Checking that the running image is switched or not after CDB_run during firmware upgrade process. - Motivation and Context CDB_run will maybe cause several seconds NACK or stretching on i2c bus which depend on the implementation of module vendor, checking the status after CDB_run for compatible with different implementation. --- sfputil/main.py | 52 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/sfputil/main.py b/sfputil/main.py index 96653bb622..efa47afcf8 100644 --- a/sfputil/main.py +++ b/sfputil/main.py @@ -1112,6 +1112,51 @@ def run_firmware(port_name, mode): return status +def is_fw_switch_done(port_name): + """ + Make sure the run_firmware cmd is done + @port_name: + Returns 1 on success, and exit_code = -1 on failure + """ + status = 0 + physical_port = logical_port_to_physical_port_index(port_name) + sfp = platform_chassis.get_sfp(physical_port) + + try: + api = sfp.get_xcvr_api() + except NotImplementedError: + click.echo("This functionality is currently not implemented for this platform") + sys.exit(ERROR_NOT_IMPLEMENTED) + + try: + MAX_WAIT = 600 # 60s timeout. + fw_info = api.get_module_fw_info() + cnt = 0 + is_busy = 1 if (fw_info['status'] == False) and (fw_info['result'] is not None) else 0 + while is_busy and cnt < MAX_WAIT: + time.sleep(0.1) + fw_info = api.get_module_fw_info() + is_busy = 1 if (fw_info['status'] == False) and (fw_info['result'] is not None) else 0 + cnt += 1 + + if fw_info['status'] == True: + (ImageA, ImageARunning, ImageACommitted, ImageAValid, + ImageB, ImageBRunning, ImageBCommitted, ImageBValid) = fw_info['result'] + + if (ImageARunning == 1) and (ImageACommitted == 0): # ImageA is running, but not committed. + status = 1 # run_firmware is done. + elif (ImageBRunning == 1) and (ImageBCommitted == 0): # ImageB is running, but not committed. + status = 1 # run_firmware is done. + else: # No image is running, or running and committed image is same. + status = -1 # Failure for Switching images. + else: + status = -1 # Timeout or check code error or CDB not supported. + + except NotImplementedError: + click.echo("This functionality is not applicable for this transceiver") + + return status + def commit_firmware(port_name): status = 0 physical_port = logical_port_to_physical_port_index(port_name) @@ -1280,6 +1325,13 @@ def upgrade(port_name, filepath): click.echo("Firmware run in mode 1 successful") + status = is_fw_switch_done(port_name) + if status != 1: + click.echo('Failed to switch firmware images!') + sys.exit(EXIT_FAIL) + + click.echo("Firmware images switch successful") + status = commit_firmware(port_name) if status != 1: click.echo('Failed to commit firmware! CDB status: {}'.format(status)) From 716fa379bc102593cba32d22fbff0d603135f88c Mon Sep 17 00:00:00 2001 From: Clive Ni Date: Wed, 7 Sep 2022 19:25:28 +0800 Subject: [PATCH 2/6] Update unit tests for sfputil. Test : Creating "is_fw_switch_done" test, this function expected to return 1 when 'status' == True and running image('result'[1, 5]) different with committed('result'[2, 6]) one, otherwise return -1. --- tests/sfputil_test.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/sfputil_test.py b/tests/sfputil_test.py index 1231ba67d7..de43964bed 100644 --- a/tests/sfputil_test.py +++ b/tests/sfputil_test.py @@ -475,6 +475,29 @@ def test_run_firmwre(self, mock_chassis): status = sfputil.run_firmware("Ethernet0", 1) assert status == 1 + @patch('sfputil.main.platform_chassis') + @patch('sfputil.main.logical_port_to_physical_port_index', MagicMock(return_value=1)) + @pytest.mark.parametrize("mock_response, expected", [ + ({'status': False, 'info': "CDB Not supported", 'result': None}, -1), + ({'status': False, 'info': "Reply payload check code error\n", 'result': None} , -1), + ({'status': True, 'info': "", 'result': ("1.0.1", 1, 1, 0, "1.0.2", 0, 0, 0)} , -1), + ({'status': True, 'info': "", 'result': ("1.0.1", 0, 0, 0, "1.0.2", 1, 1, 0)} , -1), + ({'status': True, 'info': "", 'result': ("1.0.1", 1, 0, 0, "1.0.2", 0, 1, 0)} , 1), + ({'status': True, 'info': "", 'result': ("1.0.1", 0, 1, 0, "1.0.2", 1, 0, 0)} , 1), + + # "is_fw_switch_done" function will waiting until timeout under below condition, so that this test will spend around 1min. + # ({'status': False, 'info': "Interface fail", 'result': 0} , -1), + ]) + def test_is_fw_switch_done(self, mock_chassis, mock_response, expected): + mock_sfp = MagicMock() + mock_api = MagicMock() + mock_sfp.get_xcvr_api = MagicMock(return_value=mock_api) + mock_sfp.get_presence.return_value = True + mock_chassis.get_sfp = MagicMock(return_value=mock_sfp) + mock_api.get_module_fw_info.return_value = mock_response + status = sfputil.is_fw_switch_done("Ethernet0") + assert status == expected + @patch('sfputil.main.platform_chassis') @patch('sfputil.main.logical_port_to_physical_port_index', MagicMock(return_value=1)) def test_commit_firmwre(self, mock_chassis): From 8f914004438befb446ed6acfa6bbdc84b59e202c Mon Sep 17 00:00:00 2001 From: Clive Ni Date: Tue, 20 Sep 2022 18:49:30 +0800 Subject: [PATCH 3/6] [sfputil] Firmware download/upgrade CLI support for QSFP-DD (#1947) - Description Adding error judgements in "is_fw_switch_done" function. Update unit tests for "is_fw_switch_done". - Motivation and Context Checking status of images to avoid committing image with a wrong status. --- sfputil/main.py | 22 +++++++++++++++++----- tests/sfputil_test.py | 18 +++++++++++------- 2 files changed, 28 insertions(+), 12 deletions(-) diff --git a/sfputil/main.py b/sfputil/main.py index efa47afcf8..3770a6715f 100644 --- a/sfputil/main.py +++ b/sfputil/main.py @@ -1143,13 +1143,26 @@ def is_fw_switch_done(port_name): (ImageA, ImageARunning, ImageACommitted, ImageAValid, ImageB, ImageBRunning, ImageBCommitted, ImageBValid) = fw_info['result'] - if (ImageARunning == 1) and (ImageACommitted == 0): # ImageA is running, but not committed. + if (ImageARunning == 1) and (ImageBRunning == 1): # Both imageA and B is running. + click.echo("FW info error : Both imageA and B show running!") + status = -1 # Abnormal status. + elif (ImageACommitted == 1) and (ImageBCommitted == 1): # Both imageA and B is committed. + click.echo("FW info error : Both imageA and B show committed!") + status = -1 # Abnormal status. + elif (ImageARunning == 1) and (ImageAValid == 1): # ImageA is running, but also invalid. + click.echo("FW info error : ImageA shows running, but also shows invalid!") + status = -1 # Abnormal status. + elif (ImageBRunning == 1) and (ImageBValid == 1): # ImageB is running, but also invalid. + click.echo("FW info error : ImageB shows running, but also shows invalid!") + status = -1 # Abnormal status. + elif (ImageARunning == 1) and (ImageACommitted == 0): # ImageA is running, but not committed. status = 1 # run_firmware is done. - elif (ImageBRunning == 1) and (ImageBCommitted == 0): # ImageB is running, but not committed. + elif (ImageBRunning == 1) and (ImageBCommitted == 0): # ImageB is running, but not committed. status = 1 # run_firmware is done. - else: # No image is running, or running and committed image is same. + else: # No image is running, or running and committed image is same. status = -1 # Failure for Switching images. else: + click.echo("FW switch : Timeout!") status = -1 # Timeout or check code error or CDB not supported. except NotImplementedError: @@ -1325,8 +1338,7 @@ def upgrade(port_name, filepath): click.echo("Firmware run in mode 1 successful") - status = is_fw_switch_done(port_name) - if status != 1: + if status != is_fw_switch_done(port_name): click.echo('Failed to switch firmware images!') sys.exit(EXIT_FAIL) diff --git a/tests/sfputil_test.py b/tests/sfputil_test.py index de43964bed..37b9213c47 100644 --- a/tests/sfputil_test.py +++ b/tests/sfputil_test.py @@ -478,15 +478,19 @@ def test_run_firmwre(self, mock_chassis): @patch('sfputil.main.platform_chassis') @patch('sfputil.main.logical_port_to_physical_port_index', MagicMock(return_value=1)) @pytest.mark.parametrize("mock_response, expected", [ - ({'status': False, 'info': "CDB Not supported", 'result': None}, -1), - ({'status': False, 'info': "Reply payload check code error\n", 'result': None} , -1), - ({'status': True, 'info': "", 'result': ("1.0.1", 1, 1, 0, "1.0.2", 0, 0, 0)} , -1), - ({'status': True, 'info': "", 'result': ("1.0.1", 0, 0, 0, "1.0.2", 1, 1, 0)} , -1), - ({'status': True, 'info': "", 'result': ("1.0.1", 1, 0, 0, "1.0.2", 0, 1, 0)} , 1), - ({'status': True, 'info': "", 'result': ("1.0.1", 0, 1, 0, "1.0.2", 1, 0, 0)} , 1), + ({'status': False, 'result': None} , -1), + ({'status': False, 'result': None} , -1), + ({'status': True, 'result': ("1.0.1", 1, 1, 0, "1.0.2", 0, 0, 0)} , -1), + ({'status': True, 'result': ("1.0.1", 0, 0, 0, "1.0.2", 1, 1, 0)} , -1), + ({'status': True, 'result': ("1.0.1", 1, 0, 0, "1.0.2", 0, 1, 0)} , 1), + ({'status': True, 'result': ("1.0.1", 0, 1, 0, "1.0.2", 1, 0, 0)} , 1), + ({'status': True, 'result': ("1.0.1", 1, 0, 0, "1.0.2", 1, 1, 0)} , -1), + ({'status': True, 'result': ("1.0.1", 0, 1, 0, "1.0.2", 1, 1, 0)} , -1), + ({'status': True, 'result': ("1.0.1", 1, 0, 1, "1.0.2", 0, 1, 0)} , -1), + ({'status': True, 'result': ("1.0.1", 0, 1, 0, "1.0.2", 1, 0, 1)} , -1), # "is_fw_switch_done" function will waiting until timeout under below condition, so that this test will spend around 1min. - # ({'status': False, 'info': "Interface fail", 'result': 0} , -1), + ({'status': False, 'result': 0} , -1), ]) def test_is_fw_switch_done(self, mock_chassis, mock_response, expected): mock_sfp = MagicMock() From b1e41e816a72ed23c958b6f8e36f2eb2189d44b4 Mon Sep 17 00:00:00 2001 From: Clive Ni Date: Mon, 3 Oct 2022 18:52:49 +0800 Subject: [PATCH 4/6] [sfputil] Firmware download/upgrade CLI support for QSFP-DD (#1947) Fixing : Comparing error code with a wrong variable. Refactor : Renaming variables for more suitable its purpose. Refactor : Removing if case which is low correlation with function. Feat : Adding "echo" to display detail result. --- sfputil/main.py | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/sfputil/main.py b/sfputil/main.py index 3770a6715f..1246614313 100644 --- a/sfputil/main.py +++ b/sfputil/main.py @@ -1140,26 +1140,23 @@ def is_fw_switch_done(port_name): cnt += 1 if fw_info['status'] == True: - (ImageA, ImageARunning, ImageACommitted, ImageAValid, - ImageB, ImageBRunning, ImageBCommitted, ImageBValid) = fw_info['result'] + (ImageA, ImageARunning, ImageACommitted, ImageAInvalid, + ImageB, ImageBRunning, ImageBCommitted, ImageBInvalid) = fw_info['result'] - if (ImageARunning == 1) and (ImageBRunning == 1): # Both imageA and B is running. - click.echo("FW info error : Both imageA and B show running!") - status = -1 # Abnormal status. - elif (ImageACommitted == 1) and (ImageBCommitted == 1): # Both imageA and B is committed. - click.echo("FW info error : Both imageA and B show committed!") - status = -1 # Abnormal status. - elif (ImageARunning == 1) and (ImageAValid == 1): # ImageA is running, but also invalid. + if (ImageARunning == 1) and (ImageAInvalid == 1): # ImageA is running, but also invalid. click.echo("FW info error : ImageA shows running, but also shows invalid!") status = -1 # Abnormal status. - elif (ImageBRunning == 1) and (ImageBValid == 1): # ImageB is running, but also invalid. + elif (ImageBRunning == 1) and (ImageBInvalid == 1): # ImageB is running, but also invalid. click.echo("FW info error : ImageB shows running, but also shows invalid!") status = -1 # Abnormal status. elif (ImageARunning == 1) and (ImageACommitted == 0): # ImageA is running, but not committed. + click.echo("FW images switch successful : ImageA is running") status = 1 # run_firmware is done. elif (ImageBRunning == 1) and (ImageBCommitted == 0): # ImageB is running, but not committed. + click.echo("FW images switch successful : ImageB is running") status = 1 # run_firmware is done. else: # No image is running, or running and committed image is same. + click.echo("FW info error : Failed to switch into uncommitted image!") status = -1 # Failure for Switching images. else: click.echo("FW switch : Timeout!") @@ -1338,12 +1335,10 @@ def upgrade(port_name, filepath): click.echo("Firmware run in mode 1 successful") - if status != is_fw_switch_done(port_name): + if is_fw_switch_done(port_name) != 1: click.echo('Failed to switch firmware images!') sys.exit(EXIT_FAIL) - click.echo("Firmware images switch successful") - status = commit_firmware(port_name) if status != 1: click.echo('Failed to commit firmware! CDB status: {}'.format(status)) From 88c9af4e8b6baddd1ab8c6de582719c7632bf6ce Mon Sep 17 00:00:00 2001 From: Clive Ni Date: Mon, 3 Oct 2022 19:16:06 +0800 Subject: [PATCH 5/6] Update unit tests for sfputil. --- tests/sfputil_test.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/sfputil_test.py b/tests/sfputil_test.py index 37b9213c47..e11bd3f162 100644 --- a/tests/sfputil_test.py +++ b/tests/sfputil_test.py @@ -484,8 +484,6 @@ def test_run_firmwre(self, mock_chassis): ({'status': True, 'result': ("1.0.1", 0, 0, 0, "1.0.2", 1, 1, 0)} , -1), ({'status': True, 'result': ("1.0.1", 1, 0, 0, "1.0.2", 0, 1, 0)} , 1), ({'status': True, 'result': ("1.0.1", 0, 1, 0, "1.0.2", 1, 0, 0)} , 1), - ({'status': True, 'result': ("1.0.1", 1, 0, 0, "1.0.2", 1, 1, 0)} , -1), - ({'status': True, 'result': ("1.0.1", 0, 1, 0, "1.0.2", 1, 1, 0)} , -1), ({'status': True, 'result': ("1.0.1", 1, 0, 1, "1.0.2", 0, 1, 0)} , -1), ({'status': True, 'result': ("1.0.1", 0, 1, 0, "1.0.2", 1, 0, 1)} , -1), From 1c9c0f4ad241f9eeeca994aab320eeb15f3b613a Mon Sep 17 00:00:00 2001 From: Clive Ni Date: Sat, 7 Jan 2023 14:47:47 +0800 Subject: [PATCH 6/6] [sfputil] Firmware download/upgrade CLI support for QSFP-DD (#1947) Feat : Reducing frequency of check during "is_fw_switch_done". Refactor : Removing a repeated line. --- sfputil/main.py | 12 +++++------- tests/sfputil_test.py | 1 - 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/sfputil/main.py b/sfputil/main.py index 1246614313..71c0e7fda8 100644 --- a/sfputil/main.py +++ b/sfputil/main.py @@ -1129,15 +1129,13 @@ def is_fw_switch_done(port_name): sys.exit(ERROR_NOT_IMPLEMENTED) try: - MAX_WAIT = 600 # 60s timeout. - fw_info = api.get_module_fw_info() - cnt = 0 - is_busy = 1 if (fw_info['status'] == False) and (fw_info['result'] is not None) else 0 - while is_busy and cnt < MAX_WAIT: - time.sleep(0.1) + MAX_WAIT = 60 # 60s timeout. + is_busy = 1 # Initial to 1 for entering while loop at least one time. + timeout_time = time.time() + MAX_WAIT + while is_busy and (time.time() < timeout_time): fw_info = api.get_module_fw_info() is_busy = 1 if (fw_info['status'] == False) and (fw_info['result'] is not None) else 0 - cnt += 1 + time.sleep(2) if fw_info['status'] == True: (ImageA, ImageARunning, ImageACommitted, ImageAInvalid, diff --git a/tests/sfputil_test.py b/tests/sfputil_test.py index e11bd3f162..2d87efa2e8 100644 --- a/tests/sfputil_test.py +++ b/tests/sfputil_test.py @@ -478,7 +478,6 @@ def test_run_firmwre(self, mock_chassis): @patch('sfputil.main.platform_chassis') @patch('sfputil.main.logical_port_to_physical_port_index', MagicMock(return_value=1)) @pytest.mark.parametrize("mock_response, expected", [ - ({'status': False, 'result': None} , -1), ({'status': False, 'result': None} , -1), ({'status': True, 'result': ("1.0.1", 1, 1, 0, "1.0.2", 0, 0, 0)} , -1), ({'status': True, 'result': ("1.0.1", 0, 0, 0, "1.0.2", 1, 1, 0)} , -1),