diff options
author | Daniel Watkins <oddbloke@ubuntu.com> | 2020-11-17 16:37:29 -0500 |
---|---|---|
committer | GitHub <noreply@github.com> | 2020-11-17 16:37:29 -0500 |
commit | 4f2da1cc1d24cbc47025cc8613c0d3ec287a20f9 (patch) | |
tree | 00db3d5efc4a83e7fd4251d88943075e56b165a9 | |
parent | a925b5a0ca4aa3e63b084c0f6664fe815c2c9db0 (diff) | |
download | vyos-cloud-init-4f2da1cc1d24cbc47025cc8613c0d3ec287a20f9.tar.gz vyos-cloud-init-4f2da1cc1d24cbc47025cc8613c0d3ec287a20f9.zip |
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
-rwxr-xr-x | cloudinit/distros/__init__.py | 17 | ||||
-rw-r--r-- | cloudinit/persistence.py | 67 | ||||
-rw-r--r-- | cloudinit/tests/test_persistence.py | 118 | ||||
-rw-r--r-- | cloudinit/tests/test_upgrade.py | 45 | ||||
-rw-r--r-- | tests/data/old_pickles/focal-20.1-10-g71af48df-0ubuntu5.pkl | bin | 0 -> 7135 bytes | |||
-rw-r--r-- | tests/data/old_pickles/focal-20.3-2-g371b392c-0ubuntu1~20.04.1.pkl | bin | 0 -> 7215 bytes |
6 files changed, 246 insertions, 1 deletions
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 <oddbloke@ubuntu.com> +# +# 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 <oddbloke@ubuntu.com> +# +# 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 <oddbloke@ubuntu.com> +# +# 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 Binary files differnew file mode 100644 index 00000000..358813b4 --- /dev/null +++ b/tests/data/old_pickles/focal-20.1-10-g71af48df-0ubuntu5.pkl 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 Binary files differnew file mode 100644 index 00000000..e26f98d8 --- /dev/null +++ b/tests/data/old_pickles/focal-20.3-2-g371b392c-0ubuntu1~20.04.1.pkl |