Merge "Revert "Fix vmedia network config drive handling""

ironic_python_agent/cmd/agent.py

@@ -30,9 +30,8 @@ def run():
     """Entrypoint for IronicPythonAgent."""
     # NOTE(dtantsur): this must happen very early of the files from
     # /etc/ironic-python-agent.d won't be loaded
-    vmedia_configuration = utils.copy_config_from_vmedia()
+    utils.copy_config_from_vmedia()
-    if vmedia_configuration:
+
-        utils.trigger_glean_network_refresh()
     log.register_options(CONF)
     CONF(args=sys.argv[1:])
     # Debug option comes from oslo.log, allow overriding it via kernel cmdline

ironic_python_agent/tests/unit/test_utils.py

@@ -883,7 +883,7 @@ class TestClockSyncUtils(ironic_agent_base.IronicAgentTest):
 @mock.patch.object(shutil, 'copy', autospec=True)
 @mock.patch.object(ironic_utils, 'mounted', autospec=True)
 @mock.patch.object(utils, 'execute', autospec=True)
-class TestConfigFromVmedia(testtools.TestCase):
+class TestCopyConfigFromVmedia(testtools.TestCase):
 
     def test_vmedia_found_not_booted_from_vmedia(
             self, mock_execute, mock_mount, mock_copy,
@@ -909,107 +909,45 @@ class TestConfigFromVmedia(testtools.TestCase):
         mock_check_vmedia.assert_not_called()
         self.assertFalse(mock_booted_from_vmedia.called)
 
-    @mock.patch.object(os.path, 'ismount', autospec=True)
     def test_no_files(
-            self, mock_ismount, mock_execute, mock_mount, mock_copy,
+            self, mock_execute, mock_mount, mock_copy,
             mock_find_device, mock_check_vmedia, mock_booted_from_vmedia):
-        mock_ismount.return_value = False
         mock_booted_from_vmedia.return_value = True
         temp_path = tempfile.mkdtemp()
         self.addCleanup(lambda: shutil.rmtree(temp_path))
 
-        mock_execute.side_effect = [('/mnt/config', ()),
+        mock_execute.side_effect = processutils.ProcessExecutionError
-                                    ((), ())]
         mock_find_device.return_value = '/dev/something'
         mock_mount.return_value.__enter__.return_value = temp_path
         utils.copy_config_from_vmedia()
         mock_mount.assert_called_once_with('/dev/something')
-        mock_execute.assert_has_calls([
+        mock_execute.assert_called_once_with('findmnt', '-n', '-oTARGET',
-            mock.call('findmnt', '-n', '-oTARGET', '/dev/something'),
+                                             '/dev/something')
-            mock.call('umount', '/mnt/config', run_as_root=True)])
-        self.assertEqual(2, mock_execute.call_count)
         mock_copy.assert_not_called()
         self.assertTrue(mock_booted_from_vmedia.called)
 
-    @mock.patch.object(os.path, 'ismount', autospec=True)
     def test_mounted_no_files(
-            self, mock_ismount, mock_execute, mock_mount, mock_copy,
+            self, mock_execute, mock_mount, mock_copy,
             mock_find_device, mock_check_vmedia, mock_booted_from_vmedia):
 
-        mock_ismount.side_effect = [True, False]
         mock_booted_from_vmedia.return_value = True
-        mock_execute.side_effect = [('/some/path', ''),
+        mock_execute.return_value = '/some/path', ''
-                                    ((), ()),
-                                    ((), ())]
-        mock_find_device.side_effect = ('/dev/something', '')
         mock_find_device.return_value = '/dev/something'
         utils.copy_config_from_vmedia()
-        mock_execute.assert_has_calls([
-            mock.call('findmnt', '-n', '-oTARGET', '/dev/something'),
-            mock.call('umount', '/some/path', run_as_root=True),
-            mock.call('umount', '/mnt/config', run_as_root=True)])
-        self.assertEqual(3, mock_execute.call_count)
-        mock_copy.assert_not_called()
-        mock_mount.assert_called_once_with('/dev/something')
-        self.assertTrue(mock_booted_from_vmedia.called)
-
-    @mock.patch.object(os.path, 'ismount', autospec=True)
-    @mock.patch.object(os, 'makedirs', autospec=True)
-    def test_copy(
-            self, mock_makedirs, mock_ismount, mock_execute, mock_mount,
-            mock_copy, mock_find_device, mock_check_vmedia,
-            mock_booted_from_vmedia):
-
-        mock_ismount.return_value = False
-        mock_booted_from_vmedia.return_value = True
-        mock_find_device.return_value = '/dev/something'
-        mock_execute.side_effect = processutils.ProcessExecutionError('meow')
-        path = tempfile.mkdtemp()
-        self.addCleanup(lambda: shutil.rmtree(path))
-
-        def _fake_mount(dev):
-            self.assertEqual('/dev/something', dev)
-            # NOTE(dtantsur): makedirs is mocked
-            os.mkdir(os.path.join(path, 'etc'))
-            os.mkdir(os.path.join(path, 'etc', 'ironic-python-agent'))
-            os.mkdir(os.path.join(path, 'etc', 'ironic-python-agent.d'))
-            with open(os.path.join(path, 'not copied'), 'wt') as fp:
-                fp.write('not copied')
-            with open(os.path.join(path, 'etc', 'ironic-python-agent',
-                                   'ironic.crt'), 'wt') as fp:
-                fp.write('I am a cert')
-            with open(os.path.join(path, 'etc', 'ironic-python-agent.d',
-                                   'ironic.conf'), 'wt') as fp:
-                fp.write('I am a config')
-            return mock.MagicMock(**{'__enter__.return_value': path})
-
-        mock_find_device.return_value = '/dev/something'
-        mock_mount.side_effect = _fake_mount
-
-        utils.copy_config_from_vmedia()
-
-        mock_makedirs.assert_has_calls([
-            mock.call('/etc/ironic-python-agent', exist_ok=True),
-            mock.call('/etc/ironic-python-agent.d', exist_ok=True),
-        ], any_order=True)
-        mock_mount.assert_called_once_with('/dev/something')
-        mock_copy.assert_has_calls([
-            mock.call(mock.ANY, '/etc/ironic-python-agent/ironic.crt'),
-            mock.call(mock.ANY, '/etc/ironic-python-agent.d/ironic.conf'),
-        ], any_order=True)
-        self.assertTrue(mock_booted_from_vmedia.called)
         mock_execute.assert_called_once_with(
             'findmnt', '-n', '-oTARGET', '/dev/something')
+        mock_copy.assert_not_called()
+        mock_mount.assert_not_called()
+        self.assertTrue(mock_booted_from_vmedia.called)
 
-    @mock.patch.object(os.path, 'ismount', autospec=True)
     @mock.patch.object(os, 'makedirs', autospec=True)
-    def test_copy_mounted(
+    def test_copy(
-            self, mock_makedirs, mock_ismount, mock_execute, mock_mount,
+            self, mock_makedirs, mock_execute, mock_mount, mock_copy,
-            mock_copy, mock_find_device, mock_check_vmedia,
+            mock_find_device, mock_check_vmedia, mock_booted_from_vmedia):
-            mock_booted_from_vmedia):
+
-        mock_ismount.side_effect = [True, True, False]
         mock_booted_from_vmedia.return_value = True
-        mock_find_device.side_effect = ('/dev/something', '')
+        mock_find_device.return_value = '/dev/something'
+        mock_execute.side_effect = processutils.ProcessExecutionError("")
         path = tempfile.mkdtemp()
         self.addCleanup(lambda: shutil.rmtree(path))
 
@@ -1031,10 +969,6 @@ class TestConfigFromVmedia(testtools.TestCase):
 
         mock_find_device.return_value = '/dev/something'
         mock_mount.side_effect = _fake_mount
-        mock_execute.side_effect = [(path, ''),
-                                    ((), ()),
-                                    ((), ()),
-                                    ((), ())]
 
         utils.copy_config_from_vmedia()
 
@@ -1042,16 +976,52 @@ class TestConfigFromVmedia(testtools.TestCase):
             mock.call('/etc/ironic-python-agent', exist_ok=True),
             mock.call('/etc/ironic-python-agent.d', exist_ok=True),
         ], any_order=True)
