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

@@ -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:
+        if instance_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:
             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,
+                                          associated, maintenance, marker,
-                                          sort_key, sort_dir)
+                                          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,
+                                          associated, maintenance, marker,
-                                          sort_key, sort_dir,
+                                          limit, sort_key, sort_dir, expand,
-                                          expand, resource_url)
+                                          resource_url)
 
     @wsme_pecan.wsexpose(wtypes.text, types.uuid)
     def validate(self, node_uuid):

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.

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,
+    def _add_nodes_filters(self, query, filters):
-                          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.
         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)
+        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_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)
         return _paginate_query(models.Node, limit, marker,
                                sort_key, sort_dir, query)
 

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):
 

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))