From c57deb7e760365f9fbeda0c8abb56f1f10f250ce Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Thu, 30 Nov 2023 10:33:29 +0000 Subject: [PATCH] Revert "Fix vmedia network config drive handling" This reverts commit 33f01fa3c2f32f447ed36f00fea68321c3991c2e. There are a few issues with the patch - see my comments there. The most pressing and the reasons to revert are: 1) It breaks deployments when the vmedia is present but does not have a network_data.json (the case for Metal3). 2) It assumes the presence of Glean which may not be the case. Neither Julia nor myself have time to thoroughly fix the issue, leaving a revert as the only option to unblock Metal3. Change-Id: I3f1a18a4910308699ca8f88d8e814c5efa78baee Closes-Bug: #2045255 --- ironic_python_agent/cmd/agent.py | 5 +- ironic_python_agent/tests/unit/test_utils.py | 213 +++++------------- ironic_python_agent/utils.py | 103 +-------- ...-other-devices-found-7e9ebb861ed30ad5.yaml | 19 -- 4 files changed, 66 insertions(+), 274 deletions(-) delete mode 100644 releasenotes/notes/reload-glean-config-if-other-devices-found-7e9ebb861ed30ad5.yaml diff --git a/ironic_python_agent/cmd/agent.py b/ironic_python_agent/cmd/agent.py index 4d0eeb22f..9ebe30065 100644 --- a/ironic_python_agent/cmd/agent.py +++ b/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() - if vmedia_configuration: - utils.trigger_glean_network_refresh() + utils.copy_config_from_vmedia() + log.register_options(CONF) CONF(args=sys.argv[1:]) # Debug option comes from oslo.log, allow overriding it via kernel cmdline diff --git a/ironic_python_agent/tests/unit/test_utils.py b/ironic_python_agent/tests/unit/test_utils.py index 7319780ee..fc683d22e 100644 --- a/ironic_python_agent/tests/unit/test_utils.py +++ b/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.call('findmnt', '-n', '-oTARGET', '/dev/something'), - mock.call('umount', '/mnt/config', run_as_root=True)]) - self.assertEqual(2, mock_execute.call_count) + mock_execute.assert_called_once_with('findmnt', '-n', '-oTARGET', + '/dev/something') 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_find_device.side_effect = ('/dev/something', '') + mock_execute.return_value = '/some/path', '' 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( - self, mock_makedirs, mock_ismount, mock_execute, mock_mount, - mock_copy, mock_find_device, mock_check_vmedia, - mock_booted_from_vmedia): - mock_ismount.side_effect = [True, True, False] + def test_copy( + 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.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.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_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) - 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) diff --git a/ironic_python_agent/utils.py b/ironic_python_agent/utils.py index ee5866102..c5889e9bd 100644 --- a/ironic_python_agent/utils.py +++ b/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): - return None + except processutils.ProcessExecutionError: + 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. - 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' + _copy_config_from(mounted) else: - # This is fairly safe to assume at this point. - return 'networkmanager' - - -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) + with ironic_utils.mounted(vmedia_device_file) as vmedia_mount_point: + _copy_config_from(vmedia_mount_point) def _get_cached_params(): diff --git a/releasenotes/notes/reload-glean-config-if-other-devices-found-7e9ebb861ed30ad5.yaml b/releasenotes/notes/reload-glean-config-if-other-devices-found-7e9ebb861ed30ad5.yaml deleted file mode 100644 index f78e98528..000000000 --- a/releasenotes/notes/reload-glean-config-if-other-devices-found-7e9ebb861ed30ad5.yaml +++ /dev/null @@ -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 `_. -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.