Merge lp:~fwereade/pyjuju/maas-name-constraint into lp:~fwereade/pyjuju/shadow-trunk-1204

[William Reade]

Implement maas-name constraint

Descended from lp:~julian-edwards/juju/name-constraints; but uses new-style
constraints registration.

Also uses a custom ubuntu-series constraint to forbid use of charms written
for systems we can't yet provision with MAAS.

https://codereview.appspot.com/5938050/

[William Reade (fwereade)]

Reviewers: mp+99746_code.launchpad.net,

Message:
Please take a look.

Description:
Implement maas-name constraint

Descended from lp:~julian-edwards/juju/name-constraints; but uses
new-style
constraints registration.

Also uses a custom ubuntu-series constraint to forbid use of charms
written
for systems we can't yet provision with MAAS.

https://code.launchpad.net/~fwereade/juju/maas-name-constraint/+merge/99746

Requires:
https://code.launchpad.net/~fwereade/juju/constraints-get/+merge/99704

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/5938050/

Affected files:
   A [revision details]
   M juju/environment/config.py
   M juju/environment/tests/test_config.py
   M juju/machine/constraints.py
   M juju/machine/tests/test_constraints.py
   M juju/providers/maas/launch.py
   M juju/providers/maas/maas.py
   M juju/providers/maas/provider.py
   M juju/providers/maas/tests/test_launch.py
   M juju/providers/maas/tests/test_maas.py
   M juju/providers/maas/tests/test_provider.py
   M juju/providers/maas/tests/testing.py

[Gavin Panella (allenap)]

On 2012/03/28 14:46:46, fwereade wrote:
> Please take a look.

Looks sensible to me.

https://codereview.appspot.com/5938050/

[Kapil Thangavelu (hazmat)]

LGTM

https://codereview.appspot.com/5938050/diff/1/juju/environment/config.py
File juju/environment/config.py (right):

https://codereview.appspot.com/5938050/diff/1/juju/environment/config.py#newcode68
juju/environment/config.py:68: "default-series": Constant("precise")},
deserves a comment regarding maas limitation in this regard.

https://codereview.appspot.com/5938050/diff/1/juju/environment/tests/test_config.py
File juju/environment/tests/test_config.py (right):

https://codereview.appspot.com/5938050/diff/1/juju/environment/tests/test_config.py#newcode681
juju/environment/tests/test_config.py:681: self.assertRaises(
it would be nice to check this error message for its end user usability.
the nicely messaged value error in the provider doesn't really ever get
surfaced to the user, but this one does.

https://codereview.appspot.com/5938050/

[lp:~fwereade/pyjuju/maas-name-constraint]

514. By William Reade on 2012-03-28

merge parent

[William Reade (fwereade)]

https://codereview.appspot.com/5938050/diff/1/juju/environment/config.py
File juju/environment/config.py (right):

https://codereview.appspot.com/5938050/diff/1/juju/environment/config.py#newcode68
juju/environment/config.py:68: "default-series": Constant("precise")},
On 2012/03/28 18:10:47, hazmat wrote:
> deserves a comment regarding maas limitation in this regard.

Done.

https://codereview.appspot.com/5938050/diff/1/juju/environment/tests/test_config.py
File juju/environment/tests/test_config.py (right):

