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.

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

[component] add auto_update_firmware() to support the auto update. #106

Merged
merged 8 commits into from
Jan 26, 2021

Conversation

sujinmkang
Copy link
Contributor

@sujinmkang sujinmkang commented Aug 4, 2020

To support the component firmware automatic update, add auto_update_firmware() to platform component api
This is to support fwutil auto_update command to update the platform component firmware automatically.
Fwutil HLD to support auto_update interfaces/commands.
sonic-net/SONiC#648

@sujinmkang
Copy link
Contributor Author

@padmanarayana can you review this?

The loading task will be created by API.

Args:
image_path: A string, path to firmware image

Choose a reason for hiding this comment

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

image_path : can it be mentioned that the firmware_image be a self extracting binary..

Choose a reason for hiding this comment

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

Got clarification from HLD.

The loading task will be created by API.

Args:
image_path: A string, path to firmware image
Copy link

Choose a reason for hiding this comment

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

This API will have any return values? Please mention if required.

@sujinmkang sujinmkang changed the title add boot_action option to fwutil update command Add auto_update_firmware() to platform component api Nov 16, 2020
@sujinmkang sujinmkang changed the title Add auto_update_firmware() to platform component api [component] add boot_action option to fwutil update command Nov 16, 2020
@sujinmkang sujinmkang changed the title [component] add boot_action option to fwutil update command [component] add auto_update_firmware() to support the auto update. Nov 16, 2020

Returns:
Output: A string, status and info
status: True or False, firmware auto-update status
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sujinmkang suggest to use status codes:

status_installed=1
status_updated=2
status_scheduled=3

Copy link
Contributor Author

@sujinmkang sujinmkang Nov 19, 2020

Choose a reason for hiding this comment

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

@nazariig
the original idea was for status to indicate the auto-update performed status which means performed successfully or not. For info, it gives more detail how the update has been performed. And platform vendor can return any information for the failure that is why the info was open.
Instead of having two outputs for the api, we can define the return_code as below based on your input . do you have any other failure cause to be added?

return_code

status_installed = 1
stauts_updated = 2
status_scheduled = 3
status_error_boottype = -1
status_error_image = -2
status_error_others = -3

@@ -103,3 +103,28 @@ def update_firmware(self, image_path):
RuntimeError: update failed
"""
raise NotImplementedError

def auto_update_firmware(self, image_path, boot_action):
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sujinmkang since this API is a part of SONiC firmware upgrade framework, please provide the description on which boot types are supported

@sujinmkang
Copy link
Contributor Author

@nazariig can I submit this PR if you don't have any other comment?

Output: A return code
return_code: An integer number, status of component firmware auto-update
- return code of a positive number indicates successful auto-update
- status_installed = 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sujinmkang IMHO, status installed is not relevant for this flow, since it requires some additional steps from user (e.g., power cycle for CPLD/FPGA, etc.), thus such an operation can't be claimed as automatic firmware upgrade.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nazariig This has been discussed several time. auto-update is expected to be able to perform install/ update/ creating a task for the components firmware update if the update is available for the boot_type.
The decision should be done by the platform api.
For example, with cold boot, cpld firmware update can be done if the reboot plugin can handle the powercycle flag to complete the cpld firmware update. For warm/fast reboot, the cpld firmware update won't happen and the platform api will return boot_type error? fwutil will not exit for the boot_type error since the firmware auto-update is not just supported in that boot_type case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For warm/fast reboot, the cpld firmware update won't happen and the platform api will return boot_type error? fwutil will not exit for the boot_type error since the firmware auto-update is not just supported in that boot_type case.

@sujinmkang that is clear, but i am talking about the case when we have status_installed.
What is the use case for this status code? Basically this means that user still needs to do some manual steps to finish firmware upgrade and this is something completely different to what we call automatic.

@@ -103,3 +103,35 @@ def update_firmware(self, image_path):
RuntimeError: update failed
"""
raise NotImplementedError

def auto_update_firmware(self, image_path, boot_action):
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sujinmkang suggest renaming: boot_action -> boot_type

- return_code of a negative number indicates failed auto-update
- status_err_boot_type = -1
- status_err_image = -2
- status_err_others = -3
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sujinmkang maybe it's better to use err_unknown?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nazariig please review this PR again.

@sujinmkang sujinmkang merged commit 6ad0004 into sonic-net:master Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants