launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #07126
[Merge] lp:~allenap/maas/delete-pxe-file-bug-978425 into lp:maas
Gavin Panella has proposed merging lp:~allenap/maas/delete-pxe-file-bug-978425 into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #978425 in MAAS: "Deleting a node leaves the PXE file"
https://bugs.launchpad.net/maas/+bug/978425
For more details, see:
https://code.launchpad.net/~allenap/maas/delete-pxe-file-bug-978425/+merge/102122
Cobbler tries to keep the on-disk part of its database in sync with the in-memory part, but sometimes gets it wrong. For example, deleting a network interface from a node does not cause the related netboot files to be removed from the filesystem. A full `sync` call is necessary.
This branch fixes that specific problem, and adds a regression test for it, but also adds sync calls to the end of all PAPI calls that mutate Cobbler, so we can be reasonably sure that the on-disk stuff is all in order.
--
https://code.launchpad.net/~allenap/maas/delete-pxe-file-bug-978425/+merge/102122
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/delete-pxe-file-bug-978425 into lp:maas.
=== modified file 'src/provisioningserver/api.py'
--- src/provisioningserver/api.py 2012-04-05 17:42:01 +0000
+++ src/provisioningserver/api.py 2012-04-16 14:53:14 +0000
@@ -170,6 +170,11 @@
super(ProvisioningAPI, self).__init__()
self.session = session
+ def sync(self):
+ """Request Cobbler to sync and return when it's finished."""
+ return self.session.call(
+ "sync", self.session.token_placeholder)
+
@inlineCallbacks
def add_distro(self, name, initrd, kernel):
assert isinstance(name, basestring)
@@ -180,6 +185,7 @@
"initrd": initrd,
"kernel": kernel,
})
+ yield self.sync()
returnValue(distro.name)
@inlineCallbacks
@@ -188,6 +194,7 @@
assert isinstance(distro, basestring)
profile = yield CobblerProfile.new(
self.session, name, {"distro": distro})
+ yield self.sync()
returnValue(profile.name)
@inlineCallbacks
@@ -204,17 +211,20 @@
"power_type": power_type,
}
system = yield CobblerSystem.new(self.session, name, attributes)
+ yield self.sync()
returnValue(system.name)
@inlineCallbacks
def modify_distros(self, deltas):
for name, delta in deltas.items():
yield CobblerDistro(self.session, name).modify(delta)
+ yield self.sync()
@inlineCallbacks
def modify_profiles(self, deltas):
for name, delta in deltas.items():
yield CobblerProfile(self.session, name).modify(delta)
+ yield self.sync()
@inlineCallbacks
def modify_nodes(self, deltas):
@@ -231,6 +241,7 @@
for interface_modification in interface_modifications:
yield system.modify(interface_modification)
yield system.modify(delta)
+ yield self.sync()
@inlineCallbacks
def get_objects_by_name(self, object_type, names):
@@ -278,6 +289,7 @@
assert all(isinstance(name, basestring) for name in names)
for name in names:
yield object_type(self.session, name).delete()
+ yield self.sync()
@deferred
def delete_distros_by_name(self, names):
=== modified file 'src/provisioningserver/testing/realcobbler.py'
--- src/provisioningserver/testing/realcobbler.py 2012-03-16 10:19:30 +0000
+++ src/provisioningserver/testing/realcobbler.py 2012-04-16 14:53:14 +0000
@@ -16,6 +16,7 @@
from os import environ
from textwrap import dedent
from urlparse import urlparse
+from unittest import skipIf
from provisioningserver.cobblerclient import CobblerSession
@@ -33,11 +34,17 @@
env_var = 'PSERV_TEST_COBBLER_URL'
- help_text = dedent("""
+ help_text_available = dedent("""\
Set %s to the URL for a Cobbler instance to test against,
+ e.g. http://username:password@xxxxxxxxxxx/cobbler_api.
+ WARNING: this will modify your Cobbler database.
+ """ % env_var)
+
+ help_text_local = dedent("""\
+ Set %s to the URL for a *local* Cobbler instance to test against,
e.g. http://username:password@localhost/cobbler_api.
WARNING: this will modify your Cobbler database.
- """.lstrip('\n') % env_var)
+ """ % env_var)
def __init__(self):
self.url = environ.get(self.env_var)
@@ -46,23 +53,52 @@
self.username = urlparts.username or 'cobbler'
self.password = urlparts.password or ''
+ @property
def is_available(self):
- """Is a real Cobbler available for tests?
-
- Use this to disable real-Cobbler tests if no real Cobbler is
- available: annotate them with
-
- @testtools.skipIf(
- not real_cobbler.is_available(), RealCobbler.help_text)
- """
+ """Is a real Cobbler available for tests?"""
return self.url is not None
+ @property
+ def skip_unless_available(self):
+ """Decorator to disable tests if no real Cobbler is available.
+
+ Annotate tests like so::
+
+ @real_cobbler.skip_unless_available
+ def test_something_that_requires_a_real_cobbler(self):
+ ...
+
+ """
+ return skipIf(not self.is_available, self.help_text_available)
+
+ @property
+ def is_local(self):
+ """Is a real Cobbler installed locally available for tests?"""
+ if self.is_available:
+ hostname = urlparse(self.url).hostname
+ return hostname == "localhost" or hostname.startswith("127.")
+ else:
+ return False
+
+ @property
+ def skip_unless_local(self):
+ """Decorator to disable tests if no real *local* Cobbler is available.
+
+ Annotate tests like so::
+
+ @real_cobbler.skip_unless_local
+ def test_something_that_requires_a_real_local_cobbler(self):
+ ...
+
+ """
+ return skipIf(not self.is_local, self.help_text_local)
+
def get_session(self):
"""Obtain a session on the real Cobbler.
Returns None if no real Cobbler is available.
"""
- if self.is_available():
+ if self.is_available:
return CobblerSession(self.url, self.username, self.password)
else:
return None
=== modified file 'src/provisioningserver/tests/test_api.py'
--- src/provisioningserver/tests/test_api.py 2012-04-05 18:39:06 +0000
+++ src/provisioningserver/tests/test_api.py 2012-04-16 14:53:14 +0000
@@ -19,7 +19,8 @@
abstractmethod,
)
from base64 import b64decode
-from unittest import skipIf
+from contextlib import contextmanager
+from functools import partial
from maasserver.testing.enum import map_enum
from maastesting.factory import factory
@@ -40,6 +41,11 @@
from provisioningserver.testing.realcobbler import RealCobbler
from testtools import TestCase
from testtools.deferredruntest import AsynchronousDeferredRunTest
+from testtools.matchers import (
+ FileExists,
+ Not,
+ )
+from testtools.monkey import patch
from twisted.internet.defer import inlineCallbacks
from zope.interface.verify import verifyObject
@@ -607,6 +613,84 @@
self.assertEqual(
preseed_data, b64decode(attrs['ks_meta']['MAAS_PRESEED']))
+ @contextmanager
+ def expected_sync(self, papi, times=1):
+ """Context where # calls to `papi.sync` must match `times`."""
+ sync_calls = []
+ orig_sync = papi.sync
+ fake_sync = lambda: orig_sync().addCallback(sync_calls.append)
+ unpatch = patch(papi, "sync", fake_sync)
+ try:
+ yield
+ finally:
+ unpatch()
+ self.assertEqual(times, len(sync_calls))
+
+ @inlineCallbacks
+ def test_add_distro_syncs(self):
+ # add_distro ensures that Cobbler syncs.
+ papi = self.get_provisioning_api()
+ with self.expected_sync(papi):
+ yield self.add_distro(papi)
+
+ @inlineCallbacks
+ def test_add_profile_syncs(self):
+ # add_profile ensures that Cobbler syncs.
+ papi = self.get_provisioning_api()
+ distro_name = yield self.add_distro(papi)
+ with self.expected_sync(papi):
+ yield self.add_profile(papi, distro_name=distro_name)
+
+ @inlineCallbacks
+ def test_add_node_syncs(self):
+ # add_node ensures that Cobbler syncs.
+ papi = self.get_provisioning_api()
+ profile_name = yield self.add_profile(papi)
+ with self.expected_sync(papi):
+ yield self.add_node(papi, profile_name=profile_name)
+
+ @inlineCallbacks
+ def test_modify_distros_syncs(self):
+ # modify_distros ensures that Cobbler syncs.
+ papi = self.get_provisioning_api()
+ with self.expected_sync(papi):
+ yield papi.modify_distros({})
+
+ @inlineCallbacks
+ def test_modify_profiles_syncs(self):
+ # modify_profiles ensures that Cobbler syncs.
+ papi = self.get_provisioning_api()
+ with self.expected_sync(papi):
+ yield papi.modify_profiles({})
+
+ @inlineCallbacks
+ def test_modify_nodes_syncs(self):
+ # modify_nodes ensures that Cobbler syncs.
+ papi = self.get_provisioning_api()
+ with self.expected_sync(papi):
+ yield papi.modify_nodes({})
+
+ @inlineCallbacks
+ def test_delete_distros_by_name_syncs(self):
+ # delete_distros_by_name ensures that Cobbler syncs.
+ papi = self.get_provisioning_api()
+ with self.expected_sync(papi):
+ yield papi.delete_distros_by_name([])
+
+ @inlineCallbacks
+ def test_delete_profiles_by_name_syncs(self):
+ # delete_profiles_by_name ensures that Cobbler syncs.
+ papi = self.get_provisioning_api()
+ with self.expected_sync(papi):
+ yield papi.delete_profiles_by_name([])
+
+ @inlineCallbacks
+ def test_delete_nodes_by_name_syncs(self):
+ # delete_nodes_by_name ensures that Cobbler syncs.
+ papi = self.get_provisioning_api()
+ with self.expected_sync(papi):
+ yield papi.delete_nodes_by_name([])
+
class TestFakeProvisioningAPI(ProvisioningAPITests, TestCase):
"""Test :class:`FakeAsynchronousProvisioningAPI`.
@@ -631,6 +715,22 @@
"""Return a real ProvisioningAPI, but using a fake Cobbler session."""
return ProvisioningAPI(make_fake_cobbler_session())
+ def test_sync(self):
+ """`ProvisioningAPI.sync` issues an authenticated ``sync`` call.
+
+ It is not exported - i.e. it is not part of :class:`IProvisioningAPI`
+ - but is used heavily by other methods in `IProvisioningAPI`.
+ """
+ papi = self.get_provisioning_api()
+ calls = []
+ self.patch(
+ papi.session, "call",
+ lambda *args: calls.append(args))
+ papi.sync()
+ self.assertEqual(
+ [("sync", papi.session.token_placeholder)],
+ calls)
+
class TestProvisioningAPIWithRealCobbler(ProvisioningAPITests,
ProvisioningAPITestsWithCobbler,
@@ -640,12 +740,34 @@
The URL for the Cobbler instance must be provided in the
`PSERV_TEST_COBBLER_URL` environment variable.
- Includes by inheritance all the tests in :class:`ProvisioningAPITests`.
+ Includes by inheritance all the tests in :class:`ProvisioningAPITests` and
+ :class:`ProvisioningAPITestsWithCobbler`.
"""
real_cobbler = RealCobbler()
- @skipIf(not real_cobbler.is_available(), RealCobbler.help_text)
+ @real_cobbler.skip_unless_available
def get_provisioning_api(self):
"""Return a connected :class:`ProvisioningAPI`."""
return ProvisioningAPI(self.real_cobbler.get_session())
+
+ @real_cobbler.skip_unless_local
+ @inlineCallbacks
+ def test_sync_after_modify(self):
+ # When MAAS modifies the MAC addresses of a node it triggers a sync of
+ # Cobbler. This is to ensure that netboot files are up-to-date, or
+ # removed as appropriate.
+ papi = self.get_provisioning_api()
+ node_name = yield self.add_node(papi)
+ mac_address = factory.getRandomMACAddress()
+ yield papi.modify_nodes(
+ {node_name: {"mac_addresses": [mac_address]}})
+ # The PXE file corresponding to the node's MAC address is present.
+ pxe_filename = "/var/lib/tftpboot/pxelinux.cfg/01-%s" % (
+ mac_address.replace(":", "-"),)
+ self.assertThat(pxe_filename, FileExists())
+ # Remove the MAC address again.
+ yield papi.modify_nodes(
+ {node_name: {"mac_addresses": []}})
+ # The PXE file has been removed too.
+ self.assertThat(pxe_filename, Not(FileExists()))
=== modified file 'src/provisioningserver/tests/test_cobblercatcher.py'
--- src/provisioningserver/tests/test_cobblercatcher.py 2012-03-20 02:45:17 +0000
+++ src/provisioningserver/tests/test_cobblercatcher.py 2012-04-16 14:53:14 +0000
@@ -180,7 +180,7 @@
real_cobbler = RealCobbler()
- @skipIf(not real_cobbler.is_available(), RealCobbler.help_text)
+ @real_cobbler.skip_unless_available
@deferred
def get_cobbler_session(self):
return self.real_cobbler.get_session()
