Power fault recovery follow up

This patch addresses comments relate to API and notification.

[1] https://review.openstack.org/#/c/558152
[2] https://review.openstack.org/#/c/556015/

Story: #1596107
Task: #10469

Change-Id: Ic2f7e6847790ae7af43570ca7af572539e7d5f2c

api-ref/source/baremetal-api-v1-nodes.inc

@@ -212,6 +212,9 @@ provision state, and maintenance setting for each Node.
   Added the ``resource_class`` Request parameter, allowing the list of returned
   Nodes to be filtered by this field.
 
+.. versionadded:: 1.42
+  Introduced the ``fault`` field.
+
 Normal response codes: 200
 
 Error codes: 400,403,406
@@ -227,6 +230,7 @@ Request
    - provision_state: r_provision_state
    - driver: r_driver
    - resource_class: r_resource_class
+   - fault: r_fault
    - fields: fields
    - limit: limit
    - marker: marker

api-ref/source/parameters.yaml

@@ -201,7 +201,8 @@ r_driver:
 r_fault:
   description: |
     Filter the list of returned nodes, and only return those with the specified
-    ``fault``.
+    ``fault``. Possible values are determined by faults supported by ironic,
+    e.g., ``power failure``, ``clean failure`` or ``rescue abort failure``.
   in: query
   required: false
   type: string

doc/source/admin/notifications.rst

