From e9b55e0cce874667cb1cf5e6464ffd931277c183 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Wed, 21 Jul 2021 14:41:10 -0500 Subject: [PATCH 1/6] Only invoke hotplug socket when functionality is enabled Alter's hotplug hook to have a query mechanism checking if the functionality is enabled. This allows us to avoid using the hotplug socket and service when hotplug is disabled. --- cloudinit/cmd/devel/hotplug_hook.py | 82 ++++++++++++++++++----------- tools/hook-hotplug | 9 +++- 2 files changed, 59 insertions(+), 32 deletions(-) diff --git a/cloudinit/cmd/devel/hotplug_hook.py b/cloudinit/cmd/devel/hotplug_hook.py index 0282f24a67e..39ec76564bf 100644 --- a/cloudinit/cmd/devel/hotplug_hook.py +++ b/cloudinit/cmd/devel/hotplug_hook.py @@ -31,15 +31,31 @@ def get_parser(parser=None): parser = argparse.ArgumentParser(prog=NAME, description=__doc__) parser.description = __doc__ - parser.add_argument("-d", "--devpath", required=True, - metavar="PATH", - help="sysfs path to hotplugged device") - parser.add_argument("-s", "--subsystem", required=True, - help="subsystem to act on", - choices=['net']) - parser.add_argument("-u", "--udevaction", required=True, - help="action to take", - choices=['add', 'remove']) + parser.add_argument( + "-s", "--subsystem", required=True, + help="subsystem to act on", + choices=['net'] + ) + + subparsers = parser.add_subparsers(dest='hotplug_action') + + subparsers.add_parser( + 'query', + help='query if hotplug is enabled for given subsystem' + ) + + parser_handle = subparsers.add_parser( + 'handle', help='handle the hotplug event') + parser_handle.add_argument( + "-d", "--devpath", required=True, + metavar="PATH", + help="sysfs path to hotplugged device" + ) + parser_handle.add_argument( + "-u", "--udevaction", required=True, + help="action to take", + choices=['add', 'remove'] + ) return parser @@ -133,6 +149,14 @@ def device_detected(self) -> bool: } +def is_enabled(hotplug_init, subsystem): + scope = SUBSYSTEM_PROPERTES_MAP[subsystem][1] + return hotplug_init.update_event_enabled( + event_source_type=EventType.HOTPLUG, + scope=scope + ) + + def handle_hotplug( hotplug_init: Init, devpath, subsystem, udevaction ): @@ -147,10 +171,7 @@ def handle_hotplug( LOG.debug('Fetching datasource') datasource = hotplug_init.fetch(existing="trust") - if not hotplug_init.update_event_enabled( - event_source_type=EventType.HOTPLUG, - scope=EventScope.NETWORK - ): + if not is_enabled(hotplug_init, subsystem): LOG.debug('hotplug not enabled for event of type %s', event_scope) return @@ -200,29 +221,30 @@ def handle_args(name, args): log.setupLogging(hotplug_init.cfg) if 'reporting' in hotplug_init.cfg: reporting.update_configuration(hotplug_init.cfg.get('reporting')) - # Logging isn't going to be setup until now LOG.debug( - '%s called with the following arguments: {udevaction: %s, ' - 'subsystem: %s, devpath: %s}', - name, args.udevaction, args.subsystem, args.devpath - ) - LOG.debug( - '%s called with the following arguments:\n' - 'udevaction: %s\n' - 'subsystem: %s\n' - 'devpath: %s', - name, args.udevaction, args.subsystem, args.devpath + '%s called with the following arguments: {' + 'hotplug_action: %s, subsystem: %s, udevaction: %s, devpath: %s}', + name, + args.hotplug_action, + args.subsystem, + args.udevaction if 'udevaction' in args else None, + args.devpath if 'devpath' in args else None, ) with hotplug_reporter: try: - handle_hotplug( - hotplug_init=hotplug_init, - devpath=args.devpath, - subsystem=args.subsystem, - udevaction=args.udevaction, - ) + if args.hotplug_action == 'query': + hotplug_init.fetch(existing="trust") + enabled = is_enabled(hotplug_init, args.subsystem) + print('enabled' if enabled else 'disabled') + else: + handle_hotplug( + hotplug_init=hotplug_init, + devpath=args.devpath, + subsystem=args.subsystem, + udevaction=args.udevaction, + ) except Exception: LOG.exception('Received fatal exception handling hotplug!') raise diff --git a/tools/hook-hotplug b/tools/hook-hotplug index 34e95929c9a..ced268b3aa8 100755 --- a/tools/hook-hotplug +++ b/tools/hook-hotplug @@ -8,12 +8,17 @@ is_finished() { [ -e /run/cloud-init/result.json ] } -if is_finished; then +hotplug_enabled() { + [ "$(cloud-init devel hotplug-hook -s "${SUBSYSTEM}" query)" == "enabled" ] +} + +if is_finished && hotplug_enabled; then # open cloud-init's hotplug-hook fifo rw exec 3<>/run/cloud-init/hook-hotplug-cmd env_params=( - --devpath="${DEVPATH}" --subsystem="${SUBSYSTEM}" + handle + --devpath="${DEVPATH}" --udevaction="${ACTION}" ) # write params to cloud-init's hotplug-hook fifo From 3753ee3892af5186cce46623a25254536db17e43 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Wed, 21 Jul 2021 16:54:24 -0500 Subject: [PATCH 2/6] update integration test --- tests/integration_tests/modules/test_hotplug.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/tests/integration_tests/modules/test_hotplug.py b/tests/integration_tests/modules/test_hotplug.py index b683566fd86..a42d1c8cfbc 100644 --- a/tests/integration_tests/modules/test_hotplug.py +++ b/tests/integration_tests/modules/test_hotplug.py @@ -48,7 +48,7 @@ def test_hotplug_add_remove(client: IntegrationInstance): # Add new NIC added_ip = client.instance.add_network_interface() - _wait_till_hotplug_complete(client) + _wait_till_hotplug_complete(client, expected_runs=2) ips_after_add = _get_ip_addr(client) new_addition = [ip for ip in ips_after_add if ip.ip4 == added_ip][0] @@ -63,7 +63,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=2) + _wait_till_hotplug_complete(client, expected_runs=4) 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] @@ -72,6 +72,10 @@ def test_hotplug_add_remove(client: IntegrationInstance): config = yaml.safe_load(netplan_cfg) assert new_addition.interface not in config['network']['ethernets'] + assert 'enabled' == client.execute( + 'cloud-init devel hotplug-hook -s net query' + ) + @pytest.mark.openstack def test_no_hotplug_in_userdata(client: IntegrationInstance): @@ -83,7 +87,7 @@ def test_no_hotplug_in_userdata(client: IntegrationInstance): client.instance.add_network_interface() _wait_till_hotplug_complete(client) log = client.read_from_file('/var/log/cloud-init.log') - assert 'hotplug not enabled for event of type network' in log + assert "Event Denied: scopes=['network'] EventType=hotplug" in log ips_after_add = _get_ip_addr(client) if len(ips_after_add) == len(ips_before) + 1: @@ -92,3 +96,7 @@ def test_no_hotplug_in_userdata(client: IntegrationInstance): assert new_ip.state == 'DOWN' else: assert len(ips_after_add) == len(ips_before) + + assert 'disabled' == client.execute( + 'cloud-init devel hotplug-hook -s net query' + ) From e69340f86381620ad58d3a56691c20a7895e9cb9 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Tue, 10 Aug 2021 14:26:49 -0500 Subject: [PATCH 3/6] Update cloudinit/cmd/devel/hotplug_hook.py Co-authored-by: Chad Smith --- cloudinit/cmd/devel/hotplug_hook.py | 1 + 1 file changed, 1 insertion(+) diff --git a/cloudinit/cmd/devel/hotplug_hook.py b/cloudinit/cmd/devel/hotplug_hook.py index 39ec76564bf..da89c6edd7f 100644 --- a/cloudinit/cmd/devel/hotplug_hook.py +++ b/cloudinit/cmd/devel/hotplug_hook.py @@ -38,6 +38,7 @@ def get_parser(parser=None): ) subparsers = parser.add_subparsers(dest='hotplug_action') + subparsers.required = True subparsers.add_parser( 'query', From 7623e9ef170ca864e92851c2ecf0a7347e509195 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Tue, 10 Aug 2021 15:08:02 -0500 Subject: [PATCH 4/6] review suggestion --- cloudinit/cmd/devel/hotplug_hook.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/cloudinit/cmd/devel/hotplug_hook.py b/cloudinit/cmd/devel/hotplug_hook.py index da89c6edd7f..5c85c623a0a 100644 --- a/cloudinit/cmd/devel/hotplug_hook.py +++ b/cloudinit/cmd/devel/hotplug_hook.py @@ -3,6 +3,7 @@ import abc import argparse import os +import sys import time from cloudinit import log @@ -12,7 +13,7 @@ from cloudinit.net.network_state import parse_net_config_data from cloudinit.reporting import events from cloudinit.stages import Init -from cloudinit.sources import DataSource +from cloudinit.sources import DataSource, DataSourceNotFoundException LOG = log.getLogger(__name__) @@ -236,7 +237,13 @@ def handle_args(name, args): with hotplug_reporter: try: if args.hotplug_action == 'query': - hotplug_init.fetch(existing="trust") + try: + hotplug_init.fetch(existing="trust") + except DataSourceNotFoundException: + print( + "Unable to determine hotplug state. No datasource " + "detected") + sys.exit(1) enabled = is_enabled(hotplug_init, args.subsystem) print('enabled' if enabled else 'disabled') else: From fcb9d2c053a0aea64ae7fe1891ca148938692471 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Fri, 13 Aug 2021 13:14:07 -0500 Subject: [PATCH 5/6] Ensure datasource has event enabled before updating update --- cloudinit/cmd/devel/hotplug_hook.py | 43 +++++++++++++++++------------ 1 file changed, 26 insertions(+), 17 deletions(-) diff --git a/cloudinit/cmd/devel/hotplug_hook.py b/cloudinit/cmd/devel/hotplug_hook.py index 5c85c623a0a..0cdc5eedd28 100644 --- a/cloudinit/cmd/devel/hotplug_hook.py +++ b/cloudinit/cmd/devel/hotplug_hook.py @@ -38,7 +38,10 @@ def get_parser(parser=None): choices=['net'] ) - subparsers = parser.add_subparsers(dest='hotplug_action') + subparsers = parser.add_subparsers( + title='Hotplug Action', + dest='hotplug_action' + ) subparsers.required = True subparsers.add_parser( @@ -152,31 +155,37 @@ def device_detected(self) -> bool: def is_enabled(hotplug_init, subsystem): - scope = SUBSYSTEM_PROPERTES_MAP[subsystem][1] + try: + scope = SUBSYSTEM_PROPERTES_MAP[subsystem][1] + except KeyError as e: + raise Exception( + 'hotplug-hook: cannot handle events for subsystem: {}'.format( + subsystem) + ) from e + return hotplug_init.update_event_enabled( event_source_type=EventType.HOTPLUG, scope=scope ) -def handle_hotplug( - hotplug_init: Init, devpath, subsystem, udevaction -): - handler_cls, event_scope = SUBSYSTEM_PROPERTES_MAP.get( - subsystem, (None, None) - ) - if handler_cls is None: - raise Exception( - 'hotplug-hook: cannot handle events for subsystem: {}'.format( - subsystem)) - +def initialize_datasource(hotplug_init, subsystem): LOG.debug('Fetching datasource') datasource = hotplug_init.fetch(existing="trust") if not is_enabled(hotplug_init, subsystem): - LOG.debug('hotplug not enabled for event of type %s', event_scope) + LOG.debug('hotplug not enabled for event of type %s', subsystem) return + return datasource + +def handle_hotplug( + hotplug_init: Init, devpath, subsystem, udevaction +): + datasource = initialize_datasource(hotplug_init, subsystem) + if not datasource: + return + handler_cls = SUBSYSTEM_PROPERTES_MAP[subsystem][0] LOG.debug('Creating %s event handler', subsystem) event_handler = handler_cls( datasource=datasource, @@ -238,14 +247,14 @@ def handle_args(name, args): try: if args.hotplug_action == 'query': try: - hotplug_init.fetch(existing="trust") + datasource = initialize_datasource( + hotplug_init, args.subsystem) except DataSourceNotFoundException: print( "Unable to determine hotplug state. No datasource " "detected") sys.exit(1) - enabled = is_enabled(hotplug_init, args.subsystem) - print('enabled' if enabled else 'disabled') + print('enabled' if datasource else 'disabled') else: handle_hotplug( hotplug_init=hotplug_init, From e5ce555fbc90c26636c26537051e77a118856aa5 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Fri, 13 Aug 2021 13:42:18 -0500 Subject: [PATCH 6/6] Actually do what I attempted to do in the last commit :D --- cloudinit/cmd/devel/hotplug_hook.py | 4 ++++ cloudinit/sources/__init__.py | 18 +++++++++++------- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/cloudinit/cmd/devel/hotplug_hook.py b/cloudinit/cmd/devel/hotplug_hook.py index 0cdc5eedd28..a0058f03fc7 100644 --- a/cloudinit/cmd/devel/hotplug_hook.py +++ b/cloudinit/cmd/devel/hotplug_hook.py @@ -173,6 +173,10 @@ def initialize_datasource(hotplug_init, subsystem): LOG.debug('Fetching datasource') datasource = hotplug_init.fetch(existing="trust") + if not datasource.get_supported_events([EventType.HOTPLUG]): + LOG.debug('hotplug not supported for event of type %s', subsystem) + return + if not is_enabled(hotplug_init, subsystem): LOG.debug('hotplug not enabled for event of type %s', subsystem) return diff --git a/cloudinit/sources/__init__.py b/cloudinit/sources/__init__.py index bf6bf139c5f..cc7e1c3c290 100644 --- a/cloudinit/sources/__init__.py +++ b/cloudinit/sources/__init__.py @@ -679,6 +679,16 @@ def get_hostname(self, fqdn=False, resolve_ip=False, metadata_only=False): def get_package_mirror_info(self): return self.distro.get_package_mirror_info(data_source=self) + def get_supported_events(self, source_event_types: List[EventType]): + supported_events = {} # type: Dict[EventScope, set] + for event in source_event_types: + for update_scope, update_events in self.supported_update_events.items(): # noqa: E501 + if event in update_events: + if not supported_events.get(update_scope): + supported_events[update_scope] = set() + supported_events[update_scope].add(event) + return supported_events + def update_metadata_if_supported( self, source_event_types: List[EventType] ) -> bool: @@ -694,13 +704,7 @@ def update_metadata_if_supported( @return True if the datasource did successfully update cached metadata due to source_event_type. """ - supported_events = {} # type: Dict[EventScope, set] - for event in source_event_types: - for update_scope, update_events in self.supported_update_events.items(): # noqa: E501 - if event in update_events: - if not supported_events.get(update_scope): - supported_events[update_scope] = set() - supported_events[update_scope].add(event) + supported_events = self.get_supported_events(source_event_types) for scope, matched_events in supported_events.items(): LOG.debug( "Update datasource metadata and %s config due to events: %s",