Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 37 additions & 3 deletions cloudinit/config/cc_lxd.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@
"""LXD: configure lxd with ``lxd init`` and optionally lxd-bridge"""

import os
from logging import Logger
from textwrap import dedent
from typing import List
from typing import List, Tuple

from cloudinit import log as logging
from cloudinit import subp, util
from cloudinit.cloud import Cloud
from cloudinit.config.schema import MetaSchema, get_meta_doc
from cloudinit.settings import PER_INSTANCE

Expand Down Expand Up @@ -78,7 +80,7 @@
__doc__ = get_meta_doc(meta)


def handle(name, cfg, cloud, log, args):
def handle(name, cfg, cloud: Cloud, log: Logger, args):
# Get config
lxd_cfg = cfg.get("lxd")
if not lxd_cfg:
Expand Down Expand Up @@ -118,7 +120,10 @@ def handle(name, cfg, cloud, log, args):

# Set up lxd if init config is given
if init_cfg:
init_keys = (

# type is known, number of elements is not
# in the case of the ubuntu+lvm backend workaround
init_keys: Tuple[str, ...] = (
"network_address",
"network_port",
"storage_backend",
Expand All @@ -127,7 +132,36 @@ def handle(name, cfg, cloud, log, args):
"storage_pool",
"trust_password",
)

subp.subp(["lxd", "waitready", "--timeout=300"])

# Bug https://bugs.launchpad.net/ubuntu/+source/linux-kvm/+bug/1982780
kernel = util.system_info()["uname"][2]
if init_cfg["storage_backend"] == "lvm" and not os.path.exists(
f"/lib/modules/{kernel}/kernel/drivers/md/dm-thin-pool.ko"
):
log.warning(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue isn't limited to ubuntu, it's possible that this is a problem on any given OS/kernel if the right kernel modules are not included.

I think we instead want to perform a check to see if the dm-thin-pool module is present for the active kernel.

We can either do that through:

  1. file test on required dm-thin-pools module
kernel = util.system_info()["uname"][2])
if not os.path.exists(f"/lib/modules/{kernel}/kernel/drivers/md/dm-thin-pool.ko):
   ...log.warn and setup w/ lvm.use_thinpool=False....

2 subp.subp(['modinfo', 'dm-thin-pool']) and check if we hit ProcesseExecutionError "modinfo: ERROR: Module dm-thin-pools not found.".

I'd lean toward solution #1 because solution #2 requires either a strict package dependency on kmod or optional runtime package dependency on kmow which I don't think we should grow broadly in cloud-init for this one off use-case when all we need it for is a file check.

Copy link
Copy Markdown
Member Author

@holmanb holmanb Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue isn't limited to ubuntu

Since this config module is limited to ubuntu, I think it is ubuntu-specific.

I think we instead want to perform a check to see if the dm-thin-pool module is present for the active kernel.

This approach causes a behavioral change that would be unpleasant to debug: kmod package update will change cloud-init behavior under this scheme. However, since we're logging this when the kmod is missing, I guess there is a breadcrumb.

I'll grab the first option in case we want to extend cc_lxd support to other linux distros.

Thanks!

Copy link
Copy Markdown
Member Author

@holmanb holmanb Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kernel = util.system_info()["uname"][2])

Note to self: If we get rid of the cast-to-list in system_info(), we get a named tuple for free (and more readable code).

Copy link
Copy Markdown
Collaborator

@blackboxsw blackboxsw Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 I was thinking the same, though we'd maybe have to look at how that is represented in /run/cloud-init/instance-data.json to be sure we aren't doing some funky object-style annotations in the JSON output as I believe this data is surfaced there too.

"cloud-init doesn't use thinpool by default on Ubuntu due to "
"LP #1982780. This behavior will change in the future.",
)
subp.subp(
[
"lxc",
"storage",
"create",
"default",
"lvm",
"lvm.use_thinpool=false",
]
)

# Since we're manually setting use_thinpool=false
# filter it from the lxd init commands, don't configure
# storage twice
init_keys = tuple(
key for key in init_keys if key != "storage_backend"
)

cmd = ["lxd", "init", "--auto"]
for k in init_keys:
if init_cfg.get(k):
Expand Down
101 changes: 101 additions & 0 deletions tests/integration_tests/modules/test_lxd.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
"""Integration tests for LXD bridge creation.

(This is ported from
``tests/cloud_tests/testcases/modules/lxd_bridge.yaml``.)
"""
import re
import warnings

import pytest
import yaml

from tests.integration_tests.util import verify_clean_log

BRIDGE_USER_DATA = """\
#cloud-config
lxd:
init:
storage_backend: btrfs
bridge:
mode: new
name: lxdbr0
ipv4_address: 10.100.100.1
ipv4_netmask: 24
ipv4_dhcp_first: 10.100.100.100
ipv4_dhcp_last: 10.100.100.200
ipv4_nat: true
domain: lxd
mtu: 9000
"""

STORAGE_USER_DATA = """\
#cloud-config
bootcmd: [ "apt-get --yes remove {0}", "! command -v {2}", "{3}" ]
lxd:
init:
storage_backend: {1}
"""


