summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDaniel Watkins <oddbloke@ubuntu.com>2020-11-17 16:37:29 -0500
committerGitHub <noreply@github.com>2020-11-17 16:37:29 -0500
commit4f2da1cc1d24cbc47025cc8613c0d3ec287a20f9 (patch)
tree00db3d5efc4a83e7fd4251d88943075e56b165a9
parenta925b5a0ca4aa3e63b084c0f6664fe815c2c9db0 (diff)
downloadvyos-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-xcloudinit/distros/__init__.py17
-rw-r--r--cloudinit/persistence.py67
-rw-r--r--cloudinit/tests/test_persistence.py118
-rw-r--r--cloudinit/tests/test_upgrade.py45
-rw-r--r--tests/data/old_pickles/focal-20.1-10-g71af48df-0ubuntu5.pklbin0 -> 7135 bytes
-rw-r--r--tests/data/old_pickles/focal-20.3-2-g371b392c-0ubuntu1~20.04.1.pklbin0 -> 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
new file mode 100644
index 00000000..358813b4
--- /dev/null
+++ b/tests/data/old_pickles/focal-20.1-10-g71af48df-0ubuntu5.pkl
Binary files 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
--- /dev/null
+++ b/tests/data/old_pickles/focal-20.3-2-g371b392c-0ubuntu1~20.04.1.pkl
Binary files differ