Merge lp:~allenap/maas/lldp-repopulate-tags-after-commissioning into lp:~maas-committers/maas/trunk

[Gavin Panella]

Repopulate tags when commissioning finishes.

Also removes some temporary shims used during testing, and replaces them with the new and tested inject_lshw_result().

[Jeroen T. Vermeulen (jtv)]

The advertised change looks good, and I like the use of sentinel. The documentation for inject_result() is a bit more self-referential than I would like, but that's easily fixed.

However I have strong doubts about this part:

171 === modified file 'src/metadataserver/api.py'
172 --- src/metadataserver/api.py 2013-10-07 16:34:08 +0000
173 +++ src/metadataserver/api.py 2013-10-07 16:34:08 +0000
174 @@ -252,11 +252,16 @@

177 # When moving to a terminal state, remove the allocation.
178 - if target_status is not None:
179 - node.owner = None
180 + node.owner = None

This looks significant: you're dropping the node's ownership when it starts commissioning, instead of when it finishes. That doesn't seem to belong in the branch as described. Is it deliberate? Were you trying to fix the owned-ready-nodes problem?

If it is deliberate, have you tried it in practice? I think it's worth at least a comment and possibly a test for the non-terminal signal() case. To be honest the change should have affected an existing test in the first place.

Right below that:

183 + # When moving to a successful terminal state, recalculate tags.
184 + from maasserver.populate_tags import populate_tags_for_single_node
185 + from maasserver.models.tag import Tag
186 + populate_tags_for_single_node(Tag.objects.all(), node)

Please make it a habit to keep the imports at the top of the function, so that there are no corner cases where tests might forget to exercise them.

.

In src/metadataserver/models/tests/test_commissioningscript.py, TestInjectResult uses factory.getRandomString() to create randomized names. Consider using factory.make_name() instead. It's specialized for identifier-style strings, and accepts a helpful prefix. The prefix makes the name recognizable in failure output, without making the full name deterministic.

.

As far as I can see, the tests for inject_lshw_result and inject_lldp_result would fail to notice an incompatible API change. Can you sprinkle that same mocking magic you used before, where the mock objects mimick the exact signatures of the originals?

review: Needs Fixing

[Gavin Panella (allenap)]

Thank you!

> The advertised change looks good, and I like the use of sentinel.  The
> documentation for inject_result() is a bit more self-referential than I would
> like, but that's easily fixed.

I've improved that docstring.

>
> However I have strong doubts about this part:
>
> 171     === modified file 'src/metadataserver/api.py'
> 172     --- src/metadataserver/api.py   2013-10-07 16:34:08 +0000
> 173     +++ src/metadataserver/api.py   2013-10-07 16:34:08 +0000
> 174     @@ -252,11 +252,16 @@
>
> 177              # When moving to a terminal state, remove the allocation.
> 178     -        if target_status is not None:
> 179     -            node.owner = None
> 180     +        node.owner = None
>
> This looks significant: you're dropping the node's ownership when it starts
> commissioning, instead of when it finishes.  That doesn't seem to belong in
> the branch as described.  Is it deliberate?  Were you trying to fix the owned-
> ready-nodes problem?
>
> If it is deliberate, have you tried it in practice?  I think it's worth at
> least a comment and possibly a test for the non-terminal signal() case.  To be
> honest the change should have affected an existing test in the first place.

If I add a bit more context you can see why it's harmless (or you can
point out where I'm mistaken):

        if target_status in (None, node.status):
            # No status change.  Nothing to be done.
            return rc.ALL_OK

        node.status = target_status
        # When moving to a terminal state, remove the allocation.
        if target_status is not None:
            node.owner = None

In the last conditional here, target_status will never be None,
because of the conditional further up.

>
> Right below that:
>
> 183     +        # When moving to a successful terminal state, recalculate
> tags.
> 184     +        from maasserver.populate_tags import
> populate_tags_for_single_node
> 185     +        from maasserver.models.tag import Tag
> 186     +        populate_tags_for_single_node(Tag.objects.all(), node)
>
> Please make it a habit to keep the imports at the top of the function, so that
> there are no corner cases where tests might forget to exercise them.

Oops, that was an omission rather than anything deliberate. When
coding I often pull in imports right where I need them to avoid
breaking concentration, but I sometimes forget to move them up to the
top later on. Thanks for spotting this.

