Merge lp:~gz/pyjuju/add_maas_constraints into lp:pyjuju

[Martin Packman]

Add generic and tag constraints to maas provider

Passes through standard constraints 'arch', 'cpu', and 'mem', also
specific 'maas-tags' which can be used to indicate other properties.

Launching a new machine still involves calling acquire in the MaaS api
which returns only one node rather than a list of candidates to select
from, so the constraints are still not used on the Juju side for now.

https://codereview.appspot.com/6569053/

[Martin Packman (gz)]

Reviewers: mp+126203_code.launchpad.net,

Message:
Please take a look.

Description:
Add generic and tag constraints to maas provider

Passes through standard constraints 'arch', 'cpu', and 'mem', also
specific 'maas-tags' which can be used to indicate other properties.

Launching a new machine still involves calling acquire in the MaaS api
which returns only one node rather than a list of candidates to select
from, so the constraints are still not used on the Juju side for now.

https://code.launchpad.net/~gz/juju/add_maas_constraints/+merge/126203

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   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

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: juju/providers/maas/maas.py
=== modified file 'juju/providers/maas/maas.py'
--- juju/providers/maas/maas.py 2012-05-25 06:49:32 +0000
+++ juju/providers/maas/maas.py 2012-09-25 08:58:47 +0000
@@ -44,6 +44,14 @@

  class MAASClient(MAASOAuthConnection):

+ _handled_constraints = (
+ ("maas-name", "name"),
+ ("maas-tags", "tags"),
+ ("arch", "arch"),
+ ("cpu", "cpu_count"),
+ ("mem", "mem"),
+ )
+
      def __init__(self, config):
          """Initialise an API client for MAAS.

@@ -119,9 +127,10 @@
          """
          params = {"op": "acquire"}
          if constraints is not None:
- name = constraints["maas-name"]
- if name is not None:
- params["name"] = name
+ for key_from, key_to in self._handled_constraints:
+ value = constraints.get(key_from, None)
+ if value is not None:
+ params[key_to] = str(value)
          return self.post("api/1.0/nodes/", params)

      def start_node(self, resource_uri, user_data):

Index: juju/providers/maas/provider.py
=== modified file 'juju/providers/maas/provider.py'
--- juju/providers/maas/provider.py 2012-03-28 14:40:14 +0000
+++ juju/providers/maas/provider.py 2012-09-25 08:58:47 +0000
@@ -46,6 +46,10 @@

          cs.register("ubuntu-series", converter=require_precise,
visible=False)
          cs.register("maas-name")
+ cs.register_generics([])
+ # All the logic for resolving tags lives in MaaS for now so Juju
+ # can just treat the maas-tags constraint as an opaque string.
+ cs.register("maas-tags")
          returnValue(cs)

      def get_serialization_data(self):

Index: juju/providers/maas/tests/test_launch.py
=== modified file 'juju/providers/maas/tests/test_launch.py'
--- juju/providers/maas/tests/test_launch.py 2012-05-25 06:32:24 +0000
+++ juju/providers/maas/tests/test_launch.py 2012-09-25 08:59:22...

[John A Meinel (jameinel)]

(Just adding a comment so that I will get any chatter on this proposal)

[Kapil Thangavelu (hazmat)]

from irc

Sep 25 05:27:05 <jam> mgz: well I know hazmat asked us to make it less
specific, so the provider should query Maas for a list of acceptable
tags.
Sep 25 05:27:10 <mgz> could just pick nodes with a superset of the
tags specified, or could do full boolean logic

Sep 25 05:27:18 <jam> less generic?

Sep 25 05:27:44 <mgz> I think hazmat will be fine with maas returning
good errors for non-existant tags or a bad tag string

Sep 25 05:27:49 <jam> The argument is that deploying is async, so by
the time the provider responds, it is too late to inform the user of a
typo/etc.
Sep 25 05:28:35 <mgz> right, so provided we catch before acquire
actually happens that a tag is bad, and return a 4XX straight away
without going down to the cluster, I think that's fine
Sep 25 05:30:49 <mgz> ...I probably want to add some tests for that
error case

so a user on the cli is going to do something like

   juju deploy xyz --constraints="maas-tags=foo&bar"

except bar doesn't exist, and they'll never see an error, just an
instance which stays forever in pending state, and errors in the
provisioning agent log.

given that this is effectively free form vocabulary of values, it would
be nice to see a validation of the constraint value to prevent this.

