Catch ismount not being handled

While investigating another grub issue, I was confused by the path
taken in the logs reported, and noticed that on a ramdisk, we might
not actually have a valid response to os.path.ismount, I'm guessing
depending on what in memory filesystem is in use while also coupled
with attempting to check a filesystem.

Adds a test to validate that exceptions raised on these commands
where this issue can be encountered, are properly bypassed, and also
adds additional logging to make it easier to figure out what is
going on in the entire bootloader setup sequence.

Change-Id: Ibd3060bef2e56468ada6b1a5c1cc1632a42803c3
(cherry picked from commit e5d552474b21137ae2a66f17bdab5fc1bbf31ec6)

2 files changed, 152 insertions, 7 deletions

diff --git a/ironic_python_agent/extensions/image.py b/ironic_python_agent/extensions/image.py
index 5224cb77..f5a0142d 100644
--- a/ironic_python_agent/extensions/image.py
+++ b/ironic_python_agent/extensions/image.py
@@ -282,6 +282,8 @@ def _run_efibootmgr(valid_efi_bootloaders, device, efi_partition,
     label_id = 1
     for v_bl in valid_efi_bootloaders:
         if 'csv' in v_bl.lower():
+            LOG.debug('A CSV file has been identified as a bootloader hint. '
+                      'File: %s', v_bl)
             # These files are always UTF-16 encoded, sometimes have a header.
             # Positive bonus is python silently drops the FEFF header.
             with open(mount_point + '/' + v_bl, 'r', encoding='utf-16') as csv:
@@ -366,6 +368,7 @@ def _manage_uefi(device, efi_system_part_uuid=None):
     efi_mounted = False
     efi_partition = None
 
+    LOG.debug('Attempting UEFI loader autodetection and NVRAM record setup.')
     try:
         # Force UEFI to rescan the device. Required if the deployment
         # was over iscsi.
@@ -421,6 +424,7 @@ def _manage_uefi(device, efi_system_part_uuid=None):
         else:
             # NOTE(dtantsur): if we have an empty EFI partition, try to use
             # grub-install to populate it.
+            LOG.warning('Empty EFI partition detected.')
             return False
 
     except processutils.ProcessExecutionError as e:
@@ -591,6 +595,29 @@ def _umount_all_partitions(path, path_variable, umount_warn_msg):
     return umount_binds_success
 
 
+def _mount_partition(partition, path):
+    if not os.path.ismount(path):
+        LOG.debug('Attempting to mount %(device)s to %(path)s to '
+                  'partition.',
+                  {'device': partition,
+                   'path': path})
+        try:
+            utils.execute('mount', partition, path)
+        except processutils.ProcessExecutionError as e:
+            # NOTE(TheJulia): It seems in some cases,
+            # the python os.path.ismount can return False
+            # even *if* it is actually mounted. This appears
+            # to be becasue it tries to rely on inode on device
+            # logic, yet the rules are sometimes different inside
+            # ramdisks. So lets check the error first.
+            if 'already mounted' not in e:
+                # Raise the error, since this is not a known
+                # failure case
+                raise
+            else:
+                LOG.debug('Partition already mounted, proceeding.')
+
+
 def _install_grub2(device, root_uuid, efi_system_part_uuid=None,
                    prep_boot_part_uuid=None, target_boot_mode='bios'):
     """Install GRUB2 bootloader on a given device."""
@@ -678,11 +705,9 @@ def _install_grub2(device, root_uuid, efi_system_part_uuid=None,
                 # remounted.
                 LOG.debug('No EFI assets were preserved for setup or the '
                           'ramdisk was unable to complete the setup. '
-                          'falling back to bootloader installation from'
+                          'falling back to bootloader installation from '
                           'deployed image.')
-                if not os.path.ismount(path):
-                    LOG.debug('Re-mounting the root partition.')
-                    utils.execute('mount', root_partition, path)
+                _mount_partition(root_partition, path)
 
         binary_name = "grub"
         if os.path.exists(os.path.join(path, 'usr/sbin/grub2-install')):
@@ -702,9 +727,8 @@ def _install_grub2(device, root_uuid, efi_system_part_uuid=None,
             LOG.warning("GRUB2 will be installed for UEFI on efi partition "
                         "%s using the install command which does not place "
                         "Secure Boot signed binaries.", efi_partition)
-            if not os.path.ismount(efi_partition_mount_point):
-                utils.execute('mount', efi_partition,
-                              efi_partition_mount_point)
+
+            _mount_partition(efi_partition, efi_partition_mount_point)
             efi_mounted = True
             try:
                 utils.execute('chroot %(path)s /bin/sh -c '
diff --git a/ironic_python_agent/tests/unit/extensions/test_image.py b/ironic_python_agent/tests/unit/extensions/test_image.py
index 4a2fbe74..fbd6f16e 100644
--- a/ironic_python_agent/tests/unit/extensions/test_image.py
+++ b/ironic_python_agent/tests/unit/extensions/test_image.py
@@ -1312,6 +1312,127 @@ Boot0004* ironic1      HD(1,GPT,55db8d03-c8f6-4a5b-9155-790dddc348fa,0x800,0x640
         mock_append_to_fstab.assert_called_with(self.fake_dir,
                                                 self.fake_efi_system_part_uuid)
 
+    @mock.patch.object(os.path, 'ismount', lambda *_: False)
+    @mock.patch.object(image, '_is_bootloader_loaded', lambda *_: False)
+    @mock.patch.object(image, '_append_uefi_to_fstab', autospec=True)
+    @mock.patch.object(image, '_preserve_efi_assets', autospec=True)
+    @mock.patch.object(image, '_efi_boot_setup', autospec=True)
+    @mock.patch.object(os.path, 'exists', autospec=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)
+    @mock.patch.object(os, 'makedirs', autospec=True)
+    @mock.patch.object(image, '_get_partition', autospec=True)
+    def test__install_grub2_uefi_partition_image_with_preserve_failure2(
+            self, mock_get_part_uuid, mkdir_mock,
+            environ_mock, mock_md_get_raid_devices,
+            mock_is_md_device, mock_exists,
+            mock_efi_setup,
+            mock_preserve_efi_assets,
+            mock_append_to_fstab,
+            mock_execute, mock_dispatch):
+        mock_exists.return_value = True
+        mock_efi_setup.side_effect = Exception('meow')
+        mock_get_part_uuid.side_effect = [self.fake_root_part,
+                                          self.fake_efi_system_part]
+        environ_mock.get.return_value = '/sbin'
+        mock_is_md_device.return_value = False
+        mock_md_get_raid_devices.return_value = {}
+        mock_preserve_efi_assets.return_value = None
+        exec_results = [('', '')] * 21
+        already_exists = processutils.ProcessExecutionError(
+            '/dev is already mounted at /path')
+        # Mark mounts as already mounted, which is where os.path.ismount
+        # usage corresponds.
+        exec_results[6] = already_exists
+        exec_results[8] = already_exists
+
+        image._install_grub2(
+            self.fake_dev, root_uuid=self.fake_root_uuid,
+            efi_system_part_uuid=self.fake_efi_system_part_uuid,
+            target_boot_mode='uefi')
+        self.assertFalse(mock_efi_setup.called)
+
+        expected = [mock.call('mount', '/dev/fake2', self.fake_dir),
+                    mock.call('mount', '-o', 'bind', '/dev',
+                              self.fake_dir + '/dev'),
+                    mock.call('mount', '-o', 'bind', '/proc',
+                              self.fake_dir + '/proc'),
+                    mock.call('mount', '-o', 'bind', '/run',
+                              self.fake_dir + '/run'),
+                    mock.call('mount', '-t', 'sysfs', 'none',
+                              self.fake_dir + '/sys'),
+                    mock.call(('chroot %s /bin/sh -c '
+                               '"grub2-mkconfig -o '
+                               '/boot/grub2/grub.cfg"' % self.fake_dir),
+                              shell=True,
+                              env_variables={
+                                  'PATH': '/sbin:/bin:/usr/sbin:/sbin',
+                                  'GRUB_DISABLE_OS_PROBER': 'true',
+                                  'GRUB_SAVEDEFAULT': 'true'},
+                              use_standard_locale=True),
+                    mock.call('mount', '/dev/fake2', self.fake_dir),
+                    mock.call(('chroot %s /bin/sh -c "mount -a -t vfat"' %
+                              (self.fake_dir)), shell=True,
+                              env_variables={
+                                  'PATH': '/sbin:/bin:/usr/sbin:/sbin'}),
+                    mock.call('mount', self.fake_efi_system_part,
+                              self.fake_dir + '/boot/efi'),
+                    mock.call(('chroot %s /bin/sh -c "grub2-install"' %
+                               self.fake_dir), shell=True,
+                              env_variables={
+                                  'PATH': '/sbin:/bin:/usr/sbin:/sbin'}),
+                    mock.call(('chroot %s /bin/sh -c '
+                              '"grub2-install --removable"' %
+                               self.fake_dir), shell=True,
+                              env_variables={
+                                  'PATH': '/sbin:/bin:/usr/sbin:/sbin'}),
+                    mock.call(
+                        'umount', self.fake_dir + '/boot/efi',
+                        attempts=3, delay_on_retry=True),
+                    mock.call('mount', self.fake_efi_system_part,
+                              '/tmp/fake-dir/boot/efi'),
+                    mock.call(('chroot %s /bin/sh -c '
+                               '"grub2-mkconfig -o '
+                               '/boot/grub2/grub.cfg"' % self.fake_dir),
+                              shell=True,
+                              env_variables={
+                                  'PATH': '/sbin:/bin:/usr/sbin:/sbin',
+                                  'GRUB_DISABLE_OS_PROBER': 'true',
+                                  'GRUB_SAVEDEFAULT': 'true'},
+                              use_standard_locale=True),
+                    mock.call('umount', self.fake_dir + '/boot/efi',
+                              attempts=3, delay_on_retry=True),
+                    mock.call(('chroot %s /bin/sh -c "umount -a -t vfat"' %
+                              (self.fake_dir)), shell=True,
+                              env_variables={
+                                  'PATH': '/sbin:/bin:/usr/sbin:/sbin'}),
+                    mock.call('umount', self.fake_dir + '/dev',
+                              attempts=3, delay_on_retry=True),
+                    mock.call('umount', self.fake_dir + '/proc',
+                              attempts=3, delay_on_retry=True),
+                    mock.call('umount', self.fake_dir + '/run',
+                              attempts=3, delay_on_retry=True),
+                    mock.call('umount', self.fake_dir + '/sys',
+                              attempts=3, delay_on_retry=True),
+                    mock.call('umount', self.fake_dir, attempts=3,
+                              delay_on_retry=True)]
+
+        mkdir_mock.assert_not_called()
+        mock_execute.assert_has_calls(expected)
+        mock_get_part_uuid.assert_any_call(self.fake_dev,
+                                           uuid=self.fake_root_uuid)
+        mock_get_part_uuid.assert_any_call(self.fake_dev,
+                                           uuid=self.fake_efi_system_part_uuid)
+        self.assertFalse(mock_dispatch.called)
+        mock_preserve_efi_assets.assert_called_with(
+            self.fake_dir,
+            self.fake_dir + '/boot/efi/EFI',
+            '/dev/fake1',
+            self.fake_dir + '/boot/efi')
+        mock_append_to_fstab.assert_called_with(self.fake_dir,
+                                                self.fake_efi_system_part_uuid)
+
     @mock.patch.object(image, '_is_bootloader_loaded', lambda *_: False)
     @mock.patch.object(image, '_append_uefi_to_fstab', autospec=True)
     @mock.patch.object(os, 'listdir', autospec=True)