Merge lp:~fwereade/pyjuju/cobbler-connect-production into lp:pyjuju

[William Reade]

* added schema for orchestra provider
* added CobblerConnect for communicating with cobbler
* added OrchestraLaunchMachine, which can now actually launch a machine(which won't do anything)

[lp:~fwereade/pyjuju/cobbler-connect-production]

315. By William Reade on 2011-07-27

merge parent

[Gustavo Niemeyer (niemeyer)]

Looks nice. Just a few minors, and +1.

[1]

+ @property
+ def _acquired_class(self):
+ return self._config["acquired-mgmt-class"]
+
+ @property
+ def _available_class(self):
+ return self._config["available-mgmt-class"]
+
+ @property
+ def _credentials(self):
+ return (self._config["orchestra-user"],
+ self._config["orchestra-pass"])

Not a big deal, but just as some feedback, this feels like
adding more complexity than necessary for the problem at
hand. Compare with this version:

 self._acquired_class = self._config["acquired-mgmt-class"]
 self._available_class = self._config["available-mgmt-class"]
 self._user = self._config["orchestra-user"]
 self._pass = self._config["orchestra-pass"]

[2]

+ if "invalid token" not in failure.getErrorMessage():
+ return failure
+ login = self._login()
+ login.addCallback(lambda unused: call())

Looks nice.

[3]

+ failure.trap(Fault)
+ if "invalid token" not in failure.getErrorMessage():
+ return failure

None of that logic is covered by tests, though.

[4]

+ def check(actual):
+ if actual != expect:
+ raise ProviderError(
+ "Bad result from call to %s with %s (got %r, expected %r)"
+ % (name, args, actual, expect))
+ return actual

Also untouched by tests.

[5]

+ self.mock_proxy()
+ self.mock_get_systems()
+ self.mock_acquire_system()
+ self.mock_set_ks_meta()
+ self.mock_start_system()

I guess there's no better way to test that interaction with the external system,
but I have to say I'm a bit sad about the amount of mocking here.

review: Approve

[William Reade (fwereade)]

Quick responses:

[1] I guess something in my past has caused me to become fanatical about having a single point of truth for every possible piece of data in the system. What if someone changes config underneath me!? (Yeah, I'll change it.)

[2] Cheers :-).

[3] Yes it is: test_bootstrap.py, lines 43-48. I'm open to the argument that testing a single call that happens to follow this path is a little weak, but I'm reluctant to add further complexity to the already excessive mocking here. See below...

[4] Thanks, nice catch. I have a comment I'll come back to in the light of...

[5] It'll only get worse in future branches :-(. On the bright side, I think this is a good opportunity for a philosophical ramble :-p.

I'd categorise your favoured testing style as "integration" rather than "unit", which I've tended to neglect in the past, but I'm reevaluating that: factoring the generic code out of providers.ec2 was *much* easier than I expected, largely thanks to this testing style.

My own approach has historically tended to favour ludicrously tight unit testing, in which *everything* that isn't part of the SUT is mocked out; the obvious drawback is that the burden of significant refactorings can be much heavier (and it's very possible to miss tests which expect another object to have an outdated interface), but I've generally had good experiences when my unit tests are backed up by relatively few high-level functional tests: 9 times out of 10, I've found, an end-to-end test of the happy path will flush out the inconsistencies.

In the end, I think, it's a choice between testing (1) the ultimate external effects of an interaction with the SUT or (2) the precise interactions with the SUT's dependencies; which path is better in the long term depends on the interactions' relative susceptibility to change. I do feel that only (2) really qualifies as "unit" testing, but that's frankly irrelevant: reliable coverage matters a lot more than the names we happen to use for the techniques we employ.

I'm not advocating a change of approach here, but I would say that we're we're pretty close to the line: the current strategy means that a tweak to CobblerConnect's interaction with the server will lead to failures in an arbitrary number of tests, none of which are related to the cause of the failure (except coincidentally). We'll have to see how it shakes out here; I think the fact that the cobbler API is much chattier than EC2's may eventually justify a different approach locally, but it hasn't hurt me enough to seriously complain. Yet :-).

