Enable hotplug for LXD datasource (#1787)

When a NIC appears, check for a cloud-init.network-config and apply it.

8 files changed, 420 insertions, 41 deletions

diff --git a/cloudinit/cmd/devel/hotplug_hook.py b/cloudinit/cmd/devel/hotplug_hook.py
index f95e8cc0..560857ef 100755
--- a/cloudinit/cmd/devel/hotplug_hook.py
+++ b/cloudinit/cmd/devel/hotplug_hook.py
@@ -182,7 +182,7 @@ def is_enabled(hotplug_init, subsystem):
     )
 
 
-def initialize_datasource(hotplug_init, subsystem):
+def initialize_datasource(hotplug_init: Init, subsystem: str):
     LOG.debug("Fetching datasource")
     datasource = hotplug_init.fetch(existing="trust")
 
@@ -220,8 +220,9 @@ def handle_hotplug(hotplug_init: Init, devpath, subsystem, udevaction):
         try:
             LOG.debug("Refreshing metadata")
             event_handler.update_metadata()
-            LOG.debug("Detecting device in updated metadata")
-            event_handler.detect_hotplugged_device()
+            if not datasource.skip_hotplug_detect:
+                LOG.debug("Detecting device in updated metadata")
+                event_handler.detect_hotplugged_device()
             LOG.debug("Applying config change")
             event_handler.apply()
             LOG.debug("Updating cache")
diff --git a/cloudinit/sources/DataSourceLXD.py b/cloudinit/sources/DataSourceLXD.py
index 177137ab..d873cd3d 100644
--- a/cloudinit/sources/DataSourceLXD.py
+++ b/cloudinit/sources/DataSourceLXD.py
@@ -6,7 +6,6 @@ Notes:
    still be detected on those images.
  * Detect LXD datasource when /dev/lxd/sock is an active socket file.
  * Info on dev-lxd API: https://linuxcontainers.org/lxd/docs/master/dev-lxd