>
> .
>
> In src/metadataserver/models/tests/test_commissioningscript.py,
> TestInjectResult uses factory.getRandomString() to create randomized names.
> Consider using factory.make_name() instead.  It's specialized for identifier-
> style strings, and accepts a helpful prefix.  The prefix makes the name
> recognizable in failure output, without making the full name deterministic.

Done. I'd forgotten about make_name().

>
> .
>
> As far as I can see, the tests for inject_lshw_result and inject_lldp_result
> would fail to notice an incompatible API change.  Can you sprinkle that same
> mocking magic you used before, where the mock objects mimick the exact
> signatures of the originals?

Done. Fwiw, it's mock.create_autospec().

[Jeroen T. Vermeulen (jtv)]

All good then, thanks!

For things like the forgotten temporary import, you may benefit from the habit of reviewing your own diff before submitting an MP. SmartBear's research with IBM/Rational (if memory serves) found a massive quality difference between branches that had been self-reviewed and branches that hadn't.

review: Approve

[MAAS Lander (maas-lander)]

No proposals found for merge of https://code.launchpad.net/~allenap/maas/pass-script-result-to-commissioning-hooks into https://code.launchpad.net/~maas-maintainers/maas/trunk.

=== modified file 'src/maasserver/models/tests/test_tag.py'
--- src/maasserver/models/tests/test_tag.py	2013-10-08 11:51:31 +0000
+++ src/maasserver/models/tests/test_tag.py	2013-10-09 10:11:44 +0000
@@ -18,28 +18,7 @@
 from maasserver.models.tag import Tag
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
-from metadataserver.fields import Bin
-from metadataserver.models.commissioningscript import (
-    LSHW_OUTPUT_NAME,
-    update_hardware_details,
-    )
-from metadataserver.models.nodecommissionresult import NodeCommissionResult
-
-
-def set_hardware_details(node, xmlbytes):
-    # FIXME: Gavin Panella, bug=1235174, 2013-10-04
-    #
-    # This is a temporary shim to allow removal of
-    # Node.set_hardware_details(), and thus the use of the
-    # Node.hardware_details field.
-    #
-    # It is being replaced by storing the results only in
-    # NodeCommissionResult, and reprocessing tags using all
-    # relevant probed details.
-    assert isinstance(xmlbytes, bytes)
-    NodeCommissionResult.objects.store_data(
-        node, LSHW_OUTPUT_NAME, script_result=0, data=Bin(xmlbytes))
-    update_hardware_details(node, xmlbytes, 0)
+from metadataserver.models.commissioningscript import inject_lshw_result
 
 
 class TagTest(MAASServerTestCase):
@@ -87,18 +66,18 @@
 
     def test_applies_tags_to_nodes(self):
         node1 = factory.make_node()
-        set_hardware_details(node1, b'<node><child /></node>')
+        inject_lshw_result(node1, b'<node><child /></node>')
         node2 = factory.make_node()
-        set_hardware_details(node2, b'<node />')
+        inject_lshw_result(node2, b'<node />')
         tag = factory.make_tag(definition='//node/child')
         self.assertItemsEqual([tag.name], node1.tag_names())
         self.assertItemsEqual([], node2.tag_names())
 
     def test_removes_old_values(self):
         node1 = factory.make_node()
-        set_hardware_details(node1, b'<node><foo /></node>')
+        inject_lshw_result(node1, b'<node><foo /></node>')
         node2 = factory.make_node()
-        set_hardware_details(node2, b'<node><bar /></node>')
+        inject_lshw_result(node2, b'<node><bar /></node>')
         tag = factory.make_tag(definition='//node/foo')
         self.assertItemsEqual([tag.name], node1.tag_names())
         self.assertItemsEqual([], node2.tag_names())
@@ -114,9 +93,9 @@
 
     def test_doesnt_touch_other_tags(self):
         node1 = factory.make_node()
-        set_hardware_details(node1, b'<node><foo /></node>')
+        inject_lshw_result(node1, b'<node><foo /></node>')
         node2 = factory.make_node()
