Merge "Add fstab pointer to EFI partition" into stable/victoria

ironic_python_agent/extensions/image.py

@@ -576,6 +576,7 @@ def _install_grub2(device, root_uuid, efi_system_part_uuid=None,
                 device, path, efi_system_part_uuid,
                 efi_partitions, efi_partition_mount_point)
             if efi_preserved:
+                _append_uefi_to_fstab(path, efi_system_part_uuid)
                 # Success preserving efi assets
                 return
             else:
@@ -678,6 +679,9 @@ def _install_grub2(device, root_uuid, efi_system_part_uuid=None,
         # recommended path.
         _configure_grub(device, path)
 
+        if efi_mounted:
+            _append_uefi_to_fstab(path, efi_system_part_uuid)
+
         LOG.info("GRUB2 successfully installed on %s", device)
 
     except processutils.ProcessExecutionError as e:
@@ -825,6 +829,27 @@ def _try_preserve_efi_assets(device, path,
                           'filesystem. Error: %s', e)
 
 
+def _append_uefi_to_fstab(fs_path, efi_system_part_uuid):
+    """Append the efi partition id to the filesystem table.
+
+    :param fs_path:
+    :param efi_system_part_uuid:
+    """
+    fstab_file = os.path.join(fs_path, 'etc/fstab')
+    if not os.path.exists(fstab_file):
+        return
+    try:
+        fstab_string = ("UUID=%s\t/boot/efi\tvfat\tumask=0077\t"
+                        "0\t1\n") % efi_system_part_uuid
+        with open(fstab_file, "r+") as fstab:
+            if efi_system_part_uuid not in fstab.read():
+                fstab.writelines(fstab_string)
+    except (OSError, EnvironmentError, IOError) as exc:
+        LOG.debug('Failed to add entry to /etc/fstab. Error %s', exc)
+    LOG.debug('Added entry to /etc/fstab for EFI partition auto-mount '
+              'with uuid %s', efi_system_part_uuid)
+
+
 def _efi_boot_setup(device, efi_system_part_uuid=None, target_boot_mode=None):
     """Identify and setup an EFI bootloader from supplied partition/disk.
 

ironic_python_agent/tests/unit/extensions/test_image.py

@@ -503,13 +503,15 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
         mock_execute.assert_has_calls(expected)
 
     @mock.patch.object(image, '_is_bootloader_loaded', lambda *_: False)
+    @mock.patch.object(image, '_append_uefi_to_fstab', 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(image, '_get_partition', autospec=True)
     def test__install_grub2(self, mock_get_part_uuid, environ_mock,
                             mock_md_get_raid_devices, mock_is_md_device,
-                            mock_execute, mock_dispatch):
+                            mock_append_to_fstab, mock_execute,
+                            mock_dispatch):
         mock_get_part_uuid.return_value = self.fake_root_part
         environ_mock.get.return_value = '/sbin'
         mock_is_md_device.return_value = False
@@ -561,6 +563,7 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
         mock_get_part_uuid.assert_called_once_with(self.fake_dev,
                                                    uuid=self.fake_root_uuid)
         self.assertFalse(mock_dispatch.called)
+        self.assertFalse(mock_append_to_fstab.called)
 
     @mock.patch.object(image, '_is_bootloader_loaded', lambda *_: False)
     @mock.patch.object(hardware, 'is_md_device', autospec=True)
@@ -630,6 +633,7 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
     @mock.patch.object(os.path, 'ismount', lambda *_: True)
     @mock.patch.object(os.path, 'exists', lambda *_: False)
     @mock.patch.object(image, '_is_bootloader_loaded', lambda *_: True)
+    @mock.patch.object(image, '_append_uefi_to_fstab', 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)
@@ -637,8 +641,8 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
     @mock.patch.object(image, '_get_partition', autospec=True)
     def test__install_grub2_uefi(self, mock_get_part_uuid, mkdir_mock,
                                  environ_mock, mock_md_get_raid_devices,
-                                 mock_is_md_device, mock_execute,
-                                 mock_dispatch):
+                                 mock_is_md_device, mock_append_to_fstab,
+                                 mock_execute, mock_dispatch):
         mock_get_part_uuid.side_effect = [self.fake_root_part,
                                           self.fake_efi_system_part]
         environ_mock.get.return_value = '/sbin'
@@ -711,10 +715,218 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
         mock_get_part_uuid.assert_any_call(self.fake_dev,
                                            uuid=self.fake_efi_system_part_uuid)
         self.assertFalse(mock_dispatch.called)
+        mock_append_to_fstab.assert_called_with(self.fake_dir,
+                                                self.fake_efi_system_part_uuid)
+
+    @mock.patch.object(os.path, 'ismount', lambda *_: True)
+    @mock.patch.object(image, '_is_bootloader_loaded', lambda *_: 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_fstab(self, mock_get_part_uuid, mkdir_mock,
+                                       environ_mock, mock_md_get_raid_devices,
+                                       mock_is_md_device, mock_exists,
+                                       mock_execute, mock_dispatch):
+        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_exists.side_effect = iter([False, True, False, True, True])
+        with mock.patch('builtins.open', mock.mock_open()) as mock_open:
+            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')
+            write_calls = [
+                mock.call(self.fake_dir + '/etc/fstab', 'r+'),
+                mock.call().__enter__(),
+                mock.call().read(),
+                mock.call().writelines('UUID=%s\t/boot/efi\tvfat\t'
+                                       'umask=0077\t0\t1'
+                                       '\n' % self.fake_efi_system_part_uuid),
+                mock.call().__exit__(None, None, None)]
+            mock_open.assert_has_calls(write_calls)
+
+        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 "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_called_once_with(self.fake_dir + '/boot/efi')
+        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.patch.object(image, '_efi_boot_setup', lambda *_: False)
+    @mock.patch.object(os.path, 'ismount', lambda *_: True)
+    @mock.patch.object(image, '_is_bootloader_loaded', lambda *_: 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_no_fstab(
+            self, mock_get_part_uuid,
+            mkdir_mock,
+            environ_mock, mock_md_get_raid_devices,
+            mock_is_md_device, mock_exists,
+            mock_execute, mock_dispatch):
+        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 = {}
+        fstab_data = (
+            'UUID=%s\tpath vfat option' % self.fake_efi_system_part_uuid)
+        mock_exists.side_effect = [True, False, True, True, True, False,
+                                   True, True]
+        with mock.patch('builtins.open',
+                        mock.mock_open(read_data=fstab_data)) as mock_open:
+            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')
+            write_calls = [
+                mock.call(self.fake_dir + '/etc/fstab', 'r+'),
+                mock.call().__enter__(),
+                mock.call().read(),
+                mock.call().__exit__(None, None, None)]
+            mock_open.assert_has_calls(write_calls)
+
+        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('umount', self.fake_dir + '/boot/efi'),
+                    # NOTE(TheJulia): chroot mount is for whole disk images
+                    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_called_once_with(self.fake_dir + '/boot/efi')
+        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.patch.object(os.path, 'ismount', lambda *_: False)
     @mock.patch.object(os, 'listdir', lambda *_: ['file1', 'file2'])
     @mock.patch.object(image, '_is_bootloader_loaded', lambda *_: False)
+    @mock.patch.object(image, '_append_uefi_to_fstab', autospec=True)
     @mock.patch.object(image, '_efi_boot_setup', autospec=True)
     @mock.patch.object(shutil, 'copytree', autospec=True)
     @mock.patch.object(os.path, 'exists', autospec=True)
@@ -728,8 +940,10 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
             environ_mock, mock_md_get_raid_devices,
             mock_is_md_device, mock_exists,
             mock_copytree, mock_efi_setup,
-            mock_execute, mock_dispatch):
-        mock_exists.return_value = True
+            mock_append_to_fstab, mock_execute,
+            mock_dispatch):
+        mock_exists.side_effect = [True, False, True, True, True, False, True,
+                                   False, False]
         mock_efi_setup.return_value = True
         mock_get_part_uuid.side_effect = [self.fake_root_part,
                                           self.fake_efi_system_part]
@@ -790,6 +1004,8 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
         mock_get_part_uuid.assert_any_call(self.fake_dev,
                                            uuid=self.fake_efi_system_part_uuid)
         self.assertFalse(mock_dispatch.called)
+        mock_append_to_fstab.assert_called_with(self.fake_dir,
+                                                self.fake_efi_system_part_uuid)
 
     @mock.patch.object(os, 'listdir', lambda *_: ['file1', 'file2'])
     @mock.patch.object(image, '_is_bootloader_loaded', lambda *_: False)
@@ -877,6 +1093,7 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
 
     @mock.patch.object(os.path, 'ismount', lambda *_: True)
     @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)
@@ -891,6 +1108,7 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
             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')
@@ -983,6 +1201,8 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
             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)
@@ -1076,6 +1296,7 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
     @mock.patch.object(os.path, 'ismount', lambda *_: True)
     @mock.patch.object(image, '_is_bootloader_loaded', lambda *_: False)
     @mock.patch.object(os, 'listdir', autospec=True)
+    @mock.patch.object(image, '_append_uefi_to_fstab', autospec=True)
     @mock.patch.object(image, '_efi_boot_setup', autospec=True)
     @mock.patch.object(shutil, 'copytree', autospec=True)
     @mock.patch.object(os.path, 'exists', autospec=True)
@@ -1089,8 +1310,8 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
             environ_mock, mock_md_get_raid_devices,
             mock_is_md_device, mock_exists,
             mock_copytree, mock_efi_setup,
-            mock_oslistdir, mock_execute,
-            mock_dispatch):
+            mock_append_to_fstab, mock_oslistdir,
+            mock_execute, mock_dispatch):
         mock_exists.side_effect = [True, False, False, True, True, True, True]
         mock_efi_setup.side_effect = Exception('meow')
         mock_oslistdir.return_value = ['file1']
@@ -1173,6 +1394,8 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
         mock_get_part_uuid.assert_any_call(self.fake_dev,
                                            uuid=self.fake_efi_system_part_uuid)
         self.assertFalse(mock_dispatch.called)
+        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(hardware, 'is_md_device', autospec=True)
@@ -2031,3 +2254,11 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
                               '\\EFI\\BOOT\\BOOTX64.EFI')]
         self.assertIsNone(result)
         mock_execute.assert_has_calls(expected)
+
+    @mock.patch.object(os.path, 'exists', lambda *_: True)
+    def test__append_uefi_to_fstab_handles_error(self, mock_execute,
+                                                 mock_dispatch):
+        with mock.patch('builtins.open', mock.mock_open()) as mock_open:
+            mock_open.side_effect = OSError('boom')
+            image._append_uefi_to_fstab(
+                self.fake_dir, 'abcd-efgh')

releasenotes/notes/append-efi-partition-to-fstab-e9f945a4dd19bd7a.yaml

@@ -0,0 +1,8 @@
+---
+fixes:
+  - |
+    The system file system configuration file for Linux machines, the
+    ``/etc/fstab`` file is now updated to include a reference to the
+    EFI partition in the case of a partition image base deployment.
+    Without this reference, images deployed using partition images
+    could end up in situations where upgrading the bootloader could fail.