[3/4 redux] In [3], I'm comfortable testing only one interaction with the "invalid token" code, because I "know" everything goes through ._call(). In [4], there are 3 distinct operations that can hit ._check_call(), and under the current approach I can't see any option but to test both happy and sad paths each one individually... but this makes me uncomfortable, because a minor tweak to that code would entail fixing at least 6 tests. I'll fix it anyway; I just had an urge to register my misgivings ;-).

Er, I really did intend these to be quick responses. Thanks for the thorough review!

[William Reade (fwereade)]

[4] Further thought: since all uses of _check_call() are actually in _set_on_system, I intend to write a relatively small number of tests that exercise that code, giving us a similar imperfect-but-wieldy level of testing to that in [3]. Tolerable?

[lp:~fwereade/pyjuju/cobbler-connect-production]

316. By William Reade on 2011-07-30

whitespace

317. By William Reade on 2011-07-30

merge from trunk; also changes per niemeyer's review

[Gustavo Niemeyer (niemeyer)]

[3]

Try taking the commented lines out and run test_bootstrap.

[5]

Not entirely. It is possible to do good unit testing without mocking, but I agree
that in some cases good unit testing moves toward the integration side of things,
and that sounds quite alright in most cases.

The test should be as unity as possible before it starts to get fake and invasive.
Mocking is silly in the sense that it forces you to repeat the same logic twice
(e.g. call foo() in the test, and call foo() in the code), giving a false sense
of confidence in the logic. The outcome is something that does not guarantee
correctness, because you were worried about "how-to" when you wrote the test rather
than the outcome, and that very fact binds the implementation to the test in a
tight way that makes it hard to refactor, as you noticed by counter example.

[William Reade (fwereade)]

[3] Bah. Thank you, nice catch; on it.

[5] I think we agree on the problem, at least ;).

[lp:~fwereade/pyjuju/cobbler-connect-production]

318. By William Reade on 2011-08-01

*properly* fixed testing per niemeyer's review

[Kapil Thangavelu (hazmat)]

I can't speak at all to what this means in terms of cobbler/orchestra interaction but code style/syntax looks good.

[0] This bit appears unused and extraneous.

ensemble/lib/testing.py

74 + client_got = False
75 +

[1] Just a naming thing, but i'd change CobblerConnect to CobblerConnection

[2] random thoughts.

For launching machines is there anyway to distinguish size/hardware capabilities? I'm just thinking that in the future, we'll want to have some abstracted notion of machine size so we can utilize them for deploying services that need additional hardware capabilities available to a machine provider.

[3] Some pep8/syntax items that came up captured in diff to fix.
https://pastebin.canonical.com/50619/

[4] I'm curious what the ensemble late command stuff is.. it might be nice to have some documentation of the orchestration integration when all of this said and done.

[Kapil Thangavelu (hazmat)]

review: Approve

[William Reade (fwereade)]

> [1] Just a naming thing, but i'd change CobblerConnect to CobblerConnection

Hm, it's not really a *connection* as such, but I agree it's not a very helpful name; I think CobblerClient fits better for now.

> [2] random thoughts.
>
> For launching machines is there anyway to distinguish size/hardware
> capabilities? I'm just thinking that in the future, we'll want to have some
> abstracted notion of machine size so we can utilize them for deploying
> services that need additional hardware capabilities available to a machine
> provider.

Not yet; agree we'll need it at some stage.

> [3] Some pep8/syntax items that came up captured in diff to fix.
> https://pastebin.canonical.com/50619/

Thanks :).

> [4] I'm curious what the ensemble late command stuff is.. it might be nice to
> have some documentation of the orchestration integration when all of this said
> and done.

It's basically just writing the user-data/meta-data files for cloud-init. I agree that we'll want documentation for the Orchestra provider, but there are still a fair number of unanswered questions, so I'm not sure it's quite the right time yet.

[lp:~fwereade/pyjuju/cobbler-connect-production]

319. By William Reade on 2011-08-03

merge trunk

320. By William Reade on 2011-08-03

naming, pep8, redundant code (per hazmat's review)

=== modified file 'ensemble/environment/config.py'
--- ensemble/environment/config.py	2011-07-20 22:49:19 +0000
+++ ensemble/environment/config.py	2011-08-01 20:04:31 +0000
@@ -45,6 +45,12 @@
                        optional=["access-key", "secret-key",
                                  "default-instance-type", "default-ami",
                                  "region", "ec2-uri", "s3-uri"]),
+        "orchestra": KeyDict({"orchestra-server": String(),
+                              "orchestra-user": String(),
+                              "orchestra-pass": String(),
+                              "admin-secret": String(),
+                              "acquired-mgmt-class": String(),
+                              "available-mgmt-class": String()}),
         "dummy": KeyDict({})}))},
     optional=["default"])
 

=== modified file 'ensemble/environment/tests/test_config.py'
--- ensemble/environment/tests/test_config.py	2011-07-20 22:49:19 +0000
+++ ensemble/environment/tests/test_config.py	2011-08-01 20:04:31 +0000
@@ -24,6 +24,19 @@
     nofoo: 1
 """
 
+SAMPLE_ORCHESTRA = """
+ensemble: environments
+
+environments:
+  sample:
+    type: orchestra
+    orchestra-server: somewhe.re
+    orchestra-user: user
+    orchestra-pass: pass
+    admin-secret: garden
+    acquired-mgmt-class: acquired
+    available-mgmt-class: available
+"""
 
 class EnvironmentsConfigTestBase(TestCase):
 
@@ -537,3 +550,24 @@
         else:
             self.fail("Did not properly require "
                       "control-buckets when type == 'ec2'")
+
+    def test_orchestra_schema_requires(self):
+        requires = (
+            "type orchestra-server orchestra-user orchestra-pass "
+            "admin-secret acquired-mgmt-class available-mgmt-class").split()
+        for require in requires:
+            config = yaml.load(SAMPLE_ORCHESTRA)
+            del config["environments"]["sample"][require]
+            self.write_config(yaml.dump(config), other_path=True)
+
+            try:
+                self.config.load(self.other_path)
+            except EnvironmentsConfigError as error:
+                self.assertEquals(str(error),
+                                  "Environments configuration error: %s: "
+                                  "environments.sample.%s: "
+                                  "required value not found"
+                                  % (self.other_path, require))
+            else:
+                self.fail("Did not properly require %s when type == orchestra"
+                          % require)

=== modified file 'ensemble/lib/testing.py'
--- ensemble/lib/testing.py	2011-06-14 16:07:14 +0000
+++ ensemble/lib/testing.py	2011-08-01 20:04:31 +0000
@@ -135,6 +135,8 @@
         yield self.client.exists("/zookeeper")
         returnValue(True)
 
+    client_got = False
+
     def get_zookeeper_client(self):
         client = ZookeeperClient(
             get_test_zookeeper_address(), session_timeout=1000)

=== renamed directory 'ensemble/providers/cobbler' => 'ensemble/providers/orchestra'
=== modified file 'ensemble/providers/orchestra/__init__.py'
--- ensemble/providers/cobbler/__init__.py	2011-07-20 07:10:30 +0000
+++ ensemble/providers/orchestra/__init__.py	2011-08-01 20:04:31 +0000
@@ -1,16 +1,20 @@
-from ensemble.machine import ProviderMachine
-
-
-class CobblerMachine(ProviderMachine):
-    """Cobbler-specific provider machine implementation"""
-    pass
+from twisted.internet.defer import fail, succeed
+
+from ensemble.errors import EnvironmentNotFound
+from ensemble.providers.common.bootstrap import Bootstrap
+
+from .cobbler import CobblerConnect
+from .launch import OrchestraLaunchMachine
 
 
 class MachineProvider(object):
 
+    launch_machine_class = OrchestraLaunchMachine
+
     def __init__(self, environment_name, config):
         self.environment_name = environment_name
         self.config = config
+        self.cobbler = CobblerConnect(config)
 
     def connect(self, share=False):
         """
@@ -50,7 +54,8 @@
         """
         Bootstrap an ensemble server in the provider.
         """
-        raise NotImplementedError()
+        bootstrap = Bootstrap(self)
+        return bootstrap.run()
 
     def shutdown(self):
         """
@@ -71,7 +76,7 @@
         @param state
         @type dict
         """
-        raise NotImplementedError()
+        return succeed(None)
 
     def load_state(self):
         """
@@ -111,6 +116,15 @@
         """
         raise NotImplementedError()
 
+    def get_zookeeper_machines(self):
+        """Find running zookeeper instances.
+
+        @return: the first valid instance found as a single element list.
+
+        @raise: EnvironmentNotFound or EnvironmentPending
+        """
+        return fail(EnvironmentNotFound())
+
 
 class FileStorage(object):
 