https://codereview.appspot.com/5938050/diff/1/juju/environment/tests/test_config.py#newcode681
juju/environment/tests/test_config.py:681: self.assertRaises(
On 2012/03/28 18:10:47, hazmat wrote:
> it would be nice to check this error message for its end user
usability. the
> nicely messaged value error in the provider doesn't really ever get
surfaced to
> the user, but this one does.

Good catch, thank you. Done.

https://codereview.appspot.com/5938050/

[lp:~fwereade/pyjuju/maas-name-constraint]

515. By William Reade on 2012-03-28

address review

516. By William Reade on 2012-03-28

merge parent

[William Reade (fwereade)]

*** Submitted:

Implement maas-name constraint

Descended from lp:~julian-edwards/juju/name-constraints; but uses
new-style
constraints registration.

Also uses a custom ubuntu-series constraint to forbid use of charms
written
for systems we can't yet provision with MAAS.

R=allenap, hazmat
CC=
https://codereview.appspot.com/5938050

https://codereview.appspot.com/5938050/

=== modified file 'juju/environment/config.py'
--- juju/environment/config.py	2012-03-23 11:05:05 +0000
+++ juju/environment/config.py	2012-03-28 19:04:19 +0000
@@ -65,7 +65,9 @@
             "maas-oauth": OAuthString(),
             "admin-secret": String(),
             "placement": _EITHER_PLACEMENT,
-            "default-series": String()},
+            # MAAS currently only provisions precise; any other default-series
+            # would just lead to errors down the line.
+            "default-series": Constant("precise")},
             optional=["placement"]),
         "local": KeyDict({
             "admin-secret": String(),

=== modified file 'juju/environment/tests/test_config.py'
--- juju/environment/tests/test_config.py	2012-03-27 23:22:28 +0000
+++ juju/environment/tests/test_config.py	2012-03-28 19:04:19 +0000
@@ -46,7 +46,7 @@
     maas-server: somewhe.re
     maas-oauth: foo:bar:baz
     admin-secret: garden
-    default-series: oneiric
+    default-series: precise
 """
 
 SAMPLE_LOCAL = """
@@ -674,14 +674,16 @@
                 self.fail("Did not properly require %s when type == maas"
                           % require)
 
-    def test_maas_respects_default_series(self):
+    def test_maas_default_series(self):
         config = yaml.load(SAMPLE_MAAS)
         config["environments"]["sample"]["default-series"] = "magnificent"
         self.write_config(yaml.dump(config), other_path=True)
-        self.config.load(self.other_path)
-
-        provider = self.config.get_default().get_machine_provider()
-        self.assertEqual(provider.config["default-series"], "magnificent")
+        e = self.assertRaises(
+            EnvironmentsConfigError, self.config.load, self.other_path)
+        self.assertIn(
+            "environments.sample.default-series: expected 'precise', got "
+            "'magnificent'",
+            str(e))
 
     def test_maas_verifies_placement(self):
         config = yaml.load(SAMPLE_MAAS)

=== modified file 'juju/machine/constraints.py'
--- juju/machine/constraints.py	2012-03-28 17:47:44 +0000
+++ juju/machine/constraints.py	2012-03-28 19:04:19 +0000
@@ -12,8 +12,9 @@
 ALL_NAMES = (
     "ubuntu-series", "provider-type",
     "arch", "cpu", "mem", "instance-type",
+    "ec2-zone",
+    "maas-name",
     "orchestra-classes",
-    "ec2-zone",
 )
 
 
@@ -237,7 +238,7 @@
         """Return a Constraints with the "ubuntu-series" set to `series`"""
         data = dict(self._data)
         data["ubuntu-series"] = series
-        return Constraints(self._available, data)
+        return self._available.load(data)
 
     @property
     def complete(self):

=== modified file 'juju/machine/tests/test_constraints.py'
--- juju/machine/tests/test_constraints.py	2012-03-28 08:43:23 +0000
+++ juju/machine/tests/test_constraints.py	2012-03-28 19:04:19 +0000
@@ -391,3 +391,14 @@
         self.assertEquals(c8["cpu"], 1.0)
         self.assertEquals(c8["mem"], 512.0)
         self.assertEquals(c8["instance-type"], None)
+
+    def test_load_validates(self):
+        self.allow_names("foo")
+        cs = ConstraintSet("provider")
+
+        def blam(s):
+            raise ValueError(s)
+
+        cs.register("foo", converter=blam)
+        e = self.assertRaises(ConstraintError, cs.load, {"foo": "bar"})
+        self.assertEquals(str(e), "Bad 'foo' constraint 'bar': bar")

=== modified file 'juju/providers/maas/launch.py'
--- juju/providers/maas/launch.py	2012-03-23 08:34:09 +0000
+++ juju/providers/maas/launch.py	2012-03-28 19:04:19 +0000
@@ -35,11 +35,8 @@
             representing the newly-launched machine
         :rtype: :class:`twisted.internet.defer.Deferred`
         """
-        log.warning(
-            "The maas provider does not yet support machine constraints; "
-            "machine will be chosen abritrarily.")
         maas_client = self._provider.maas_client
-        instance_data = yield maas_client.acquire_node()
+        instance_data = yield maas_client.acquire_node(self._constraints)
         instance_uri = instance_data["resource_uri"]
         # If anything goes wrong after the acquire and before the launch
         # actually happens, we attempt to release the node.

=== modified file 'juju/providers/maas/maas.py'
--- juju/providers/maas/maas.py	2012-03-27 10:58:36 +0000
+++ juju/providers/maas/maas.py	2012-03-28 19:04:19 +0000
@@ -96,13 +96,17 @@
                 for resource_uri in resource_uris)
         return self.get("api/1.0/nodes/", params)
 
-    def acquire_node(self):
+    def acquire_node(self, constraints=None):
         """Ask MAAS to assign a node to us.
 
         :return: A Deferred whose value is the resource URI to the node
             that was acquired.
         """
         params = {"op": "acquire"}
+        if constraints is not None:
+            name = constraints["maas-name"]
+            if name is not None:
+                params["name"] = name
         return self.post("api/1.0/nodes/", params)
 
     def start_node(self, resource_uri, user_data):

=== modified file 'juju/providers/maas/provider.py'
--- juju/providers/maas/provider.py	2012-03-28 07:04:10 +0000
+++ juju/providers/maas/provider.py	2012-03-28 19:04:19 +0000
@@ -33,6 +33,21 @@
         """Return a WebDAV-backed FileStorage abstraction."""
         return self._storage
 
+    @inlineCallbacks
+    def get_constraint_set(self):
+        """Return the set of constraints that are valid for this provider."""
+        cs = yield super(MachineProvider, self).get_constraint_set()
+
+        def require_precise(s):
+            if s != "precise":
+                raise ValueError(
+                     "MAAS currently only provisions machines running precise")
+            return s
+
+        cs.register("ubuntu-series", converter=require_precise, visible=False)
+        cs.register("maas-name")
+        returnValue(cs)
+
     def get_serialization_data(self):
         """Get provider configuration suitable for serialization.
 

=== modified file 'juju/providers/maas/tests/test_launch.py'
--- juju/providers/maas/tests/test_launch.py	2012-03-28 07:04:10 +0000
+++ juju/providers/maas/tests/test_launch.py	2012-03-28 19:04:19 +0000
@@ -42,7 +42,7 @@
         provider._storage = FakeStorage()
         return provider
 
-    def test_bad_data(self):
+    def test_no_machine_id(self):
         provider = self._get_provider()
         d = provider.start_machine({})
         self.assertFailure(d, ProviderError)
@@ -54,12 +54,25 @@
         d.addCallback(verify)
         return d
 
+    def test_no_constraints(self):
+        provider = self._get_provider()
+        d = provider.start_machine({"machine-id": 99})
+        self.assertFailure(d, ProviderError)
+
+        def verify(error):
+            self.assertEqual(
+                str(error),
+                "Cannot launch a machine without specifying constraints")
+        d.addCallback(verify)
+        return d
+
     def test_no_available_machines(self):
         # Requesting a startup of an already-acquired machine should
         # result in a Fault being returned.
         provider = self._get_provider(
             FakeMAASHTTPConnectionWithNoAvailableNodes)
-        machine_data = {"machine-id": "foo", "constraints": {}}
+        machine_data = {
+            "machine-id": "foo", "constraints": {"maas-name": None}}
         d = provider.start_machine(machine_data)
 
         # These arbitrary fake failure values come from
@@ -82,7 +95,8 @@
 
         # Try to start up that node using the fake MAAS.
         provider = self._get_provider(FakeMAASHTTPConnection)
-        machine_data = {"machine-id": machine_id, "constraints": {}}
+        machine_data = {
+            "machine-id": machine_id, "constraints": {"maas-name": None}}
         machine_list = yield provider.start_machine(machine_data)
 
         # Test that it returns a list containing a single MAASMachine
@@ -96,6 +110,25 @@
             "MAASMachine values of instance_id / dns_name don't match expected"
             "values")
 
+    @inlineCallbacks
+    def test_launch_with_name_constraint(self):
+        # Try to launch a particular machine by its name using the
+        # "maas-name" constraint.
+
+        # Pick the "moon" node out of the test data.
+        target_node = NODE_JSON[1]
+        self.assertEqual("moon", target_node["hostname"])
+        machine_id = "foo"
+        provider = self._get_provider(FakeMAASHTTPConnection)
+        constraints = {"maas-name": "moon"}
+        machine_data = {"machine-id": machine_id, "constraints": constraints}
+        machine_list = yield provider.start_machine(machine_data)
+
+        # Check that "moon" was started.
+        [machine] = machine_list
+        self.assertIsInstance(machine, MAASMachine)
+        self.assertEqual(machine.dns_name, "moon")
+
     def test_failed_launch(self):
         """If an acquired node fails to start, it is released."""
         # Throw away logs.
@@ -116,7 +149,7 @@
         # The following operations happen in sequence.
         mocker.order()
         # First, the node is acquired.
-        mock_client.acquire_node()
+        mock_client.acquire_node({"maas-name": None})
         mocker.result({"resource_uri": "/node/123"})
         # Second, the node is started. We simulate a failure at this stage.
         mock_client.start_node("/node/123", ANY)
@@ -127,5 +160,5 @@
 
         mocker.replay()
         return self.assertFailure(
-            MAASLaunchMachine(mock_provider, {}).run("fred"),
+            MAASLaunchMachine(mock_provider, {"maas-name": None}).run("fred"),
             ZeroDivisionError)

=== modified file 'juju/providers/maas/tests/test_maas.py'
--- juju/providers/maas/tests/test_maas.py	2012-03-27 11:26:50 +0000
+++ juju/providers/maas/tests/test_maas.py	2012-03-28 19:04:19 +0000
@@ -116,13 +116,16 @@
         return succeed(json.dumps(request_url))
 
 
-class TestMAASClientWithTwisted(TestCase):
+class TestMAASClientBase:
 
     def get_client(self):
         """Return a MAASClient with a FakeMAASHTTPConnection factory."""
         log = self.setup_connection(MAASClient, FakeMAASHTTPConnection)
         return MAASClient(CONFIG), log
 
+
+class TestMAASClientWithTwisted(TestCase, TestMAASClientBase):
+
     def assertPathEqual(self, expected_path, observed_url):
         """Assert path of `observed_url` is equal to `expected_path`."""
         observed_path = urlparse(observed_url).path
@@ -261,3 +264,37 @@
         client, log = self.get_client()
         returned_data = yield client.release_node(resource_uri)
         self.assertEqual(returned_data, NODE_JSON[0])
+
+
+class TestConstraints(TestCase, TestMAASClientBase):
+
+    class fake_post:
+        def __call__(self, uri, params):
+            self.params_used = params
+
+    def set_up_client_with_fake(self):
+        fake = self.fake_post()
+        client, log = self.get_client()
+        self.patch(client, 'post', fake)
+        return client
+
+    def test_acquire_node_handles_name_constraint(self):
+        # Ensure that the name constraint is passed through to the post
+        # method.
+        client = self.set_up_client_with_fake()
+        constraints = {"maas-name": "gargleblaster"}
+        client.acquire_node(constraints)
+
+        name = client.post.params_used.get("name")
+        self.assertEqual("gargleblaster", name)
+
+    def test_acquire_node_ignores_unknown_constraints(self):
+        # If an unknown constraint is passed it should be ignored.
+        client = self.set_up_client_with_fake()
+        constraints = {"maas-name": "zaphod", "guinness": "widget"}
+        client.acquire_node(constraints)
+
+        guinness = client.post.params_used.get("guinness")
+        self.assertIs(None, guinness)
+        name = client.post.params_used.get("name")
+        self.assertEqual("zaphod", name)

=== modified file 'juju/providers/maas/tests/test_provider.py'
--- juju/providers/maas/tests/test_provider.py	2012-03-27 12:47:35 +0000
+++ juju/providers/maas/tests/test_provider.py	2012-03-28 19:04:19 +0000
@@ -5,7 +5,7 @@
 
 from twisted.internet.defer import inlineCallbacks
 
-from juju.errors import MachinesNotFound, ProviderError
+from juju.errors import ConstraintError, MachinesNotFound, ProviderError
 from juju.lib.mocker import ANY
 from juju.providers.maas import MachineProvider
 from juju.providers.maas.maas import MAASClient
@@ -138,3 +138,41 @@
         self.assertEqual(
             [machines[0].instance_id],
             [machine.instance_id for machine in machines_terminated])
+
+    @inlineCallbacks
+    def test_constraints(self):
+        provider = MachineProvider("blah", CONFIG)
+        cs = yield provider.get_constraint_set()
+        self.assertEquals(cs.parse([]), {
+            "provider-type": "maas",
+            "ubuntu-series": None,
+            "maas-name": None})
+        self.assertEquals(cs.parse(["maas-name=totoro"]), {
+            "provider-type": "maas",
+            "ubuntu-series": None,
+            "maas-name": "totoro"})
+
+        bill = cs.parse(["maas-name=bill"]).with_series("precise")
+        ben = cs.parse(["maas-name=ben"]).with_series("precise")
+        nil = cs.parse([]).with_series("precise")
+        self.assertTrue(bill.can_satisfy(bill))
+        self.assertTrue(bill.can_satisfy(nil))
+        self.assertTrue(ben.can_satisfy(ben))
+        self.assertTrue(ben.can_satisfy(nil))
+        self.assertTrue(nil.can_satisfy(nil))
+        self.assertFalse(nil.can_satisfy(bill))
+        self.assertFalse(nil.can_satisfy(ben))
+        self.assertFalse(ben.can_satisfy(bill))
+        self.assertFalse(bill.can_satisfy(ben))
+
+    @inlineCallbacks
+    def test_precise_only_constraints(self):
+        provider = MachineProvider("blah", CONFIG)
+        cs = yield provider.get_constraint_set()
+        e = self.assertRaises(
+             ConstraintError, cs.parse([]).with_series, "not-precise")
+        self.assertEquals(
+            str(e),
+            "Bad 'ubuntu-series' constraint 'not-precise': MAAS currently "
+            "only provisions machines running precise")
+

=== modified file 'juju/providers/maas/tests/testing.py'
--- juju/providers/maas/tests/testing.py	2012-03-27 11:26:50 +0000
+++ juju/providers/maas/tests/testing.py	2012-03-28 19:04:19 +0000
@@ -142,10 +142,17 @@
             raise AssertionError("Unknown API method called")
 
     def start_node(self):
-        # TODO: Simply returns the first node, ignoring the data. Fix.
+        # Poor man's node selection.
+        if NODE_JSON[1]["system_id"] in self.url:
+            return defer.succeed(json.dumps(NODE_JSON[1]))
         return defer.succeed(json.dumps(NODE_JSON[0]))
 
     def acquire_node(self):
+        # Implement a poor man's name constraints.
+        if "moon" in self.data:
+            return defer.succeed(json.dumps(NODE_JSON[1]))
+        elif "sun" in self.data:
+            return defer.succeed(json.dumps(NODE_JSON[0]))
         return defer.succeed(json.dumps(NODE_JSON[0]))
 
     def stop_node(self):