Merge "Catch ismount not being handled"

2 files changed, 153 insertions, 9 deletions

diff --git a/ironic_python_agent/extensions/image.py b/ironic_python_agent/extensions/image.py
index 03fc7636..84e86f17 100644
--- a/ironic_python_agent/extensions/image.py
+++ b/ironic_python_agent/extensions/image.py
@@ -273,7 +273,7 @@ def _run_efibootmgr(valid_efi_bootloaders, device, efi_partition,
     """
 
     # Before updating let's get information about the bootorder
-    LOG.debug("Getting information about boot order")
+    LOG.debug("Getting information about boot order.")
     utils.execute('efibootmgr')
     # NOTE(iurygregory): regex used to identify the Warning in the stderr after
     # we add the new entry. Example:
@@ -283,6 +283,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:
@@ -328,7 +330,7 @@ def _manage_uefi(device, efi_system_part_uuid=None):
     """
     efi_partition_mount_point = None
     efi_mounted = False
-
+    LOG.debug('Attempting UEFI loader autodetection and NVRAM record setup.')
     try:
         # Force UEFI to rescan the device.
         _rescan_device(device)
@@ -381,6 +383,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:
@@ -551,6 +554,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."""
@@ -638,11 +664,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')):
@@ -662,9 +686,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 9661e6f3..05e937b8 100644
--- a/ironic_python_agent/tests/unit/extensions/test_image.py
+++ b/ironic_python_agent/tests/unit/extensions/test_image.py
@@ -1154,6 +1154,127 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
         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(os, 'listdir', autospec=True)
     @mock.patch.object(shutil, 'copy2', autospec=True)