=== added file 'ensemble/providers/orchestra/cobbler.py'
--- ensemble/providers/orchestra/cobbler.py	1970-01-01 00:00:00 +0000
+++ ensemble/providers/orchestra/cobbler.py	2011-08-01 20:04:31 +0000
@@ -0,0 +1,107 @@
+from xmlrpclib import Fault
+
+from twisted.internet.defer import inlineCallbacks, returnValue
+from twisted.web.xmlrpc import Proxy
+
+from ensemble.errors import ProviderError
+
+
+def _acceptable_system(systems, available_class):
+    for system in systems:
+        if available_class not in system["mgmt_classes"]:
+            continue
+        if not system["netboot_enabled"]:
+            continue
+        return system
+    raise ProviderError("no acceptable systems available")
+
+
+class CobblerConnect(object):
+    """Handles the details of communicating with a Cobbler server"""
+
+    def __init__(self, config):
+        self._user = config["orchestra-user"]
+        self._pass = config["orchestra-pass"]
+        self._token = ""
+        self._acquired_class = config["acquired-mgmt-class"]
+        self._available_class = config["available-mgmt-class"]
+        self._proxy = Proxy("http://%(orchestra-server)s/cobbler_api" % config)
+
+    def _login(self):
+        login = self._call("login", (self._user, self._pass))
+        login.addCallback(self._set_token)
+        return login
+
+    def _set_token(self, token):
+        self._token  = token
+
+    def _call(self, name, args=(), auth=False):
+
+        def call():
+            call_args = args
+            if auth:
+                call_args += (self._token,)
+            return self._proxy.callRemote(name, *call_args)
+
+        def login_retry(failure):
+            # Login tokens expire after an hour: it seems more sensible
+            # to assume we always have a valid one, and to relogin and
+            # retry if it fails, than to try to maintain validity state.
+            # NOTE: some methods, such as get_system_handle, expect
+            # tokens but appear not to check validity.
+            failure.trap(Fault)
+            if "invalid token" not in failure.getErrorMessage():
+                return failure
+            login = self._login()
+            login.addCallback(lambda unused: call())
+            return login
+
+        result = call()
+        if auth:
+            result.addErrback(login_retry)
+        return result
+
+    def _check_call(self, name, args=(), auth=False, expect=None):
+
+        def check(actual):
+            if actual != expect:
+                raise ProviderError(
+                    "Bad result from call to %s with %s (got %r, expected %r)"
+                    % (name, args, actual, expect))
+            return actual
+
+        call = self._call(name, args, auth=auth)
+        call.addCallback(check)
+        return call
+
+    @inlineCallbacks
+    def _set_on_system(self, name, key, value):
+        handle = yield self._call("get_system_handle", (name,), auth=True)
+        yield self._check_call("modify_system", (handle, key, value),
+                               auth=True, expect=True)
+        yield self._check_call("save_system", (handle,), auth=True, expect=True)
+
+    @inlineCallbacks
+    def acquire_system(self):
+        """Find a system marked as available and mark it as acquired.
+
+        @return: the name of the acquired system.
+
+        @raise: ProviderError if no suitable system can be found.
+        """
+        systems = yield self._call("get_systems")
+        system = _acceptable_system(systems, self._available_class)
+        name = system["name"]
+        # TODO surely we should preserve unrelated management classes?
+        yield self._set_on_system(name, "mgmt_classes", [self._acquired_class])
+        returnValue(name)
+
+    def set_system_ks_meta(self, name, ks_meta):
+        return self._set_on_system(name, "ks_meta", ks_meta)
+
+    @inlineCallbacks
+    def start_system(self, name):
+        yield self._set_on_system(name, "netboot_enabled", True)
+        yield self._call("background_power_system",
+                         ({"power": "on", "systems": [name]},),
+                         auth=True)

