Unmount config drives

If this seems like deja vu, that is because it is. We had this
very same issue with the original CoreOS ramdisk. Since we don't
control the whole OS of the ramdisk, it only made sense to teach
the agent to umount the folder.

The folder is referenced already, and the agent does have safeguards
in place, but unfortunately this issue led to a rebuild breaking where
cloud-init, glean, and the agent were all trying do the right thing
as they thought, and there were just multiple /mnt/config folders
present in the OS. These are separate issues we also need to try and
remedy.

What happens is when the device is locked via a mount, the partition
table is never updated to the running OS as the mount creates a lock.
So the agent ends up thinking, in the case of a rebuild, that everything
including creating a configuration drive on that device has been
successful, but when you reboot, there is no partition table entry
for the new partition as the change was not successfully written.
This state prevented the workload from rebooting properly.
This change eliminates that possibility moving forward by attempting
to ensure that the cloud configuration folder is no longer mounted.

Depends-On: https://review.opendev.org/c/openstack/ironic/+/918118
Change-Id: I4399dd0934361003cca9ff95a7e3e3ae9bba3dab
(cherry picked from commit 6ac3f35)
(cherry picked from commit 0337474)

Author: juliakreger

ironic_python_agent/tests/unit/test_utils.py

@@ -22,6 +22,7 @@
 import subprocess
 import tarfile
 import tempfile
+import time
 from unittest import mock
 
 from ironic_lib import utils as ironic_utils
@@ -877,6 +878,7 @@ def test_sync_clock_ntp_server_is_none(self, mock_time_method,
         self.assertEqual(0, mock_execute.call_count)
 
 
+@mock.patch.object(utils, '_unmount_any_config_drives', autospec=True)
 @mock.patch.object(utils, '_booted_from_vmedia', autospec=True)
 @mock.patch.object(utils, '_check_vmedia_device', autospec=True)
 @mock.patch.object(utils, '_find_vmedia_device_by_labels', autospec=True)
@@ -887,7 +889,8 @@ class TestCopyConfigFromVmedia(testtools.TestCase):
 
     def test_vmedia_found_not_booted_from_vmedia(
             self, mock_execute, mock_mount, mock_copy,
-            mock_find_device, mock_check_vmedia, mock_booted_from_vmedia):
+            mock_find_device, mock_check_vmedia, mock_booted_from_vmedia,
+            mock_unmount_config):
         mock_booted_from_vmedia.return_value = False
         mock_find_device.return_value = '/dev/fake'
         utils.copy_config_from_vmedia()
@@ -896,10 +899,12 @@ def test_vmedia_found_not_booted_from_vmedia(
         mock_copy.assert_not_called()
         mock_check_vmedia.assert_not_called()
         self.assertTrue(mock_booted_from_vmedia.called)
+        self.assertTrue(mock_unmount_config.called)
 
     def test_no_vmedia(
             self, mock_execute, mock_mount, mock_copy,
-            mock_find_device, mock_check_vmedia, mock_booted_from_vmedia):
+            mock_find_device, mock_check_vmedia, mock_booted_from_vmedia,
+            mock_unmount_config):
         mock_booted_from_vmedia.return_value = True
         mock_find_device.return_value = None
         utils.copy_config_from_vmedia()
@@ -908,10 +913,12 @@ def test_no_vmedia(
         mock_copy.assert_not_called()
         mock_check_vmedia.assert_not_called()
         self.assertFalse(mock_booted_from_vmedia.called)
+        self.assertTrue(mock_unmount_config.called)
 
     def test_no_files(
             self, mock_execute, mock_mount, mock_copy,
-            mock_find_device, mock_check_vmedia, mock_booted_from_vmedia):
+            mock_find_device, mock_check_vmedia, mock_booted_from_vmedia,
+            mock_unmount_config):
         mock_booted_from_vmedia.return_value = True
         temp_path = tempfile.mkdtemp()
         self.addCleanup(lambda: shutil.rmtree(temp_path))
@@ -925,10 +932,12 @@ def test_no_files(
                                              '/dev/something')
         mock_copy.assert_not_called()
         self.assertTrue(mock_booted_from_vmedia.called)
+        self.assertTrue(mock_unmount_config.called)
 
     def test_mounted_no_files(
             self, mock_execute, mock_mount, mock_copy,
-            mock_find_device, mock_check_vmedia, mock_booted_from_vmedia):
+            mock_find_device, mock_check_vmedia, mock_booted_from_vmedia,
+            mock_unmount_config):
 
         mock_booted_from_vmedia.return_value = True
         mock_execute.return_value = '/some/path', ''
@@ -939,11 +948,13 @@ def test_mounted_no_files(
         mock_copy.assert_not_called()
         mock_mount.assert_not_called()
         self.assertTrue(mock_booted_from_vmedia.called)
+        self.assertTrue(mock_unmount_config.called)
 
     @mock.patch.object(os, 'makedirs', autospec=True)
     def test_copy(
             self, mock_makedirs, mock_execute, mock_mount, mock_copy,
-            mock_find_device, mock_check_vmedia, mock_booted_from_vmedia):
+            mock_find_device, mock_check_vmedia, mock_booted_from_vmedia,
+            mock_unmount_config):
 
         mock_booted_from_vmedia.return_value = True
         mock_find_device.return_value = '/dev/something'
