-
Notifications
You must be signed in to change notification settings - Fork 20
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
Add firmware_update phase support for HPE servers #181
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry I just spotted this, I think it was proposed while I was out on vacation and I missed it until now. I see a lot of very large json files getting added, which seem to only contain hp specific data. Also, they are date stamped so I'm guessing they'll need to be updated periodically? But I don't see any documentation about where they came from or how to update them, and I'm guessing they aren't from us.
Ideally, would it be possible to import these from somewhere only when needed? it would be nice to not have to land them all here and keep them updated in our source tree if we can avoid it.
@plars I don't think we want to maintain these json files either. I explained the situation in description (quoted below). I'm expecting HPE to update their json files on their firmware repository so that we can use them directly whenever we need (they promise to do that after the new year). Once they're ready, I would update the json files fetching script. It would be apprecitated if you can take a look of other parts first. :)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just saw that you had updated this, and I definitely need to spend some more time looking at it but it may take some time because it's quite large. I wanted to get a few comments/questions in quickly though...
- see comment below about logmsg()
- is this the new json files? https://downloads.linux.hpe.com/SDR/repo/fwpp-gen10/next/fwrepodata/fwrepo.json
- you mentioned a script to download these from the source... I don't see that here though. My general concern is that this hosts a LOT of data from external sources, but I don't see anything in documentation about where it came from, etc. And I wonder if we should be hosting it at all rather than pulling it from the original source.
f"SPP {spp} is not available in HPE FW repository." | ||
" Please check if it's a valid SPP." | ||
) | ||
logmsg( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a WIP branch that removes logmsg(). I would definitely recommend replacing all new calls you have here to logmsg to just do the normal logger.{loglevel}(...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed logmsg().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your changes, just few small comments here
bmc_password: str, | ||
): | ||
super().__init__(ipaddr, user) | ||
self.bmc_ip = bmc_ip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be any validation of the IP address?
|
||
def run_cmd( | ||
self, cmds: str, raise_stderr: bool = True, timeout: int = 30 | ||
) -> Tuple[int, str, str]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you’re using Python >=3.9, there is no need to use Tuple from typing library, you can just use tuple
instead. Here are some examples in the doc: https://docs.python.org/3/library/typing.html#type-aliases
logger.info( | ||
"check and wait for %ds until %s is SSHable" | ||
% (timeout, self.ipaddr) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can pass variables to log as function arguments, and this is used even in the documentation
logger.info( | |
"check and wait for %ds until %s is SSHable" | |
% (timeout, self.ipaddr) | |
) | |
logger.info( | |
"check and wait for %ds until %s is SSHable", | |
timeout, self.ipaddr | |
) |
move the changes to #255 |
Description
Based on CR033 - Firmware updates for regression testing, add
firmware_update
support for HPE servers utilisingilorest
(apt install from HPE repository) and the firmware files downloaded from HPE firmware repository.Since the firmware repository index files (fwrepo.json) on HPE firmware repository are not having complete list of devices supported by each file (presenting in "target" field, which should be a list instead of single target ID), a temporary implementation is hosting our own customised index files in the codebase until HPE update their index files in the repository. Once the files being updated in the repository, we can remove the local index files, and refer to remote index file in the run time.
Resolved issues
N/A
Documentation
The documentation is already in https://github.com/canonical/testflinger/blob/main/docs/reference/test-phases.rst
Tests
HPE.py