launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06932
[Merge] lp:~jtv/maas/accept-node-api into lp:maas
Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/accept-node-api into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~jtv/maas/accept-node-api/+merge/99914
When nodes are anonymously enlisted, they are held in the Declared state until a MAAS user verifies that they have been correctly enlisted. Don't want to start wiping those disks if they're not!
This branch extends the “nodes/” API with a call that accepts enlistment of any number of nodes. It returns the nodes that were actually moved out of the Declared state; if a node is already accepted (i.e. either Commissioning or Ready) then accepting it does nothing, and does not return it.
The new call is on the nodes resource, rather than an individual node, to permit this to scale without getting chatty. However its implementation on the model does act on a single Node. This _could_ have been done by SQL for serious scalability, but if in the future this call kicks off commissioning, we'll have to do more python-side work on each individual node anyway.
Along the way, this branch separates two stages of “creating a node”: a node is always created as Declared, but non-anonymous enlistment then accepts the enlistment so that the node moves on to Ready. This is a bit cleaner than setting an initial status (which the provisional implementation did) and again, makes room for additional work that the acceptance step will need to do in the future.
Oh, why the distinction between declared, already accepted, and not acceptable, you ask? “Already accepted” states are tolerated to minimize the API's sensitivity to concurrency. “This request requires no work” is not good grounds for raising an error. It's probably just somebody else doing the same work concurrently — a useful thing to indicate perhaps, but we want to see that in the listings of Declared nodes, not as errors while you're trying to work through the backlog.
The un-acceptable states are different. If you try to accept a node that is already assigned a role, or one that is broken or missing, that suggests some kind of misunderstanding that you should be aware of: you're basically telling the system it can start wiping the node's disks when really it can't or shouldn't. In the accepted states, that permission is already given.
That's also why the already-accepted states are explicitly listed in the code and tests, whereas the non-acceptable states are coded as “all other states.” When we define a new node status, the safe assumption is that it's not a state where attempts to accept enlistment can be quietly tolerated.
--
https://code.launchpad.net/~jtv/maas/accept-node-api/+merge/99914
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/accept-node-api into lp:maas.
=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py 2012-03-30 03:20:41 +0000
+++ src/maasserver/api.py 2012-03-30 04:53:19 +0000
@@ -92,6 +92,7 @@
NodesNotAvailable,
NodeStateViolation,
PermissionDenied,
+ Unauthorized,
)
from maasserver.fields import validate_mac
from maasserver.forms import NodeWithMACAddressesForm
@@ -398,12 +399,12 @@
return node
-def create_node(request, initial_status):
+def create_node(request):
"""Service an http request to create a node.
+ The node will be in the Declared state.
+
:param request: The http request for this node to be created.
- :param initial_status: The state the new node should be in.
- :type initial_status: A value from enum :class:`NODE_STATUS`.
:return: A suitable return value for the handler receiving the "new"
request that this implements.
:rtype: :class:`maasserver.models.Node` or
@@ -412,7 +413,7 @@
form = NodeWithMACAddressesForm(request.data)
if form.is_valid():
node = form.save()
- node.status = initial_status
+ node.status = NODE_STATUS.DECLARED
node.save()
return node
else:
@@ -435,7 +436,12 @@
therefore, the node is held in the "Declared" state for approval by a
MAAS user.
"""
- return create_node(request, NODE_STATUS.DECLARED)
+ return create_node(request)
+
+ @api_exported('accept', 'POST')
+ def accept(self, request):
+ """Accept a node's enlistment: not allowed to anonymous users."""
+ raise Unauthorized("You must be logged in to accept nodes.")
@classmethod
def resource_uri(cls, *args, **kwargs):
@@ -470,7 +476,30 @@
When a node has been added to MAAS by a logged-in MAAS user, it is
ready for allocation to services running on the MAAS.
"""
- return create_node(request, NODE_STATUS.READY)
+ node = create_node(request)
+ node.accept_enlistment()
+ return node
+
+ @api_exported('accept', 'POST')
+ def accept(self, request):
+ """Accept declared nodes into the MAAS.
+
+ Nodes can be enlisted in the MAAS anonymously, as opposed to by a
+ logged-in user, at the nodes' own request. These nodes are held in
+ the Declared state; a MAAS user must first verify the authenticity of
+ these enlistments, and accept them.
+
+ Enlistments can be accepted en masse, by passing multiple nodes to
+ this call. Accepting an already accepted node is not an error, but
+ accepting one that is already allocated, broken, etc. is.
+ """
+ system_ids = set(request.POST.getlist('node'))
+ nodes = Node.objects.filter(system_id__in=system_ids)
+ found_ids = set(node.system_id for node in nodes)
+ if len(nodes) < len(system_ids):
+ raise MAASAPIBadRequest(
+ "Unknown node(s): %s" % ', '.join(system_ids - found_ids))
+ return filter(None, [node.accept_enlistment() for node in nodes])
@api_exported('list', 'GET')
def list(self, request):
=== modified file 'src/maasserver/models.py'
--- src/maasserver/models.py 2012-03-29 11:40:39 +0000
+++ src/maasserver/models.py 2012-03-30 04:53:19 +0000
@@ -51,6 +51,7 @@
from django.utils.safestring import mark_safe
from maasserver.exceptions import (
CannotDeleteUserException,
+ NodeStateViolation,
PermissionDenied,
)
from maasserver.fields import (
@@ -468,6 +469,34 @@
if mac:
mac.delete()
+ def accept_enlistment(self):
+ """Accept this node's (anonymous) enlistment.
+
+ This call makes sense only on a node in Declared state, i.e. one that
+ has been anonymously enlisted and is now waiting for a MAAS user to
+ accept that enlistment as authentic. Calling it on a node that is in
+ Ready or Commissioning state, however, is not an error -- it probably
+ just means that somebody else has beaten you to it.
+
+ :return: This node if it has made the transition from Declared, or
+ None if it was already in an accepted state.
+ """
+ # Accepting a node puts it into Ready state. This will change
+ # once we implement commissioning.
+ target_state = NODE_STATUS.READY
+
+ accepted_states = [NODE_STATUS.READY, NODE_STATUS.COMMISSIONING]
+ if self.status in accepted_states:
+ return None
+ if self.status != NODE_STATUS.DECLARED:
+ raise NodeStateViolation(
+ "Cannot accept node enlistment: node %s is in state %s."
+ % (self.system_id, NODE_STATUS_CHOICES_DICT[self.status]))
+
+ self.status = target_state
+ self.save()
+ return self
+
def delete(self):
# Delete the related mac addresses first.
self.macaddress_set.all().delete()
=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py 2012-03-30 03:20:41 +0000
+++ src/maasserver/tests/test_api.py 2012-03-30 04:53:19 +0000
@@ -35,6 +35,7 @@
MACAddress,
Node,
NODE_STATUS,
+ NODE_STATUS_CHOICES_DICT,
)
from maasserver.testing import (
reload_object,
@@ -317,6 +318,17 @@
self.assertIn('application/json', response['Content-Type'])
self.assertItemsEqual(['architecture'], parsed_result)
+ def test_POST_accept_not_allowed(self):
+ # An anonymous user is not allowed to accept an anonymously
+ # enlisted node. That would defeat the whole purpose of holding
+ # those nodes for approval.
+ node_id = factory.make_node(status=NODE_STATUS.DECLARED).system_id
+ response = self.client.post(
+ self.get_uri('nodes/'), {'op': 'accept', 'node': [node_id]})
+ self.assertEqual(
+ (httplib.UNAUTHORIZED, "You must be logged in to accept nodes."),
+ (response.status_code, response.content))
+
class NodeAnonAPITest(APIv10TestMixin, TestCase):
@@ -928,6 +940,107 @@
oauth_key = self.client.token.key
self.assertEqual(oauth_key, node.token.key)
+ def test_POST_accept_gets_node_out_of_declared_state(self):
+ # This will change when we add provisioning. Until then,
+ # acceptance gets a node straight to Ready state.
+ target_state = NODE_STATUS.READY
+
+ node = factory.make_node(status=NODE_STATUS.DECLARED)
+ response = self.client.post(
+ self.get_uri('nodes/'),
+ {'op': 'accept', 'node': [node.system_id]})
+ accepted_ids = [
+ accepted_node['system_id']
+ for accepted_node in json.loads(response.content)]
+ self.assertEqual(
+ (httplib.OK, [node.system_id]),
+ (response.status_code, accepted_ids))
+ self.assertEqual(target_state, reload_object(node).status)
+
+ def test_POST_quietly_accepts_empty_set(self):
+ response = self.client.post(self.get_uri('nodes/'), {'op': 'accept'})
+ self.assertEqual(
+ (httplib.OK, "[]"), (response.status_code, response.content))
+
+ def test_POST_accept_rejects_impossible_state_changes(self):
+ acceptable_states = set([
+ NODE_STATUS.DECLARED,
+ NODE_STATUS.COMMISSIONING,
+ NODE_STATUS.READY,
+ ])
+ unacceptable_states = (
+ set(map_enum(NODE_STATUS).values()) - acceptable_states)
+ nodes = {
+ status: factory.make_node(status=status)
+ for status in unacceptable_states}
+ responses = {
+ status: self.client.post(
+ self.get_uri('nodes/'), {
+ 'op': 'accept',
+ 'node': node.system_id,
+ })
+ for status, node in nodes.items()}
+ # All of these attempts are rejected with Conflict errors.
+ self.assertEqual(
+ {status: httplib.CONFLICT for status in unacceptable_states},
+ {
+ status: responses[status].status_code
+ for status in unacceptable_states})
+ # Each error describes the problem.
+ for response in responses.values():
+ self.assertIn("Cannot accept node enlistment", response.content)
+ # Each error names the node it encountered a problem with.
+ for status, response in responses.items():
+ self.assertIn(nodes[status].system_id, response.content)
+ # Each error names the node state that the request conflicted
+ # with.
+ for status, response in responses.items():
+ self.assertIn(NODE_STATUS_CHOICES_DICT[status], response.content)
+
+ def test_POST_accept_fails_if_node_does_not_exist(self):
+ node_id = factory.getRandomString()
+ response = self.client.post(
+ self.get_uri('nodes/'), {'op': 'accept', 'node': node_id})
+ self.assertEqual(
+ (httplib.BAD_REQUEST, "Unknown node(s): %s" % node_id),
+ (response.status_code, response.content))
+
+ def test_POST_accept_accepts_multiple_nodes(self):
+ # This will change when we add provisioning. Until then,
+ # acceptance gets a node straight to Ready state.
+ target_state = NODE_STATUS.READY
+
+ nodes = [
+ factory.make_node(status=NODE_STATUS.DECLARED)
+ for counter in range(2)]
+ node_ids = [node.system_id for node in nodes]
+ response = self.client.post(self.get_uri('nodes/'), {
+ 'op': 'accept',
+ 'node': node_ids,
+ })
+ self.assertEqual(httplib.OK, response.status_code)
+ self.assertEqual(
+ [target_state] * len(nodes),
+ [reload_object(node).status for node in nodes])
+
+ def test_POST_accept_returns_actually_accepted_nodes(self):
+ acceptable_nodes = [
+ factory.make_node(status=NODE_STATUS.DECLARED)
+ for counter in range(2)
+ ]
+ accepted_node = factory.make_node(status=NODE_STATUS.READY)
+ nodes = acceptable_nodes + [accepted_node]
+ response = self.client.post(self.get_uri('nodes/'), {
+ 'op': 'accept',
+ 'node': [node.system_id for node in nodes],
+ })
+ self.assertEqual(httplib.OK, response.status_code)
+ accepted_ids = [
+ node['system_id'] for node in json.loads(response.content)]
+ self.assertItemsEqual(
+ [node.system_id for node in acceptable_nodes], accepted_ids)
+ self.assertNotIn(accepted_node.system_id, accepted_ids)
+
class MACAddressAPITest(APITestCase):
=== modified file 'src/maasserver/tests/test_models.py'
--- src/maasserver/tests/test_models.py 2012-03-29 13:19:40 +0000
+++ src/maasserver/tests/test_models.py 2012-03-30 04:53:19 +0000
@@ -25,6 +25,7 @@
from fixtures import TestWithFixtures
from maasserver.exceptions import (
CannotDeleteUserException,
+ NodeStateViolation,
PermissionDenied,
)
from maasserver.models import (
@@ -188,6 +189,61 @@
node.release()
self.assertEqual((NODE_STATUS.READY, None), (node.status, node.owner))
+ def test_accept_enlistment_gets_node_out_of_declared_state(self):
+ # If called on a node in Declared state, accept_enlistment()
+ # changes the node's status, and returns the node.
+
+ # This will change when we add commissioning. Until then,
+ # acceptance gets a node straight to Ready state.
+ target_state = NODE_STATUS.READY
+
+ node = factory.make_node(status=NODE_STATUS.DECLARED)
+ return_value = node.accept_enlistment()
+ self.assertEqual((node, target_state), (return_value, node.status))
+
+ def test_accept_enlistment_does_nothing_if_already_accepted(self):
+ # If a node has already been accepted, but not assigned a role
+ # yet, calling accept_enlistment on it is meaningless but not an
+ # error. The method returns None in this case.
+ accepted_states = [
+ NODE_STATUS.COMMISSIONING,
+ NODE_STATUS.READY,
+ ]
+ nodes = {
+ status: factory.make_node(status=status)
+ for status in accepted_states}
+
+ return_values = {
+ status: node.accept_enlistment()
+ for status, node in nodes.items()}
+
+ self.assertEqual(
+ {status: None for status in accepted_states}, return_values)
+ self.assertEqual(
+ {status: status for status in accepted_states},
+ {status: node.status for status, node in nodes.items()})
+
+ def test_accept_enlistment_rejects_bad_state_change(self):
+ # If a node is neither Declared nor in one of the "accepted"
+ # states where acceptance is a safe no-op, accept_enlistment
+ # raises a node state violation and leaves the node's state
+ # unchanged.
+ all_states = map_enum(NODE_STATUS).values()
+ acceptable_states = [
+ NODE_STATUS.DECLARED,
+ NODE_STATUS.COMMISSIONING,
+ NODE_STATUS.READY,
+ ]
+ unacceptable_states = set(all_states) - set(acceptable_states)
+ nodes = {
+ status: factory.make_node(status=status)
+ for status in unacceptable_states}
+ for node in nodes.values():
+ self.assertRaises(NodeStateViolation, node.accept_enlistment)
+ self.assertEqual(
+ {status: status for status in unacceptable_states},
+ {status: node.status for status, node in nodes.items()})
+
class NodeManagerTest(TestCase):
