openstack/ironic
Code Issues Proposed changes

Merge "Follow-up of firmware update(iLO) as manual cleaning step"

Browse Source
This commit is contained in:
Jenkins 2016-03-17 08:49:34 +00:00 committed by Gerrit Code Review
commit cb1a83f740
5 changed files with 120 additions and 50 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
ironic/drivers/modules/ilo/common.py
View File

@ -137,10 +137,10 @@ def copy_image_to_web_server(source_file_path, destination):
def remove_image_from_web_server(object_name): def remove_image_from_web_server(object_name):
"""Removes the given image from the configured http web server. """Removes the given image from the configured web server.
This method removes the given image from the http_root location. It deletes This method removes the given image from the http_root location,
the image if it exists in web server. if the image exists.
:param object_name: The name of the image file which needs to be removed :param object_name: The name of the image file which needs to be removed
from the web server root. from the web server root.
@ -362,7 +362,7 @@ def _prepare_floppy_image(task, params):
:raises: ImageUploadFailed, if copying the source file to the :raises: ImageUploadFailed, if copying the source file to the
web server fails. web server fails.
:raises: SwiftOperationError, if any operation with Swift fails. :raises: SwiftOperationError, if any operation with Swift fails.
:returns: the http image URL or the Swift temp url for the floppy image. :returns: the HTTP image URL or the Swift temp url for the floppy image.
""" """
with tempfile.NamedTemporaryFile( with tempfile.NamedTemporaryFile(
dir=CONF.tempdir) as vfat_image_tmpfile_obj: dir=CONF.tempdir) as vfat_image_tmpfile_obj:

30
ironic/drivers/modules/ilo/firmware_processor.py
View File

