Merge "Don't try to put a bootloader in place when bootable" into stable/train

bindep.txt

@@ -26,3 +26,4 @@ python-pip [imagebuild]
 unzip [imagebuild]
 sudo [imagebuild]
 gawk [imagebuild]
+file [imagebuild]

ironic_python_agent/extensions/image.py

@@ -134,6 +134,51 @@ def _has_dracut(root):
     return True
 
 
+def _is_bootloader_loaded(dev):
+    """Checks the device to see if a MBR bootloader is present.
+
+    :param str dev: Block device upon which to check if it appears
+                       to be bootable via MBR.
+    :returns: True if a device appears to be bootable with a boot
+              loader, otherwise False.
+    """
+
+    def _has_boot_sector(device):
+        """Check the device for a boot sector indiator."""
+        stdout, stderr = utils.execute('file', '-s', device)
+        if 'boot sector' in stdout:
+            # Now lets check the signature
+            ddout, dderr = utils.execute(
+                'dd', 'if=%s' % device, 'bs=218', 'count=1')
+            stdout, stderr = utils.execute('file', '-', process_input=ddout)
+            # The bytes recovered by dd show as a "dos executable" when
+            # examined with file. In other words, the bootloader is present.
+            if 'executable' in stdout:
+                return True
+        return False
+
+    try:
+        # Looking for things marked "bootable" in the partition table
+        stdout, stderr = utils.execute('parted', dev, '-s', '-m',
+                                       '--', 'print')
+    except processutils.ProcessExecutionError:
+        return False
+
+    lines = stdout.splitlines()
+    for line in lines:
+        partition = line.split(':')
+        try:
+            # Find the bootable device, and check the base
+            # device and partition for bootloader contents.
+            if 'boot' in partition[6]:
+                if (_has_boot_sector(dev)
+                    or _has_boot_sector(partition[0])):
+                    return True
+        except IndexError:
+            continue
+    return False
+
+
 def _install_grub2(device, root_uuid, efi_system_part_uuid=None,
                    prep_boot_part_uuid=None):
     """Install GRUB2 bootloader on a given device."""
@@ -143,6 +188,19 @@ def _install_grub2(device, root_uuid, efi_system_part_uuid=None,
     efi_partition_mount_point = None
     efi_mounted = False
 
+    # If the root device is an md device (or partition), restart the device
+    # (to help grub finding it) and identify the underlying holder disks
+    # to install grub.
+    if hardware.is_md_device(device):
+        hardware.md_restart(device)
+    elif (_is_bootloader_loaded(device)
+          and not (efi_system_part_uuid
+                   or prep_boot_part_uuid)):
+        # We always need to put the bootloader in place with software raid
+        # so it is okay to elif into the skip doing a bootloader step.
+        LOG.info("Skipping installation of bootloader on device %s "
+                 "as it is already marked bootable.", device)
+        return
     try:
         # Mount the partition and binds
         path = tempfile.mkdtemp()
@@ -155,11 +213,9 @@ def _install_grub2(device, root_uuid, efi_system_part_uuid=None,
         if prep_boot_part_uuid:
             device = _get_partition(device, uuid=prep_boot_part_uuid)
 
-        # If the root device is an md device (or partition), restart the device
+        # If the root device is an md device (or partition),
-        # (to help grub finding it) and identify the underlying holder disks
+        # identify the underlying holder disks to install grub.
-        # to install grub.
         if hardware.is_md_device(device):
-            hardware.md_restart(device)
             disks = hardware.get_holder_disks(device)
         else:
             disks = [device]

ironic_python_agent/tests/unit/extensions/test_image.py

@@ -91,6 +91,7 @@ class TestImageExtension(base.IronicAgentTest):
             prep_boot_part_uuid=self.fake_prep_boot_part_uuid)
         mock_iscsi_clean.assert_called_once_with(self.fake_dev)
 
+    @mock.patch.object(image, '_is_bootloader_loaded', lambda *_: False)
     @mock.patch.object(hardware, 'is_md_device', autospec=True)
     @mock.patch.object(hardware, 'md_get_raid_devices', autospec=True)
     @mock.patch.object(os, 'environ', autospec=True)
@@ -100,7 +101,7 @@ class TestImageExtension(base.IronicAgentTest):
                             mock_execute, mock_dispatch):
         mock_get_part_uuid.return_value = self.fake_root_part
         environ_mock.get.return_value = '/sbin'
