From 487f6738bcb331115b49a88f21520b412cdcc49c Mon Sep 17 00:00:00 2001 From: Yuriy Zveryanskyy Date: Tue, 24 Dec 2013 15:41:54 +0200 Subject: [PATCH] Add parameter for filtering nodes by maintenance mode Boolean parameter 'maintenance' added to API for filtering nodes by maintenance mode, example: /nodes?maintenance=false strutils.bool_from_string() used for converting 'associated' and 'maintenance' parameters, accepted values now for True ('1', 't', 'true', 'on', 'y', 'yes'), for False ('0', 'f', 'false', 'off', 'n', 'no') case-insensitive. Refactored dbapi method get_node_list with filters used for _get_nodes_collection in nodes controller. Partial-Bug: #1260099 Partial-Bug: #1271291 Change-Id: I01a62eb641316e890d191cf569bb3cf00b7619f2 --- ironic/api/controllers/v1/node.py | 80 +++++++++++--------- ironic/db/api.py | 34 +++------ ironic/db/sqlalchemy/api.py | 60 ++++++--------- ironic/tests/api/test_nodes.py | 41 ++++++++++ ironic/tests/db/test_nodes.py | 121 +++++++++++++++--------------- 5 files changed, 180 insertions(+), 156 deletions(-) diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index ff78a8d38e..205775505e 100644 --- a/ironic/api/controllers/v1/node.py +++ b/ironic/api/controllers/v1/node.py @@ -37,6 +37,8 @@ from ironic.common import states as ir_states from ironic import objects from ironic.openstack.common import excutils from ironic.openstack.common import log +from ironic.openstack.common import strutils + CONF = cfg.CONF CONF.import_opt('heartbeat_timeout', 'ironic.conductor.manager', @@ -415,7 +417,7 @@ class NodesController(rest.RestController): self._from_chassis = from_chassis def _get_nodes_collection(self, chassis_uuid, instance_uuid, associated, - marker, limit, sort_key, sort_dir, + maintenance, marker, limit, sort_key, sort_dir, expand=False, resource_url=None): if self._from_chassis and not chassis_uuid: raise exception.InvalidParameterValue(_( @@ -429,25 +431,36 @@ class NodesController(rest.RestController): marker_obj = objects.Node.get_by_uuid(pecan.request.context, marker) - if chassis_uuid: - nodes = pecan.request.dbapi.get_nodes_by_chassis(chassis_uuid, - limit, marker_obj, - sort_key=sort_key, - sort_dir=sort_dir) - elif instance_uuid: + if instance_uuid: nodes = self._get_nodes_by_instance(instance_uuid) - elif associated: - nodes = self._get_nodes_by_instance_association(associated, - limit, marker_obj, - sort_key, sort_dir) else: - nodes = pecan.request.dbapi.get_node_list(limit, marker_obj, + filters = {} + if chassis_uuid: + filters['chassis_uuid'] = chassis_uuid + try: + if associated: + param = 'associated' + filters[param] = strutils.bool_from_string(associated, + strict=True) + if maintenance: + param = 'maintenance' + filters[param] = strutils.bool_from_string(maintenance, + strict=True) + except ValueError as e: + raise wsme.exc.ClientSideError(_( + "Invalid parameter '%(param)s' value: %(msg)s") % + {'param': param, 'msg': e}) + + nodes = pecan.request.dbapi.get_node_list(filters, limit, + marker_obj, sort_key=sort_key, sort_dir=sort_dir) parameters = {'sort_key': sort_key, 'sort_dir': sort_dir} if associated: parameters['associated'] = associated.lower() + if maintenance: + parameters['maintenance'] = maintenance.lower() return NodeCollection.convert_with_links(nodes, limit, url=resource_url, expand=expand, @@ -464,25 +477,12 @@ class NodesController(rest.RestController): except exception.InstanceNotFound: return [] - def _get_nodes_by_instance_association(self, associated, limit, marker_obj, - sort_key, sort_dir): - """Retrieve nodes by instance association.""" - if associated.lower() == 'true': - nodes = pecan.request.dbapi.get_associated_nodes(limit, - marker_obj, sort_key=sort_key, sort_dir=sort_dir) - elif associated.lower() == 'false': - nodes = pecan.request.dbapi.get_unassociated_nodes(limit, - marker_obj, sort_key=sort_key, sort_dir=sort_dir) - else: - raise wsme.exc.ClientSideError(_( - "Invalid parameter value: %s, 'associated' " - "can only be true or false.") % associated) - return nodes - @wsme_pecan.wsexpose(NodeCollection, types.uuid, types.uuid, - wtypes.text, types.uuid, int, wtypes.text, wtypes.text) + wtypes.text, wtypes.text, types.uuid, int, wtypes.text, + wtypes.text) def get_all(self, chassis_uuid=None, instance_uuid=None, associated=None, - marker=None, limit=None, sort_key='id', sort_dir='asc'): + maintenance=None, marker=None, limit=None, sort_key='id', + sort_dir='asc'): """Retrieve a list of nodes. :param chassis_uuid: Optional UUID of a chassis, to get only nodes for @@ -492,19 +492,24 @@ class NodesController(rest.RestController): :param associated: Optional boolean whether to return a list of associated or unassociated nodes. May be combined with other parameters. + :param maintenance: Optional boolean value that indicates whether + to get nodes in maintenance mode ("True"), or not + in maintenance mode ("False"). :param marker: pagination marker for large data sets. :param limit: maximum number of resources to return in a single result. :param sort_key: column to sort results by. Default: id. :param sort_dir: direction to sort. "asc" or "desc". Default: asc. """ return self._get_nodes_collection(chassis_uuid, instance_uuid, - associated, marker, limit, - sort_key, sort_dir) + associated, maintenance, marker, + limit, sort_key, sort_dir) @wsme_pecan.wsexpose(NodeCollection, types.uuid, types.uuid, - wtypes.text, types.uuid, int, wtypes.text, wtypes.text) + wtypes.text, wtypes.text, types.uuid, int, wtypes.text, + wtypes.text) def detail(self, chassis_uuid=None, instance_uuid=None, associated=None, - marker=None, limit=None, sort_key='id', sort_dir='asc'): + maintenance=None, marker=None, limit=None, sort_key='id', + sort_dir='asc'): """Retrieve a list of nodes with detail. :param chassis_uuid: Optional UUID of a chassis, to get only nodes for @@ -514,6 +519,9 @@ class NodesController(rest.RestController): :param associated: Optional boolean whether to return a list of associated or unassociated nodes. May be combined with other parameters. + :param maintenance: Optional boolean value that indicates whether + to get nodes in maintenance mode ("True"), or not + in maintenance mode ("False"). :param marker: pagination marker for large data sets. :param limit: maximum number of resources to return in a single result. :param sort_key: column to sort results by. Default: id. @@ -527,9 +535,9 @@ class NodesController(rest.RestController): expand = True resource_url = '/'.join(['nodes', 'detail']) return self._get_nodes_collection(chassis_uuid, instance_uuid, - associated, marker, limit, - sort_key, sort_dir, - expand, resource_url) + associated, maintenance, marker, + limit, sort_key, sort_dir, expand, + resource_url) @wsme_pecan.wsexpose(wtypes.text, types.uuid) def validate(self, node_uuid): diff --git a/ironic/db/api.py b/ironic/db/api.py index f1bc043418..586564c41e 100644 --- a/ironic/db/api.py +++ b/ironic/db/api.py @@ -52,6 +52,9 @@ class Connection(object): :param filters: Filters to apply. Defaults to None. 'associated': True | False 'reserved': True | False + 'maintenance': True | False + 'chassis_uuid': uuid of chassis + 'driver': driver's name :param limit: Maximum number of nodes to return. :param marker: the last item of the previous page; we return the next result set. @@ -62,10 +65,16 @@ class Connection(object): """ @abc.abstractmethod - def get_node_list(self, limit=None, marker=None, + def get_node_list(self, filters=None, limit=None, marker=None, sort_key=None, sort_dir=None): """Return a list of nodes. + :param filters: Filters to apply. Defaults to None. + 'associated': True | False + 'reserved': True | False + 'maintenance': True | False + 'chassis_uuid': uuid of chassis + 'driver': driver's name :param limit: Maximum number of nodes to return. :param marker: the last item of the previous page; we return the next result set. @@ -74,14 +83,6 @@ class Connection(object): (asc, desc) """ - @abc.abstractmethod - def get_associated_nodes(self): - """Return a list of ids of all associated nodes.""" - - @abc.abstractmethod - def get_unassociated_nodes(self): - """Return a list of ids of all unassociated nodes.""" - @abc.abstractmethod def reserve_nodes(self, tag, nodes): """Reserve a set of nodes atomically. @@ -144,21 +145,6 @@ class Connection(object): :returns: A node. """ - @abc.abstractmethod - def get_nodes_by_chassis(self, chassis_id, limit=None, marker=None, - sort_key=None, sort_dir=None): - """List all the nodes for a given chassis. - - :param chassis_id: The id or uuid of a chassis. - :param limit: Maximum number of nodes to return. - :param marker: the last item of the previous page; we returns the next - results after this value. - :param sort_key: Attribute by which results should be sorted - :param sort_dir: direction in which results should be sorted - (asc, desc) - :returns: A list of nodes. - """ - @abc.abstractmethod def destroy_node(self, node_id): """Destroy a node and all associated interfaces. diff --git a/ironic/db/sqlalchemy/api.py b/ironic/db/sqlalchemy/api.py index b5b4e1d100..7eddac76a6 100644 --- a/ironic/db/sqlalchemy/api.py +++ b/ironic/db/sqlalchemy/api.py @@ -183,18 +183,14 @@ class Connection(api.Connection): def __init__(self): pass - def get_nodeinfo_list(self, columns=None, filters=None, limit=None, - marker=None, sort_key=None, sort_dir=None): - # list-ify columns and filters default values because it is bad form - # to include a mutable list in function definitions. + def _add_nodes_filters(self, query, filters): if filters is None: filters = [] - if columns is None: - columns = [models.Node.id] - else: - columns = [getattr(models.Node, c) for c in columns] - query = model_query(*columns, base_model=models.Node) + if 'chassis_uuid' in filters: + # get_chassis() to raise an exception if the chassis is not found + chassis_obj = self.get_chassis(filters['chassis_uuid']) + query = query.filter_by(chassis_id=chassis_obj.id) if 'associated' in filters: if filters['associated']: query = query.filter(models.Node.instance_uuid != None) @@ -205,40 +201,32 @@ class Connection(api.Connection): query = query.filter(models.Node.reservation != None) else: query = query.filter(models.Node.reservation == None) + if 'maintenance' in filters: + query = query.filter_by(maintenance=filters['maintenance']) if 'driver' in filters: - query = query.filter(models.Node.driver == filters['driver']) + query = query.filter_by(driver=filters['driver']) + + return query + + def get_nodeinfo_list(self, columns=None, filters=None, limit=None, + marker=None, sort_key=None, sort_dir=None): + # list-ify columns default values because it is bad form + # to include a mutable list in function definitions. + if columns is None: + columns = [models.Node.id] + else: + columns = [getattr(models.Node, c) for c in columns] + + query = model_query(*columns, base_model=models.Node) + query = self._add_nodes_filters(query, filters) return _paginate_query(models.Node, limit, marker, sort_key, sort_dir, query) @objects.objectify(objects.Node) - def get_node_list(self, limit=None, marker=None, + def get_node_list(self, filters=None, limit=None, marker=None, sort_key=None, sort_dir=None): - return _paginate_query(models.Node, limit, marker, - sort_key, sort_dir) - - @objects.objectify(objects.Node) - def get_nodes_by_chassis(self, chassis_id, limit=None, marker=None, - sort_key=None, sort_dir=None): - # get_chassis() to raise an exception if the chassis is not found - chassis_obj = self.get_chassis(chassis_id) query = model_query(models.Node) - query = query.filter_by(chassis_id=chassis_obj.id) - return _paginate_query(models.Node, limit, marker, - sort_key, sort_dir, query) - - @objects.objectify(objects.Node) - def get_associated_nodes(self, limit=None, marker=None, - sort_key=None, sort_dir=None): - query = model_query(models.Node).\ - filter(models.Node.instance_uuid != None) - return _paginate_query(models.Node, limit, marker, - sort_key, sort_dir, query) - - @objects.objectify(objects.Node) - def get_unassociated_nodes(self, limit=None, marker=None, - sort_key=None, sort_dir=None): - query = model_query(models.Node).\ - filter(models.Node.instance_uuid == None) + query = self._add_nodes_filters(query, filters) return _paginate_query(models.Node, limit, marker, sort_key, sort_dir, query) diff --git a/ironic/tests/api/test_nodes.py b/ironic/tests/api/test_nodes.py index 4f3a1eb0c8..d39fc3a4e3 100644 --- a/ironic/tests/api/test_nodes.py +++ b/ironic/tests/api/test_nodes.py @@ -324,6 +324,47 @@ class TestListNodes(base.FunctionalTest): # never expose the chassis_id self.assertNotIn('chassis_id', data['nodes'][0]) + def test_maintenance_nodes(self): + nodes = [] + for id in range(5): + ndict = dbutils.get_test_node(id=id, uuid=utils.generate_uuid(), + maintenance=id % 2) + node = self.dbapi.create_node(ndict) + nodes.append(node) + + data = self.get_json('/nodes?maintenance=true') + uuids = [n['uuid'] for n in data['nodes']] + test_uuids_1 = [n.uuid for n in nodes if n.maintenance] + self.assertEqual(sorted(test_uuids_1), sorted(uuids)) + + data = self.get_json('/nodes?maintenance=false') + uuids = [n['uuid'] for n in data['nodes']] + test_uuids_0 = [n.uuid for n in nodes if not n.maintenance] + self.assertEqual(sorted(test_uuids_0), sorted(uuids)) + + def test_maintenance_nodes_error(self): + response = self.get_json('/nodes?associated=true&maintenance=blah', + expect_errors=True) + self.assertEqual('application/json', response.content_type) + self.assertEqual(400, response.status_code) + self.assertTrue(response.json['error_message']) + + def test_maintenance_nodes_associated(self): + self._create_association_test_nodes() + ndict = dbutils.get_test_node(instance_uuid=utils.generate_uuid(), + maintenance=True) + node = self.dbapi.create_node(ndict) + + data = self.get_json('/nodes?associated=true&maintenance=false') + uuids = [n['uuid'] for n in data['nodes']] + self.assertNotIn(node.uuid, uuids) + data = self.get_json('/nodes?associated=true&maintenance=true') + uuids = [n['uuid'] for n in data['nodes']] + self.assertIn(node.uuid, uuids) + data = self.get_json('/nodes?associated=true&maintenance=TruE') + uuids = [n['uuid'] for n in data['nodes']] + self.assertIn(node.uuid, uuids) + class TestPatch(base.FunctionalTest): diff --git a/ironic/tests/db/test_nodes.py b/ironic/tests/db/test_nodes.py index 06cca2e132..292301aa0e 100644 --- a/ironic/tests/db/test_nodes.py +++ b/ironic/tests/db/test_nodes.py @@ -76,28 +76,6 @@ class DbNodeTestCase(base.DbTestCase): del n['chassis_id'] self.dbapi.create_node(n) - def test_get_nodes_by_chassis_id(self): - ch = utils.get_test_chassis() - ch = self.dbapi.create_chassis(ch) - n = self._create_test_node(chassis_id=ch['id']) - nodes = self.dbapi.get_nodes_by_chassis(ch['id']) - self.assertEqual(n['uuid'], nodes[0]['uuid']) - - def test_get_nodes_by_chassis_uuid(self): - ch = utils.get_test_chassis() - ch = self.dbapi.create_chassis(ch) - n = self._create_test_node(chassis_id=ch['id']) - nodes = self.dbapi.get_nodes_by_chassis(ch['uuid']) - self.assertEqual(n['id'], nodes[0]['id']) - - def test_get_nodes_by_chassis_that_does_not_exist(self): - self.assertRaises(exception.ChassisNotFound, - self.dbapi.get_nodes_by_chassis, - 33) - self.assertRaises(exception.ChassisNotFound, - self.dbapi.get_nodes_by_chassis, - '12345678-9999-0000-aaaa-123456789012') - def test_get_node_by_id(self): n = self._create_test_node() res = self.dbapi.get_node(n['id']) @@ -144,7 +122,8 @@ class DbNodeTestCase(base.DbTestCase): reservation='fake-host', uuid=ironic_utils.generate_uuid()) n2 = utils.get_test_node(id=2, driver='driver-two', - uuid=ironic_utils.generate_uuid()) + uuid=ironic_utils.generate_uuid(), + maintenance=True) self.dbapi.create_node(n1) self.dbapi.create_node(n2) @@ -166,6 +145,12 @@ class DbNodeTestCase(base.DbTestCase): res = self.dbapi.get_nodeinfo_list(filters={'reserved': False}) self.assertEqual([2], [r[0] for r in res]) + res = self.dbapi.get_node_list(filters={'maintenance': True}) + self.assertEqual([2], [r.id for r in res]) + + res = self.dbapi.get_node_list(filters={'maintenance': False}) + self.assertEqual([1], [r.id for r in res]) + def test_get_node_list(self): uuids = [] for i in range(1, 6): @@ -176,6 +161,59 @@ class DbNodeTestCase(base.DbTestCase): res_uuids = [r.uuid for r in res] self.assertEqual(uuids.sort(), res_uuids.sort()) + def test_get_node_list_with_filters(self): + ch1 = utils.get_test_chassis(id=1, uuid=ironic_utils.generate_uuid()) + ch2 = utils.get_test_chassis(id=2, uuid=ironic_utils.generate_uuid()) + self.dbapi.create_chassis(ch1) + self.dbapi.create_chassis(ch2) + + n1 = utils.get_test_node(id=1, driver='driver-one', + instance_uuid=ironic_utils.generate_uuid(), + reservation='fake-host', + uuid=ironic_utils.generate_uuid(), + chassis_id=ch1['id']) + n2 = utils.get_test_node(id=2, driver='driver-two', + uuid=ironic_utils.generate_uuid(), + chassis_id=ch2['id'], + maintenance=True) + self.dbapi.create_node(n1) + self.dbapi.create_node(n2) + + res = self.dbapi.get_node_list(filters={'chassis_uuid': ch1['uuid']}) + self.assertEqual([1], [r.id for r in res]) + + res = self.dbapi.get_node_list(filters={'chassis_uuid': ch2['uuid']}) + self.assertEqual([2], [r.id for r in res]) + + res = self.dbapi.get_node_list(filters={'driver': 'driver-one'}) + self.assertEqual([1], [r.id for r in res]) + + res = self.dbapi.get_node_list(filters={'driver': 'bad-driver'}) + self.assertEqual([], [r.id for r in res]) + + res = self.dbapi.get_node_list(filters={'associated': True}) + self.assertEqual([1], [r.id for r in res]) + + res = self.dbapi.get_node_list(filters={'associated': False}) + self.assertEqual([2], [r.id for r in res]) + + res = self.dbapi.get_node_list(filters={'reserved': True}) + self.assertEqual([1], [r.id for r in res]) + + res = self.dbapi.get_node_list(filters={'reserved': False}) + self.assertEqual([2], [r.id for r in res]) + + res = self.dbapi.get_node_list(filters={'maintenance': True}) + self.assertEqual([2], [r.id for r in res]) + + res = self.dbapi.get_node_list(filters={'maintenance': False}) + self.assertEqual([1], [r.id for r in res]) + + def test_get_node_list_chassis_not_found(self): + self.assertRaises(exception.ChassisNotFound, + self.dbapi.get_node_list, + {'chassis_uuid': ironic_utils.generate_uuid()}) + def test_get_node_by_instance(self): n = self._create_test_node( instance_uuid='12345678-9999-0000-aaaa-123456789012') @@ -424,40 +462,3 @@ class DbNodeTestCase(base.DbTestCase): for uuid in uuids: res = self.dbapi.get_node(uuid) self.assertEqual(None, res['reservation']) - - def test_get_associated_nodes(self): - (uuids, uuids_with_instance) = self._create_associated_nodes() - - res = self.dbapi.get_associated_nodes() - res_uuids = [r.uuid for r in res] - res_uuids.sort() - self.assertEqual(uuids_with_instance, res_uuids) - - def test_get_associated_nodes_with_limit(self): - (uuids, uuids_with_instance) = self._create_associated_nodes() - - res = self.dbapi.get_associated_nodes(limit=1) - - res_uuids = [r.uuid for r in res] - self.assertEqual(len(res_uuids), 1) - self.assertTrue(len(uuids_with_instance) > len(res_uuids)) - - def test_get_unassociated_nodes(self): - (uuids, uuids_with_instance) = self._create_associated_nodes() - uuids_without_instance = list(set(uuids) - set(uuids_with_instance)) - uuids_without_instance.sort() - - res = self.dbapi.get_unassociated_nodes() - res_uuids = [r.uuid for r in res] - res_uuids.sort() - self.assertEqual(uuids_without_instance, res_uuids) - - def test_get_unassociated_nodes_with_limit(self): - (uuids, uuids_with_instance) = self._create_associated_nodes() - uuids_without_instance = list(set(uuids) - set(uuids_with_instance)) - - res = self.dbapi.get_unassociated_nodes(limit=1) - - res_uuids = [r.uuid for r in res] - self.assertEqual(len(res_uuids), 1) - self.assertTrue(len(uuids_without_instance) > len(res_uuids))