-        set_hardware_details(node2, b'<node><bar /></node>')
+        inject_lshw_result(node2, b'<node><bar /></node>')
         tag1 = factory.make_tag(definition='//node/foo')
         self.assertItemsEqual([tag1.name], node1.tag_names())
         self.assertItemsEqual([], node2.tag_names())
@@ -126,7 +105,7 @@
 
     def test_rollsback_invalid_xpath(self):
         node = factory.make_node()
-        set_hardware_details(node, b'<node><foo /></node>')
+        inject_lshw_result(node, b'<node><foo /></node>')
         tag = factory.make_tag(definition='//node/foo')
         self.assertItemsEqual([tag.name], node.tag_names())
         tag.definition = 'invalid::tag'

=== modified file 'src/maasserver/tests/test_api_tag.py'
--- src/maasserver/tests/test_api_tag.py	2013-10-08 11:51:31 +0000
+++ src/maasserver/tests/test_api_tag.py	2013-10-09 10:11:44 +0000
@@ -27,28 +27,7 @@
     )
 from maasserver.testing.factory import factory
 from maasserver.testing.oauthclient import OAuthAuthenticatedClient
-from metadataserver.fields import Bin
-from metadataserver.models.commissioningscript import (
-    LSHW_OUTPUT_NAME,
-    update_hardware_details,
-    )
-from metadataserver.models.nodecommissionresult import NodeCommissionResult
-
-
-def set_hardware_details(node, xmlbytes):
-    # FIXME: Gavin Panella, bug=1235174, 2013-10-04
-    #
-    # This is a temporary shim to allow removal of
-    # Node.set_hardware_details(), and thus the use of the
-    # Node.hardware_details field.
-    #
-    # It is being replaced by storing the results only in
-    # NodeCommissionResult, and reprocessing tags using all
-    # relevant probed details.
-    assert isinstance(xmlbytes, bytes)
-    NodeCommissionResult.objects.store_data(
-        node, LSHW_OUTPUT_NAME, script_result=0, data=Bin(xmlbytes))
-    update_hardware_details(node, xmlbytes, 0)
+from metadataserver.models.commissioningscript import inject_lshw_result
 
 
 class TestTagAPI(APITestCase):
@@ -117,9 +96,9 @@
 
     def test_PUT_updates_node_associations(self):
         node1 = factory.make_node()
-        set_hardware_details(node1, b'<node><foo/></node>')
+        inject_lshw_result(node1, b'<node><foo/></node>')
         node2 = factory.make_node()
-        set_hardware_details(node2, b'<node><bar/></node>')
+        inject_lshw_result(node2, b'<node><bar/></node>')
         tag = factory.make_tag(definition='//node/foo')
         self.assertItemsEqual([tag.name], node1.tag_names())
         self.assertItemsEqual([], node2.tag_names())
@@ -155,9 +134,9 @@
     def test_GET_nodes_hides_invisible_nodes(self):
         user2 = factory.make_user()
         node1 = factory.make_node()
-        set_hardware_details(node1, b'<node><foo/></node>')
+        inject_lshw_result(node1, b'<node><foo/></node>')
         node2 = factory.make_node(status=NODE_STATUS.ALLOCATED, owner=user2)
-        set_hardware_details(node2, b'<node><bar/></node>')
+        inject_lshw_result(node2, b'<node><bar/></node>')
         tag = factory.make_tag(definition='//node')
         response = self.client.get(self.get_tag_uri(tag), {'op': 'nodes'})
 
@@ -176,7 +155,7 @@
     def test_PUT_invalid_definition(self):
         self.become_admin()
         node = factory.make_node()
