diff --git a/bindep.txt b/bindep.txt index a1e6deab5..03c56614d 100644 --- a/bindep.txt +++ b/bindep.txt @@ -26,3 +26,4 @@ python-pip [imagebuild] unzip [imagebuild] sudo [imagebuild] gawk [imagebuild] +file [imagebuild] diff --git a/ironic_python_agent/extensions/image.py b/ironic_python_agent/extensions/image.py index 9ea4f6f05..61d20c76d 100644 --- a/ironic_python_agent/extensions/image.py +++ b/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 - # (to help grub finding it) and identify the underlying holder disks - # to install grub. + # If the root device is an md device (or partition), + # identify the underlying holder disks to install grub. if hardware.is_md_device(device): - hardware.md_restart(device) disks = hardware.get_holder_disks(device) else: disks = [device] diff --git a/ironic_python_agent/tests/unit/extensions/test_image.py b/ironic_python_agent/tests/unit/extensions/test_image.py index 3b6cf9fbb..aa8022755 100644 --- a/ironic_python_agent/tests/unit/extensions/test_image.py +++ b/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, - mock_dispatch): + def test__get_partition(self, mock_is_md_device, mock_is_bootloader, + 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) diff --git a/releasenotes/notes/skips-bootloader-install-35c463195aa61800.yaml b/releasenotes/notes/skips-bootloader-install-35c463195aa61800.yaml new file mode 100644 index 000000000..128e626b8 --- /dev/null +++ b/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.