From 8ee2f4b58f62d2c96e7da640e17102e979322eb1 Mon Sep 17 00:00:00 2001 From: Kaifeng Wang Date: Tue, 5 Jun 2018 20:33:00 +0800 Subject: [PATCH] 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 | 4 +++ api-ref/source/parameters.yaml | 3 +- doc/source/admin/notifications.rst | 18 ++++++++---- ironic/api/controllers/v1/node.py | 24 ++++++++++----- ironic/api/controllers/v1/utils.py | 13 +++++++++ .../unit/api/controllers/v1/test_node.py | 29 +++++++++++++++++++ ironic/tests/unit/objects/test_node.py | 1 + 7 files changed, 77 insertions(+), 15 deletions(-) diff --git a/api-ref/source/baremetal-api-v1-nodes.inc b/api-ref/source/baremetal-api-v1-nodes.inc index 40f6195db6..e85d5f279e 100644 --- a/api-ref/source/baremetal-api-v1-nodes.inc +++ b/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 diff --git a/api-ref/source/parameters.yaml b/api-ref/source/parameters.yaml index 982525c6c0..3c81dc5062 100644 --- a/api-ref/source/parameters.yaml +++ b/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 diff --git a/doc/source/admin/notifications.rst b/doc/source/admin/notifications.rst index 52f7ed50ec..4304f5097c 100644 --- a/doc/source/admin/notifications.rst +++ b/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", diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index c02e693df9..bcbdbb40f0 100644 --- a/ironic/api/controllers/v1/node.py +++ b/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_url=None, fields=None): + resource_class=None, resource_url=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) diff --git a/ironic/api/controllers/v1/utils.py b/ironic/api/controllers/v1/utils.py index 7ea7cf5782..e97cadb75c 100644 --- a/ironic/api/controllers/v1/utils.py +++ b/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. diff --git a/ironic/tests/unit/api/controllers/v1/test_node.py b/ironic/tests/unit/api/controllers/v1/test_node.py index e04eb2da4f..cbc89c9009 100644 --- a/ironic/tests/unit/api/controllers/v1/test_node.py +++ b/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'} diff --git a/ironic/tests/unit/objects/test_node.py b/ironic/tests/unit/objects/test_node.py index aab717af13..731857655d 100644 --- a/ironic/tests/unit/objects/test_node.py +++ b/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)