From aa19e1fc9586ce98f807159a20ea1b4c3b28ae9d Mon Sep 17 00:00:00 2001 From: James Falcon Date: Thu, 14 Oct 2021 10:57:03 -0500 Subject: [PATCH 1/6] Add "install hotplug" module This commit removes automatically installing udev rules for hotplug and adds a module to install them instead. Automatically including the udev rules and checking if hotplug was enabled consumed too many resources in certain circumstances. Moving the rules to a module ensures we don't spend extra extra cycles on hotplug if hotplug functionality isn't desired. LP: #1946003 --- cloudinit/cmd/devel/hotplug_hook.py | 5 +- cloudinit/config/cc_install_hotplug.py | 67 +++++++++++++ cloudinit/stages.py | 95 +++++++++++-------- config/cloud.cfg.tmpl | 1 + doc/rtd/topics/modules.rst | 1 + .../integration_tests/modules/test_hotplug.py | 13 ++- .../unittests/cmd/devel/test_hotplug_hook.py | 24 +++-- .../test_handler_install_hotplug.py | 65 +++++++++++++ tools/hook-hotplug | 6 +- udev/10-cloud-init-hook-hotplug.rules | 6 -- 10 files changed, 218 insertions(+), 65 deletions(-) create mode 100644 cloudinit/config/cc_install_hotplug.py create mode 100644 tests/unittests/test_handler/test_handler_install_hotplug.py delete mode 100644 udev/10-cloud-init-hook-hotplug.rules diff --git a/cloudinit/cmd/devel/hotplug_hook.py b/cloudinit/cmd/devel/hotplug_hook.py index d4f0547e329..f6f36a007cd 100644 --- a/cloudinit/cmd/devel/hotplug_hook.py +++ b/cloudinit/cmd/devel/hotplug_hook.py @@ -8,6 +8,7 @@ from cloudinit import log from cloudinit import reporting +from cloudinit import stages from cloudinit.event import EventScope, EventType from cloudinit.net import activators, read_sys_net_safe from cloudinit.net.network_state import parse_net_config_data @@ -164,7 +165,9 @@ def is_enabled(hotplug_init, subsystem): subsystem) ) from e - return hotplug_init.update_event_enabled( + return stages.update_event_enabled( + datasource=hotplug_init.datasource, + cfg=hotplug_init.cfg, event_source_type=EventType.HOTPLUG, scope=scope ) diff --git a/cloudinit/config/cc_install_hotplug.py b/cloudinit/config/cc_install_hotplug.py new file mode 100644 index 00000000000..25004fbf8a5 --- /dev/null +++ b/cloudinit/config/cc_install_hotplug.py @@ -0,0 +1,67 @@ +# This file is part of cloud-init. See LICENSE file for license information. +""" +Install Hotplug +-------------- +**Summary:** Install hotplug if supported and enabled + +This module will install the udev rules to enable hotplug if supported by +the datasource and enabled in the userdata. The udev rules will be installed +as ``/etc/udev/rules.d/10-cloud-init-hook-hotplug.rules``. + +**Internal name:** ``cc_install_hotplug`` + +**Module frequency:** always + +**Supported distros:** all + +**Config keys**:: + + updates: + network: + when: ['hotplug'] +""" +import os + +from cloudinit import util +from cloudinit import subp +from cloudinit import stages +from cloudinit.event import EventType, EventScope +from cloudinit.settings import PER_ALWAYS + + +frequency = PER_ALWAYS +distros = ['all'] + + +HOTPLUG_UDEV_PATH = "/etc/udev/rules.d/10-cloud-init-hook-hotplug.rules" +HOTPLUG_UDEV_RULES = """\ +ACTION!="add|remove", GOTO="cloudinit_end" +LABEL="cloudinit_hook" +SUBSYSTEM=="net", RUN+="/usr/lib/cloud-init/hook-hotplug" +LABEL="cloudinit_end" +""" + + +def handle(_name, cfg, cloud, log, _args): + if not stages.update_event_enabled( + datasource=cloud.datasource, + cfg=cfg, + event_source_type=EventType.HOTPLUG, + scope=EventScope.NETWORK, + ): + if os.path.exists(HOTPLUG_UDEV_PATH): + log.debug("Uninstalling hotplug, not enabled") + util.del_file(HOTPLUG_UDEV_PATH) + subp.subp(['udevadm', 'control', '--reload-rules']) + else: + log.debug("Skipping hotplug install, not enabled") + return + if not subp.which('udevadm'): + log.debug("Skipping hotplug install, udevadm not found") + return + + util.write_file( + filename=HOTPLUG_UDEV_PATH, + content=HOTPLUG_UDEV_RULES, + ) + subp.subp(['udevadm', 'control', '--reload-rules']) diff --git a/cloudinit/stages.py b/cloudinit/stages.py index 80aa9f5e321..731b29822c8 100644 --- a/cloudinit/stages.py +++ b/cloudinit/stages.py @@ -49,6 +49,54 @@ NO_PREVIOUS_INSTANCE_ID = "NO_PREVIOUS_INSTANCE_ID" +def update_event_enabled( + datasource: sources.DataSource, + cfg: dict, + event_source_type: EventType, + scope: EventScope = None +) -> bool: + """Determine if a particular EventType is enabled. + + For the `event_source_type` passed in, check whether this EventType + is enabled in the `updates` section of the userdata. If `updates` + is not enabled in userdata, check if defined as one of the + `default_events` on the datasource. `scope` may be used to + narrow the check to a particular `EventScope`. + + Note that on first boot, userdata may NOT be available yet. In this + case, we only have the data source's `default_update_events`, + so an event that should be enabled in userdata may be denied. + """ + default_events = datasource.default_update_events # type: Dict[EventScope, Set[EventType]] # noqa: E501 + user_events = userdata_to_events(cfg.get('updates', {})) # type: Dict[EventScope, Set[EventType]] # noqa: E501 + # A value in the first will override a value in the second + allowed = util.mergemanydict([ + copy.deepcopy(user_events), + copy.deepcopy(default_events), + ]) + LOG.debug('Allowed events: %s', allowed) + + if not scope: + scopes = allowed.keys() + else: + scopes = [scope] + scope_values = [s.value for s in scopes] + + for evt_scope in scopes: + if event_source_type in allowed.get(evt_scope, []): + LOG.debug( + 'Event Allowed: scope=%s EventType=%s', + evt_scope.value, event_source_type + ) + return True + + LOG.debug( + 'Event Denied: scopes=%s EventType=%s', + scope_values, event_source_type + ) + return False + + class Init(object): def __init__(self, ds_deps=None, reporter=None): if ds_deps is not None: @@ -715,46 +763,6 @@ def _find_networking_config(self): return (self.distro.generate_fallback_config(), NetworkConfigSource.fallback) - def update_event_enabled( - self, event_source_type: EventType, scope: EventScope = None - ) -> bool: - """Determine if a particular EventType is enabled. - - For the `event_source_type` passed in, check whether this EventType - is enabled in the `updates` section of the userdata. If `updates` - is not enabled in userdata, check if defined as one of the - `default_events` on the datasource. `scope` may be used to - narrow the check to a particular `EventScope`. - - Note that on first boot, userdata may NOT be available yet. In this - case, we only have the data source's `default_update_events`, - so an event that should be enabled in userdata may be denied. - """ - default_events = self.datasource.default_update_events # type: Dict[EventScope, Set[EventType]] # noqa: E501 - user_events = userdata_to_events(self.cfg.get('updates', {})) # type: Dict[EventScope, Set[EventType]] # noqa: E501 - # A value in the first will override a value in the second - allowed = util.mergemanydict([ - copy.deepcopy(user_events), - copy.deepcopy(default_events), - ]) - LOG.debug('Allowed events: %s', allowed) - - if not scope: - scopes = allowed.keys() - else: - scopes = [scope] - scope_values = [s.value for s in scopes] - - for evt_scope in scopes: - if event_source_type in allowed.get(evt_scope, []): - LOG.debug('Event Allowed: scope=%s EventType=%s', - evt_scope.value, event_source_type) - return True - - LOG.debug('Event Denied: scopes=%s EventType=%s', - scope_values, event_source_type) - return False - def _apply_netcfg_names(self, netcfg): try: LOG.debug("applying net config names for %s", netcfg) @@ -784,8 +792,11 @@ def apply_network_config(self, bring_up): return def event_enabled_and_metadata_updated(event_type): - return self.update_event_enabled( - event_type, scope=EventScope.NETWORK + return update_event_enabled( + datasource=self.datasource, + cfg=self.cfg, + event_source_type=event_type, + scope=EventScope.NETWORK ) and self.datasource.update_metadata_if_supported([event_type]) def should_run_on_boot_event(): diff --git a/config/cloud.cfg.tmpl b/config/cloud.cfg.tmpl index de1d75e5040..60065a1756a 100644 --- a/config/cloud.cfg.tmpl +++ b/config/cloud.cfg.tmpl @@ -165,6 +165,7 @@ cloud_final_modules: - scripts-user - ssh-authkey-fingerprints - keys-to-console + - install-hotplug - phone-home - final-message - power-state-change diff --git a/doc/rtd/topics/modules.rst b/doc/rtd/topics/modules.rst index e30fe0fe3d2..3ca6b9e38e8 100644 --- a/doc/rtd/topics/modules.rst +++ b/doc/rtd/topics/modules.rst @@ -22,6 +22,7 @@ Modules .. automodule:: cloudinit.config.cc_foo .. automodule:: cloudinit.config.cc_growpart .. automodule:: cloudinit.config.cc_grub_dpkg +.. automodule:: cloudinit.config.cc_install_hotplug .. automodule:: cloudinit.config.cc_keys_to_console .. automodule:: cloudinit.config.cc_landscape .. automodule:: cloudinit.config.cc_locale diff --git a/tests/integration_tests/modules/test_hotplug.py b/tests/integration_tests/modules/test_hotplug.py index 88cd8c16389..f5abc86f445 100644 --- a/tests/integration_tests/modules/test_hotplug.py +++ b/tests/integration_tests/modules/test_hotplug.py @@ -49,10 +49,13 @@ def test_hotplug_add_remove(client: IntegrationInstance): ips_before = _get_ip_addr(client) log = client.read_from_file('/var/log/cloud-init.log') assert 'Exiting hotplug handler' not in log + assert client.execute( + 'test -f /etc/udev/rules.d/10-cloud-init-hook-hotplug.rules' + ).ok # Add new NIC added_ip = client.instance.add_network_interface() - _wait_till_hotplug_complete(client, expected_runs=2) + _wait_till_hotplug_complete(client, expected_runs=1) ips_after_add = _get_ip_addr(client) new_addition = [ip for ip in ips_after_add if ip.ip4 == added_ip][0] @@ -67,7 +70,7 @@ def test_hotplug_add_remove(client: IntegrationInstance): # Remove new NIC client.instance.remove_network_interface(added_ip) - _wait_till_hotplug_complete(client, expected_runs=4) + _wait_till_hotplug_complete(client, expected_runs=2) ips_after_remove = _get_ip_addr(client) assert len(ips_after_remove) == len(ips_before) assert added_ip not in [ip.ip4 for ip in ips_after_remove] @@ -86,12 +89,14 @@ def test_no_hotplug_in_userdata(client: IntegrationInstance): ips_before = _get_ip_addr(client) log = client.read_from_file('/var/log/cloud-init.log') assert 'Exiting hotplug handler' not in log + assert client.execute( + 'test -f /etc/udev/rules.d/10-cloud-init-hook-hotplug.rules' + ).failed # Add new NIC client.instance.add_network_interface() - _wait_till_hotplug_complete(client) log = client.read_from_file('/var/log/cloud-init.log') - assert "Event Denied: scopes=['network'] EventType=hotplug" in log + assert 'hotplug-hook' not in log ips_after_add = _get_ip_addr(client) if len(ips_after_add) == len(ips_before) + 1: diff --git a/tests/unittests/cmd/devel/test_hotplug_hook.py b/tests/unittests/cmd/devel/test_hotplug_hook.py index 63d2490e007..e1c64e2f46b 100644 --- a/tests/unittests/cmd/devel/test_hotplug_hook.py +++ b/tests/unittests/cmd/devel/test_hotplug_hook.py @@ -30,6 +30,11 @@ def mocks(): return_value=FAKE_MAC ) + update_event_enabled = mock.patch( + 'cloudinit.stages.update_event_enabled', + return_value=True, + ) + m_network_state = mock.MagicMock(spec=NetworkState) parse_net = mock.patch( 'cloudinit.cmd.devel.hotplug_hook.parse_net_config_data', @@ -45,6 +50,7 @@ def mocks(): sleep = mock.patch('time.sleep') read_sys_net.start() + update_event_enabled.start() parse_net.start() select_activator.start() m_sleep = sleep.start() @@ -57,6 +63,7 @@ def mocks(): ) read_sys_net.stop() + update_event_enabled.stop() parse_net.stop() select_activator.stop() sleep.stop() @@ -122,13 +129,16 @@ def test_successful_remove(self, mocks): def test_update_event_disabled(self, mocks, caplog): init = mocks.m_init - init.update_event_enabled.return_value = False - handle_hotplug( - hotplug_init=init, - devpath='/dev/fake', - udevaction='remove', - subsystem='net' - ) + with mock.patch( + 'cloudinit.stages.update_event_enabled', + return_value=False + ): + handle_hotplug( + hotplug_init=init, + devpath='/dev/fake', + udevaction='remove', + subsystem='net' + ) assert 'hotplug not enabled for event of type' in caplog.text init.datasource.update_metadata_if_supported.assert_not_called() mocks.m_activator.bring_up_interface.assert_not_called() diff --git a/tests/unittests/test_handler/test_handler_install_hotplug.py b/tests/unittests/test_handler/test_handler_install_hotplug.py new file mode 100644 index 00000000000..4e13bf6012a --- /dev/null +++ b/tests/unittests/test_handler/test_handler_install_hotplug.py @@ -0,0 +1,65 @@ +# This file is part of cloud-init. See LICENSE file for license information. +from unittest import mock + +from cloudinit.config.cc_install_hotplug import ( + handle, + HOTPLUG_UDEV_PATH, + HOTPLUG_UDEV_RULES, +) + + +@mock.patch('cloudinit.stages.update_event_enabled') +@mock.patch('cloudinit.util.write_file', autospec=True) +@mock.patch('cloudinit.util.del_file', autospec=True) +@mock.patch('cloudinit.subp.subp') +class TestInstallHotplug: + @mock.patch('cloudinit.subp.which', return_value='udevadm') + def test_rules_installed_when_enabled( + self, m_which, m_subp, m_del, m_write, m_update_enabled + ): + m_update_enabled.return_value = True + + handle(None, None, mock.Mock(), mock.Mock(), None) + m_write.assert_called_once_with( + filename=HOTPLUG_UDEV_PATH, + content=HOTPLUG_UDEV_RULES, + ) + assert m_subp.call_args_list == [mock.call([ + 'udevadm', 'control', '--reload-rules', + ])] + assert m_del.call_args_list == [] + + @mock.patch('os.path.exists', return_value=False) + def test_rules_not_installed_when_disabled( + self, m_exists, m_subp, m_del, m_write, m_update_enabled + ): + m_update_enabled.return_value = False + + handle(None, None, mock.Mock(), mock.Mock(), None) + assert m_write.call_args_list == [] + assert m_del.call_args_list == [] + assert m_subp.call_args_list == [] + + @mock.patch('os.path.exists', return_value=True) + def test_rules_uninstalled_when_disabled( + self, m_exists, m_subp, m_del, m_write, m_update_enabled + ): + m_update_enabled.return_value = False + + handle(None, None, mock.Mock(), mock.Mock(), None) + m_del.assert_called_with(HOTPLUG_UDEV_PATH) + assert m_subp.call_args_list == [mock.call([ + 'udevadm', 'control', '--reload-rules', + ])] + assert m_write.call_args_list == [] + + @mock.patch('cloudinit.subp.which', return_value=None) + def test_rules_not_installed_when_no_udevadm( + self, m_which, m_subp, m_del, m_write, m_update_enabled + ): + m_update_enabled.return_value = True + + handle(None, None, mock.Mock(), mock.Mock(), None) + assert m_del.call_args_list == [] + assert m_write.call_args_list == [] + assert m_subp.call_args_list == [] diff --git a/tools/hook-hotplug b/tools/hook-hotplug index ced268b3aa8..35bd3da27a3 100755 --- a/tools/hook-hotplug +++ b/tools/hook-hotplug @@ -8,11 +8,7 @@ is_finished() { [ -e /run/cloud-init/result.json ] } -hotplug_enabled() { - [ "$(cloud-init devel hotplug-hook -s "${SUBSYSTEM}" query)" == "enabled" ] -} - -if is_finished && hotplug_enabled; then +if is_finished; then # open cloud-init's hotplug-hook fifo rw exec 3<>/run/cloud-init/hook-hotplug-cmd env_params=( diff --git a/udev/10-cloud-init-hook-hotplug.rules b/udev/10-cloud-init-hook-hotplug.rules deleted file mode 100644 index 2e382679263..00000000000 --- a/udev/10-cloud-init-hook-hotplug.rules +++ /dev/null @@ -1,6 +0,0 @@ -# This file is part of cloud-init. See LICENSE file for license information. -# Handle device adds only -ACTION!="add|remove", GOTO="cloudinit_end" -LABEL="cloudinit_hook" -SUBSYSTEM=="net|block", RUN+="/usr/lib/cloud-init/hook-hotplug" -LABEL="cloudinit_end" From 51b6a7d56cb3af05bba19ef790ec3965ae7fcf15 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Tue, 19 Oct 2021 22:09:37 -0500 Subject: [PATCH 2/6] Added - supported check - json schema - supported datasources - warning if enabled but not supported - test refactor. --- cloudinit/config/cc_install_hotplug.py | 128 +++++++++++++---- .../test_handler_install_hotplug.py | 130 +++++++++++------- tests/unittests/test_handler/test_schema.py | 3 +- 3 files changed, 183 insertions(+), 78 deletions(-) diff --git a/cloudinit/config/cc_install_hotplug.py b/cloudinit/config/cc_install_hotplug.py index 25004fbf8a5..f928892dcb9 100644 --- a/cloudinit/config/cc_install_hotplug.py +++ b/cloudinit/config/cc_install_hotplug.py @@ -1,36 +1,87 @@ # This file is part of cloud-init. See LICENSE file for license information. -""" -Install Hotplug --------------- -**Summary:** Install hotplug if supported and enabled - -This module will install the udev rules to enable hotplug if supported by -the datasource and enabled in the userdata. The udev rules will be installed -as ``/etc/udev/rules.d/10-cloud-init-hook-hotplug.rules``. - -**Internal name:** ``cc_install_hotplug`` - -**Module frequency:** always - -**Supported distros:** all - -**Config keys**:: - - updates: - network: - when: ['hotplug'] -""" +"""Install hotplug udev rules if supported and enabled""" import os +from textwrap import dedent from cloudinit import util from cloudinit import subp from cloudinit import stages +from cloudinit.config.schema import get_schema_doc, validate_cloudconfig_schema +from cloudinit.distros import ALL_DISTROS from cloudinit.event import EventType, EventScope from cloudinit.settings import PER_ALWAYS frequency = PER_ALWAYS -distros = ['all'] +distros = [ALL_DISTROS] + +schema = { + "id": "cc_install_hotplug", + "name": "Install Hotplug", + "title": "Install hotplug if supported and enabled", + "description": dedent("""\ + This module will install the udev rules to enable hotplug if + supported by the datasource and enabled in the userdata. The udev + rules will be installed as + ``/etc/udev/rules.d/10-cloud-init-hook-hotplug.rules``. + + When hotplug is enabled, newly added network devices will be added + to the system by cloud-init. After udev detects the event, + cloud-init will referesh the instance metadata from the datasource, + detect the device in the updated metadata, then apply the updated + network configuration. + + Currently supported datasources: Openstack, EC2 + """), + "distros": distros, + "examples": [ + dedent("""\ + # Enable hotplug of network devices + updates: + network: + when: ["hotplug"] + """), + dedent("""\ + # Enalble network hotplug alongside boot event + updates: + network: + when: ["boot", "hotplug"] + """), + ], + "frequency": frequency, + "type": "object", + "properties": { + "updates": { + "type": "object", + "additionalProperties": False, + "properties": { + "network": { + "type": "object", + "required": ["when"], + "additionalProperties": False, + "properties": { + "when": { + "type": "array", + "additionalProperties": False, + "items": { + "type": "string", + "additionalProperties": False, + "enum": [ + "boot-new-instance", + "boot-legacy", + "boot", + "hotplug", + ] + } + } + } + } + } + } + } +} + +__doc__ == get_schema_doc(schema) HOTPLUG_UDEV_PATH = "/etc/udev/rules.d/10-cloud-init-hook-hotplug.rules" @@ -43,20 +94,37 @@ def handle(_name, cfg, cloud, log, _args): - if not stages.update_event_enabled( - datasource=cloud.datasource, - cfg=cfg, - event_source_type=EventType.HOTPLUG, - scope=EventScope.NETWORK, + validate_cloudconfig_schema(cfg, schema) + network_hotplug_enabled = ( + 'updates' in cfg and + 'network' in cfg['updates'] and + 'when' in cfg['updates']['network'] and + 'hotplug' in cfg['updates']['network']['when'] + ) + if not ( + EventType.HOTPLUG in cloud.datasource.get_supported_events( + [EventType.HOTPLUG] + ) and + stages.update_event_enabled( + datasource=cloud.datasource, + cfg=cfg, + event_source_type=EventType.HOTPLUG, + scope=EventScope.NETWORK, + ) ): if os.path.exists(HOTPLUG_UDEV_PATH): log.debug("Uninstalling hotplug, not enabled") util.del_file(HOTPLUG_UDEV_PATH) - subp.subp(['udevadm', 'control', '--reload-rules']) + subp.subp(["udevadm", "control", "--reload-rules"]) + elif network_hotplug_enabled: + log.warning( + "Hotplug is unsupported by current datasource. " + "Udev rules will NOT be installed." + ) else: log.debug("Skipping hotplug install, not enabled") return - if not subp.which('udevadm'): + if not subp.which("udevadm"): log.debug("Skipping hotplug install, udevadm not found") return @@ -64,4 +132,4 @@ def handle(_name, cfg, cloud, log, _args): filename=HOTPLUG_UDEV_PATH, content=HOTPLUG_UDEV_RULES, ) - subp.subp(['udevadm', 'control', '--reload-rules']) + subp.subp(["udevadm", "control", "--reload-rules"]) diff --git a/tests/unittests/test_handler/test_handler_install_hotplug.py b/tests/unittests/test_handler/test_handler_install_hotplug.py index 4e13bf6012a..cf39e4632e6 100644 --- a/tests/unittests/test_handler/test_handler_install_hotplug.py +++ b/tests/unittests/test_handler/test_handler_install_hotplug.py @@ -1,65 +1,101 @@ # This file is part of cloud-init. See LICENSE file for license information. +from collections import namedtuple from unittest import mock +import pytest + from cloudinit.config.cc_install_hotplug import ( handle, HOTPLUG_UDEV_PATH, HOTPLUG_UDEV_RULES, ) +from cloudinit.event import EventType + + +@pytest.yield_fixture() +def mocks(): + m_update_enabled = mock.patch('cloudinit.stages.update_event_enabled') + m_write = mock.patch('cloudinit.util.write_file', autospec=True) + m_del = mock.patch('cloudinit.util.del_file', autospec=True) + m_subp = mock.patch('cloudinit.subp.subp') + m_which = mock.patch('cloudinit.subp.which', return_value=None) + m_path_exists = mock.patch('os.path.exists', return_value=False) + + yield namedtuple( + 'Mocks', + 'm_update_enabled m_write m_del m_subp m_which m_path_exists' + )( + m_update_enabled.start(), m_write.start(), m_del.start(), + m_subp.start(), m_which.start(), m_path_exists.start() + ) + + m_update_enabled.stop() + m_write.stop() + m_del.stop() + m_subp.stop() + m_which.stop() + m_path_exists.stop() -@mock.patch('cloudinit.stages.update_event_enabled') -@mock.patch('cloudinit.util.write_file', autospec=True) -@mock.patch('cloudinit.util.del_file', autospec=True) -@mock.patch('cloudinit.subp.subp') class TestInstallHotplug: - @mock.patch('cloudinit.subp.which', return_value='udevadm') - def test_rules_installed_when_enabled( - self, m_which, m_subp, m_del, m_write, m_update_enabled - ): - m_update_enabled.return_value = True - - handle(None, None, mock.Mock(), mock.Mock(), None) - m_write.assert_called_once_with( + def test_rules_installed_when_supported_and_enabled(self, mocks): + mocks.m_which.return_value = 'udevadm' + mocks.m_update_enabled.return_value = True + m_cloud = mock.MagicMock() + m_cloud.datasource.get_supported_events.return_value = [ + EventType.HOTPLUG] + + handle(None, {}, m_cloud, mock.Mock(), None) + mocks.m_write.assert_called_once_with( filename=HOTPLUG_UDEV_PATH, content=HOTPLUG_UDEV_RULES, ) - assert m_subp.call_args_list == [mock.call([ + assert mocks.m_subp.call_args_list == [mock.call([ 'udevadm', 'control', '--reload-rules', ])] - assert m_del.call_args_list == [] - - @mock.patch('os.path.exists', return_value=False) - def test_rules_not_installed_when_disabled( - self, m_exists, m_subp, m_del, m_write, m_update_enabled - ): - m_update_enabled.return_value = False - - handle(None, None, mock.Mock(), mock.Mock(), None) - assert m_write.call_args_list == [] - assert m_del.call_args_list == [] - assert m_subp.call_args_list == [] - - @mock.patch('os.path.exists', return_value=True) - def test_rules_uninstalled_when_disabled( - self, m_exists, m_subp, m_del, m_write, m_update_enabled - ): - m_update_enabled.return_value = False - - handle(None, None, mock.Mock(), mock.Mock(), None) - m_del.assert_called_with(HOTPLUG_UDEV_PATH) - assert m_subp.call_args_list == [mock.call([ + assert mocks.m_del.call_args_list == [] + + def test_rules_not_installed_when_unsupported(self, mocks): + mocks.m_update_enabled.return_value = True + m_cloud = mock.MagicMock() + m_cloud.datasource.get_supported_events.return_value = [] + + handle(None, {}, m_cloud, mock.Mock(), None) + assert mocks.m_write.call_args_list == [] + assert mocks.m_del.call_args_list == [] + assert mocks.m_subp.call_args_list == [] + + def test_rules_not_installed_when_disabled(self, mocks): + mocks.m_update_enabled.return_value = False + m_cloud = mock.MagicMock() + m_cloud.datasource.get_supported_events.return_value = [ + EventType.HOTPLUG] + + handle(None, {}, m_cloud, mock.Mock(), None) + assert mocks.m_write.call_args_list == [] + assert mocks.m_del.call_args_list == [] + assert mocks.m_subp.call_args_list == [] + + def test_rules_uninstalled_when_disabled(self, mocks): + mocks.m_path_exists.return_value = True + mocks.m_update_enabled.return_value = False + m_cloud = mock.MagicMock() + m_cloud.datasource.get_supported_events.return_value = [] + + handle(None, {}, m_cloud, mock.Mock(), None) + mocks.m_del.assert_called_with(HOTPLUG_UDEV_PATH) + assert mocks.m_subp.call_args_list == [mock.call([ 'udevadm', 'control', '--reload-rules', ])] - assert m_write.call_args_list == [] - - @mock.patch('cloudinit.subp.which', return_value=None) - def test_rules_not_installed_when_no_udevadm( - self, m_which, m_subp, m_del, m_write, m_update_enabled - ): - m_update_enabled.return_value = True - - handle(None, None, mock.Mock(), mock.Mock(), None) - assert m_del.call_args_list == [] - assert m_write.call_args_list == [] - assert m_subp.call_args_list == [] + assert mocks.m_write.call_args_list == [] + + def test_rules_not_installed_when_no_udevadm(self, mocks): + mocks.m_update_enabled.return_value = True + m_cloud = mock.MagicMock() + m_cloud.datasource.get_supported_events.return_value = [ + EventType.HOTPLUG] + + handle(None, {}, m_cloud, mock.Mock(), None) + assert mocks.m_del.call_args_list == [] + assert mocks.m_write.call_args_list == [] + assert mocks.m_subp.call_args_list == [] diff --git a/tests/unittests/test_handler/test_schema.py b/tests/unittests/test_handler/test_schema.py index 6b0b1f74000..8fb1b6dfe0c 100644 --- a/tests/unittests/test_handler/test_schema.py +++ b/tests/unittests/test_handler/test_schema.py @@ -35,7 +35,8 @@ def test_get_schema_coalesces_known_schema(self): 'cc_ubuntu_drivers', 'cc_write_files', 'cc_zypper_add_repo', - 'cc_chef' + 'cc_chef', + 'cc_install_hotplug', ], [subschema['id'] for subschema in schema['allOf']]) self.assertEqual('cloud-config-schema', schema['id']) From 437129f1dfed6391f6d805f96254d877b82ea7c5 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Fri, 22 Oct 2021 09:52:25 -0500 Subject: [PATCH 3/6] review comments --- cloudinit/config/cc_install_hotplug.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cloudinit/config/cc_install_hotplug.py b/cloudinit/config/cc_install_hotplug.py index f928892dcb9..a840920738b 100644 --- a/cloudinit/config/cc_install_hotplug.py +++ b/cloudinit/config/cc_install_hotplug.py @@ -42,7 +42,7 @@ when: ["hotplug"] """), dedent("""\ - # Enalble network hotplug alongside boot event + # Enable network hotplug alongside boot event updates: network: when: ["boot", "hotplug"] @@ -86,6 +86,7 @@ HOTPLUG_UDEV_PATH = "/etc/udev/rules.d/10-cloud-init-hook-hotplug.rules" HOTPLUG_UDEV_RULES = """\ +# Installed by cloud-init due to network hotplug userdata ACTION!="add|remove", GOTO="cloudinit_end" LABEL="cloudinit_hook" SUBSYSTEM=="net", RUN+="/usr/lib/cloud-init/hook-hotplug" From 5248cc3a8898db0840f56b01930323f12f816c04 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Fri, 22 Oct 2021 11:21:04 -0500 Subject: [PATCH 4/6] pylint finding my stupid typos --- cloudinit/config/cc_install_hotplug.py | 2 +- requirements.txt | 2 +- test-requirements.txt | 2 +- tox.ini | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cloudinit/config/cc_install_hotplug.py b/cloudinit/config/cc_install_hotplug.py index a840920738b..18b7a8b7ead 100644 --- a/cloudinit/config/cc_install_hotplug.py +++ b/cloudinit/config/cc_install_hotplug.py @@ -81,7 +81,7 @@ } } -__doc__ == get_schema_doc(schema) +__doc__ = get_schema_doc(schema) HOTPLUG_UDEV_PATH = "/etc/udev/rules.d/10-cloud-init-hook-hotplug.rules" diff --git a/requirements.txt b/requirements.txt index 27cef184b15..c4adc455b74 100644 --- a/requirements.txt +++ b/requirements.txt @@ -31,7 +31,7 @@ requests jsonpatch # For validating cloud-config sections per schema definitions -jsonschema==3.2.0 +jsonschema # Used by DataSourceVMware to inspect the host's network configuration during # the "setup()" function. diff --git a/test-requirements.txt b/test-requirements.txt index 61fb9b03597..06dfbbec156 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -5,4 +5,4 @@ pytest-cov # Only really needed on older versions of python setuptools -jsonschema==3.2.0 +jsonschema diff --git a/tox.ini b/tox.ini index f0c22cdb039..874d3f20b12 100644 --- a/tox.ini +++ b/tox.ini @@ -77,7 +77,7 @@ deps = pyserial==3.0.1 configobj==5.0.6 requests==2.9.1 - jsonschema==3.2.0 + jsonschema # test-requirements pytest-catchlog==1.2.1 From 1af35c723189c1a36d9f3b80d41e0eb3b8f4af3c Mon Sep 17 00:00:00 2001 From: James Falcon Date: Fri, 22 Oct 2021 22:50:36 -0500 Subject: [PATCH 5/6] review comments --- cloudinit/config/cc_install_hotplug.py | 22 +++++++++---------- .../test_handler_install_hotplug.py | 21 ++++++++++-------- 2 files changed, 23 insertions(+), 20 deletions(-) diff --git a/cloudinit/config/cc_install_hotplug.py b/cloudinit/config/cc_install_hotplug.py index 18b7a8b7ead..dd3ec358a80 100644 --- a/cloudinit/config/cc_install_hotplug.py +++ b/cloudinit/config/cc_install_hotplug.py @@ -102,17 +102,17 @@ def handle(_name, cfg, cloud, log, _args): 'when' in cfg['updates']['network'] and 'hotplug' in cfg['updates']['network']['when'] ) - if not ( - EventType.HOTPLUG in cloud.datasource.get_supported_events( - [EventType.HOTPLUG] - ) and - stages.update_event_enabled( - datasource=cloud.datasource, - cfg=cfg, - event_source_type=EventType.HOTPLUG, - scope=EventScope.NETWORK, - ) - ): + hotplug_supported = EventType.HOTPLUG in ( + cloud.datasource.get_supported_events( + [EventType.HOTPLUG]).get(EventScope.NETWORK, set()) + ) + hotplug_enabled = stages.update_event_enabled( + datasource=cloud.datasource, + cfg=cfg, + event_source_type=EventType.HOTPLUG, + scope=EventScope.NETWORK, + ) + if not (hotplug_supported and hotplug_enabled): if os.path.exists(HOTPLUG_UDEV_PATH): log.debug("Uninstalling hotplug, not enabled") util.del_file(HOTPLUG_UDEV_PATH) diff --git a/tests/unittests/test_handler/test_handler_install_hotplug.py b/tests/unittests/test_handler/test_handler_install_hotplug.py index cf39e4632e6..19b0cc4113e 100644 --- a/tests/unittests/test_handler/test_handler_install_hotplug.py +++ b/tests/unittests/test_handler/test_handler_install_hotplug.py @@ -9,7 +9,7 @@ HOTPLUG_UDEV_PATH, HOTPLUG_UDEV_RULES, ) -from cloudinit.event import EventType +from cloudinit.event import EventScope, EventType @pytest.yield_fixture() @@ -42,8 +42,9 @@ def test_rules_installed_when_supported_and_enabled(self, mocks): mocks.m_which.return_value = 'udevadm' mocks.m_update_enabled.return_value = True m_cloud = mock.MagicMock() - m_cloud.datasource.get_supported_events.return_value = [ - EventType.HOTPLUG] + m_cloud.datasource.get_supported_events.return_value = { + EventScope.NETWORK: {EventType.HOTPLUG} + } handle(None, {}, m_cloud, mock.Mock(), None) mocks.m_write.assert_called_once_with( @@ -58,7 +59,7 @@ def test_rules_installed_when_supported_and_enabled(self, mocks): def test_rules_not_installed_when_unsupported(self, mocks): mocks.m_update_enabled.return_value = True m_cloud = mock.MagicMock() - m_cloud.datasource.get_supported_events.return_value = [] + m_cloud.datasource.get_supported_events.return_value = {} handle(None, {}, m_cloud, mock.Mock(), None) assert mocks.m_write.call_args_list == [] @@ -68,8 +69,9 @@ def test_rules_not_installed_when_unsupported(self, mocks): def test_rules_not_installed_when_disabled(self, mocks): mocks.m_update_enabled.return_value = False m_cloud = mock.MagicMock() - m_cloud.datasource.get_supported_events.return_value = [ - EventType.HOTPLUG] + m_cloud.datasource.get_supported_events.return_value = { + EventScope.NETWORK: {EventType.HOTPLUG} + } handle(None, {}, m_cloud, mock.Mock(), None) assert mocks.m_write.call_args_list == [] @@ -80,7 +82,7 @@ def test_rules_uninstalled_when_disabled(self, mocks): mocks.m_path_exists.return_value = True mocks.m_update_enabled.return_value = False m_cloud = mock.MagicMock() - m_cloud.datasource.get_supported_events.return_value = [] + m_cloud.datasource.get_supported_events.return_value = {} handle(None, {}, m_cloud, mock.Mock(), None) mocks.m_del.assert_called_with(HOTPLUG_UDEV_PATH) @@ -92,8 +94,9 @@ def test_rules_uninstalled_when_disabled(self, mocks): def test_rules_not_installed_when_no_udevadm(self, mocks): mocks.m_update_enabled.return_value = True m_cloud = mock.MagicMock() - m_cloud.datasource.get_supported_events.return_value = [ - EventType.HOTPLUG] + m_cloud.datasource.get_supported_events.return_value = { + EventScope.NETWORK: {EventType.HOTPLUG} + } handle(None, {}, m_cloud, mock.Mock(), None) assert mocks.m_del.call_args_list == [] From 9c7d117f6014560c560ca037beece8e7166ef7e5 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Mon, 25 Oct 2021 19:11:11 -0500 Subject: [PATCH 6/6] PER_ALWAYS->PER_INSTANCE --- cloudinit/config/cc_install_hotplug.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cloudinit/config/cc_install_hotplug.py b/cloudinit/config/cc_install_hotplug.py index dd3ec358a80..d6b2a2df449 100644 --- a/cloudinit/config/cc_install_hotplug.py +++ b/cloudinit/config/cc_install_hotplug.py @@ -9,10 +9,10 @@ from cloudinit.config.schema import get_schema_doc, validate_cloudconfig_schema from cloudinit.distros import ALL_DISTROS from cloudinit.event import EventType, EventScope -from cloudinit.settings import PER_ALWAYS +from cloudinit.settings import PER_INSTANCE -frequency = PER_ALWAYS +frequency = PER_INSTANCE distros = [ALL_DISTROS] schema = {