launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06226
[Merge] lp:~jtv/maas/realistic-cobbler into lp:maas
Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/realistic-cobbler into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~jtv/maas/realistic-cobbler/+merge/91242
Some updates to the cobbler client and the fake cobbler, in response to testing with the real cobbler:
* Twisted fails to encode its service URL, leading to a "Data must not be unicode" error.
* Twisted fails to encode method names, leading to the same error. Bastards. I hid the encoding in _issue_call.
* By the way, it seems there's no need to append "/RPC2" to the service URL like the Twisted docs seem to suggest.
* Cobbler object creation requires certain attributes per type, and will fail otherwise. Not by raising an exception; that would be too easy. It returns an undocumented False through several layers of call stack, and does not create the object. Bastards.
* The CobblerClient now checks for missing attributes, and a new test helper fake_cobbler_object creates an object with any missing required attributes filled out.
* We had a stateless base class with some test helpers; I extracted its methods to functions and deleted the class.
Jeroen
--
https://code.launchpad.net/~jtv/maas/realistic-cobbler/+merge/91242
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/realistic-cobbler into lp:maas.
=== modified file 'src/provisioningserver/cobblerclient.py'
--- src/provisioningserver/cobblerclient.py 2012-01-31 16:09:25 +0000
+++ src/provisioningserver/cobblerclient.py 2012-02-02 11:35:48 +0000
@@ -69,8 +69,12 @@
For internal use only, but overridable for test purposes.
"""
-# TODO: Is the /RPC2 needed?
- return Proxy(self.url + '/RPC2')
+ # Twisted does not encode the URL, and breaks with "Data must
+ # not be unicode" if it's in Unicode. We'll have to decode it
+ # here, and hope it doesn't lead to breakage in Twisted. We'll
+ # figure out what to do about non-ASCII characters in URLs
+ # later.
+ return Proxy(self.url.encode('ascii'))
def record_state(self):
"""Return a cookie representing the session's current state.
@@ -118,7 +122,7 @@
self.token = None
# Now initiate our new authentication.
- self.token = yield self.proxy.callRemote(
+ self.token = yield self._issue_call(
'login', self.user, self.password)
finally:
self.authentication_lock.release()
@@ -157,6 +161,10 @@
substituted in its place.
:return: `Deferred`.
"""
+ # Twisted XMLRPC does not encode the method name, but breaks if
+ # we give it in Unicode. Encode it here; thankfully we're
+ # dealing with ASCII only in method names.
+ method = method.encode('ascii')
args = map(self._substitute_token, args)
d = self._with_timeout(self.proxy.callRemote(method, *args))
return d
@@ -226,6 +234,9 @@
# underscores. In MaaS, use only underscores.
known_attributes = []
+ # What attributes does Cobbler require for this type of object?
+ required_attributes = []
+
def __init__(self, session, name=None, values=None):
"""Reference an object in Cobbler.
@@ -319,6 +330,12 @@
:param name: Identifying name for the new object.
:param attributes: Dict mapping attribute names to values.
"""
+ missing_attributes = (
+ set(cls.required_attributes) - set(attributes.keys()))
+ assert len(missing_attributes) == 0, (
+ "Required attributes for %s missing: %s"
+ % (cls.object_type, missing_attributes))
+
args = dict(
(cls._normalize_attribute(key), value)
for key, value in attributes.items())
@@ -371,6 +388,10 @@
'virt_path',
'virt_ram',
]
+ required_attributes = [
+ 'name',
+ 'distro',
+ ]
class CobblerImage(CobblerObject):
@@ -416,6 +437,11 @@
'owners',
'template-files',
]
+ required_attributes = [
+ 'initrd',
+ 'kernel',
+ 'name',
+ ]
class CobblerRepo(CobblerObject):
@@ -433,6 +459,10 @@
'owners',
'priority',
]
+ required_attributes = [
+ 'name',
+ 'mirror',
+ ]
class CobblerSystem(CobblerObject):
@@ -474,6 +504,10 @@
'virt_path',
'virt_type',
]
+ required_attribute = [
+ 'name',
+ 'profile',
+ ]
# The modify_interface dict can contain:
# * "macaddress-eth0" etc.
=== modified file 'src/provisioningserver/tests/test_cobblersession.py'
--- src/provisioningserver/tests/test_cobblersession.py 2012-02-01 16:03:41 +0000
+++ src/provisioningserver/tests/test_cobblersession.py 2012-02-02 11:35:48 +0000
@@ -124,50 +124,48 @@
return self.fake_proxy
-class TestCobblerSessionBase:
- """Base class helpers for testing `CobblerSession`."""
-
- def make_url_user_password(self):
- """Produce arbitrary API URL, username, and password."""
- return (
- 'http://api.example.com/%d' % pick_number(),
- 'username%d' % pick_number(),
- 'password%d' % pick_number(),
- )
-
- def make_recording_session(self, session_args=None, token=None,
- fake_proxy=RecordingFakeProxy):
- """Create a `RecordingSession`."""
- if session_args is None:
- session_args = self.make_url_user_password()
- if token is None:
- token = fake_token()
- return RecordingSession(
- *session_args, fake_token=token, fake_proxy=fake_proxy)
-
-
-class TestCobblerSession(TestCase, TestCobblerSessionBase):
+def make_url_user_password():
+ """Produce arbitrary API URL, username, and password."""
+ return (
+ 'http://api.example.com/%d' % pick_number(),
+ 'username%d' % pick_number(),
+ 'password%d' % pick_number(),
+ )
+
+
+def make_recording_session(session_args=None, token=None,
+ fake_proxy=RecordingFakeProxy):
+ """Create a `RecordingSession`."""
+ if session_args is None:
+ session_args = make_url_user_password()
+ if token is None:
+ token = fake_token()
+ return RecordingSession(
+ *session_args, fake_token=token, fake_proxy=fake_proxy)
+
+
+class TestCobblerSession(TestCase):
"""Test session management against a fake XMLRPC session."""
run_tests_with = AsynchronousDeferredRunTest
def test_initializes_but_does_not_authenticate_on_creation(self):
- url, user, password = self.make_url_user_password()
- session = self.make_recording_session(
+ url, user, password = make_url_user_password()
+ session = make_recording_session(
token=fake_token(user, 'not-yet-authenticated'))
self.assertEqual(None, session.token)
@inlineCallbacks
def test_authenticate_authenticates_initially(self):
token = fake_token('authenticated')
- session = self.make_recording_session(token=token)
+ session = make_recording_session(token=token)
self.assertEqual(None, session.token)
yield session._authenticate()
self.assertEqual(token, session.token)
@inlineCallbacks
def test_state_cookie_stays_constant_during_normal_use(self):
- session = self.make_recording_session()
+ session = make_recording_session()
state = session.record_state()
self.assertEqual(state, session.record_state())
yield session.call("some_method")
@@ -175,14 +173,14 @@
@inlineCallbacks
def test_authentication_changes_state_cookie(self):
- session = self.make_recording_session()
+ session = make_recording_session()
old_cookie = session.record_state()
yield session._authenticate()
self.assertNotEqual(old_cookie, session.record_state())
@inlineCallbacks
def test_authenticate_backs_off_from_overwriting_concurrent_auth(self):
- session = self.make_recording_session()
+ session = make_recording_session()
# Two requests are made concurrently.
cookie_before_request_1 = session.record_state()
cookie_before_request_2 = session.record_state()
@@ -207,8 +205,7 @@
@inlineCallbacks
def test_substitute_token_substitutes_only_placeholder(self):
- token = fake_token('for-substitution')
- session = self.make_recording_session(token=token)
+ session = make_recording_session(token=fake_token('for-subst'))
yield session._authenticate()
arbitrary_number = pick_number()
arbitrary_string = 'string-%d' % pick_number()
@@ -220,7 +217,7 @@
]
outputs = [
arbitrary_number,
- token,
+ session.token,
arbitrary_string,
None,
]
@@ -229,7 +226,7 @@
@inlineCallbacks
def test_call_calls_xmlrpc(self):
- session = self.make_recording_session()
+ session = make_recording_session()
return_value = 'returnval-%d' % pick_number()
method = 'method_%d' % pick_number()
arg = 'arg-%d' % pick_number()
@@ -242,7 +239,7 @@
def test_call_reauthenticates_and_retries_on_auth_failure(self):
# If a call triggers an authentication error, call()
# re-authenticates and then re-issues the call.
- session = self.make_recording_session()
+ session = make_recording_session()
yield session._authenticate()
successful_return_value = pick_number()
session.proxy.set_return_values([
@@ -270,7 +267,7 @@
# is called just once, with the state cookie from before the
# call. This ensures that it will always notice a concurrent
# re-authentication that it needs to back off from.
- session = self.make_recording_session()
+ session = make_recording_session()
yield session._authenticate()
authenticate_cookies = []
@@ -286,7 +283,7 @@
@inlineCallbacks
def test_call_raises_repeated_auth_failure(self):
- session = self.make_recording_session()
+ session = make_recording_session()
yield session._authenticate()
failures = [
# Initial operation fails: not authenticated.
@@ -302,7 +299,7 @@
@inlineCallbacks
def test_call_raises_general_failure(self):
- session = self.make_recording_session()
+ session = make_recording_session()
yield session._authenticate()
failure = Exception("Memory error. Where did I put it?")
session.proxy.set_return_values([failure])
@@ -315,7 +312,7 @@
# If there is no auth token, and authentication is required,
# call() authenticates right away rather than waiting for the
# first call attempt to fail.
- session = self.make_recording_session()
+ session = make_recording_session()
session.token = None
session.proxy.set_return_values([pick_number()])
yield session.call(
@@ -326,8 +323,7 @@
[('authenticate_me_first', session.token)], session.proxy.calls)
-class TestConnectionTimeouts(TestCase, TestCobblerSessionBase,
- fixtures.TestWithFixtures):
+class TestConnectionTimeouts(TestCase, fixtures.TestWithFixtures):
"""Tests for connection timeouts on `CobblerSession`."""
run_tests_with = AsynchronousDeferredRunTestForBrokenTwisted
@@ -336,7 +332,7 @@
# Winding a clock reactor past the timeout value should cancel
# the original Deferred.
clock = Clock()
- session = self.make_recording_session()
+ session = make_recording_session()
d = session._with_timeout(defer.Deferred(), 1, clock)
clock.advance(2)
return assert_fails_with(d, defer.CancelledError)
@@ -346,7 +342,7 @@
# (defer.succeed() is pre-fired) Deferred should not trigger a
# cancellation.
clock = Clock()
- session = self.make_recording_session()
+ session = make_recording_session()
d = session._with_timeout(defer.succeed("frobnicle"), 1, clock)
clock.advance(2)
@@ -360,7 +356,7 @@
# Winding a clock reactor forwards but not past the timeout
# should result in no cancellation.
clock = Clock()
- session = self.make_recording_session()
+ session = make_recording_session()
d = session._with_timeout(defer.Deferred(), 5, clock)
clock.advance(1)
self.assertFalse(d.called)
@@ -371,7 +367,7 @@
"provisioningserver.cobblerclient.default_reactor", clock)
self.useFixture(patch)
- session = self.make_recording_session(fake_proxy=DeadProxy)
+ session = make_recording_session(fake_proxy=DeadProxy)
d = session._issue_call("login", "foo")
clock.advance(cobblerclient.DEFAULT_TIMEOUT + 1)
return assert_fails_with(d, defer.CancelledError)
@@ -389,3 +385,11 @@
self.assertEqual(
'x_systems_y',
cobblerclient.CobblerSystem._name_method('x_%s_y', plural=True))
+
+ def test_new_checks_required_attributes(self):
+ # CobblerObject.new asserts that all required attributes for a
+ # type of object are provided.
+ session = make_recording_session()
+ with ExpectedException(AssertionError):
+ yield cobblerclient.CobblerSystem.new(
+ session, 'incomplete_system', {})
=== modified file 'src/provisioningserver/tests/test_fakecobbler.py'
--- src/provisioningserver/tests/test_fakecobbler.py 2012-02-01 13:02:19 +0000
+++ src/provisioningserver/tests/test_fakecobbler.py 2012-02-02 11:35:48 +0000
@@ -74,6 +74,29 @@
returnValue(session)
+def fake_cobbler_object(session, object_class, name=None, attributes=None):
+ """Create a fake Cobbler object.
+
+ :param session: `CobblerSession`.
+ :param object_class: concrete `CobblerObject` class to instantiate.
+ :param name: Option name for the object.
+ :param attributes: Optional dict of attribute values for the object.
+ """
+ if attributes is None:
+ attributes = {}
+ else:
+ attributes = dict(attributes)
+ unique_int = next(unique_ints)
+ if name is None:
+ name = 'name-%s-%d' % (object_class.object_type, unique_int)
+ attributes['name'] = name
+ for attr in object_class.required_attributes:
+ if attr not in attributes:
+ attributes[attr] = '%s-%d' % (attr, unique_int)
+ d = object_class.new(session, name, attributes)
+ return d
+
+
class TestFakeCobbler(TestCase):
"""Test `FakeCobbler`.
@@ -100,7 +123,7 @@
# Use of the token will now fail with an "invalid token"
# error. The Cobbler client notices this, re-authenticates, and
# re-runs the method.
- yield CobblerSystem.new(session, 'my-session', {'comment': 'Mine'})
+ yield fake_cobbler_object(session, CobblerSystem)
# The re-authentication results in a fresh token.
self.assertNotEqual(old_token, session.token)
@@ -109,7 +132,7 @@
def test_valid_token_does_not_raise_auth_error(self):
session = yield fake_cobbler_session()
old_token = session.token
- yield CobblerSystem.new(session, 'some-session', {'comment': 'Boo'})
+ yield fake_cobbler_object(session, CobblerSystem)
self.assertEqual(old_token, session.token)
@@ -141,7 +164,7 @@
def test_create_object(self):
session = yield fake_cobbler_session()
name = self.make_name()
- obj = yield self.cobbler_class.new(session, name, {})
+ obj = yield fake_cobbler_object(session, self.cobbler_class, name)
self.assertEqual(name, obj.name)
@inlineCallbacks
@@ -155,7 +178,7 @@
def test_find_matches_name(self):
session = yield fake_cobbler_session()
name = self.make_name()
- yield self.cobbler_class.new(session, name, {})
+ yield fake_cobbler_object(session, self.cobbler_class, name)
[by_comment] = yield self.cobbler_class.find(session, name=name)
self.assertEqual(name, by_comment.name)
@@ -163,7 +186,8 @@
def test_find_matches_attribute(self):
session = yield fake_cobbler_session()
name = self.make_name()
- yield self.cobbler_class.new(session, name, {'comment': 'Hi'})
+ yield fake_cobbler_object(
+ session, self.cobbler_class, name, {'comment': 'Hi'})
[by_comment] = yield self.cobbler_class.find(session, comment='Hi')
self.assertEqual(name, by_comment.name)
@@ -171,7 +195,7 @@
def test_find_without_args_finds_everything(self):
session = yield fake_cobbler_session()
name = self.make_name()
- yield self.cobbler_class.new(session, name, {'comment': 'Hi'})
+ yield fake_cobbler_object(session, self.cobbler_class, name)
found_items = yield self.cobbler_class.find(session)
self.assertEqual([name], [item.name for item in found_items])
@@ -179,24 +203,23 @@
def test_get_handle_finds_handle(self):
session = yield fake_cobbler_session()
name = self.make_name()
- obj = yield self.cobbler_class.new(session, name, {})
+ obj = yield fake_cobbler_object(session, self.cobbler_class, name)
handle = yield obj._get_handle()
self.assertNotEqual(None, handle)
@inlineCallbacks
def test_get_handle_distinguishes_objects(self):
session = yield fake_cobbler_session()
- obj1 = yield self.cobbler_class.new(session, self.make_name(), {})
+ obj1 = yield fake_cobbler_object(session, self.cobbler_class)
handle1 = yield obj1._get_handle()
- obj2 = yield self.cobbler_class.new(session, self.make_name(), {})
+ obj2 = yield fake_cobbler_object(session, self.cobbler_class)
handle2 = yield obj2._get_handle()
self.assertNotEqual(handle1, handle2)
@inlineCallbacks
def test_get_handle_is_consistent(self):
session = yield fake_cobbler_session()
- name = self.make_name()
- obj = yield self.cobbler_class.new(session, name, {})
+ obj = yield fake_cobbler_object(session, self.cobbler_class)
handle1 = yield obj._get_handle()
handle2 = yield obj._get_handle()
self.assertEqual(handle1, handle2)
@@ -205,7 +228,7 @@
def test_delete_removes_object(self):
session = yield fake_cobbler_session()
name = self.make_name()
- obj = yield self.cobbler_class.new(session, name, {'name': name})
+ obj = yield fake_cobbler_object(session, self.cobbler_class, name)
obj.delete()
matches = yield self.cobbler_class.find(session, name=name)
self.assertEqual([], matches)
@@ -295,7 +318,8 @@
names = [self.make_name() for counter in range(num_systems)]
systems = []
for name in names:
- new_system = yield self.cobbler_class.new(session, name, {})
+ new_system = yield fake_cobbler_object(
+ session, self.cobbler_class, name)
systems.append(new_system)
returnValue((names, systems))
