cloud-init-dev team mailing list archive

[Scott Moser <ssmoser2+ubuntu@xxxxxxxxx>]

Diff comments:

> diff --git a/cloudinit/event.py b/cloudinit/event.py
> index f7b311f..77ce631 100644
> --- a/cloudinit/event.py
> +++ b/cloudinit/event.py
> @@ -2,16 +2,68 @@
>  
>  """Classes and functions related to event handling."""
>  
> +from cloudinit import log as logging
> +from cloudinit import util
> +
> +
> +LOG = logging.getLogger(__name__)
> +
>  
>  # Event types which can generate maintenance requests for cloud-init.
>  class EventType(object):
>      BOOT = "System boot"
>      BOOT_NEW_INSTANCE = "New instance first boot"
> +    UDEV = "Udev add|change event on net|storage"
>  
>      # TODO: Cloud-init will grow support for the follow event types:
> -    # UDEV
>      # METADATA_CHANGE
>      # USER_REQUEST
>  
> +EventTypeMap = {
> +    'boot': EventType.BOOT,
> +    'boot-new-instance': EventType.BOOT_NEW_INSTANCE,
> +    'udev': EventType.UDEV,

i don't know how i feel about the name 'udev'.
is there a reason you didn't use 'hotplug' ?

> +}
> +
> +# inverted mapping
> +EventNameMap = {v: k for k, v in EventTypeMap.items()}
> +
> +
> +def get_allowed_events(sys_events, ds_events):
> +    '''Merge datasource capabilties with system config to determine which
> +       update events are allowed.'''
> +
> +    # updates:
> +    #   policy-version: 1
> +    #   network:
> +    #     when: [boot-new-instance, boot, udev]
> +    #   storage:
> +    #     when: [boot-new-instance, udev]
> +    #     watch: http://169.254.169.254/metadata/storage_config/
> +
> +    LOG.debug('updates: system   cfg: %s', sys_events)
> +    LOG.debug('updates: datasrc caps: %s', ds_events)
> +
> +    updates = util.mergemanydict([sys_events, ds_events])
> +    LOG.debug('updates: merged  cfg: %s', updates)
> +
> +    events = {}
> +    for etype in ['network', 'storage']:
> +        events[etype] = (
> +            set([EventTypeMap.get(evt)
> +                 for evt in updates.get(etype, {}).get('when', [])
> +                 if evt in EventTypeMap]))
> +
> +    LOG.debug('updates: allowed events: %s', events)
> +    return events
> +
> +
> +def get_update_events_config(update_events):
> +    '''Return a dictionary of updates config'''
> +    evt_cfg = {'policy-version': 1}
> +    for scope, events in update_events.items():
> +        evt_cfg[scope] = {'when': [EventNameMap[evt] for evt in events]}
> +
> +    return evt_cfg
>  
>  # vi: ts=4 expandtab
> diff --git a/systemd/cloud-init-hotplugd.service b/systemd/cloud-init-hotplugd.service
> new file mode 100644
> index 0000000..6f231cd
> --- /dev/null
> +++ b/systemd/cloud-init-hotplugd.service
> @@ -0,0 +1,11 @@

https://www.linux.com/blog/end-road-systemds-socket-units
"In most cases, the service will have the same name the socket unit, except with an @ and the service suffix. As your socket unit was echo.socket, your service will be echo@.service."

that implies this should be cloud-init-hotplugd@.service.
should it be?

> +[Unit]
> +Description=cloud-init hotplug hook daemon
> +After=cloud-init-hotplugd.socket
> +
> +[Service]
> +Type=simple
> +ExecStart=/bin/bash -c 'read args <&3; echo "args=$args"; \
> +                        exec /usr/bin/cloud-init devel hotplug-hook $args; \
> +                        exit 0'
> +SyslogIdentifier=cloud-init-hotplugd
> +TimeoutStopSec=5
> diff --git a/tools/hook-hotplug b/tools/hook-hotplug
> new file mode 100755
> index 0000000..697d3ad
> --- /dev/null
> +++ b/tools/hook-hotplug
> @@ -0,0 +1,26 @@
> +#!/bin/bash
> +# This file is part of cloud-init. See LICENSE file for license information.
> +
> +# This script checks if cloud-init has hotplug hooked and if
> +# cloud-init has finished; if so invoke cloud-init hotplug-hook
> +
> +is_finished() {
> +    [ -e /run/cloud-init/result.json ] || return 1

you don't need the 'return 1'

> +}
> +
> +if is_finished; then
> +    # only hook pci devices at this time
> +    case ${DEVPATH} in
> +        /devices/pci*)
> +            # open cloud-init's hotplug-hook fifo rw
> +            exec 3<>/run/cloud-init/hook-hotplug-cmd

probably dont need to *read* from that file. (isn't that what <> does?)

> +            env_params=( \

don't need trailing \ here.

> +                --devpath=${DEVPATH}

quote these things.
even though they're in all reality going to not include space or odd chars, they could.
ie, DEVPATH="/dev/disk/by-name/this is my n ame" would mess it up.

> +                --subsystem=${SUBSYSTEM}
> +                --udevaction=${ACTION}
> +            )
> +            # write params to cloud-init's hotplug-hook fifo
> +            echo "--hotplug-debug ${env_params[@]}" >&3
> +            ;;
> +    esac
> +fi


-- 
https://code.launchpad.net/~raharper/cloud-init/+git/cloud-init/+merge/356152
Your team cloud-init commiters is requested to review the proposed merge of ~raharper/cloud-init:feature/cloud-init-hotplug-handler into cloud-init:master.