launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06366
[Merge] lp:~jtv/maas/stop-node into lp:maas
Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/stop-node into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~jtv/maas/stop-node/+merge/92960
This lets you stop (shut down) a node (system) through the REST API. Needed for the Juju machine provider.
The functionality as requested is a POST call on a Node. (You'll note that there were no POST operations for Node yet, only for the set of all Nodes). The API part is responsible for reporting permissions errors, but otherwise just forwards to the model.
The model side is set-based and fault-tolerant, so that we don't introduce scaling limitations too deeply in the system. It stops any number of nodes. A privilege check filters out nodes that the user isn't allowed to shut down: if the request was made by a non-admin user, then it can only operate on nodes owned by that user. Not un-owned nodes as the various "visible nodes" checks allow in addition.
>From there, the call gets forwarded to the Provisioning API, entirely in fire-and-forget mode. We have no convenient mechanism to keep track of success or failure. If it turns out that we need that, that'll have to be be a separate job — probably of a “time-out” nature.
Repeating a shutdown request is harmless: doing it again for luck won't make much difference, so long as the damned thing gets powered down. We'll just repeat the Cobbler request.
--
https://code.launchpad.net/~jtv/maas/stop-node/+merge/92960
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/stop-node into lp:maas.
=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py 2012-02-13 17:07:22 +0000
+++ src/maasserver/api.py 2012-02-14 12:12:49 +0000
@@ -38,6 +38,7 @@
MaasAPIBadRequest,
MaasAPINotFound,
NodesNotAvailable,
+ PermissionDenied,
)
from maasserver.forms import NodeWithMACAddressesForm
from maasserver.macaddress import validate_mac
@@ -192,9 +193,10 @@
return cls
+@api_operations
class NodeHandler(BaseHandler):
"""Manage individual Nodes."""
- allowed_methods = ('GET', 'DELETE', 'PUT')
+ allowed_methods = ('GET', 'DELETE', 'POST', 'PUT')
model = Node
fields = ('system_id', 'hostname', ('macaddress_set', ('mac_address',)))
@@ -232,6 +234,15 @@
node_system_id = node.system_id
return ('node_handler', (node_system_id, ))
+ @api_exported('stop', 'POST')
+ def stop(self, request, system_id):
+ """Shut down a node."""
+ nodes = Node.objects.stop_nodes([system_id], request.user)
+ if len(nodes) == 0:
+ raise PermissionDenied(
+ "You are not allowed to shut down this system.")
+ return nodes[0]
+
@api_operations
class NodesHandler(BaseHandler):
=== modified file 'src/maasserver/models.py'
--- src/maasserver/models.py 2012-02-13 13:07:48 +0000
+++ src/maasserver/models.py 2012-02-14 12:12:49 +0000
@@ -134,8 +134,32 @@
class NodeManager(models.Manager):
"""A utility to manage the collection of Nodes."""
+ # Twisted XMLRPC proxy for talking to the provisioning API. Created
+ # on demand.
+ provisioning_proxy = None
+
+ def _set_provisioning_proxy(self):
+ """Set up the provisioning-API proxy if needed."""
+ # Avoid circular imports.
+ from maasserver.provisioning import get_provisioning_api_proxy
+ if self.provisioning_proxy is None:
+ self.provisioning_proxy = get_provisioning_api_proxy()
+
+ def filter_by_ids(self, query, ids=None):
+ """Filter `query` result set by system_id values.
+
+ :param query: A result set of Nodes.
+ :param ids: Optional set of ids to filter by. If given, nodes whose
+ system_ids are not in this sequence will be ignored.
+ :return: A filtered version of `query`.
+ """
+ if ids is None:
+ return query
+ else:
+ return query.filter(system_id__in=ids)
+
def get_visible_nodes(self, user, ids=None):
- """Fetch all the Nodes visible by a User_.
+ """Fetch Nodes visible by a User_.
:param user: The user that should be used in the permission check.
:type user: User_
@@ -152,10 +176,23 @@
else:
visible_nodes = self.filter(
models.Q(owner__isnull=True) | models.Q(owner=user))
- if ids is None:
- return visible_nodes
+ return self.filter_by_ids(visible_nodes, ids)
+
+ def get_editable_nodes(self, user, ids=None):
+ """Fetch Nodes a User_ has ownership privileges on.
+
+ An admin has ownership privileges on all nodes.
+
+ :param user: The user that should be used in the permission check.
+ :type user: User_
+ :param ids: If given, limit result to nodes with these system_ids.
+ :type ids: Sequence.
+ """
+ if user.is_superuser:
+ visible_nodes = self.all()
else:
- return visible_nodes.filter(system_id__in=ids)
+ visible_nodes = self.filter(models.Q(owner=user))
+ return self.filter_by_ids(visible_nodes, ids)
def get_visible_node_or_404(self, system_id, user):
"""Fetch a `Node` by system_id. Raise exceptions if no `Node` with
@@ -193,6 +230,21 @@
else:
return available_nodes[0]
+ def stop_nodes(self, ids, by_user):
+ """Request on given user's behalf that the given nodes be shut down.
+
+ Shutdown is only requested for nodes that the user has ownership
+ privileges for; any other nodes in the request are ignored.
+
+ :param ids: Sequence of `system_id` values for nodes to shut down.
+ :param by_user: Requesting user.
+ :return: A list of Nodes whose shutdown was actually requested.
+ """
+ self._set_provisioning_proxy()
+ nodes = self.get_editable_nodes(by_user, ids=ids)
+ self.provisioning_proxy.stop_nodes([node.system_id for node in nodes])
+ return nodes
+
class Node(CommonInfo):
"""A `Node` represents a physical machine used by the MaaS Server.
@@ -207,7 +259,6 @@
commissioning. See vocabulary
:class:`NODE_AFTER_COMMISSIONING_ACTION`.
:ivar objects: The :class:`NodeManager`.
- :ivar hostname: This `Node`'s hostname.
"""
=== modified file 'src/maasserver/provisioning.py'
--- src/maasserver/provisioning.py 2012-02-13 15:27:35 +0000
+++ src/maasserver/provisioning.py 2012-02-14 12:12:49 +0000
@@ -9,7 +9,9 @@
)
__metaclass__ = type
-__all__ = []
+__all__ = [
+ 'get_provisioning_api_proxy',
+ ]
from uuid import uuid1
import xmlrpclib
=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py 2012-02-14 11:22:44 +0000
+++ src/maasserver/tests/test_api.py 2012-02-14 12:12:49 +0000
@@ -103,6 +103,11 @@
username='test', password='test')
self.client = OAuthAuthenticatedClient(self.logged_in_user)
+ def become_admin(self):
+ """Promote the logged-in user to admin."""
+ self.logged_in_user.is_superuser = True
+ self.logged_in_user.save()
+
def extract_system_ids(parsed_result):
"""List the system_ids of the nodes in `parsed_result`."""
@@ -124,10 +129,14 @@
class TestNodeAPI(APITestCase):
"""Tests for /api/nodes/<node>/."""
+ def get_uri(self, node):
+ """Get the API URI for `node`."""
+ return '/api/nodes/%s/' % node.system_id
+
def test_GET_returns_node(self):
# The api allows for fetching a single Node (using system_id).
node = factory.make_node(set_hostname=True)
- response = self.client.get('/api/nodes/%s/' % node.system_id)
+ response = self.client.get(self.get_uri(node))
parsed_result = json.loads(response.content)
self.assertEqual(httplib.OK, response.status_code)
@@ -140,7 +149,7 @@
other_node = factory.make_node(
status=NODE_STATUS.DEPLOYED, owner=factory.make_user())
- response = self.client.get('/api/nodes/%s/' % other_node.system_id)
+ response = self.client.get(self.get_uri(other_node))
self.assertEqual(httplib.FORBIDDEN, response.status_code)
@@ -151,11 +160,29 @@
self.assertEqual(httplib.NOT_FOUND, response.status_code)
+ def test_POST_stop_checks_permission(self):
+ node = factory.make_node()
+ response = self.client.post(self.get_uri(node), {'op': 'stop'})
+ self.assertEqual(httplib.FORBIDDEN, response.status_code)
+
+ def test_POST_stop_returns_node(self):
+ node = factory.make_node(owner=self.logged_in_user)
+ response = self.client.post(self.get_uri(node), {'op': 'stop'})
+ self.assertEqual(httplib.OK, response.status_code)
+ self.assertEqual(
+ node.system_id, json.loads(response.content)['system_id'])
+
+ def test_POST_stop_may_be_repeated(self):
+ node = factory.make_node(owner=self.logged_in_user)
+ self.client.post(self.get_uri(node), {'op': 'stop'})
+ response = self.client.post(self.get_uri(node), {'op': 'stop'})
+ self.assertEqual(httplib.OK, response.status_code)
+
def test_PUT_updates_node(self):
# The api allows to update a Node.
node = factory.make_node(hostname='diane')
response = self.client.put(
- '/api/nodes/%s/' % node.system_id, {'hostname': 'francis'})
+ self.get_uri(node), {'hostname': 'francis'})
parsed_result = json.loads(response.content)
self.assertEqual(httplib.OK, response.status_code)
@@ -167,8 +194,7 @@
# When a Node is returned by the API, the field 'resource_uri'
# provides the URI for this Node.
node = factory.make_node(hostname='diane')
- response = self.client.put(
- '/api/nodes/%s/' % node.system_id, {'hostname': 'francis'})
+ response = self.client.put(self.get_uri(node), {'hostname': 'francis'})
parsed_result = json.loads(response.content)
self.assertEqual(
@@ -180,8 +206,7 @@
# response is returned.
node = factory.make_node(hostname='diane')
response = self.client.put(
- '/api/nodes/%s/' % node.system_id,
- {'hostname': 'too long' * 100})
+ self.get_uri(node), {'hostname': 'too long' * 100})
parsed_result = json.loads(response.content)
self.assertEqual(httplib.BAD_REQUEST, response.status_code)
@@ -197,7 +222,7 @@
other_node = factory.make_node(
status=NODE_STATUS.DEPLOYED, owner=factory.make_user())
- response = self.client.put('/api/nodes/%s/' % other_node.system_id)
+ response = self.client.put(self.get_uri(other_node))
self.assertEqual(httplib.FORBIDDEN, response.status_code)
@@ -212,11 +237,10 @@
# The api allows to delete a Node.
node = factory.make_node(set_hostname=True)
system_id = node.system_id
- response = self.client.delete('/api/nodes/%s/' % node.system_id)
+ response = self.client.delete(self.get_uri(node))
self.assertEqual(204, response.status_code)
- self.assertEqual(
- [], list(Node.objects.filter(system_id=system_id)))
+ self.assertItemsEqual([], Node.objects.filter(system_id=system_id))
def test_DELETE_refuses_to_delete_invisible_node(self):
# The request to delete a single node is denied if the node isn't
@@ -224,7 +248,7 @@
other_node = factory.make_node(
status=NODE_STATUS.DEPLOYED, owner=factory.make_user())
- response = self.client.delete('/api/nodes/%s/' % other_node.system_id)
+ response = self.client.delete(self.get_uri(other_node))
self.assertEqual(httplib.FORBIDDEN, response.status_code)
=== modified file 'src/maasserver/tests/test_models.py'
--- src/maasserver/tests/test_models.py 2012-02-13 16:32:31 +0000
+++ src/maasserver/tests/test_models.py 2012-02-14 12:12:49 +0000
@@ -83,13 +83,25 @@
status = NODE_STATUS.DEPLOYED
return factory.make_node(set_hostname=True, status=status, owner=user)
+ def test_filter_by_ids_filters_nodes_by_ids(self):
+ nodes = [factory.make_node() for counter in range(5)]
+ ids = [node.system_id for node in nodes]
+ selection = slice(1, 3)
+ self.assertItemsEqual(
+ nodes[selection],
+ Node.objects.filter_by_ids(Node.objects.all(), ids[selection]))
+
+ def test_filter_by_ids_with_empty_list_returns_empty(self):
+ factory.make_node()
+ self.assertItemsEqual(
+ [], Node.objects.filter_by_ids(Node.objects.all(), []))
+
+ def test_filter_by_ids_without_ids_returns_full(self):
+ node = factory.make_node()
+ self.assertItemsEqual(
+ [node], Node.objects.filter_by_ids(Node.objects.all(), None))
+
def test_get_visible_nodes_for_user_lists_visible_nodes(self):
- """get_visible_nodes lists the nodes a user has access to.
-
- When run for a regular user it returns unowned nodes, and nodes
- owned by that user.
-
- """
user = factory.make_user()
visible_nodes = [self.make_node(owner) for owner in [None, user]]
self.make_node(factory.make_user())
@@ -97,7 +109,6 @@
visible_nodes, Node.objects.get_visible_nodes(user))
def test_get_visible_nodes_admin_lists_all_nodes(self):
- """get_visible_nodes for an admin lists all nodes."""
admin = factory.make_admin()
owners = [
None,
@@ -109,7 +120,6 @@
self.assertItemsEqual(nodes, Node.objects.get_visible_nodes(admin))
def test_get_visible_nodes_filters_by_id(self):
- """get_visible_nodes optionally filters nodes by system_id."""
user = factory.make_user()
nodes = [self.make_node(user) for counter in range(5)]
ids = [node.system_id for node in nodes]
@@ -118,15 +128,33 @@
nodes[wanted_slice],
Node.objects.get_visible_nodes(user, ids=ids[wanted_slice]))
- def test_get_visible_nodes_with_ids_still_hides_invisible_nodes(self):
- """Even when passing ids, a user won't get nodes they can't see."""
- user = factory.make_user()
- visible_nodes = [self.make_node(user), self.make_node()]
- invisible_nodes = [self.make_node(factory.make_user())]
- all_ids = [
- node.system_id for node in (visible_nodes + invisible_nodes)]
- self.assertItemsEqual(
- visible_nodes, Node.objects.get_visible_nodes(user, ids=all_ids))
+ def test_get_editable_nodes_for_user_lists_owned_nodes(self):
+ user = factory.make_user()
+ visible_node = self.make_node(user)
+ self.make_node(None)
+ self.make_node(factory.make_user())
+ self.assertItemsEqual(
+ [visible_node], Node.objects.get_editable_nodes(user))
+
+ def test_get_editable_nodes_admin_lists_all_nodes(self):
+ admin = factory.make_admin()
+ owners = [
+ None,
+ factory.make_user(),
+ factory.make_admin(),
+ admin,
+ ]
+ nodes = [self.make_node(owner) for owner in owners]
+ self.assertItemsEqual(nodes, Node.objects.get_editable_nodes(admin))
+
+ def test_get_editable_nodes_filters_by_id(self):
+ user = factory.make_user()
+ nodes = [self.make_node(user) for counter in range(5)]
+ ids = [node.system_id for node in nodes]
+ wanted_slice = slice(0, 3)
+ self.assertItemsEqual(
+ nodes[wanted_slice],
+ Node.objects.get_editable_nodes(user, ids=ids[wanted_slice]))
def test_get_visible_node_or_404_ok(self):
"""get_visible_node_or_404 fetches nodes by system_id."""
@@ -173,6 +201,24 @@
self.assertEqual(
None, Node.objects.get_available_node_for_acquisition(user))
+ def test_stop_nodes_stops_nodes(self):
+ user = factory.make_user()
+ node = self.make_node(user)
+ output = Node.objects.stop_nodes([node.system_id], user)
+
+ self.assertItemsEqual([node], output)
+ self.assertEqual(
+ 'stop',
+ Node.objects.provisioning_proxy.power_status[node.system_id])
+
+ def test_stop_nodes_ignores_uneditable_nodes(self):
+ nodes = [self.make_node(factory.make_user()) for counter in range(3)]
+ ids = [node.system_id for node in nodes]
+ stoppable_node = nodes[0]
+ self.assertItemsEqual(
+ [stoppable_node],
+ Node.objects.stop_nodes(ids, stoppable_node.owner))
+
class MACAddressTest(TestCase):
=== modified file 'src/provisioningserver/api.py'
--- src/provisioningserver/api.py 2012-02-13 17:37:39 +0000
+++ src/provisioningserver/api.py 2012-02-14 12:12:49 +0000
@@ -286,3 +286,13 @@
# adding filtering options to this function before using it in anger.
d = CobblerSystem.get_all_values(self.session)
return d.addCallback(cobbler_mapping_to_papi_nodes)
+
+ @deferred
+ def start_nodes(self, names):
+ d = CobblerSystem.powerOnMultiple(names)
+ return d
+
+ @deferred
+ def stop_nodes(self, names):
+ d = CobblerSystem.powerOffMultiple(names)
+ return d
=== modified file 'src/provisioningserver/interfaces.py'
--- src/provisioningserver/interfaces.py 2012-02-13 10:55:45 +0000
+++ src/provisioningserver/interfaces.py 2012-02-14 12:12:49 +0000
@@ -123,6 +123,12 @@
:return: A dict of node-names -> node-values.
"""
+ def start_nodes(names):
+ """Start up nodes with the given `names`."""
+
+ def stop_nodes(names):
+ """Shut down nodes with the given `names`."""
+
# All public methods defined in IProvisioningAPI_Template.
PAPI_FUNCTIONS = {
=== modified file 'src/provisioningserver/testing/fakeapi.py'
--- src/provisioningserver/testing/fakeapi.py 2012-02-13 10:50:33 +0000
+++ src/provisioningserver/testing/fakeapi.py 2012-02-14 12:12:49 +0000
@@ -73,6 +73,8 @@
self.distros = FakeProvisioningDatabase()
self.profiles = FakeProvisioningDatabase()
self.nodes = FakeProvisioningDatabase()
+ # To record power-on/power-off commands:
+ self.power_status = {}
def add_distro(self, name, initrd, kernel):
self.distros[name]["initrd"] = initrd
@@ -130,6 +132,14 @@
def get_nodes(self):
return self.nodes.dump()
+ def start_nodes(self, names):
+ for name in names:
+ self.power_status[name] = 'start'
+
+ def stop_nodes(self, names):
+ for name in names:
+ self.power_status[name] = 'stop'
+
def async(func):
"""Decorate a function so that it always return a `defer.Deferred`."""
