From f5b3ad741679cd42d2c145e574168dafe3ac15c1 Mon Sep 17 00:00:00 2001 From: Daniel Watkins Date: Fri, 23 Oct 2020 15:20:18 -0400 Subject: stages: don't reset permissions of cloud-init.log every boot (#624) ensure_file needed modification to support doing this, so this commit also includes the following changes: test_util: add tests for util.ensure_file util: add preserve_mode parameter to ensure_file util: add (partial) type annotations to ensure_file LP: #1900837 --- cloudinit/tests/test_stages.py | 62 ++++++++++++++++++++++++++++++++++++++++++ cloudinit/tests/test_util.py | 45 ++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+) (limited to 'cloudinit/tests') diff --git a/cloudinit/tests/test_stages.py b/cloudinit/tests/test_stages.py index d5c9c0e4..d2d1b37f 100644 --- a/cloudinit/tests/test_stages.py +++ b/cloudinit/tests/test_stages.py @@ -3,6 +3,9 @@ """Tests related to cloudinit.stages module.""" import os +import stat + +import pytest from cloudinit import stages from cloudinit import sources @@ -341,4 +344,63 @@ class TestInit(CiTestCase): self.init.distro.apply_network_config.assert_called_with( net_cfg, bring_up=True) + +class TestInit_InitializeFilesystem: + """Tests for cloudinit.stages.Init._initialize_filesystem. + + TODO: Expand these tests to cover all of _initialize_filesystem's behavior. + """ + + @pytest.yield_fixture + def init(self, paths): + """A fixture which yields a stages.Init instance with paths and cfg set + + As it is replaced with a mock, consumers of this fixture can set + `init.cfg` if the default empty dict configuration is not appropriate. + """ + with mock.patch( + "cloudinit.stages.Init.cfg", mock.PropertyMock(return_value={}) + ): + with mock.patch("cloudinit.stages.util.ensure_dirs"): + init = stages.Init() + init._paths = paths + yield init + + @mock.patch("cloudinit.stages.util.ensure_file") + def test_ensure_file_not_called_if_no_log_file_configured( + self, m_ensure_file, init + ): + """If no log file is configured, we should not ensure its existence.""" + init.cfg = {} + + init._initialize_filesystem() + + assert 0 == m_ensure_file.call_count + + def test_log_files_existence_is_ensured_if_configured(self, init, tmpdir): + """If a log file is configured, we should ensure its existence.""" + log_file = tmpdir.join("cloud-init.log") + init.cfg = {"def_log_file": str(log_file)} + + init._initialize_filesystem() + + assert log_file.exists + + def test_existing_file_permissions_are_not_modified(self, init, tmpdir): + """If the log file already exists, we should not modify its permissions + + See https://bugs.launchpad.net/cloud-init/+bug/1900837. + """ + # Use a mode that will never be made the default so this test will + # always be valid + mode = 0o606 + log_file = tmpdir.join("cloud-init.log") + log_file.ensure() + log_file.chmod(mode) + init.cfg = {"def_log_file": str(log_file)} + + init._initialize_filesystem() + + assert mode == stat.S_IMODE(log_file.stat().mode) + # vi: ts=4 expandtab diff --git a/cloudinit/tests/test_util.py b/cloudinit/tests/test_util.py index 096a3037..77714928 100644 --- a/cloudinit/tests/test_util.py +++ b/cloudinit/tests/test_util.py @@ -771,4 +771,49 @@ class TestMountCb: ] == callback.call_args_list +@mock.patch("cloudinit.util.write_file") +class TestEnsureFile: + """Tests for ``cloudinit.util.ensure_file``.""" + + def test_parameters_passed_through(self, m_write_file): + """Test the parameters in the signature are passed to write_file.""" + util.ensure_file( + mock.sentinel.path, + mode=mock.sentinel.mode, + preserve_mode=mock.sentinel.preserve_mode, + ) + + assert 1 == m_write_file.call_count + args, kwargs = m_write_file.call_args + assert (mock.sentinel.path,) == args + assert mock.sentinel.mode == kwargs["mode"] + assert mock.sentinel.preserve_mode == kwargs["preserve_mode"] + + @pytest.mark.parametrize( + "kwarg,expected", + [ + # Files should be world-readable by default + ("mode", 0o644), + # The previous behaviour of not preserving mode should be retained + ("preserve_mode", False), + ], + ) + def test_defaults(self, m_write_file, kwarg, expected): + """Test that ensure_file defaults appropriately.""" + util.ensure_file(mock.sentinel.path) + + assert 1 == m_write_file.call_count + _args, kwargs = m_write_file.call_args + assert expected == kwargs[kwarg] + + def test_static_parameters_are_passed(self, m_write_file): + """Test that the static write_files parameters are passed correctly.""" + util.ensure_file(mock.sentinel.path) + + assert 1 == m_write_file.call_count + _args, kwargs = m_write_file.call_args + assert "" == kwargs["content"] + assert "ab" == kwargs["omode"] + + # vi: ts=4 expandtab -- cgit v1.2.3 From 34f8e2213c42106b3e1568b5c5aac5565df954e3 Mon Sep 17 00:00:00 2001 From: Mina Galić Date: Mon, 2 Nov 2020 18:01:41 +0100 Subject: util: fix mounting of vfat on *BSD (#637) Fix mounting of vfat filesystems by normalizing the different names for vfat to "msdos" which works across BSDs. --- cloudinit/tests/test_util.py | 35 +++++++++++++++++++++++++++++++++++ cloudinit/util.py | 14 ++++++++++---- 2 files changed, 45 insertions(+), 4 deletions(-) (limited to 'cloudinit/tests') diff --git a/cloudinit/tests/test_util.py b/cloudinit/tests/test_util.py index 77714928..b7a302f1 100644 --- a/cloudinit/tests/test_util.py +++ b/cloudinit/tests/test_util.py @@ -730,6 +730,41 @@ class TestMountCb: """already_mounted_device_and_mountdict, but return only the device""" return already_mounted_device_and_mountdict[0] + @pytest.mark.parametrize( + "mtype,expected", + [ + # While the filesystem is called iso9660, the mount type is cd9660 + ("iso9660", "cd9660"), + # vfat is generally called "msdos" on BSD + ("vfat", "msdos"), + # judging from man pages, only FreeBSD has this alias + ("msdosfs", "msdos"), + # Test happy path + ("ufs", "ufs") + ], + ) + @mock.patch("cloudinit.util.is_Linux", autospec=True) + @mock.patch("cloudinit.util.is_BSD", autospec=True) + @mock.patch("cloudinit.util.subp.subp") + @mock.patch("cloudinit.temp_utils.tempdir", autospec=True) + def test_normalize_mtype_on_bsd( + self, m_tmpdir, m_subp, m_is_BSD, m_is_Linux, mtype, expected + ): + m_is_BSD.return_value = True + m_is_Linux.return_value = False + m_tmpdir.return_value.__enter__ = mock.Mock( + autospec=True, return_value="/tmp/fake" + ) + m_tmpdir.return_value.__exit__ = mock.Mock( + autospec=True, return_value=True + ) + callback = mock.Mock(autospec=True) + + util.mount_cb('/dev/fake0', callback, mtype=mtype) + assert mock.call( + ["mount", "-o", "ro", "-t", expected, "/dev/fake0", "/tmp/fake"], + update_env=None) in m_subp.call_args_list + @pytest.mark.parametrize("invalid_mtype", [int(0), float(0.0), dict()]) def test_typeerror_raised_for_invalid_mtype(self, invalid_mtype): with pytest.raises(TypeError): diff --git a/cloudinit/util.py b/cloudinit/util.py index 83727544..b8856af1 100644 --- a/cloudinit/util.py +++ b/cloudinit/util.py @@ -417,6 +417,11 @@ def multi_log(text, console=True, stderr=True, log.log(log_level, text) +@lru_cache() +def is_Linux(): + return 'Linux' in platform.system() + + @lru_cache() def is_BSD(): return 'BSD' in platform.system() @@ -1661,16 +1666,17 @@ def mount_cb(device, callback, data=None, mtype=None, _type=type(mtype))) # clean up 'mtype' input a bit based on platform. - platsys = platform.system().lower() - if platsys == "linux": + if is_Linux(): if mtypes is None: mtypes = ["auto"] - elif platsys.endswith("bsd"): + elif is_BSD(): if mtypes is None: - mtypes = ['ufs', 'cd9660', 'vfat'] + mtypes = ['ufs', 'cd9660', 'msdos'] for index, mtype in enumerate(mtypes): if mtype == "iso9660": mtypes[index] = "cd9660" + if mtype in ["vfat", "msdosfs"]: + mtypes[index] = "msdos" else: # we cannot do a smart "auto", so just call 'mount' once with no -t mtypes = [''] -- cgit v1.2.3 From 0af1ff1eaf593c325b4f53181a572110eb016c50 Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Mon, 2 Nov 2020 15:41:11 -0500 Subject: cloudinit: move dmi functions out of util (#622) This just separates the reading of dmi values into its own file. Some things of note: * left import of util in dmi.py only for 'is_container' It'd be good if is_container was not in util. * just the use of 'util.is_x86' to dmi.py * open() is used directly rather than load_file. --- cloudinit/dmi.py | 131 ++++++++++++++++++++++ cloudinit/sources/DataSourceAliYun.py | 4 +- cloudinit/sources/DataSourceAltCloud.py | 3 +- cloudinit/sources/DataSourceAzure.py | 5 +- cloudinit/sources/DataSourceCloudSigma.py | 4 +- cloudinit/sources/DataSourceEc2.py | 9 +- cloudinit/sources/DataSourceExoscale.py | 3 +- cloudinit/sources/DataSourceGCE.py | 5 +- cloudinit/sources/DataSourceHetzner.py | 5 +- cloudinit/sources/DataSourceNoCloud.py | 3 +- cloudinit/sources/DataSourceOVF.py | 5 +- cloudinit/sources/DataSourceOpenStack.py | 5 +- cloudinit/sources/DataSourceOracle.py | 5 +- cloudinit/sources/DataSourceScaleway.py | 3 +- cloudinit/sources/DataSourceSmartOS.py | 3 +- cloudinit/sources/__init__.py | 3 +- cloudinit/sources/helpers/digitalocean.py | 5 +- cloudinit/sources/tests/test_oracle.py | 6 +- cloudinit/tests/test_dmi.py | 131 ++++++++++++++++++++++ cloudinit/util.py | 119 -------------------- tests/unittests/test_datasource/test_aliyun.py | 6 +- tests/unittests/test_datasource/test_altcloud.py | 21 ++-- tests/unittests/test_datasource/test_azure.py | 10 +- tests/unittests/test_datasource/test_nocloud.py | 3 +- tests/unittests/test_datasource/test_openstack.py | 16 +-- tests/unittests/test_datasource/test_ovf.py | 16 +-- tests/unittests/test_datasource/test_scaleway.py | 8 +- tests/unittests/test_util.py | 123 -------------------- 28 files changed, 348 insertions(+), 312 deletions(-) create mode 100644 cloudinit/dmi.py create mode 100644 cloudinit/tests/test_dmi.py (limited to 'cloudinit/tests') diff --git a/cloudinit/dmi.py b/cloudinit/dmi.py new file mode 100644 index 00000000..96e0e423 --- /dev/null +++ b/cloudinit/dmi.py @@ -0,0 +1,131 @@ +# This file is part of cloud-init. See LICENSE file for license information. +from cloudinit import log as logging +from cloudinit import subp +from cloudinit.util import is_container + +import os + +LOG = logging.getLogger(__name__) + +# Path for DMI Data +DMI_SYS_PATH = "/sys/class/dmi/id" + +# dmidecode and /sys/class/dmi/id/* use different names for the same value, +# this allows us to refer to them by one canonical name +DMIDECODE_TO_DMI_SYS_MAPPING = { + 'baseboard-asset-tag': 'board_asset_tag', + 'baseboard-manufacturer': 'board_vendor', + 'baseboard-product-name': 'board_name', + 'baseboard-serial-number': 'board_serial', + 'baseboard-version': 'board_version', + 'bios-release-date': 'bios_date', + 'bios-vendor': 'bios_vendor', + 'bios-version': 'bios_version', + 'chassis-asset-tag': 'chassis_asset_tag', + 'chassis-manufacturer': 'chassis_vendor', + 'chassis-serial-number': 'chassis_serial', + 'chassis-version': 'chassis_version', + 'system-manufacturer': 'sys_vendor', + 'system-product-name': 'product_name', + 'system-serial-number': 'product_serial', + 'system-uuid': 'product_uuid', + 'system-version': 'product_version', +} + + +def _read_dmi_syspath(key): + """ + Reads dmi data with from /sys/class/dmi/id + """ + if key not in DMIDECODE_TO_DMI_SYS_MAPPING: + return None + mapped_key = DMIDECODE_TO_DMI_SYS_MAPPING[key] + dmi_key_path = "{0}/{1}".format(DMI_SYS_PATH, mapped_key) + + LOG.debug("querying dmi data %s", dmi_key_path) + if not os.path.exists(dmi_key_path): + LOG.debug("did not find %s", dmi_key_path) + return None + + try: + with open(dmi_key_path, "rb") as fp: + key_data = fp.read() + except PermissionError: + LOG.debug("Could not read %s", dmi_key_path) + return None + + # uninitialized dmi values show as all \xff and /sys appends a '\n'. + # in that event, return empty string. + if key_data == b'\xff' * (len(key_data) - 1) + b'\n': + key_data = b"" + + try: + return key_data.decode('utf8').strip() + except UnicodeDecodeError as e: + LOG.error("utf-8 decode of content (%s) in %s failed: %s", + dmi_key_path, key_data, e) + + return None + + +def _call_dmidecode(key, dmidecode_path): + """ + Calls out to dmidecode to get the data out. This is mostly for supporting + OS's without /sys/class/dmi/id support. + """ + try: + cmd = [dmidecode_path, "--string", key] + (result, _err) = subp.subp(cmd) + result = result.strip() + LOG.debug("dmidecode returned '%s' for '%s'", result, key) + if result.replace(".", "") == "": + return "" + return result + except (IOError, OSError) as e: + LOG.debug('failed dmidecode cmd: %s\n%s', cmd, e) + return None + + +def read_dmi_data(key): + """ + Wrapper for reading DMI data. + + If running in a container return None. This is because DMI data is + assumed to be not useful in a container as it does not represent the + container but rather the host. + + This will do the following (returning the first that produces a + result): + 1) Use a mapping to translate `key` from dmidecode naming to + sysfs naming and look in /sys/class/dmi/... for a value. + 2) Use `key` as a sysfs key directly and look in /sys/class/dmi/... + 3) Fall-back to passing `key` to `dmidecode --string`. + + If all of the above fail to find a value, None will be returned. + """ + + if is_container(): + return None + + syspath_value = _read_dmi_syspath(key) + if syspath_value is not None: + return syspath_value + + def is_x86(arch): + return (arch == 'x86_64' or (arch[0] == 'i' and arch[2:] == '86')) + + # running dmidecode can be problematic on some arches (LP: #1243287) + uname_arch = os.uname()[4] + if not (is_x86(uname_arch) or uname_arch in ('aarch64', 'amd64')): + LOG.debug("dmidata is not supported on %s", uname_arch) + return None + + dmidecode_path = subp.which('dmidecode') + if dmidecode_path: + return _call_dmidecode(key, dmidecode_path) + + LOG.warning("did not find either path %s or dmidecode command", + DMI_SYS_PATH) + return None + +# vi: ts=4 expandtab diff --git a/cloudinit/sources/DataSourceAliYun.py b/cloudinit/sources/DataSourceAliYun.py index 45cc9f00..09052873 100644 --- a/cloudinit/sources/DataSourceAliYun.py +++ b/cloudinit/sources/DataSourceAliYun.py @@ -1,8 +1,8 @@ # This file is part of cloud-init. See LICENSE file for license information. +from cloudinit import dmi from cloudinit import sources from cloudinit.sources import DataSourceEc2 as EC2 -from cloudinit import util ALIYUN_PRODUCT = "Alibaba Cloud ECS" @@ -30,7 +30,7 @@ class DataSourceAliYun(EC2.DataSourceEc2): def _is_aliyun(): - return util.read_dmi_data('system-product-name') == ALIYUN_PRODUCT + return dmi.read_dmi_data('system-product-name') == ALIYUN_PRODUCT def parse_public_keys(public_keys): diff --git a/cloudinit/sources/DataSourceAltCloud.py b/cloudinit/sources/DataSourceAltCloud.py index ac3ecc3d..cd93412a 100644 --- a/cloudinit/sources/DataSourceAltCloud.py +++ b/cloudinit/sources/DataSourceAltCloud.py @@ -16,6 +16,7 @@ import errno import os import os.path +from cloudinit import dmi from cloudinit import log as logging from cloudinit import sources from cloudinit import subp @@ -109,7 +110,7 @@ class DataSourceAltCloud(sources.DataSource): CLOUD_INFO_FILE) return 'UNKNOWN' return cloud_type - system_name = util.read_dmi_data("system-product-name") + system_name = dmi.read_dmi_data("system-product-name") if not system_name: return 'UNKNOWN' diff --git a/cloudinit/sources/DataSourceAzure.py b/cloudinit/sources/DataSourceAzure.py index 70e32f46..fa3e0a2b 100755 --- a/cloudinit/sources/DataSourceAzure.py +++ b/cloudinit/sources/DataSourceAzure.py @@ -15,6 +15,7 @@ from time import time from xml.dom import minidom import xml.etree.ElementTree as ET +from cloudinit import dmi from cloudinit import log as logging from cloudinit import net from cloudinit.event import EventType @@ -630,7 +631,7 @@ class DataSourceAzure(sources.DataSource): def _iid(self, previous=None): prev_iid_path = os.path.join( self.paths.get_cpath('data'), 'instance-id') - iid = util.read_dmi_data('system-uuid') + iid = dmi.read_dmi_data('system-uuid') if os.path.exists(prev_iid_path): previous = util.load_file(prev_iid_path).strip() if is_byte_swapped(previous, iid): @@ -1630,7 +1631,7 @@ def _is_platform_viable(seed_dir): description="found azure asset tag", parent=azure_ds_reporter ) as evt: - asset_tag = util.read_dmi_data('chassis-asset-tag') + asset_tag = dmi.read_dmi_data('chassis-asset-tag') if asset_tag == AZURE_CHASSIS_ASSET_TAG: return True msg = "Non-Azure DMI asset tag '%s' discovered." % asset_tag diff --git a/cloudinit/sources/DataSourceCloudSigma.py b/cloudinit/sources/DataSourceCloudSigma.py index df88f677..f63baf74 100644 --- a/cloudinit/sources/DataSourceCloudSigma.py +++ b/cloudinit/sources/DataSourceCloudSigma.py @@ -9,9 +9,9 @@ import re from cloudinit.cs_utils import Cepko, SERIAL_PORT +from cloudinit import dmi from cloudinit import log as logging from cloudinit import sources -from cloudinit import util LOG = logging.getLogger(__name__) @@ -38,7 +38,7 @@ class DataSourceCloudSigma(sources.DataSource): """ LOG.debug("determining hypervisor product name via dmi data") - sys_product_name = util.read_dmi_data("system-product-name") + sys_product_name = dmi.read_dmi_data("system-product-name") if not sys_product_name: LOG.debug("system-product-name not available in dmi data") return False diff --git a/cloudinit/sources/DataSourceEc2.py b/cloudinit/sources/DataSourceEc2.py index 1d09c12a..1930a509 100644 --- a/cloudinit/sources/DataSourceEc2.py +++ b/cloudinit/sources/DataSourceEc2.py @@ -11,6 +11,7 @@ import os import time +from cloudinit import dmi from cloudinit import ec2_utils as ec2 from cloudinit import log as logging from cloudinit import net @@ -699,26 +700,26 @@ def _collect_platform_data(): uuid = util.load_file("/sys/hypervisor/uuid").strip() data['uuid_source'] = 'hypervisor' except Exception: - uuid = util.read_dmi_data('system-uuid') + uuid = dmi.read_dmi_data('system-uuid') data['uuid_source'] = 'dmi' if uuid is None: uuid = '' data['uuid'] = uuid.lower() - serial = util.read_dmi_data('system-serial-number') + serial = dmi.read_dmi_data('system-serial-number') if serial is None: serial = '' data['serial'] = serial.lower() - asset_tag = util.read_dmi_data('chassis-asset-tag') + asset_tag = dmi.read_dmi_data('chassis-asset-tag') if asset_tag is None: asset_tag = '' data['asset_tag'] = asset_tag.lower() - vendor = util.read_dmi_data('system-manufacturer') + vendor = dmi.read_dmi_data('system-manufacturer') data['vendor'] = (vendor if vendor else '').lower() return data diff --git a/cloudinit/sources/DataSourceExoscale.py b/cloudinit/sources/DataSourceExoscale.py index d59aefd1..adee6d79 100644 --- a/cloudinit/sources/DataSourceExoscale.py +++ b/cloudinit/sources/DataSourceExoscale.py @@ -3,6 +3,7 @@ # # This file is part of cloud-init. See LICENSE file for license information. +from cloudinit import dmi from cloudinit import ec2_utils as ec2 from cloudinit import log as logging from cloudinit import sources @@ -135,7 +136,7 @@ class DataSourceExoscale(sources.DataSource): return self.extra_config def _is_platform_viable(self): - return util.read_dmi_data('system-product-name').startswith( + return dmi.read_dmi_data('system-product-name').startswith( EXOSCALE_DMI_NAME) diff --git a/cloudinit/sources/DataSourceGCE.py b/cloudinit/sources/DataSourceGCE.py index 0ec5f6ec..746caddb 100644 --- a/cloudinit/sources/DataSourceGCE.py +++ b/cloudinit/sources/DataSourceGCE.py @@ -7,6 +7,7 @@ import json from base64 import b64decode +from cloudinit import dmi from cloudinit.distros import ug_util from cloudinit import log as logging from cloudinit import sources @@ -248,12 +249,12 @@ def read_md(address=None, platform_check=True): def platform_reports_gce(): - pname = util.read_dmi_data('system-product-name') or "N/A" + pname = dmi.read_dmi_data('system-product-name') or "N/A" if pname == "Google Compute Engine": return True # system-product-name is not always guaranteed (LP: #1674861) - serial = util.read_dmi_data('system-serial-number') or "N/A" + serial = dmi.read_dmi_data('system-serial-number') or "N/A" if serial.startswith("GoogleCloud-"): return True diff --git a/cloudinit/sources/DataSourceHetzner.py b/cloudinit/sources/DataSourceHetzner.py index 8e4d4b69..c7c88dd7 100644 --- a/cloudinit/sources/DataSourceHetzner.py +++ b/cloudinit/sources/DataSourceHetzner.py @@ -6,6 +6,7 @@ """Hetzner Cloud API Documentation https://docs.hetzner.cloud/""" +from cloudinit import dmi from cloudinit import log as logging from cloudinit import net as cloudnet from cloudinit import sources @@ -113,11 +114,11 @@ class DataSourceHetzner(sources.DataSource): def get_hcloud_data(): - vendor_name = util.read_dmi_data('system-manufacturer') + vendor_name = dmi.read_dmi_data('system-manufacturer') if vendor_name != "Hetzner": return (False, None) - serial = util.read_dmi_data("system-serial-number") + serial = dmi.read_dmi_data("system-serial-number") if serial: LOG.debug("Running on Hetzner Cloud: serial=%s", serial) else: diff --git a/cloudinit/sources/DataSourceNoCloud.py b/cloudinit/sources/DataSourceNoCloud.py index d4a175e8..a126aad3 100644 --- a/cloudinit/sources/DataSourceNoCloud.py +++ b/cloudinit/sources/DataSourceNoCloud.py @@ -11,6 +11,7 @@ import errno import os +from cloudinit import dmi from cloudinit import log as logging from cloudinit.net import eni from cloudinit import sources @@ -61,7 +62,7 @@ class DataSourceNoCloud(sources.DataSource): # Parse the system serial label from dmi. If not empty, try parsing # like the commandline md = {} - serial = util.read_dmi_data('system-serial-number') + serial = dmi.read_dmi_data('system-serial-number') if serial and load_cmdline_data(md, serial): found.append("dmi") mydata = _merge_new_seed(mydata, {'meta-data': md}) diff --git a/cloudinit/sources/DataSourceOVF.py b/cloudinit/sources/DataSourceOVF.py index a5ccb8f6..741c140a 100644 --- a/cloudinit/sources/DataSourceOVF.py +++ b/cloudinit/sources/DataSourceOVF.py @@ -14,6 +14,7 @@ import re import time from xml.dom import minidom +from cloudinit import dmi from cloudinit import log as logging from cloudinit import sources from cloudinit import subp @@ -83,7 +84,7 @@ class DataSourceOVF(sources.DataSource): (seedfile, contents) = get_ovf_env(self.paths.seed_dir) - system_type = util.read_dmi_data("system-product-name") + system_type = dmi.read_dmi_data("system-product-name") if system_type is None: LOG.debug("No system-product-name found") @@ -322,7 +323,7 @@ class DataSourceOVF(sources.DataSource): return True def _get_subplatform(self): - system_type = util.read_dmi_data("system-product-name").lower() + system_type = dmi.read_dmi_data("system-product-name").lower() if system_type == 'vmware': return 'vmware (%s)' % self.seed return 'ovf (%s)' % self.seed diff --git a/cloudinit/sources/DataSourceOpenStack.py b/cloudinit/sources/DataSourceOpenStack.py index 0ede0a0e..b3406c67 100644 --- a/cloudinit/sources/DataSourceOpenStack.py +++ b/cloudinit/sources/DataSourceOpenStack.py @@ -6,6 +6,7 @@ import time +from cloudinit import dmi from cloudinit import log as logging from cloudinit.net.dhcp import EphemeralDHCPv4, NoDHCPLeaseError from cloudinit import sources @@ -225,10 +226,10 @@ def detect_openstack(accept_oracle=False): """Return True when a potential OpenStack platform is detected.""" if not util.is_x86(): return True # Non-Intel cpus don't properly report dmi product names - product_name = util.read_dmi_data('system-product-name') + product_name = dmi.read_dmi_data('system-product-name') if product_name in VALID_DMI_PRODUCT_NAMES: return True - elif util.read_dmi_data('chassis-asset-tag') in VALID_DMI_ASSET_TAGS: + elif dmi.read_dmi_data('chassis-asset-tag') in VALID_DMI_ASSET_TAGS: return True elif accept_oracle and oracle._is_platform_viable(): return True diff --git a/cloudinit/sources/DataSourceOracle.py b/cloudinit/sources/DataSourceOracle.py index 20d6487d..bf81b10b 100644 --- a/cloudinit/sources/DataSourceOracle.py +++ b/cloudinit/sources/DataSourceOracle.py @@ -17,6 +17,7 @@ import base64 from collections import namedtuple from contextlib import suppress as noop +from cloudinit import dmi from cloudinit import log as logging from cloudinit import net, sources, util from cloudinit.net import ( @@ -273,12 +274,12 @@ class DataSourceOracle(sources.DataSource): def _read_system_uuid(): - sys_uuid = util.read_dmi_data('system-uuid') + sys_uuid = dmi.read_dmi_data('system-uuid') return None if sys_uuid is None else sys_uuid.lower() def _is_platform_viable(): - asset_tag = util.read_dmi_data('chassis-asset-tag') + asset_tag = dmi.read_dmi_data('chassis-asset-tag') return asset_tag == CHASSIS_ASSET_TAG diff --git a/cloudinit/sources/DataSourceScaleway.py b/cloudinit/sources/DataSourceScaleway.py index 83c2bf65..41be7665 100644 --- a/cloudinit/sources/DataSourceScaleway.py +++ b/cloudinit/sources/DataSourceScaleway.py @@ -25,6 +25,7 @@ import requests from requests.packages.urllib3.connection import HTTPConnection from requests.packages.urllib3.poolmanager import PoolManager +from cloudinit import dmi from cloudinit import log as logging from cloudinit import sources from cloudinit import url_helper @@ -56,7 +57,7 @@ def on_scaleway(): * the initrd created the file /var/run/scaleway. * "scaleway" is in the kernel cmdline. """ - vendor_name = util.read_dmi_data('system-manufacturer') + vendor_name = dmi.read_dmi_data('system-manufacturer') if vendor_name == 'Scaleway': return True diff --git a/cloudinit/sources/DataSourceSmartOS.py b/cloudinit/sources/DataSourceSmartOS.py index f1f903bc..fd292baa 100644 --- a/cloudinit/sources/DataSourceSmartOS.py +++ b/cloudinit/sources/DataSourceSmartOS.py @@ -30,6 +30,7 @@ import random import re import socket +from cloudinit import dmi from cloudinit import log as logging from cloudinit import serial from cloudinit import sources @@ -767,7 +768,7 @@ def get_smartos_environ(uname_version=None, product_name=None): return SMARTOS_ENV_LX_BRAND if product_name is None: - system_type = util.read_dmi_data("system-product-name") + system_type = dmi.read_dmi_data("system-product-name") else: system_type = product_name diff --git a/cloudinit/sources/__init__.py b/cloudinit/sources/__init__.py index c4d60fff..9dccc687 100644 --- a/cloudinit/sources/__init__.py +++ b/cloudinit/sources/__init__.py @@ -14,6 +14,7 @@ import json import os from collections import namedtuple +from cloudinit import dmi from cloudinit import importer from cloudinit import log as logging from cloudinit import net @@ -809,7 +810,7 @@ def instance_id_matches_system_uuid(instance_id, field='system-uuid'): if not instance_id: return False - dmi_value = util.read_dmi_data(field) + dmi_value = dmi.read_dmi_data(field) if not dmi_value: return False return instance_id.lower() == dmi_value.lower() diff --git a/cloudinit/sources/helpers/digitalocean.py b/cloudinit/sources/helpers/digitalocean.py index b545c4d6..f9be4ecb 100644 --- a/cloudinit/sources/helpers/digitalocean.py +++ b/cloudinit/sources/helpers/digitalocean.py @@ -5,6 +5,7 @@ import json import random +from cloudinit import dmi from cloudinit import log as logging from cloudinit import net as cloudnet from cloudinit import url_helper @@ -195,11 +196,11 @@ def read_sysinfo(): # SMBIOS information # Detect if we are on DigitalOcean and return the Droplet's ID - vendor_name = util.read_dmi_data("system-manufacturer") + vendor_name = dmi.read_dmi_data("system-manufacturer") if vendor_name != "DigitalOcean": return (False, None) - droplet_id = util.read_dmi_data("system-serial-number") + droplet_id = dmi.read_dmi_data("system-serial-number") if droplet_id: LOG.debug("system identified via SMBIOS as DigitalOcean Droplet: %s", droplet_id) diff --git a/cloudinit/sources/tests/test_oracle.py b/cloudinit/sources/tests/test_oracle.py index 7bd23813..a7bbdfd9 100644 --- a/cloudinit/sources/tests/test_oracle.py +++ b/cloudinit/sources/tests/test_oracle.py @@ -153,20 +153,20 @@ class TestDataSourceOracle: class TestIsPlatformViable(test_helpers.CiTestCase): - @mock.patch(DS_PATH + ".util.read_dmi_data", + @mock.patch(DS_PATH + ".dmi.read_dmi_data", return_value=oracle.CHASSIS_ASSET_TAG) def test_expected_viable(self, m_read_dmi_data): """System with known chassis tag is viable.""" self.assertTrue(oracle._is_platform_viable()) m_read_dmi_data.assert_has_calls([mock.call('chassis-asset-tag')]) - @mock.patch(DS_PATH + ".util.read_dmi_data", return_value=None) + @mock.patch(DS_PATH + ".dmi.read_dmi_data", return_value=None) def test_expected_not_viable_dmi_data_none(self, m_read_dmi_data): """System without known chassis tag is not viable.""" self.assertFalse(oracle._is_platform_viable()) m_read_dmi_data.assert_has_calls([mock.call('chassis-asset-tag')]) - @mock.patch(DS_PATH + ".util.read_dmi_data", return_value="LetsGoCubs") + @mock.patch(DS_PATH + ".dmi.read_dmi_data", return_value="LetsGoCubs") def test_expected_not_viable_other(self, m_read_dmi_data): """System with unnown chassis tag is not viable.""" self.assertFalse(oracle._is_platform_viable()) diff --git a/cloudinit/tests/test_dmi.py b/cloudinit/tests/test_dmi.py new file mode 100644 index 00000000..4a8af257 --- /dev/null +++ b/cloudinit/tests/test_dmi.py @@ -0,0 +1,131 @@ +from cloudinit.tests import helpers +from cloudinit import dmi +from cloudinit import util +from cloudinit import subp + +import os +import tempfile +import shutil +from unittest import mock + + +class TestReadDMIData(helpers.FilesystemMockingTestCase): + + def setUp(self): + super(TestReadDMIData, self).setUp() + self.new_root = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, self.new_root) + self.reRoot(self.new_root) + p = mock.patch("cloudinit.dmi.is_container", return_value=False) + self.addCleanup(p.stop) + self._m_is_container = p.start() + + def _create_sysfs_parent_directory(self): + util.ensure_dir(os.path.join('sys', 'class', 'dmi', 'id')) + + def _create_sysfs_file(self, key, content): + """Mocks the sys path found on Linux systems.""" + self._create_sysfs_parent_directory() + dmi_key = "/sys/class/dmi/id/{0}".format(key) + util.write_file(dmi_key, content) + + def _configure_dmidecode_return(self, key, content, error=None): + """ + In order to test a missing sys path and call outs to dmidecode, this + function fakes the results of dmidecode to test the results. + """ + def _dmidecode_subp(cmd): + if cmd[-1] != key: + raise subp.ProcessExecutionError() + return (content, error) + + self.patched_funcs.enter_context( + mock.patch("cloudinit.dmi.subp.which", side_effect=lambda _: True)) + self.patched_funcs.enter_context( + mock.patch("cloudinit.dmi.subp.subp", side_effect=_dmidecode_subp)) + + def patch_mapping(self, new_mapping): + self.patched_funcs.enter_context( + mock.patch('cloudinit.dmi.DMIDECODE_TO_DMI_SYS_MAPPING', + new_mapping)) + + def test_sysfs_used_with_key_in_mapping_and_file_on_disk(self): + self.patch_mapping({'mapped-key': 'mapped-value'}) + expected_dmi_value = 'sys-used-correctly' + self._create_sysfs_file('mapped-value', expected_dmi_value) + self._configure_dmidecode_return('mapped-key', 'wrong-wrong-wrong') + self.assertEqual(expected_dmi_value, dmi.read_dmi_data('mapped-key')) + + def test_dmidecode_used_if_no_sysfs_file_on_disk(self): + self.patch_mapping({}) + self._create_sysfs_parent_directory() + expected_dmi_value = 'dmidecode-used' + self._configure_dmidecode_return('use-dmidecode', expected_dmi_value) + with mock.patch("cloudinit.util.os.uname") as m_uname: + m_uname.return_value = ('x-sysname', 'x-nodename', + 'x-release', 'x-version', 'x86_64') + self.assertEqual(expected_dmi_value, + dmi.read_dmi_data('use-dmidecode')) + + def test_dmidecode_not_used_on_arm(self): + self.patch_mapping({}) + print("current =%s", subp) + self._create_sysfs_parent_directory() + dmi_val = 'from-dmidecode' + dmi_name = 'use-dmidecode' + self._configure_dmidecode_return(dmi_name, dmi_val) + print("now =%s", subp) + + expected = {'armel': None, 'aarch64': dmi_val, 'x86_64': dmi_val} + found = {} + # we do not run the 'dmi-decode' binary on some arches + # verify that anything requested that is not in the sysfs dir + # will return None on those arches. + with mock.patch("cloudinit.util.os.uname") as m_uname: + for arch in expected: + m_uname.return_value = ('x-sysname', 'x-nodename', + 'x-release', 'x-version', arch) + print("now2 =%s", subp) + found[arch] = dmi.read_dmi_data(dmi_name) + self.assertEqual(expected, found) + + def test_none_returned_if_neither_source_has_data(self): + self.patch_mapping({}) + self._configure_dmidecode_return('key', 'value') + self.assertIsNone(dmi.read_dmi_data('expect-fail')) + + def test_none_returned_if_dmidecode_not_in_path(self): + self.patched_funcs.enter_context( + mock.patch.object(subp, 'which', lambda _: False)) + self.patch_mapping({}) + self.assertIsNone(dmi.read_dmi_data('expect-fail')) + + def test_empty_string_returned_instead_of_foxfox(self): + # uninitialized dmi values show as \xff, return empty string + my_len = 32 + dmi_value = b'\xff' * my_len + b'\n' + expected = "" + dmi_key = 'system-product-name' + sysfs_key = 'product_name' + self._create_sysfs_file(sysfs_key, dmi_value) + self.assertEqual(expected, dmi.read_dmi_data(dmi_key)) + + def test_container_returns_none(self): + """In a container read_dmi_data should always return None.""" + + # first verify we get the value if not in container + self._m_is_container.return_value = False + key, val = ("system-product-name", "my_product") + self._create_sysfs_file('product_name', val) + self.assertEqual(val, dmi.read_dmi_data(key)) + + # then verify in container returns None + self._m_is_container.return_value = True + self.assertIsNone(dmi.read_dmi_data(key)) + + def test_container_returns_none_on_unknown(self): + """In a container even bogus keys return None.""" + self._m_is_container.return_value = True + self._create_sysfs_file('product_name', "should-be-ignored") + self.assertIsNone(dmi.read_dmi_data("bogus")) + self.assertIsNone(dmi.read_dmi_data("system-product-name")) diff --git a/cloudinit/util.py b/cloudinit/util.py index b8856af1..bdb3694d 100644 --- a/cloudinit/util.py +++ b/cloudinit/util.py @@ -159,32 +159,6 @@ def fully_decoded_payload(part): return cte_payload -# Path for DMI Data -DMI_SYS_PATH = "/sys/class/dmi/id" - -# dmidecode and /sys/class/dmi/id/* use different names for the same value, -# this allows us to refer to them by one canonical name -DMIDECODE_TO_DMI_SYS_MAPPING = { - 'baseboard-asset-tag': 'board_asset_tag', - 'baseboard-manufacturer': 'board_vendor', - 'baseboard-product-name': 'board_name', - 'baseboard-serial-number': 'board_serial', - 'baseboard-version': 'board_version', - 'bios-release-date': 'bios_date', - 'bios-vendor': 'bios_vendor', - 'bios-version': 'bios_version', - 'chassis-asset-tag': 'chassis_asset_tag', - 'chassis-manufacturer': 'chassis_vendor', - 'chassis-serial-number': 'chassis_serial', - 'chassis-version': 'chassis_version', - 'system-manufacturer': 'sys_vendor', - 'system-product-name': 'product_name', - 'system-serial-number': 'product_serial', - 'system-uuid': 'product_uuid', - 'system-version': 'product_version', -} - - class SeLinuxGuard(object): def __init__(self, path, recursive=False): # Late import since it might not always @@ -2421,57 +2395,6 @@ def human2bytes(size): return int(num * mpliers[mplier]) -def _read_dmi_syspath(key): - """ - Reads dmi data with from /sys/class/dmi/id - """ - if key not in DMIDECODE_TO_DMI_SYS_MAPPING: - return None - mapped_key = DMIDECODE_TO_DMI_SYS_MAPPING[key] - dmi_key_path = "{0}/{1}".format(DMI_SYS_PATH, mapped_key) - LOG.debug("querying dmi data %s", dmi_key_path) - try: - if not os.path.exists(dmi_key_path): - LOG.debug("did not find %s", dmi_key_path) - return None - - key_data = load_file(dmi_key_path, decode=False) - if not key_data: - LOG.debug("%s did not return any data", dmi_key_path) - return None - - # uninitialized dmi values show as all \xff and /sys appends a '\n'. - # in that event, return a string of '.' in the same length. - if key_data == b'\xff' * (len(key_data) - 1) + b'\n': - key_data = b"" - - str_data = key_data.decode('utf8').strip() - LOG.debug("dmi data %s returned %s", dmi_key_path, str_data) - return str_data - - except Exception: - logexc(LOG, "failed read of %s", dmi_key_path) - return None - - -def _call_dmidecode(key, dmidecode_path): - """ - Calls out to dmidecode to get the data out. This is mostly for supporting - OS's without /sys/class/dmi/id support. - """ - try: - cmd = [dmidecode_path, "--string", key] - (result, _err) = subp.subp(cmd) - result = result.strip() - LOG.debug("dmidecode returned '%s' for '%s'", result, key) - if result.replace(".", "") == "": - return "" - return result - except (IOError, OSError) as e: - LOG.debug('failed dmidecode cmd: %s\n%s', cmd, e) - return None - - def is_x86(uname_arch=None): """Return True if platform is x86-based""" if uname_arch is None: @@ -2482,48 +2405,6 @@ def is_x86(uname_arch=None): return x86_arch_match -def read_dmi_data(key): - """ - Wrapper for reading DMI data. - - If running in a container return None. This is because DMI data is - assumed to be not useful in a container as it does not represent the - container but rather the host. - - This will do the following (returning the first that produces a - result): - 1) Use a mapping to translate `key` from dmidecode naming to - sysfs naming and look in /sys/class/dmi/... for a value. - 2) Use `key` as a sysfs key directly and look in /sys/class/dmi/... - 3) Fall-back to passing `key` to `dmidecode --string`. - - If all of the above fail to find a value, None will be returned. - """ - - if is_container(): - return None - - syspath_value = _read_dmi_syspath(key) - if syspath_value is not None: - return syspath_value - - # running dmidecode can be problematic on some arches (LP: #1243287) - uname_arch = os.uname()[4] - if not (is_x86(uname_arch) or - uname_arch == 'aarch64' or - uname_arch == 'amd64'): - LOG.debug("dmidata is not supported on %s", uname_arch) - return None - - dmidecode_path = subp.which('dmidecode') - if dmidecode_path: - return _call_dmidecode(key, dmidecode_path) - - LOG.warning("did not find either path %s or dmidecode command", - DMI_SYS_PATH) - return None - - def message_from_string(string): if sys.version_info[:2] < (2, 7): return email.message_from_file(io.StringIO(string)) diff --git a/tests/unittests/test_datasource/test_aliyun.py b/tests/unittests/test_datasource/test_aliyun.py index b626229e..eb2828d5 100644 --- a/tests/unittests/test_datasource/test_aliyun.py +++ b/tests/unittests/test_datasource/test_aliyun.py @@ -188,7 +188,7 @@ class TestIsAliYun(test_helpers.CiTestCase): ALIYUN_PRODUCT = 'Alibaba Cloud ECS' read_dmi_data_expected = [mock.call('system-product-name')] - @mock.patch("cloudinit.sources.DataSourceAliYun.util.read_dmi_data") + @mock.patch("cloudinit.sources.DataSourceAliYun.dmi.read_dmi_data") def test_true_on_aliyun_product(self, m_read_dmi_data): """Should return true if the dmi product data has expected value.""" m_read_dmi_data.return_value = self.ALIYUN_PRODUCT @@ -197,7 +197,7 @@ class TestIsAliYun(test_helpers.CiTestCase): m_read_dmi_data.call_args_list) self.assertEqual(True, ret) - @mock.patch("cloudinit.sources.DataSourceAliYun.util.read_dmi_data") + @mock.patch("cloudinit.sources.DataSourceAliYun.dmi.read_dmi_data") def test_false_on_empty_string(self, m_read_dmi_data): """Should return false on empty value returned.""" m_read_dmi_data.return_value = "" @@ -206,7 +206,7 @@ class TestIsAliYun(test_helpers.CiTestCase): m_read_dmi_data.call_args_list) self.assertEqual(False, ret) - @mock.patch("cloudinit.sources.DataSourceAliYun.util.read_dmi_data") + @mock.patch("cloudinit.sources.DataSourceAliYun.dmi.read_dmi_data") def test_false_on_unknown_string(self, m_read_dmi_data): """Should return false on an unrelated string.""" m_read_dmi_data.return_value = "cubs win" diff --git a/tests/unittests/test_datasource/test_altcloud.py b/tests/unittests/test_datasource/test_altcloud.py index fc59d1d5..7a5393ac 100644 --- a/tests/unittests/test_datasource/test_altcloud.py +++ b/tests/unittests/test_datasource/test_altcloud.py @@ -14,6 +14,7 @@ import os import shutil import tempfile +from cloudinit import dmi from cloudinit import helpers from cloudinit import subp from cloudinit import util @@ -88,14 +89,14 @@ class TestGetCloudType(CiTestCase): super(TestGetCloudType, self).setUp() self.tmp = self.tmp_dir() self.paths = helpers.Paths({'cloud_dir': self.tmp}) - self.dmi_data = util.read_dmi_data + self.dmi_data = dmi.read_dmi_data # We have a different code path for arm to deal with LP1243287 # We have to switch arch to x86_64 to avoid test failure force_arch('x86_64') def tearDown(self): # Reset - util.read_dmi_data = self.dmi_data + dmi.read_dmi_data = self.dmi_data force_arch() def test_cloud_info_file_ioerror(self): @@ -123,7 +124,7 @@ class TestGetCloudType(CiTestCase): Test method get_cloud_type() for RHEVm systems. Forcing read_dmi_data return to match a RHEVm system: RHEV Hypervisor ''' - util.read_dmi_data = _dmi_data('RHEV') + dmi.read_dmi_data = _dmi_data('RHEV') dsrc = dsac.DataSourceAltCloud({}, None, self.paths) self.assertEqual('RHEV', dsrc.get_cloud_type()) @@ -132,7 +133,7 @@ class TestGetCloudType(CiTestCase): Test method get_cloud_type() for vSphere systems. Forcing read_dmi_data return to match a vSphere system: RHEV Hypervisor ''' - util.read_dmi_data = _dmi_data('VMware Virtual Platform') + dmi.read_dmi_data = _dmi_data('VMware Virtual Platform') dsrc = dsac.DataSourceAltCloud({}, None, self.paths) self.assertEqual('VSPHERE', dsrc.get_cloud_type()) @@ -141,7 +142,7 @@ class TestGetCloudType(CiTestCase): Test method get_cloud_type() for unknown systems. Forcing read_dmi_data return to match an unrecognized return. ''' - util.read_dmi_data = _dmi_data('Unrecognized Platform') + dmi.read_dmi_data = _dmi_data('Unrecognized Platform') dsrc = dsac.DataSourceAltCloud({}, None, self.paths) self.assertEqual('UNKNOWN', dsrc.get_cloud_type()) @@ -219,7 +220,7 @@ class TestGetDataNoCloudInfoFile(CiTestCase): self.tmp = self.tmp_dir() self.paths = helpers.Paths( {'cloud_dir': self.tmp, 'run_dir': self.tmp}) - self.dmi_data = util.read_dmi_data + self.dmi_data = dmi.read_dmi_data dsac.CLOUD_INFO_FILE = \ 'no such file' # We have a different code path for arm to deal with LP1243287 @@ -230,14 +231,14 @@ class TestGetDataNoCloudInfoFile(CiTestCase): # Reset dsac.CLOUD_INFO_FILE = \ '/etc/sysconfig/cloud-info' - util.read_dmi_data = self.dmi_data + dmi.read_dmi_data = self.dmi_data # Return back to original arch force_arch() def test_rhev_no_cloud_file(self): '''Test No cloud info file module get_data() forcing RHEV.''' - util.read_dmi_data = _dmi_data('RHEV Hypervisor') + dmi.read_dmi_data = _dmi_data('RHEV Hypervisor') dsrc = dsac.DataSourceAltCloud({}, None, self.paths) dsrc.user_data_rhevm = lambda: True self.assertEqual(True, dsrc.get_data()) @@ -245,7 +246,7 @@ class TestGetDataNoCloudInfoFile(CiTestCase): def test_vsphere_no_cloud_file(self): '''Test No cloud info file module get_data() forcing VSPHERE.''' - util.read_dmi_data = _dmi_data('VMware Virtual Platform') + dmi.read_dmi_data = _dmi_data('VMware Virtual Platform') dsrc = dsac.DataSourceAltCloud({}, None, self.paths) dsrc.user_data_vsphere = lambda: True self.assertEqual(True, dsrc.get_data()) @@ -253,7 +254,7 @@ class TestGetDataNoCloudInfoFile(CiTestCase): def test_failure_no_cloud_file(self): '''Test No cloud info file module get_data() forcing unrecognized.''' - util.read_dmi_data = _dmi_data('Unrecognized Platform') + dmi.read_dmi_data = _dmi_data('Unrecognized Platform') dsrc = dsac.DataSourceAltCloud({}, None, self.paths) self.assertEqual(False, dsrc.get_data()) diff --git a/tests/unittests/test_datasource/test_azure.py b/tests/unittests/test_datasource/test_azure.py index 56c1cf18..433fbc66 100644 --- a/tests/unittests/test_datasource/test_azure.py +++ b/tests/unittests/test_datasource/test_azure.py @@ -570,7 +570,7 @@ scbus-1 on xpt0 bus 0 (dsaz, 'set_hostname', mock.MagicMock()), (dsaz, 'get_metadata_from_fabric', self.get_metadata_from_fabric), (dsaz.subp, 'which', lambda x: True), - (dsaz.util, 'read_dmi_data', mock.MagicMock( + (dsaz.dmi, 'read_dmi_data', mock.MagicMock( side_effect=_dmi_mocks)), (dsaz.util, 'wait_for_files', mock.MagicMock( side_effect=_wait_for_files)), @@ -1427,7 +1427,7 @@ class TestAzureBounce(CiTestCase): raise RuntimeError('should not get here') self.patches.enter_context( - mock.patch.object(dsaz.util, 'read_dmi_data', + mock.patch.object(dsaz.dmi, 'read_dmi_data', mock.MagicMock(side_effect=_dmi_mocks))) def setUp(self): @@ -2294,14 +2294,14 @@ class TestWBIsPlatformViable(CiTestCase): """White box tests for _is_platform_viable.""" with_logs = True - @mock.patch(MOCKPATH + 'util.read_dmi_data') + @mock.patch(MOCKPATH + 'dmi.read_dmi_data') def test_true_on_non_azure_chassis(self, m_read_dmi_data): """Return True if DMI chassis-asset-tag is AZURE_CHASSIS_ASSET_TAG.""" m_read_dmi_data.return_value = dsaz.AZURE_CHASSIS_ASSET_TAG self.assertTrue(dsaz._is_platform_viable('doesnotmatter')) @mock.patch(MOCKPATH + 'os.path.exists') - @mock.patch(MOCKPATH + 'util.read_dmi_data') + @mock.patch(MOCKPATH + 'dmi.read_dmi_data') def test_true_on_azure_ovf_env_in_seed_dir(self, m_read_dmi_data, m_exist): """Return True if ovf-env.xml exists in known seed dirs.""" # Non-matching Azure chassis-asset-tag @@ -2322,7 +2322,7 @@ class TestWBIsPlatformViable(CiTestCase): MOCKPATH, {'os.path.exists': False, # Non-matching Azure chassis-asset-tag - 'util.read_dmi_data': dsaz.AZURE_CHASSIS_ASSET_TAG + 'X', + 'dmi.read_dmi_data': dsaz.AZURE_CHASSIS_ASSET_TAG + 'X', 'subp.which': None}, dsaz._is_platform_viable, 'doesnotmatter')) self.assertIn( diff --git a/tests/unittests/test_datasource/test_nocloud.py b/tests/unittests/test_datasource/test_nocloud.py index 2e6b53ff..02cc9b38 100644 --- a/tests/unittests/test_datasource/test_nocloud.py +++ b/tests/unittests/test_datasource/test_nocloud.py @@ -1,5 +1,6 @@ # This file is part of cloud-init. See LICENSE file for license information. +from cloudinit import dmi from cloudinit import helpers from cloudinit.sources.DataSourceNoCloud import ( DataSourceNoCloud as dsNoCloud, @@ -30,7 +31,7 @@ class TestNoCloudDataSource(CiTestCase): self.mocks.enter_context( mock.patch.object(util, 'get_cmdline', return_value=self.cmdline)) self.mocks.enter_context( - mock.patch.object(util, 'read_dmi_data', return_value=None)) + mock.patch.object(dmi, 'read_dmi_data', return_value=None)) def _test_fs_config_is_read(self, fs_label, fs_label_to_search): vfat_device = 'device-1' diff --git a/tests/unittests/test_datasource/test_openstack.py b/tests/unittests/test_datasource/test_openstack.py index 9b0c1b8a..415755aa 100644 --- a/tests/unittests/test_datasource/test_openstack.py +++ b/tests/unittests/test_datasource/test_openstack.py @@ -459,7 +459,7 @@ class TestDetectOpenStack(test_helpers.CiTestCase): ds.detect_openstack(), 'Expected detect_openstack == True') @test_helpers.mock.patch(MOCK_PATH + 'util.get_proc_env') - @test_helpers.mock.patch(MOCK_PATH + 'util.read_dmi_data') + @test_helpers.mock.patch(MOCK_PATH + 'dmi.read_dmi_data') def test_not_detect_openstack_intel_x86_ec2(self, m_dmi, m_proc_env, m_is_x86): """Return False on EC2 platforms.""" @@ -479,7 +479,7 @@ class TestDetectOpenStack(test_helpers.CiTestCase): ds.detect_openstack(), 'Expected detect_openstack == False on EC2') m_proc_env.assert_called_with(1) - @test_helpers.mock.patch(MOCK_PATH + 'util.read_dmi_data') + @test_helpers.mock.patch(MOCK_PATH + 'dmi.read_dmi_data') def test_detect_openstack_intel_product_name_compute(self, m_dmi, m_is_x86): """Return True on OpenStack compute and nova instances.""" @@ -491,7 +491,7 @@ class TestDetectOpenStack(test_helpers.CiTestCase): self.assertTrue( ds.detect_openstack(), 'Failed to detect_openstack') - @test_helpers.mock.patch(MOCK_PATH + 'util.read_dmi_data') + @test_helpers.mock.patch(MOCK_PATH + 'dmi.read_dmi_data') def test_detect_openstack_opentelekomcloud_chassis_asset_tag(self, m_dmi, m_is_x86): """Return True on OpenStack reporting OpenTelekomCloud asset-tag.""" @@ -509,7 +509,7 @@ class TestDetectOpenStack(test_helpers.CiTestCase): ds.detect_openstack(), 'Expected detect_openstack == True on OpenTelekomCloud') - @test_helpers.mock.patch(MOCK_PATH + 'util.read_dmi_data') + @test_helpers.mock.patch(MOCK_PATH + 'dmi.read_dmi_data') def test_detect_openstack_sapccloud_chassis_asset_tag(self, m_dmi, m_is_x86): """Return True on OpenStack reporting SAP CCloud VM asset-tag.""" @@ -527,7 +527,7 @@ class TestDetectOpenStack(test_helpers.CiTestCase): ds.detect_openstack(), 'Expected detect_openstack == True on SAP CCloud VM') - @test_helpers.mock.patch(MOCK_PATH + 'util.read_dmi_data') + @test_helpers.mock.patch(MOCK_PATH + 'dmi.read_dmi_data') def test_detect_openstack_oraclecloud_chassis_asset_tag(self, m_dmi, m_is_x86): """Return True on OpenStack reporting Oracle cloud asset-tag.""" @@ -566,20 +566,20 @@ class TestDetectOpenStack(test_helpers.CiTestCase): ds.detect_openstack(), 'Expected detect_openstack == True on Generic OpenStack Platform') - @test_helpers.mock.patch(MOCK_PATH + 'util.read_dmi_data') + @test_helpers.mock.patch(MOCK_PATH + 'dmi.read_dmi_data') def test_detect_openstack_nova_chassis_asset_tag(self, m_dmi, m_is_x86): self._test_detect_openstack_nova_compute_chassis_asset_tag( m_dmi, m_is_x86, 'OpenStack Nova') - @test_helpers.mock.patch(MOCK_PATH + 'util.read_dmi_data') + @test_helpers.mock.patch(MOCK_PATH + 'dmi.read_dmi_data') def test_detect_openstack_compute_chassis_asset_tag(self, m_dmi, m_is_x86): self._test_detect_openstack_nova_compute_chassis_asset_tag( m_dmi, m_is_x86, 'OpenStack Compute') @test_helpers.mock.patch(MOCK_PATH + 'util.get_proc_env') - @test_helpers.mock.patch(MOCK_PATH + 'util.read_dmi_data') + @test_helpers.mock.patch(MOCK_PATH + 'dmi.read_dmi_data') def test_detect_openstack_by_proc_1_environ(self, m_dmi, m_proc_env, m_is_x86): """Return True when nova product_name specified in /proc/1/environ.""" diff --git a/tests/unittests/test_datasource/test_ovf.py b/tests/unittests/test_datasource/test_ovf.py index 1d088577..16773de5 100644 --- a/tests/unittests/test_datasource/test_ovf.py +++ b/tests/unittests/test_datasource/test_ovf.py @@ -129,7 +129,7 @@ class TestDatasourceOVF(CiTestCase): ds = self.datasource(sys_cfg={}, distro={}, paths=paths) retcode = wrap_and_call( 'cloudinit.sources.DataSourceOVF', - {'util.read_dmi_data': None, + {'dmi.read_dmi_data': None, 'transport_iso9660': NOT_FOUND, 'transport_vmware_guestinfo': NOT_FOUND}, ds.get_data) @@ -145,7 +145,7 @@ class TestDatasourceOVF(CiTestCase): paths=paths) retcode = wrap_and_call( 'cloudinit.sources.DataSourceOVF', - {'util.read_dmi_data': 'vmware', + {'dmi.read_dmi_data': 'vmware', 'transport_iso9660': NOT_FOUND, 'transport_vmware_guestinfo': NOT_FOUND}, ds.get_data) @@ -174,7 +174,7 @@ class TestDatasourceOVF(CiTestCase): with self.assertRaises(CustomScriptNotFound) as context: wrap_and_call( 'cloudinit.sources.DataSourceOVF', - {'util.read_dmi_data': 'vmware', + {'dmi.read_dmi_data': 'vmware', 'util.del_dir': True, 'search_file': self.tdir, 'wait_for_imc_cfg_file': conf_file, @@ -211,7 +211,7 @@ class TestDatasourceOVF(CiTestCase): with self.assertRaises(RuntimeError) as context: wrap_and_call( 'cloudinit.sources.DataSourceOVF', - {'util.read_dmi_data': 'vmware', + {'dmi.read_dmi_data': 'vmware', 'util.del_dir': True, 'search_file': self.tdir, 'wait_for_imc_cfg_file': conf_file, @@ -246,7 +246,7 @@ class TestDatasourceOVF(CiTestCase): with self.assertRaises(CustomScriptNotFound) as context: wrap_and_call( 'cloudinit.sources.DataSourceOVF', - {'util.read_dmi_data': 'vmware', + {'dmi.read_dmi_data': 'vmware', 'util.del_dir': True, 'search_file': self.tdir, 'wait_for_imc_cfg_file': conf_file, @@ -290,7 +290,7 @@ class TestDatasourceOVF(CiTestCase): with self.assertRaises(CustomScriptNotFound) as context: wrap_and_call( 'cloudinit.sources.DataSourceOVF', - {'util.read_dmi_data': 'vmware', + {'dmi.read_dmi_data': 'vmware', 'util.del_dir': True, 'search_file': self.tdir, 'wait_for_imc_cfg_file': conf_file, @@ -313,7 +313,7 @@ class TestDatasourceOVF(CiTestCase): self.assertEqual('ovf', ds.cloud_name) self.assertEqual('ovf', ds.platform_type) - with mock.patch(MPATH + 'util.read_dmi_data', return_value='!VMware'): + with mock.patch(MPATH + 'dmi.read_dmi_data', return_value='!VMware'): with mock.patch(MPATH + 'transport_vmware_guestinfo') as m_guestd: with mock.patch(MPATH + 'transport_iso9660') as m_iso9660: m_iso9660.return_value = NOT_FOUND @@ -334,7 +334,7 @@ class TestDatasourceOVF(CiTestCase): self.assertEqual('ovf', ds.cloud_name) self.assertEqual('ovf', ds.platform_type) - with mock.patch(MPATH + 'util.read_dmi_data', return_value='VMWare'): + with mock.patch(MPATH + 'dmi.read_dmi_data', return_value='VMWare'): with mock.patch(MPATH + 'transport_vmware_guestinfo') as m_guestd: with mock.patch(MPATH + 'transport_iso9660') as m_iso9660: m_iso9660.return_value = NOT_FOUND diff --git a/tests/unittests/test_datasource/test_scaleway.py b/tests/unittests/test_datasource/test_scaleway.py index 9d82bda9..32f3274a 100644 --- a/tests/unittests/test_datasource/test_scaleway.py +++ b/tests/unittests/test_datasource/test_scaleway.py @@ -87,7 +87,7 @@ class TestOnScaleway(CiTestCase): @mock.patch('cloudinit.util.get_cmdline') @mock.patch('os.path.exists') - @mock.patch('cloudinit.util.read_dmi_data') + @mock.patch('cloudinit.dmi.read_dmi_data') def test_not_on_scaleway(self, m_read_dmi_data, m_file_exists, m_get_cmdline): self.install_mocks( @@ -105,7 +105,7 @@ class TestOnScaleway(CiTestCase): @mock.patch('cloudinit.util.get_cmdline') @mock.patch('os.path.exists') - @mock.patch('cloudinit.util.read_dmi_data') + @mock.patch('cloudinit.dmi.read_dmi_data') def test_on_scaleway_dmi(self, m_read_dmi_data, m_file_exists, m_get_cmdline): """ @@ -121,7 +121,7 @@ class TestOnScaleway(CiTestCase): @mock.patch('cloudinit.util.get_cmdline') @mock.patch('os.path.exists') - @mock.patch('cloudinit.util.read_dmi_data') + @mock.patch('cloudinit.dmi.read_dmi_data') def test_on_scaleway_var_run_scaleway(self, m_read_dmi_data, m_file_exists, m_get_cmdline): """ @@ -136,7 +136,7 @@ class TestOnScaleway(CiTestCase): @mock.patch('cloudinit.util.get_cmdline') @mock.patch('os.path.exists') - @mock.patch('cloudinit.util.read_dmi_data') + @mock.patch('cloudinit.dmi.read_dmi_data') def test_on_scaleway_cmdline(self, m_read_dmi_data, m_file_exists, m_get_cmdline): """ diff --git a/tests/unittests/test_util.py b/tests/unittests/test_util.py index cca53123..857629f1 100644 --- a/tests/unittests/test_util.py +++ b/tests/unittests/test_util.py @@ -492,129 +492,6 @@ class TestIsX86(helpers.CiTestCase): self.assertTrue(util.is_x86()) -class TestReadDMIData(helpers.FilesystemMockingTestCase): - - def setUp(self): - super(TestReadDMIData, self).setUp() - self.new_root = tempfile.mkdtemp() - self.addCleanup(shutil.rmtree, self.new_root) - self.patchOS(self.new_root) - self.patchUtils(self.new_root) - p = mock.patch("cloudinit.util.is_container", return_value=False) - self.addCleanup(p.stop) - self._m_is_container = p.start() - - def _create_sysfs_parent_directory(self): - util.ensure_dir(os.path.join('sys', 'class', 'dmi', 'id')) - - def _create_sysfs_file(self, key, content): - """Mocks the sys path found on Linux systems.""" - self._create_sysfs_parent_directory() - dmi_key = "/sys/class/dmi/id/{0}".format(key) - util.write_file(dmi_key, content) - - def _configure_dmidecode_return(self, key, content, error=None): - """ - In order to test a missing sys path and call outs to dmidecode, this - function fakes the results of dmidecode to test the results. - """ - def _dmidecode_subp(cmd): - if cmd[-1] != key: - raise subp.ProcessExecutionError() - return (content, error) - - self.patched_funcs.enter_context( - mock.patch("cloudinit.subp.which", side_effect=lambda _: True)) - self.patched_funcs.enter_context( - mock.patch("cloudinit.subp.subp", side_effect=_dmidecode_subp)) - - def patch_mapping(self, new_mapping): - self.patched_funcs.enter_context( - mock.patch('cloudinit.util.DMIDECODE_TO_DMI_SYS_MAPPING', - new_mapping)) - - def test_sysfs_used_with_key_in_mapping_and_file_on_disk(self): - self.patch_mapping({'mapped-key': 'mapped-value'}) - expected_dmi_value = 'sys-used-correctly' - self._create_sysfs_file('mapped-value', expected_dmi_value) - self._configure_dmidecode_return('mapped-key', 'wrong-wrong-wrong') - self.assertEqual(expected_dmi_value, util.read_dmi_data('mapped-key')) - - def test_dmidecode_used_if_no_sysfs_file_on_disk(self): - self.patch_mapping({}) - self._create_sysfs_parent_directory() - expected_dmi_value = 'dmidecode-used' - self._configure_dmidecode_return('use-dmidecode', expected_dmi_value) - with mock.patch("cloudinit.util.os.uname") as m_uname: - m_uname.return_value = ('x-sysname', 'x-nodename', - 'x-release', 'x-version', 'x86_64') - self.assertEqual(expected_dmi_value, - util.read_dmi_data('use-dmidecode')) - - def test_dmidecode_not_used_on_arm(self): - self.patch_mapping({}) - print("current =%s", subp) - self._create_sysfs_parent_directory() - dmi_val = 'from-dmidecode' - dmi_name = 'use-dmidecode' - self._configure_dmidecode_return(dmi_name, dmi_val) - print("now =%s", subp) - - expected = {'armel': None, 'aarch64': dmi_val, 'x86_64': dmi_val} - found = {} - # we do not run the 'dmi-decode' binary on some arches - # verify that anything requested that is not in the sysfs dir - # will return None on those arches. - with mock.patch("cloudinit.util.os.uname") as m_uname: - for arch in expected: - m_uname.return_value = ('x-sysname', 'x-nodename', - 'x-release', 'x-version', arch) - print("now2 =%s", subp) - found[arch] = util.read_dmi_data(dmi_name) - self.assertEqual(expected, found) - - def test_none_returned_if_neither_source_has_data(self): - self.patch_mapping({}) - self._configure_dmidecode_return('key', 'value') - self.assertIsNone(util.read_dmi_data('expect-fail')) - - def test_none_returned_if_dmidecode_not_in_path(self): - self.patched_funcs.enter_context( - mock.patch.object(subp, 'which', lambda _: False)) - self.patch_mapping({}) - self.assertIsNone(util.read_dmi_data('expect-fail')) - - def test_dots_returned_instead_of_foxfox(self): - # uninitialized dmi values show as \xff, return those as . - my_len = 32 - dmi_value = b'\xff' * my_len + b'\n' - expected = "" - dmi_key = 'system-product-name' - sysfs_key = 'product_name' - self._create_sysfs_file(sysfs_key, dmi_value) - self.assertEqual(expected, util.read_dmi_data(dmi_key)) - - def test_container_returns_none(self): - """In a container read_dmi_data should always return None.""" - - # first verify we get the value if not in container - self._m_is_container.return_value = False - key, val = ("system-product-name", "my_product") - self._create_sysfs_file('product_name', val) - self.assertEqual(val, util.read_dmi_data(key)) - - # then verify in container returns None - self._m_is_container.return_value = True - self.assertIsNone(util.read_dmi_data(key)) - - def test_container_returns_none_on_unknown(self): - """In a container even bogus keys return None.""" - self._m_is_container.return_value = True - self._create_sysfs_file('product_name', "should-be-ignored") - self.assertIsNone(util.read_dmi_data("bogus")) - self.assertIsNone(util.read_dmi_data("system-product-name")) - - class TestGetConfigLogfiles(helpers.CiTestCase): def test_empty_cfg_returns_empty_list(self): -- cgit v1.2.3 From d83c0bb4baca0b57166a74055f410fa4f75a08f5 Mon Sep 17 00:00:00 2001 From: Mina Galić Date: Fri, 6 Nov 2020 19:49:05 +0100 Subject: replace usage of dmidecode with kenv on FreeBSD (#621) FreeBSD lets us read out kernel parameters with kenv(1), a user-space utility that's shipped in "base" We can use it in place of dmidecode(8), thus removing the dependency on sysutils/dmidecode, and the restrictions to i386 and x86_64 architectures that this utility imposes on FreeBSD. Co-authored-by: Scott Moser --- cloudinit/dmi.py | 86 +++++++++++++++++++++++++------------ cloudinit/tests/test_dmi.py | 27 +++++++++++- cloudinit/util.py | 55 ++++++++++++++++++------ tests/unittests/test_ds_identify.py | 25 +++++++++-- tools/build-on-freebsd | 1 - tools/ds-identify | 42 ++++++++++++++++-- 6 files changed, 185 insertions(+), 51 deletions(-) (limited to 'cloudinit/tests') diff --git a/cloudinit/dmi.py b/cloudinit/dmi.py index 96e0e423..f0e69a5a 100644 --- a/cloudinit/dmi.py +++ b/cloudinit/dmi.py @@ -1,8 +1,9 @@ # This file is part of cloud-init. See LICENSE file for license information. from cloudinit import log as logging from cloudinit import subp -from cloudinit.util import is_container +from cloudinit.util import is_container, is_FreeBSD +from collections import namedtuple import os LOG = logging.getLogger(__name__) @@ -10,38 +11,43 @@ LOG = logging.getLogger(__name__) # Path for DMI Data DMI_SYS_PATH = "/sys/class/dmi/id" -# dmidecode and /sys/class/dmi/id/* use different names for the same value, -# this allows us to refer to them by one canonical name -DMIDECODE_TO_DMI_SYS_MAPPING = { - 'baseboard-asset-tag': 'board_asset_tag', - 'baseboard-manufacturer': 'board_vendor', - 'baseboard-product-name': 'board_name', - 'baseboard-serial-number': 'board_serial', - 'baseboard-version': 'board_version', - 'bios-release-date': 'bios_date', - 'bios-vendor': 'bios_vendor', - 'bios-version': 'bios_version', - 'chassis-asset-tag': 'chassis_asset_tag', - 'chassis-manufacturer': 'chassis_vendor', - 'chassis-serial-number': 'chassis_serial', - 'chassis-version': 'chassis_version', - 'system-manufacturer': 'sys_vendor', - 'system-product-name': 'product_name', - 'system-serial-number': 'product_serial', - 'system-uuid': 'product_uuid', - 'system-version': 'product_version', +kdmi = namedtuple('KernelNames', ['linux', 'freebsd']) +kdmi.__new__.defaults__ = (None, None) + +# FreeBSD's kenv(1) and Linux /sys/class/dmi/id/* both use different names from +# dmidecode. The values are the same, and ultimately what we're interested in. +# These tools offer a "cheaper" way to access those values over dmidecode. +# This is our canonical translation table. If we add more tools on other +# platforms to find dmidecode's values, their keys need to be put in here. +DMIDECODE_TO_KERNEL = { + 'baseboard-asset-tag': kdmi('board_asset_tag', 'smbios.planar.tag'), + 'baseboard-manufacturer': kdmi('board_vendor', 'smbios.planar.maker'), + 'baseboard-product-name': kdmi('board_name', 'smbios.planar.product'), + 'baseboard-serial-number': kdmi('board_serial', 'smbios.planar.serial'), + 'baseboard-version': kdmi('board_version', 'smbios.planar.version'), + 'bios-release-date': kdmi('bios_date', 'smbios.bios.reldate'), + 'bios-vendor': kdmi('bios_vendor', 'smbios.bios.vendor'), + 'bios-version': kdmi('bios_version', 'smbios.bios.version'), + 'chassis-asset-tag': kdmi('chassis_asset_tag', 'smbios.chassis.tag'), + 'chassis-manufacturer': kdmi('chassis_vendor', 'smbios.chassis.maker'), + 'chassis-serial-number': kdmi('chassis_serial', 'smbios.chassis.serial'), + 'chassis-version': kdmi('chassis_version', 'smbios.chassis.version'), + 'system-manufacturer': kdmi('sys_vendor', 'smbios.system.maker'), + 'system-product-name': kdmi('product_name', 'smbios.system.product'), + 'system-serial-number': kdmi('product_serial', 'smbios.system.serial'), + 'system-uuid': kdmi('product_uuid', 'smbios.system.uuid'), + 'system-version': kdmi('product_version', 'smbios.system.version'), } def _read_dmi_syspath(key): """ - Reads dmi data with from /sys/class/dmi/id + Reads dmi data from /sys/class/dmi/id """ - if key not in DMIDECODE_TO_DMI_SYS_MAPPING: + kmap = DMIDECODE_TO_KERNEL.get(key) + if kmap is None or kmap.linux is None: return None - mapped_key = DMIDECODE_TO_DMI_SYS_MAPPING[key] - dmi_key_path = "{0}/{1}".format(DMI_SYS_PATH, mapped_key) - + dmi_key_path = "{0}/{1}".format(DMI_SYS_PATH, kmap.linux) LOG.debug("querying dmi data %s", dmi_key_path) if not os.path.exists(dmi_key_path): LOG.debug("did not find %s", dmi_key_path) @@ -68,6 +74,29 @@ def _read_dmi_syspath(key): return None +def _read_kenv(key): + """ + Reads dmi data from FreeBSD's kenv(1) + """ + kmap = DMIDECODE_TO_KERNEL.get(key) + if kmap is None or kmap.freebsd is None: + return None + + LOG.debug("querying dmi data %s", kmap.freebsd) + + try: + cmd = ["kenv", "-q", kmap.freebsd] + (result, _err) = subp.subp(cmd) + result = result.strip() + LOG.debug("kenv returned '%s' for '%s'", result, kmap.freebsd) + return result + except subp.ProcessExecutionError as e: + LOG.debug('failed kenv cmd: %s\n%s', cmd, e) + return None + + return None + + def _call_dmidecode(key, dmidecode_path): """ Calls out to dmidecode to get the data out. This is mostly for supporting @@ -81,7 +110,7 @@ def _call_dmidecode(key, dmidecode_path): if result.replace(".", "") == "": return "" return result - except (IOError, OSError) as e: + except subp.ProcessExecutionError as e: LOG.debug('failed dmidecode cmd: %s\n%s', cmd, e) return None @@ -107,6 +136,9 @@ def read_dmi_data(key): if is_container(): return None + if is_FreeBSD(): + return _read_kenv(key) + syspath_value = _read_dmi_syspath(key) if syspath_value is not None: return syspath_value diff --git a/cloudinit/tests/test_dmi.py b/cloudinit/tests/test_dmi.py index 4a8af257..78a72122 100644 --- a/cloudinit/tests/test_dmi.py +++ b/cloudinit/tests/test_dmi.py @@ -19,6 +19,9 @@ class TestReadDMIData(helpers.FilesystemMockingTestCase): p = mock.patch("cloudinit.dmi.is_container", return_value=False) self.addCleanup(p.stop) self._m_is_container = p.start() + p = mock.patch("cloudinit.dmi.is_FreeBSD", return_value=False) + self.addCleanup(p.stop) + self._m_is_FreeBSD = p.start() def _create_sysfs_parent_directory(self): util.ensure_dir(os.path.join('sys', 'class', 'dmi', 'id')) @@ -44,13 +47,26 @@ class TestReadDMIData(helpers.FilesystemMockingTestCase): self.patched_funcs.enter_context( mock.patch("cloudinit.dmi.subp.subp", side_effect=_dmidecode_subp)) + def _configure_kenv_return(self, key, content, error=None): + """ + In order to test a FreeBSD system call outs to kenv, this + function fakes the results of kenv to test the results. + """ + def _kenv_subp(cmd): + if cmd[-1] != dmi.DMIDECODE_TO_KERNEL[key].freebsd: + raise subp.ProcessExecutionError() + return (content, error) + + self.patched_funcs.enter_context( + mock.patch("cloudinit.dmi.subp.subp", side_effect=_kenv_subp)) + def patch_mapping(self, new_mapping): self.patched_funcs.enter_context( - mock.patch('cloudinit.dmi.DMIDECODE_TO_DMI_SYS_MAPPING', + mock.patch('cloudinit.dmi.DMIDECODE_TO_KERNEL', new_mapping)) def test_sysfs_used_with_key_in_mapping_and_file_on_disk(self): - self.patch_mapping({'mapped-key': 'mapped-value'}) + self.patch_mapping({'mapped-key': dmi.kdmi('mapped-value', None)}) expected_dmi_value = 'sys-used-correctly' self._create_sysfs_file('mapped-value', expected_dmi_value) self._configure_dmidecode_return('mapped-key', 'wrong-wrong-wrong') @@ -129,3 +145,10 @@ class TestReadDMIData(helpers.FilesystemMockingTestCase): self._create_sysfs_file('product_name', "should-be-ignored") self.assertIsNone(dmi.read_dmi_data("bogus")) self.assertIsNone(dmi.read_dmi_data("system-product-name")) + + def test_freebsd_uses_kenv(self): + """On a FreeBSD system, kenv is called.""" + self._m_is_FreeBSD.return_value = True + key, val = ("system-product-name", "my_product") + self._configure_kenv_return(key, val) + self.assertEqual(dmi.read_dmi_data(key), val) diff --git a/cloudinit/util.py b/cloudinit/util.py index bdb3694d..769f3425 100644 --- a/cloudinit/util.py +++ b/cloudinit/util.py @@ -62,12 +62,6 @@ TRUE_STRINGS = ('true', '1', 'on', 'yes') FALSE_STRINGS = ('off', '0', 'no', 'false') -# Helper utils to see if running in a container -CONTAINER_TESTS = (['systemd-detect-virt', '--quiet', '--container'], - ['running-in-container'], - ['lxc-is-container']) - - def kernel_version(): return tuple(map(int, os.uname().release.split('.')[:2])) @@ -1928,19 +1922,52 @@ def strip_prefix_suffix(line, prefix=None, suffix=None): return line +def _cmd_exits_zero(cmd): + if subp.which(cmd[0]) is None: + return False + try: + subp.subp(cmd) + except subp.ProcessExecutionError: + return False + return True + + +def _is_container_systemd(): + return _cmd_exits_zero(["systemd-detect-virt", "--quiet", "--container"]) + + +def _is_container_upstart(): + return _cmd_exits_zero(["running-in-container"]) + + +def _is_container_old_lxc(): + return _cmd_exits_zero(["lxc-is-container"]) + + +def _is_container_freebsd(): + if not is_FreeBSD(): + return False + cmd = ["sysctl", "-qn", "security.jail.jailed"] + if subp.which(cmd[0]) is None: + return False + out, _ = subp.subp(cmd) + return out.strip() == "1" + + +@lru_cache() def is_container(): """ Checks to see if this code running in a container of some sort """ - - for helper in CONTAINER_TESTS: - try: - # try to run a helper program. if it returns true/zero - # then we're inside a container. otherwise, no - subp.subp(helper) + checks = ( + _is_container_systemd, + _is_container_freebsd, + _is_container_upstart, + _is_container_old_lxc) + + for helper in checks: + if helper(): return True - except (IOError, OSError): - pass # this code is largely from the logic in # ubuntu's /etc/init/container-detect.conf diff --git a/tests/unittests/test_ds_identify.py b/tests/unittests/test_ds_identify.py index 5f8a4a29..1d8aaf18 100644 --- a/tests/unittests/test_ds_identify.py +++ b/tests/unittests/test_ds_identify.py @@ -146,6 +146,8 @@ class DsIdentifyBase(CiTestCase): 'out': 'No value found', 'ret': 1}, {'name': 'dmi_decode', 'ret': 1, 'err': 'No dmidecode program. ERROR.'}, + {'name': 'get_kenv_field', 'ret': 1, + 'err': 'No kenv program. ERROR.'}, ] written = [d['name'] for d in mocks] @@ -651,14 +653,22 @@ class TestDsIdentify(DsIdentifyBase): class TestBSDNoSys(DsIdentifyBase): """Test *BSD code paths - FreeBSD doesn't have /sys so we use dmidecode(8) here - It also doesn't have systemd-detect-virt(8), so we use sysctl(8) to query + FreeBSD doesn't have /sys so we use kenv(1) here. + Other BSD systems fallback to dmidecode(8). + BSDs also doesn't have systemd-detect-virt(8), so we use sysctl(8) to query kern.vm_guest, and optionally map it""" - def test_dmi_decode(self): + def test_dmi_kenv(self): + """Test that kenv(1) works on systems which don't have /sys + + This will be used on FreeBSD systems. + """ + self._test_ds_found('Hetzner-kenv') + + def test_dmi_dmidecode(self): """Test that dmidecode(8) works on systems which don't have /sys - This will be used on *BSD systems. + This will be used on all other BSD systems. """ self._test_ds_found('Hetzner-dmidecode') @@ -1026,6 +1036,13 @@ VALID_CFG = { 'ds': 'Hetzner', 'files': {P_SYS_VENDOR: 'Hetzner\n'}, }, + 'Hetzner-kenv': { + 'ds': 'Hetzner', + 'mocks': [ + MOCK_UNAME_IS_FREEBSD, + {'name': 'get_kenv_field', 'ret': 0, 'RET': 'Hetzner'} + ], + }, 'Hetzner-dmidecode': { 'ds': 'Hetzner', 'mocks': [ diff --git a/tools/build-on-freebsd b/tools/build-on-freebsd index 94b03433..1e876905 100755 --- a/tools/build-on-freebsd +++ b/tools/build-on-freebsd @@ -21,7 +21,6 @@ py_prefix=$(${PYTHON} -c 'import sys; print("py%d%d" % (sys.version_info.major, depschecked=/tmp/c-i.dependencieschecked pkgs=" bash - dmidecode e2fsprogs $py_prefix-Jinja2 $py_prefix-boto diff --git a/tools/ds-identify b/tools/ds-identify index ec9e775e..496dbb8a 100755 --- a/tools/ds-identify +++ b/tools/ds-identify @@ -180,13 +180,43 @@ debug() { echo "$@" 1>&3 } +get_kenv_field() { + local sys_field="$1" kenv_field="" val="" + command -v kenv >/dev/null 2>&1 || { + warn "No kenv program. Cannot read $sys_field." + return 1 + } + case "$sys_field" in + board_asset_tag) kenv_field="smbios.planar.tag";; + board_vendor) kenv_field='smbios.planar.maker';; + board_name) kenv_field='smbios.planar.product';; + board_serial) kenv_field='smbios.planar.serial';; + board_version) kenv_field='smbios.planar.version';; + bios_date) kenv_field='smbios.bios.reldate';; + bios_vendor) kenv_field='smbios.bios.vendor';; + bios_version) kenv_field='smbios.bios.version';; + chassis_asset_tag) kenv_field='smbios.chassis.tag';; + chassis_vendor) kenv_field='smbios.chassis.maker';; + chassis_serial) kenv_field='smbios.chassis.serial';; + chassis_version) kenv_field='smbios.chassis.version';; + sys_vendor) kenv_field='smbios.system.maker';; + product_name) kenv_field='smbios.system.product';; + product_serial) kenv_field='smbios.system.serial';; + product_uuid) kenv_field='smbios.system.uuid';; + *) error "Unknown field $sys_field. Cannot call kenv." + return 1;; + esac + val=$(kenv -q "$kenv_field" 2>/dev/null) || return 1 + _RET="$val" +} + dmi_decode() { local sys_field="$1" dmi_field="" val="" command -v dmidecode >/dev/null 2>&1 || { warn "No dmidecode program. Cannot read $sys_field." return 1 } - case "$1" in + case "$sys_field" in sys_vendor) dmi_field="system-manufacturer";; product_name) dmi_field="system-product-name";; product_uuid) dmi_field="system-uuid";; @@ -200,8 +230,14 @@ dmi_decode() { } get_dmi_field() { - local path="${PATH_SYS_CLASS_DMI_ID}/$1" _RET="$UNAVAILABLE" + + if [ "$DI_UNAME_KERNEL_NAME" = "FreeBSD" ]; then + get_kenv_field "$1" || _RET="$ERROR" + return $? + fi + + local path="${PATH_SYS_CLASS_DMI_ID}/$1" if [ -d "${PATH_SYS_CLASS_DMI_ID}" ]; then if [ -f "$path" ] && [ -r "$path" ]; then read _RET < "${path}" || _RET="$ERROR" @@ -1310,10 +1346,10 @@ dscheck_IBMCloud() { } collect_info() { + read_uname_info read_virt read_pid1_product_name read_kernel_cmdline - read_uname_info read_config read_datasource_list read_dmi_sys_vendor -- cgit v1.2.3 From a925b5a0ca4aa3e63b084c0f6664fe815c2c9db0 Mon Sep 17 00:00:00 2001 From: Till Riedel Date: Tue, 17 Nov 2020 20:54:14 +0100 Subject: add --no-tty option to gpg (#669) Make sure that gpg works even if the instance has no /dev/tty. This has been observed on Debian. LP: #1813396 --- cloudinit/gpg.py | 2 +- cloudinit/tests/test_gpg.py | 3 ++- tools/.github-cla-signers | 1 + 3 files changed, 4 insertions(+), 2 deletions(-) (limited to 'cloudinit/tests') diff --git a/cloudinit/gpg.py b/cloudinit/gpg.py index be0ca0ea..3780326c 100644 --- a/cloudinit/gpg.py +++ b/cloudinit/gpg.py @@ -42,7 +42,7 @@ def recv_key(key, keyserver, retries=(1, 1)): @param retries: an iterable of sleep lengths for retries. Use None to indicate no retries.""" LOG.debug("Importing key '%s' from keyserver '%s'", key, keyserver) - cmd = ["gpg", "--keyserver=%s" % keyserver, "--recv-keys", key] + cmd = ["gpg", "--no-tty", "--keyserver=%s" % keyserver, "--recv-keys", key] if retries is None: retries = [] trynum = 0 diff --git a/cloudinit/tests/test_gpg.py b/cloudinit/tests/test_gpg.py index f96f5372..311dfad6 100644 --- a/cloudinit/tests/test_gpg.py +++ b/cloudinit/tests/test_gpg.py @@ -49,6 +49,7 @@ class TestReceiveKeys(CiTestCase): m_subp.return_value = ('', '') gpg.recv_key(key, keyserver, retries=retries) m_subp.assert_called_once_with( - ['gpg', '--keyserver=%s' % keyserver, '--recv-keys', key], + ['gpg', '--no-tty', + '--keyserver=%s' % keyserver, '--recv-keys', key], capture=True) m_sleep.assert_not_called() diff --git a/tools/.github-cla-signers b/tools/.github-cla-signers index 2090dc2a..b2464768 100644 --- a/tools/.github-cla-signers +++ b/tools/.github-cla-signers @@ -21,6 +21,7 @@ matthewruffell nishigori omBratteng onitake +riedel slyon smoser sshedi -- cgit v1.2.3 From 4f2da1cc1d24cbc47025cc8613c0d3ec287a20f9 Mon Sep 17 00:00:00 2001 From: Daniel Watkins Date: Tue, 17 Nov 2020 16:37:29 -0500 Subject: introduce an upgrade framework and related testing (#659) This commit does the following: * introduces the `cloudinit.persistence` module, containing `CloudInitPickleMixin` which provides lightweight versioning of objects' pickled representations (and associated testing) * introduces a basic upgrade testing framework (in `cloudinit.tests.test_upgrade`) which unpickles pickles from previous versions of cloud-init (stored in `tests/data/old_pickles`) and tests invariants that the current cloud-init codebase expects * uses the versioning framework to address an upgrade issue where `Distro.networking` could get into an unexpected state, and uses the upgrade testing framework to confirm that the issue is addressed --- cloudinit/distros/__init__.py | 17 ++- cloudinit/persistence.py | 67 ++++++++++++ cloudinit/tests/test_persistence.py | 118 +++++++++++++++++++++ cloudinit/tests/test_upgrade.py | 45 ++++++++ .../focal-20.1-10-g71af48df-0ubuntu5.pkl | Bin 0 -> 7135 bytes .../focal-20.3-2-g371b392c-0ubuntu1~20.04.1.pkl | Bin 0 -> 7215 bytes 6 files changed, 246 insertions(+), 1 deletion(-) create mode 100644 cloudinit/persistence.py create mode 100644 cloudinit/tests/test_persistence.py create mode 100644 cloudinit/tests/test_upgrade.py create mode 100644 tests/data/old_pickles/focal-20.1-10-g71af48df-0ubuntu5.pkl create mode 100644 tests/data/old_pickles/focal-20.3-2-g371b392c-0ubuntu1~20.04.1.pkl (limited to 'cloudinit/tests') diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py index fac8cf67..1e118472 100755 --- a/cloudinit/distros/__init__.py +++ b/cloudinit/distros/__init__.py @@ -23,6 +23,7 @@ from cloudinit import net from cloudinit.net import eni from cloudinit.net import network_state from cloudinit.net import renderers +from cloudinit import persistence from cloudinit import ssh_util from cloudinit import type_utils from cloudinit import subp @@ -62,7 +63,7 @@ PREFERRED_NTP_CLIENTS = ['chrony', 'systemd-timesyncd', 'ntp', 'ntpdate'] LDH_ASCII_CHARS = string.ascii_letters + string.digits + "-" -class Distro(metaclass=abc.ABCMeta): +class Distro(persistence.CloudInitPickleMixin, metaclass=abc.ABCMeta): usr_lib_exec = "/usr/lib" hosts_fn = "/etc/hosts" @@ -77,12 +78,26 @@ class Distro(metaclass=abc.ABCMeta): # subclasses shutdown_options_map = {'halt': '-H', 'poweroff': '-P', 'reboot': '-r'} + _ci_pkl_version = 1 + def __init__(self, name, cfg, paths): self._paths = paths self._cfg = cfg self.name = name self.networking = self.networking_cls() + def _unpickle(self, ci_pkl_version: int) -> None: + """Perform deserialization fixes for Distro.""" + if "networking" not in self.__dict__ or not self.networking.__dict__: + # This is either a Distro pickle with no networking attribute OR + # this is a Distro pickle with a networking attribute but from + # before ``Networking`` had any state (meaning that + # Networking.__setstate__ will not be called). In either case, we + # want to ensure that `self.networking` is freshly-instantiated: + # either because it isn't present at all, or because it will be + # missing expected instance state otherwise. + self.networking = self.networking_cls() + @abc.abstractmethod def install_packages(self, pkglist): raise NotImplementedError() diff --git a/cloudinit/persistence.py b/cloudinit/persistence.py new file mode 100644 index 00000000..85aa79df --- /dev/null +++ b/cloudinit/persistence.py @@ -0,0 +1,67 @@ +# Copyright (C) 2020 Canonical Ltd. +# +# Author: Daniel Watkins +# +# This file is part of cloud-init. See LICENSE file for license information. + + +class CloudInitPickleMixin: + """Scaffolding for versioning of pickles. + + This class implements ``__getstate__`` and ``__setstate__`` to provide + lightweight versioning of the pickles that are generated for classes which + use it. Versioning is done at the class level. + + The current version of a class's pickle should be set in the class variable + ``_ci_pkl_version``, as an int. If not overriden, it will default to 0. + + On unpickle, the object's state will be restored and then + ``self._unpickle`` is called with the version of the stored pickle as the + only argument: this is where classes should implement any deserialization + fixes they require. (If the stored pickle has no version, 0 is passed.) + """ + + _ci_pkl_version = 0 + + def __getstate__(self): + """Persist instance state, adding a pickle version attribute. + + This adds a ``_ci_pkl_version`` attribute to ``self.__dict__`` and + returns that for serialisation. The attribute is stripped out in + ``__setstate__`` on unpickle. + + The value of ``_ci_pkl_version`` is ``type(self)._ci_pkl_version``. + """ + state = self.__dict__.copy() + state["_ci_pkl_version"] = type(self)._ci_pkl_version + return state + + def __setstate__(self, state: dict) -> None: + """Restore instance state and handle missing attributes on upgrade. + + This will be called when an instance of this class is unpickled; the + previous instance's ``__dict__`` is passed as ``state``. This method + removes the pickle version from the stored state, restores the + remaining state into the current instance, and then calls + ``self._unpickle`` with the version (or 0, if no version is found in + the stored state). + + See https://docs.python.org/3/library/pickle.html#object.__setstate__ + for further background. + """ + version = state.pop("_ci_pkl_version", 0) + self.__dict__.update(state) + self._unpickle(version) + + def _unpickle(self, ci_pkl_version: int) -> None: + """Perform any deserialization fixes required. + + By default, this does nothing. Classes using this mixin should + override this method if they have fixes they need to apply. + + ``ci_pkl_version`` will be the version stored in the pickle for this + object, or 0 if no version is present. + """ + + +# vi: ts=4 expandtab diff --git a/cloudinit/tests/test_persistence.py b/cloudinit/tests/test_persistence.py new file mode 100644 index 00000000..7b64ced9 --- /dev/null +++ b/cloudinit/tests/test_persistence.py @@ -0,0 +1,118 @@ +# Copyright (C) 2020 Canonical Ltd. +# +# Author: Daniel Watkins +# +# This file is part of cloud-init. See LICENSE file for license information. +""" +Tests for cloudinit.persistence. + +Per https://docs.python.org/3/library/pickle.html, only "classes that are +defined at the top level of a module" can be pickled. This means that all of +our ``CloudInitPickleMixin`` subclasses for testing must be defined at +module-level (rather than being defined inline or dynamically in the body of +test methods, as we would do without this constraint). + +``TestPickleMixin.test_subclasses`` iterates over a list of all of these +classes, and tests that they round-trip through a pickle dump/load. As the +interface we're testing is that ``_unpickle`` is called appropriately on +subclasses, our subclasses define their assertions in their ``_unpickle`` +implementation. (This means that the assertions will not be executed if +``_unpickle`` is not called at all; we have +``TestPickleMixin.test_unpickle_called`` to ensure it is called.) + +To avoid manually maintaining a list of classes for parametrization we use a +simple metaclass, ``_Collector``, to gather them up. +""" + +import pickle +from unittest import mock + +import pytest + +from cloudinit.persistence import CloudInitPickleMixin + + +class _Collector(type): + """Any class using this as a metaclass will be stored in test_classes.""" + + test_classes = [] + + def __new__(cls, *args): + new_cls = super().__new__(cls, *args) + _Collector.test_classes.append(new_cls) + return new_cls + + +class InstanceVersionNotUsed(CloudInitPickleMixin, metaclass=_Collector): + """Test that the class version is used over one set in instance state.""" + + _ci_pkl_version = 1 + + def __init__(self): + self._ci_pkl_version = 2 + + def _unpickle(self, ci_pkl_version: int) -> None: + assert 1 == ci_pkl_version + + +class MissingVersionHandled(CloudInitPickleMixin, metaclass=_Collector): + """Test that pickles without ``_ci_pkl_version`` are handled gracefully. + + This is tested by overriding ``__getstate__`` so the dumped pickle of this + class will not have ``_ci_pkl_version`` included. + """ + + def __getstate__(self): + return self.__dict__ + + def _unpickle(self, ci_pkl_version: int) -> None: + assert 0 == ci_pkl_version + + +class OverridenVersionHonored(CloudInitPickleMixin, metaclass=_Collector): + """Test that the subclass's version is used.""" + + _ci_pkl_version = 1 + + def _unpickle(self, ci_pkl_version: int) -> None: + assert 1 == ci_pkl_version + + +class StateIsRestored(CloudInitPickleMixin, metaclass=_Collector): + """Instance state should be restored before ``_unpickle`` is called.""" + + def __init__(self): + self.some_state = "some state" + + def _unpickle(self, ci_pkl_version: int) -> None: + assert "some state" == self.some_state + + +class UnpickleCanBeUnoverriden(CloudInitPickleMixin, metaclass=_Collector): + """Subclasses should not need to override ``_unpickle``.""" + + +class VersionDefaultsToZero(CloudInitPickleMixin, metaclass=_Collector): + """Test that the default version is 0.""" + + def _unpickle(self, ci_pkl_version: int) -> None: + assert 0 == ci_pkl_version + + +class TestPickleMixin: + def test_unpickle_called(self): + """Test that self._unpickle is called on unpickle.""" + with mock.patch.object( + CloudInitPickleMixin, "_unpickle" + ) as m_unpickle: + pickle.loads(pickle.dumps(CloudInitPickleMixin())) + assert 1 == m_unpickle.call_count + + @pytest.mark.parametrize("cls", _Collector.test_classes) + def test_subclasses(self, cls): + """For each collected class, round-trip through pickle dump/load. + + Assertions are implemented in ``cls._unpickle``, and so are evoked as + part of the pickle load. + """ + pickle.loads(pickle.dumps(cls())) diff --git a/cloudinit/tests/test_upgrade.py b/cloudinit/tests/test_upgrade.py new file mode 100644 index 00000000..f79a2536 --- /dev/null +++ b/cloudinit/tests/test_upgrade.py @@ -0,0 +1,45 @@ +# Copyright (C) 2020 Canonical Ltd. +# +# Author: Daniel Watkins +# +# This file is part of cloud-init. See LICENSE file for license information. + +"""Upgrade testing for cloud-init. + +This module tests cloud-init's behaviour across upgrades. Specifically, it +specifies a set of invariants that the current codebase expects to be true (as +tests in ``TestUpgrade``) and then checks that these hold true after unpickling +``obj.pkl``s from previous versions of cloud-init; those pickles are stored in +``tests/data/old_pickles/``. +""" + +import operator +import pathlib + +import pytest + +from cloudinit.stages import _pkl_load +from cloudinit.tests.helpers import resourceLocation + + +class TestUpgrade: + @pytest.fixture( + params=pathlib.Path(resourceLocation("old_pickles")).glob("*.pkl"), + scope="class", + ids=operator.attrgetter("name"), + ) + def previous_obj_pkl(self, request): + """Load each pickle to memory once, then run all tests against it. + + Test implementations _must not_ modify the ``previous_obj_pkl`` which + they are passed, as that will affect tests that run after them. + """ + return _pkl_load(str(request.param)) + + def test_networking_set_on_distro(self, previous_obj_pkl): + """We always expect to have ``.networking`` on ``Distro`` objects.""" + assert previous_obj_pkl.distro.networking is not None + + def test_blacklist_drivers_set_on_networking(self, previous_obj_pkl): + """We always expect Networking.blacklist_drivers to be initialised.""" + assert previous_obj_pkl.distro.networking.blacklist_drivers is None diff --git a/tests/data/old_pickles/focal-20.1-10-g71af48df-0ubuntu5.pkl b/tests/data/old_pickles/focal-20.1-10-g71af48df-0ubuntu5.pkl new file mode 100644 index 00000000..358813b4 Binary files /dev/null and b/tests/data/old_pickles/focal-20.1-10-g71af48df-0ubuntu5.pkl differ diff --git a/tests/data/old_pickles/focal-20.3-2-g371b392c-0ubuntu1~20.04.1.pkl b/tests/data/old_pickles/focal-20.3-2-g371b392c-0ubuntu1~20.04.1.pkl new file mode 100644 index 00000000..e26f98d8 Binary files /dev/null and b/tests/data/old_pickles/focal-20.3-2-g371b392c-0ubuntu1~20.04.1.pkl differ -- cgit v1.2.3 From e1bde919923ff1f9d1d197f64ea43b976f034062 Mon Sep 17 00:00:00 2001 From: Daniel Watkins Date: Tue, 17 Nov 2020 16:55:55 -0500 Subject: test_persistence: add VersionIsPoppedFromState test (#673) --- cloudinit/tests/test_persistence.py | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'cloudinit/tests') diff --git a/cloudinit/tests/test_persistence.py b/cloudinit/tests/test_persistence.py index 7b64ced9..3e5eaa8d 100644 --- a/cloudinit/tests/test_persistence.py +++ b/cloudinit/tests/test_persistence.py @@ -99,6 +99,16 @@ class VersionDefaultsToZero(CloudInitPickleMixin, metaclass=_Collector): assert 0 == ci_pkl_version +class VersionIsPoppedFromState(CloudInitPickleMixin, metaclass=_Collector): + """Test _ci_pkl_version is popped from state before being restored.""" + + def _unpickle(self, ci_pkl_version: int) -> None: + # `self._ci_pkl_version` returns the type's _ci_pkl_version if it isn't + # in instance state, so we need to explicitly check self.__dict__. + sentinel = mock.sentinel.default + assert self.__dict__.get("_ci_pkl_version", sentinel) == sentinel + + class TestPickleMixin: def test_unpickle_called(self): """Test that self._unpickle is called on unpickle.""" -- cgit v1.2.3 From 96d21dfbee308cd8fe00809184f78da9231ece4a Mon Sep 17 00:00:00 2001 From: Daniel Watkins Date: Wed, 18 Nov 2020 11:11:58 -0500 Subject: test_persistence: simplify VersionIsPoppedFromState (#674) --- cloudinit/tests/test_persistence.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'cloudinit/tests') diff --git a/cloudinit/tests/test_persistence.py b/cloudinit/tests/test_persistence.py index 3e5eaa8d..ec1152a9 100644 --- a/cloudinit/tests/test_persistence.py +++ b/cloudinit/tests/test_persistence.py @@ -105,8 +105,7 @@ class VersionIsPoppedFromState(CloudInitPickleMixin, metaclass=_Collector): def _unpickle(self, ci_pkl_version: int) -> None: # `self._ci_pkl_version` returns the type's _ci_pkl_version if it isn't # in instance state, so we need to explicitly check self.__dict__. - sentinel = mock.sentinel.default - assert self.__dict__.get("_ci_pkl_version", sentinel) == sentinel + assert "_ci_pkl_version" not in self.__dict__ class TestPickleMixin: -- cgit v1.2.3