From 3f5e25e182c0b369b7491c39a6fb1c72e38ac728 Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Tue, 11 Apr 2023 15:59:47 -0700 Subject: [PATCH] DPU modeling - parent_node DB/Model/API Adds the parent node support and tests in one change including all DB/Model/API changes along with RBAC and basic API tests. * Updates the API version to 1.83 * Adds parent_node and related index to the nodes table. * Adds new API parameters to list by parent node relationship. Depends-On: https://review.opendev.org/c/openstack/ironic/+/883967 Change-Id: I8d64fee7105718199986db4994e13352d639f04f --- api-ref/source/baremetal-api-v1-nodes.inc | 18 ++ api-ref/source/parameters.yaml | 14 ++ .../contributor/webapi-version-history.rst | 21 ++ ironic/api/controllers/v1/node.py | 199 +++++++++++++----- ironic/api/controllers/v1/utils.py | 17 +- ironic/api/controllers/v1/versions.py | 4 +- ironic/common/policy.py | 8 + ironic/common/release_mappings.py | 4 +- .../fe222f476baf_add_parent_node_field.py | 35 +++ ironic/db/sqlalchemy/api.py | 13 +- ironic/db/sqlalchemy/models.py | 4 +- ironic/objects/node.py | 4 +- .../unit/api/controllers/v1/test_node.py | 177 ++++++++++++++++ ironic/tests/unit/api/test_acl.py | 11 + .../unit/api/test_rbac_project_scoped.yaml | 62 +++++- .../unit/api/test_rbac_system_scoped.yaml | 31 ++- ironic/tests/unit/db/utils.py | 3 +- ironic/tests/unit/objects/test_objects.py | 2 +- ...-parent-node-support-10bd42abd008db6f.yaml | 23 ++ 19 files changed, 580 insertions(+), 70 deletions(-) create mode 100644 ironic/db/sqlalchemy/alembic/versions/fe222f476baf_add_parent_node_field.py create mode 100644 releasenotes/notes/add-parent-node-support-10bd42abd008db6f.yaml diff --git a/api-ref/source/baremetal-api-v1-nodes.inc b/api-ref/source/baremetal-api-v1-nodes.inc index 47bcceb58c..9ffb39a36b 100644 --- a/api-ref/source/baremetal-api-v1-nodes.inc +++ b/api-ref/source/baremetal-api-v1-nodes.inc @@ -107,6 +107,9 @@ supplied when the Node is created, or the resource may be updated later. .. versionadded:: 1.82 Introduced the ``shard`` field. +.. versionadded: 1.83 + Introduced the ``parent_node`` field. + Normal response codes: 201 Error codes: 400,403,406 @@ -147,6 +150,7 @@ Request - maintenance: req_maintenance - maintenance_reason: maintenance_reason - network_data: network_data + - parent_node: parent_node - protected: protected - protected_reason: protected_reason - retired: retired @@ -212,6 +216,7 @@ microversion 1.81. - vendor_interface: vendor_interface - volume: n_volume - conductor_group: conductor_group + - parent_node: parent_node - protected: protected - protected_reason: protected_reason - conductor: conductor @@ -288,6 +293,13 @@ provision state, and maintenance setting for each Node. .. versionadded:: 1.82 Introduced the ``shard`` field. Introduced the ``sharded`` request parameter. +.. versionadded:: 1.83 + Introduced the ``parent_node`` field and query parameter to identify + matching nodes. + Introduced the ``include_children`` parameter which allows for all child + nodes to be enumerated, which are normally hidden as child nodes are not + normally intended for direct consumption by end users. + Normal response codes: 200 Error codes: 400,403,406 @@ -317,6 +329,8 @@ Request - sort_dir: sort_dir - sort_key: sort_key - detail: detail + - parent_node: parent_node + - include_children: include_children Response -------- @@ -461,6 +475,7 @@ Response - vendor_interface: vendor_interface - volume: n_volume - conductor_group: conductor_group + - parent_node: parent_node - protected: protected - protected_reason: protected_reason - owner: owner @@ -527,6 +542,9 @@ only the specified set. .. versionadded:: 1.82 Introduced the ``shard`` field. +.. versionadded:: 1.83 + Introduced the ``parent_node`` field. + Normal response codes: 200 Error codes: 400,403,404,406 diff --git a/api-ref/source/parameters.yaml b/api-ref/source/parameters.yaml index 6a50c94523..960396b0cf 100644 --- a/api-ref/source/parameters.yaml +++ b/api-ref/source/parameters.yaml @@ -207,6 +207,12 @@ fields_for_conductor: in: query required: false type: array +include_children: + description: | + Wheter to show child nodes in the node list, which are normally hidden. + in: query + required: false + type: boolean limit: description: | Requests a page size of items. Returns a number of items up to a limit @@ -1321,6 +1327,14 @@ owner: in: body required: false type: string +parent_node: + description: | + A UUID representing a node which is a parent_node to the present + node. If populated, the node will disappear from the normal node + list, as it is *not* intended for *normal* usage directly. + in: body + required: false + type: string passthru_async: description: | If True the passthru function is invoked asynchronously; if False, diff --git a/doc/source/contributor/webapi-version-history.rst b/doc/source/contributor/webapi-version-history.rst index 11e529292f..0395cbc767 100644 --- a/doc/source/contributor/webapi-version-history.rst +++ b/doc/source/contributor/webapi-version-history.rst @@ -2,6 +2,27 @@ REST API Version History ======================== +1.83 (Bobcat) +---------------------- + +This version adds a concept of child nodes through the use of a +``parent_node`` field which can be set on a node. + +Under normal conditions, child nodes are not visible in the normal node +list, as they are more for nested resources and not machines which can be +freely used outside of an integrated context of the parent node. +Think of a "child node" as a node with it's own BMC embedded inside of +an existing node. + +Additionally: + +- Adds ``GET /v1/nodes/{node_ident}/children`` to return a list of node + UUIDs which represent children, which can acted upon individually. +- Adds ``GET /v1/nodes/?include_children=True`` to return a list of all + parent nodes and children. +- Adds ``GET /v1/nodes?parent_node={node_ident}`` to explicitly request + a detailed list of nodes by parent relationship. + 1.82 (Antelope) ---------------------- diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index 65adee5447..7c6d1745e0 100644 --- a/ironic/api/controllers/v1/node.py +++ b/ironic/api/controllers/v1/node.py @@ -172,6 +172,7 @@ def node_schema(): ]}, 'network_interface': {'type': ['string', 'null']}, 'owner': {'type': ['string', 'null']}, + 'parent_node': {'type': ['string', 'null'], 'maxLength': 36}, 'power_interface': {'type': ['string', 'null']}, 'properties': {'type': ['object', 'null']}, 'raid_interface': {'type': ['string', 'null']}, @@ -270,7 +271,8 @@ PATCH_ALLOWED_FIELDS = [ 'retired_reason', 'shard', 'storage_interface', - 'vendor_interface' + 'vendor_interface', + 'parent_node' ] TRAITS_SCHEMA = { @@ -1370,6 +1372,7 @@ def _get_fields_for_node_query(fields=None): 'network_data', 'network_interface', 'owner', + 'parent_node', 'power_interface', 'power_state', 'properties', @@ -1977,6 +1980,51 @@ class NodeInventoryController(rest.RestController): return inspect_utils.get_inspection_data(node, api.request.context) +class NodeChildrenController(rest.RestController): + + def __init__(self, node_ident): + if hasattr(self, 'parent'): + # Short circuit any attempt to access + # /v1/nodes//children//children + raise exception.HTTPNotFound() + if api.request.version.minor < versions.MINOR_83_PARENT_CHILD_NODES: + # Minimum Client version is required. + raise exception.HTTPNotFound() + + super(NodeChildrenController).__init__() + self.parent_node = node_ident + + @METRICS.timer('NodeHistoryController.get_all') + @method.expose() + def get_all(self): + try: + # retrieve the parent node and validate access is permitted. + rpc_node = api_utils.check_node_policy_and_retrieve( + 'baremetal:node:get', self.parent_node) + except exception.HTTPForbidden: + # If access is forbidden, we cannot tell the user they don't + # have access. + raise exception.HTTPNotFound() + + filters = {} + # Extract the project ID or get None if not applicable. + project = api_utils.check_list_policy('node', None) + url = api.request.public_url + if project: + filters['project'] = project + filters['parent_node'] = rpc_node.uuid + nodes = objects.Node.list(api.request.context, + filters=filters, fields=['uuid']) + node_list = [] + for node in nodes: + node_list.append(node.uuid) + # todo, need to check the format for links + return { + 'children': node_list, + 'links': link.make_link('children', url, 'nodes', + '?parent_node={}'.format(rpc_node.uuid))} + + class NodesController(rest.RestController): """REST controller for Nodes.""" @@ -2003,6 +2051,10 @@ class NodesController(rest.RestController): """A flag to indicate if the requests to this controller are coming from the top-level resource Chassis""" + parent_node = None + """An indicator to signal if this resource is being accessed + by a sub-controller.""" + _custom_actions = { 'detail': ['GET'], 'validate': ['GET'], @@ -2024,6 +2076,7 @@ class NodesController(rest.RestController): 'allocation': allocation.NodeAllocationController, 'history': NodeHistoryController, 'inventory': NodeInventoryController, + 'children': NodeChildrenController, } @pecan.expose() @@ -2056,6 +2109,7 @@ class NodesController(rest.RestController): # behaviour of previous releases for microversions without this # endpoint. return + subcontroller = self._subcontroller_map.get(remainder[0]) if subcontroller: return subcontroller(node_ident=ident), remainder[1:] @@ -2082,7 +2136,8 @@ class NodesController(rest.RestController): detail=None, conductor=None, owner=None, lessee=None, project=None, description_contains=None, shard=None, - sharded=None): + sharded=None, include_children=None, + parent_node=None): if self.from_chassis and not chassis_uuid: raise exception.MissingParameterValue( _("Chassis id not specified.")) @@ -2124,7 +2179,9 @@ class NodesController(rest.RestController): 'description_contains': description_contains, 'retired': retired, 'instance_uuid': instance_uuid, - 'sharded': sharded + 'sharded': sharded, + 'include_children': include_children, + 'parent_node': parent_node, } filters = {} for key, value in possible_filters.items(): @@ -2143,6 +2200,7 @@ class NodesController(rest.RestController): # map the name for the call, as we did not pickup a specific # list of fields to return. obj_fields = fields + # NOTE(TheJulia): When a data set of the nodes list is being # requested, this method takes approximately 3-3.5% of the time # when requesting specific fields aligning with Nova's sync @@ -2176,7 +2234,6 @@ class NodesController(rest.RestController): # and we cannot pass a limit of 0 to sqlalchemy # and expect a response. limit = 0 - return node_list_convert_with_links(nodes, limit, url=resource_url, fields=fields, @@ -2269,14 +2326,16 @@ class NodesController(rest.RestController): detail=args.boolean, conductor=args.string, owner=args.string, description_contains=args.string, lessee=args.string, project=args.string, - shard=args.string_list, sharded=args.boolean) + shard=args.string_list, sharded=args.boolean, + include_children=args.boolean, parent_node=args.string) def get_all(self, chassis_uuid=None, instance_uuid=None, associated=None, maintenance=None, retired=None, provision_state=None, marker=None, limit=None, sort_key='id', sort_dir='asc', driver=None, fields=None, resource_class=None, fault=None, conductor_group=None, detail=None, conductor=None, owner=None, description_contains=None, lessee=None, - project=None, shard=None, sharded=None): + project=None, shard=None, sharded=None, include_children=None, + parent_node=None): """Retrieve a list of nodes. :param chassis_uuid: Optional UUID of a chassis, to get only nodes for @@ -2342,6 +2401,9 @@ class NodesController(rest.RestController): api_utils.check_allow_filter_by_shard(shard) # Sharded is guarded by the same API version as shard api_utils.check_allow_filter_by_shard(sharded) + api_utils.check_allow_child_node_params( + include_children=include_children, + parent_node=parent_node) fields = api_utils.get_request_return_fields(fields, detail, _DEFAULT_RETURN_FIELDS) @@ -2359,7 +2421,10 @@ class NodesController(rest.RestController): conductor=conductor, owner=owner, lessee=lessee, shard=shard, sharded=sharded, - project=project, **extra_args) + project=project, + include_children=include_children, + parent_node=parent_node, + **extra_args) @METRICS.timer('NodesController.detail') @method.expose() @@ -2379,7 +2444,8 @@ class NodesController(rest.RestController): driver=None, resource_class=None, fault=None, conductor_group=None, conductor=None, owner=None, description_contains=None, lessee=None, project=None, - shard=None, sharded=None): + shard=None, sharded=None, include_children=None, + parent_node=None): """Retrieve a list of nodes with detail. :param chassis_uuid: Optional UUID of a chassis, to get only nodes for @@ -2457,7 +2523,10 @@ class NodesController(rest.RestController): conductor=conductor, owner=owner, lessee=lessee, project=project, shard=shard, - sharded=sharded, **extra_args) + sharded=sharded, + include_children=include_children, + parent_node=parent_node, + **extra_args) @METRICS.timer('NodesController.validate') @method.expose() @@ -2639,57 +2708,77 @@ class NodesController(rest.RestController): for network_data in network_data_fields: validate_network_data(network_data) + parent_node = api_utils.get_patch_values(patch, '/parent_node') + if parent_node: + try: + # Verify we can see the parent node + api_utils.check_node_policy_and_retrieve( + 'baremetal:node:get', parent_node[0]) + except Exception: + msg = _("Unable to apply the requested parent_node. " + "Requested value was invalid.") + raise exception.Invalid(msg) + def _authorize_patch_and_get_node(self, node_ident, patch): # deal with attribute-specific policy rules policy_checks = [] generic_update = False - for p in patch: - if p['path'].startswith('/instance_info'): - policy_checks.append('baremetal:node:update_instance_info') - elif p['path'].startswith('/extra'): - policy_checks.append('baremetal:node:update_extra') - elif (p['path'].startswith('/automated_clean') - and strutils.bool_from_string(p['value'], default=None) - is False): - policy_checks.append('baremetal:node:disable_cleaning') - elif p['path'].startswith('/driver_info'): - policy_checks.append('baremetal:node:update:driver_info') - elif p['path'].startswith('/properties'): - policy_checks.append('baremetal:node:update:properties') - elif p['path'].startswith('/chassis_uuid'): - policy_checks.append('baremetal:node:update:chassis_uuid') - elif p['path'].startswith('/instance_uuid'): - policy_checks.append('baremetal:node:update:instance_uuid') - elif p['path'].startswith('/lessee'): - policy_checks.append('baremetal:node:update:lessee') - elif p['path'].startswith('/owner'): - policy_checks.append('baremetal:node:update:owner') - elif p['path'].startswith('/driver'): - policy_checks.append('baremetal:node:update:driver_interfaces') - elif ((p['path'].lstrip('/').rsplit(sep="_", maxsplit=1)[0] - in driver_base.ALL_INTERFACES) - and (p['path'].lstrip('/').rsplit(sep="_", maxsplit=1)[-1] - == "interface")): - # TODO(TheJulia): Replace the above check with something like - # elif (p['path'].lstrip('/').removesuffix('_interface') - # when the minimum supported version is Python 3.9. - policy_checks.append('baremetal:node:update:driver_interfaces') - elif p['path'].startswith('/network_data'): - policy_checks.append('baremetal:node:update:network_data') - elif p['path'].startswith('/conductor_group'): - policy_checks.append('baremetal:node:update:conductor_group') - elif p['path'].startswith('/name'): - policy_checks.append('baremetal:node:update:name') - elif p['path'].startswith('/retired'): - policy_checks.append('baremetal:node:update:retired') - elif p['path'].startswith('/shard'): - policy_checks.append('baremetal:node:update:shard') - else: - generic_update = True - # always do at least one check - if generic_update or not policy_checks: - policy_checks.append('baremetal:node:update') + paths_to_policy = ( + ('/instance_info', 'baremetal:node:update_instance_info'), + ('/extra', 'baremetal:node:update_extra'), + ('/driver_info', 'baremetal:node:update:driver_info'), + ('/properties', 'baremetal:node:update:properties'), + ('/chassis_uuid', 'baremetal:node:update:chassis_uuid'), + ('/instance_uuid', 'baremetal:node:update:instance_uuid'), + ('/lessee', 'baremetal:node:update:lessee'), + ('/owner', 'baremetal:node:update:owner'), + ('/driver', 'baremetal:node:update:driver_interfaces'), + ('/network_data', 'baremetal:node:update:network_data'), + ('/conductor_group', 'baremetal:node:update:conductor_group'), + ('/name', 'baremetal:node:update:name'), + ('/retired', 'baremetal:node:update:retired'), + ('/shard', 'baremetal:node:update:shard'), + ('/parent_node', 'baremetal:node:update:parent_node') + ) + for p in patch: + # Process general direct path to policy map + rule_match_found = False + for check_path, policy_name in paths_to_policy: + if p['path'].startswith(check_path): + policy_checks.append(policy_name) + # Break from the loop as there is no reason to + # continue iterating + rule_match_found = True + break + + # Process more advanced checks and conditional behavior checks. + if not rule_match_found: + if ((p['path'].lstrip('/').rsplit(sep="_", maxsplit=1)[0] + in driver_base.ALL_INTERFACES) + and (p['path'].lstrip('/').rsplit(sep="_", maxsplit=1)[-1] + == "interface")): + # TODO(TheJulia): Replace the above check with something + # like elif (p['path'].lstrip('/').removesuffix( + # '_interface') when the minimum supported version is + # Python 3.9. + policy_checks.append( + 'baremetal:node:update:driver_interfaces') + rule_match_found = True + elif (p['path'].startswith('/automated_clean') + and strutils.bool_from_string(p['value'], default=None) + is False): + policy_checks.append('baremetal:node:disable_cleaning') + rule_match_found = True + if not rule_match_found: + generic_update = True + # End of loop over patch to determine rules to apply. + + if generic_update or not policy_checks: + # General policy check, either we no specific policy to apply + # on a node, or we fell through completely, regardless, + # we apply the update policy check. + policy_checks.append('baremetal:node:update') return api_utils.check_multiple_node_policies_and_retrieve( policy_checks, node_ident, with_suffix=True) diff --git a/ironic/api/controllers/v1/utils.py b/ironic/api/controllers/v1/utils.py index 12cba1a4a3..48ca79ab6d 100644 --- a/ironic/api/controllers/v1/utils.py +++ b/ironic/api/controllers/v1/utils.py @@ -531,7 +531,6 @@ def get_rpc_node(node_ident): # as a UUID. if uuidutils.is_uuid_like(node_ident): return objects.Node.get_by_uuid(api.request.context, node_ident) - # We can refer to nodes by their name, if the client supports it if allow_node_logical_names(): if is_valid_logical_name(node_ident): @@ -807,7 +806,8 @@ VERSIONED_FIELDS = { 'network_data': versions.MINOR_66_NODE_NETWORK_DATA, 'boot_mode': versions.MINOR_75_NODE_BOOT_MODE, 'secure_boot': versions.MINOR_75_NODE_BOOT_MODE, - 'shard': versions.MINOR_82_NODE_SHARD + 'shard': versions.MINOR_82_NODE_SHARD, + 'parent_node': versions.MINOR_83_PARENT_CHILD_NODES } for field in V31_FIELDS: @@ -1080,6 +1080,19 @@ def check_allow_filter_by_shard(shard): 'opr': versions.MINOR_82_NODE_SHARD}) +def check_allow_child_node_params(include_children=None, + parent_node=None): + if ((include_children is not None + or parent_node is not None) + and api.request.version.minor + < versions.MINOR_83_PARENT_CHILD_NODES): + raise exception.NotAcceptable(_( + "Request not acceptable. The minimal required API version " + "should be %(base)s.%(opr)s") % + {'base': versions.BASE_VERSION, + 'opr': versions.MINOR_83_PARENT_CHILD_NODES}) + + def initial_node_provision_state(): """Return node state to use by default when creating new nodes. diff --git a/ironic/api/controllers/v1/versions.py b/ironic/api/controllers/v1/versions.py index f4cd26c0f2..8b33c9b306 100644 --- a/ironic/api/controllers/v1/versions.py +++ b/ironic/api/controllers/v1/versions.py @@ -120,6 +120,7 @@ BASE_VERSION = 1 # v1.80: Marker to represent self service node creation/deletion # v1.81: Add node inventory # v1.82: Add node sharding capability +# v1.83: Add child node modeling MINOR_0_JUNO = 0 MINOR_1_INITIAL_VERSION = 1 MINOR_2_AVAILABLE_STATE = 2 @@ -203,6 +204,7 @@ MINOR_79_ALLOCATION_NODE_NAME = 79 MINOR_80_PROJECT_CREATE_DELETE_NODE = 80 MINOR_81_NODE_INVENTORY = 81 MINOR_82_NODE_SHARD = 82 +MINOR_83_PARENT_CHILD_NODES = 83 # When adding another version, update: # - MINOR_MAX_VERSION @@ -210,7 +212,7 @@ MINOR_82_NODE_SHARD = 82 # explanation of what changed in the new version # - common/release_mappings.py, RELEASE_MAPPING['master']['api'] -MINOR_MAX_VERSION = MINOR_82_NODE_SHARD +MINOR_MAX_VERSION = MINOR_83_PARENT_CHILD_NODES # String representations of the minor and maximum versions _MIN_VERSION_STRING = '{}.{}'.format(BASE_VERSION, MINOR_1_INITIAL_VERSION) diff --git a/ironic/common/policy.py b/ironic/common/policy.py index b877611af6..de451bd88e 100644 --- a/ironic/common/policy.py +++ b/ironic/common/policy.py @@ -1001,6 +1001,14 @@ node_policies = [ description='Governs if shards can be read via the API clients.', operations=[{'path': '/shards', 'method': 'GET'}], ), + policy.DocumentedRuleDefault( + name='baremetal:node:update:parent_node', + check_str=SYSTEM_MEMBER, + scope_types=['system', 'project'], + description='Governs if node parent_node field can be updated via ' + 'the API clients.', + operations=[{'path': '/nodes/{node_ident}', 'method': 'PATCH'}], + ), ] deprecated_port_reason = """ diff --git a/ironic/common/release_mappings.py b/ironic/common/release_mappings.py index eb0eb79566..eb2ddd313f 100644 --- a/ironic/common/release_mappings.py +++ b/ironic/common/release_mappings.py @@ -574,12 +574,12 @@ RELEASE_MAPPING = { } }, 'master': { - 'api': '1.82', + 'api': '1.83', 'rpc': '1.55', 'objects': { 'Allocation': ['1.1'], 'BIOSSetting': ['1.1'], - 'Node': ['1.37'], + 'Node': ['1.38', '1.37'], 'NodeHistory': ['1.0'], 'NodeInventory': ['1.0'], 'Conductor': ['1.3'], diff --git a/ironic/db/sqlalchemy/alembic/versions/fe222f476baf_add_parent_node_field.py b/ironic/db/sqlalchemy/alembic/versions/fe222f476baf_add_parent_node_field.py new file mode 100644 index 0000000000..b19cbf596b --- /dev/null +++ b/ironic/db/sqlalchemy/alembic/versions/fe222f476baf_add_parent_node_field.py @@ -0,0 +1,35 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + + +from alembic import op +import sqlalchemy as sa + + +"""add parent_node field + +Revision ID: fe222f476baf +Revises: 4dbec778866e +Create Date: 2023-04-10 11:59:29.633401 + +""" + +# revision identifiers, used by Alembic. +revision = 'fe222f476baf' +down_revision = '4dbec778866e' + + +def upgrade(): + op.add_column('nodes', sa.Column('parent_node', sa.String(length=36), + nullable=True)) + op.create_index( + 'parent_node_idx', 'nodes', ['parent_node'], unique=False) diff --git a/ironic/db/sqlalchemy/api.py b/ironic/db/sqlalchemy/api.py index 3ba31e2339..9f8a81f253 100644 --- a/ironic/db/sqlalchemy/api.py +++ b/ironic/db/sqlalchemy/api.py @@ -410,7 +410,8 @@ class Connection(api.Connection): 'sharded': 'shard'} _NODE_FILTERS = ({'chassis_uuid', 'reserved_by_any_of', 'provisioned_before', 'inspection_started_before', - 'description_contains', 'project'} + 'description_contains', 'project', 'include_children', + 'parent_node'} | _NODE_QUERY_FIELDS | set(_NODE_IN_QUERY_FIELDS) | set(_NODE_NON_NULL_FILTERS)) @@ -472,7 +473,17 @@ class Connection(api.Connection): project = filters['project'] query = query.filter((models.Node.owner == project) | (models.Node.lessee == project)) + # Determine parent/child node handling + if not filters.get('include_children', False): + if 'parent_node' in filters: + query = query.filter( + models.Node.parent_node == filters.get('parent_node') + ) + else: + query = query.filter(models.Node.parent_node == sql.null()) + # The presence of ``include_children`` as a filter results in + # a full list of both parents and children being conveyed. return query def _add_allocations_filters(self, query, filters): diff --git a/ironic/db/sqlalchemy/models.py b/ironic/db/sqlalchemy/models.py index 3424914175..62e144d097 100644 --- a/ironic/db/sqlalchemy/models.py +++ b/ironic/db/sqlalchemy/models.py @@ -135,6 +135,7 @@ class NodeBase(Base): Index('conductor_group_idx', 'conductor_group'), Index('resource_class_idx', 'resource_class'), Index('shard_idx', 'shard'), + Index('parent_node_idx', 'parent_node'), table_args()) id = Column(Integer, primary_key=True) uuid = Column(String(36)) @@ -211,11 +212,10 @@ class NodeBase(Base): storage_interface = Column(String(255), nullable=True) power_interface = Column(String(255), nullable=True) vendor_interface = Column(String(255), nullable=True) - boot_mode = Column(String(16), nullable=True) secure_boot = Column(Boolean, nullable=True) - shard = Column(String(255), nullable=True) + parent_node = Column(String(36), nullable=True) class Node(NodeBase): diff --git a/ironic/objects/node.py b/ironic/objects/node.py index b680ac60a4..1567baa9a5 100644 --- a/ironic/objects/node.py +++ b/ironic/objects/node.py @@ -79,7 +79,8 @@ class Node(base.IronicObject, object_base.VersionedObjectDictCompat): # Version 1.35: Add network_data field # Version 1.36: Add boot_mode and secure_boot fields # Version 1.37: Add shard field - VERSION = '1.37' + # Version 1.38: Add parent_node field + VERSION = '1.38' dbapi = db_api.get_instance() @@ -172,6 +173,7 @@ class Node(base.IronicObject, object_base.VersionedObjectDictCompat): 'boot_mode': object_fields.StringField(nullable=True), 'secure_boot': object_fields.BooleanField(nullable=True), 'shard': object_fields.StringField(nullable=True), + 'parent_node': object_fields.StringField(nullable=True), } def as_dict(self, secure=False, mask_configdrive=True): diff --git a/ironic/tests/unit/api/controllers/v1/test_node.py b/ironic/tests/unit/api/controllers/v1/test_node.py index 87a0290576..c21fd42b0f 100644 --- a/ironic/tests/unit/api/controllers/v1/test_node.py +++ b/ironic/tests/unit/api/controllers/v1/test_node.py @@ -8164,3 +8164,180 @@ class TestNodeShardPatch(test_api_base.BaseApiTest): body, expect_errors=True, headers=headers) self.mock_update_node.assert_not_called() self.assertEqual(http_client.NOT_ACCEPTABLE, response.status_code) + + +class TestNodeChildrenTestCase(test_api_base.BaseApiTest): + def setUp(self): + super(TestNodeChildrenTestCase, self).setUp() + self.node = obj_utils.create_test_node(self.context, name='din') + self.child_node = obj_utils.create_test_node( + self.context, + uuid=uuidutils.generate_uuid(), + name='not-yoda', + parent_node=self.node.uuid) + self.headers = {api_base.Version.string: '1.83'} + + def test_list_nodes(self): + response = self.get_json( + '/nodes/', headers=self.headers) + self.assertEqual(1, len(response['nodes'])) + self.assertEqual('din', response['nodes'][0]['name']) + + def test_get_child_node(self): + response = self.get_json( + '/nodes/%s/children' % self.node.uuid, headers=self.headers) + self.assertEqual(1, len(response['children'])) + self.assertEqual(self.child_node.uuid, response['children'][0]) + + def test_list_nodes_with_include_children(self): + response = self.get_json( + '/nodes/?include_children=True', headers=self.headers) + self.assertEqual(2, len(response['nodes'])) + + def test_list_nodes_ignores_parent_if_include_children_indicated(self): + response = self.get_json( + '/nodes/?include_children=True&parent_node=111', + headers=self.headers) + self.assertEqual(2, len(response['nodes'])) + + @mock.patch.object(api_utils, 'check_list_policy', autospec=True) + def test_list_nodes_cannot_see_children_if_not_owned(self, mock_policy): + project_id = uuidutils.generate_uuid() + mock_policy.return_value = project_id + self.node['owner'] = project_id + self.node.save() + response = self.get_json( + '/nodes/?parent_node={}'.format(project_id), + headers=self.headers) + self.assertEqual(0, len(response['nodes'])) + + @mock.patch.object(api_utils, 'check_list_policy', autospec=True) + def test_list_nodes_with_children_only_parent(self, mock_policy): + project_id = uuidutils.generate_uuid() + headers = self.headers.copy() + mock_policy.return_value = project_id + self.node['lessee'] = project_id + self.node.save() + response = self.get_json( + '/nodes/?include_children=True&' + 'fields=uuid,lessee,name,parent_node', + headers=headers) + self.assertEqual(1, len(response['nodes'])) + self.assertEqual(self.node.uuid, response['nodes'][0]['uuid']) + + def test_list_nodes_lists_empty_for_specific_parent(self): + node = obj_utils.create_test_node( + self.context, + uuid=uuidutils.generate_uuid(), + name='kryze', + parent_node=self.node.uuid) + response = self.get_json( + '/nodes/?parent_node={}'.format(node.uuid), headers=self.headers) + self.assertEqual(0, len(response['nodes'])) + + +@mock.patch.object(rpcapi.ConductorAPI, 'create_node', + lambda _api, _ctx, node, _topic: _create_node_locally(node)) +class TestNodeParentNodePost(test_api_base.BaseApiTest): + def setUp(self): + super(TestNodeParentNodePost, self).setUp() + self.node = obj_utils.create_test_node(self.context, name='din') + + p = mock.patch.object(rpcapi.ConductorAPI, 'get_topic_for', + autospec=True) + self.mock_gtf = p.start() + self.mock_gtf.return_value = 'test-topic' + self.addCleanup(p.stop) + self.chassis = obj_utils.create_test_chassis(self.context) + + def test_create_node_with_parent_node(self): + ndict = test_api_utils.post_get_test_node( + uuid=uuidutils.generate_uuid()) + ndict['parent_node'] = self.node.uuid + headers = {api_base.Version.string: '1.83'} + response = self.post_json('/nodes', ndict, headers=headers) + self.assertEqual(http_client.CREATED, response.status_int) + + result = self.get_json('/nodes/{}'.format(ndict['uuid']), + headers=headers) + self.assertEqual(ndict['uuid'], result['uuid']) + self.assertEqual(self.node.uuid, result['parent_node']) + + def test_create_node_with_parent_node_fail_wrong_version(self): + headers = {api_base.Version.string: '1.82'} + ndict = test_api_utils.post_get_test_node( + uuid=uuidutils.generate_uuid()) + ndict['parent_node'] = self.node.uuid + response = self.post_json( + '/nodes', ndict, expect_errors=True, headers=headers) + self.assertEqual(http_client.NOT_ACCEPTABLE, response.status_int) + + +class TestNodeParentNodePatch(test_api_base.BaseApiTest): + def setUp(self): + super(TestNodeParentNodePatch, self).setUp() + self.node = obj_utils.create_test_node( + self.context, + name='djarin') + self.child_node = obj_utils.create_test_node( + self.context, + name='the_child', + uuid=uuidutils.generate_uuid()) + + p = mock.patch.object(rpcapi.ConductorAPI, 'get_topic_for', + autospec=True) + self.mock_gtf = p.start() + self.mock_gtf.return_value = 'test-topic' + self.addCleanup(p.stop) + p = mock.patch.object(rpcapi.ConductorAPI, 'update_node', + autospec=True) + self.mock_update_node = p.start() + self.addCleanup(p.stop) + + def test_node_add_parent(self): + self.mock_update_node.return_value = self.node + (self + .mock_update_node + .return_value + .updated_at) = "2013-12-03T06:20:41.184720+00:00" + headers = {api_base.Version.string: '1.83'} + body = [{ + 'path': '/parent_node', + 'value': self.node.uuid, + 'op': 'add', + }] + + response = self.patch_json( + '/nodes/%s' % self.child_node.uuid, body, headers=headers) + self.assertEqual(http_client.OK, response.status_code) + self.mock_update_node.assert_called_once() + + def test_node_add_parent_node_fail_wrong_version(self): + headers = {api_base.Version.string: '1.82'} + body = [{ + 'path': '/parent_node', + 'value': self.node.uuid, + 'op': 'add', + }] + + response = self.patch_json('/nodes/%s' % self.child_node.uuid, + body, expect_errors=True, headers=headers) + self.mock_update_node.assert_not_called() + self.assertEqual(http_client.NOT_ACCEPTABLE, response.status_code) + + def test_node_remove_parent(self): + self.mock_update_node.return_value = self.node + (self + .mock_update_node + .return_value + .updated_at) = "2013-12-03T06:20:41.184720+00:00" + headers = {api_base.Version.string: '1.83'} + body = [{ + 'path': '/parent_node', + 'op': 'remove', + }] + + response = self.patch_json( + '/nodes/%s' % self.child_node.uuid, body, headers=headers) + self.assertEqual(http_client.OK, response.status_code) + self.mock_update_node.assert_called_once() diff --git a/ironic/tests/unit/api/test_acl.py b/ironic/tests/unit/api/test_acl.py index f5cbe498d7..4ac76eef96 100644 --- a/ironic/tests/unit/api/test_acl.py +++ b/ironic/tests/unit/api/test_acl.py @@ -392,6 +392,17 @@ class TestRBACProjectScoped(TestACLBase): owner=owner_project_id, last_error='meow', reservation='lolcats') + # invisible child node, rbac + project query enforcement + # prevents it from being visible. + db_utils.create_test_node( + uuid='2b3b8adb-add7-4fd0-8e82-dcb714d848e7', + parent_node=owned_node.uuid) + # Child node which will appear in child node endpoint + # queries. + db_utils.create_test_node( + uuid='3c3b8adb-edd7-3ed0-8e82-aab714d8411a', + parent_node=owned_node.uuid, + owner=owner_project_id) owned_node_port = db_utils.create_test_port( uuid='ebe30f19-358d-41e1-8d28-fd7357a0164c', node_id=owned_node['id'], diff --git a/ironic/tests/unit/api/test_rbac_project_scoped.yaml b/ironic/tests/unit/api/test_rbac_project_scoped.yaml index 0e21076588..f4a48df512 100644 --- a/ironic/tests/unit/api/test_rbac_project_scoped.yaml +++ b/ironic/tests/unit/api/test_rbac_project_scoped.yaml @@ -84,9 +84,8 @@ values: X-Roles: service owner_project_id: &owner_project_id 70e5e25a-2ca2-4cb1-8ae8-7d8739cee205 lessee_project_id: &lessee_project_id f11853c7-fa9c-4db3-a477-c9d8e0dbbf13 - owned_node_ident: &owned_node_ident f11853c7-fa9c-4db3-a477-c9d8e0dbbf13 + owned_node_ident: &owned_node_ident 1ab63b9e-66d7-4cd7-8618-dddd0f9f7881 lessee_node_ident: &lessee_node_ident 38d5abed-c585-4fce-a57e-a2ffc2a2ec6f - # Nodes - https://docs.openstack.org/api-ref/baremetal/?expanded=#nodes-nodes # Based on nodes_post_admin test. @@ -3888,3 +3887,62 @@ shard_patch_set_node_shard_disallowed: path: /shard value: 'TestShard' assert_status: 403 + +# Update node parent_node field - baremetal:node:update:parent_node + +parent_node_patch_by_admin: + path: '/v1/nodes/{lessee_node_ident}' + method: patch + headers: *owner_admin_headers + body: &patch_parent_node + - op: replace + path: /parent_node + value: *owned_node_ident + assert_status: 403 + +parent_node_patch_by_member: + path: '/v1/nodes/{lessee_node_ident}' + method: patch + headers: *owner_member_headers + body: *patch_parent_node + assert_status: 403 + +parent_node_patch_by_reader: + path: '/v1/nodes/{lessee_node_ident}' + method: patch + headers: *owner_reader_headers + body: *patch_parent_node + assert_status: 403 + +parent_node_patch_by_manager: + path: '/v1/nodes/{lessee_node_ident}' + method: patch + headers: *owner_manager_headers + body: *patch_parent_node + assert_status: 403 + +parent_node_patch_by_cannot_see_node: + # This node cannot be seen, and also just doesn't exist. + # Just to verify we return a 400 on a node we can change. + path: '/v1/nodes/{lessee_node_ident}' + method: patch + headers: *owner_admin_headers + body: + - op: replace + path: /parent_node + value: 'f11853c7-fa9c-4db3-a477-c9d8e0dbbf13' + assert_status: 400 + +parent_node_children_can_get_list_of_children: + path: '/v1/nodes/{owner_node_ident}/children' + method: get + headers: *owner_reader_headers + assert_status: 200 + assert_list_length: + children: 1 + +lessee_cannot_get_a_nodes_children: + path: '/v1/nodes/{owner_node_ident}/children' + method: get + headers: *lessee_reader_headers + assert_status: 404 diff --git a/ironic/tests/unit/api/test_rbac_system_scoped.yaml b/ironic/tests/unit/api/test_rbac_system_scoped.yaml index 16f0fded6c..8a56d31695 100644 --- a/ironic/tests/unit/api/test_rbac_system_scoped.yaml +++ b/ironic/tests/unit/api/test_rbac_system_scoped.yaml @@ -3,7 +3,7 @@ values: # System scoped admin token admin_headers: &admin_headers X-Auth-Token: 'baremetal-admin-token' - X-Roles: admin + X-Roles: admin,member,reader OpenStack-System-Scope: all # System scoped other member token. scoped_member_headers: &scoped_member_headers @@ -21,7 +21,7 @@ values: other_admin_headers: &other_admin_headers X-Auth-Token: 'other-admin-token' X-Project-ID: a1111111111111111111111111111111 - X-Roles: admin + X-Roles: admin,member,reader X-Project-Name: 'other-project' service_headers: &service_headers X-Auth-Token: 'baremetal-service-token' @@ -30,6 +30,7 @@ values: owner_project_id: &owner_project_id '{owner_project_id}' other_project_id: &other_project_id '{other_project_id}' node_ident: &node_ident '{node_ident}' + allocated_node_ident: &allocated_node_ident 22e26c0b-03f2-4d2e-ae87-c02d7f33c000 # Nodes - https://docs.openstack.org/api-ref/baremetal/?expanded=#nodes-nodes @@ -2313,3 +2314,29 @@ shard_patch_set_node_shard_disallowed: headers: *scoped_member_headers body: *replace_shard assert_status: 403 + +# Update node parent_node field - baremetal:node:update:parent_node + +parent_node_patch_by_admin: + path: '/v1/nodes/{node_ident}' + method: patch + headers: *admin_headers + body: &patch_parent_node + - op: replace + path: /parent_node + value: *allocated_node_ident + assert_status: 503 + +parent_node_patch_by_member: + path: '/v1/nodes/{node_ident}' + method: patch + headers: *scoped_member_headers + body: *patch_parent_node + assert_status: 503 + +parent_node_patch_by_reader: + path: '/v1/nodes/{node_ident}' + method: patch + headers: *reader_headers + body: *patch_parent_node + assert_status: 403 diff --git a/ironic/tests/unit/db/utils.py b/ironic/tests/unit/db/utils.py index fc5bee2262..d01c384996 100644 --- a/ironic/tests/unit/db/utils.py +++ b/ironic/tests/unit/db/utils.py @@ -236,7 +236,8 @@ def get_test_node(**kw): 'network_data': kw.get('network_data'), 'boot_mode': kw.get('boot_mode', None), 'secure_boot': kw.get('secure_boot', None), - 'shard': kw.get('shard', None) + 'shard': kw.get('shard', None), + 'parent_node': kw.get('parent_node', None) } for iface in drivers_base.ALL_INTERFACES: diff --git a/ironic/tests/unit/objects/test_objects.py b/ironic/tests/unit/objects/test_objects.py index 55b34a1727..3246a26b8e 100644 --- a/ironic/tests/unit/objects/test_objects.py +++ b/ironic/tests/unit/objects/test_objects.py @@ -676,7 +676,7 @@ class TestObject(_LocalTest, _TestObject): # version bump. It is an MD5 hash of the object fields and remotable methods. # The fingerprint values should only be changed if there is a version bump. expected_object_fingerprints = { - 'Node': '1.37-6b38eb91aec57532547ea8607f95675a', + 'Node': '1.38-7e7fdaa2c2bb01153ad567c9f1081cb7', 'MyObj': '1.5-9459d30d6954bffc7a9afd347a807ca6', 'Chassis': '1.3-d656e039fd8ae9f34efc232ab3980905', 'Port': '1.11-97bf15b61224f26c65e90f007d78bfd2', diff --git a/releasenotes/notes/add-parent-node-support-10bd42abd008db6f.yaml b/releasenotes/notes/add-parent-node-support-10bd42abd008db6f.yaml new file mode 100644 index 0000000000..8930a80fd6 --- /dev/null +++ b/releasenotes/notes/add-parent-node-support-10bd42abd008db6f.yaml @@ -0,0 +1,23 @@ +--- +features: + - | + Adds the concept of ``parent_node`` which allows a "child node", such as + an independently managed BMC controlled device deployed within a + ``parent_node`` as part of API version *1.83*. Child nodes are hidden + from normal node lists as they are not "general purpose" machines, + but have a specific embedded usage. In this model, RBAC rules also apply + so if you wish an ``owner`` or ``lessee`` to have the child node visible, + they must also have the the appropriate ``owner`` or ``lessee`` value set + matching the parent node. + - | + Adds a ``/v1/nodes/?include_children=True`` parameter to get a list of + all nodes and their children. + - | + Adds a ``/v1/nodes/?parent_node=`` query parameter to permit + retrieval of a list of child nodes assigned to the parent denoted by + ````. +upgrade: + - | + This upgrade contains an additional field for the ``nodes`` table, named + ``parent_node``. This update also indexes the ``parent_node`` database + column to prevent performance issues in large deployments.