- * TODO( Hotplug support using websockets API 1.0/events )
 """
 
 import os
@@ -14,7 +13,7 @@ import socket
 import stat
 from enum import Flag, auto
 from json.decoder import JSONDecodeError
-from typing import Any, Dict, Union, cast
+from typing import Any, Dict, List, Optional, Union, cast
 
 import requests
 from requests.adapters import HTTPAdapter
@@ -25,6 +24,7 @@ from urllib3.connectionpool import HTTPConnectionPool
 
 from cloudinit import log as logging
 from cloudinit import sources, subp, url_helper, util
+from cloudinit.net import find_fallback_nic
 
 LOG = logging.getLogger(__name__)
 
@@ -43,18 +43,8 @@ CONFIG_KEY_ALIASES = {
 }
 
 
-def generate_fallback_network_config() -> dict:
-    """Return network config V1 dict representing instance network config."""
-    network_v1: Dict[str, Any] = {
-        "version": 1,
-        "config": [
-            {
-                "type": "physical",
-                "name": "eth0",
-                "subnets": [{"type": "dhcp", "control": "auto"}],
-            }
-        ],
-    }
+def _get_fallback_interface_name() -> str:
+    default_name = "eth0"
     if subp.which("systemd-detect-virt"):
         try:
             virt_type, _ = subp.subp(["systemd-detect-virt"])
@@ -64,19 +54,43 @@ def generate_fallback_network_config() -> dict:
                 " Rendering default network config.",
                 err,
             )
-            return network_v1
+            return default_name
         if virt_type.strip() in (
             "kvm",
             "qemu",
         ):  # instance.type VIRTUAL-MACHINE
             arch = util.system_info()["uname"][4]
             if arch == "ppc64le":
-                network_v1["config"][0]["name"] = "enp0s5"
+                return "enp0s5"
             elif arch == "s390x":
-                network_v1["config"][0]["name"] = "enc9"
+                return "enc9"
             else:
-                network_v1["config"][0]["name"] = "enp5s0"
-    return network_v1
+                return "enp5s0"
+    return default_name
+
+
+def generate_network_config(
+    nics: Optional[List[str]] = None,
+) -> Dict[str, Any]:
+    """Return network config V1 dict representing instance network config."""
+    if not nics:
+        primary_nic = _get_fallback_interface_name()
+    elif len(nics) > 1:
+        fallback_nic = find_fallback_nic()
+        primary_nic = nics[0] if fallback_nic not in nics else fallback_nic
+    else:
+        primary_nic = nics[0]
+
+    return {
+        "version": 1,
+        "config": [
+            {
+                "type": "physical",
+                "name": primary_nic,
+                "subnets": [{"type": "dhcp", "control": "auto"}],
+            }
+        ],
+    }
 
 
 class SocketHTTPConnection(HTTPConnection):
@@ -146,6 +160,12 @@ class DataSourceLXD(sources.DataSource):
         "user.user-data",
     )
 
+    skip_hotplug_detect = True
+
+    def _unpickle(self, ci_pkl_version: int) -> None:
+        super()._unpickle(ci_pkl_version)
+        self.skip_hotplug_detect = True
+
     def _is_platform_viable(self) -> bool:
         """Check platform environment to report if this datasource may run."""
         return is_platform_viable()
@@ -207,14 +227,33 @@ class DataSourceLXD(sources.DataSource):
         if self._network_config == sources.UNSET:
             if self._crawled_metadata == sources.UNSET:
                 self._get_data()
-            if isinstance(
-                self._crawled_metadata, dict
-            ) and self._crawled_metadata.get("network-config"):
-                self._network_config = self._crawled_metadata.get(
-                    "network-config", {}
-                )
-            else:
-                self._network_config = generate_fallback_network_config()
+            if isinstance(self._crawled_metadata, dict):
+                if self._crawled_metadata.get("network-config"):
+                    LOG.debug("LXD datasource using provided network config")
+                    self._network_config = self._crawled_metadata[
+                        "network-config"
+                    ]
+                elif self._crawled_metadata.get("devices"):
+                    # If no explicit network config, but we have net devices
+                    # available to us, find the primary and set it up.
+                    devices: List[str] = [
+                        k
+                        for k, v in self._crawled_metadata["devices"].items()
+                        if v["type"] == "nic"
+                    ]
+                    LOG.debug(
+                        "LXD datasource generating network config using "
+                        "devices: %s",
+                        ", ".join(devices),
+                    )
+                    self._network_config = generate_network_config(devices)
+        if self._network_config == sources.UNSET:
+            # We know nothing about network, so setup fallback
+            LOG.debug(
+                "LXD datasource generating network config using fallback."
+            )
+            self._network_config = generate_network_config()
+
         return cast(dict, self._network_config)
 
 
@@ -338,13 +377,13 @@ class _MetaDataReader:
                 md.update(self._process_config(session))
             if MetaDataKeys.DEVICES in metadata_keys:
                 url = url_helper.combine_url(self._version_url, "devices")
-                md.update({"devices": _get_json_response(session, url)})
+                md["devices"] = _get_json_response(session, url)
             return md
 
 
 def read_metadata(
     api_version: str = LXD_SOCKET_API_VERSION,
-    metadata_keys: MetaDataKeys = MetaDataKeys.CONFIG | MetaDataKeys.META_DATA,
+    metadata_keys: MetaDataKeys = MetaDataKeys.ALL,
 ) -> dict:
     """Fetch metadata from the /dev/lxd/socket routes.
 
diff --git a/cloudinit/sources/__init__.py b/cloudinit/sources/__init__.py
index 3a483c26..85e094ac 100644
--- a/cloudinit/sources/__init__.py
+++ b/cloudinit/sources/__init__.py
@@ -251,6 +251,10 @@ class DataSource(CloudInitPickleMixin, metaclass=abc.ABCMeta):
         "security-credentials",
     )
 
+    # True on datasources that may not see hotplugged devices reflected
+    # in the updated metadata
+    skip_hotplug_detect = False
+
     _ci_pkl_version = 1
 
     def __init__(self, sys_cfg, distro: Distro, paths: Paths, ud_proc=None):
@@ -282,6 +286,8 @@ class DataSource(CloudInitPickleMixin, metaclass=abc.ABCMeta):
             self.vendordata2 = None
         if not hasattr(self, "vendordata2_raw"):
             self.vendordata2_raw = None
+        if not hasattr(self, "skip_hotplug_detect"):
+            self.skip_hotplug_detect = False
         if hasattr(self, "userdata") and self.userdata is not None:
             # If userdata stores MIME data, on < python3.6 it will be
             # missing the 'policy' attribute that exists on >=python3.6.
