Merge lp:~rvb/maas/agent-bug-1239488-acquire into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Change API's acquire() and release() so that they set/reset the agent_name field.

[Gavin Panella (allenap)]

Looks good, but I have some questions. Approved, but please consider
them before landing.

[1]

Node.agent_name is only ever set in Node.acquire() and cleared in
Node.release(). Node.acquire() sets the node's status to ALLOCATED,
and Node.release() to READY. However, a node /could/ also transition
to RETIRED or MISSING from ALLOCATED:

NODE_TRANSITIONS = {
    ...
    NODE_STATUS.ALLOCATED: [
        NODE_STATUS.READY,
        NODE_STATUS.RETIRED,
        NODE_STATUS.MISSING,
        ],

I'm not sure we're doing this anywhere, but my point is that nothing
is managing agent_name - or token or owner, other attributes set and
reset by acquire() and release() - in the event that a status
transition like this occurs.

Can we instead sort out these invariants in Node.clean()? For example,
Node.agent_name would always be "" if status != ALLOCATED.

[2]

+    def test_POST_release_resets_agent_name(self):
...
+    def test_POST_acquire_sets_agent_name(self):
...
+    def test_POST_acquire_agent_name_defaults_to_empty_string(self):

These are fine, but they're testing model code indirectly too. There
are no unit tests for the changes to the Node model. I think we ought
to nail down the behaviour of agent_name to the model code.

review: Approve

[Raphaël Badin (rvb)]

> Looks good, but I have some questions. Approved, but please consider
> them before landing.
>
>
> [1]
>
> Node.agent_name is only ever set in Node.acquire() and cleared in
> Node.release(). Node.acquire() sets the node's status to ALLOCATED,
> and Node.release() to READY. However, a node /could/ also transition
> to RETIRED or MISSING from ALLOCATED:
>
> NODE_TRANSITIONS = {
>    ...
>    NODE_STATUS.ALLOCATED: [
>        NODE_STATUS.READY,
>        NODE_STATUS.RETIRED,
>        NODE_STATUS.MISSING,
>        ],
>
> I'm not sure we're doing this anywhere, but my point is that nothing
> is managing agent_name - or token or owner, other attributes set and
> reset by acquire() and release() - in the event that a status
> transition like this occurs.
>
> Can we instead sort out these invariants in Node.clean()? For example,
> Node.agent_name would always be "" if status != ALLOCATED.

I think this is a very good point. I'd rather tackle that in a follow-up branch though. This is of no consequence now because we don't use NODE_STATUS.RETIRED or NODE_STATUS.MISSING.

> [2]
>
> +    def test_POST_release_resets_agent_name(self):
> ...
> +    def test_POST_acquire_sets_agent_name(self):
> ...
> +    def test_POST_acquire_agent_name_defaults_to_empty_string(self):
>
> These are fine, but they're testing model code indirectly too. There
> are no unit tests for the changes to the Node model. I think we ought
> to nail down the behaviour of agent_name to the model code.

Well, I also test the changed behavior of the model's code. See the changes to test_node.py.

test_POST_release_resets_agent_name is a test I /could/ remove… but I don't think a little bit of duplication hurts if it means thorough testing of the expected behavior.

[Gavin Panella (allenap)]

> > Can we instead sort out these invariants in Node.clean()? For example,
> > Node.agent_name would always be "" if status != ALLOCATED.
>
> I think this is a very good point.  I'd rather tackle that in a follow-up
> branch though.

Sure, that's fine.

> This is of no consequence now because we don't use
> NODE_STATUS.RETIRED or NODE_STATUS.MISSING.

Then we should remove them (in another branch). They're cognitive
baggage right now.

>
> > [2]
> >
> > +    def test_POST_release_resets_agent_name(self):
> > ...
> > +    def test_POST_acquire_sets_agent_name(self):
> > ...
> > +    def test_POST_acquire_agent_name_defaults_to_empty_string(self):
> >
> > These are fine, but they're testing model code indirectly too. There
> > are no unit tests for the changes to the Node model. I think we ought
> > to nail down the behaviour of agent_name to the model code.
>
> Well, I also test the changed behavior of the model's code.  See the changes
> to test_node.py.

So you have. I'm going code-blind :-/

>
> test_POST_release_resets_agent_name is a test I /could/ remove… but I don't
> think a little bit of duplication hurts if it means thorough testing of the
> expected behavior.

Yeah, that's fine.

[Gavin Panella (allenap)]

> Then we should remove them (in another branch). They're cognitive
> baggage right now.

Filed as bug 1240051.

[Julian Edwards (julian-edwards)]

> I'm not sure we're doing this anywhere, but my point is that nothing
> is managing agent_name - or token or owner, other attributes set and
> reset by acquire() and release() - in the event that a status
> transition like this occurs.

This has long bothered me.

I think we need a single place for state transitions, for example:

Node.transitionToStatus(NODE_STATUS.MISSING)

Then all the logic is in one place rather than littered around.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2013-10-07 09:12:40 +0000
+++ src/maasserver/api.py	2013-10-15 11:53:34 +0000
@@ -796,7 +796,10 @@
         :param not_connected_to: List of routers' MAC Addresses the returned
             node must not be connected to.
         :type connected_to: list of unicodes
-        """
+        :param agent_name: An optional agent name to attach to the
+            acquired node.
+        :type agent_name: unicode
+         """
         form = AcquireNodeForm(data=request.data)
         if form.is_valid():
             nodes = Node.objects.get_available_nodes_for_acquisition(
@@ -805,7 +808,10 @@
             node = get_first(nodes)
             if node is None:
                 raise NodesNotAvailable("No matching node is available.")
-            node.acquire(request.user, get_oauth_token(request))
+            agent_name = request.data.get('agent_name', '')
+            node.acquire(
+                request.user, get_oauth_token(request),
+                agent_name=agent_name)
             return node
         raise ValidationError(form.errors)
 

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2013-10-15 11:53:34 +0000
+++ src/maasserver/models/node.py	2013-10-15 11:53:34 +0000
@@ -770,12 +770,13 @@
                 power_params['mac_address'] = primary_mac.mac_address
         return power_params
 
-    def acquire(self, user, token=None):
+    def acquire(self, user, token=None, agent_name=''):
         """Mark commissioned node as acquired by the given user and token."""
         assert self.owner is None
         assert token is None or token.user == user
         self.status = NODE_STATUS.ALLOCATED
         self.owner = user
+        self.agent_name = agent_name
         self.token = token
         self.save()
 
@@ -785,6 +786,7 @@
         self.status = NODE_STATUS.READY
         self.owner = None
         self.token = None
+        self.agent_name = ''
         self.set_netboot()
         self.save()
 

=== modified file 'src/maasserver/models/tests/test_node.py'
--- src/maasserver/models/tests/test_node.py	2013-10-08 10:48:28 +0000
+++ src/maasserver/models/tests/test_node.py	2013-10-15 11:53:34 +0000
@@ -374,15 +374,21 @@
         node = factory.make_node(status=NODE_STATUS.READY)
         user = factory.make_user()
         token = create_auth_token(user)
-        node.acquire(user, token)
-        self.assertEqual(user, node.owner)
-        self.assertEqual(NODE_STATUS.ALLOCATED, node.status)
+        agent_name = factory.make_name('agent-name')
+        node.acquire(user, token, agent_name)
+        self.assertEqual(
+            (user, NODE_STATUS.ALLOCATED, agent_name),
+            (node.owner, node.status, node.agent_name))
 
     def test_release(self):
+        agent_name = factory.make_name('agent-name')
         node = factory.make_node(
-            status=NODE_STATUS.ALLOCATED, owner=factory.make_user())
+            status=NODE_STATUS.ALLOCATED, owner=factory.make_user(),
+            agent_name=agent_name)
         node.release()
-        self.assertEqual((NODE_STATUS.READY, None), (node.status, node.owner))
+        self.assertEqual(
+            (NODE_STATUS.READY, None, node.agent_name),
+            (node.status, node.owner, ''))
 
     def test_ip_addresses_queries_leases(self):
         node = factory.make_node()

=== modified file 'src/maasserver/tests/test_api_node.py'
--- src/maasserver/tests/test_api_node.py	2013-10-07 09:12:40 +0000
+++ src/maasserver/tests/test_api_node.py	2013-10-15 11:53:34 +0000
@@ -297,6 +297,15 @@
         self.client.post(self.get_node_uri(node), {'op': 'release'})
         self.assertEqual('', reload_object(node).distro_series)
 
+    def test_POST_release_resets_agent_name(self):
+        agent_name = factory.make_name('agent-name')
+        node = factory.make_node(
+            status=NODE_STATUS.ALLOCATED, owner=self.logged_in_user,
+            distro_series=factory.getRandomEnum(DISTRO_SERIES),
+            agent_name=agent_name)
+        self.client.post(self.get_node_uri(node), {'op': 'release'})
+        self.assertEqual('', reload_object(node).agent_name)
+
     def test_POST_release_removes_token_and_user(self):
         node = factory.make_node(status=NODE_STATUS.READY)
         self.client.post(self.get_uri('nodes/'), {'op': 'acquire'})

=== modified file 'src/maasserver/tests/test_api_nodes.py'
--- src/maasserver/tests/test_api_nodes.py	2013-10-07 09:12:40 +0000
+++ src/maasserver/tests/test_api_nodes.py	2013-10-15 11:53:34 +0000
@@ -374,6 +374,27 @@
         node = Node.objects.get(system_id=node.system_id)
         self.assertEqual(self.logged_in_user, node.owner)
 
+    def test_POST_acquire_sets_agent_name(self):
+        available_status = NODE_STATUS.READY
+        node = factory.make_node(
+            status=available_status, owner=None,
+            agent_name=factory.make_name('agent-name'))
+        agent_name = factory.make_name('agent-name')
+        self.client.post(
+            self.get_uri('nodes/'),
+            {'op': 'acquire', 'agent_name': agent_name})
+        node = Node.objects.get(system_id=node.system_id)
+        self.assertEqual(agent_name, node.agent_name)
+
+    def test_POST_acquire_agent_name_defaults_to_empty_string(self):
+        available_status = NODE_STATUS.READY
+        agent_name = factory.make_name('agent-name')
+        node = factory.make_node(
+            status=available_status, owner=None, agent_name=agent_name)
+        self.client.post(self.get_uri('nodes/'), {'op': 'acquire'})
+        node = Node.objects.get(system_id=node.system_id)
+        self.assertEqual('', node.agent_name)
+
     def test_POST_acquire_fails_if_no_node_present(self):
         # The "acquire" operation returns a Conflict error if no nodes
         # are available.