@pytest.mark.no_container
@pytest.mark.user_data(BRIDGE_USER_DATA)
class TestLxdBridge:
@pytest.mark.parametrize("binary_name", ["lxc", "lxd"])
def test_binaries_installed(self, class_client, binary_name):
"""Check that the expected LXD binaries are installed"""
assert class_client.execute(["which", binary_name]).ok

def test_bridge(self, class_client):
"""Check that the given bridge is configured"""
cloud_init_log = class_client.read_from_file("/var/log/cloud-init.log")
verify_clean_log(cloud_init_log)

# The bridge should exist
assert class_client.execute("ip addr show lxdbr0")

raw_network_config = class_client.execute("lxc network show lxdbr0")
network_config = yaml.safe_load(raw_network_config)
assert "10.100.100.1/24" == network_config["config"]["ipv4.address"]


def validate_storage(validate_client, pkg_name, command):
log = validate_client.read_from_file("/var/log/cloud-init.log")
assert re.search(f"apt-get.*install.*{pkg_name}", log) is not None
verify_clean_log(log, ignore_deprecations=False)
return log


@pytest.mark.no_container
@pytest.mark.user_data(
STORAGE_USER_DATA.format("btrfs-progs", "btrfs", "mkfs.btrfs", "true")
)
def test_storage_btrfs(client):
validate_storage(client, "btrfs-progs", "mkfs.btrfs")


@pytest.mark.no_container
@pytest.mark.user_data(
STORAGE_USER_DATA.format(
"lvm2",
"lvm",
"lvcreate",
"apt-get install "
"thin-provisioning-tools && systemctl unmask lvm2-lvmpolld.socket",
)
)
def test_storage_lvm(client):
log = client.read_from_file("/var/log/cloud-init.log")

# Note to self
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one!

if "doesn't use thinpool by default on Ubuntu due to LP" not in log:
warnings.warn("LP 1982780 has been fixed, update to allow thinpools")

validate_storage(client, "lvm2", "lvcreate")


@pytest.mark.no_container
@pytest.mark.user_data(
STORAGE_USER_DATA.format("zfsutils-linux", "zfs", "zpool", "true")
)
def test_storage_zfs(client):
validate_storage(client, "zfsutils-linux", "zpool")
48 changes: 0 additions & 48 deletions tests/integration_tests/modules/test_lxd_bridge.py

This file was deleted.

4 changes: 3 additions & 1 deletion tests/integration_tests/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,9 @@ def verify_clean_log(log: str, ignore_deprecations: bool = True):
warning_texts = [
# Consistently on all Azure launches:
# azure.py[WARNING]: No lease found; using default endpoint
"No lease found; using default endpoint"
"No lease found; using default endpoint",
# Ubuntu lxd storage
"thinpool by default on Ubuntu due to LP #1982780",
]
traceback_texts = []
if "oracle" in log:
Expand Down
33 changes: 25 additions & 8 deletions tests/unittests/config/test_cc_lxd.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,30 +35,34 @@ class TestLxd(t_help.CiTestCase):
("dir", None, None),
)

@mock.patch("cloudinit.config.cc_lxd.util.system_info")
@mock.patch("cloudinit.config.cc_lxd.os.path.exists", return_value=True)
@mock.patch("cloudinit.config.cc_lxd.subp.subp", return_value=True)
@mock.patch("cloudinit.config.cc_lxd.subp.which", return_value=False)
@mock.patch(
"cloudinit.config.cc_lxd.maybe_cleanup_default", return_value=None
)
def test_lxd_init(self, m_maybe_clean, m_which, m_subp):
def test_lxd_init(self, maybe_clean, which, subp, exists, system_info):
system_info.return_value = {"uname": [0, 1, "mykernel"]}
cc = get_cloud(mocked_distro=True)
m_install = cc.distro.install_packages
install = cc.distro.install_packages

for backend, cmd, package in self.backend_def:
lxd_cfg = deepcopy(self.lxd_cfg)
lxd_cfg["lxd"]["init"]["storage_backend"] = backend
m_subp.call_args_list = []
m_install.call_args_list = []
subp.call_args_list = []
install.call_args_list = []
exists.call_args_list = []
cc_lxd.handle("cc_lxd", lxd_cfg, cc, self.logger, [])
if cmd:
m_which.assert_called_with(cmd)
which.assert_called_with(cmd)
# no bridge config, so maybe_cleanup should not be called.
self.assertFalse(m_maybe_clean.called)
self.assertFalse(maybe_clean.called)
self.assertEqual(
[
mock.call(list(filter(None, ["lxd", package]))),
],
m_install.call_args_list,
install.call_args_list,
)
self.assertEqual(
[
Expand All @@ -74,9 +78,22 @@ def test_lxd_init(self, m_maybe_clean, m_which, m_subp):
]
),
],
m_subp.call_args_list,
subp.call_args_list,
)

if backend == "lvm":
self.assertEqual(
[
mock.call(
"/lib/modules/mykernel/"
"kernel/drivers/md/dm-thin-pool.ko"
)
],
exists.call_args_list,
)
else:
self.assertEqual([], exists.call_args_list)

@mock.patch("cloudinit.config.cc_lxd.subp.which", return_value=False)
def test_lxd_package_install(self, m_which):
for backend, _, package in self.backend_def:
Expand Down