From f8c84aeead77b7e508644d94889ee701f20e8d31 Mon Sep 17 00:00:00 2001 From: dermotbradley Date: Fri, 30 Oct 2020 20:12:38 +0000 Subject: Correct documentation and testcase data for some user-data YAML (#618) For cc_users_groups the user setting "expiredate" must be quoted in order for the relevant flag and value to be then passed to the useradd command. It its vaiue is not quoted then it is treated as Python type datetime.date and in `cloudinit/distros/__init__.py` the below "is it a string" condition fails and so no "--expiredate" parameter is passed to useradd and therefore it has no effect: ``` if key in useradd_opts and val and isinstance(val, str): useradd_cmd.extend([useradd_opts[key], val]) ``` For cc_users_groups, the user setting "inactive" does not actually disable accounts, the useradd "--inactive" option actually defines the number of days after password expiry that users can still login. So I have changed the docs to show it taking a quoted value of days (which works with the current code) rather than a boolean value. The quotes are necessary, like expiredate above, so that the value is also passed to the useradd command. For cc_power_state_change.py the "delay" setting value needs to have quotes around it as otherwise its leading plus sign will be stripped off. --- tests/data/merge_sources/expected10.yaml | 2 +- tests/data/merge_sources/expected7.yaml | 6 +++--- tests/data/merge_sources/source10-1.yaml | 2 +- tests/data/merge_sources/source7-1.yaml | 4 ++-- tests/data/merge_sources/source7-2.yaml | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) (limited to 'tests/data') diff --git a/tests/data/merge_sources/expected10.yaml b/tests/data/merge_sources/expected10.yaml index b865db16..e9f88f7b 100644 --- a/tests/data/merge_sources/expected10.yaml +++ b/tests/data/merge_sources/expected10.yaml @@ -1,7 +1,7 @@ #cloud-config power_state: - delay: 30 + delay: '+30' mode: poweroff message: [Bye, Bye, Pew, Pew] diff --git a/tests/data/merge_sources/expected7.yaml b/tests/data/merge_sources/expected7.yaml index d32988e8..8186d13a 100644 --- a/tests/data/merge_sources/expected7.yaml +++ b/tests/data/merge_sources/expected7.yaml @@ -7,7 +7,7 @@ users: primary_group: foobar groups: users selinux_user: staff_u - expiredate: 2012-09-01 + expiredate: '2012-09-01' ssh_import_id: foobar lock-passwd: false passwd: $6$j212wezy$7H/1LT4f9/N3wpgNunhsIqtMj62OKiS3nyNwuizouQc3u7MbYCarYeAHWYPYb2FT.lbioDm2RrkJPb9BZMN1O/ @@ -22,7 +22,7 @@ users: - - name: cloudy gecos: Magic Cloud App Daemon User - inactive: true + inactive: '5' system: true - bob - joe @@ -32,7 +32,7 @@ users: primary_group: foobar groups: users selinux_user: staff_u - expiredate: 2012-09-01 + expiredate: '2012-09-01' ssh_import_id: foobar lock-passwd: false passwd: $6$j212wezy$7H/1LT4f9/N3wpgNunhsIqtMj62OKiS3nyNwuizouQc3u7MbYCarYeAHWYPYb2FT.lbioDm2RrkJPb9BZMN1O/ diff --git a/tests/data/merge_sources/source10-1.yaml b/tests/data/merge_sources/source10-1.yaml index 6ae72a13..36fd336d 100644 --- a/tests/data/merge_sources/source10-1.yaml +++ b/tests/data/merge_sources/source10-1.yaml @@ -1,6 +1,6 @@ #cloud-config power_state: - delay: 30 + delay: '+30' mode: poweroff message: [Bye, Bye] diff --git a/tests/data/merge_sources/source7-1.yaml b/tests/data/merge_sources/source7-1.yaml index 6405fc9b..ec93079f 100644 --- a/tests/data/merge_sources/source7-1.yaml +++ b/tests/data/merge_sources/source7-1.yaml @@ -7,7 +7,7 @@ users: primary_group: foobar groups: users selinux_user: staff_u - expiredate: 2012-09-01 + expiredate: '2012-09-01' ssh_import_id: foobar lock-passwd: false passwd: $6$j212wezy$7H/1LT4f9/N3wpgNunhsIqtMj62OKiS3nyNwuizouQc3u7MbYCarYeAHWYPYb2FT.lbioDm2RrkJPb9BZMN1O/ @@ -22,6 +22,6 @@ users: - - name: cloudy gecos: Magic Cloud App Daemon User - inactive: true + inactive: '5' system: true diff --git a/tests/data/merge_sources/source7-2.yaml b/tests/data/merge_sources/source7-2.yaml index 0cd28978..0c02abff 100644 --- a/tests/data/merge_sources/source7-2.yaml +++ b/tests/data/merge_sources/source7-2.yaml @@ -9,7 +9,7 @@ users: primary_group: foobar groups: users selinux_user: staff_u - expiredate: 2012-09-01 + expiredate: '2012-09-01' ssh_import_id: foobar lock-passwd: false passwd: $6$j212wezy$7H/1LT4f9/N3wpgNunhsIqtMj62OKiS3nyNwuizouQc3u7MbYCarYeAHWYPYb2FT.lbioDm2RrkJPb9BZMN1O/ -- 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 'tests/data') 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