-        set_hardware_details(node, b'<node ><child/></node>')
+        inject_lshw_result(node, b'<node ><child/></node>')
         tag = factory.make_tag(definition='//child')
         self.assertItemsEqual([tag.name], node.tag_names())
         response = self.client_put(
@@ -336,9 +315,9 @@
         tag = factory.make_tag(definition='//foo/bar')
         # Only one node matches the tag definition, rebuilding should notice
         node_matching = factory.make_node()
-        set_hardware_details(node_matching, b'<foo><bar/></foo>')
+        inject_lshw_result(node_matching, b'<foo><bar/></foo>')
         node_bogus = factory.make_node()
-        set_hardware_details(node_bogus, b'<foo/>')
+        inject_lshw_result(node_bogus, b'<foo/>')
         node_matching.tags.add(tag)
         node_bogus.tags.add(tag)
         self.assertItemsEqual(
@@ -480,10 +459,10 @@
     def test_POST_new_populates_nodes(self):
         self.become_admin()
         node1 = factory.make_node()
-        set_hardware_details(node1, b'<node><child/></node>')
+        inject_lshw_result(node1, b'<node><child/></node>')
         # Create another node that doesn't have a 'child'
         node2 = factory.make_node()
-        set_hardware_details(node2, b'<node/>')
+        inject_lshw_result(node2, b'<node/>')
         self.assertItemsEqual([], node1.tag_names())
         self.assertItemsEqual([], node2.tag_names())
         name = factory.getRandomString()

=== modified file 'src/metadataserver/api.py'
--- src/metadataserver/api.py	2013-10-07 15:19:41 +0000
+++ src/metadataserver/api.py	2013-10-09 10:11:44 +0000
@@ -51,6 +51,8 @@
     Node,
     SSHKey,
     )
+from maasserver.models.tag import Tag
+from maasserver.populate_tags import populate_tags_for_single_node
 from maasserver.preseed import (
     get_curtin_userdata,
     get_enlist_preseed,
@@ -252,11 +254,14 @@
 
         node.status = target_status
         # When moving to a terminal state, remove the allocation.
-        if target_status is not None:
-            node.owner = None
+        node.owner = None
         node.error = request.POST.get('error', '')
+
+        # When moving to a successful terminal state, recalculate tags.
+        populate_tags_for_single_node(Tag.objects.all(), node)
+
+        # Done.
         node.save()
-
         return rc.ALL_OK
 
     @operation(idempotent=False)

=== modified file 'src/metadataserver/models/commissioningscript.py'
--- src/metadataserver/models/commissioningscript.py	2013-10-08 13:17:51 +0000
+++ src/metadataserver/models/commissioningscript.py	2013-10-09 10:11:44 +0000
@@ -16,6 +16,9 @@
 __all__ = [
     'BUILTIN_COMMISSIONING_SCRIPTS',
     'CommissioningScript',
+    'inject_lldp_result',
+    'inject_lshw_result',
+    'inject_result',
     'LLDP_OUTPUT_NAME',
     'LSHW_OUTPUT_NAME',
     ]
@@ -41,7 +44,11 @@
 from maasserver.fields import MAC
 from maasserver.models.tag import Tag
 from metadataserver import DefaultMeta
-from metadataserver.fields import BinaryField
+from metadataserver.fields import (
+    Bin,
+    BinaryField,
+    )
+from metadataserver.models.nodecommissionresult import NodeCommissionResult
 
 
 logger = logging.getLogger(__name__)
@@ -436,3 +443,28 @@
 
     name = CharField(max_length=255, null=False, editable=True, unique=True)
     content = BinaryField(null=False)
+
+
+def inject_result(node, name, output, exit_status=0):
+    """Inject a `name` result and trigger related hooks, if any.
+
+    `output` and `exit_status` are recorded as `NodeCommissionResult`
+    instances with the `name` given. A built-in hook is then searched
+    for; if found, it is invoked.
+    """
+    assert isinstance(output, bytes)
+    NodeCommissionResult.objects.store_data(
+        node, name, script_result=exit_status, data=Bin(output))
+    if name in BUILTIN_COMMISSIONING_SCRIPTS:
+        postprocess_hook = BUILTIN_COMMISSIONING_SCRIPTS[name]['hook']
+        postprocess_hook(node=node, output=output, exit_status=exit_status)
+
+
+def inject_lshw_result(node, output, exit_status=0):
+    """Convenience to call `inject_result(name=LSHW_OUTPUT_NAME, ...)`."""
+    return inject_result(node, LSHW_OUTPUT_NAME, output, exit_status)
+
+
+def inject_lldp_result(node, output, exit_status=0):
+    """Convenience to call `inject_result(name=LLDP_OUTPUT_NAME, ...)`."""
+    return inject_result(node, LLDP_OUTPUT_NAME, output, exit_status)

=== modified file 'src/metadataserver/models/tests/test_commissioningscript.py'
--- src/metadataserver/models/tests/test_commissioningscript.py	2013-10-08 13:43:12 +0000
+++ src/metadataserver/models/tests/test_commissioningscript.py	2013-10-09 10:11:44 +0000
@@ -50,14 +50,28 @@
 from metadataserver.models.commissioningscript import (
     ARCHIVE_PREFIX,
     extract_router_mac_addresses,
+    inject_lldp_result,
+    inject_lshw_result,
+    inject_result,
+    LLDP_OUTPUT_NAME,
+    LSHW_OUTPUT_NAME,
     make_function_call_script,
     set_node_routers,
     set_virtual_tag,
     update_hardware_details,
     )
-from mock import call
+from metadataserver.models.nodecommissionresult import NodeCommissionResult
+from mock import (
+    call,
+    create_autospec,
+    Mock,
+    sentinel,
+    )
 from testtools.content import text_content
-from testtools.matchers import DocTestMatches
+from testtools.matchers import (
+    DocTestMatches,
+    MatchesStructure,
+    )
 
 
 def open_tarfile(content):
@@ -332,6 +346,56 @@
         self.assertItemsEqual(routers_before, routers_after)
 
 
+class TestInjectResult(MAASServerTestCase):
+
+    def test_inject_result_stores_data(self):
+        node = factory.make_node()
+        name = factory.make_name("result")
+        output = factory.getRandomBytes()
+        exit_status = next(factory.random_octets)
+
+        inject_result(node, name, output, exit_status)
+
+        self.assertThat(
+            NodeCommissionResult.objects.get(node=node, name=name),
+            MatchesStructure.byEquality(
+                node=node, name=name, script_result=exit_status,
+                data=output))
+
+    def test_inject_result_calls_hook(self):
+        node = factory.make_node()
+        name = factory.make_name("result")
+        output = factory.getRandomBytes()
+        exit_status = next(factory.random_octets)
+        hook = Mock()
+        self.patch(
+            cs_module, "BUILTIN_COMMISSIONING_SCRIPTS",
+            {name: {"hook": hook}})
+
+        inject_result(node, name, output, exit_status)
+
+        hook.assert_called_once_with(
+            node=node, output=output, exit_status=exit_status)
+
+    def inject_lshw_result(self):
+        # inject_lshw_result() just calls through to inject_result().
+        inject_result = self.patch(
+            cs_module, "inject_result",
+            create_autospec(cs_module.inject_result))
+        inject_lshw_result(sentinel.node, sentinel.output, sentinel.status)
+        inject_result.assert_called_once_with(
+            sentinel.node, LSHW_OUTPUT_NAME, sentinel.output, sentinel.status)
+
+    def inject_lldp_result(self):
+        # inject_lldp_result() just calls through to inject_result().
+        inject_result = self.patch(
+            cs_module, "inject_result",
+            create_autospec(cs_module.inject_result))
+        inject_lldp_result(sentinel.node, sentinel.output, sentinel.status)
+        inject_result.assert_called_once_with(
+            sentinel.node, LLDP_OUTPUT_NAME, sentinel.output, sentinel.status)
+
+
 class TestSetVirtualTag(MAASServerTestCase):
 
     def getVirtualTag(self):

=== modified file 'src/metadataserver/tests/test_api.py'
--- src/metadataserver/tests/test_api.py	2013-10-07 15:19:41 +0000
+++ src/metadataserver/tests/test_api.py	2013-10-09 10:11:44 +0000
@@ -436,6 +436,17 @@
         self.assertEqual(
             NODE_STATUS.COMMISSIONING, reload_object(node).status)
 
+    def test_signaling_commissioning_OK_repopulates_tags(self):
+        populate_tags_for_single_node = self.patch(
+            api, "populate_tags_for_single_node")
+        node = factory.make_node(status=NODE_STATUS.COMMISSIONING)
+        client = make_node_client(node)
+        response = call_signal(client, status='OK', script_result='0')
+        self.assertEqual(httplib.OK, response.status_code)
+        self.assertEqual(NODE_STATUS.READY, reload_object(node).status)
+        from mock import ANY
+        populate_tags_for_single_node.assert_called_once_with(ANY, node)
+
     def test_signaling_requires_status_code(self):
         node = factory.make_node(status=NODE_STATUS.COMMISSIONING)
         client = make_node_client(node=node)