-        mock_execute.assert_has_calls([
+        mock_mount.assert_called_once_with('/dev/something')
-            mock.call('findmnt', '-n', '-oTARGET', '/dev/something'),
-            mock.call('umount', mock.ANY, run_as_root=True),
-            mock.call('umount', '/mnt/config', run_as_root=True),
-            mock.call('umount', '/mnt/config', run_as_root=True)])
         mock_copy.assert_has_calls([
             mock.call(mock.ANY, '/etc/ironic-python-agent/ironic.crt'),
             mock.call(mock.ANY, '/etc/ironic-python-agent.d/ironic.conf'),
         ], any_order=True)
-        mock_mount.assert_called_once_with('/dev/something')
+        self.assertTrue(mock_booted_from_vmedia.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.return_value = True
+        mock_find_device.return_value = '/dev/something'
+        path = tempfile.mkdtemp()
+        self.addCleanup(lambda: shutil.rmtree(path))
+
+        # NOTE(dtantsur): makedirs is mocked
+        os.mkdir(os.path.join(path, 'etc'))
+        os.mkdir(os.path.join(path, 'etc', 'ironic-python-agent'))
+        os.mkdir(os.path.join(path, 'etc', 'ironic-python-agent.d'))
+        with open(os.path.join(path, 'not copied'), 'wt') as fp:
+            fp.write('not copied')
+        with open(os.path.join(path, 'etc', 'ironic-python-agent',
+                               'ironic.crt'), 'wt') as fp:
+            fp.write('I am a cert')
+        with open(os.path.join(path, 'etc', 'ironic-python-agent.d',
+                               'ironic.conf'), 'wt') as fp:
+            fp.write('I am a config')
+
+        mock_execute.return_value = path, ''
+        mock_find_device.return_value = '/dev/something'
+
+        utils.copy_config_from_vmedia()
+
+        mock_makedirs.assert_has_calls([
+            mock.call('/etc/ironic-python-agent', exist_ok=True),
+            mock.call('/etc/ironic-python-agent.d', exist_ok=True),
+        ], any_order=True)
+        mock_execute.assert_called_once_with(
+            'findmnt', '-n', '-oTARGET', '/dev/something')
+        mock_copy.assert_has_calls([
+            mock.call(mock.ANY, '/etc/ironic-python-agent/ironic.crt'),
+            mock.call(mock.ANY, '/etc/ironic-python-agent.d/ironic.conf'),
+        ], any_order=True)
+        mock_mount.assert_not_called()
         self.assertTrue(mock_booted_from_vmedia.called)
 
 
@@ -1152,73 +1122,6 @@ class TestCheckVirtualMedia(ironic_agent_base.IronicAgentTest):
                                         '/dev/sdh')
 
 
