Merge lp:~racb/maas/arch-constraint-wildcard into lp:maas/trunk

[Robie Basak]

Add automatic architecture constraint wildcarding and arm alias

[Martin Packman (gz)]

Looks good.

The generate_architecture_wildcards is very cute, I'm not totally convinced it's needed versus just making someone type that out next to the arch enum, but as you'll be the one who has to add arm flavours it's fair enough to save yourself the bother. :)

Some small notes:

+ # Replace an alias with its value if it is an alias
+ try:
+ aliased_value = primary_architecture_aliases[value]
+ except KeyError:
+ aliased_value = value

This is not an exceptional circumstance, so I'd use the following (still non-recursive) spelling instead:

    aliased_value = primary_architecture_aliases.get(value, value)

+ if aliased_value in (choice[0] for choice in ARCHITECTURE_CHOICES):

Rather than making a generator on the fly here to test membership, I'd do either:
* Add the full form as a mapping to a 1-tuple of itself in generate_architecture_wildcards and use the block below only
* Create an ARCHITECTURE_CHOICES_DICT in maasserver.enum as per other objects there and use `if aliased_value in ARCHITECTURE_CHOICES_DICT` instead.

I find test_generate_architecture_wildcards slightly hard to understand, a docstring, comment or more assertions might help.

review: Approve

[lp:~racb/maas/arch-constraint-wildcard]

1218. By Robie Basak on 2012-10-08

Post-review fixes

[Jeroen T. Vermeulen (jtv)]

Looks good, but I have some notes that I'd like you to address. Don't worry -- I usually do!

Notes follow.

.

In node_constraint_filter.py:

52 +def constrain_architecture(nodes, key, value):
53 + assert(key == 'architecture')

In python, "assert" is a statement:

    assert key == 'architecture'

This can matter because it takes a second argument, namely an error string that will be shown if the assertion fails:

    assert key == 'architecture', "This filter is for architecture only."

If you parenthesize as one might in C, things go mysteriously wrong:

    assert(key == 'architecture', "This filter is for architecture only.")

This creates a tuple of a boolean and a string, and then asserts that the tuple, not the boolean, evaluates to True. Not what you want! If you need a line break, the idiomatic way of doing it is:

    assert key == 'architecture', (
        "This filter is for architecture only.")

.

55 + # Replace an alias with its value if it is an alias
56 + try:
57 + aliased_value = primary_architecture_aliases[value]
58 + except KeyError:
59 + aliased_value = value

It seems misleading to write this such that the normal, successful, expected-to-be-fast code path is the one taking an exception. An exception conveys both to the reader and to the language implementation that something is out of the ordinary, that the intended code flow is different, and that it's probably worth sacrificing performance in favour of the non-exceptional code path even in extreme trade-offs.

So I think it would be clearer to write it thus:

    aliased_value = primary_architecture.aliases.get(value, value)

.

68 + else:
69 + raise InvalidConstraint(
70 + 'architecture', value, 'Architecture not recognised')

I would recommend "double quotes" for free-form text. This is not shell where you have to keep a constant eye on weird characters in double-quoted strings, and indeed the character most worth worrying about is the apostrophe. It'd be silly to change your quoting just because your sentence sprouted a contraction!

.

In test_api.py:

91 - def test_POST_acquire_treats_unknown_arch_as_resource_conflict(self):
92 + def test_POST_acquire_treats_unknown_arch_as_bad_request(self):
93 # Asking for an unknown arch returns an HTTP conflict

The comment still needs updating!

.

In test_node_constraint_filter.py:

124 + def test_generate_architecture_wildcards(self):
125 + single_subarch = factory.getRandomString(), factory.getRandomString()
126 + double_subarch_1 = factory.getRandomString(), factory.getRandomString()
127 + double_subarch_2 = double_subarch_1[0], factory.getRandomString()

Might I recommend factory.make_name()? Depending on the exact string you pass, factory.make_name('arch') is not going to be much longer than factory.getRandomString() but it produces a more recognizable string, which can be useful in failure output.

.

In the same test module, test_architecture is getting out of hand. There is little enough shared test setup that it doesn't seem worth the flexibility cost to bundle all these different behaviours and outcomes together into o...

review: Approve

[lp:~racb/maas/arch-constraint-wildcard]

1219. By Robie Basak on 2012-10-08

More post-review fixes from jtv's review

[Robie Basak (racb)]

Thanks for the reviews, Martin and Jeroen! Loads of improvements there. I've fixed all of these except for refactoring of the architecture constraint tests for which I'll submit a separate proposal, so I'll land this now.

[Robie Basak (racb)]

Separate proposal is https://code.launchpad.net/~racb/maas/refactor-arch-constraint-tests/+merge/128578

=== modified file 'src/maasserver/enum.py'
--- src/maasserver/enum.py	2012-10-02 17:53:24 +0000
+++ src/maasserver/enum.py	2012-10-08 17:35:23 +0000
@@ -13,6 +13,7 @@
 __all__ = [
     'ARCHITECTURE',
     'ARCHITECTURE_CHOICES',
+    'ARCHITECTURE_CHOICES_DICT',
     'COMPONENT',
     'NODEGROUP_STATUS',
     'NODEGROUP_STATUS_CHOICES',
@@ -127,6 +128,9 @@
 )
 
 
+ARCHITECTURE_CHOICES_DICT = OrderedDict(ARCHITECTURE_CHOICES)
+
+
 class DISTRO_SERIES:
     """List of supported ubuntu releases."""
     #:

=== modified file 'src/maasserver/models/node_constraint_filter.py'
--- src/maasserver/models/node_constraint_filter.py	2012-10-04 10:52:31 +0000
+++ src/maasserver/models/node_constraint_filter.py	2012-10-08 17:35:23 +0000
@@ -12,8 +12,13 @@
     'constrain_nodes',
     ]
 
+import itertools
 import math
 
+from maasserver.enum import (
+    ARCHITECTURE_CHOICES,
+    ARCHITECTURE_CHOICES_DICT,
+    )
 from maasserver.exceptions import (
     InvalidConstraint,
     )
@@ -51,12 +56,59 @@
     return nodes
 
 
+def generate_architecture_wildcards(choices=ARCHITECTURE_CHOICES):
+    """Map 'primary' architecture names to a list of full expansions.
+
+    Return a dictionary keyed by the primary architecture name (the part before
+    the '/'). The value of an entry is a frozenset of full architecture names
+    ('primary_arch/subarch') under the keyed primary architecture.
+
+    """
+
+    sorted_arch_list = sorted(choice[0] for choice in choices)
+
+    def extract_primary_arch(arch):
+        return arch.split('/')[0]
+
+    return {
+        primary_arch: frozenset(subarch_generator)
+        for primary_arch, subarch_generator in itertools.groupby(
+            sorted_arch_list, key=extract_primary_arch
+        )
+    }
+
+
+architecture_wildcards = generate_architecture_wildcards()
+
+
+# juju uses a general "arm" architecture constraint across all of its
+# providers. Since armhf is the cross-distro agreed Linux userspace
+# architecture and ABI and ARM servers are expected to only use armhf,
+# interpret "arm" to mean "armhf" in MAAS.
+architecture_wildcards['arm'] = architecture_wildcards['armhf']
+
+
+def constrain_architecture(nodes, key, value):
+    assert key == 'architecture', "This filter is for architecture only."
+
+    if value in ARCHITECTURE_CHOICES_DICT:
+        # Full 'arch/subarch' specified directly
+        return nodes.filter(architecture=value)
+    elif value in architecture_wildcards:
+        # Try to expand 'arch' to all available 'arch/subarch' matches
+        return nodes.filter(
+            architecture__in=architecture_wildcards[value])
+    else:
+        raise InvalidConstraint(
+            'architecture', value, "Architecture not recognised.")
+
+
 # this is the mapping of constraint names to how to apply the constraint
 constraint_filters = {
     # Currently architecture only supports exact matching. Eventually, we will
     # probably want more logic to note that eg, amd64 can be used for an i386
     # request
-    'architecture': constrain_identical,
+    'architecture': constrain_architecture,
     'hostname': constrain_identical,
     'cpu_count': constrain_int_greater_or_equal,
     'memory': constrain_int_greater_or_equal,

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-10-08 11:46:58 +0000
+++ src/maasserver/tests/test_api.py	2012-10-08 17:35:23 +0000
@@ -1914,14 +1914,14 @@
         response_json = json.loads(response.content)
         self.assertEqual(node.architecture, response_json['architecture'])
 
-    def test_POST_acquire_treats_unknown_arch_as_resource_conflict(self):
-        # Asking for an unknown arch returns an HTTP conflict
+    def test_POST_acquire_treats_unknown_arch_as_bad_request(self):
+        # Asking for an unknown arch returns an HTTP "400 Bad Request"
         factory.make_node(status=NODE_STATUS.READY)
         response = self.client.post(self.get_uri('nodes/'), {
             'op': 'acquire',
             'arch': 'sparc',
         })
-        self.assertEqual(httplib.CONFLICT, response.status_code)
+        self.assertEqual(httplib.BAD_REQUEST, response.status_code)
 
     def test_POST_acquire_allocates_node_by_cpu(self):
         # Asking for enough cpu acquires a node with at least that.

=== modified file 'src/maasserver/tests/test_node_constraint_filter.py'
--- src/maasserver/tests/test_node_constraint_filter.py	2012-10-04 10:52:31 +0000
+++ src/maasserver/tests/test_node_constraint_filter.py	2012-10-08 17:35:23 +0000
@@ -15,7 +15,10 @@
 from maasserver.enum import ARCHITECTURE
 from maasserver.exceptions import InvalidConstraint
 from maasserver.models import Node
-from maasserver.models.node_constraint_filter import constrain_nodes
+from maasserver.models.node_constraint_filter import (
+    constrain_nodes,
+    generate_architecture_wildcards,
+)
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import TestCase
 from maasserver.utils import ignore_unused
@@ -27,6 +30,29 @@
         nodes = constrain_nodes(Node.objects.all(), constraints)
         self.assertItemsEqual(expected_nodes, nodes)
 
+    def test_generate_architecture_wildcards(self):
+        # Create a test architecture choice list of one architecture that only
+        # has one available subarch (single_subarch) and two architectures that
+        # have a matching primary architecture (double_subarch_{1,2})
+        single_subarch = factory.make_name('arch'), factory.make_name('arch')
+        double_subarch_1 = factory.make_name('arch'), factory.make_name('arch')
+        double_subarch_2 = double_subarch_1[0], factory.make_name('arch')
+        choices = (
+            ('/'.join(single_subarch), None),
+            ('/'.join(double_subarch_1), None),
+            ('/'.join(double_subarch_2), None),
+        )
+
+        # single_subarch should end up in the dict essentially unchanged, and
+        # the double_subarchs should have been flattened into a single dict
+        # element with a list of them.
+        self.assertEquals({
+            single_subarch[0]: frozenset([choices[0][0]]),
+            double_subarch_1[0]: frozenset([choices[1][0], choices[2][0]]),
+            },
+            generate_architecture_wildcards(choices=choices)
+        )
+
     def test_no_constraints(self):
         node1 = factory.make_node()
         node2 = factory.make_node()
@@ -43,10 +69,18 @@
     def test_architecture(self):
         node1 = factory.make_node(architecture=ARCHITECTURE.i386)
         node2 = factory.make_node(architecture=ARCHITECTURE.armhf_highbank)
+        self.assertConstrainedNodes([node1], {'architecture': 'i386'})
         self.assertConstrainedNodes([node1], {'architecture': 'i386/generic'})
         self.assertConstrainedNodes(
+            [node2], {'architecture': 'arm'})
+        self.assertConstrainedNodes(
+            [node2], {'architecture': 'armhf'})
+        self.assertConstrainedNodes(
             [node2], {'architecture': 'armhf/highbank'})
-        self.assertConstrainedNodes([], {'architecture': 'sparc'})
+        self.assertRaises(InvalidConstraint,
+            self.assertConstrainedNodes, [], {'architecture': 'armhf/generic'})
+        self.assertRaises(InvalidConstraint,
+            self.assertConstrainedNodes, [], {'architecture': 'sparc'})
 
     def test_cpu_count(self):
         node1 = factory.make_node(cpu_count=1)