Merge "Attempt to read the partition table after writing an image"

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':

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',

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.