@ -1,4 +1,4 @@
# Copyright 2016 Hewlett Packard Enterprise Development Company, L.P. # Copyright 2016 Hewlett Packard Enterprise Development Company LP
# #
# Licensed under the Apache License, Version 2.0 (the "License"); you may # Licensed under the Apache License, Version 2.0 (the "License"); you may
# not use this file except in compliance with the License. You may obtain # not use this file except in compliance with the License. You may obtain
@ -64,14 +64,15 @@ def verify_firmware_update_args(func):
firmware_images = kwargs.get('firmware_images') firmware_images = kwargs.get('firmware_images')
if firmware_update_mode != 'ilo': if firmware_update_mode != 'ilo':
msg = (_("Invalid firmware update mode: %(mode)s. " msg = (_("Invalid firmware update mode '%(mode)s' provided for "
"'ilo' is the only supported firmware update mode.") "node: %(node)s. 'ilo' is the only supported firmware "
% {'mode': firmware_update_mode}) "update mode.")
% {'mode': firmware_update_mode, 'node': task.node.uuid})
LOG.error(msg) LOG.error(msg)
raise exception.InvalidParameterValue(msg) raise exception.InvalidParameterValue(msg)
if not firmware_images: if not firmware_images:
msg = _("Firmware images cannot be an empty list or None. ") msg = _("Firmware images cannot be an empty list or None.")
LOG.error(msg) LOG.error(msg)
raise exception.InvalidParameterValue(msg) raise exception.InvalidParameterValue(msg)
@ -310,12 +311,13 @@ def _extract_fw_from_file(node, target_file):
fw_image_filename) fw_image_filename)
if to_upload: if to_upload:
is_different_file = True is_different_file = True
LOG.debug("For firmware update, hosting firmware file: %s ... ",
fw_image_location)
# fw_image_filename = os.path.basename(fw_image_location)
try: try:
if CONF.ilo.use_web_server_for_images: if CONF.ilo.use_web_server_for_images:
# upload firmware image file to conductor webserver # upload firmware image file to conductor webserver
LOG.debug("For firmware update on node %(node)s, hosting "
"firmware file %(firmware_image)s on web server ...",
{'firmware_image': fw_image_location,
'node': node.uuid})
fw_image_uploaded_url = ilo_common.copy_image_to_web_server( fw_image_uploaded_url = ilo_common.copy_image_to_web_server(
fw_image_location, fw_image_filename) fw_image_location, fw_image_filename)
@ -324,6 +326,10 @@ def _extract_fw_from_file(node, target_file):
_remove_webserver_based_me, fw_image_location_obj) _remove_webserver_based_me, fw_image_location_obj)
else: else:
# upload firmware image file to swift # upload firmware image file to swift
LOG.debug("For firmware update on node %(node)s, hosting "
"firmware file %(firmware_image)s on swift ...",
{'firmware_image': fw_image_location,
'node': node.uuid})
fw_image_uploaded_url = ilo_common.copy_image_to_swift( fw_image_uploaded_url = ilo_common.copy_image_to_swift(
fw_image_location, fw_image_filename) fw_image_location, fw_image_filename)
@ -339,10 +345,10 @@ def _extract_fw_from_file(node, target_file):
# takes care of handling the deletion of the file. # takes care of handling the deletion of the file.
ilo_common.remove_single_or_list_of_files(fw_image_location) ilo_common.remove_single_or_list_of_files(fw_image_location)
LOG.debug("For firmware update, hosting firmware file: " LOG.debug("For firmware update on node %(node)s, hosting firmware "
"%(fw_image_location)s ... done. Hosted firmware file: " "file: %(fw_image_location)s ... done. Hosted firmware "
"%(fw_image_uploaded_url)s", "file: %(fw_image_uploaded_url)s",
{'fw_image_location': fw_image_location, {'fw_image_location': fw_image_location, 'node': node.uuid,
'fw_image_uploaded_url': fw_image_uploaded_url}) 'fw_image_uploaded_url': fw_image_uploaded_url})
else: else:
fw_image_location_obj.remove = types.MethodType( fw_image_location_obj.remove = types.MethodType(

32
ironic/drivers/modules/ilo/management.py
View File

@ -339,23 +339,25 @@ class IloManagement(base.ManagementInterface):
@base.clean_step(priority=0, abortable=False, argsinfo={ @base.clean_step(priority=0, abortable=False, argsinfo={
'firmware_update_mode': { 'firmware_update_mode': {
'description': ( 'description': (
'This argument indicates the mode (or mechanism) of ' "This argument indicates the mode (or mechanism) of firmware "
'Out-of-Band (OOB) firmware update procedure. ' "update procedure. Supported value is 'ilo'."
'Currently, the only supported value is `ilo`.'
), ),
'required': True 'required': True
}, },
'firmware_images': { 'firmware_images': {
'description': ( 'description': (
'This argument represents the ordered list of dictionaries of ' "This argument represents the ordered list of JSON "
'firmware image url, its checksum (md5) and component type. ' "dictionaries of firmware images. Each firmware image "
'The firmware images will be applied (in the order given) ' "dictionary consists of three mandatory fields, namely 'url', "
'one by one on the baremetal server. The different types of ' "'checksum' and 'component'. These fields represent firmware "
'firmware url schemes supported are: ' "image location URL, md5 checksum of image file and firmware "
'`file`, `http`, `https` & `swift`. ' "component type respectively. The supported firmware URL "
'And the different firmware components that can be updated ' "schemes are 'file', 'http', 'https' and 'swift'. The "
'are: ' "supported values for firmware component are 'ilo', 'cpld', "
'`ilo`, `cpld`, `power_pic`, `bios` & `chassis`.' "'power_pic', 'bios' and 'chassis'. The firmware images will "
"be applied (in the order given) one by one on the baremetal "
"server. For more information, see "
"http://docs.openstack.org/developer/ironic/drivers/ilo.html#initiating-firmware-update-as-manual-clean-step" # noqa
), ),
'required': True 'required': True
} }
@ -424,11 +426,11 @@ class IloManagement(base.ManagementInterface):
except exception.NodeCleaningFailure: except exception.NodeCleaningFailure:
with excutils.save_and_reraise_exception(): with excutils.save_and_reraise_exception():
LOG.error(_LE("Firmware update for %(firmware_file)s on " LOG.error(_LE("Firmware update for %(firmware_file)s on "
"node: %(node)s ... failed"), "node: %(node)s failed."),
{'firmware_file': fw_location, 'node': node.uuid}) {'firmware_file': fw_location, 'node': node.uuid})
finally: finally:
for fw_loc_obj_n_comp_tup in fw_location_objs_n_components: for fw_loc_obj_n_comp_tup in fw_location_objs_n_components:
fw_loc_obj_n_comp_tup[0].remove() fw_loc_obj_n_comp_tup[0].remove()
LOG.info(_LI("All Firmware operations completed successfully for " LOG.info(_LI("All Firmware update operations completed successfully "
"node: %s."), node.uuid) "for node: %s."), node.uuid)

53
ironic/tests/unit/drivers/modules/ilo/test_common.py
View File

@ -787,6 +787,47 @@ class IloCommonMethodsTestCase(db_base.DbTestCase):
swift_obj_mock.delete_object.assert_called_once_with( swift_obj_mock.delete_object.assert_called_once_with(
container, object_name) container, object_name)
@mock.patch.object(ilo_common, 'LOG')
@mock.patch.object(swift, 'SwiftAPI', spec_set=True, autospec=True)
def test_remove_image_from_swift_suppresses_notfound_exc(
self, swift_api_mock, LOG_mock):
# | GIVEN |
self.config(swift_ilo_container='ilo_container', group='ilo')
container = CONF.ilo.swift_ilo_container
swift_obj_mock = swift_api_mock.return_value
object_name = 'object_name'
raised_exc = exception.SwiftObjectNotFoundError(
operation='delete_object', object=object_name, container=container)
swift_obj_mock.delete_object.side_effect = raised_exc
# | WHEN |
ilo_common.remove_image_from_swift(object_name)
# | THEN |
LOG_mock.warning.assert_called_once_with(
mock.ANY, {'associated_with_msg': "", 'err': raised_exc})
@mock.patch.object(ilo_common, 'LOG')
@mock.patch.object(swift, 'SwiftAPI', spec_set=True, autospec=True)
def test_remove_image_from_swift_suppresses_operror_exc(
self, swift_api_mock, LOG_mock):
# | GIVEN |
self.config(swift_ilo_container='ilo_container', group='ilo')
container = CONF.ilo.swift_ilo_container
swift_obj_mock = swift_api_mock.return_value
object_name = 'object_name'
raised_exc = exception.SwiftOperationError(operation='delete_object',
error='failed')
swift_obj_mock.delete_object.side_effect = raised_exc
# | WHEN |
ilo_common.remove_image_from_swift(object_name, 'alice_in_wonderland')
# | THEN |
LOG_mock.exception.assert_called_once_with(
mock.ANY, {'object_name': object_name, 'container': container,
'associated_with_msg': ("associated with "
"alice_in_wonderland"),
'err': raised_exc})
@mock.patch.object(ironic_utils, 'unlink_without_raise', spec_set=True, @mock.patch.object(ironic_utils, 'unlink_without_raise', spec_set=True,
autospec=True) autospec=True)
@mock.patch.object(ilo_common, '_get_floppy_image_name', spec_set=True, @mock.patch.object(ilo_common, '_get_floppy_image_name', spec_set=True,
@ -846,7 +887,7 @@ class IloCommonMethodsTestCase(db_base.DbTestCase):
@mock.patch.object(ironic_utils, 'unlink_without_raise', spec_set=True, @mock.patch.object(ironic_utils, 'unlink_without_raise', spec_set=True,
autospec=True) autospec=True)
def test_remove_single_or_list_of_files(self, unlink_mock): def test_remove_single_or_list_of_files_with_file_list(self, unlink_mock):
# | GIVEN | # | GIVEN |
file_list = ['/any_path1/any_file1', file_list = ['/any_path1/any_file1',
'/any_path2/any_file2', '/any_path2/any_file2',
@ -859,6 +900,16 @@ class IloCommonMethodsTestCase(db_base.DbTestCase):
mock.call('/any_path3/any_file3')] mock.call('/any_path3/any_file3')]
unlink_mock.assert_has_calls(calls) unlink_mock.assert_has_calls(calls)
@mock.patch.object(ironic_utils, 'unlink_without_raise', spec_set=True,
autospec=True)
def test_remove_single_or_list_of_files_with_file_str(self, unlink_mock):
# | GIVEN |
file_path = '/any_path1/any_file'
# | WHEN |
ilo_common.remove_single_or_list_of_files(file_path)
# | THEN |
unlink_mock.assert_called_once_with('/any_path1/any_file')
@mock.patch.object(__builtin__, 'open', autospec=True) @mock.patch.object(__builtin__, 'open', autospec=True)
def test_verify_image_checksum(self, open_mock): def test_verify_image_checksum(self, open_mock):
# | GIVEN | # | GIVEN |

47
ironic/tests/unit/drivers/modules/ilo/test_firmware_processor.py
View File

@ -1,4 +1,4 @@
# Copyright 2016 Hewlett Packard Enterprise Development Company, L.P. # Copyright 2016 Hewlett Packard Enterprise Development Company LP
# #
# Licensed under the Apache License, Version 2.0 (the "License"); you may # Licensed under the Apache License, Version 2.0 (the "License"); you may
# not use this file except in compliance with the License. You may obtain # not use this file except in compliance with the License. You may obtain
@ -51,11 +51,13 @@ class FirmwareProcessorTestCase(base.TestCase):
update_firmware_mock.__name__ = 'update_firmware_mock' update_firmware_mock.__name__ = 'update_firmware_mock'
wrapped_func = (ilo_fw_processor. wrapped_func = (ilo_fw_processor.
verify_firmware_update_args(update_firmware_mock)) verify_firmware_update_args(update_firmware_mock))
node_fake = mock.MagicMock(uuid='fake_node_uuid')
task_fake = mock.MagicMock(node=node_fake)
# | WHEN & THEN | # | WHEN & THEN |
self.assertRaises(exception.InvalidParameterValue, self.assertRaises(exception.InvalidParameterValue,
wrapped_func, wrapped_func,
mock.ANY, mock.ANY,
mock.ANY, task_fake,
**firmware_update_args) **firmware_update_args)
def test_verify_firmware_update_args_throws_for_no_firmware_url(self): def test_verify_firmware_update_args_throws_for_no_firmware_url(self):
@ -76,7 +78,7 @@ class FirmwareProcessorTestCase(base.TestCase):
def test_get_and_validate_firmware_image_info(self): def test_get_and_validate_firmware_image_info(self):
# | GIVEN | # | GIVEN |
firmware_image_info = { firmware_image_info = {
'url': 'any_valid_url', 'url': self.any_url,
'checksum': 'b64c8f7799cfbb553d384d34dc43fafe336cc889', 'checksum': 'b64c8f7799cfbb553d384d34dc43fafe336cc889',
'component': 'BIOS' 'component': 'BIOS'
} }
@ -85,7 +87,7 @@ class FirmwareProcessorTestCase(base.TestCase):
ilo_fw_processor.get_and_validate_firmware_image_info( ilo_fw_processor.get_and_validate_firmware_image_info(
firmware_image_info)) firmware_image_info))
# | THEN | # | THEN |
self.assertEqual('any_valid_url', url) self.assertEqual(self.any_url, url)
self.assertEqual('b64c8f7799cfbb553d384d34dc43fafe336cc889', checksum) self.assertEqual('b64c8f7799cfbb553d384d34dc43fafe336cc889', checksum)
self.assertEqual('bios', component) self.assertEqual('bios', component)
@ -93,12 +95,12 @@ class FirmwareProcessorTestCase(base.TestCase):
self): self):
# | GIVEN | # | GIVEN |
invalid_firmware_image_info = { invalid_firmware_image_info = {
'url': 'any_valid_url', 'url': self.any_url,
'component': 'bios' 'component': 'bios'
} }
# | WHEN | & | THEN | # | WHEN | & | THEN |
self.assertRaises( self.assertRaisesRegexp(
exception.MissingParameterValue, exception.MissingParameterValue, 'checksum',
ilo_fw_processor.get_and_validate_firmware_image_info, ilo_fw_processor.get_and_validate_firmware_image_info,
invalid_firmware_image_info) invalid_firmware_image_info)
@ -106,13 +108,13 @@ class FirmwareProcessorTestCase(base.TestCase):
self): self):
# | GIVEN | # | GIVEN |
invalid_firmware_image_info = { invalid_firmware_image_info = {
'url': 'any_valid_url', 'url': self.any_url,
'checksum': 'valid_checksum', 'checksum': 'valid_checksum',
'component': '' 'component': ''
} }
# | WHEN | & | THEN | # | WHEN | & | THEN |
self.assertRaises( self.assertRaisesRegexp(
exception.MissingParameterValue, exception.MissingParameterValue, 'component',
ilo_fw_processor.get_and_validate_firmware_image_info, ilo_fw_processor.get_and_validate_firmware_image_info,
invalid_firmware_image_info) invalid_firmware_image_info)
@ -120,7 +122,7 @@ class FirmwareProcessorTestCase(base.TestCase):
self): self):
# | GIVEN | # | GIVEN |
invalid_firmware_image_info = { invalid_firmware_image_info = {
'url': 'any_valid_url', 'url': self.any_url,
'checksum': 'valid_checksum', 'checksum': 'valid_checksum',
'component': 'INVALID' 'component': 'INVALID'
} }
@ -277,6 +279,8 @@ class FirmwareProcessorTestCase(base.TestCase):
fw_processor.process_fw_on, fw_processor.process_fw_on,
node_mock, node_mock,
checksum_fake) checksum_fake)
shutil_mock.rmtree.assert_called_once_with(
tempfile_mock.mkdtemp(), ignore_errors=True)
@mock.patch.object(ilo_fw_processor, 'tempfile', autospec=True) @mock.patch.object(ilo_fw_processor, 'tempfile', autospec=True)
@mock.patch.object(ilo_fw_processor, 'os', autospec=True) @mock.patch.object(ilo_fw_processor, 'os', autospec=True)
@ -310,6 +314,8 @@ class FirmwareProcessorTestCase(base.TestCase):
actual_return_location.fw_image_location) actual_return_location.fw_image_location)
self.assertEqual(expected_return_location.fw_image_filename, self.assertEqual(expected_return_location.fw_image_filename,
actual_return_location.fw_image_filename) actual_return_location.fw_image_filename)
shutil_mock.rmtree.assert_called_once_with(
tempfile_mock.mkdtemp(), ignore_errors=True)
@mock.patch.object(__builtin__, 'open', autospec=True) @mock.patch.object(__builtin__, 'open', autospec=True)
@mock.patch.object( @mock.patch.object(
@ -384,19 +390,24 @@ class FirmwareProcessorTestCase(base.TestCase):
any_target_file = 'any_target_file' any_target_file = 'any_target_file'
self.fw_processor_fake.parsed_url = urlparse.urlparse( self.fw_processor_fake.parsed_url = urlparse.urlparse(
any_swift_based_firmware_file) any_swift_based_firmware_file)
urlparse_mock.reset_mock()
# | WHEN | # | WHEN |
ilo_fw_processor._download_swift_based_fw_to(self.fw_processor_fake, ilo_fw_processor._download_swift_based_fw_to(self.fw_processor_fake,
any_target_file) any_target_file)
# | THEN | # | THEN |
_download_http_based_fw_to_mock.assert_called_once_with( _download_http_based_fw_to_mock.assert_called_once_with(
self.fw_processor_fake, any_target_file) self.fw_processor_fake, any_target_file)
urlparse_mock.assert_called_once_with(
swift_mock.SwiftAPI().get_temp_url.return_value)
self.assertEqual(
urlparse_mock.return_value, self.fw_processor_fake.parsed_url)
@mock.patch.object(ilo_fw_processor, 'ilo_common', autospec=True) @mock.patch.object(ilo_fw_processor, 'ilo_common', autospec=True)
@mock.patch.object(ilo_fw_processor, 'proliantutils_utils', autospec=True) @mock.patch.object(ilo_fw_processor, 'proliantutils_utils', autospec=True)
def test__extract_fw_from_file_calls_process_firmware_image( def test__extract_fw_from_file_calls_process_firmware_image(
self, utils_mock, ilo_common_mock): self, utils_mock, ilo_common_mock):
# | GIVEN | # | GIVEN |
node_mock = mock.ANY node_mock = mock.MagicMock(uuid='fake_node_uuid')
any_target_file = 'any_target_file' any_target_file = 'any_target_file'
ilo_object_mock = ilo_common_mock.get_ilo_object.return_value ilo_object_mock = ilo_common_mock.get_ilo_object.return_value
utils_mock.process_firmware_image.return_value = ('some_location', utils_mock.process_firmware_image.return_value = ('some_location',
@ -412,7 +423,7 @@ class FirmwareProcessorTestCase(base.TestCase):
def test__extract_fw_from_file_doesnt_upload_firmware( def test__extract_fw_from_file_doesnt_upload_firmware(
self, utils_mock, ilo_common_mock): self, utils_mock, ilo_common_mock):
# | GIVEN | # | GIVEN |
node_mock = mock.ANY node_mock = mock.MagicMock(uuid='fake_node_uuid')
any_target_file = 'any_target_file' any_target_file = 'any_target_file'
utils_mock.process_firmware_image.return_value = ( utils_mock.process_firmware_image.return_value = (
'some_location/some_fw_file', False, True) 'some_location/some_fw_file', False, True)
@ -428,7 +439,7 @@ class FirmwareProcessorTestCase(base.TestCase):
def test__extract_fw_from_file_sets_loc_obj_remove_to_file_if_no_upload( def test__extract_fw_from_file_sets_loc_obj_remove_to_file_if_no_upload(
self, _remove_mock, utils_mock, ilo_common_mock): self, _remove_mock, utils_mock, ilo_common_mock):
# | GIVEN | # | GIVEN |
node_mock = mock.ANY node_mock = mock.MagicMock(uuid='fake_node_uuid')
any_target_file = 'any_target_file' any_target_file = 'any_target_file'
utils_mock.process_firmware_image.return_value = ( utils_mock.process_firmware_image.return_value = (
'some_location/some_fw_file', False, True) 'some_location/some_fw_file', False, True)
@ -444,7 +455,7 @@ class FirmwareProcessorTestCase(base.TestCase):
def test__extract_fw_from_file_uploads_firmware_to_webserver( def test__extract_fw_from_file_uploads_firmware_to_webserver(
self, utils_mock, ilo_common_mock): self, utils_mock, ilo_common_mock):
# | GIVEN | # | GIVEN |
node_mock = mock.ANY node_mock = mock.MagicMock(uuid='fake_node_uuid')
any_target_file = 'any_target_file' any_target_file = 'any_target_file'
utils_mock.process_firmware_image.return_value = ( utils_mock.process_firmware_image.return_value = (
'some_location/some_fw_file', True, True) 'some_location/some_fw_file', True, True)
@ -462,7 +473,7 @@ class FirmwareProcessorTestCase(base.TestCase):
def test__extract_fw_from_file_sets_loc_obj_remove_to_webserver( def test__extract_fw_from_file_sets_loc_obj_remove_to_webserver(
self, _remove_mock, utils_mock, ilo_common_mock): self, _remove_mock, utils_mock, ilo_common_mock):
# | GIVEN | # | GIVEN |
node_mock = mock.ANY node_mock = mock.MagicMock(uuid='fake_node_uuid')
any_target_file = 'any_target_file' any_target_file = 'any_target_file'
utils_mock.process_firmware_image.return_value = ( utils_mock.process_firmware_image.return_value = (
'some_location/some_fw_file', True, True) 'some_location/some_fw_file', True, True)
@ -479,7 +490,7 @@ class FirmwareProcessorTestCase(base.TestCase):
def test__extract_fw_from_file_uploads_firmware_to_swift( def test__extract_fw_from_file_uploads_firmware_to_swift(
self, utils_mock, ilo_common_mock): self, utils_mock, ilo_common_mock):
# | GIVEN | # | GIVEN |
node_mock = mock.ANY node_mock = mock.MagicMock(uuid='fake_node_uuid')
any_target_file = 'any_target_file' any_target_file = 'any_target_file'
utils_mock.process_firmware_image.return_value = ( utils_mock.process_firmware_image.return_value = (
'some_location/some_fw_file', True, True) 'some_location/some_fw_file', True, True)
@ -497,7 +508,7 @@ class FirmwareProcessorTestCase(base.TestCase):
def test__extract_fw_from_file_sets_loc_obj_remove_to_swift( def test__extract_fw_from_file_sets_loc_obj_remove_to_swift(
self, _remove_mock, utils_mock, ilo_common_mock): self, _remove_mock, utils_mock, ilo_common_mock):
# | GIVEN | # | GIVEN |
node_mock = mock.ANY node_mock = mock.MagicMock(uuid='fake_node_uuid')
any_target_file = 'any_target_file' any_target_file = 'any_target_file'
utils_mock.process_firmware_image.return_value = ( utils_mock.process_firmware_image.return_value = (
'some_location/some_fw_file', True, True) 'some_location/some_fw_file', True, True)