@@ -132,7 +132,7 @@ Example of node CRUD notification::
     "payload":{
         "ironic_object.namespace":"ironic",
         "ironic_object.name":"NodeCRUDPayload",
-        "ironic_object.version":"1.4",
+        "ironic_object.version":"1.5",
         "ironic_object.data":{
             "chassis_uuid": "db0eef9d-45b2-4dc0-94a8-fc283c01171f",
             "clean_step": None,
@@ -150,6 +150,7 @@ Example of node CRUD notification::
             "last_error": None,
             "maintenance": False,
             "maintenance_reason": None,
+            "fault": None,
             "boot_interface": "pxe",
             "console_interface": "no-console",
             "deploy_interface": "iscsi",
@@ -358,7 +359,7 @@ node maintenance notification::
     "payload":{
         "ironic_object.namespace":"ironic",
         "ironic_object.name":"NodePayload",
-        "ironic_object.version":"1.6",
+        "ironic_object.version":"1.7",
         "ironic_object.data":{
             "clean_step": None,
             "console_enabled": False,
@@ -372,6 +373,7 @@ node maintenance notification::
             "last_error": None,
             "maintenance": True,
             "maintenance_reason": "hw upgrade",
+            "fault": None,
             "boot_interface": "pxe",
             "console_interface": "no-console",
             "deploy_interface": "iscsi",
@@ -437,7 +439,7 @@ level, "error" has ERROR. Example of node console notification::
     "payload":{
         "ironic_object.namespace":"ironic",
         "ironic_object.name":"NodePayload",
-        "ironic_object.version":"1.6",
+        "ironic_object.version":"1.7",
         "ironic_object.data":{
             "clean_step": None,
             "console_enabled": True,
@@ -451,6 +453,7 @@ level, "error" has ERROR. Example of node console notification::
             "last_error": None,
             "maintenance": False,
             "maintenance_reason": None,
+            "fault": None,
             "boot_interface": "pxe",
             "console_interface": "no-console",
             "deploy_interface": "iscsi",
@@ -510,7 +513,7 @@ ironic-conductor is attempting to change the node::
     "payload":{
         "ironic_object.namespace":"ironic",
         "ironic_object.name":"NodeSetPowerStatePayload",
-        "ironic_object.version":"1.6",
+        "ironic_object.version":"1.7",
         "ironic_object.data":{
             "clean_step": None,
             "console_enabled": False,
@@ -523,6 +526,7 @@ ironic-conductor is attempting to change the node::
             "last_error": None,
             "maintenance": False,
             "maintenance_reason": None,
+            "fault": None,
             "boot_interface": "pxe",
             "console_interface": "no-console",
             "deploy_interface": "iscsi",
@@ -577,7 +581,7 @@ prior to the correction::
     "payload":{
         "ironic_object.namespace":"ironic",
         "ironic_object.name":"NodeCorrectedPowerStatePayload",
-        "ironic_object.version":"1.6",
+        "ironic_object.version":"1.7",
         "ironic_object.data":{
             "clean_step": None,
             "console_enabled": False,
@@ -590,6 +594,7 @@ prior to the correction::
             "last_error": None,
             "maintenance": False,
             "maintenance_reason": None,
+            "fault": None,
             "boot_interface": "pxe",
             "console_interface": "no-console",
             "deploy_interface": "iscsi",
@@ -655,7 +660,7 @@ indicate a node's provision states before state change, "event" is the FSM
     "payload":{
         "ironic_object.namespace":"ironic",
         "ironic_object.name":"NodeSetProvisionStatePayload",
-        "ironic_object.version":"1.6",
+        "ironic_object.version":"1.7",
         "ironic_object.data":{
             "clean_step": None,
             "console_enabled": False,
@@ -669,6 +674,7 @@ indicate a node's provision states before state change, "event" is the FSM
             "last_error": None,
             "maintenance": False,
             "maintenance_reason": None,
+            "fault": None,
             "boot_interface": "pxe",
             "console_interface": "no-console",
             "deploy_interface": "iscsi",

ironic/api/controllers/v1/node.py

@@ -1523,8 +1523,8 @@ class NodesController(rest.RestController):
     def _get_nodes_collection(self, chassis_uuid, instance_uuid, associated,
                               maintenance, provision_state, marker, limit,
                               sort_key, sort_dir, driver=None,
-                              resource_class=None,
+                              resource_class=None, resource_url=None,
-                              resource_url=None, fields=None):
+                              fields=None, fault=None):
         if self.from_chassis and not chassis_uuid:
             raise exception.MissingParameterValue(
                 _("Chassis id not specified."))
@@ -1570,6 +1570,8 @@ class NodesController(rest.RestController):
                 filters['driver'] = driver
             if resource_class is not None:
                 filters['resource_class'] = resource_class
+            if fault is not None:
+                filters['fault'] = fault
 
             nodes = objects.Node.list(pecan.request.context, limit, marker_obj,
                                       sort_key=sort_key, sort_dir=sort_dir,
@@ -1673,11 +1675,12 @@ class NodesController(rest.RestController):
     @METRICS.timer('NodesController.get_all')
     @expose.expose(NodeCollection, types.uuid, types.uuid, types.boolean,
                    types.boolean, wtypes.text, types.uuid, int, wtypes.text,
-                   wtypes.text, wtypes.text, types.listtype, wtypes.text)
+                   wtypes.text, wtypes.text, types.listtype, wtypes.text,
+                   wtypes.text)
     def get_all(self, chassis_uuid=None, instance_uuid=None, associated=None,
                 maintenance=None, provision_state=None, marker=None,
                 limit=None, sort_key='id', sort_dir='asc', driver=None,
-                fields=None, resource_class=None):
+                fields=None, resource_class=None, fault=None):
         """Retrieve a list of nodes.
 
         :param chassis_uuid: Optional UUID of a chassis, to get only nodes for
@@ -1705,6 +1708,7 @@ class NodesController(rest.RestController):
                                that resource_class.
         :param fields: Optional, a list with a specified set of fields
                        of the resource to be returned.
+        :param fault: Optional string value to get only nodes with that fault.
         """
         cdict = pecan.request.context.to_policy_values()
         policy.authorize('baremetal:node:get', cdict, cdict)
@@ -1715,6 +1719,7 @@ class NodesController(rest.RestController):
         api_utils.check_for_invalid_state_and_allow_filter(provision_state)
         api_utils.check_allow_specify_driver(driver)
         api_utils.check_allow_specify_resource_class(resource_class)
+        api_utils.check_allow_filter_by_fault(fault)
         if fields is None:
             fields = _DEFAULT_RETURN_FIELDS
         return self._get_nodes_collection(chassis_uuid, instance_uuid,
@@ -1723,16 +1728,16 @@ class NodesController(rest.RestController):
                                           limit, sort_key, sort_dir,
                                           driver=driver,
                                           resource_class=resource_class,
-                                          fields=fields)
+                                          fields=fields, fault=fault)
 
     @METRICS.timer('NodesController.detail')
     @expose.expose(NodeCollection, types.uuid, types.uuid, types.boolean,
                    types.boolean, wtypes.text, types.uuid, int, wtypes.text,
-                   wtypes.text, wtypes.text, wtypes.text)
+                   wtypes.text, wtypes.text, wtypes.text, wtypes.text)
     def detail(self, chassis_uuid=None, instance_uuid=None, associated=None,
                maintenance=None, provision_state=None, marker=None,
                limit=None, sort_key='id', sort_dir='asc', driver=None,
-               resource_class=None):
+               resource_class=None, fault=None):
         """Retrieve a list of nodes with detail.
 
         :param chassis_uuid: Optional UUID of a chassis, to get only nodes for
@@ -1758,6 +1763,7 @@ class NodesController(rest.RestController):
                        driver.
         :param resource_class: Optional string value to get only nodes with
                                that resource_class.
+        :param fault: Optional string value to get only nodes with that fault.
         """
         cdict = pecan.request.context.to_policy_values()
         policy.authorize('baremetal:node:get', cdict, cdict)
@@ -1765,6 +1771,7 @@ class NodesController(rest.RestController):
         api_utils.check_for_invalid_state_and_allow_filter(provision_state)
         api_utils.check_allow_specify_driver(driver)
         api_utils.check_allow_specify_resource_class(resource_class)
+        api_utils.check_allow_filter_by_fault(fault)
         api_utils.check_allowed_fields([sort_key])
         # /detail should only work against collections
         parent = pecan.request.path.split('/')[:-1][-1]
@@ -1778,7 +1785,8 @@ class NodesController(rest.RestController):
                                           limit, sort_key, sort_dir,
                                           driver=driver,
                                           resource_class=resource_class,
-                                          resource_url=resource_url)
+                                          resource_url=resource_url,
+                                          fault=fault)
 
     @METRICS.timer('NodesController.validate')
     @expose.expose(wtypes.text, types.uuid_or_name, types.uuid)

ironic/api/controllers/v1/utils.py

@@ -479,6 +479,19 @@ def check_allow_configdrive(target):
             msg, status_code=http_client.BAD_REQUEST)
 
 
+def check_allow_filter_by_fault(fault):
+    """Check if filtering nodes by fault is allowed.
+
+    Version 1.42 of the API allows filtering nodes by fault.
+    """
+    if (fault is not None and pecan.request.version.minor
+            < versions.MINOR_42_FAULT):
+        raise exception.NotAcceptable(_(
+            "Request not acceptable. The minimal required API version "
+            "should be %(base)s.%(opr)s") % {'base': versions.BASE_VERSION,
+                                             'opr': versions.MINOR_42_FAULT})
+
+
 def initial_node_provision_state():
     """Return node state to use by default when creating new nodes.
 

ironic/tests/unit/api/controllers/v1/test_node.py

@@ -1295,6 +1295,35 @@ class TestListNodes(test_api_base.BaseApiTest):
     def test_get_nodes_by_traits_not_allowed_detail(self):
         self._test_get_nodes_by_traits_not_allowed(detail=True)
 
+    def test_get_nodes_by_fault(self):
+        node1 = obj_utils.create_test_node(self.context,
+                                           uuid=uuidutils.generate_uuid(),
+                                           fault='power failure')
+        node2 = obj_utils.create_test_node(self.context,
+                                           uuid=uuidutils.generate_uuid(),
+                                           fault="clean failure")
+
+        for base_url in ('/nodes', '/nodes/detail'):
+            data = self.get_json(base_url + '?fault=power failure',
+                                 headers={api_base.Version.string: "1.42"})
+            uuids = [n['uuid'] for n in data['nodes']]
+            self.assertIn(node1.uuid, uuids)
+            self.assertNotIn(node2.uuid, uuids)
+            data = self.get_json(base_url + '?fault=clean failure',
+                                 headers={api_base.Version.string: "1.42"})
+            uuids = [n['uuid'] for n in data['nodes']]
+            self.assertIn(node2.uuid, uuids)
+            self.assertNotIn(node1.uuid, uuids)
+
+    def test_get_nodes_by_fault_not_allowed(self):
+        for url in ('/nodes?fault=test', '/nodes/detail?fault=test'):
+            response = self.get_json(
+                url, headers={api_base.Version.string: "1.41"},
+                expect_errors=True)
+            self.assertEqual('application/json', response.content_type)
+            self.assertEqual(http_client.NOT_ACCEPTABLE, response.status_code)
+            self.assertTrue(response.json['error_message'])
+
     def test_get_console_information(self):
         node = obj_utils.create_test_node(self.context)
         expected_console_info = {'test': 'test-data'}

ironic/tests/unit/objects/test_node.py

@@ -636,6 +636,7 @@ class TestNodePayloads(db_base.DbTestCase):
         self.assertEqual(self.node.maintenance, payload.maintenance)
         self.assertEqual(self.node.maintenance_reason,
                          payload.maintenance_reason)
+        self.assertEqual(self.node.fault, payload.fault)
         self.assertEqual(self.node.boot_interface, payload.boot_interface)
         self.assertEqual(self.node.console_interface,
                          payload.console_interface)