From 3738e38316c086f553d401ebacd054ab3eaaa766 Mon Sep 17 00:00:00 2001 From: Debayan Ray Date: Thu, 10 Mar 2016 01:41:44 -0800 Subject: [PATCH] Follow-up of firmware update(iLO) as manual cleaning step In 3e113386ce3cf5c1faacccc9b27b9b62353eaf4c, there were some outstanding review comments which are addressed as part of this follow-up patch. Change-Id: Ib98c338b9dfa3628d40f4fa644205abf0ab24400 Partial-Bug: #1526216 --- ironic/drivers/modules/ilo/common.py | 8 +-- .../drivers/modules/ilo/firmware_processor.py | 30 ++++++----- ironic/drivers/modules/ilo/management.py | 32 +++++------ .../unit/drivers/modules/ilo/test_common.py | 53 ++++++++++++++++++- .../modules/ilo/test_firmware_processor.py | 47 +++++++++------- 5 files changed, 120 insertions(+), 50 deletions(-) diff --git a/ironic/drivers/modules/ilo/common.py b/ironic/drivers/modules/ilo/common.py index 7e13ec9598..ee8a578971 100644 --- a/ironic/drivers/modules/ilo/common.py +++ b/ironic/drivers/modules/ilo/common.py @@ -137,10 +137,10 @@ def copy_image_to_web_server(source_file_path, destination): 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 - the image if it exists in web server. + This method removes the given image from the http_root location, + if the image exists. :param object_name: The name of the image file which needs to be removed from the web server root. @@ -362,7 +362,7 @@ def _prepare_floppy_image(task, params): :raises: ImageUploadFailed, if copying the source file to the web server 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( dir=CONF.tempdir) as vfat_image_tmpfile_obj: diff --git a/ironic/drivers/modules/ilo/firmware_processor.py b/ironic/drivers/modules/ilo/firmware_processor.py index 63b80bedd7..52838ed374 100644 --- a/ironic/drivers/modules/ilo/firmware_processor.py +++ b/ironic/drivers/modules/ilo/firmware_processor.py @@ -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 # 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') if firmware_update_mode != 'ilo': - msg = (_("Invalid firmware update mode: %(mode)s. " - "'ilo' is the only supported firmware update mode.") - % {'mode': firmware_update_mode}) + msg = (_("Invalid firmware update mode '%(mode)s' provided for " + "node: %(node)s. 'ilo' is the only supported firmware " + "update mode.") + % {'mode': firmware_update_mode, 'node': task.node.uuid}) LOG.error(msg) raise exception.InvalidParameterValue(msg) 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) raise exception.InvalidParameterValue(msg) @@ -310,12 +311,13 @@ def _extract_fw_from_file(node, target_file): fw_image_filename) if to_upload: 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: if CONF.ilo.use_web_server_for_images: # 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_location, fw_image_filename) @@ -324,6 +326,10 @@ def _extract_fw_from_file(node, target_file): _remove_webserver_based_me, fw_image_location_obj) else: # 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_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. ilo_common.remove_single_or_list_of_files(fw_image_location) - LOG.debug("For firmware update, hosting firmware file: " - "%(fw_image_location)s ... done. Hosted firmware file: " - "%(fw_image_uploaded_url)s", - {'fw_image_location': fw_image_location, + LOG.debug("For firmware update on node %(node)s, hosting firmware " + "file: %(fw_image_location)s ... done. Hosted firmware " + "file: %(fw_image_uploaded_url)s", + {'fw_image_location': fw_image_location, 'node': node.uuid, 'fw_image_uploaded_url': fw_image_uploaded_url}) else: fw_image_location_obj.remove = types.MethodType( diff --git a/ironic/drivers/modules/ilo/management.py b/ironic/drivers/modules/ilo/management.py index 4281f7dc31..d7863441b3 100644 --- a/ironic/drivers/modules/ilo/management.py +++ b/ironic/drivers/modules/ilo/management.py @@ -339,23 +339,25 @@ class IloManagement(base.ManagementInterface): @base.clean_step(priority=0, abortable=False, argsinfo={ 'firmware_update_mode': { 'description': ( - 'This argument indicates the mode (or mechanism) of ' - 'Out-of-Band (OOB) firmware update procedure. ' - 'Currently, the only supported value is `ilo`.' + "This argument indicates the mode (or mechanism) of firmware " + "update procedure. Supported value is 'ilo'." ), 'required': True }, 'firmware_images': { 'description': ( - 'This argument represents the ordered list of dictionaries of ' - 'firmware image url, its checksum (md5) and component type. ' - 'The firmware images will be applied (in the order given) ' - 'one by one on the baremetal server. The different types of ' - 'firmware url schemes supported are: ' - '`file`, `http`, `https` & `swift`. ' - 'And the different firmware components that can be updated ' - 'are: ' - '`ilo`, `cpld`, `power_pic`, `bios` & `chassis`.' + "This argument represents the ordered list of JSON " + "dictionaries of firmware images. Each firmware image " + "dictionary consists of three mandatory fields, namely 'url', " + "'checksum' and 'component'. These fields represent firmware " + "image location URL, md5 checksum of image file and firmware " + "component type respectively. The supported firmware URL " + "schemes are 'file', 'http', 'https' and 'swift'. The " + "supported values for firmware component are 'ilo', 'cpld', " + "'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 } @@ -424,11 +426,11 @@ class IloManagement(base.ManagementInterface): except exception.NodeCleaningFailure: with excutils.save_and_reraise_exception(): 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}) finally: for fw_loc_obj_n_comp_tup in fw_location_objs_n_components: fw_loc_obj_n_comp_tup[0].remove() - LOG.info(_LI("All Firmware operations completed successfully for " - "node: %s."), node.uuid) + LOG.info(_LI("All Firmware update operations completed successfully " + "for node: %s."), node.uuid) diff --git a/ironic/tests/unit/drivers/modules/ilo/test_common.py b/ironic/tests/unit/drivers/modules/ilo/test_common.py index d991cac1f1..ec692601cc 100644 --- a/ironic/tests/unit/drivers/modules/ilo/test_common.py +++ b/ironic/tests/unit/drivers/modules/ilo/test_common.py @@ -787,6 +787,47 @@ class IloCommonMethodsTestCase(db_base.DbTestCase): swift_obj_mock.delete_object.assert_called_once_with( 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, autospec=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, 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 | file_list = ['/any_path1/any_file1', '/any_path2/any_file2', @@ -859,6 +900,16 @@ class IloCommonMethodsTestCase(db_base.DbTestCase): mock.call('/any_path3/any_file3')] 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) def test_verify_image_checksum(self, open_mock): # | GIVEN | diff --git a/ironic/tests/unit/drivers/modules/ilo/test_firmware_processor.py b/ironic/tests/unit/drivers/modules/ilo/test_firmware_processor.py index 729cf58aa6..d5f0f6f146 100644 --- a/ironic/tests/unit/drivers/modules/ilo/test_firmware_processor.py +++ b/ironic/tests/unit/drivers/modules/ilo/test_firmware_processor.py @@ -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 # 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' wrapped_func = (ilo_fw_processor. verify_firmware_update_args(update_firmware_mock)) + node_fake = mock.MagicMock(uuid='fake_node_uuid') + task_fake = mock.MagicMock(node=node_fake) # | WHEN & THEN | self.assertRaises(exception.InvalidParameterValue, wrapped_func, mock.ANY, - mock.ANY, + task_fake, **firmware_update_args) 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): # | GIVEN | firmware_image_info = { - 'url': 'any_valid_url', + 'url': self.any_url, 'checksum': 'b64c8f7799cfbb553d384d34dc43fafe336cc889', 'component': 'BIOS' } @@ -85,7 +87,7 @@ class FirmwareProcessorTestCase(base.TestCase): ilo_fw_processor.get_and_validate_firmware_image_info( firmware_image_info)) # | THEN | - self.assertEqual('any_valid_url', url) + self.assertEqual(self.any_url, url) self.assertEqual('b64c8f7799cfbb553d384d34dc43fafe336cc889', checksum) self.assertEqual('bios', component) @@ -93,12 +95,12 @@ class FirmwareProcessorTestCase(base.TestCase): self): # | GIVEN | invalid_firmware_image_info = { - 'url': 'any_valid_url', + 'url': self.any_url, 'component': 'bios' } # | WHEN | & | THEN | - self.assertRaises( - exception.MissingParameterValue, + self.assertRaisesRegexp( + exception.MissingParameterValue, 'checksum', ilo_fw_processor.get_and_validate_firmware_image_info, invalid_firmware_image_info) @@ -106,13 +108,13 @@ class FirmwareProcessorTestCase(base.TestCase): self): # | GIVEN | invalid_firmware_image_info = { - 'url': 'any_valid_url', + 'url': self.any_url, 'checksum': 'valid_checksum', 'component': '' } # | WHEN | & | THEN | - self.assertRaises( - exception.MissingParameterValue, + self.assertRaisesRegexp( + exception.MissingParameterValue, 'component', ilo_fw_processor.get_and_validate_firmware_image_info, invalid_firmware_image_info) @@ -120,7 +122,7 @@ class FirmwareProcessorTestCase(base.TestCase): self): # | GIVEN | invalid_firmware_image_info = { - 'url': 'any_valid_url', + 'url': self.any_url, 'checksum': 'valid_checksum', 'component': 'INVALID' } @@ -277,6 +279,8 @@ class FirmwareProcessorTestCase(base.TestCase): fw_processor.process_fw_on, node_mock, 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, 'os', autospec=True) @@ -310,6 +314,8 @@ class FirmwareProcessorTestCase(base.TestCase): actual_return_location.fw_image_location) self.assertEqual(expected_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( @@ -384,19 +390,24 @@ class FirmwareProcessorTestCase(base.TestCase): any_target_file = 'any_target_file' self.fw_processor_fake.parsed_url = urlparse.urlparse( any_swift_based_firmware_file) + urlparse_mock.reset_mock() # | WHEN | ilo_fw_processor._download_swift_based_fw_to(self.fw_processor_fake, any_target_file) # | THEN | _download_http_based_fw_to_mock.assert_called_once_with( 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, 'proliantutils_utils', autospec=True) def test__extract_fw_from_file_calls_process_firmware_image( self, utils_mock, ilo_common_mock): # | GIVEN | - node_mock = mock.ANY + node_mock = mock.MagicMock(uuid='fake_node_uuid') any_target_file = 'any_target_file' ilo_object_mock = ilo_common_mock.get_ilo_object.return_value 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( self, utils_mock, ilo_common_mock): # | GIVEN | - node_mock = mock.ANY + node_mock = mock.MagicMock(uuid='fake_node_uuid') any_target_file = 'any_target_file' utils_mock.process_firmware_image.return_value = ( '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( self, _remove_mock, utils_mock, ilo_common_mock): # | GIVEN | - node_mock = mock.ANY + node_mock = mock.MagicMock(uuid='fake_node_uuid') any_target_file = 'any_target_file' utils_mock.process_firmware_image.return_value = ( '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( self, utils_mock, ilo_common_mock): # | GIVEN | - node_mock = mock.ANY + node_mock = mock.MagicMock(uuid='fake_node_uuid') any_target_file = 'any_target_file' utils_mock.process_firmware_image.return_value = ( '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( self, _remove_mock, utils_mock, ilo_common_mock): # | GIVEN | - node_mock = mock.ANY + node_mock = mock.MagicMock(uuid='fake_node_uuid') any_target_file = 'any_target_file' utils_mock.process_firmware_image.return_value = ( '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( self, utils_mock, ilo_common_mock): # | GIVEN | - node_mock = mock.ANY + node_mock = mock.MagicMock(uuid='fake_node_uuid') any_target_file = 'any_target_file' utils_mock.process_firmware_image.return_value = ( '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( self, _remove_mock, utils_mock, ilo_common_mock): # | GIVEN | - node_mock = mock.ANY + node_mock = mock.MagicMock(uuid='fake_node_uuid') any_target_file = 'any_target_file' utils_mock.process_firmware_image.return_value = ( 'some_location/some_fw_file', True, True)