-@mock.patch.object(shutil, 'copy', autospec=True)
-@mock.patch.object(os, 'makedirs', autospec=True)
-@mock.patch.object(utils, '_determine_networking_path', autospec=True)
-@mock.patch.object(utils, 'execute', autospec=True)
-class TestNetworkConfigRefresh(ironic_agent_base.IronicAgentTest):
-
-    def test_tinycore(self, mock_exec, mock_path, mock_makedirs,
-                      mock_copy):
-        mock_path.return_value = 'tinycore'
-        utils.trigger_glean_network_refresh()
-        self.assertEqual(1, mock_path.call_count)
-        mock_exec.assert_has_calls([
-            mock.call('glean', '--distro', 'tinycore', run_as_root=True),
-            mock.call('/bin/sh', '/opt/network.sh', run_as_root=True)])
-        mock_makedirs.assert_called_once_with('/mnt/config/openstack/latest/',
-                                              exist_ok=True)
-
-    def test_execution_error_is_handled(self, mock_exec, mock_path,
-                                        mock_makedirs, mock_copy):
-        mock_path.return_value = 'tinycore'
-        mock_exec.side_effect = OSError()
-        utils.trigger_glean_network_refresh()
-        self.assertEqual(1, mock_path.call_count)
-        mock_exec.assert_called_once_with(
-            'glean', '--distro', 'tinycore', run_as_root=True)
-        mock_makedirs.assert_called_once_with('/mnt/config/openstack/latest/',
-                                              exist_ok=True)
-        mock_copy.assert_called_once_with(
-            '/etc/ironic-python-agent.d/network_data.json',
-            '/mnt/config/openstack/latest/network_data.json')
-
-    def test_networkd(self, mock_exec, mock_path,
-                      mock_makedirs, mock_copy):
-        mock_path.return_value = 'networkd'
-        utils.trigger_glean_network_refresh()
-        self.assertEqual(1, mock_path.call_count)
-        mock_exec.assert_has_calls([
-            mock.call('glean', '--distro', 'networkd', run_as_root=True),
-            mock.call('systemctl', 'restart', 'systemd-networkd.service',
-                      run_as_root=True)])
-        mock_makedirs.assert_called_once_with('/mnt/config/openstack/latest/',
-                                              exist_ok=True)
-        mock_copy.assert_called_once_with(
-            '/etc/ironic-python-agent.d/network_data.json',
-            '/mnt/config/openstack/latest/network_data.json')
-
-    def test_networkmanager(self, mock_exec, mock_path,
-                            mock_makedirs, mock_copy):
-        mock_path.return_value = 'networkmanager'
-        mock_exec.side_effect = [('', ''),
-                                 OSError(),
-                                 ('', '')]
-        utils.trigger_glean_network_refresh()
-        self.assertEqual(1, mock_path.call_count)
-        mock_exec.assert_has_calls([
-            mock.call('glean', '--use-nm', run_as_root=True),
-            mock.call('systemctl', 'restart', 'NetworkManager',
-                      run_as_root=True),
-            mock.call('systemctl', 'restart', 'NetworkManager.service',
-                      run_as_root=True)])
-        mock_makedirs.assert_called_once_with('/mnt/config/openstack/latest/',
-                                              exist_ok=True)
-        mock_copy.assert_called_once_with(
-            '/etc/ironic-python-agent.d/network_data.json',
-            '/mnt/config/openstack/latest/network_data.json')
-
-
 class TestCheckEarlyLogging(ironic_agent_base.IronicAgentTest):
 
     @mock.patch.object(utils, 'LOG', autospec=True)