diff --git a/doc/rtd/topics/datasources/lxd.rst b/doc/rtd/topics/datasources/lxd.rst
index 99b42cfa..3b523d50 100644
--- a/doc/rtd/topics/datasources/lxd.rst
+++ b/doc/rtd/topics/datasources/lxd.rst
@@ -75,3 +75,36 @@ of static NoCloud seed files.
 
 .. _LXD socket device: https://linuxcontainers.org/lxd/docs/master/dev-lxd
 .. vi: textwidth=79
+
+Hotplug
+-------
+
+Network hotplug functionality is supported for the LXD datasource as described
+in the :ref:`events` documentation. As hotplug functionality relies on the
+cloud provided network metadata, the LXD datasource will only meaningfully
+react to a hotplug event if it has the configuration necessary to respond to
+the change has been provided to LXD. Practically, this means that
+even with hotplug enabled, **the default behavior for adding a new virtual
+NIC will result no change**.
+
+To update the configuration to be used by hotplug, first pass the network
+configuration via the ``cloud-init.network-config`` (or
+``user.network-config`` on older versions).
+
+For example, given an LXD instance named ``my-lxd`` with hotplug enabled and
+an LXD bridge named ``my-bridge``, the following will allow for additional
+DHCP configuration of ``eth1``:
+
+.. code-block:: shell-session
+
+    $ cat /tmp/cloud-network-config.yaml
+    version: 2
+    ethernets:
+        eth0:
+            dhcp4: true
+        eth1:
+            dhcp4: true
+
+    $ lxc config set my-lxd cloud-init.network-config="$(cat /tmp/cloud-network-config.yaml)"
+    $ lxc config device add my-lxd eth1 nic name=eth1 nictype=bridged parent=my-bridge
+    Device eth1 added to my-lxd
diff --git a/tests/integration_tests/datasources/test_lxd_discovery.py b/tests/integration_tests/datasources/test_lxd_discovery.py
index 806ec109..f3ca7158 100644
--- a/tests/integration_tests/datasources/test_lxd_discovery.py
+++ b/tests/integration_tests/datasources/test_lxd_discovery.py
@@ -86,9 +86,13 @@ def test_lxd_datasource_discovery(client: IntegrationInstance):
     assert "lxd" == v1["platform"]
     assert "LXD socket API v. 1.0 (/dev/lxd/sock)" == v1["subplatform"]
     ds_cfg = json.loads(client.execute("cloud-init query ds").stdout)