constraint validation facilities are a bit weak but effectively on
register('maas-tags') you can just pass the a converter function with
the vocabulary introspected from the maas server. the logical operators
can be stripped and the remainder differenced against the vocabulary to
detect unknown values, raising a constrainterror on invalid values.

https://codereview.appspot.com/6569053/diff/1/juju/providers/maas/maas.py
File juju/providers/maas/maas.py (right):

https://codereview.appspot.com/6569053/diff/1/juju/providers/maas/maas.py#newcode48
juju/providers/maas/maas.py:48: ("maas-name", "name"),
is maas-name still supported?

https://codereview.appspot.com/6569053/

[Martin Packman (gz)]

> so a user on the cli is going to do something like
>
> juju deploy xyz --constraints="maas-tags=foo&bar"
>
> except bar doesn't exist, and they'll never see an error, just an
> instance which stays forever in pending state, and errors in the
> provisioning agent log.

Thanks for clarifying, that is a painful scenario. So, this is already an issue with providers that permit constraints that can't be fully validated at the juju cli level before passing on to the provisioning agent? For instance, supplying 'ec2-zone' that is a simble ascii character, but does not exist in the region? Is there a bug filed already for better error reporting in cases like this where the provisioning agent fails to deploy something? Just retrying forever isn't really ideal.

> given that this is effectively free form vocabulary of values, it would
> be nice to see a validation of the constraint value to prevent this.

Okay, I'll go back to the earlier code I had which checked tags were valid at constraint creation. It's a little painful because juju can't sensibly cache the list of tags, as the scenario where someone creates a new tag using the maas cli client then wants to juju deploy a service constrained on that tag immediately seems quite likely. As convert callbacks aren't deferred, it's not possible to recheck for a newly added tag on the fly.

> https://codereview.appspot.com/6569053/diff/1/juju/providers/maas/maas.py#newc
> ode48
> juju/providers/maas/maas.py:48: ("maas-name", "name"),
> is maas-name still supported?

I see no reason to drop it as part of this branch, and the underlying code in MaaS is still present for now. It's simply not useful if you want to use other constraints.

[Kapil Thangavelu (hazmat)]

On Thursday, September 27, 2012, Martin Packman wrote:

> > so a user on the cli is going to do something like
> >
> > juju deploy xyz --constraints="maas-tags=foo&bar"
> >
> > except bar doesn't exist, and they'll never see an error, just an
> > instance which stays forever in pending state, and errors in the
> > provisioning agent log.
>
> Thanks for clarifying, that is a painful scenario. So, this is already an
> issue with providers that permit constraints that can't be fully validated
> at the juju cli level before passing on to the provisioning agent? For
> instance, supplying 'ec2-zone' that is a simble ascii character, but does
> not exist in the region? Is there a bug filed already for better error
> reporting in cases like this where the provisioning agent fails to deploy
> something? Just retrying forever isn't really ideal.

It's definitely an issue, the other providers export a fairly well known
set of symbols, there is some limited validation of bits like zone, which
actually does correspond to an ec2 entity albeit in abbreviated form, and
the symbol itself is well symbolic at the aws level.

A further notion is tracking state creation, and properly
reporting/transitioning these to error, when some reasonable time period
has passed.

> > given that this is effectively free form vocabulary of values, it would
> > be nice to see a validation of the constraint value to prevent this.
>
> Okay, I'll go back to the earlier code I had which checked tags were valid
> at constraint creation. It's a little painful because juju can't sensibly
> cache the list of tags, as the scenario where someone creates a new tag
> using the maas cli client then wants to juju deploy a service constrained
> on that tag immediately seems quite likely. As convert callbacks aren't
> deferred, it's not possible to recheck for a newly added tag on the fly.
>
>
I'll need to address some of that in the security work, re serialization of
constraints, to bypass non privileged agents using the constraints
definition, unfortunately constraints got at the env level, get looked up
from machine states, and unit states, as it's effectively an ordered
multidict lookup for those entities.

> >
> https://codereview.appspot.com/6569053/diff/1/juju/providers/maas/maas.py#newc
> > ode48
> > juju/providers/maas/maas.py:48: ("maas-name", "name"),
> > is maas-name still supported?
>
> I see no reason to drop it as part of this branch, and the underlying code
> in MaaS is still present for now. It's simply not useful if you want to use
> other constraints.

It's worse than not useful IMO, its proven actively detrimental, but your
right re keeping, hopefully with real constraints people can stop using it.

> --
> https://code.launchpad.net/~gz/juju/add_maas_constraints/+merge/126203
> You are subscribed to branch lp:juju.
>

