Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[sfputil] Firmware download/upgrade CLI support for QSFP-DD (#1947) #2349

Merged
merged 6 commits into from
Jan 9, 2023
Merged

[sfputil] Firmware download/upgrade CLI support for QSFP-DD (#1947) #2349

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3134557
[sfputil] Firmware download/upgrade CLI support for QSFP-DD (#1947)
CliveNi Sep 1, 2022
716fa37
Update unit tests for sfputil.
CliveNi Sep 7, 2022
8f91400
[sfputil] Firmware download/upgrade CLI support for QSFP-DD (#1947)
CliveNi Sep 20, 2022
b1e41e8
[sfputil] Firmware download/upgrade CLI support for QSFP-DD (#1947)
CliveNi Oct 3, 2022
88c9af4
Update unit tests for sfputil.
CliveNi Oct 3, 2022
1c9c0f4
[sfputil] Firmware download/upgrade CLI support for QSFP-DD (#1947)
CliveNi Jan 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions sfputil/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1112,6 +1112,59 @@ def run_firmware(port_name, mode):

return status

def is_fw_switch_done(port_name):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure why this API is required because there is already a return status from run_firmware() to indicate a failure. An imageA could be running while the committed Image could be Image B. For eg.

  1. Image A is running and its is the committed image
  2. User updates Image B
  3. Runs the image B

Now the running image is B but the committed image is A

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"run_firmware" indicates the status which read before reset to target image. This status only means module received and done the judgement for "run_firmware" cmd, but not means module already jumped into target image.
"run_firmware" will maybe involved non-volatile memory operation in module according to module implementation, so that it will maybe spend several or tens of seconds. That will cause commit failed if sends "commit_firmware" once "run_firmware" done without "is_fw_switch_done".
imageCheckPurpose

"""
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 = 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
time.sleep(2)

if fw_info['status'] == True:
(ImageA, ImageARunning, ImageACommitted, ImageAInvalid,
ImageB, ImageBRunning, ImageBCommitted, ImageBInvalid) = fw_info['result']

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 (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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ImageACommitted check is NOT correct here because a running image need NOT be committed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, so that (Running == 1) and (Committed == 0) will return success, isn't it?

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commit_firmware() happens after the run_firmware() so why are you checking for ImageBCommitted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_fw_switch_done() executes between run_firmware() and commit_firmware() in upgrade() and assuming the running and committed image is same before upgrade(). run_firmware() will change the running image, so that checking running and committed image are different will be a easy way for judging image switch done.

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!")
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)
Expand Down Expand Up @@ -1280,6 +1333,10 @@ def upgrade(port_name, filepath):

click.echo("Firmware run in mode 1 successful")

if is_fw_switch_done(port_name) != 1:
click.echo('Failed to switch firmware images!')
sys.exit(EXIT_FAIL)

Comment on lines +1336 to +1339
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are you checking for committed image inside this function when the commit_firmware() happens at line 1342

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above reply.

status = commit_firmware(port_name)
if status != 1:
click.echo('Failed to commit firmware! CDB status: {}'.format(status))
Expand Down
24 changes: 24 additions & 0 deletions tests/sfputil_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,6 +475,30 @@ 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, '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, 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, '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):
Expand Down