-    assert ["_doc", "_metadata_api_version", "config", "meta-data"] == sorted(
-        list(ds_cfg.keys())
-    )
+    assert [
+        "_doc",
+        "_metadata_api_version",
+        "config",
+        "devices",
+        "meta-data",
+    ] == sorted(list(ds_cfg.keys()))
     if (
         client.settings.PLATFORM == "lxd_vm"
         and ImageSpecification.from_os_image().release == "bionic"
diff --git a/tests/integration_tests/datasources/test_lxd_hotplug.py b/tests/integration_tests/datasources/test_lxd_hotplug.py
new file mode 100644
index 00000000..0768b902
--- /dev/null
+++ b/tests/integration_tests/datasources/test_lxd_hotplug.py
@@ -0,0 +1,153 @@
+import json
+
+import pytest
+
+from cloudinit import safeyaml
+from cloudinit.subp import subp
+from tests.integration_tests.clouds import ImageSpecification
+from tests.integration_tests.decorators import retry
+from tests.integration_tests.instances import IntegrationInstance
+from tests.integration_tests.util import lxd_has_nocloud
+
+USER_DATA = """\
+#cloud-config
+updates:
+  network:
+    when: ["hotplug"]
+"""
+
+UPDATED_NETWORK_CONFIG = """\
+version: 2
+ethernets:
+    eth0:
+        dhcp4: true
+    eth2:
+        dhcp4: true
+"""
+
+
+@retry()
+def ensure_hotplug_exited(client):
+    assert "cloud-init" not in client.execute("ps -A")
+
+
+def get_parent_network(instance_name: str):
+    lxd_network = json.loads(
+        subp("lxc network list --format json".split()).stdout
+    )
+    for net in lxd_network:
+        if net["type"] == "bridge" and net["managed"]:
+            if f"/1.0/instances/{instance_name}" in net.get("used_by", []):
+                return net["name"]
+    return "lxdbr0"
+
+
+def _prefer_lxd_datasource_over_nocloud(client: IntegrationInstance):
+    """For hotplug support we need LXD datasource detected instead of NoCloud
+
+    Bionic and Focal still deliver nocloud-net seed files so override it
+    with /etc/cloud/cloud.cfg.d/99-detect-lxd-first.cfg
+    """
+    client.write_to_file(
+        "/etc/cloud/cloud.cfg.d/99-detect-lxd-first.cfg",
+        "datasource_list: [LXD, NoCloud]\n",
+    )
+    client.execute("cloud-init clean --logs")
+    client.restart()
+
+
+@pytest.mark.lxd_container
+@pytest.mark.lxd_vm
+@pytest.mark.user_data(USER_DATA)
+class TestLxdHotplug:
+    @pytest.fixture(autouse=True, scope="class")
+    def class_teardown(self, class_client: IntegrationInstance):
+        # We need a teardown here because on IntegrationInstance teardown,
+        # if KEEP_INSTANCE=True, we grab the instance IP for logging, but
+        # we're currently running into
+        # https://github.com/canonical/pycloudlib/issues/220 .
+        # Once that issue is fixed, we can remove this teardown
+        yield
+        name = class_client.instance.name
+        subp(f"lxc config device remove {name} eth1".split())
+        subp(f"lxc config device remove {name} eth2".split())
+        subp("lxc network delete ci-test-br-eth1".split())
+        subp("lxc network delete ci-test-br-eth2".split())
+
+    def test_no_network_change_default(
+        self, class_client: IntegrationInstance
+    ):
+        client = class_client
+        if lxd_has_nocloud(client):
+            _prefer_lxd_datasource_over_nocloud(client)
+        assert "eth1" not in client.execute("ip address")
+        pre_netplan = client.read_from_file("/etc/netplan/50-cloud-init.yaml")
+
+        networks = subp("lxc network list".split())
+        if "ci-test-br-eth1" not in networks.stdout:
+            subp(
+                "lxc network create ci-test-br-eth1 --type=bridge "
+                "ipv4.address=10.10.41.1/24 ipv4.nat=true".split()
+            )
+        subp(
+            f"lxc config device add {client.instance.name} eth1 nic name=eth1 "
+            f"nictype=bridged parent=ci-test-br-eth1".split()
+        )
+        ensure_hotplug_exited(client)
+        post_netplan = client.read_from_file("/etc/netplan/50-cloud-init.yaml")
+        assert pre_netplan == post_netplan
+        ip_info = json.loads(client.execute("ip --json address"))
+        eth1s = [i for i in ip_info if i["ifname"] == "eth1"]
+        assert len(eth1s) == 1
+        assert eth1s[0]["operstate"] == "DOWN"
+
+    def test_network_config_applied(self, class_client: IntegrationInstance):
+        client = class_client
+        if lxd_has_nocloud(client):
+            _prefer_lxd_datasource_over_nocloud(client)
+        assert "eth2" not in client.execute("ip address")
+        pre_netplan = client.read_from_file("/etc/netplan/50-cloud-init.yaml")
+        assert "eth2" not in pre_netplan
+        if ImageSpecification.from_os_image().release in [
+            "bionic",
+            "focal",
+        ]:  # pyright: ignore
+            top_key = "user"
+        else:
+            top_key = "cloud-init"
+        assert subp(
+            [
+                "lxc",
+                "config",
+                "set",
+                client.instance.name,
+                f"{top_key}.network-config={UPDATED_NETWORK_CONFIG}",
+            ]
+        )
+        assert (
+            client.read_from_file("/etc/netplan/50-cloud-init.yaml")
+            == pre_netplan
+        )
+        networks = subp("lxc network list".split())
+        if "ci-test-br-eth2" not in networks.stdout:
+            assert subp(
+                "lxc network create ci-test-br-eth2 --type=bridge"
+                " ipv4.address=10.10.42.1/24 ipv4.nat=true".split()
+            )
+        assert subp(
+            f"lxc config device add {client.instance.name} eth2 nic name=eth2 "
+            f"nictype=bridged parent=ci-test-br-eth2".split()
+        )
+        ensure_hotplug_exited(client)
+        post_netplan = safeyaml.load(
+            client.read_from_file("/etc/netplan/50-cloud-init.yaml")
+        )
+        expected_netplan = safeyaml.load(UPDATED_NETWORK_CONFIG)
+        expected_netplan = {"network": expected_netplan}
+        assert post_netplan == expected_netplan, client.read_from_file(
+            "/var/log/cloud-init.log"
+        )
+        ip_info = json.loads(client.execute("ip --json address"))
+        eth2s = [i for i in ip_info if i["ifname"] == "eth2"]
+        assert len(eth2s) == 1
+        assert eth2s[0]["operstate"] == "UP"
diff --git a/tests/unittests/cmd/devel/test_hotplug_hook.py b/tests/unittests/cmd/devel/test_hotplug_hook.py
index d2ef82b1..b1372925 100644
--- a/tests/unittests/cmd/devel/test_hotplug_hook.py
+++ b/tests/unittests/cmd/devel/test_hotplug_hook.py
@@ -24,6 +24,7 @@ def mocks():
     m_distro.network_activator = mock.PropertyMock(return_value=m_activator)
     m_datasource = mock.MagicMock(spec=DataSource)
     m_datasource.distro = m_distro
+    m_datasource.skip_hotplug_detect = False
     m_init.datasource = m_datasource
     m_init.fetch.return_value = m_datasource
 
@@ -80,8 +81,8 @@ class TestUnsupportedActions:
             handle_hotplug(
                 hotplug_init=mocks.m_init,
                 devpath="/dev/fake",
-                udevaction="not_real",
                 subsystem="net",
+                udevaction="not_real",
             )
 
 
@@ -122,6 +123,21 @@ class TestHotplug:
         mocks.m_activator.bring_up_interface.assert_not_called()
         init._write_to_cache.assert_called_once_with()
 
+    @mock.patch(
+        "cloudinit.cmd.devel.hotplug_hook.NetHandler.detect_hotplugged_device"
+    )
+    @pytest.mark.parametrize("skip", [True, False])
+    def test_skip_detected(self, m_detect, skip, mocks):
+        mocks.m_init.datasource.skip_hotplug_detect = skip
+        expected_call_count = 0 if skip else 1
+        handle_hotplug(
+            hotplug_init=mocks.m_init,
+            devpath="/dev/fake",
+            udevaction="add",
+            subsystem="net",
+        )
+        assert m_detect.call_count == expected_call_count
+
     def test_update_event_disabled(self, mocks, caplog):
         init = mocks.m_init
         with mock.patch(
diff --git a/tests/unittests/sources/test_lxd.py b/tests/unittests/sources/test_lxd.py
index 0cb54e22..96bd37a0 100644
--- a/tests/unittests/sources/test_lxd.py
+++ b/tests/unittests/sources/test_lxd.py
@@ -1,5 +1,6 @@
 # This file is part of cloud-init. See LICENSE file for license information.
 
+import copy
 import json
 import re
 import stat
@@ -62,6 +63,29 @@ LXD_V1_METADATA_NO_NETWORK_CONFIG = {
     },
 }
 
+DEVICES = {
+    "devices": {
+        "some-disk": {
+            "path": "/path/in/container",
+            "source": "/path/on/host",
+            "type": "disk",
+        },
+        "enp1s0": {
+            "ipv4.address": "10.20.30.40",
+            "name": "eth0",
+            "network": "lxdbr0",
+            "type": "nic",
+        },
+        "root": {"path": "/", "pool": "default", "type": "disk"},
+        "enp1s1": {
+            "ipv4.address": "10.20.30.50",
+            "name": "eth1",
+            "network": "lxdbr0",
+            "type": "nic",
+        },
+    }
+}
+
 
 def lxd_metadata():
     return LXD_V1_METADATA
@@ -143,7 +167,7 @@ class TestGenerateFallbackNetworkConfig:
             m_which.return_value = None
         m_system_info.return_value = {"uname": ["", "", "", "", uname_machine]}
         m_subp.return_value = (systemd_detect_virt, "")
-        assert expected == lxd.generate_fallback_network_config()
+        assert expected == lxd.generate_network_config()
         if systemd_detect_virt is None:
             assert 0 == m_subp.call_count
             assert 0 == m_system_info.call_count
@@ -157,6 +181,111 @@ class TestGenerateFallbackNetworkConfig:
                 assert 1 == m_system_info.call_count
 
 
+class TestNetworkConfig:
+    @pytest.fixture(autouse=True)
+    def mocks(self, mocker):
+        mocker.patch(f"{DS_PATH}subp.subp", return_value=("whatever", ""))
+
+    def test_provided_network_config(self, lxd_ds, mocker):
+        def _get_data(self):
+            self._crawled_metadata = copy.deepcopy(DEVICES)
+            self._crawled_metadata["network-config"] = "hi"
+
+        mocker.patch.object(
+            lxd.DataSourceLXD,
+            "_get_data",
+            autospec=True,
+            side_effect=_get_data,
+        )
+        assert lxd_ds.network_config == "hi"
+
+    @pytest.mark.parametrize(
+        "devices_to_remove,expected_config",
+        [
+            pytest.param(
+                # When two nics are presented with no passed network-config,
+                # Never configure more than one device.
+                # Always choose lowest sorted device over higher
+                # Always configure with DHCP
+                [],
+                {
+                    "version": 1,
+                    "config": [
+                        {
+                            "name": "enp1s0",
+                            "subnets": [{"control": "auto", "type": "dhcp"}],
+                            "type": "physical",
+                        }
+                    ],
+                },
+                id="multi-device",
+            ),
+            pytest.param(
+                # When one device is presented, use it
+                ["enp1s0"],
+                {
+                    "version": 1,
+                    "config": [
+                        {
+                            "name": "enp1s1",
+                            "subnets": [{"control": "auto", "type": "dhcp"}],
+                            "type": "physical",
+                        }
+                    ],
+                },
+                id="no-eth0",
+            ),
+            pytest.param(
+                # When one device is presented, use it
+                ["enp1s1"],
+                {
+                    "version": 1,
+                    "config": [
+                        {
+                            "name": "enp1s0",
+                            "subnets": [{"control": "auto", "type": "dhcp"}],
+                            "type": "physical",
+                        }
+                    ],
+                },
+                id="no-eth1",
+            ),
+            pytest.param(
+                # When no devices are presented, generate fallback
+                ["enp1s0", "enp1s1"],
+                {
+                    "version": 1,
+                    "config": [
+                        {
+                            "name": "eth0",
+                            "subnets": [{"control": "auto", "type": "dhcp"}],
+                            "type": "physical",
+                        }
+                    ],
+                },
+                id="device-list-empty",
+            ),
+        ],
+    )
+    def test_provided_devices(
+        self, devices_to_remove, expected_config, lxd_ds, mocker
+    ):
+        devices = copy.deepcopy(DEVICES)
+        for name in devices_to_remove:
+            del devices["devices"][name]
+
+        def _get_data(self):
+            self._crawled_metadata = devices
+
+        mocker.patch.object(
+            lxd.DataSourceLXD,
+            "_get_data",
+            autospec=True,
+            side_effect=_get_data,
+        )
+        assert lxd_ds.network_config == expected_config
+
+
 class TestDataSourceLXD:
     def test_platform_info(self, lxd_ds):
         assert "LXD" == lxd_ds.dsname
@@ -196,9 +325,7 @@ class TestDataSourceLXD:
         """network_config is correctly computed when _network_config is unset
         and _crawled_metadata does not contain network_config.
         """
-        lxd.generate_fallback_network_config = mock.Mock(
-            return_value=NETWORK_V1
-        )
+        lxd.generate_network_config = mock.Mock(return_value=NETWORK_V1)
         assert UNSET == lxd_ds_no_network_config._crawled_metadata
         assert UNSET == lxd_ds_no_network_config._network_config
         assert None is lxd_ds_no_network_config.userdata_raw
@@ -208,7 +335,7 @@ class TestDataSourceLXD:
             LXD_V1_METADATA_NO_NETWORK_CONFIG
             == lxd_ds_no_network_config._crawled_metadata
         )
-        assert 1 == lxd.generate_fallback_network_config.call_count
+        assert 1 == lxd.generate_network_config.call_count
 
 
 class TestIsPlatformViable: