Merge lp:~rvb/maas/zone-placement into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Add a constraint to acquire nodes located in a particular availability zone.

The CSS fix is a simple drive-by fix to reduce the size of the search box on the node listing page.

[Jeroen T. Vermeulen (jtv)]

Not quite voting yet, because the diff isn't updating for some reason and I haven't seen the final branch yet.

As for the actual code I do see, two notes that apply in multiple places:

1. Where possible, please avoid implicit boolean conversions! Instead of “if zone:” say “if zone is not None:” or “if zone != '':” etc.

2. In several places you basically do:

    node = factory.make_node()
    zone = factory.make_zone(nodes=[node])

I think it would be just slightly more obvious to do...

    zone = factory.make_zone()
    node = factory.make_node(zone=zone)

[Jeroen T. Vermeulen (jtv)]

OK, the CSS fix looks trivially fine. Approved, with the two notes above.

review: Approve

[Raphaël Badin (rvb)]

> 1. Where possible, please avoid implicit boolean conversions! Instead of “if
> zone:” say “if zone is not None:” or “if zone != '':” etc.

I'm doing "if zone" here because '' and None really mean the same thing. And both are possible.

>
> 2. In several places you basically do:
>
> node = factory.make_node()
> zone = factory.make_zone(nodes=[node])
>
> I think it would be just slightly more obvious to do...
>
> zone = factory.make_zone()
> node = factory.make_node(zone=zone)

All right, fixed.

Thanks for the review!

[MAAS Lander (maas-lander)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2013-12-09 12:02:14 +0000
+++ src/maasserver/api.py	2013-12-11 13:09:41 +0000
@@ -812,6 +812,9 @@
         :param not_connected_to: List of routers' MAC Addresses the returned
             node must not be connected to.
         :type connected_to: list of unicodes
+        :param zone: An optional name for an availability zone the acquired
+            node should be located in.
+        :type zone: unicode
         :param agent_name: An optional agent name to attach to the
             acquired node.
         :type agent_name: unicode

=== modified file 'src/maasserver/node_constraint_filter_forms.py'
--- src/maasserver/node_constraint_filter_forms.py	2013-10-07 09:12:40 +0000
+++ src/maasserver/node_constraint_filter_forms.py	2013-12-11 13:09:41 +0000
@@ -31,7 +31,10 @@
     UnconstrainedMultipleChoiceField,
     ValidatorMultipleChoiceField,
     )
-from maasserver.models import Tag
+from maasserver.models import (
+    Tag,
+    Zone,
+    )
 from maasserver.utils.orm import (
     macs_contain,
     macs_do_not_contain,
@@ -148,6 +151,8 @@
             'invalid_list':
             "Invalid parameter: list of MAC addresses required."})
 
+    zone = forms.CharField(label="Availability zone", required=False)
+
     ignore_unknown_constraints = True
 
     @classmethod
@@ -189,6 +194,17 @@
             return tag_names
         return None
 
+    def clean_zone(self):
+        value = self.cleaned_data[self.get_field_name('zone')]
+        if value:
+            zone_names = Zone.objects.all().values_list('name', flat=True)
+            if value not in zone_names:
+                error_msg = "No such zone: '%s'." % value
+                raise ValidationError(
+                    {self.get_field_name('zone'): [error_msg]})
+            return value
+        return None
+
     def clean(self):
         if not self.ignore_unknown_constraints:
             unknown_constraints = set(
@@ -239,6 +255,11 @@
             for tag in tags:
                 filtered_nodes = filtered_nodes.filter(tags__name=tag)
 
+        # Filter by zone.
+        zone = self.cleaned_data.get(self.get_field_name('zone'))
+        if zone:
+            filtered_nodes = filtered_nodes.filter(zone=zone)
+
         # Filter by connected_to.
         connected_to = self.cleaned_data.get(
             self.get_field_name('connected_to'))

=== modified file 'src/maasserver/static/css/components/search_box.css'
--- src/maasserver/static/css/components/search_box.css	2013-11-15 18:15:51 +0000
+++ src/maasserver/static/css/components/search_box.css	2013-12-11 13:09:41 +0000
@@ -14,7 +14,7 @@
     position: absolute;
     top: 0;
     left: 0;
-    width: 450px;
+    width: 420px;
     padding-left: 22px;
     background-color: #fff;
     }

=== modified file 'src/maasserver/tests/test_api_nodes.py'
--- src/maasserver/tests/test_api_nodes.py	2013-12-03 17:46:05 +0000
+++ src/maasserver/tests/test_api_nodes.py	2013-12-11 13:09:41 +0000
@@ -622,6 +622,27 @@
         response_json = json.loads(response.content)
         self.assertEqual(node_tag_names, response_json['tag_names'])
 
+    def test_POST_acquire_allocates_node_by_zone(self):
+        factory.make_node(status=NODE_STATUS.READY)
+        zone = factory.make_zone()
+        node = factory.make_node(status=NODE_STATUS.READY, zone=zone)
+        response = self.client.post(reverse('nodes_handler'), {
+            'op': 'acquire',
+            'zone': zone.name,
+        })
+        self.assertResponseCode(httplib.OK, response)
+        response_json = json.loads(response.content)
+        self.assertEqual(node.system_id, response_json['system_id'])
+
+    def test_POST_acquire_allocates_node_by_zone_fails_if_no_node(self):
+        factory.make_node(status=NODE_STATUS.READY)
+        zone = factory.make_zone()
+        response = self.client.post(reverse('nodes_handler'), {
+            'op': 'acquire',
+            'zone': zone.name,
+        })
+        self.assertResponseCode(httplib.CONFLICT, response)
+
     def test_POST_acquire_allocates_node_by_tags_comma_separated(self):
         node = factory.make_node(status=NODE_STATUS.READY)
         node_tag_names = ["fast", "stable", "cute"]

=== modified file 'src/maasserver/tests/test_node_constraint_filter_forms.py'
--- src/maasserver/tests/test_node_constraint_filter_forms.py	2013-12-03 10:27:44 +0000
+++ src/maasserver/tests/test_node_constraint_filter_forms.py	2013-12-11 13:09:41 +0000
@@ -230,6 +230,28 @@
                 ["Invalid parameter: list of MAC addresses required."]}),
             (form.is_valid(), form.errors))
 
+    def test_zone(self):
+        node1 = factory.make_node()
+        node2 = factory.make_node()
+        node3 = factory.make_node()
+        zone1 = factory.make_zone(nodes=[node1, node2])
+        zone2 = factory.make_zone()
+
+        self.assertConstrainedNodes(
+            [node1, node2], {'zone': zone1.name})
+        self.assertConstrainedNodes(
+            [node1, node2, node3], {'zone': ''})
+        self.assertConstrainedNodes(
+            [node1, node2, node3], {})
+        self.assertConstrainedNodes(
+            [], {'zone': zone2.name})
+
+    def test_invalid_zone(self):
+        form = AcquireNodeForm(data={'zone': 'unknown'})
+        self.assertEquals(
+            (False, {'zone': ["No such zone: 'unknown'."]}),
+            (form.is_valid(), form.errors))
+
     def test_tags(self):
         tag_big = factory.make_tag(name='big')
         tag_burly = factory.make_tag(name='burly')