=== added file 'ensemble/providers/orchestra/launch.py'
--- ensemble/providers/orchestra/launch.py	1970-01-01 00:00:00 +0000
+++ ensemble/providers/orchestra/launch.py	2011-08-01 20:04:31 +0000
@@ -0,0 +1,20 @@
+from twisted.internet.defer import inlineCallbacks, returnValue
+
+from ensemble.providers.common.launch import LaunchMachine
+from ensemble.providers.orchestra.machine import OrchestraMachine
+
+
+class OrchestraLaunchMachine(LaunchMachine):
+
+    def get_instance_id_command(self):
+        return "this doesn't work yet"
+
+    @inlineCallbacks
+    def start_machine(self, variables):
+        name = yield self._provider.cobbler.acquire_system()
+        ks_meta = 'ENSEMBLE_LATE_COMMAND="true"'
+
+        yield self._provider.cobbler.set_system_ks_meta(name, ks_meta)
+        yield self._provider.cobbler.start_system(name)
+
+        returnValue([OrchestraMachine(name, name)])

=== added file 'ensemble/providers/orchestra/machine.py'
--- ensemble/providers/orchestra/machine.py	1970-01-01 00:00:00 +0000
+++ ensemble/providers/orchestra/machine.py	2011-08-01 20:04:31 +0000
@@ -0,0 +1,6 @@
+from ensemble.machine import ProviderMachine
+
+
+class OrchestraMachine(ProviderMachine):
+    """Orchestra-specific provider machine implementation"""
+    pass