[John A Meinel (jameinel)]

So at this point on the Maas side, we raise a InvalidConstraint error if you ask for a tag that doesn't exist in the DB. (Which gets turned into a proper BAD_REQUEST error with a helpful error message.)

Is that enough for this to work nicely? (Or is the actual request so async that it still isn't in time.)

If so, you can query api/1.0/tags?op=list and Maas will give you a list of all tags in the db (it probably gives too much information, but you will get the list of tag names out of that).

I agree that we shouldn't be caching the known list of tags.

[Kapil Thangavelu (hazmat)]

> So at this point on the Maas side, we raise a InvalidConstraint error if you
> ask for a tag that doesn't exist in the DB. (Which gets turned into a proper
> BAD_REQUEST error with a helpful error message.)
>
> Is that enough for this to work nicely? (Or is the actual request so async
> that it still isn't in time.)

it does not, the scenario in my initial reply still holds true. the user commands don't execute calls to providers for normal usage (exceptions bootstrap/destroy-environment), instead a separate provisioning agent in the environment does.

>
> If so, you can query api/1.0/tags?op=list and Maas will give you a list of all
> tags in the db (it probably gives too much information, but you will get the
> list of tag names out of that).
>

sounds good, what's the return value of an individual tag in that result look like?

> I agree that we shouldn't be caching the known list of tags.

looking back my reply about this was a little unclear previously, but agreed no need to cache.

[lp:~gz/pyjuju/add_maas_constraints]

581. By Martin Packman on 2012-09-28

Revert to checking tag constraint directly, fetch list of valid tags from maas

[Martin Packman (gz)]

Please take a look.

https://codereview.appspot.com/6569053/

[lp:~gz/pyjuju/add_maas_constraints]

582. By Martin Packman on 2012-09-28

Merge trunk to resolve conflicts with removal limitation to precise

[Kapil Thangavelu (hazmat)]

looks nice, thanks.

One concern for which i'd request you'd file a new bug for future ref,
is that given the valid tag set could change at any time, a previously
valid set could become invalid. That's deferrable for now, at the moment
it will just yield a problem creating services.

https://codereview.appspot.com/6569053/

[Kapil Thangavelu (hazmat)]

s/creating services/creating service units.

[Martin Packman (gz)]

On 2012/10/01 14:39:59, hazmat wrote:
> looks nice, thanks.

> One concern for which i'd request you'd file a new bug for future ref,
is that
> given the valid tag set could change at any time, a previously valid
set could
> become invalid. That's deferrable for now, at the moment it will just
yield a
> problem creating services.

I've filed bug 1059753 which I hope captures this issue, feel free to
edit the summary if I have any terminology or details confused. Thanks!

https://codereview.appspot.com/6569053/

=== modified file 'juju/providers/maas/maas.py'
--- juju/providers/maas/maas.py	2012-09-20 15:11:08 +0000
+++ juju/providers/maas/maas.py	2012-09-28 11:52:20 +0000
@@ -44,6 +44,14 @@
 
 class MAASClient(MAASOAuthConnection):
 
+    _handled_constraints = (
+        ("maas-name", "name"),
+        ("maas-tags", "tags"),
+        ("arch", "arch"),
+        ("cpu", "cpu_count"),
+        ("mem", "mem"),
+        )
+
     def __init__(self, config):
         """Initialise an API client for MAAS.
 
@@ -119,9 +127,10 @@
         """
         params = {"op": "acquire"}
         if constraints is not None:
-            name = constraints["maas-name"]
-            if name is not None:
-                params["name"] = name
+            for key_from, key_to in self._handled_constraints:
+                value = constraints.get(key_from, None)
+                if value is not None:
+                    params[key_to] = str(value)
         return self.post("api/1.0/nodes/", params)
 
     def start_node(self, resource_uri, ubuntu_series, user_data):
@@ -157,3 +166,11 @@
         """
         params = {"op": "release"}
         return self.post(resource_uri, params)
+
+    def list_tags(self):
+        """Ask MAAS to return a list of all the tags defined.
+
+        :return: A Deferred whose value is the list of tags.
+        """
+        params = {"op": "list"}
+        return self.get("api/1.0/tags/", params)

=== modified file 'juju/providers/maas/provider.py'
--- juju/providers/maas/provider.py	2012-09-14 20:02:17 +0000
+++ juju/providers/maas/provider.py	2012-09-28 11:52:20 +0000
@@ -17,6 +17,36 @@
 log = logging.getLogger("juju.maas")
 
 
+class _TagHandler(object):
+    """Parser and validator for tags constraint expressions
+
+    Tag names are extracted and checked against the list of known tags
+    reported by the api.
+
+    Currently tag constraints consist of just comma and/or whitespace tag
+    names, all of are required for a match. Extending this to support full
+    boolean expressions would be possible, and some forward compatibility
+    is attempted.
+    """
+
+    def __init__(self, tags_info):
+        self.tag_names = [tag['name'] for tag in tags_info]
+
+    compare = staticmethod(set.issuperset)
+
+    def convert(self, tag_expression):
+        """Extract set of names in tag_expression checking they all exist"""
+        tags = set()
+        for c in (",", "&", "|", "!"):
+            tag_expression = tag_expression.replace(c, " ")
+        for word in tag_expression.strip().split():
+            tag = word.lower()
+            if tag not in self.tag_names:
+                raise ValueError("tag %r does not exist" % (tag,))
+            tags.add(tag)
+        return tags
+
+
 class MachineProvider(MachineProviderBase):
     """MachineProvider for use in a MAAS environment"""
 
@@ -40,6 +70,15 @@
 
         cs.register("ubuntu-series", visible=False)
         cs.register("maas-name")
+        cs.register_generics([])
+        # MaaS client errors on the provisioning agent are not reported by the
+        # juju cli so try validating tags when the constraint is created.
+        # Because new tags may be registered at any point, caching the list of
+        # tags is not safe and they must be refetched every time.
+        tags_info = yield self.maas_client.list_tags()
+        handler = _TagHandler(tags_info)
+        cs.register("maas-tags", converter=handler.convert,
+            comparer=handler.compare)
         returnValue(cs)
 
     def get_serialization_data(self):

=== modified file 'juju/providers/maas/tests/test_launch.py'
--- juju/providers/maas/tests/test_launch.py	2012-09-27 04:36:51 +0000
+++ juju/providers/maas/tests/test_launch.py	2012-09-28 11:52:20 +0000
@@ -71,8 +71,7 @@
         provider = self._get_provider(
             FakeMAASHTTPConnectionWithNoAvailableNodes)
         machine_data = {
-            "machine-id": "foo", "constraints": {"maas-name": None,
-            "ubuntu-series": "splendid"}}
+            "machine-id": "foo", "constraints": {"ubuntu-series": "splendid"}}
         d = provider.start_machine(machine_data)
 
         # These arbitrary fake failure values come from
@@ -94,8 +93,7 @@
         # Try to start up that node using the fake MAAS.
         provider = self._get_provider(FakeMAASHTTPConnection)
         machine_data = {
-            "machine-id": machine_id, "constraints": {"maas-name": None,
-            "ubuntu-series": "splendid"}}
+            "machine-id": "foo", "constraints": {"ubuntu-series": "splendid"}}
         machine_list = yield provider.start_machine(machine_data)
 
         # Test that it returns a list containing a single MAASMachine
@@ -148,7 +146,7 @@
         # The following operations happen in sequence.
         mocker.order()
         # First, the node is acquired.
-        mock_client.acquire_node({"maas-name": None, "ubuntu-series": "precise"})
+        mock_client.acquire_node({"ubuntu-series": "precise"})
         mocker.result({"resource_uri": "/node/123"})
         # Second, the node is started. We simulate a failure at this stage.
         mock_client.start_node("/node/123", "precise", ANY)
@@ -160,5 +158,5 @@
         mocker.replay()
         return self.assertFailure(
             MAASLaunchMachine(mock_provider,
-                              {"maas-name": None, "ubuntu-series": "precise"}).run("fred"),
+                              {"ubuntu-series": "precise"}).run("fred"),
             ZeroDivisionError)

=== modified file 'juju/providers/maas/tests/test_maas.py'
--- juju/providers/maas/tests/test_maas.py	2012-09-14 20:02:17 +0000
+++ juju/providers/maas/tests/test_maas.py	2012-09-28 11:52:20 +0000
@@ -337,3 +337,35 @@
         self.assertIs(None, guinness)
         name = client.post.params_used.get("name")
         self.assertEqual("zaphod", name)
+
+    def test_acquire_node_handles_arch_constraint(self):
+        client = self.set_up_client_with_fake()
+        constraints = {"arch": "i386"}
+        client.acquire_node(constraints)
+
+        arch = client.post.params_used.get("arch")
+        self.assertEqual("i386", arch)
+
+    def test_acquire_node_handles_cpu_constraint(self):
+        client = self.set_up_client_with_fake()
+        constraints = {"cpu": 2}
+        client.acquire_node(constraints)
+
+        cpu_count = client.post.params_used.get("cpu_count")
+        self.assertEqual("2", cpu_count)
+
+    def test_acquire_node_handles_mem_constraint(self):
+        client = self.set_up_client_with_fake()
+        constraints = {"mem": 2048}
+        client.acquire_node(constraints)
+
+        mem = client.post.params_used.get("mem")
+        self.assertEqual("2048", mem)
+
+    def test_acquire_node_handles_arbitrary_tag_query(self):
+        client = self.set_up_client_with_fake()
+        constraints = {"maas-tags": "red&!white|blue"}
+        client.acquire_node(constraints)
+
+        tags = client.post.params_used.get("tags")
+        self.assertEqual("red&!white|blue", tags)

=== modified file 'juju/providers/maas/tests/test_provider.py'
--- juju/providers/maas/tests/test_provider.py	2012-09-27 04:36:51 +0000
+++ juju/providers/maas/tests/test_provider.py	2012-09-28 11:52:20 +0000
@@ -3,9 +3,9 @@
 
 """Test cases for juju.providers.maas.provider"""
 
-from twisted.internet.defer import inlineCallbacks
+from twisted.internet.defer import inlineCallbacks, succeed
 
-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
@@ -141,15 +141,24 @@
 
     @inlineCallbacks
     def test_constraints(self):
+        self.setup_connection(MAASClient, FakeMAASHTTPConnection)
         provider = MachineProvider("blah", CONFIG)
         cs = yield provider.get_constraint_set()
         self.assertEquals(cs.parse([]), {
             "provider-type": "maas",
             "ubuntu-series": None,
+            "arch": "amd64",
+            "cpu": 1,
+            "mem": 512,
+            "maas-tags": None,
             "maas-name": None})
         self.assertEquals(cs.parse(["maas-name=totoro"]), {
             "provider-type": "maas",
             "ubuntu-series": None,
+            "arch": "amd64",
+            "cpu": 1,
+            "mem": 512,
+            "maas-tags": None,
             "maas-name": "totoro"})
 
         bill = cs.parse(["maas-name=bill"]).with_series("precise")
@@ -164,3 +173,23 @@
         self.assertFalse(nil.can_satisfy(ben))
         self.assertFalse(ben.can_satisfy(bill))
         self.assertFalse(bill.can_satisfy(ben))
+
+    @inlineCallbacks
+    def test_constraints_on_tags(self):
+        mock_client = self.mocker.patch(MAASClient(CONFIG))
+        mock_client.list_tags()
+        self.mocker.result(succeed([
+            {'name': "furry", 'definition': "HAS fur", 'comment': ""},
+            {'name': "clawed", 'definition': "HAS claws", 'comment': ""},
+            ]))
+        self.mocker.replay()
+        provider = MachineProvider("maasiv", CONFIG)
+        provider.maas_client = mock_client
+        cs = yield provider.get_constraint_set()
+        bear = cs.parse(["maas-tags=clawed, furry"])
+        bear.can_satisfy(bear)
+        err = self.assertRaises(ConstraintError,
+            cs.parse, ["maas-tags=furry, bouncy"])
+        self.assertEqual("Bad 'maas-tags' constraint 'furry, bouncy': "
+            "tag 'bouncy' does not exist",
+            str(err))

=== modified file 'juju/providers/maas/tests/testing.py'
--- juju/providers/maas/tests/testing.py	2012-05-25 06:49:32 +0000
+++ juju/providers/maas/tests/testing.py	2012-09-28 11:52:20 +0000
@@ -114,6 +114,9 @@
         # List some nodes.
         elif "nodes/?op=list_allocated&id=" in self.url:
             return self.list_some_nodes()
+        # List tags.
+        elif self.url.endswith("/tags/?op=list"):
+            return self.list_tags()
         # Not recognized.
         else:
             raise AssertionError("Unknown API method called")
@@ -161,6 +164,9 @@
     def release_node(self):
         return defer.succeed(json.dumps(NODE_JSON[0]))
 
+    def list_tags(self):
+        return defer.succeed("[]")
+
 
 class FakeMAASHTTPConnectionWithNoAvailableNodes(FakeMAASHTTPConnection):
     """Special version of L{FakeMAASHTTPConnection} that fakes that no nodes