-        mock_is_md_device.side_effect = [False, False]
+        mock_is_md_device.return_value = False
         mock_md_get_raid_devices.return_value = {}
         image._install_grub2(self.fake_dev, self.fake_root_uuid)
 
@@ -137,6 +138,7 @@ class TestImageExtension(base.IronicAgentTest):
                                                    uuid=self.fake_root_uuid)
         self.assertFalse(mock_dispatch.called)
 
+    @mock.patch.object(image, '_is_bootloader_loaded', lambda *_: False)
     @mock.patch.object(hardware, 'is_md_device', autospec=True)
     @mock.patch.object(hardware, 'md_get_raid_devices', autospec=True)
     @mock.patch.object(os, 'environ', autospec=True)
@@ -147,9 +149,8 @@ class TestImageExtension(base.IronicAgentTest):
         mock_get_part_uuid.side_effect = [self.fake_root_part,
                                           self.fake_prep_boot_part]
         environ_mock.get.return_value = '/sbin'
-        mock_is_md_device.side_effect = [False, False]
+        mock_is_md_device.return_value = False
         mock_md_get_raid_devices.return_value = {}
-
         image._install_grub2(self.fake_dev, self.fake_root_uuid,
                              prep_boot_part_uuid=self.fake_prep_boot_part_uuid)
 
@@ -189,6 +190,7 @@ class TestImageExtension(base.IronicAgentTest):
                                            uuid=self.fake_prep_boot_part_uuid)
         self.assertFalse(mock_dispatch.called)
 
+    @mock.patch.object(image, '_is_bootloader_loaded', lambda *_: True)
     @mock.patch.object(hardware, 'is_md_device', autospec=True)
     @mock.patch.object(hardware, 'md_get_raid_devices', autospec=True)
     @mock.patch.object(os, 'environ', autospec=True)
@@ -252,6 +254,7 @@ class TestImageExtension(base.IronicAgentTest):
                                            uuid=self.fake_efi_system_part_uuid)
         self.assertFalse(mock_dispatch.called)
 
+    @mock.patch.object(image, '_is_bootloader_loaded', lambda *_: False)
     @mock.patch.object(hardware, 'is_md_device', autospec=True)
     @mock.patch.object(hardware, 'md_get_raid_devices', autospec=True)
     @mock.patch.object(os, 'environ', autospec=True)
@@ -305,6 +308,7 @@ class TestImageExtension(base.IronicAgentTest):
                               attempts=3, delay_on_retry=True)]
         mock_execute.assert_has_calls(expected)
 
+    @mock.patch.object(image, '_is_bootloader_loaded', lambda *_: False)
     @mock.patch.object(hardware, 'is_md_device', autospec=True)
     @mock.patch.object(hardware, 'md_get_raid_devices', autospec=True)
     @mock.patch.object(os, 'environ', autospec=True)
@@ -342,6 +346,7 @@ class TestImageExtension(base.IronicAgentTest):
                               delay_on_retry=True)]
         mock_execute.assert_has_calls(expected)
 
+    @mock.patch.object(image, '_is_bootloader_loaded', lambda *_: False)
     @mock.patch.object(image, '_get_partition', autospec=True)
     def test__install_grub2_command_fail(self, mock_get_part_uuid,
                                          mock_execute,
@@ -356,9 +361,11 @@ class TestImageExtension(base.IronicAgentTest):
                                                    uuid=self.fake_root_uuid)
         self.assertFalse(mock_dispatch.called)
 
+    @mock.patch.object(image, '_is_bootloader_loaded', autospec=True)
     @mock.patch.object(hardware, 'is_md_device', autospec=True)
