Don't try to put a bootloader in place when bootable

Lets not do silly things and if the disk looks bootable,
and we're not trying to do UEFI, then let us assume the
proper thing will occur upon power-up.

Looks at the boot sector data and if an executable is
found in the first 218 bytes, then it bypasses loading
a boot loader.

Also adds a dependency on the "file" linux distribution
package.

Change-Id: I11bc26670a08ee13174a43d7cd0f1ab9c1bd35cf
Story: 2006474
Task: 36410

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.