From fc2dfcee60577888ad76d00e943db8e391baaccb Mon Sep 17 00:00:00 2001 From: Sam Betts Date: Thu, 29 Mar 2018 13:17:45 +0100 Subject: [PATCH] Attempt to read the partition table after writing an image This patch adds code that tries to read the partition table after we've successfully written an image to make sure the image that we wrote has a valid partition table so we can more easily guarantee that what we've written is bootable and not just junk. Without a valid partition table writing a config drive will fail for whole disk images. Co-Authored-By: Dmitry Tantsur Change-Id: I5cfd8c433a4db3e0d2d5086250e629d16234b7a4 Story: 2001760 Task: 12159 --- ironic_python_agent/extensions/standby.py | 37 +++++++++ .../tests/unit/extensions/test_standby.py | 82 ++++++++++++++++++- ...-table-after-writing-34efbd557d8de7cb.yaml | 6 ++ 3 files changed, 122 insertions(+), 3 deletions(-) create mode 100644 releasenotes/notes/check-partition-table-after-writing-34efbd557d8de7cb.yaml diff --git a/ironic_python_agent/extensions/standby.py b/ironic_python_agent/extensions/standby.py index dd5ffda1c..36be96d3e 100644 --- a/ironic_python_agent/extensions/standby.py +++ b/ironic_python_agent/extensions/standby.py @@ -17,6 +17,7 @@ import os import time from ironic_lib import disk_utils +from ironic_lib import exception from oslo_concurrency import processutils from oslo_config import cfg from oslo_log import log @@ -359,6 +360,40 @@ def _validate_image_info(ext, image_info=None, **kwargs): 'Image \'os_hash_value\' must be a non-empty string.') +def _validate_partitioning(device): + """Validate the final partition table. + + Check if after writing the image to disk we have a valid partition + table by trying to read it. This will fail if the disk is junk. + """ + try: + # Ensure we re-read the partition table before we try to list + # partitions + utils.execute('partprobe', device, run_as_root=True, + attempts=CONF.disk_utils.partprobe_attempts) + except (processutils.UnknownArgumentError, + processutils.ProcessExecutionError, OSError) as e: + LOG.warning("Unable to probe for partitions on device %(device)s " + "after writing the image, the partitioning table may " + "be broken. Error: %(error)s", + {'device': device, 'error': e}) + + try: + nparts = len(disk_utils.list_partitions(device)) + except (processutils.UnknownArgumentError, + processutils.ProcessExecutionError, OSError) as e: + msg = ("Unable to find a valid partition table on the disk after " + "writing the image. Error {}".format(e)) + raise exception.InstanceDeployFailure(msg) + + # Check if there is at least one partition in the partition table after + # deploy + if not nparts: + msg = ("No partitions found on the device {} after writing " + "the image.".format(device)) + raise exception.InstanceDeployFailure(msg) + + class StandbyExtension(base.BaseAgentExtension): """Extension which adds stand-by related functionality to agent.""" def __init__(self, agent=None): @@ -495,6 +530,8 @@ class StandbyExtension(base.BaseAgentExtension): else: self._cache_and_write_image(image_info, device) + _validate_partitioning(device) + # the configdrive creation is taken care by ironic-lib's # work_on_disk(). if image_info.get('image_type') != 'partition': diff --git a/ironic_python_agent/tests/unit/extensions/test_standby.py b/ironic_python_agent/tests/unit/extensions/test_standby.py index a69aec580..2b5e7b66d 100644 --- a/ironic_python_agent/tests/unit/extensions/test_standby.py +++ b/ironic_python_agent/tests/unit/extensions/test_standby.py @@ -603,6 +603,10 @@ class TestStandbyExtension(base.IronicAgentTest): @mock.patch('ironic_lib.disk_utils.get_disk_identifier', lambda dev: 'ROOT') + @mock.patch('ironic_python_agent.utils.execute', + autospec=True) + @mock.patch('ironic_lib.disk_utils.list_partitions', + autospec=True) @mock.patch('ironic_lib.disk_utils.create_config_drive_partition', autospec=True) @mock.patch('ironic_python_agent.hardware.dispatch_to_managers', @@ -615,12 +619,15 @@ class TestStandbyExtension(base.IronicAgentTest): download_mock, write_mock, dispatch_mock, - configdrive_copy_mock): + configdrive_copy_mock, + list_part_mock, + execute_mock): image_info = _build_fake_image_info() download_mock.return_value = None write_mock.return_value = None dispatch_mock.return_value = 'manager' configdrive_copy_mock.return_value = None + list_part_mock.return_value = [mock.MagicMock()] async_result = self.agent_extension.prepare_image( image_info=image_info, @@ -640,7 +647,14 @@ class TestStandbyExtension(base.IronicAgentTest): cmd_result = ('prepare_image: image ({}) written to device {} ' 'root_uuid=ROOT').format(image_info['id'], 'manager') self.assertEqual(cmd_result, async_result.command_result['result']) + list_part_mock.assert_called_with('manager') + execute_mock.assert_called_with('partprobe', 'manager', + run_as_root=True, + attempts=mock.ANY) + @mock.patch('ironic_python_agent.utils.execute', autospec=True) + @mock.patch('ironic_lib.disk_utils.list_partitions', + autospec=True) @mock.patch('ironic_lib.disk_utils.create_config_drive_partition', autospec=True) @mock.patch('ironic_python_agent.hardware.dispatch_to_managers', @@ -653,12 +667,15 @@ class TestStandbyExtension(base.IronicAgentTest): download_mock, write_mock, dispatch_mock, - configdrive_copy_mock): + configdrive_copy_mock, + list_part_mock, + execute_mock): image_info = _build_fake_partition_image_info() download_mock.return_value = None write_mock.return_value = {'root uuid': 'root_uuid'} dispatch_mock.return_value = 'manager' configdrive_copy_mock.return_value = None + list_part_mock.return_value = [mock.MagicMock()] async_result = self.agent_extension.prepare_image( image_info=image_info, @@ -698,11 +715,18 @@ class TestStandbyExtension(base.IronicAgentTest): 'root_uuid={}').format( image_info['id'], 'manager', 'root_uuid') self.assertEqual(cmd_result, async_result.command_result['result']) + list_part_mock.assert_called_with('manager') + execute_mock.assert_called_with('partprobe', 'manager', + run_as_root=True, + attempts=mock.ANY) @mock.patch('ironic_lib.disk_utils.get_disk_identifier', lambda dev: 'ROOT') + @mock.patch('ironic_python_agent.utils.execute', autospec=True) @mock.patch('ironic_lib.disk_utils.create_config_drive_partition', autospec=True) + @mock.patch('ironic_lib.disk_utils.list_partitions', + autospec=True) @mock.patch('ironic_python_agent.hardware.dispatch_to_managers', autospec=True) @mock.patch('ironic_python_agent.extensions.standby._write_image', @@ -713,12 +737,15 @@ class TestStandbyExtension(base.IronicAgentTest): download_mock, write_mock, dispatch_mock, - configdrive_copy_mock): + list_part_mock, + configdrive_copy_mock, + execute_mock): image_info = _build_fake_image_info() download_mock.return_value = None write_mock.return_value = None dispatch_mock.return_value = 'manager' configdrive_copy_mock.return_value = None + list_part_mock.return_value = [mock.MagicMock()] async_result = self.agent_extension.prepare_image( image_info=image_info, @@ -740,6 +767,55 @@ class TestStandbyExtension(base.IronicAgentTest): @mock.patch('ironic_lib.disk_utils.get_disk_identifier', lambda dev: 'ROOT') @mock.patch('ironic_lib.disk_utils.work_on_disk', autospec=True) + @mock.patch('ironic_lib.disk_utils.create_config_drive_partition', + autospec=True) + @mock.patch('ironic_lib.disk_utils.list_partitions', + autospec=True) + @mock.patch('ironic_python_agent.hardware.dispatch_to_managers', + autospec=True) + @mock.patch('ironic_python_agent.extensions.standby._write_image', + autospec=True) + @mock.patch('ironic_python_agent.extensions.standby._download_image', + autospec=True) + def test_prepare_image_bad_partition(self, + download_mock, + write_mock, + dispatch_mock, + list_part_mock, + configdrive_copy_mock, + work_on_disk_mock): + list_part_mock.side_effect = processutils.ProcessExecutionError + image_info = _build_fake_image_info() + download_mock.return_value = None + write_mock.return_value = None + dispatch_mock.return_value = 'manager' + configdrive_copy_mock.return_value = None + work_on_disk_mock.return_value = { + 'root uuid': 'a318821b-2a60-40e5-a011-7ac07fce342b', + 'partitions': { + 'root': '/dev/foo-part1', + } + } + + async_result = self.agent_extension.prepare_image( + image_info=image_info, + configdrive=None + ) + async_result.join() + + download_mock.assert_called_once_with(image_info) + write_mock.assert_called_once_with(image_info, 'manager') + dispatch_mock.assert_called_once_with('get_os_install_device') + + self.assertFalse(configdrive_copy_mock.called) + self.assertEqual('FAILED', async_result.command_status) + + @mock.patch('ironic_python_agent.utils.execute', mock.Mock()) + @mock.patch('ironic_lib.disk_utils.list_partitions', + lambda _dev: [mock.Mock()]) + @mock.patch('ironic_lib.disk_utils.get_disk_identifier', + lambda dev: 'ROOT') + @mock.patch('ironic_lib.disk_utils.work_on_disk', autospec=True) @mock.patch('ironic_lib.disk_utils.create_config_drive_partition', autospec=True) @mock.patch('ironic_python_agent.hardware.dispatch_to_managers', diff --git a/releasenotes/notes/check-partition-table-after-writing-34efbd557d8de7cb.yaml b/releasenotes/notes/check-partition-table-after-writing-34efbd557d8de7cb.yaml new file mode 100644 index 000000000..e0157a3c5 --- /dev/null +++ b/releasenotes/notes/check-partition-table-after-writing-34efbd557d8de7cb.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + Fixes a bug with bad whole disk images and config drives, where we would + attempt to write a config drive partition to the disk without a valid + partition table.