ironic_python_agent/utils.py

@@ -21,7 +21,6 @@ import glob
 import io
 import json
 import os
-import platform
 import re
 import shutil
 import subprocess
@@ -34,7 +33,6 @@ from oslo_concurrency import processutils
 from oslo_config import cfg
 from oslo_log import log as logging
 from oslo_utils import units
-import psutil
 import requests
 import tenacity
 
@@ -128,7 +126,6 @@ def _find_vmedia_device_by_labels(labels):
     for device in ironic_utils.parse_device_tags(lsblk_output):
         for label in labels:
             if label.upper() == device['LABEL'].upper():
-                _early_log('Found vmedia candidate %s', device['KNAME'])
                 candidates.append(device['KNAME'])
 
     for candidate in candidates:
@@ -213,21 +210,13 @@ def _copy_config_from(path):
             msg = ("Unable to copy vmedia configuration %s to %s: %s"
                    % (src, dest, exc))
             raise errors.VirtualMediaBootError(msg)
-        network_data_path = os.path.join(
-            path, 'openstack/latest/network_data.json')
-        if os.path.isfile(network_data_path):
-            dest_path = '/etc/ironic-python-agent%s' % ext
-            _early_log('Copying network_data.json to %s', dest_path)
-            os.makedirs(dest_path, exist_ok=True)
-            shutil.copy(network_data_path, os.path.join(dest_path,
-                                                        'network_data.json'))
 
 
 def _find_mount_point(device):
     try:
         path, _e = execute('findmnt', '-n', '-oTARGET', device)
-    except (processutils.ProcessExecutionError, OSError):
+    except processutils.ProcessExecutionError:
-        return None
+        return
     else:
         return path.strip()
 
@@ -244,7 +233,6 @@ def _check_vmedia_device(vmedia_device_file):
               valid.
     """
     try:
-        _early_log('Checking device %s vmedia status.', vmedia_device_file)
         output, _e = execute('lsblk', '-n', '-s', '-P', '-b',
                              '-oKNAME,TRAN,TYPE,SIZE',
                              vmedia_device_file)
@@ -255,8 +243,6 @@ def _check_vmedia_device(vmedia_device_file):
     try:
         for device in ironic_utils.parse_device_tags(output):
             if device['TYPE'] == 'part':
-                # This would exclude any existing configuration drive written
-                # by ironic previously.
                 _early_log('Excluding device %s from virtual media'
                            'consideration as it is a partition.',
                            device['KNAME'])
@@ -267,7 +253,7 @@ def _check_vmedia_device(vmedia_device_file):
                 # registered for the scsi transport and thus type used.
                 # This will most likely be a qemu driven testing VM,
                 # or an older machine where SCSI transport is directly
-                # used to convey in a virtual media device.
+                # used to convey in a virtual
                 return True
             if device['TYPE'] == 'disk' and device['TRAN'] == 'usb':
                 # We know from experience on HPE machines, with ilo4/5, we see
@@ -320,9 +306,6 @@ def copy_config_from_vmedia():
     """Copies any configuration from a virtual media device.
 
     Copies files under /etc/ironic-python-agent and /etc/ironic-python-agent.d.