-    def test__get_partition(self, mock_is_md_device, mock_execute,
+    def test__get_partition(self, mock_is_md_device, mock_is_bootloader,
-                            mock_dispatch):
+                            mock_execute, mock_dispatch):
+        mock_is_md_device.side_effect = [False]
         mock_is_md_device.side_effect = [False, False]
         lsblk_output = ('''KNAME="test" UUID="" TYPE="disk"
         KNAME="test1" UUID="256a39e3-ca3c-4fb8-9cc2-b32eec441f47" TYPE="part"
@@ -374,6 +381,7 @@ class TestImageExtension(base.IronicAgentTest):
                               self.fake_dev)]
         mock_execute.assert_has_calls(expected)
         self.assertFalse(mock_dispatch.called)
+        self.assertFalse(mock_is_bootloader.called)
 
     @mock.patch.object(hardware, 'is_md_device', autospec=True)
     def test__get_partition_no_device_found(self, mock_is_md_device,
@@ -459,3 +467,59 @@ class TestImageExtension(base.IronicAgentTest):
                               self.fake_dev)]
         mock_execute.assert_has_calls(expected)
         self.assertFalse(mock_dispatch.called)
+
+    def test__is_bootloader_loaded(self, mock_execute,
+                                   mock_dispatch):
+        parted_output = ('BYT;\n'
+                         '/dev/loop1:46.1MB:loopback:512:512:gpt:Loopback '
+                         'device:;\n'
+                         '15:1049kB:9437kB:8389kB:::boot;\n'
+                         '1:9437kB:46.1MB:36.7MB:ext3::;\n')
+        disk_file_output = ('/dev/loop1: partition 1: ID=0xee, starthead 0, '
+                            'startsector 1, 90111 sectors, extended '
+                            'partition table (last)\011, code offset 0x48')
+
+        part_file_output = ('/dev/loop1p15: x86 boot sector, mkdosfs boot '
+                            'message display, code offset 0x3c, OEM-ID '
+                            '"mkfs.fat", sectors/cluster 8, root entries '
+                            '512, sectors 16384 (volumes <=32 MB) , Media '
+                            'descriptor 0xf8, sectors/FAT 8, heads 255, '
+                            'serial number 0x23a08feb, unlabeled, '
+                            'FAT (12 bit)')
+
+        # NOTE(TheJulia): File evaluates this data, so it is pointless to
+        # try and embed the raw bytes in the test.
+        dd_output = ('')
+
+        file_output = ('/dev/loop1: DOS executable (COM)\n')
+
+        mock_execute.side_effect = iter([(parted_output, ''),
+                                         (disk_file_output, ''),
+                                         (part_file_output, ''),
+                                         (dd_output, ''),
+                                         (file_output, '')])
+
+        result = image._is_bootloader_loaded(self.fake_dev)
+        self.assertTrue(result)
+
+    def test__is_bootloader_loaded_not_bootable(self,
+                                                mock_execute,
+                                                mock_dispatch):
+        parted_output = ('BYT;\n'
+                         '/dev/loop1:46.1MB:loopback:512:512:gpt:Loopback '
+                         'device:;\n'
+                         '15:1049kB:9437kB:8389kB:::;\n'
+                         '1:9437kB:46.1MB:36.7MB:ext3::;\n')
+        mock_execute.return_value = (parted_output, '')
+        result = image._is_bootloader_loaded(self.fake_dev)
+        self.assertFalse(result)
+
+    def test__is_bootloader_loaded_empty(self,
+                                         mock_execute,
+                                         mock_dispatch):
+        parted_output = ('BYT;\n'
+                         '/dev/loop1:46.1MB:loopback:512:512:gpt:Loopback '
+                         'device:;\n')
+        mock_execute.return_value = (parted_output, '')
+        result = image._is_bootloader_loaded(self.fake_dev)
+        self.assertFalse(result)

releasenotes/notes/skips-bootloader-install-35c463195aa61800.yaml

@@ -0,0 +1,8 @@
+---
+fixes:
+  - |
+    Fixes an issue where wholedisk images are requested for deployment and
+    the bootloader is overridden. IPA now explicitly looks for the boot
+    partition, and examines the contents if the disk appears to be MBR
+    bootable. If override/skip bootloader installation does not apply if
+    UEFI or PREP boot partitions are present on the disk.