=== added file 'ensemble/providers/orchestra/tests/test_bootstrap.py'
--- ensemble/providers/orchestra/tests/test_bootstrap.py	1970-01-01 00:00:00 +0000
+++ ensemble/providers/orchestra/tests/test_bootstrap.py	2011-08-01 20:04:31 +0000
@@ -0,0 +1,143 @@
+from xmlrpclib import Fault
+
+from twisted.internet.defer import fail, succeed
+from twisted.web.xmlrpc import Proxy
+
+from ensemble.errors import ProviderError
+from ensemble.lib.mocker import MATCH
+from ensemble.lib.testing import TestCase
+from ensemble.providers.orchestra import MachineProvider
+from ensemble.providers.orchestra.machine import OrchestraMachine
+
+CONFIG = {"orchestra-server": "somewhe.re",
+          "orchestra-user": "user",
+          "orchestra-pass": "pass",
+          "acquired-mgmt-class": "acquired",
+          "available-mgmt-class": "available",
+          "admin-secret": "SEEKRIT"}
+
+
+class OrchestraBootstrapTest(TestCase):
+
+    def mock_proxy(self):
+        self.proxy_m = self.mocker.mock(Proxy)
+        Proxy_m = self.mocker.replace(Proxy, spec=None)
+        Proxy_m("http://somewhe.re/cobbler_api")
+        self.mocker.result(self.proxy_m)
+
+    def mock_get_systems(self, acceptable=True):
+        systems = [
+            {"mgmt_classes": ["unknown"]}, # skipped: wrong class
+            {"mgmt_classes": ["available"], # skipped: cannot remote boot
+             "netboot_enabled": False}]
+        if acceptable:
+            systems.append(
+                {"mgmt_classes": ["available"], # relevant
+                 "netboot_enabled": True,
+                 "name": "winston"})
+
+        self.proxy_m.callRemote("get_systems")
+        self.mocker.result(succeed(systems))
+
+    def mock_acquire_system(self, unexpected_auth_error=None):
+        self.proxy_m.callRemote("get_system_handle", "winston", "")
+        if unexpected_auth_error is not None:
+            self.mocker.result(fail(unexpected_auth_error))
+            return
+        self.mocker.result(fail(Fault("blah", "blah invalid token blah")))
+        self.proxy_m.callRemote("login", "user", "pass")
+        self.mocker.result(succeed("TOKEN"))
+        self.proxy_m.callRemote("get_system_handle", "winston", "TOKEN")
+        self.mocker.result(succeed("smith"))
+        self.proxy_m.callRemote(
+            "modify_system", "smith", "mgmt_classes", ["acquired"], "TOKEN")
+        self.mocker.result(succeed(True))
+        self.proxy_m.callRemote("save_system", "smith", "TOKEN")
+        self.mocker.result(succeed(True))
+
+    def mock_set_ks_meta(self, fail_modify=False, fail_save=False):
+        self.proxy_m.callRemote("get_system_handle", "winston", "TOKEN")
+        self.mocker.result(succeed("smith"))
+        # TODO: parameterise this match?
+        match_ks_meta = MATCH(lambda s: s.startswith('ENSEMBLE_LATE_COMMAND="'))
+        self.proxy_m.callRemote(
+            "modify_system", "smith", "ks_meta", match_ks_meta, "TOKEN")
+        if fail_modify:
+            self.mocker.result(succeed(False))
+            return
+        self.mocker.result(succeed(True))
+        self.proxy_m.callRemote("save_system", "smith", "TOKEN")
+        if fail_save:
+            self.mocker.result(succeed(False))
+            return
+        self.mocker.result(succeed(True))
+
+    def mock_start_system(self):
+        self.proxy_m.callRemote("get_system_handle", "winston", "TOKEN")
+        self.mocker.result(succeed("smith"))
+        self.proxy_m.callRemote(
+            "modify_system", "smith", "netboot_enabled", True, "TOKEN")
+        self.mocker.result(succeed(True))
+        self.proxy_m.callRemote("save_system", "smith", "TOKEN")
+        self.mocker.result(succeed(True))
+        self.proxy_m.callRemote(
+            "background_power_system",
+            {"power": "on", "systems": ["winston"]},
+            "TOKEN")
+        self.mocker.result(succeed("[some-timestamp]_power"))
+
+    def test_no_machines_available(self):
+        self.mock_proxy()
+        self.mock_get_systems(acceptable=False)
+        self.mocker.replay()
+        provider = MachineProvider("tetrascape", CONFIG)
+        d = provider.bootstrap()
+        self.assertFailure(d, ProviderError)
+
+    def verify_auth_error(self, error):
+        self.mock_proxy()
+        self.mock_get_systems()
+        self.mock_acquire_system(error)
+        self.mocker.replay()
+        provider = MachineProvider("tetrascape", CONFIG)
+        d = provider.bootstrap()
+        self.assertFailure(d, type(error))
+
+    def test_non_auth_fault(self):
+        return self.verify_auth_error(Fault("blah", "some random error"))
+
+    def test_non_auth_error(self):
+        return self.verify_auth_error(Exception("fiddlesticks"))
+
+    def verify_change_failures(self, **kwargs):
+        self.mock_proxy()
+        self.mock_get_systems()
+        self.mock_acquire_system()
+        self.mock_set_ks_meta(**kwargs)
+        self.mocker.replay()
+        provider = MachineProvider("tetrascape", CONFIG)
+        d = provider.bootstrap()
+        self.assertFailure(d, ProviderError)
+
+    def test_cannot_modify_machine(self):
+        self.verify_change_failures(fail_modify=True)
+
+    def test_cannot_save_machine(self):
+        self.verify_change_failures(fail_save=True)
+
+    def test_launch_available_machine(self):
+        self.mock_proxy()
+        self.mock_get_systems()
+        self.mock_acquire_system()
+        self.mock_set_ks_meta()
+        self.mock_start_system()
+        self.mocker.replay()
+
+        provider = MachineProvider("tetrascape", CONFIG)
+        d = provider.bootstrap()
+        def verify_machines(machines):
+            (machine,) = machines
+            self.assertTrue(isinstance(machine, OrchestraMachine))
+            self.assertEquals(machine.instance_id, "winston")
+        d.addCallback(verify_machines)
+        return d

=== modified file 'ensemble/providers/orchestra/tests/test_provider.py'
--- ensemble/providers/cobbler/tests/test_provider.py	2011-07-20 07:10:30 +0000
+++ ensemble/providers/orchestra/tests/test_provider.py	2011-08-01 20:04:31 +0000
@@ -1,10 +1,19 @@
 from ensemble.lib.testing import TestCase
-from ensemble.providers.cobbler import MachineProvider
+from ensemble.providers.orchestra import MachineProvider
+
+CONFIG = {"orchestra-server": "somewhe.re",
+          "orchestra-user": "henricus",
+          "orchestra-pass": "barbatus",
+          "acquired-mgmt-class": "acquired",
+          "available-mgmt-class": "available"}
 
 
 class ProviderTestCase(TestCase):
 
     def test_create_provider(self):
-        provider = MachineProvider("tetrascape", {"cheese": "edam"})
+        provider = MachineProvider(
+            "tetrascape", CONFIG)
         self.assertEquals(provider.environment_name, "tetrascape")
-        self.assertEquals(provider.config, {"cheese": "edam"})
+        self.assertEquals(provider.config, CONFIG)
+
+