update ucsfirmware.py/def firmware_add_remote #33
Conversation
fixed example, and removed local filepath check. This is adding a remote source, so it fails when checking os.path.exists(filepath)
|
|
||
| if not os.path.exists(file_path): | ||
| raise IOError("Image does not exist") | ||
|
|
There was a problem hiding this comment.
@jimliming thanks for the PR. May I know why check for existence of the file is not required?
There was a problem hiding this comment.
Sorry for not providing more detail.
in this function, os.path.exists doesn't provide any value. It's looking locally, when we're downloading from a remote source.
it seems we have a few options:
-
add functionality to connect to remote sources and verify the file exists before we submit the firmwareDownloader object. This seems beyond the scope of these samples.
-
submit to ucsm, and query the status of the download from ucsm.
(example of failure)
firmware_add_remote(handle, file_name="doesntexist", remote_path="/", protocol="ftp", server="ip_address", user="user", pwd="password")
file_name="doesntexist"
print (h.query_dn(dn='sys/fw-catalogue/dnld-%s' % file_name)).transfer_state
failed
(example of success)
file_name="ucs-k9-bundle-infra.2.2.5b.A.bin"
print (h.query_dn(dn='sys/fw-catalogue/dnld-%s' % file_name)).transfer_state
downloaded
- we leave this feature out of the firmware_add_remote function. it should be separate.
my PR assumes option 3. However, option 2 seems reasonable. I added the functionality.
There was a problem hiding this comment.
@jimliming looks good to me
@ragupta-git can you also review ?
| @@ -52,6 52,20 @@ def firmware_available(username, password, mdf_id_list=None, proxy=None): | |||
| image_names = [image.image_name for image in images] | |||
| return sorted(image_names) | |||
|
|
|||
| def return_download_state(handle, file_name): | |||
| ''' | |||
| Returns Transfer Status of FirmwareDownloader object for a single download. | |||
| file_name (string): firmware image name | ||
|
|
||
| Example: | ||
| return_download_status(handle, 'ucs-k9-bundle-infra.2.2.5b.A.bin') |
There was a problem hiding this comment.
return_download_status -> return_download_state
| Example: | ||
| return_download_status(handle, 'ucs-k9-bundle-infra.2.2.5b.A.bin') | ||
| ''' | ||
| download_state = (h.query_dn(dn='sys/fw-catalogue/dnld-%s' % file_name)).transfer_state |
There was a problem hiding this comment.
h -> handle.
Check and raise exception UcsOperationError if handle.query_dn returns None.
| @@ -388,6 398,11 @@ def firmware_add_remote(handle, file_name, remote_path, protocol, server, | |||
| handle.add_mo(firmware_downloader) | |||
| # handle.set_dump_xml() | |||
| handle.commit() | |||
|
|
|||
| time.sleep(5) | |||
| if return_download_state(handle, file_name) == 'failed': | |||
There was a problem hiding this comment.
Better poll for TRANSFER_STATE_DOWNLOADED and raise exception if TRANSFER_STATE_FAILED.
@vvb - should we add timeout here?
| @@ -52,6 52,20 @@ def firmware_available(username, password, mdf_id_list=None, proxy=None): | |||
| image_names = [image.image_name for image in images] | |||
| return sorted(image_names) | |||
|
|
|||
| def return_download_state(handle, file_name): | |||
There was a problem hiding this comment.
Don't think we need this function just for querying state.
Can be a part of the firmware_add_remote function itself.
…has time to respond with a proper fail. Maybe watch until there is progress and timeout within a window?
fixed example, and removed local filepath check. This is adding a remote source, so it always fails when checking os.path.exists(file_path).