-
-    :returns: True when we successfully copied content from the virtual media
-              device. Otherwise None.
     """
     vmedia_device_file = _find_vmedia_device_by_labels(
         ['config-2', 'vmedia_boot_iso'])
@@ -336,84 +319,10 @@ def copy_config_from_vmedia():
     # Determine the device
     mounted = _find_mount_point(vmedia_device_file)
     if mounted:
-        # Unmount if already mounted by something like glean/cloud init.
+        _copy_config_from(mounted)
-        execute("umount", mounted, run_as_root=True)
-    with ironic_utils.mounted(vmedia_device_file) as vmedia_mount_point:
-        _copy_config_from(vmedia_mount_point)
-
-    # Finally, check mount and ensure unmounted before proceeding.
-    # NOTE(TheJulia): This is a reserved path by convention for configuration
-    # drive handling. Since you *can* stack multiple devices, we need to make
-    # sure no other tooling left anything in an unclean state.
-    while os.path.ismount("/mnt/config"):
-        try:
-            execute("umount", "/mnt/config", run_as_root=True)
-            _early_log("Sucessfully unmounted /mnt/config as an orphaned "
-                       "source of configuration.")
-        except (OSError, processutils.ProcessExecutionError):
-            _early_log("We failed to umount /mnt/config. This may be fatal "
-                       "if virtual media is in use for configuration.")
-            return False
-    return True
-
-
-def _determine_networking_path():
-    os_ver = platform.uname().version.lower()
-    if 'tinycore' in os_ver:
-        # Compact CI oriented IPA Image.
-        return 'tinycore'
-    if True in ['networkd' in x.name().lower()
-                for x in psutil.process_iter(['name'])]:
-        # Basically, this would be Ubuntu AFAIK.
-        return 'networkd'
     else:
-        # This is fairly safe to assume at this point.
+        with ironic_utils.mounted(vmedia_device_file) as vmedia_mount_point:
-        return 'networkmanager'
+            _copy_config_from(vmedia_mount_point)
-
-
-def trigger_glean_network_refresh():
-    """Trigger procesing and refresh of network configuration."""
-
-    # NOTE(TheJulia): Ironic explicitly doesn't support cloud-init
-    # as the consumer of this information in ramdisks. This stance
-    # may change in the future, but as of the beginning of the
-    # Caracal development cycle, cloud-init is uninstalled
-    # by ironic-python-agent-builder.
-    glean_read_path = '/mnt/config/openstack/latest/'
-    # NOTE(TheJulia) Exist_okay because we might end up re-triggering down
-    # this path if the agent restarts due to a transient failure.
-    os.makedirs(glean_read_path, exist_ok=True)
-    backup_copy = '/etc/ironic-python-agent.d/network_data.json'
-    network_data = os.path.join(glean_read_path, 'network_data.json')
-    shutil.copy(backup_copy, network_data)
-
-    _early_log('Working to apply network configuration refresh.')
-    network_type = _determine_networking_path()
-    # TODO(TheJulia): Check if python-glean is even in the $PATH.
-    # Two uniform actions, run glean, then restart all networking.
-    try:
-        if network_type == 'networkmanager':
-            # network manager present
-            execute('glean', '--use-nm', run_as_root=True)
-            try:
-                execute('systemctl', 'restart', 'NetworkManager',
-                        run_as_root=True)
-            except Exception:
-                # Inconsistent naming across distributions.
-                execute('systemctl', 'restart', 'NetworkManager.service',
-                        run_as_root=True)
-        if network_type == 'networkd':
-            execute('glean', '--distro', 'networkd', run_as_root=True)
-            execute('systemctl', 'restart', 'systemd-networkd.service',
-                    run_as_root=True)
-        if network_type == 'tinycore':
-            execute('glean', '--distro', 'tinycore', run_as_root=True)
-            # While a shell script, glean doesn't set this file to
-            # be executable once it writes it.
-            execute('/bin/sh', '/opt/network.sh', run_as_root=True)
-    except Exception as e:
-        _early_log('Unable to execute configuration refresh for '
-                   'configuration drive data. Error: %s' % e)
 
 
 def _get_cached_params():

releasenotes/notes/reload-glean-config-if-other-devices-found-7e9ebb861ed30ad5.yaml

@@ -1,19 +0,0 @@
----
-fixes:
-  - |
-    Fixes an issue with the agent where the presence of a configuration drive
-    from a prior node deployment could prevent networking configuration from
-    being determined and loaded in DHCP-less use cases.
-    The agent now attempts to identify the appropriate configuration drive
-    networking data source utilizing the existing device identification
-    process, and then triggers glean configuration to be re-generated,
-    after which networking is restarted. For more information, please
-    view `bug 2032377 <https://bugs.launchpad.net/ironic/+bug/2032377>`_.
-security:
-  - |
-    The ironic agent is now able to re-assert networking configuration
-    in DHCP-less modes should a prior node's ``config drive`` be
-    discovered as readable. Normally this issue is not a security
-    issue as long as ``ironic-python-agent-builder`` is utilized.
-    Ironic's agent automatically exlcudes possible configuration
-    drive copies from previously deployed baremetal nodes.