launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06647
[Merge] lp:~allenap/maas/cobbler-edit-before-add into lp:maas
Gavin Panella has proposed merging lp:~allenap/maas/cobbler-edit-before-add into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~allenap/maas/cobbler-edit-before-add/+merge/97038
--
https://code.launchpad.net/~allenap/maas/cobbler-edit-before-add/+merge/97038
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/cobbler-edit-before-add into lp:maas.
=== modified file 'Makefile'
--- Makefile 2012-03-12 13:50:40 +0000
+++ Makefile 2012-03-12 15:09:00 +0000
@@ -44,8 +44,8 @@
utilities/maasdb start ./db/ disposable
test: bin/test.maas bin/test.pserv
- bin/test.maas
- bin/test.pserv
+ bin/test.maas -v
+ bin/test.pserv -v
lint: sources = setup.py src templates utilities
lint: bin/flake8
=== modified file 'src/provisioningserver/cobblerclient.py'
--- src/provisioningserver/cobblerclient.py 2012-03-09 05:53:22 +0000
+++ src/provisioningserver/cobblerclient.py 2012-03-12 15:09:00 +0000
@@ -96,6 +96,15 @@
return "invalid token: " in exception.faultString
+def looks_like_object_not_found(exception):
+ """Does `exception` look like an unknown object failure?"""
+ if not hasattr(exception, 'faultString'):
+ # An unknown object failure would come as an xmlrpclib.Fault.
+ return False
+ else:
+ return "internal error, unknown " in exception.faultString
+
+
class CobblerSession:
"""A session on the Cobbler XMLRPC API.
@@ -486,14 +495,24 @@
(cls._normalize_attribute(key), value)
for key, value in attributes.items())
- # Overwrite any existing object of the same name. Unfortunately
- # this parameter goes into the "attributes," and seems to be
- # stored along with them. Its value doesn't matter.
- args.setdefault('clobber', True)
-
- success = yield session.call(
- 'xapi_object_edit', cls.object_type, name, 'add', args,
- session.token_placeholder)
+ # Do not clobber under any circumstances.
+ if "clobber" in args:
+ del args["clobber"]
+
+ try:
+ # Attempt to edit an existing object.
+ success = yield session.call(
+ 'xapi_object_edit', cls.object_type, name, 'edit', args,
+ session.token_placeholder)
+ except xmlrpclib.Fault as e:
+ # If it was not found, add the object.
+ if looks_like_object_not_found(e):
+ success = yield session.call(
+ 'xapi_object_edit', cls.object_type, name, 'add', args,
+ session.token_placeholder)
+ else:
+ raise
+
if not success:
raise RuntimeError(
"Cobbler refused to create %s '%s'. Attributes: %s"
=== modified file 'src/provisioningserver/testing/fakecobbler.py'
--- src/provisioningserver/testing/fakecobbler.py 2012-03-09 05:56:28 +0000
+++ src/provisioningserver/testing/fakecobbler.py 2012-03-12 15:09:00 +0000
@@ -11,9 +11,10 @@
__metaclass__ = type
__all__ = [
'fake_auth_failure_string',
+ 'fake_object_not_found_string',
+ 'fake_token',
'FakeCobbler',
'FakeTwistedProxy',
- 'fake_token',
'make_fake_cobbler_session',
]
@@ -41,6 +42,13 @@
return "<class 'cobbler.exceptions.CX'>:'invalid token: %s'" % token
+def fake_object_not_found_string(object_type, object_name):
+ """Fake a Cobbler unknown object fault string."""
+ return (
+ "<class 'cobbler.cexceptions.CX'>:'internal error, "
+ "unknown %s name %s'" % (object_type, object_name))
+
+
def fake_token(user=None, custom_id=None):
"""Make up a fake auth token.
@@ -211,7 +219,7 @@
if handle is None:
handle = self._get_handle_if_present(None, object_type, name)
if handle is None:
- raise Fault(1, "Unknown %s: %s." % (object_type, name))
+ raise Fault(1, fake_object_not_found_string(object_type, name))
return handle
def _api_find_objects(self, object_type, criteria):
=== modified file 'src/provisioningserver/tests/test_api.py'
--- src/provisioningserver/tests/test_api.py 2012-03-09 11:57:46 +0000
+++ src/provisioningserver/tests/test_api.py 2012-03-12 15:09:00 +0000
@@ -266,17 +266,20 @@
return d
@inlineCallbacks
- def add_distro(self, papi):
+ def add_distro(self, papi, name=None):
"""Creates a new distro object via `papi`.
- Arranges for it to be deleted during test clean-up.
+ Arranges for it to be deleted during test clean-up. If `name` is not
+ specified, `fake_name` will be called to obtain one.
"""
+ if name is None:
+ name = fake_name()
# For the initrd and kernel, use a file that we know will exist for a
# running Cobbler instance (at least, on Ubuntu) so that we can test
# against remote instances, like one in odev.
initrd = "/etc/cobbler/settings"
kernel = "/etc/cobbler/version"
- distro_name = yield papi.add_distro(fake_name(), initrd, kernel)
+ distro_name = yield papi.add_distro(name, initrd, kernel)
self.addCleanup(
self.cleanup_objects,
papi.delete_distros_by_name,
@@ -284,15 +287,18 @@
returnValue(distro_name)
@inlineCallbacks
- def add_profile(self, papi, distro_name=None):
+ def add_profile(self, papi, name=None, distro_name=None):
"""Creates a new profile object via `papi`.
- Arranges for it to be deleted during test clean-up. If `distro_name`
+ Arranges for it to be deleted during test clean-up. If `name` is not
+ specified, `fake_name` will be called to obtain one. If `distro_name`
is not specified, one will be obtained by calling `add_distro`.
"""
+ if name is None:
+ name = fake_name()
if distro_name is None:
distro_name = yield self.add_distro(papi)
- profile_name = yield papi.add_profile(fake_name(), distro_name)
+ profile_name = yield papi.add_profile(name, distro_name)
self.addCleanup(
self.cleanup_objects,
papi.delete_profiles_by_name,
@@ -300,20 +306,23 @@
returnValue(profile_name)
@inlineCallbacks
- def add_node(self, papi, profile_name=None, metadata=None):
+ def add_node(self, papi, name=None, profile_name=None, metadata=None):
"""Creates a new node object via `papi`.
- Arranges for it to be deleted during test clean-up. If `profile_name`
+ Arranges for it to be deleted during test clean-up. If `name` is not
+ specified, `fake_name` will be called to obtain one. If `profile_name`
is not specified, one will be obtained by calling `add_profile`. If
`metadata` is not specified, it will be obtained by calling
`fake_node_metadata`.
"""
+ if name is None:
+ name = fake_name()
if profile_name is None:
profile_name = yield self.add_profile(papi)
if metadata is None:
metadata = fake_node_metadata()
node_name = yield papi.add_node(
- fake_name(), profile_name, metadata)
+ name, profile_name, metadata)
self.addCleanup(
self.cleanup_objects,
papi.delete_nodes_by_name,
@@ -349,6 +358,23 @@
self.assertItemsEqual([node_name], nodes)
@inlineCallbacks
+ def _test_add_object_twice(self, method):
+ # Adding an object twice is allowed.
+ papi = self.get_provisioning_api()
+ object_name1 = yield method(papi)
+ object_name2 = yield method(papi, object_name1)
+ self.assertEqual(object_name1, object_name2)
+
+ def test_add_distro_twice(self):
+ return self._test_add_object_twice(self.add_distro)
+
+ def test_add_profile_twice(self):
+ return self._test_add_object_twice(self.add_profile)
+
+ def test_add_node_twice(self):
+ return self._test_add_object_twice(self.add_node)
+
+ @inlineCallbacks
def test_modify_distros(self):
papi = self.get_provisioning_api()
distro_name = yield self.add_distro(papi)
@@ -371,7 +397,7 @@
papi = self.get_provisioning_api()
distro1_name = yield self.add_distro(papi)
distro2_name = yield self.add_distro(papi)
- profile_name = yield self.add_profile(papi, distro1_name)
+ profile_name = yield self.add_profile(papi, None, distro1_name)
yield papi.modify_profiles({profile_name: {"distro": distro2_name}})
values = yield papi.get_profiles_by_name([profile_name])
self.assertEqual(distro2_name, values[profile_name]["distro"])
@@ -380,9 +406,9 @@
def test_modify_nodes(self):
papi = self.get_provisioning_api()
distro_name = yield self.add_distro(papi)
- profile1_name = yield self.add_profile(papi, distro_name)
- profile2_name = yield self.add_profile(papi, distro_name)
- node_name = yield self.add_node(papi, profile1_name)
+ profile1_name = yield self.add_profile(papi, None, distro_name)
+ profile2_name = yield self.add_profile(papi, None, distro_name)
+ node_name = yield self.add_node(papi, None, profile1_name)
yield papi.modify_nodes({node_name: {"profile": profile2_name}})
values = yield papi.get_nodes_by_name([node_name])
self.assertEqual(profile2_name, values[node_name]["profile"])
@@ -468,7 +494,7 @@
# Create some profiles via the Provisioning API.
profiles_expected = set()
for num in range(3):
- profile_name = yield self.add_profile(papi, distro_name)
+ profile_name = yield self.add_profile(papi, None, distro_name)
profiles_expected.add(profile_name)
profiles_after = yield papi.get_profiles()
profiles_created = set(profiles_after) - set(profiles_before)
@@ -480,7 +506,7 @@
profile_name = yield self.add_profile(papi)
node_names = set()
for num in range(3):
- node_name = yield self.add_node(papi, profile_name)
+ node_name = yield self.add_node(papi, None, profile_name)
node_names.add(node_name)
nodes = yield papi.get_nodes()
self.assertSetEqual(node_names, node_names.intersection(nodes))
=== modified file 'src/provisioningserver/tests/test_cobblersession.py'
--- src/provisioningserver/tests/test_cobblersession.py 2012-03-09 06:17:46 +0000
+++ src/provisioningserver/tests/test_cobblersession.py 2012-03-12 15:09:00 +0000
@@ -18,6 +18,7 @@
from provisioningserver import cobblerclient
from provisioningserver.testing.fakecobbler import (
fake_auth_failure_string,
+ fake_object_not_found_string,
fake_token,
)
from testtools.content import text_content
@@ -254,6 +255,20 @@
self.assertTrue(
cobblerclient.looks_like_auth_expiry(make_auth_failure()))
+ def test_looks_like_object_not_found_for_regular_exception(self):
+ self.assertFalse(
+ cobblerclient.looks_like_object_not_found(RuntimeError("Error")))
+
+ def test_looks_like_object_not_found_for_other_Fault(self):
+ self.assertFalse(
+ cobblerclient.looks_like_object_not_found(
+ Fault(1, "Missing sprocket")))
+
+ def test_looks_like_object_not_found_recognizes_object_not_found(self):
+ error = Fault(1, fake_object_not_found_string("distro", "bob"))
+ self.assertTrue(
+ cobblerclient.looks_like_object_not_found(error))
+
@inlineCallbacks
def test_call_reauthenticates_and_retries_on_auth_failure(self):
# If a call triggers an authentication error, call()
@@ -416,6 +431,29 @@
session, 'incomplete_system', {})
@inlineCallbacks
+ def test_new_attempts_edit_before_creating_new(self):
+ # CobblerObject.new always attempts an extended edit operation on the
+ # given object first, following by an add should the object not yet
+ # exist.
+ session = make_recording_session()
+ not_found_string = fake_object_not_found_string("system", "carcass")
+ not_found = Fault(1, not_found_string)
+ session.proxy.set_return_values([not_found, True])
+ yield cobblerclient.CobblerSystem.new(
+ session, "carcass", {"profile": "heartwork"})
+ expected_calls = [
+ # First an edit is attempted...
+ ("xapi_object_edit", "system", "carcass", "edit",
+ {"name": "carcass", "profile": "heartwork"},
+ session.token),
+ # Followed by an add.
+ ("xapi_object_edit", "system", "carcass", "add",
+ {"name": "carcass", "profile": "heartwork"},
+ session.token),
+ ]
+ self.assertEqual(expected_calls, session.proxy.calls)
+
+ @inlineCallbacks
def test_modify(self):
session = make_recording_session()
session.proxy.set_return_values([True])
@@ -447,7 +485,6 @@
# Fake that Cobbler holds the following attributes about the distro
# just created.
values_stored = {
- "clobber": True,
"initrd": "an_initrd",
"kernel": "a_kernel",
"likes": "cabbage",
@@ -472,8 +509,7 @@
# Fake that Cobbler holds the following attributes about the distros
# just created.
values_stored = [
- {"clobber": True,
- "initrd": "an_initrd",
+ {"initrd": "an_initrd",
"kernel": "a_kernel",
"likes": "cabbage",
"name": "alice"},