@@ -982,12 +993,14 @@ def _fake_mount(dev):
             mock.call(mock.ANY, '/etc/ironic-python-agent.d/ironic.conf'),
         ], any_order=True)
         self.assertTrue(mock_booted_from_vmedia.called)
+        self.assertTrue(mock_unmount_config.called)
 
     @mock.patch.object(os, 'makedirs', autospec=True)
     def test_copy_mounted(
             self, mock_makedirs, mock_execute, mock_mount,
             mock_copy, mock_find_device, mock_check_vmedia,
-            mock_booted_from_vmedia):
+            mock_booted_from_vmedia,
+            mock_unmount_config):
         mock_booted_from_vmedia.return_value = True
         mock_find_device.return_value = '/dev/something'
         path = tempfile.mkdtemp()
@@ -1023,6 +1036,7 @@ def test_copy_mounted(
         ], any_order=True)
         mock_mount.assert_not_called()
         self.assertTrue(mock_booted_from_vmedia.called)
+        self.assertTrue(mock_unmount_config.called)
 
 
 @mock.patch.object(requests, 'get', autospec=True)
@@ -1143,3 +1157,20 @@ def test_early_logging_goes_to_logger(self, mock_log):
         expected_calls = [mock.call('Early logging: %s', 'line 1.'),
                           mock.call('Early logging: %s', 'line 2 message')]
         info.assert_has_calls(expected_calls)
+
+
+class TestUnmountOfConfig(ironic_agent_base.IronicAgentTest):
+
+    @mock.patch.object(utils, '_early_log', autospec=True)
+    @mock.patch.object(os.path, 'ismount', autospec=True)
+    @mock.patch.object(utils, 'execute', autospec=True)
+    @mock.patch.object(time, 'sleep', autospec=True)
+    def test__unmount_any_config_drives(self, mock_sleep, mock_exec,
+                                        mock_ismount, mock_log,):
+        mock_ismount.side_effect = iter([True, True, False])
+        utils._unmount_any_config_drives()
+        self.assertEqual(2, mock_sleep.call_count)
+        self.assertEqual(2, mock_log.call_count)
+        mock_exec.assert_has_calls([
+            mock.call('umount', '/mnt/config'),
+            mock.call('umount', '/mnt/config')])

ironic_python_agent/utils.py

@@ -311,18 +311,27 @@ def copy_config_from_vmedia():
         ['config-2', 'vmedia_boot_iso'])
     if not vmedia_device_file:
         _early_log('No virtual media device detected')
+        _unmount_any_config_drives()
         return
     if not _booted_from_vmedia():
         _early_log('Cannot use configuration from virtual media as the '
                    'agent was not booted from virtual media.')
+        _unmount_any_config_drives()
         return
     # Determine the device
     mounted = _find_mount_point(vmedia_device_file)
     if mounted:
+        # In this case a utility like a configuration drive tool
+        # has *already* mounted the device we believe to be the
+        # configuration drive.
         _copy_config_from(mounted)
     else:
         with ironic_utils.mounted(vmedia_device_file) as vmedia_mount_point:
+            # In this case, we use a temporary folder and extract the contents
+            # for our configuration.
             _copy_config_from(vmedia_mount_point)
+    # As a last act, just make sure there is nothing else mounted.
+    _unmount_any_config_drives()
 
 
 def _get_cached_params():
@@ -917,3 +926,22 @@ def rescan_device(device):
     except processutils.ProcessExecutionError as e:
         LOG.warning('Something went wrong when waiting for udev '
                     'to settle. Error: %s', e)
+
+
+def _unmount_any_config_drives():
+    """Umount anything mounted to /mnt/config
+
+    As part of the configuration drive model, utilities like cloud-init
+    and glean leverage a folder at /mnt/config to convey configuration
+    to a booting OS.
+
+    The possibility exists that one of the utilties mounted one or multiple
+    such folders, even if the configuration was not used, and this can
+    result in locked devices which can prevent rebuild operations from
+    completing successfully as as long as the folder is mounted, it is
+    a "locked" device to the operating system.
+    """
+    while os.path.ismount('/mnt/config'):
+        _early_log('Issuing an umount command for /mnt/config...')
+        execute('umount', '/mnt/config')
+        time.sleep(1)

releasenotes/notes/always-make-sure-config-drive-is-umounted-a3985bbb45e89051.yaml

@@ -0,0 +1,8 @@
+---
+fixes:
+  - |
+    Fixes an issue where configuration drive volumes which are mounted
+    by the operating system could remain mounted and cause a lock to be
+    held, which may conflict with actions such as ``rebuild``.
+    The agent now always makes sure the folder used by Glean and Cloud-init
+    is not mounted.